Re: [PATCH v1] ACPI: PM: s2idle: Add module parameter for LPS0 constraints checking

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

On 1/13/2026 7:36 AM, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Commit 32ece31db4df ("ACPI: PM: s2idle: Only retrieve constraints when
> needed") attempted to avoid useless evaluation of LPS0 _DSM Function 1
> in lps0_device_attach() because pm_debug_messages_on might never be set
> (and that is the case on production systems most of the time), but it
> turns out that LPS0 _DSM Function 1 is generally problematic on some
> platforms and causes suspend issues to occur when pm_debug_messages_on
> is set now.

Any ideas why it's causing problems?  AML doing something it shouldn't?

> 
> In Linux, LPS0 _DSM Function 1 is only useful for diagnostics and only
> in the cases when the system does not reach the deepest platform idle
> state during suspend-to-idle for some reason.  If such diagnostics is
> not necessary, evaluating it is a loss of time, so using it along with
> the other pm_debug_messages_on diagnostics is questionable because the
> latter is expected to be suitable for collecting debug information even
> during production use of system suspend.
> 
> For this reason, add a module parameter called check_lps0_constraints
> to control whether or not the list of LPS0 constraints will be checked
> in acpi_s2idle_prepare_late_lps0() and so whether or not to evaluate
> LPS0 _DSM Function 1 (once) in acpi_s2idle_begin_lps0().
> 
> Fixes: 32ece31db4df ("ACPI: PM: s2idle: Only retrieve constraints when needed")
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>   drivers/acpi/x86/s2idle.c |    8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
> 
> --- a/drivers/acpi/x86/s2idle.c
> +++ b/drivers/acpi/x86/s2idle.c
> @@ -28,6 +28,10 @@ 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");
>   
> +static bool check_lps0_constraints __read_mostly;
> +module_param(check_lps0_constraints, bool, 0644);
> +MODULE_PARM_DESC(check_lps0_constraints, "Check LPS0 device constraints");

I'm personally not really a fan of another module parameter for 
debugging.  I know some other subsystem maintainers are very anti-module 
parameters too.

I did like having /sys/power/pm_debug_messages able to be a one stop 
shop for debugging messages at runtime.

So I had another idea I wanted to throw around.  What if instead we had 
a file /sys/kernel/debug/x86/lps0_constraints?

If the file is never accessed never evaluate constraints.  If you read 
it once then you can get a dump of all the current constraints and any 
future suspends during that boot will also include constraints in the 
logs (IE call lpi_check_constraints()).

> +
>   static const struct acpi_device_id lps0_device_ids[] = {
>   	{"PNP0D80", },
>   	{"", },
> @@ -515,7 +519,7 @@ static struct acpi_scan_handler lps0_han
>   
>   static int acpi_s2idle_begin_lps0(void)
>   {
> -	if (lps0_device_handle && !sleep_no_lps0 && pm_debug_messages_on &&
> +	if (lps0_device_handle && !sleep_no_lps0 && check_lps0_constraints &&
>   	    !lpi_constraints_table) {
>   		if (acpi_s2idle_vendor_amd())
>   			lpi_device_get_constraints_amd();
> @@ -540,7 +544,7 @@ static int acpi_s2idle_prepare_late_lps0
>   	if (!lps0_device_handle || sleep_no_lps0)
>   		return 0;
>   
> -	if (pm_debug_messages_on)
> +	if (check_lps0_constraints)
>   		lpi_check_constraints();
>   
>   	/* Screen off */
> 
> 
>