Hi Alan,
Thanks for the time and effort on the review, really appreciate it.
On Thu, Feb 6, 2014 at 12:07 PM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> On Fri, 31 Jan 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.
>
>> --- a/drivers/usb/core/port.c
>> +++ b/drivers/usb/core/port.c
>> @@ -21,6 +21,7 @@
>>
>> #include "hub.h"
>>
>> +DEFINE_SPINLOCK(peer_lock);
>
> Later on in the series you change this spinlock to a mutex. Why not
> just make it a mutex from the start?
Here it's only protecting a few pointer manipulations later it
encompasses the sleeping sysfs operations. However, I agree that it
makes sense to lock the entire operation, so I'll make this a mutex
and wrap find_peer_port().
>> @@ -146,9 +147,37 @@ struct device_type usb_port_device_type = {
>> .pm = &usb_port_pm_ops,
>> };
>>
>> +static struct usb_port *find_peer_port(struct usb_hub *hub, int port1)
>> +{
>> + struct usb_device *hdev = hub->hdev;
>> + struct usb_port *peer = NULL;
>> +
>> + /* set the default peer port for root hubs. Platform firmware
>> + * can later fix this up if tier-mismatch is present. Assumes
>> + * the primary_hcd is usb2.0 and registered first
>
> This is an important restriction, and I think it would be better to
> move it to a comment preceding the start of the function.
Ok.
> But come to think of it, you don't _really_ depend on the fact that the
> USB-2 root hub is registered first. What matters is that the _primary_
> hcd is registered first.
>
>> + */
>> + 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 (!hub_is_superspeed(hdev)
>> + || WARN_ON_ONCE(!peer_hcd || hcd == peer_hcd))
>
> So here, this could be changed to:
>
> if (!peer_hcd || hcd == peer_hcd)
>
>> + return NULL;
>
>> @@ -164,8 +193,21 @@ int usb_hub_create_port_device(struct usb_hub *hub, int port1)
>> port_dev->dev.groups = port_dev_group;
>> port_dev->dev.type = &usb_port_device_type;
>> dev_set_name(&port_dev->dev, "port%d", port1);
>> + device_initialize(&port_dev->dev);
>
> Why did you split up the device_register into device_initialize and
> device_add? So that you could put the dev_dbg in between? Why not
> stick the dev_dbg after the device_register call?
>
No, the impetus was to complete the peer finding process before
registering the port. However, this only minimizes cases where we
operate without an established peer and does not close the
possibility, so might as well use the simpler registration semantic.
>> +
>> + peer = find_peer_port(hub, port1);
>> + dev_dbg(&hub->hdev->dev, "port%d peer = %s\n",
>> + port1, peer ? dev_name(peer->dev.parent->parent) : "[none]");
>> + if (peer) {
>> + spin_lock(&peer_lock);
>> + get_device(&peer->dev);
>> + port_dev->peer = peer;
>> + get_device(&port_dev->dev);
>> + peer->peer = port_dev;
>> + spin_unlock(&peer_lock);
>> + }
>
> I kind of think it would be better to include the find_peer_port call
> in the locked region. Maybe it doesn't matter, but in principle
> find_peer_port is going to be looking at the peer pointers in other
> structures, so you don't want them to change while it is running.
>
I agree.
> It would also be a good idea to verify that peer->peer is NULL before
> doing these assignments.
>
I do it later, but let's move it here.
>> - retval = device_register(&port_dev->dev);
>> + retval = device_add(&port_dev->dev);
>> if (retval)
>> goto error_register;
>>
>> @@ -188,9 +230,19 @@ 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;
>> +
>> + spin_lock(&peer_lock);
>> + if (peer) {
>> + peer->peer = NULL;
>> + port_dev->peer = NULL;
>> + put_device(&port_dev->dev);
>> + put_device(&peer->dev);
>> + }
>> + spin_unlock(&peer_lock);
>
> This was added in the wrong place; it should go in
> usb_port_device_release. The way things are now, this code won't get
> executed if the device_add call fails.
Not really, but you saw that already... yes, I need to handle the
device_registration failure case.
--
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
