On Thu, Jul 11, 2013 at 02:58:04PM -0400, Alan Stern wrote:
> The hub driver was recently changed to use "global" suspend for system
> suspend transitions on non-SuperSpeed buses. This means that we don't
> suspend devices individually by setting the suspend feature on the
> upstream hub port; instead devices all go into suspend automatically
> when the root hub stops transmitting packets. The idea was to save
> time and to avoid certain kinds of wakeup races.
>
> Now it turns out that many hubs are buggy; they don't relay wakeup
> requests from a downstream port to their upstream port if the
> downstream port's suspend feature is not set (depending on the speed
> of the downstream port, whether or not the hub is enabled for remote
> wakeup, and possibly other factors).
>
> We can't have hubs dropping wakeup requests. Therefore this patch
> goes partway back to the old policy: It sets the suspend feature for a
> port if the device attached to that port or any of its descendants is
> enabled for wakeup. People will still be able to benefit from the
> time savings if they don't care about wakeup and leave it disabled on
> all their devices.
>
I have a question what kinds of cases we can get the time saving?
- For no hub cases (like most embedded devices), it just moves
set suspendM at portsc from usb_port_suspend to ehci_bus_suspend
- For hub cases, since you said it must send suspend to hub at
global suspend case (system suspend procedure), this suspend
request procedure can't be skipped.
> In order to accomplish this, the patch adds a new field to the usb_hub
> structure: wakeup_enabled_descendants is a count of how many devices
> below a suspended hub are enabled for remote wakeup. A corresponding
> new subroutine determines the number of wakeup-enabled devices at or
> below an arbitrary suspended USB device.
>
> This should be applied to the 3.10 stable kernel.
>
> Signed-off-by: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
> Reported-and-tested-by: Toralf Förster <toralf.foerster@xxxxxx>
> CC: <stable@xxxxxxxxxxxxxxx>
>
> ---
>
>
> [as1692]
>
> drivers/usb/core/hub.c | 39 +++++++++++++++++++++++++++++++--------
> drivers/usb/core/hub.h | 3 +++
> 2 files changed, 34 insertions(+), 8 deletions(-)
>
> Index: usb-3.10/drivers/usb/core/hub.h
> ===================================================================
> --- usb-3.10.orig/drivers/usb/core/hub.h
> +++ usb-3.10/drivers/usb/core/hub.h
> @@ -59,6 +59,9 @@ struct usb_hub {
> struct usb_tt tt; /* Transaction Translator */
>
> unsigned mA_per_port; /* current for each child */
> +#ifdef CONFIG_PM
> + unsigned wakeup_enabled_descendants;
> +#endif
>
> unsigned limited_power:1;
> unsigned quiescing:1;
> Index: usb-3.10/drivers/usb/core/hub.c
> ===================================================================
> --- usb-3.10.orig/drivers/usb/core/hub.c
> +++ usb-3.10/drivers/usb/core/hub.c
> @@ -2848,6 +2848,15 @@ static int usb_disable_function_remotewa
> USB_CTRL_SET_TIMEOUT);
> }
>
> +/* Count of wakeup-enabled devices at or below udev */
> +static unsigned wakeup_enabled_descendants(struct usb_device *udev)
> +{
> + struct usb_hub *hub = usb_hub_to_struct_hub(udev);
> +
> + return udev->do_remote_wakeup +
> + (hub ? hub->wakeup_enabled_descendants : 0);
> +}
> +
> /*
> * usb_port_suspend - suspend a usb device's upstream port
> * @udev: device that's no longer in active use, not a root hub
> @@ -2888,8 +2897,8 @@ static int usb_disable_function_remotewa
> * Linux (2.6) currently has NO mechanisms to initiate that: no khubd
> * timer, no SRP, no requests through sysfs.
> *
> - * If Runtime PM isn't enabled or used, non-SuperSpeed devices really get
> - * suspended only when their bus goes into global suspend (i.e., the root
> + * If Runtime PM isn't enabled or used, non-SuperSpeed devices may not get
> + * suspended until their bus goes into global suspend (i.e., the root
> * hub is suspended). Nevertheless, we change @udev->state to
> * USB_STATE_SUSPENDED as this is the device's "logical" state. The actual
> * upstream port setting is stored in @udev->port_is_suspended.
> @@ -2960,15 +2969,21 @@ int usb_port_suspend(struct usb_device *
> /* see 7.1.7.6 */
> if (hub_is_superspeed(hub->hdev))
> status = hub_set_port_link_state(hub, port1, USB_SS_PORT_LS_U3);
> - else if (PMSG_IS_AUTO(msg))
> - status = set_port_feature(hub->hdev, port1,
> - USB_PORT_FEAT_SUSPEND);
> +
> /*
> * For system suspend, we do not need to enable the suspend feature
> * on individual USB-2 ports. The devices will automatically go
> * into suspend a few ms after the root hub stops sending packets.
> * The USB 2.0 spec calls this "global suspend".
> + *
> + * However, many USB hubs have a bug: They don't relay wakeup requests
> + * from a downstream port if the port's suspend feature isn't on.
> + * Therefore we will turn on the suspend feature if udev or any of its
> + * descendants is enabled for remote wakeup.
> */
> + else if (PMSG_IS_AUTO(msg) || wakeup_enabled_descendants(udev) > 0)
> + status = set_port_feature(hub->hdev, port1,
> + USB_PORT_FEAT_SUSPEND);
> else {
> really_suspend = false;
> status = 0;
> @@ -3003,15 +3018,16 @@ int usb_port_suspend(struct usb_device *
> if (!PMSG_IS_AUTO(msg))
> status = 0;
> } else {
> - /* device has up to 10 msec to fully suspend */
> dev_dbg(&udev->dev, "usb %ssuspend, wakeup %d\n",
> (PMSG_IS_AUTO(msg) ? "auto-" : ""),
> udev->do_remote_wakeup);
> - usb_set_device_state(udev, USB_STATE_SUSPENDED);
> if (really_suspend) {
> udev->port_is_suspended = 1;
> +
> + /* device has up to 10 msec to fully suspend */
> msleep(10);
> }
> + usb_set_device_state(udev, USB_STATE_SUSPENDED);
> }
>
> /*
> @@ -3293,7 +3309,11 @@ static int hub_suspend(struct usb_interf
> unsigned port1;
> int status;
>
> - /* Warn if children aren't already suspended */
> + /*
> + * Warn if children aren't already suspended.
> + * Also, add up the number of wakeup-enabled descendants.
> + */
> + hub->wakeup_enabled_descendants = 0;
> for (port1 = 1; port1 <= hdev->maxchild; port1++) {
> struct usb_device *udev;
>
> @@ -3303,6 +3323,9 @@ static int hub_suspend(struct usb_interf
> if (PMSG_IS_AUTO(msg))
> return -EBUSY;
> }
> + if (udev)
> + hub->wakeup_enabled_descendants +=
> + wakeup_enabled_descendants(udev);
> }
>
> if (hdev->do_remote_wakeup && hub->quirk_check_port_auto_suspend) {
>
>
--
Best Regards,
Peter Chen
--
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
