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.
> @@ -171,7 +235,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);
> +
> + mutex_lock(&usb_port_peer_mutex);
> + hub->ports[port1 - 1] = port_dev;
> retval = device_register(&port_dev->dev);
> + if (retval == 0)
> + peer = find_default_peer(hub, port1);
> + if (peer)
> + link_peers(port_dev, peer);
> + mutex_unlock(&usb_port_peer_mutex);
> if (retval)
> goto error_register;
The mutex stuff won't be needed here. The other changes are good.
> @@ -196,9 +268,15 @@ 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;
> +
> + mutex_lock(&usb_port_peer_mutex);
> + peer = port_dev->peer;
> + if (peer)
> + unlink_peers(port_dev, peer);
> + device_unregister(&port_dev->dev);
> + mutex_unlock(&usb_port_peer_mutex);
> }
Likewise here.
This is how I think the locking should work. I haven't carefully
thought through all the possibilities, so please double-check that this
really will do what we want.
Alan Stern
Index: usb-3.14/drivers/usb/core/hub.c
===================================================================
--- usb-3.14.orig/drivers/usb/core/hub.c
+++ usb-3.14/drivers/usb/core/hub.c
@@ -55,6 +55,9 @@ static DECLARE_WAIT_QUEUE_HEAD(khubd_wai
static struct task_struct *khubd_task;
+/* synchronize hub-port add/remove and peering operations */
+static 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);
@@ -1300,6 +1303,7 @@ static int hub_configure(struct usb_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) {
@@ -1338,12 +1342,11 @@ static int hub_configure(struct usb_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;
@@ -1364,11 +1367,11 @@ static int hub_configure(struct usb_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");
@@ -1480,7 +1483,7 @@ static int hub_configure(struct usb_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 {
@@ -1495,7 +1498,7 @@ static int hub_configure(struct usb_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");
@@ -1563,15 +1566,19 @@ static int hub_configure(struct usb_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);
@@ -1579,8 +1586,6 @@ static int hub_configure(struct usb_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 */
@@ -1616,6 +1621,8 @@ static void hub_disconnect(struct usb_in
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;
@@ -1626,6 +1633,8 @@ static void hub_disconnect(struct usb_in
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--;
--
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
