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

[Rafael J. Wysocki]

On Tue, Jul 12, 2022 at 10:17 PM Limonciello, Mario
<mario.limonciello@xxxxxxx> wrote:
>
> On 7/12/2022 14:06, Rafael J. Wysocki wrote:
> > On Fri, Jul 1, 2022 at 4:33 AM Mario Limonciello
> > <mario.limonciello@xxxxxxx> 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@xxxxxxx>
> >> ---
> >>   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.

Checking ACPI_FADT_LOW_POWER_S0 is questionable in the majority of
cases, because if S3 is not supported, that flag is in fact
irrelevant, because Linux will allow suspend-to-idle to be used in
that case regardless of it.

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

I need to look at the individual pieces of code that check
ACPI_FADT_LOW_POWER_S0.