Re: [PATCH v3 01/10] PM: suspend: Introduce `pm_suspend_preferred_s2idle`

From: "Limonciello, Mario" <mario.limonciello@amd.com>
To: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Pavel Machek <pavel@ucw.cz>, Len Brown <len.brown@intel.com>,
	Linux PM <linux-pm@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v3 01/10] PM: suspend: Introduce `pm_suspend_preferred_s2idle`
Date: Tue, 12 Jul 2022 15:16:53 -0500	[thread overview]
Message-ID: <fd572ac1-9447-b148-8ad5-c6ecca5c6477@amd.com> (raw)
In-Reply-To: <CAJZ5v0g5Zsbddid+w2qxa_bqwmeP-FSk_42SZ3doMoFs0r8S8g@mail.gmail.com>

On 7/12/2022 14:06, Rafael J. Wysocki wrote:
> On Fri, Jul 1, 2022 at 4:33 AM Mario Limonciello
> <mario.limonciello@amd.com> wrote:
>>
>> Many drivers in the kernel will check the FADT to determine if low
>> power idle is supported and use this information to set up a policy
>> decision in the driver.  To abstract this information from drivers
>> introduce a new helper symbol that can indicate this information.
>>
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> ---
>>   include/linux/suspend.h |  1 +
>>   kernel/power/suspend.c  | 17 +++++++++++++++++
>>   2 files changed, 18 insertions(+)
>>
>> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
>> index 70f2921e2e70..9d911e026720 100644
>> --- a/include/linux/suspend.h
>> +++ b/include/linux/suspend.h
>> @@ -305,6 +305,7 @@ static inline bool idle_should_enter_s2idle(void)
>>          return unlikely(s2idle_state == S2IDLE_STATE_ENTER);
>>   }
>>
>> +extern bool pm_suspend_preferred_s2idle(void);
>>   extern bool pm_suspend_default_s2idle(void);
>>   extern void __init pm_states_init(void);
>>   extern void s2idle_set_ops(const struct platform_s2idle_ops *ops);
>> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
>> index 827075944d28..0030e7dfe6cf 100644
>> --- a/kernel/power/suspend.c
>> +++ b/kernel/power/suspend.c
>> @@ -9,6 +9,7 @@
>>
>>   #define pr_fmt(fmt) "PM: " fmt
>>
>> +#include <linux/acpi.h>
>>   #include <linux/string.h>
>>   #include <linux/delay.h>
>>   #include <linux/errno.h>
>> @@ -61,6 +62,22 @@ static DECLARE_SWAIT_QUEUE_HEAD(s2idle_wait_head);
>>   enum s2idle_states __read_mostly s2idle_state;
>>   static DEFINE_RAW_SPINLOCK(s2idle_lock);
>>
>> +/**
>> + * pm_suspend_preferred_s2idle - Check if suspend-to-idle is the preferred suspend method.
>> + *
>> + * Return 'true' if suspend-to-idle is preferred by the system designer for the default
>> + * suspend method.
>> + */
>> +bool pm_suspend_preferred_s2idle(void)
>> +{
>> +#ifdef CONFIG_ACPI
>> +       return acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0;
>> +#else
>> +       return false;
>> +#endif
>> +}
>> +EXPORT_SYMBOL_GPL(pm_suspend_preferred_s2idle);
> 
> First, this is ACPI-specific, so please don't try to generalize it
> artificially.  This confuses things and doesn't really help.
> 
> Second, ACPI_FADT_LOW_POWER_S0 means that "low power S0 idle" is
> supported, not that suspend-to-idle is the preferred suspend method in
> Linux.
> 
> System designers who set that bit in FADT may not even know what
> suspend-to-idle is.

In "practice" it means that the system has been enabled for Modern 
Standby in Windows.

> 
> But it is good that you have identified the code checking that bit,
> because it should not be checked without a valid reason.  I need to
> review that code and see what's going on in there.

Within this series the intent is to see that the vendor meant the system 
to be using Modern Standby on Windows (and implicitly Suspend To Idle on 
Linux).

With this I was trying to distinguish intent of the OEM between intent 
of the user for helping to declare policy.  Maybe the distinction of OEM 
and user decision don't really matter though and "mem_sleep_current" is 
better to use?  I think in a lot of the cases that I outlined I think 
that mem_sleep_current can drop right in instead of acpi_gbl_FADT.flags.

If you would like I'm happy to do that and send a new series.

> 
>> +
>>   /**
>>    * pm_suspend_default_s2idle - Check if suspend-to-idle is the default suspend.
>>    *
>> --
>> 2.34.1
>>