[PATCH v2] ACPI: EC: Clear GPE on interrupt handling only

[PATCH v2] ACPI: EC: Clear GPE on interrupt handling only

[Compostella, Jeremy]

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.

Signed-off-by: Jeremy Compostella <jeremy.compostella@intel.com>
---
 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);
 }
-- 
2.40.1

Re: [PATCH v2] ACPI: EC: Clear GPE on interrupt handling only

[Rafael J. Wysocki]

On Tue, Jun 6, 2023 at 6:17 PM Compostella, Jeremy
<jeremy.compostella@intel.com> wrote:
>
> 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.
>
> Signed-off-by: Jeremy Compostella <jeremy.compostella@intel.com>
> ---
>  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);
>  }
> --

Applied as 6.5 material, thanks!