On Thu, 13 Mar 2014, Dan Williams wrote:
> Sorry for the delay, I know it's distracting to lose context like this.
No problem.
> > In fact, do we need the pre/post_modify actions at all? Wouldn't it be
> > sufficient to do just a pm_runtime_get_sync on the SuperSpeed peer?
>
> I think we miss a SuperSpeed suspend event that way:
>
> CPU1 CPU2
> link_peers(usb3, usb2)
> pm_runtime_get(usb3) /* wake */
> usb_port_runtime_suspend(usb2)
> usb3->peer = usb2
> /* usb2->peer == NULL */
> if (usb2->peer)
> pm_runtime_put(usb3) /* skip */
> usb2->peer = usb3
> pm_runtime_get(usb3) /* on behalf of usb2 */
> pm_runtime_put(usb3) /* miss suspend */
>
>
> ...but we don't necessarily need to wake the usb3 port at link time,
> that will happen as a matter of course of adding the reference on behalf
> of the usb2 port. See below.
You're right. But maybe a simple solution would be for the
runtime-suspend and -resume routines to acquire the usb_port_peer_mutex
whenever they are called for a USB-2 port. The mutex would protect
their accesses of port->peer, and I don't think it would result in any
deadlocks.
> Another revision in this patch is making sure the port resume sequence
> waits the hub-specified power-on delay before proceeding with reconnect
> timeouts. This cleaned up cases where the usb2-port powered on and
> connected before usb3.
Okay, good.
> 8<-----------
> usb: block suspension of superspeed port while hispeed peer is active
>
> From: Dan Williams <dan.j.williams@xxxxxxxxx>
>
> ClearPortFeature(PORT_POWER) on a usb3 port places the port in either a
> DSPORT.Powered-off-detect / DSPORT.Powered-off-reset loop, or the
> DSPORT.Powered-off state. There is no way to ensure that RX
> terminations will persist in this state, so it is possible a device will
> degrade to its usb2 connection. Prevent this by blocking power-off of a
> usb3 port while its usb2 peer is active, and powering on a usb3 port
> before its usb2 peer.
>
> By default the latency between peer power-on events is 0. In order for
> the device to not see usb2 active while usb3 is still powering up inject
> the hub recommended power_on_good delay.
>
> Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
> @@ -810,14 +805,9 @@ int usb_hub_clear_tt_buffer(struct urb *urb)
> }
> EXPORT_SYMBOL_GPL(usb_hub_clear_tt_buffer);
>
> -/* If do_delay is false, return the number of milliseconds the caller
> - * needs to delay.
> - */
> -static unsigned hub_power_on(struct usb_hub *hub, bool do_delay)
> +static void hub_power_on(struct usb_hub *hub)
> {
> int port1;
> - unsigned pgood_delay = hub->descriptor->bPwrOn2PwrGood * 2;
> - unsigned delay;
>
> /* Enable power on each port. Some hubs have reserved values
> * of LPSM (> 2) in their descriptors, even though they are
> @@ -836,12 +826,6 @@ static unsigned hub_power_on(struct usb_hub *hub, bool do_delay)
> else
> usb_clear_port_feature(hub->hdev, port1,
> USB_PORT_FEAT_POWER);
> -
> - /* Wait at least 100 msec for power to become stable */
> - delay = max(pgood_delay, (unsigned) 100);
> - if (do_delay)
> - msleep(delay);
> - return delay;
> }
I'm not sure about this. Separating out hub_power_on_good_delay into
its own routine is fine, and so is changing the return type to void.
But only one place calls hub_power_on without asking for the delay, so
we probably should keep the do_delay argument. Otherwise you end up
with msleep(hub_power_on_good_delay(hub)) sprinkled all over the place.
> @@ -1073,10 +1059,11 @@ static void hub_activate(struct usb_hub *hub, enum hub_activation_type type)
> "under this hub\n.");
> }
> }
> - hub_power_on(hub, true);
> + hub_power_on(hub);
> } else {
> - hub_power_on(hub, true);
> + hub_power_on(hub);
> }
> + msleep(hub_power_on_good_delay(hub));
Like here.
> @@ -178,9 +200,40 @@ static int link_peers(struct usb_port *left, struct usb_port *right)
> return rc;
> }
>
> + /*
> + * By default the SuperSpeed port will not suspend while its
> + * ->peer link is NULL,
Hmmm. Is this really true? If so, what is to prevent it?
And what about non-visible SS ports attached to built-in devices?
They might not have any peers, but they are prime candidates for
powering off.
> but we need to wake the HiSpeed port to
> + * make sure we don't race setting ->peer with
> + * usb_port_runtime_suspend(). Otherwise we may miss a suspend
> + * event for the SuperSpeed port.
> + */
> + l_hdev = to_usb_device(left->dev.parent->parent);
> + r_hdev = to_usb_device(right->dev.parent->parent);
> + if (hub_is_superspeed(l_hdev)) {
Would it be worthwhile adding an "is_superspeed" field to the port
structure?
> + ss_port = left;
> + WARN_ON(hub_is_superspeed(r_hdev));
> + hs_port = right;
> + } else {
> + ss_port = right;
> + WARN_ON(!hub_is_superspeed(r_hdev));
> + hs_port = left;
> + }
> + pm_runtime_get_sync(&hs_port->dev);
> +
> left->peer = right;
> right->peer = left;
>
> + /*
> + * The SuperSpeed reference is dropped when the HiSpeed port in
> + * this relationship suspends, i.e. when it is safe to allow a
> + * SuperSpeed connection to drop since there is no risk of a
> + * device degrading to its suspended HiSpeed connection
> + *
> + * Also drop the HiSpeed ref taken above
> + */
> + pm_runtime_get(&ss_port->dev);
> + pm_runtime_put(&hs_port->dev);
> +
> return 0;
> }
>
> @@ -200,14 +253,39 @@ static void link_peers_report(struct usb_port *left, struct usb_port *right)
>
> static void unlink_peers(struct usb_port *left, struct usb_port *right)
> {
> + struct usb_device *l_hdev;
> + struct usb_port *ss_port;
> +
> WARN(right->peer != left || left->peer != right,
> "%s and %s are not peers?\n",
> dev_name(&left->dev), dev_name(&right->dev));
>
> + /*
> + * We wake the SuperSpeed port when unlinking since it must be
> + * active while ->peer is NULL. We wake the HiSpeed port to
> + * make sure we don't race it's usb_port_runtime_resume() event
> + * which takes a SuperSpeed ref when ->peer is !NULL
> + */
> + pm_runtime_get_sync(&left->dev);
> + pm_runtime_get_sync(&right->dev);
> +
> sysfs_remove_link(&left->dev.kobj, "peer");
> right->peer = NULL;
> sysfs_remove_link(&right->dev.kobj, "peer");
> left->peer = NULL;
> +
> + l_hdev = to_usb_device(left->dev.parent->parent);
> + if (hub_is_superspeed(l_hdev))
> + ss_port = left;
> + else
> + ss_port = right;
> +
> + /* drop the SuperSpeed ref held on behalf of the active HiSpeed port */
> + pm_runtime_put(&ss_port->dev);
> +
> + /* drop the refs taken above */
> + pm_runtime_put(&left->dev);
> + pm_runtime_put(&right->dev);
If we adopt the change mentioned near the top of this email then
link_peers merely has to do pm_runtime_get_sync on the USB-3 port if
the USB-2 port is active. Similarly, unlink_peers merely has to do a
pm_runtime_put on the USB-3 port if the USB-2 port is active.
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
