On Fri, 6 Jan 2012, Sarah Sharp wrote:
> USB 3.0 hubs have a different remote wakeup policy than USB 2.0 hubs.
> USB 2.0 hubs, once they have remote wakeup enabled, will always send
> remote wakes when anything changes on a port.
>
> However, USB 3.0 hubs have a per-port remote wake up policy that is off
> by default. The Set Feature remote wake mask can be changed for any
> port, enabling remote wakeup for a connect, disconnect, or overcurrent
> event, much like EHCI and xHCI host controller "wake on" port status
> bits. The bits are cleared to zero on the initial hub power on, or
> after the hub has been reset.
>
> Without this patch, when a USB 3.0 hub gets suspended, it will not send
> a remote wakeup on device connect or disconnect. This would show up to
> the user as "dead ports" unless they ran lsusb -v (since newer versions
> of lsusb use the sysfs files, rather than sending control transfers).
>
> Change the hub driver's suspend method to enable remote wake up for
> disconnect, connect, and overcurrent for all ports on the hub. Modify
> the xHCI driver's roothub code to handle that request, and set the "wake
> on" bits in the port status registers accordingly.
>
> Alan: there are three things I'm uncertain about here, and I would
> appreciate your input.
>
> First, should we follow what EHCI and xHCI do during bus suspend, by
> only enabling disconnect and overcurrent wake notifications for ports
> with a device connected to a USB 3.0 hub, and enabling connect and
> overcurrent wake notifications for ports without a connection? I've
> long wondered why xHCI can't just set all the wake-on bits for a port,
> rather than try to make the racy distinction between connected and
> unconnected ports.
I can't give you a direct answer; it may depend on the design of the
hubs. Or maybe the USB-3.0 spec explicitly says; I haven't checked.
In the case of EHCI, there's a simple reason for the way we do it now:
Some Intel controllers don't work right if you set the inapplicable
bits. At least, that's what I was told by somebody at Intel. You
ought to be able to find out who it was (I don't remember) by seeing
who wrote the commit that changed the behavior -- previously we always
enabled all the bits.
The same may be true for xHCI; again you should be able to find
someone at Intel who knows.
> Second, what is the expected behavior of system suspend when
> CONFIG_USB_SUSPEND is turned off? It looks like in the current EHCI and
> xHCI code that the "wake on" bits are set in bus_suspend. Does that
> method get called if CONFIG_USB_SUSPEND is turned off? Is the intention
> that the system will resume if a new USB device is plugged in, even if
> CONFIG_USB_SUSPEND is off?
If CONFIG_USB_SUSPEND isn't enabled then the kernel never sets any
port-suspend features. It would be surprising if anybody leaves it
disabled nowadays (the default really should be changed to Y -- I
suggested this to Dave Brownell years ago and he agreed, but I never
got around to it). In addition, the kernel never sets any
wakeup-enable features, so non-root devices won't do remote wakeups.
However, the bus_suspend routines _do_ get called. More or less as a
side effect, remote wakeup will work for root ports. But people
shouldn't depend on this; if they want remote wakeup to work then they
should enable CONFIG_USB_SUSPEND.
> I ask because it seems like hub_suspend is not going to be called for
> the USB 3.0 roothub if CONFIG_USB_SUSPEND is off. If I want to be sure
> the wake on bits are set for USB 3.0 ports (rather than relying on the
> new code in hub_suspend), I have to have duplicate code in xHCI's
> bus_suspend method to set those bits.
No, you must have misread the code. hub_suspend and all the other
drivers' suspend methods get called whether or not CONFIG_USB_SUSPEND
is enabled. The only real effect is in usb_port_suspend -- without
CONFIG_USB_SUSPEND that routine doesn't do anything.
> Third, should we only set the wake on bits for USB 3.0 hubs during the
> hub initialization, or should we wait to do that in hub_suspend?
Well, the wakeup setting can be changed at any time by the user.
Therefore you can't just set those bits once and forget about them; you
have to set them properly every time the hub suspends.
There remains a question of whether they should be cleared in
hub_resume. Does it hurt to leave the bits enabled while the hub is
active?
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -2691,6 +2691,7 @@ static int hub_suspend(struct usb_interface *intf, pm_message_t msg)
> struct usb_hub *hub = usb_get_intfdata (intf);
> struct usb_device *hdev = hub->hdev;
> unsigned port1;
> + int status;
>
> /* Warn if children aren't already suspended */
> for (port1 = 1; port1 <= hdev->maxchild; port1++) {
> @@ -2703,6 +2704,17 @@ static int hub_suspend(struct usb_interface *intf, pm_message_t msg)
> return -EBUSY;
> }
> }
> + if (hub_is_superspeed(hdev) && hdev->do_remote_wakeup) {
> + /* Enable hub to send remote wakeup for all ports. */
> + for (port1 = 1; port1 <= hdev->maxchild; port1++) {
> + status = set_port_feature(hdev,
> + port1 |
> + USB_PORT_FEAT_REMOTE_WAKE_CONNECT |
> + USB_PORT_FEAT_REMOTE_WAKE_DISCONNECT |
> + USB_PORT_FEAT_REMOTE_WAKE_OVER_CURRENT,
> + USB_PORT_FEAT_REMOTE_WAKE_MASK);
> + }
> + }
>
> dev_dbg(&intf->dev, "%s\n", __func__);
Does "status" get used for anything? If not, why have it?
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
