| Message ID | 1638956391-20149-1-git-send-email-zhuyinbo@loongson.cn |
|---|---|
| State | New |
| Headers | show |
| Series | [v1,1/2] HID: usbhid: enable remote wakeup function for usbhid device | expand |
在 2021/12/8 下午6:03, Oliver Neukum 写道: > > On 08.12.21 10:39, Yinbo Zhu wrote: >> The remote wake-up function is a regular function on usb hid device >> and I think keeping it enabled by default will make usb application >> more convenient. This patch is to enable remote wakeup function for >> usb hid device. >> > Hi, > > I am afraid we cannot do this. It will cause regression. > Consider for example the case of laptops with touchscreens > that will trigger a wake up when the laptop is closed. > > Regards > > Oliver Hi oliver, system ask that must it must be accped a acpi lid open event then system will always into resume state for laptop, otherwise, eventhough that system be wakeuped by other event then system will continue into suspend. and for laptop usb wakeup that as general ask bios to enable usb wakeup then if need do more things to enable usb wakeup I think this usb wakeup function isn't friendly and inconveient, so enable it by default. after add this patch, if want to use usb wakeup function it only need enable bios configure it think it is appropriate. BRs, Yinbo. >
On Fri, Dec 10, 2021 at 05:54:33PM +0800, zhuyinbo wrote: > > > 在 2021/12/8 下午7:55, Greg Kroah-Hartman 写道: > > On Wed, Dec 08, 2021 at 05:39:50PM +0800, Yinbo Zhu wrote: > > > The remote wake-up function is a regular function on usb hid device > > > and I think keeping it enabled by default will make usb application > > > more convenient. This patch is to enable remote wakeup function for > > > usb hid device. > > > > How many devices did you test this on? > > > > As Oliver said, this will cause problems, there's a reason no operating > > system does this :( > > > > sorry, > > > > greg k-h > Hi greg, > > About that oliver said that I had expained, and I add this change was > according that usb device whether support remote wakeup and if it support > wakeup then to enabled it so I think it should be okay for all hid device. Again, what devices did you test this on? And look at other operating systems, as I said, there is a reason that no one does this. thanks, greg k-h
On Fri, Dec 10, 2021 at 05:27:30PM +0800, zhuyinbo wrote: > > > 在 2021/12/9 上午6:04, Alan Stern 写道: > > On Wed, Dec 08, 2021 at 05:39:51PM +0800, Yinbo Zhu wrote: > > > The remote wake up function is a regular function on usb device and > > > I think keeping it enabled by default will make the usb application > > > more convenient and usb device remote wake up function keep enabled > > > that ask usb controller remote wake up was enabled at first. > > > > > > This patch only enable wake up on usb root hub device, among which, > > > > You say the patch only affects root hub devices, but this doesn't appear > > to be true. > > > > > usb3.0 root hub doesn't be set wakeup node property but use command > > > USB_INTRF_FUNC_SUSPEND to enable remote wake up function. > > > > > > Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn> > > > --- > > > drivers/usb/core/hub.c | 20 ++++++++++++++++++-- > > > include/linux/usb.h | 4 +++- > > > 2 files changed, 21 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c > > > index 86658a8..cb4b956 100644 > > > --- a/drivers/usb/core/hub.c > > > +++ b/drivers/usb/core/hub.c > > > @@ -2509,6 +2509,8 @@ static void set_usb_port_removable(struct usb_device *udev) > > > */ > > > int usb_new_device(struct usb_device *udev) > > > { > > > + struct usb_host_config *config; > > > + int ncfg; > > > int err; > > > if (udev->parent) { > > > @@ -2540,6 +2542,18 @@ int usb_new_device(struct usb_device *udev) > > > udev->dev.devt = MKDEV(USB_DEVICE_MAJOR, > > > (((udev->bus->busnum-1) * 128) + (udev->devnum-1))); > > > + for (ncfg = 0; ncfg < udev->descriptor.bNumConfigurations; ncfg++) { > > > + config = &udev->config[ncfg]; > > > + if ((config->desc.bmAttributes & (1 << 5)) == 0) > > > + break; > > > + if (ncfg + 1 == udev->descriptor.bNumConfigurations) { > > > + err = usb_enable_remote_wakeup(udev); > > > + if (err) > > > + dev_dbg(&udev->dev, > > > + "won't remote wakeup, err %d\n", err); > > > + } > > > + } > > > > I don't see anything in there which treats root hubs differently from > > other devices. > > > Hi Alan Stern, > > You can find following code, non-root-hub had removed Wakeup sysfs > attributes and disabled wakeup and root-hub had added wakeup sysfs attibutes > before call usb_new_device, so this patch was only enabled > remote wakeup for root-hub device. > int usb_new_device(struct usb_device *udev) > { > if (udev->parent) { > /* Initialize non-root-hub device wakeup to disabled; > * device (un)configuration controls wakeup capable > * sysfs power/wakeup controls wakeup enabled/disabled > */ > device_init_wakeup(&udev->dev, 0); > } Okay. But in any case, you're doing this in the wrong place. Remote wakeup capability depends on the configuration, so you must not enable in usb_new_device() before the configuration has been chosen. Furthermore, remote wakeup gets turned on only at the time when the device is suspended. We don't leave it on all the time. > > Besides, enabling wakeup for root hubs is generally a bad idea. Suppose > > you closed a laptop's lid and then unplugged a USB device -- with wakeup > > enabled, the unplug would cause the laptop to wake up again without your > > knowledge. > > > > Alan Stern > when closed laptop's lid and then unplugged a non-hid usb device it doesn't > cause laptop to wakeup. and if that usb device is hid type and cause laptop > into wakeup state then system will continue into suspend state becuase > system ask that need accepted a acpi lid open event. Not all laptops have ACPI. > and for laptop usb wakeup that as general ask bios to enable usb wakeup then > if need do more things to enable usb wakeup I think this usb wakeup function > isn't friendly and inconveient, so enable it by default. > after add this patch, if want to use usb wakeup function it only need enable > bios configure it think it is appropriate. The decision about whether or not to enable remote wakeup for a USB device is a matter of policy. It has to be decided by the user, not by the kernel. This is why there are userspace tools, like Powertop, that automatically enable remote wakeup when devices are detected and that allow the user to control which devices get enabled. Using these tools is easy and convenient -- that's why they exist -- so the kernel's interface does not need to be friendly. Alan Stern
在 2021/12/10 下午6:45, Greg Kroah-Hartman 写道: > On Fri, Dec 10, 2021 at 05:54:33PM +0800, zhuyinbo wrote: >> >> >> 在 2021/12/8 下午7:55, Greg Kroah-Hartman 写道: >>> On Wed, Dec 08, 2021 at 05:39:50PM +0800, Yinbo Zhu wrote: >>>> The remote wake-up function is a regular function on usb hid device >>>> and I think keeping it enabled by default will make usb application >>>> more convenient. This patch is to enable remote wakeup function for >>>> usb hid device. >>> >>> How many devices did you test this on? >>> >>> As Oliver said, this will cause problems, there's a reason no operating >>> system does this :( >>> >>> sorry, >>> >>> greg k-h >> Hi greg, >> >> About that oliver said that I had expained, and I add this change was >> according that usb device whether support remote wakeup and if it support >> wakeup then to enabled it so I think it should be okay for all hid device. > > Again, what devices did you test this on? Hi greg k-h mouse device and keyboard device. in hid code that keyboad was enabled on wakeup node by default. > > And look at other operating systems, as I said, there is a reason that > no one does this. System should be do that. otherwise that lid open event was no make sense. and you need know the order that after lid open then system will be wakeup then system will accept lid open event by input subsystem . in addition, whatever that usb wakeup was disabled in bios by default even though after apply my patch. > > thanks, > > greg k-h >
diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c index 2dcaf31..3619b95 100644 --- a/drivers/hid/usbhid/hid-core.c +++ b/drivers/hid/usbhid/hid-core.c @@ -1063,7 +1063,9 @@ static int usbhid_start(struct hid_device *hid) struct usb_host_interface *interface = intf->cur_altsetting; struct usb_device *dev = interface_to_usbdev(intf); struct usbhid_device *usbhid = hid->driver_data; + struct usb_host_config *config; unsigned int n, insize = 0; + int ncfg; int ret; mutex_lock(&usbhid->mutex); @@ -1179,14 +1181,19 @@ 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); + } + + for (ncfg = 0; ncfg < dev->descriptor.bNumConfigurations; ncfg++) { + config = &dev->config[ncfg]; + if ((config->desc.bmAttributes & (1 << 5)) == 0) + break; + if (ncfg + 1 == dev->descriptor.bNumConfigurations) + device_set_wakeup_enable(&dev->dev, 1); } mutex_unlock(&usbhid->mutex);
The remote wake-up function is a regular function on usb hid device and I think keeping it enabled by default will make usb application more convenient. This patch is to enable remote wakeup function for usb hid device. Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn> --- drivers/hid/usbhid/hid-core.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)