On Mon, Mar 3, 2014 at 5:13 PM, Dan Williams <dan.j.willi...@intel.com> wrote: > On Mon, 2014-03-03 at 17:21 -0500, Alan Stern wrote: >> On Fri, 28 Feb 2014, Dan Williams wrote: >> >> > Assume that the peer of a superspeed port is the port with the same id >> > on the shared_hcd root hub. This identification scheme is required of >> > external hubs by the USB3 spec [1]. However, for root hubs, tier mismatch >> > may be in effect [2]. Tier mismatch can only be enumerated via platform >> > firmware. For now, simply perform the nominal association. >> > >> > Once the root hub is marked as unregistered we block attempts to walk >> > the ->shared_hcd pointer to find a root-peer port. >> > >> > [1]: usb 3.1 section 10.3.3 >> > [2]: xhci 1.1 appendix D >> > >> > Signed-off-by: Dan Williams <dan.j.willi...@intel.com> >> >> Okay, I think the locking still isn't right. Below is an example patch >> showing how it ought to work. Basically, all the locking can be done >> in hub.c; port.c doesn't have to worry about it at all. >> >> > diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c >> > index 2518c3250750..e380b8a80830 100644 >> > --- a/drivers/usb/core/hcd.c >> > +++ b/drivers/usb/core/hcd.c >> > @@ -2778,9 +2778,17 @@ void usb_remove_hcd(struct usb_hcd *hcd) >> > hcd->state = HC_STATE_QUIESCING; >> > >> > dev_dbg(hcd->self.controller, "roothub graceful disconnect\n"); >> > + >> > + /* >> > + * Flush + disable peering operations, and atomically update the >> > + * hcd state relative to other root hub state >> > + * changes/evaluations >> > + */ >> > + mutex_lock(&usb_port_peer_mutex); >> > spin_lock_irq (&hcd_root_hub_lock); >> > hcd->rh_registered = 0; >> > spin_unlock_irq (&hcd_root_hub_lock); >> > + mutex_unlock(&usb_port_peer_mutex); >> >> Not needed. We don't care whether the root hub device is registered; >> we only care whether it qualifies as a hub according to the tests in >> usb_hub_to_struct_hub. > > In general I agree, and I like the compartmentalization of only needing > to take the lock in hub.c. But I still think we have a hole with a > scenario like the following (granted, this should never happen in > current code...): > > CPU1 CPU2 > usb_remove_hcd(hcdA) > ... > mutex_lock(peer_lock) > hdevA->maxchild = 0; > mutex_unlock(peer_lock) > usb_add_hcd(hcdB) > ... > mutex_lock(peer_lock) > hdevA = hcdA->self.root_hub > usb_put_dev(hdevA) // free > hubA = usb_hub_to_struct_hub(hdevA) // use > after free
...and if this is a hole then so is the hcdA->self.root_hub. It makes sense that when the host controller breaks the peering relationship at the root it should hold the lock and clear ->shared_hcd and ->primary_hcd. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
