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 <[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]> > > > @@ -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 <[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. 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 <[email protected]> --- 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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
