On Wed, 5 Mar 2014, Dan Williams wrote:
> 8<---------
> Subject: 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.
>
> Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
> @@ -129,6 +139,15 @@ static int usb_port_runtime_suspend(struct device *dev)
> usb_clear_port_feature(hdev, port1, USB_PORT_FEAT_C_ENABLE);
> clear_bit(port1, hub->busy_bits);
> usb_autopm_put_interface(intf);
> +
> + /*
> + * Our peer usb3 port may now be able to suspend, asynchronously
s/,/, so/
> + * queue a suspend request to observe that this usb2 peer port
s/peer //
> + * is now off.
> + */
> + if (!hub_is_superspeed(hdev) && peer)
> + pm_runtime_put(&peer->dev);
> +
> return retval;
> }
> #endif
> @@ -151,8 +170,26 @@ static struct device_driver usb_port_driver = {
> .owner = THIS_MODULE,
> };
>
> +/*
> + * Modifying ->peer affects usb_port_runtime_{suspend|resume} so make
> + * sure devices are active before the change and re-evaluate
> + * afterwards
> + */
> +static void pre_modify_peers(struct usb_port *left, struct usb_port *right)
> +{
> + pm_runtime_get_sync(&left->dev);
> + pm_runtime_get_sync(&right->dev);
> +}
> +
> +static void post_modify_peers(struct usb_port *left, struct usb_port *right)
> +{
> + pm_runtime_put(&left->dev);
> + pm_runtime_put(&right->dev);
> +}
You can put the contents of these routines directly inline; no need to
split them out.
> @@ -178,9 +215,26 @@ static int link_peers(struct usb_port *left, struct usb_port *right)
> return rc;
> }
>
> + pre_modify_peers(left, right);
> left->peer = right;
> right->peer = left;
>
> + /*
> + * Ports are peer linked, hold a reference on the superspeed
> + * port which the hispeed port drops when it suspends. This
> + * ensures that superspeed ports only suspend after their
> + * hispeed peer.
> + */
> + ldev = to_usb_device(left->dev.parent->parent);
> + rdev = to_usb_device(right->dev.parent->parent);
Please call these lhdev and rhdev.
> + if (hub_is_superspeed(ldev))
> + pm_runtime_get_noresume(&left->dev);
> + else {
> + WARN_ON(!hub_is_superspeed(rdev));
> + pm_runtime_get_noresume(&right->dev);
> + }
> + post_modify_peers(left, right);
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?
> @@ -200,14 +254,32 @@ 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 *ldev, *rdev;
> +
> WARN(right->peer != left || left->peer != right,
> "%s and %s are not peers?\n",
> dev_name(&left->dev), dev_name(&right->dev));
>
> + pre_modify_peers(left, right);
I don't think this is necessary. If the peer is already suspended, why
power it up again?
> sysfs_remove_link(&left->dev.kobj, "peer");
> right->peer = NULL;
> sysfs_remove_link(&right->dev.kobj, "peer");
> left->peer = NULL;
> +
> + /*
> + * Ports are no longer peer linked, drop the reference that
> + * keeps the superspeed port (may be 'right' or 'left') powered
> + * when its peer is active
> + */
> + ldev = to_usb_device(left->dev.parent->parent);
> + rdev = to_usb_device(right->dev.parent->parent);
Again, lhdev and rhdev.
> + if (hub_is_superspeed(ldev))
> + pm_runtime_put_noidle(&left->dev);
> + else {
> + WARN_ON(!hub_is_superspeed(rdev));
The WARN_ON above should be sufficient; we don't need this one as well.
> + pm_runtime_put_noidle(&right->dev);
These calls should become pm_runtime_put().
> + }
> + post_modify_peers(left, right);
Not needed.
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
