[PATCH] hid: usbhid: Enable remote wake-up based on device configuration

[PATCH] hid: usbhid: Enable remote wake-up based on device configuration

[ryan]

According to the USB protocol, the host should automatically
adapt the remote wake-up function based on the configuration
descriptor reported by the device, rather than only the default
keyboard support. Therefore, it's necessary to support other hid
devices, such as digital headsets,mice,etc.

Signed-off-by: ryan <ryanzhou54@gmail.com>
---
 drivers/hid/usbhid/hid-core.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index a90ed2ceae84..d2901ad9a871 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -1179,16 +1179,16 @@ static int usbhid_start(struct hid_device *hid)
 	/* 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 (dev->actconfig->desc.bmAttributes & USB_CONFIG_ATT_WAKEUP)
+		device_set_wakeup_enable(&dev->dev, 1);
+
 	mutex_unlock(&usbhid->mutex);
 	return 0;
 
-- 
2.17.1

Re: [PATCH] hid: usbhid: Enable remote wake-up based on device configuration

[Alan Stern]

On Thu, Jul 11, 2024 at 07:16:06AM +0800, ryan wrote:
> According to the USB protocol, the host should automatically
> adapt the remote wake-up function based on the configuration
> descriptor reported by the device, rather than only the default
> keyboard support. Therefore, it's necessary to support other hid
> devices, such as digital headsets,mice,etc.

It's true that the host shouldn't try to enable remote wakeup if the 
configuration descriptor shows that the device doesn't support it.

However, it's not true that the host should try to enable remote wakeup 
for devices other than keyboards with boot-protocol support.  History 
has shown that quite a few HID devices don't handle remote wakeup 
properly; the decision about whether to enable it should be left to the 
user.

Alan Stern

> Signed-off-by: ryan <ryanzhou54@gmail.com>
> ---
>  drivers/hid/usbhid/hid-core.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> index a90ed2ceae84..d2901ad9a871 100644
> --- a/drivers/hid/usbhid/hid-core.c
> +++ b/drivers/hid/usbhid/hid-core.c
> @@ -1179,16 +1179,16 @@ static int usbhid_start(struct hid_device *hid)
>  	/* 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 (dev->actconfig->desc.bmAttributes & USB_CONFIG_ATT_WAKEUP)
> +		device_set_wakeup_enable(&dev->dev, 1);
> +
>  	mutex_unlock(&usbhid->mutex);
>  	return 0;
>  
> -- 
> 2.17.1
> 
> 

Re: [PATCH] hid: usbhid: Enable remote wake-up based on device configuration

[Greg KH]

On Wed, Jul 10, 2024 at 09:47:39PM -0400, Alan Stern wrote:
> On Thu, Jul 11, 2024 at 07:16:06AM +0800, ryan wrote:
> > According to the USB protocol, the host should automatically
> > adapt the remote wake-up function based on the configuration
> > descriptor reported by the device, rather than only the default
> > keyboard support. Therefore, it's necessary to support other hid
> > devices, such as digital headsets,mice,etc.
> 
> It's true that the host shouldn't try to enable remote wakeup if the 
> configuration descriptor shows that the device doesn't support it.
> 
> However, it's not true that the host should try to enable remote wakeup 
> for devices other than keyboards with boot-protocol support.  History 
> has shown that quite a few HID devices don't handle remote wakeup 
> properly; the decision about whether to enable it should be left to the 
> user.

I agree, this patch isn't acceptable.  Ryan, why do you want this
applied?  What userspace control is missing to allow you to do this
today on your systems with no kernel changes for devices that you know
will work properly?

thanks,

greg k-h

Re: [PATCH] hid: usbhid: Enable remote wake-up based on device configuration

[ryan zhou]

On Thu, Jul 11, 2024 at 3:41 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Wed, Jul 10, 2024 at 09:47:39PM -0400, Alan Stern wrote:
> > On Thu, Jul 11, 2024 at 07:16:06AM +0800, ryan wrote:
> > > According to the USB protocol, the host should automatically
> > > adapt the remote wake-up function based on the configuration
> > > descriptor reported by the device, rather than only the default
> > > keyboard support. Therefore, it's necessary to support other hid
> > > devices, such as digital headsets,mice,etc.
> >
> > It's true that the host shouldn't try to enable remote wakeup if the
> > configuration descriptor shows that the device doesn't support it.
> >
> > However, it's not true that the host should try to enable remote wakeup
> > for devices other than keyboards with boot-protocol support.  History
> > has shown that quite a few HID devices don't handle remote wakeup
> > properly; the decision about whether to enable it should be left to the
> > user.
>
> I agree, this patch isn't acceptable.  Ryan, why do you want this
> applied?  What userspace control is missing to allow you to do this
> today on your systems with no kernel changes for devices that you know
> will work properly?
>
> thanks,
>
> greg k-h


Many thanks to Greg KH and Alan Stern for reviewing the patch and
replying to me.
I'd like to start by asking Greg KH's question.

A1:This patch is expected to be applied to the USB digital headset,
mouse, and keyboard,
and we expect to wake up the system by operating them when the system
has suspended.

A2:I've verified that user-space control does the trick, but
Personally speaking, it's not a good solution.
For each device plugged into the host, the user space needs to check whether
it is one of the three and to enable wakeup.It may be better to enable
wakeup when loading
a HID class drivers, from my perspective. Could you please give me
some advice if possible.

I have spent some time studying your responses, and learned a lot. I
absolutely agree with many
of your points, but still have some doubts.

Q1 for Alan Stern: Boot device includes a boot mouse and boot keyboard,
why the patch(3d61510f4ecac) only enables boot keyboard by default,
and in addation boot
protocol is used in BIOS,why is it used as a wakeup judgment condition
in the OS?

Q2: for Alan Stern:  As you comment 'History has shown that quite a
few HID devices don't
handle remote wakeup properly'  I consulted the USB20 Spec in Chapter
9.2.5.2 and it has
this description:'If a device supports remote wakeup, it must also
allow the capability to be
enabled and disabled using the standard USB request'  So these devices
that you're talking about
are not compliant with the USB20 protocol specification to my mind. If
so, shouldn't we
support these non-standard devices.


Thanks

ryan

Re: [PATCH] hid: usbhid: Enable remote wake-up based on device configuration

[Greg KH]

On Mon, Jul 15, 2024 at 11:36:57PM +0800, ryan zhou wrote:
> On Thu, Jul 11, 2024 at 3:41 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Wed, Jul 10, 2024 at 09:47:39PM -0400, Alan Stern wrote:
> > > On Thu, Jul 11, 2024 at 07:16:06AM +0800, ryan wrote:
> > > > According to the USB protocol, the host should automatically
> > > > adapt the remote wake-up function based on the configuration
> > > > descriptor reported by the device, rather than only the default
> > > > keyboard support. Therefore, it's necessary to support other hid
> > > > devices, such as digital headsets,mice,etc.
> > >
> > > It's true that the host shouldn't try to enable remote wakeup if the
> > > configuration descriptor shows that the device doesn't support it.
> > >
> > > However, it's not true that the host should try to enable remote wakeup
> > > for devices other than keyboards with boot-protocol support.  History
> > > has shown that quite a few HID devices don't handle remote wakeup
> > > properly; the decision about whether to enable it should be left to the
> > > user.
> >
> > I agree, this patch isn't acceptable.  Ryan, why do you want this
> > applied?  What userspace control is missing to allow you to do this
> > today on your systems with no kernel changes for devices that you know
> > will work properly?
> >
> > thanks,
> >
> > greg k-h
> 
> 
> Many thanks to Greg KH and Alan Stern for reviewing the patch and
> replying to me.
> I'd like to start by asking Greg KH's question.
> 
> A1:This patch is expected to be applied to the USB digital headset,
> mouse, and keyboard,
> and we expect to wake up the system by operating them when the system
> has suspended.
> 
> A2:I've verified that user-space control does the trick, but
> Personally speaking, it's not a good solution.
> For each device plugged into the host, the user space needs to check whether
> it is one of the three and to enable wakeup.It may be better to enable
> wakeup when loading
> a HID class drivers, from my perspective. Could you please give me
> some advice if possible.

You can run anything you want at device-plugin-time in userspace by
writing a udev rule, that's exactly what that was designed for.  The
policy you decide is under your control in userspace, no need to do
anything special in the kernel at all.

> I have spent some time studying your responses, and learned a lot. I
> absolutely agree with many
> of your points, but still have some doubts.
> 
> Q1 for Alan Stern: Boot device includes a boot mouse and boot keyboard,
> why the patch(3d61510f4ecac) only enables boot keyboard by default,
> and in addation boot
> protocol is used in BIOS,why is it used as a wakeup judgment condition
> in the OS?
> 
> Q2: for Alan Stern:  As you comment 'History has shown that quite a
> few HID devices don't
> handle remote wakeup properly'  I consulted the USB20 Spec in Chapter
> 9.2.5.2 and it has
> this description:'If a device supports remote wakeup, it must also
> allow the capability to be
> enabled and disabled using the standard USB request'  So these devices
> that you're talking about
> are not compliant with the USB20 protocol specification to my mind. If
> so, shouldn't we
> support these non-standard devices.

If you do not support "non-standard" devices, your operating system will
not be used by anyone in the real-world as there are TONS of
"non-standard" devices out there, sorry.

try it and see, go to the local store and buy a shopping cart of cheap
mice and keyboards and see what happens...

thanks,

greg k-h

Re: [PATCH] hid: usbhid: Enable remote wake-up based on device configuration

[Alan Stern]

On Mon, Jul 15, 2024 at 11:36:57PM +0800, ryan zhou wrote:
> On Thu, Jul 11, 2024 at 3:41 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Wed, Jul 10, 2024 at 09:47:39PM -0400, Alan Stern wrote:
> > > On Thu, Jul 11, 2024 at 07:16:06AM +0800, ryan wrote:
> > > > According to the USB protocol, the host should automatically
> > > > adapt the remote wake-up function based on the configuration
> > > > descriptor reported by the device, rather than only the default
> > > > keyboard support. Therefore, it's necessary to support other hid
> > > > devices, such as digital headsets,mice,etc.
> > >
> > > It's true that the host shouldn't try to enable remote wakeup if the
> > > configuration descriptor shows that the device doesn't support it.
> > >
> > > However, it's not true that the host should try to enable remote wakeup
> > > for devices other than keyboards with boot-protocol support.  History
> > > has shown that quite a few HID devices don't handle remote wakeup
> > > properly; the decision about whether to enable it should be left to the
> > > user.
> >
> > I agree, this patch isn't acceptable.  Ryan, why do you want this
> > applied?  What userspace control is missing to allow you to do this
> > today on your systems with no kernel changes for devices that you know
> > will work properly?
> >
> > thanks,
> >
> > greg k-h
> 
> 
> Many thanks to Greg KH and Alan Stern for reviewing the patch and
> replying to me.
> I'd like to start by asking Greg KH's question.
> 
> A1:This patch is expected to be applied to the USB digital headset,
> mouse, and keyboard,
> and we expect to wake up the system by operating them when the system
> has suspended.
> 
> A2:I've verified that user-space control does the trick, but
> Personally speaking, it's not a good solution.
> For each device plugged into the host, the user space needs to check whether
> it is one of the three and to enable wakeup.It may be better to enable
> wakeup when loading
> a HID class drivers, from my perspective. Could you please give me
> some advice if possible.
> 
> I have spent some time studying your responses, and learned a lot. I
> absolutely agree with many
> of your points, but still have some doubts.
> 
> Q1 for Alan Stern: Boot device includes a boot mouse and boot keyboard,
> why the patch(3d61510f4ecac) only enables boot keyboard by default,
> and in addation boot
> protocol is used in BIOS,why is it used as a wakeup judgment condition
> in the OS?

In general we did not want to enable wakeup by default for mouse 
devices.  We didn't want to have a situation where somebody puts their 
computer to sleep and then accidentally moves the mouse, and that 
causes the computer to wake up.

I don't remember the reason why only keyboards supporting the boot 
protocol are enabled by default.  Maybe we thought those were likely to 
be the most reliable ones.

> Q2: for Alan Stern:  As you comment 'History has shown that quite a
> few HID devices don't
> handle remote wakeup properly'  I consulted the USB20 Spec in Chapter
> 9.2.5.2 and it has
> this description:'If a device supports remote wakeup, it must also
> allow the capability to be
> enabled and disabled using the standard USB request'  So these devices
> that you're talking about
> are not compliant with the USB20 protocol specification to my mind. If
> so, shouldn't we
> support these non-standard devices.

Sometimes supporting a device means _not_ enabling it for remote wakeup, 
because its wakeup support is broken.

Alan Stern