Skip to contentSkip to conversation

ACPI/EC: Enhance EC driver state machine

RFC PATCH In review Lv Zheng 2 participants 20 messages Dec 19, 2014 – Jan 23, 2015 ACPI

lore
Share & tools
lore

Lv Zheng

author
#0 lore raw

cover ACPI/EC: Enhance EC driver state machine

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(-)
Signature · 1 line
1.7.10
author
#1 lore raw

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

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

Signed-off-by Lv Zheng <[email protected]>
---
 drivers/acpi/ec.c |   17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)
drivers/acpi/ec.c +9 -8
index 1b5853f..3e19123 100644
@@ -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;
Signature · 1 line
1.7.10
author
#2 lore raw

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

In reply to Lv Zheng

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

Signed-off-by Lv Zheng <[email protected]>
Tested-by Steffen Weber <[email protected]>
Tested-by Ortwin Glück <[email protected]>
Acked-by Rafael J. Wysocki <[email protected]>
---
 drivers/acpi/ec.c |   43 +++++++++++++++++++++++++++++++++----------
 1 file changed, 33 insertions(+), 10 deletions(-)
drivers/acpi/ec.c +33 -10
index 3e19123..87b9911 100644
@@ -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;
Signature · 6 lines
1.7.10

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
author
#3 lore raw

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

In reply to 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 <[email protected]>
---
 drivers/acpi/ec.c |   16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)
drivers/acpi/ec.c +10 -6
index 87b9911..a94ee9f 100644
@@ -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)
Signature · 1 line
1.7.10
author
#4 lore raw

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

In reply to 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 <[email protected]>
---
 drivers/acpi/ec.c |   10 ++++++++++
 1 file changed, 10 insertions(+)
drivers/acpi/ec.c +10 -0
index a94ee9f..3c97122 100644
@@ -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);
 }
Signature · 1 line
1.7.10
author
#5 lore raw

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

In reply to 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 <[email protected]>
Signed-off-by Lv Zheng <[email protected]>
---
 drivers/acpi/ec.c       |   59 ++++++++++++++++++++---------------------------
 drivers/acpi/internal.h |    1 +
 2 files changed, 26 insertions(+), 34 deletions(-)
drivers/acpi/ec.c +25 -34
index 3c97122..89e89b2 100644
@@ -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;
 }
drivers/acpi/internal.h +1 -0
index 163e82f..dc42078 100644
@@ -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;
Signature · 1 line
1.7.10
author
#6 lore raw

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

In reply to 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 <[email protected]>
---
 drivers/acpi/ec.c |   70 +++++++++++++++--------------------------------------
 1 file changed, 20 insertions(+), 50 deletions(-)
drivers/acpi/ec.c +20 -50
index 89e89b2..c385606 100644
@@ -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;
 }
Signature · 1 line
1.7.10
#7 lore raw
In reply to Lv Zheng
Quoted 17 lines · 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

Quoted 3 lines
 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.

Signature · 2 lines
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
author
#8 lore raw

cover ACPI/EC: Enhance EC driver state machine

In reply to 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(-)
Signature · 1 line
1.7.10
author
#9 lore raw

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

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

Signed-off-by Lv Zheng <[email protected]>
---
 drivers/acpi/ec.c |   17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)
drivers/acpi/ec.c +9 -8
index 1b5853f..3e19123 100644
@@ -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;
Signature · 1 line
1.7.10
#10 lore raw

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

Quoted 45 lines · Lv Zheng

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

Signed-off-by Lv Zheng <[email protected]>
---
 drivers/acpi/ec.c |   17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)
drivers/acpi/ec.c +4 -4
index 1b5853f..3e19123 100644
@@ -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:

drivers/acpi/ec.c err: · line 274

Quoted 10 lines
drivers/acpi/ec.c +3 -1
+	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.

drivers/acpi/ec.c static int ec_poll(struct acpi_ec *ec) · line 309

Quoted 9 lines
drivers/acpi/ec.c +1 -1
 }
 
 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?

Quoted 14 lines
drivers/acpi/ec.c +1 -2
 			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;
Signature · 2 lines
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
author
#11 lore raw

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

Hi, Rafael

Quoted 53 lines

From: Rafael J. Wysocki [mailto:[email protected]]
Sent: Thursday, January 22, 2015 6:17 AM

Quoted 49 lines · Lv Zheng

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

Signed-off-by Lv Zheng <[email protected]>
---
 drivers/acpi/ec.c |   17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)
drivers/acpi/ec.c +6 -4
index 1b5853f..3e19123 100644
@@ -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);
 }
Quoted 9 lines

drivers/acpi/ec.c err: · line 274

Quoted 6 lines
drivers/acpi/ec.c +1 -1
 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.

Quoted 12 lines

drivers/acpi/ec.c static int ec_poll(struct acpi_ec *ec) · line 309

Quoted 10 lines
drivers/acpi/ec.c +1 -1
 }
 
 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

Quoted 21 lines

Or am I missing anything?

Quoted 5 lines
drivers/acpi/ec.c +0 -0
 			spin_unlock_irqrestore(&ec->lock, flags);
 		} while (time_before(jiffies, delay));
 		pr_debug("controller reset, restart transaction\n");

drivers/acpi/ec.c static u32 acpi_ec_gpe_handler(acpi_handle gpe_device, · line 696

Quoted 9 lines
drivers/acpi/ec.c +1 -2
@@ -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.

#12 lore raw

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

Quoted 84 lines · Lv

Hi, Rafael

Quoted 53 lines

From: Rafael J. Wysocki [mailto:[email protected]]
Sent: Thursday, January 22, 2015 6:17 AM

Quoted 49 lines · Lv Zheng

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

Signed-off-by Lv Zheng <[email protected]>
---
 drivers/acpi/ec.c |   17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)
drivers/acpi/ec.c +6 -4
index 1b5853f..3e19123 100644
@@ -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);
 }
Quoted 9 lines

drivers/acpi/ec.c err: · line 274

Quoted 6 lines
drivers/acpi/ec.c +1 -1
 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.

Quoted 12 lines

drivers/acpi/ec.c static int ec_poll(struct acpi_ec *ec) · line 309

Quoted 10 lines
drivers/acpi/ec.c +1 -1
 }
 
 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.

Quoted 7 lines

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.

Signature · 2 lines
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
author
#13 lore raw

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

Hi, Rafael

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

Quoted 10 lines

From: Rafael J. Wysocki [mailto:[email protected]]
Sent: Friday, January 23, 2015 12:01 AM

Quoted 6 lines · Lv

Hi, Rafael

Quoted 4 lines

From: Rafael J. Wysocki [mailto:[email protected]]
Sent: Thursday, January 22, 2015 6:17 AM

On Wednesday, January 14, 2015 07:28:22 PM Lv Zheng wrote:

<skip>

Quoted 10 lines
Quoted 10 lines
Quoted 10 lines

drivers/acpi/ec.c err: · line 269

Quoted 10 lines
drivers/acpi/ec.c +3 -1
@@ -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.

Quoted 13 lines
Quoted 13 lines
Quoted 12 lines

drivers/acpi/ec.c err: · line 277

Quoted 9 lines
drivers/acpi/ec.c +1 -1
 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.

Quoted 5 lines
Quoted 5 lines

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.

Quoted 10 lines
Quoted 10 lines
Quoted 8 lines

drivers/acpi/ec.c static int ec_poll(struct acpi_ec *ec) · line 309

Quoted 6 lines
drivers/acpi/ec.c +1 -1
@@ -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.

Quoted 2 lines
Quoted 2 lines

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.

Quoted 3 lines
Quoted 3 lines

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.

Quoted 1 line
Quoted 1 line

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

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

Quoted 1 line

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

#14 lore raw

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

Quoted 96 lines · Lv

Hi, Rafael

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

Quoted 10 lines

From: Rafael J. Wysocki [mailto:[email protected]]
Sent: Friday, January 23, 2015 12:01 AM

Quoted 6 lines · Lv

Hi, Rafael

Quoted 4 lines

From: Rafael J. Wysocki [mailto:[email protected]]
Sent: Thursday, January 22, 2015 6:17 AM

On Wednesday, January 14, 2015 07:28:22 PM Lv Zheng wrote:

<skip>

Quoted 10 lines
Quoted 10 lines
Quoted 10 lines

drivers/acpi/ec.c err: · line 269

Quoted 10 lines
drivers/acpi/ec.c +3 -1
@@ -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.

Quoted 13 lines
Quoted 13 lines
Quoted 12 lines

drivers/acpi/ec.c err: · line 277

Quoted 9 lines
drivers/acpi/ec.c +1 -1
 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.

Quoted 5 lines
Quoted 5 lines

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.

Quoted 10 lines
Quoted 10 lines
Quoted 8 lines

drivers/acpi/ec.c static int ec_poll(struct acpi_ec *ec) · line 309

Quoted 6 lines
drivers/acpi/ec.c +1 -1
@@ -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.

Quoted 2 lines
Quoted 2 lines

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.

Quoted 3 lines
Quoted 3 lines

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.

Quoted 7 lines
Quoted 1 line
Quoted 1 line

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

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

Quoted 1 line

Yes, please.

It seems I needn't to change anything.

OK, I see. Thanks for the analysis.

Quoted 4 lines

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!

Signature · 2 lines
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
author
#15 lore raw

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

In reply to Lv Zheng

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

Signed-off-by Lv Zheng <[email protected]>
Tested-by Steffen Weber <[email protected]>
Tested-by Ortwin Glück <[email protected]>
Acked-by Rafael J. Wysocki <[email protected]>
---
 drivers/acpi/ec.c |   43 +++++++++++++++++++++++++++++++++----------
 1 file changed, 33 insertions(+), 10 deletions(-)
drivers/acpi/ec.c +33 -10
index 3e19123..87b9911 100644
@@ -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;
Signature · 6 lines
1.7.10

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
author
#16 lore raw

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

In reply to 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 <[email protected]>
---
 drivers/acpi/ec.c |   16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)
drivers/acpi/ec.c +10 -6
index 87b9911..a94ee9f 100644
@@ -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)
Signature · 1 line
1.7.10
author
#17 lore raw

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

In reply to 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 <[email protected]>
---
 drivers/acpi/ec.c |   10 ++++++++++
 1 file changed, 10 insertions(+)
drivers/acpi/ec.c +10 -0
index a94ee9f..3c97122 100644
@@ -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);
 }
Signature · 1 line
1.7.10
author
#18 lore raw

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

In reply to 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 <[email protected]>
Signed-off-by Lv Zheng <[email protected]>
---
 drivers/acpi/ec.c       |   59 ++++++++++++++++++++---------------------------
 drivers/acpi/internal.h |    1 +
 2 files changed, 26 insertions(+), 34 deletions(-)
drivers/acpi/ec.c +25 -34
index 3c97122..89e89b2 100644
@@ -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;
 }
drivers/acpi/internal.h +1 -0
index 163e82f..dc42078 100644
@@ -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;
Signature · 1 line
1.7.10
author
#19 lore raw

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

In reply to 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 <[email protected]>
---
 drivers/acpi/ec.c |   70 +++++++++++++++--------------------------------------
 1 file changed, 20 insertions(+), 50 deletions(-)
drivers/acpi/ec.c +20 -50
index 89e89b2..c385606 100644
@@ -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;
 }
Signature · 1 line
1.7.10