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

[Mario Limonciello <superm1@kernel.org>]

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@antheas.dev>
> ---
>   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?

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