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

[Rafael J. Wysocki]

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.

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.

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