Commit 896e97bf authored by Compostella, Jeremy's avatar Compostella, Jeremy Committed by Rafael J. Wysocki

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: default avatarJeremy Compostella <jeremy.compostella@intel.com>
[ rjw: Changelog edits ]
Signed-off-by: default avatarRafael J. Wysocki <rafael.j.wysocki@intel.com>
parent 858fd168
...@@ -662,21 +662,6 @@ static void advance_transaction(struct acpi_ec *ec, bool interrupt) ...@@ -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()); 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); status = acpi_ec_read_status(ec);
/* /*
...@@ -1287,6 +1272,22 @@ static void acpi_ec_handle_interrupt(struct acpi_ec *ec) ...@@ -1287,6 +1272,22 @@ static void acpi_ec_handle_interrupt(struct acpi_ec *ec)
unsigned long flags; unsigned long flags;
spin_lock_irqsave(&ec->lock, 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); advance_transaction(ec, true);
spin_unlock_irqrestore(&ec->lock, flags); spin_unlock_irqrestore(&ec->lock, flags);
} }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment