Re: [PATCH v5 02/16] usb: assign default peer ports for root hubs

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux