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

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