On Mon, Feb 24, 2014 at 1:46 PM, Alan Stern <[email protected]> wrote:
> 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 <[email protected]>
>
>> +/*
>> + * 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);
I like it, however isn't this an ABI change that could mess up existing scripts?
What's worse, in-kernel gymnastics to print out useful names, or
adding sysfs links from the old short name to the new name?
>
>> + 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.
I'll take a look.
Ack on all the other comments.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
