[PATCH] HID: usbhid: enable remote wakeup for mice

[PATCH] HID: usbhid: enable remote wakeup for mice

[Benson Leung]

This patch enables remote wakeup by default on USB mouse
devices. This only covers USB mice that support
the boot protocol. See commit 3d61510f for the
equivalent patch for USB keyboard devices.

Signed-off-by: Benson Leung <bleung@chromium.org>
---
 drivers/hid/usbhid/hid-core.c |    8 ++++----
 drivers/hid/usbhid/usbmouse.c |    1 +
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index b403fce..02a66f1 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -1073,10 +1073,10 @@ static int usbhid_start(struct hid_device *hid)
 	 * 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);
+	if (interface->desc.bInterfaceSubClass == USB_INTERFACE_SUBCLASS_BOOT) {
+		if (interface->desc.bInterfaceProtocol ==
+				USB_INTERFACE_PROTOCOL_KEYBOARD)
+			usbhid_set_leds(hid);
 		device_set_wakeup_enable(&dev->dev, 1);
 	}
 	return 0;
diff --git a/drivers/hid/usbhid/usbmouse.c b/drivers/hid/usbhid/usbmouse.c
index 79b2bf8..6ca3321 100644
--- a/drivers/hid/usbhid/usbmouse.c
+++ b/drivers/hid/usbhid/usbmouse.c
@@ -200,6 +200,7 @@ static int usb_mouse_probe(struct usb_interface *intf, const struct usb_device_i
 		goto fail3;
 
 	usb_set_intfdata(intf, mouse);
+	device_set_wakeup_enable(&dev->dev, 1);
 	return 0;
 
 fail3:	
-- 
1.7.1

Re: [PATCH] HID: usbhid: enable remote wakeup for mice

[Oliver Neukum]

Am Freitag, 14. Oktober 2011, 09:54:48 schrieb Benson Leung:
> This patch enables remote wakeup by default on USB mouse
> devices. This only covers USB mice that support
> the boot protocol. See commit 3d61510f for the
> equivalent patch for USB keyboard devices.

This doesn't loo like a very good idea.
Firstly your patch introduces gratious change in the hid driver.
Secondly you are changing the usbmouse driver, which is to
be used under special circumstances only, only.
Thirdly a fundamental change like that needs a commit entry
explaining the reason for the change.

	Regards
		Oliver

Re: [PATCH] HID: usbhid: enable remote wakeup for mice

[Alan Stern]

On Fri, 14 Oct 2011, Oliver Neukum wrote:

> Am Freitag, 14. Oktober 2011, 09:54:48 schrieb Benson Leung:
> > This patch enables remote wakeup by default on USB mouse
> > devices. This only covers USB mice that support
> > the boot protocol. See commit 3d61510f for the
> > equivalent patch for USB keyboard devices.
> 
> This doesn't loo like a very good idea.
> Firstly your patch introduces gratious change in the hid driver.
> Secondly you are changing the usbmouse driver, which is to
> be used under special circumstances only, only.
> Thirdly a fundamental change like that needs a commit entry
> explaining the reason for the change.

In addition, unlike keyboards, a lot of mice don't work very well when 
they are suspended.  For example, many won't wake up when they are 
moved (especially optical mice) and some don't even wake up when a 
button is pressed.

This patch definitely should not be merged.

Alan Stern

Re: [PATCH] HID: usbhid: enable remote wakeup for mice

[Jiri Kosina]

On Fri, 14 Oct 2011, Alan Stern wrote:

> In addition, unlike keyboards, a lot of mice don't work very well when 
> they are suspended.  For example, many won't wake up when they are 
> moved (especially optical mice) and some don't even wake up when a 
> button is pressed.

To stress it even more, I have tried with various types and brands of 
optical mouse, and I haven't encountered a single one that would wake up 
when moved.

> This patch definitely should not be merged.

I am not planning to do that.

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] HID: usbhid: enable remote wakeup for mice

[Benson Leung]

Thanks for the prompt response.

My initial reasoning was to set as default the wakeup behavior so a
mouse button press would wake the system. I am well aware that most if
not all mice don't support wake on move, but at least wake on button
press would have been more desirable than doing nothing.

Anyway, I'll solve this problem on my system by writing to wakeup
property on device attach. Consider my change abandoned.

Thanks,
Benson

On Fri, Oct 14, 2011 at 7:00 AM, Jiri Kosina <jkosina@suse.cz> wrote:
> On Fri, 14 Oct 2011, Alan Stern wrote:
>
>> In addition, unlike keyboards, a lot of mice don't work very well when
>> they are suspended.  For example, many won't wake up when they are
>> moved (especially optical mice) and some don't even wake up when a
>> button is pressed.
>
> To stress it even more, I have tried with various types and brands of
> optical mouse, and I haven't encountered a single one that would wake up
> when moved.
>
>> This patch definitely should not be merged.
>
> I am not planning to do that.
>
> Thanks,
>
> --
> Jiri Kosina
> SUSE Labs
>



-- 
Benson Leung
Software Engineer, Chrom* OS
bleung@chromium.org
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] HID: usbhid: enable remote wakeup for mice

[Alan Stern]

On Fri, 14 Oct 2011, Benson Leung wrote:

> Thanks for the prompt response.
> 
> My initial reasoning was to set as default the wakeup behavior so a
> mouse button press would wake the system. I am well aware that most if
> not all mice don't support wake on move, but at least wake on button
> press would have been more desirable than doing nothing.

I'm sorry, I misunderstood the purpose of the original patch.  Probably 
read it too quickly -- I thought you were enabling autosuspend rather 
than wakeup.

Enabling wakeup by default has its own problems.  Some mice do send
wakeup requests when they are moved.  You wouldn't want a stray motion
or unintentional touch to wake up a sleeping system.  Or at least, a
lot of people wouldn't want that.

> Anyway, I'll solve this problem on my system by writing to wakeup
> property on device attach. Consider my change abandoned.

That seems like the best approach.

Alan Stern

Re: [PATCH] HID: usbhid: enable remote wakeup for mice

[Oliver Neukum]

Am Freitag, 14. Oktober 2011, 17:09:32 schrieb Alan Stern:
> > Anyway, I'll solve this problem on my system by writing to wakeup
> > property on device attach. Consider my change abandoned.
> 
> That seems like the best approach.

Do you think this could be added to the USB configuration tool I wrote
last week?

In the longer run, we might consider that this properties may also need
to be changed based on AC vs. DC or the battery's charge level.

	Regards
		Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] HID: usbhid: enable remote wakeup for mice

[Alan Stern]

On Sat, 15 Oct 2011, Oliver Neukum wrote:

> Am Freitag, 14. Oktober 2011, 17:09:32 schrieb Alan Stern:
> > > Anyway, I'll solve this problem on my system by writing to wakeup
> > > property on device attach. Consider my change abandoned.
> > 
> > That seems like the best approach.
> 
> Do you think this could be added to the USB configuration tool I wrote
> last week?

I don't see why not.  It's exactly the sort of thing that should be 
handled on a per-user basis.

> In the longer run, we might consider that this properties may also need
> to be changed based on AC vs. DC or the battery's charge level.

Is that really true?  I'm not sure -- in general people want their 
computers to be consistent.  In particular, they want to conserve power 
regardless of the power source.

However, you could add such a mechanism to the program in case some 
people do want to use it.

Alan Stern

[PATCH] HID: usbhid: enable remote wakeup for mice

[Michael Wu]

This patch fixes a problem that USB mouse can't wake up the device that
enters standby.

At present, the kernel only checks whether certain USB manufacturers
support wake-up, which will easily cause inconvenience to the
development work of other manufacturers and add unnecessary work to the
maintenance of kernel.

The USB protocol supports judging whether a usb supports the wake-up
function, so it should be more reasonable to add a wake-up source by
directly checking the settings from the USB protocol.

There was a similar issue on the keyboard before, which was fixed by
this patch (3d61510f4eca), but now the problem happened on the mouse.
This patch uses a similar idea to fix this problem.

Signed-off-by: Michael Wu <michael@allwinnertech.com>
---
 drivers/hid/usbhid/hid-core.c | 8 ++++++++
 drivers/hid/usbhid/usbmouse.c | 1 +
 2 files changed, 9 insertions(+)

diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index be4c731aaa65..d3a6755cca09 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -1189,6 +1189,14 @@ static int usbhid_start(struct hid_device *hid)
 		device_set_wakeup_enable(&dev->dev, 1);
 	}
 
+	/**
+	 * NOTE: enable remote wakeup by default for all mouse devices
+	 * supporting the boot protocol.
+	 */
+	if (interface->desc.bInterfaceSubClass == USB_INTERFACE_SUBCLASS_BOOT &&
+	    interface->desc.bInterfaceProtocol == USB_INTERFACE_PROTOCOL_MOUSE)
+		device_set_wakeup_enable(&dev->dev, 1);
+
 	mutex_unlock(&usbhid->mutex);
 	return 0;
 
diff --git a/drivers/hid/usbhid/usbmouse.c b/drivers/hid/usbhid/usbmouse.c
index 3fd93c2e4f4a..2fbc3f49e420 100644
--- a/drivers/hid/usbhid/usbmouse.c
+++ b/drivers/hid/usbhid/usbmouse.c
@@ -188,6 +188,7 @@ static int usb_mouse_probe(struct usb_interface *intf, const struct usb_device_i
 		goto fail3;
 
 	usb_set_intfdata(intf, mouse);
+	device_set_wakeup_enable(&dev->dev, 1);
 	return 0;
 
 fail3:	
-- 
2.29.0

Re: [PATCH] HID: usbhid: enable remote wakeup for mice

[Greg KH]

On Wed, Feb 22, 2023 at 09:39:44AM +0800, Michael Wu wrote:
> This patch fixes a problem that USB mouse can't wake up the device that
> enters standby.

This not a problem, it is that way by design.

> At present, the kernel only checks whether certain USB manufacturers
> support wake-up, which will easily cause inconvenience to the
> development work of other manufacturers and add unnecessary work to the
> maintenance of kernel.
> 
> The USB protocol supports judging whether a usb supports the wake-up
> function, so it should be more reasonable to add a wake-up source by
> directly checking the settings from the USB protocol.

But you do not do that in this patch, why not?

> There was a similar issue on the keyboard before, which was fixed by
> this patch (3d61510f4eca), but now the problem happened on the mouse.
> This patch uses a similar idea to fix this problem.
> 
> Signed-off-by: Michael Wu <michael@allwinnertech.com>
> ---
>  drivers/hid/usbhid/hid-core.c | 8 ++++++++
>  drivers/hid/usbhid/usbmouse.c | 1 +
>  2 files changed, 9 insertions(+)
> 
> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> index be4c731aaa65..d3a6755cca09 100644
> --- a/drivers/hid/usbhid/hid-core.c
> +++ b/drivers/hid/usbhid/hid-core.c
> @@ -1189,6 +1189,14 @@ static int usbhid_start(struct hid_device *hid)
>  		device_set_wakeup_enable(&dev->dev, 1);
>  	}
>  
> +	/**
> +	 * NOTE: enable remote wakeup by default for all mouse devices
> +	 * supporting the boot protocol.
> +	 */
> +	if (interface->desc.bInterfaceSubClass == USB_INTERFACE_SUBCLASS_BOOT &&
> +	    interface->desc.bInterfaceProtocol == USB_INTERFACE_PROTOCOL_MOUSE)
> +		device_set_wakeup_enable(&dev->dev, 1);

Sorry, but we can not take this unless it is proven that this will work
properly for all of these devices.  Other operating systems do not do
this last I checked, so there will be problems.

thanks,

greg k-h

Re: [PATCH] HID: usbhid: enable remote wakeup for mice

[Sergei Shtylyov]

Hello!

On 2/22/23 4:39 AM, Michael Wu wrote:

> This patch fixes a problem that USB mouse can't wake up the device that
> enters standby.
> 
> At present, the kernel only checks whether certain USB manufacturers
> support wake-up, which will easily cause inconvenience to the
> development work of other manufacturers and add unnecessary work to the
> maintenance of kernel.
> 
> The USB protocol supports judging whether a usb supports the wake-up
> function, so it should be more reasonable to add a wake-up source by
> directly checking the settings from the USB protocol.
> 
> There was a similar issue on the keyboard before, which was fixed by
> this patch (3d61510f4eca), but now the problem happened on the mouse.
> This patch uses a similar idea to fix this problem.
> 
> Signed-off-by: Michael Wu <michael@allwinnertech.com>
> ---
>  drivers/hid/usbhid/hid-core.c | 8 ++++++++
>  drivers/hid/usbhid/usbmouse.c | 1 +
>  2 files changed, 9 insertions(+)
> 
> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> index be4c731aaa65..d3a6755cca09 100644
> --- a/drivers/hid/usbhid/hid-core.c
> +++ b/drivers/hid/usbhid/hid-core.c
> @@ -1189,6 +1189,14 @@ static int usbhid_start(struct hid_device *hid)
>  		device_set_wakeup_enable(&dev->dev, 1);
>  	}
>  
> +	/**

   The kernel-doc comments should not be used here in the function body.

> +	 * NOTE: enable remote wakeup by default for all mouse devices
> +	 * supporting the boot protocol.
> +	 */
> +	if (interface->desc.bInterfaceSubClass == USB_INTERFACE_SUBCLASS_BOOT &&
> +	    interface->desc.bInterfaceProtocol == USB_INTERFACE_PROTOCOL_MOUSE)
> +		device_set_wakeup_enable(&dev->dev, 1);
> +
>  	mutex_unlock(&usbhid->mutex);
>  	return 0;
>  
[...]

MBR, Sergey

Re: [PATCH] HID: usbhid: enable remote wakeup for mice

[Oliver Neukum]

On 22.02.23 02:39, Michael Wu wrote:
> This patch fixes a problem that USB mouse can't wake up the device that
> enters standby.

Hi,

I am afraid I need to ask you to be a bit more precise here.
Are you referring to USB mice being unable to wake up a system,
even if you so request via sysfs, or that they won't by default?

	Regards
		Oliver

Re: [PATCH] HID: usbhid: enable remote wakeup for mice

[Limonciello, Mario]

+Richard

On 2/22/2023 00:04, Greg KH wrote:
> On Wed, Feb 22, 2023 at 09:39:44AM +0800, Michael Wu wrote:
>> This patch fixes a problem that USB mouse can't wake up the device that
>> enters standby.
> 
> This not a problem, it is that way by design.
> 
>> At present, the kernel only checks whether certain USB manufacturers
>> support wake-up, which will easily cause inconvenience to the
>> development work of other manufacturers and add unnecessary work to the
>> maintenance of kernel.
>>
>> The USB protocol supports judging whether a usb supports the wake-up
>> function, so it should be more reasonable to add a wake-up source by
>> directly checking the settings from the USB protocol.
> 
> But you do not do that in this patch, why not?
> 
>> There was a similar issue on the keyboard before, which was fixed by
>> this patch (3d61510f4eca), but now the problem happened on the mouse.
>> This patch uses a similar idea to fix this problem.
>>
>> Signed-off-by: Michael Wu <michael@allwinnertech.com>
>> ---
>>   drivers/hid/usbhid/hid-core.c | 8 ++++++++
>>   drivers/hid/usbhid/usbmouse.c | 1 +
>>   2 files changed, 9 insertions(+)
>>
>> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
>> index be4c731aaa65..d3a6755cca09 100644
>> --- a/drivers/hid/usbhid/hid-core.c
>> +++ b/drivers/hid/usbhid/hid-core.c
>> @@ -1189,6 +1189,14 @@ static int usbhid_start(struct hid_device *hid)
>>   		device_set_wakeup_enable(&dev->dev, 1);
>>   	}
>>   
>> +	/**
>> +	 * NOTE: enable remote wakeup by default for all mouse devices
>> +	 * supporting the boot protocol.
>> +	 */
>> +	if (interface->desc.bInterfaceSubClass == USB_INTERFACE_SUBCLASS_BOOT &&
>> +	    interface->desc.bInterfaceProtocol == USB_INTERFACE_PROTOCOL_MOUSE)
>> +		device_set_wakeup_enable(&dev->dev, 1);
> 
> Sorry, but we can not take this unless it is proven that this will work
> properly for all of these devices.  Other operating systems do not do
> this last I checked, so there will be problems.
> 
> thanks,
> 
> greg k-h
> 

Richard and I both sent out relatively similar patches in the past, but 
they never went anywhere.
We did confirm that Windows does set a similar policy as well though, 
which is what prompted us to attempt this.

As there is more interest again maybe we can revive that discussion and 
merge together some ideas from the sets of patches.

Previous submissions:

v4:
https://lore.kernel.org/linux-usb/20220825045517.16791-1-mario.limonciello@amd.com/

v3:
https://lore.kernel.org/linux-usb/20220701023328.2783-10-mario.limonciello@amd.com/

v2:
https://lore.kernel.org/linux-usb/20220616183142.14472-1-mario.limonciello@amd.com/

v1:
https://lore.kernel.org/linux-usb/20220404214557.3329796-1-richard.gong@amd.com/

Re: [PATCH] HID: usbhid: enable remote wakeup for mice

[Michael Wu]

Dear Sergei:

On 2/22/2023 4:59 PM, Sergei Shtylyov wrote:
> Hello!
> 
> On 2/22/23 4:39 AM, Michael Wu wrote:
> 
>> This patch fixes a problem that USB mouse can't wake up the device that
>> enters standby.
>>
>> At present, the kernel only checks whether certain USB manufacturers
>> support wake-up, which will easily cause inconvenience to the
>> development work of other manufacturers and add unnecessary work to the
>> maintenance of kernel.
>>
>> The USB protocol supports judging whether a usb supports the wake-up
>> function, so it should be more reasonable to add a wake-up source by
>> directly checking the settings from the USB protocol.
>>
>> There was a similar issue on the keyboard before, which was fixed by
>> this patch (3d61510f4eca), but now the problem happened on the mouse.
>> This patch uses a similar idea to fix this problem.
>>
>> Signed-off-by: Michael Wu <michael@allwinnertech.com>
>> ---
>>   drivers/hid/usbhid/hid-core.c | 8 ++++++++
>>   drivers/hid/usbhid/usbmouse.c | 1 +
>>   2 files changed, 9 insertions(+)
>>
>> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
>> index be4c731aaa65..d3a6755cca09 100644
>> --- a/drivers/hid/usbhid/hid-core.c
>> +++ b/drivers/hid/usbhid/hid-core.c
>> @@ -1189,6 +1189,14 @@ static int usbhid_start(struct hid_device *hid)
>>   		device_set_wakeup_enable(&dev->dev, 1);
>>   	}
>>   
>> +	/**
> 

>     The kernel-doc comments should not be used here in the function body.

We will remove the NOTE comments.

> 
>> +	 * NOTE: enable remote wakeup by default for all mouse devices
>> +	 * supporting the boot protocol.
>> +	 */
>> +	if (interface->desc.bInterfaceSubClass == USB_INTERFACE_SUBCLASS_BOOT &&
>> +	    interface->desc.bInterfaceProtocol == USB_INTERFACE_PROTOCOL_MOUSE)
>> +		device_set_wakeup_enable(&dev->dev, 1);
>> +
>>   	mutex_unlock(&usbhid->mutex);
>>   	return 0;
>>   
> [...]
> 
> MBR, Sergey

-- 
Regards,
Michael Wu

Re: [PATCH] HID: usbhid: enable remote wakeup for mice

[Michael Wu]

Dear Greg,

On 2/22/2023 2:04 PM, Greg KH wrote:
> On Wed, Feb 22, 2023 at 09:39:44AM +0800, Michael Wu wrote:
>> This patch fixes a problem that USB mouse can't wake up the device that
>> enters standby.
> 
> This not a problem, it is that way by design.

I got it, maybe it's a little problem to say that.

> 
>> At present, the kernel only checks whether certain USB manufacturers
>> support wake-up, which will easily cause inconvenience to the
>> development work of other manufacturers and add unnecessary work to the
>> maintenance of kernel.
>>
>> The USB protocol supports judging whether a usb supports the wake-up
>> function, so it should be more reasonable to add a wake-up source by
>> directly checking the settings from the USB protocol.
> 
> But you do not do that in this patch, why not?

I just want to explain the background of my patch, to prove we could use 
a similar way to avoid such a "disturbing" situation.
To reduce the influence, my patch enables remote wakeup for USB mouse 
devices refer to what keyboard do.

> 
>> There was a similar issue on the keyboard before, which was fixed by
>> this patch (3d61510f4eca), but now the problem happened on the mouse.
>> This patch uses a similar idea to fix this problem.
>>
>> Signed-off-by: Michael Wu <michael@allwinnertech.com>
>> ---
>>   drivers/hid/usbhid/hid-core.c | 8 ++++++++
>>   drivers/hid/usbhid/usbmouse.c | 1 +
>>   2 files changed, 9 insertions(+)
>>
>> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
>> index be4c731aaa65..d3a6755cca09 100644
>> --- a/drivers/hid/usbhid/hid-core.c
>> +++ b/drivers/hid/usbhid/hid-core.c
>> @@ -1189,6 +1189,14 @@ static int usbhid_start(struct hid_device *hid)
>>   		device_set_wakeup_enable(&dev->dev, 1);
>>   	}
>>   
>> +	/**
>> +	 * NOTE: enable remote wakeup by default for all mouse devices
>> +	 * supporting the boot protocol.
>> +	 */
>> +	if (interface->desc.bInterfaceSubClass == USB_INTERFACE_SUBCLASS_BOOT &&
>> +	    interface->desc.bInterfaceProtocol == USB_INTERFACE_PROTOCOL_MOUSE)
>> +		device_set_wakeup_enable(&dev->dev, 1);
> 
> Sorry, but we can not take this unless it is proven that this will work
> properly for all of these devices.  Other operating systems do not do
> this last I checked, so there will be problems.

As Mario Limonciello says, they has confirmed that the Microsoft Windows 
does set a similar policy as well. Can we talk about more in this topic: 
why does Linux not support it?
Of course, if you have other great idea, I will appreciate that if we 
can have some further discussion.

> 
> thanks,
> 
> greg k-h

-- 
Regards,
Michael Wu

Re: [PATCH] HID: usbhid: enable remote wakeup for mice

[Michael Wu]

Dear Oliver,

On 2/22/2023 5:34 PM, Oliver Neukum wrote:
> On 22.02.23 02:39, Michael Wu wrote:
>> This patch fixes a problem that USB mouse can't wake up the device that
>> enters standby.
> 
> Hi,
> 
> I am afraid I need to ask you to be a bit more precise here.
> Are you referring to USB mice being unable to wake up a system,
> even if you so request via sysfs, or that they won't by default?
> 
>      Regards
>          Oliver

Yes. I can't use any USB mouse which supports Remote Wakeup to wake up 
the system by default.
If I enable the wakeup node in /sys/bus/usb/devices which is disabled by 
default, the mouse can wakeup the system successfully.
Clearly, the only thing I can do is to register the wakeup source for 
USB Mouse devices.

-- 
Regards,
Michael Wu

Re: [PATCH] HID: usbhid: enable remote wakeup for mice

[Greg KH]

On Thu, Feb 23, 2023 at 07:18:12PM +0800, Michael Wu wrote:
> Dear Greg,
> 
> On 2/22/2023 2:04 PM, Greg KH wrote:
> > On Wed, Feb 22, 2023 at 09:39:44AM +0800, Michael Wu wrote:
> > > This patch fixes a problem that USB mouse can't wake up the device that
> > > enters standby.
> > 
> > This not a problem, it is that way by design.
> 
> I got it, maybe it's a little problem to say that.

It is.

> > > At present, the kernel only checks whether certain USB manufacturers
> > > support wake-up, which will easily cause inconvenience to the
> > > development work of other manufacturers and add unnecessary work to the
> > > maintenance of kernel.
> > > 
> > > The USB protocol supports judging whether a usb supports the wake-up
> > > function, so it should be more reasonable to add a wake-up source by
> > > directly checking the settings from the USB protocol.
> > 
> > But you do not do that in this patch, why not?
> 
> I just want to explain the background of my patch, to prove we could use a
> similar way to avoid such a "disturbing" situation.
> To reduce the influence, my patch enables remote wakeup for USB mouse
> devices refer to what keyboard do.

Keyboards are not mice :)

> > > There was a similar issue on the keyboard before, which was fixed by
> > > this patch (3d61510f4eca), but now the problem happened on the mouse.
> > > This patch uses a similar idea to fix this problem.
> > > 
> > > Signed-off-by: Michael Wu <michael@allwinnertech.com>
> > > ---
> > >   drivers/hid/usbhid/hid-core.c | 8 ++++++++
> > >   drivers/hid/usbhid/usbmouse.c | 1 +
> > >   2 files changed, 9 insertions(+)
> > > 
> > > diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> > > index be4c731aaa65..d3a6755cca09 100644
> > > --- a/drivers/hid/usbhid/hid-core.c
> > > +++ b/drivers/hid/usbhid/hid-core.c
> > > @@ -1189,6 +1189,14 @@ static int usbhid_start(struct hid_device *hid)
> > >   		device_set_wakeup_enable(&dev->dev, 1);
> > >   	}
> > > +	/**
> > > +	 * NOTE: enable remote wakeup by default for all mouse devices
> > > +	 * supporting the boot protocol.
> > > +	 */
> > > +	if (interface->desc.bInterfaceSubClass == USB_INTERFACE_SUBCLASS_BOOT &&
> > > +	    interface->desc.bInterfaceProtocol == USB_INTERFACE_PROTOCOL_MOUSE)
> > > +		device_set_wakeup_enable(&dev->dev, 1);
> > 
> > Sorry, but we can not take this unless it is proven that this will work
> > properly for all of these devices.  Other operating systems do not do
> > this last I checked, so there will be problems.
> 
> As Mario Limonciello says, they has confirmed that the Microsoft Windows
> does set a similar policy as well. Can we talk about more in this topic: why
> does Linux not support it?
> Of course, if you have other great idea, I will appreciate that if we can
> have some further discussion.

You need to provide some sort of "proof" that this has been heavily
tested on a huge range of devices before we can change this.

When this was first implemented, Windows did not work this way and many
devices on the market were broken if this were to be enabled.  I'm sure
the mailing list archives from 20+ years ago have many more details,
please dig around there for specifics.

If you feel strongly that this is the way forward, why not do it in
userspace today for your systems as part of testing this out?  It should
not require a kernel change, right?

thanks,

greg k-h

Re: [PATCH] HID: usbhid: enable remote wakeup for mice

[Oliver Neukum]

On 22.02.23 20:50, Limonciello, Mario wrote:

> Richard and I both sent out relatively similar patches in the past, but they never went anywhere.

The problem here is that the default will never satisfy a great majority. just a majority.
So if we admit that there is no optimal solution, why not pick the tested,
that is, current solution, as long as it does least?

> We did confirm that Windows does set a similar policy as well though, which is what prompted us to attempt this.
> 
> As there is more interest again maybe we can revive that discussion and merge together some ideas from the sets of patches.

Well, the most obvious question, as this can also be done with a udev rule, is,
what are the risks of a change?
  
> Previous submissions:
> 
> v4:
> https://lore.kernel.org/linux-usb/20220825045517.16791-1-mario.limonciello@amd.com/

This one at least restricts it to some cases. And as much as it pains me,
we need to ask whether there are risks in not emulating Windows.

And on the other hand, what happens if we do this. Are you sure, for example,
that you do not break use cases for S4 with this change?

If a laptop goes into S4 because power is too low, we really do not want it
to wake up just because somebody accidentally hits a mouse button.

	Regards
		Oliver

Re: [PATCH] HID: usbhid: enable remote wakeup for mice

[Oliver Neukum]

On 23.02.23 12:22, Michael Wu wrote:

> Yes. I can't use any USB mouse which supports Remote Wakeup to wake up the system by default.
> If I enable the wakeup node in /sys/bus/usb/devices which is disabled by default, the mouse can wakeup the system successfully.
> Clearly, the only thing I can do is to register the wakeup source for USB Mouse devices.
> 
Hi,

thank you for answering this. I feel a long discussion coming up,
which I would avoid if I could, so it is important that we are
clear and do not discuss about a misunderstanding.

	Regards
		Oliver

Re: [PATCH] HID: usbhid: enable remote wakeup for mice

[Oliver Neukum]

On 23.02.23 12:23, Greg KH wrote:

>> I just want to explain the background of my patch, to prove we could use a
>> similar way to avoid such a "disturbing" situation.
>> To reduce the influence, my patch enables remote wakeup for USB mouse
>> devices refer to what keyboard do.
> 
> Keyboards are not mice :)
> 

OK, let me explain, why I never proposed switching on autosuspend
for mice or using them as a system wakeup source. The reasons are very
similar.

Basically the standard gives us no way to ask a device what constitutes
a wakeup event for it. We just get the blanket statement of support
or no support.

For runtime PM I would want my mouse to generate a remote wakeup
whenever a button is pressed or the mouse is moved. Under this
premise runtime PM with a mouse works wonderfully. Testing that,
however, is a challenge.
It turns out that mice that claim that they support remote wakeup
by and large deactivate their LED/laser when you send them into
suspension. Hence they react only to buttons being pressed or mouse
wheels moved.

As a system wakeup source a mouse that generates events when
it is moved, however, would make the system unsuspendable, whenever even
a bit of vibration is acting on the system.
And as S4 is used in many setups to prevent an uncontrolled shutdown
at low power, this must work.

I suspect that most, but _not_ _all_ mice, are designed for use
with a system that ties power management to the screen saver.
That is a mode of operation that due to the lack of coupling
between GUI and kernel is hard to copy.

Frankly given these constraints the only default safe in every
case seems to me to leave the kernel's default at off and
put the work into udev's hwdb. Not that that is a good solution,
merely the best I can come up with.

	Regards
		Oliver

RE: [PATCH] HID: usbhid: enable remote wakeup for mice

[Limonciello, Mario]

[AMD Official Use Only - General]



> -----Original Message-----
> From: Oliver Neukum <oneukum@suse.com>
> Sent: Thursday, February 23, 2023 06:02
> To: Greg KH <gregkh@linuxfoundation.org>; Michael Wu
> <michael@allwinnertech.com>
> Cc: jikos@kernel.org; benjamin.tissoires@redhat.com; linux-
> usb@vger.kernel.org; linux-input@vger.kernel.org; linux-
> kernel@vger.kernel.org; Limonciello, Mario <Mario.Limonciello@amd.com>;
> Gong, Richard <Richard.Gong@amd.com>
> Subject: Re: [PATCH] HID: usbhid: enable remote wakeup for mice
> 
> On 23.02.23 12:23, Greg KH wrote:
> 
> >> I just want to explain the background of my patch, to prove we could use
> a
> >> similar way to avoid such a "disturbing" situation.
> >> To reduce the influence, my patch enables remote wakeup for USB
> mouse
> >> devices refer to what keyboard do.
> >
> > Keyboards are not mice :)
> >
> 
> OK, let me explain, why I never proposed switching on autosuspend
> for mice or using them as a system wakeup source. The reasons are very
> similar.
> 
> Basically the standard gives us no way to ask a device what constitutes
> a wakeup event for it. We just get the blanket statement of support
> or no support.
> 
> For runtime PM I would want my mouse to generate a remote wakeup
> whenever a button is pressed or the mouse is moved. Under this
> premise runtime PM with a mouse works wonderfully. Testing that,
> however, is a challenge.
> It turns out that mice that claim that they support remote wakeup
> by and large deactivate their LED/laser when you send them into
> suspension. Hence they react only to buttons being pressed or mouse
> wheels moved.
> 
> As a system wakeup source a mouse that generates events when
> it is moved, however, would make the system unsuspendable, whenever
> even
> a bit of vibration is acting on the system.
> And as S4 is used in many setups to prevent an uncontrolled shutdown
> at low power, this must work.

At least in my version of the series, this is part of the reason that it was
only intended to be used with s2idle.

The kernel driver is well aware of what power state you're in the suspend
callback (pm_suspend_target_state).

What about if we agreed to treat this one special by examining that?

If the sysfs is set to "enabled"
* During suspend if your target is s2idle -> program it
* During suspend if your target is mem -> disable it
* During suspend if your target is hibernate -> disable it

With that type of policy on how to handle the suspend call in place
perhaps we could set it to enabled by default?

> 
> I suspect that most, but _not_ _all_ mice, are designed for use
> with a system that ties power management to the screen saver.
> That is a mode of operation that due to the lack of coupling
> between GUI and kernel is hard to copy.
> 
> Frankly given these constraints the only default safe in every
> case seems to me to leave the kernel's default at off and
> put the work into udev's hwdb. Not that that is a good solution,
> merely the best I can come up with.
> 
> 	Regards
> 		Oliver

Turning on "autosuspend" for USB mice makes them behave pretty
similarly to how they work when they're marked for remote wakeup.

On some mice the lasers turn off, and they only wakeup when you
press a button or roll a wheel.

Re: [PATCH] HID: usbhid: enable remote wakeup for mice

[Michael Wu]

Dear Greg：

On 2/23/2023 7:23 PM, Greg KH wrote:
> On Thu, Feb 23, 2023 at 07:18:12PM +0800, Michael Wu wrote:
>> Dear Greg,
>>
>> On 2/22/2023 2:04 PM, Greg KH wrote:
>>> On Wed, Feb 22, 2023 at 09:39:44AM +0800, Michael Wu wrote:
>>>> This patch fixes a problem that USB mouse can't wake up the device that
>>>> enters standby.
>>>
>>> This not a problem, it is that way by design.
>>
>> I got it, maybe it's a little problem to say that.
> 
> It is.
> 
>>>> At present, the kernel only checks whether certain USB manufacturers
>>>> support wake-up, which will easily cause inconvenience to the
>>>> development work of other manufacturers and add unnecessary work to the
>>>> maintenance of kernel.
>>>>
>>>> The USB protocol supports judging whether a usb supports the wake-up
>>>> function, so it should be more reasonable to add a wake-up source by
>>>> directly checking the settings from the USB protocol.
>>>
>>> But you do not do that in this patch, why not?
>>
>> I just want to explain the background of my patch, to prove we could use a
>> similar way to avoid such a "disturbing" situation.
>> To reduce the influence, my patch enables remote wakeup for USB mouse
>> devices refer to what keyboard do.
> 
> Keyboards are not mice :)

Sorry, What I wanted to say is that we registered the mouse wake-up 
source by referring to the practice of the keyboard.

> 
>>>> There was a similar issue on the keyboard before, which was fixed by
>>>> this patch (3d61510f4eca), but now the problem happened on the mouse.
>>>> This patch uses a similar idea to fix this problem.
>>>>
>>>> Signed-off-by: Michael Wu <michael@allwinnertech.com>
>>>> ---
>>>>    drivers/hid/usbhid/hid-core.c | 8 ++++++++
>>>>    drivers/hid/usbhid/usbmouse.c | 1 +
>>>>    2 files changed, 9 insertions(+)
>>>>
>>>> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
>>>> index be4c731aaa65..d3a6755cca09 100644
>>>> --- a/drivers/hid/usbhid/hid-core.c
>>>> +++ b/drivers/hid/usbhid/hid-core.c
>>>> @@ -1189,6 +1189,14 @@ static int usbhid_start(struct hid_device *hid)
>>>>    		device_set_wakeup_enable(&dev->dev, 1);
>>>>    	}
>>>> +	/**
>>>> +	 * NOTE: enable remote wakeup by default for all mouse devices
>>>> +	 * supporting the boot protocol.
>>>> +	 */
>>>> +	if (interface->desc.bInterfaceSubClass == USB_INTERFACE_SUBCLASS_BOOT &&
>>>> +	    interface->desc.bInterfaceProtocol == USB_INTERFACE_PROTOCOL_MOUSE)
>>>> +		device_set_wakeup_enable(&dev->dev, 1);
>>>
>>> Sorry, but we can not take this unless it is proven that this will work
>>> properly for all of these devices.  Other operating systems do not do
>>> this last I checked, so there will be problems.
>>
>> As Mario Limonciello says, they has confirmed that the Microsoft Windows
>> does set a similar policy as well. Can we talk about more in this topic: why
>> does Linux not support it?
>> Of course, if you have other great idea, I will appreciate that if we can
>> have some further discussion.
> 
> You need to provide some sort of "proof" that this has been heavily
> tested on a huge range of devices before we can change this.
> 
> When this was first implemented, Windows did not work this way and many
> devices on the market were broken if this were to be enabled.  I'm sure
> the mailing list archives from 20+ years ago have many more details,
> please dig around there for specifics.
> 
> If you feel strongly that this is the way forward, why not do it in
> userspace today for your systems as part of testing this out?  It should
> not require a kernel change, right?

Thanks for your advises. I'm clear now. I will try it in userspace.

> 
> thanks,
> 
> greg k-h

-- 
Regards,
Michael Wu

Re: [PATCH] HID: usbhid: enable remote wakeup for mice

[Oliver Neukum]

On 23.02.23 20:41, Limonciello, Mario wrote:

Hi,

>> As a system wakeup source a mouse that generates events when
>> it is moved, however, would make the system unsuspendable, whenever
>> even
>> a bit of vibration is acting on the system.
>> And as S4 is used in many setups to prevent an uncontrolled shutdown
>> at low power, this must work.
> 
> At least in my version of the series, this is part of the reason that it was
> only intended to be used with s2idle.

Yes, that is sensible. If these patches are to be taken at all, that will
be a necessary condition to meet. But it is not sufficient.
  
> The kernel driver is well aware of what power state you're in the suspend
> callback (pm_suspend_target_state).
> 
> What about if we agreed to treat this one special by examining that?
> 
> If the sysfs is set to "enabled"

If user space needs to manipulate sysfs at all, we can have user space
tell kernel space exactly what to do. Hence I see no point in
conditional interpretations values in sysfs at that point.

We are discussing the kernel's default here.

> * During suspend if your target is s2idle -> program it
> * During suspend if your target is mem -> disable it
> * During suspend if your target is hibernate -> disable it

To my mind these defaults make sense.
However, do they make much more sense than what we are doing now?

> With that type of policy on how to handle the suspend call in place
> perhaps we could set it to enabled by default?

It pains me to say, but I am afraid in that regard the only
decision that will not cause ugly surprises is to follow Windows.
Yet, what is wrong about the current defaults?
  
> Turning on "autosuspend" for USB mice makes them behave pretty
> similarly to how they work when they're marked for remote wakeup.

Because it is exactly the same mechanism.
  
> On some mice the lasers turn off, and they only wakeup when you
> press a button or roll a wheel.

Yes. And _some_ is the exact problem. If we could tell, _how_ mice
react, this discussion were unnecessary.

	Regards
		Oliver

RE: [PATCH] HID: usbhid: enable remote wakeup for mice

[Limonciello, Mario]

[Public]



> -----Original Message-----
> From: Oliver Neukum <oneukum@suse.com>
> Sent: Tuesday, February 28, 2023 03:03
> To: Limonciello, Mario <Mario.Limonciello@amd.com>; Oliver Neukum
> <oneukum@suse.com>; Greg KH <gregkh@linuxfoundation.org>; Michael
> Wu <michael@allwinnertech.com>
> Cc: jikos@kernel.org; benjamin.tissoires@redhat.com; linux-
> usb@vger.kernel.org; linux-input@vger.kernel.org; linux-
> kernel@vger.kernel.org; Gong, Richard <Richard.Gong@amd.com>
> Subject: Re: [PATCH] HID: usbhid: enable remote wakeup for mice
> 
> On 23.02.23 20:41, Limonciello, Mario wrote:
> 
> Hi,
> 
> >> As a system wakeup source a mouse that generates events when
> >> it is moved, however, would make the system unsuspendable, whenever
> >> even
> >> a bit of vibration is acting on the system.
> >> And as S4 is used in many setups to prevent an uncontrolled shutdown
> >> at low power, this must work.
> >
> > At least in my version of the series, this is part of the reason that it was
> > only intended to be used with s2idle.
> 
> Yes, that is sensible. If these patches are to be taken at all, that will
> be a necessary condition to meet. But it is not sufficient.

Ack.

> 
> > The kernel driver is well aware of what power state you're in the suspend
> > callback (pm_suspend_target_state).
> >
> > What about if we agreed to treat this one special by examining that?
> >
> > If the sysfs is set to "enabled"
> 
> If user space needs to manipulate sysfs at all, we can have user space
> tell kernel space exactly what to do. Hence I see no point in
> conditional interpretations values in sysfs at that point.
> 
> We are discussing the kernel's default here.

Right, I was meaning if the kernel defaulted to enabled or if userspace
changed it to enabled to follow this behavior.

> 
> > * During suspend if your target is s2idle -> program it
> > * During suspend if your target is mem -> disable it
> > * During suspend if your target is hibernate -> disable it
> 
> To my mind these defaults make sense.
> However, do they make much more sense than what we are doing now?

If you're talking about purely "policy default", I think it makes more sense.

Userspace can still change it, and it better aligns with what Windows does
out of the box.

> 
> > With that type of policy on how to handle the suspend call in place
> > perhaps we could set it to enabled by default?
> 
> It pains me to say, but I am afraid in that regard the only
> decision that will not cause ugly surprises is to follow Windows.
> Yet, what is wrong about the current defaults?

I still keep getting inquiries about this where teams that work on the same
hardware for Windows and Linux complain about this difference during
their testing.

I keep educating them to change it in sysfs (or to use a udev rule), but
you have to question if you keep getting something asked about policy
over and over if it's actually the right policy.

Re: [PATCH] HID: usbhid: enable remote wakeup for mice

[Greg KH]

On Tue, Feb 28, 2023 at 06:50:18PM +0000, Limonciello, Mario wrote:
> I still keep getting inquiries about this where teams that work on the same
> hardware for Windows and Linux complain about this difference during
> their testing.
> 
> I keep educating them to change it in sysfs (or to use a udev rule), but
> you have to question if you keep getting something asked about policy
> over and over if it's actually the right policy.

Why not complain to the Windows team to get them to change their policy
back as they are the ones that changed it over time and are not
backwards-compatible with older systems?

:)

thanks,

greg k-h

RE: [PATCH] HID: usbhid: enable remote wakeup for mice

[Limonciello, Mario]

[AMD Official Use Only - General]



> -----Original Message-----
> From: Greg KH <gregkh@linuxfoundation.org>
> Sent: Tuesday, February 28, 2023 13:05
> To: Limonciello, Mario <Mario.Limonciello@amd.com>
> Cc: Oliver Neukum <oneukum@suse.com>; Michael Wu
> <michael@allwinnertech.com>; jikos@kernel.org;
> benjamin.tissoires@redhat.com; linux-usb@vger.kernel.org; linux-
> input@vger.kernel.org; linux-kernel@vger.kernel.org; Gong, Richard
> <Richard.Gong@amd.com>
> Subject: Re: [PATCH] HID: usbhid: enable remote wakeup for mice
> 
> On Tue, Feb 28, 2023 at 06:50:18PM +0000, Limonciello, Mario wrote:
> > I still keep getting inquiries about this where teams that work on the same
> > hardware for Windows and Linux complain about this difference during
> > their testing.
> >
> > I keep educating them to change it in sysfs (or to use a udev rule), but
> > you have to question if you keep getting something asked about policy
> > over and over if it's actually the right policy.
> 
> Why not complain to the Windows team to get them to change their policy
> back as they are the ones that changed it over time and are not
> backwards-compatible with older systems?
> 

Heh.

I don't think that's actually true though - Modern Standby is relatively new.
Picking new policies tied to that shouldn't break backwards compatibility.