On Mon, 2014-03-03 at 17:13 -0800, Dan Williams 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)
> 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.
>
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.
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.
[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;
struct usb_device *rhdev = hcd->self.root_hub;
dev_info(hcd->self.controller, "remove, state %x\n", hcd->state);
@@ -2821,7 +2823,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 +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);
+
+ usb_put_dev(hcd->self.root_hub);
}
EXPORT_SYMBOL_GPL(usb_remove_hcd);
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 67de9b63a98d..67e074482f80 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--;
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..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
+ */
+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;
+
+ 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;
+}
+
+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;
+}
+
int usb_hub_create_port_device(struct usb_hub *hub, int port1)
{
- struct usb_port *port_dev = NULL;
+ struct usb_port *port_dev, *peer = NULL;
int retval;
port_dev = kzalloc(sizeof(*port_dev), GFP_KERNEL);
@@ -171,10 +230,15 @@ int usb_hub_create_port_device(struct usb_hub *hub, int port1)
port_dev->dev.driver = &usb_port_driver;
dev_set_name(&port_dev->dev, "%s-port%d", dev_name(&hub->hdev->dev),
port1);
+
retval = device_register(&port_dev->dev);
if (retval)
goto error_register;
+ peer = find_default_peer(hub, port1);
+ if (peer)
+ link_peers(port_dev, peer);
+
pm_runtime_set_active(&port_dev->dev);
/*
@@ -197,9 +261,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);
+}
--
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
