On Mon, 3 Mar 2014, Dan Williams wrote:
> Ok, so the root issue is that the peering code needs to see
> hcd->primary_hcd = NULL to know that there is no longer a peer. I
> update usb_remove_hcd() to clear out ->shared_hcd and ->primary_hcd
> under the peer lock before we allow the root hub to be freed.
Even if there is a peer hcd, the peer's root hub may not exist. More
locking is needed than you have here.
> I add some description of the new locking scheme to the changelog as
> well.
>
> 8<----------
> Subject: usb: assign default peer ports for root hubs
>
> From: Dan Williams <dan.j.williams@xxxxxxxxx>
>
> 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.
>
> A new lock 'usb_port_peer_mutex' is introduced to synchronize port
> device add/remove with peer lookups. We hold the lock for the duration
> of registration allowing default peers (and later firmware identified
> peers) to be discovered. We also hold the lock whenever hub devices are
> validated (hdev->maxchild set to non-zero) and invalidated
> (hdev->maxchild set to zero). Marking a hub valid and registering its
> ports is a locked operation and conversely invalidating a hub and
> removing its ports is another locked operation. This prevents a port
> from associating with an invalid peer. Finally, we hold the lock when
> breaking the "shared hcd" relationship at usb_remove_hcd() time.
More generally, I would say that the new mutex protects the
hcd->shared_hcd, hcd->self.root_hub, and port_dev->child pointers, in
addition to hdev->maxchild.
> [1]: usb 3.1 section 10.3.3
> [2]: xhci 1.1 appendix D
>
> Cc: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
> [alan: usb_port_peer_mutex locking scheme]
> Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
> ---
> drivers/usb/core/hcd.c | 21 ++++++++++++-
> drivers/usb/core/hub.c | 37 ++++++++++++++--------
> drivers/usb/core/hub.h | 2 +
> drivers/usb/core/port.c | 78 ++++++++++++++++++++++++++++++++++++++++++++---
> 4 files changed, 118 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 2518c3250750..259990a982f3 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -2766,6 +2766,8 @@ EXPORT_SYMBOL_GPL(usb_add_hcd);
> */
> void usb_remove_hcd(struct usb_hcd *hcd)
> {
> + /* only routine outside of hub.c that needs this mutex */
> + extern struct mutex usb_port_peer_mutex;
To be thorough, you also should hold the mutex when initially setting
hcd->self.root_hub. And in usb_create_shared_hcd, when the shared_hcd
pointers are initialized.
Also, don't forget to clear hcd->self.root_hub (under protection
of the mutex) in the failure pathway of usb_add_hcd.
> @@ -2829,6 +2830,24 @@ void usb_remove_hcd(struct usb_hcd *hcd)
> usb_put_phy(hcd->phy);
> hcd->phy = NULL;
> }
> +
> + /*
> + * Before we free this device, flush in-flight peering attempts
> + * and disable peer lookups
> + */
> + mutex_lock(&usb_port_peer_mutex);
> + if (hcd->shared_hcd) {
> + struct usb_hcd *peer_hcd = hcd->shared_hcd;
> +
> + hcd->shared_hcd = NULL;
> + hcd->primary_hcd = NULL;
> + peer_hcd->shared_hcd = NULL;
> + if (peer_hcd->primary_hcd == hcd)
> + peer_hcd->primary_hcd = NULL;
> + }
> + mutex_unlock(&usb_port_peer_mutex);
This belongs in hcd_release, not here, corresponding to the fact that
these pointers are initialized in usb_create_shared_hcd.
> +
> + usb_put_dev(hcd->self.root_hub);
Before doing this, you need to set hcd->self.root_hub to NULL, under
the protection of the mutex. Unfortunately, it's not possible to move
the usb_put_dev call into hcd_release -- this is because each USB
device (including the root hub) holds a reference to the hcd.
> diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
> index 531a591a7b1e..cb42352b97e6 100644
> --- a/drivers/usb/core/port.c
> +++ b/drivers/usb/core/port.c
> @@ -151,9 +151,68 @@ static struct device_driver usb_port_driver = {
> .owner = THIS_MODULE,
> };
>
> +/*
> + * Set the default peer port for root hubs. Assumes the primary_hcd is
> + * registered first
I realized last night that the second sentence is wrong. We have to be
able to go both ways, from the secondary hcd to the primary and vice
versa. This is because userspace can unbind and rebind the hub driver
to the primary root hub at any time.
> + */
> +static struct usb_port *find_default_peer(struct usb_hub *hub, int port1)
> +{
> + struct usb_device *hdev = hub->hdev;
> + struct usb_port *peer = NULL;
> +
> + if (!hdev->parent) {
> + struct usb_hub *peer_hub;
> + struct usb_device *peer_hdev;
> + struct usb_hcd *hcd = bus_to_hcd(hdev->bus);
> + struct usb_hcd *peer_hcd = hcd->primary_hcd;
So this should be hcd->shared_hcd.
> +
> + if (!peer_hcd || hcd == peer_hcd)
> + return NULL;
> +
> + peer_hdev = peer_hcd->self.root_hub;
> + peer_hub = usb_hub_to_struct_hub(peer_hdev);
> + if (!peer_hub || port1 > peer_hdev->maxchild)
> + return NULL;
> +
> + peer = peer_hub->ports[port1 - 1];
> + }
> +
> + return peer;
> +}
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
