| Message ID | 20240710231606.3029-1-ryanzhou54@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | hid: usbhid: Enable remote wake-up based on device configuration | expand |
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 > >
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
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
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
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
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;
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(-)