From 896e97bf99ecf0ecb6cc420bc2c9eb268d3edc05 Mon Sep 17 00:00:00 2001 From: "Compostella, Jeremy" Date: Tue, 6 Jun 2023 09:13:23 -0700 Subject: ACPI: EC: Clear GPE on interrupt handling only On multiple devices I work on, we noticed that /sys/firmware/acpi/interrupts/sci_not is non-zero and keeps increasing over time. It turns out that there is a race condition between servicing a GPE interrupt and handling task driven transactions. If a GPE interrupt is received at the same time ec_poll() is running, the advance_transaction() clears the GPE flag and the interrupt is not serviced as acpi_ev_detect_gpe() relies on the GPE flag to call the handler. As a result, `sci_not' is increased. To address this, move the GPE status check and clearing from advance_transaction() directly into acpi_ec_handle_interrupt(), so the EC GPE only gets cleared in the interrupt handling path. Signed-off-by: Jeremy Compostella [ rjw: Changelog edits ] Signed-off-by: Rafael J. Wysocki --- drivers/acpi/ec.c | 31 ++++++++++++++++--------------- 1 file changed, 16 insertions(+), 15 deletions(-) diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c index 928899ab9502..8569f55e55b6 100644 --- a/drivers/acpi/ec.c +++ b/drivers/acpi/ec.c @@ -662,21 +662,6 @@ static void advance_transaction(struct acpi_ec *ec, bool interrupt) ec_dbg_stm("%s (%d)", interrupt ? "IRQ" : "TASK", smp_processor_id()); - /* - * Clear GPE_STS upfront to allow subsequent hardware GPE_STS 0->1 - * changes to always trigger a GPE interrupt. - * - * GPE STS is a W1C register, which means: - * - * 1. Software can clear it without worrying about clearing the other - * GPEs' STS bits when the hardware sets them in parallel. - * - * 2. As long as software can ensure only clearing it when it is set, - * hardware won't set it in parallel. - */ - if (ec->gpe >= 0 && acpi_ec_gpe_status_set(ec)) - acpi_clear_gpe(NULL, ec->gpe); - status = acpi_ec_read_status(ec); /* @@ -1287,6 +1272,22 @@ static void acpi_ec_handle_interrupt(struct acpi_ec *ec) unsigned long flags; spin_lock_irqsave(&ec->lock, flags); + + /* + * Clear GPE_STS upfront to allow subsequent hardware GPE_STS 0->1 + * changes to always trigger a GPE interrupt. + * + * GPE STS is a W1C register, which means: + * + * 1. Software can clear it without worrying about clearing the other + * GPEs' STS bits when the hardware sets them in parallel. + * + * 2. As long as software can ensure only clearing it when it is set, + * hardware won't set it in parallel. + */ + if (ec->gpe >= 0 && acpi_ec_gpe_status_set(ec)) + acpi_clear_gpe(NULL, ec->gpe); + advance_transaction(ec, true); spin_unlock_irqrestore(&ec->lock, flags); } -- cgit v1.2.3