| Message ID | 1638956391-20149-2-git-send-email-zhuyinbo@loongson.cn (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1,1/2] HID: usbhid: enable remote wakeup function for usbhid device | expand |
Hi Yinbo,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on usb/usb-testing]
[also build test ERROR on hid/for-next v5.16-rc4 next-20211208]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Yinbo-Zhu/HID-usbhid-enable-remote-wakeup-function-for-usbhid-device/20211208-174035
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
config: hexagon-randconfig-r045-20211207 (https://download.01.org/0day-ci/archive/20211208/202112082256.02orEkq6-lkp@intel.com/config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 097a1cb1d5ebb3a0ec4bcaed8ba3ff6a8e33c00a)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/e900e8c9eb7fd4fda123036095e7e7ec87439547
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Yinbo-Zhu/HID-usbhid-enable-remote-wakeup-function-for-usbhid-device/20211208-174035
git checkout e900e8c9eb7fd4fda123036095e7e7ec87439547
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/usb/core/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
>> drivers/usb/core/hub.c:2550:10: error: implicit declaration of function 'usb_enable_remote_wakeup' [-Werror,-Wimplicit-function-declaration]
err = usb_enable_remote_wakeup(udev);
^
1 error generated.
vim +/usb_enable_remote_wakeup +2550 drivers/usb/core/hub.c
2488
2489 /**
2490 * usb_new_device - perform initial device setup (usbcore-internal)
2491 * @udev: newly addressed device (in ADDRESS state)
2492 *
2493 * This is called with devices which have been detected but not fully
2494 * enumerated. The device descriptor is available, but not descriptors
2495 * for any device configuration. The caller must have locked either
2496 * the parent hub (if udev is a normal device) or else the
2497 * usb_bus_idr_lock (if udev is a root hub). The parent's pointer to
2498 * udev has already been installed, but udev is not yet visible through
2499 * sysfs or other filesystem code.
2500 *
2501 * This call is synchronous, and may not be used in an interrupt context.
2502 *
2503 * Only the hub driver or root-hub registrar should ever call this.
2504 *
2505 * Return: Whether the device is configured properly or not. Zero if the
2506 * interface was registered with the driver core; else a negative errno
2507 * value.
2508 *
2509 */
2510 int usb_new_device(struct usb_device *udev)
2511 {
2512 struct usb_host_config *config;
2513 int ncfg;
2514 int err;
2515
2516 if (udev->parent) {
2517 /* Initialize non-root-hub device wakeup to disabled;
2518 * device (un)configuration controls wakeup capable
2519 * sysfs power/wakeup controls wakeup enabled/disabled
2520 */
2521 device_init_wakeup(&udev->dev, 0);
2522 }
2523
2524 /* Tell the runtime-PM framework the device is active */
2525 pm_runtime_set_active(&udev->dev);
2526 pm_runtime_get_noresume(&udev->dev);
2527 pm_runtime_use_autosuspend(&udev->dev);
2528 pm_runtime_enable(&udev->dev);
2529
2530 /* By default, forbid autosuspend for all devices. It will be
2531 * allowed for hubs during binding.
2532 */
2533 usb_disable_autosuspend(udev);
2534
2535 err = usb_enumerate_device(udev); /* Read descriptors */
2536 if (err < 0)
2537 goto fail;
2538 dev_dbg(&udev->dev, "udev %d, busnum %d, minor = %d\n",
2539 udev->devnum, udev->bus->busnum,
2540 (((udev->bus->busnum-1) * 128) + (udev->devnum-1)));
2541 /* export the usbdev device-node for libusb */
2542 udev->dev.devt = MKDEV(USB_DEVICE_MAJOR,
2543 (((udev->bus->busnum-1) * 128) + (udev->devnum-1)));
2544
2545 for (ncfg = 0; ncfg < udev->descriptor.bNumConfigurations; ncfg++) {
2546 config = &udev->config[ncfg];
2547 if ((config->desc.bmAttributes & (1 << 5)) == 0)
2548 break;
2549 if (ncfg + 1 == udev->descriptor.bNumConfigurations) {
> 2550 err = usb_enable_remote_wakeup(udev);
2551 if (err)
2552 dev_dbg(&udev->dev,
2553 "won't remote wakeup, err %d\n", err);
2554 }
2555 }
2556
2557 /* Tell the world! */
2558 announce_device(udev);
2559
2560 if (udev->serial)
2561 add_device_randomness(udev->serial, strlen(udev->serial));
2562 if (udev->product)
2563 add_device_randomness(udev->product, strlen(udev->product));
2564 if (udev->manufacturer)
2565 add_device_randomness(udev->manufacturer,
2566 strlen(udev->manufacturer));
2567
2568 device_enable_async_suspend(&udev->dev);
2569
2570 /* check whether the hub or firmware marks this port as non-removable */
2571 set_usb_port_removable(udev);
2572
2573 /* Register the device. The device driver is responsible
2574 * for configuring the device and invoking the add-device
2575 * notifier chain (used by usbfs and possibly others).
2576 */
2577 err = device_add(&udev->dev);
2578 if (err) {
2579 dev_err(&udev->dev, "can't device_add, error %d\n", err);
2580 goto fail;
2581 }
2582
2583 /* Create link files between child device and usb port device. */
2584 if (udev->parent) {
2585 struct usb_hub *hub = usb_hub_to_struct_hub(udev->parent);
2586 int port1 = udev->portnum;
2587 struct usb_port *port_dev = hub->ports[port1 - 1];
2588
2589 err = sysfs_create_link(&udev->dev.kobj,
2590 &port_dev->dev.kobj, "port");
2591 if (err)
2592 goto fail;
2593
2594 err = sysfs_create_link(&port_dev->dev.kobj,
2595 &udev->dev.kobj, "device");
2596 if (err) {
2597 sysfs_remove_link(&udev->dev.kobj, "port");
2598 goto fail;
2599 }
2600
2601 if (!test_and_set_bit(port1, hub->child_usage_bits))
2602 pm_runtime_get_sync(&port_dev->dev);
2603 }
2604
2605 (void) usb_create_ep_devs(&udev->dev, &udev->ep0, udev);
2606 usb_mark_last_busy(udev);
2607 pm_runtime_put_sync_autosuspend(&udev->dev);
2608 return err;
2609
2610 fail:
2611 usb_set_device_state(udev, USB_STATE_NOTATTACHED);
2612 pm_runtime_disable(&udev->dev);
2613 pm_runtime_set_suspended(&udev->dev);
2614 return err;
2615 }
2616
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
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. 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
在 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); } > 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. 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: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
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); + } + } + /* Tell the world! */ announce_device(udev); @@ -3234,7 +3248,7 @@ void usb_enable_ltm(struct usb_device *udev) * enable remote wake for the first interface. FIXME if the interface * association descriptor shows there's more than one function. */ -static int usb_enable_remote_wakeup(struct usb_device *udev) +int usb_enable_remote_wakeup(struct usb_device *udev) { if (udev->speed < USB_SPEED_SUPER) return usb_control_msg(udev, usb_sndctrlpipe(udev, 0), @@ -3249,6 +3263,7 @@ static int usb_enable_remote_wakeup(struct usb_device *udev) USB_INTRF_FUNC_SUSPEND_LP, NULL, 0, USB_CTRL_SET_TIMEOUT); } +EXPORT_SYMBOL_GPL(usb_enable_remote_wakeup); /* * usb_disable_remote_wakeup - disable remote wakeup for a device @@ -3260,7 +3275,7 @@ static int usb_enable_remote_wakeup(struct usb_device *udev) * disable remote wake for the first interface. FIXME if the interface * association descriptor shows there's more than one function. */ -static int usb_disable_remote_wakeup(struct usb_device *udev) +int usb_disable_remote_wakeup(struct usb_device *udev) { if (udev->speed < USB_SPEED_SUPER) return usb_control_msg(udev, usb_sndctrlpipe(udev, 0), @@ -3273,6 +3288,7 @@ static int usb_disable_remote_wakeup(struct usb_device *udev) USB_INTRF_FUNC_SUSPEND, 0, NULL, 0, USB_CTRL_SET_TIMEOUT); } +EXPORT_SYMBOL_GPL(usb_disable_remote_wakeup); /* Count of wakeup-enabled devices at or below udev */ unsigned usb_wakeup_enabled_descendants(struct usb_device *udev) diff --git a/include/linux/usb.h b/include/linux/usb.h index 7ccaa76..8097eee 100644 --- a/include/linux/usb.h +++ b/include/linux/usb.h @@ -754,8 +754,10 @@ static inline bool usb_acpi_power_manageable(struct usb_device *hdev, int index) { return true; } #endif -/* USB autosuspend and autoresume */ #ifdef CONFIG_PM +extern int usb_enable_remote_wakeup(struct usb_device *udev); +extern int usb_disable_remote_wakeup(struct usb_device *udev); + extern void usb_enable_autosuspend(struct usb_device *udev); extern void usb_disable_autosuspend(struct usb_device *udev);
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, 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(-)