"Rafael J. Wysocki" <rafael@xxxxxxxxxx> writes:
> On Mon, Jun 5, 2023 at 6:14 PM Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote:
>>
>> On Tue, May 16, 2023 at 2:02 AM Compostella, Jeremy
>> <jeremy.compostella@xxxxxxxxx> 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).
>From 42fa736fcd5d6a2e17c550f493a12e8df2e7cd72 Mon Sep 17 00:00:00 2001
From: Jeremy Compostella <jeremy.compostella@xxxxxxxxx>
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@xxxxxxxxx>
---
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
>> > Signed-off-by: Jeremy Compostella <jeremy.compostella@xxxxxxxxx>
>> > ---
>> > 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);
>> > --
