Discussion:
[RFC v2 00/22] USB 3.0 hub support & xHCI split roothub for 2.6.38
Sarah Sharp
2010-12-30 23:22:52 UTC
Permalink
This is the second version of the patchset to add support for USB 3.0
hubs, and make the xHCI roothub act like an external USB 3.0 hub by
registering two roothubs: a USB 2.0 hub and a USB 3.0 hub.

Most of the code that changes USB core behavior is in patches 1-3 and
6-11. Patch one is only slightly updated from last time, with the
addition of using DeviceRemovable instead of bitmap. Patch three is
unchanged from the last RFC.

The real meat of the changes to the USB core are in patches 8-11. I
would especially like some feedback on patch 11 (USB: Set usb_hcd->state
and flags for shared roothubs).

Changes from the last RFC include:

- introduce a new host controller flag, HCD_SHARED, to indicate a host
wants to have the USB core allocate a shared roothub.

- introduce a new variable in usb_hcd, bcdUSB, that indicates what
speed the roothub operates at. This originally mirrors
usb_hcd->driver->flags & HCD_MASK, but this field is writable, and
can be changed after the second roothub is allocated.

- introduce a new variable in usb_hcd, primary_hcd, that indicates
which host controller is the primary HCD. The primary HCD is the
roothub that is allocated first.

- Support for bus and PCI suspend of shared roothubs.

- Some removal of the translation of USB 3.0 port status into USB 2.0
port status. I was able to get rid of USB_PORT_STAT_SUPER_SPEED, but
I haven't had time to look at getting rid of the port power
translation. There isn't any other translations in the core.

- Disabling USB 3.0 hub auto-suspend, since the mechanisms to do device
suspend are slightly different for USB 3.0 devices, and we don't have
support for that.

The patchset is stable, and everything seems to work just as well as it
did before the xHCI roothub was split (with the bonus of USB 3.0 hub
support, of course). Andiry, I would really appreciate it if you could
run this patchset on your machine that keeps the power session active
across suspend.


John Youn (1):
USB 3.0 Hub Changes

Sarah Sharp (21):
USB: Clear "warm" port reset change.
usb: Make USB 3.0 roothub have a SS EP comp descriptor.
xhci: Always use usb_hcd in URB instead of converting xhci_hcd.
xhci: Change hcd_priv into a pointer.
usb: Make usb_hcd_pci_probe labels more descriptive.
usb: Refactor irq enabling out of usb_add_hcd()
usb: Change usb_hcd->bandwidth_mutex to a pointer.
usb: Store bus type in usb_hcd, not in driver flags.
usb: Make core allocate resources per PCI-device.
USB: Set usb_hcd->state and flags for shared roothubs.
xhci: Index with a port array instead of PORTSC addresses.
xhci: Refactor bus suspend state into a struct.
xhci: Change xhci_find_slot_id_by_port() API.
xhci: Register second xHCI roothub.
xhci: Return a USB 3.0 hub descriptor for USB3 roothub.
xhci: Limit roothub ports to 15 USB3 & 31 USB2 ports.
xhci: Make roothub functions deal with device removal.
xhci: Fix re-init on power loss after resume.
xhci: Fixes for suspend/resume of shared HCDs.
USB: Remove bogus USB_PORT_STAT_SUPER_SPEED symbol.
usb: Disable auto-suspend for USB 3.0 hubs.

drivers/staging/usbip/vhci_hcd.c | 4 +-
drivers/usb/core/hcd-pci.c | 93 ++++++++--
drivers/usb/core/hcd.c | 181 ++++++++++++++-----
drivers/usb/core/hub.c | 131 ++++++++++----
drivers/usb/core/message.c | 22 ++--
drivers/usb/gadget/dummy_hcd.c | 4 +-
drivers/usb/host/ehci-hub.c | 4 +-
drivers/usb/host/imx21-hcd.c | 4 +-
drivers/usb/host/isp116x-hcd.c | 4 +-
drivers/usb/host/isp1362-hcd.c | 4 +-
drivers/usb/host/isp1760-hcd.c | 4 +-
drivers/usb/host/ohci-hub.c | 11 +-
drivers/usb/host/oxu210hp-hcd.c | 4 +-
drivers/usb/host/r8a66597-hcd.c | 5 +-
drivers/usb/host/sl811-hcd.c | 4 +-
drivers/usb/host/u132-hcd.c | 11 +-
drivers/usb/host/xhci-hub.c | 363 +++++++++++++++++++++++++++-----------
drivers/usb/host/xhci-mem.c | 88 +++++++++-
drivers/usb/host/xhci-pci.c | 54 +++++-
drivers/usb/host/xhci-ring.c | 112 ++++++++++--
drivers/usb/host/xhci.c | 107 +++++++++---
drivers/usb/host/xhci.h | 37 +++--
drivers/usb/musb/musb_virthub.c | 4 +-
drivers/usb/wusbcore/rh.c | 4 +-
include/linux/usb/ch11.h | 42 ++++-
include/linux/usb/hcd.h | 12 ++-
26 files changed, 997 insertions(+), 316 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Sarah Sharp
2010-12-30 23:22:57 UTC
Permalink
From: John Youn <John.Youn-HKixBCOQz3hWk0Htik3J/***@public.gmane.org>

Update the USB core to deal with USB 3.0 hubs. These hubs have a slightly
different hub descriptor than USB 2.0 hubs, with a fixed (rather than
variable length) size. Change the USB core's hub descriptor to have a
union for the last fields that differ. Change the host controller drivers
that access those last fields (DeviceRemovable and PortPowerCtrlMask) to
use the union.

Translate the new version of the hub port status field into the old
version that khubd understands. (Note: we need to fix it to translate the
roothub's port status once we stop converting it to USB 2.0 hub status
internally.)

Add new code to handle link state change status. Send out new control
messages that are needed for USB 3.0 hubs, like Set Hub Depth.

This patch is a modified version of the original patch submitted by John
Youn. It's updated to reflect the removal of the "bitmap" #define, and
change the hub descriptor accesses of a couple new host controller
drivers.

Signed-off-by: John Youn <johnyoun-HKixBCOQz3hWk0Htik3J/***@public.gmane.org>
Signed-off-by: Sarah Sharp <sarah.a.sharp-VuQAYsv1563Yd54FQh9/***@public.gmane.org>
Cc: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.yj-***@public.gmane.org>
Cc: Inaky Perez-Gonzalez <inaky.perez-gonzalez-***@public.gmane.org>
Cc: Tony Olech <tony.olech-ze5PQcJzaeOs0fm86SgGizGjJy/***@public.gmane.org>
Cc: "Robert P. J. Day" <rpjday-L09J2beyid0N/***@public.gmane.org>
Cc: Max Vozeler <mvz-/***@public.gmane.org>
Cc: Tejun Heo <tj-DgEjT+Ai2ygdnm+***@public.gmane.org>
Cc: Yoshihiro Shimoda <yoshihiro.shimoda.uh-***@public.gmane.org>
Cc: Rodolfo Giometti <giometti-***@public.gmane.org>
Cc: Mike Frysinger <vapier-aBrp7R+bbdUdnm+***@public.gmane.org>
Cc: Anton Vorontsov <avorontsov-Igf4POYTYCDQT0dZR+***@public.gmane.org>
Cc: Sebastian Siewior <bigeasy-***@public.gmane.org>
Cc: Lothar Wassmann <LW-bxm8fMRDkQLDiMYJYoSAnRvVK+***@public.gmane.org>
Cc: Olav Kongas <ok-7zpu9HQY+***@public.gmane.org>
Cc: Martin Fuzzey <mfuzzey-***@public.gmane.org>
Cc: Alan Stern <stern-nwvwT67g6+6dFdvTe/***@public.gmane.org>
Cc: David Brownell <dbrownell-Rn4VEauK+AKRv+***@public.gmane.org>
---
drivers/staging/usbip/vhci_hcd.c | 4 +-
drivers/usb/core/hub.c | 73 +++++++++++++++++++++++++++++++++----
drivers/usb/gadget/dummy_hcd.c | 4 +-
drivers/usb/host/ehci-hub.c | 4 +-
drivers/usb/host/imx21-hcd.c | 4 +-
drivers/usb/host/isp116x-hcd.c | 4 +-
drivers/usb/host/isp1362-hcd.c | 4 +-
drivers/usb/host/isp1760-hcd.c | 4 +-
drivers/usb/host/ohci-hub.c | 11 +++---
drivers/usb/host/oxu210hp-hcd.c | 4 +-
drivers/usb/host/r8a66597-hcd.c | 5 ++-
drivers/usb/host/sl811-hcd.c | 4 +-
drivers/usb/host/u132-hcd.c | 11 +++---
drivers/usb/host/xhci-hub.c | 4 +-
drivers/usb/musb/musb_virthub.c | 4 +-
drivers/usb/wusbcore/rh.c | 4 +-
include/linux/usb/ch11.h | 41 +++++++++++++++++++--
17 files changed, 141 insertions(+), 48 deletions(-)

diff --git a/drivers/staging/usbip/vhci_hcd.c b/drivers/staging/usbip/vhci_hcd.c
index 9e24ec3..e5cbc4d 100644
--- a/drivers/staging/usbip/vhci_hcd.c
+++ b/drivers/staging/usbip/vhci_hcd.c
@@ -257,8 +257,8 @@ static inline void hub_descriptor(struct usb_hub_descriptor *desc)
desc->wHubCharacteristics = (__force __u16)
(__constant_cpu_to_le16(0x0001));
desc->bNbrPorts = VHCI_NPORTS;
- desc->DeviceRemovable[0] = 0xff;
- desc->DeviceRemovable[1] = 0xff;
+ desc->u.hs.DeviceRemovable[0] = 0xff;
+ desc->u.hs.DeviceRemovable[1] = 0xff;
}

static int vhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 4310cc4..0015638 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -82,6 +82,10 @@ struct usb_hub {
void **port_owners;
};

+static inline int hub_is_superspeed(struct usb_device *hdev)
+{
+ return (hdev->descriptor.bDeviceProtocol == 3);
+}

/* Protect struct usb_device->state and ->children members
* Note: Both are also protected by ->dev.sem, except that ->state can
@@ -172,14 +176,23 @@ static struct usb_hub *hdev_to_hub(struct usb_device *hdev)
}

/* USB 2.0 spec Section 11.24.4.5 */
-static int get_hub_descriptor(struct usb_device *hdev, void *data, int size)
+static int get_hub_descriptor(struct usb_device *hdev, void *data)
{
- int i, ret;
+ int i, ret, size;
+ unsigned dtype;
+
+ if (hub_is_superspeed(hdev)) {
+ dtype = USB_DT_SS_HUB;
+ size = USB_DT_SS_HUB_SIZE;
+ } else {
+ dtype = USB_DT_HUB;
+ size = sizeof(struct usb_hub_descriptor);
+ }

for (i = 0; i < 3; i++) {
ret = usb_control_msg(hdev, usb_rcvctrlpipe(hdev, 0),
USB_REQ_GET_DESCRIPTOR, USB_DIR_IN | USB_RT_HUB,
- USB_DT_HUB << 8, 0, data, size,
+ dtype << 8, 0, data, size,
USB_CTRL_GET_TIMEOUT);
if (ret >= (USB_DT_HUB_NONVAR_SIZE + 2))
return ret;
@@ -365,6 +378,19 @@ static int hub_port_status(struct usb_hub *hub, int port1,
} else {
*status = le16_to_cpu(hub->status->port.wPortStatus);
*change = le16_to_cpu(hub->status->port.wPortChange);
+
+ if ((hub->hdev->parent != NULL) &&
+ hub_is_superspeed(hub->hdev)) {
+ /* Translate the USB 3 port status */
+ u16 tmp = *status & USB_SS_PORT_STAT_MASK;
+ if (*status & USB_SS_PORT_STAT_POWER)
+ tmp |= USB_PORT_STAT_POWER;
+ if ((*status & USB_SS_PORT_STAT_SPEED) ==
+ USB_PORT_STAT_SPEED_5GBPS)
+ tmp |= USB_PORT_STAT_SUPER_SPEED;
+ *status = tmp;
+ }
+
ret = 0;
}
mutex_unlock(&hub->status_mutex);
@@ -607,7 +633,7 @@ static int hub_port_disable(struct usb_hub *hub, int port1, int set_state)
if (hdev->children[port1-1] && set_state)
usb_set_device_state(hdev->children[port1-1],
USB_STATE_NOTATTACHED);
- if (!hub->error)
+ if (!hub->error && !hub_is_superspeed(hub->hdev))
ret = clear_port_feature(hdev, port1, USB_PORT_FEAT_ENABLE);
if (ret)
dev_err(hub->intfdev, "cannot disable port %d (err = %d)\n",
@@ -795,6 +821,11 @@ static void hub_activate(struct usb_hub *hub, enum hub_activation_type type)
clear_port_feature(hub->hdev, port1,
USB_PORT_FEAT_C_ENABLE);
}
+ if (portchange & USB_PORT_STAT_C_LINK_STATE) {
+ need_debounce_delay = true;
+ clear_port_feature(hub->hdev, port1,
+ USB_PORT_FEAT_C_PORT_LINK_STATE);
+ }

/* We can forget about a "removed" device when there's a
* physical disconnect or the connect status changes.
@@ -964,12 +995,23 @@ static int hub_configure(struct usb_hub *hub,
goto fail;
}

+ if (hub_is_superspeed(hdev) && (hdev->parent != NULL)) {
+ ret = usb_control_msg(hdev, usb_sndctrlpipe(hdev, 0),
+ HUB_SET_DEPTH, USB_RT_HUB,
+ hdev->level - 1, 0, NULL, 0,
+ USB_CTRL_SET_TIMEOUT);
+
+ if (ret < 0) {
+ message = "can't set hub depth";
+ goto fail;
+ }
+ }
+
/* Request the entire hub descriptor.
* hub->descriptor can handle USB_MAXCHILDREN ports,
* but the hub can/will return fewer bytes here.
*/
- ret = get_hub_descriptor(hdev, hub->descriptor,
- sizeof(*hub->descriptor));
+ ret = get_hub_descriptor(hdev, hub->descriptor);
if (ret < 0) {
message = "can't read hub descriptor";
goto fail;
@@ -991,12 +1033,14 @@ static int hub_configure(struct usb_hub *hub,

wHubCharacteristics = le16_to_cpu(hub->descriptor->wHubCharacteristics);

- if (wHubCharacteristics & HUB_CHAR_COMPOUND) {
+ /* FIXME for USB 3.0, skip for now */
+ if ((wHubCharacteristics & HUB_CHAR_COMPOUND) &&
+ !(hub_is_superspeed(hdev))) {
int i;
char portstr [USB_MAXCHILDREN + 1];

for (i = 0; i < hdev->maxchild; i++)
- portstr[i] = hub->descriptor->DeviceRemovable
+ portstr[i] = hub->descriptor->u.hs.DeviceRemovable
[((i + 1) / 8)] & (1 << ((i + 1) % 8))
? 'F' : 'R';
portstr[hdev->maxchild] = 0;
@@ -2021,6 +2065,8 @@ static int hub_port_wait_reset(struct usb_hub *hub, int port1,
udev->speed = USB_SPEED_HIGH;
else if (portstatus & USB_PORT_STAT_LOW_SPEED)
udev->speed = USB_SPEED_LOW;
+ else if (portstatus & USB_PORT_STAT_SUPER_SPEED)
+ udev->speed = USB_SPEED_SUPER;
else
udev->speed = USB_SPEED_FULL;
return 0;
@@ -3422,6 +3468,17 @@ static void hub_events(void)
clear_port_feature(hdev, i,
USB_PORT_FEAT_C_RESET);
}
+ if (portchange & USB_PORT_STAT_C_LINK_STATE) {
+ clear_port_feature(hub->hdev, i,
+ USB_PORT_FEAT_C_PORT_LINK_STATE);
+ }
+ if (portchange & USB_PORT_STAT_C_CONFIG_ERROR) {
+ dev_warn(hub_dev,
+ "config error on port %d\n",
+ i);
+ clear_port_feature(hub->hdev, i,
+ USB_PORT_FEAT_C_PORT_CONFIG_ERROR);
+ }

if (connect_change)
hub_port_connect_change(hub, i,
diff --git a/drivers/usb/gadget/dummy_hcd.c b/drivers/usb/gadget/dummy_hcd.c
index f2040e8..3214ca3 100644
--- a/drivers/usb/gadget/dummy_hcd.c
+++ b/drivers/usb/gadget/dummy_hcd.c
@@ -1593,8 +1593,8 @@ hub_descriptor (struct usb_hub_descriptor *desc)
desc->bDescLength = 9;
desc->wHubCharacteristics = cpu_to_le16(0x0001);
desc->bNbrPorts = 1;
- desc->DeviceRemovable[0] = 0xff;
- desc->DeviceRemovable[1] = 0xff;
+ desc->u.hs.DeviceRemovable[0] = 0xff;
+ desc->u.hs.DeviceRemovable[1] = 0xff;
}

static int dummy_hub_control (
diff --git a/drivers/usb/host/ehci-hub.c b/drivers/usb/host/ehci-hub.c
index 13df433..d2b14fe 100644
--- a/drivers/usb/host/ehci-hub.c
+++ b/drivers/usb/host/ehci-hub.c
@@ -688,8 +688,8 @@ ehci_hub_descriptor (
desc->bDescLength = 7 + 2 * temp;

/* two bitmaps: ports removable, and usb 1.0 legacy PortPwrCtrlMask */
- memset(&desc->DeviceRemovable[0], 0, temp);
- memset(&desc->DeviceRemovable[temp], 0xff, temp);
+ memset(&desc->u.hs.DeviceRemovable[0], 0, temp);
+ memset(&desc->u.hs.DeviceRemovable[temp], 0xff, temp);

temp = 0x0008; /* per-port overcurrent reporting */
if (HCS_PPC (ehci->hcs_params))
diff --git a/drivers/usb/host/imx21-hcd.c b/drivers/usb/host/imx21-hcd.c
index c58ce2d..2673c8b 100644
--- a/drivers/usb/host/imx21-hcd.c
+++ b/drivers/usb/host/imx21-hcd.c
@@ -1471,8 +1471,8 @@ static int get_hub_descriptor(struct usb_hcd *hcd,
0x0010 | /* No over current protection */
0);

- desc->DeviceRemovable[0] = 1 << 1;
- desc->DeviceRemovable[1] = ~0;
+ desc->u.hs.DeviceRemovable[0] = 1 << 1;
+ desc->u.hs.DeviceRemovable[1] = ~0;
return 0;
}

diff --git a/drivers/usb/host/isp116x-hcd.c b/drivers/usb/host/isp116x-hcd.c
index 2a60a50..c0e22f2 100644
--- a/drivers/usb/host/isp116x-hcd.c
+++ b/drivers/usb/host/isp116x-hcd.c
@@ -952,8 +952,8 @@ static void isp116x_hub_descriptor(struct isp116x *isp116x,
desc->wHubCharacteristics = cpu_to_le16((u16) ((reg >> 8) & 0x1f));
desc->bPwrOn2PwrGood = (u8) ((reg >> 24) & 0xff);
/* ports removable, and legacy PortPwrCtrlMask */
- desc->DeviceRemovable[0] = 0;
- desc->DeviceRemovable[1] = ~0;
+ desc->u.hs.DeviceRemovable[0] = 0;
+ desc->u.hs.DeviceRemovable[1] = ~0;
}

/* Perform reset of a given port.
diff --git a/drivers/usb/host/isp1362-hcd.c b/drivers/usb/host/isp1362-hcd.c
index 99517ce..1f7ab77 100644
--- a/drivers/usb/host/isp1362-hcd.c
+++ b/drivers/usb/host/isp1362-hcd.c
@@ -1556,8 +1556,8 @@ static void isp1362_hub_descriptor(struct isp1362_hcd *isp1362_hcd,
DBG(0, "%s: hubcharacteristics = %02x\n", __func__, cpu_to_le16((reg >> 8) & 0x1f));
desc->bPwrOn2PwrGood = (reg >> 24) & 0xff;
/* ports removable, and legacy PortPwrCtrlMask */
- desc->DeviceRemovable[0] = desc->bNbrPorts == 1 ? 1 << 1 : 3 << 1;
- desc->DeviceRemovable[1] = ~0;
+ desc->u.hs.DeviceRemovable[0] = desc->bNbrPorts == 1 ? 1 << 1 : 3 << 1;
+ desc->u.hs.DeviceRemovable[1] = ~0;

DBG(3, "%s: exit\n", __func__);
}
diff --git a/drivers/usb/host/isp1760-hcd.c b/drivers/usb/host/isp1760-hcd.c
index 5e39442..7faf677 100644
--- a/drivers/usb/host/isp1760-hcd.c
+++ b/drivers/usb/host/isp1760-hcd.c
@@ -1845,8 +1845,8 @@ static void isp1760_hub_descriptor(struct isp1760_hcd *priv,
desc->bDescLength = 7 + 2 * temp;

/* ports removable, and usb 1.0 legacy PortPwrCtrlMask */
- memset(&desc->DeviceRemovable[0], 0, temp);
- memset(&desc->DeviceRemovable[temp], 0xff, temp);
+ memset(&desc->u.hs.DeviceRemovable[0], 0, temp);
+ memset(&desc->u.hs.DeviceRemovable[temp], 0xff, temp);

/* per-port overcurrent reporting */
temp = 0x0008;
diff --git a/drivers/usb/host/ohci-hub.c b/drivers/usb/host/ohci-hub.c
index cd4b74f..9154615 100644
--- a/drivers/usb/host/ohci-hub.c
+++ b/drivers/usb/host/ohci-hub.c
@@ -582,13 +582,14 @@ ohci_hub_descriptor (

/* ports removable, and usb 1.0 legacy PortPwrCtrlMask */
rh = roothub_b (ohci);
- memset(desc->DeviceRemovable, 0xff, sizeof(desc->DeviceRemovable));
- desc->DeviceRemovable[0] = rh & RH_B_DR;
+ memset(desc->u.hs.DeviceRemovable, 0xff,
+ sizeof(desc->u.hs.DeviceRemovable));
+ desc->u.hs.DeviceRemovable[0] = rh & RH_B_DR;
if (ohci->num_ports > 7) {
- desc->DeviceRemovable[1] = (rh & RH_B_DR) >> 8;
- desc->DeviceRemovable[2] = 0xff;
+ desc->u.hs.DeviceRemovable[1] = (rh & RH_B_DR) >> 8;
+ desc->u.hs.DeviceRemovable[2] = 0xff;
} else
- desc->DeviceRemovable[1] = 0xff;
+ desc->u.hs.DeviceRemovable[1] = 0xff;
}

/*-------------------------------------------------------------------------*/
diff --git a/drivers/usb/host/oxu210hp-hcd.c b/drivers/usb/host/oxu210hp-hcd.c
index a88bbf4..ec277f2 100644
--- a/drivers/usb/host/oxu210hp-hcd.c
+++ b/drivers/usb/host/oxu210hp-hcd.c
@@ -452,8 +452,8 @@ static void ehci_hub_descriptor(struct oxu_hcd *oxu,
desc->bDescLength = 7 + 2 * temp;

/* ports removable, and usb 1.0 legacy PortPwrCtrlMask */
- memset(&desc->DeviceRemovable[0], 0, temp);
- memset(&desc->DeviceRemovable[temp], 0xff, temp);
+ memset(&desc->u.hs.DeviceRemovable[0], 0, temp);
+ memset(&desc->u.hs.DeviceRemovable[temp], 0xff, temp);

temp = 0x0008; /* per-port overcurrent reporting */
if (HCS_PPC(oxu->hcs_params))
diff --git a/drivers/usb/host/r8a66597-hcd.c b/drivers/usb/host/r8a66597-hcd.c
index 98afe75..db6f8b9 100644
--- a/drivers/usb/host/r8a66597-hcd.c
+++ b/drivers/usb/host/r8a66597-hcd.c
@@ -2150,8 +2150,9 @@ static void r8a66597_hub_descriptor(struct r8a66597 *r8a66597,
desc->bDescLength = 9;
desc->bPwrOn2PwrGood = 0;
desc->wHubCharacteristics = cpu_to_le16(0x0011);
- desc->DeviceRemovable[0] = ((1 << r8a66597->max_root_hub) - 1) << 1;
- desc->DeviceRemovable[1] = ~0;
+ desc->u.hs.DeviceRemovable[0] =
+ ((1 << r8a66597->max_root_hub) - 1) << 1;
+ desc->u.hs.DeviceRemovable[1] = ~0;
}

static int r8a66597_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
diff --git a/drivers/usb/host/sl811-hcd.c b/drivers/usb/host/sl811-hcd.c
index c73fdae..fa66830 100644
--- a/drivers/usb/host/sl811-hcd.c
+++ b/drivers/usb/host/sl811-hcd.c
@@ -1111,8 +1111,8 @@ sl811h_hub_descriptor (
desc->wHubCharacteristics = cpu_to_le16(temp);

/* ports removable, and legacy PortPwrCtrlMask */
- desc->DeviceRemovable[0] = 0 << 1;
- desc->DeviceRemovable[1] = ~0;
+ desc->u.hs.DeviceRemovable[0] = 0 << 1;
+ desc->u.hs.DeviceRemovable[1] = ~0;
}

static void
diff --git a/drivers/usb/host/u132-hcd.c b/drivers/usb/host/u132-hcd.c
index a659e15..b478593 100644
--- a/drivers/usb/host/u132-hcd.c
+++ b/drivers/usb/host/u132-hcd.c
@@ -2604,13 +2604,14 @@ static int u132_roothub_descriptor(struct u132 *u132,
retval = u132_read_pcimem(u132, roothub.b, &rh_b);
if (retval)
return retval;
- memset(desc->DeviceRemovable, 0xff, sizeof(desc->DeviceRemovable));
- desc->DeviceRemovable[0] = rh_b & RH_B_DR;
+ memset(desc->u.hs.DeviceRemovable, 0xff,
+ sizeof(desc->u.hs.DeviceRemovable));
+ desc->u.hs.DeviceRemovable[0] = rh_b & RH_B_DR;
if (u132->num_ports > 7) {
- desc->DeviceRemovable[1] = (rh_b & RH_B_DR) >> 8;
- desc->DeviceRemovable[2] = 0xff;
+ desc->u.hs.DeviceRemovable[1] = (rh_b & RH_B_DR) >> 8;
+ desc->u.hs.DeviceRemovable[2] = 0xff;
} else
- desc->DeviceRemovable[1] = 0xff;
+ desc->u.hs.DeviceRemovable[1] = 0xff;
return 0;
}

diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 0261198..2d8119b 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -45,8 +45,8 @@ static void xhci_hub_descriptor(struct xhci_hcd *xhci,
temp = 1 + (ports / 8);
desc->bDescLength = 7 + 2 * temp;

- memset(&desc->DeviceRemovable[0], 0, temp);
- memset(&desc->DeviceRemovable[temp], 0xff, temp);
+ memset(&desc->u.hs.DeviceRemovable[0], 0, temp);
+ memset(&desc->u.hs.DeviceRemovable[temp], 0xff, temp);

/* Ugh, these should be #defines, FIXME */
/* Using table 11-13 in USB 2.0 spec. */
diff --git a/drivers/usb/musb/musb_virthub.c b/drivers/usb/musb/musb_virthub.c
index b46d187..489104a 100644
--- a/drivers/usb/musb/musb_virthub.c
+++ b/drivers/usb/musb/musb_virthub.c
@@ -305,8 +305,8 @@ int musb_hub_control(
desc->bHubContrCurrent = 0;

/* workaround bogus struct definition */
- desc->DeviceRemovable[0] = 0x02; /* port 1 */
- desc->DeviceRemovable[1] = 0xff;
+ desc->u.hs.DeviceRemovable[0] = 0x02; /* port 1 */
+ desc->u.hs.DeviceRemovable[1] = 0xff;
}
break;
case GetHubStatus:
diff --git a/drivers/usb/wusbcore/rh.c b/drivers/usb/wusbcore/rh.c
index b995aa6..a8f5a27 100644
--- a/drivers/usb/wusbcore/rh.c
+++ b/drivers/usb/wusbcore/rh.c
@@ -184,8 +184,8 @@ static int wusbhc_rh_get_hub_descr(struct wusbhc *wusbhc, u16 wValue,
descr->bPwrOn2PwrGood = 0;
descr->bHubContrCurrent = 0;
/* two bitmaps: ports removable, and usb 1.0 legacy PortPwrCtrlMask */
- memset(&descr->DeviceRemovable[0], 0, temp);
- memset(&descr->DeviceRemovable[temp], 0xff, temp);
+ memset(&descr->u.hs.DeviceRemovable[0], 0, temp);
+ memset(&descr->u.hs.DeviceRemovable[temp], 0xff, temp);
return 0;
}

diff --git a/include/linux/usb/ch11.h b/include/linux/usb/ch11.h
index 10ec069..8618ee6 100644
--- a/include/linux/usb/ch11.h
+++ b/include/linux/usb/ch11.h
@@ -26,6 +26,7 @@
#define HUB_RESET_TT 9
#define HUB_GET_TT_STATE 10
#define HUB_STOP_TT 11
+#define HUB_SET_DEPTH 12

/*
* Hub class additional requests defined by USB 3.0 spec
@@ -61,6 +62,12 @@
#define USB_PORT_FEAT_TEST 21
#define USB_PORT_FEAT_INDICATOR 22
#define USB_PORT_FEAT_C_PORT_L1 23
+#define USB_PORT_FEAT_C_PORT_LINK_STATE 25
+#define USB_PORT_FEAT_C_PORT_CONFIG_ERROR 26
+#define USB_PORT_FEAT_PORT_REMOTE_WAKE_MASK 27
+#define USB_PORT_FEAT_BH_PORT_RESET 28
+#define USB_PORT_FEAT_C_BH_PORT_RESET 29
+#define USB_PORT_FEAT_FORCE_LINKPM_ACCEPT 30

/*
* Port feature selectors added by USB 3.0 spec.
@@ -110,8 +117,14 @@ struct usb_port_status {
*/
#define USB_PORT_STAT_LINK_STATE 0x01e0
#define USB_SS_PORT_STAT_POWER 0x0200
+#define USB_SS_PORT_STAT_SPEED 0x1c00
#define USB_PORT_STAT_SPEED_5GBPS 0x0000
/* Valid only if port is enabled */
+/* Bits that are the same from USB 2.0 */
+#define USB_SS_PORT_STAT_MASK (USB_PORT_STAT_CONNECTION | \
+ USB_PORT_STAT_ENABLE | \
+ USB_PORT_STAT_OVERCURRENT | \
+ USB_PORT_STAT_RESET)

/*
* Definitions for PORT_LINK_STATE values
@@ -141,6 +154,13 @@ struct usb_port_status {
#define USB_PORT_STAT_C_OVERCURRENT 0x0008
#define USB_PORT_STAT_C_RESET 0x0010
#define USB_PORT_STAT_C_L1 0x0020
+/*
+ * USB 3.0 wPortChange bit fields
+ * See USB 3.0 spec Table 10-11
+ */
+#define USB_PORT_STAT_C_BH_RESET 0x0020
+#define USB_PORT_STAT_C_LINK_STATE 0x0040
+#define USB_PORT_STAT_C_CONFIG_ERROR 0x0080

/*
* wHubCharacteristics (masks)
@@ -175,7 +195,9 @@ struct usb_hub_status {
*/

#define USB_DT_HUB (USB_TYPE_CLASS | 0x09)
+#define USB_DT_SS_HUB (USB_TYPE_CLASS | 0x0a)
#define USB_DT_HUB_NONVAR_SIZE 7
+#define USB_DT_SS_HUB_SIZE 12

struct usb_hub_descriptor {
__u8 bDescLength;
@@ -184,11 +206,22 @@ struct usb_hub_descriptor {
__le16 wHubCharacteristics;
__u8 bPwrOn2PwrGood;
__u8 bHubContrCurrent;
- /* add 1 bit for hub status change; round to bytes */
- __u8 DeviceRemovable[(USB_MAXCHILDREN + 1 + 7) / 8];
- __u8 PortPwrCtrlMask[(USB_MAXCHILDREN + 1 + 7) / 8];
-} __attribute__ ((packed));

+ /* 2.0 and 3.0 hubs differ here */
+ union {
+ struct {
+ /* add 1 bit for hub status change; round to bytes */
+ __u8 DeviceRemovable[(USB_MAXCHILDREN + 1 + 7) / 8];
+ __u8 PortPwrCtrlMask[(USB_MAXCHILDREN + 1 + 7) / 8];
+ } __attribute__ ((packed)) hs;
+
+ struct {
+ __u8 bHubHdrDecLat;
+ __u16 wHubDelay;
+ __u16 DeviceRemovable;
+ } __attribute__ ((packed)) ss;
+ } u;
+} __attribute__ ((packed));

/* port indicator status selectors, tables 11-7 and 11-25 */
#define HUB_LED_AUTO 0
--
1.6.3.3

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Sarah Sharp
2010-12-30 23:23:07 UTC
Permalink
In USB 3.0, there are two types of resets: a "hot" port reset and a "warm"
port reset. The hot port reset is always tried first, and involves
sending the reset signaling for a shorter amount of time. But sometimes
devices don't respond to the hot reset, and a "Bigger Hammer" is needed.

External hubs and roothubs will automatically try a warm reset when the
hot reset fails, and they will set a status change bit to indicate when
there is a "BH reset" change. Make sure the USB core clears that port
status change bit, or we'll get lots of status change notifications on the
interrupt endpoint of the USB 3.0 hub.

(Side note: you may be confused why the USB 3.0 spec calls the same type
of reset "warm reset" in some places and "BH reset" in other places. "BH"
reset is supposed to stand for "Big Hammer" reset, but it also stands for
"Brad Hosler". Brad died shortly after the USB 3.0 bus specification was
started, and they decided to name the reset after him. The suggestion was
made shortly before the spec was finalized, so the wording is a bit
inconsistent.)

Signed-off-by: Sarah Sharp <sarah.a.sharp-VuQAYsv1563Yd54FQh9/***@public.gmane.org>
---
drivers/usb/core/hub.c | 8 ++++++++
1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 0015638..a9c9986 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -3468,6 +3468,14 @@ static void hub_events(void)
clear_port_feature(hdev, i,
USB_PORT_FEAT_C_RESET);
}
+ if ((portchange & USB_PORT_STAT_C_BH_RESET) &&
+ hub_is_superspeed(hub->hdev)) {
+ dev_dbg(hub_dev,
+ "warm reset change on port %d\n",
+ i);
+ clear_port_feature(hdev, i,
+ USB_PORT_FEAT_C_BH_PORT_RESET);
+ }
if (portchange & USB_PORT_STAT_C_LINK_STATE) {
clear_port_feature(hub->hdev, i,
USB_PORT_FEAT_C_PORT_LINK_STATE);
--
1.6.3.3

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Sarah Sharp
2010-12-30 23:23:13 UTC
Permalink
Make the USB 3.0 roothub registered by the USB core have a SuperSpeed
Endpoint Companion Descriptor after the interrupt endpoint. All USB 3.0
devices are required to have this, and the USB 3.0 bus specification
(section 10.13.1) says which values the descriptor should have.

Signed-off-by: Sarah Sharp <sarah.a.sharp-VuQAYsv1563Yd54FQh9/***@public.gmane.org>
---
drivers/usb/core/hcd.c | 15 +++++++++------
1 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 7bf32c5..65989e5 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -297,7 +297,7 @@ static const u8 ss_rh_config_descriptor[] = {
/* one configuration */
0x09, /* __u8 bLength; */
0x02, /* __u8 bDescriptorType; Configuration */
- 0x19, 0x00, /* __le16 wTotalLength; FIXME */
+ 0x1f, 0x00, /* __le16 wTotalLength; */
0x01, /* __u8 bNumInterfaces; (1) */
0x01, /* __u8 bConfigurationValue; */
0x00, /* __u8 iConfiguration; */
@@ -327,11 +327,14 @@ static const u8 ss_rh_config_descriptor[] = {
/* __le16 ep_wMaxPacketSize; 1 + (MAX_ROOT_PORTS / 8)
* see hub.c:hub_configure() for details. */
(USB_MAXCHILDREN + 1 + 7) / 8, 0x00,
- 0x0c /* __u8 ep_bInterval; (256ms -- usb 2.0 spec) */
- /*
- * All 3.0 hubs should have an endpoint companion descriptor,
- * but we're ignoring that for now. FIXME?
- */
+ 0x0c, /* __u8 ep_bInterval; (256ms -- usb 2.0 spec) */
+
+ /* one SuperSpeed endpoint companion descriptor */
+ 0x06, /* __u8 ss_bLength */
+ 0x30, /* __u8 ss_bDescriptorType; SuperSpeed EP Companion */
+ 0x00, /* __u8 ss_bMaxBurst; allows 1 TX between ACKs */
+ 0x00, /* __u8 ss_bmAttributes; 1 packet per service interval */
+ 0x02, 0x00 /* __le16 ss_wBytesPerInterval; 15 bits for max 15 ports */
};

/*-------------------------------------------------------------------------*/
--
1.6.3.3

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Sarah Sharp
2010-12-30 23:23:22 UTC
Permalink
Make sure to call into the USB core's link, unlink, and giveback URB
functions with the usb_hcd pointer found by using urb->dev->bus. This
will avoid confusion later, when the xHCI driver will deal with URBs from
two separate buses (the USB 3.0 roothub and the faked USB 2.0 roothub).

Assume xhci_urb_dequeue() will be called with the proper usb_hcd.

Signed-off-by: Sarah Sharp <sarah.a.sharp-VuQAYsv1563Yd54FQh9/***@public.gmane.org>
---
drivers/usb/host/xhci-ring.c | 9 +++++----
drivers/usb/host/xhci.c | 2 +-
2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 6abe2e9..0a58be8 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -599,13 +599,14 @@ static inline void xhci_stop_watchdog_timer_in_irq(struct xhci_hcd *xhci,
static void xhci_giveback_urb_in_irq(struct xhci_hcd *xhci,
struct xhci_td *cur_td, int status, char *adjective)
{
- struct usb_hcd *hcd = xhci_to_hcd(xhci);
+ struct usb_hcd *hcd;
struct urb *urb;
struct urb_priv *urb_priv;

urb = cur_td->urb;
urb_priv = urb->hcpriv;
urb_priv->td_cnt++;
+ hcd = bus_to_hcd(urb->dev->bus);

/* Only giveback urb when this is the last td in urb */
if (urb_priv->td_cnt == urb_priv->length) {
@@ -1990,12 +1991,12 @@ cleanup:
trb_comp_code != COMP_BABBLE))
xhci_urb_free_priv(xhci, urb_priv);

- usb_hcd_unlink_urb_from_ep(xhci_to_hcd(xhci), urb);
+ usb_hcd_unlink_urb_from_ep(bus_to_hcd(urb->dev->bus), urb);
xhci_dbg(xhci, "Giveback URB %p, len = %d, "
"status = %d\n",
urb, urb->actual_length, status);
spin_unlock(&xhci->lock);
- usb_hcd_giveback_urb(xhci_to_hcd(xhci), urb, status);
+ usb_hcd_giveback_urb(bus_to_hcd(urb->dev->bus), urb, status);
spin_lock(&xhci->lock);
}

@@ -2332,7 +2333,7 @@ static int prepare_transfer(struct xhci_hcd *xhci,
INIT_LIST_HEAD(&td->cancelled_td_list);

if (td_index == 0) {
- ret = usb_hcd_link_urb_to_ep(xhci_to_hcd(xhci), urb);
+ ret = usb_hcd_link_urb_to_ep(bus_to_hcd(urb->dev->bus), urb);
if (unlikely(ret)) {
xhci_urb_free_priv(xhci, urb_priv);
urb->hcpriv = NULL;
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 64f82b9..dfc0099 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -1163,7 +1163,7 @@ int xhci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)

usb_hcd_unlink_urb_from_ep(hcd, urb);
spin_unlock_irqrestore(&xhci->lock, flags);
- usb_hcd_giveback_urb(xhci_to_hcd(xhci), urb, -ESHUTDOWN);
+ usb_hcd_giveback_urb(hcd, urb, -ESHUTDOWN);
xhci_urb_free_priv(xhci, urb_priv);
return ret;
}
--
1.6.3.3

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Sergei Shtylyov
2011-01-06 12:39:42 UTC
Permalink
Hello.
Post by Sarah Sharp
Make sure to call into the USB core's link, unlink, and giveback URB
functions with the usb_hcd pointer found by using urb->dev->bus. This
will avoid confusion later, when the xHCI driver will deal with URBs from
two separate buses (the USB 3.0 roothub and the faked USB 2.0 roothub).
Assume xhci_urb_dequeue() will be called with the proper usb_hcd.
---
drivers/usb/host/xhci-ring.c | 9 +++++----
drivers/usb/host/xhci.c | 2 +-
2 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 6abe2e9..0a58be8 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -599,13 +599,14 @@ static inline void xhci_stop_watchdog_timer_in_irq(struct xhci_hcd *xhci,
static void xhci_giveback_urb_in_irq(struct xhci_hcd *xhci,
struct xhci_td *cur_td, int status, char *adjective)
{
- struct usb_hcd *hcd = xhci_to_hcd(xhci);
+ struct usb_hcd *hcd;
Perhaps time to align with below variables?
Post by Sarah Sharp
struct urb *urb;
struct urb_priv *urb_priv;
WBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Sarah Sharp
2011-01-06 16:33:44 UTC
Permalink
Post by Sergei Shtylyov
Hello.
Post by Sarah Sharp
Make sure to call into the USB core's link, unlink, and giveback URB
functions with the usb_hcd pointer found by using urb->dev->bus. This
will avoid confusion later, when the xHCI driver will deal with URBs from
two separate buses (the USB 3.0 roothub and the faked USB 2.0 roothub).
Assume xhci_urb_dequeue() will be called with the proper usb_hcd.
---
drivers/usb/host/xhci-ring.c | 9 +++++----
drivers/usb/host/xhci.c | 2 +-
2 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 6abe2e9..0a58be8 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -599,13 +599,14 @@ static inline void xhci_stop_watchdog_timer_in_irq(struct xhci_hcd *xhci,
static void xhci_giveback_urb_in_irq(struct xhci_hcd *xhci,
struct xhci_td *cur_td, int status, char *adjective)
{
- struct usb_hcd *hcd = xhci_to_hcd(xhci);
+ struct usb_hcd *hcd;
Perhaps time to align with below variables?
I never align variables with tabs. I don't like the fact that if you
introduce a new variable with a longer type, you have to re-align all
the other variable declarations. It produces noise in the diff stats
that make it hard to pick out what really changed. CodingStyle says
nothing about aligning variables, so why bring it up when the style of
the driver isn't to align them?

Sarah Sharp
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Sergei Shtylyov
2011-01-06 22:00:36 UTC
Permalink
Hello.
Post by Sarah Sharp
Post by Sergei Shtylyov
Post by Sarah Sharp
Make sure to call into the USB core's link, unlink, and giveback URB
functions with the usb_hcd pointer found by using urb->dev->bus. This
will avoid confusion later, when the xHCI driver will deal with URBs from
two separate buses (the USB 3.0 roothub and the faked USB 2.0 roothub).
Assume xhci_urb_dequeue() will be called with the proper usb_hcd.
[...]
Post by Sarah Sharp
Post by Sergei Shtylyov
Post by Sarah Sharp
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 6abe2e9..0a58be8 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -599,13 +599,14 @@ static inline void xhci_stop_watchdog_timer_in_irq(struct xhci_hcd *xhci,
static void xhci_giveback_urb_in_irq(struct xhci_hcd *xhci,
struct xhci_td *cur_td, int status, char *adjective)
{
- struct usb_hcd *hcd = xhci_to_hcd(xhci);
+ struct usb_hcd *hcd;
Perhaps time to align with below variables?
I never align variables with tabs.
Hm, I thought xHCI driver is your code...
Post by Sarah Sharp
I don't like the fact that if you
introduce a new variable with a longer type, you have to re-align all
the other variable declarations. It produces noise in the diff stats
that make it hard to pick out what really changed.
I'd agree with you.
Post by Sarah Sharp
CodingStyle says
nothing about aligning variables, so why bring it up when the style of
the driver isn't to align them?
I have no idea about the style of the driver overall; I could only judge
by the style used in this very function.
Post by Sarah Sharp
Sarah Sharp
WBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Sarah Sharp
2010-12-30 23:23:28 UTC
Permalink
Instead of allocating space for the whole xhci_hcd structure at the end of
usb_hcd, make the USB core allocate enough space for a pointer to the
xhci_hcd structure. This will make it easy to share the xhci_hcd
structure across the two roothubs (the USB 3.0 usb_hcd and the USB 2.0
usb_hcd).

Deallocate the xhci_hcd at PCI remove time, so the hcd_priv will be
deallocated after the usb_hcd is deallocated. We do this by registering a
different PCI remove function that calls the usb_hcd_pci_remove()
function, and then frees the xhci_hcd. usb_hcd_pci_remove() calls
kput() on the usb_hcd structure, which will deallocate the memory that
contains the hcd_priv pointer, but not the memory it points to.

Signed-off-by: Sarah Sharp <sarah.a.sharp-VuQAYsv1563Yd54FQh9/***@public.gmane.org>
---
drivers/usb/host/xhci-pci.c | 33 +++++++++++++++++++++++++++------
drivers/usb/host/xhci.h | 5 +++--
2 files changed, 30 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index bb668a8..0090828 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -57,6 +57,12 @@ static int xhci_pci_setup(struct usb_hcd *hcd)

hcd->self.sg_tablesize = TRBS_PER_SEGMENT - 2;

+ xhci = kzalloc(sizeof(struct xhci_hcd), GFP_KERNEL);
+ if (!xhci)
+ return -ENOMEM;
+ *((struct xhci_hcd **) hcd->hcd_priv) = xhci;
+ xhci->main_hcd = hcd;
+
xhci->cap_regs = hcd->regs;
xhci->op_regs = hcd->regs +
HC_LENGTH(xhci_readl(xhci, &xhci->cap_regs->hc_capbase));
@@ -85,13 +91,13 @@ static int xhci_pci_setup(struct usb_hcd *hcd)
/* Make sure the HC is halted. */
retval = xhci_halt(xhci);
if (retval)
- return retval;
+ goto error;

xhci_dbg(xhci, "Resetting HCD\n");
/* Reset the internal HC memory state and registers. */
retval = xhci_reset(xhci);
if (retval)
- return retval;
+ goto error;
xhci_dbg(xhci, "Reset complete\n");

temp = xhci_readl(xhci, &xhci->cap_regs->hcc_params);
@@ -106,14 +112,29 @@ static int xhci_pci_setup(struct usb_hcd *hcd)
/* Initialize HCD and host controller data structures. */
retval = xhci_init(hcd);
if (retval)
- return retval;
+ goto error;
xhci_dbg(xhci, "Called HCD init\n");

pci_read_config_byte(pdev, XHCI_SBRN_OFFSET, &xhci->sbrn);
xhci_dbg(xhci, "Got SBRN %u\n", (unsigned int) xhci->sbrn);

/* Find any debug ports */
- return xhci_pci_reinit(xhci, pdev);
+ retval = xhci_pci_reinit(xhci, pdev);
+ if (!retval)
+ return retval;
+
+error:
+ kfree(xhci);
+ return retval;
+}
+
+static void xhci_pci_remove(struct pci_dev *dev)
+{
+ struct xhci_hcd *xhci;
+
+ xhci = hcd_to_xhci(pci_get_drvdata(dev));
+ usb_hcd_pci_remove(dev);
+ kfree(xhci);
}

#ifdef CONFIG_PM
@@ -143,7 +164,7 @@ static int xhci_pci_resume(struct usb_hcd *hcd, bool hibernated)
static const struct hc_driver xhci_pci_hc_driver = {
.description = hcd_name,
.product_desc = "xHCI Host Controller",
- .hcd_priv_size = sizeof(struct xhci_hcd),
+ .hcd_priv_size = sizeof(struct xhci_hcd *),

/*
* generic hardware linkage
@@ -211,7 +232,7 @@ static struct pci_driver xhci_pci_driver = {
.id_table = pci_ids,

.probe = usb_hcd_pci_probe,
- .remove = usb_hcd_pci_remove,
+ .remove = xhci_pci_remove,
/* suspend and resume implemented later */

.shutdown = usb_hcd_pci_shutdown,
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 3cc4ebc..243d8d9 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1167,6 +1167,7 @@ struct s3_save {

/* There is one ehci_hci structure per controller */
struct xhci_hcd {
+ struct usb_hcd *main_hcd;
/* glue to PCI and HCD framework */
struct xhci_cap_regs __iomem *cap_regs;
struct xhci_op_regs __iomem *op_regs;
@@ -1270,12 +1271,12 @@ struct xhci_hcd {
/* convert between an HCD pointer and the corresponding EHCI_HCD */
static inline struct xhci_hcd *hcd_to_xhci(struct usb_hcd *hcd)
{
- return (struct xhci_hcd *) (hcd->hcd_priv);
+ return *((struct xhci_hcd **) (hcd->hcd_priv));
}

static inline struct usb_hcd *xhci_to_hcd(struct xhci_hcd *xhci)
{
- return container_of((void *) xhci, struct usb_hcd, hcd_priv);
+ return xhci->main_hcd;
}

#ifdef CONFIG_USB_XHCI_HCD_DEBUGGING
--
1.6.3.3

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Sarah Sharp
2010-12-30 23:23:33 UTC
Permalink
Make the labels for the goto statements in usb_hcd_pci_probe()
describe the cleanup they do, rather than being numbered err[1-4].
This makes it easier to add error handling later.

The error handling for this function looks a little fishy, since
set_hs_companion() isn't called until the very end of the function, and
clear_hs_companion() is called if this function fails earlier than that.
But it should be harmless to clear a NULL pointer, so leave the error
handling as-is.

Signed-off-by: Sarah Sharp <sarah.a.sharp-VuQAYsv1563Yd54FQh9/***@public.gmane.org>
---
drivers/usb/core/hcd-pci.c | 20 ++++++++++----------
1 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/core/hcd-pci.c b/drivers/usb/core/hcd-pci.c
index f71e8e3..5792bb4 100644
--- a/drivers/usb/core/hcd-pci.c
+++ b/drivers/usb/core/hcd-pci.c
@@ -192,13 +192,13 @@ int usb_hcd_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
"Found HC with no IRQ. Check BIOS/PCI %s setup!\n",
pci_name(dev));
retval = -ENODEV;
- goto err1;
+ goto disable_pci;
}

hcd = usb_create_hcd(driver, &dev->dev, pci_name(dev));
if (!hcd) {
retval = -ENOMEM;
- goto err1;
+ goto disable_pci;
}

if (driver->flags & HCD_MEMORY) {
@@ -209,13 +209,13 @@ int usb_hcd_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
driver->description)) {
dev_dbg(&dev->dev, "controller already in use\n");
retval = -EBUSY;
- goto err2;
+ goto clear_companion;
}
hcd->regs = ioremap_nocache(hcd->rsrc_start, hcd->rsrc_len);
if (hcd->regs == NULL) {
dev_dbg(&dev->dev, "error mapping memory\n");
retval = -EFAULT;
- goto err3;
+ goto release_mem_region;
}

} else {
@@ -236,7 +236,7 @@ int usb_hcd_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
if (region == PCI_ROM_RESOURCE) {
dev_dbg(&dev->dev, "no i/o regions available\n");
retval = -EBUSY;
- goto err2;
+ goto clear_companion;
}
}

@@ -244,24 +244,24 @@ int usb_hcd_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)

retval = usb_add_hcd(hcd, dev->irq, IRQF_DISABLED | IRQF_SHARED);
if (retval != 0)
- goto err4;
+ goto unmap_registers;
set_hs_companion(dev, hcd);

if (pci_dev_run_wake(dev))
pm_runtime_put_noidle(&dev->dev);
return retval;

- err4:
+unmap_registers:
if (driver->flags & HCD_MEMORY) {
iounmap(hcd->regs);
- err3:
+release_mem_region:
release_mem_region(hcd->rsrc_start, hcd->rsrc_len);
} else
release_region(hcd->rsrc_start, hcd->rsrc_len);
- err2:
+clear_companion:
clear_hs_companion(dev, hcd);
usb_put_hcd(hcd);
- err1:
+disable_pci:
pci_disable_device(dev);
dev_err(&dev->dev, "init %s fail, %d\n", pci_name(dev), retval);
return retval;
--
1.6.3.3

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Sarah Sharp
2010-12-30 23:23:37 UTC
Permalink
Refactor out the code in usb_add_hcd() to request the IRQ line for the
HCD. This will only need to be called once for the two xHCI roothubs, so
it's easier to refactor it into a function, rather than wrapping the long
if-else block into another if statement.

Signed-off-by: Sarah Sharp <sarah.a.sharp-VuQAYsv1563Yd54FQh9/***@public.gmane.org>
---
drivers/usb/core/hcd.c | 75 +++++++++++++++++++++++++++--------------------
1 files changed, 43 insertions(+), 32 deletions(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 65989e5..51104b4 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2199,6 +2199,46 @@ void usb_put_hcd (struct usb_hcd *hcd)
}
EXPORT_SYMBOL_GPL(usb_put_hcd);

+static int usb_hcd_request_irqs(struct usb_hcd *hcd,
+ unsigned int irqnum, unsigned long irqflags)
+{
+ int retval;
+
+ if (hcd->driver->irq) {
+
+ /* IRQF_DISABLED doesn't work as advertised when used together
+ * with IRQF_SHARED. As usb_hcd_irq() will always disable
+ * interrupts we can remove it here.
+ */
+ if (irqflags & IRQF_SHARED)
+ irqflags &= ~IRQF_DISABLED;
+
+ snprintf(hcd->irq_descr, sizeof(hcd->irq_descr), "%s:usb%d",
+ hcd->driver->description, hcd->self.busnum);
+ retval = request_irq(irqnum, &usb_hcd_irq, irqflags,
+ hcd->irq_descr, hcd);
+ if (retval != 0) {
+ dev_err(hcd->self.controller,
+ "request interrupt %d failed\n",
+ irqnum);
+ return retval;
+ }
+ hcd->irq = irqnum;
+ dev_info(hcd->self.controller, "irq %d, %s 0x%08llx\n", irqnum,
+ (hcd->driver->flags & HCD_MEMORY) ?
+ "io mem" : "io base",
+ (unsigned long long)hcd->rsrc_start);
+ } else {
+ hcd->irq = -1;
+ if (hcd->rsrc_start)
+ dev_info(hcd->self.controller, "%s 0x%08llx\n",
+ (hcd->driver->flags & HCD_MEMORY) ?
+ "io mem" : "io base",
+ (unsigned long long)hcd->rsrc_start);
+ }
+ return 0;
+}
+
/**
* usb_add_hcd - finish generic HCD structure initialization and register
* @hcd: the usb_hcd structure to initialize
@@ -2274,38 +2314,9 @@ int usb_add_hcd(struct usb_hcd *hcd,
dev_dbg(hcd->self.controller, "supports USB remote wakeup\n");

/* enable irqs just before we start the controller */
- if (hcd->driver->irq) {
-
- /* IRQF_DISABLED doesn't work as advertised when used together
- * with IRQF_SHARED. As usb_hcd_irq() will always disable
- * interrupts we can remove it here.
- */
- if (irqflags & IRQF_SHARED)
- irqflags &= ~IRQF_DISABLED;
-
- snprintf(hcd->irq_descr, sizeof(hcd->irq_descr), "%s:usb%d",
- hcd->driver->description, hcd->self.busnum);
- retval = request_irq(irqnum, &usb_hcd_irq, irqflags,
- hcd->irq_descr, hcd);
- if (retval != 0) {
- dev_err(hcd->self.controller,
- "request interrupt %d failed\n",
- irqnum);
- goto err_request_irq;
- }
- hcd->irq = irqnum;
- dev_info(hcd->self.controller, "irq %d, %s 0x%08llx\n", irqnum,
- (hcd->driver->flags & HCD_MEMORY) ?
- "io mem" : "io base",
- (unsigned long long)hcd->rsrc_start);
- } else {
- hcd->irq = -1;
- if (hcd->rsrc_start)
- dev_info(hcd->self.controller, "%s 0x%08llx\n",
- (hcd->driver->flags & HCD_MEMORY) ?
- "io mem" : "io base",
- (unsigned long long)hcd->rsrc_start);
- }
+ retval = usb_hcd_request_irqs(hcd, irqnum, irqflags);
+ if (retval)
+ goto err_request_irq;

retval = hcd->driver->start(hcd);
if (retval < 0) {
--
1.6.3.3

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Sarah Sharp
2010-12-30 23:23:42 UTC
Permalink
Change the bandwith_mutex in struct usb_hcd to a pointer. This will allow
the pointer to be shared across usb_hcds for the upcoming work to split
the xHCI driver roothub into a USB 2.0/1.1 and a USB 3.0 bus.

Signed-off-by: Sarah Sharp <sarah.a.sharp-VuQAYsv1563Yd54FQh9/***@public.gmane.org>
---
drivers/usb/core/hcd.c | 11 ++++++++++-
drivers/usb/core/hub.c | 8 ++++----
drivers/usb/core/message.c | 22 +++++++++++-----------
include/linux/usb/hcd.h | 2 +-
4 files changed, 26 insertions(+), 17 deletions(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 51104b4..47fb05f 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2154,6 +2154,15 @@ struct usb_hcd *usb_create_hcd (const struct hc_driver *driver,
dev_dbg (dev, "hcd alloc failed\n");
return NULL;
}
+ hcd->bandwidth_mutex = kmalloc(sizeof(*hcd->bandwidth_mutex),
+ GFP_KERNEL);
+ if (!hcd->bandwidth_mutex) {
+ kfree(hcd);
+ dev_dbg(dev, "hcd bandwidth mutex alloc failed\n");
+ return NULL;
+ }
+ mutex_init(hcd->bandwidth_mutex);
+
dev_set_drvdata(dev, hcd);
kref_init(&hcd->kref);

@@ -2168,7 +2177,6 @@ struct usb_hcd *usb_create_hcd (const struct hc_driver *driver,
#ifdef CONFIG_USB_SUSPEND
INIT_WORK(&hcd->wakeup_work, hcd_resume_work);
#endif
- mutex_init(&hcd->bandwidth_mutex);

hcd->driver = driver;
hcd->product_desc = (driver->product_desc) ? driver->product_desc :
@@ -2181,6 +2189,7 @@ static void hcd_release (struct kref *kref)
{
struct usb_hcd *hcd = container_of (kref, struct usb_hcd, kref);

+ kfree(hcd->bandwidth_mutex);
kfree(hcd);
}

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index a9c9986..928bbfe 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -3761,13 +3761,13 @@ static int usb_reset_and_verify_device(struct usb_device *udev)
if (!udev->actconfig)
goto done;

- mutex_lock(&hcd->bandwidth_mutex);
+ mutex_lock(hcd->bandwidth_mutex);
ret = usb_hcd_alloc_bandwidth(udev, udev->actconfig, NULL, NULL);
if (ret < 0) {
dev_warn(&udev->dev,
"Busted HC? Not enough HCD resources for "
"old configuration.\n");
- mutex_unlock(&hcd->bandwidth_mutex);
+ mutex_unlock(hcd->bandwidth_mutex);
goto re_enumerate;
}
ret = usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
@@ -3778,10 +3778,10 @@ static int usb_reset_and_verify_device(struct usb_device *udev)
dev_err(&udev->dev,
"can't restore configuration #%d (error=%d)\n",
udev->actconfig->desc.bConfigurationValue, ret);
- mutex_unlock(&hcd->bandwidth_mutex);
+ mutex_unlock(hcd->bandwidth_mutex);
goto re_enumerate;
}
- mutex_unlock(&hcd->bandwidth_mutex);
+ mutex_unlock(hcd->bandwidth_mutex);
usb_set_device_state(udev, USB_STATE_CONFIGURED);

/* Put interfaces back into the same altsettings as before.
diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
index 8324874..5701e85 100644
--- a/drivers/usb/core/message.c
+++ b/drivers/usb/core/message.c
@@ -1284,12 +1284,12 @@ int usb_set_interface(struct usb_device *dev, int interface, int alternate)
/* Make sure we have enough bandwidth for this alternate interface.
* Remove the current alt setting and add the new alt setting.
*/
- mutex_lock(&hcd->bandwidth_mutex);
+ mutex_lock(hcd->bandwidth_mutex);
ret = usb_hcd_alloc_bandwidth(dev, NULL, iface->cur_altsetting, alt);
if (ret < 0) {
dev_info(&dev->dev, "Not enough bandwidth for altsetting %d\n",
alternate);
- mutex_unlock(&hcd->bandwidth_mutex);
+ mutex_unlock(hcd->bandwidth_mutex);
return ret;
}

@@ -1311,10 +1311,10 @@ int usb_set_interface(struct usb_device *dev, int interface, int alternate)
} else if (ret < 0) {
/* Re-instate the old alt setting */
usb_hcd_alloc_bandwidth(dev, NULL, alt, iface->cur_altsetting);
- mutex_unlock(&hcd->bandwidth_mutex);
+ mutex_unlock(hcd->bandwidth_mutex);
return ret;
}
- mutex_unlock(&hcd->bandwidth_mutex);
+ mutex_unlock(hcd->bandwidth_mutex);

/* FIXME drivers shouldn't need to replicate/bugfix the logic here
* when they implement async or easily-killable versions of this or
@@ -1413,7 +1413,7 @@ int usb_reset_configuration(struct usb_device *dev)

config = dev->actconfig;
retval = 0;
- mutex_lock(&hcd->bandwidth_mutex);
+ mutex_lock(hcd->bandwidth_mutex);
/* Make sure we have enough bandwidth for each alternate setting 0 */
for (i = 0; i < config->desc.bNumInterfaces; i++) {
struct usb_interface *intf = config->interface[i];
@@ -1442,7 +1442,7 @@ reset_old_alts:
usb_hcd_alloc_bandwidth(dev, NULL,
alt, intf->cur_altsetting);
}
- mutex_unlock(&hcd->bandwidth_mutex);
+ mutex_unlock(hcd->bandwidth_mutex);
return retval;
}
retval = usb_control_msg(dev, usb_sndctrlpipe(dev, 0),
@@ -1451,7 +1451,7 @@ reset_old_alts:
NULL, 0, USB_CTRL_SET_TIMEOUT);
if (retval < 0)
goto reset_old_alts;
- mutex_unlock(&hcd->bandwidth_mutex);
+ mutex_unlock(hcd->bandwidth_mutex);

/* re-init hc/hcd interface/endpoint state */
for (i = 0; i < config->desc.bNumInterfaces; i++) {
@@ -1739,10 +1739,10 @@ free_interfaces:
* host controller will not allow submissions to dropped endpoints. If
* this call fails, the device state is unchanged.
*/
- mutex_lock(&hcd->bandwidth_mutex);
+ mutex_lock(hcd->bandwidth_mutex);
ret = usb_hcd_alloc_bandwidth(dev, cp, NULL, NULL);
if (ret < 0) {
- mutex_unlock(&hcd->bandwidth_mutex);
+ mutex_unlock(hcd->bandwidth_mutex);
usb_autosuspend_device(dev);
goto free_interfaces;
}
@@ -1761,11 +1761,11 @@ free_interfaces:
if (!cp) {
usb_set_device_state(dev, USB_STATE_ADDRESS);
usb_hcd_alloc_bandwidth(dev, NULL, NULL, NULL);
- mutex_unlock(&hcd->bandwidth_mutex);
+ mutex_unlock(hcd->bandwidth_mutex);
usb_autosuspend_device(dev);
goto free_interfaces;
}
- mutex_unlock(&hcd->bandwidth_mutex);
+ mutex_unlock(hcd->bandwidth_mutex);
usb_set_device_state(dev, USB_STATE_CONFIGURED);

/* Initialize the new interface structures and the
diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index 991aa4b..b6e6e30 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -138,7 +138,7 @@ struct usb_hcd {
* bandwidth_mutex should be dropped after a successful control message
* to the device, or resetting the bandwidth after a failed attempt.
*/
- struct mutex bandwidth_mutex;
+ struct mutex *bandwidth_mutex;


#define HCD_BUFFER_POOLS 4
--
1.6.3.3

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Sarah Sharp
2010-12-30 23:23:47 UTC
Permalink
The xHCI driver essentially has both a USB 2.0 and a USB 3.0 roothub. So
setting the HCD_USB3 bits in the hcd->driver->flags is a bit misleading.
Add a new field to usb_hcd, bcdUSB. Store the result of
hcd->driver->flags & HCD_MASK in it. Later, when we have the xHCI driver
register the two roothubs, we'll set the usb_hcd->bcdUSB field to HCD_USB2
for the USB 2.0 roothub, and HCD_USB3 for the USB 3.0 roothub.

Signed-off-by: Sarah Sharp <sarah.a.sharp-VuQAYsv1563Yd54FQh9/***@public.gmane.org>
---
drivers/usb/core/hcd.c | 7 ++++---
include/linux/usb/hcd.h | 3 +++
2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 47fb05f..79bad2c 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -507,7 +507,7 @@ static int rh_call_control (struct usb_hcd *hcd, struct urb *urb)
case DeviceRequest | USB_REQ_GET_DESCRIPTOR:
switch (wValue & 0xff00) {
case USB_DT_DEVICE << 8:
- switch (hcd->driver->flags & HCD_MASK) {
+ switch (hcd->bcdUSB) {
case HCD_USB3:
bufp = usb3_rh_dev_descriptor;
break;
@@ -525,7 +525,7 @@ static int rh_call_control (struct usb_hcd *hcd, struct urb *urb)
patch_protocol = 1;
break;
case USB_DT_CONFIG << 8:
- switch (hcd->driver->flags & HCD_MASK) {
+ switch (hcd->bcdUSB) {
case HCD_USB3:
bufp = ss_rh_config_descriptor;
len = sizeof ss_rh_config_descriptor;
@@ -2179,6 +2179,7 @@ struct usb_hcd *usb_create_hcd (const struct hc_driver *driver,
#endif

hcd->driver = driver;
+ hcd->bcdUSB = driver->flags & HCD_MASK;
hcd->product_desc = (driver->product_desc) ? driver->product_desc :
"USB Host Controller";
return hcd;
@@ -2288,7 +2289,7 @@ int usb_add_hcd(struct usb_hcd *hcd,
}
hcd->self.root_hub = rhdev;

- switch (hcd->driver->flags & HCD_MASK) {
+ switch (hcd->bcdUSB) {
case HCD_USB11:
rhdev->speed = USB_SPEED_FULL;
break;
diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index b6e6e30..ac2dc66 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -76,6 +76,9 @@ struct usb_hcd {
struct kref kref; /* reference counter */

const char *product_desc; /* product/vendor string */
+ int bcdUSB; /* May be different from
+ * hcd->driver->flags & HCD_MASK
+ */
char irq_descr[24]; /* driver + bus # */

struct timer_list rh_timer; /* drives root-hub polling */
--
1.6.3.3

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Alan Stern
2010-12-31 17:26:11 UTC
Permalink
Post by Sarah Sharp
The xHCI driver essentially has both a USB 2.0 and a USB 3.0 roothub. So
setting the HCD_USB3 bits in the hcd->driver->flags is a bit misleading.
Add a new field to usb_hcd, bcdUSB. Store the result of
hcd->driver->flags & HCD_MASK in it. Later, when we have the xHCI driver
register the two roothubs, we'll set the usb_hcd->bcdUSB field to HCD_USB2
for the USB 2.0 roothub, and HCD_USB3 for the USB 3.0 roothub.
This new field shouldn't be called "bcdUSB" unless it actually uses the
same binary-coded-decimal format as the bcdUSB field in the USB spec.
Since the HCD_USBx values aren't in this format, the new field should
be called something like "usb_version".

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Sarah Sharp
2010-12-30 23:23:51 UTC
Permalink
Introduce the notion of a PCI device that may be associated with more than
one USB host controller driver (struct usb_hcd). This patch is the start
of the work to separate the xHCI host controller into two roothubs: a USB
3.0 roothub with SuperSpeed-only ports, and a USB 2.0 roothub with
HS/FS/LS ports.

One usb_hcd structure is designated to be the "primary HCD", and a pointer
is added to the usb_hcd structure to keep track of that. A new function
call, usb_hcd_is_primary_hcd() is added to check whether the USB hcd is
marked as the primary HCD (or if it is not part of a roothub pair). To
allow the USB core and xHCI driver to access either roothub in a pair, a
"shared_hcd" pointer is added to the usb_hcd structure.

Add a new function, usb_create_shared_hcd(), that does roothub allocation
for paired roothubs. It will act just like usb_create_hcd() did if the
primary_hcd pointer argument is NULL. If it is passed a non-NULL
primary_hcd pointer, it sets usb_hcd->shared_hcd and usb_hcd->primary_hcd
fields. It will also skip the bandwidth_mutex allocation, and set the
secondary hcd's bandwidth_mutex pointer to the primary HCD's mutex.

IRQs are only allocated once for the primary roothub.

Introduce a new usb_hcd driver flag that indicates the host controller
driver wants to create two roothubs. If the HCD_SHARED flag is set, then
the USB core PCI probe methods will allocate a second roothub, and make
sure that second roothub gets freed during rmmod and in initialization
error paths.

If the driver that wants to create two roothubs is an xHCI driver, the
roothub that is allocated last is marked as the USB 2.0 roothub.

Signed-off-by: Sarah Sharp <sarah.a.sharp-VuQAYsv1563Yd54FQh9/***@public.gmane.org>
---
drivers/usb/core/hcd-pci.c | 38 +++++++++++++++++
drivers/usb/core/hcd.c | 95 +++++++++++++++++++++++++++++++++++--------
include/linux/usb/hcd.h | 7 +++
3 files changed, 122 insertions(+), 18 deletions(-)

diff --git a/drivers/usb/core/hcd-pci.c b/drivers/usb/core/hcd-pci.c
index 5792bb4..cf3fcbe 100644
--- a/drivers/usb/core/hcd-pci.c
+++ b/drivers/usb/core/hcd-pci.c
@@ -172,6 +172,7 @@ int usb_hcd_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
{
struct hc_driver *driver;
struct usb_hcd *hcd;
+ struct usb_hcd *shared_hcd = NULL;
int retval;

if (usb_disabled())
@@ -200,6 +201,17 @@ int usb_hcd_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
retval = -ENOMEM;
goto disable_pci;
}
+ if (driver->flags & HCD_SHARED) {
+ shared_hcd = usb_create_shared_hcd(driver, &dev->dev,
+ pci_name(dev), hcd);
+ if (!shared_hcd) {
+ retval = -ENOMEM;
+ goto dealloc_hcd;
+ }
+ /* Mark the non-primary HCD as the xHCI USB 2.0 roothub */
+ if ((driver->flags & HCD_MASK) == HCD_USB3)
+ shared_hcd->bcdUSB = HCD_USB2;
+ }

if (driver->flags & HCD_MEMORY) {
/* EHCI, OHCI */
@@ -245,12 +257,20 @@ int usb_hcd_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
retval = usb_add_hcd(hcd, dev->irq, IRQF_DISABLED | IRQF_SHARED);
if (retval != 0)
goto unmap_registers;
+ if (driver->flags & HCD_SHARED) {
+ retval = usb_add_hcd(shared_hcd, dev->irq,
+ IRQF_DISABLED | IRQF_SHARED);
+ if (retval != 0)
+ goto remove_hcd;
+ }
set_hs_companion(dev, hcd);

if (pci_dev_run_wake(dev))
pm_runtime_put_noidle(&dev->dev);
return retval;

+remove_hcd:
+ usb_remove_hcd(hcd);
unmap_registers:
if (driver->flags & HCD_MEMORY) {
iounmap(hcd->regs);
@@ -260,6 +280,8 @@ release_mem_region:
release_region(hcd->rsrc_start, hcd->rsrc_len);
clear_companion:
clear_hs_companion(dev, hcd);
+ usb_put_hcd(shared_hcd);
+dealloc_hcd:
usb_put_hcd(hcd);
disable_pci:
pci_disable_device(dev);
@@ -286,6 +308,7 @@ EXPORT_SYMBOL_GPL(usb_hcd_pci_probe);
void usb_hcd_pci_remove(struct pci_dev *dev)
{
struct usb_hcd *hcd;
+ struct usb_hcd *shared_hcd;

hcd = pci_get_drvdata(dev);
if (!hcd)
@@ -302,15 +325,30 @@ void usb_hcd_pci_remove(struct pci_dev *dev)
usb_hcd_irq(0, hcd);
local_irq_enable();

+ if (hcd->driver->flags & HCD_SHARED)
+ shared_hcd = hcd->shared_hcd;
+ else
+ shared_hcd = NULL;
+
+ if (shared_hcd)
+ usb_remove_hcd(shared_hcd);
usb_remove_hcd(hcd);
+
if (hcd->driver->flags & HCD_MEMORY) {
iounmap(hcd->regs);
release_mem_region(hcd->rsrc_start, hcd->rsrc_len);
} else {
release_region(hcd->rsrc_start, hcd->rsrc_len);
}
+
+ if (shared_hcd)
+ clear_hs_companion(dev, shared_hcd);
clear_hs_companion(dev, hcd);
+
+ if (shared_hcd)
+ usb_put_hcd(shared_hcd);
usb_put_hcd(hcd);
+
pci_disable_device(dev);
}
EXPORT_SYMBOL_GPL(usb_hcd_pci_remove);
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 79bad2c..a061c31 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2132,10 +2132,12 @@ EXPORT_SYMBOL_GPL (usb_hc_died);
/*-------------------------------------------------------------------------*/

/**
- * usb_create_hcd - create and initialize an HCD structure
+ * usb_create_shared_hcd - create and initialize an HCD structure
* @driver: HC driver that will use this hcd
* @dev: device for this HC, stored in hcd->self.controller
* @bus_name: value to store in hcd->self.bus_name
+ * @primary_hcd: a pointer to the usb_hcd structure that is sharing the
+ * PCI device. Only allocate certain resources for the primary HCD
* Context: !in_interrupt()
*
* Allocate a struct usb_hcd, with extra space at the end for the
@@ -2144,8 +2146,9 @@ EXPORT_SYMBOL_GPL (usb_hc_died);
*
* If memory is unavailable, returns NULL.
*/
-struct usb_hcd *usb_create_hcd (const struct hc_driver *driver,
- struct device *dev, const char *bus_name)
+struct usb_hcd *usb_create_shared_hcd(const struct hc_driver *driver,
+ struct device *dev, const char *bus_name,
+ struct usb_hcd *primary_hcd)
{
struct usb_hcd *hcd;

@@ -2154,16 +2157,24 @@ struct usb_hcd *usb_create_hcd (const struct hc_driver *driver,
dev_dbg (dev, "hcd alloc failed\n");
return NULL;
}
- hcd->bandwidth_mutex = kmalloc(sizeof(*hcd->bandwidth_mutex),
- GFP_KERNEL);
- if (!hcd->bandwidth_mutex) {
- kfree(hcd);
- dev_dbg(dev, "hcd bandwidth mutex alloc failed\n");
- return NULL;
+ if (primary_hcd == NULL) {
+ hcd->bandwidth_mutex = kmalloc(sizeof(*hcd->bandwidth_mutex),
+ GFP_KERNEL);
+ if (!hcd->bandwidth_mutex) {
+ kfree(hcd);
+ dev_dbg(dev, "hcd bandwidth mutex alloc failed\n");
+ return NULL;
+ }
+ mutex_init(hcd->bandwidth_mutex);
+ dev_set_drvdata(dev, hcd);
+ } else {
+ hcd->bandwidth_mutex = primary_hcd->bandwidth_mutex;
+ hcd->primary_hcd = primary_hcd;
+ primary_hcd->primary_hcd = primary_hcd;
+ hcd->shared_hcd = primary_hcd;
+ primary_hcd->shared_hcd = hcd;
}
- mutex_init(hcd->bandwidth_mutex);

- dev_set_drvdata(dev, hcd);
kref_init(&hcd->kref);

usb_bus_init(&hcd->self);
@@ -2184,13 +2195,49 @@ struct usb_hcd *usb_create_hcd (const struct hc_driver *driver,
"USB Host Controller";
return hcd;
}
+EXPORT_SYMBOL_GPL(usb_create_shared_hcd);
+
+/**
+ * usb_create_hcd - create and initialize an HCD structure
+ * @driver: HC driver that will use this hcd
+ * @dev: device for this HC, stored in hcd->self.controller
+ * @bus_name: value to store in hcd->self.bus_name
+ * @shared_hcd: a pointer to the usb_hcd structure that is sharing the
+ * PCI device. Used for allocating two usb_hcd structures
+ * under one xHCI host controller PCI device (a USB 2.0 bus
+ * and a USB 3.0 bus).
+ * Context: !in_interrupt()
+ *
+ * Allocate a struct usb_hcd, with extra space at the end for the
+ * HC driver's private data. Initialize the generic members of the
+ * hcd structure.
+ *
+ * If memory is unavailable, returns NULL.
+ */
+struct usb_hcd *usb_create_hcd(const struct hc_driver *driver,
+ struct device *dev, const char *bus_name)
+{
+ return usb_create_shared_hcd(driver, dev, bus_name, NULL);
+}
EXPORT_SYMBOL_GPL(usb_create_hcd);

+/*
+ * Roothubs that share one PCI device must also share the bandwidth mutex.
+ * Don't deallocate the bandwidth_mutex until the last shared usb_hcd is
+ * deallocated.
+ *
+ * The first shared roothub that is released will set shared_hcd to NULL. The
+ * second shared roothub that is released (or a USB host controller driver that
+ * doesn't set up shared roothubs) will deallocate the bandwidth mutex.
+ */
static void hcd_release (struct kref *kref)
{
struct usb_hcd *hcd = container_of (kref, struct usb_hcd, kref);

- kfree(hcd->bandwidth_mutex);
+ if (hcd->shared_hcd)
+ hcd->shared_hcd->shared_hcd = NULL;
+ else
+ kfree(hcd->bandwidth_mutex);
kfree(hcd);
}

@@ -2209,6 +2256,14 @@ void usb_put_hcd (struct usb_hcd *hcd)
}
EXPORT_SYMBOL_GPL(usb_put_hcd);

+inline int usb_hcd_is_primary_hcd(struct usb_hcd *hcd)
+{
+ if (!hcd->primary_hcd)
+ return 1;
+ return hcd == hcd->primary_hcd;
+}
+EXPORT_SYMBOL_GPL(usb_hcd_is_primary_hcd);
+
static int usb_hcd_request_irqs(struct usb_hcd *hcd,
unsigned int irqnum, unsigned long irqflags)
{
@@ -2324,9 +2379,11 @@ int usb_add_hcd(struct usb_hcd *hcd,
dev_dbg(hcd->self.controller, "supports USB remote wakeup\n");

/* enable irqs just before we start the controller */
- retval = usb_hcd_request_irqs(hcd, irqnum, irqflags);
- if (retval)
- goto err_request_irq;
+ if (usb_hcd_is_primary_hcd(hcd)) {
+ retval = usb_hcd_request_irqs(hcd, irqnum, irqflags);
+ if (retval)
+ goto err_request_irq;
+ }

retval = hcd->driver->start(hcd);
if (retval < 0) {
@@ -2371,7 +2428,7 @@ err_register_root_hub:
clear_bit(HCD_FLAG_POLL_RH, &hcd->flags);
del_timer_sync(&hcd->rh_timer);
err_hcd_driver_start:
- if (hcd->irq >= 0)
+ if (usb_hcd_is_primary_hcd(hcd) && hcd->irq >= 0)
free_irq(irqnum, hcd);
err_request_irq:
err_hcd_driver_setup:
@@ -2434,8 +2491,10 @@ void usb_remove_hcd(struct usb_hcd *hcd)
clear_bit(HCD_FLAG_POLL_RH, &hcd->flags);
del_timer_sync(&hcd->rh_timer);

- if (hcd->irq >= 0)
- free_irq(hcd->irq, hcd);
+ if (usb_hcd_is_primary_hcd(hcd)) {
+ if (hcd->irq >= 0)
+ free_irq(hcd->irq, hcd);
+ }

usb_put_dev(hcd->self.root_hub);
usb_deregister_bus(&hcd->self);
diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index ac2dc66..d8b17d4 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -142,6 +142,8 @@ struct usb_hcd {
* to the device, or resetting the bandwidth after a failed attempt.
*/
struct mutex *bandwidth_mutex;
+ struct usb_hcd *shared_hcd;
+ struct usb_hcd *primary_hcd;


#define HCD_BUFFER_POOLS 4
@@ -204,6 +206,7 @@ struct hc_driver {
int flags;
#define HCD_MEMORY 0x0001 /* HC regs use memory (else I/O) */
#define HCD_LOCAL_MEM 0x0002 /* HC needs local memory */
+#define HCD_SHARED 0x0004 /* Two (or more) usb_hcds share HW */
#define HCD_USB11 0x0010 /* USB 1.1 */
#define HCD_USB2 0x0020 /* USB 2.0 */
#define HCD_USB3 0x0040 /* USB 3.0 */
@@ -350,8 +353,12 @@ extern int usb_hcd_get_frame_number(struct usb_device *udev);

extern struct usb_hcd *usb_create_hcd(const struct hc_driver *driver,
struct device *dev, const char *bus_name);
+extern struct usb_hcd *usb_create_shared_hcd(const struct hc_driver *driver,
+ struct device *dev, const char *bus_name,
+ struct usb_hcd *shared_hcd);
extern struct usb_hcd *usb_get_hcd(struct usb_hcd *hcd);
extern void usb_put_hcd(struct usb_hcd *hcd);
+extern inline int usb_hcd_is_primary_hcd(struct usb_hcd *hcd);
extern int usb_add_hcd(struct usb_hcd *hcd,
unsigned int irqnum, unsigned long irqflags);
extern void usb_remove_hcd(struct usb_hcd *hcd);
--
1.6.3.3

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Alan Stern
2010-12-31 17:38:37 UTC
Permalink
Post by Sarah Sharp
Introduce the notion of a PCI device that may be associated with more than
one USB host controller driver (struct usb_hcd). This patch is the start
of the work to separate the xHCI host controller into two roothubs: a USB
3.0 roothub with SuperSpeed-only ports, and a USB 2.0 roothub with
HS/FS/LS ports.
One usb_hcd structure is designated to be the "primary HCD", and a pointer
is added to the usb_hcd structure to keep track of that. A new function
call, usb_hcd_is_primary_hcd() is added to check whether the USB hcd is
marked as the primary HCD (or if it is not part of a roothub pair). To
allow the USB core and xHCI driver to access either roothub in a pair, a
"shared_hcd" pointer is added to the usb_hcd structure.
I'm not sure this "shared_hcd" pointer is needed. Does it ever get
used in a non-trivial way?
Post by Sarah Sharp
Add a new function, usb_create_shared_hcd(), that does roothub allocation
for paired roothubs. It will act just like usb_create_hcd() did if the
primary_hcd pointer argument is NULL. If it is passed a non-NULL
primary_hcd pointer, it sets usb_hcd->shared_hcd and usb_hcd->primary_hcd
fields. It will also skip the bandwidth_mutex allocation, and set the
secondary hcd's bandwidth_mutex pointer to the primary HCD's mutex.
IRQs are only allocated once for the primary roothub.
Introduce a new usb_hcd driver flag that indicates the host controller
driver wants to create two roothubs. If the HCD_SHARED flag is set, then
the USB core PCI probe methods will allocate a second roothub, and make
sure that second roothub gets freed during rmmod and in initialization
error paths.
This doesn't seem like the right approach. The creation of a secondary
usb_hcd should be handled by the driver that needs it; it isn't such a
common activity that we need to implement it in the USB core. In
particular, hcd-pci.c shouldn't be changed.
Post by Sarah Sharp
If the driver that wants to create two roothubs is an xHCI driver, the
roothub that is allocated last is marked as the USB 2.0 roothub.
Didn't we agree earlier that xhci-hcd needs to register its USB-2 root
hub first?
Post by Sarah Sharp
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 79bad2c..a061c31 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2184,13 +2195,49 @@ struct usb_hcd *usb_create_hcd (const struct hc_driver *driver,
"USB Host Controller";
return hcd;
}
+EXPORT_SYMBOL_GPL(usb_create_shared_hcd);
+
+/**
+ * usb_create_hcd - create and initialize an HCD structure
+ * PCI device. Used for allocating two usb_hcd structures
+ * under one xHCI host controller PCI device (a USB 2.0 bus
+ * and a USB 3.0 bus).
This last argument doesn't really exist, as you can see below.
Post by Sarah Sharp
+ * Context: !in_interrupt()
+ *
+ * Allocate a struct usb_hcd, with extra space at the end for the
+ * HC driver's private data. Initialize the generic members of the
+ * hcd structure.
+ *
+ * If memory is unavailable, returns NULL.
+ */
+struct usb_hcd *usb_create_hcd(const struct hc_driver *driver,
+ struct device *dev, const char *bus_name)
+{
+ return usb_create_shared_hcd(driver, dev, bus_name, NULL);
+}
EXPORT_SYMBOL_GPL(usb_create_hcd);
@@ -2209,6 +2256,14 @@ void usb_put_hcd (struct usb_hcd *hcd)
}
EXPORT_SYMBOL_GPL(usb_put_hcd);
+inline int usb_hcd_is_primary_hcd(struct usb_hcd *hcd)
+{
+ if (!hcd->primary_hcd)
+ return 1;
+ return hcd == hcd->primary_hcd;
Do you really need to set hcd->primary_hcd to point to hcd itself? My
inclination would be to leave it unset.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Sarah Sharp
2011-01-03 23:23:44 UTC
Permalink
Post by Alan Stern
Post by Sarah Sharp
Introduce the notion of a PCI device that may be associated with more than
one USB host controller driver (struct usb_hcd). This patch is the start
of the work to separate the xHCI host controller into two roothubs: a USB
3.0 roothub with SuperSpeed-only ports, and a USB 2.0 roothub with
HS/FS/LS ports.
One usb_hcd structure is designated to be the "primary HCD", and a pointer
is added to the usb_hcd structure to keep track of that. A new function
call, usb_hcd_is_primary_hcd() is added to check whether the USB hcd is
marked as the primary HCD (or if it is not part of a roothub pair). To
allow the USB core and xHCI driver to access either roothub in a pair, a
"shared_hcd" pointer is added to the usb_hcd structure.
I'm not sure this "shared_hcd" pointer is needed. Does it ever get
used in a non-trivial way?
I'm not sure what you mean by "non-trivial". The USB core needs it, for
instance, to make sure that both buses are suspended before suspending
the PCI device:

if (hcd->driver->pci_suspend) {
/* Optimization: Don't suspend if a root-hub wakeup is
* pending and it would cause the HCD to wake up anyway.
*/
if (do_wakeup && HCD_WAKEUP_PENDING(hcd))
return -EBUSY;
if (do_wakeup && hcd->shared_hcd &&
HCD_WAKEUP_PENDING(hcd->shared_hcd))
return -EBUSY;
retval = hcd->driver->pci_suspend(hcd, do_wakeup);

Since there's only one usb_hcd stored in the PCI device pointer, there
needs to be a way to access the second usb_hcd. Other functions also
need to use shared_hcd to set various flags that are part of the usb_hcd
state. Do you have a different suggestion as to how to do that?
Post by Alan Stern
Post by Sarah Sharp
Add a new function, usb_create_shared_hcd(), that does roothub allocation
for paired roothubs. It will act just like usb_create_hcd() did if the
primary_hcd pointer argument is NULL. If it is passed a non-NULL
primary_hcd pointer, it sets usb_hcd->shared_hcd and usb_hcd->primary_hcd
fields. It will also skip the bandwidth_mutex allocation, and set the
secondary hcd's bandwidth_mutex pointer to the primary HCD's mutex.
IRQs are only allocated once for the primary roothub.
Introduce a new usb_hcd driver flag that indicates the host controller
driver wants to create two roothubs. If the HCD_SHARED flag is set, then
the USB core PCI probe methods will allocate a second roothub, and make
sure that second roothub gets freed during rmmod and in initialization
error paths.
This doesn't seem like the right approach. The creation of a secondary
usb_hcd should be handled by the driver that needs it; it isn't such a
common activity that we need to implement it in the USB core. In
particular, hcd-pci.c shouldn't be changed.
I have thought about your suggestion to make the xHCI driver call
usb_create_shared_hcd() instead of having the USB PCI layer do the
allocation and registering of the second roothub. However, I wanted to
ensure that the shared_hcd pointer was set before the first usb_hcd was
registered, and I don't think I can do that if I push the call into the
xHCI driver.

I need to set the shared_hcd pointer before the first usb_hcd is
registered so that, for example, the PCI device wouldn't get suspended
until the initialization process completed for both roothubs and both
buses are suspended. Things like flags also need to get set for both
host controllers, and I want to make sure the shared_hcd pointer is set
before the bus is used by the USB core at all.

I could potentially allocate the shared HCD in one of the xHCI
initialization routines, before usb_add_hcd() has a chance to call
register_root_hub(). But I'm not sure how the USB core will react to
being called recursively, i.e. usb_register_bus() calls xhci_run() which
calls usb_alloc_shared_hcd() and usb_register_bus(). And what if
someone decides to add a new lock to the USB core functions later? If
they don't have an xHCI device to test with, then they may not notice
some obscure code in the xHCI driver that calls the functions
recursively.

I agree that it's code that's not likely to be used by any other host
controller, but I think this code needs to be visible in the USB core.
Post by Alan Stern
Post by Sarah Sharp
If the driver that wants to create two roothubs is an xHCI driver, the
roothub that is allocated last is marked as the USB 2.0 roothub.
Didn't we agree earlier that xhci-hcd needs to register its USB-2 root
hub first?
Yes, you're right, that suggestion got lost in the mass of other changes
I had to make since then. But now that there's a usb_hcd->bcdUSB flag
(or usb_hcd->version, once I rename it), it should be trivial to fix.
Post by Alan Stern
Post by Sarah Sharp
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 79bad2c..a061c31 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2184,13 +2195,49 @@ struct usb_hcd *usb_create_hcd (const struct hc_driver *driver,
"USB Host Controller";
return hcd;
}
+EXPORT_SYMBOL_GPL(usb_create_shared_hcd);
+
+/**
+ * usb_create_hcd - create and initialize an HCD structure
+ * PCI device. Used for allocating two usb_hcd structures
+ * under one xHCI host controller PCI device (a USB 2.0 bus
+ * and a USB 3.0 bus).
This last argument doesn't really exist, as you can see below.
You're correct, that's leftover from the previous patchset. I'll remove it.
Post by Alan Stern
Post by Sarah Sharp
+ * Context: !in_interrupt()
+ *
+ * Allocate a struct usb_hcd, with extra space at the end for the
+ * HC driver's private data. Initialize the generic members of the
+ * hcd structure.
+ *
+ * If memory is unavailable, returns NULL.
+ */
+struct usb_hcd *usb_create_hcd(const struct hc_driver *driver,
+ struct device *dev, const char *bus_name)
+{
+ return usb_create_shared_hcd(driver, dev, bus_name, NULL);
+}
EXPORT_SYMBOL_GPL(usb_create_hcd);
@@ -2209,6 +2256,14 @@ void usb_put_hcd (struct usb_hcd *hcd)
}
EXPORT_SYMBOL_GPL(usb_put_hcd);
+inline int usb_hcd_is_primary_hcd(struct usb_hcd *hcd)
+{
+ if (!hcd->primary_hcd)
+ return 1;
+ return hcd == hcd->primary_hcd;
Do you really need to set hcd->primary_hcd to point to hcd itself? My
inclination would be to leave it unset.
I do use it in the xHCI driver initialization code, to ensure I'm only
starting the host controller when the second (i.e. non-primary) host
controller is registered. I would have to see if I can just test for
NULL instead, but I feel that it's more symmetrical somehow to set
primary_hcd for both HCDs.

Sarah Sharp
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Alan Stern
2011-01-04 16:22:30 UTC
Permalink
Post by Sarah Sharp
Post by Alan Stern
I'm not sure this "shared_hcd" pointer is needed. Does it ever get
used in a non-trivial way?
I'm not sure what you mean by "non-trivial". The USB core needs it, for
instance, to make sure that both buses are suspended before suspending
if (hcd->driver->pci_suspend) {
/* Optimization: Don't suspend if a root-hub wakeup is
* pending and it would cause the HCD to wake up anyway.
*/
if (do_wakeup && HCD_WAKEUP_PENDING(hcd))
return -EBUSY;
if (do_wakeup && hcd->shared_hcd &&
HCD_WAKEUP_PENDING(hcd->shared_hcd))
return -EBUSY;
retval = hcd->driver->pci_suspend(hcd, do_wakeup);
We shouldn't have to change this. It's not necessary to check that the
root hubs are suspended; the PM core is now smart enough never to
suspend a controller if the root hubs aren't already suspended. It
isn't even really necessary to avoid races between suspending the
controller and remote wakeup of the root hubs; the code you see above
is merely an optimization. It could be removed (all but the first and
last lines).

Regardless, I'm pretty sure that having a "shared_hcd" pointer isn't
the way to go. Originally we assumed that one controller = one bus.
xHCI has broken that assumption; it seems awfully foolish now to make
the assumption that one controller = one or two buses.

One possibility would be to allow each hcd to have a list of buses.
But I think this just adds unnecessary complexity. In addition, the
dividing line between a controller and a bus isn't clear. For example,
xHCI uses a single IRQ line to report events on either of the buses,
but it's easy to imagine a controller using a different IRQ for each
bus.
Post by Sarah Sharp
Since there's only one usb_hcd stored in the PCI device pointer, there
needs to be a way to access the second usb_hcd. Other functions also
need to use shared_hcd to set various flags that are part of the usb_hcd
state. Do you have a different suggestion as to how to do that?
What other functions? My suggestion is that we figure out a way to
change them so that they don't need to look at the secondary hcd or to
remove them entirely.
Post by Sarah Sharp
I have thought about your suggestion to make the xHCI driver call
usb_create_shared_hcd() instead of having the USB PCI layer do the
allocation and registering of the second roothub. However, I wanted to
ensure that the shared_hcd pointer was set before the first usb_hcd was
registered, and I don't think I can do that if I push the call into the
xHCI driver.
You don't have to worry about it if there is no shared_hcd pointer.
:-)
Post by Sarah Sharp
I need to set the shared_hcd pointer before the first usb_hcd is
registered so that, for example, the PCI device wouldn't get suspended
until the initialization process completed for both roothubs and both
buses are suspended.
That can't happen; devices are never suspended while being probed.
Post by Sarah Sharp
Things like flags also need to get set for both
host controllers, and I want to make sure the shared_hcd pointer is set
before the bus is used by the USB core at all.
Be more specific. Why exactly do you think you need this?
Post by Sarah Sharp
I could potentially allocate the shared HCD in one of the xHCI
initialization routines, before usb_add_hcd() has a chance to call
register_root_hub(). But I'm not sure how the USB core will react to
being called recursively, i.e. usb_register_bus() calls xhci_run() which
calls usb_alloc_shared_hcd() and usb_register_bus(). And what if
someone decides to add a new lock to the USB core functions later? If
they don't have an xHCI device to test with, then they may not notice
some obscure code in the xHCI driver that calls the functions
recursively.
I agree that it's code that's not likely to be used by any other host
controller, but I think this code needs to be visible in the USB core.
I still think you should be able to call usb_hcd_pci_probe for the
primary hcd, let it be registered and started, and then call
usb_add_shared_hcd for the other one.
Post by Sarah Sharp
Post by Alan Stern
Post by Sarah Sharp
+inline int usb_hcd_is_primary_hcd(struct usb_hcd *hcd)
+{
+ if (!hcd->primary_hcd)
+ return 1;
+ return hcd == hcd->primary_hcd;
Do you really need to set hcd->primary_hcd to point to hcd itself? My
inclination would be to leave it unset.
I do use it in the xHCI driver initialization code, to ensure I'm only
starting the host controller when the second (i.e. non-primary) host
controller is registered. I would have to see if I can just test for
NULL instead, but I feel that it's more symmetrical somehow to set
primary_hcd for both HCDs.
It's not a big deal. Doing it this way just means you have to make two
tests instead of only one.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Sarah Sharp
2011-01-04 20:18:08 UTC
Permalink
Alan,

I feel like your need to make this patchset "perfect" is getting in the
way of merging perfectly "good" code. I feel like no matter how many
RFCs I make, you'll still come with some optimization that needs to be
done, because the USB core is your territory. Instead, can we please
submit the code as-is for 2.6.38 and try to optimize it later? If we
end up ripping out all the references to shared_hcd later, that's fine,
because we don't guarantee a stable kernel API.
Post by Alan Stern
Post by Sarah Sharp
Post by Alan Stern
I'm not sure this "shared_hcd" pointer is needed. Does it ever get
used in a non-trivial way?
I'm not sure what you mean by "non-trivial". The USB core needs it, for
instance, to make sure that both buses are suspended before suspending
if (hcd->driver->pci_suspend) {
/* Optimization: Don't suspend if a root-hub wakeup is
* pending and it would cause the HCD to wake up anyway.
*/
if (do_wakeup && HCD_WAKEUP_PENDING(hcd))
return -EBUSY;
if (do_wakeup && hcd->shared_hcd &&
HCD_WAKEUP_PENDING(hcd->shared_hcd))
return -EBUSY;
retval = hcd->driver->pci_suspend(hcd, do_wakeup);
We shouldn't have to change this. It's not necessary to check that the
root hubs are suspended; the PM core is now smart enough never to
suspend a controller if the root hubs aren't already suspended. It
isn't even really necessary to avoid races between suspending the
controller and remote wakeup of the root hubs; the code you see above
is merely an optimization. It could be removed (all but the first and
last lines).
I don't feel like I have enough experience with the USB core and those
flags to make that sort of patch. Perhaps you could make that patch?
Post by Alan Stern
Be more specific. Why exactly do you think you need this?
There's also the calls in check_root_hub_suspended() and resume_common()
to check the HCD state. Are those also just an optimization?
Post by Alan Stern
Regardless, I'm pretty sure that having a "shared_hcd" pointer isn't
the way to go. Originally we assumed that one controller = one bus.
xHCI has broken that assumption; it seems awfully foolish now to make
the assumption that one controller = one or two buses.
One possibility would be to allow each hcd to have a list of buses.
But I think this just adds unnecessary complexity. In addition, the
dividing line between a controller and a bus isn't clear. For example,
xHCI uses a single IRQ line to report events on either of the buses,
but it's easy to imagine a controller using a different IRQ for each
bus.
I think this falls into the zero, one, many category. The patchset
changes the USB core to accept the fact that there can be more than one
bus per controller. It should be easy to extend it to "many" buses
after that.
Post by Alan Stern
I still think you should be able to call usb_hcd_pci_probe for the
primary hcd, let it be registered and started, and then call
usb_add_shared_hcd for the other one.
As we discussed before, that approach would open up a potential can of
worms if someone decided to add a new resource, like the bandwidth
mutex, that would need to be shared across both roothubs.

Sarah Sharp
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Alan Stern
2011-01-04 20:43:24 UTC
Permalink
Post by Sarah Sharp
Alan,
I feel like your need to make this patchset "perfect" is getting in the
way of merging perfectly "good" code. I feel like no matter how many
RFCs I make, you'll still come with some optimization that needs to be
done, because the USB core is your territory. Instead, can we please
submit the code as-is for 2.6.38 and try to optimize it later? If we
end up ripping out all the references to shared_hcd later, that's fine,
because we don't guarantee a stable kernel API.
I'm sorry you got that impression. It wasn't deliberate. Large
changes (and this is not a small one) often provoke many rounds of
back-and-forth argumentation before they are accepted.

And anyway, there shouldn't be too much farther to go. You're down to
solving the last few remaining issues.
Post by Sarah Sharp
Post by Alan Stern
Post by Sarah Sharp
Post by Alan Stern
I'm not sure this "shared_hcd" pointer is needed. Does it ever get
used in a non-trivial way?
I'm not sure what you mean by "non-trivial". The USB core needs it, for
instance, to make sure that both buses are suspended before suspending
if (hcd->driver->pci_suspend) {
/* Optimization: Don't suspend if a root-hub wakeup is
* pending and it would cause the HCD to wake up anyway.
*/
if (do_wakeup && HCD_WAKEUP_PENDING(hcd))
return -EBUSY;
if (do_wakeup && hcd->shared_hcd &&
HCD_WAKEUP_PENDING(hcd->shared_hcd))
return -EBUSY;
retval = hcd->driver->pci_suspend(hcd, do_wakeup);
We shouldn't have to change this. It's not necessary to check that the
root hubs are suspended; the PM core is now smart enough never to
suspend a controller if the root hubs aren't already suspended. It
isn't even really necessary to avoid races between suspending the
controller and remote wakeup of the root hubs; the code you see above
is merely an optimization. It could be removed (all but the first and
last lines).
I don't feel like I have enough experience with the USB core and those
flags to make that sort of patch. Perhaps you could make that patch?
In fact nothing needs to be changed here. We can keep this code as is;
it will detect races with the primary bus but not with the secondary
bus. That's okay; if the secondary bus has a pending wakeup request
then the controller will be resumed again right after it is suspended.
Post by Sarah Sharp
Post by Alan Stern
Be more specific. Why exactly do you think you need this?
There's also the calls in check_root_hub_suspended() and resume_common()
to check the HCD state. Are those also just an optimization?
No, they are the snows of yesteryear -- you can sort of tell from
reading the comment in suspend_common, where it talks about
power/state. check_root_hub_suspended isn't needed any more at all; it
ought to be safe to remove all references to it. Would you like me to
do this?

The check in resume_common is a little more complicated. The idea here
is that we don't want to try to resume a host controller after it has
died. That test should be okay the way it is, right?
Post by Sarah Sharp
Post by Alan Stern
Regardless, I'm pretty sure that having a "shared_hcd" pointer isn't
the way to go. Originally we assumed that one controller = one bus.
xHCI has broken that assumption; it seems awfully foolish now to make
the assumption that one controller = one or two buses.
One possibility would be to allow each hcd to have a list of buses.
But I think this just adds unnecessary complexity. In addition, the
dividing line between a controller and a bus isn't clear. For example,
xHCI uses a single IRQ line to report events on either of the buses,
but it's easy to imagine a controller using a different IRQ for each
bus.
I think this falls into the zero, one, many category. The patchset
changes the USB core to accept the fact that there can be more than one
bus per controller. It should be easy to extend it to "many" buses
after that.
Okay, I'll buy this. However it may still turn out that shared_hcd
isn't needed.
Post by Sarah Sharp
Post by Alan Stern
I still think you should be able to call usb_hcd_pci_probe for the
primary hcd, let it be registered and started, and then call
usb_add_shared_hcd for the other one.
As we discussed before, that approach would open up a potential can of
worms if someone decided to add a new resource, like the bandwidth
mutex, that would need to be shared across both roothubs.
Maybe. I might feel better about it if you replaced shared_hcd with a
list_head, so we could accomodate an arbitrary number of buses. But
that can be done later.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Sarah Sharp
2011-01-05 18:07:18 UTC
Permalink
Post by Alan Stern
Post by Sarah Sharp
Alan,
I feel like your need to make this patchset "perfect" is getting in the
way of merging perfectly "good" code. I feel like no matter how many
RFCs I make, you'll still come with some optimization that needs to be
done, because the USB core is your territory. Instead, can we please
submit the code as-is for 2.6.38 and try to optimize it later? If we
end up ripping out all the references to shared_hcd later, that's fine,
because we don't guarantee a stable kernel API.
I'm sorry you got that impression. It wasn't deliberate. Large
changes (and this is not a small one) often provoke many rounds of
back-and-forth argumentation before they are accepted.
And anyway, there shouldn't be too much farther to go. You're down to
solving the last few remaining issues.
The trivial fixes from the feedback I got on the other patches I've
already integrated. But I'm not sure what exactly you want fixed for
the design issues.

I think I'm hearing you want some of the shared_hcd references to go
away, with the eventual goal of perhaps getting rid of the pointer out
of the usb_hcd entirely. I agree that dead code like the SAW_IRQ flag
and the check_root_hub_suspended() code should go away, but I don't
think we can get rid of shared_hcd entirely.

You've addressed the use cases in the suspend and resume path, but
there's still an issue with the allocation and freeing. If you agree
that the shared roothub allocation should go in the PCI core because of
the shared resource visibility issue, then the PCI core needs to also
deallocate the shared_hcd. How do you propose to do that when the USB
PCI core doesn't have a reference to the non-primary usb_hcd?
usb_hcd_pci_remove() needs to have a pointer to call usb_put_hcd() with.

Also, in digging around the deallocation path, I found another design
decision I made based on shared_hcd being a part of usb_hcd. Right now,
the shared_hcd pointer is acting like a reference count on the bandwidth
mutex:

/*
* Roothubs that share one PCI device must also share the bandwidth mutex.
* Don't deallocate the bandwidth_mutex until the last shared usb_hcd is
* deallocated.
*
* The first shared roothub that is released will set shared_hcd to NULL. The
* second shared roothub that is released (or a USB host controller driver that
* doesn't set up shared roothubs) will deallocate the bandwidth mutex.
*/
static void hcd_release (struct kref *kref)
{
struct usb_hcd *hcd = container_of (kref, struct usb_hcd, kref);

if (hcd->shared_hcd)
hcd->shared_hcd->shared_hcd = NULL;
else
kfree(hcd->bandwidth_mutex);
kfree(hcd);
}

If the USB core has no notion of whether a shared roothub is allocated
(which you seem to be advocating for by making the xHCI driver call into
usb_hcd_pci_probe() and then allocating the shared_hcd), then the USB
core has no idea when to deallocate shared resources.

Sarah Sharp
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Alan Stern
2011-01-05 20:54:30 UTC
Permalink
Post by Sarah Sharp
I think I'm hearing you want some of the shared_hcd references to go
away, with the eventual goal of perhaps getting rid of the pointer out
of the usb_hcd entirely. I agree that dead code like the SAW_IRQ flag
and the check_root_hub_suspended() code should go away, but I don't
think we can get rid of shared_hcd entirely.
Yeah, that's what I'd like. Since you still feel you need it, go ahead
and redo the RFC using the other changes we discussed and keep the
shared_hcd pointer. Afterward we can go through and figure out how to
eliminate it (or even whether it _should_ be eliminated).
Post by Sarah Sharp
You've addressed the use cases in the suspend and resume path, but
there's still an issue with the allocation and freeing. If you agree
that the shared roothub allocation should go in the PCI core because of
the shared resource visibility issue,
Refresh my memory: What is the shared resource visibility issue? It's
probably sitting in an old email message somewhere, but so many of them
have gone by that I can't put my finger on it.
Post by Sarah Sharp
then the PCI core needs to also
deallocate the shared_hcd. How do you propose to do that when the USB
PCI core doesn't have a reference to the non-primary usb_hcd?
usb_hcd_pci_remove() needs to have a pointer to call usb_put_hcd() with.
Also, in digging around the deallocation path, I found another design
decision I made based on shared_hcd being a part of usb_hcd. Right now,
the shared_hcd pointer is acting like a reference count on the bandwidth
...
Post by Sarah Sharp
If the USB core has no notion of whether a shared roothub is allocated
(which you seem to be advocating for by making the xHCI driver call into
usb_hcd_pci_probe() and then allocating the shared_hcd), then the USB
core has no idea when to deallocate shared resources.
That's simple. You allocate the primary hcd first (along with all the
necessary resources) in the usual way by calling usb_hcd_pci_probe, and
then you add the secondary hcd, letting it share the primary's
resources (usb_add_shared_hcd). When it's time for the driver to be
unloaded, you deallocate the secondary hcd (usb_remove_shared_hcd)
leaving the shared resources intact, and then you deallocate the
primary hcd by calling usb_hcd_pci_remove normally. In other words,
you reverse the actions taken during the original registration.

This also handles the problem of the bandwidth mutex; it gets treated
like all the other shared resources. hcd_release can simply do:

if (primary_hcd(hcd))
kfree(hcd->bandwidth_mutex);

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Sarah Sharp
2011-01-05 22:50:13 UTC
Permalink
Post by Alan Stern
Post by Sarah Sharp
I think I'm hearing you want some of the shared_hcd references to go
away, with the eventual goal of perhaps getting rid of the pointer out
of the usb_hcd entirely. I agree that dead code like the SAW_IRQ flag
and the check_root_hub_suspended() code should go away, but I don't
think we can get rid of shared_hcd entirely.
Yeah, that's what I'd like. Since you still feel you need it, go ahead
and redo the RFC using the other changes we discussed and keep the
shared_hcd pointer. Afterward we can go through and figure out how to
eliminate it (or even whether it _should_ be eliminated).
Post by Sarah Sharp
You've addressed the use cases in the suspend and resume path, but
there's still an issue with the allocation and freeing. If you agree
that the shared roothub allocation should go in the PCI core because of
the shared resource visibility issue,
Refresh my memory: What is the shared resource visibility issue? It's
probably sitting in an old email message somewhere, but so many of them
have gone by that I can't put my finger on it.
Right now, the bandwidth mutex needs to be shared between the two
roothubs, and in the future there could be other usb_hcd variables that
also need to get shared. My argument was if we push the shared_hcd
allocation into the xHCI driver, the changes are less visible in the USB
PCI core, and people won't think about obscure code in xHCI before
adding new resources. But even with your suggestions to get rid of
shared_hcd, usb_create_shared_hcd() still stays in the USB core, so
people would have to be pretty blind not to see it.
Post by Alan Stern
That's simple. You allocate the primary hcd first (along with all the
necessary resources) in the usual way by calling usb_hcd_pci_probe, and
then you add the secondary hcd, letting it share the primary's
resources (usb_add_shared_hcd). When it's time for the driver to be
unloaded, you deallocate the secondary hcd (usb_remove_shared_hcd)
leaving the shared resources intact, and then you deallocate the
primary hcd by calling usb_hcd_pci_remove normally. In other words,
you reverse the actions taken during the original registration.
This also handles the problem of the bandwidth mutex; it gets treated
if (primary_hcd(hcd))
kfree(hcd->bandwidth_mutex);
Ok, yes, I think that might work. I'll finish up the other changes and
then start to think about how to remove shared_hcd.

Sarah Sharp
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Sarah Sharp
2010-12-30 23:23:59 UTC
Permalink
In the upcoming patches, the roothub emulation code will need to return
port status and port change buffers based on whether they are called wi=
th
the xHCI USB 2.0 or USB 3.0 roothub. To facilitate that, make the root=
hub
code index into an array of port addresses with wIndex, rather than
calculating the address using the offset and the address of the PORTSC
registers. Later we can set the port array to be the array of USB 3.0
port addresses, or the USB 2.0 port addresses, depending on the roothub
passed in.

Create a temporary (statically sized) port array and fill it in with bo=
th
USB 3.0 and USB 2.0 port addresses. This is inefficient to do for ever=
y
roothub call, but this is needed for git bisect compatibility. The
temporary port array will be deleted in a subsequent patch.

Signed-off-by: Sarah Sharp <sarah.a.sharp-VuQAYsv1563Yd54FQh9/***@public.gmane.org>
---
drivers/usb/host/xhci-hub.c | 126 ++++++++++++++++++++++++++--------=
--------
drivers/usb/host/xhci-ring.c | 22 +++++--
2 files changed, 95 insertions(+), 53 deletions(-)

diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 2d8119b..9cde68f 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -288,10 +288,18 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typ=
eReq, u16 wValue,
unsigned long flags;
u32 temp, temp1, status;
int retval =3D 0;
- u32 __iomem *addr;
+ u32 __iomem *port_array[15 + USB_MAXCHILDREN];
+ int i;
int slot_id;
=20
ports =3D HCS_MAX_PORTS(xhci->hcs_params1);
+ for (i =3D 0; i < ports; i++) {
+ if (i < xhci->num_usb3_ports)
+ port_array[i] =3D xhci->usb3_ports[i];
+ else
+ port_array[i] =3D
+ xhci->usb2_ports[i - xhci->num_usb3_ports];
+ }
=20
spin_lock_irqsave(&xhci->lock, flags);
switch (typeReq) {
@@ -307,8 +315,7 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeR=
eq, u16 wValue,
goto error;
wIndex--;
status =3D 0;
- addr =3D &xhci->op_regs->port_status_base + NUM_PORT_REGS*(wIndex & =
0xff);
- temp =3D xhci_readl(xhci, addr);
+ temp =3D xhci_readl(xhci, port_array[wIndex]);
xhci_dbg(xhci, "get port status, actual port %d status =3D 0x%x\n",=
wIndex, temp);
=20
/* wPortChange bits */
@@ -336,7 +343,7 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeR=
eq, u16 wValue,
temp1 =3D xhci_port_state_to_neutral(temp);
temp1 &=3D ~PORT_PLS_MASK;
temp1 |=3D PORT_LINK_STROBE | XDEV_U0;
- xhci_writel(xhci, temp1, addr);
+ xhci_writel(xhci, temp1, port_array[wIndex]);
=20
xhci_dbg(xhci, "set port %d resume\n",
wIndex + 1);
@@ -379,12 +386,11 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typ=
eReq, u16 wValue,
if (!wIndex || wIndex > ports)
goto error;
wIndex--;
- addr =3D &xhci->op_regs->port_status_base + NUM_PORT_REGS*(wIndex & =
0xff);
- temp =3D xhci_readl(xhci, addr);
+ temp =3D xhci_readl(xhci, port_array[wIndex]);
temp =3D xhci_port_state_to_neutral(temp);
switch (wValue) {
case USB_PORT_FEAT_SUSPEND:
- temp =3D xhci_readl(xhci, addr);
+ temp =3D xhci_readl(xhci, port_array[wIndex]);
/* In spec software should not attempt to suspend
* a port unless the port reports that it is in the
* enabled (PED =3D =E2=80=981=E2=80=99,PLS < =E2=80=983=E2=80=99) =
state.
@@ -409,13 +415,13 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typ=
eReq, u16 wValue,
temp =3D xhci_port_state_to_neutral(temp);
temp &=3D ~PORT_PLS_MASK;
temp |=3D PORT_LINK_STROBE | XDEV_U3;
- xhci_writel(xhci, temp, addr);
+ xhci_writel(xhci, temp, port_array[wIndex]);
=20
spin_unlock_irqrestore(&xhci->lock, flags);
msleep(10); /* wait device to enter */
spin_lock_irqsave(&xhci->lock, flags);
=20
- temp =3D xhci_readl(xhci, addr);
+ temp =3D xhci_readl(xhci, port_array[wIndex]);
xhci->suspended_ports |=3D 1 << wIndex;
break;
case USB_PORT_FEAT_POWER:
@@ -425,34 +431,34 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typ=
eReq, u16 wValue,
* However, khubd will ignore the roothub events until
* the roothub is registered.
*/
- xhci_writel(xhci, temp | PORT_POWER, addr);
+ xhci_writel(xhci, temp | PORT_POWER,
+ port_array[wIndex]);
=20
- temp =3D xhci_readl(xhci, addr);
+ temp =3D xhci_readl(xhci, port_array[wIndex]);
xhci_dbg(xhci, "set port power, actual port %d status =3D 0x%x\n",=
wIndex, temp);
break;
case USB_PORT_FEAT_RESET:
temp =3D (temp | PORT_RESET);
- xhci_writel(xhci, temp, addr);
+ xhci_writel(xhci, temp, port_array[wIndex]);
=20
- temp =3D xhci_readl(xhci, addr);
+ temp =3D xhci_readl(xhci, port_array[wIndex]);
xhci_dbg(xhci, "set port reset, actual port %d status =3D 0x%x\n",=
wIndex, temp);
break;
default:
goto error;
}
- temp =3D xhci_readl(xhci, addr); /* unblock any posted writes */
+ /* unblock any posted writes */
+ temp =3D xhci_readl(xhci, port_array[wIndex]);
break;
case ClearPortFeature:
if (!wIndex || wIndex > ports)
goto error;
wIndex--;
- addr =3D &xhci->op_regs->port_status_base +
- NUM_PORT_REGS*(wIndex & 0xff);
- temp =3D xhci_readl(xhci, addr);
+ temp =3D xhci_readl(xhci, port_array[wIndex]);
temp =3D xhci_port_state_to_neutral(temp);
switch (wValue) {
case USB_PORT_FEAT_SUSPEND:
- temp =3D xhci_readl(xhci, addr);
+ temp =3D xhci_readl(xhci, port_array[wIndex]);
xhci_dbg(xhci, "clear USB_PORT_FEAT_SUSPEND\n");
xhci_dbg(xhci, "PORTSC %04x\n", temp);
if (temp & PORT_RESET)
@@ -464,24 +470,28 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typ=
eReq, u16 wValue,
temp =3D xhci_port_state_to_neutral(temp);
temp &=3D ~PORT_PLS_MASK;
temp |=3D PORT_LINK_STROBE | XDEV_U0;
- xhci_writel(xhci, temp, addr);
- xhci_readl(xhci, addr);
+ xhci_writel(xhci, temp,
+ port_array[wIndex]);
+ xhci_readl(xhci, port_array[wIndex]);
} else {
temp =3D xhci_port_state_to_neutral(temp);
temp &=3D ~PORT_PLS_MASK;
temp |=3D PORT_LINK_STROBE | XDEV_RESUME;
- xhci_writel(xhci, temp, addr);
+ xhci_writel(xhci, temp,
+ port_array[wIndex]);
=20
spin_unlock_irqrestore(&xhci->lock,
flags);
msleep(20);
spin_lock_irqsave(&xhci->lock, flags);
=20
- temp =3D xhci_readl(xhci, addr);
+ temp =3D xhci_readl(xhci,
+ port_array[wIndex]);
temp =3D xhci_port_state_to_neutral(temp);
temp &=3D ~PORT_PLS_MASK;
temp |=3D PORT_LINK_STROBE | XDEV_U0;
- xhci_writel(xhci, temp, addr);
+ xhci_writel(xhci, temp,
+ port_array[wIndex]);
}
xhci->port_c_suspend |=3D 1 << wIndex;
}
@@ -500,10 +510,11 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typ=
eReq, u16 wValue,
case USB_PORT_FEAT_C_OVER_CURRENT:
case USB_PORT_FEAT_C_ENABLE:
xhci_clear_port_change_bit(xhci, wValue, wIndex,
- addr, temp);
+ port_array[wIndex], temp);
break;
case USB_PORT_FEAT_ENABLE:
- xhci_disable_port(xhci, wIndex, addr, temp);
+ xhci_disable_port(xhci, wIndex,
+ port_array[wIndex], temp);
break;
default:
goto error;
@@ -534,9 +545,16 @@ int xhci_hub_status_data(struct usb_hcd *hcd, char=
*buf)
int i, retval;
struct xhci_hcd *xhci =3D hcd_to_xhci(hcd);
int ports;
- u32 __iomem *addr;
+ u32 __iomem *port_array[15 + USB_MAXCHILDREN];
=20
ports =3D HCS_MAX_PORTS(xhci->hcs_params1);
+ for (i =3D 0; i < ports; i++) {
+ if (i < xhci->num_usb3_ports)
+ port_array[i] =3D xhci->usb3_ports[i];
+ else
+ port_array[i] =3D
+ xhci->usb2_ports[i - xhci->num_usb3_ports];
+ }
=20
/* Initial status is no changes */
retval =3D (ports + 8) / 8;
@@ -548,9 +566,7 @@ int xhci_hub_status_data(struct usb_hcd *hcd, char =
*buf)
spin_lock_irqsave(&xhci->lock, flags);
/* For each port, did anything change? If so, set that bit in buf. *=
/
for (i =3D 0; i < ports; i++) {
- addr =3D &xhci->op_regs->port_status_base +
- NUM_PORT_REGS*i;
- temp =3D xhci_readl(xhci, addr);
+ temp =3D xhci_readl(xhci, port_array[i]);
if ((temp & mask) !=3D 0 ||
(xhci->port_c_suspend & 1 << i) ||
(xhci->resume_done[i] && time_after_eq(
@@ -569,10 +585,19 @@ int xhci_bus_suspend(struct usb_hcd *hcd)
{
struct xhci_hcd *xhci =3D hcd_to_xhci(hcd);
int max_ports, port_index;
+ u32 __iomem *port_array[15 + USB_MAXCHILDREN];
+ int i;
unsigned long flags;
=20
xhci_dbg(xhci, "suspend root hub\n");
max_ports =3D HCS_MAX_PORTS(xhci->hcs_params1);
+ for (i =3D 0; i < max_ports; i++) {
+ if (i < xhci->num_usb3_ports)
+ port_array[i] =3D xhci->usb3_ports[i];
+ else
+ port_array[i] =3D
+ xhci->usb2_ports[i - xhci->num_usb3_ports];
+ }
=20
spin_lock_irqsave(&xhci->lock, flags);
=20
@@ -593,13 +618,10 @@ int xhci_bus_suspend(struct usb_hcd *hcd)
xhci->bus_suspended =3D 0;
while (port_index--) {
/* suspend the port if the port is not suspended */
- u32 __iomem *addr;
u32 t1, t2;
int slot_id;
=20
- addr =3D &xhci->op_regs->port_status_base +
- NUM_PORT_REGS * (port & 0xff);
- t1 =3D xhci_readl(xhci, addr);
+ t1 =3D xhci_readl(xhci, port_array[port_index]);
t2 =3D xhci_port_state_to_neutral(t1);
=20
if ((t1 & PORT_PE) && !(t1 & PORT_PLS_MASK)) {
@@ -628,15 +650,17 @@ int xhci_bus_suspend(struct usb_hcd *hcd)
=20
t1 =3D xhci_port_state_to_neutral(t1);
if (t1 !=3D t2)
- xhci_writel(xhci, t2, addr);
+ xhci_writel(xhci, t2, port_array[port_index]);
=20
if (DEV_HIGHSPEED(t1)) {
/* enable remote wake up for USB 2.0 */
u32 __iomem *addr;
u32 tmp;
=20
- addr =3D &xhci->op_regs->port_power_base +
- NUM_PORT_REGS * (port & 0xff);
+ /* Add one to the port status register address to get
+ * the port power control register address.
+ */
+ addr =3D port_array[port_index] + 1;
tmp =3D xhci_readl(xhci, addr);
tmp |=3D PORT_RWE;
xhci_writel(xhci, tmp, addr);
@@ -652,11 +676,20 @@ int xhci_bus_resume(struct usb_hcd *hcd)
{
struct xhci_hcd *xhci =3D hcd_to_xhci(hcd);
int max_ports, port_index;
+ u32 __iomem *port_array[15 + USB_MAXCHILDREN];
+ int i;
u32 temp;
unsigned long flags;
=20
xhci_dbg(xhci, "resume root hub\n");
max_ports =3D HCS_MAX_PORTS(xhci->hcs_params1);
+ for (i =3D 0; i < max_ports; i++) {
+ if (i < xhci->num_usb3_ports)
+ port_array[i] =3D xhci->usb3_ports[i];
+ else
+ port_array[i] =3D
+ xhci->usb2_ports[i - xhci->num_usb3_ports];
+ }
=20
if (time_before(jiffies, xhci->next_statechange))
msleep(5);
@@ -676,13 +709,10 @@ int xhci_bus_resume(struct usb_hcd *hcd)
while (port_index--) {
/* Check whether need resume ports. If needed
resume port and disable remote wakeup */
- u32 __iomem *addr;
u32 temp;
int slot_id;
=20
- addr =3D &xhci->op_regs->port_status_base +
- NUM_PORT_REGS * (port & 0xff);
- temp =3D xhci_readl(xhci, addr);
+ temp =3D xhci_readl(xhci, port_array[port_index]);
if (DEV_SUPERSPEED(temp))
temp &=3D ~(PORT_RWC_BITS | PORT_CEC | PORT_WAKE_BITS);
else
@@ -693,36 +723,38 @@ int xhci_bus_resume(struct usb_hcd *hcd)
temp =3D xhci_port_state_to_neutral(temp);
temp &=3D ~PORT_PLS_MASK;
temp |=3D PORT_LINK_STROBE | XDEV_U0;
- xhci_writel(xhci, temp, addr);
+ xhci_writel(xhci, temp, port_array[port_index]);
} else {
temp =3D xhci_port_state_to_neutral(temp);
temp &=3D ~PORT_PLS_MASK;
temp |=3D PORT_LINK_STROBE | XDEV_RESUME;
- xhci_writel(xhci, temp, addr);
+ xhci_writel(xhci, temp, port_array[port_index]);
=20
spin_unlock_irqrestore(&xhci->lock, flags);
msleep(20);
spin_lock_irqsave(&xhci->lock, flags);
=20
- temp =3D xhci_readl(xhci, addr);
+ temp =3D xhci_readl(xhci, port_array[port_index]);
temp =3D xhci_port_state_to_neutral(temp);
temp &=3D ~PORT_PLS_MASK;
temp |=3D PORT_LINK_STROBE | XDEV_U0;
- xhci_writel(xhci, temp, addr);
+ xhci_writel(xhci, temp, port_array[port_index]);
}
slot_id =3D xhci_find_slot_id_by_port(xhci, port_index + 1);
if (slot_id)
xhci_ring_device(xhci, slot_id);
} else
- xhci_writel(xhci, temp, addr);
+ xhci_writel(xhci, temp, port_array[port_index]);
=20
if (DEV_HIGHSPEED(temp)) {
/* disable remote wake up for USB 2.0 */
u32 __iomem *addr;
u32 tmp;
=20
- addr =3D &xhci->op_regs->port_power_base +
- NUM_PORT_REGS * (port & 0xff);
+ /* Add one to the port status register address to get
+ * the port power control register address.
+ */
+ addr =3D port_array[port_index] + 1;
tmp =3D xhci_readl(xhci, addr);
tmp &=3D ~PORT_RWE;
xhci_writel(xhci, tmp, addr);
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.=
c
index e9f1aec..a24c6c5 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -1168,9 +1168,11 @@ static void handle_port_status(struct xhci_hcd *=
xhci,
struct usb_hcd *hcd =3D xhci_to_hcd(xhci);
u32 port_id;
u32 temp, temp1;
- u32 __iomem *addr;
int max_ports;
int slot_id;
+ unsigned int faked_port_index;
+ u32 __iomem *port_array[15 + USB_MAXCHILDREN];
+ int i;
=20
/* Port status change events always have a successful completion code=
*/
if (GET_COMP_CODE(event->generic.field[2]) !=3D COMP_SUCCESS) {
@@ -1186,8 +1188,16 @@ static void handle_port_status(struct xhci_hcd *=
xhci,
goto cleanup;
}
=20
- addr =3D &xhci->op_regs->port_status_base + NUM_PORT_REGS * (port_id =
- 1);
- temp =3D xhci_readl(xhci, addr);
+ for (i =3D 0; i < max_ports; i++) {
+ if (i < xhci->num_usb3_ports)
+ port_array[i] =3D xhci->usb3_ports[i];
+ else
+ port_array[i] =3D
+ xhci->usb2_ports[i - xhci->num_usb3_ports];
+ }
+
+ faked_port_index =3D port_id;
+ temp =3D xhci_readl(xhci, port_array[faked_port_index]);
if (hcd->state =3D=3D HC_STATE_SUSPENDED) {
xhci_dbg(xhci, "resume root hub\n");
usb_hcd_resume_root_hub(hcd);
@@ -1207,7 +1217,7 @@ static void handle_port_status(struct xhci_hcd *x=
hci,
temp =3D xhci_port_state_to_neutral(temp);
temp &=3D ~PORT_PLS_MASK;
temp |=3D PORT_LINK_STROBE | XDEV_U0;
- xhci_writel(xhci, temp, addr);
+ xhci_writel(xhci, temp, port_array[faked_port_index]);
slot_id =3D xhci_find_slot_id_by_port(xhci, port_id);
if (!slot_id) {
xhci_dbg(xhci, "slot_id is zero\n");
@@ -1216,10 +1226,10 @@ static void handle_port_status(struct xhci_hcd =
*xhci,
xhci_ring_device(xhci, slot_id);
xhci_dbg(xhci, "resume SS port %d finished\n", port_id);
/* Clear PORT_PLC */
- temp =3D xhci_readl(xhci, addr);
+ temp =3D xhci_readl(xhci, port_array[faked_port_index]);
temp =3D xhci_port_state_to_neutral(temp);
temp |=3D PORT_PLC;
- xhci_writel(xhci, temp, addr);
+ xhci_writel(xhci, temp, port_array[faked_port_index]);
} else {
xhci_dbg(xhci, "resume HS port %d\n", port_id);
xhci->resume_done[port_id - 1] =3D jiffies +
--=20
1.6.3.3

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Sarah Sharp
2010-12-30 23:23:55 UTC
Permalink
The hcd->flags are in a sorry state. Some of them are clearly specific to
the particular roothub (HCD_POLL_RH, HCD_POLL_PENDING, and
HCD_WAKEUP_PENDING), but some flags are related to PCI device state
(HCD_HW_ACCESSIBLE and HCD_SAW_IRQ). This is an issue when one PCI device
can have two roothubs that share the same IRQ line and hardware.

Make sure to set HCD_FLAG_SAW_IRQ for both roothubs when an interrupt is
serviced, or an URB is unlinked without an interrupt. (We can't tell if
the host actually serviced an interrupt for a particular bus, but we can
tell it serviced some interrupt.)

HCD_HW_ACCESSIBLE is set once by usb_add_hcd(), which is set for both
roothubs as they are added, so it doesn't need to be modified.
HCD_POLL_RH and HCD_POLL_PENDING are only checked by the USB core, and
they are never set by the xHCI driver, since the roothub never needs to be
polled.

The usb_hcd's state field is a similar mess. Sometimes the state applies
to the underlying hardware: HC_STATE_HALT, HC_STATE_RUNNING, and
HC_STATE_QUIESCING. But sometimes the state refers to the roothub state:
HC_STATE_RESUMING and HC_STATE_SUSPENDED.

This poses an issue with the xHCI split roothub, where two buses are
registered for one PCI device. Each bus in the xHCI split roothub can be
suspended separately, but both buses must be suspended before the PCI
device can be suspended. Therefore, make sure that the USB core checks
hcd->state equals HC_STATE_SUSPENDED for both roothubs before suspending
the PCI host.

Make sure to kill off the shared roothub when the PCI resume fails.

The xHCI driver will need to ensure that HC_STATE_HALT, HC_STATE_RUNNING,
and HC_STATE_QUIESCING will be set for both the roothubs.

I'm not quite sure if the code in hcd_pci_suspend_noirq() is correct,
please check. If one roothub is halted, then both roothubs should be
halted (since they share the same hardware). But I suppose there could be
a race condition where one usb_hcd->state is set to HC_STATE_HALT, but the
other isn't yet?

Signed-off-by: Sarah Sharp <sarah.a.sharp-VuQAYsv1563Yd54FQh9/***@public.gmane.org>
---
drivers/usb/core/hcd-pci.c | 35 ++++++++++++++++++++++++++++++-----
drivers/usb/core/hcd.c | 4 ++++
drivers/usb/host/xhci-ring.c | 2 ++
3 files changed, 36 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/core/hcd-pci.c b/drivers/usb/core/hcd-pci.c
index cf3fcbe..e8d36e7 100644
--- a/drivers/usb/core/hcd-pci.c
+++ b/drivers/usb/core/hcd-pci.c
@@ -406,6 +406,14 @@ static int check_root_hub_suspended(struct device *dev)
dev_warn(dev, "Root hub is not suspended\n");
return -EBUSY;
}
+ if (hcd->shared_hcd) {
+ hcd = hcd->shared_hcd;
+ if (!(hcd->state == HC_STATE_SUSPENDED ||
+ hcd->state == HC_STATE_HALT)) {
+ dev_warn(dev, "Secondary root hub is not suspended\n");
+ return -EBUSY;
+ }
+ }
return 0;
}

@@ -430,11 +438,16 @@ static int suspend_common(struct device *dev, bool do_wakeup)
*/
if (do_wakeup && HCD_WAKEUP_PENDING(hcd))
return -EBUSY;
+ if (do_wakeup && hcd->shared_hcd &&
+ HCD_WAKEUP_PENDING(hcd->shared_hcd))
+ return -EBUSY;
retval = hcd->driver->pci_suspend(hcd, do_wakeup);
suspend_report_result(hcd->driver->pci_suspend, retval);

/* Check again in case wakeup raced with pci_suspend */
- if (retval == 0 && do_wakeup && HCD_WAKEUP_PENDING(hcd)) {
+ if ((retval == 0 && do_wakeup && HCD_WAKEUP_PENDING(hcd)) ||
+ (retval == 0 && do_wakeup && hcd->shared_hcd &&
+ HCD_WAKEUP_PENDING(hcd->shared_hcd))) {
if (hcd->driver->pci_resume)
hcd->driver->pci_resume(hcd, false);
retval = -EBUSY;
@@ -465,7 +478,9 @@ static int resume_common(struct device *dev, int event)
struct usb_hcd *hcd = pci_get_drvdata(pci_dev);
int retval;

- if (hcd->state != HC_STATE_SUSPENDED) {
+ if (hcd->state != HC_STATE_SUSPENDED ||
+ (hcd->shared_hcd &&
+ hcd->shared_hcd->state != HC_STATE_SUSPENDED)) {
dev_dbg(dev, "can't resume, not suspended!\n");
return 0;
}
@@ -479,6 +494,8 @@ static int resume_common(struct device *dev, int event)
pci_set_master(pci_dev);

clear_bit(HCD_FLAG_SAW_IRQ, &hcd->flags);
+ if (hcd->shared_hcd)
+ clear_bit(HCD_FLAG_SAW_IRQ, &hcd->shared_hcd->flags);

if (hcd->driver->pci_resume) {
if (event != PM_EVENT_AUTO_RESUME)
@@ -488,6 +505,8 @@ static int resume_common(struct device *dev, int event)
event == PM_EVENT_RESTORE);
if (retval) {
dev_err(dev, "PCI post-resume error %d!\n", retval);
+ if (hcd->shared_hcd)
+ usb_hc_died(hcd->shared_hcd);
usb_hc_died(hcd);
}
}
@@ -513,11 +532,17 @@ static int hcd_pci_suspend_noirq(struct device *dev)

pci_save_state(pci_dev);

- /* If the root hub is HALTed rather than SUSPENDed,
+ /* If both roothubs are HALTed rather than SUSPENDed,
* disallow remote wakeup.
*/
- if (hcd->state == HC_STATE_HALT)
- device_set_wakeup_enable(dev, 0);
+ if (!hcd->shared_hcd) {
+ if (hcd->state == HC_STATE_HALT)
+ device_set_wakeup_enable(dev, 0);
+ } else {
+ if (hcd->state == HC_STATE_HALT &&
+ hcd->shared_hcd->state == HC_STATE_HALT)
+ device_set_wakeup_enable(dev, 0);
+ }
dev_dbg(dev, "wakeup: %d\n", device_may_wakeup(dev));

/* Possibly enable remote wakeup,
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index a061c31..48717b6 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -1156,6 +1156,8 @@ int usb_hcd_check_unlink_urb(struct usb_hcd *hcd, struct urb *urb,
dev_warn(hcd->self.controller, "Unlink after no-IRQ? "
"Controller is probably using the wrong IRQ.\n");
set_bit(HCD_FLAG_SAW_IRQ, &hcd->flags);
+ if (hcd->shared_hcd)
+ set_bit(HCD_FLAG_SAW_IRQ, &hcd->shared_hcd->flags);
}

return 0;
@@ -2089,6 +2091,8 @@ irqreturn_t usb_hcd_irq (int irq, void *__hcd)
rc = IRQ_NONE;
} else {
set_bit(HCD_FLAG_SAW_IRQ, &hcd->flags);
+ if (hcd->shared_hcd)
+ set_bit(HCD_FLAG_SAW_IRQ, &hcd->shared_hcd->flags);

if (unlikely(hcd->state == HC_STATE_HALT))
usb_hc_died(hcd);
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 0a58be8..e9f1aec 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -2190,6 +2190,8 @@ irqreturn_t xhci_msi_irq(int irq, struct usb_hcd *hcd)
irqreturn_t ret;

set_bit(HCD_FLAG_SAW_IRQ, &hcd->flags);
+ if (hcd->shared_hcd)
+ set_bit(HCD_FLAG_SAW_IRQ, &hcd->shared_hcd->flags);

ret = xhci_irq(hcd);
--
1.6.3.3

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Alan Stern
2010-12-31 17:53:24 UTC
Permalink
Post by Sarah Sharp
The hcd->flags are in a sorry state. Some of them are clearly specific to
the particular roothub (HCD_POLL_RH, HCD_POLL_PENDING, and
HCD_WAKEUP_PENDING), but some flags are related to PCI device state
(HCD_HW_ACCESSIBLE and HCD_SAW_IRQ). This is an issue when one PCI device
can have two roothubs that share the same IRQ line and hardware.
Make sure to set HCD_FLAG_SAW_IRQ for both roothubs when an interrupt is
serviced, or an URB is unlinked without an interrupt. (We can't tell if
the host actually serviced an interrupt for a particular bus, but we can
tell it serviced some interrupt.)
You know, SAW_IRQ isn't used much any more. It was originally added to
help debug problems involving IRQ routing, but those have been pretty
much all fixed up by now. Instead of worrying about it, you can simply
get rid of this flag entirely.
Post by Sarah Sharp
HCD_HW_ACCESSIBLE is set once by usb_add_hcd(), which is set for both
roothubs as they are added, so it doesn't need to be modified.
HCD_POLL_RH and HCD_POLL_PENDING are only checked by the USB core, and
they are never set by the xHCI driver, since the roothub never needs to be
polled.
The usb_hcd's state field is a similar mess. Sometimes the state applies
to the underlying hardware: HC_STATE_HALT, HC_STATE_RUNNING, and
HC_STATE_RESUMING and HC_STATE_SUSPENDED.
I'm not sure this distinction makes any sense. But in any case, it
certainly is true that hcd->state is a terrible mess.
Post by Sarah Sharp
This poses an issue with the xHCI split roothub, where two buses are
registered for one PCI device. Each bus in the xHCI split roothub can be
suspended separately, but both buses must be suspended before the PCI
device can be suspended. Therefore, make sure that the USB core checks
hcd->state equals HC_STATE_SUSPENDED for both roothubs before suspending
the PCI host.
This is tricky. Normally both buses will indeed be suspended before
the controller; the only situation where that wouldn't be true is if a
remote wakeup races with the controller suspend. I wonder if we can't
detect these races in a better way.
Post by Sarah Sharp
Make sure to kill off the shared roothub when the PCI resume fails.
You can do this in the xhci resume routine instead of in the core.
Post by Sarah Sharp
The xHCI driver will need to ensure that HC_STATE_HALT, HC_STATE_RUNNING,
and HC_STATE_QUIESCING will be set for both the roothubs.
I'm not quite sure if the code in hcd_pci_suspend_noirq() is correct,
please check. If one roothub is halted, then both roothubs should be
halted (since they share the same hardware). But I suppose there could be
a race condition where one usb_hcd->state is set to HC_STATE_HALT, but the
other isn't yet?
That business about not allowing remote wakeup if the controller is
HALTed can be handled elsewhere, such as in usb_hc_died().

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Sarah Sharp
2011-01-03 23:30:55 UTC
Permalink
Post by Alan Stern
Post by Sarah Sharp
The hcd->flags are in a sorry state. Some of them are clearly specific to
the particular roothub (HCD_POLL_RH, HCD_POLL_PENDING, and
HCD_WAKEUP_PENDING), but some flags are related to PCI device state
(HCD_HW_ACCESSIBLE and HCD_SAW_IRQ). This is an issue when one PCI device
can have two roothubs that share the same IRQ line and hardware.
Make sure to set HCD_FLAG_SAW_IRQ for both roothubs when an interrupt is
serviced, or an URB is unlinked without an interrupt. (We can't tell if
the host actually serviced an interrupt for a particular bus, but we can
tell it serviced some interrupt.)
You know, SAW_IRQ isn't used much any more. It was originally added to
help debug problems involving IRQ routing, but those have been pretty
much all fixed up by now. Instead of worrying about it, you can simply
get rid of this flag entirely.
Yay! I'm always happy to get rid of dead code.
Post by Alan Stern
Post by Sarah Sharp
HCD_HW_ACCESSIBLE is set once by usb_add_hcd(), which is set for both
roothubs as they are added, so it doesn't need to be modified.
HCD_POLL_RH and HCD_POLL_PENDING are only checked by the USB core, and
they are never set by the xHCI driver, since the roothub never needs to be
polled.
The usb_hcd's state field is a similar mess. Sometimes the state applies
to the underlying hardware: HC_STATE_HALT, HC_STATE_RUNNING, and
HC_STATE_RESUMING and HC_STATE_SUSPENDED.
I'm not sure this distinction makes any sense. But in any case, it
certainly is true that hcd->state is a terrible mess.
I had thought that HC_STATE_SUSPENDED referred to when the usb_bus was
suspended. Now that either bus under the xHCI host can be suspended,
HC_STATE_RESUMING and HC_STATE_SUSPENDED doesn't make sense when applied
to a usb_hcd, only to a specific usb_bus.
Post by Alan Stern
Post by Sarah Sharp
This poses an issue with the xHCI split roothub, where two buses are
registered for one PCI device. Each bus in the xHCI split roothub can be
suspended separately, but both buses must be suspended before the PCI
device can be suspended. Therefore, make sure that the USB core checks
hcd->state equals HC_STATE_SUSPENDED for both roothubs before suspending
the PCI host.
This is tricky. Normally both buses will indeed be suspended before
the controller; the only situation where that wouldn't be true is if a
remote wakeup races with the controller suspend. I wonder if we can't
detect these races in a better way.
Hmm, interesting. I don't have any ideas currently about that; I would
have to look at the code.
Post by Alan Stern
Post by Sarah Sharp
Make sure to kill off the shared roothub when the PCI resume fails.
You can do this in the xhci resume routine instead of in the core.
Ok, I'll look into doing that.
Post by Alan Stern
Post by Sarah Sharp
The xHCI driver will need to ensure that HC_STATE_HALT, HC_STATE_RUNNING,
and HC_STATE_QUIESCING will be set for both the roothubs.
I'm not quite sure if the code in hcd_pci_suspend_noirq() is correct,
please check. If one roothub is halted, then both roothubs should be
halted (since they share the same hardware). But I suppose there could be
a race condition where one usb_hcd->state is set to HC_STATE_HALT, but the
other isn't yet?
That business about not allowing remote wakeup if the controller is
HALTed can be handled elsewhere, such as in usb_hc_died().
Ok, sure.

Sarah Sharp
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Alan Stern
2011-01-04 16:32:01 UTC
Permalink
Post by Sarah Sharp
Post by Alan Stern
Post by Sarah Sharp
The usb_hcd's state field is a similar mess. Sometimes the state applies
to the underlying hardware: HC_STATE_HALT, HC_STATE_RUNNING, and
HC_STATE_RESUMING and HC_STATE_SUSPENDED.
I'm not sure this distinction makes any sense. But in any case, it
certainly is true that hcd->state is a terrible mess.
I had thought that HC_STATE_SUSPENDED referred to when the usb_bus was
suspended. Now that either bus under the xHCI host can be suspended,
HC_STATE_RESUMING and HC_STATE_SUSPENDED doesn't make sense when applied
to a usb_hcd, only to a specific usb_bus.
Look at how hcd->state is used in hcd.c and hcd-pci.c. I believe most
of the usages refer to the bus, not the controller.

One thing we will have to worry about is usb_hc_died(). When anything
goes wrong with an xHCI controller, both buses have to be stopped. In
the end, it may be necessary for xhci-hcd to take care of this
directly.
Post by Sarah Sharp
Post by Alan Stern
Post by Sarah Sharp
This poses an issue with the xHCI split roothub, where two buses are
registered for one PCI device. Each bus in the xHCI split roothub can be
suspended separately, but both buses must be suspended before the PCI
device can be suspended. Therefore, make sure that the USB core checks
hcd->state equals HC_STATE_SUSPENDED for both roothubs before suspending
the PCI host.
This is tricky. Normally both buses will indeed be suspended before
the controller; the only situation where that wouldn't be true is if a
remote wakeup races with the controller suspend. I wonder if we can't
detect these races in a better way.
Hmm, interesting. I don't have any ideas currently about that; I would
have to look at the code.
I can't think of any simple way to do it, offhand. But since it's only
an optimization, we don't _need_ to do it at all.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Sarah Sharp
2010-12-30 23:24:03 UTC
Permalink
There are several variables in the xhci_hcd structure that are related to
bus suspend and resume state. There are a couple different port status
arrays that are accessed by port index. Move those variables into a
separate structure, xhci_bus_state. Stash that structure in xhci_hcd.

When we have two roothhubs that can be suspended and resumed separately,
we can have two xhci_bus_states, and index into the port arrays in each
structure with the fake roothub port index (not the real hardware port
index).

Signed-off-by: Sarah Sharp <sarah.a.sharp-VuQAYsv1563Yd54FQh9/***@public.gmane.org>
---
drivers/usb/host/xhci-hub.c | 50 ++++++++++++++++++++++++-----------------
drivers/usb/host/xhci-mem.c | 4 +-
drivers/usb/host/xhci-ring.c | 6 +++-
drivers/usb/host/xhci.c | 5 +++-
drivers/usb/host/xhci.h | 28 ++++++++++++++++------
5 files changed, 59 insertions(+), 34 deletions(-)

diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 9cde68f..6858a04 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -291,6 +291,7 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
u32 __iomem *port_array[15 + USB_MAXCHILDREN];
int i;
int slot_id;
+ struct xhci_bus_state *bus_state;

ports = HCS_MAX_PORTS(xhci->hcs_params1);
for (i = 0; i < ports; i++) {
@@ -300,6 +301,7 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
port_array[i] =
xhci->usb2_ports[i - xhci->num_usb3_ports];
}
+ bus_state = &xhci->bus_state[hcd_index(hcd)];

spin_lock_irqsave(&xhci->lock, flags);
switch (typeReq) {
@@ -336,10 +338,10 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
if ((temp & PORT_RESET) || !(temp & PORT_PE))
goto error;
if (!DEV_SUPERSPEED(temp) && time_after_eq(jiffies,
- xhci->resume_done[wIndex])) {
+ bus_state->resume_done[wIndex])) {
xhci_dbg(xhci, "Resume USB2 port %d\n",
wIndex + 1);
- xhci->resume_done[wIndex] = 0;
+ bus_state->resume_done[wIndex] = 0;
temp1 = xhci_port_state_to_neutral(temp);
temp1 &= ~PORT_PLS_MASK;
temp1 |= PORT_LINK_STROBE | XDEV_U0;
@@ -354,15 +356,15 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
goto error;
}
xhci_ring_device(xhci, slot_id);
- xhci->port_c_suspend |= 1 << wIndex;
- xhci->suspended_ports &= ~(1 << wIndex);
+ bus_state->port_c_suspend |= 1 << wIndex;
+ bus_state->suspended_ports &= ~(1 << wIndex);
}
}
if ((temp & PORT_PLS_MASK) == XDEV_U0
&& (temp & PORT_POWER)
- && (xhci->suspended_ports & (1 << wIndex))) {
- xhci->suspended_ports &= ~(1 << wIndex);
- xhci->port_c_suspend |= 1 << wIndex;
+ && (bus_state->suspended_ports & (1 << wIndex))) {
+ bus_state->suspended_ports &= ~(1 << wIndex);
+ bus_state->port_c_suspend |= 1 << wIndex;
}
if (temp & PORT_CONNECT) {
status |= USB_PORT_STAT_CONNECTION;
@@ -376,7 +378,7 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
status |= USB_PORT_STAT_RESET;
if (temp & PORT_POWER)
status |= USB_PORT_STAT_POWER;
- if (xhci->port_c_suspend & (1 << wIndex))
+ if (bus_state->port_c_suspend & (1 << wIndex))
status |= 1 << USB_PORT_FEAT_C_SUSPEND;
xhci_dbg(xhci, "Get port status returned 0x%x\n", status);
put_unaligned(cpu_to_le32(status), (__le32 *) buf);
@@ -422,7 +424,7 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
spin_lock_irqsave(&xhci->lock, flags);

temp = xhci_readl(xhci, port_array[wIndex]);
- xhci->suspended_ports |= 1 << wIndex;
+ bus_state->suspended_ports |= 1 << wIndex;
break;
case USB_PORT_FEAT_POWER:
/*
@@ -493,7 +495,7 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
xhci_writel(xhci, temp,
port_array[wIndex]);
}
- xhci->port_c_suspend |= 1 << wIndex;
+ bus_state->port_c_suspend |= 1 << wIndex;
}

slot_id = xhci_find_slot_id_by_port(xhci, wIndex + 1);
@@ -504,7 +506,7 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
xhci_ring_device(xhci, slot_id);
break;
case USB_PORT_FEAT_C_SUSPEND:
- xhci->port_c_suspend &= ~(1 << wIndex);
+ bus_state->port_c_suspend &= ~(1 << wIndex);
case USB_PORT_FEAT_C_RESET:
case USB_PORT_FEAT_C_CONNECTION:
case USB_PORT_FEAT_C_OVER_CURRENT:
@@ -546,6 +548,7 @@ int xhci_hub_status_data(struct usb_hcd *hcd, char *buf)
struct xhci_hcd *xhci = hcd_to_xhci(hcd);
int ports;
u32 __iomem *port_array[15 + USB_MAXCHILDREN];
+ struct xhci_bus_state *bus_state;

ports = HCS_MAX_PORTS(xhci->hcs_params1);
for (i = 0; i < ports; i++) {
@@ -555,6 +558,7 @@ int xhci_hub_status_data(struct usb_hcd *hcd, char *buf)
port_array[i] =
xhci->usb2_ports[i - xhci->num_usb3_ports];
}
+ bus_state = &xhci->bus_state[hcd_index(hcd)];

/* Initial status is no changes */
retval = (ports + 8) / 8;
@@ -568,9 +572,9 @@ int xhci_hub_status_data(struct usb_hcd *hcd, char *buf)
for (i = 0; i < ports; i++) {
temp = xhci_readl(xhci, port_array[i]);
if ((temp & mask) != 0 ||
- (xhci->port_c_suspend & 1 << i) ||
- (xhci->resume_done[i] && time_after_eq(
- jiffies, xhci->resume_done[i]))) {
+ (bus_state->port_c_suspend & 1 << i) ||
+ (bus_state->resume_done[i] && time_after_eq(
+ jiffies, bus_state->resume_done[i]))) {
buf[(i + 1) / 8] |= 1 << (i + 1) % 8;
status = 1;
}
@@ -587,6 +591,7 @@ int xhci_bus_suspend(struct usb_hcd *hcd)
int max_ports, port_index;
u32 __iomem *port_array[15 + USB_MAXCHILDREN];
int i;
+ struct xhci_bus_state *bus_state;
unsigned long flags;

xhci_dbg(xhci, "suspend root hub\n");
@@ -598,13 +603,14 @@ int xhci_bus_suspend(struct usb_hcd *hcd)
port_array[i] =
xhci->usb2_ports[i - xhci->num_usb3_ports];
}
+ bus_state = &xhci->bus_state[hcd_index(hcd)];

spin_lock_irqsave(&xhci->lock, flags);

if (hcd->self.root_hub->do_remote_wakeup) {
port_index = max_ports;
while (port_index--) {
- if (xhci->resume_done[port_index] != 0) {
+ if (bus_state->resume_done[port_index] != 0) {
spin_unlock_irqrestore(&xhci->lock, flags);
xhci_dbg(xhci, "suspend failed because "
"port %d is resuming\n",
@@ -615,7 +621,7 @@ int xhci_bus_suspend(struct usb_hcd *hcd)
}

port_index = max_ports;
- xhci->bus_suspended = 0;
+ bus_state->bus_suspended = 0;
while (port_index--) {
/* suspend the port if the port is not suspended */
u32 t1, t2;
@@ -635,7 +641,7 @@ int xhci_bus_suspend(struct usb_hcd *hcd)
}
t2 &= ~PORT_PLS_MASK;
t2 |= PORT_LINK_STROBE | XDEV_U3;
- set_bit(port_index, &xhci->bus_suspended);
+ set_bit(port_index, &bus_state->bus_suspended);
}
if (hcd->self.root_hub->do_remote_wakeup) {
if (t1 & PORT_CONNECT) {
@@ -667,7 +673,7 @@ int xhci_bus_suspend(struct usb_hcd *hcd)
}
}
hcd->state = HC_STATE_SUSPENDED;
- xhci->next_statechange = jiffies + msecs_to_jiffies(10);
+ bus_state->next_statechange = jiffies + msecs_to_jiffies(10);
spin_unlock_irqrestore(&xhci->lock, flags);
return 0;
}
@@ -678,6 +684,7 @@ int xhci_bus_resume(struct usb_hcd *hcd)
int max_ports, port_index;
u32 __iomem *port_array[15 + USB_MAXCHILDREN];
int i;
+ struct xhci_bus_state *bus_state;
u32 temp;
unsigned long flags;

@@ -690,8 +697,9 @@ int xhci_bus_resume(struct usb_hcd *hcd)
port_array[i] =
xhci->usb2_ports[i - xhci->num_usb3_ports];
}
+ bus_state = &xhci->bus_state[hcd_index(hcd)];

- if (time_before(jiffies, xhci->next_statechange))
+ if (time_before(jiffies, bus_state->next_statechange))
msleep(5);

spin_lock_irqsave(&xhci->lock, flags);
@@ -717,7 +725,7 @@ int xhci_bus_resume(struct usb_hcd *hcd)
temp &= ~(PORT_RWC_BITS | PORT_CEC | PORT_WAKE_BITS);
else
temp &= ~(PORT_RWC_BITS | PORT_WAKE_BITS);
- if (test_bit(port_index, &xhci->bus_suspended) &&
+ if (test_bit(port_index, &bus_state->bus_suspended) &&
(temp & PORT_PLS_MASK)) {
if (DEV_SUPERSPEED(temp)) {
temp = xhci_port_state_to_neutral(temp);
@@ -763,7 +771,7 @@ int xhci_bus_resume(struct usb_hcd *hcd)

(void) xhci_readl(xhci, &xhci->op_regs->command);

- xhci->next_statechange = jiffies + msecs_to_jiffies(5);
+ bus_state->next_statechange = jiffies + msecs_to_jiffies(5);
hcd->state = HC_STATE_RUNNING;
/* re-enable irqs */
temp = xhci_readl(xhci, &xhci->op_regs->command);
diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 2b85667..ce37dd9 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -1452,7 +1452,7 @@ void xhci_mem_cleanup(struct xhci_hcd *xhci)

xhci->page_size = 0;
xhci->page_shift = 0;
- xhci->bus_suspended = 0;
+ xhci->bus_state[0].bus_suspended = 0;
}

static int xhci_test_trb_in_td(struct xhci_hcd *xhci,
@@ -1972,7 +1972,7 @@ int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags)
for (i = 0; i < MAX_HC_SLOTS; ++i)
xhci->devs[i] = NULL;
for (i = 0; i < USB_MAXCHILDREN; ++i)
- xhci->resume_done[i] = 0;
+ xhci->bus_state[0].resume_done[i] = 0;

if (scratchpad_alloc(xhci, flags))
goto fail;
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index a24c6c5..18fc25a 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -1173,7 +1173,9 @@ static void handle_port_status(struct xhci_hcd *xhci,
unsigned int faked_port_index;
u32 __iomem *port_array[15 + USB_MAXCHILDREN];
int i;
+ struct xhci_bus_state *bus_state;

+ bus_state = &xhci->bus_state[0];
/* Port status change events always have a successful completion code */
if (GET_COMP_CODE(event->generic.field[2]) != COMP_SUCCESS) {
xhci_warn(xhci, "WARN: xHC returned failed port status event\n");
@@ -1232,10 +1234,10 @@ static void handle_port_status(struct xhci_hcd *xhci,
xhci_writel(xhci, temp, port_array[faked_port_index]);
} else {
xhci_dbg(xhci, "resume HS port %d\n", port_id);
- xhci->resume_done[port_id - 1] = jiffies +
+ bus_state->resume_done[port_id - 1] = jiffies +
msecs_to_jiffies(20);
mod_timer(&hcd->rh_timer,
- xhci->resume_done[port_id - 1]);
+ bus_state->resume_done[port_id - 1]);
/* Do the rest in GetPortStatus */
}
}
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index dfc0099..64b8197 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -699,7 +699,10 @@ int xhci_resume(struct xhci_hcd *xhci, bool hibernated)
int old_state, retval;

old_state = hcd->state;
- if (time_before(jiffies, xhci->next_statechange))
+ /* Wait a bit if the bus needs to settle from the transistion to
+ * suspend.
+ */
+ if (time_before(jiffies, xhci->bus_state[0].next_statechange))
msleep(100);

spin_lock_irq(&xhci->lock);
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 243d8d9..d6eff49 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1165,6 +1165,22 @@ struct s3_save {
u64 erst_dequeue;
};

+struct xhci_bus_state {
+ unsigned long bus_suspended;
+ unsigned long next_statechange;
+
+ /* Port suspend arrays are indexed by the portnum of the fake roothub */
+ /* ports suspend status arrays - max 31 ports for USB2, 15 for USB3 */
+ u32 port_c_suspend;
+ u32 suspended_ports;
+ unsigned long resume_done[USB_MAXCHILDREN];
+};
+
+static inline unsigned int hcd_index(struct usb_hcd *hcd)
+{
+ return 0;
+}
+
/* There is one ehci_hci structure per controller */
struct xhci_hcd {
struct usb_hcd *main_hcd;
@@ -1229,9 +1245,6 @@ struct xhci_hcd {
/* Host controller watchdog timer structures */
unsigned int xhc_state;

- unsigned long bus_suspended;
- unsigned long next_statechange;
Sarah Sharp
2010-12-30 23:24:27 UTC
Permalink
xhci_find_slot_id_by_port() tries to map the port index to the slot ID for
the USB device. In the future, there will be two xHCI roothubs, and their
port indices will overlap. Therefore, xhci_find_slot_id_by_port() will
need to use information in the roothub's usb_hcd structure to map the port
index and roothub speed to the right slot ID.

Add a new parameter to xhci_find_slot_id_by_port(), in order to pass in
the roothub's usb_hcd structure.

Signed-off-by: Sarah Sharp <sarah.a.sharp-VuQAYsv1563Yd54FQh9/***@public.gmane.org>
---
drivers/usb/host/xhci-hub.c | 16 ++++++++++------
drivers/usb/host/xhci-ring.c | 3 ++-
drivers/usb/host/xhci.h | 3 ++-
3 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 6858a04..3ec5ae2 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -135,7 +135,8 @@ u32 xhci_port_state_to_neutral(u32 state)
/*
* find slot id based on port number.
*/
-int xhci_find_slot_id_by_port(struct xhci_hcd *xhci, u16 port)
+int xhci_find_slot_id_by_port(struct usb_hcd *hcd, struct xhci_hcd *xhci,
+ u16 port)
{
int slot_id;
int i;
@@ -349,7 +350,7 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,

xhci_dbg(xhci, "set port %d resume\n",
wIndex + 1);
- slot_id = xhci_find_slot_id_by_port(xhci,
+ slot_id = xhci_find_slot_id_by_port(hcd, xhci,
wIndex + 1);
if (!slot_id) {
xhci_dbg(xhci, "slot_id is zero\n");
@@ -404,7 +405,8 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
goto error;
}

- slot_id = xhci_find_slot_id_by_port(xhci, wIndex + 1);
+ slot_id = xhci_find_slot_id_by_port(hcd, xhci,
+ wIndex + 1);
if (!slot_id) {
xhci_warn(xhci, "slot_id is zero\n");
goto error;
@@ -498,7 +500,8 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
bus_state->port_c_suspend |= 1 << wIndex;
}

- slot_id = xhci_find_slot_id_by_port(xhci, wIndex + 1);
+ slot_id = xhci_find_slot_id_by_port(hcd, xhci,
+ wIndex + 1);
if (!slot_id) {
xhci_dbg(xhci, "slot_id is zero\n");
goto error;
@@ -632,7 +635,7 @@ int xhci_bus_suspend(struct usb_hcd *hcd)

if ((t1 & PORT_PE) && !(t1 & PORT_PLS_MASK)) {
xhci_dbg(xhci, "port %d not suspended\n", port_index);
- slot_id = xhci_find_slot_id_by_port(xhci,
+ slot_id = xhci_find_slot_id_by_port(hcd, xhci,
port_index + 1);
if (slot_id) {
spin_unlock_irqrestore(&xhci->lock, flags);
@@ -748,7 +751,8 @@ int xhci_bus_resume(struct usb_hcd *hcd)
temp |= PORT_LINK_STROBE | XDEV_U0;
xhci_writel(xhci, temp, port_array[port_index]);
}
- slot_id = xhci_find_slot_id_by_port(xhci, port_index + 1);
+ slot_id = xhci_find_slot_id_by_port(hcd,
+ xhci, port_index + 1);
if (slot_id)
xhci_ring_device(xhci, slot_id);
} else
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 18fc25a..68cb552 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -1220,7 +1220,8 @@ static void handle_port_status(struct xhci_hcd *xhci,
temp &= ~PORT_PLS_MASK;
temp |= PORT_LINK_STROBE | XDEV_U0;
xhci_writel(xhci, temp, port_array[faked_port_index]);
- slot_id = xhci_find_slot_id_by_port(xhci, port_id);
+ slot_id = xhci_find_slot_id_by_port(hcd, xhci,
+ faked_port_index);
if (!slot_id) {
xhci_dbg(xhci, "slot_id is zero\n");
goto cleanup;
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index d6eff49..7f4dfda 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1537,7 +1537,8 @@ int xhci_bus_resume(struct usb_hcd *hcd);
#endif /* CONFIG_PM */

u32 xhci_port_state_to_neutral(u32 state);
-int xhci_find_slot_id_by_port(struct xhci_hcd *xhci, u16 port);
+int xhci_find_slot_id_by_port(struct usb_hcd *hcd, struct xhci_hcd *xhci,
+ u16 port);
void xhci_ring_device(struct xhci_hcd *xhci, int slot_id);

/* xHCI contexts */
--
1.6.3.3

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Sarah Sharp
2010-12-30 23:24:36 UTC
Permalink
This patch changes the xHCI driver to allocate two roothubs. This touches
the driver initialization and shutdown paths, roothub emulation code, and
port status change event handlers. This is a rather large patch, but it
can't be broken up, or it would break git-bisect.

The roothub emulation code is changed to return the correct number of
ports for the two roothubs. For the USB 3.0 roothub, it only reports the
USB 3.0 ports. For the USB 2.0 roothub, it reports all the LS/FS/HS
ports. The code to disable a port now checks the speed of the roothub,
and refuses to disable SuperSpeed ports under the USB 3.0 roothub.

The code for initializing a new device context must be changed to set the
proper roothub port number. Since we've split the xHCI host into two
roothubs, we can't just use the port number in the ancestor hub. Instead,
we loop through the array of hardware port status register speeds and find
the Nth port with a similar speed.

The port status change event handler is updated to figure out whether the
port that reported the change is a USB 3.0 port, or a non-SuperSpeed port.
Once it figures out the port speed, it kicks the proper roothub.

The function to find a slot ID based on the port index is updated to take
into account that the two roothubs will have over-lapping port indices.
It checks that the virtual device with a matching port index is the same
speed as the passed in roothub.

There's also changes to the driver initialization and shutdown paths:

1. Make sure that the xhci_hcd pointer is shared across the two
usb_hcd structures. The xhci_hcd pointer is allocated and the
registers are mapped in when xhci_pci_setup() is called with the
primary HCD. When xhci_pci_setup() is called with the non-primary
HCD, the xhci_hcd pointer is stored.

2. Make sure to set the sg_tablesize for both usb_hcd structures. Set
the PCI DMA mask for the non-primary HCD to allow for 64-bit or 32-bit
DMA. (The PCI DMA mask is set from the primary HCD further down in
the xhci_pci_setup() function.)

3. Ensure that the host controller doesn't start kicking khubd in
response to port status changes before both usb_hcd structures are
registered. xhci_run() only starts the xHC running once it has been
called with the non-primary roothub. Similarly, the xhci_stop()
function only halts the host controller when it is called with the
non-primary HCD. Then on the second call, it resets and cleans up the
MSI-X irqs.

When there's an issue that halts the xHCI host controller, we need to set
the hcd->state to HC_STATE_HALT for both shared roothubs (and possibly let
the USB core know *both* died). Make sure to let the USB core know the
non-primary host controller died first, because xhci_stop() and other
deallocation code assumes that the non-primary host controller will be
deallocated first.

Signed-off-by: Sarah Sharp <sarah.a.sharp-VuQAYsv1563Yd54FQh9/***@public.gmane.org>
---
drivers/usb/host/xhci-hub.c | 97 ++++++++++++++++++++++-------------------
drivers/usb/host/xhci-mem.c | 66 +++++++++++++++++++++++++++--
drivers/usb/host/xhci-pci.c | 28 +++++++++---
drivers/usb/host/xhci-ring.c | 90 +++++++++++++++++++++++++++++++++------
drivers/usb/host/xhci.c | 57 ++++++++++++++++++++-----
drivers/usb/host/xhci.h | 11 +++--
6 files changed, 264 insertions(+), 85 deletions(-)

diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 3ec5ae2..da64a3d 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -28,13 +28,20 @@
#define PORT_RWC_BITS (PORT_CSC | PORT_PEC | PORT_WRC | PORT_OCC | \
PORT_RC | PORT_PLC | PORT_PE)

-static void xhci_hub_descriptor(struct xhci_hcd *xhci,
+static void xhci_hub_descriptor(struct usb_hcd *hcd, struct xhci_hcd *xhci,
struct usb_hub_descriptor *desc)
{
int ports;
u16 temp;

- ports = HCS_MAX_PORTS(xhci->hcs_params1);
+ if (hcd->bcdUSB == HCD_USB3)
+ ports = xhci->num_usb3_ports;
+ else
+ ports = xhci->num_usb2_ports;
+
+ /* FIXME: return a USB 3.0 hub descriptor if this request was for the
+ * USB3 roothub.
+ */

/* USB 3.0 hubs have a different descriptor, but we fake this for now */
desc->bDescriptorType = 0x29;
@@ -134,18 +141,22 @@ u32 xhci_port_state_to_neutral(u32 state)

/*
* find slot id based on port number.
+ * @port: The one-based port number from one of the two split roothubs.
*/
int xhci_find_slot_id_by_port(struct usb_hcd *hcd, struct xhci_hcd *xhci,
u16 port)
{
int slot_id;
int i;
+ enum usb_device_speed speed;

slot_id = 0;
for (i = 0; i < MAX_HC_SLOTS; i++) {
if (!xhci->devs[i])
continue;
- if (xhci->devs[i]->port == port) {
+ speed = xhci->devs[i]->udev->speed;
+ if (((speed == USB_SPEED_SUPER) == (hcd->bcdUSB == HCD_USB3))
+ && xhci->devs[i]->port == port) {
slot_id = i;
break;
}
@@ -226,11 +237,11 @@ void xhci_ring_device(struct xhci_hcd *xhci, int slot_id)
return;
}

-static void xhci_disable_port(struct xhci_hcd *xhci, u16 wIndex,
- u32 __iomem *addr, u32 port_status)
+static void xhci_disable_port(struct usb_hcd *hcd, struct xhci_hcd *xhci,
+ u16 wIndex, u32 __iomem *addr, u32 port_status)
{
/* Don't allow the USB core to disable SuperSpeed ports. */
- if (xhci->port_array[wIndex] == 0x03) {
+ if (hcd->bcdUSB == HCD_USB3) {
xhci_dbg(xhci, "Ignoring request to disable "
"SuperSpeed port.\n");
return;
@@ -289,18 +300,16 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
unsigned long flags;
u32 temp, temp1, status;
int retval = 0;
- u32 __iomem *port_array[15 + USB_MAXCHILDREN];
- int i;
+ u32 __iomem **port_array;
int slot_id;
struct xhci_bus_state *bus_state;

- ports = HCS_MAX_PORTS(xhci->hcs_params1);
- for (i = 0; i < ports; i++) {
- if (i < xhci->num_usb3_ports)
- port_array[i] = xhci->usb3_ports[i];
- else
- port_array[i] =
- xhci->usb2_ports[i - xhci->num_usb3_ports];
+ if (hcd->bcdUSB == HCD_USB3) {
+ ports = xhci->num_usb3_ports;
+ port_array = xhci->usb3_ports;
+ } else {
+ ports = xhci->num_usb2_ports;
+ port_array = xhci->usb2_ports;
}
bus_state = &xhci->bus_state[hcd_index(hcd)];

@@ -311,7 +320,8 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
memset(buf, 0, 4);
break;
case GetHubDescriptor:
- xhci_hub_descriptor(xhci, (struct usb_hub_descriptor *) buf);
+ xhci_hub_descriptor(hcd, xhci,
+ (struct usb_hub_descriptor *) buf);
break;
case GetPortStatus:
if (!wIndex || wIndex > ports)
@@ -518,7 +528,7 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
port_array[wIndex], temp);
break;
case USB_PORT_FEAT_ENABLE:
- xhci_disable_port(xhci, wIndex,
+ xhci_disable_port(hcd, xhci, wIndex,
port_array[wIndex], temp);
break;
default:
@@ -550,16 +560,15 @@ int xhci_hub_status_data(struct usb_hcd *hcd, char *buf)
int i, retval;
struct xhci_hcd *xhci = hcd_to_xhci(hcd);
int ports;
- u32 __iomem *port_array[15 + USB_MAXCHILDREN];
+ u32 __iomem **port_array;
struct xhci_bus_state *bus_state;

- ports = HCS_MAX_PORTS(xhci->hcs_params1);
- for (i = 0; i < ports; i++) {
- if (i < xhci->num_usb3_ports)
- port_array[i] = xhci->usb3_ports[i];
- else
- port_array[i] =
- xhci->usb2_ports[i - xhci->num_usb3_ports];
+ if (hcd->bcdUSB == HCD_USB3) {
+ ports = xhci->num_usb3_ports;
+ port_array = xhci->usb3_ports;
+ } else {
+ ports = xhci->num_usb2_ports;
+ port_array = xhci->usb2_ports;
}
bus_state = &xhci->bus_state[hcd_index(hcd)];

@@ -592,19 +601,18 @@ int xhci_bus_suspend(struct usb_hcd *hcd)
{
struct xhci_hcd *xhci = hcd_to_xhci(hcd);
int max_ports, port_index;
- u32 __iomem *port_array[15 + USB_MAXCHILDREN];
- int i;
+ u32 __iomem **port_array;
struct xhci_bus_state *bus_state;
unsigned long flags;

- xhci_dbg(xhci, "suspend root hub\n");
- max_ports = HCS_MAX_PORTS(xhci->hcs_params1);
- for (i = 0; i < max_ports; i++) {
- if (i < xhci->num_usb3_ports)
- port_array[i] = xhci->usb3_ports[i];
- else
- port_array[i] =
- xhci->usb2_ports[i - xhci->num_usb3_ports];
+ if (hcd->bcdUSB == HCD_USB3) {
+ max_ports = xhci->num_usb3_ports;
+ port_array = xhci->usb3_ports;
+ xhci_dbg(xhci, "suspend USB 3.0 root hub\n");
+ } else {
+ max_ports = xhci->num_usb2_ports;
+ port_array = xhci->usb2_ports;
+ xhci_dbg(xhci, "suspend USB 2.0 root hub\n");
}
bus_state = &xhci->bus_state[hcd_index(hcd)];

@@ -685,20 +693,19 @@ int xhci_bus_resume(struct usb_hcd *hcd)
{
struct xhci_hcd *xhci = hcd_to_xhci(hcd);
int max_ports, port_index;
- u32 __iomem *port_array[15 + USB_MAXCHILDREN];
- int i;
+ u32 __iomem **port_array;
struct xhci_bus_state *bus_state;
u32 temp;
unsigned long flags;

- xhci_dbg(xhci, "resume root hub\n");
- max_ports = HCS_MAX_PORTS(xhci->hcs_params1);
- for (i = 0; i < max_ports; i++) {
- if (i < xhci->num_usb3_ports)
- port_array[i] = xhci->usb3_ports[i];
- else
- port_array[i] =
- xhci->usb2_ports[i - xhci->num_usb3_ports];
+ if (hcd->bcdUSB == HCD_USB3) {
+ max_ports = xhci->num_usb3_ports;
+ port_array = xhci->usb3_ports;
+ xhci_dbg(xhci, "resume USB 3.0 root hub\n");
+ } else {
+ max_ports = xhci->num_usb2_ports;
+ port_array = xhci->usb2_ports;
+ xhci_dbg(xhci, "resume USB 2.0 root hub\n");
}
bus_state = &xhci->bus_state[hcd_index(hcd)];

diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index ce37dd9..76e4b95 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -814,14 +814,64 @@ void xhci_copy_ep0_dequeue_into_input_ctx(struct xhci_hcd *xhci,
ep0_ctx->deq |= ep_ring->cycle_state;
}

+/*
+ * The xHCI roothub may have ports of differing speeds in any order in the port
+ * status registers. xhci->port_array provides an array of the port speed for
+ * each offset into the port status registers.
+ *
+ * The xHCI hardware wants to know the roothub port number that the USB device
+ * is attached to (or the roothub port its ancestor hub is attached to). All we
+ * know is the index of that port under either the USB 2.0 or the USB 3.0
+ * roothub, but that doesn't give us the real index into the HW port status
+ * registers. Scan through the xHCI roothub port array, looking for the Nth
+ * entry of the correct port speed. Return the port number of that entry.
+ */
+static u32 xhci_find_real_port_number(struct xhci_hcd *xhci,
+ struct usb_device *udev)
+{
+ struct usb_device *top_dev;
+ unsigned int num_similar_speed_ports;
+ unsigned int faked_port_num;
+ int i;
+
+ for (top_dev = udev; top_dev->parent && top_dev->parent->parent;
+ top_dev = top_dev->parent)
+ /* Found device below root hub */;
+ faked_port_num = top_dev->portnum;
+ for (i = 0, num_similar_speed_ports = 0;
+ i < HCS_MAX_PORTS(xhci->hcs_params1); i++) {
+ u8 port_speed = xhci->port_array[i];
+
+ /*
+ * Skip ports that don't have known speeds, or have duplicate
+ * Extended Capabilities port speed entries.
+ */
+ if (port_speed == 0 || port_speed == -1)
+ continue;
+
+ /*
+ * USB 3.0 ports are always under a USB 3.0 hub. USB 2.0 and
+ * 1.1 ports are under the USB 2.0 hub. If the port speed
+ * matches the device speed, it's a similar speed port.
+ */
+ if ((port_speed == 0x03) == (udev->speed == USB_SPEED_SUPER))
+ num_similar_speed_ports++;
+ if (num_similar_speed_ports == faked_port_num)
+ /* Roothub ports are numbered from 1 to N */
+ return i+1;
+ }
+ return 0;
+}
+
/* Setup an xHCI virtual device for a Set Address command */
int xhci_setup_addressable_virt_dev(struct xhci_hcd *xhci, struct usb_device *udev)
{
struct xhci_virt_device *dev;
struct xhci_ep_ctx *ep0_ctx;
- struct usb_device *top_dev;
struct xhci_slot_ctx *slot_ctx;
struct xhci_input_control_ctx *ctrl_ctx;
+ u32 port_num;
+ struct usb_device *top_dev;

dev = xhci->devs[udev->slot_id];
/* Slot ID 0 is reserved */
@@ -863,12 +913,17 @@ int xhci_setup_addressable_virt_dev(struct xhci_hcd *xhci, struct usb_device *ud
BUG();
}
/* Find the root hub port this device is under */
+ port_num = xhci_find_real_port_number(xhci, udev);
+ if (!port_num)
+ return -EINVAL;
+ slot_ctx->dev_info2 |= (u32) ROOT_HUB_PORT(port_num);
+ /* Set the port number in the virtual_device to the faked port number */
for (top_dev = udev; top_dev->parent && top_dev->parent->parent;
top_dev = top_dev->parent)
/* Found device below root hub */;
- slot_ctx->dev_info2 |= (u32) ROOT_HUB_PORT(top_dev->portnum);
dev->port = top_dev->portnum;
- xhci_dbg(xhci, "Set root hub portnum to %d\n", top_dev->portnum);
+ xhci_dbg(xhci, "Set root hub portnum to %d\n", port_num);
+ xhci_dbg(xhci, "Set fake root hub portnum to %d\n", dev->port);

/* Is this a LS/FS device under an external HS hub? */
if ((udev->speed == USB_SPEED_LOW || udev->speed == USB_SPEED_FULL) &&
@@ -1453,6 +1508,7 @@ void xhci_mem_cleanup(struct xhci_hcd *xhci)
xhci->page_size = 0;
xhci->page_shift = 0;
xhci->bus_state[0].bus_suspended = 0;
+ xhci->bus_state[1].bus_suspended = 0;
}

static int xhci_test_trb_in_td(struct xhci_hcd *xhci,
@@ -1971,8 +2027,10 @@ int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags)
init_completion(&xhci->addr_dev);
for (i = 0; i < MAX_HC_SLOTS; ++i)
xhci->devs[i] = NULL;
- for (i = 0; i < USB_MAXCHILDREN; ++i)
+ for (i = 0; i < USB_MAXCHILDREN; ++i) {
xhci->bus_state[0].resume_done[i] = 0;
+ xhci->bus_state[1].resume_done[i] = 0;
+ }

if (scratchpad_alloc(xhci, flags))
goto fail;
diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index 0090828..69c235f 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -50,18 +50,32 @@ static int xhci_pci_reinit(struct xhci_hcd *xhci, struct pci_dev *pdev)
/* called during probe() after chip reset completes */
static int xhci_pci_setup(struct usb_hcd *hcd)
{
- struct xhci_hcd *xhci = hcd_to_xhci(hcd);
+ struct xhci_hcd *xhci;
struct pci_dev *pdev = to_pci_dev(hcd->self.controller);
int retval;
u32 temp;

hcd->self.sg_tablesize = TRBS_PER_SEGMENT - 2;

- xhci = kzalloc(sizeof(struct xhci_hcd), GFP_KERNEL);
- if (!xhci)
- return -ENOMEM;
- *((struct xhci_hcd **) hcd->hcd_priv) = xhci;
- xhci->main_hcd = hcd;
+ if (usb_hcd_is_primary_hcd(hcd)) {
+ xhci = kzalloc(sizeof(struct xhci_hcd), GFP_KERNEL);
+ if (!xhci)
+ return -ENOMEM;
+ *((struct xhci_hcd **) hcd->hcd_priv) = xhci;
+ xhci->main_hcd = hcd;
+ } else {
+ xhci = hcd_to_xhci(hcd->shared_hcd);
+ *((struct xhci_hcd **) hcd->hcd_priv) = xhci;
+
+ temp = xhci_readl(xhci, &xhci->cap_regs->hcc_params);
+ if (HCC_64BIT_ADDR(temp)) {
+ xhci_dbg(xhci, "Enabling 64-bit DMA addresses.\n");
+ dma_set_mask(hcd->self.controller, DMA_BIT_MASK(64));
+ } else {
+ dma_set_mask(hcd->self.controller, DMA_BIT_MASK(32));
+ }
+ return 0;
+ }

xhci->cap_regs = hcd->regs;
xhci->op_regs = hcd->regs +
@@ -170,7 +184,7 @@ static const struct hc_driver xhci_pci_hc_driver = {
* generic hardware linkage
*/
.irq = xhci_irq,
- .flags = HCD_MEMORY | HCD_USB3,
+ .flags = HCD_MEMORY | HCD_USB3 | HCD_SHARED,

/*
* basic lifecycle operations
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 68cb552..ebb7033 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -872,8 +872,10 @@ void xhci_stop_endpoint_command_watchdog(unsigned long arg)
}
spin_unlock(&xhci->lock);
xhci_to_hcd(xhci)->state = HC_STATE_HALT;
+ xhci_to_hcd(xhci)->shared_hcd->state = HC_STATE_HALT;
xhci_dbg(xhci, "Calling usb_hc_died()\n");
- usb_hc_died(xhci_to_hcd(xhci));
+ usb_hc_died(xhci_to_hcd(xhci)->primary_hcd->shared_hcd);
+ usb_hc_died(xhci_to_hcd(xhci)->primary_hcd);
xhci_dbg(xhci, "xHCI host controller is dead.\n");
}

@@ -1162,20 +1164,53 @@ static void handle_vendor_event(struct xhci_hcd *xhci,
handle_cmd_completion(xhci, &event->event_cmd);
}

+/* @port_id: the one-based port ID from the hardware (indexed from array of all
+ * port registers -- USB 3.0 and USB 2.0).
+ */
+static unsigned int find_faked_portnum_from_hw_portnum(struct usb_hcd *hcd,
+ struct xhci_hcd *xhci, u32 port_id)
+{
+ unsigned int i;
+ unsigned int num_similar_speed_ports = 0;
+
+ /* port_id from the hardware is 1-based, but port_array[], usb3_ports[],
+ * and usb2_ports are 0-based indexes. Count the number of similar
+ * speed ports, up to 1 port before this port.
+ */
+ for (i = 0; i < (port_id - 1); i++) {
+ u8 port_speed = xhci->port_array[i];
+
+ /*
+ * Skip ports that don't have known speeds, or have duplicate
+ * Extended Capabilities port speed entries.
+ */
+ if (port_speed == 0 || port_speed == -1)
+ continue;
+
+ /*
+ * USB 3.0 ports are always under a USB 3.0 hub. USB 2.0 and
+ * 1.1 ports are under the USB 2.0 hub. If the port speed
+ * matches the device speed, it's a similar speed port.
+ */
+ if ((port_speed == 0x03) == (hcd->bcdUSB == HCD_USB3))
+ num_similar_speed_ports++;
+ }
+ return num_similar_speed_ports;
+}
+
static void handle_port_status(struct xhci_hcd *xhci,
union xhci_trb *event)
{
- struct usb_hcd *hcd = xhci_to_hcd(xhci);
+ struct usb_hcd *hcd;
u32 port_id;
u32 temp, temp1;
int max_ports;
int slot_id;
unsigned int faked_port_index;
- u32 __iomem *port_array[15 + USB_MAXCHILDREN];
- int i;
+ u8 major_revision;
struct xhci_bus_state *bus_state;
+ u32 __iomem **port_array;

- bus_state = &xhci->bus_state[0];
/* Port status change events always have a successful completion code */
if (GET_COMP_CODE(event->generic.field[2]) != COMP_SUCCESS) {
xhci_warn(xhci, "WARN: xHC returned failed port status event\n");
@@ -1190,15 +1225,43 @@ static void handle_port_status(struct xhci_hcd *xhci,
goto cleanup;
}

- for (i = 0; i < max_ports; i++) {
- if (i < xhci->num_usb3_ports)
- port_array[i] = xhci->usb3_ports[i];
- else
- port_array[i] =
- xhci->usb2_ports[i - xhci->num_usb3_ports];
+ /* Figure out which usb_hcd this port is attached to:
+ * is it a USB 3.0 port or a USB 2.0/1.1 port?
+ */
+ major_revision = xhci->port_array[port_id - 1];
+ if (major_revision == 0) {
+ xhci_warn(xhci, "Event for port %u not in "
+ "Extended Capabilities, ignoring.\n",
+ port_id);
+ goto cleanup;
}
+ if (major_revision == (u8) -1) {
+ xhci_warn(xhci, "Event for port %u duplicated in"
+ "Extended Capabilities, ignoring.\n",
+ port_id);
+ goto cleanup;
+ }
+
+ /*
+ * Hardware port IDs reported by a Port Status Change Event include USB
+ * 3.0 and USB 2.0 ports. We want to check if the port has reported a
+ * resume event, but we first need to translate the hardware port ID
+ * into the index into the ports on the correct split roothub, and the
+ * correct bus_state structure.
+ */
+ /* Find the right roothub. */
+ hcd = xhci_to_hcd(xhci);
+ if ((major_revision == 0x03) != (hcd->bcdUSB == HCD_USB3))
+ hcd = xhci_to_hcd(xhci)->shared_hcd;
+ bus_state = &xhci->bus_state[hcd_index(hcd)];
+ if (hcd->bcdUSB == HCD_USB3)
+ port_array = xhci->usb3_ports;
+ else
+ port_array = xhci->usb2_ports;
+ /* Find the faked port hub number */
+ faked_port_index = find_faked_portnum_from_hw_portnum(hcd, xhci,
+ port_id);

- faked_port_index = port_id;
temp = xhci_readl(xhci, port_array[faked_port_index]);
if (hcd->state == HC_STATE_SUSPENDED) {
xhci_dbg(xhci, "resume root hub\n");
@@ -1249,7 +1312,7 @@ cleanup:

spin_unlock(&xhci->lock);
/* Pass this up to the core */
- usb_hcd_poll_rh_status(xhci_to_hcd(xhci));
+ usb_hcd_poll_rh_status(hcd);
spin_lock(&xhci->lock);
}

@@ -2134,6 +2197,7 @@ irqreturn_t xhci_irq(struct usb_hcd *hcd)
xhci_halt(xhci);
hw_died:
xhci_to_hcd(xhci)->state = HC_STATE_HALT;
+ xhci_to_hcd(xhci)->shared_hcd->state = HC_STATE_HALT;
spin_unlock(&xhci->lock);
return -ESHUTDOWN;
}
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 64b8197..7ebf9fb 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -378,6 +378,21 @@ void xhci_event_ring_work(unsigned long arg)
}
#endif

+static int xhci_run_finished(struct xhci_hcd *xhci)
+{
+ if (xhci_start(xhci)) {
+ xhci_halt(xhci);
+ return -ENODEV;
+ }
+ xhci_to_hcd(xhci)->shared_hcd->state = HC_STATE_RUNNING;
+
+ if (xhci->quirks & XHCI_NEC_HOST)
+ xhci_ring_cmd_db(xhci);
+
+ xhci_dbg(xhci, "Finished xhci_run for USB2 roothub\n");
+ return 0;
+}
+
/*
* Start the HC after it was halted.
*
@@ -398,7 +413,13 @@ int xhci_run(struct usb_hcd *hcd)
struct xhci_hcd *xhci = hcd_to_xhci(hcd);
struct pci_dev *pdev = to_pci_dev(xhci_to_hcd(xhci)->self.controller);

+ /* Start the xHCI host controller running only after the USB 2.0 roothub
+ * is setup.
+ */
+
hcd->uses_new_polling = 1;
+ if (!usb_hcd_is_primary_hcd(hcd))
+ return xhci_run_finished(xhci);

xhci_dbg(xhci, "xhci_run\n");
/* unregister the legacy interrupt */
@@ -473,16 +494,18 @@ int xhci_run(struct usb_hcd *hcd)
xhci_queue_vendor_command(xhci, 0, 0, 0,
TRB_TYPE(TRB_NEC_GET_FW));

- if (xhci_start(xhci)) {
- xhci_halt(xhci);
- return -ENODEV;
- }
+ xhci_dbg(xhci, "Finished xhci_run for USB3 roothub\n");
+ return 0;
+}

- if (xhci->quirks & XHCI_NEC_HOST)
- xhci_ring_cmd_db(xhci);
+static void xhci_only_stop_hcd(struct usb_hcd *hcd)
+{
+ struct xhci_hcd *xhci = hcd_to_xhci(hcd);

- xhci_dbg(xhci, "Finished xhci_run\n");
- return 0;
+ spin_lock_irq(&xhci->lock);
+ xhci_halt(xhci);
+ xhci_to_hcd(xhci)->shared_hcd->state = HC_STATE_HALT;
+ spin_unlock_irq(&xhci->lock);
}

/*
@@ -499,7 +522,15 @@ void xhci_stop(struct usb_hcd *hcd)
u32 temp;
struct xhci_hcd *xhci = hcd_to_xhci(hcd);

+ if (!usb_hcd_is_primary_hcd(hcd)) {
+ xhci_only_stop_hcd(hcd->shared_hcd);
+ return;
+ }
+
spin_lock_irq(&xhci->lock);
+ /* Make sure the xHC is halted for a USB3 roothub
+ * (xhci_stop() could be called as part of failed init).
+ */
xhci_halt(xhci);
xhci_reset(xhci);
spin_unlock_irq(&xhci->lock);
@@ -532,6 +563,8 @@ void xhci_stop(struct usb_hcd *hcd)
* This is called when the machine is rebooting or halting. We assume that the
* machine will be powered off, and the HC's internal state will be reset.
* Don't bother to free memory.
+ *
+ * This will only ever be called with the main usb_hcd (the USB3 roothub).
*/
void xhci_shutdown(struct usb_hcd *hcd)
{
@@ -699,10 +732,12 @@ int xhci_resume(struct xhci_hcd *xhci, bool hibernated)
int old_state, retval;

old_state = hcd->state;
- /* Wait a bit if the bus needs to settle from the transistion to
- * suspend.
+ /* Wait a bit if either of the roothubs need to settle from the
+ * transistion into bus suspend.
*/
- if (time_before(jiffies, xhci->bus_state[0].next_statechange))
+ if (time_before(jiffies, xhci->bus_state[0].next_statechange) ||
+ time_before(jiffies,
+ xhci->bus_state[1].next_statechange))
msleep(100);

spin_lock_irq(&xhci->lock);
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 7f4dfda..9440d73 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1178,7 +1178,10 @@ struct xhci_bus_state {

static inline unsigned int hcd_index(struct usb_hcd *hcd)
{
- return 0;
+ if (hcd->bcdUSB == HCD_USB3)
+ return 0;
+ else
+ return 1;
}

/* There is one ehci_hci structure per controller */
@@ -1266,10 +1269,8 @@ struct xhci_hcd {
#define XHCI_LINK_TRB_QUIRK (1 << 0)
#define XHCI_RESET_EP_QUIRK (1 << 1)
#define XHCI_NEC_HOST (1 << 2)
- /* There's only one roothub to keep track of bus suspend info for
- * (right now).
- */
- struct xhci_bus_state bus_state[1];
+ /* There are two roothubs to keep track of bus suspend info for */
+ struct xhci_bus_state bus_state[2];
/* Is each xHCI roothub port a USB 3.0, USB 2.0, or USB 1.1 port? */
u8 *port_array;
/* Array of pointers to USB 3.0 PORTSC registers */
--
1.6.3.3

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Alan Stern
2011-01-01 03:30:49 UTC
Permalink
Post by Sarah Sharp
This patch changes the xHCI driver to allocate two roothubs. This touches
the driver initialization and shutdown paths, roothub emulation code, and
port status change event handlers. This is a rather large patch, but it
can't be broken up, or it would break git-bisect.
Instead of relying on usb_hcd_pci_probe() to register both root hubs,
you should use it only for the first (primary) bus. The second bus you
should register by calling usb_create_shared_hcd() directly. After
all, with the secondary root hub you get no benefit from the PCI
services provided by usb_hcd_pci_probe().

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Sarah Sharp
2010-12-30 23:24:43 UTC
Permalink
Return the correct xHCI roothub descriptor, based on whether the roothub
is marked as USB 3.0 or USB 2.0 in usb_hcd->bcdUSB. Fill in
DeviceRemovable for the USB 2.0 and USB 3.0 roothub descriptors, using the
Device Removable bit in the port status and control registers. xHCI is
the first host controller to actually properly set these bits (other hosts
say all devices are removable).

When userspace asks for a USB 2.0-style hub descriptor for the USB 3.0
roothub, stall the endpoint. This is what real external USB 3.0 hubs do,
and we don't want to return a descriptor that userspace didn't ask for.

The USB core is already fixed to always ask for USB 3.0-style hub
descriptors. Only usbfs (typically lsusb) will ask for the USB 2.0-style
hub descriptors. This has already been fixed in usbutils version 0.91,
but the kernel needs to deal with older usbutils versions.

Signed-off-by: Sarah Sharp <sarah.a.sharp-VuQAYsv1563Yd54FQh9/***@public.gmane.org>
---
drivers/usb/host/xhci-hub.c | 134 ++++++++++++++++++++++++++++++++++++-------
1 files changed, 114 insertions(+), 20 deletions(-)

diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index da64a3d..744eabd 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -28,33 +28,15 @@
#define PORT_RWC_BITS (PORT_CSC | PORT_PEC | PORT_WRC | PORT_OCC | \
PORT_RC | PORT_PLC | PORT_PE)

-static void xhci_hub_descriptor(struct usb_hcd *hcd, struct xhci_hcd *xhci,
- struct usb_hub_descriptor *desc)
+static void xhci_common_hub_descriptor(struct xhci_hcd *xhci,
+ struct usb_hub_descriptor *desc, int ports)
{
- int ports;
u16 temp;

- if (hcd->bcdUSB == HCD_USB3)
- ports = xhci->num_usb3_ports;
- else
- ports = xhci->num_usb2_ports;
-
- /* FIXME: return a USB 3.0 hub descriptor if this request was for the
- * USB3 roothub.
- */
-
- /* USB 3.0 hubs have a different descriptor, but we fake this for now */
- desc->bDescriptorType = 0x29;
desc->bPwrOn2PwrGood = 10; /* xhci section 5.4.9 says 20ms max */
desc->bHubContrCurrent = 0;

desc->bNbrPorts = ports;
- temp = 1 + (ports / 8);
- desc->bDescLength = 7 + 2 * temp;
-
- memset(&desc->u.hs.DeviceRemovable[0], 0, temp);
- memset(&desc->u.hs.DeviceRemovable[temp], 0xff, temp);
-
/* Ugh, these should be #defines, FIXME */
/* Using table 11-13 in USB 2.0 spec. */
temp = 0;
@@ -71,6 +53,102 @@ static void xhci_hub_descriptor(struct usb_hcd *hcd, struct xhci_hcd *xhci,
desc->wHubCharacteristics = (__force __u16) cpu_to_le16(temp);
}

+/* Fill in the USB 2.0 roothub descriptor */
+static void xhci_usb2_hub_descriptor(struct usb_hcd *hcd, struct xhci_hcd *xhci,
+ struct usb_hub_descriptor *desc)
+{
+ int ports;
+ u16 temp;
+ __u8 port_removable[(USB_MAXCHILDREN + 1 + 7) / 8];
+ u32 portsc;
+ unsigned int i;
+
+ ports = xhci->num_usb2_ports;
+
+ xhci_common_hub_descriptor(xhci, desc, ports);
+ desc->bDescriptorType = 0x29;
+ temp = 1 + (ports / 8);
+ desc->bDescLength = 7 + 2 * temp;
+
+ /* The Device Removable bits are reported on a byte granularity.
+ * If the port doesn't exist within that byte, the bit is set to 0.
+ */
+ memset(port_removable, 0, sizeof(port_removable));
+ for (i = 0; i < ports; i++) {
+ portsc = xhci_readl(xhci, xhci->usb3_ports[i]);
+ /* If a device is removable, PORTSC reports a 0, same as in the
+ * hub descriptor DeviceRemovable bits.
+ */
+ if (portsc & PORT_DEV_REMOVE)
+ /* This math is hairy because bit 0 of DeviceRemovable
+ * is reserved, and bit 1 is for port 1, etc.
+ */
+ port_removable[(i + 1) / 8] |= 1 << ((i + 1) % 8);
+ }
+
+ /* ch11.h defines a hub descriptor that has room for USB_MAXCHILDREN
+ * ports on it. The USB 2.0 specification says that there are two
+ * variable length fields at the end of the hub descriptor:
+ * DeviceRemovable and PortPwrCtrlMask. But since we can have less than
+ * USB_MAXCHILDREN ports, we may need to use the DeviceRemovable array
+ * to set PortPwrCtrlMask bits. PortPwrCtrlMask must always be set to
+ * 0xFF, so we initialize the both arrays (DeviceRemovable and
+ * PortPwrCtrlMask) to 0xFF. Then we set the DeviceRemovable for each
+ * set of ports that actually exist.
+ */
+ memset(desc->u.hs.DeviceRemovable, 0xff,
+ sizeof(desc->u.hs.DeviceRemovable));
+ memset(desc->u.hs.PortPwrCtrlMask, 0xff,
+ sizeof(desc->u.hs.PortPwrCtrlMask));
+
+ for (i = 0; i < (ports + 1 + 7) / 8; i++)
+ memset(&desc->u.hs.DeviceRemovable[i], port_removable[i],
+ sizeof(__u8));
+}
+
+/* Fill in the USB 3.0 roothub descriptor */
+static void xhci_usb3_hub_descriptor(struct usb_hcd *hcd, struct xhci_hcd *xhci,
+ struct usb_hub_descriptor *desc)
+{
+ int ports;
+ u16 port_removable;
+ u32 portsc;
+ unsigned int i;
+
+ ports = xhci->num_usb3_ports;
+ xhci_common_hub_descriptor(xhci, desc, ports);
+ desc->bDescriptorType = 0x2a;
+ desc->bDescLength = 12;
+
+ /* header decode latency should be zero for roothubs,
+ * see section 4.23.5.2.
+ */
+ desc->u.ss.bHubHdrDecLat = 0;
+ desc->u.ss.wHubDelay = 0;
+
+ port_removable = 0;
+ /* bit 0 is reserved, bit 1 is for port 1, etc. */
+ for (i = 0; i < ports; i++) {
+ portsc = xhci_readl(xhci, xhci->usb3_ports[i]);
+ if (portsc & PORT_DEV_REMOVE)
+ port_removable |= 1 << (i + 1);
+ }
+ memset(&desc->u.ss.DeviceRemovable,
+ (__force __u16) cpu_to_le16(port_removable),
+ sizeof(__u16));
+}
+
+static void xhci_hub_descriptor(struct usb_hcd *hcd, struct xhci_hcd *xhci,
+ struct usb_hub_descriptor *desc)
+{
+
+ if (hcd->bcdUSB == HCD_USB3)
+ xhci_usb3_hub_descriptor(hcd, xhci, desc);
+ else
+ xhci_usb2_hub_descriptor(hcd, xhci, desc);
+
+}
+
static unsigned int xhci_port_speed(unsigned int port_status)
{
if (DEV_LOWSPEED(port_status))
@@ -320,6 +398,17 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
memset(buf, 0, 4);
break;
case GetHubDescriptor:
+ /* Check to make sure userspace is asking for the USB 3.0 hub
+ * descriptor for the USB 3.0 roothub. If not, we stall the
+ * endpoint, like external hubs do.
+ */
+ if (hcd->bcdUSB == HCD_USB3 &&
+ (wLength < USB_DT_SS_HUB_SIZE ||
+ wValue != (USB_DT_SS_HUB << 8))) {
+ xhci_dbg(xhci, "Wrong hub descriptor type for "
+ "USB 3.0 roothub.\n");
+ goto error;
+ }
xhci_hub_descriptor(hcd, xhci,
(struct usb_hub_descriptor *) buf);
break;
@@ -331,6 +420,9 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
temp = xhci_readl(xhci, port_array[wIndex]);
xhci_dbg(xhci, "get port status, actual port %d status = 0x%x\n", wIndex, temp);

+ /* FIXME - should we return a port status value like the USB
+ * 3.0 external hubs do?
+ */
/* wPortChange bits */
if (temp & PORT_CSC)
status |= USB_PORT_STAT_C_CONNECTION << 16;
@@ -401,6 +493,7 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
wIndex--;
temp = xhci_readl(xhci, port_array[wIndex]);
temp = xhci_port_state_to_neutral(temp);
+ /* FIXME: What new port features do we need to support? */
switch (wValue) {
case USB_PORT_FEAT_SUSPEND:
temp = xhci_readl(xhci, port_array[wIndex]);
@@ -469,6 +562,7 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
goto error;
wIndex--;
temp = xhci_readl(xhci, port_array[wIndex]);
+ /* FIXME: What new port features do we need to support? */
temp = xhci_port_state_to_neutral(temp);
switch (wValue) {
case USB_PORT_FEAT_SUSPEND:
--
1.6.3.3

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Sarah Sharp
2010-12-30 23:27:51 UTC
Permalink
The USB core allocates a USB 2.0 roothub descriptor that has room for 31
(USB_MAXCHILDREN) ports' worth of DeviceRemovable and PortPwrCtrlMask
fields. Limit the number of USB 2.0 roothub ports accordingly. I don't
expect to run into this limitation ever, but this prevents a buffer
overflow issue in the roothub descriptor filling code.

Similarly, a USB 3.0 hub can only have 15 downstream ports, so limit the
USB 3.0 roothub to 15 USB 3.0 ports.

Signed-off-by: Sarah Sharp <sarah.a.sharp-VuQAYsv1563Yd54FQh9/***@public.gmane.org>
---
drivers/usb/host/xhci-mem.c | 18 ++++++++++++++++++
1 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 76e4b95..6f30635 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -1804,6 +1804,20 @@ static int xhci_setup_port_arrays(struct xhci_hcd *xhci, gfp_t flags)
}
xhci_dbg(xhci, "Found %u USB 2.0 ports and %u USB 3.0 ports.\n",
xhci->num_usb2_ports, xhci->num_usb3_ports);
+
+ /* Place limits on the number of roothub ports so that the hub
+ * descriptors aren't longer than the USB core will allocate.
+ */
+ if (xhci->num_usb3_ports > 15) {
+ xhci_dbg(xhci, "Limiting USB 3.0 roothub ports to 15.\n");
+ xhci->num_usb3_ports = 15;
+ }
+ if (xhci->num_usb2_ports > USB_MAXCHILDREN) {
+ xhci_dbg(xhci, "Limiting USB 2.0 roothub ports to %u.\n",
+ USB_MAXCHILDREN);
+ xhci->num_usb2_ports = USB_MAXCHILDREN;
+ }
+
/*
* Note we could have all USB 3.0 ports, or all USB 2.0 ports.
* Not sure how the USB core will handle a hub with no ports...
@@ -1828,6 +1842,8 @@ static int xhci_setup_port_arrays(struct xhci_hcd *xhci, gfp_t flags)
"addr = %p\n", i,
xhci->usb2_ports[port_index]);
port_index++;
+ if (port_index == xhci->num_usb2_ports)
+ break;
}
}
if (xhci->num_usb3_ports) {
@@ -1846,6 +1862,8 @@ static int xhci_setup_port_arrays(struct xhci_hcd *xhci, gfp_t flags)
"addr = %p\n", i,
xhci->usb3_ports[port_index]);
port_index++;
+ if (port_index == xhci->num_usb3_ports)
+ break;
}
}
return 0;
--
1.6.3.3

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Sarah Sharp
2010-12-30 23:37:13 UTC
Permalink
Return early in the roothub control and status functions if the xHCI host
controller is not electrically present in the system (register reads
return all "fs"). This issue only shows up when the xHCI driver registers
two roothubs and the host controller is removed from the system.

Signed-off-by: Sarah Sharp <sarah.a.sharp-VuQAYsv1563Yd54FQh9/***@public.gmane.org>
---
drivers/usb/host/xhci-hub.c | 16 ++++++++++++++++
1 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 744eabd..4cce2d1 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -418,6 +418,10 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
wIndex--;
status = 0;
temp = xhci_readl(xhci, port_array[wIndex]);
+ if (temp == 0xffffffff) {
+ retval = -ENODEV;
+ break;
+ }
xhci_dbg(xhci, "get port status, actual port %d status = 0x%x\n", wIndex, temp);

/* FIXME - should we return a port status value like the USB
@@ -492,6 +496,10 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
goto error;
wIndex--;
temp = xhci_readl(xhci, port_array[wIndex]);
+ if (temp == 0xffffffff) {
+ retval = -ENODEV;
+ break;
+ }
temp = xhci_port_state_to_neutral(temp);
/* FIXME: What new port features do we need to support? */
switch (wValue) {
@@ -562,6 +570,10 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
goto error;
wIndex--;
temp = xhci_readl(xhci, port_array[wIndex]);
+ if (temp == 0xffffffff) {
+ retval = -ENODEV;
+ break;
+ }
/* FIXME: What new port features do we need to support? */
temp = xhci_port_state_to_neutral(temp);
switch (wValue) {
@@ -677,6 +689,10 @@ int xhci_hub_status_data(struct usb_hcd *hcd, char *buf)
/* For each port, did anything change? If so, set that bit in buf. */
for (i = 0; i < ports; i++) {
temp = xhci_readl(xhci, port_array[i]);
+ if (temp == 0xffffffff) {
+ retval = -ENODEV;
+ break;
+ }
if ((temp & mask) != 0 ||
(bus_state->port_c_suspend & 1 << i) ||
(bus_state->resume_done[i] && time_after_eq(
--
1.6.3.3

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Alan Stern
2011-01-01 03:32:37 UTC
Permalink
Post by Sarah Sharp
Return early in the roothub control and status functions if the xHCI host
controller is not electrically present in the system (register reads
return all "fs"). This issue only shows up when the xHCI driver registers
two roothubs and the host controller is removed from the system.
Do you really think it's necessary to test in all these different
places? I would think that a single test in the interrupt routine
would be sufficient.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Sarah Sharp
2011-01-03 23:35:55 UTC
Permalink
Post by Alan Stern
Post by Sarah Sharp
Return early in the roothub control and status functions if the xHCI host
controller is not electrically present in the system (register reads
return all "fs"). This issue only shows up when the xHCI driver registers
two roothubs and the host controller is removed from the system.
Do you really think it's necessary to test in all these different
places? I would think that a single test in the interrupt routine
would be sufficient.
I thought so too. The xHCI driver does test in the interrupt handler.
But when I registered the second roothub, and the xHCI Express Card was
removed, I saw the USB core access the roothub to try and get the port
status before the second roothub was disconnected. That's why I added
this patch.

Sarah Sharp
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Alan Stern
2011-01-04 16:33:43 UTC
Permalink
Post by Sarah Sharp
Post by Alan Stern
Post by Sarah Sharp
Return early in the roothub control and status functions if the xHCI host
controller is not electrically present in the system (register reads
return all "fs"). This issue only shows up when the xHCI driver registers
two roothubs and the host controller is removed from the system.
Do you really think it's necessary to test in all these different
places? I would think that a single test in the interrupt routine
would be sufficient.
I thought so too. The xHCI driver does test in the interrupt handler.
But when I registered the second roothub, and the xHCI Express Card was
removed, I saw the USB core access the roothub to try and get the port
status before the second roothub was disconnected. That's why I added
this patch.
What happened? That is, what's wrong with letting the driver try to
access the second root hub after the card is removed?

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Sarah Sharp
2010-12-30 23:37:32 UTC
Permalink
USB_PORT_STAT_SUPER_SPEED is a made up symbol that the USB core used to
track whether USB ports had a SuperSpeed device attached. This is a
linux-internal symbol that was used when SuperSpeed and non-SuperSpeed
devices would show up under the same xHCI roothub. This particular
port status is never returned by external USB 3.0 hubs. (Instead they
have a USB_PORT_STAT_SPEED_5GBPS that uses a completely different speed
mask.)

Now that the xHCI driver registers two roothubs, USB 3.0 devices will only
show up under USB 3.0 hubs. Rip out USB_PORT_STAT_SUPER_SPEED and replace
it with calls to hub_is_superspeed().

Signed-off-by: Sarah Sharp <sarah.a.sharp-VuQAYsv1563Yd54FQh9/***@public.gmane.org>
---
drivers/usb/core/hub.c | 43 ++++++++++++++-----------------------------
drivers/usb/host/xhci-hub.c | 2 --
include/linux/usb/ch11.h | 1 -
3 files changed, 14 insertions(+), 32 deletions(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 928bbfe..8a5ea2b 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -161,8 +161,6 @@ static inline char *portspeed(int portstatus)
return "480 Mb/s";
else if (portstatus & USB_PORT_STAT_LOW_SPEED)
return "1.5 Mb/s";
- else if (portstatus & USB_PORT_STAT_SUPER_SPEED)
- return "5.0 Gb/s";
else
return "12 Mb/s";
}
@@ -385,9 +383,6 @@ static int hub_port_status(struct usb_hub *hub, int port1,
u16 tmp = *status & USB_SS_PORT_STAT_MASK;
if (*status & USB_SS_PORT_STAT_POWER)
tmp |= USB_PORT_STAT_POWER;
- if ((*status & USB_SS_PORT_STAT_SPEED) ==
- USB_PORT_STAT_SPEED_5GBPS)
- tmp |= USB_PORT_STAT_SUPER_SPEED;
*status = tmp;
}

@@ -795,12 +790,8 @@ static void hub_activate(struct usb_hub *hub, enum hub_activation_type type)
* USB3 protocol ports will automatically transition
* to Enabled state when detect an USB3.0 device attach.
* Do not disable USB3 protocol ports.
- * FIXME: USB3 root hub and external hubs are treated
- * differently here.
*/
- if (hdev->descriptor.bDeviceProtocol != 3 ||
- (!hdev->parent &&
- !(portstatus & USB_PORT_STAT_SUPER_SPEED))) {
+ if (!hub_is_superspeed(hdev)) {
clear_port_feature(hdev, port1,
USB_PORT_FEAT_ENABLE);
portstatus &= ~USB_PORT_STAT_ENABLE;
@@ -2059,14 +2050,12 @@ static int hub_port_wait_reset(struct usb_hub *hub, int port1,
(portstatus & USB_PORT_STAT_ENABLE)) {
if (hub_is_wusb(hub))
udev->speed = USB_SPEED_WIRELESS;
- else if (portstatus & USB_PORT_STAT_SUPER_SPEED)
+ else if (hub_is_superspeed(hub->hdev))
udev->speed = USB_SPEED_SUPER;
else if (portstatus & USB_PORT_STAT_HIGH_SPEED)
udev->speed = USB_SPEED_HIGH;
else if (portstatus & USB_PORT_STAT_LOW_SPEED)
udev->speed = USB_SPEED_LOW;
- else if (portstatus & USB_PORT_STAT_SUPER_SPEED)
- udev->speed = USB_SPEED_SUPER;
else
udev->speed = USB_SPEED_FULL;
return 0;
@@ -3056,9 +3045,16 @@ static void hub_port_connect_change(struct usb_hub *hub, int port1,
struct usb_device *udev;
int status, i;

- dev_dbg (hub_dev,
- "port %d, status %04x, change %04x, %s\n",
- port1, portstatus, portchange, portspeed (portstatus));
+ /* Only USB 3.0 devices connect to SuperSpeed hubs. */
+ if (hub_is_superspeed(hub->hdev))
+ dev_dbg(hub_dev,
+ "port %d, status %04x, change %04x, %s\n",
+ port1, portstatus, portchange, "5.0 Gb/s");
+ else
+ dev_dbg(hub_dev,
+ "port %d, status %04x, change %04x, %s\n",
+ port1, portstatus, portchange,
+ portspeed(portstatus));

if (hub->has_indicators) {
set_port_led(hub, port1, HUB_LED_AUTO);
@@ -3159,19 +3155,8 @@ static void hub_port_connect_change(struct usb_hub *hub, int port1,
udev->level = hdev->level + 1;
udev->wusb = hub_is_wusb(hub);

- /*
- * USB 3.0 devices are reset automatically before the connect
- * port status change appears, and the root hub port status
- * shows the correct speed. We also get port change
- * notifications for USB 3.0 devices from the USB 3.0 portion of
- * an external USB 3.0 hub, but this isn't handled correctly yet
- * FIXME.
- */
-
- if (!(hcd->driver->flags & HCD_USB3))
- udev->speed = USB_SPEED_UNKNOWN;
- else if ((hdev->parent == NULL) &&
- (portstatus & USB_PORT_STAT_SUPER_SPEED))
+ /* Only USB 3.0 devices are connected to SuperSpeed hubs. */
+ if (hub_is_superspeed(hub->hdev))
udev->speed = USB_SPEED_SUPER;
else
udev->speed = USB_SPEED_UNKNOWN;
diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 4cce2d1..f365f35 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -155,8 +155,6 @@ static unsigned int xhci_port_speed(unsigned int port_status)
return USB_PORT_STAT_LOW_SPEED;
if (DEV_HIGHSPEED(port_status))
return USB_PORT_STAT_HIGH_SPEED;
- if (DEV_SUPERSPEED(port_status))
- return USB_PORT_STAT_SUPER_SPEED;
/*
* FIXME: Yes, we should check for full speed, but the core uses that as
* a default in portspeed() in usb/core/hub.c (which is the only place
diff --git a/include/linux/usb/ch11.h b/include/linux/usb/ch11.h
index 8618ee6..ff50fa4 100644
--- a/include/linux/usb/ch11.h
+++ b/include/linux/usb/ch11.h
@@ -109,7 +109,6 @@ struct usb_port_status {
#define USB_PORT_STAT_TEST 0x0800
#define USB_PORT_STAT_INDICATOR 0x1000
/* bits 13 to 15 are reserved */
-#define USB_PORT_STAT_SUPER_SPEED 0x8000 /* Linux-internal */

/*
* Additions to wPortStatus bit field from USB 3.0
--
1.6.3.3

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Alan Stern
2011-01-01 03:36:26 UTC
Permalink
Post by Sarah Sharp
USB_PORT_STAT_SUPER_SPEED is a made up symbol that the USB core used to
track whether USB ports had a SuperSpeed device attached. This is a
linux-internal symbol that was used when SuperSpeed and non-SuperSpeed
devices would show up under the same xHCI roothub. This particular
port status is never returned by external USB 3.0 hubs. (Instead they
have a USB_PORT_STAT_SPEED_5GBPS that uses a completely different speed
mask.)
Now that the xHCI driver registers two roothubs, USB 3.0 devices will only
show up under USB 3.0 hubs. Rip out USB_PORT_STAT_SUPER_SPEED and replace
it with calls to hub_is_superspeed().
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 928bbfe..8a5ea2b 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -161,8 +161,6 @@ static inline char *portspeed(int portstatus)
return "480 Mb/s";
else if (portstatus & USB_PORT_STAT_LOW_SPEED)
return "1.5 Mb/s";
- else if (portstatus & USB_PORT_STAT_SUPER_SPEED)
- return "5.0 Gb/s";
else
return "12 Mb/s";
}
@@ -3056,9 +3045,16 @@ static void hub_port_connect_change(struct usb_hub *hub, int port1,
struct usb_device *udev;
int status, i;
- dev_dbg (hub_dev,
- "port %d, status %04x, change %04x, %s\n",
- port1, portstatus, portchange, portspeed (portstatus));
+ /* Only USB 3.0 devices connect to SuperSpeed hubs. */
+ if (hub_is_superspeed(hub->hdev))
+ dev_dbg(hub_dev,
+ "port %d, status %04x, change %04x, %s\n",
+ port1, portstatus, portchange, "5.0 Gb/s");
+ else
+ dev_dbg(hub_dev,
+ "port %d, status %04x, change %04x, %s\n",
+ port1, portstatus, portchange,
+ portspeed(portstatus));
Instead of duplicating this dev_dbg() statement, simply change the
portspeed() routine to accept a pointer to the hub structure as an
additional argument.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Sarah Sharp
2011-01-03 23:36:20 UTC
Permalink
Post by Alan Stern
Post by Sarah Sharp
USB_PORT_STAT_SUPER_SPEED is a made up symbol that the USB core used to
track whether USB ports had a SuperSpeed device attached. This is a
linux-internal symbol that was used when SuperSpeed and non-SuperSpeed
devices would show up under the same xHCI roothub. This particular
port status is never returned by external USB 3.0 hubs. (Instead they
have a USB_PORT_STAT_SPEED_5GBPS that uses a completely different speed
mask.)
Now that the xHCI driver registers two roothubs, USB 3.0 devices will only
show up under USB 3.0 hubs. Rip out USB_PORT_STAT_SUPER_SPEED and replace
it with calls to hub_is_superspeed().
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 928bbfe..8a5ea2b 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -161,8 +161,6 @@ static inline char *portspeed(int portstatus)
return "480 Mb/s";
else if (portstatus & USB_PORT_STAT_LOW_SPEED)
return "1.5 Mb/s";
- else if (portstatus & USB_PORT_STAT_SUPER_SPEED)
- return "5.0 Gb/s";
else
return "12 Mb/s";
}
@@ -3056,9 +3045,16 @@ static void hub_port_connect_change(struct usb_hub *hub, int port1,
struct usb_device *udev;
int status, i;
- dev_dbg (hub_dev,
- "port %d, status %04x, change %04x, %s\n",
- port1, portstatus, portchange, portspeed (portstatus));
+ /* Only USB 3.0 devices connect to SuperSpeed hubs. */
+ if (hub_is_superspeed(hub->hdev))
+ dev_dbg(hub_dev,
+ "port %d, status %04x, change %04x, %s\n",
+ port1, portstatus, portchange, "5.0 Gb/s");
+ else
+ dev_dbg(hub_dev,
+ "port %d, status %04x, change %04x, %s\n",
+ port1, portstatus, portchange,
+ portspeed(portstatus));
Instead of duplicating this dev_dbg() statement, simply change the
portspeed() routine to accept a pointer to the hub structure as an
additional argument.
Sure, will do.

Sarah Sharp
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Sarah Sharp
2010-12-30 23:37:37 UTC
Permalink
USB 3.0 devices have a slightly different suspend sequence than USB
2.0/1.1 devices. There isn't support for USB 3.0 device suspend yet, so
make khubd leave autosuspend disabled for USB 3.0 hubs.

Signed-off-by: Sarah Sharp <sarah.a.sharp-VuQAYsv1563Yd54FQh9/***@public.gmane.org>
---
drivers/usb/core/hub.c | 9 +++++++--
1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 8a5ea2b..aca1cce 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -1288,8 +1288,13 @@ static int hub_probe(struct usb_interface *intf, const struct usb_device_id *id)
desc = intf->cur_altsetting;
hdev = interface_to_usbdev(intf);

- /* Hubs have proper suspend/resume support */
- usb_enable_autosuspend(hdev);
+ /* Hubs have proper suspend/resume support. USB 3.0 device suspend
+ * is different than USB 2.0/1.1 device suspend, and unfortunately we
+ * don't support it yet. So leave autosuspend disabled that for
+ * USB 3.0 hubs for now.
+ */
+ if (!hub_is_superspeed(hdev))
+ usb_enable_autosuspend(hdev);

if (hdev->level == MAX_TOPO_LEVEL) {
dev_err(&intf->dev,
--
1.6.3.3

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Sergei Shtylyov
2010-12-31 16:39:43 UTC
Permalink
Hello.
Post by Sarah Sharp
USB 3.0 devices have a slightly different suspend sequence than USB
2.0/1.1 devices. There isn't support for USB 3.0 device suspend yet, so
make khubd leave autosuspend disabled for USB 3.0 hubs.
[...]
Post by Sarah Sharp
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 8a5ea2b..aca1cce 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -1288,8 +1288,13 @@ static int hub_probe(struct usb_interface *intf, const struct usb_device_id *id)
desc = intf->cur_altsetting;
hdev = interface_to_usbdev(intf);
- /* Hubs have proper suspend/resume support */
- usb_enable_autosuspend(hdev);
+ /* Hubs have proper suspend/resume support. USB 3.0 device suspend
+ * is different than USB 2.0/1.1 device suspend, and unfortunately we
+ * don't support it yet. So leave autosuspend disabled that for
^^^^ not
needed here?
Post by Sarah Sharp
+ * USB 3.0 hubs for now.
+ */
+ if (!hub_is_superspeed(hdev))
+ usb_enable_autosuspend(hdev);
WBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Alan Stern
2011-01-01 03:39:03 UTC
Permalink
Post by Sarah Sharp
USB 3.0 devices have a slightly different suspend sequence than USB
2.0/1.1 devices. There isn't support for USB 3.0 device suspend yet, so
make khubd leave autosuspend disabled for USB 3.0 hubs.
---
drivers/usb/core/hub.c | 9 +++++++--
1 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 8a5ea2b..aca1cce 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -1288,8 +1288,13 @@ static int hub_probe(struct usb_interface *intf, const struct usb_device_id *id)
desc = intf->cur_altsetting;
hdev = interface_to_usbdev(intf);
- /* Hubs have proper suspend/resume support */
- usb_enable_autosuspend(hdev);
+ /* Hubs have proper suspend/resume support. USB 3.0 device suspend
+ * is different than USB 2.0/1.1 device suspend, and unfortunately we
s/different than/different from/ (and likewise in the patch
description)
Post by Sarah Sharp
+ * don't support it yet. So leave autosuspend disabled that for
s/disabled that/disabled/
Post by Sarah Sharp
+ * USB 3.0 hubs for now.
+ */
+ if (!hub_is_superspeed(hdev))
+ usb_enable_autosuspend(hdev);
Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Sarah Sharp
2010-12-30 23:37:24 UTC
Permalink
When a host controller has lost power during a suspend, we must
reinitialize it. Now that the xHCI host has two roothubs, xhci_run() and
xhci_stop() expect to be called with both usb_hcd structures. Be sure
that the re-initialization code in xhci_resume() mirrors the process the
USB PCI probe function uses.

Signed-off-by: Sarah Sharp <sarah.a.sharp-VuQAYsv1563Yd54FQh9/***@public.gmane.org>
---
drivers/usb/host/xhci.c | 23 +++++++++++++++++++----
1 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 7ebf9fb..e3e5eea 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -729,6 +729,7 @@ int xhci_resume(struct xhci_hcd *xhci, bool hibernated)
{
u32 command, temp = 0;
struct usb_hcd *hcd = xhci_to_hcd(xhci);
+ struct usb_hcd *secondary_hcd;
int old_state, retval;

old_state = hcd->state;
@@ -790,15 +791,29 @@ int xhci_resume(struct xhci_hcd *xhci, bool hibernated)
xhci_dbg(xhci, "xhci_stop completed - status = %x\n",
xhci_readl(xhci, &xhci->op_regs->status));

- xhci_dbg(xhci, "Initialize the HCD\n");
- retval = xhci_init(hcd);
+ /* USB core calls the PCI reinit and start functions twice:
+ * first with the primary HCD, and then with the secondary HCD.
+ * If we don't do the same, the host will never be started.
+ */
+ if (!usb_hcd_is_primary_hcd(hcd))
+ secondary_hcd = hcd;
+ else
+ secondary_hcd = hcd->shared_hcd;
+
+ xhci_dbg(xhci, "Initialize the xhci_hcd\n");
+ retval = xhci_init(hcd->primary_hcd);
if (retval)
return retval;
+ xhci_dbg(xhci, "Start the primary HCD\n");
+ retval = xhci_run(hcd->primary_hcd);
+ if (retval)
+ goto failed_restart;

- xhci_dbg(xhci, "Start the HCD\n");
- retval = xhci_run(hcd);
+ xhci_dbg(xhci, "Start the secondary HCD\n");
+ retval = xhci_run(secondary_hcd);
if (!retval)
set_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
+failed_restart:
hcd->state = HC_STATE_SUSPENDED;
return retval;
}
--
1.6.3.3

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Sarah Sharp
2010-12-30 23:37:28 UTC
Permalink
Make sure the HCD_FLAG_HW_ACCESSIBLE flag is mirrored by both roothubs,
since it refers to whether the shared hardware is accessible. Make sure
each bus is marked as suspended by setting usb_hcd->state to
HC_STATE_SUSPENDED when the PCI host controller is resumed.

If the host is resumed, and the power session is not lost, then make sure
to set the state of each host controller to what it was before. (NOTE:
this code is on an unreachable code path, and I'm waiting on a formal
patch from Andiry to fix it.)

Signed-off-by: Sarah Sharp <sarah.a.sharp-VuQAYsv1563Yd54FQh9/***@public.gmane.org>
---
drivers/usb/host/xhci-pci.c | 3 ++-
drivers/usb/host/xhci.c | 24 ++++++++++++++++++------
2 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index 69c235f..63bd55b 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -157,7 +157,8 @@ static int xhci_pci_suspend(struct usb_hcd *hcd, bool do_wakeup)
struct xhci_hcd *xhci = hcd_to_xhci(hcd);
int retval = 0;

- if (hcd->state != HC_STATE_SUSPENDED)
+ if (hcd->state != HC_STATE_SUSPENDED ||
+ hcd->shared_hcd->state != HC_STATE_SUSPENDED)
return -EINVAL;

retval = xhci_suspend(xhci);
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index e3e5eea..0a5d168 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -680,6 +680,7 @@ int xhci_suspend(struct xhci_hcd *xhci)

spin_lock_irq(&xhci->lock);
clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
+ clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->shared_hcd->flags);
/* step 1: stop endpoint */
/* skipped assuming that port suspend has done */

@@ -730,9 +731,12 @@ int xhci_resume(struct xhci_hcd *xhci, bool hibernated)
u32 command, temp = 0;
struct usb_hcd *hcd = xhci_to_hcd(xhci);
struct usb_hcd *secondary_hcd;
- int old_state, retval;
+ int primary_hcd_old_state;
+ int secondary_hcd_old_state;
+ int retval;

- old_state = hcd->state;
+ primary_hcd_old_state = hcd->primary_hcd->state;
+ secondary_hcd_old_state = hcd->primary_hcd->shared_hcd->state;
/* Wait a bit if either of the roothubs need to settle from the
* transistion into bus suspend.
*/
@@ -811,10 +815,14 @@ int xhci_resume(struct xhci_hcd *xhci, bool hibernated)

xhci_dbg(xhci, "Start the secondary HCD\n");
retval = xhci_run(secondary_hcd);
- if (!retval)
+ if (!retval) {
set_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
+ set_bit(HCD_FLAG_HW_ACCESSIBLE,
+ &hcd->shared_hcd->flags);
+ }
failed_restart:
hcd->state = HC_STATE_SUSPENDED;
+ hcd->shared_hcd->state = HC_STATE_SUSPENDED;
return retval;
}

@@ -835,10 +843,14 @@ failed_restart:
*/

set_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
- if (!hibernated)
- hcd->state = old_state;
- else
+ set_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->shared_hcd->flags);
+ if (!hibernated) {
+ hcd->primary_hcd->state = primary_hcd_old_state;
+ hcd->primary_hcd->shared_hcd->state = secondary_hcd_old_state;
+ } else {
hcd->state = HC_STATE_SUSPENDED;
+ hcd->shared_hcd->state = HC_STATE_SUSPENDED;
+ }

spin_unlock_irq(&xhci->lock);
return 0;
--
1.6.3.3

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Xu, Andiry
2011-01-04 09:42:32 UTC
Permalink
-----Original Message-----
Sent: Friday, December 31, 2010 7:23 AM
Cc: John Youn; Dwight Schauer; Alan Stern; Xu, Andiry
Subject: [RFC v2 00/22] USB 3.0 hub support & xHCI split roothub for
2.6.38
This is the second version of the patchset to add support for USB 3.0
hubs, and make the xHCI roothub act like an external USB 3.0 hub by
registering two roothubs: a USB 2.0 hub and a USB 3.0 hub.
Most of the code that changes USB core behavior is in patches 1-3 and
6-11. Patch one is only slightly updated from last time, with the
addition of using DeviceRemovable instead of bitmap. Patch three is
unchanged from the last RFC.
The real meat of the changes to the USB core are in patches 8-11. I
would especially like some feedback on patch 11 (USB: Set
usb_hcd->state
and flags for shared roothubs).
- introduce a new host controller flag, HCD_SHARED, to indicate a host
wants to have the USB core allocate a shared roothub.
- introduce a new variable in usb_hcd, bcdUSB, that indicates what
speed the roothub operates at. This originally mirrors
usb_hcd->driver->flags & HCD_MASK, but this field is writable, and
can be changed after the second roothub is allocated.
- introduce a new variable in usb_hcd, primary_hcd, that indicates
which host controller is the primary HCD. The primary HCD is the
roothub that is allocated first.
- Support for bus and PCI suspend of shared roothubs.
- Some removal of the translation of USB 3.0 port status into USB 2.0
port status. I was able to get rid of USB_PORT_STAT_SUPER_SPEED, but
I haven't had time to look at getting rid of the port power
translation. There isn't any other translations in the core.
- Disabling USB 3.0 hub auto-suspend, since the mechanisms to do device
suspend are slightly different for USB 3.0 devices, and we don't have
support for that.
The patchset is stable, and everything seems to work just as well as it
did before the xHCI roothub was split (with the bonus of USB 3.0 hub
support, of course). Andiry, I would really appreciate it if you could
run this patchset on your machine that keeps the power session active
across suspend.
Thanks for the patchset. I've run some basic S3 tests on my platform
with CONFIG_USB_SUSPEND set. Here is the result:

USB2.0 HDD connected to USB2.0 split root hub: suspend and resume OK
USB3.0 HDD connected to USB3.0 split root hub: suspend and resume OK
USB2.0 HDD connected to USB2.0 hub in USB3.0 external hub: suspend and
resume OK
USB3.0 HDD connected to USB3.0 hub in USB3.0 external hub: suspend
failed.
set_port_feature(hub->hdev, port1, USB_PORT_FEAT_SUSPEND) in
usb_port_suspend() returned with error -32. There is a Stall Error in
xhci driver.

Thanks,
Andiry
USB 3.0 Hub Changes
USB: Clear "warm" port reset change.
usb: Make USB 3.0 roothub have a SS EP comp descriptor.
xhci: Always use usb_hcd in URB instead of converting xhci_hcd.
xhci: Change hcd_priv into a pointer.
usb: Make usb_hcd_pci_probe labels more descriptive.
usb: Refactor irq enabling out of usb_add_hcd()
usb: Change usb_hcd->bandwidth_mutex to a pointer.
usb: Store bus type in usb_hcd, not in driver flags.
usb: Make core allocate resources per PCI-device.
USB: Set usb_hcd->state and flags for shared roothubs.
xhci: Index with a port array instead of PORTSC addresses.
xhci: Refactor bus suspend state into a struct.
xhci: Change xhci_find_slot_id_by_port() API.
xhci: Register second xHCI roothub.
xhci: Return a USB 3.0 hub descriptor for USB3 roothub.
xhci: Limit roothub ports to 15 USB3 & 31 USB2 ports.
xhci: Make roothub functions deal with device removal.
xhci: Fix re-init on power loss after resume.
xhci: Fixes for suspend/resume of shared HCDs.
USB: Remove bogus USB_PORT_STAT_SUPER_SPEED symbol.
usb: Disable auto-suspend for USB 3.0 hubs.
drivers/staging/usbip/vhci_hcd.c | 4 +-
drivers/usb/core/hcd-pci.c | 93 ++++++++--
drivers/usb/core/hcd.c | 181 ++++++++++++++-----
drivers/usb/core/hub.c | 131 ++++++++++----
drivers/usb/core/message.c | 22 ++--
drivers/usb/gadget/dummy_hcd.c | 4 +-
drivers/usb/host/ehci-hub.c | 4 +-
drivers/usb/host/imx21-hcd.c | 4 +-
drivers/usb/host/isp116x-hcd.c | 4 +-
drivers/usb/host/isp1362-hcd.c | 4 +-
drivers/usb/host/isp1760-hcd.c | 4 +-
drivers/usb/host/ohci-hub.c | 11 +-
drivers/usb/host/oxu210hp-hcd.c | 4 +-
drivers/usb/host/r8a66597-hcd.c | 5 +-
drivers/usb/host/sl811-hcd.c | 4 +-
drivers/usb/host/u132-hcd.c | 11 +-
drivers/usb/host/xhci-hub.c | 363
+++++++++++++++++++++++++++------
-----
drivers/usb/host/xhci-mem.c | 88 +++++++++-
drivers/usb/host/xhci-pci.c | 54 +++++-
drivers/usb/host/xhci-ring.c | 112 ++++++++++--
drivers/usb/host/xhci.c | 107 +++++++++---
drivers/usb/host/xhci.h | 37 +++--
drivers/usb/musb/musb_virthub.c | 4 +-
drivers/usb/wusbcore/rh.c | 4 +-
include/linux/usb/ch11.h | 42 ++++-
include/linux/usb/hcd.h | 12 ++-
26 files changed, 997 insertions(+), 316 deletions(-)
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Sarah Sharp
2011-01-04 17:23:30 UTC
Permalink
Post by Xu, Andiry
Thanks for the patchset. I've run some basic S3 tests on my platform
USB2.0 HDD connected to USB2.0 split root hub: suspend and resume OK
USB3.0 HDD connected to USB3.0 split root hub: suspend and resume OK
USB2.0 HDD connected to USB2.0 hub in USB3.0 external hub: suspend and
resume OK
USB3.0 HDD connected to USB3.0 hub in USB3.0 external hub: suspend
failed.
set_port_feature(hub->hdev, port1, USB_PORT_FEAT_SUSPEND) in
usb_port_suspend() returned with error -32. There is a Stall Error in
xhci driver.
That's expected, since we don't handle USB 3.0 device suspend properly
in the USB core. The devices will suspend fine behind an xHCI roothub,
but won't suspend behind an external USB 3.0 hub. I'm not sure whether
I want to push this patchset with that result. We won't break anyone's
current setup (since they can't use USB 3.0 external hubs anyway), but a
lot of people might complain about it later...

Sarah Sharp
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Loading...