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?
> @@ -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.
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?
> +
> + 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.
It would also be a good idea to verify that peer->peer is NULL before
doing these assignments.
> - 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.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
