Re: [PATCH v2] HID: usbhid: set mouse as a wakeup resource

Greg KH
2022-06-16T21:28:17Z

On Thu, Jun 16, 2022 at 01:31:42PM -0500, Mario Limonciello wrote:
> The USB HID transport layer doesn't set mice for wakeup by default so users
> can not wake system from s2idle using wired USB mouse. However, users can
> wake the same system from Modern Standby on Windows with the same wired
> USB mouse.
>
> Microsoft documentation indicates that all USB mice and touchpads should
> be waking the system from Modern Standby. To align expectations from users
> make this behavior the same when the system is configured by the OEM and
> the user to use s2idle in Linux.
>
> 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.

How many different devices did you test this on?

>
> Microsoft documentation doesn't indicate any allowlist for this behavior, and
> they actually prescribe it for all USB mice and touchpads.
> 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 | 36 ++++++++++++++++++++++++-----------
> 1 file changed, 25 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> index 4490e2f7252a..3a1214ecec49 100644
> --- a/drivers/hid/usbhid/hid-core.c
> +++ b/drivers/hid/usbhid/hid-core.c
> @@ -12,6 +12,7 @@
> /*
> */
>
> +#include <linux/acpi.h>
> #include <linux/module.h>
> #include <linux/slab.h>
> #include <linux/init.h>
> @@ -26,6 +27,7 @@
> #include <linux/wait.h>
> #include <linux/workqueue.h>
> #include <linux/string.h>
> +#include <linux/suspend.h>
>
> #include <linux/usb.h>
>
> @@ -1176,17 +1178,29 @@ 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;
> +#ifdef CONFIG_ACPI

Why a #ifdef?

> + /* Setup remote wakeup by default for mice supporting boot
> + * protocol if the system supports s2idle
> + */
> + case USB_INTERFACE_PROTOCOL_MOUSE:
> + if (acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0 &&

That seems odd, there's no acpi/pm call for this instead?

> + pm_suspend_default_s2idle())
> + device_set_wakeup_enable(&dev->dev, 1);
> + break;
> +#endif

thanks,

greg k-h