On Thu, Feb 6, 2014 at 12:46 PM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> 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?
No plans to extend it, although I haven't looked if device-tree
platforms would need anything more verbose than this. Just wanted
some semblance of type-safety.
>> @@ -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.
Exactly, I changed this to:
if (port_dev->peer)
unlink_peers(port_dev, port_dev->peer);
if (peer->peer)
unlink_peers(peer, peer->peer);
link_peers(port_dev, peer);
>
>> + 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?
"dev" is the current child at a given level in the device topology
under the root hub and "data" (I've renamed it "match"), is the
usb_port from the shared_hcd that we are trying to match against (by
location 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?
>
Ugh, yes, this needs to walk the ->child link to find the downstream
hubs that are affected by this change. This needs more testing, I'll
throw together some mock acpi data to check this out...
At some point I do want to revive that "make usb_ports proper parents"
patch, but for now this series will work within the current
arrangement.
>> +}
>> +
>> +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.
The naming is confusing because we passed in "port_dev" to the initial
device_for_each_child() call on the peer_hdev. But, this this
correct. We want to evaluate whether dev is a usb_port and check
whether its location matches the "data" ("match") usb_port.
>
>> +
>> + 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?
Actually, we do want to check because even though the spec mandates a
maximum hub topology depth we have no idea how deep the hierarchy goes
of other types of devices. Ostensibly, if it's not a hub then it
won't have children and device_for_each_child() will find nothing to
do. Each call to device_for_each_child starts off one level in the
topology deeper, so unless there's a loop in the device hierarchy
we'll eventually terminate.
That said we can track and limit recursion to the maximum allowed by
the spec, and be careful to not iterate the device tree beneath an usb
interface device.
>
>> +
>> + 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...
Not really, either we trust ACPI or we don't, unfortunately. There
might be some sanity checks we could do, like see if more than 2 ports
share the same location data, or 2 ports of the the same speed have
the same location, but I have not found an implementation where the
location data differs from the USB standard port labeling.
>> + 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.
Yes, it collided with a patch from Raphael that went into 3.14-rc1, I
have now rebased.
--
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
