On Fri, 31 Jan 2014, Dan Williams wrote:
> ACPI identifies peer ports by setting their 'group_token' and
> 'group_position' _PLD data to the same value. If a platform has tier
> mismatch [1] , ACPI can override the default, USB3 defined, peer port
> association for internal hubs. External hubs follow the default peer
> association scheme.
>
> Location data is cached as an opaque cookie in usb_port_location data.
>
> Note that we only consider the group_token and group_position attributes
> from the _PLD data as ACPI specifies that group_token is a unique
> identifier.
>
> [1]: xhci 1.1 appendix D figure 131
> [2]: acpi 5 section 6.1.8
> --- a/drivers/usb/core/hub.h
> +++ b/drivers/usb/core/hub.h
> @@ -76,6 +76,10 @@ struct usb_hub {
> struct usb_port **ports;
> };
>
> +struct usb_port_location {
> + u32 cookie;
> +};
Does the cookie really need to be wrapped inside a structure? Are you
planning to add other fields to this structure later on?
> @@ -193,6 +196,97 @@ static struct usb_port *find_peer_port(struct usb_hub *hub, int port1)
> return peer;
> }
>
> +static void reset_peer(struct usb_port *port_dev, struct usb_port *peer)
> +{
> + if (!peer)
> + return;
> +
> + spin_lock(&peer_lock);
> + if (port_dev->peer)
> + put_device(&port_dev->peer->dev);
> + if (peer->peer)
> + put_device(&peer->peer->dev);
Don't you also need to set port_dev->peer->peer to NULL? And the same
for peer->peer->peer?
This is a good argument for having a separate subroutine for breaking
peer associations.
> + port_dev->peer = peer;
> + peer->peer = port_dev;
> + get_device(&peer->dev);
> + get_device(&port_dev->dev);
> + spin_unlock(&peer_lock);
> +}
> +
> +/* assumes that location data is only set for external connectors and that
> + * external hubs never have tier mismatch
> + */
> +static int redo_find_peer_port(struct device *dev, void *data)
This is awfully confusing. More explicit comments would help. What
exactly are dev and data?
> +{
> + struct usb_port *port_dev = to_usb_port(dev);
> +
> + if (is_usb_port(dev)) {
> + struct usb_device *hdev = to_usb_device(dev->parent->parent);
> + struct usb_hub *hub = usb_hub_to_struct_hub(hdev);
> + int port1 = port_dev->portnum;
> + struct usb_port *peer;
> +
> + peer = find_peer_port(hub, port1);
> + reset_peer(port_dev, peer);
> + }
> +
> + return device_for_each_child(dev, NULL, redo_find_peer_port);
If dev really is a usb_port, what children is it going to have?
> +}
> +
> +static int pair_port(struct device *dev, void *data)
> +{
> + struct usb_port *peer = data;
> + struct usb_port *port_dev = to_usb_port(dev);
Isn't this backward? data is the port you want to match, and dev
is the device (hopefully a hub) where you're looking for possible
matches.
> +
> + if (!is_usb_port(dev)
> + || port_dev->location.cookie != peer->location.cookie)
> + return device_for_each_child(dev, peer, pair_port);
I'm not sure what's going on here, but it looks awfully suspicious.
Like, this subroutine will recursively call itself over and over and
end up causing a stack overflow. Don't you even want to check that dev
is a hub before recursing?
> +
> + dev_dbg(dev->parent->parent, "port%d peer = %s port%d (by location)\n",
> + port_dev->portnum, dev_name(peer->dev.parent->parent),
> + peer->portnum);
> + if (port_dev->peer != peer) {
> + /* Sigh, tier mismatch has invalidated our ancestry.
> + * This should not be too onerous even in deep hub
> + * topologies as we will discover tier mismatch early
> + * (after platform internal hubs have been enumerated),
> + * before external hubs are probed.
> + */
Is there any way to guarantee that mistakes caused by tier mismatches
won't propagate at all? Probably not...
> + reset_peer(port_dev, peer);
> + device_for_each_child(dev, NULL, redo_find_peer_port);
> + }
> +
> + return true;
> +}
> +
> +void usb_set_hub_port_location(struct usb_device *hdev, int port1,
> + u32 cookie)
> +{
> + struct usb_hub *hub = usb_hub_to_struct_hub(hdev);
> + struct usb_hcd *hcd = bus_to_hcd(hdev->bus);
> + struct usb_hcd *peer_hcd = hcd->shared_hcd;
> + struct usb_device *peer_hdev;
> + struct usb_port *port_dev;
> +
> + if (cookie == 0)
> + return;
> +
> + if (!hub)
> + return;
> +
> + port_dev = hub->ports[port1 - 1];
> + port_dev->location.cookie = cookie;
> +
> + /* see if a port with the same location data exists in a peer
> + * usb domain
> + */
> + if (!peer_hcd)
> + return;
> +
> + peer_hdev = peer_hcd->self.root_hub;
> + device_for_each_child(&peer_hdev->dev, port_dev, pair_port);
Here at least you do know that the device you are searching under
(i.e., &peer_hdev->dev) really is a hub.
> +}
> +
> int usb_hub_create_port_device(struct usb_hub *hub, int port1)
> {
> struct usb_port *port_dev, *peer;
> diff --git a/drivers/usb/core/usb-acpi.c b/drivers/usb/core/usb-acpi.c
> index d7cb822d6eab..8a466438bff7 100644
> --- a/drivers/usb/core/usb-acpi.c
> +++ b/drivers/usb/core/usb-acpi.c
I'm not going to comment on this file because I haven't been following
it at all closely. One thing came up, though:
> @@ -179,6 +189,9 @@ static int usb_acpi_find_device(struct device *dev, acpi_handle *handle)
> return -ENODEV;
> return 0;
> } else if (is_usb_port(dev)) {
> + struct acpi_pld_info *pld;
> + acpi_status status;
> +
> sscanf(dev_name(dev), "port%d", &port_num);
> /* Get the struct usb_device point of port's hub */
> udev = to_usb_device(dev->parent->parent);
> @@ -209,7 +222,15 @@ static int usb_acpi_find_device(struct device *dev, acpi_handle *handle)
> if (!*handle)
> return -ENODEV;
> }
> - usb_acpi_check_port_connect_type(udev, *handle, port_num);
> +
> + status = acpi_get_physical_device_location(*handle, &pld);
> + if (ACPI_FAILURE(status))
> + pld = NULL;
> +
> + usb_acpi_check_port_connect_type(udev, *handle, port_num, pld);
> + usb_acpi_check_port_peer(udev, *handle, port_num, pld);
> +
> + ACPI_FREE(pld);
> } else
> return -ENODEV;
>
These two hunks didn't apply in my tree. My copy of the file doesn't
have a usb_acpi_find_device subroutine.
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
