Re: [RFC 11/13] acpi/x86: s2idle: add quirk table for modern standby delays

[Mario Limonciello <mario.limonciello@amd.com>]

On 11/21/2024 11:22, Antheas Kapenekakis wrote:
> Unfortunately, some modern standby systems, including the ROG Ally, rely
> on a delay between modern standby transitions. Add a quirk table for
> introducing delays between modern standby transitions, and quirk the
> ROG Ally on "Display Off", which needs a bit of time to turn off its
> controllers prior to suspending.
> 
> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
> ---
>   drivers/acpi/x86/s2idle.c | 56 +++++++++++++++++++++++++++++++++++++++
>   1 file changed, 56 insertions(+)
> 
> diff --git a/drivers/acpi/x86/s2idle.c b/drivers/acpi/x86/s2idle.c
> index d389c57d2963..504e6575d7ad 100644
> --- a/drivers/acpi/x86/s2idle.c
> +++ b/drivers/acpi/x86/s2idle.c
> @@ -18,6 +18,7 @@
>   #include <linux/acpi.h>
>   #include <linux/device.h>
>   #include <linux/dmi.h>
> +#include <linux/delay.h>
>   #include <linux/suspend.h>
>   
>   #include "../sleep.h"
> @@ -91,11 +92,50 @@ struct lpi_device_constraint_amd {
>   	int min_dstate;
>   };
>   
> +struct s2idle_delay_quirks {
> +	int delay_display_off;
> +	int delay_sleep_entry;
> +	int delay_sleep_exit;
> +	int delay_display_on;
> +};

Historically these "kinds" of quirks are kept in drivers/acpi/x86/utils.c.

Could it be moved there?  Or perhaps stored in the ASUS drivers and 
callbacks?

This feels cleaner if you used "struct acpi_s2idle_dev_ops" and 
callbacks.  More below.

> +
> +/*
> + * The ROG Ally series disconnects its controllers on Display Off and performs
> + * a fancy shutdown sequence, which requires around half a second to complete.
> + * If the power is cut earlier by entering it into D3, the original Ally unit
> + * might not disconnect its XInput MCU, causing excess battery drain, and the
> + * Ally X will make the controller restart post-suspend. In addition, the EC
> + * of the device rarely (1/20 attempts) may get stuck asserting PROCHOT after
> + * suspend (for various reasons), so split the delay between Display Off and
> + * Sleep Entry.
> + */
> +static const struct s2idle_delay_quirks rog_ally_quirks = {
> +	.delay_display_off = 350,
> +	.delay_sleep_entry = 150,
> +};

Is this delay still needed with Ally MCU 319 that has the fixes from ASUS?

I'm suspecting not, which means this quirk should be made more narrow IMO.

In the various ASUS drivers you can lookup the MCU firmware version. 
Those drivers can do acpi_register_lps0_dev() when the older firmware is 
present and use the callbacks.  If the newer firmware is there less code 
to worry about.

This also would mean less static quirk tables in the kernel tree.

> +
> +static const struct dmi_system_id s2idle_delay_quirks[] = {
> +	{
> +		.matches = {
> +			DMI_MATCH(DMI_BOARD_NAME, "RC71L"),
> +		},
> +		.driver_data = (void *)&rog_ally_quirks
> +	},
> +	{
> +		.matches = {
> +			DMI_MATCH(DMI_BOARD_NAME, "RC72L"),
> +		},
> +		.driver_data = (void *)&rog_ally_quirks
> +	},
> +	{}
> +};
> +
>   static LIST_HEAD(lps0_s2idle_devops_head);
>   
>   static struct lpi_constraints *lpi_constraints_table;
>   static int lpi_constraints_table_size;
>   static int rev_id;
> +struct s2idle_delay_quirks *delay_quirks;
>   
>   #define for_each_lpi_constraint(entry)						\
>   	for (int i = 0;								\
> @@ -566,6 +606,9 @@ static int acpi_s2idle_display_off(void)
>   		acpi_sleep_run_lps0_dsm(ACPI_LPS0_DISPLAY_OFF,
>   				lps0_dsm_func_mask_microsoft, lps0_dsm_guid_microsoft);
>   
> +	if (delay_quirks && delay_quirks->delay_display_off)
> +		msleep(delay_quirks->delay_display_off);
> +
>   	acpi_scan_lock_release();
>   
>   	return 0;
> @@ -587,6 +630,9 @@ static int acpi_s2idle_sleep_entry(void)
>   		acpi_sleep_run_lps0_dsm(ACPI_LPS0_SLEEP_ENTRY,
>   				lps0_dsm_func_mask_microsoft, lps0_dsm_guid_microsoft);
>   
> +	if (delay_quirks && delay_quirks->delay_sleep_entry)
> +		msleep(delay_quirks->delay_sleep_entry);
> +
>   	acpi_scan_lock_release();
>   
>   	return 0;
> @@ -627,6 +673,9 @@ static int acpi_s2idle_sleep_exit(void)
>   	acpi_scan_lock_acquire();
>   
>   	/* Modern Standby Sleep Exit */
> +	if (delay_quirks && delay_quirks->delay_sleep_exit)
> +		msleep(delay_quirks->delay_sleep_exit);
> +
>   	if (lps0_dsm_func_mask_microsoft > 0)
>   		acpi_sleep_run_lps0_dsm(ACPI_LPS0_SLEEP_EXIT,
>   				lps0_dsm_func_mask_microsoft, lps0_dsm_guid_microsoft);
> @@ -648,6 +697,9 @@ static int acpi_s2idle_display_on(void)
>   	acpi_scan_lock_acquire();
>   
>   	/* Display on */
> +	if (delay_quirks && delay_quirks->delay_display_on)
> +		msleep(delay_quirks->delay_display_on);
> +
>   	if (lps0_dsm_func_mask_microsoft > 0)
>   		acpi_sleep_run_lps0_dsm(ACPI_LPS0_DISPLAY_ON,
>   				lps0_dsm_func_mask_microsoft, lps0_dsm_guid_microsoft);
> @@ -760,6 +812,10 @@ int acpi_register_lps0_dev(struct acpi_s2idle_dev_ops *arg)
>   
>   	sleep_flags = lock_system_sleep();
>   	list_add(&arg->list_node, &lps0_s2idle_devops_head);
> +	const struct dmi_system_id *s2idle_sysid = dmi_first_match(
> +		s2idle_delay_quirks
> +	);
> +	delay_quirks = s2idle_sysid ? s2idle_sysid->driver_data : NULL;
>   	unlock_system_sleep(sleep_flags);
>   
>   	return 0;