| Message ID | 20180420170527.143240-1-ravisadineni@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 20 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: Ravi Chandra Sadineni <ravisadineni@chromium.org> > --- At this point you're supposed to list the differences between this patch and the preceding versions. > drivers/usb/core/hcd.c | 2 ++ > drivers/usb/core/hub.c | 10 +++++++++- > 2 files changed, 11 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c > index 777036ae63674..b8024ae4fdcaa 100644 > --- a/drivers/usb/core/hcd.c > +++ b/drivers/usb/core/hcd.c > @@ -2375,6 +2375,8 @@ void usb_hcd_resume_root_hub (struct usb_hcd *hcd) > { > unsigned long flags; > > + if (hcd->rh_registered) > + pm_wakeup_event(&hcd->self.root_hub->dev, 0); > spin_lock_irqsave (&hcd_root_hub_lock, flags); > if (hcd->rh_registered) { > set_bit(HCD_FLAG_WAKEUP_PENDING, &hcd->flags); This isn't good enough. hcd->rh_registered can change at any time; it is protected by the hcd_root_hub_lock spinlock. That's why I said your code should be moved inside the existing "if" statement. Alan Stern > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c > index f6ea16e9f6bb9..aa9968d90a48c 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); > } > @@ -3434,8 +3439,11 @@ int usb_port_resume(struct usb_device *udev, pm_message_t msg) > > /* 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)) > + 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 Fri, Apr 20, 2018 at 10:29 AM, Alan Stern <stern@rowland.harvard.edu> wrote: > On Fri, 20 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: Ravi Chandra Sadineni <ravisadineni@chromium.org> >> --- > > At this point you're supposed to list the differences between this > patch and the preceding versions. Mentioned the changes between different versions in V5. Thanks. > >> drivers/usb/core/hcd.c | 2 ++ >> drivers/usb/core/hub.c | 10 +++++++++- >> 2 files changed, 11 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c >> index 777036ae63674..b8024ae4fdcaa 100644 >> --- a/drivers/usb/core/hcd.c >> +++ b/drivers/usb/core/hcd.c >> @@ -2375,6 +2375,8 @@ void usb_hcd_resume_root_hub (struct usb_hcd *hcd) >> { >> unsigned long flags; >> >> + if (hcd->rh_registered) >> + pm_wakeup_event(&hcd->self.root_hub->dev, 0); >> spin_lock_irqsave (&hcd_root_hub_lock, flags); >> if (hcd->rh_registered) { >> set_bit(HCD_FLAG_WAKEUP_PENDING, &hcd->flags); > > This isn't good enough. hcd->rh_registered can change at any time; > it is protected by the hcd_root_hub_lock spinlock. That's why I said > your code should be moved inside the existing "if" statement. Sorry about this. Fixed it now. Hope this is fine. Thanks. > > Alan Stern > >> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c >> index f6ea16e9f6bb9..aa9968d90a48c 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); >> } >> @@ -3434,8 +3439,11 @@ int usb_port_resume(struct usb_device *udev, pm_message_t msg) >> >> /* 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)) >> + 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..b8024ae4fdcaa 100644 --- a/drivers/usb/core/hcd.c +++ b/drivers/usb/core/hcd.c @@ -2375,6 +2375,8 @@ void usb_hcd_resume_root_hub (struct usb_hcd *hcd) { unsigned long flags; + if (hcd->rh_registered) + pm_wakeup_event(&hcd->self.root_hub->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..aa9968d90a48c 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); } @@ -3434,8 +3439,11 @@ int usb_port_resume(struct usb_device *udev, pm_message_t msg) /* 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)) + 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: Ravi Chandra Sadineni <ravisadineni@chromium.org> --- drivers/usb/core/hcd.c | 2 ++ drivers/usb/core/hub.c | 10 +++++++++- 2 files changed, 11 insertions(+), 1 deletion(-)