| Message ID | 20220701023328.2783-10-mario.limonciello@amd.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | [v3,01/10] PM: suspend: Introduce `pm_suspend_preferred_s2idle` | expand |
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
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
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
(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.
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);
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(-)