ACPI: EC: Clear GPE on interrupt handling only

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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 928899ab9502..42af09732238 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -674,7 +674,7 @@ static void advance_transaction(struct acpi_ec *ec, bool interrupt)
 	 * 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))
+	if (interrupt && ec->gpe >= 0 && acpi_ec_gpe_status_set(ec))
 		acpi_clear_gpe(NULL, ec->gpe);
 
 	status = acpi_ec_read_status(ec);
-- 
2.40.1

Re: ACPI: EC: Clear GPE on interrupt handling only

[Rafael J. Wysocki]

On Tue, May 16, 2023 at 2:02 AM 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.

And if I'm not mistaken, it is not necessary to run the entire
interrupt handler in that case, because the currently running
advance_transaction() will take care of the pending event anyway.

I agree that it is confusing to increase sci_not in that case, but I'm
not sure if running the entire advance_transaction() for the same
transaction twice in a row, once from ec_poll() and once from the
interrupt handler is entirely correct.

> Signed-off-by: Jeremy Compostella <jeremy.compostella@intel.com>
> ---
>  drivers/acpi/ec.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
> index 928899ab9502..42af09732238 100644
> --- a/drivers/acpi/ec.c
> +++ b/drivers/acpi/ec.c
> @@ -674,7 +674,7 @@ static void advance_transaction(struct acpi_ec *ec, bool interrupt)
>          * 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))
> +       if (interrupt && ec->gpe >= 0 && acpi_ec_gpe_status_set(ec))
>                 acpi_clear_gpe(NULL, ec->gpe);
>
>         status = acpi_ec_read_status(ec);
> --
> 2.40.1
>

Re: ACPI: EC: Clear GPE on interrupt handling only

[Rafael J. Wysocki]

On Mon, Jun 5, 2023 at 6:14 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Tue, May 16, 2023 at 2:02 AM 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.
>
> And if I'm not mistaken, it is not necessary to run the entire
> interrupt handler in that case, because the currently running
> advance_transaction() will take care of the pending event anyway.
>
> I agree that it is confusing to increase sci_not in that case, but I'm
> not sure if running the entire advance_transaction() for the same
> transaction twice in a row, once from ec_poll() and once from the
> interrupt handler is entirely correct.

However, if the interrupt handler wins the race, advance_transaction()
will run for the same transaction twice in a row anyway, so this
change will only make it happen more often.

So no objections, but I would move the GPE clearing piece directly
into acpi_ec_handle_interrupt(), because it will only be needed there
and it doesn't depend on anything else in advance_transaction().

> > Signed-off-by: Jeremy Compostella <jeremy.compostella@intel.com>
> > ---
> >  drivers/acpi/ec.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
> > index 928899ab9502..42af09732238 100644
> > --- a/drivers/acpi/ec.c
> > +++ b/drivers/acpi/ec.c
> > @@ -674,7 +674,7 @@ static void advance_transaction(struct acpi_ec *ec, bool interrupt)
> >          * 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))
> > +       if (interrupt && ec->gpe >= 0 && acpi_ec_gpe_status_set(ec))
> >                 acpi_clear_gpe(NULL, ec->gpe);
> >
> >         status = acpi_ec_read_status(ec);
> > --

Re: ACPI: EC: Clear GPE on interrupt handling only

[Compostella, Jeremy]

[-- Attachment #1: Type: text/plain, Size: 1752 bytes --]

"Rafael J. Wysocki" <rafael@kernel.org> writes:

> On Mon, Jun 5, 2023 at 6:14 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>>
>> On Tue, May 16, 2023 at 2:02 AM 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.
>>
>> And if I'm not mistaken, it is not necessary to run the entire
>> interrupt handler in that case, because the currently running
>> advance_transaction() will take care of the pending event anyway.
>>
>> I agree that it is confusing to increase sci_not in that case, but I'm
>> not sure if running the entire advance_transaction() for the same
>> transaction twice in a row, once from ec_poll() and once from the
>> interrupt handler is entirely correct.
>
> However, if the interrupt handler wins the race, advance_transaction()
> will run for the same transaction twice in a row anyway, so this
> change will only make it happen more often.
>
> So no objections, but I would move the GPE clearing piece directly
> into acpi_ec_handle_interrupt(), because it will only be needed there
> and it doesn't depend on anything else in advance_transaction().

I took into account your suggestion (cf. patch in attachment).


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-ACPI-EC-Clear-GPE-on-interrupt-handling-only.patch --]
[-- Type: text/x-patch, Size: 2547 bytes --]

From 42fa736fcd5d6a2e17c550f493a12e8df2e7cd72 Mon Sep 17 00:00:00 2001
From: Jeremy Compostella <jeremy.compostella@intel.com>
Date: Mon, 15 May 2023 16:49:19 -0700
Subject: [PATCH] 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.

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


[-- Attachment #3: Type: text/plain, Size: 821 bytes --]


>> > Signed-off-by: Jeremy Compostella <jeremy.compostella@intel.com>
>> > ---
>> >  drivers/acpi/ec.c | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
>> > index 928899ab9502..42af09732238 100644
>> > --- a/drivers/acpi/ec.c
>> > +++ b/drivers/acpi/ec.c
>> > @@ -674,7 +674,7 @@ static void advance_transaction(struct acpi_ec *ec, bool interrupt)
>> >          * 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))
>> > +       if (interrupt && ec->gpe >= 0 && acpi_ec_gpe_status_set(ec))
>> >                 acpi_clear_gpe(NULL, ec->gpe);
>> >
>> >         status = acpi_ec_read_status(ec);
>> > --

Re: ACPI: EC: Clear GPE on interrupt handling only

[Rafael J. Wysocki]

On Tue, Jun 6, 2023 at 12:27 AM Compostella, Jeremy
<jeremy.compostella@intel.com> wrote:
>
> "Rafael J. Wysocki" <rafael@kernel.org> writes:
>
> > On Mon, Jun 5, 2023 at 6:14 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >>
> >> On Tue, May 16, 2023 at 2:02 AM 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.
> >>
> >> And if I'm not mistaken, it is not necessary to run the entire
> >> interrupt handler in that case, because the currently running
> >> advance_transaction() will take care of the pending event anyway.
> >>
> >> I agree that it is confusing to increase sci_not in that case, but I'm
> >> not sure if running the entire advance_transaction() for the same
> >> transaction twice in a row, once from ec_poll() and once from the
> >> interrupt handler is entirely correct.
> >
> > However, if the interrupt handler wins the race, advance_transaction()
> > will run for the same transaction twice in a row anyway, so this
> > change will only make it happen more often.
> >
> > So no objections, but I would move the GPE clearing piece directly
> > into acpi_ec_handle_interrupt(), because it will only be needed there
> > and it doesn't depend on anything else in advance_transaction().
>
> I took into account your suggestion (cf. patch in attachment).

Yes, this is what I meant.

I think that you can resend it as a v2 with the same changelog.

>
> >> > Signed-off-by: Jeremy Compostella <jeremy.compostella@intel.com>
> >> > ---
> >> >  drivers/acpi/ec.c | 2 +-
> >> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >> >
> >> > diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
> >> > index 928899ab9502..42af09732238 100644
> >> > --- a/drivers/acpi/ec.c
> >> > +++ b/drivers/acpi/ec.c
> >> > @@ -674,7 +674,7 @@ static void advance_transaction(struct acpi_ec *ec, bool interrupt)
> >> >          * 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))
> >> > +       if (interrupt && ec->gpe >= 0 && acpi_ec_gpe_status_set(ec))
> >> >                 acpi_clear_gpe(NULL, ec->gpe);
> >> >
> >> >         status = acpi_ec_read_status(ec);
> >> > --