On Thu, 2014-03-13 at 17:33 -0400, Alan Stern wrote:
> 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.
I'm not convinced it's the best way forward, at least to me it still
seems prickly. In the development of the series I've been bitten by
deadlocks when trying to to put locking or wait_event() into the
usb_port_runtime_{suspend|resume} methods. Even if we make it deadlock
free now, it's an entanglement that future refactoring efforts (like
making usb_ports device-model parents of their child usb_devices) may
trip over. In general I think it is hard to audit descendant-device
runtime-pm events relative to the locking constraints of an ancestor.
> > 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.
Ok, will do just the helper and return value change.
>
> > @@ -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?
>
Ah, you're right. I deleted that check when you suggested this cleaner
way of achieving the "reference on behalf of the hispeed port" approach
back in v4.
http://marc.info/?l=linux-usb&m=139172072116604&w=2
> 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?
That is slightly cleaner, done.
> > + 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.
>
Yes, but I'm opting for "can't cause a deadlock" vs "probably won't
cause a deadlock".
after the cleanups
3 files changed, 16 insertions(+), 20 deletions(-)
...of course should have waited to get this acked first as it causes a
couple collisions in part2.
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.
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 | 22 +++-----------
drivers/usb/core/hub.h | 14 +++++++++
drivers/usb/core/port.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 93 insertions(+), 17 deletions(-)
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index c025edb90e1f..4e533b5f19d7 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, bool do_delay)
{
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,8 @@ 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;
+ msleep(hub_power_on_good_delay(hub));
}
static int hub_hub_status(struct usb_hub *hub,
@@ -1045,7 +1031,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, false);
PREPARE_DELAYED_WORK(&hub->init_work, hub_init_func2);
queue_delayed_work(system_power_efficient_wq,
&hub->init_work,
diff --git a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h
index 6858a55eceb5..8d63a420fb45 100644
--- a/drivers/usb/core/hub.h
+++ b/drivers/usb/core/hub.h
@@ -98,6 +98,7 @@ struct usb_port {
u8 portnum;
unsigned power_is_on:1;
unsigned did_runtime_put:1;
+ unsigned is_superspeed:1;
};
#define to_usb_port(_dev) \
@@ -125,6 +126,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..2e631cec7436 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 (!port_dev->is_superspeed && 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 (!port_dev->is_superspeed && peer)
+ pm_runtime_put(&peer->dev);
+
return retval;
}
#endif
@@ -153,6 +173,7 @@ static struct device_driver usb_port_driver = {
static int link_peers(struct usb_port *left, struct usb_port *right)
{
+ struct usb_port *ss_port, *hs_port;
int rc;
if (left->peer == right && right->peer == left)
@@ -178,9 +199,38 @@ 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.
+ */
+ if (left->is_superspeed) {
+ ss_port = left;
+ WARN_ON(right->is_superspeed);
+ hs_port = right;
+ } else {
+ ss_port = right;
+ WARN_ON(!right->is_superspeed);
+ 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 +250,37 @@ 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_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;
+
+ if (left->is_superspeed)
+ 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);
}
/*
@@ -319,6 +392,7 @@ int usb_hub_create_port_device(struct usb_hub *hub, int port1)
port_dev->dev.groups = port_dev_group;
port_dev->dev.type = &usb_port_device_type;
port_dev->dev.driver = &usb_port_driver;
+ port_dev->is_superspeed = hub_is_superspeed(hub->hdev);
dev_set_name(&port_dev->dev, "%s-port%d", dev_name(&hub->hdev->dev),
port1);
retval = device_register(&port_dev->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
