| Message ID | 87353x87p7.fsf@jcompost-mobl.amr.corp.intel.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | ACPI: EC: Clear GPE on interrupt handling only | expand |
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 >
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); > > --
"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). >> > 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); >> > --
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); > >> > --
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);
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(-)