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

[Rafael J. Wysocki (Intel)]

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.

> ---
>  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;
}

>                 }
>
>                 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.

> +       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?

> +
> +               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.

> +
> +       pm_system_cancel_wakeup();

What is the role of this call?

> +
> +       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");
> --