| Message ID | 20220701023328.2783-1-mario.limonciello@amd.com (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
| Series | [v3,01/10] PM: suspend: Introduce `pm_suspend_preferred_s2idle` | expand |
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. 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 >
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 >>
On Tue, Jul 12, 2022 at 10:17 PM Limonciello, Mario <mario.limonciello@amd.com> wrote: > > 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. 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.
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); + /** * pm_suspend_default_s2idle - Check if suspend-to-idle is the default suspend. *
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(+)