On Mon, 3 Mar 2014, Dan Williams wrote:
> Subject: usb: find internal hub tier mismatch via acpi
>
> From: Dan Williams <dan.j.williams@xxxxxxxxx>
>
> 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.
>
> When we find port location data for a port then we assume that the
> firmware will also describe its peer port. This allows the
> implementation to only ever set the peer once. This leads to a question
> about what happens when a pm runtime event occurs while the peer
> associations are still resolving. Since we only ever set the peer
> information once, a USB3 port needs to be prevented from suspending
> while its ->peer pointer is NULL (implemented in a subsequent patch).
>
> There is always the possibility that firmware mis-identifies the ports,
> but there is not much the kernel can do in that case.
>
> [1]: xhci 1.1 appendix D figure 131
> [2]: acpi 5 section 6.1.8
>
> [alan]: don't do default peering when acpi data present
> Suggested-by: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
> static struct usb_port *find_default_peer(struct usb_hub *hub, int port1)
> {
> + struct usb_port *port_dev = hub->ports[port1 - 1];
> struct usb_device *hdev = hub->hdev;
> struct usb_device *peer_hdev = NULL;
> struct usb_hub *peer_hub;
>
> + /*
> + * If location data is available then we can only peer this port
> + * by a location match, not the default peer (lest we create a
> + * situation where we need to go back and undo a default peering
> + * when the port is later peered by location data)
> + */
> + if (port_dev->location)
> + return NULL;
I think you probably also want to reject matches where
port_dev->location is 0 but the peer port does have location data.
> @@ -222,9 +233,92 @@ static void unlink_peers(struct usb_port *left, struct usb_port *right)
> left->peer = NULL;
> }
>
> +/**
> + * for_each_child_port() - invoke 'fn' on all usb_port instances beneath 'hdev'
> + * @hdev: potential hub usb_device (validated by usb_hub_to_struct_hub)
> + * @level: track recursion level to stop after reaching USB spec max depth
> + * @p: parameter to pass to 'fn'
> + * @fn: routine to invoke on each port
> + *
> + * Recursively iterate all ports (depth-first) beneath 'hdev' until 'fn'
> + * returns a non-NULL usb_port or all ports have been visited.
> + */
> +static struct usb_port *for_each_child_port(struct usb_device *hdev, int level,
> + void *p, struct usb_port * (*fn)(struct usb_port *, void *))
> +{
> + struct usb_hub *hub = usb_hub_to_struct_hub(hdev);
> + int port1;
> +
> +#define MAX_HUB_DEPTH 5
> + if (!hub || level > MAX_HUB_DEPTH || hub->disconnected
> + || hdev->state == USB_STATE_NOTATTACHED)
> + return NULL;
> +
> + level++;
> + for (port1 = 1; port1 <= hdev->maxchild; port1++) {
> + struct usb_port *ret, *port_dev = hub->ports[port1 - 1];
> +
> + if (!port_dev)
> + continue;
> + ret = fn(port_dev, p);
> + if (ret)
> + return ret;
> + ret = for_each_child_port(port_dev->child, level, p, fn);
> + if (ret)
> + return ret;
> + }
> +
> + return NULL;
> +}
> +
> +static struct usb_port *do_match_location(struct usb_port *port_dev, void *_loc)
> +{
> + usb_port_location_t *loc = _loc;
> +
> + if (port_dev->location == *loc)
> + return port_dev;
> + return NULL;
> +}
> +
> +static struct usb_port *find_port_by_location(struct usb_device *hdev,
> + usb_port_location_t *loc)
> +{
> + return for_each_child_port(hdev, 1, loc, do_match_location);
> +}
> +
> +void usb_set_hub_port_location(struct usb_device *hdev, int port1,
> + usb_port_location_t location)
> +{
> + 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_port *peer, *port_dev;
> + struct usb_device *peer_hdev;
> +
> + if (!hub)
> + return;
> +
> + port_dev = hub->ports[port1 - 1];
> + port_dev->location = location;
> + if (port_dev->peer) {
> + WARN_ON(1);
> + return;
> + }
> +
> + if (!peer_hcd || !peer_hcd->rh_registered)
> + return;
> +
> + peer_hdev = peer_hcd->self.root_hub;
> + peer = find_port_by_location(peer_hdev, &port_dev->location);
> + if (!peer)
> + return;
> +
> + link_peers(port_dev, peer);
> +}
This all looks more complicated than necessary. Why not define a
usb_for_each_port function, like the usb_for_each_dev routine in usb.c?
And instead of calling it in a new usb_set_hub_port_location routine,
why not call it from find_default_peer?
> @@ -247,9 +341,12 @@ int usb_hub_create_port_device(struct usb_hub *hub, int port1)
> if (retval)
> goto error_register;
>
> - peer = find_default_peer(hub, port1);
> - if (peer)
> - link_peers(port_dev, peer);
> + if (!port_dev->peer) {
port_dev->peer should never be set at this point, if you don't try to
do the location matching during the ACPI callback.
> + struct usb_port *peer = find_default_peer(hub, port1);
> +
> + if (peer)
> + link_peers(port_dev, peer);
> + }
>
> pm_runtime_set_active(&port_dev->dev);
>
> diff --git a/drivers/usb/core/usb-acpi.c b/drivers/usb/core/usb-acpi.c
> index ce0f4e8d81cb..e1e1f40f6950 100644
> --- a/drivers/usb/core/usb-acpi.c
> +++ b/drivers/usb/core/usb-acpi.c
> @@ -41,6 +41,17 @@ bool usb_acpi_power_manageable(struct usb_device *hdev, int index)
> }
> EXPORT_SYMBOL_GPL(usb_acpi_power_manageable);
>
> +static void usb_acpi_check_port_peer(struct usb_device *hdev,
> + acpi_handle *handle, int port1, struct acpi_pld_info *pld)
> +{
> + if (!pld)
> + return;
> +
> + #define USB_ACPI_LOCATION_VALID (1 << 31)
> + usb_set_hub_port_location(hdev, port1, USB_ACPI_LOCATION_VALID
> + | pld->group_token << 8 | pld->group_position);
> +}
Why call an external function? Just store the value in the appropriate
port_dev->location.
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
