Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 16 Jun 2022 23:28:05 +0200
From:   Greg KH <gregkh@linuxfoundation.org>
To:     Mario Limonciello <mario.limonciello@amd.com>
Cc:     Jiri Kosina <jikos@kernel.org>,
        Benjamin Tissoires <benjamin.tissoires@redhat.com>,
        "open list:USB HID/HIDBP DRIVERS [USB KEYBOARDS, MICE, REM..." 
        <linux-usb@vger.kernel.org>,
        "open list:HID CORE LAYER" <linux-input@vger.kernel.org>,
        open list <linux-kernel@vger.kernel.org>,
        Richard Gong <richard.gong@amd.com>
Subject: Re: [PATCH v2] HID: usbhid: set mouse as a wakeup resource
Message-ID: <YqugZQiDu35Y8lTu@kroah.com>
References: <20220616183142.14472-1-mario.limonciello@amd.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220616183142.14472-1-mario.limonciello@amd.com>
Precedence: bulk

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