On Fri, 21 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.
>
> [1]: usb 3.1 section 10.3.3
> [2]: xhci 1.1 appendix D
>
> Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
> +/*
> + * Set the default peer port for root hubs. Platform firmware will have
> + * already set the peer if tier-mismatch is present. Assumes the
> + * primary_hcd is registered first
> + */
> +static struct usb_port *find_default_peer(struct usb_hub *hub, int port1)
The second sentence isn't accurate, at least, not as of this patch.
> +static void link_peers(struct usb_port *left, struct usb_port *right)
> +{
> + struct device *rdev;
> + struct device *ldev;
> +
For safety, add
if (left->peer == right && right->peer == left)
return;
> + if (left->peer) {
> + right = left->peer;
> + ldev = left->dev.parent->parent;
> + rdev = right->dev.parent->parent;
At this point we don't know if left->peer points to anything valid.
It would be better to print out the names of left and right rather than
left and left->peer. For debugging, print the value of left->peer
as well.
> +
> + WARN_ONCE(1, "%s port%d already peered with %s %d\n",
> + dev_name(ldev), left->portnum, dev_name(rdev),
> + right->portnum);
Since this isn't expected ever to happen, I think WARN is more
appropriate than WARN_ONCE.
Also, the gyrations you have to go through here and elsewhere to print
useful names indicate that we should set up better names for the port
devices. How about:
dev_set_name(&port_dev->dev, "%s-port%d", dev_name(&hub->hdev->dev),
port1);
> + return;
> + } else if (right->peer) {
> + left = right->peer;
> + ldev = left->dev.parent->parent;
> + rdev = right->dev.parent->parent;
> +
> + WARN_ONCE(1, "%s port%d already peered with %s %d\n",
> + dev_name(rdev), right->portnum, dev_name(ldev),
> + left->portnum);
Similar comments.
> + return;
> + }
> +
> + get_device(&right->dev);
> + left->peer = right;
> + get_device(&left->dev);
> + right->peer = left;
Add
dev_dbg(&left->dev, "(%p) peered with %s (%p)\n", left,
dev_name(&right->dev), right);
> +}
> +
> +static void unlink_peers(struct usb_port *left, struct usb_port *right)
> +{
> + struct device *rdev = right->dev.parent->parent;
> + struct device *ldev = left->dev.parent->parent;
> +
> + WARN_ONCE(right->peer != left || left->peer != right,
> + "%s port%d and %s port%d are not peers?\n",
> + dev_name(ldev), left->portnum, dev_name(rdev), right->portnum);
Include left->peer and right->peer for debugging and use WARN.
> +
Add
dev_dbg(&left->dev, "(%p) unpeered from %s (%p)\n", left,
dev_name(&right->dev), right);
> + put_device(&left->dev);
> + right->peer = NULL;
> + put_device(&right->dev);
> + left->peer = NULL;
> +}
> @@ -190,9 +270,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 = port_dev->peer;
>
> + mutex_lock(&peer_lock);
> + if (peer)
> + unlink_peers(port_dev, peer);
> + mutex_unlock(&peer_lock);
> +
There's a race; another thread could peer port_dev to something else
right here. One possible solution: Add a flag to the port structure
indicating that it is being removed, and check the peer's flag when
setting the peer.
> + device_unregister(&port_dev->dev);
> +}
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
