Re: [RFC v2 03/10] acpi/x86: s2idle: Add support for adding a delay after begin MS calls

[Antheas Kapenekakis]

On Tue, 28 Apr 2026 at 03:57, Mario Limonciello <superm1@xxxxxxxxxx> wrote:
>
> On 4/25/26 16:57, Antheas Kapenekakis wrote:
> > Certain platform/USB devices interact with Modern Standby firmware
> > notifications. This is particularly true with Asus, where the keyboards
> > are wired up to turn off their backlight during the Display Off
> > notification using a predetermined delay. While for Asus Keyboards this
> > does not cause an issue, it does manifest in ROG Ally devices, where the
> > controller waits for the animation to complete before saving its state.
> >
> > In Windows, this is not a problem, because there is an ample delay after
> > these calls and before LPS0, typically seconds to minutes.
> >
> > Therefore, introduce a delay quirk after these calls, to ensure affected
> > devices have time to uninitialize, and attach it to acpi_s2idle_dev_ops
> > so it can be consumed by device drivers.
> >
> > Signed-off-by: Antheas Kapenekakis <lkml@xxxxxxxxxxx>
> > ---
> >   drivers/acpi/x86/s2idle.c | 11 +++++++++++
> >   include/linux/acpi.h      |  1 +
> >   2 files changed, 12 insertions(+)
> >
> > diff --git a/drivers/acpi/x86/s2idle.c b/drivers/acpi/x86/s2idle.c
> > index f5aefba8b191..8b48f999e0e9 100644
> > --- a/drivers/acpi/x86/s2idle.c
> > +++ b/drivers/acpi/x86/s2idle.c
> > @@ -16,6 +16,7 @@
> >    */
> >
> >   #include <linux/acpi.h>
> > +#include <linux/delay.h>
> >   #include <linux/device.h>
> >   #include <linux/dmi.h>
> >   #include <linux/suspend.h>
> > @@ -517,6 +518,9 @@ static struct acpi_scan_handler lps0_handler = {
> >
> >   static int acpi_s2idle_begin_lps0(void)
> >   {
> > +     struct acpi_s2idle_dev_ops *handler;
> > +     int delay = 0;
> > +
> >       if (!lps0_device_handle || sleep_no_lps0)
> >               return acpi_s2idle_begin();
> >
> > @@ -552,6 +556,13 @@ static int acpi_s2idle_begin_lps0(void)
> >                                       lps0_dsm_func_mask_microsoft,
> >                                       lps0_dsm_guid_microsoft);
> >
> > +     list_for_each_entry(handler, &lps0_s2idle_devops_head, list_node) {
> > +             if (handler->begin_delay && handler->begin_delay > delay)
> > +                     delay = handler->begin_delay;
> > +     }
> > +     if (delay > 0)
> > +             msleep(delay);
>
> Is this the correct location?  You wouldn't want it at the check callback?

This delay is meant to be added between Display Off/Sleep Entry and
suspend to allow devices that need it to settle before they are put on
D3, so it needs to go here. check() happens after suspend (they are
already on D3). So e.g. check() cannot replace it. We would need
something like a begin_check()

I will reply to the other patch with more context.

> > +
> >       return acpi_s2idle_begin();
> >   }
> >
> > diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> > index 4d2f0bed7a06..a416e5c5798a 100644
> > --- a/include/linux/acpi.h
> > +++ b/include/linux/acpi.h
> > @@ -1154,6 +1154,7 @@ struct acpi_s2idle_dev_ops {
> >       void (*prepare)(void);
> >       void (*check)(void);
> >       void (*restore)(void);
> > +     int begin_delay;
> >   };
> >   #if defined(CONFIG_SUSPEND) && defined(CONFIG_X86)
> >   int acpi_register_lps0_dev(struct acpi_s2idle_dev_ops *arg);
>
>