On Sat, Jan 07, 2012 at 09:29:00PM -0500, Alan Stern wrote:
> On Fri, 6 Jan 2012, Sarah Sharp wrote:
> > @@ -3569,11 +3569,17 @@ static void hub_events(void)
> > }
> > }
> >
> > - if (portchange & USB_PORT_STAT_C_SUSPEND) {
> > + if ((portchange & USB_PORT_STAT_C_SUSPEND) ||
> > + (hub_is_superspeed(hdev) &&
> > + (portchange & USB_PORT_STAT_C_LINK_STATE))) {
> > struct usb_device *udev;
> >
> > - clear_port_feature(hdev, i,
> > - USB_PORT_FEAT_C_SUSPEND);
> > + if (!hub_is_superspeed(hdev))
> > + clear_port_feature(hdev, i,
> > + USB_PORT_FEAT_C_SUSPEND);
> > + else
> > + clear_port_feature(hdev, i,
> > + USB_PORT_FEAT_C_PORT_LINK_STATE);
> > udev = hdev->children[i-1];
> > if (udev) {
> > /* TRSMRCY = 10 msec */
>
> This is ugly. Instead, have two separate tests: one for !superspeed &&
> C_SUSPEND and the other for superspeed && C_LINK_STATE. Sort of like
> we have in usb_port_resume(). The common parts of the code that
> follows can be moved into a subroutine.
Ok, what about something like this? I feel like portions of the
hub_events really need to get refactored into smaller functions that do
one thing (like handle remote wakeup), with the hub_events() function
basically keeping track of the future actions it needs to take care of
(like handling a connect status change).
Sarah Sharp
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index db6b751..cee525c 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -3442,6 +3442,47 @@ done:
hcd->driver->relinquish_port(hcd, port1);
}
+/* Returns 1 if there was a remote wakeup and a connect status change. */
+static int hub_check_remote_wakeup(struct usb_hub *hub, unsigned int port,
+ u16 portchange)
+{
+ struct usb_device *hdev;
+ struct usb_device *udev;
+ int connect_change = 0;
+ int ret;
+
+ hdev = hub->hdev;
+ if (!hub_is_superspeed(hdev) &&
+ !(portchange & USB_PORT_STAT_C_SUSPEND))
+ return 0;
+ if (hub_is_superspeed(hdev) &&
+ !(portchange & USB_PORT_STAT_C_LINK_STATE))
+ return 0;
+
+ if (!hub_is_superspeed(hdev))
+ clear_port_feature(hdev, port, USB_PORT_FEAT_C_SUSPEND);
+ else
+ clear_port_feature(hdev, port, USB_PORT_FEAT_C_PORT_LINK_STATE);
+
+ udev = hdev->children[port-1];
+ if (udev) {
+ /* TRSMRCY = 10 msec */
+ msleep(10);
+
+ usb_lock_device(udev);
+ ret = usb_remote_wakeup(hdev->children[port-1]);
+ usb_unlock_device(udev);
+ if (ret < 0)
+ connect_change = 1;
+ } else {
+ ret = -ENODEV;
+ hub_port_disable(hub, port, 1);
+ }
+ dev_dbg(hub->intfdev, "resume on port %d, status %d\n",
+ port, ret);
+ return connect_change;
+}
+
static void hub_events(void)
{
struct list_head *tmp;
@@ -3575,37 +3616,9 @@ static void hub_events(void)
}
}
- if ((portchange & USB_PORT_STAT_C_SUSPEND) ||
- (hub_is_superspeed(hdev) &&
- (portchange & USB_PORT_STAT_C_LINK_STATE))) {
- struct usb_device *udev;
+ if (hub_check_remote_wakeup(hub, i, portchange))
+ connect_change = 1;
- if (!hub_is_superspeed(hdev))
- clear_port_feature(hdev, i,
- USB_PORT_FEAT_C_SUSPEND);
- else
- clear_port_feature(hdev, i,
- USB_PORT_FEAT_C_PORT_LINK_STATE);
- udev = hdev->children[i-1];
- if (udev) {
- /* TRSMRCY = 10 msec */
- msleep(10);
-
- usb_lock_device(udev);
- ret = usb_remote_wakeup(hdev->
- children[i-1]);
- usb_unlock_device(udev);
- if (ret < 0)
- connect_change = 1;
- } else {
- ret = -ENODEV;
- hub_port_disable(hub, i, 1);
- }
- dev_dbg (hub_dev,
- "resume on port %d, status %d\n",
- i, ret);
- }
-
if (portchange & USB_PORT_STAT_C_OVERCURRENT) {
u16 status = 0;
u16 unused;
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
