Sigh. So it turns out the behavior of USB 3.0 external hubs and the
xHCI roothub port link state change bit is different.
The USB 3.0 bus spec, section 10.14.2.6.2 says the port link state
change bit will be set when the port goes from U3 to U0 when the host
initiated the resume, but the bit will *not* be set if the device has
signaled remote wakeup. But the xHCI spec says that the link state
change bit will be set on a remote wakeup.
I think the idea is that software is supposed to know to resume a device
by looking at the device notification the device sends (and the xHCI
host passes on in an event) when the roothub port finishes resuming.
Only the xHCI host doesn't send device notifications itself, so it uses
the link state change.
The xHCI driver needs a way to tell khubd which device caused the remote
wakeup, because khubd won't be able to tell by looking at the link state
change bits. It could possibly tell, if it internally kept track of
which link state each USB 3.0 device should be in, and checked which
ports changed link states to U0.
I'll go look at the hub code more closely to see what it currently does,
but I thought I'd pass my finding on to you and see if you have any
thoughts.
Sarah Sharp
On Mon, Jan 09, 2012 at 08:57:16PM -0500, Alan Stern wrote:
> On Mon, 9 Jan 2012, Sarah Sharp wrote:
>
> > 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).
>
> Okay, that's sensible.
>
> > 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);
>
> I would have arranged the code to avoid repeated tests:
>
> if (hub_is_superspeed(hdev)) {
> if (!(portchange & USB_PORT_STAT_C_LINK_STATE))
> return 0;
> clear_port_feature(hdev, port, USB_PORT_FEAT_C_LINK_STATE);
> } else {
> if (!(portchange & USB_PORT_STAT_C_SUSPEND))
> return 0;
> clear_port_feature(hdev, port, USB_PORT_FEAT_C_SUSPEND)
> }
>
> > + udev = hdev->children[port-1];
> > + if (udev) {
> > + /* TRSMRCY = 10 msec */
> > + msleep(10);
> > +
> > + usb_lock_device(udev);
> > + ret = usb_remote_wakeup(hdev->children[port-1]);
>
> Although this is simply a copy of the existing code, you might as well
> replace "hdev->children[port-1]" with "udev". I think the
> usb_remote_wakeup() call was there first, and when the udev local
> variable was added, the call's argument didn't get simplified.
>
> > + 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;
> > +}
>
> Otherwise fine.
>
> Alan Stern
>
--
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
