On Fri, 2014-03-14 at 10:25 -0400, Alan Stern wrote:
> On Thu, 13 Mar 2014, Dan Williams wrote:
>
> > > 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.
>
> Okay, we can play it safe.
>
> > @@ -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,
>
> Weren't you going to remove this phrase?
My mistake, yes, it needs to go.
> > + /*
> > + * 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
>
> s/suspended/powered-off/
Done.
>
> > + *
> > + * Also drop the HiSpeed ref taken above
> > + */
>
> You're inconsistent about ending sentences in your comments with a
> period. Some of the comments in this patch do and some don't.
It's the e.e. cummings style of code commentary...
Fixed up.
>
> > + pm_runtime_get(&ss_port->dev);
>
> Should this be get_sync?
Yes, if we are in the RPM_RESUMING state after the _get() then the put
will return an error, so it should be _get_sync().
> There's some question about how to handle the
> power states when the peering relation is first determined.
>
> For example, suppose a USB-3 device is plugged into the port initially.
> If the HS hub is discovered and powered up first, the device will
> connect to it. Then later, when the SS hub is discovered and powered
> on, it will be too late.
Yes, given we do not and should not block superspeed port suspension
while ->peer is NULL, there is a hole with respect to suspend events
while peers are undiscovered.
We can mitigate with the RFC approach I posted:
http://marc.info/?l=linux-usb&m=139302787028482&w=2
The hole will still be open of course since userspace can manually
unload the hub driver, but at that point userspace gets to keep the
pieces.
In the absence of mitigation we are simply relying on the default
setting of pm_qos_no_power_off and hoping that is not cleared until the
"Right Time".
>
> > + 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.
>
> What's to prevent the SS port from powering off after this routine is
> finished? If the answer is "Nothing", then what reason is there for
> powering-on the port here?
Yes, the comment and the logic is stale in this version. I deleted
both.
1 files changed, 22 insertions(+), 24 deletions(-)
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 | 72 +++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 91 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..24faa88a25c4 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,36 @@ static int link_peers(struct usb_port *left, struct usb_port *right)
return rc;
}
+ /*
+ * 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 powered-off HiSpeed connection.
+ *
+ * Also, drop the HiSpeed ref taken above.
+ */
+ pm_runtime_get_sync(&ss_port->dev);
+ pm_runtime_put(&hs_port->dev);
+
return 0;
}
@@ -200,14 +248,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, *hs_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 HiSpeed port to make sure we don't race its
+ * usb_port_runtime_resume() event which takes a SuperSpeed ref
+ * when ->peer is !NULL.
+ */
+ if (left->is_superspeed) {
+ ss_port = left;
+ hs_port = right;
+ } else {
+ ss_port = right;
+ hs_port = left;
+ }
+
+ pm_runtime_get_sync(&hs_port->dev);
+
sysfs_remove_link(&left->dev.kobj, "peer");
right->peer = NULL;
sysfs_remove_link(&right->dev.kobj, "peer");
left->peer = NULL;
+
+ /* Drop the SuperSpeed ref held on behalf of the active HiSpeed port */
+ pm_runtime_put(&ss_port->dev);
+
+ /* Drop the ref taken above */
+ pm_runtime_put(&hs_port->dev);
}
/*
@@ -319,6 +390,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
