On Fri, 2014-02-21 at 16:09 -0800, 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.
>
> Signed-off-by: Dan Williams <[email protected]>
> ---
> drivers/usb/core/hub.c | 5 -----
> drivers/usb/core/hub.h | 5 +++++
> drivers/usb/core/port.c | 50
> +++++++++++++++++++++++++++++++++++++++++++----
> 3 files changed, 51 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 4e967f613e70..479abbe0ba09 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -36,11 +36,6 @@
> #define USB_VENDOR_GENESYS_LOGIC 0x05e3
> #define HUB_QUIRK_CHECK_PORT_AUTOSUSPEND 0x01
>
> -static inline int hub_is_superspeed(struct usb_device *hdev)
> -{
> - return (hdev->descriptor.bDeviceProtocol == USB_HUB_PR_SS);
> -}
> -
> /* Protect struct usb_device->state and ->children members
> * Note: Both are also protected by ->dev.sem, except that ->state can
> * change to USB_STATE_NOTATTACHED even when the semaphore isn't held. */
> diff --git a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h
> index e7a9666e7cd6..87efea1424d3 100644
> --- a/drivers/usb/core/hub.h
> +++ b/drivers/usb/core/hub.h
> @@ -129,6 +129,11 @@ static inline bool hub_is_port_power_switchable(struct
> usb_hub *hub)
> return (le16_to_cpu(hcs) & HUB_CHAR_LPSM) < HUB_CHAR_NO_LPSM;
> }
>
> +static inline int hub_is_superspeed(struct usb_device *hdev)
> +{
> + return hdev->descriptor.bDeviceProtocol == USB_HUB_PR_SS;
> +}
> +
> static inline int hub_port_debounce_be_connected(struct usb_hub *hub,
> int port1)
> {
> diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
> index 86e78bbd2e4e..1e38f123ed8c 100644
> --- a/drivers/usb/core/port.c
> +++ b/drivers/usb/core/port.c
> @@ -77,12 +77,20 @@ 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);
> +
> usb_autopm_get_interface(intf);
> set_bit(port1, hub->busy_bits);
>
> @@ -104,6 +112,7 @@ static int usb_port_runtime_resume(struct device *dev)
>
> clear_bit(port1, hub->busy_bits);
> usb_autopm_put_interface(intf);
> +
> return retval;
> }
>
> @@ -113,6 +122,7 @@ static int usb_port_runtime_suspend(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;
>
> @@ -130,6 +140,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
> + * queue a suspend request to observe that this usb2 peer port
> + * is now off.
> + */
> + if (!hub_is_superspeed(hdev) && peer)
> + pm_runtime_put(&peer->dev);
> +
> return retval;
> }
> #endif
> @@ -198,13 +217,12 @@ static struct usb_port *find_default_peer(struct
> usb_hub *hub, int port1)
>
> static int link_peers(struct usb_port *left, struct usb_port *right)
> {
> - struct device *rdev;
> - struct device *ldev;
> + struct device *ldev = left->dev.parent->parent;
> + struct device *rdev = right->dev.parent->parent;
> int rc;
>
> if (left->peer) {
> right = left->peer;
> - ldev = left->dev.parent->parent;
> rdev = right->dev.parent->parent;
>
> WARN_ONCE(1, "%s port%d already peered with %s %d\n",
> @@ -214,7 +232,6 @@ static int link_peers(struct usb_port *left, struct
> usb_port *right)
> } else if (right->peer) {
> left = right->peer;
> ldev = left->dev.parent->parent;
> - rdev = right->dev.parent->parent;
>
> WARN_ONCE(1, "%s port%d already peered with %s %d\n",
> dev_name(rdev), right->portnum, dev_name(ldev),
> @@ -236,6 +253,19 @@ static int link_peers(struct usb_port *left, struct
> usb_port *right)
> get_device(&left->dev);
> 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.
> + */
> + if (hub_is_superspeed(to_usb_device(ldev)))
> + pm_runtime_get_noresume(&left->dev);
> + else {
> + WARN_ON_ONCE(!hub_is_superspeed(to_usb_device(rdev)));
> + pm_runtime_get_noresume(&right->dev);
> + }
> +
> return 0;
> }
>
> @@ -273,6 +303,18 @@ static void unlink_peers(struct usb_port *left, struct
> usb_port *right)
> sysfs_remove_link(&right->dev.kobj, "peer");
> put_device(&right->dev);
> 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
> + */
> + if (hub_is_superspeed(to_usb_device(ldev)))
> + pm_runtime_put_noidle(&left->dev);
> + else {
> + WARN_ON_ONCE(!hub_is_superspeed(to_usb_device(rdev)));
> + pm_runtime_put_noidle(&right->dev);
> + }
> }
>
As mentioned in the comments on patch 2, while ->peer is being modified
we don't want usb_port_runtime_{suspend|resume} to run. Introduce
pre_modify_peers() and post_modify_peers() to close that hole.
8<-----------
Subject: usb: defer suspension of superspeed port while peer is powered
From: Dan Williams <[email protected]>
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 <[email protected]>
---
drivers/usb/core/hub.c | 5 ---
drivers/usb/core/hub.h | 5 +++
drivers/usb/core/port.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 77 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 4e967f613e70..479abbe0ba09 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -36,11 +36,6 @@
#define USB_VENDOR_GENESYS_LOGIC 0x05e3
#define HUB_QUIRK_CHECK_PORT_AUTOSUSPEND 0x01
-static inline int hub_is_superspeed(struct usb_device *hdev)
-{
- return (hdev->descriptor.bDeviceProtocol == USB_HUB_PR_SS);
-}
-
/* Protect struct usb_device->state and ->children members
* Note: Both are also protected by ->dev.sem, except that ->state can
* change to USB_STATE_NOTATTACHED even when the semaphore isn't held. */
diff --git a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h
index e7a9666e7cd6..87efea1424d3 100644
--- a/drivers/usb/core/hub.h
+++ b/drivers/usb/core/hub.h
@@ -129,6 +129,11 @@ static inline bool hub_is_port_power_switchable(struct
usb_hub *hub)
return (le16_to_cpu(hcs) & HUB_CHAR_LPSM) < HUB_CHAR_NO_LPSM;
}
+static inline int hub_is_superspeed(struct usb_device *hdev)
+{
+ return hdev->descriptor.bDeviceProtocol == USB_HUB_PR_SS;
+}
+
static inline int hub_port_debounce_be_connected(struct usb_hub *hub,
int port1)
{
diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
index 2a560cd64758..f21567889019 100644
--- a/drivers/usb/core/port.c
+++ b/drivers/usb/core/port.c
@@ -77,12 +77,20 @@ 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);
+
usb_autopm_get_interface(intf);
set_bit(port1, hub->busy_bits);
@@ -104,6 +112,7 @@ static int usb_port_runtime_resume(struct device *dev)
clear_bit(port1, hub->busy_bits);
usb_autopm_put_interface(intf);
+
return retval;
}
@@ -113,6 +122,7 @@ static int usb_port_runtime_suspend(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;
@@ -130,6 +140,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
+ * queue a suspend request to observe that this usb2 peer port
+ * is now off.
+ */
+ if (!hub_is_superspeed(hdev) && peer)
+ pm_runtime_put(&peer->dev);
+
return retval;
}
#endif
@@ -196,8 +215,26 @@ static struct usb_port *find_default_peer(struct usb_hub
*hub, int port1)
return peer;
}
+/*
+ * 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);
+}
+
static int link_peers(struct usb_port *left, struct usb_port *right)
{
+ struct usb_device *ldev, *rdev;
int rc;
if (left->peer == right && right->peer == left)
@@ -225,11 +262,28 @@ static int link_peers(struct usb_port *left, struct
usb_port *right)
return rc;
}
+ pre_modify_peers(left, right);
get_device(&right->dev);
left->peer = right;
get_device(&left->dev);
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);
+ 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);
+
return 0;
}
@@ -249,16 +303,34 @@ 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);
sysfs_remove_link(&left->dev.kobj, "peer");
put_device(&left->dev);
right->peer = NULL;
sysfs_remove_link(&right->dev.kobj, "peer");
put_device(&right->dev);
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);
+ if (hub_is_superspeed(ldev))
+ pm_runtime_put_noidle(&left->dev);
+ else {
+ WARN_ON(!hub_is_superspeed(rdev));
+ pm_runtime_put_noidle(&right->dev);
+ }
+ post_modify_peers(left, right);
}
/**
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
