Re: [PATCH v3] ACPI: s2idle: Handle shared _Ixx GPEs without an EC

[Rafael J. Wysocki (Intel)]

On Fri, Nov 6, 2026 at 1:59 PM 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.
>
> Make acpi_ec_dispatch_gpe() return an int and return -ENODEV when no
> EC instance is available so that acpi_s2idle_wake() can handle that
> case: on x86, treat the first GPE armed for wake via an _Ixx method
> like the EC GPE, mirroring acpi_ec_dispatch_gpe(). That is, report a
> wakeup if the status bit is set for any enabled GPE other than the
> _Ixx one. Otherwise, cancel the SCI-related wakeup and dispatch the
> _Ixx GPE if its status bit is set, so the events behind it can be
> processed. Drivers handling genuine wake events are expected to
> report them via pm_wakeup_event(), which the common s2idle path will
> catch after synchronizing event processing.
>
> If no _Ixx GPEs have been armed, and in the non-x86 fallback, the
> previous any-GPE status check is retained. If GPEs are armed via more
> than one _Ixx method, the first one is used as the wake indicator and
> the remaining ones are treated as regular wake sources.
>
> Fixes: 46767b17fda3 ("ACPI: PM: s2idle: Add _Ixx GPE wake indicator support")
> Signed-off-by: Saranya Gopal <saranya.gopal@xxxxxxxxx>
> ---
> Changes from v2:
>  - Restructured ret checks in sleep.c as per Rafael's suggestion to
>    cover !CONFIG_ACPI_EC case that returns -EOPNOTSUPP (Sashiko report)
>  - Drop the open-coded scan over acpi_current_gpe_count that conflated
>    sequential GPE indices with raw GPE numbers (Sashiko report)
>  - Mirror acpi_ec_dispatch_gpe() with the first GPE armed via an _Ixx
>    method playing the role of the EC GPE, so the existing single-skip
>    acpi_any_gpe_status_set() is sufficient and no ACPICA changes are
>    needed.
>  - Follow acpi_ec_dispatch_gpe() for the status check and dispatch of
>    the _Ixx GPE as well, which also drops the error paths that
>    requested a system wakeup in v2 (questioned by Rafael).
>  - Add a comment describing acpi_x86_s2idle_check_ixx_gpes(), including
>    the role of pm_system_cancel_wakeup().
>  - Preserve the previous any-GPE behavior when no _Ixx GPEs have been
>    armed for wakeup.
> Changes from v1:
>  - -ENOTSUP replaced with the correct -EOPNOTSUPP for !CONFIG_PM_SLEEP
>    case
>
>  drivers/acpi/ec.c         | 10 +++----
>  drivers/acpi/internal.h   |  6 ++--
>  drivers/acpi/sleep.c      | 11 +++++--
>  drivers/acpi/sleep.h      |  8 +++++
>  drivers/acpi/x86/s2idle.c | 61 +++++++++++++++++++++++++++++++++++++++
>  5 files changed, 86 insertions(+), 10 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..09bd661187b6 100644
> --- a/drivers/acpi/sleep.c
> +++ b/drivers/acpi/sleep.c
> @@ -761,6 +761,8 @@ bool acpi_s2idle_wake(void)
>                 return pm_wakeup_pending();
>
>         while (pm_wakeup_pending()) {
> +               int ret;
> +
>                 /*
>                  * If IRQD_WAKEUP_ARMED is set for the SCI at this point, the
>                  * SCI has not triggered while suspended, so bail out (the
> @@ -789,11 +791,16 @@ bool acpi_s2idle_wake(void)
>
>                 /*
>                  * Check non-EC GPE wakeups and if there are none, cancel the
> -                * SCI-related wakeup and dispatch the EC GPE.
> +                * SCI-related wakeup and dispatch the EC GPE.  If there is no
> +                * EC to consult, check the GPEs armed for wakeup via _Ixx
> +                * methods instead.
>                  */
> -               if (acpi_ec_dispatch_gpe()) {
> +               ret = acpi_ec_dispatch_gpe();
> +               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;
>                 }
>
>                 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..b79bf0fa9a7f 100644
> --- a/drivers/acpi/x86/s2idle.c
> +++ b/drivers/acpi/x86/s2idle.c
> @@ -24,6 +24,13 @@
>
>  #ifdef CONFIG_SUSPEND
>
> +/*
> + * First GPE armed for wake via an _Ixx method, U32_MAX if none.  On
> + * platforms without an ACPI-enumerated EC, this GPE plays the role of
> + * the EC GPE in acpi_ec_dispatch_gpe().
> + */
> +static u32 acpi_s2idle_ixx_gpe = U32_MAX;
> +
>  static void acpi_setup_ixx_gpes(void)
>  {
>         acpi_handle gpe_root;
> @@ -47,11 +54,65 @@ static void acpi_setup_ixx_gpes(void)
>
>                 acpi_mark_gpe_for_wake(NULL, i);
>                 acpi_set_gpe_wake_mask(NULL, i, ACPI_GPE_ENABLE);
> +               if (acpi_s2idle_ixx_gpe == U32_MAX)
> +                       acpi_s2idle_ixx_gpe = i;
>
>                 pm_pr_dbg("ACPI: GPE0x%02x armed for wake via %s\n", i, gpe_nr_str);
>         }
>  }
>
> +/*
> + * acpi_x86_s2idle_check_ixx_gpes - Handle an SCI wakeup without an EC
> + *
> + * Called to decide whether or not to wake up the system when an SCI has
> + * triggered during suspend-to-idle on a platform without an EC instance,
> + * mirroring acpi_ec_dispatch_gpe() with the first GPE armed for wake via
> + * an _Ixx method playing the role of the EC GPE.
> + *
> + * Report a wakeup if the status bit is set for any enabled GPE other than
> + * the _Ixx one.  If no _Ixx GPE has been armed, acpi_s2idle_ixx_gpe is
> + * U32_MAX and this amounts to checking all of the GPEs, which is the
> + * previous behavior.
> + *
> + * Otherwise, cancel the pending SCI-related wakeup, so it does not cause
> + * the pm_wakeup_pending() check in the caller to succeed by itself, and
> + * dispatch the _Ixx GPE if it is active, so the events behind it can be
> + * processed.  If any of those events turn out to be genuine wake events,
> + * the drivers handling them are expected to call pm_wakeup_event() and
> + * the caller will catch that via pm_wakeup_pending() after synchronizing
> + * the event processing.
> + */
> +bool acpi_x86_s2idle_check_ixx_gpes(void)
> +{
> +       acpi_event_status status = 0;
> +
> +       /*
> +        * Report wakeup if the status bit is set for any enabled GPE other
> +        * than the _Ixx one.
> +        */
> +       if (acpi_any_gpe_status_set(acpi_s2idle_ixx_gpe))
> +               return true;
> +
> +       if (acpi_s2idle_ixx_gpe == U32_MAX)
> +               return false;
> +
> +       /*
> +        * Cancel the SCI wakeup and dispatch the _Ixx GPE in case there are
> +        * any wakeup events behind it.
> +        */
> +       pm_system_cancel_wakeup();
> +
> +       acpi_get_gpe_status(NULL, acpi_s2idle_ixx_gpe, &status);
> +       if (status & ACPI_EVENT_FLAG_STATUS_SET) {
> +               pm_pr_dbg("ACPI _Ixx GPE 0x%02x dispatched\n",
> +                         acpi_s2idle_ixx_gpe);
> +
> +               acpi_dispatch_gpe(NULL, acpi_s2idle_ixx_gpe);
> +       }
> +
> +       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");
> --

Applied as 7.4-rc material, thanks!