On Wed, 2014-03-05 at 16:44 -0500, Alan Stern wrote:
> On Wed, 5 Mar 2014, Dan Williams wrote:
>
> > 8<------------
> > 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).
>
> Strictly speaking, the peer information could be set more than once (if
> the hub driver gets unbound and rebound to an internal hub).
Once per lifetime of the hub object.
>
> > 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>
>
> > @@ -182,8 +182,40 @@ static void unlink_peers(struct usb_port *left, struct usb_port *right)
> > }
> >
> > /*
> > - * Set the default peer port for root hubs, or via the upstream peer
> > - * relationship for all other hubs
> > + * For each usb hub device in the system check to see if it is in the
> > + * peer domain of the given port_dev, and if it is check to see if it
> > + * has a port that matches the given port by location
> > + */
> > +static int match_location(struct usb_device *peer_hdev, void *p)
> > +{
> > + int port1;
> > + struct usb_hcd *peer_hcd;
> > + struct usb_port *port_dev = p, *peer;
> > + struct usb_hub *peer_hub = usb_hub_to_struct_hub(peer_hdev);
> > + struct usb_device *hdev = to_usb_device(port_dev->dev.parent->parent);
> > +
> > + if (!peer_hub)
> > + return 0;
> > +
> > + peer_hcd = bus_to_hcd(hdev->bus)->shared_hcd;
> > + if (!peer_hcd || peer_hcd != bus_to_hcd(peer_hdev->bus))
> > + return 0;
>
> This is correct but a little difficult to follow. How about something
> along these lines instead?
>
> hcd = bus_to_hcd(hdev->bus);
> peer_hcd = bus_to_hcd(peer_hdev->bus);
> if (peer_hcd != hcd->shared_hcd)
> return 0;
Looks good, I added a clarifying comment as well...
>
> > @@ -192,7 +224,17 @@ static void find_and_link_peer(struct usb_hub *hub, int port1)
> > struct usb_device *peer_hdev;
> > struct usb_hub *peer_hub;
> >
> > - if (!hdev->parent) {
> > + /*
> > + * 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) {
> > + /* we link the peer in match_location() if found */
> > + usb_for_each_dev(port_dev, match_location);
> > + return;
>
> I took a closer look at the example in Appendix D of the xHCI spec.
> Somewhat distressingly, the sample ACPI code in D.1.1 does not include
> _PLD information for the internal (not user-visible) ports. Thus, of
> the four root-hub ports in Figure 128, there is _PLD data only for
> HCP4. This would lead us to assume that ports HCP1 and HCP3 are peers,
> but they aren't. (And we would fail to realize that HCP3 and IP3 _are_
> peers.)
>
> In this case the error would be harmless, because HCP1 is not
> connectable. But there could be other cases where it would matter.
> (In theory, a platform could hard-wire a high-speed device to the
> HS port _and_ a SuperSpeed device to the corresponding SS port! I
> don't know if this would comply with the spec -- but manufacturers
> wouldn't let a little drawback like that affect their designs.)
Indeed.
> (It's also somewhat distressing that the sample _UPC data for ports
> HCP1 and HCP2 has the "connectable" attributes backwards! In addition,
> the example in Figure 128 disobeys the requirement in 4.24.2.2 that
> when an embedded hub is present, a tier mismatch is not allowed!)
>
> In order to make this work, maybe we need to look at the USB2 xHCI
> Supported Protocol Capability Integrated Hub Implemented (IHI) flag
> (section 2.24.2.1).
Possibly. However, at this point I would defer doing anything more
complicated until we actually start to see platforms land with tier
mismatch. In my testing I have yet to see either tier mismatch, or
root/internal hubs where hub_is_port_power_switchable() returned true.
>
> > @@ -194,7 +190,16 @@ static struct acpi_device *usb_acpi_find_companion(struct device *dev)
> > if (!adev)
> > return NULL;
> > }
> > - usb_acpi_check_port_connect_type(udev, adev->handle, port1);
> > + handle = adev->handle;
> > + status = acpi_get_physical_device_location(handle, &pld);
> > + if (ACPI_FAILURE(status) || !pld)
> > + return adev;
>
> The logic here and in usb_acpi_get_connect_type above seems wrong.
> The ACPI spec states that _PLD data need not be available for ports
> that aren't visible to the user. (See the example code in section 9.14
> of the ACPI-3.0 spec.)
>
> So do we want to treat absence of _PLD as meaning the connect type is
> unknown? It could very well be hard-wired.
It very well could be. I refactored that routine to keep the same
behavior and missed that the original implementation was erroring out
too early ('pld' == NULL). Nothing in the kernel currently cares about
the distinction of non-visible ports. Yes, there was a plan to
automatically power off such ports, but no patches being proposed
currently that exploit that classification. I think we can delay a
patch to fix this up until after the current round of port power control
patches land.
> > --- a/drivers/usb/core/usb.h
> > +++ b/drivers/usb/core/usb.h
> > @@ -175,6 +175,10 @@ extern void usbfs_conn_disc_event(void);
> > extern int usb_devio_init(void);
> > extern void usb_devio_cleanup(void);
> >
> > +/* firmware specific cookie identifying a port's location */
> > +#define USB_PORT_LOCATION_NONE 0
>
> This #define isn't used anywhere. We don't need it.
It's documentation, but might as well use good old /* */ for that.
Here it is... no rebases required for patch 7, 8.
8<--------
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>
---
drivers/usb/core/hub.h | 2 ++
drivers/usb/core/port.c | 56 ++++++++++++++++++++++++++++++++++++++++---
drivers/usb/core/usb-acpi.c | 41 ++++++++++++++++++-------------
drivers/usb/core/usb.h | 6 +++++
4 files changed, 83 insertions(+), 22 deletions(-)
diff --git a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h
index d51feb68165b..6858a55eceb5 100644
--- a/drivers/usb/core/hub.h
+++ b/drivers/usb/core/hub.h
@@ -83,6 +83,7 @@ struct usb_hub {
* @port_owner: port's owner
* @peer: related usb2 and usb3 ports (share the same connector)
* @connect_type: port's connect type
+ * @location: opaque representation of platform connector location
* @portnum: port index num based one
* @power_is_on: port's power state
* @did_runtime_put: port has done pm_runtime_put().
@@ -93,6 +94,7 @@ struct usb_port {
struct dev_state *port_owner;
struct usb_port *peer;
enum usb_port_connect_type connect_type;
+ usb_port_location_t location;
u8 portnum;
unsigned power_is_on:1;
unsigned did_runtime_put:1;
diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
index fd53257de6a9..d29e9dc8ea9f 100644
--- a/drivers/usb/core/port.c
+++ b/drivers/usb/core/port.c
@@ -182,8 +182,42 @@ static void unlink_peers(struct usb_port *left, struct usb_port *right)
}
/*
- * Set the default peer port for root hubs, or via the upstream peer
- * relationship for all other hubs
+ * For each usb hub device in the system check to see if it is in the
+ * peer domain of the given port_dev, and if it is check to see if it
+ * has a port that matches the given port by location
+ */
+static int match_location(struct usb_device *peer_hdev, void *p)
+{
+ int port1;
+ struct usb_hcd *hcd, *peer_hcd;
+ struct usb_port *port_dev = p, *peer;
+ struct usb_hub *peer_hub = usb_hub_to_struct_hub(peer_hdev);
+ struct usb_device *hdev = to_usb_device(port_dev->dev.parent->parent);
+
+ if (!peer_hub)
+ return 0;
+
+ hcd = bus_to_hcd(hdev->bus);
+ peer_hcd = bus_to_hcd(peer_hdev->bus);
+ /* peer_hcd is provisional until we verify it against the known peer */
+ if (peer_hcd != hcd->shared_hcd)
+ return 0;
+
+ for (port1 = 1; port1 <= peer_hdev->maxchild; port1++) {
+ peer = peer_hub->ports[port1 - 1];
+ if (peer && peer->location == port_dev->location) {
+ link_peers(port_dev, peer);
+ return 1; /* done */
+ }
+ }
+
+ return 0;
+}
+
+/*
+ * Find the peer port either via explicit platform firmware "location"
+ * data, the peer hcd for root hubs, or the upstream peer relationship
+ * for all other hubs.
*/
static void find_and_link_peer(struct usb_hub *hub, int port1)
{
@@ -192,7 +226,17 @@ static void find_and_link_peer(struct usb_hub *hub, int port1)
struct usb_device *peer_hdev;
struct usb_hub *peer_hub;
- if (!hdev->parent) {
+ /*
+ * 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) {
+ /* we link the peer in match_location() if found */
+ usb_for_each_dev(port_dev, match_location);
+ return;
+ } else if (!hdev->parent) {
struct usb_hcd *hcd = bus_to_hcd(hdev->bus);
struct usb_hcd *peer_hcd = hcd->shared_hcd;
@@ -219,8 +263,12 @@ static void find_and_link_peer(struct usb_hub *hub, int port1)
if (!peer_hub || port1 > peer_hdev->maxchild)
return;
+ /*
+ * we found a valid default peer, last check is to make sure it
+ * does not have location data
+ */
peer = peer_hub->ports[port1 - 1];
- if (peer)
+ if (peer && peer->location == 0)
link_peers(port_dev, peer);
}
diff --git a/drivers/usb/core/usb-acpi.c b/drivers/usb/core/usb-acpi.c
index ce0f4e8d81cb..25fc4e0a1c59 100644
--- a/drivers/usb/core/usb-acpi.c
+++ b/drivers/usb/core/usb-acpi.c
@@ -85,19 +85,13 @@ int usb_acpi_set_power_state(struct usb_device *hdev, int index, bool enable)
}
EXPORT_SYMBOL_GPL(usb_acpi_set_power_state);
-static int usb_acpi_check_port_connect_type(struct usb_device *hdev,
- acpi_handle handle, int port1)
+static enum usb_port_connect_type usb_acpi_get_connect_type(acpi_handle handle,
+ struct acpi_pld_info *pld)
{
enum usb_port_connect_type connect_type = USB_PORT_CONNECT_TYPE_UNKNOWN;
struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
- struct usb_hub *hub = usb_hub_to_struct_hub(hdev);
- struct acpi_pld_info *pld;
union acpi_object *upc;
acpi_status status;
- int ret = 0;
-
- if (!hub)
- return 0;
/*
* According to ACPI Spec 9.13. PLD indicates whether usb port is
@@ -107,15 +101,10 @@ static int usb_acpi_check_port_connect_type(struct usb_device *hdev,
* a usb device is directly hard-wired to the port. If no visible and
* no connectable, the port would be not used.
*/
- status = acpi_get_physical_device_location(handle, &pld);
- if (ACPI_FAILURE(status))
- return -ENODEV;
-
status = acpi_evaluate_object(handle, "_UPC", NULL, &buffer);
upc = buffer.pointer;
if (!upc || (upc->type != ACPI_TYPE_PACKAGE)
|| upc->package.count != 4) {
- ret = -EINVAL;
goto out;
}
@@ -126,14 +115,18 @@ static int usb_acpi_check_port_connect_type(struct usb_device *hdev,
connect_type = USB_PORT_CONNECT_TYPE_HARD_WIRED;
else if (!pld->user_visible)
connect_type = USB_PORT_NOT_USED;
- hub->ports[port1 - 1]->connect_type = connect_type;
-
out:
- ACPI_FREE(pld);
kfree(upc);
- return ret;
+ return connect_type;
}
+
+/*
+ * Private to usb-acpi, all the core needs to know is that
+ * port_dev->location is non-zero when it has been set by the firmware.
+ */
+#define USB_ACPI_LOCATION_VALID (1 << 31)
+
static struct acpi_device *usb_acpi_find_companion(struct device *dev)
{
struct usb_device *udev;
@@ -164,6 +157,9 @@ static struct acpi_device *usb_acpi_find_companion(struct device *dev)
} else if (is_usb_port(dev)) {
struct usb_port *port_dev = to_usb_port(dev);
int port1 = port_dev->portnum;
+ struct acpi_pld_info *pld;
+ acpi_handle *handle;
+ acpi_status status;
/* Get the struct usb_device point of port's hub */
udev = to_usb_device(dev->parent->parent);
@@ -194,7 +190,16 @@ static struct acpi_device *usb_acpi_find_companion(struct device *dev)
if (!adev)
return NULL;
}
- usb_acpi_check_port_connect_type(udev, adev->handle, port1);
+ handle = adev->handle;
+ status = acpi_get_physical_device_location(handle, &pld);
+ if (ACPI_FAILURE(status) || !pld)
+ return adev;
+
+ port_dev->location = USB_ACPI_LOCATION_VALID
+ | pld->group_token << 8 | pld->group_position;
+ port_dev->connect_type = usb_acpi_get_connect_type(handle, pld);
+ ACPI_FREE(pld);
+
return adev;
}
diff --git a/drivers/usb/core/usb.h b/drivers/usb/core/usb.h
index af89070d771d..7ddcecac12e7 100644
--- a/drivers/usb/core/usb.h
+++ b/drivers/usb/core/usb.h
@@ -175,6 +175,12 @@ extern void usbfs_conn_disc_event(void);
extern int usb_devio_init(void);
extern void usb_devio_cleanup(void);
+/*
+ * Firmware specific cookie identifying a port's location. '0' == no location
+ * data available
+ */
+typedef u32 usb_port_location_t;
+
/* internal notify stuff */
extern void usb_notify_add_device(struct usb_device *udev);
extern void usb_notify_remove_device(struct usb_device *udev);
--
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
