ACPI: EC: Rework flushing of EC work while suspended to idle

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: f0ac20c3f6 ("ACPI: EC: Fix flushing of pending work")
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
This commit is contained in:
Rafael J. Wysocki 2021-11-23 19:36:51 +01:00
parent d58071a8a7
commit 4a9af6cac0
2 changed files with 45 additions and 14 deletions

View File

@ -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 acpi_ec *ec)
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 acpi_ec *ec)
#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 acpi_ec *ec, u8 query_bit)
}
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_create_query(u8 *pval)
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(struct work_struct *work)
{
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 *ec, u8 *data)
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 *ec, u8 *data)
}
/*
* 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 work_struct *work)
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;
}

View File

@ -183,6 +183,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;
};