On Mon, 2014-02-24 at 16:46 -0500, Alan Stern wrote:
> On Fri, 21 Feb 2014, Dan Williams wrote:
>
> > Assume that the peer of a superspeed port is the port with the same id
> > on the shared_hcd root hub. This identification scheme is required of
> > external hubs by the USB3 spec [1]. However, for root hubs, tier mismatch
> > may be in effect [2]. Tier mismatch can only be enumerated via platform
> > firmware. For now, simply perform the nominal association.
> >
> > [1]: usb 3.1 section 10.3.3
> > [2]: xhci 1.1 appendix D
> >
> > Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
>
> > +/*
> > + * Set the default peer port for root hubs. Platform firmware will have
> > + * already set the peer if tier-mismatch is present. Assumes the
> > + * primary_hcd is registered first
> > + */
> > +static struct usb_port *find_default_peer(struct usb_hub *hub, int port1)
>
> The second sentence isn't accurate, at least, not as of this patch.
>
Moved it to patch 4 "usb: find internal hub tier mismatch via acpi"
> > +static void link_peers(struct usb_port *left, struct usb_port *right)
> > +{
> > + struct device *rdev;
> > + struct device *ldev;
> > +
>
> For safety, add
>
> if (left->peer == right && right->peer == left)
> return;
>
Done.
> > + if (left->peer) {
> > + right = left->peer;
> > + ldev = left->dev.parent->parent;
> > + rdev = right->dev.parent->parent;
>
> At this point we don't know if left->peer points to anything valid.
> It would be better to print out the names of left and right rather than
> left and left->peer. For debugging, print the value of left->peer
> as well.
>
> > +
> > + WARN_ONCE(1, "%s port%d already peered with %s %d\n",
> > + dev_name(ldev), left->portnum, dev_name(rdev),
> > + right->portnum);
>
> Since this isn't expected ever to happen, I think WARN is more
> appropriate than WARN_ONCE.
>
> Also, the gyrations you have to go through here and elsewhere to print
> useful names indicate that we should set up better names for the port
> devices. How about:
>
> dev_set_name(&port_dev->dev, "%s-port%d", dev_name(&hub->hdev->dev),
> port1);
>
> > + return;
> > + } else if (right->peer) {
> > + left = right->peer;
> > + ldev = left->dev.parent->parent;
> > + rdev = right->dev.parent->parent;
> > +
> > + WARN_ONCE(1, "%s port%d already peered with %s %d\n",
> > + dev_name(rdev), right->portnum, dev_name(ldev),
> > + left->portnum);
>
> Similar comments.
>
Replaced with:
+ if (left->peer || right->peer) {
+ struct usb_port *lpeer = left->peer;
+ struct usb_port *rpeer = right->peer;
+
+ WARN(1, "failed to peer %s and %s (%s -> %s) (%s -> %s)\n",
+ dev_name(&left->dev), dev_name(&right->dev),
+ dev_name(&left->dev),
+ lpeer ? dev_name(&lpeer->dev) : "[none]",
+ dev_name(&right->dev),
+ rpeer ? dev_name(&rpeer->dev) : "[none]");
+ return;
+ }
> > + return;
> > + }
> > +
> > + get_device(&right->dev);
> > + left->peer = right;
> > + get_device(&left->dev);
> > + right->peer = left;
>
> Add
> dev_dbg(&left->dev, "(%p) peered with %s (%p)\n", left,
> dev_name(&right->dev), right);
>
Deferred this to patch 5 "usb: sysfs link peer ports" which adds a
general facility for reporting the result of link_peers.
> > +}
> > +
> > +static void unlink_peers(struct usb_port *left, struct usb_port *right)
> > +{
> > + struct device *rdev = right->dev.parent->parent;
> > + struct device *ldev = left->dev.parent->parent;
> > +
> > + WARN_ONCE(right->peer != left || left->peer != right,
> > + "%s port%d and %s port%d are not peers?\n",
> > + dev_name(ldev), left->portnum, dev_name(rdev), right->portnum);
>
> Include left->peer and right->peer for debugging and use WARN.
Done.
>
> > +
>
> Add
> dev_dbg(&left->dev, "(%p) unpeered from %s (%p)\n", left,
> dev_name(&right->dev), right);
Done.
>
> > + put_device(&left->dev);
> > + right->peer = NULL;
> > + put_device(&right->dev);
> > + left->peer = NULL;
> > +}
>
> > @@ -190,9 +270,15 @@ exit:
> > return retval;
> > }
> >
> > -void usb_hub_remove_port_device(struct usb_hub *hub,
> > - int port1)
> > +void usb_hub_remove_port_device(struct usb_hub *hub, int port1)
> > {
> > - device_unregister(&hub->ports[port1 - 1]->dev);
> > -}
> > + struct usb_port *port_dev = hub->ports[port1 - 1];
> > + struct usb_port *peer = port_dev->peer;
> >
> > + mutex_lock(&peer_lock);
> > + if (peer)
> > + unlink_peers(port_dev, peer);
> > + mutex_unlock(&peer_lock);
> > +
>
> There's a race; another thread could peer port_dev to something else
> right here. One possible solution: Add a flag to the port structure
> indicating that it is being removed, and check the peer's flag when
> setting the peer.
I think it's enough to just de-reference port_dev->peer under the lock.
We have a reference count so it's ok if this port gets re-peered between
when we drop the lock and unregister the device.
But, this does bring up a good point about modifying ->peer, it needs
to be done while both ports in the relationship are pinned active. I
went ahead and added that to patch 6 "usb: defer suspension of
superspeed port while peer is powered" which is where we start caring
about whether ports are peered or not in usb_port_runtime_{suspend|
resume}
8<-------------
Subject: usb: assign default peer ports for root hubs
From: Dan Williams <dan.j.williams@xxxxxxxxx>
Assume that the peer of a superspeed port is the port with the same id
on the shared_hcd root hub. This identification scheme is required of
external hubs by the USB3 spec [1]. However, for root hubs, tier mismatch
may be in effect [2]. Tier mismatch can only be enumerated via platform
firmware. For now, simply perform the nominal association.
[1]: usb 3.1 section 10.3.3
[2]: xhci 1.1 appendix D
Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
---
drivers/usb/core/hub.h | 2 +
drivers/usb/core/port.c | 91 +++++++++++++++++++++++++++++++++++++++++++----
2 files changed, 86 insertions(+), 7 deletions(-)
diff --git a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h
index baf5b48b79f7..d51feb68165b 100644
--- a/drivers/usb/core/hub.h
+++ b/drivers/usb/core/hub.h
@@ -81,6 +81,7 @@ struct usb_hub {
* @child: usb device attached to the port
* @dev: generic device interface
* @port_owner: port's owner
+ * @peer: related usb2 and usb3 ports (share the same connector)
* @connect_type: port's connect type
* @portnum: port index num based one
* @power_is_on: port's power state
@@ -90,6 +91,7 @@ struct usb_port {
struct usb_device *child;
struct device dev;
struct dev_state *port_owner;
+ struct usb_port *peer;
enum usb_port_connect_type connect_type;
u8 portnum;
unsigned power_is_on:1;
diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
index 9ae8a499b70f..1974ba2145cf 100644
--- a/drivers/usb/core/port.c
+++ b/drivers/usb/core/port.c
@@ -21,6 +21,7 @@
#include "hub.h"
+static DEFINE_MUTEX(peer_lock);
static const struct attribute_group *port_dev_group[];
static ssize_t connect_type_show(struct device *dev,
@@ -146,9 +147,72 @@ struct device_type usb_port_device_type = {
.pm = &usb_port_pm_ops,
};
+/*
+ * Set the default peer port for root hubs. Assumes the primary_hcd is
+ * registered first
+ */
+static struct usb_port *find_default_peer(struct usb_hub *hub, int port1)
+{
+ struct usb_device *hdev = hub->hdev;
+ struct usb_port *peer = NULL;
+
+ if (!hdev->parent) {
+ struct usb_hub *peer_hub;
+ struct usb_device *peer_hdev;
+ struct usb_hcd *hcd = bus_to_hcd(hdev->bus);
+ struct usb_hcd *peer_hcd = hcd->primary_hcd;
+
+ if (!peer_hcd || hcd == peer_hcd)
+ return NULL;
+
+ peer_hdev = peer_hcd->self.root_hub;
+ peer_hub = usb_hub_to_struct_hub(peer_hdev);
+ if (peer_hub && port1 <= peer_hdev->maxchild)
+ peer = peer_hub->ports[port1 - 1];
+ }
+
+ return peer;
+}
+
+static void link_peers(struct usb_port *left, struct usb_port *right)
+{
+ if (left->peer == right && right->peer == left)
+ return;
+
+ if (left->peer || right->peer) {
+ struct usb_port *lpeer = left->peer;
+ struct usb_port *rpeer = right->peer;
+
+ WARN(1, "failed to peer %s and %s (%s -> %s) (%s -> %s)\n",
+ dev_name(&left->dev), dev_name(&right->dev),
+ dev_name(&left->dev),
+ lpeer ? dev_name(&lpeer->dev) : "[none]",
+ dev_name(&right->dev),
+ rpeer ? dev_name(&rpeer->dev) : "[none]");
+ return;
+ }
+
+ get_device(&right->dev);
+ left->peer = right;
+ get_device(&left->dev);
+ right->peer = left;
+}
+
+static void unlink_peers(struct usb_port *left, struct usb_port *right)
+{
+ WARN(right->peer != left || left->peer != right,
+ "%s and %s are not peers?\n",
+ dev_name(&left->dev), dev_name(&right->dev));
+
+ put_device(&left->dev);
+ right->peer = NULL;
+ put_device(&right->dev);
+ left->peer = NULL;
+}
+
int usb_hub_create_port_device(struct usb_hub *hub, int port1)
{
- struct usb_port *port_dev = NULL;
+ struct usb_port *port_dev, *peer;
int retval;
port_dev = kzalloc(sizeof(*port_dev), GFP_KERNEL);
@@ -163,12 +227,18 @@ int usb_hub_create_port_device(struct usb_hub *hub, int port1)
port_dev->dev.parent = hub->intfdev;
port_dev->dev.groups = port_dev_group;
port_dev->dev.type = &usb_port_device_type;
- dev_set_name(&port_dev->dev, "port%d", port1);
-
+ dev_set_name(&port_dev->dev, "%s-port%d", dev_name(&hub->hdev->dev),
+ port1);
retval = device_register(&port_dev->dev);
if (retval)
goto error_register;
+ mutex_lock(&peer_lock);
+ peer = find_default_peer(hub, port1);
+ if (peer)
+ link_peers(port_dev, peer);
+ mutex_unlock(&peer_lock);
+
pm_runtime_set_active(&port_dev->dev);
/* It would be dangerous if user space couldn't prevent usb
@@ -190,9 +260,16 @@ exit:
return retval;
}
-void usb_hub_remove_port_device(struct usb_hub *hub,
- int port1)
+void usb_hub_remove_port_device(struct usb_hub *hub, int port1)
{
- device_unregister(&hub->ports[port1 - 1]->dev);
-}
+ struct usb_port *port_dev = hub->ports[port1 - 1];
+ struct usb_port *peer;
+ mutex_lock(&peer_lock);
+ peer = port_dev->peer;
+ if (peer)
+ unlink_peers(port_dev, peer);
+ mutex_unlock(&peer_lock);
+
+ device_unregister(&port_dev->dev);
+}
--
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
