| Message ID | 20180419001850.133110-1-ravisadineni@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Ravi,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on usb/usb-testing]
[also build test ERROR on v4.17-rc1 next-20180419]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Ravi-Chandra-Sadineni/USB-Increment-wakeup-count-on-remote-wakeup/20180419-165317
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
config: x86_64-randconfig-x013-201815 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
drivers/usb//core/hcd.c: In function 'usb_hcd_resume_root_hub':
>> drivers/usb//core/hcd.c:2378:18: error: 'dev' undeclared (first use in this function); did you mean 'cdev'?
pm_wakeup_event(dev, 0);
^~~
cdev
drivers/usb//core/hcd.c:2378:18: note: each undeclared identifier is reported only once for each function it appears in
vim +2378 drivers/usb//core/hcd.c
2364
2365 /**
2366 * usb_hcd_resume_root_hub - called by HCD to resume its root hub
2367 * @hcd: host controller for this root hub
2368 *
2369 * The USB host controller calls this function when its root hub is
2370 * suspended (with the remote wakeup feature enabled) and a remote
2371 * wakeup request is received. The routine submits a workqueue request
2372 * to resume the root hub (that is, manage its downstream ports again).
2373 */
2374 void usb_hcd_resume_root_hub (struct usb_hcd *hcd)
2375 {
2376 unsigned long flags;
2377
> 2378 pm_wakeup_event(dev, 0);
2379 spin_lock_irqsave (&hcd_root_hub_lock, flags);
2380 if (hcd->rh_registered) {
2381 set_bit(HCD_FLAG_WAKEUP_PENDING, &hcd->flags);
2382 queue_work(pm_wq, &hcd->wakeup_work);
2383 }
2384 spin_unlock_irqrestore (&hcd_root_hub_lock, flags);
2385 }
2386 EXPORT_SYMBOL_GPL(usb_hcd_resume_root_hub);
2387
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Ravi,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on usb/usb-testing]
[also build test ERROR on v4.17-rc1 next-20180419]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Ravi-Chandra-Sadineni/USB-Increment-wakeup-count-on-remote-wakeup/20180419-165317
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
config: i386-randconfig-s0-201815 (attached as .config)
compiler: gcc-6 (Debian 6.4.0-9) 6.4.0 20171026
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
drivers/usb/core/hcd.c: In function 'usb_hcd_resume_root_hub':
>> drivers/usb/core/hcd.c:2378:18: error: 'dev' undeclared (first use in this function)
pm_wakeup_event(dev, 0);
^~~
drivers/usb/core/hcd.c:2378:18: note: each undeclared identifier is reported only once for each function it appears in
vim +/dev +2378 drivers/usb/core/hcd.c
2364
2365 /**
2366 * usb_hcd_resume_root_hub - called by HCD to resume its root hub
2367 * @hcd: host controller for this root hub
2368 *
2369 * The USB host controller calls this function when its root hub is
2370 * suspended (with the remote wakeup feature enabled) and a remote
2371 * wakeup request is received. The routine submits a workqueue request
2372 * to resume the root hub (that is, manage its downstream ports again).
2373 */
2374 void usb_hcd_resume_root_hub (struct usb_hcd *hcd)
2375 {
2376 unsigned long flags;
2377
> 2378 pm_wakeup_event(dev, 0);
2379 spin_lock_irqsave (&hcd_root_hub_lock, flags);
2380 if (hcd->rh_registered) {
2381 set_bit(HCD_FLAG_WAKEUP_PENDING, &hcd->flags);
2382 queue_work(pm_wq, &hcd->wakeup_work);
2383 }
2384 spin_unlock_irqrestore (&hcd_root_hub_lock, flags);
2385 }
2386 EXPORT_SYMBOL_GPL(usb_hcd_resume_root_hub);
2387
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Wed, 18 Apr 2018, Ravi Chandra Sadineni wrote: > On chromebooks we depend on wakeup count to identify the wakeup source. > But currently USB devices do not increment the wakeup count when they > trigger the remote wake. This patch addresses the same. > > Resume condition is reported differently on USB 2.0 and USB 3.0 devices. > > On USB 2.0 devices, a wake capable device, if wake enabled, drives > resume signal to indicate a remote wake (USB 2.0 spec section 7.1.7.7). > The upstream facing port then sets C_PORT_SUSPEND bit and reports a > port change event (USB 2.0 spec section 11.24.2.7.2.3). Thus if a port > has resumed before driving the resume signal from the host and > C_PORT_SUSPEND is set, then the device attached to the given port might > be the reason for the last system wakeup. Increment the wakeup count for > the same. > > On USB 3.0 devices, a function may signal that it wants to exit from device > suspend by sending a Function Wake Device Notification to the host (USB3.0 > spec section 8.5.6.4) Thus on receiving the Function Wake, increment the > wakeup count. > > Signed-off-by: ravisadineni@chromium.org > --- > drivers/usb/core/hcd.c | 1 + > drivers/usb/core/hub.c | 12 ++++++++++-- > 2 files changed, 11 insertions(+), 2 deletions(-) > > diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c > index 777036ae63674..79f95a878fb6e 100644 > --- a/drivers/usb/core/hcd.c > +++ b/drivers/usb/core/hcd.c > @@ -2375,6 +2375,7 @@ void usb_hcd_resume_root_hub (struct usb_hcd *hcd) > { > unsigned long flags; > > + pm_wakeup_event(dev, 0); Instead of dev, you probably want to use hcd->self.sysdev. Or maybe hcd->self.controller, although the difference probably doesn't matter for your purposes. On the other hand, this wakeup event may already have been counted by the host controller's bus subsystem. Does it matter if the same wakeup event gets counted twice? (This is inevitable with USB devices, in any case. If a device sends a wakeup request, it will be counted for that device, for all the intermediate hubs, and for the host controller.) > @@ -3432,10 +3437,13 @@ int usb_port_resume(struct usb_device *udev, pm_message_t msg) > > usb_lock_port(port_dev); > > - /* Skip the initial Clear-Suspend step for a remote wakeup */ > status = hub_port_status(hub, port1, &portstatus, &portchange); > - if (status == 0 && !port_is_suspended(hub, portstatus)) > + /* Skip the initial Clear-Suspend step for a remote wakeup */ What is the reason for moving the comment line down after the hub_port_status() call? Alan Stern > + if (status == 0 && !port_is_suspended(hub, portstatus)) { > + if (portchange & USB_PORT_STAT_C_SUSPEND) > + pm_wakeup_event(&udev->dev, 0); > goto SuspendCleared; > + } > > /* see 7.1.7.7; affects power usage, but not budgeting */ > if (hub_is_superspeed(hub->hdev)) > -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Apr 19, 2018 at 8:01 AM, Alan Stern <stern@rowland.harvard.edu> wrote: > On Wed, 18 Apr 2018, Ravi Chandra Sadineni wrote: > >> On chromebooks we depend on wakeup count to identify the wakeup source. >> But currently USB devices do not increment the wakeup count when they >> trigger the remote wake. This patch addresses the same. >> >> Resume condition is reported differently on USB 2.0 and USB 3.0 devices. >> >> On USB 2.0 devices, a wake capable device, if wake enabled, drives >> resume signal to indicate a remote wake (USB 2.0 spec section 7.1.7.7). >> The upstream facing port then sets C_PORT_SUSPEND bit and reports a >> port change event (USB 2.0 spec section 11.24.2.7.2.3). Thus if a port >> has resumed before driving the resume signal from the host and >> C_PORT_SUSPEND is set, then the device attached to the given port might >> be the reason for the last system wakeup. Increment the wakeup count for >> the same. >> >> On USB 3.0 devices, a function may signal that it wants to exit from device >> suspend by sending a Function Wake Device Notification to the host (USB3.0 >> spec section 8.5.6.4) Thus on receiving the Function Wake, increment the >> wakeup count. >> >> Signed-off-by: ravisadineni@chromium.org >> --- >> drivers/usb/core/hcd.c | 1 + >> drivers/usb/core/hub.c | 12 ++++++++++-- >> 2 files changed, 11 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c >> index 777036ae63674..79f95a878fb6e 100644 >> --- a/drivers/usb/core/hcd.c >> +++ b/drivers/usb/core/hcd.c >> @@ -2375,6 +2375,7 @@ void usb_hcd_resume_root_hub (struct usb_hcd *hcd) >> { >> unsigned long flags; >> >> + pm_wakeup_event(dev, 0); > > Instead of dev, you probably want to use hcd->self.sysdev. Or maybe > hcd->self.controller, although the difference probably doesn't matter > for your purposes. > > On the other hand, this wakeup event may already have been counted by > the host controller's bus subsystem. Does it matter if the same wakeup > event gets counted twice? No. The context is that we're interested in making user space behave differently if the wake up source was a user input device (e.g. USB keyboard) v/s some other kind of USB device. So all that matters is that the USB device wakeup count gets incremented. > > (This is inevitable with USB devices, in any case. If a device sends a > wakeup request, it will be counted for that device, for all the > intermediate hubs, and for the host controller.) > >> @@ -3432,10 +3437,13 @@ int usb_port_resume(struct usb_device *udev, pm_message_t msg) >> >> usb_lock_port(port_dev); >> >> - /* Skip the initial Clear-Suspend step for a remote wakeup */ >> status = hub_port_status(hub, port1, &portstatus, &portchange); >> - if (status == 0 && !port_is_suspended(hub, portstatus)) >> + /* Skip the initial Clear-Suspend step for a remote wakeup */ > > What is the reason for moving the comment line down after the > hub_port_status() call? > > Alan Stern > >> + if (status == 0 && !port_is_suspended(hub, portstatus)) { >> + if (portchange & USB_PORT_STAT_C_SUSPEND) >> + pm_wakeup_event(&udev->dev, 0); >> goto SuspendCleared; >> + } >> >> /* see 7.1.7.7; affects power usage, but not budgeting */ >> if (hub_is_superspeed(hub->hdev)) >> > -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Alan, Thanks for reviewing the change. Appreciate your time. I tried to address your comments in the V2 of the patch. On Thu, Apr 19, 2018 at 8:01 AM, Alan Stern <stern@rowland.harvard.edu> wrote: > On Wed, 18 Apr 2018, Ravi Chandra Sadineni wrote: > >> On chromebooks we depend on wakeup count to identify the wakeup source. >> But currently USB devices do not increment the wakeup count when they >> trigger the remote wake. This patch addresses the same. >> >> Resume condition is reported differently on USB 2.0 and USB 3.0 devices. >> >> On USB 2.0 devices, a wake capable device, if wake enabled, drives >> resume signal to indicate a remote wake (USB 2.0 spec section 7.1.7.7). >> The upstream facing port then sets C_PORT_SUSPEND bit and reports a >> port change event (USB 2.0 spec section 11.24.2.7.2.3). Thus if a port >> has resumed before driving the resume signal from the host and >> C_PORT_SUSPEND is set, then the device attached to the given port might >> be the reason for the last system wakeup. Increment the wakeup count for >> the same. >> >> On USB 3.0 devices, a function may signal that it wants to exit from device >> suspend by sending a Function Wake Device Notification to the host (USB3.0 >> spec section 8.5.6.4) Thus on receiving the Function Wake, increment the >> wakeup count. >> >> Signed-off-by: ravisadineni@chromium.org >> --- >> drivers/usb/core/hcd.c | 1 + >> drivers/usb/core/hub.c | 12 ++++++++++-- >> 2 files changed, 11 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c >> index 777036ae63674..79f95a878fb6e 100644 >> --- a/drivers/usb/core/hcd.c >> +++ b/drivers/usb/core/hcd.c >> @@ -2375,6 +2375,7 @@ void usb_hcd_resume_root_hub (struct usb_hcd *hcd) >> { >> unsigned long flags; >> >> + pm_wakeup_event(dev, 0); > > Instead of dev, you probably want to use hcd->self.sysdev. Or maybe > hcd->self.controller, although the difference probably doesn't matter > for your purposes. Trying to increment the wakeup count for the roothub. So pointed dev to &hcd->self.root_hub->dev. Hope this is fine. > > On the other hand, this wakeup event may already have been counted by > the host controller's bus subsystem. Does it matter if the same wakeup > event gets counted twice? > > (This is inevitable with USB devices, in any case. If a device sends a > wakeup request, it will be counted for that device, for all the > intermediate hubs, and for the host controller.) We are o.k. with the wake-up count getting incremented for the intermediate hubs. We just want to identify the leaf hid devices, if they are cause of the remote wake. This way, we can differentiate user triggered wake from a automatic wakes (Ex: wakes triggered by WOLAN packets from USB ethernet adapter). We are also o.k. with the wake-up count getting incremented twice. All we look for is a change in the wake-up count for the interested devices. > >> @@ -3432,10 +3437,13 @@ int usb_port_resume(struct usb_device *udev, pm_message_t msg) >> >> usb_lock_port(port_dev); >> >> - /* Skip the initial Clear-Suspend step for a remote wakeup */ >> status = hub_port_status(hub, port1, &portstatus, &portchange); >> - if (status == 0 && !port_is_suspended(hub, portstatus)) >> + /* Skip the initial Clear-Suspend step for a remote wakeup */ > > What is the reason for moving the comment line down after the > hub_port_status() call? Sorry. This was a mistake. Changed this back in V2. > > Alan Stern > >> + if (status == 0 && !port_is_suspended(hub, portstatus)) { >> + if (portchange & USB_PORT_STAT_C_SUSPEND) >> + pm_wakeup_event(&udev->dev, 0); >> goto SuspendCleared; >> + } >> >> /* see 7.1.7.7; affects power usage, but not budgeting */ >> if (hub_is_superspeed(hub->hdev)) >> > Thanks, Ravi -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c index 777036ae63674..79f95a878fb6e 100644 --- a/drivers/usb/core/hcd.c +++ b/drivers/usb/core/hcd.c @@ -2375,6 +2375,7 @@ void usb_hcd_resume_root_hub (struct usb_hcd *hcd) { unsigned long flags; + pm_wakeup_event(dev, 0); spin_lock_irqsave (&hcd_root_hub_lock, flags); if (hcd->rh_registered) { set_bit(HCD_FLAG_WAKEUP_PENDING, &hcd->flags); diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c index f6ea16e9f6bb9..6abc5be1bcbf5 100644 --- a/drivers/usb/core/hub.c +++ b/drivers/usb/core/hub.c @@ -653,12 +653,17 @@ void usb_wakeup_notification(struct usb_device *hdev, unsigned int portnum) { struct usb_hub *hub; + struct usb_port *port_dev; if (!hdev) return; hub = usb_hub_to_struct_hub(hdev); if (hub) { + port_dev = hub->ports[portnum - 1]; + if (port_dev && port_dev->child) + pm_wakeup_event(&port_dev->child->dev, 0); + set_bit(portnum, hub->wakeup_bits); kick_hub_wq(hub); } @@ -3432,10 +3437,13 @@ int usb_port_resume(struct usb_device *udev, pm_message_t msg) usb_lock_port(port_dev); - /* Skip the initial Clear-Suspend step for a remote wakeup */ status = hub_port_status(hub, port1, &portstatus, &portchange); - if (status == 0 && !port_is_suspended(hub, portstatus)) + /* Skip the initial Clear-Suspend step for a remote wakeup */ + if (status == 0 && !port_is_suspended(hub, portstatus)) { + if (portchange & USB_PORT_STAT_C_SUSPEND) + pm_wakeup_event(&udev->dev, 0); goto SuspendCleared; + } /* see 7.1.7.7; affects power usage, but not budgeting */ if (hub_is_superspeed(hub->hdev))
On chromebooks we depend on wakeup count to identify the wakeup source. But currently USB devices do not increment the wakeup count when they trigger the remote wake. This patch addresses the same. Resume condition is reported differently on USB 2.0 and USB 3.0 devices. On USB 2.0 devices, a wake capable device, if wake enabled, drives resume signal to indicate a remote wake (USB 2.0 spec section 7.1.7.7). The upstream facing port then sets C_PORT_SUSPEND bit and reports a port change event (USB 2.0 spec section 11.24.2.7.2.3). Thus if a port has resumed before driving the resume signal from the host and C_PORT_SUSPEND is set, then the device attached to the given port might be the reason for the last system wakeup. Increment the wakeup count for the same. On USB 3.0 devices, a function may signal that it wants to exit from device suspend by sending a Function Wake Device Notification to the host (USB3.0 spec section 8.5.6.4) Thus on receiving the Function Wake, increment the wakeup count. Signed-off-by: ravisadineni@chromium.org --- drivers/usb/core/hcd.c | 1 + drivers/usb/core/hub.c | 12 ++++++++++-- 2 files changed, 11 insertions(+), 2 deletions(-)