On Fri, 31 Jan 2014, Dan Williams wrote:
> 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.
> --- a/drivers/usb/core/port.c
> +++ b/drivers/usb/core/port.c
> @@ -77,12 +77,19 @@ static int usb_port_runtime_resume(struct device *dev)
> struct usb_device *hdev = to_usb_device(dev->parent->parent);
> struct usb_interface *intf = to_usb_interface(dev->parent);
> struct usb_hub *hub = usb_hub_to_struct_hub(hdev);
> + struct usb_port *peer = port_dev->peer;
> int port1 = port_dev->portnum;
> int retval;
>
> if (!hub)
> return -EINVAL;
>
> + /* power on our usb3 peer before this usb2 port to prevent a usb3
> + * device from degrading to its usb2 connection
> + */
> + if (!hub_is_superspeed(hdev) && peer)
> + pm_runtime_get_sync(&peer->dev);
> +
Have you considered what would happen if this runs before the ACPI data
causes the peers to be re-matched?
> usb_autopm_get_interface(intf);
> set_bit(port1, hub->busy_bits);
>
> @@ -104,6 +111,13 @@ static int usb_port_runtime_resume(struct device *dev)
>
> clear_bit(port1, hub->busy_bits);
> usb_autopm_put_interface(intf);
> +
> + /* usb3 peer is marked active so we can drop the reference we took to
> + * ensure ordering above
> + */
> + if (!hub_is_superspeed(hdev) && peer)
> + pm_runtime_put(&peer->dev);
I don't understand this. Doesn't it defeat your purpose? You still
want to prevent the SS port from suspending while the HS port remains
active, right?
> @@ -123,6 +138,16 @@ static int usb_port_runtime_suspend(struct device *dev)
> == PM_QOS_FLAGS_ALL)
> return -EAGAIN;
>
> + /* block poweroff of superspeed ports while highspeed peer is on */
> + dev_WARN_ONCE(&hdev->dev, hub_is_superspeed(hdev) && !peer,
> + "port%d missing peer info\n", port1);
> +
> + /* prevent a usb3 port from powering off while its usb2 peer is
> + * powered on
> + */
> + if (hub_is_superspeed(hdev) && (!peer || peer->power_is_on))
> + return -EBUSY;
You wouldn't have to do this if you eliminated the pm_runtime_put
above.
> @@ -130,6 +155,17 @@ 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
> + * queue a suspend request to observe that this usb2 peer port
> + * is now off. There is a small race since there is no way to
> + * flush this suspend, userspace is advised to order suspends.
> + */
> + if (!hub_is_superspeed(hdev) && peer) {
> + pm_runtime_get(&peer->dev);
> + pm_runtime_put(&peer->dev);
> + }
This is where that pm_runtime_put belongs. And you wouldn't need the
extraneous pm_runtime_get. In fact, the combination of those two async
calls probably doesn't do what you might think.
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
