RE: [PATCH v2] ACPI: s2idle: Handle shared _Ixx GPEs without an EC

[Gopal, Saranya]

Hi Rafael,

Thanks for reviewing this.
I will be sending out v3 addressing all the comments shortly.

> -----Original Message-----
> From: Rafael J. Wysocki (Intel) <rafael@xxxxxxxxxx>
> Sent: Thursday, November 5, 2026 12:28 AM
> To: Gopal, Saranya <saranya.gopal@xxxxxxxxx>
> Cc: linux-acpi@xxxxxxxxxxxxxxx; linux-pm@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; Wysocki, Rafael J <rafael.j.wysocki@xxxxxxxxx>
> Subject: Re: [PATCH v2] ACPI: s2idle: Handle shared _Ixx GPEs without an EC
> 
> On Mon, Nov 2, 2026 at 7:37 AM Saranya Gopal <saranya.gopal@xxxxxxxxx>
> wrote:
> >
> > When first_ec is NULL, acpi_ec_dispatch_gpe() previously treated any
> > active GPE as a wake source. On platforms where an _Ixx GPE is shared
> > by user-wakeup and background events, that can cause unnecessary
> > wakeups.
> >
> > Return -ENODEV when no EC instance is available so the s2idle wake
> > path can handle that case. On x86, check active GPEs against those
> > registered from _Ixx methods. Preserve immediate wakeup for active
> > non-_Ixx GPEs; otherwise cancel the SCI wake request and dispatch
> > active _Ixx GPEs. The common s2idle path then synchronizes event
> > processing and checks whether a driver requested wakeup. Lookup or
> > dispatch failures conservatively request wakeup.
> >
> > The non-x86 fallback retains the previous any-GPE status check.
> >
> > Fixes: 46767b17fda3 ("ACPI: PM: s2idle: Add _Ixx GPE wake indicator
> support")
> > Signed-off-by: Saranya Gopal <saranya.gopal@xxxxxxxxx>
> 
> First, please see Sashiko's feedback on this:
> 
> https://sashiko.dev/#/patchset/20261102063610.3242714-1-
> saranya.gopal%40intel.com
> 
> and let me know what you think about it.
> 
Sashiko's feedback seems valid. I will address both of its feedback in v3.

> > ---
> >  drivers/acpi/ec.c         | 10 ++++-----
> >  drivers/acpi/internal.h   |  6 +++---
> >  drivers/acpi/sleep.c      | 18 +++++++++-------
> >  drivers/acpi/sleep.h      |  8 ++++++++
> >  drivers/acpi/x86/s2idle.c | 43
> +++++++++++++++++++++++++++++++++++++++
> >  5 files changed, 70 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
> > index 64ad4cfa6208..8be572af4115 100644
> > --- a/drivers/acpi/ec.c
> > +++ b/drivers/acpi/ec.c
> > @@ -2155,19 +2155,19 @@ static bool acpi_ec_work_in_progress(struct
> acpi_ec *ec)
> >         return ec->events_in_progress + ec->queries_in_progress > 0;
> >  }
> >
> > -bool acpi_ec_dispatch_gpe(void)
> > +int acpi_ec_dispatch_gpe(void)
> >  {
> >         bool work_in_progress = false;
> >
> >         if (!first_ec)
> > -               return acpi_any_gpe_status_set(U32_MAX);
> > +               return -ENODEV;
> >
> >         /*
> >          * Report wakeup if the status bit is set for any enabled GPE other
> >          * than the EC one.
> >          */
> >         if (acpi_any_gpe_status_set(first_ec->gpe))
> > -               return true;
> > +               return 1;
> >
> >         /*
> >          * Cancel the SCI wakeup and process all pending events in case there
> > @@ -2195,7 +2195,7 @@ bool acpi_ec_dispatch_gpe(void)
> >         spin_unlock_irq(&first_ec->lock);
> >
> >         if (!work_in_progress)
> > -               return false;
> > +               return 0;
> >
> >         pm_pr_dbg("ACPI EC GPE dispatched\n");
> >
> > @@ -2212,7 +2212,7 @@ bool acpi_ec_dispatch_gpe(void)
> >                 spin_unlock_irq(&first_ec->lock);
> >         } while (work_in_progress && !pm_wakeup_pending());
> >
> > -       return false;
> > +       return 0;
> >  }
> >  #endif /* CONFIG_PM_SLEEP */
> >
> > diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
> > index 40f875b265a9..d5435338d6ca 100644
> > --- a/drivers/acpi/internal.h
> > +++ b/drivers/acpi/internal.h
> > @@ -236,7 +236,7 @@ void acpi_ec_register_opregions(struct acpi_device
> *adev);
> >
> >  #ifdef CONFIG_PM_SLEEP
> >  void acpi_ec_flush_work(void);
> > -bool acpi_ec_dispatch_gpe(void);
> > +int acpi_ec_dispatch_gpe(void);
> >  #endif
> >
> >  #else
> > @@ -256,9 +256,9 @@ static inline void
> acpi_ec_remove_query_handler(struct acpi_ec *ec, u8 query_bit
> >  static inline void acpi_ec_register_opregions(struct acpi_device *adev) {}
> >
> >  static inline void acpi_ec_flush_work(void) {}
> > -static inline bool acpi_ec_dispatch_gpe(void)
> > +static inline int acpi_ec_dispatch_gpe(void)
> >  {
> > -       return false;
> > +       return -EOPNOTSUPP;
> >  }
> >
> >  #endif
> > diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
> > index 132a9df98471..9366c3d927c2 100644
> > --- a/drivers/acpi/sleep.c
> > +++ b/drivers/acpi/sleep.c
> > @@ -787,13 +787,17 @@ bool acpi_s2idle_wake(void)
> >                         return true;
> >                 }
> >
> > -               /*
> > -                * Check non-EC GPE wakeups and if there are none, cancel the
> > -                * SCI-related wakeup and dispatch the EC GPE.
> > -                */
> > -               if (acpi_ec_dispatch_gpe()) {
> > -                       pm_pr_dbg("ACPI non-EC GPE wakeup\n");
> > -                       return true;
> > +               /* Check EC handling and other GPE wake sources. */
> > +               {
> > +                       int ret = acpi_ec_dispatch_gpe();
> > +
> > +                       if (ret == -ENODEV) {
> > +                               if (acpi_x86_s2idle_check_ixx_gpes())
> > +                                       return true;
> > +                       } else if (ret > 0) {
> > +                               pm_pr_dbg("ACPI non-EC GPE wakeup\n");
> > +                               return true;
> > +                       }
> 
> I would rewrite the above as
> 
> if (ret > 0) {
>     pm_pr_dbg("ACPI non-EC GPE wakeup\n");
>     return true;
> } else if (ret < 0 && acpi_x86_s2idle_check_ixx_gpes() {
>     return true;
> }
>
Yes, making this change in v3 as this will address Sashiko's review comment too.
 
> >                 }
> >
> >                 acpi_os_wait_events_complete();
> > diff --git a/drivers/acpi/sleep.h b/drivers/acpi/sleep.h
> > index 9c3cb109c5d2..955e9c22bf0d 100644
> > --- a/drivers/acpi/sleep.h
> > +++ b/drivers/acpi/sleep.h
> > @@ -20,6 +20,14 @@ extern int acpi_s2idle_prepare(void);
> >  extern bool acpi_s2idle_wake(void);
> >  extern void acpi_s2idle_restore(void);
> >  extern void acpi_s2idle_end(void);
> > +#if defined(CONFIG_X86) && defined(CONFIG_SUSPEND)
> > +extern bool acpi_x86_s2idle_check_ixx_gpes(void);
> > +#else
> > +static inline bool acpi_x86_s2idle_check_ixx_gpes(void)
> > +{
> > +       return acpi_any_gpe_status_set(U32_MAX);
> > +}
> > +#endif
> >
> >  extern void acpi_s2idle_setup(void);
> >
> > diff --git a/drivers/acpi/x86/s2idle.c b/drivers/acpi/x86/s2idle.c
> > index c278ea14c0b3..979684950f24 100644
> > --- a/drivers/acpi/x86/s2idle.c
> > +++ b/drivers/acpi/x86/s2idle.c
> > @@ -16,6 +16,7 @@
> >   */
> >
> >  #include <linux/acpi.h>
> > +#include <linux/bitmap.h>
> >  #include <linux/device.h>
> >  #include <linux/dmi.h>
> >  #include <linux/suspend.h>
> > @@ -24,6 +25,8 @@
> >
> >  #ifdef CONFIG_SUSPEND
> >
> > +static DECLARE_BITMAP(acpi_s2idle_ixx_gpes, 256);
> > +
> >  static void acpi_setup_ixx_gpes(void)
> >  {
> >         acpi_handle gpe_root;
> > @@ -47,11 +50,51 @@ static void acpi_setup_ixx_gpes(void)
> >
> >                 acpi_mark_gpe_for_wake(NULL, i);
> >                 acpi_set_gpe_wake_mask(NULL, i, ACPI_GPE_ENABLE);
> > +               set_bit(i, acpi_s2idle_ixx_gpes);
> >
> >                 pm_pr_dbg("ACPI: GPE0x%02x armed for wake via %s\n", i,
> gpe_nr_str);
> >         }
> >  }
> >
> > +bool acpi_x86_s2idle_check_ixx_gpes(void)
> > +{
> > +       acpi_handle gpe_device;
> > +       acpi_event_status status;
> > +       u32 gpe;
> > +
> 
> It would be good to add a comment describing what happens here.

Yes, let me add a comment in v3.
> 
> > +       for (gpe = 0; gpe < acpi_current_gpe_count; gpe++) {
> > +               if (ACPI_FAILURE(acpi_get_gpe_device(gpe, &gpe_device)))
> > +                       return true;
> 
> Why does it return true on errors (here and below)?
> 
> It will cause a system wakeup to happen on errors occurring here, so
> what's the rationale?
I was thinking that we should wake on failures.
But, understood that it will deviate from existing behavior.
I will take ec.c behavior as example and mirror the flow in v3.

> 
> > +
> > +               if (!gpe_device && gpe < 256 &&
> > +                   test_bit(gpe, acpi_s2idle_ixx_gpes))
> > +                       continue;
> > +
> > +               if (ACPI_FAILURE(acpi_get_gpe_status(gpe_device, gpe, &status)))
> > +                       return true;
> > +
> > +               if ((status & ACPI_EVENT_FLAG_STATUS_SET) &&
> > +                   (status & ACPI_EVENT_FLAG_ENABLE_SET))
> > +                       return true;
> > +       }
> 
> You'd need a variant of acpi_any_gpe_status_set() to do the above
> properly AFAICS.
Agreed. I will make this change in v3.

> 
> > +
> > +       pm_system_cancel_wakeup();
> 
> What is the role of this call?
The same as in acpi_ec_dispatch_gpe(): it cancels the pending wakeup
triggered by the SCI interrupt. Without it, the pm_wakeup_pending()
check in acpi_s2idle_wake() would always report a wakeup even when the
only active GPE is the _Ixx one signaling background events. After the
cancellation, the _Ixx GPE is dispatched and drivers handling genuine
wake events report them via pm_wakeup_event(), which acpi_s2idle_wake()
then catches through pm_wakeup_pending() after
acpi_os_wait_events_complete(). This will be explained in the comment
added above the function in v3.

Thanks,
Saranya
> 
> > +
> > +       for_each_set_bit(gpe, acpi_s2idle_ixx_gpes, 256) {
> > +               if (ACPI_FAILURE(acpi_get_gpe_status(NULL, gpe, &status)))
> > +                       return true;
> > +               if (!(status & ACPI_EVENT_FLAG_STATUS_SET) ||
> > +                   !(status & ACPI_EVENT_FLAG_ENABLE_SET))
> > +                       continue;
> > +
> > +               pm_pr_dbg("ACPI _Ixx GPE 0x%02x status set\n", gpe);
> > +               if (acpi_dispatch_gpe(NULL, gpe) != ACPI_INTERRUPT_HANDLED)
> > +                       return true;
> > +       }
> > +
> > +       return false;
> > +}
> > +
> >  static bool sleep_no_lps0 __read_mostly;
> >  module_param(sleep_no_lps0, bool, 0644);
> >  MODULE_PARM_DESC(sleep_no_lps0, "Do not use the special LPS0 device
> interface");
> > --