[PATCH 00/10] ACPI: EC: Two fixes and cleanups

[PATCH 00/10] ACPI: EC: Two fixes and cleanups

[Rafael J. Wysocki]

Hi All,

This series addresses two issues in the EC driver (patches [1-2/10] and cleans
up the code in it on top of that.

Please see the patch changelogs for details.

Thanks!

[PATCH 01/10] ACPI: EC: Rework flushing of EC work while suspended to idle

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

The flushing of pending work in the EC driver uses drain_workqueue()
to flush the event handling work that can requeue itself via
advance_transaction(), but this is problematic, because that
work may also be requeued from the query workqueue.

Namely, if an EC transaction is carried out during the execution of
a query handler, it involves calling advance_transaction() which
may queue up the event handling work again.  This causes the kernel
to complain about attempts to add a work item to the EC event
workqueue while it is being drained and worst-case it may cause a
valid event to be skipped.

To avoid this problem, introduce two new counters, events_in_progress
and queries_in_progress, incremented when a work item is queued on
the event workqueue or the query workqueue, respectively, and
decremented at the end of the corresponding work function, and make
acpi_ec_dispatch_gpe() the workqueues in a loop until the both of
these counters are zero (or system wakeup is pending) instead of
calling acpi_ec_flush_work().

At the same time, change __acpi_ec_flush_work() to call
flush_workqueue() instead of drain_workqueue() to flush the event
workqueue.

While at it, use the observation that the work item queued in
acpi_ec_query() cannot be pending at that time, because it is used
only once, to simplify the code in there.

Additionally, clean up a comment in acpi_ec_query() and adjust white
space in acpi_ec_event_processor().

Fixes: f0ac20c3f613 ("ACPI: EC: Fix flushing of pending work")
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/ec.c       |   57 ++++++++++++++++++++++++++++++++++++------------
 drivers/acpi/internal.h |    2 +
 2 files changed, 45 insertions(+), 14 deletions(-)

Index: linux-pm/drivers/acpi/ec.c
===================================================================
--- linux-pm.orig/drivers/acpi/ec.c
+++ linux-pm/drivers/acpi/ec.c
@@ -166,6 +166,7 @@ struct acpi_ec_query {
 	struct transaction transaction;
 	struct work_struct work;
 	struct acpi_ec_query_handler *handler;
+	struct acpi_ec *ec;
 };
 
 static int acpi_ec_query(struct acpi_ec *ec, u8 *data);
@@ -452,6 +453,7 @@ static void acpi_ec_submit_query(struct
 		ec_dbg_evt("Command(%s) submitted/blocked",
 			   acpi_ec_cmd_string(ACPI_EC_COMMAND_QUERY));
 		ec->nr_pending_queries++;
+		ec->events_in_progress++;
 		queue_work(ec_wq, &ec->work);
 	}
 }
@@ -518,7 +520,7 @@ static void acpi_ec_enable_event(struct
 #ifdef CONFIG_PM_SLEEP
 static void __acpi_ec_flush_work(void)
 {
-	drain_workqueue(ec_wq); /* flush ec->work */
+	flush_workqueue(ec_wq); /* flush ec->work */
 	flush_workqueue(ec_query_wq); /* flush queries */
 }
 
@@ -1103,7 +1105,7 @@ void acpi_ec_remove_query_handler(struct
 }
 EXPORT_SYMBOL_GPL(acpi_ec_remove_query_handler);
 
-static struct acpi_ec_query *acpi_ec_create_query(u8 *pval)
+static struct acpi_ec_query *acpi_ec_create_query(struct acpi_ec *ec, u8 *pval)
 {
 	struct acpi_ec_query *q;
 	struct transaction *t;
@@ -1111,11 +1113,13 @@ static struct acpi_ec_query *acpi_ec_cre
 	q = kzalloc(sizeof (struct acpi_ec_query), GFP_KERNEL);
 	if (!q)
 		return NULL;
+
 	INIT_WORK(&q->work, acpi_ec_event_processor);
 	t = &q->transaction;
 	t->command = ACPI_EC_COMMAND_QUERY;
 	t->rdata = pval;
 	t->rlen = 1;
+	q->ec = ec;
 	return q;
 }
 
@@ -1132,13 +1136,21 @@ static void acpi_ec_event_processor(stru
 {
 	struct acpi_ec_query *q = container_of(work, struct acpi_ec_query, work);
 	struct acpi_ec_query_handler *handler = q->handler;
+	struct acpi_ec *ec = q->ec;
 
 	ec_dbg_evt("Query(0x%02x) started", handler->query_bit);
+
 	if (handler->func)
 		handler->func(handler->data);
 	else if (handler->handle)
 		acpi_evaluate_object(handler->handle, NULL, NULL, NULL);
+
 	ec_dbg_evt("Query(0x%02x) stopped", handler->query_bit);
+
+	spin_lock_irq(&ec->lock);
+	ec->queries_in_progress--;
+	spin_unlock_irq(&ec->lock);
+
 	acpi_ec_delete_query(q);
 }
 
@@ -1148,7 +1160,7 @@ static int acpi_ec_query(struct acpi_ec
 	int result;
 	struct acpi_ec_query *q;
 
-	q = acpi_ec_create_query(&value);
+	q = acpi_ec_create_query(ec, &value);
 	if (!q)
 		return -ENOMEM;
 
@@ -1170,19 +1182,20 @@ static int acpi_ec_query(struct acpi_ec
 	}
 
 	/*
-	 * It is reported that _Qxx are evaluated in a parallel way on
-	 * Windows:
+	 * It is reported that _Qxx are evaluated in a parallel way on Windows:
 	 * https://bugzilla.kernel.org/show_bug.cgi?id=94411
 	 *
-	 * Put this log entry before schedule_work() in order to make
-	 * it appearing before any other log entries occurred during the
-	 * work queue execution.
+	 * Put this log entry before queue_work() to make it appear in the log
+	 * before any other messages emitted during workqueue handling.
 	 */
 	ec_dbg_evt("Query(0x%02x) scheduled", value);
-	if (!queue_work(ec_query_wq, &q->work)) {
-		ec_dbg_evt("Query(0x%02x) overlapped", value);
-		result = -EBUSY;
-	}
+
+	spin_lock_irq(&ec->lock);
+
+	ec->queries_in_progress++;
+	queue_work(ec_query_wq, &q->work);
+
+	spin_unlock_irq(&ec->lock);
 
 err_exit:
 	if (result)
@@ -1240,6 +1253,10 @@ static void acpi_ec_event_handler(struct
 	ec_dbg_evt("Event stopped");
 
 	acpi_ec_check_event(ec);
+
+	spin_lock_irqsave(&ec->lock, flags);
+	ec->events_in_progress--;
+	spin_unlock_irqrestore(&ec->lock, flags);
 }
 
 static void acpi_ec_handle_interrupt(struct acpi_ec *ec)
@@ -2021,6 +2038,7 @@ void acpi_ec_set_gpe_wake_mask(u8 action
 
 bool acpi_ec_dispatch_gpe(void)
 {
+	bool work_in_progress;
 	u32 ret;
 
 	if (!first_ec)
@@ -2041,8 +2059,19 @@ bool acpi_ec_dispatch_gpe(void)
 	if (ret == ACPI_INTERRUPT_HANDLED)
 		pm_pr_dbg("ACPI EC GPE dispatched\n");
 
-	/* Flush the event and query workqueues. */
-	acpi_ec_flush_work();
+	/* Drain EC work. */
+	do {
+		acpi_ec_flush_work();
+
+		pm_pr_dbg("ACPI EC work flushed\n");
+
+		spin_lock_irq(&first_ec->lock);
+
+		work_in_progress = first_ec->events_in_progress +
+			first_ec->queries_in_progress > 0;
+
+		spin_unlock_irq(&first_ec->lock);
+	} while (work_in_progress && !pm_wakeup_pending());
 
 	return false;
 }
Index: linux-pm/drivers/acpi/internal.h
===================================================================
--- linux-pm.orig/drivers/acpi/internal.h
+++ linux-pm/drivers/acpi/internal.h
@@ -184,6 +184,8 @@ struct acpi_ec {
 	struct work_struct work;
 	unsigned long timestamp;
 	unsigned long nr_pending_queries;
+	unsigned int events_in_progress;
+	unsigned int queries_in_progress;
 	bool busy_polling;
 	unsigned int polling_guard;
 };

[PATCH 02/10] ACPI: EC: Call advance_transaction() from acpi_ec_dispatch_gpe()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Calling acpi_dispatch_gpe() from acpi_ec_dispatch_gpe() is generally
problematic, because it may cause the spurious interrupt handling in
advance_transaction() to trigger in theory.

However, instead of calling acpi_dispatch_gpe() to dispatch the EC
GPE, acpi_ec_dispatch_gpe() can call advance_transaction() directly
on first_ec and it can pass 'false' as its second argument to indicate
calling it from process context.

Moreover, if advance_transaction() is modified to return a bool value
indicating whether or not the EC work needs to be flushed, it can be
used to avoid unnecessary EC work flushing in acpi_ec_dispatch_gpe(),
so change the code accordingly.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/ec.c |   39 ++++++++++++++++++++++++++++-----------
 1 file changed, 28 insertions(+), 11 deletions(-)

Index: linux-pm/drivers/acpi/ec.c
===================================================================
--- linux-pm.orig/drivers/acpi/ec.c
+++ linux-pm/drivers/acpi/ec.c
@@ -170,7 +170,7 @@ struct acpi_ec_query {
 };
 
 static int acpi_ec_query(struct acpi_ec *ec, u8 *data);
-static void advance_transaction(struct acpi_ec *ec, bool interrupt);
+static bool advance_transaction(struct acpi_ec *ec, bool interrupt);
 static void acpi_ec_event_handler(struct work_struct *work);
 static void acpi_ec_event_processor(struct work_struct *work);
 
@@ -444,18 +444,25 @@ static bool acpi_ec_submit_flushable_req
 	return true;
 }
 
-static void acpi_ec_submit_query(struct acpi_ec *ec)
+static bool acpi_ec_submit_query(struct acpi_ec *ec)
 {
 	acpi_ec_mask_events(ec);
 	if (!acpi_ec_event_enabled(ec))
-		return;
+		return false;
+
 	if (!test_and_set_bit(EC_FLAGS_QUERY_PENDING, &ec->flags)) {
 		ec_dbg_evt("Command(%s) submitted/blocked",
 			   acpi_ec_cmd_string(ACPI_EC_COMMAND_QUERY));
 		ec->nr_pending_queries++;
 		ec->events_in_progress++;
-		queue_work(ec_wq, &ec->work);
+		return queue_work(ec_wq, &ec->work);
 	}
+
+	/*
+	 * The event handling work has not been completed yet, so it needs to be
+	 * flushed.
+	 */
+	return true;
 }
 
 static void acpi_ec_complete_query(struct acpi_ec *ec)
@@ -628,10 +635,11 @@ static void acpi_ec_spurious_interrupt(s
 		acpi_ec_mask_events(ec);
 }
 
-static void advance_transaction(struct acpi_ec *ec, bool interrupt)
+static bool advance_transaction(struct acpi_ec *ec, bool interrupt)
 {
 	struct transaction *t = ec->curr;
 	bool wakeup = false;
+	bool ret = false;
 	u8 status;
 
 	ec_dbg_stm("%s (%d)", interrupt ? "IRQ" : "TASK", smp_processor_id());
@@ -698,10 +706,12 @@ static void advance_transaction(struct a
 
 out:
 	if (status & ACPI_EC_FLAG_SCI)
-		acpi_ec_submit_query(ec);
+		ret = acpi_ec_submit_query(ec);
 
 	if (wakeup && interrupt)
 		wake_up(&ec->wait);
+
+	return ret;
 }
 
 static void start_transaction(struct acpi_ec *ec)
@@ -2038,8 +2048,7 @@ void acpi_ec_set_gpe_wake_mask(u8 action
 
 bool acpi_ec_dispatch_gpe(void)
 {
-	bool work_in_progress;
-	u32 ret;
+	bool work_in_progress = false;
 
 	if (!first_ec)
 		return acpi_any_gpe_status_set(U32_MAX);
@@ -2055,9 +2064,17 @@ bool acpi_ec_dispatch_gpe(void)
 	 * Dispatch the EC GPE in-band, but do not report wakeup in any case
 	 * to allow the caller to process events properly after that.
 	 */
-	ret = acpi_dispatch_gpe(NULL, first_ec->gpe);
-	if (ret == ACPI_INTERRUPT_HANDLED)
-		pm_pr_dbg("ACPI EC GPE dispatched\n");
+	spin_lock_irq(&first_ec->lock);
+
+	if (acpi_ec_gpe_status_set(first_ec))
+		work_in_progress = advance_transaction(first_ec, false);
+
+	spin_unlock_irq(&first_ec->lock);
+
+	if (!work_in_progress)
+		return false;
+
+	pm_pr_dbg("ACPI EC GPE dispatched\n");
 
 	/* Drain EC work. */
 	do {

[PATCH 03/10] ACPI: EC: Pass one argument to acpi_ec_query()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Notice that the second argument to acpi_ec_query() is redundant,
because in the only case when it is not NULL, the value passed
through it is only checked against 0 and it can only be 0 when
acpi_ec_query() returns an error code, but its return value
is checked along with the value passed through its second
argument.

Accordingly, modify acpi_ec_query() to take only one argument
and while at it, change its handling of the case when
acpi_ec_transaction() returns an error so as to return that
error value to the caller right away.

No expected functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/ec.c |   26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

Index: linux-pm/drivers/acpi/ec.c
===================================================================
--- linux-pm.orig/drivers/acpi/ec.c
+++ linux-pm/drivers/acpi/ec.c
@@ -169,7 +169,7 @@ struct acpi_ec_query {
 	struct acpi_ec *ec;
 };
 
-static int acpi_ec_query(struct acpi_ec *ec, u8 *data);
+static int acpi_ec_query(struct acpi_ec *ec);
 static bool advance_transaction(struct acpi_ec *ec, bool interrupt);
 static void acpi_ec_event_handler(struct work_struct *work);
 static void acpi_ec_event_processor(struct work_struct *work);
@@ -496,12 +496,10 @@ static inline void __acpi_ec_disable_eve
  */
 static void acpi_ec_clear(struct acpi_ec *ec)
 {
-	int i, status;
-	u8 value = 0;
+	int i;
 
 	for (i = 0; i < ACPI_EC_CLEAR_MAX; i++) {
-		status = acpi_ec_query(ec, &value);
-		if (status || !value)
+		if (acpi_ec_query(ec))
 			break;
 	}
 	if (unlikely(i == ACPI_EC_CLEAR_MAX))
@@ -1164,11 +1162,11 @@ static void acpi_ec_event_processor(stru
 	acpi_ec_delete_query(q);
 }
 
-static int acpi_ec_query(struct acpi_ec *ec, u8 *data)
+static int acpi_ec_query(struct acpi_ec *ec)
 {
+	struct acpi_ec_query *q;
 	u8 value = 0;
 	int result;
-	struct acpi_ec_query *q;
 
 	q = acpi_ec_create_query(ec, &value);
 	if (!q)
@@ -1180,11 +1178,14 @@ static int acpi_ec_query(struct acpi_ec
 	 * bit to be cleared (and thus clearing the interrupt source).
 	 */
 	result = acpi_ec_transaction(ec, &q->transaction);
-	if (!value)
-		result = -ENODATA;
 	if (result)
 		goto err_exit;
 
+	if (!value) {
+		result = -ENODATA;
+		goto err_exit;
+	}
+
 	q->handler = acpi_ec_get_query_handler_by_value(ec, value);
 	if (!q->handler) {
 		result = -ENODATA;
@@ -1210,8 +1211,7 @@ static int acpi_ec_query(struct acpi_ec
 err_exit:
 	if (result)
 		acpi_ec_delete_query(q);
-	if (data)
-		*data = value;
+
 	return result;
 }
 
@@ -1243,7 +1243,9 @@ static void acpi_ec_event_handler(struct
 	spin_lock_irqsave(&ec->lock, flags);
 	while (ec->nr_pending_queries) {
 		spin_unlock_irqrestore(&ec->lock, flags);
-		(void)acpi_ec_query(ec, NULL);
+
+		acpi_ec_query(ec);
+
 		spin_lock_irqsave(&ec->lock, flags);
 		ec->nr_pending_queries--;
 		/*

[PATCH 04/10] ACPI: EC: Fold acpi_ec_check_event() into acpi_ec_event_handler()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Because acpi_ec_event_handler() is the only caller of
acpi_ec_check_event() and the separation of these two functions
makes it harder to follow the code flow, fold the latter into the
former (and simplify that code while at it).

No expected functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/ec.c |   28 +++++++++-------------------
 1 file changed, 9 insertions(+), 19 deletions(-)

Index: linux-pm/drivers/acpi/ec.c
===================================================================
--- linux-pm.orig/drivers/acpi/ec.c
+++ linux-pm/drivers/acpi/ec.c
@@ -1215,24 +1215,6 @@ err_exit:
 	return result;
 }
 
-static void acpi_ec_check_event(struct acpi_ec *ec)
-{
-	unsigned long flags;
-
-	if (ec_event_clearing == ACPI_EC_EVT_TIMING_EVENT) {
-		if (ec_guard(ec)) {
-			spin_lock_irqsave(&ec->lock, flags);
-			/*
-			 * Take care of the SCI_EVT unless no one else is
-			 * taking care of it.
-			 */
-			if (!ec->curr)
-				advance_transaction(ec, false);
-			spin_unlock_irqrestore(&ec->lock, flags);
-		}
-	}
-}
-
 static void acpi_ec_event_handler(struct work_struct *work)
 {
 	unsigned long flags;
@@ -1264,7 +1246,15 @@ static void acpi_ec_event_handler(struct
 
 	ec_dbg_evt("Event stopped");
 
-	acpi_ec_check_event(ec);
+	if (ec_event_clearing == ACPI_EC_EVT_TIMING_EVENT && ec_guard(ec)) {
+		spin_lock_irqsave(&ec->lock, flags);
+
+		/* Take care of SCI_EVT unless someone else is doing that. */
+		if (!ec->curr)
+			advance_transaction(ec, false);
+
+		spin_unlock_irqrestore(&ec->lock, flags);
+	}
 
 	spin_lock_irqsave(&ec->lock, flags);
 	ec->events_in_progress--;

[PATCH 05/10] ACPI: EC: Rearrange the loop in acpi_ec_event_handler()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

It is not necessary to check ec->nr_pending_queries against 0 in the
while () loop in acpi_ec_event_handler(), because that loop terminates
when ec->nr_pending_queries is 0 and the code depending on that can be
run after the loop has ended.

Modify the code accordingly and while at it rewrite the comment
regarding that code to make it clearer.

No intentional functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/ec.c |   21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

Index: linux-pm/drivers/acpi/ec.c
===================================================================
--- linux-pm.orig/drivers/acpi/ec.c
+++ linux-pm/drivers/acpi/ec.c
@@ -1230,18 +1230,17 @@ static void acpi_ec_event_handler(struct
 
 		spin_lock_irqsave(&ec->lock, flags);
 		ec->nr_pending_queries--;
-		/*
-		 * Before exit, make sure that this work item can be
-		 * scheduled again. There might be QR_EC failures, leaving
-		 * EC_FLAGS_QUERY_PENDING uncleared and preventing this work
-		 * item from being scheduled again.
-		 */
-		if (!ec->nr_pending_queries) {
-			if (ec_event_clearing == ACPI_EC_EVT_TIMING_STATUS ||
-			    ec_event_clearing == ACPI_EC_EVT_TIMING_QUERY)
-				acpi_ec_complete_query(ec);
-		}
 	}
+
+	/*
+	 * Before exit, make sure that the it will be possible to queue up the
+	 * event handling work again regardless of whether or not the query
+	 * queued up above is processed successfully.
+	 */
+	if (ec_event_clearing == ACPI_EC_EVT_TIMING_STATUS ||
+	    ec_event_clearing == ACPI_EC_EVT_TIMING_QUERY)
+		acpi_ec_complete_query(ec);
+
 	spin_unlock_irqrestore(&ec->lock, flags);
 
 	ec_dbg_evt("Event stopped");

[PATCH 06/10] ACPI: EC: Simplify locking in acpi_ec_event_handler()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Because acpi_ec_event_handler() is a work function, it always
runs in process context with interrupts enabled, so it can use
spin_lock_irq() and spin_unlock_irq() for the locking.

Make it do so and adjust white space around those calls.

No expected functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/ec.c |   18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

Index: linux-pm/drivers/acpi/ec.c
===================================================================
--- linux-pm.orig/drivers/acpi/ec.c
+++ linux-pm/drivers/acpi/ec.c
@@ -1217,18 +1217,18 @@ err_exit:
 
 static void acpi_ec_event_handler(struct work_struct *work)
 {
-	unsigned long flags;
 	struct acpi_ec *ec = container_of(work, struct acpi_ec, work);
 
 	ec_dbg_evt("Event started");
 
-	spin_lock_irqsave(&ec->lock, flags);
+	spin_lock_irq(&ec->lock);
+
 	while (ec->nr_pending_queries) {
-		spin_unlock_irqrestore(&ec->lock, flags);
+		spin_unlock_irq(&ec->lock);
 
 		acpi_ec_query(ec);
 
-		spin_lock_irqsave(&ec->lock, flags);
+		spin_lock_irq(&ec->lock);
 		ec->nr_pending_queries--;
 	}
 
@@ -1241,23 +1241,23 @@ static void acpi_ec_event_handler(struct
 	    ec_event_clearing == ACPI_EC_EVT_TIMING_QUERY)
 		acpi_ec_complete_query(ec);
 
-	spin_unlock_irqrestore(&ec->lock, flags);
+	spin_unlock_irq(&ec->lock);
 
 	ec_dbg_evt("Event stopped");
 
 	if (ec_event_clearing == ACPI_EC_EVT_TIMING_EVENT && ec_guard(ec)) {
-		spin_lock_irqsave(&ec->lock, flags);
+		spin_lock_irq(&ec->lock);
 
 		/* Take care of SCI_EVT unless someone else is doing that. */
 		if (!ec->curr)
 			advance_transaction(ec, false);
 
-		spin_unlock_irqrestore(&ec->lock, flags);
+		spin_unlock_irq(&ec->lock);
 	}
 
-	spin_lock_irqsave(&ec->lock, flags);
+	spin_lock_irq(&ec->lock);
 	ec->events_in_progress--;
-	spin_unlock_irqrestore(&ec->lock, flags);
+	spin_unlock_irq(&ec->lock);
 }
 
 static void acpi_ec_handle_interrupt(struct acpi_ec *ec)

[PATCH 07/10] ACPI: EC: Rename three functions

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Rename acpi_ec_submit_query() to acpi_ec_submit_event(),
acpi_ec_query() to acpi_ec_submit_query(), and
acpi_ec_complete_query() to acpi_ec_close_event() to make
the names reflect what the functions do.

No expected functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/ec.c |   22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

Index: linux-pm/drivers/acpi/ec.c
===================================================================
--- linux-pm.orig/drivers/acpi/ec.c
+++ linux-pm/drivers/acpi/ec.c
@@ -169,7 +169,7 @@ struct acpi_ec_query {
 	struct acpi_ec *ec;
 };
 
-static int acpi_ec_query(struct acpi_ec *ec);
+static int acpi_ec_submit_query(struct acpi_ec *ec);
 static bool advance_transaction(struct acpi_ec *ec, bool interrupt);
 static void acpi_ec_event_handler(struct work_struct *work);
 static void acpi_ec_event_processor(struct work_struct *work);
@@ -444,7 +444,7 @@ static bool acpi_ec_submit_flushable_req
 	return true;
 }
 
-static bool acpi_ec_submit_query(struct acpi_ec *ec)
+static bool acpi_ec_submit_event(struct acpi_ec *ec)
 {
 	acpi_ec_mask_events(ec);
 	if (!acpi_ec_event_enabled(ec))
@@ -465,7 +465,7 @@ static bool acpi_ec_submit_query(struct
 	return true;
 }
 
-static void acpi_ec_complete_query(struct acpi_ec *ec)
+static void acpi_ec_close_event(struct acpi_ec *ec)
 {
 	if (test_and_clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags))
 		ec_dbg_evt("Command(%s) unblocked",
@@ -499,7 +499,7 @@ static void acpi_ec_clear(struct acpi_ec
 	int i;
 
 	for (i = 0; i < ACPI_EC_CLEAR_MAX; i++) {
-		if (acpi_ec_query(ec))
+		if (acpi_ec_submit_query(ec))
 			break;
 	}
 	if (unlikely(i == ACPI_EC_CLEAR_MAX))
@@ -613,10 +613,10 @@ static inline void ec_transaction_transi
 	if (ec->curr->command == ACPI_EC_COMMAND_QUERY) {
 		if (ec_event_clearing == ACPI_EC_EVT_TIMING_STATUS &&
 		    flag == ACPI_EC_COMMAND_POLL)
-			acpi_ec_complete_query(ec);
+			acpi_ec_close_event(ec);
 		if (ec_event_clearing == ACPI_EC_EVT_TIMING_QUERY &&
 		    flag == ACPI_EC_COMMAND_COMPLETE)
-			acpi_ec_complete_query(ec);
+			acpi_ec_close_event(ec);
 		if (ec_event_clearing == ACPI_EC_EVT_TIMING_EVENT &&
 		    flag == ACPI_EC_COMMAND_COMPLETE)
 			set_bit(EC_FLAGS_QUERY_GUARDING, &ec->flags);
@@ -668,7 +668,7 @@ static bool advance_transaction(struct a
 		    (!ec->nr_pending_queries ||
 		     test_bit(EC_FLAGS_QUERY_GUARDING, &ec->flags))) {
 			clear_bit(EC_FLAGS_QUERY_GUARDING, &ec->flags);
-			acpi_ec_complete_query(ec);
+			acpi_ec_close_event(ec);
 		}
 		if (!t)
 			goto out;
@@ -704,7 +704,7 @@ static bool advance_transaction(struct a
 
 out:
 	if (status & ACPI_EC_FLAG_SCI)
-		ret = acpi_ec_submit_query(ec);
+		ret = acpi_ec_submit_event(ec);
 
 	if (wakeup && interrupt)
 		wake_up(&ec->wait);
@@ -1162,7 +1162,7 @@ static void acpi_ec_event_processor(stru
 	acpi_ec_delete_query(q);
 }
 
-static int acpi_ec_query(struct acpi_ec *ec)
+static int acpi_ec_submit_query(struct acpi_ec *ec)
 {
 	struct acpi_ec_query *q;
 	u8 value = 0;
@@ -1226,7 +1226,7 @@ static void acpi_ec_event_handler(struct
 	while (ec->nr_pending_queries) {
 		spin_unlock_irq(&ec->lock);
 
-		acpi_ec_query(ec);
+		acpi_ec_submit_query(ec);
 
 		spin_lock_irq(&ec->lock);
 		ec->nr_pending_queries--;
@@ -1239,7 +1239,7 @@ static void acpi_ec_event_handler(struct
 	 */
 	if (ec_event_clearing == ACPI_EC_EVT_TIMING_STATUS ||
 	    ec_event_clearing == ACPI_EC_EVT_TIMING_QUERY)
-		acpi_ec_complete_query(ec);
+		acpi_ec_close_event(ec);
 
 	spin_unlock_irq(&ec->lock);
 

[PATCH 08/10] ACPI: EC: Avoid queuing unnecessary work in acpi_ec_submit_event()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Notice that it is not necessary to queue up the event work again
if the while () loop in acpi_ec_event_handler() is still running
which is the case if nr_pending_queries is greater than 0 at the
beginning of acpi_ec_submit_event() and modify the code to avoid
doing that.

While at it, rename nr_pending_queries in struct acpi_ec to
events_to_process which actually matches the role of that field
and change its data type to unsigned int which is sufficient.

No expected functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/ec.c       |   17 +++++++++++++----
 drivers/acpi/internal.h |    2 +-
 2 files changed, 14 insertions(+), 5 deletions(-)

Index: linux-pm/drivers/acpi/internal.h
===================================================================
--- linux-pm.orig/drivers/acpi/internal.h
+++ linux-pm/drivers/acpi/internal.h
@@ -183,7 +183,7 @@ struct acpi_ec {
 	spinlock_t lock;
 	struct work_struct work;
 	unsigned long timestamp;
-	unsigned long nr_pending_queries;
+	unsigned int events_to_process;
 	unsigned int events_in_progress;
 	unsigned int queries_in_progress;
 	bool busy_polling;
Index: linux-pm/drivers/acpi/ec.c
===================================================================
--- linux-pm.orig/drivers/acpi/ec.c
+++ linux-pm/drivers/acpi/ec.c
@@ -453,7 +453,16 @@ static bool acpi_ec_submit_event(struct
 	if (!test_and_set_bit(EC_FLAGS_QUERY_PENDING, &ec->flags)) {
 		ec_dbg_evt("Command(%s) submitted/blocked",
 			   acpi_ec_cmd_string(ACPI_EC_COMMAND_QUERY));
-		ec->nr_pending_queries++;
+		/*
+		 * If events_to_process is greqter than 0 at this point, the
+		 * while () loop in acpi_ec_event_handler() is still running
+		 * and incrementing events_to_process will cause it to invoke
+		 * acpi_ec_submit_query() once more, so it is not necessary to
+		 * queue up the event work to start the same loop again.
+		 */
+		if (ec->events_to_process++ > 0)
+			return true;
+
 		ec->events_in_progress++;
 		return queue_work(ec_wq, &ec->work);
 	}
@@ -665,7 +674,7 @@ static bool advance_transaction(struct a
 	 */
 	if (!t || !(t->flags & ACPI_EC_COMMAND_POLL)) {
 		if (ec_event_clearing == ACPI_EC_EVT_TIMING_EVENT &&
-		    (!ec->nr_pending_queries ||
+		    (!ec->events_to_process ||
 		     test_bit(EC_FLAGS_QUERY_GUARDING, &ec->flags))) {
 			clear_bit(EC_FLAGS_QUERY_GUARDING, &ec->flags);
 			acpi_ec_close_event(ec);
@@ -1223,13 +1232,13 @@ static void acpi_ec_event_handler(struct
 
 	spin_lock_irq(&ec->lock);
 
-	while (ec->nr_pending_queries) {
+	while (ec->events_to_process) {
 		spin_unlock_irq(&ec->lock);
 
 		acpi_ec_submit_query(ec);
 
 		spin_lock_irq(&ec->lock);
-		ec->nr_pending_queries--;
+		ec->events_to_process--;
 	}
 
 	/*

[PATCH 09/10] ACPI: EC: Make the event work state machine visible

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

The EC driver uses a relatively simple state machine for the event
work handling, but it is not really straightforward to figure out.

The states are as follows:

 "Ready": The event handling work can be submitted.

  In this state, the EC_FLAGS_QUERY_PENDING flag is clear.
 
 "In progress": The event handling work is pending or is being
                processed.  It cannot be submitted again.

  In ths state, the EC_FLAGS_QUERY_PENDING flag is set and both the
  events_to_process count is nonzero and the EC_FLAGS_QUERY_GUARDING
  flag is clear.

 "Complete": The event handling work has been completed, but it still
             cannot be submitted again.

  In ths state, the EC_FLAGS_QUERY_PENDING flag is set and the
  events_to_process count is zero or the EC_FLAGS_QUERY_GUARDING
  flag is set.

The state changes from "Ready" to "In progress" when new event is
detected by advance_transaction() and acpi_ec_submit_event() is
called by it.

Next, the state can change from "In progress" directly to "Ready" in
the following situations:

 * ec_event_clearing is ACPI_EC_EVT_TIMING_STATUS and the state of
   an ACPI_EC_COMMAND_QUERY transaction becomes ACPI_EC_COMMAND_POLL.

 * ec_event_clearing is ACPI_EC_EVT_TIMING_QUERY and the state of
   an ACPI_EC_COMMAND_QUERY transaction becomes
   ACPI_EC_COMMAND_COMPLETE.

 * ec_event_clearing is either ACPI_EC_EVT_TIMING_STATUS or
   ACPI_EC_EVT_TIMING_QUERY and there are no more events to
   process (ie. ec->events_to_process becomes 0).

If ec_event_clearing is ACPI_EC_EVT_TIMING_EVENT, however, the
state must change from "In progress" to "Complete" before it
can change to "Ready".  The changes from "In progress" to
"Complete" in that case occur in the following situations:

 * The state of an ACPI_EC_COMMAND_QUERY transaction becomes
   ACPI_EC_COMMAND_COMPLETE.

 * There are no more events to process (ie. ec->events_to_process
   becomes 0).

Finally, the state changes from "Complete" to "Ready" when
advance_transaction() is invoked when the state is "Complete" and
the state of the current transaction is not ACPI_EC_COMMAND_POLL.

To make this state machine visible in the code, add a new
event_state field to struct acpi_ec and modify the code to use
it istead the EC_FLAGS_QUERY_PENDING and EC_FLAGS_QUERY_GUARDING
flags.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/ec.c       |   75 ++++++++++++++++++++++++++++--------------------
 drivers/acpi/internal.h |    8 +++++
 2 files changed, 52 insertions(+), 31 deletions(-)

Index: linux-pm/drivers/acpi/ec.c
===================================================================
--- linux-pm.orig/drivers/acpi/ec.c
+++ linux-pm/drivers/acpi/ec.c
@@ -92,8 +92,6 @@ enum ec_command {
 
 enum {
 	EC_FLAGS_QUERY_ENABLED,		/* Query is enabled */
-	EC_FLAGS_QUERY_PENDING,		/* Query is pending */
-	EC_FLAGS_QUERY_GUARDING,	/* Guard for SCI_EVT check */
 	EC_FLAGS_EVENT_HANDLER_INSTALLED,	/* Event handler installed */
 	EC_FLAGS_EC_HANDLER_INSTALLED,	/* OpReg handler installed */
 	EC_FLAGS_QUERY_METHODS_INSTALLED, /* _Qxx handlers installed */
@@ -450,9 +448,11 @@ static bool acpi_ec_submit_event(struct
 	if (!acpi_ec_event_enabled(ec))
 		return false;
 
-	if (!test_and_set_bit(EC_FLAGS_QUERY_PENDING, &ec->flags)) {
+	if (ec->event_state == EC_EVENT_READY) {
 		ec_dbg_evt("Command(%s) submitted/blocked",
 			   acpi_ec_cmd_string(ACPI_EC_COMMAND_QUERY));
+
+		ec->event_state = EC_EVENT_IN_PROGRESS;
 		/*
 		 * If events_to_process is greqter than 0 at this point, the
 		 * while () loop in acpi_ec_event_handler() is still running
@@ -474,11 +474,19 @@ static bool acpi_ec_submit_event(struct
 	return true;
 }
 
+static void acpi_ec_complete_event(struct acpi_ec *ec)
+{
+	if (ec->event_state == EC_EVENT_IN_PROGRESS)
+		ec->event_state = EC_EVENT_COMPLETE;
+}
+
 static void acpi_ec_close_event(struct acpi_ec *ec)
 {
-	if (test_and_clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags))
+	if (ec->event_state != EC_EVENT_READY)
 		ec_dbg_evt("Command(%s) unblocked",
 			   acpi_ec_cmd_string(ACPI_EC_COMMAND_QUERY));
+
+	ec->event_state = EC_EVENT_READY;
 	acpi_ec_unmask_events(ec);
 }
 
@@ -565,8 +573,8 @@ void acpi_ec_flush_work(void)
 
 static bool acpi_ec_guard_event(struct acpi_ec *ec)
 {
-	bool guarded = true;
 	unsigned long flags;
+	bool guarded;
 
 	spin_lock_irqsave(&ec->lock, flags);
 	/*
@@ -575,19 +583,15 @@ static bool acpi_ec_guard_event(struct a
 	 * evaluating _Qxx, so we need to re-check SCI_EVT after waiting an
 	 * acceptable period.
 	 *
-	 * The guarding period begins when EC_FLAGS_QUERY_PENDING is
-	 * flagged, which means SCI_EVT check has just been performed.
-	 * But if the current transaction is ACPI_EC_COMMAND_QUERY, the
-	 * guarding should have already been performed (via
-	 * EC_FLAGS_QUERY_GUARDING) and should not be applied so that the
-	 * ACPI_EC_COMMAND_QUERY transaction can be transitioned into
-	 * ACPI_EC_COMMAND_POLL state immediately.
+	 * The guarding period is applicable if the event state is not
+	 * EC_EVENT_READY, but otherwise if the current transaction is of the
+	 * ACPI_EC_COMMAND_QUERY type, the guarding should have elapsed already
+	 * and it should not be applied to let the transaction transition into
+	 * the ACPI_EC_COMMAND_POLL state immediately.
 	 */
-	if (ec_event_clearing == ACPI_EC_EVT_TIMING_STATUS ||
-	    ec_event_clearing == ACPI_EC_EVT_TIMING_QUERY ||
-	    !test_bit(EC_FLAGS_QUERY_PENDING, &ec->flags) ||
-	    (ec->curr && ec->curr->command == ACPI_EC_COMMAND_QUERY))
-		guarded = false;
+	guarded = ec_event_clearing == ACPI_EC_EVT_TIMING_EVENT &&
+		ec->event_state != EC_EVENT_READY &&
+		(!ec->curr || ec->curr->command != ACPI_EC_COMMAND_QUERY);
 	spin_unlock_irqrestore(&ec->lock, flags);
 	return guarded;
 }
@@ -619,16 +623,26 @@ static int ec_transaction_completed(stru
 static inline void ec_transaction_transition(struct acpi_ec *ec, unsigned long flag)
 {
 	ec->curr->flags |= flag;
-	if (ec->curr->command == ACPI_EC_COMMAND_QUERY) {
-		if (ec_event_clearing == ACPI_EC_EVT_TIMING_STATUS &&
-		    flag == ACPI_EC_COMMAND_POLL)
+
+	if (ec->curr->command != ACPI_EC_COMMAND_QUERY)
+		return;
+
+	switch (ec_event_clearing) {
+	case ACPI_EC_EVT_TIMING_STATUS:
+		if (flag == ACPI_EC_COMMAND_POLL)
 			acpi_ec_close_event(ec);
-		if (ec_event_clearing == ACPI_EC_EVT_TIMING_QUERY &&
-		    flag == ACPI_EC_COMMAND_COMPLETE)
+
+		return;
+
+	case ACPI_EC_EVT_TIMING_QUERY:
+		if (flag == ACPI_EC_COMMAND_COMPLETE)
 			acpi_ec_close_event(ec);
-		if (ec_event_clearing == ACPI_EC_EVT_TIMING_EVENT &&
-		    flag == ACPI_EC_COMMAND_COMPLETE)
-			set_bit(EC_FLAGS_QUERY_GUARDING, &ec->flags);
+
+		return;
+
+	case ACPI_EC_EVT_TIMING_EVENT:
+		if (flag == ACPI_EC_COMMAND_COMPLETE)
+			acpi_ec_complete_event(ec);
 	}
 }
 
@@ -674,11 +688,9 @@ static bool advance_transaction(struct a
 	 */
 	if (!t || !(t->flags & ACPI_EC_COMMAND_POLL)) {
 		if (ec_event_clearing == ACPI_EC_EVT_TIMING_EVENT &&
-		    (!ec->events_to_process ||
-		     test_bit(EC_FLAGS_QUERY_GUARDING, &ec->flags))) {
-			clear_bit(EC_FLAGS_QUERY_GUARDING, &ec->flags);
+		    ec->event_state == EC_EVENT_COMPLETE)
 			acpi_ec_close_event(ec);
-		}
+
 		if (!t)
 			goto out;
 	}
@@ -1246,8 +1258,9 @@ static void acpi_ec_event_handler(struct
 	 * event handling work again regardless of whether or not the query
 	 * queued up above is processed successfully.
 	 */
-	if (ec_event_clearing == ACPI_EC_EVT_TIMING_STATUS ||
-	    ec_event_clearing == ACPI_EC_EVT_TIMING_QUERY)
+	if (ec_event_clearing == ACPI_EC_EVT_TIMING_EVENT)
+		acpi_ec_complete_event(ec);
+	else
 		acpi_ec_close_event(ec);
 
 	spin_unlock_irq(&ec->lock);
Index: linux-pm/drivers/acpi/internal.h
===================================================================
--- linux-pm.orig/drivers/acpi/internal.h
+++ linux-pm/drivers/acpi/internal.h
@@ -167,6 +167,13 @@ static inline void acpi_early_processor_
 /* --------------------------------------------------------------------------
                                   Embedded Controller
    -------------------------------------------------------------------------- */
+
+enum acpi_ec_event_state {
+	EC_EVENT_READY = 0,	/* Event work can be submitted */
+	EC_EVENT_IN_PROGRESS,	/* Event work is pending or being processed */
+	EC_EVENT_COMPLETE,	/* Event work processing has completed */
+};
+
 struct acpi_ec {
 	acpi_handle handle;
 	int gpe;
@@ -183,6 +190,7 @@ struct acpi_ec {
 	spinlock_t lock;
 	struct work_struct work;
 	unsigned long timestamp;
+	enum acpi_ec_event_state event_state;
 	unsigned int events_to_process;
 	unsigned int events_in_progress;
 	unsigned int queries_in_progress;

[PATCH 10/10] ACPI: EC: Relocate acpi_ec_create_query() and drop acpi_ec_delete_query()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Move acpi_ec_create_query() after acpi_ec_event_processor(), drop the
no longer needed forward declaration of the latter, and eliminate
acpi_ec_delete_query() which isn't really necessary.

No intentional functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/ec.c |   54 +++++++++++++++++++++++-------------------------------
 1 file changed, 23 insertions(+), 31 deletions(-)

Index: linux-pm/drivers/acpi/ec.c
===================================================================
--- linux-pm.orig/drivers/acpi/ec.c
+++ linux-pm/drivers/acpi/ec.c
@@ -170,7 +170,6 @@ struct acpi_ec_query {
 static int acpi_ec_submit_query(struct acpi_ec *ec);
 static bool advance_transaction(struct acpi_ec *ec, bool interrupt);
 static void acpi_ec_event_handler(struct work_struct *work);
-static void acpi_ec_event_processor(struct work_struct *work);
 
 struct acpi_ec *first_ec;
 EXPORT_SYMBOL(first_ec);
@@ -1134,33 +1133,6 @@ void acpi_ec_remove_query_handler(struct
 }
 EXPORT_SYMBOL_GPL(acpi_ec_remove_query_handler);
 
-static struct acpi_ec_query *acpi_ec_create_query(struct acpi_ec *ec, u8 *pval)
-{
-	struct acpi_ec_query *q;
-	struct transaction *t;
-
-	q = kzalloc(sizeof (struct acpi_ec_query), GFP_KERNEL);
-	if (!q)
-		return NULL;
-
-	INIT_WORK(&q->work, acpi_ec_event_processor);
-	t = &q->transaction;
-	t->command = ACPI_EC_COMMAND_QUERY;
-	t->rdata = pval;
-	t->rlen = 1;
-	q->ec = ec;
-	return q;
-}
-
-static void acpi_ec_delete_query(struct acpi_ec_query *q)
-{
-	if (q) {
-		if (q->handler)
-			acpi_ec_put_query_handler(q->handler);
-		kfree(q);
-	}
-}
-
 static void acpi_ec_event_processor(struct work_struct *work)
 {
 	struct acpi_ec_query *q = container_of(work, struct acpi_ec_query, work);
@@ -1180,7 +1152,26 @@ static void acpi_ec_event_processor(stru
 	ec->queries_in_progress--;
 	spin_unlock_irq(&ec->lock);
 
-	acpi_ec_delete_query(q);
+	acpi_ec_put_query_handler(handler);
+	kfree(q);
+}
+
+static struct acpi_ec_query *acpi_ec_create_query(struct acpi_ec *ec, u8 *pval)
+{
+	struct acpi_ec_query *q;
+	struct transaction *t;
+
+	q = kzalloc(sizeof (struct acpi_ec_query), GFP_KERNEL);
+	if (!q)
+		return NULL;
+
+	INIT_WORK(&q->work, acpi_ec_event_processor);
+	t = &q->transaction;
+	t->command = ACPI_EC_COMMAND_QUERY;
+	t->rdata = pval;
+	t->rlen = 1;
+	q->ec = ec;
+	return q;
 }
 
 static int acpi_ec_submit_query(struct acpi_ec *ec)
@@ -1229,9 +1220,10 @@ static int acpi_ec_submit_query(struct a
 
 	spin_unlock_irq(&ec->lock);
 
+	return 0;
+
 err_exit:
-	if (result)
-		acpi_ec_delete_query(q);
+	kfree(q);
 
 	return result;
 }

Re: [PATCH 00/10] ACPI: EC: Two fixes and cleanups

[Rafael J. Wysocki]

On Tue, Nov 23, 2021 at 7:49 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> Hi All,
>
> This series addresses two issues in the EC driver (patches [1-2/10] and cleans
> up the code in it on top of that.
>
> Please see the patch changelogs for details.

No negative feedback, so applied as 5.17 material.

Thanks!