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

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

[Mario Limonciello]

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);
+
 /**
  * pm_suspend_default_s2idle - Check if suspend-to-idle is the default suspend.
  *
-- 
2.34.1

[PATCH v3 02/10] ACPI / x86: Use new `pm_suspend_preferred_s2idle`

[Mario Limonciello]

Drop the direct check from the FADT and use the helper instead.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/acpi/x86/s2idle.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/acpi/x86/s2idle.c b/drivers/acpi/x86/s2idle.c
index 2963229062f8..b1483d5092c1 100644
--- a/drivers/acpi/x86/s2idle.c
+++ b/drivers/acpi/x86/s2idle.c
@@ -369,7 +369,7 @@ static int lps0_device_attach(struct acpi_device *adev,
 	if (lps0_device_handle)
 		return 0;
 
-	if (!(acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0))
+	if (!pm_suspend_preferred_s2idle())
 		return 0;
 
 	if (acpi_s2idle_vendor_amd()) {
-- 
2.34.1

[PATCH v3 03/10] ACPI: LPIT: Use new `pm_suspend_preferred_s2idle`

[Mario Limonciello]

Drop the direct check from the FADT and use the helper instead.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/acpi/acpi_lpit.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/acpi_lpit.c b/drivers/acpi/acpi_lpit.c
index 48e5059d67ca..31c49107c579 100644
--- a/drivers/acpi/acpi_lpit.c
+++ b/drivers/acpi/acpi_lpit.c
@@ -6,6 +6,7 @@
  * Copyright (C) 2017 Intel Corporation. All rights reserved.
  */
 
+#include <linux/suspend.h>
 #include <linux/cpu.h>
 #include <linux/acpi.h>
 #include <asm/msr.h>
@@ -109,7 +110,7 @@ static void lpit_update_residency(struct lpit_residency_info *info,
 		if (!info->iomem_addr)
 			return;
 
-		if (!(acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0))
+		if (!pm_suspend_preferred_s2idle())
 			return;
 
 		/* Silently fail, if cpuidle attribute group is not present */
@@ -117,7 +118,7 @@ static void lpit_update_residency(struct lpit_residency_info *info,
 					&dev_attr_low_power_idle_system_residency_us.attr,
 					"cpuidle");
 	} else if (info->gaddr.space_id == ACPI_ADR_SPACE_FIXED_HARDWARE) {
-		if (!(acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0))
+		if (!pm_suspend_preferred_s2idle())
 			return;
 
 		/* Silently fail, if cpuidle attribute group is not present */
-- 
2.34.1

[PATCH v3 04/10] ata: ahci: Use `pm_suspend_preferred_s2idle`

[Mario Limonciello]

Drop the direct check from the FADT and use the helper instead.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/ata/ahci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index c1eca72b4575..3f79b732dd00 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -1611,7 +1611,7 @@ static void ahci_update_initial_lpm_policy(struct ata_port *ap,
 
 #ifdef CONFIG_ACPI
 	if (policy > ATA_LPM_MED_POWER &&
-	    (acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0)) {
+	    pm_suspend_preferred_s2idle()) {
 		if (hpriv->cap & HOST_CAP_PART)
 			policy = ATA_LPM_MIN_POWER_WITH_PARTIAL;
 		else if (hpriv->cap & HOST_CAP_SSC)
-- 
2.34.1

[PATCH v3 05/10] rtc: cmos: Use `pm_suspend_preferred_s2idle`

[Mario Limonciello]

Drop the direct check from the FADT and use the helper instead.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/rtc/rtc-cmos.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c
index 7c006c2b125f..2e3d35e87061 100644
--- a/drivers/rtc/rtc-cmos.c
+++ b/drivers/rtc/rtc-cmos.c
@@ -32,6 +32,7 @@
 #include <linux/init.h>
 #include <linux/interrupt.h>
 #include <linux/spinlock.h>
+#include <linux/suspend.h>
 #include <linux/platform_device.h>
 #include <linux/log2.h>
 #include <linux/pm.h>
@@ -1260,7 +1261,7 @@ static void use_acpi_alarm_quirks(void)
 	if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL)
 		return;
 
-	if (!(acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0))
+	if (!pm_suspend_preferred_s2idle())
 		return;
 
 	if (!is_hpet_enabled())
-- 
2.34.1

[PATCH v3 06/10] thermal: intel: pch: Use `pm_suspend_preferred_s2idle`

[Mario Limonciello]

Drop the direct check from the FADT and use the helper instead.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/thermal/intel/intel_pch_thermal.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/thermal/intel/intel_pch_thermal.c b/drivers/thermal/intel/intel_pch_thermal.c
index c1fa2b29b153..3124c4b1da76 100644
--- a/drivers/thermal/intel/intel_pch_thermal.c
+++ b/drivers/thermal/intel/intel_pch_thermal.c
@@ -208,12 +208,8 @@ static int pch_wpt_suspend(struct pch_thermal_device *ptd)
 	}
 
 	/* Do not check temperature if it is not a S0ix capable platform */
-#ifdef CONFIG_ACPI
-	if (!(acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0))
+	if (!pm_suspend_preferred_s2idle())
 		return 0;
-#else
-	return 0;
-#endif
 
 	/* Do not check temperature if it is not s2idle */
 	if (pm_suspend_via_firmware())
-- 
2.34.1

[PATCH v3 07/10] drm/amd: Use `pm_suspend_preferred_s2idle`

[Mario Limonciello]

Drop the direct check from the FADT and use the helper instead.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
index 98ac53ee6bb5..2146232c62ef 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
@@ -1072,7 +1072,7 @@ bool amdgpu_acpi_is_s0ix_active(struct amdgpu_device *adev)
 	    (pm_suspend_target_state != PM_SUSPEND_TO_IDLE))
 		return false;
 
-	if (!(acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0)) {
+	if (!pm_suspend_preferred_s2idle()) {
 		dev_warn_once(adev->dev,
 			      "Power consumption will be higher as BIOS has not been configured for suspend-to-idle.\n"
 			      "To use suspend-to-idle change the sleep mode in BIOS setup.\n");
-- 
2.34.1

[PATCH v3 08/10] drm/amd: Use `pm_suspend_default_s2idle`

[Mario Limonciello]

Rather than examining the suspend target, examine what the system is
configured to use.  This should be no functional change, just improves
readability by taking the helper instead.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
index 2146232c62ef..fc2c6e311979 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
@@ -1056,7 +1056,7 @@ bool amdgpu_acpi_should_gpu_reset(struct amdgpu_device *adev)
 {
 	if (adev->flags & AMD_IS_APU)
 		return false;
-	return pm_suspend_target_state != PM_SUSPEND_TO_IDLE;
+	return !pm_suspend_default_s2idle();
 }
 
 /**
@@ -1069,7 +1069,7 @@ bool amdgpu_acpi_should_gpu_reset(struct amdgpu_device *adev)
 bool amdgpu_acpi_is_s0ix_active(struct amdgpu_device *adev)
 {
 	if (!(adev->flags & AMD_IS_APU) ||
-	    (pm_suspend_target_state != PM_SUSPEND_TO_IDLE))
+	    !pm_suspend_default_s2idle())
 		return false;
 
 	if (!pm_suspend_preferred_s2idle()) {
-- 
2.34.1

[PATCH v3 09/10] HID: I2C-HID: Use `pm_suspend_preferred_s2idle`

[Mario Limonciello]

Drop the direct check from the FADT and use the helper instead.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/hid/i2c-hid/i2c-hid-acpi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid-acpi.c b/drivers/hid/i2c-hid/i2c-hid-acpi.c
index b96ae15e0ad9..84d51f309c53 100644
--- a/drivers/hid/i2c-hid/i2c-hid-acpi.c
+++ b/drivers/hid/i2c-hid/i2c-hid-acpi.c
@@ -105,7 +105,7 @@ static int i2c_hid_acpi_probe(struct i2c_client *client)
 
 	acpi_device_fix_up_power(adev);
 
-	if (acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0) {
+	if (pm_suspend_preferred_s2idle()) {
 		device_set_wakeup_capable(dev, true);
 		device_set_wakeup_enable(dev, false);
 	}
-- 
2.34.1

[PATCH v3 10/10] HID: usbhid: Set USB mice as s2idle wakeup resources

[Mario Limonciello]

The USB HID transport layer doesn't configure mice for wakeup by default.
Thus users can not wake system from s2idle via USB mouse. However, users
can wake the same system from Modern Standby on Windows with the same USB
mouse.

Microsoft documentation indicates that all USB mice and touchpads should
be waking the system from Modern Standby.

Many people who have used Windows on a PC that supports Modern Standby
have an expectation that s2idle wakeup sources should behave the same in
Linux. For example if your PC is configured "dual-boot" and is used docked
it's very common to wakeup by using a USB mouse connected to your dock in
Windows. Switching to Linux this is not enabled by default and you'll
need to manually turn it on or use a different wakeup source than you did
for Windows.

Changes for wakeups have been made in other subsystems such as the PS/2
keyboard driver which align how wakeup sources in Linux and Modern Standby
in Windows behave. To align expectations from users on USB mice, make this
behavior the same when the system is configured both by the OEM and the
user to use s2idle in Linux.

This means that at a minimum supported mice will be able to wakeup by
clicking a button. If the USB mouse is powered over the s2idle cycle (such
as a wireless mouse with a battery) it's also possible that moving it
may wake up the system.  This is HW dependent behavior.

If the user sets the system to use S3 instead of s2idle, or the OEM ships
the system defaulting to S3, this behavior will not be turned on by
default.

Users who have a modern laptop that supports s2idle and use s2idle but
prefer the previous Linux kernel behavior can turn this off via a udev
rule.

Link: https://docs.microsoft.com/en-us/windows-hardware/design/device-experiences/modern-standby-wake-sources#input-devices-1
Link: https://lore.kernel.org/linux-usb/20220404214557.3329796-1-richard.gong@amd.com/
Suggested-by: Richard Gong <richard.gong@amd.com>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
More people keep coming to us confused that they couldn't wake a Linux system
up from sleep using a mouse, so this patch is being revived.

Microsoft documentation doesn't indicate any allowlist for this behavior, and
they actually prescribe it for all USB mice and touchpads.

changes from v2->v3:
 * Use `pm_suspend_preferred_s2idle`
 * Drop now unnecessary acpi.h header inclusion
 * Update commit message
 * Adjust comments from v2 per thread

changes from v1->v2:
 * Resubmit by Mario
 * Update commit message
 * Only activate on systems configured by user and OEM for using s2idle
---
 drivers/hid/usbhid/hid-core.c | 37 ++++++++++++++++++++++++-----------
 1 file changed, 26 insertions(+), 11 deletions(-)

diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index 4490e2f7252a..d08511f00d3b 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -26,6 +26,7 @@
 #include <linux/wait.h>
 #include <linux/workqueue.h>
 #include <linux/string.h>
+#include <linux/suspend.h>
 
 #include <linux/usb.h>
 
@@ -1176,17 +1177,31 @@ static int usbhid_start(struct hid_device *hid)
 		usb_autopm_put_interface(usbhid->intf);
 	}
 
-	/* Some keyboards don't work until their LEDs have been set.
-	 * Since BIOSes do set the LEDs, it must be safe for any device
-	 * that supports the keyboard boot protocol.
-	 * In addition, enable remote wakeup by default for all keyboard
-	 * devices supporting the boot protocol.
-	 */
-	if (interface->desc.bInterfaceSubClass == USB_INTERFACE_SUBCLASS_BOOT &&
-			interface->desc.bInterfaceProtocol ==
-				USB_INTERFACE_PROTOCOL_KEYBOARD) {
-		usbhid_set_leds(hid);
-		device_set_wakeup_enable(&dev->dev, 1);
+	if (interface->desc.bInterfaceSubClass == USB_INTERFACE_SUBCLASS_BOOT) {
+		switch (interface->desc.bInterfaceProtocol) {
+		/* Some keyboards don't work until their LEDs have been set.
+		 * Since BIOSes do set the LEDs, it must be safe for any device
+		 * that supports the keyboard boot protocol.
+		 * In addition, enable remote wakeup by default for all keyboard
+		 * devices supporting the boot protocol.
+		 */
+		case USB_INTERFACE_PROTOCOL_KEYBOARD:
+			usbhid_set_leds(hid);
+			device_set_wakeup_enable(&dev->dev, 1);
+			break;
+		/*
+		 * Windows configures USB mice to be a wakeup source from Modern
+		 * Standby, and users have expectations that s2idle wakeup sources
+		 * behave the same.  Thus setup remote wakeup by default for mice
+		 * supporting boot protocol if the system supports s2idle and the user
+		 * has not disabled it on the kernel command line.
+		 */
+		case USB_INTERFACE_PROTOCOL_MOUSE:
+			if (pm_suspend_preferred_s2idle() &&
+			    pm_suspend_default_s2idle())
+				device_set_wakeup_enable(&dev->dev, 1);
+			break;
+		}
 	}
 
 	mutex_unlock(&usbhid->mutex);
-- 
2.34.1

Re: [PATCH v3 04/10] ata: ahci: Use `pm_suspend_preferred_s2idle`

[Damien Le Moal]

On 7/1/22 11:33, Mario Limonciello wrote:
> Drop the direct check from the FADT and use the helper instead.

Where is this helper defined ? Seeing that this is patch 4/10, I have no
context for this patch. Please send the whole series next time.

> 
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/ata/ahci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> index c1eca72b4575..3f79b732dd00 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -1611,7 +1611,7 @@ static void ahci_update_initial_lpm_policy(struct ata_port *ap,
>  
>  #ifdef CONFIG_ACPI
>  	if (policy > ATA_LPM_MED_POWER &&
> -	    (acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0)) {
> +	    pm_suspend_preferred_s2idle()) {
>  		if (hpriv->cap & HOST_CAP_PART)
>  			policy = ATA_LPM_MIN_POWER_WITH_PARTIAL;
>  		else if (hpriv->cap & HOST_CAP_SSC)


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v3 10/10] HID: usbhid: Set USB mice as s2idle wakeup resources

[Greg KH]

On Thu, Jun 30, 2022 at 09:33:28PM -0500, Mario Limonciello wrote:
> The USB HID transport layer doesn't configure mice for wakeup by default.
> Thus users can not wake system from s2idle via USB mouse. However, users
> can wake the same system from Modern Standby on Windows with the same USB
> mouse.
> 
> Microsoft documentation indicates that all USB mice and touchpads should
> be waking the system from Modern Standby.
> 
> Many people who have used Windows on a PC that supports Modern Standby
> have an expectation that s2idle wakeup sources should behave the same in
> Linux. For example if your PC is configured "dual-boot" and is used docked
> it's very common to wakeup by using a USB mouse connected to your dock in
> Windows. Switching to Linux this is not enabled by default and you'll
> need to manually turn it on or use a different wakeup source than you did
> for Windows.
> 
> Changes for wakeups have been made in other subsystems such as the PS/2
> keyboard driver which align how wakeup sources in Linux and Modern Standby
> in Windows behave. To align expectations from users on USB mice, make this
> behavior the same when the system is configured both by the OEM and the
> user to use s2idle in Linux.
> 
> This means that at a minimum supported mice will be able to wakeup by
> clicking a button. If the USB mouse is powered over the s2idle cycle (such
> as a wireless mouse with a battery) it's also possible that moving it
> may wake up the system.  This is HW dependent behavior.
> 
> If the user sets the system to use S3 instead of s2idle, or the OEM ships
> the system defaulting to S3, this behavior will not be turned on by
> default.
> 
> Users who have a modern laptop that supports s2idle and use s2idle but
> prefer the previous Linux kernel behavior can turn this off via a udev
> rule.
> 
> Link: https://docs.microsoft.com/en-us/windows-hardware/design/device-experiences/modern-standby-wake-sources#input-devices-1
> Link: https://lore.kernel.org/linux-usb/20220404214557.3329796-1-richard.gong@amd.com/
> Suggested-by: Richard Gong <richard.gong@amd.com>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
> More people keep coming to us confused that they couldn't wake a Linux system
> up from sleep using a mouse, so this patch is being revived.
> 
> Microsoft documentation doesn't indicate any allowlist for this behavior, and
> they actually prescribe it for all USB mice and touchpads.

Note, this is very different than older versions of Windows.  Any idea
when this behavior and prescription changed?

Also, how are you going to handle the differences between "internal" and
"external" USB mice and touchpads?  The microsoft link above only
references external USB and Bluetooth devices, not internal ones.

> 
> changes from v2->v3:
>  * Use `pm_suspend_preferred_s2idle`
>  * Drop now unnecessary acpi.h header inclusion
>  * Update commit message
>  * Adjust comments from v2 per thread
> 
> changes from v1->v2:
>  * Resubmit by Mario
>  * Update commit message
>  * Only activate on systems configured by user and OEM for using s2idle
> ---
>  drivers/hid/usbhid/hid-core.c | 37 ++++++++++++++++++++++++-----------
>  1 file changed, 26 insertions(+), 11 deletions(-)

Where are patches 1-9 of this series?

confused,

greg k-h

Re: [PATCH v3 10/10] HID: usbhid: Set USB mice as s2idle wakeup resources

[Greg KH]

On Thu, Jun 30, 2022 at 09:33:28PM -0500, Mario Limonciello wrote:
> The USB HID transport layer doesn't configure mice for wakeup by default.
> Thus users can not wake system from s2idle via USB mouse. However, users
> can wake the same system from Modern Standby on Windows with the same USB
> mouse.
> 
> Microsoft documentation indicates that all USB mice and touchpads should
> be waking the system from Modern Standby.

As I said before, their documentation indicates that all _EXTERNAL_ mice
and touchpads.  You forgot about internally connected mice and touchpads
here, you wouldn't want them to wake up a device asleep, right?

thanks,

greg k-h

Re: [PATCH v3 10/10] HID: usbhid: Set USB mice as s2idle wakeup resources

[kernel test robot]

Hi Mario,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on rafael-pm/linux-next]
[also build test ERROR on drm-misc/drm-misc-next hid/for-next linus/master v5.19-rc4 next-20220701]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Mario-Limonciello/PM-suspend-Introduce-pm_suspend_preferred_s2idle/20220701-103534
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
config: i386-randconfig-a002 (https://download.01.org/0day-ci/archive/20220701/202207011931.a4oqLMtN-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project a9119143a2d1f4d0d0bc1fe0d819e5351b4e0deb)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/54a1cce9cd825e0570d307b44a695f04bba77fd2
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Mario-Limonciello/PM-suspend-Introduce-pm_suspend_preferred_s2idle/20220701-103534
        git checkout 54a1cce9cd825e0570d307b44a695f04bba77fd2
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/hid/usbhid/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/hid/usbhid/hid-core.c:1200:8: error: call to undeclared function 'pm_suspend_preferred_s2idle'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
                           if (pm_suspend_preferred_s2idle() &&
                               ^
   drivers/hid/usbhid/hid-core.c:1200:8: note: did you mean 'pm_suspend_default_s2idle'?
   include/linux/suspend.h:343:20: note: 'pm_suspend_default_s2idle' declared here
   static inline bool pm_suspend_default_s2idle(void) { return false; }
                      ^
   1 error generated.


vim +/pm_suspend_preferred_s2idle +1200 drivers/hid/usbhid/hid-core.c

  1060	
  1061	static int usbhid_start(struct hid_device *hid)
  1062	{
  1063		struct usb_interface *intf = to_usb_interface(hid->dev.parent);
  1064		struct usb_host_interface *interface = intf->cur_altsetting;
  1065		struct usb_device *dev = interface_to_usbdev(intf);
  1066		struct usbhid_device *usbhid = hid->driver_data;
  1067		unsigned int n, insize = 0;
  1068		int ret;
  1069	
  1070		mutex_lock(&usbhid->mutex);
  1071	
  1072		clear_bit(HID_DISCONNECTED, &usbhid->iofl);
  1073	
  1074		usbhid->bufsize = HID_MIN_BUFFER_SIZE;
  1075		hid_find_max_report(hid, HID_INPUT_REPORT, &usbhid->bufsize);
  1076		hid_find_max_report(hid, HID_OUTPUT_REPORT, &usbhid->bufsize);
  1077		hid_find_max_report(hid, HID_FEATURE_REPORT, &usbhid->bufsize);
  1078	
  1079		if (usbhid->bufsize > HID_MAX_BUFFER_SIZE)
  1080			usbhid->bufsize = HID_MAX_BUFFER_SIZE;
  1081	
  1082		hid_find_max_report(hid, HID_INPUT_REPORT, &insize);
  1083	
  1084		if (insize > HID_MAX_BUFFER_SIZE)
  1085			insize = HID_MAX_BUFFER_SIZE;
  1086	
  1087		if (hid_alloc_buffers(dev, hid)) {
  1088			ret = -ENOMEM;
  1089			goto fail;
  1090		}
  1091	
  1092		for (n = 0; n < interface->desc.bNumEndpoints; n++) {
  1093			struct usb_endpoint_descriptor *endpoint;
  1094			int pipe;
  1095			int interval;
  1096	
  1097			endpoint = &interface->endpoint[n].desc;
  1098			if (!usb_endpoint_xfer_int(endpoint))
  1099				continue;
  1100	
  1101			interval = endpoint->bInterval;
  1102	
  1103			/* Some vendors give fullspeed interval on highspeed devides */
  1104			if (hid->quirks & HID_QUIRK_FULLSPEED_INTERVAL &&
  1105			    dev->speed == USB_SPEED_HIGH) {
  1106				interval = fls(endpoint->bInterval*8);
  1107				pr_info("%s: Fixing fullspeed to highspeed interval: %d -> %d\n",
  1108					hid->name, endpoint->bInterval, interval);
  1109			}
  1110	
  1111			/* Change the polling interval of mice, joysticks
  1112			 * and keyboards.
  1113			 */
  1114			switch (hid->collection->usage) {
  1115			case HID_GD_MOUSE:
  1116				if (hid_mousepoll_interval > 0)
  1117					interval = hid_mousepoll_interval;
  1118				break;
  1119			case HID_GD_JOYSTICK:
  1120				if (hid_jspoll_interval > 0)
  1121					interval = hid_jspoll_interval;
  1122				break;
  1123			case HID_GD_KEYBOARD:
  1124				if (hid_kbpoll_interval > 0)
  1125					interval = hid_kbpoll_interval;
  1126				break;
  1127			}
  1128	
  1129			ret = -ENOMEM;
  1130			if (usb_endpoint_dir_in(endpoint)) {
  1131				if (usbhid->urbin)
  1132					continue;
  1133				if (!(usbhid->urbin = usb_alloc_urb(0, GFP_KERNEL)))
  1134					goto fail;
  1135				pipe = usb_rcvintpipe(dev, endpoint->bEndpointAddress);
  1136				usb_fill_int_urb(usbhid->urbin, dev, pipe, usbhid->inbuf, insize,
  1137						 hid_irq_in, hid, interval);
  1138				usbhid->urbin->transfer_dma = usbhid->inbuf_dma;
  1139				usbhid->urbin->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
  1140			} else {
  1141				if (usbhid->urbout)
  1142					continue;
  1143				if (!(usbhid->urbout = usb_alloc_urb(0, GFP_KERNEL)))
  1144					goto fail;
  1145				pipe = usb_sndintpipe(dev, endpoint->bEndpointAddress);
  1146				usb_fill_int_urb(usbhid->urbout, dev, pipe, usbhid->outbuf, 0,
  1147						 hid_irq_out, hid, interval);
  1148				usbhid->urbout->transfer_dma = usbhid->outbuf_dma;
  1149				usbhid->urbout->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
  1150			}
  1151		}
  1152	
  1153		usbhid->urbctrl = usb_alloc_urb(0, GFP_KERNEL);
  1154		if (!usbhid->urbctrl) {
  1155			ret = -ENOMEM;
  1156			goto fail;
  1157		}
  1158	
  1159		usb_fill_control_urb(usbhid->urbctrl, dev, 0, (void *) usbhid->cr,
  1160				     usbhid->ctrlbuf, 1, hid_ctrl, hid);
  1161		usbhid->urbctrl->transfer_dma = usbhid->ctrlbuf_dma;
  1162		usbhid->urbctrl->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
  1163	
  1164		set_bit(HID_STARTED, &usbhid->iofl);
  1165	
  1166		if (hid->quirks & HID_QUIRK_ALWAYS_POLL) {
  1167			ret = usb_autopm_get_interface(usbhid->intf);
  1168			if (ret)
  1169				goto fail;
  1170			set_bit(HID_IN_POLLING, &usbhid->iofl);
  1171			usbhid->intf->needs_remote_wakeup = 1;
  1172			ret = hid_start_in(hid);
  1173			if (ret) {
  1174				dev_err(&hid->dev,
  1175					"failed to start in urb: %d\n", ret);
  1176			}
  1177			usb_autopm_put_interface(usbhid->intf);
  1178		}
  1179	
  1180		if (interface->desc.bInterfaceSubClass == USB_INTERFACE_SUBCLASS_BOOT) {
  1181			switch (interface->desc.bInterfaceProtocol) {
  1182			/* Some keyboards don't work until their LEDs have been set.
  1183			 * Since BIOSes do set the LEDs, it must be safe for any device
  1184			 * that supports the keyboard boot protocol.
  1185			 * In addition, enable remote wakeup by default for all keyboard
  1186			 * devices supporting the boot protocol.
  1187			 */
  1188			case USB_INTERFACE_PROTOCOL_KEYBOARD:
  1189				usbhid_set_leds(hid);
  1190				device_set_wakeup_enable(&dev->dev, 1);
  1191				break;
  1192			/*
  1193			 * Windows configures USB mice to be a wakeup source from Modern
  1194			 * Standby, and users have expectations that s2idle wakeup sources
  1195			 * behave the same.  Thus setup remote wakeup by default for mice
  1196			 * supporting boot protocol if the system supports s2idle and the user
  1197			 * has not disabled it on the kernel command line.
  1198			 */
  1199			case USB_INTERFACE_PROTOCOL_MOUSE:
> 1200				if (pm_suspend_preferred_s2idle() &&
  1201				    pm_suspend_default_s2idle())
  1202					device_set_wakeup_enable(&dev->dev, 1);
  1203				break;
  1204			}
  1205		}
  1206	
  1207		mutex_unlock(&usbhid->mutex);
  1208		return 0;
  1209	
  1210	fail:
  1211		usb_free_urb(usbhid->urbin);
  1212		usb_free_urb(usbhid->urbout);
  1213		usb_free_urb(usbhid->urbctrl);
  1214		usbhid->urbin = NULL;
  1215		usbhid->urbout = NULL;
  1216		usbhid->urbctrl = NULL;
  1217		hid_free_buffers(dev, hid);
  1218		mutex_unlock(&usbhid->mutex);
  1219		return ret;
  1220	}
  1221	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH v3 05/10] rtc: cmos: Use `pm_suspend_preferred_s2idle`

[kernel test robot]

Hi Mario,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on rafael-pm/linux-next]
[also build test ERROR on drm-misc/drm-misc-next hid/for-next linus/master v5.19-rc4 next-20220701]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Mario-Limonciello/PM-suspend-Introduce-pm_suspend_preferred_s2idle/20220701-103534
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
config: i386-randconfig-a001 (https://download.01.org/0day-ci/archive/20220701/202207011940.s0fsRy23-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/f5c9ad12802ecfaf2fc73e35ee9b563d7283b049
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Mario-Limonciello/PM-suspend-Introduce-pm_suspend_preferred_s2idle/20220701-103534
        git checkout f5c9ad12802ecfaf2fc73e35ee9b563d7283b049
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/rtc/rtc-cmos.c: In function 'use_acpi_alarm_quirks':
>> drivers/rtc/rtc-cmos.c:1264:14: error: implicit declaration of function 'pm_suspend_preferred_s2idle'; did you mean 'pm_suspend_default_s2idle'? [-Werror=implicit-function-declaration]
    1264 |         if (!pm_suspend_preferred_s2idle())
         |              ^~~~~~~~~~~~~~~~~~~~~~~~~~~
         |              pm_suspend_default_s2idle
   cc1: some warnings being treated as errors


vim +1264 drivers/rtc/rtc-cmos.c

  1256	
  1257	#ifdef CONFIG_X86
  1258	/* Enable use_acpi_alarm mode for Intel platforms no earlier than 2015 */
  1259	static void use_acpi_alarm_quirks(void)
  1260	{
  1261		if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL)
  1262			return;
  1263	
> 1264		if (!pm_suspend_preferred_s2idle())
  1265			return;
  1266	
  1267		if (!is_hpet_enabled())
  1268			return;
  1269	
  1270		if (dmi_get_bios_year() < 2015)
  1271			return;
  1272	
  1273		use_acpi_alarm = true;
  1274	}
  1275	#else
  1276	static inline void use_acpi_alarm_quirks(void) { }
  1277	#endif
  1278	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH v3 03/10] ACPI: LPIT: Use new `pm_suspend_preferred_s2idle`

[kernel test robot]

Hi Mario,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on rafael-pm/linux-next]
[also build test ERROR on drm-misc/drm-misc-next hid/for-next linus/master v5.19-rc4 next-20220701]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Mario-Limonciello/PM-suspend-Introduce-pm_suspend_preferred_s2idle/20220701-103534
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
config: x86_64-randconfig-a004 (https://download.01.org/0day-ci/archive/20220701/202207012033.3AjPSdQe-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/9603006925839aeb40b8a65adc7be87f4e89813f
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Mario-Limonciello/PM-suspend-Introduce-pm_suspend_preferred_s2idle/20220701-103534
        git checkout 9603006925839aeb40b8a65adc7be87f4e89813f
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/acpi/acpi_lpit.c: In function 'lpit_update_residency':
>> drivers/acpi/acpi_lpit.c:113:22: error: implicit declaration of function 'pm_suspend_preferred_s2idle'; did you mean 'pm_suspend_default_s2idle'? [-Werror=implicit-function-declaration]
     113 |                 if (!pm_suspend_preferred_s2idle())
         |                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~
         |                      pm_suspend_default_s2idle
   drivers/acpi/acpi_lpit.c: At top level:
   drivers/acpi/acpi_lpit.c:149:6: warning: no previous prototype for 'acpi_init_lpit' [-Wmissing-prototypes]
     149 | void acpi_init_lpit(void)
         |      ^~~~~~~~~~~~~~
   cc1: some warnings being treated as errors


vim +113 drivers/acpi/acpi_lpit.c

    97	
    98	static void lpit_update_residency(struct lpit_residency_info *info,
    99					 struct acpi_lpit_native *lpit_native)
   100	{
   101		info->frequency = lpit_native->counter_frequency ?
   102					lpit_native->counter_frequency : tsc_khz * 1000;
   103		if (!info->frequency)
   104			info->frequency = 1;
   105	
   106		info->gaddr = lpit_native->residency_counter;
   107		if (info->gaddr.space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) {
   108			info->iomem_addr = ioremap(info->gaddr.address,
   109							   info->gaddr.bit_width / 8);
   110			if (!info->iomem_addr)
   111				return;
   112	
 > 113			if (!pm_suspend_preferred_s2idle())
   114				return;
   115	
   116			/* Silently fail, if cpuidle attribute group is not present */
   117			sysfs_add_file_to_group(&cpu_subsys.dev_root->kobj,
   118						&dev_attr_low_power_idle_system_residency_us.attr,
   119						"cpuidle");
   120		} else if (info->gaddr.space_id == ACPI_ADR_SPACE_FIXED_HARDWARE) {
   121			if (!pm_suspend_preferred_s2idle())
   122				return;
   123	
   124			/* Silently fail, if cpuidle attribute group is not present */
   125			sysfs_add_file_to_group(&cpu_subsys.dev_root->kobj,
   126						&dev_attr_low_power_idle_cpu_residency_us.attr,
   127						"cpuidle");
   128		}
   129	}
   130	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH v3 10/10] HID: usbhid: Set USB mice as s2idle wakeup resources

[Mario Limonciello]

(FYI - combined both of Greg's responses in this message rather than 
responding to them separately)

 > Note, this is very different than older versions of Windows.  Any idea
when this behavior and prescription changed?

Older versions of Windows also didn't support Modern Standby, so I would 
hypothesize it's either Windows 8 or Windows 10 that changed it.

 > Where are patches 1-9 of this series?

Patch 1 was based on your comment in v2 that there was no helper (it 
introduces a helper).

Patches 2-9 are just changes across the kernel to use this helper.

The entire series was submitted to LKML, but individual patches only to 
the relevant maintainers for the subsystem.  If there is a v4 patch I'll 
CC everyone on everything.

Here is a lore link to the whole series:
https://lore.kernel.org/lkml/20220701023328.2783-1-mario.limonciello@amd.com/

On 7/1/22 01:58, Greg KH wrote:
> On Thu, Jun 30, 2022 at 09:33:28PM -0500, Mario Limonciello wrote:
>> The USB HID transport layer doesn't configure mice for wakeup by default.
>> Thus users can not wake system from s2idle via USB mouse. However, users
>> can wake the same system from Modern Standby on Windows with the same USB
>> mouse.
>>
>> Microsoft documentation indicates that all USB mice and touchpads should
>> be waking the system from Modern Standby.
> 
> As I said before, their documentation indicates that all _EXTERNAL_ mice
> and touchpads.  You forgot about internally connected mice and touchpads
> here, you wouldn't want them to wake up a device asleep, right?
> 

Is this actually a thing?  I've been in the PC industry a while but 
never seen a design that supported Modern Standby/s2idle that opted to 
use USB to connect an "internal" touchpad/mouse.  Microsoft's own 
documentation in the same link advises against it at least too.

"We recommend using HIDI2C for input peripherals whenever possible for 
better power efficiency, but this is not a requirement".

But in any case - wakeup from this type of device is a grey area.  They 
require that internal touchpads connected to I2C be a wake source and 
also USB touchpads.
There is nothing on that page prescribing the behavior for an "internal" 
USB touchpad.

Reading between the lines given the intent, I would argue if such a 
design is created or exists internal USB mice and touchpads should also 
be a wake up.

Re: [PATCH v3 04/10] ata: ahci: Use `pm_suspend_preferred_s2idle`

[Mario Limonciello]

On 7/1/22 00:06, Damien Le Moal wrote:
> On 7/1/22 11:33, Mario Limonciello wrote:
>> Drop the direct check from the FADT and use the helper instead.
> 
> Where is this helper defined ? Seeing that this is patch 4/10, I have no
> context for this patch. Please send the whole series next time.
> 

My apologies; I should have also included you at least on 1/10, but was 
trying to save your inbox for the other 8 patches that were not relevant 
to you.

The whole series is on LKML here:
https://lore.kernel.org/lkml/20220701023328.2783-1-mario.limonciello@amd.com/

If there's a v4 I'll include you on 1/10 (unless you also want to be on 
the others too).

>>
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> ---
>>   drivers/ata/ahci.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
>> index c1eca72b4575..3f79b732dd00 100644
>> --- a/drivers/ata/ahci.c
>> +++ b/drivers/ata/ahci.c
>> @@ -1611,7 +1611,7 @@ static void ahci_update_initial_lpm_policy(struct ata_port *ap,
>>   
>>   #ifdef CONFIG_ACPI
>>   	if (policy > ATA_LPM_MED_POWER &&
>> -	    (acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0)) {
>> +	    pm_suspend_preferred_s2idle()) {
>>   		if (hpriv->cap & HOST_CAP_PART)
>>   			policy = ATA_LPM_MIN_POWER_WITH_PARTIAL;
>>   		else if (hpriv->cap & HOST_CAP_SSC)
> 
> 

Re: [PATCH v3 04/10] ata: ahci: Use `pm_suspend_preferred_s2idle`

[kernel test robot]

Hi Mario,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on rafael-pm/linux-next]
[also build test ERROR on drm-misc/drm-misc-next hid/for-next linus/master v5.19-rc4 next-20220701]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Mario-Limonciello/PM-suspend-Introduce-pm_suspend_preferred_s2idle/20220701-103534
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
config: i386-randconfig-a011 (https://download.01.org/0day-ci/archive/20220702/202207021730.qxUxPlRq-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project bcd153485ebf07fe79e2b843ed5f1cb74997df1b)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/ceb90ecfef1f252424a29b31a547cfc80c8f5135
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Mario-Limonciello/PM-suspend-Introduce-pm_suspend_preferred_s2idle/20220701-103534
        git checkout ceb90ecfef1f252424a29b31a547cfc80c8f5135
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/ata/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/ata/ahci.c:1614:6: error: call to undeclared function 'pm_suspend_preferred_s2idle'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
               pm_suspend_preferred_s2idle()) {
               ^
   drivers/ata/ahci.c:1614:6: note: did you mean 'pm_suspend_default_s2idle'?
   include/linux/suspend.h:343:20: note: 'pm_suspend_default_s2idle' declared here
   static inline bool pm_suspend_default_s2idle(void) { return false; }
                      ^
   1 error generated.


vim +/pm_suspend_preferred_s2idle +1614 drivers/ata/ahci.c

  1595	
  1596	static void ahci_update_initial_lpm_policy(struct ata_port *ap,
  1597						   struct ahci_host_priv *hpriv)
  1598	{
  1599		int policy = CONFIG_SATA_MOBILE_LPM_POLICY;
  1600	
  1601	
  1602		/* Ignore processing for chipsets that don't use policy */
  1603		if (!(hpriv->flags & AHCI_HFLAG_USE_LPM_POLICY))
  1604			return;
  1605	
  1606		/* user modified policy via module param */
  1607		if (mobile_lpm_policy != -1) {
  1608			policy = mobile_lpm_policy;
  1609			goto update_policy;
  1610		}
  1611	
  1612	#ifdef CONFIG_ACPI
  1613		if (policy > ATA_LPM_MED_POWER &&
> 1614		    pm_suspend_preferred_s2idle()) {
  1615			if (hpriv->cap & HOST_CAP_PART)
  1616				policy = ATA_LPM_MIN_POWER_WITH_PARTIAL;
  1617			else if (hpriv->cap & HOST_CAP_SSC)
  1618				policy = ATA_LPM_MIN_POWER;
  1619		}
  1620	#endif
  1621	
  1622	update_policy:
  1623		if (policy >= ATA_LPM_UNKNOWN && policy <= ATA_LPM_MIN_POWER)
  1624			ap->target_lpm_policy = policy;
  1625	}
  1626	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

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@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
>

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

[Limonciello, Mario]

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
>>

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@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.

Re: [PATCH v3 02/10] ACPI / x86: Use new `pm_suspend_preferred_s2idle`

[Rafael J. Wysocki]

On Fri, Jul 1, 2022 at 4:33 AM Mario Limonciello
<mario.limonciello@amd.com> wrote:
>
> Drop the direct check from the FADT and use the helper instead.
>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/acpi/x86/s2idle.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/x86/s2idle.c b/drivers/acpi/x86/s2idle.c
> index 2963229062f8..b1483d5092c1 100644
> --- a/drivers/acpi/x86/s2idle.c
> +++ b/drivers/acpi/x86/s2idle.c
> @@ -369,7 +369,7 @@ static int lps0_device_attach(struct acpi_device *adev,
>         if (lps0_device_handle)
>                 return 0;
>
> -       if (!(acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0))
> +       if (!pm_suspend_preferred_s2idle())

This needs to be checked in a different place in this function.

Let me cut a patch for that.

>                 return 0;
>
>         if (acpi_s2idle_vendor_amd()) {
> --