On Thu, Jan 26, 2012 at 01:36:29PM -0500, Alan Stern wrote:
> On Wed, 25 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 host-initiated resume finishes, the hub sets
> > the port link state change bit.
> >
> > When a USB 3.0 device initiates remote wakeup, the parent hubs with
> > their upstream links in U3 will pass the LFPS up the chain. The first
> > hub that has an upstream link in U0 (which may be the roothub) will
> > reflect that LFPS back down the path to the device.
> >
> > However, the parent hubs in the resumed path will not set their link
> > state change bit. Instead, the device that initiated the resume has to
> > send an asynchronous "Function Wake" Device Notification up to the host
> > controller.
> >
> > Unfortunately, this notification can take up to 2.5 seconds
> > (tNotification in section 8.13 of the USB3 spec). That means that we
> > need to prevent the parent hub from being suspended before the
> > notification has been sent.
>
> Do we really need to worry about this? The worst that can happen is
> the parent hub suspends, then the device requests another wakeup and
> sends another notification.
The device will send another device notification, but it won't attempt
to signal a resume, because from its standpoint, the link state is still
in U0. Here's why:
All USB 3.0 hubs are powered by wall worts. USB 2.0 hubs suspend after
they haven't seen an SOF for a period of time, but they got rid of SOFs
for USB 3.0 devices (too much power consumption). USB 3.0 devices and
hubs communicate via link commands to transition between U0, U1, U2, and
U3. In this case, the child device thinks its link is in U0, and will
continue to do so until it gets an LGO_U3 request and responds with an
LAU (link accept U-state). But the parent hub only sends an LGO_U3 if
SW set that port's link state to U3. Obviously SW won't set that link
state to U3 because we think the device is still suspended.
So what does the USB 3.0 hub do if it receives an LG0_U3 request on its
upstream port, and one of its downstream ports is in a higher-power link
state (U0 in our case)?
The USB 3.0 spec is ambiguous about this behavior of the hub. The smart
thing to do would be accept the LGO_U3 command and immediately signal a
resume. However, in talking with the hub chapter owner, he says there
is some ambiguity about whether the hub will do that, or simply ignore
the LGO_U3 command.
If it does ignore the LGO_U3 command, the results are pretty disastrous.
After automatically trying the U3 transition three times, the parent hub
will place the port in SS.Inactive, and signal a port status change
event to SW. Then we have to issue a warm reset on that port, and I
think that means we'll have to re-enumerate the device.
So I would like to avoid this ambiguity by starting the resume from the
roothub down when the roothub indicates there was resume signaling. Of
course, that doesn't cover the case where the link between the roothub
and the first-tier hub is in U0, like so:
roothub
| (U0)
hub A
| (U3)
hub B
| (U3)
device C
If device C signals a remote wakeup, hub A will reflect the resume
signaling, but not give any indication to the host SW that it has done
so. I'm working to see if we can get a spec errata or hub compliance
test to make sure that the hubs have the sane behavior of accepting the
LGO_U3 and immediately transitioning back to U0. There are no
certified hubs out there yet, but you can certainly find them online at
Amazon and Newegg. I have no idea if they actually handle U3, or
they'll just ignore the LGO_U3, or they'll do the sane thing of
immediately resuming.
> Also, what you're saying makes no sense. We have no way to know that
> the parent hub should be prevented from suspending, because we don't
> realize that anything special has happened until we receive the
> notification.
We do, if the link between hub A and the roothub is in U3. Then the
roothub will reflect the resume signaling, the xHCI driver gets a port
status change event, and it can call usb_super_speed_remote_wakeup() for
the roothub.
> > First, make the xHCI roothub act like an external USB 3.0 hub and not
> > pass up the port link state change bit when a device-initiated resume
> > finishes. Introduce a new xHCI bit field, port_remote_wakeup, so that
> > we can tell the difference between a port coming out of the U3Exit state
> > (host-initiated resume) and the RExit state (ending state of
> > device-initiated resume).
> >
> > Since the USB core can't tell whether a port on a hub has resumed by
> > looking at the Hub Status buffer, we need to introduce a bitfield,
> > wakeup_bits, that indicates which ports have resumed. When the xHCI
> > driver notices a port finishing a device-initiated resume, we call into
> > a new USB core function, usb_super_speed_remote_wakeup(), that will set
> > the right bit in wakeup_bits, and kick khubd for that hub.
>
> Why do we need to do this? Won't the device itself send a wakeup
> notification?
khubd's hub_events() looks at the hub status, and each port bit will be
set if there is a port status change bit set. However, the link state
change bits don't change when a device-initiated resume transitions the
port from U3 to U0. hub_events() sees nothing has changed when it polls
the hub, so it skips that port. Eventually the hub gets suspended
again.
Basically, the wakeup_bits are supposed to work like the USB 2.0 hub
suspend change bits. When we're tracing down the roothub to find out
which device caused resume signaling on a root port, we need to set
these bits for all child ports, so hub_events() double checks the link
status of each child port. If nothing has changed on that port,
hub_events just moves on.
> After all, we don't do anything when a non-root hub finishes a
> device-initiated resume. In fact, we don't even know it has happened
> until the notification arrives -- the hub itself doesn't tell us about
> it. Root hubs should behave the same way.
>
> > We also call usb_super_speed_remote_wakeup() when the Function Wake
> > Device Notification is received by the xHCI driver. This covers the
> > case where the link between the roothub and the first-tier hub is in U0,
>
> Or second-tier, or higher.
>
> > and the hub reflects the resume signaling back to the device without
> > giving any indication it has done so until the device sends the Function
> > Wake notification.
> >
> > Change the code in khubd that handles the remote wakeup to look at the
> > state the USB core thinks the device is in, and handle the remote wakeup
> > if the port's wakeup bit is set.
> >
> > This patch only takes care of the case where the device is attached
> > directly to the roothub, or the USB 3.0 hub that is attached to the root
> > hub is the device sending the Function Wake Device Notification (e.g.
> > because a new USB device was attached). The other cases will be covered
> > in a second patch.
>
> I'm not clear on this. What difference does it make if the device
> sending the notification is not directly connected to the root hub?
It's because of the above corner case, where the host link is in U0, and
the external USB 3.0 hub reflects the resume signaling.
> > @@ -411,6 +413,34 @@ void usb_kick_khubd(struct usb_device *hdev)
> > kick_khubd(hub);
> > }
> >
> > +/*
> > + * USB 3.0 devices are supposed to send a Device Notification Function Wake
> > + * message after they are finished signaling resume. The USB 3.0 bus
> > + * specification does not specify the maximum amount of time between the end of
> > + * resume signaling and the first wake notification. It does specify
> > + * TNotification, the rate at which the device should send a Function Wake
> > + * notification if the device has not been accessed. That rate is 2.5 seconds,
> > + * which is far too long when the hub will be re-suspended by default after
> > + * 2 seconds.
> > + *
> > + * To prevent suspend of hubs while we're waiting for the device notification
> > + * to be sent, mark the resumed port as woken up and kick khubd for the hub.
> > + */
>
> This doesn't sound right at all. The purpose of this routine is to let
> khubd know that this port on this hub has changed to U0 because of a
> remote wakeup. It has nothing to do with notification rates or
> resuspends.
It's used for both the Function Wake device notification, and for the
case where a root port change event indicates there's been a device
resume, see the scenario above.
> > +void usb_super_speed_remote_wakeup(struct usb_device *hdev,
> > + unsigned int portnum)
> > +{
> > + struct usb_hub *hub;
> > +
> > + if (!hdev)
> > + return;
> > +
> > + hub = hdev_to_hub(hdev);
> > + if (hub) {
> > + set_bit(portnum, hub->wakeup_bits);
> > + kick_khubd(hub);
> > + }
> > +}
> > +EXPORT_SYMBOL_GPL(usb_super_speed_remote_wakeup);
> >
> > /* completion function, fires on port status changes and various faults */
> > static void hub_irq(struct urb *urb)
> > @@ -807,12 +837,6 @@ 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) {
> > - need_debounce_delay = true;
> > - clear_port_feature(hub->hdev, port1,
> > - USB_PORT_FEAT_C_PORT_LINK_STATE);
> > - }
>
> Are you sure about this? Won't the hub's interrupt URB fire if the
> C_PORT_LINK_STATE status-change bit is set? You probably want to leave
> the clear_port_feature() call and get rid of the need_debounce_delay
> line.
You have to apply the whole patchset to see that the link state change
bit will get cleared. This code was removed simply because it's before
the remote wakeup checking. The link state change bit clearing was
moved after it in a previous patch. So even if there's no resume, the
link state change bit should still get cleared.
> > -
> > if ((portchange & USB_PORT_STAT_C_BH_RESET) &&
> > hub_is_superspeed(hub->hdev)) {
> > need_debounce_delay = true;
> > @@ -3458,11 +3482,18 @@ static int hub_handle_remote_wakeup(struct usb_hub *hub, unsigned int port,
> > int ret;
> >
> > hdev = hub->hdev;
> > - 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 (!hub_is_superspeed(hdev)) {
> > + if (!(portchange & USB_PORT_STAT_C_SUSPEND))
> > + return 0;
> > + clear_port_feature(hdev, port, USB_PORT_FEAT_C_SUSPEND);
> > + } else {
> > + if (!udev || udev->state != USB_STATE_SUSPENDED ||
> > + !test_and_clear_bit(udev->portnum,
> > + hub->wakeup_bits))
>
> I'm concerned that, being buried at the end of a short-circuit boolean
> expression like this, the wakeup_bits entry might not get cleared when
> it should. You can move it to the start of the test expression and
> replace udev->portnum with port.
Ah, right, thanks.
> > + return 0;
> > + }
> > +
> > if (udev) {
> > /* TRSMRCY = 10 msec */
> > msleep(10);
>
> > @@ -3648,8 +3680,8 @@ static void hub_events(void)
> > clear_port_feature(hdev, i,
> > USB_PORT_FEAT_C_BH_PORT_RESET);
> > }
> > - if (portchange & USB_PORT_STAT_C_LINK_STATE) {
> > - clear_port_feature(hub->hdev, i,
> > + if (portchange & USB_PORT_FEAT_C_PORT_LINK_STATE) {
>
> This should remain the way it was. The values in portchange are status
> flags, not feature bits.
>
> > + clear_port_feature(hdev, i,
> > USB_PORT_FEAT_C_PORT_LINK_STATE);
Ok, sure.
> > }
> > if (portchange & USB_PORT_STAT_C_CONFIG_ERROR) {
>
> > --- a/include/linux/usb/hcd.h
> > +++ b/include/linux/usb/hcd.h
> > @@ -412,6 +412,8 @@ extern irqreturn_t usb_hcd_irq(int irq, void
> *__hcd);
> >
> > extern void usb_hc_died(struct usb_hcd *hcd);
> > extern void usb_hcd_poll_rh_status(struct usb_hcd *hcd);
> > +extern void usb_super_speed_remote_wakeup(struct usb_device *roothub,
>
> This should say hdev, not roothub.
Ok, I'll change that.
>
> > + unsigned int portnum);
> >
> > /* The D0/D1 toggle bits ... USE WITH CAUTION (they're almost hcd-internal) $
> > #define usb_gettoggle(dev, ep, out) (((dev)->toggle[out] >> (ep)) & 1)
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
