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.)
> 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. The code doesn't appear to be
careful about filtering out non-USB entities from its scan. So for
example, if dev happens to be a usb-storage device, you will end up
recursing through its children (the SCSI host, then the SCSI target,
then the SCSI device, etc.) indefinitely. Not limited by the USB
topology.
> >> + 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.
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?
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
