[PATCH] acpi/sleep: Use the FADT to prefer Suspend-to-Idle instead of S3

[PATCH] acpi/sleep: Use the FADT to prefer Suspend-to-Idle instead of S3

[Mario Limonciello]

If the ACPI_FADT_LOW_POWER_S0 bit is set, this indicates the platform
should get identical or better performance using Suspend-To-Idle.

By removing S3 and S1 userspace will prefer suspend-to-idle in
these situations too.

Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
---
 drivers/acpi/sleep.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
index deb0ff7..ff1d8f1 100644
--- a/drivers/acpi/sleep.c
+++ b/drivers/acpi/sleep.c
@@ -171,6 +171,12 @@ static int __init init_nvs_nosave(const struct dmi_system_id *d)
 	return 0;
 }
 
+static bool suspend_to_idle_preferred(void)
+{
+	return (acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0);
+}
+
+
 static struct dmi_system_id acpisleep_dmi_table[] __initdata = {
 	{
 	.callback = init_old_suspend_ordering,
@@ -908,6 +914,11 @@ int __init acpi_sleep_init(void)
 		acpi_no_s5 = true;
 	}
 
+	if (suspend_to_idle_preferred()) {
+		sleep_states[ACPI_STATE_S3] = 0;
+		sleep_states[ACPI_STATE_S1] = 0;
+	}
+
 	supported[0] = 0;
 	for (i = 0; i < ACPI_S_STATE_COUNT; i++) {
 		if (sleep_states[i])
-- 
2.7.4

Re: [PATCH] acpi/sleep: Use the FADT to prefer Suspend-to-Idle instead of S3

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2900 bytes --]

Hi Mario,

[auto build test ERROR on pm/linux-next]
[also build test ERROR on v4.9-rc4 next-20161110]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Mario-Limonciello/acpi-sleep-Use-the-FADT-to-prefer-Suspend-to-Idle-instead-of-S3/20161111-055555
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
config: i386-randconfig-x006-201645 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All error/warnings (new ones prefixed by >>):

   In file included from include/linux/linkage.h:4:0,
                    from include/linux/kernel.h:6,
                    from include/linux/delay.h:10,
                    from drivers/acpi/sleep.c:13:
   drivers/acpi/sleep.c: In function 'acpi_sleep_init':
>> drivers/acpi/sleep.c:917:6: error: implicit declaration of function 'suspend_to_idle_preferred' [-Werror=implicit-function-declaration]
     if (suspend_to_idle_preferred()) {
         ^
   include/linux/compiler.h:149:30: note: in definition of macro '__trace_if'
     if (__builtin_constant_p(!!(cond)) ? !!(cond) :   \
                                 ^~~~
>> drivers/acpi/sleep.c:917:2: note: in expansion of macro 'if'
     if (suspend_to_idle_preferred()) {
     ^~
   Cyclomatic Complexity 1 arch/x86/include/asm/paravirt.h:arch_local_irq_disable
   Cyclomatic Complexity 1 drivers/acpi/sleep.c:acpi_sleep_dmi_check
   Cyclomatic Complexity 1 drivers/acpi/sleep.c:acpi_sleep_suspend_setup
   Cyclomatic Complexity 1 drivers/acpi/sleep.c:acpi_sleep_syscore_init
   Cyclomatic Complexity 1 drivers/acpi/sleep.c:acpi_sleep_hibernate_setup
   Cyclomatic Complexity 9 drivers/acpi/sleep.c:acpi_sleep_tts_switch
   Cyclomatic Complexity 9 drivers/acpi/sleep.c:acpi_sleep_pts_switch
   Cyclomatic Complexity 1 drivers/acpi/sleep.c:sleep_notify_reboot
   Cyclomatic Complexity 1 drivers/acpi/sleep.c:acpi_power_off
   Cyclomatic Complexity 1 drivers/acpi/sleep.c:acpi_sleep_prepare
   Cyclomatic Complexity 1 drivers/acpi/sleep.c:acpi_power_off_prepare
   Cyclomatic Complexity 5 drivers/acpi/sleep.c:acpi_sleep_state_supported
   Cyclomatic Complexity 6 drivers/acpi/sleep.c:acpi_sleep_init
   cc1: some warnings being treated as errors

vim +/suspend_to_idle_preferred +917 drivers/acpi/sleep.c

   911			pm_power_off_prepare = acpi_power_off_prepare;
   912			pm_power_off = acpi_power_off;
   913		} else {
   914			acpi_no_s5 = true;
   915		}
   916	
 > 917		if (suspend_to_idle_preferred()) {
   918			sleep_states[ACPI_STATE_S3] = 0;
   919			sleep_states[ACPI_STATE_S1] = 0;
   920		}

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 26262 bytes --]

RE: [PATCH] acpi/sleep: Use the FADT to prefer Suspend-to-Idle instead of S3

[Mario.Limonciello]

> -----Original Message-----
> From: kbuild test robot [mailto:lkp@intel.com]
> Sent: Thursday, November 10, 2016 4:15 PM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> Cc: kbuild-all@01.org; linux-acpi@vger.kernel.org; Len Brown
> <lenb@kernel.org>; Rafael J . Wysocki <rjw@rjwysocki.net>; Andy
> Lutomirski <luto@kernel.org>; Limonciello, Mario
> <Mario_Limonciello@Dell.com>
> Subject: Re: [PATCH] acpi/sleep: Use the FADT to prefer Suspend-to-Idle
> instead of S3
> 
> Hi Mario,
> 
> [auto build test ERROR on pm/linux-next]
> [also build test ERROR on v4.9-rc4 next-20161110]
> [if your patch is applied to the wrong git tree, please drop us a note to help
> improve the system]
> 
> url:    https://github.com/0day-ci/linux/commits/Mario-Limonciello/acpi-
> sleep-Use-the-FADT-to-prefer-Suspend-to-Idle-instead-of-S3/20161111-
> 055555
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git
> linux-next
> config: i386-randconfig-x006-201645 (attached as .config)
> compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
> reproduce:
>         # save the attached .config to linux build tree
>         make ARCH=i386
> 
> All error/warnings (new ones prefixed by >>):
> 
>    In file included from include/linux/linkage.h:4:0,
>                     from include/linux/kernel.h:6,
>                     from include/linux/delay.h:10,
>                     from drivers/acpi/sleep.c:13:
>    drivers/acpi/sleep.c: In function 'acpi_sleep_init':
> >> drivers/acpi/sleep.c:917:6: error: implicit declaration of function
> 'suspend_to_idle_preferred' [-Werror=implicit-function-declaration]
>      if (suspend_to_idle_preferred()) {
>          ^
>    include/linux/compiler.h:149:30: note: in definition of macro '__trace_if'
>      if (__builtin_constant_p(!!(cond)) ? !!(cond) :   \
>                                  ^~~~
> >> drivers/acpi/sleep.c:917:2: note: in expansion of macro 'if'
>      if (suspend_to_idle_preferred()) {
>      ^~
>    Cyclomatic Complexity 1
> arch/x86/include/asm/paravirt.h:arch_local_irq_disable
>    Cyclomatic Complexity 1 drivers/acpi/sleep.c:acpi_sleep_dmi_check
>    Cyclomatic Complexity 1 drivers/acpi/sleep.c:acpi_sleep_suspend_setup
>    Cyclomatic Complexity 1 drivers/acpi/sleep.c:acpi_sleep_syscore_init
>    Cyclomatic Complexity 1 drivers/acpi/sleep.c:acpi_sleep_hibernate_setup
>    Cyclomatic Complexity 9 drivers/acpi/sleep.c:acpi_sleep_tts_switch
>    Cyclomatic Complexity 9 drivers/acpi/sleep.c:acpi_sleep_pts_switch
>    Cyclomatic Complexity 1 drivers/acpi/sleep.c:sleep_notify_reboot
>    Cyclomatic Complexity 1 drivers/acpi/sleep.c:acpi_power_off
>    Cyclomatic Complexity 1 drivers/acpi/sleep.c:acpi_sleep_prepare
>    Cyclomatic Complexity 1 drivers/acpi/sleep.c:acpi_power_off_prepare
>    Cyclomatic Complexity 5 drivers/acpi/sleep.c:acpi_sleep_state_supported
>    Cyclomatic Complexity 6 drivers/acpi/sleep.c:acpi_sleep_init
>    cc1: some warnings being treated as errors
> 
> vim +/suspend_to_idle_preferred +917 drivers/acpi/sleep.c
> 
>    911			pm_power_off_prepare = acpi_power_off_prepare;
>    912			pm_power_off = acpi_power_off;
>    913		} else {
>    914			acpi_no_s5 = true;
>    915		}
>    916
>  > 917		if (suspend_to_idle_preferred()) {
>    918			sleep_states[ACPI_STATE_S3] = 0;
>    919			sleep_states[ACPI_STATE_S1] = 0;
>    920		}
> 
> ---
> 0-DAY kernel test infrastructure                Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

In v2 I'll resubmit with the test moved into acpi_sleep_suspend_setup(void) so
that this works properly even if CONFIG_ACPI_SLEEP isn't enabled.
I'll look for any other comments however about this approach before I submit v2.

Thanks,

Re: [PATCH] acpi/sleep: Use the FADT to prefer Suspend-to-Idle instead of S3

[Rafael J. Wysocki]

On Thu, Nov 10, 2016 at 10:35 PM, Mario Limonciello
<mario.limonciello@dell.com> wrote:
> If the ACPI_FADT_LOW_POWER_S0 bit is set, this indicates the platform
> should get identical or better performance using Suspend-To-Idle.
>
> By removing S3 and S1 userspace will prefer suspend-to-idle in
> these situations too.
>
> Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
> ---
>  drivers/acpi/sleep.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
> index deb0ff7..ff1d8f1 100644
> --- a/drivers/acpi/sleep.c
> +++ b/drivers/acpi/sleep.c
> @@ -171,6 +171,12 @@ static int __init init_nvs_nosave(const struct dmi_system_id *d)
>         return 0;
>  }
>
> +static bool suspend_to_idle_preferred(void)
> +{
> +       return (acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0);
> +}
> +
> +
>  static struct dmi_system_id acpisleep_dmi_table[] __initdata = {
>         {
>         .callback = init_old_suspend_ordering,
> @@ -908,6 +914,11 @@ int __init acpi_sleep_init(void)
>                 acpi_no_s5 = true;
>         }
>
> +       if (suspend_to_idle_preferred()) {
> +               sleep_states[ACPI_STATE_S3] = 0;
> +               sleep_states[ACPI_STATE_S1] = 0;
> +       }
> +
>         supported[0] = 0;
>         for (i = 0; i < ACPI_S_STATE_COUNT; i++) {
>                 if (sleep_states[i])
> --

I'd do that in a different way.

Let me cut a patch for that (I haven't had the time to do that yet)
and we'll see.

Thanks,
Rafael

RE: [PATCH] acpi/sleep: Use the FADT to prefer Suspend-to-Idle instead of S3

[Mario.Limonciello]

> -----Original Message-----
> From: rjwysocki@gmail.com [mailto:rjwysocki@gmail.com] On Behalf Of
> Rafael J. Wysocki
> Sent: Thursday, November 10, 2016 5:54 PM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> Cc: ACPI Devel Maling List <linux-acpi@vger.kernel.org>; Len Brown
> <lenb@kernel.org>; Rafael J . Wysocki <rjw@rjwysocki.net>; Andy
> Lutomirski <luto@kernel.org>
> Subject: Re: [PATCH] acpi/sleep: Use the FADT to prefer Suspend-to-Idle
> instead of S3
> 
> On Thu, Nov 10, 2016 at 10:35 PM, Mario Limonciello
> <mario.limonciello@dell.com> wrote:
> > If the ACPI_FADT_LOW_POWER_S0 bit is set, this indicates the platform
> > should get identical or better performance using Suspend-To-Idle.
> >
> > By removing S3 and S1 userspace will prefer suspend-to-idle in
> > these situations too.
> >
> > Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
> > ---
> >  drivers/acpi/sleep.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
> > index deb0ff7..ff1d8f1 100644
> > --- a/drivers/acpi/sleep.c
> > +++ b/drivers/acpi/sleep.c
> > @@ -171,6 +171,12 @@ static int __init init_nvs_nosave(const struct
> dmi_system_id *d)
> >         return 0;
> >  }
> >
> > +static bool suspend_to_idle_preferred(void)
> > +{
> > +       return (acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0);
> > +}
> > +
> > +
> >  static struct dmi_system_id acpisleep_dmi_table[] __initdata = {
> >         {
> >         .callback = init_old_suspend_ordering,
> > @@ -908,6 +914,11 @@ int __init acpi_sleep_init(void)
> >                 acpi_no_s5 = true;
> >         }
> >
> > +       if (suspend_to_idle_preferred()) {
> > +               sleep_states[ACPI_STATE_S3] = 0;
> > +               sleep_states[ACPI_STATE_S1] = 0;
> > +       }
> > +
> >         supported[0] = 0;
> >         for (i = 0; i < ACPI_S_STATE_COUNT; i++) {
> >                 if (sleep_states[i])
> > --
> 
> I'd do that in a different way.
> 
> Let me cut a patch for that (I haven't had the time to do that yet)
> and we'll see.
> 
> Thanks,
> Rafael

OK, look forward to seeing the approach you want to go with this
instead.

Thanks,

Re: [PATCH] acpi/sleep: Use the FADT to prefer Suspend-to-Idle instead of S3

[Rafael J. Wysocki]

On Wednesday, November 16, 2016 08:18:13 PM Mario.Limonciello@dell.com wrote:
> > -----Original Message-----
> > From: rjwysocki@gmail.com [mailto:rjwysocki@gmail.com] On Behalf Of
> > Rafael J. Wysocki
> > Sent: Thursday, November 10, 2016 5:54 PM
> > To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> > Cc: ACPI Devel Maling List <linux-acpi@vger.kernel.org>; Len Brown
> > <lenb@kernel.org>; Rafael J . Wysocki <rjw@rjwysocki.net>; Andy
> > Lutomirski <luto@kernel.org>
> > Subject: Re: [PATCH] acpi/sleep: Use the FADT to prefer Suspend-to-Idle
> > instead of S3
> > 
> > On Thu, Nov 10, 2016 at 10:35 PM, Mario Limonciello
> > <mario.limonciello@dell.com> wrote:
> > > If the ACPI_FADT_LOW_POWER_S0 bit is set, this indicates the platform
> > > should get identical or better performance using Suspend-To-Idle.
> > >
> > > By removing S3 and S1 userspace will prefer suspend-to-idle in
> > > these situations too.
> > >
> > > Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
> > > ---
> > >  drivers/acpi/sleep.c | 11 +++++++++++
> > >  1 file changed, 11 insertions(+)
> > >
> > > diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
> > > index deb0ff7..ff1d8f1 100644
> > > --- a/drivers/acpi/sleep.c
> > > +++ b/drivers/acpi/sleep.c
> > > @@ -171,6 +171,12 @@ static int __init init_nvs_nosave(const struct
> > dmi_system_id *d)
> > >         return 0;
> > >  }
> > >
> > > +static bool suspend_to_idle_preferred(void)
> > > +{
> > > +       return (acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0);
> > > +}
> > > +
> > > +
> > >  static struct dmi_system_id acpisleep_dmi_table[] __initdata = {
> > >         {
> > >         .callback = init_old_suspend_ordering,
> > > @@ -908,6 +914,11 @@ int __init acpi_sleep_init(void)
> > >                 acpi_no_s5 = true;
> > >         }
> > >
> > > +       if (suspend_to_idle_preferred()) {
> > > +               sleep_states[ACPI_STATE_S3] = 0;
> > > +               sleep_states[ACPI_STATE_S1] = 0;
> > > +       }
> > > +
> > >         supported[0] = 0;
> > >         for (i = 0; i < ACPI_S_STATE_COUNT; i++) {
> > >                 if (sleep_states[i])
> > > --
> > 
> > I'd do that in a different way.
> > 
> > Let me cut a patch for that (I haven't had the time to do that yet)
> > and we'll see.
> > 
> > Thanks,
> > Rafael
> 
> OK, look forward to seeing the approach you want to go with this
> instead.

Please have a look at

https://patchwork.kernel.org/patch/9433421/
https://patchwork.kernel.org/patch/9433429/

Thanks,
Rafael