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?
> 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
s/suspended/powered-off/
> + *
> + * 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.
> + pm_runtime_get(&ss_port->dev);
Should this 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.
> + 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?
> 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);
> }
Alan Stern
--
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
