On Tue, 2014-03-04 at 10:58 -0500, Alan Stern wrote:
> 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.willi...@intel.com>
> >
> > 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.
Yes, much more succinct.
>
> > [1]: usb 3.1 section 10.3.3
> > [2]: xhci 1.1 appendix D
> >
> > Cc: Alan Stern <st...@rowland.harvard.edu>
> > [alan: usb_port_peer_mutex locking scheme]
> > Signed-off-by: Dan Williams <dan.j.willi...@intel.com>
> > ---
> > 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.
Ok, I added this.
>
> Also, don't forget to clear hcd->self.root_hub (under protection
> of the mutex) in the failure pathway of usb_add_hcd.
I made a new usb_put_invalidate_rhdev() to handle cases where we can set
hcd->self.root_hub to NULL.
>
> > @@ -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.
>
Yes, I overlooked hcd_release.
> > +
> > + 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.
>
Fixed.
> > + */
> > +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;
> > +}
>
Please have a look at the following. I'm sure Greg will be happy that
we are killing these bugs before they become fodder for -stable. Thanks
for the review! The other change I made was to convert
find_default_peer to find_and_link_peer per your other comments about
centralizing peering on patch 6.
8<---------
Subject: usb: assign default peer ports for root hubs
From: Dan Williams <dan.j.willi...@intel.com>
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.
A new lock 'usb_port_peer_mutex' is introduced to synchronize port
device add/remove with peer lookups. It protects peering against
changes to hcd->shared_hcd, hcd->self.root_hub, hdev->maxchild, and
port_dev->child pointers.
[1]: usb 3.1 section 10.3.3
[2]: xhci 1.1 appendix D
Cc: Alan Stern <st...@rowland.harvard.edu>
[alan: usb_port_peer_mutex locking scheme]
Signed-off-by: Dan Williams <dan.j.willi...@intel.com>
---
drivers/usb/core/hcd.c | 43 +++++++++++++++++++++++-----
drivers/usb/core/hub.c | 42 ++++++++++++++++++---------
drivers/usb/core/hub.h | 2 +
drivers/usb/core/port.c | 73 ++++++++++++++++++++++++++++++++++++++++++++---
drivers/usb/core/usb.h | 1 +
5 files changed, 134 insertions(+), 27 deletions(-)
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 2518c3250750..af9f053b9626 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2437,11 +2437,13 @@ struct usb_hcd *usb_create_shared_hcd(const struct
hc_driver *driver,
mutex_init(hcd->bandwidth_mutex);
dev_set_drvdata(dev, hcd);
} else {
+ mutex_lock(&usb_port_peer_mutex);
hcd->bandwidth_mutex = primary_hcd->bandwidth_mutex;
hcd->primary_hcd = primary_hcd;
primary_hcd->primary_hcd = primary_hcd;
hcd->shared_hcd = primary_hcd;
primary_hcd->shared_hcd = hcd;
+ mutex_unlock(&usb_port_peer_mutex);
}
kref_init(&hcd->kref);
@@ -2493,18 +2495,25 @@ EXPORT_SYMBOL_GPL(usb_create_hcd);
* deallocated.
*
* Make sure to only deallocate the bandwidth_mutex when the primary HCD is
- * freed. When hcd_release() is called for the non-primary HCD, set the
- * primary_hcd's shared_hcd pointer to null (since the non-primary HCD will be
- * freed shortly).
+ * freed. When hcd_release() is called for either hcd in a peer set
+ * invalidate the peer's ->shared_hcd and ->primary_hcd pointers to
+ * block new peering attempts
*/
-static void hcd_release (struct kref *kref)
+static void hcd_release(struct kref *kref)
{
struct usb_hcd *hcd = container_of (kref, struct usb_hcd, kref);
+ mutex_lock(&usb_port_peer_mutex);
if (usb_hcd_is_primary_hcd(hcd))
kfree(hcd->bandwidth_mutex);
- else
- hcd->shared_hcd->shared_hcd = NULL;
+ if (hcd->shared_hcd) {
+ struct usb_hcd *peer = hcd->shared_hcd;
+
+ peer->shared_hcd = NULL;
+ if (peer->primary_hcd == hcd)
+ peer->primary_hcd = NULL;
+ }
+ mutex_unlock(&usb_port_peer_mutex);
kfree(hcd);
}
@@ -2572,6 +2581,21 @@ static int usb_hcd_request_irqs(struct usb_hcd *hcd,
return 0;
}
+/*
+ * Before we free this root hub, flush in-flight peering attempts
+ * and disable peer lookups
+ */
+static void usb_put_invalidate_rhdev(struct usb_hcd *hcd)
+{
+ struct usb_device *rhdev;
+
+ mutex_lock(&usb_port_peer_mutex);
+ rhdev = hcd->self.root_hub;
+ hcd->self.root_hub = NULL;
+ mutex_unlock(&usb_port_peer_mutex);
+ usb_put_dev(rhdev);
+}
+
/**
* usb_add_hcd - finish generic HCD structure initialization and register
* @hcd: the usb_hcd structure to initialize
@@ -2632,7 +2656,9 @@ int usb_add_hcd(struct usb_hcd *hcd,
retval = -ENOMEM;
goto err_allocate_root_hub;
}
+ mutex_lock(&usb_port_peer_mutex);
hcd->self.root_hub = rhdev;
+ mutex_unlock(&usb_port_peer_mutex);
switch (hcd->speed) {
case HCD_USB11:
@@ -2741,7 +2767,7 @@ err_hcd_driver_start:
err_request_irq:
err_hcd_driver_setup:
err_set_rh_speed:
- usb_put_dev(hcd->self.root_hub);
+ usb_put_invalidate_rhdev(hcd);
err_allocate_root_hub:
usb_deregister_bus(&hcd->self);
err_register_bus:
@@ -2821,7 +2847,6 @@ void usb_remove_hcd(struct usb_hcd *hcd)
free_irq(hcd->irq, hcd);
}
- usb_put_dev(hcd->self.root_hub);
usb_deregister_bus(&hcd->self);
hcd_buffer_destroy(hcd);
if (hcd->remove_phy && hcd->phy) {
@@ -2829,6 +2854,8 @@ void usb_remove_hcd(struct usb_hcd *hcd)
usb_put_phy(hcd->phy);
hcd->phy = NULL;
}
+
+ usb_put_invalidate_rhdev(hcd);
}
EXPORT_SYMBOL_GPL(usb_remove_hcd);
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 67de9b63a98d..c025edb90e1f 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -55,6 +55,9 @@ static DECLARE_WAIT_QUEUE_HEAD(khubd_wait);
static struct task_struct *khubd_task;
+/* synchronize hub-port add/remove and peering operations */
+DEFINE_MUTEX(usb_port_peer_mutex);
+
/* cycle leds on hubs that aren't blinking for attention */
static bool blinkenlights = 0;
module_param (blinkenlights, bool, S_IRUGO);
@@ -1303,6 +1306,7 @@ static int hub_configure(struct usb_hub *hub,
char *message = "out of memory";
unsigned unit_load;
unsigned full_load;
+ unsigned maxchild;
hub->buffer = kmalloc(sizeof(*hub->buffer), GFP_KERNEL);
if (!hub->buffer) {
@@ -1341,12 +1345,11 @@ static int hub_configure(struct usb_hub *hub,
goto fail;
}
- hdev->maxchild = hub->descriptor->bNbrPorts;
- dev_info (hub_dev, "%d port%s detected\n", hdev->maxchild,
- (hdev->maxchild == 1) ? "" : "s");
+ maxchild = hub->descriptor->bNbrPorts;
+ dev_info(hub_dev, "%d port%s detected\n", maxchild,
+ (maxchild == 1) ? "" : "s");
- hub->ports = kzalloc(hdev->maxchild * sizeof(struct usb_port *),
- GFP_KERNEL);
+ hub->ports = kzalloc(maxchild * sizeof(struct usb_port *), GFP_KERNEL);
if (!hub->ports) {
ret = -ENOMEM;
goto fail;
@@ -1367,11 +1370,11 @@ static int hub_configure(struct usb_hub *hub,
int i;
char portstr[USB_MAXCHILDREN + 1];
- for (i = 0; i < hdev->maxchild; i++)
+ for (i = 0; i < maxchild; i++)
portstr[i] = hub->descriptor->u.hs.DeviceRemovable
[((i + 1) / 8)] & (1 << ((i + 1) % 8))
? 'F' : 'R';
- portstr[hdev->maxchild] = 0;
+ portstr[maxchild] = 0;
dev_dbg(hub_dev, "compound device; port removable status:
%s\n", portstr);
} else
dev_dbg(hub_dev, "standalone hub\n");
@@ -1483,7 +1486,7 @@ static int hub_configure(struct usb_hub *hub,
if (hcd->power_budget > 0)
hdev->bus_mA = hcd->power_budget;
else
- hdev->bus_mA = full_load * hdev->maxchild;
+ hdev->bus_mA = full_load * maxchild;
if (hdev->bus_mA >= full_load)
hub->mA_per_port = full_load;
else {
@@ -1498,7 +1501,7 @@ static int hub_configure(struct usb_hub *hub,
hub->descriptor->bHubContrCurrent);
hub->limited_power = 1;
- if (remaining < hdev->maxchild * unit_load)
+ if (remaining < maxchild * unit_load)
dev_warn(hub_dev,
"insufficient power available "
"to use all downstream ports\n");
@@ -1566,15 +1569,19 @@ static int hub_configure(struct usb_hub *hub,
if (hub->has_indicators && blinkenlights)
hub->indicator[0] = INDICATOR_CYCLE;
- for (i = 0; i < hdev->maxchild; i++) {
+ mutex_lock(&usb_port_peer_mutex);
+ for (i = 0; i < maxchild; i++) {
ret = usb_hub_create_port_device(hub, i + 1);
if (ret < 0) {
dev_err(hub->intfdev,
"couldn't create port%d device.\n", i + 1);
- hdev->maxchild = i;
- goto fail_keep_maxchild;
+ break;
}
}
+ hdev->maxchild = i;
+ mutex_unlock(&usb_port_peer_mutex);
+ if (ret < 0)
+ goto fail;
usb_hub_adjust_deviceremovable(hdev, hub->descriptor);
@@ -1582,8 +1589,6 @@ static int hub_configure(struct usb_hub *hub,
return 0;
fail:
- hdev->maxchild = 0;
-fail_keep_maxchild:
dev_err (hub_dev, "config failed, %s (err %d)\n",
message, ret);
/* hub_disconnect() frees urb and descriptor */
@@ -1619,6 +1624,8 @@ static void hub_disconnect(struct usb_interface *intf)
hub->error = 0;
hub_quiesce(hub, HUB_DISCONNECT);
+ mutex_lock(&usb_port_peer_mutex);
+
/* Avoid races with recursively_mark_NOTATTACHED() */
spin_lock_irq(&device_state_lock);
port1 = hdev->maxchild;
@@ -1629,6 +1636,8 @@ static void hub_disconnect(struct usb_interface *intf)
for (; port1 > 0; --port1)
usb_hub_remove_port_device(hub, port1);
+ mutex_unlock(&usb_port_peer_mutex);
+
if (hub->hdev->speed == USB_SPEED_HIGH)
highspeed_hubs--;
@@ -4574,6 +4583,8 @@ static void hub_port_connect_change(struct usb_hub *hub,
int port1,
*/
status = 0;
+ mutex_lock(&usb_port_peer_mutex);
+
/* We mustn't add new devices if the parent hub has
* been disconnected; we would race with the
* recursively_mark_NOTATTACHED() routine.
@@ -4584,14 +4595,17 @@ static void hub_port_connect_change(struct usb_hub
*hub, int port1,
else
port_dev->child = udev;
spin_unlock_irq(&device_state_lock);
+ mutex_unlock(&usb_port_peer_mutex);
/* Run it through the hoops (find a driver, etc) */
if (!status) {
status = usb_new_device(udev);
if (status) {
+ mutex_lock(&usb_port_peer_mutex);
spin_lock_irq(&device_state_lock);
port_dev->child = NULL;
spin_unlock_irq(&device_state_lock);
+ mutex_unlock(&usb_port_peer_mutex);
}
}
diff --git a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h
index baf5b48b79f7..d51feb68165b 100644
--- a/drivers/usb/core/hub.h
+++ b/drivers/usb/core/hub.h
@@ -81,6 +81,7 @@ struct usb_hub {
* @child: usb device attached to the port
* @dev: generic device interface
* @port_owner: port's owner
+ * @peer: related usb2 and usb3 ports (share the same connector)
* @connect_type: port's connect type
* @portnum: port index num based one
* @power_is_on: port's power state
@@ -90,6 +91,7 @@ struct usb_port {
struct usb_device *child;
struct device dev;
struct dev_state *port_owner;
+ struct usb_port *peer;
enum usb_port_connect_type connect_type;
u8 portnum;
unsigned power_is_on:1;
diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
index 531a591a7b1e..0d36610af156 100644
--- a/drivers/usb/core/port.c
+++ b/drivers/usb/core/port.c
@@ -151,9 +151,66 @@ static struct device_driver usb_port_driver = {
.owner = THIS_MODULE,
};
+static void link_peers(struct usb_port *left, struct usb_port *right)
+{
+ if (left->peer == right && right->peer == left)
+ return;
+
+ if (left->peer || right->peer) {
+ struct usb_port *lpeer = left->peer;
+ struct usb_port *rpeer = right->peer;
+
+ WARN(1, "failed to peer %s and %s (%s -> %p) (%s -> %p)\n",
+ dev_name(&left->dev), dev_name(&right->dev),
+ dev_name(&left->dev), lpeer,
+ dev_name(&right->dev), rpeer);
+ return;
+ }
+
+ left->peer = right;
+ right->peer = left;
+}
+
+static void unlink_peers(struct usb_port *left, struct usb_port *right)
+{
+ WARN(right->peer != left || left->peer != right,
+ "%s and %s are not peers?\n",
+ dev_name(&left->dev), dev_name(&right->dev));
+
+ right->peer = NULL;
+ left->peer = NULL;
+}
+
+/* set the default peer port for root hubs */
+static void find_and_link_peer(struct usb_hub *hub, int port1)
+{
+ struct usb_port *port_dev = hub->ports[port1 - 1], *peer;
+ struct usb_device *hdev = hub->hdev;
+
+ 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->shared_hcd;
+
+ if (!peer_hcd)
+ return;
+
+ peer_hdev = peer_hcd->self.root_hub;
+ peer_hub = usb_hub_to_struct_hub(peer_hdev);
+ if (!peer_hub || port1 > peer_hdev->maxchild)
+ return;
+
+ peer = peer_hub->ports[port1 - 1];
+
+ if (peer)
+ link_peers(port_dev, peer);
+ }
+}
+
int usb_hub_create_port_device(struct usb_hub *hub, int port1)
{
- struct usb_port *port_dev = NULL;
+ struct usb_port *port_dev;
int retval;
port_dev = kzalloc(sizeof(*port_dev), GFP_KERNEL);
@@ -175,6 +232,8 @@ int usb_hub_create_port_device(struct usb_hub *hub, int
port1)
if (retval)
goto error_register;
+ find_and_link_peer(hub, port1);
+
pm_runtime_set_active(&port_dev->dev);
/*
@@ -197,9 +256,13 @@ exit:
return retval;
}
-void usb_hub_remove_port_device(struct usb_hub *hub,
- int port1)
+void usb_hub_remove_port_device(struct usb_hub *hub, int port1)
{
- device_unregister(&hub->ports[port1 - 1]->dev);
-}
+ struct usb_port *port_dev = hub->ports[port1 - 1];
+ struct usb_port *peer;
+ peer = port_dev->peer;
+ if (peer)
+ unlink_peers(port_dev, peer);
+ device_unregister(&port_dev->dev);
+}
diff --git a/drivers/usb/core/usb.h b/drivers/usb/core/usb.h
index 28f272884c0d..af89070d771d 100644
--- a/drivers/usb/core/usb.h
+++ b/drivers/usb/core/usb.h
@@ -123,6 +123,7 @@ static inline int usb_set_usb2_hardware_lpm(struct
usb_device *udev, int enable)
#endif
extern struct bus_type usb_bus_type;
+extern struct mutex usb_port_peer_mutex;
extern struct device_type usb_device_type;
extern struct device_type usb_if_device_type;
extern struct device_type usb_ep_device_type;
--
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
