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

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

On 1/13/2026 3:57 PM, Rafael J. Wysocki wrote:
> On Tue, Jan 13, 2026 at 10:48 PM Mario Limonciello
> <mario.limonciello@amd.com> wrote:
>> 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?
> 
> It's not a clear AML bug AFAICS.  Rather, it seems to have
> dependencies on something that is not ready when it is evaluated, so
> an ordering issue or similar.

Ah I see.

> 
>>>
>>> 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.
> 
> Well, this is not just debug messages, rather a whole debug facility
> enabled via pm_debug_messages, which is kind of confusing.

Good point.

> 
>> So I had another idea I wanted to throw around.  What if instead we had
>> a file /sys/kernel/debug/x86/lps0_constraints?
> 
> Then you cannot use this without debugfs.

I mean; yeah.  But it really is debugging and most distros have debugfs 
enabled by default these days don't they?

> 
>> 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()).
> 
> So if it is not in debugfs, it would need to be in sysfs and then I
> don't see much difference between it and a module param, honestly.
> 
> I actually prefer the latter because it uses an existing infra.

I agree if debugfs is decided to be out sysfs and module parameter are 
equal footing and then this patch makes sense.