[RFC PATCH 0/6] ACPI/EC: Enhance EC driver state machine

[RFC PATCH 0/6] ACPI/EC: Enhance EC driver state machine

[Lv Zheng]

This patchset fixes several small issues in current EC driver.
After applying this patchset, we can have all EC IRQ handling code
collected in advance_transaction() which is the core function of the EC
driver state machine.

These fixes are required by the EC GPE raw handler mode switch but not
dependent on the GPE raw handler mode. So I collect them to form this
small patchset.

Lv Zheng (6):
  ACPI/EC: Cleanup transaction wakeup code
  ACPI/EC: Add reference counting for query handlers
  ACPI/EC: Fix returning values in acpi_ec_sync_query()
  ACPI/EC: Fix a code path that global lock is not held
  ACPI/EC: Fix issues related to the SCI_EVT handling
  ACPI/EC: Cleanup QR_EC related code

 drivers/acpi/ec.c       |  183 +++++++++++++++++++++++------------------------
 drivers/acpi/internal.h |    1 +
 2 files changed, 92 insertions(+), 92 deletions(-)

-- 
1.7.10

[RFC PATCH 1/6] ACPI/EC: Cleanup transaction wakeup code

[Lv Zheng]

This patch moves transaction wakeup code into advance_transaction().
No functional changes.

Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 drivers/acpi/ec.c |   17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 1b5853f..3e19123 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -200,7 +200,7 @@ static int ec_transaction_completed(struct acpi_ec *ec)
 	return ret;
 }
 
-static bool advance_transaction(struct acpi_ec *ec)
+static void advance_transaction(struct acpi_ec *ec)
 {
 	struct transaction *t;
 	u8 status;
@@ -235,7 +235,7 @@ static bool advance_transaction(struct acpi_ec *ec)
 			t->flags |= ACPI_EC_COMMAND_COMPLETE;
 			wakeup = true;
 		}
-		return wakeup;
+		goto out;
 	} else {
 		if (EC_FLAGS_QUERY_HANDSHAKE &&
 		    !(status & ACPI_EC_FLAG_SCI) &&
@@ -251,7 +251,7 @@ static bool advance_transaction(struct acpi_ec *ec)
 			t->flags |= ACPI_EC_COMMAND_POLL;
 		} else
 			goto err;
-		return wakeup;
+		goto out;
 	}
 err:
 	/*
@@ -262,14 +262,16 @@ err:
 		if (in_interrupt() && t)
 			++t->irq_count;
 	}
-	return wakeup;
+out:
+	if (wakeup && in_interrupt())
+		wake_up(&ec->wait);
 }
 
 static void start_transaction(struct acpi_ec *ec)
 {
 	ec->curr->irq_count = ec->curr->wi = ec->curr->ri = 0;
 	ec->curr->flags = 0;
-	(void)advance_transaction(ec);
+	advance_transaction(ec);
 }
 
 static int acpi_ec_sync_query(struct acpi_ec *ec, u8 *data);
@@ -304,7 +306,7 @@ static int ec_poll(struct acpi_ec *ec)
 					return 0;
 			}
 			spin_lock_irqsave(&ec->lock, flags);
-			(void)advance_transaction(ec);
+			advance_transaction(ec);
 			spin_unlock_irqrestore(&ec->lock, flags);
 		} while (time_before(jiffies, delay));
 		pr_debug("controller reset, restart transaction\n");
@@ -688,8 +690,7 @@ static u32 acpi_ec_gpe_handler(acpi_handle gpe_device,
 	struct acpi_ec *ec = data;
 
 	spin_lock_irqsave(&ec->lock, flags);
-	if (advance_transaction(ec))
-		wake_up(&ec->wait);
+	advance_transaction(ec);
 	spin_unlock_irqrestore(&ec->lock, flags);
 	ec_check_sci(ec, acpi_ec_read_status(ec));
 	return ACPI_INTERRUPT_HANDLED | ACPI_REENABLE_GPE;
-- 
1.7.10

[RFC PATCH 2/6] ACPI/EC: Add reference counting for query handlers

[Lv Zheng]

This patch adds reference counting for query handlers in order to eliminate
kmalloc()/kfree() usage.

Signed-off-by: Lv Zheng <lv.zheng@intel.com>
Tested-by: Steffen Weber <steffen.weber@gmail.com>
Tested-by: Ortwin Glück <odi@odi.ch>
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/ec.c |   43 +++++++++++++++++++++++++++++++++----------
 1 file changed, 33 insertions(+), 10 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 3e19123..87b9911 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -105,6 +105,7 @@ struct acpi_ec_query_handler {
 	acpi_handle handle;
 	void *data;
 	u8 query_bit;
+	struct kref kref;
 };
 
 struct transaction {
@@ -580,6 +581,27 @@ static int acpi_ec_query_unlocked(struct acpi_ec *ec, u8 *data)
 /* --------------------------------------------------------------------------
                                 Event Management
    -------------------------------------------------------------------------- */
+static struct acpi_ec_query_handler *
+acpi_ec_get_query_handler(struct acpi_ec_query_handler *handler)
+{
+	if (handler)
+		kref_get(&handler->kref);
+	return handler;
+}
+
+static void acpi_ec_query_handler_release(struct kref *kref)
+{
+	struct acpi_ec_query_handler *handler =
+		container_of(kref, struct acpi_ec_query_handler, kref);
+
+	kfree(handler);
+}
+
+static void acpi_ec_put_query_handler(struct acpi_ec_query_handler *handler)
+{
+	kref_put(&handler->kref, acpi_ec_query_handler_release);
+}
+
 int acpi_ec_add_query_handler(struct acpi_ec *ec, u8 query_bit,
 			      acpi_handle handle, acpi_ec_query_func func,
 			      void *data)
@@ -595,6 +617,7 @@ int acpi_ec_add_query_handler(struct acpi_ec *ec, u8 query_bit,
 	handler->func = func;
 	handler->data = data;
 	mutex_lock(&ec->mutex);
+	kref_init(&handler->kref);
 	list_add(&handler->node, &ec->list);
 	mutex_unlock(&ec->mutex);
 	return 0;
@@ -604,15 +627,18 @@ EXPORT_SYMBOL_GPL(acpi_ec_add_query_handler);
 void acpi_ec_remove_query_handler(struct acpi_ec *ec, u8 query_bit)
 {
 	struct acpi_ec_query_handler *handler, *tmp;
+	LIST_HEAD(free_list);
 
 	mutex_lock(&ec->mutex);
 	list_for_each_entry_safe(handler, tmp, &ec->list, node) {
 		if (query_bit == handler->query_bit) {
-			list_del(&handler->node);
-			kfree(handler);
+			list_del_init(&handler->node);
+			list_add(&handler->node, &free_list);
 		}
 	}
 	mutex_unlock(&ec->mutex);
+	list_for_each_entry(handler, &free_list, node)
+		acpi_ec_put_query_handler(handler);
 }
 EXPORT_SYMBOL_GPL(acpi_ec_remove_query_handler);
 
@@ -628,14 +654,14 @@ static void acpi_ec_run(void *cxt)
 	else if (handler->handle)
 		acpi_evaluate_object(handler->handle, NULL, NULL, NULL);
 	pr_debug("##### Query(0x%02x) stopped #####\n", handler->query_bit);
-	kfree(handler);
+	acpi_ec_put_query_handler(handler);
 }
 
 static int acpi_ec_sync_query(struct acpi_ec *ec, u8 *data)
 {
 	u8 value = 0;
 	int status;
-	struct acpi_ec_query_handler *handler, *copy;
+	struct acpi_ec_query_handler *handler;
 
 	status = acpi_ec_query_unlocked(ec, &value);
 	if (data)
@@ -646,15 +672,12 @@ static int acpi_ec_sync_query(struct acpi_ec *ec, u8 *data)
 	list_for_each_entry(handler, &ec->list, node) {
 		if (value == handler->query_bit) {
 			/* have custom handler for this bit */
-			copy = kmalloc(sizeof(*handler), GFP_KERNEL);
-			if (!copy)
-				return -ENOMEM;
-			memcpy(copy, handler, sizeof(*copy));
+			handler = acpi_ec_get_query_handler(handler);
 			pr_debug("##### Query(0x%02x) scheduled #####\n",
 				 handler->query_bit);
-			return acpi_os_execute((copy->func) ?
+			return acpi_os_execute((handler->func) ?
 				OSL_NOTIFY_HANDLER : OSL_GPE_HANDLER,
-				acpi_ec_run, copy);
+				acpi_ec_run, handler);
 		}
 	}
 	return 0;
-- 
1.7.10

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC PATCH 3/6] ACPI/EC: Fix returning values in acpi_ec_sync_query()

[Lv Zheng]

The returning value of acpi_os_execute() is erroneously handled as errno.
This patch corrects it by returning EBUSY to indicate the work queue item
creation failure.

Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 drivers/acpi/ec.c |   16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 87b9911..a94ee9f 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -660,14 +660,15 @@ static void acpi_ec_run(void *cxt)
 static int acpi_ec_sync_query(struct acpi_ec *ec, u8 *data)
 {
 	u8 value = 0;
-	int status;
+	int result;
+	acpi_status status;
 	struct acpi_ec_query_handler *handler;
 
-	status = acpi_ec_query_unlocked(ec, &value);
+	result = acpi_ec_query_unlocked(ec, &value);
 	if (data)
 		*data = value;
-	if (status)
-		return status;
+	if (result)
+		return result;
 
 	list_for_each_entry(handler, &ec->list, node) {
 		if (value == handler->query_bit) {
@@ -675,12 +676,15 @@ static int acpi_ec_sync_query(struct acpi_ec *ec, u8 *data)
 			handler = acpi_ec_get_query_handler(handler);
 			pr_debug("##### Query(0x%02x) scheduled #####\n",
 				 handler->query_bit);
-			return acpi_os_execute((handler->func) ?
+			status = acpi_os_execute((handler->func) ?
 				OSL_NOTIFY_HANDLER : OSL_GPE_HANDLER,
 				acpi_ec_run, handler);
+			if (ACPI_FAILURE(status))
+				result = -EBUSY;
+			break;
 		}
 	}
-	return 0;
+	return result;
 }
 
 static void acpi_ec_gpe_query(void *ec_cxt)
-- 
1.7.10

[RFC PATCH 4/6] ACPI/EC: Fix a code path that global lock is not held

[Lv Zheng]

Currently QR_EC is queued up on CPU 0 to be safe with SMM because there is
no global lock held for acpi_ec_gpe_query(). As we are about to move QR_EC
to a non CPU 0 bound work queue to avoid invoking kmalloc() in
advance_transaction(), we have to acquire global lock for the new QR_EC
work item to avoid regressions.

Known issue:
1. Global lock for acpi_ec_clear().
   This is an existing issue that acpi_ec_clear() which invokes
   acpi_ec_sync_query() also suffers from the same issue. But this patch's
   target is only the code to invoke acpi_ec_sync_query() in a CPU 0 bound
   work queue item, and the acpi_ec_clear() can be automatically fixed by
   further patch that merges the redundant code, so it is left unchanged.

Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 drivers/acpi/ec.c |   10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index a94ee9f..3c97122 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -690,11 +690,21 @@ static int acpi_ec_sync_query(struct acpi_ec *ec, u8 *data)
 static void acpi_ec_gpe_query(void *ec_cxt)
 {
 	struct acpi_ec *ec = ec_cxt;
+	acpi_status status;
+	u32 glk;
 
 	if (!ec)
 		return;
 	mutex_lock(&ec->mutex);
+	if (ec->global_lock) {
+		status = acpi_acquire_global_lock(ACPI_EC_UDELAY_GLK, &glk);
+		if (ACPI_FAILURE(status))
+			goto unlock;
+	}
 	acpi_ec_sync_query(ec, NULL);
+	if (ec->global_lock)
+		acpi_release_global_lock(glk);
+unlock:
 	mutex_unlock(&ec->mutex);
 }
 
-- 
1.7.10

[RFC PATCH 5/6] ACPI/EC: Fix issues related to the SCI_EVT handling

[Lv Zheng]

This patch fixes 2 issues related to the draining behavior. But it doesn't
implement the draining support, it only cleans up code so that further
draining support is possible.

The draining behavior is expected by some platforms (for example, Samsung)
where SCI_EVT is set only once for a set of events and might be cleared for
the very first QR_EC command issued after SCI_EVT is set. EC firmware on
such platforms will return 0x00 to indicate "no outstanding event". Thus
after seeing an SCI_EVT indication, EC driver need to fetch events until
0x00 returned (see acpi_ec_clear()).

Issue 1 - acpi_ec_submit_query():
It's reported on Samsung laptops that SCI_EVT isn't checked when the
transactions are advanced in ec_poll(), which leads to SCI_EVT triggering
source lost:
 If no EC GPE IRQs are arrived after that, EC driver cannot detect this
 event and handle it.
See comment 244/247 for kernel bugzilla 44161.
This patch fixes this issue by moving SCI_EVT checks into
advance_transaction(). So that SCI_EVT is checked each time we are going to
handle the EC firmware indications. And this check will happen for both IRQ
context and task context.
Since after doing that, SCI_EVT is also checked after completing a
transaction, ec_check_sci() and ec_check_sci_sync() can be removed.

Issue 2 - acpi_ec_complete_query():
We expect to clear EC_FLAGS_QUERY_PENDING to allow queuing another draining
QR_EC after writing a QR_EC command and before reading the event. After
reading the event, SCI_EVT might be cleared by the firmware, thus it may
not be possible to queue such a draining QR_EC at that time.
But putting the EC_FLAGS_QUERY_PENDING clearing code after
start_transaction() is wrong as there are chances that after
start_transaction(), QR_EC can fail to be sent. If this happens,
EC_FLAG_QUERY_PENDING will be cleared earlier. As a consequence, the
draining QR_EC will also be queued earlier than expected.
This patch also moves this code into advance_transaction() where QR_EC is
just sent (ACPI_EC_COMMAND_POLL flagged) to fix this issue.

Notes:
1. After introducing the 2 SCI_EVT related handlings into
   advance_transaction(), a next QR_EC can be queued right after writing
   the current QR_EC command and before reading the event. But this still
   hasn't implemented the draining behavior as the draining support
   requires:
     If a previous returned event value isn't 0x00, a draining QR_EC need
     to be issued even when SCI_EVT isn't set.
2. In this patch, acpi_os_execute() is also converted into a seperate work
   item to avoid invoking kmalloc() in the atomic context. We can do this
   because of the previous global lock fix.
3. Originally, EC_FLAGS_EVENT_PENDING is also used to avoid queuing up
   multiple work items (created by acpi_os_execute()), this can be covered
   by only using a single work item. But this patch still keeps this flag
   as there are different usages in the driver initialization steps relying
   on this flag.

Reference: https://bugzilla.kernel.org/show_bug.cgi?id=44161
Reported-by: Kieran Clancy <clancy.kieran@gmail.com>
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 drivers/acpi/ec.c       |   59 ++++++++++++++++++++---------------------------
 drivers/acpi/internal.h |    1 +
 2 files changed, 26 insertions(+), 34 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 3c97122..89e89b2 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -120,6 +120,8 @@ struct transaction {
 	u8 flags;
 };
 
+static int acpi_ec_sync_query(struct acpi_ec *ec, u8 *data);
+
 struct acpi_ec *boot_ec, *first_ec;
 EXPORT_SYMBOL(first_ec);
 
@@ -189,6 +191,22 @@ static const char *acpi_ec_cmd_string(u8 cmd)
 #define acpi_ec_cmd_string(cmd)		"UNDEF"
 #endif
 
+static void acpi_ec_submit_query(struct acpi_ec *ec)
+{
+	if (!test_and_set_bit(EC_FLAGS_QUERY_PENDING, &ec->flags)) {
+		pr_debug("***** Event started *****\n");
+		schedule_work(&ec->work);
+	}
+}
+
+static void acpi_ec_complete_query(struct acpi_ec *ec)
+{
+	if (ec->curr->command == ACPI_EC_COMMAND_QUERY) {
+		clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags);
+		pr_debug("***** Event stopped *****\n");
+	}
+}
+
 static int ec_transaction_completed(struct acpi_ec *ec)
 {
 	unsigned long flags;
@@ -242,6 +260,7 @@ static void advance_transaction(struct acpi_ec *ec)
 		    !(status & ACPI_EC_FLAG_SCI) &&
 		    (t->command == ACPI_EC_COMMAND_QUERY)) {
 			t->flags |= ACPI_EC_COMMAND_POLL;
+			acpi_ec_complete_query(ec);
 			t->rdata[t->ri++] = 0x00;
 			t->flags |= ACPI_EC_COMMAND_COMPLETE;
 			pr_debug("***** Command(%s) software completion *****\n",
@@ -250,6 +269,7 @@ static void advance_transaction(struct acpi_ec *ec)
 		} else if ((status & ACPI_EC_FLAG_IBF) == 0) {
 			acpi_ec_write_cmd(ec, t->command);
 			t->flags |= ACPI_EC_COMMAND_POLL;
+			acpi_ec_complete_query(ec);
 		} else
 			goto err;
 		goto out;
@@ -264,6 +284,8 @@ err:
 			++t->irq_count;
 	}
 out:
+	if (status & ACPI_EC_FLAG_SCI)
+		acpi_ec_submit_query(ec);
 	if (wakeup && in_interrupt())
 		wake_up(&ec->wait);
 }
@@ -275,17 +297,6 @@ static void start_transaction(struct acpi_ec *ec)
 	advance_transaction(ec);
 }
 
-static int acpi_ec_sync_query(struct acpi_ec *ec, u8 *data);
-
-static int ec_check_sci_sync(struct acpi_ec *ec, u8 state)
-{
-	if (state & ACPI_EC_FLAG_SCI) {
-		if (!test_and_set_bit(EC_FLAGS_QUERY_PENDING, &ec->flags))
-			return acpi_ec_sync_query(ec, NULL);
-	}
-	return 0;
-}
-
 static int ec_poll(struct acpi_ec *ec)
 {
 	unsigned long flags;
@@ -333,10 +344,6 @@ static int acpi_ec_transaction_unlocked(struct acpi_ec *ec,
 	pr_debug("***** Command(%s) started *****\n",
 		 acpi_ec_cmd_string(t->command));
 	start_transaction(ec);
-	if (ec->curr->command == ACPI_EC_COMMAND_QUERY) {
-		clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags);
-		pr_debug("***** Event stopped *****\n");
-	}
 	spin_unlock_irqrestore(&ec->lock, tmp);
 	ret = ec_poll(ec);
 	spin_lock_irqsave(&ec->lock, tmp);
@@ -376,8 +383,6 @@ static int acpi_ec_transaction(struct acpi_ec *ec, struct transaction *t)
 
 	status = acpi_ec_transaction_unlocked(ec, t);
 
-	/* check if we received SCI during transaction */
-	ec_check_sci_sync(ec, acpi_ec_read_status(ec));
 	if (test_bit(EC_FLAGS_GPE_STORM, &ec->flags)) {
 		msleep(1);
 		/* It is safe to enable the GPE outside of the transaction. */
@@ -687,14 +692,12 @@ static int acpi_ec_sync_query(struct acpi_ec *ec, u8 *data)
 	return result;
 }
 
-static void acpi_ec_gpe_query(void *ec_cxt)
+static void acpi_ec_gpe_poller(struct work_struct *work)
 {
-	struct acpi_ec *ec = ec_cxt;
 	acpi_status status;
 	u32 glk;
+	struct acpi_ec *ec = container_of(work, struct acpi_ec, work);
 
-	if (!ec)
-		return;
 	mutex_lock(&ec->mutex);
 	if (ec->global_lock) {
 		status = acpi_acquire_global_lock(ACPI_EC_UDELAY_GLK, &glk);
@@ -708,18 +711,6 @@ unlock:
 	mutex_unlock(&ec->mutex);
 }
 
-static int ec_check_sci(struct acpi_ec *ec, u8 state)
-{
-	if (state & ACPI_EC_FLAG_SCI) {
-		if (!test_and_set_bit(EC_FLAGS_QUERY_PENDING, &ec->flags)) {
-			pr_debug("***** Event started *****\n");
-			return acpi_os_execute(OSL_NOTIFY_HANDLER,
-				acpi_ec_gpe_query, ec);
-		}
-	}
-	return 0;
-}
-
 static u32 acpi_ec_gpe_handler(acpi_handle gpe_device,
 	u32 gpe_number, void *data)
 {
@@ -729,7 +720,6 @@ static u32 acpi_ec_gpe_handler(acpi_handle gpe_device,
 	spin_lock_irqsave(&ec->lock, flags);
 	advance_transaction(ec);
 	spin_unlock_irqrestore(&ec->lock, flags);
-	ec_check_sci(ec, acpi_ec_read_status(ec));
 	return ACPI_INTERRUPT_HANDLED | ACPI_REENABLE_GPE;
 }
 
@@ -793,6 +783,7 @@ static struct acpi_ec *make_acpi_ec(void)
 	init_waitqueue_head(&ec->wait);
 	INIT_LIST_HEAD(&ec->list);
 	spin_lock_init(&ec->lock);
+	INIT_WORK(&ec->work, acpi_ec_gpe_poller);
 	return ec;
 }
 
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
index 163e82f..dc42078 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -127,6 +127,7 @@ struct acpi_ec {
 	struct list_head list;
 	struct transaction *curr;
 	spinlock_t lock;
+	struct work_struct work;
 };
 
 extern struct acpi_ec *first_ec;
-- 
1.7.10

[RFC PATCH 6/6] ACPI/EC: Cleanup QR_EC related code

[Lv Zheng]

The QR_EC related code pieces have redundants, this patch merges them into
acpi_ec_query() which invokes acpi_ec_transaction() where EC mutex and the
global lock are already held. After doing so, query handler traversal still
need to be locked by EC mutex after invoking acpi_ec_transaction().

Note that EC event handling is sequential. We fetch one event from firmware
event queue and process it until 0x00 or error returned. So we don't need
to hold mutex for whole acpi_ec_clear() process to determine whether we
should continue to drain. And for the same reason, we don't need to hold
mutex for the whole procedure from the QR_EC transaction to the query
handler traversal.

Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 drivers/acpi/ec.c |   70 +++++++++++++++--------------------------------------
 1 file changed, 20 insertions(+), 50 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 89e89b2..c385606 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -120,7 +120,7 @@ struct transaction {
 	u8 flags;
 };
 
-static int acpi_ec_sync_query(struct acpi_ec *ec, u8 *data);
+static int acpi_ec_query(struct acpi_ec *ec, u8 *data);
 
 struct acpi_ec *boot_ec, *first_ec;
 EXPORT_SYMBOL(first_ec);
@@ -508,7 +508,7 @@ static void acpi_ec_clear(struct acpi_ec *ec)
 	u8 value = 0;
 
 	for (i = 0; i < ACPI_EC_CLEAR_MAX; i++) {
-		status = acpi_ec_sync_query(ec, &value);
+		status = acpi_ec_query(ec, &value);
 		if (status || !value)
 			break;
 	}
@@ -539,14 +539,11 @@ void acpi_ec_unblock_transactions(void)
 	if (!ec)
 		return;
 
-	mutex_lock(&ec->mutex);
 	/* Allow transactions to be carried out again */
 	clear_bit(EC_FLAGS_BLOCKED, &ec->flags);
 
 	if (EC_FLAGS_CLEAR_ON_RESUME)
 		acpi_ec_clear(ec);
-
-	mutex_unlock(&ec->mutex);
 }
 
 void acpi_ec_unblock_transactions_early(void)
@@ -559,30 +556,6 @@ void acpi_ec_unblock_transactions_early(void)
 		clear_bit(EC_FLAGS_BLOCKED, &first_ec->flags);
 }
 
-static int acpi_ec_query_unlocked(struct acpi_ec *ec, u8 *data)
-{
-	int result;
-	u8 d;
-	struct transaction t = {.command = ACPI_EC_COMMAND_QUERY,
-				.wdata = NULL, .rdata = &d,
-				.wlen = 0, .rlen = 1};
-
-	if (!ec || !data)
-		return -EINVAL;
-	/*
-	 * Query the EC to find out which _Qxx method we need to evaluate.
-	 * Note that successful completion of the query causes the ACPI_EC_SCI
-	 * bit to be cleared (and thus clearing the interrupt source).
-	 */
-	result = acpi_ec_transaction_unlocked(ec, &t);
-	if (result)
-		return result;
-	if (!d)
-		return -ENODATA;
-	*data = d;
-	return 0;
-}
-
 /* --------------------------------------------------------------------------
                                 Event Management
    -------------------------------------------------------------------------- */
@@ -662,19 +635,30 @@ static void acpi_ec_run(void *cxt)
 	acpi_ec_put_query_handler(handler);
 }
 
-static int acpi_ec_sync_query(struct acpi_ec *ec, u8 *data)
+static int acpi_ec_query(struct acpi_ec *ec, u8 *data)
 {
 	u8 value = 0;
 	int result;
 	acpi_status status;
 	struct acpi_ec_query_handler *handler;
+	struct transaction t = {.command = ACPI_EC_COMMAND_QUERY,
+				.wdata = NULL, .rdata = &value,
+				.wlen = 0, .rlen = 1};
 
-	result = acpi_ec_query_unlocked(ec, &value);
-	if (data)
-		*data = value;
+	/*
+	 * Query the EC to find out which _Qxx method we need to evaluate.
+	 * Note that successful completion of the query causes the ACPI_EC_SCI
+	 * bit to be cleared (and thus clearing the interrupt source).
+	 */
+	result = acpi_ec_transaction(ec, &t);
 	if (result)
 		return result;
+	if (data)
+		*data = value;
+	if (!value)
+		return -ENODATA;
 
+	mutex_lock(&ec->mutex);
 	list_for_each_entry(handler, &ec->list, node) {
 		if (value == handler->query_bit) {
 			/* have custom handler for this bit */
@@ -689,26 +673,15 @@ static int acpi_ec_sync_query(struct acpi_ec *ec, u8 *data)
 			break;
 		}
 	}
+	mutex_unlock(&ec->mutex);
 	return result;
 }
 
 static void acpi_ec_gpe_poller(struct work_struct *work)
 {
-	acpi_status status;
-	u32 glk;
 	struct acpi_ec *ec = container_of(work, struct acpi_ec, work);
 
-	mutex_lock(&ec->mutex);
-	if (ec->global_lock) {
-		status = acpi_acquire_global_lock(ACPI_EC_UDELAY_GLK, &glk);
-		if (ACPI_FAILURE(status))
-			goto unlock;
-	}
-	acpi_ec_sync_query(ec, NULL);
-	if (ec->global_lock)
-		acpi_release_global_lock(glk);
-unlock:
-	mutex_unlock(&ec->mutex);
+	acpi_ec_query(ec, NULL);
 }
 
 static u32 acpi_ec_gpe_handler(acpi_handle gpe_device,
@@ -932,11 +905,8 @@ static int acpi_ec_add(struct acpi_device *device)
 	clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags);
 
 	/* Clear stale _Q events if hardware might require that */
-	if (EC_FLAGS_CLEAR_ON_RESUME) {
-		mutex_lock(&ec->mutex);
+	if (EC_FLAGS_CLEAR_ON_RESUME)
 		acpi_ec_clear(ec);
-		mutex_unlock(&ec->mutex);
-	}
 	return ret;
 }
 
-- 
1.7.10

Re: [RFC PATCH 0/6] ACPI/EC: Enhance EC driver state machine

[Rafael J. Wysocki]

On Friday, December 19, 2014 01:59:19 PM Lv Zheng wrote:
> This patchset fixes several small issues in current EC driver.
> After applying this patchset, we can have all EC IRQ handling code
> collected in advance_transaction() which is the core function of the EC
> driver state machine.
> 
> These fixes are required by the EC GPE raw handler mode switch but not
> dependent on the GPE raw handler mode. So I collect them to form this
> small patchset.
> 
> Lv Zheng (6):
>   ACPI/EC: Cleanup transaction wakeup code
>   ACPI/EC: Add reference counting for query handlers
>   ACPI/EC: Fix returning values in acpi_ec_sync_query()
>   ACPI/EC: Fix a code path that global lock is not held
>   ACPI/EC: Fix issues related to the SCI_EVT handling
>   ACPI/EC: Cleanup QR_EC related code
> 
>  drivers/acpi/ec.c       |  183 +++++++++++++++++++++++------------------------
>  drivers/acpi/internal.h |    1 +
>  2 files changed, 92 insertions(+), 92 deletions(-)

All of this makes sense to me, but I'd rather not push it for 3.19.

I can queue it up for 3.20 after clearing my current 3.19 queue.


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

[PATCH 0/6] ACPI/EC: Enhance EC driver state machine

[Lv Zheng]

This patchset fixes several small issues in current EC driver.
After applying this patchset, we can have all EC IRQ handling code
collected in advance_transaction() which is the core function of the EC
driver state machine.

Lv Zheng (6):
  ACPI/EC: Cleanup transaction wakeup code
  ACPI/EC: Add reference counting for query handlers
  ACPI/EC: Fix returning values in acpi_ec_sync_query()
  ACPI/EC: Fix a code path that global lock is not held
  ACPI/EC: Fix issues related to the SCI_EVT handling
  ACPI/EC: Cleanup QR_EC related code

 drivers/acpi/ec.c       |  183 +++++++++++++++++++++++------------------------
 drivers/acpi/internal.h |    1 +
 2 files changed, 92 insertions(+), 92 deletions(-)

-- 
1.7.10

[PATCH 1/6] ACPI/EC: Cleanup transaction wakeup code

[Lv Zheng]

This patch moves transaction wakeup code into advance_transaction().
No functional changes.

Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 drivers/acpi/ec.c |   17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 1b5853f..3e19123 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -200,7 +200,7 @@ static int ec_transaction_completed(struct acpi_ec *ec)
 	return ret;
 }
 
-static bool advance_transaction(struct acpi_ec *ec)
+static void advance_transaction(struct acpi_ec *ec)
 {
 	struct transaction *t;
 	u8 status;
@@ -235,7 +235,7 @@ static bool advance_transaction(struct acpi_ec *ec)
 			t->flags |= ACPI_EC_COMMAND_COMPLETE;
 			wakeup = true;
 		}
-		return wakeup;
+		goto out;
 	} else {
 		if (EC_FLAGS_QUERY_HANDSHAKE &&
 		    !(status & ACPI_EC_FLAG_SCI) &&
@@ -251,7 +251,7 @@ static bool advance_transaction(struct acpi_ec *ec)
 			t->flags |= ACPI_EC_COMMAND_POLL;
 		} else
 			goto err;
-		return wakeup;
+		goto out;
 	}
 err:
 	/*
@@ -262,14 +262,16 @@ err:
 		if (in_interrupt() && t)
 			++t->irq_count;
 	}
-	return wakeup;
+out:
+	if (wakeup && in_interrupt())
+		wake_up(&ec->wait);
 }
 
 static void start_transaction(struct acpi_ec *ec)
 {
 	ec->curr->irq_count = ec->curr->wi = ec->curr->ri = 0;
 	ec->curr->flags = 0;
-	(void)advance_transaction(ec);
+	advance_transaction(ec);
 }
 
 static int acpi_ec_sync_query(struct acpi_ec *ec, u8 *data);
@@ -304,7 +306,7 @@ static int ec_poll(struct acpi_ec *ec)
 					return 0;
 			}
 			spin_lock_irqsave(&ec->lock, flags);
-			(void)advance_transaction(ec);
+			advance_transaction(ec);
 			spin_unlock_irqrestore(&ec->lock, flags);
 		} while (time_before(jiffies, delay));
 		pr_debug("controller reset, restart transaction\n");
@@ -688,8 +690,7 @@ static u32 acpi_ec_gpe_handler(acpi_handle gpe_device,
 	struct acpi_ec *ec = data;
 
 	spin_lock_irqsave(&ec->lock, flags);
-	if (advance_transaction(ec))
-		wake_up(&ec->wait);
+	advance_transaction(ec);
 	spin_unlock_irqrestore(&ec->lock, flags);
 	ec_check_sci(ec, acpi_ec_read_status(ec));
 	return ACPI_INTERRUPT_HANDLED | ACPI_REENABLE_GPE;
-- 
1.7.10

[PATCH 2/6] ACPI/EC: Add reference counting for query handlers

[Lv Zheng]

This patch adds reference counting for query handlers in order to eliminate
kmalloc()/kfree() usage.

Signed-off-by: Lv Zheng <lv.zheng@intel.com>
Tested-by: Steffen Weber <steffen.weber@gmail.com>
Tested-by: Ortwin Glück <odi@odi.ch>
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/ec.c |   43 +++++++++++++++++++++++++++++++++----------
 1 file changed, 33 insertions(+), 10 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 3e19123..87b9911 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -105,6 +105,7 @@ struct acpi_ec_query_handler {
 	acpi_handle handle;
 	void *data;
 	u8 query_bit;
+	struct kref kref;
 };
 
 struct transaction {
@@ -580,6 +581,27 @@ static int acpi_ec_query_unlocked(struct acpi_ec *ec, u8 *data)
 /* --------------------------------------------------------------------------
                                 Event Management
    -------------------------------------------------------------------------- */
+static struct acpi_ec_query_handler *
+acpi_ec_get_query_handler(struct acpi_ec_query_handler *handler)
+{
+	if (handler)
+		kref_get(&handler->kref);
+	return handler;
+}
+
+static void acpi_ec_query_handler_release(struct kref *kref)
+{
+	struct acpi_ec_query_handler *handler =
+		container_of(kref, struct acpi_ec_query_handler, kref);
+
+	kfree(handler);
+}
+
+static void acpi_ec_put_query_handler(struct acpi_ec_query_handler *handler)
+{
+	kref_put(&handler->kref, acpi_ec_query_handler_release);
+}
+
 int acpi_ec_add_query_handler(struct acpi_ec *ec, u8 query_bit,
 			      acpi_handle handle, acpi_ec_query_func func,
 			      void *data)
@@ -595,6 +617,7 @@ int acpi_ec_add_query_handler(struct acpi_ec *ec, u8 query_bit,
 	handler->func = func;
 	handler->data = data;
 	mutex_lock(&ec->mutex);
+	kref_init(&handler->kref);
 	list_add(&handler->node, &ec->list);
 	mutex_unlock(&ec->mutex);
 	return 0;
@@ -604,15 +627,18 @@ EXPORT_SYMBOL_GPL(acpi_ec_add_query_handler);
 void acpi_ec_remove_query_handler(struct acpi_ec *ec, u8 query_bit)
 {
 	struct acpi_ec_query_handler *handler, *tmp;
+	LIST_HEAD(free_list);
 
 	mutex_lock(&ec->mutex);
 	list_for_each_entry_safe(handler, tmp, &ec->list, node) {
 		if (query_bit == handler->query_bit) {
-			list_del(&handler->node);
-			kfree(handler);
+			list_del_init(&handler->node);
+			list_add(&handler->node, &free_list);
 		}
 	}
 	mutex_unlock(&ec->mutex);
+	list_for_each_entry(handler, &free_list, node)
+		acpi_ec_put_query_handler(handler);
 }
 EXPORT_SYMBOL_GPL(acpi_ec_remove_query_handler);
 
@@ -628,14 +654,14 @@ static void acpi_ec_run(void *cxt)
 	else if (handler->handle)
 		acpi_evaluate_object(handler->handle, NULL, NULL, NULL);
 	pr_debug("##### Query(0x%02x) stopped #####\n", handler->query_bit);
-	kfree(handler);
+	acpi_ec_put_query_handler(handler);
 }
 
 static int acpi_ec_sync_query(struct acpi_ec *ec, u8 *data)
 {
 	u8 value = 0;
 	int status;
-	struct acpi_ec_query_handler *handler, *copy;
+	struct acpi_ec_query_handler *handler;
 
 	status = acpi_ec_query_unlocked(ec, &value);
 	if (data)
@@ -646,15 +672,12 @@ static int acpi_ec_sync_query(struct acpi_ec *ec, u8 *data)
 	list_for_each_entry(handler, &ec->list, node) {
 		if (value == handler->query_bit) {
 			/* have custom handler for this bit */
-			copy = kmalloc(sizeof(*handler), GFP_KERNEL);
-			if (!copy)
-				return -ENOMEM;
-			memcpy(copy, handler, sizeof(*copy));
+			handler = acpi_ec_get_query_handler(handler);
 			pr_debug("##### Query(0x%02x) scheduled #####\n",
 				 handler->query_bit);
-			return acpi_os_execute((copy->func) ?
+			return acpi_os_execute((handler->func) ?
 				OSL_NOTIFY_HANDLER : OSL_GPE_HANDLER,
-				acpi_ec_run, copy);
+				acpi_ec_run, handler);
 		}
 	}
 	return 0;
-- 
1.7.10

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/6] ACPI/EC: Fix returning values in acpi_ec_sync_query()

[Lv Zheng]

The returning value of acpi_os_execute() is erroneously handled as errno.
This patch corrects it by returning EBUSY to indicate the work queue item
creation failure.

Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 drivers/acpi/ec.c |   16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 87b9911..a94ee9f 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -660,14 +660,15 @@ static void acpi_ec_run(void *cxt)
 static int acpi_ec_sync_query(struct acpi_ec *ec, u8 *data)
 {
 	u8 value = 0;
-	int status;
+	int result;
+	acpi_status status;
 	struct acpi_ec_query_handler *handler;
 
-	status = acpi_ec_query_unlocked(ec, &value);
+	result = acpi_ec_query_unlocked(ec, &value);
 	if (data)
 		*data = value;
-	if (status)
-		return status;
+	if (result)
+		return result;
 
 	list_for_each_entry(handler, &ec->list, node) {
 		if (value == handler->query_bit) {
@@ -675,12 +676,15 @@ static int acpi_ec_sync_query(struct acpi_ec *ec, u8 *data)
 			handler = acpi_ec_get_query_handler(handler);
 			pr_debug("##### Query(0x%02x) scheduled #####\n",
 				 handler->query_bit);
-			return acpi_os_execute((handler->func) ?
+			status = acpi_os_execute((handler->func) ?
 				OSL_NOTIFY_HANDLER : OSL_GPE_HANDLER,
 				acpi_ec_run, handler);
+			if (ACPI_FAILURE(status))
+				result = -EBUSY;
+			break;
 		}
 	}
-	return 0;
+	return result;
 }
 
 static void acpi_ec_gpe_query(void *ec_cxt)
-- 
1.7.10

[PATCH 4/6] ACPI/EC: Fix a code path that global lock is not held

[Lv Zheng]

Currently QR_EC is queued up on CPU 0 to be safe with SMM because there is
no global lock held for acpi_ec_gpe_query(). As we are about to move QR_EC
to a non CPU 0 bound work queue to avoid invoking kmalloc() in
advance_transaction(), we have to acquire global lock for the new QR_EC
work item to avoid regressions.

Known issue:
1. Global lock for acpi_ec_clear().
   This is an existing issue that acpi_ec_clear() which invokes
   acpi_ec_sync_query() also suffers from the same issue. But this patch's
   target is only the code to invoke acpi_ec_sync_query() in a CPU 0 bound
   work queue item, and the acpi_ec_clear() can be automatically fixed by
   further patch that merges the redundant code, so it is left unchanged.

Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 drivers/acpi/ec.c |   10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index a94ee9f..3c97122 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -690,11 +690,21 @@ static int acpi_ec_sync_query(struct acpi_ec *ec, u8 *data)
 static void acpi_ec_gpe_query(void *ec_cxt)
 {
 	struct acpi_ec *ec = ec_cxt;
+	acpi_status status;
+	u32 glk;
 
 	if (!ec)
 		return;
 	mutex_lock(&ec->mutex);
+	if (ec->global_lock) {
+		status = acpi_acquire_global_lock(ACPI_EC_UDELAY_GLK, &glk);
+		if (ACPI_FAILURE(status))
+			goto unlock;
+	}
 	acpi_ec_sync_query(ec, NULL);
+	if (ec->global_lock)
+		acpi_release_global_lock(glk);
+unlock:
 	mutex_unlock(&ec->mutex);
 }
 
-- 
1.7.10

[PATCH 5/6] ACPI/EC: Fix issues related to the SCI_EVT handling

[Lv Zheng]

This patch fixes 2 issues related to the draining behavior. But it doesn't
implement the draining support, it only cleans up code so that further
draining support is possible.

The draining behavior is expected by some platforms (for example, Samsung)
where SCI_EVT is set only once for a set of events and might be cleared for
the very first QR_EC command issued after SCI_EVT is set. EC firmware on
such platforms will return 0x00 to indicate "no outstanding event". Thus
after seeing an SCI_EVT indication, EC driver need to fetch events until
0x00 returned (see acpi_ec_clear()).

Issue 1 - acpi_ec_submit_query():
It's reported on Samsung laptops that SCI_EVT isn't checked when the
transactions are advanced in ec_poll(), which leads to SCI_EVT triggering
source lost:
 If no EC GPE IRQs are arrived after that, EC driver cannot detect this
 event and handle it.
See comment 244/247 for kernel bugzilla 44161.
This patch fixes this issue by moving SCI_EVT checks into
advance_transaction(). So that SCI_EVT is checked each time we are going to
handle the EC firmware indications. And this check will happen for both IRQ
context and task context.
Since after doing that, SCI_EVT is also checked after completing a
transaction, ec_check_sci() and ec_check_sci_sync() can be removed.

Issue 2 - acpi_ec_complete_query():
We expect to clear EC_FLAGS_QUERY_PENDING to allow queuing another draining
QR_EC after writing a QR_EC command and before reading the event. After
reading the event, SCI_EVT might be cleared by the firmware, thus it may
not be possible to queue such a draining QR_EC at that time.
But putting the EC_FLAGS_QUERY_PENDING clearing code after
start_transaction() is wrong as there are chances that after
start_transaction(), QR_EC can fail to be sent. If this happens,
EC_FLAG_QUERY_PENDING will be cleared earlier. As a consequence, the
draining QR_EC will also be queued earlier than expected.
This patch also moves this code into advance_transaction() where QR_EC is
just sent (ACPI_EC_COMMAND_POLL flagged) to fix this issue.

Notes:
1. After introducing the 2 SCI_EVT related handlings into
   advance_transaction(), a next QR_EC can be queued right after writing
   the current QR_EC command and before reading the event. But this still
   hasn't implemented the draining behavior as the draining support
   requires:
     If a previous returned event value isn't 0x00, a draining QR_EC need
     to be issued even when SCI_EVT isn't set.
2. In this patch, acpi_os_execute() is also converted into a seperate work
   item to avoid invoking kmalloc() in the atomic context. We can do this
   because of the previous global lock fix.
3. Originally, EC_FLAGS_EVENT_PENDING is also used to avoid queuing up
   multiple work items (created by acpi_os_execute()), this can be covered
   by only using a single work item. But this patch still keeps this flag
   as there are different usages in the driver initialization steps relying
   on this flag.

Reference: https://bugzilla.kernel.org/show_bug.cgi?id=44161
Reported-by: Kieran Clancy <clancy.kieran@gmail.com>
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 drivers/acpi/ec.c       |   59 ++++++++++++++++++++---------------------------
 drivers/acpi/internal.h |    1 +
 2 files changed, 26 insertions(+), 34 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 3c97122..89e89b2 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -120,6 +120,8 @@ struct transaction {
 	u8 flags;
 };
 
+static int acpi_ec_sync_query(struct acpi_ec *ec, u8 *data);
+
 struct acpi_ec *boot_ec, *first_ec;
 EXPORT_SYMBOL(first_ec);
 
@@ -189,6 +191,22 @@ static const char *acpi_ec_cmd_string(u8 cmd)
 #define acpi_ec_cmd_string(cmd)		"UNDEF"
 #endif
 
+static void acpi_ec_submit_query(struct acpi_ec *ec)
+{
+	if (!test_and_set_bit(EC_FLAGS_QUERY_PENDING, &ec->flags)) {
+		pr_debug("***** Event started *****\n");
+		schedule_work(&ec->work);
+	}
+}
+
+static void acpi_ec_complete_query(struct acpi_ec *ec)
+{
+	if (ec->curr->command == ACPI_EC_COMMAND_QUERY) {
+		clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags);
+		pr_debug("***** Event stopped *****\n");
+	}
+}
+
 static int ec_transaction_completed(struct acpi_ec *ec)
 {
 	unsigned long flags;
@@ -242,6 +260,7 @@ static void advance_transaction(struct acpi_ec *ec)
 		    !(status & ACPI_EC_FLAG_SCI) &&
 		    (t->command == ACPI_EC_COMMAND_QUERY)) {
 			t->flags |= ACPI_EC_COMMAND_POLL;
+			acpi_ec_complete_query(ec);
 			t->rdata[t->ri++] = 0x00;
 			t->flags |= ACPI_EC_COMMAND_COMPLETE;
 			pr_debug("***** Command(%s) software completion *****\n",
@@ -250,6 +269,7 @@ static void advance_transaction(struct acpi_ec *ec)
 		} else if ((status & ACPI_EC_FLAG_IBF) == 0) {
 			acpi_ec_write_cmd(ec, t->command);
 			t->flags |= ACPI_EC_COMMAND_POLL;
+			acpi_ec_complete_query(ec);
 		} else
 			goto err;
 		goto out;
@@ -264,6 +284,8 @@ err:
 			++t->irq_count;
 	}
 out:
+	if (status & ACPI_EC_FLAG_SCI)
+		acpi_ec_submit_query(ec);
 	if (wakeup && in_interrupt())
 		wake_up(&ec->wait);
 }
@@ -275,17 +297,6 @@ static void start_transaction(struct acpi_ec *ec)
 	advance_transaction(ec);
 }
 
-static int acpi_ec_sync_query(struct acpi_ec *ec, u8 *data);
-
-static int ec_check_sci_sync(struct acpi_ec *ec, u8 state)
-{
-	if (state & ACPI_EC_FLAG_SCI) {
-		if (!test_and_set_bit(EC_FLAGS_QUERY_PENDING, &ec->flags))
-			return acpi_ec_sync_query(ec, NULL);
-	}
-	return 0;
-}
-
 static int ec_poll(struct acpi_ec *ec)
 {
 	unsigned long flags;
@@ -333,10 +344,6 @@ static int acpi_ec_transaction_unlocked(struct acpi_ec *ec,
 	pr_debug("***** Command(%s) started *****\n",
 		 acpi_ec_cmd_string(t->command));
 	start_transaction(ec);
-	if (ec->curr->command == ACPI_EC_COMMAND_QUERY) {
-		clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags);
-		pr_debug("***** Event stopped *****\n");
-	}
 	spin_unlock_irqrestore(&ec->lock, tmp);
 	ret = ec_poll(ec);
 	spin_lock_irqsave(&ec->lock, tmp);
@@ -376,8 +383,6 @@ static int acpi_ec_transaction(struct acpi_ec *ec, struct transaction *t)
 
 	status = acpi_ec_transaction_unlocked(ec, t);
 
-	/* check if we received SCI during transaction */
-	ec_check_sci_sync(ec, acpi_ec_read_status(ec));
 	if (test_bit(EC_FLAGS_GPE_STORM, &ec->flags)) {
 		msleep(1);
 		/* It is safe to enable the GPE outside of the transaction. */
@@ -687,14 +692,12 @@ static int acpi_ec_sync_query(struct acpi_ec *ec, u8 *data)
 	return result;
 }
 
-static void acpi_ec_gpe_query(void *ec_cxt)
+static void acpi_ec_gpe_poller(struct work_struct *work)
 {
-	struct acpi_ec *ec = ec_cxt;
 	acpi_status status;
 	u32 glk;
+	struct acpi_ec *ec = container_of(work, struct acpi_ec, work);
 
-	if (!ec)
-		return;
 	mutex_lock(&ec->mutex);
 	if (ec->global_lock) {
 		status = acpi_acquire_global_lock(ACPI_EC_UDELAY_GLK, &glk);
@@ -708,18 +711,6 @@ unlock:
 	mutex_unlock(&ec->mutex);
 }
 
-static int ec_check_sci(struct acpi_ec *ec, u8 state)
-{
-	if (state & ACPI_EC_FLAG_SCI) {
-		if (!test_and_set_bit(EC_FLAGS_QUERY_PENDING, &ec->flags)) {
-			pr_debug("***** Event started *****\n");
-			return acpi_os_execute(OSL_NOTIFY_HANDLER,
-				acpi_ec_gpe_query, ec);
-		}
-	}
-	return 0;
-}
-
 static u32 acpi_ec_gpe_handler(acpi_handle gpe_device,
 	u32 gpe_number, void *data)
 {
@@ -729,7 +720,6 @@ static u32 acpi_ec_gpe_handler(acpi_handle gpe_device,
 	spin_lock_irqsave(&ec->lock, flags);
 	advance_transaction(ec);
 	spin_unlock_irqrestore(&ec->lock, flags);
-	ec_check_sci(ec, acpi_ec_read_status(ec));
 	return ACPI_INTERRUPT_HANDLED | ACPI_REENABLE_GPE;
 }
 
@@ -793,6 +783,7 @@ static struct acpi_ec *make_acpi_ec(void)
 	init_waitqueue_head(&ec->wait);
 	INIT_LIST_HEAD(&ec->list);
 	spin_lock_init(&ec->lock);
+	INIT_WORK(&ec->work, acpi_ec_gpe_poller);
 	return ec;
 }
 
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
index 163e82f..dc42078 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -127,6 +127,7 @@ struct acpi_ec {
 	struct list_head list;
 	struct transaction *curr;
 	spinlock_t lock;
+	struct work_struct work;
 };
 
 extern struct acpi_ec *first_ec;
-- 
1.7.10

[PATCH 6/6] ACPI/EC: Cleanup QR_EC related code

[Lv Zheng]

The QR_EC related code pieces have redundants, this patch merges them into
acpi_ec_query() which invokes acpi_ec_transaction() where EC mutex and the
global lock are already held. After doing so, query handler traversal still
need to be locked by EC mutex after invoking acpi_ec_transaction().

Note that EC event handling is sequential. We fetch one event from firmware
event queue and process it until 0x00 or error returned. So we don't need
to hold mutex for whole acpi_ec_clear() process to determine whether we
should continue to drain. And for the same reason, we don't need to hold
mutex for the whole procedure from the QR_EC transaction to the query
handler traversal.

Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 drivers/acpi/ec.c |   70 +++++++++++++++--------------------------------------
 1 file changed, 20 insertions(+), 50 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 89e89b2..c385606 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -120,7 +120,7 @@ struct transaction {
 	u8 flags;
 };
 
-static int acpi_ec_sync_query(struct acpi_ec *ec, u8 *data);
+static int acpi_ec_query(struct acpi_ec *ec, u8 *data);
 
 struct acpi_ec *boot_ec, *first_ec;
 EXPORT_SYMBOL(first_ec);
@@ -508,7 +508,7 @@ static void acpi_ec_clear(struct acpi_ec *ec)
 	u8 value = 0;
 
 	for (i = 0; i < ACPI_EC_CLEAR_MAX; i++) {
-		status = acpi_ec_sync_query(ec, &value);
+		status = acpi_ec_query(ec, &value);
 		if (status || !value)
 			break;
 	}
@@ -539,14 +539,11 @@ void acpi_ec_unblock_transactions(void)
 	if (!ec)
 		return;
 
-	mutex_lock(&ec->mutex);
 	/* Allow transactions to be carried out again */
 	clear_bit(EC_FLAGS_BLOCKED, &ec->flags);
 
 	if (EC_FLAGS_CLEAR_ON_RESUME)
 		acpi_ec_clear(ec);
-
-	mutex_unlock(&ec->mutex);
 }
 
 void acpi_ec_unblock_transactions_early(void)
@@ -559,30 +556,6 @@ void acpi_ec_unblock_transactions_early(void)
 		clear_bit(EC_FLAGS_BLOCKED, &first_ec->flags);
 }
 
-static int acpi_ec_query_unlocked(struct acpi_ec *ec, u8 *data)
-{
-	int result;
-	u8 d;
-	struct transaction t = {.command = ACPI_EC_COMMAND_QUERY,
-				.wdata = NULL, .rdata = &d,
-				.wlen = 0, .rlen = 1};
-
-	if (!ec || !data)
-		return -EINVAL;
-	/*
-	 * Query the EC to find out which _Qxx method we need to evaluate.
-	 * Note that successful completion of the query causes the ACPI_EC_SCI
-	 * bit to be cleared (and thus clearing the interrupt source).
-	 */
-	result = acpi_ec_transaction_unlocked(ec, &t);
-	if (result)
-		return result;
-	if (!d)
-		return -ENODATA;
-	*data = d;
-	return 0;
-}
-
 /* --------------------------------------------------------------------------
                                 Event Management
    -------------------------------------------------------------------------- */
@@ -662,19 +635,30 @@ static void acpi_ec_run(void *cxt)
 	acpi_ec_put_query_handler(handler);
 }
 
-static int acpi_ec_sync_query(struct acpi_ec *ec, u8 *data)
+static int acpi_ec_query(struct acpi_ec *ec, u8 *data)
 {
 	u8 value = 0;
 	int result;
 	acpi_status status;
 	struct acpi_ec_query_handler *handler;
+	struct transaction t = {.command = ACPI_EC_COMMAND_QUERY,
+				.wdata = NULL, .rdata = &value,
+				.wlen = 0, .rlen = 1};
 
-	result = acpi_ec_query_unlocked(ec, &value);
-	if (data)
-		*data = value;
+	/*
+	 * Query the EC to find out which _Qxx method we need to evaluate.
+	 * Note that successful completion of the query causes the ACPI_EC_SCI
+	 * bit to be cleared (and thus clearing the interrupt source).
+	 */
+	result = acpi_ec_transaction(ec, &t);
 	if (result)
 		return result;
+	if (data)
+		*data = value;
+	if (!value)
+		return -ENODATA;
 
+	mutex_lock(&ec->mutex);
 	list_for_each_entry(handler, &ec->list, node) {
 		if (value == handler->query_bit) {
 			/* have custom handler for this bit */
@@ -689,26 +673,15 @@ static int acpi_ec_sync_query(struct acpi_ec *ec, u8 *data)
 			break;
 		}
 	}
+	mutex_unlock(&ec->mutex);
 	return result;
 }
 
 static void acpi_ec_gpe_poller(struct work_struct *work)
 {
-	acpi_status status;
-	u32 glk;
 	struct acpi_ec *ec = container_of(work, struct acpi_ec, work);
 
-	mutex_lock(&ec->mutex);
-	if (ec->global_lock) {
-		status = acpi_acquire_global_lock(ACPI_EC_UDELAY_GLK, &glk);
-		if (ACPI_FAILURE(status))
-			goto unlock;
-	}
-	acpi_ec_sync_query(ec, NULL);
-	if (ec->global_lock)
-		acpi_release_global_lock(glk);
-unlock:
-	mutex_unlock(&ec->mutex);
+	acpi_ec_query(ec, NULL);
 }
 
 static u32 acpi_ec_gpe_handler(acpi_handle gpe_device,
@@ -932,11 +905,8 @@ static int acpi_ec_add(struct acpi_device *device)
 	clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags);
 
 	/* Clear stale _Q events if hardware might require that */
-	if (EC_FLAGS_CLEAR_ON_RESUME) {
-		mutex_lock(&ec->mutex);
+	if (EC_FLAGS_CLEAR_ON_RESUME)
 		acpi_ec_clear(ec);
-		mutex_unlock(&ec->mutex);
-	}
 	return ret;
 }
 
-- 
1.7.10

Re: [PATCH 1/6] ACPI/EC: Cleanup transaction wakeup code

[Rafael J. Wysocki]

On Wednesday, January 14, 2015 07:28:22 PM Lv Zheng wrote:
> This patch moves transaction wakeup code into advance_transaction().
> No functional changes.
> 
> Signed-off-by: Lv Zheng <lv.zheng@intel.com>
> ---
>  drivers/acpi/ec.c |   17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
> index 1b5853f..3e19123 100644
> --- a/drivers/acpi/ec.c
> +++ b/drivers/acpi/ec.c
> @@ -200,7 +200,7 @@ static int ec_transaction_completed(struct acpi_ec *ec)
>  	return ret;
>  }
>  
> -static bool advance_transaction(struct acpi_ec *ec)
> +static void advance_transaction(struct acpi_ec *ec)
>  {
>  	struct transaction *t;
>  	u8 status;
> @@ -235,7 +235,7 @@ static bool advance_transaction(struct acpi_ec *ec)
>  			t->flags |= ACPI_EC_COMMAND_COMPLETE;
>  			wakeup = true;
>  		}
> -		return wakeup;
> +		goto out;
>  	} else {
>  		if (EC_FLAGS_QUERY_HANDSHAKE &&
>  		    !(status & ACPI_EC_FLAG_SCI) &&
> @@ -251,7 +251,7 @@ static bool advance_transaction(struct acpi_ec *ec)
>  			t->flags |= ACPI_EC_COMMAND_POLL;
>  		} else
>  			goto err;
> -		return wakeup;
> +		goto out;
>  	}
>  err:
>  	/*
> @@ -262,14 +262,16 @@ err:
>  		if (in_interrupt() && t)
>  			++t->irq_count;
>  	}
> -	return wakeup;
> +out:
> +	if (wakeup && in_interrupt())
> +		wake_up(&ec->wait);
>  }
>  
>  static void start_transaction(struct acpi_ec *ec)
>  {
>  	ec->curr->irq_count = ec->curr->wi = ec->curr->ri = 0;
>  	ec->curr->flags = 0;
> -	(void)advance_transaction(ec);
> +	advance_transaction(ec);

Well, this looks like a functional change, because we wouldn't call
wake_up(&ec->wait) here before.

>  }
>  
>  static int acpi_ec_sync_query(struct acpi_ec *ec, u8 *data);
> @@ -304,7 +306,7 @@ static int ec_poll(struct acpi_ec *ec)
>  					return 0;
>  			}
>  			spin_lock_irqsave(&ec->lock, flags);
> -			(void)advance_transaction(ec);
> +			advance_transaction(ec);

Ditto.

Or am I missing anything?

>  			spin_unlock_irqrestore(&ec->lock, flags);
>  		} while (time_before(jiffies, delay));
>  		pr_debug("controller reset, restart transaction\n");
> @@ -688,8 +690,7 @@ static u32 acpi_ec_gpe_handler(acpi_handle gpe_device,
>  	struct acpi_ec *ec = data;
>  
>  	spin_lock_irqsave(&ec->lock, flags);
> -	if (advance_transaction(ec))
> -		wake_up(&ec->wait);
> +	advance_transaction(ec);
>  	spin_unlock_irqrestore(&ec->lock, flags);
>  	ec_check_sci(ec, acpi_ec_read_status(ec));
>  	return ACPI_INTERRUPT_HANDLED | ACPI_REENABLE_GPE;
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

RE: [PATCH 1/6] ACPI/EC: Cleanup transaction wakeup code

[Zheng, Lv]

Hi, Rafael

> From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net]
> Sent: Thursday, January 22, 2015 6:17 AM
> 
> On Wednesday, January 14, 2015 07:28:22 PM Lv Zheng wrote:
> > This patch moves transaction wakeup code into advance_transaction().
> > No functional changes.
> >
> > Signed-off-by: Lv Zheng <lv.zheng@intel.com>
> > ---
> >  drivers/acpi/ec.c |   17 +++++++++--------
> >  1 file changed, 9 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
> > index 1b5853f..3e19123 100644
> > --- a/drivers/acpi/ec.c
> > +++ b/drivers/acpi/ec.c
> > @@ -200,7 +200,7 @@ static int ec_transaction_completed(struct acpi_ec *ec)
> >  	return ret;
> >  }
> >
> > -static bool advance_transaction(struct acpi_ec *ec)
> > +static void advance_transaction(struct acpi_ec *ec)
> >  {
> >  	struct transaction *t;
> >  	u8 status;
> > @@ -235,7 +235,7 @@ static bool advance_transaction(struct acpi_ec *ec)
> >  			t->flags |= ACPI_EC_COMMAND_COMPLETE;
> >  			wakeup = true;
> >  		}
> > -		return wakeup;
> > +		goto out;
> >  	} else {
> >  		if (EC_FLAGS_QUERY_HANDSHAKE &&
> >  		    !(status & ACPI_EC_FLAG_SCI) &&
> > @@ -251,7 +251,7 @@ static bool advance_transaction(struct acpi_ec *ec)
> >  			t->flags |= ACPI_EC_COMMAND_POLL;
> >  		} else
> >  			goto err;
> > -		return wakeup;
> > +		goto out;
> >  	}
> >  err:
> >  	/*
> > @@ -262,14 +262,16 @@ err:
> >  		if (in_interrupt() && t)
> >  			++t->irq_count;
> >  	}
> > -	return wakeup;
> > +out:
> > +	if (wakeup && in_interrupt())
> > +		wake_up(&ec->wait);
> >  }
> >
> >  static void start_transaction(struct acpi_ec *ec)
> >  {
> >  	ec->curr->irq_count = ec->curr->wi = ec->curr->ri = 0;
> >  	ec->curr->flags = 0;
> > -	(void)advance_transaction(ec);
> > +	advance_transaction(ec);
> 
> Well, this looks like a functional change, because we wouldn't call
> wake_up(&ec->wait) here before.

Ah, Yes.
But here, since the only advancement that can happen here is to send the EC command and there are always further advancements until transaction completion, the wake_up() won't be invoked at this point.
So IMO, there is no functional changes here.

> 
> >  }
> >
> >  static int acpi_ec_sync_query(struct acpi_ec *ec, u8 *data);
> > @@ -304,7 +306,7 @@ static int ec_poll(struct acpi_ec *ec)
> >  					return 0;
> >  			}
> >  			spin_lock_irqsave(&ec->lock, flags);
> > -			(void)advance_transaction(ec);
> > +			advance_transaction(ec);
> 
> Ditto.

Yes. I changed this logic.

By invoking wake_up() here, we can break the ec_poll() loop.
Because if the transaction has completed due to the polling advancement, we really don't need to wait for another interrupt.
So this new logic makes this patch more like a race fix, not a simple cleanup.
And IMO, it might also be a stable material.
I didn't notice this race bug because there is no bug report against this.

Should I change the description around this and re-send the patch?

Thanks and best regards
-Lv

> 
> Or am I missing anything?
> 
> >  			spin_unlock_irqrestore(&ec->lock, flags);
> >  		} while (time_before(jiffies, delay));
> >  		pr_debug("controller reset, restart transaction\n");
> > @@ -688,8 +690,7 @@ static u32 acpi_ec_gpe_handler(acpi_handle gpe_device,
> >  	struct acpi_ec *ec = data;
> >
> >  	spin_lock_irqsave(&ec->lock, flags);
> > -	if (advance_transaction(ec))
> > -		wake_up(&ec->wait);
> > +	advance_transaction(ec);
> >  	spin_unlock_irqrestore(&ec->lock, flags);
> >  	ec_check_sci(ec, acpi_ec_read_status(ec));
> >  	return ACPI_INTERRUPT_HANDLED | ACPI_REENABLE_GPE;
> >
> 
> --
> I speak only for myself.
> Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 1/6] ACPI/EC: Cleanup transaction wakeup code

[Rafael J. Wysocki]

On Thursday, January 22, 2015 06:37:28 AM Zheng, Lv wrote:
> Hi, Rafael
> 
> > From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net]
> > Sent: Thursday, January 22, 2015 6:17 AM
> > 
> > On Wednesday, January 14, 2015 07:28:22 PM Lv Zheng wrote:
> > > This patch moves transaction wakeup code into advance_transaction().
> > > No functional changes.
> > >
> > > Signed-off-by: Lv Zheng <lv.zheng@intel.com>
> > > ---
> > >  drivers/acpi/ec.c |   17 +++++++++--------
> > >  1 file changed, 9 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
> > > index 1b5853f..3e19123 100644
> > > --- a/drivers/acpi/ec.c
> > > +++ b/drivers/acpi/ec.c
> > > @@ -200,7 +200,7 @@ static int ec_transaction_completed(struct acpi_ec *ec)
> > >  	return ret;
> > >  }
> > >
> > > -static bool advance_transaction(struct acpi_ec *ec)
> > > +static void advance_transaction(struct acpi_ec *ec)
> > >  {
> > >  	struct transaction *t;
> > >  	u8 status;
> > > @@ -235,7 +235,7 @@ static bool advance_transaction(struct acpi_ec *ec)
> > >  			t->flags |= ACPI_EC_COMMAND_COMPLETE;
> > >  			wakeup = true;
> > >  		}
> > > -		return wakeup;
> > > +		goto out;
> > >  	} else {
> > >  		if (EC_FLAGS_QUERY_HANDSHAKE &&
> > >  		    !(status & ACPI_EC_FLAG_SCI) &&
> > > @@ -251,7 +251,7 @@ static bool advance_transaction(struct acpi_ec *ec)
> > >  			t->flags |= ACPI_EC_COMMAND_POLL;
> > >  		} else
> > >  			goto err;
> > > -		return wakeup;
> > > +		goto out;
> > >  	}
> > >  err:
> > >  	/*
> > > @@ -262,14 +262,16 @@ err:
> > >  		if (in_interrupt() && t)
> > >  			++t->irq_count;
> > >  	}
> > > -	return wakeup;
> > > +out:
> > > +	if (wakeup && in_interrupt())
> > > +		wake_up(&ec->wait);
> > >  }
> > >
> > >  static void start_transaction(struct acpi_ec *ec)
> > >  {
> > >  	ec->curr->irq_count = ec->curr->wi = ec->curr->ri = 0;
> > >  	ec->curr->flags = 0;
> > > -	(void)advance_transaction(ec);
> > > +	advance_transaction(ec);
> > 
> > Well, this looks like a functional change, because we wouldn't call
> > wake_up(&ec->wait) here before.
> 
> Ah, Yes.
> But here, since the only advancement that can happen here is to send the EC command and there are always further advancements until transaction completion, the wake_up() won't be invoked at this point.
> So IMO, there is no functional changes here.
> 
> > 
> > >  }
> > >
> > >  static int acpi_ec_sync_query(struct acpi_ec *ec, u8 *data);
> > > @@ -304,7 +306,7 @@ static int ec_poll(struct acpi_ec *ec)
> > >  					return 0;
> > >  			}
> > >  			spin_lock_irqsave(&ec->lock, flags);
> > > -			(void)advance_transaction(ec);
> > > +			advance_transaction(ec);
> > 
> > Ditto.
> 
> Yes. I changed this logic.
> 
> By invoking wake_up() here, we can break the ec_poll() loop.
> Because if the transaction has completed due to the polling advancement, we really don't need to wait for another interrupt.
> So this new logic makes this patch more like a race fix, not a simple cleanup.
> And IMO, it might also be a stable material.
> I didn't notice this race bug because there is no bug report against this.
> 
> Should I change the description around this and re-send the patch?

Yes, please.


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

RE: [PATCH 1/6] ACPI/EC: Cleanup transaction wakeup code

[Zheng, Lv]

Hi, Rafael

I'm sorry something is wrong in my previous reply.
Let me correct it.

> From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net]
> Sent: Friday, January 23, 2015 12:01 AM
> 
> On Thursday, January 22, 2015 06:37:28 AM Zheng, Lv wrote:
> > Hi, Rafael
> >
> > > From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net]
> > > Sent: Thursday, January 22, 2015 6:17 AM
> > >
> > > On Wednesday, January 14, 2015 07:28:22 PM Lv Zheng wrote:

<skip>

> > > > @@ -262,14 +262,16 @@ err:
> > > >  		if (in_interrupt() && t)
> > > >  			++t->irq_count;
> > > >  	}
> > > > -	return wakeup;
> > > > +out:
> > > > +	if (wakeup && in_interrupt())
> > > > +		wake_up(&ec->wait);
> > > >  }
> > > >

I forgot this diff block.
The wake_up() is still invoked for the advance_transaction() invoked in
the GPE handler because of in_interrupt() check.
For the 2 task context invocations, wake_up() won't be invoked.

> > > >  static void start_transaction(struct acpi_ec *ec)
> > > >  {
> > > >  	ec->curr->irq_count = ec->curr->wi = ec->curr->ri = 0;
> > > >  	ec->curr->flags = 0;
> > > > -	(void)advance_transaction(ec);
> > > > +	advance_transaction(ec);
> > > >  }
> > > >
> > > >  static int acpi_ec_sync_query(struct acpi_ec *ec, u8 *data);
> > >
> > > Well, this looks like a functional change, because we wouldn't call
> > > wake_up(&ec->wait) here before.
> >
> > Ah, Yes.
> > But here, since the only advancement that can happen here is to send
> > the EC command and there are always further advancements
> > until transaction completion, the wake_up() won't be invoked at this point.
> > So IMO, there is no functional changes here.

Thus I didn't change the logic here.

> > > > @@ -304,7 +306,7 @@ static int ec_poll(struct acpi_ec *ec)
> > > >  					return 0;
> > > >  			}
> > > >  			spin_lock_irqsave(&ec->lock, flags);
> > > > -			(void)advance_transaction(ec);
> > > > +			advance_transaction(ec);
> > >
> > > Ditto.
> >
> > Yes. I changed this logic.

And I didn't change the logic here.

> > By invoking wake_up() here, we can break the ec_poll() loop.
> > Because if the transaction has completed due to the polling advancement, we really don't need to wait for another interrupt.

This statement is also wrong.
After checking include/linux/wait.h I realized that we originally broke
ec_poll() loop because of ec_transaction_complete() check.

See include/linux/wait.h:
The condition is checked in __wait_cond_timeout() prior than doing
__wait_event_timeout() which may put the task in the wait queue.
So if the condition is matched, wait_event_timeout() just returns.

For the above 2 advance_transaction() invocations, they are both
invoked in the task that might be put into the wait queue.
But at this point, the task is already in the run queue, so wake_up()
takes no effect to this and wait_event_timeout() can break ec_poll()
because of the condition check (ec_transaction_complete()).

As a conclusion, in fact, even there is no in_interrupt() check made
before wake_up(), the modification can still be a no-op.

> > So this new logic makes this patch more like a race fix, not a simple cleanup.
> > And IMO, it might also be a stable material.
> > I didn't notice this race bug because there is no bug report against this.

There is no bug because ec_poll() is broken with ec_transaction_complete()
check. Thus this is not a bug fix and cannot be a stable candidate.

> > Should I change the description around this and re-send the patch?

Thus I needn't change the description for this patch.

> Yes, please.

It seems I needn't to change anything.
So it might not be suitable to re-send the patchset because of the above discussion.
My bad sorry for the mistake.

Could you help to check the rest patches of this revision.
Thanks in advance.

Best regards
-Lv

Re: [PATCH 1/6] ACPI/EC: Cleanup transaction wakeup code

[Rafael J. Wysocki]

On Friday, January 23, 2015 02:18:10 AM Zheng, Lv wrote:
> Hi, Rafael
> 
> I'm sorry something is wrong in my previous reply.
> Let me correct it.
> 
> > From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net]
> > Sent: Friday, January 23, 2015 12:01 AM
> > 
> > On Thursday, January 22, 2015 06:37:28 AM Zheng, Lv wrote:
> > > Hi, Rafael
> > >
> > > > From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net]
> > > > Sent: Thursday, January 22, 2015 6:17 AM
> > > >
> > > > On Wednesday, January 14, 2015 07:28:22 PM Lv Zheng wrote:
> 
> <skip>
> 
> > > > > @@ -262,14 +262,16 @@ err:
> > > > >  		if (in_interrupt() && t)
> > > > >  			++t->irq_count;
> > > > >  	}
> > > > > -	return wakeup;
> > > > > +out:
> > > > > +	if (wakeup && in_interrupt())
> > > > > +		wake_up(&ec->wait);
> > > > >  }
> > > > >
> 
> I forgot this diff block.
> The wake_up() is still invoked for the advance_transaction() invoked in
> the GPE handler because of in_interrupt() check.
> For the 2 task context invocations, wake_up() won't be invoked.
> 
> > > > >  static void start_transaction(struct acpi_ec *ec)
> > > > >  {
> > > > >  	ec->curr->irq_count = ec->curr->wi = ec->curr->ri = 0;
> > > > >  	ec->curr->flags = 0;
> > > > > -	(void)advance_transaction(ec);
> > > > > +	advance_transaction(ec);
> > > > >  }
> > > > >
> > > > >  static int acpi_ec_sync_query(struct acpi_ec *ec, u8 *data);
> > > >
> > > > Well, this looks like a functional change, because we wouldn't call
> > > > wake_up(&ec->wait) here before.
> > >
> > > Ah, Yes.
> > > But here, since the only advancement that can happen here is to send
> > > the EC command and there are always further advancements
> > > until transaction completion, the wake_up() won't be invoked at this point.
> > > So IMO, there is no functional changes here.
> 
> Thus I didn't change the logic here.
> 
> > > > > @@ -304,7 +306,7 @@ static int ec_poll(struct acpi_ec *ec)
> > > > >  					return 0;
> > > > >  			}
> > > > >  			spin_lock_irqsave(&ec->lock, flags);
> > > > > -			(void)advance_transaction(ec);
> > > > > +			advance_transaction(ec);
> > > >
> > > > Ditto.
> > >
> > > Yes. I changed this logic.
> 
> And I didn't change the logic here.
> 
> > > By invoking wake_up() here, we can break the ec_poll() loop.
> > > Because if the transaction has completed due to the polling advancement, we really don't need to wait for another interrupt.
> 
> This statement is also wrong.
> After checking include/linux/wait.h I realized that we originally broke
> ec_poll() loop because of ec_transaction_complete() check.
> 
> See include/linux/wait.h:
> The condition is checked in __wait_cond_timeout() prior than doing
> __wait_event_timeout() which may put the task in the wait queue.
> So if the condition is matched, wait_event_timeout() just returns.
> 
> For the above 2 advance_transaction() invocations, they are both
> invoked in the task that might be put into the wait queue.
> But at this point, the task is already in the run queue, so wake_up()
> takes no effect to this and wait_event_timeout() can break ec_poll()
> because of the condition check (ec_transaction_complete()).
> 
> As a conclusion, in fact, even there is no in_interrupt() check made
> before wake_up(), the modification can still be a no-op.
> 
> > > So this new logic makes this patch more like a race fix, not a simple cleanup.
> > > And IMO, it might also be a stable material.
> > > I didn't notice this race bug because there is no bug report against this.
> 
> There is no bug because ec_poll() is broken with ec_transaction_complete()
> check. Thus this is not a bug fix and cannot be a stable candidate.
> 
> > > Should I change the description around this and re-send the patch?
> 
> Thus I needn't change the description for this patch.
> 
> > Yes, please.
> 
> It seems I needn't to change anything.

OK, I see.  Thanks for the analysis.

> So it might not be suitable to re-send the patchset because of the above discussion.
> My bad sorry for the mistake.
> 
> Could you help to check the rest patches of this revision.

I will, thanks!


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.