RE: [PATCH] ACPI / PM: Prefer suspend-to-idle over S3 on some systems

[Mario.Limonciello]

> -----Original Message-----
> From: Rafael J. Wysocki [mailto:rjw@xxxxxxxxxxxxx]
> Sent: Monday, July 31, 2017 4:43 PM
> To: Linux ACPI <linux-acpi@xxxxxxxxxxxxxxx>
> Cc: LKML <linux-kernel@xxxxxxxxxxxxxxx>; Len Brown <len.brown@xxxxxxxxx>;
> Linux PM <linux-pm@xxxxxxxxxxxxxxx>; Srinivas Pandruvada
> <srinivas.pandruvada@xxxxxxxxxxxxxxx>; Limonciello, Mario
> <Mario_Limonciello@xxxxxxxx>
> Subject: [PATCH] ACPI / PM: Prefer suspend-to-idle over S3 on some systems
> 
> From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> 
> Modify the ACPI system sleep support setup code to select
> suspend-to-idle as the default system sleep state if
> (1) the ACPI_FADT_LOW_POWER_S0 flag is set in the FADT and
> (2) the Low Power Idle S0 _DSM interface has been discovered and
> (3) the default sleep state was not selected from the kernel command
> line.
> 
> The main motivation for this change is that systems where the (1) and
> (2) conditions are met typically ship with OSes that don't exercise
> the S3 path in the platform firmware which remains untested and turns
> out to be non-functional at least in some cases.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> ---
> 
> This patch depends on the intel-hid change at
> 
> https://patchwork.kernel.org/patch/9867703/
> 
> ---
>  Documentation/power/states.txt |    4 +++-
>  drivers/acpi/sleep.c           |    6 ++++++
>  include/linux/suspend.h        |    3 +++
>  kernel/power/power.h           |    1 -
>  kernel/power/suspend.c         |    4 ++--
>  5 files changed, 14 insertions(+), 4 deletions(-)
> 
> Index: linux-pm/Documentation/power/states.txt
> ===================================================================
> --- linux-pm.orig/Documentation/power/states.txt
> +++ linux-pm/Documentation/power/states.txt
> @@ -35,7 +35,9 @@ only one way to cause the system to go i
>  The default suspend mode (ie. the one to be used without writing anything into
>  /sys/power/mem_sleep) is either "deep" (if Suspend-To-RAM is supported) or
>  "s2idle", but it can be overridden by the value of the "mem_sleep_default"
> -parameter in the kernel command line.
> +parameter in the kernel command line.  On some ACPI-based systems, depending
> on
> +the information in the FADT, the default may be "s2idle" even if Suspend-To-RAM
> +is supported.
> 
>  The properties of all of the sleep states are described below.
> 
> Index: linux-pm/drivers/acpi/sleep.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/sleep.c
> +++ linux-pm/drivers/acpi/sleep.c
> @@ -714,6 +714,12 @@ static int lps0_device_attach(struct acp
>  		if ((bitmask & ACPI_S2IDLE_FUNC_MASK) ==
> ACPI_S2IDLE_FUNC_MASK) {
>  			lps0_dsm_func_mask = bitmask;
>  			lps0_device_handle = adev->handle;
> +			/*
> +			 * Use suspend-to-idle by default if the default
> +			 * suspend mode was not set from the command line.
> +			 */
> +			if (mem_sleep_default > PM_SUSPEND_MEM)
> +				mem_sleep_current = PM_SUSPEND_FREEZE;
>  		}
> 
>  		acpi_handle_debug(adev->handle, "_DSM function mask: 0x%x\n",
> Index: linux-pm/include/linux/suspend.h
> ===================================================================
> --- linux-pm.orig/include/linux/suspend.h
> +++ linux-pm/include/linux/suspend.h
> @@ -196,6 +196,9 @@ struct platform_freeze_ops {
>  };
> 
>  #ifdef CONFIG_SUSPEND
> +extern suspend_state_t mem_sleep_current;
> +extern suspend_state_t mem_sleep_default;
> +
>  /**
>   * suspend_set_ops - set platform dependent suspend operations
>   * @ops: The new suspend operations to set.
> Index: linux-pm/kernel/power/suspend.c
> ===================================================================
> --- linux-pm.orig/kernel/power/suspend.c
> +++ linux-pm/kernel/power/suspend.c
> @@ -48,7 +48,7 @@ static const char * const mem_sleep_labe
>  const char *mem_sleep_states[PM_SUSPEND_MAX];
> 
>  suspend_state_t mem_sleep_current = PM_SUSPEND_FREEZE;
> -static suspend_state_t mem_sleep_default = PM_SUSPEND_MEM;
> +suspend_state_t mem_sleep_default = PM_SUSPEND_MAX;
>  suspend_state_t pm_suspend_target_state;
>  EXPORT_SYMBOL_GPL(pm_suspend_target_state);
> 
> @@ -216,7 +216,7 @@ void suspend_set_ops(const struct platfo
>  	}
>  	if (valid_state(PM_SUSPEND_MEM)) {
>  		mem_sleep_states[PM_SUSPEND_MEM] =
> mem_sleep_labels[PM_SUSPEND_MEM];
> -		if (mem_sleep_default == PM_SUSPEND_MEM)
> +		if (mem_sleep_default >= PM_SUSPEND_MEM)
>  			mem_sleep_current = PM_SUSPEND_MEM;
>  	}
> 
> Index: linux-pm/kernel/power/power.h
> ===================================================================
> --- linux-pm.orig/kernel/power/power.h
> +++ linux-pm/kernel/power/power.h
> @@ -192,7 +192,6 @@ extern void swsusp_show_speed(ktime_t, k
>  extern const char * const pm_labels[];
>  extern const char *pm_states[];
>  extern const char *mem_sleep_states[];
> -extern suspend_state_t mem_sleep_current;
> 
>  extern int suspend_devices_and_enter(suspend_state_t state);
>  #else /* !CONFIG_SUSPEND */

Rafael,

This patch works for me on XPS 9365.

I ran into some problems with linux-pm.git/linux-next including
this patch though.  I spent a little time debugging and sent a follow 
up patch to intel-vbtn.  It's not strictly caused by this patch, but I 
just noticed it now with this default in place.

Tested-by: Mario Limonciello <mario.limonciello@xxxxxxxx>

Thanks,