On Fri, Feb 7, 2014 at 7:15 AM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> On Fri, 7 Feb 2014, Dan Williams wrote:
>
>> >> +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.
>
> This needs to be a lot clearer. Also, you need to explain and check
> more explicitly for the device types that matter: hubs, hub interfaces,
> and ports. Anything else should be filtered out immediately.
>
>> >> +
>> >> + 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
>
> Don't be silly. All sorts of non-hub devices have children. (They may
> not be USB children, but they count as children in
> device_for_each_child.)
Precisely, I think you misunderstood my "actually we *do* want to
check..." comment.
>
>> 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.
>
> I think you're missing the point.
No, I got it ;-).
[..]
>> > 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.
>
> Not what I meant. Assuming we do trust ACPI, is there any way we can
> use the ACPI information right away to get the correct pairing before
> any mistaken pairings have been set up?
Ah, I see. No, not really, and it's actually harder now after
Raphael's recent change. We only get to probe ACPI information at
device registration time and the search key is an existing struct
device. So, we can't, for instance, look for a peer that has not been
registered yet.
--
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
