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 <[email protected]>
>
> 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)
peer_hdev = hdevA
usb_put_dev(hdevA) // free
hubA = usb_hub_to_struct_hub(hdevA) // use
after free
Am I missing something that mitigates this? I still think we need to
check against ->rh_registered.
Otherwise changes look good and I have folded this into the patch.
--
Dan
--
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
