Re: [PATCH v6 part1 4/8] usb: assign default peer ports for root hubs

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Mar 3, 2014 at 5:13 PM, Dan Williams <dan.j.williams@xxxxxxxxx> 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.williams@xxxxxxxxx>
>>
>> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux