On Sat, Jan 07, 2012 at 09:29:00PM -0500, Alan Stern wrote:
> On Fri, 6 Jan 2012, Sarah Sharp wrote:
>
> > USB 3.0 hubs don't have a port suspend change bit (that bit is now
> > reserved). Instead, when a port resumes, the hub sets the port link
> > state change bit. (An xHCI roothub does have a resume change bit, but
> > we don't pass it up to the USB core because it's different behavior from
> > external USB 3.0 hubs.)
>
> > XXX: We could clear PLC in hub_activate() when hub_activate() is called for a
> > hub resume only (HUB_RESUME). I'm not sure about the port state machine here.
> > Alan?
>
> Why would you want to clear it there? As far as I can see,
> C_LINK_STATE on a USB-3 hub is more or less equivalent to C_SUSPEND on
> a USB-2 hub. Therefore the HUB_RESUME case should treat them the same.
> Or have I misunderstood something?
Oh, sorry, that was an old comment I left in the patch. You're right
that the link state change is basically like a suspend change. However,
a link state change can also mean that something bad happened after we
enabled the USB 3.0 link power management. But I think we should cross
that bridge when the USB 3.0 LPM code gets added.
> > --- a/drivers/usb/core/hub.c
> > +++ b/drivers/usb/core/hub.c
> > @@ -807,11 +807,11 @@ static void hub_activate(struct usb_hub *hub, enum hub_activation_type type)
> > clear_port_feature(hub->hdev, port1,
> > USB_PORT_FEAT_C_ENABLE);
> > }
> > - if (portchange & USB_PORT_STAT_C_LINK_STATE) {
> > + /* We can't clear the link state change here because it's the
> > + * only indication we have a resume on a port!
> > + */
> > + if (portchange & USB_PORT_STAT_C_LINK_STATE)
> > need_debounce_delay = true;
> > - clear_port_feature(hub->hdev, port1,
> > - USB_PORT_FEAT_C_PORT_LINK_STATE);
> > - }
>
> Is the debounce delay
> really needed in this case? Maybe the whole test can be removed.
> There's no comparable test for C_SUSPEND.
I wasn't sure if the debounce delay was necessary. I'll remove it and
see if khubd still notices the port has resumed.
> > @@ -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, sure. The whole of hub_events() is pretty ugly, but I'm not sure
how to make it better. The xHCI hub handler is also pretty ugly. Maybe
there's something unique to port status handling that leads to long,
ugly code (possibly the sheer amount of status bit cases?).
Sarah Sharp
--
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
