Sorry for the delay, I know it's distracting to lose context like this.
I was traveling this week for a personal matter, and I spent some extra
time experimenting and convincing myself that we do not want to
warm-reset by default.
Picking up where we left off...
On Thu, 2014-03-06 at 15:14 -0500, Alan Stern wrote:
> 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 //
ok.
>
> > + * 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.
>
done.
> > @@ -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.
>
Point taken. I decided to call them l_hdev and r_hdev, because rhdev
already means "root-hub hdev".
> > + 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?
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.
> > @@ -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?
>
To make sure the usb2 port does not suspend while we are taking a usb3
ref on its behalf.
> > 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.
>
yup.
> > + 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.
>
true.
> > + pm_runtime_put_noidle(&right->dev);
>
> These calls should become pm_runtime_put().
>
Ok.
> > + }
> > + post_modify_peers(left, right);
>
> Not needed.
>
The ref could go negative otherwise. I added comments to clarify.
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.
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>
---
drivers/usb/core/hub.c | 33 +++++++-------------
drivers/usb/core/hub.h | 13 ++++++++
drivers/usb/core/port.c | 78 +++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 102 insertions(+), 22 deletions(-)
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index c025edb90e1f..3ec50009be5a 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. */
@@ -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;
}
static int hub_hub_status(struct usb_hub *hub,
@@ -1045,7 +1029,9 @@ static void hub_activate(struct usb_hub *hub, enum hub_activation_type type)
* for HUB_POST_RESET, but it's easier not to.
*/
if (type == HUB_INIT) {
- delay = hub_power_on(hub, false);
+ unsigned delay = hub_power_on_good_delay(hub);
+
+ hub_power_on(hub);
PREPARE_DELAYED_WORK(&hub->init_work, hub_init_func2);
queue_delayed_work(system_power_efficient_wq,
&hub->init_work,
@@ -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));
}
init2:
@@ -4824,7 +4811,8 @@ static void hub_events(void)
usb_clear_port_feature(hdev, i,
USB_PORT_FEAT_C_OVER_CURRENT);
msleep(100); /* Cool down */
- hub_power_on(hub, true);
+ hub_power_on(hub);
+ msleep(hub_power_on_good_delay(hub));
hub_port_status(hub, i, &status, &unused);
if (status & USB_PORT_STAT_OVERCURRENT)
dev_err(&port_dev->dev,
@@ -4903,7 +4891,8 @@ static void hub_events(void)
dev_dbg(hub_dev, "over-current change\n");
clear_hub_feature(hdev, C_HUB_OVER_CURRENT);
msleep(500); /* Cool down */
- hub_power_on(hub, true);
+ hub_power_on(hub);
+ msleep(hub_power_on_good_delay(hub));
hub_hub_status(hub, &status, &unused);
if (status & HUB_STATUS_OVERCURRENT)
dev_err(hub_dev, "over-current "
diff --git a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h
index 6858a55eceb5..2537bfbf7aca 100644
--- a/drivers/usb/core/hub.h
+++ b/drivers/usb/core/hub.h
@@ -125,6 +125,19 @@ 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 unsigned hub_power_on_good_delay(struct usb_hub *hub)
+{
+ unsigned delay = hub->descriptor->bPwrOn2PwrGood * 2;
+
+ /* Wait at least 100 msec for power to become stable */
+ return max(delay, 100U);
+}
+
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 b7f5225cee2b..9d9a3bb00f40 100644
--- a/drivers/usb/core/port.c
+++ b/drivers/usb/core/port.c
@@ -76,16 +76,25 @@ 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);
retval = usb_hub_set_port_power(hdev, hub, port1, true);
+ msleep(hub_power_on_good_delay(hub));
if (port_dev->child && !retval) {
/*
* Attempt to wait for usb hub port to be reconnected in order
@@ -103,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;
}
@@ -112,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;
@@ -129,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, so
+ * asynchronously queue a suspend request to observe that this
+ * usb2 port is now off.
+ */
+ if (!hub_is_superspeed(hdev) && peer)
+ pm_runtime_put(&peer->dev);
+
return retval;
}
#endif
@@ -153,6 +173,8 @@ static struct device_driver usb_port_driver = {
static int link_peers(struct usb_port *left, struct usb_port *right)
{
+ struct usb_device *l_hdev, *r_hdev;
+ struct usb_port *ss_port, *hs_port;
int rc;
if (left->peer == right && right->peer == left)
@@ -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, 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)) {
+ 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);
}
/*
--
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
