Re: [PATCH v5 04/16] usb: find internal hub tier mismatch via acpi
On Thu, 27 Feb 2014, Dan Williams wrote: > > BTW, after some more thought, I decided the right way to avoid races in > > the peer assignments is to do some of the work under hub.c's > > device_state_lock. Add a "usb_" prefix to the name and share it with > > port.c. We already are guaranteed that all changes to port->child are > > protected by this lock, which is exactly what we need. > > > > I had wrapped locations that set port->child to NULL in the peer_lock, > but I'll look at just re-using the device_state_lock for that > scenario. You're right; an alternative to using device_state_lock in port.c is using peer_lock in hub.c. In fact, that would be superior; it wouldn't mean repurposing an existing lock. (Maybe change "peer_lock" to "usb_port_peer_mutex" while we're at it.) > I'm still thinking the start of a disconnect operation > should flush in-flight peering operations. Otherwise we'll need to > take a reference under the device_state_lock, but I'll give it more > thought. If hub_disconnect holds the mutex around the loop that removes the port devices, wouldn't that be enough? Especially if we also include the region where device_state_lock is held? Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 04/16] usb: find internal hub tier mismatch via acpi
On Thu, Feb 27, 2014 at 10:48 AM, Alan Stern wrote: > On Thu, 27 Feb 2014, Dan Williams wrote: > >> > If the port's ACPI data agrees with the default matching, there's no >> > issue. But if they disagree, don't accept the default match. That way >> > you never have to correct a mistaken match. >> >> So it turns out this simplifies the patch a bit, by getting rid of >> invalidate_dependent_peers(). However, we still need >> enumerate_dependent_peers() to handle cases where descendants are >> registered prior to peering the parents via location data. The big > > How could that happen? > > Suppose B is a child of A, and they both have been registered for some > time but don't have any peers assigned yet. Now along comes A', and > the firmware location data tells us that A' and A are peers. > > B still doesn't have a matching peer, though. Not until B' (a child of > A') is registered. At that time, B' will automatically be peered with > B, so no extra work is needed. > > The main point is that the location data for A' is available before any > descendants of A' can be created. Therefore descendants cannot be > registered prior to peering the parents via location data. Ah, yes, there's no possible work for enumerate_dependent_peers() to do because by definition if it could find a dependent peer, that same peer would have done the same association in reverse via its default mapping. Ok, that code can die too. >> complexity savings is killing the need for a solution like the one >> proposed in "[RFC PATCH v5 16/16] usb, xhci: flush initial hub >> discovery to gate port power control" >> >> i went ahead and changed 'struct usb_port_location' to 'typedef u32 >> usb_port_location_t', and am preparing to release a v6 addressing the >> current comments. > > Okay. I suggest that when v6 is ready, you start by posting just the > equivalents to v5 patches 1-5 (or 1-6). Once we agree on the details > of the peering, we'll move forward to consider synchronization of hub > and port PM operations. Will do. > BTW, after some more thought, I decided the right way to avoid races in > the peer assignments is to do some of the work under hub.c's > device_state_lock. Add a "usb_" prefix to the name and share it with > port.c. We already are guaranteed that all changes to port->child are > protected by this lock, which is exactly what we need. > I had wrapped locations that set port->child to NULL in the peer_lock, but I'll look at just re-using the device_state_lock for that scenario. I'm still thinking the start of a disconnect operation should flush in-flight peering operations. Otherwise we'll need to take a reference under the device_state_lock, but I'll give it more thought. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 04/16] usb: find internal hub tier mismatch via acpi
On Thu, 27 Feb 2014, Dan Williams wrote: > > If the port's ACPI data agrees with the default matching, there's no > > issue. But if they disagree, don't accept the default match. That way > > you never have to correct a mistaken match. > > So it turns out this simplifies the patch a bit, by getting rid of > invalidate_dependent_peers(). However, we still need > enumerate_dependent_peers() to handle cases where descendants are > registered prior to peering the parents via location data. The big How could that happen? Suppose B is a child of A, and they both have been registered for some time but don't have any peers assigned yet. Now along comes A', and the firmware location data tells us that A' and A are peers. B still doesn't have a matching peer, though. Not until B' (a child of A') is registered. At that time, B' will automatically be peered with B, so no extra work is needed. The main point is that the location data for A' is available before any descendants of A' can be created. Therefore descendants cannot be registered prior to peering the parents via location data. > complexity savings is killing the need for a solution like the one > proposed in "[RFC PATCH v5 16/16] usb, xhci: flush initial hub > discovery to gate port power control" > > i went ahead and changed 'struct usb_port_location' to 'typedef u32 > usb_port_location_t', and am preparing to release a v6 addressing the > current comments. Okay. I suggest that when v6 is ready, you start by posting just the equivalents to v5 patches 1-5 (or 1-6). Once we agree on the details of the peering, we'll move forward to consider synchronization of hub and port PM operations. BTW, after some more thought, I decided the right way to avoid races in the peer assignments is to do some of the work under hub.c's device_state_lock. Add a "usb_" prefix to the name and share it with port.c. We already are guaranteed that all changes to port->child are protected by this lock, which is exactly what we need. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 04/16] usb: find internal hub tier mismatch via acpi
On Wed, Feb 26, 2014 at 2:07 PM, Alan Stern wrote: > On Wed, 26 Feb 2014, Dan Williams wrote: > >> > I've been thinking about this. Maybe it isn't a problem, because now >> > you don't set up the peer matching until after the port has been >> > registered. All you have to do is allow the ACPI data to prevent a >> > default match if the location data values don't agree. >> > >> > That would simplify this patch an awful lot. >> >> Hm, interesting. It relies on the fact that the firmware must >> identify both peers if it has location data for one, but I think that >> is a reasonable constraint. > > If the firmware doesn't have location data for both peers in a > non-default matching (which presumably means there's a tier mismatch) > then there's no way for us to match them up correctly anyhow. > >> If a port has acpi data, don't permit a default matching... sounds good to >> me! > > If the port's ACPI data agrees with the default matching, there's no > issue. But if they disagree, don't accept the default match. That way > you never have to correct a mistaken match. So it turns out this simplifies the patch a bit, by getting rid of invalidate_dependent_peers(). However, we still need enumerate_dependent_peers() to handle cases where descendants are registered prior to peering the parents via location data. The big complexity savings is killing the need for a solution like the one proposed in "[RFC PATCH v5 16/16] usb, xhci: flush initial hub discovery to gate port power control" i went ahead and changed 'struct usb_port_location' to 'typedef u32 usb_port_location_t', and am preparing to release a v6 addressing the current comments. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 04/16] usb: find internal hub tier mismatch via acpi
On Wed, 26 Feb 2014, Dan Williams wrote: > > I've been thinking about this. Maybe it isn't a problem, because now > > you don't set up the peer matching until after the port has been > > registered. All you have to do is allow the ACPI data to prevent a > > default match if the location data values don't agree. > > > > That would simplify this patch an awful lot. > > Hm, interesting. It relies on the fact that the firmware must > identify both peers if it has location data for one, but I think that > is a reasonable constraint. If the firmware doesn't have location data for both peers in a non-default matching (which presumably means there's a tier mismatch) then there's no way for us to match them up correctly anyhow. > If a port has acpi data, don't permit a default matching... sounds good to me! If the port's ACPI data agrees with the default matching, there's no issue. But if they disagree, don't accept the default match. That way you never have to correct a mistaken match. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 04/16] usb: find internal hub tier mismatch via acpi
On Wed, Feb 26, 2014 at 1:35 PM, Alan Stern wrote: > On Fri, 21 Feb 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. >> >> The bulk of the implementation is recursively fixing up peer >> associations once we detect tier mismatch. Due to the way acpi data is >> associated to a usb_port device (via a callback triggered by > > Is the callback _triggered_ (i.e., asynchronously) by device_register, > or is it simply _invoked_ (synchronously) by device_register? Synchronously, we complete the acpi notification/registration before device_register() for the port returns. >> device_register()) we do not discover tier mismatch until after the port >> has been registered. > > I've been thinking about this. Maybe it isn't a problem, because now > you don't set up the peer matching until after the port has been > registered. All you have to do is allow the ACPI data to prevent a > default match if the location data values don't agree. > > That would simplify this patch an awful lot. Hm, interesting. It relies on the fact that the firmware must identify both peers if it has location data for one, but I think that is a reasonable constraint. If a port has acpi data, don't permit a default matching... sounds good to me! -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 04/16] usb: find internal hub tier mismatch via acpi
On Fri, 21 Feb 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. > > The bulk of the implementation is recursively fixing up peer > associations once we detect tier mismatch. Due to the way acpi data is > associated to a usb_port device (via a callback triggered by Is the callback _triggered_ (i.e., asynchronously) by device_register, or is it simply _invoked_ (synchronously) by device_register? > device_register()) we do not discover tier mismatch until after the port > has been registered. I've been thinking about this. Maybe it isn't a problem, because now you don't set up the peer matching until after the port has been registered. All you have to do is allow the ACPI data to prevent a default match if the location data values don't agree. That would simplify this patch an awful lot. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 04/16] usb: find internal hub tier mismatch via acpi
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.
The bulk of the implementation is recursively fixing up peer
associations once we detect tier mismatch. Due to the way acpi data is
associated to a usb_port device (via a callback triggered by
device_register()) we do not discover tier mismatch until after the port
has been registered. This leads to a question about what happens when a
pm runtime event occurs while the peer associations are wrong, and can
we prevent that from occurring? The result of wrong peer associations
is always the possibility that a USB3 device switches to its USB2
connection upon detecting the USB3 port being turned off. As far as
closing the race there are 2 considerations:
1/ the acpi data may be wrong so the risk of wrong peer associations
cannot be entirely eliminated by the kernel
2/ if the acpi data is good then we can potentially close the race by
waiting for all the initial hub discovery to complete before processing
the first pm runtime suspend event. (not implemented in this patch).
[1]: xhci 1.1 appendix D figure 131
[2]: acpi 5 section 6.1.8
Signed-off-by: Dan Williams
---
drivers/usb/core/hub.h |6 ++
drivers/usb/core/port.c | 161 ++-
drivers/usb/core/usb-acpi.c | 35 +++--
drivers/usb/core/usb.h |2 +
4 files changed, 192 insertions(+), 12 deletions(-)
diff --git a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h
index d51feb68165b..e7a9666e7cd6 100644
--- 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;
+};
+
/**
* struct usb port - kernel's representation of a usb port
* @child: usb device attached to the port
@@ -83,6 +87,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 +98,7 @@ struct usb_port {
struct dev_state *port_owner;
struct usb_port *peer;
enum usb_port_connect_type connect_type;
+ struct usb_port_location 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 068d495007e1..0b8ae73b0466 100644
--- a/drivers/usb/core/port.c
+++ b/drivers/usb/core/port.c
@@ -154,11 +154,14 @@ struct device_type usb_port_device_type = {
*/
static struct usb_port *find_default_peer(struct usb_hub *hub, int port1)
{
- struct usb_device *hdev = hub->hdev;
+ struct usb_device *hdev = hub ? hub->hdev : NULL;
struct usb_device *peer_hdev;
struct usb_port *peer = NULL;
struct usb_hub *peer_hub;
+ if (!hub)
+ return NULL;
+
if (!hdev->parent) {
struct usb_hcd *hcd = bus_to_hcd(hdev->bus);
struct usb_hcd *peer_hcd = hcd->primary_hcd;
@@ -239,9 +242,154 @@ 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)
+ return NULL;
+
+ level++;
+ for (port1 = 1; port1 <= hdev->maxchild; port1++) {
+ struct usb_port *ret, *port_dev = hub->ports[port1 - 1];
+
+ 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;
+}
+
+stati
