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. > 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); > -- > 2.40.1 >
