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?
> --- 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);
> - }
I'd suggest leaving in the {}'s, even though technically the body is
only a single statement -- the comments make it look like a
multi-statement body. In addition, the comments need to be indented
one tab stop farther.
Or else just omit the {}'s and the comments. Is the debounce delay
really needed in this case? Maybe the whole test can be removed.
There's no comparable test for C_SUSPEND.
> @@ -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.
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
