Re: [RFC 1/7] usb: Always return 0 or -EBUSY to the runtime PM core.

Previous thread: Re: USB3 device suspend/runtime PM issue by Alan Stern on Thursday, December 30, 2010 - 3:05 pm. (2 messages)

Next thread: [RFC v2 00/22] USB 3.0 hub support & xHCI split roothub for 2.6.38 by Sarah Sharp on Thursday, December 30, 2010 - 4:22 pm. (23 messages)
From: Sarah Sharp
Date: Thursday, December 30, 2010 - 4:21 pm

These are a series of miscellaneous cleanups, refactoring, and bug fixes that
apply only to the patches Greg has queued for 2.6.38.  The split roothub
patches depend on them, which is why I'm sending them out before that
patchset.

Patch two (xhci: Remove old no-op test) was already sent out to the
linux-usb mailing list as part of the original split roothub RFC.

Sarah Sharp (7):
  usb: Always return 0 or -EBUSY to the runtime PM core.
  xhci: Remove old no-op test.
  xhci: Fix check for LS/FS device attached to roothub.
  USB: Remove bitmap #define from hcd.h
  USB: Fix usb_add_hcd() checkpatch errors.
  xhci: Rework port suspend structures for limited ports.
  xhci: Rename variables and reduce register reads.

 drivers/staging/usbip/vhci_hcd.c |    4 +-
 drivers/usb/core/driver.c        |    5 +++
 drivers/usb/core/hcd.c           |   11 ++++--
 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   |    6 ++--
 drivers/usb/host/isp1362-hcd.c   |    6 ++--
 drivers/usb/host/isp1760-hcd.c   |    6 ++--
 drivers/usb/host/ohci-hub.c      |   12 ++++----
 drivers/usb/host/oxu210hp-hcd.c  |    6 ++--
 drivers/usb/host/r8a66597-hcd.c  |    4 +-
 drivers/usb/host/sl811-hcd.c     |    6 ++--
 drivers/usb/host/u132-hcd.c      |   10 +++---
 drivers/usb/host/xhci-hub.c      |   62 +++++++++++++++++---------------------
 drivers/usb/host/xhci-mem.c      |    6 ++--
 drivers/usb/host/xhci-ring.c     |   25 ++-------------
 drivers/usb/host/xhci.c          |   10 ------
 drivers/usb/host/xhci.h          |   15 +++------
 drivers/usb/wusbcore/rh.c        |    4 +-
 include/linux/usb/hcd.h          |    7 ----
 21 files changed, 89 insertions(+), 128 deletions(-)

--

From: Sarah Sharp
Date: Thursday, December 30, 2010 - 4:21 pm

The PM core reacts badly when the return code from usb_runtime_suspend()
is not 0, -EAGAIN, or -EBUSY.  The PM core regards this as a fatal error,
and refuses to run anymore PM helper functions.  In particular,
usbfs_open() and other usbfs functions will fail because the PM core will
return an error code when usb_autoresume_device() is called.  This causes
libusb and/or lsusb to either hang or segfault.

If a USB device cannot suspend for some reason (e.g. a hub doesn't report
it has remote wakeup capabilities), we still want lsusb and other
userspace programs to work.  So return -EBUSY, which will fill people's
log files with failed tries, but will ensure userspace still works.

Signed-off-by: Sarah Sharp <sarah.a.sharp@linux.intel.com>
---
 drivers/usb/core/driver.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
index b9278a1..a6d9297 100644
--- a/drivers/usb/core/driver.c
+++ b/drivers/usb/core/driver.c
@@ -1660,6 +1660,11 @@ static int usb_runtime_suspend(struct device *dev)
 		return -EAGAIN;
 
 	status = usb_suspend_both(udev, PMSG_AUTO_SUSPEND);
+	/* The PM core reacts badly core unless the return code is 0,
+	 * -EAGAIN, or -EBUSY, so always return -EBUSY on an error.
+	 */
+	if (status != 0)
+		return -EBUSY;
 	return status;
 }
 
-- 
1.6.3.3

--

From: Sergei Shtylyov
Date: Friday, December 31, 2010 - 9:32 am

Hello.


WBR, Sergei
--

From: Sarah Sharp
Date: Thursday, December 30, 2010 - 4:22 pm

The test of placing a number of command no-ops on the command ring and
counting the number of no-op events that were generated was only used
during the initial xHCI driver bring up.  This test is no longer used, so
delete it.

Signed-off-by: Sarah Sharp <sarah.a.sharp@linux.intel.com>
---
 drivers/usb/host/xhci-ring.c |   19 -------------------
 drivers/usb/host/xhci.c      |   10 ----------
 drivers/usb/host/xhci.h      |    6 ------
 3 files changed, 0 insertions(+), 35 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 62c70c2..497612d 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -1118,7 +1118,6 @@ bandwidth_change:
 		handle_set_deq_completion(xhci, event, xhci->cmd_ring->dequeue);
 		break;
 	case TRB_TYPE(TRB_CMD_NOOP):
-		++xhci->noops_handled;
 		break;
 	case TRB_TYPE(TRB_RESET_EP):
 		handle_reset_ep_completion(xhci, event, xhci->cmd_ring->dequeue);
@@ -3114,24 +3113,6 @@ static int queue_command(struct xhci_hcd *xhci, u32 field1, u32 field2,
 	return 0;
 }
 
-/* Queue a no-op command on the command ring */
-static int queue_cmd_noop(struct xhci_hcd *xhci)
-{
-	return queue_command(xhci, 0, 0, 0, TRB_TYPE(TRB_CMD_NOOP), false);
-}
-
-/*
- * Place a no-op command on the command ring to test the command and
- * event ring.
- */
-void *xhci_setup_one_noop(struct xhci_hcd *xhci)
-{
-	if (queue_cmd_noop(xhci) < 0)
-		return NULL;
-	xhci->noops_submitted++;
-	return xhci_ring_cmd_db;
-}
-
 /* Queue a slot enable or disable request on the command ring */
 int xhci_queue_slot_control(struct xhci_hcd *xhci, u32 trb_type, u32 slot_id)
 {
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 34cf4e1..64f82b9 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -350,7 +350,6 @@ void xhci_event_ring_work(unsigned long arg)
 
 	temp = xhci_readl(xhci, &xhci->ir_set->irq_pending);
 	xhci_dbg(xhci, "ir_set 0 pending = 0x%x\n", temp);
-	xhci_dbg(xhci, ...
From: Sarah Sharp
Date: Thursday, December 30, 2010 - 4:22 pm

Once the xHCI host controller has a USB 2.0 roothub and a USB 3.0 roothub,
the TT field of the USB 2.0 roothub usb device will no longer be null.  To
check whether a LS/FS device is plugged into the HS roothub, see if the
parent's parent is NULL.  We don't need the TT information if the LS/FS
device is plugged into a roothub port.

This bug would only show up after the split roothub code is added.

Signed-off-by: Sarah Sharp <sarah.a.sharp@linux.intel.com>
---
 drivers/usb/host/xhci-mem.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 1d0f45f..76a9f1c 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -870,9 +870,9 @@ int xhci_setup_addressable_virt_dev(struct xhci_hcd *xhci, struct usb_device *ud
 	dev->port = top_dev->portnum;
 	xhci_dbg(xhci, "Set root hub portnum to %d\n", top_dev->portnum);
 
-	/* Is this a LS/FS device under a HS hub? */
+	/* Is this a LS/FS device under an external HS hub? */
 	if ((udev->speed == USB_SPEED_LOW || udev->speed == USB_SPEED_FULL) &&
-			udev->tt) {
+			udev->parent->parent && udev->tt) {
 		slot_ctx->tt_info = udev->tt->hub->slot_id;
 		slot_ctx->tt_info |= udev->ttport << 8;
 		if (udev->tt->multi)
-- 
1.6.3.3

--

From: Sarah Sharp
Date: Thursday, December 30, 2010 - 4:22 pm

Using a #define to redefine a common variable name is a bad thing,
especially when the #define is in a header.  include/linux/usb/hcd.h
redefined bitmap to DeviceRemovable to avoid typing a long field in the
hub descriptor.  This has unintended side effects for files like
drivers/usb/core/devio.c that include that file, since another header
included after hcd.h has different variables named bitmap.

Remove the bitmap #define and replace instances of it in the host
controller code.  Cleanup the spaces around function calls and square
brackets while we're at it.

Signed-off-by: Sarah Sharp <sarah.a.sharp@linux.intel.com>
Cc: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.yj@renesas.com>
Cc: Inaky Perez-Gonzalez <inaky.perez-gonzalez@intel.com>
Cc: Tony Olech <tony.olech@elandigitalsystems.com>
Cc: "Robert P. J. Day" <rpjday@crashcourse.ca>
Cc: Max Vozeler <mvz@vozeler.com>
Cc: Tejun Heo <tj@kernel.org>
Cc: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Cc: Rodolfo Giometti <giometti@linux.it>
Cc: Mike Frysinger <vapier@gentoo.org>
Cc: Anton Vorontsov <avorontsov@mvista.com>
Cc: Sebastian Siewior <bigeasy@linutronix.de>
Cc: Lothar Wassmann <LW@KARO-electronics.de>
Cc: Olav Kongas <ok@artecdesign.ee>
Cc: Martin Fuzzey <mfuzzey@gmail.com>
Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: David Brownell <dbrownell@users.sourceforge.net>
---
 drivers/staging/usbip/vhci_hcd.c |    4 ++--
 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   |    6 +++---
 drivers/usb/host/isp1362-hcd.c   |    6 +++---
 drivers/usb/host/isp1760-hcd.c   |    6 +++---
 drivers/usb/host/ohci-hub.c      |   12 ++++++------
 drivers/usb/host/oxu210hp-hcd.c  |    6 +++---
 drivers/usb/host/r8a66597-hcd.c  |    4 ++--
 drivers/usb/host/sl811-hcd.c     |    6 +++---
 drivers/usb/host/u132-hcd.c      |   10 +++++-----
 drivers/usb/host/xhci-hub.c      |    1 -
 drivers/usb/wusbcore/rh.c        ...
From: Sarah Sharp
Date: Thursday, December 30, 2010 - 4:22 pm

The irq enabling code is going to be refactored into a new function, so
clean up some checkpatch errors before moving it.

Signed-off-by: Sarah Sharp <sarah.a.sharp@linux.intel.com>
---
 drivers/usb/core/hcd.c |   11 +++++++----
 1 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 6a95017..7bf32c5 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2282,10 +2282,12 @@ int usb_add_hcd(struct usb_hcd *hcd,
 
 		snprintf(hcd->irq_descr, sizeof(hcd->irq_descr), "%s:usb%d",
 				hcd->driver->description, hcd->self.busnum);
-		if ((retval = request_irq(irqnum, &usb_hcd_irq, irqflags,
-				hcd->irq_descr, hcd)) != 0) {
+		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);
+					"request interrupt %d failed\n",
+					irqnum);
 			goto err_request_irq;
 		}
 		hcd->irq = irqnum;
@@ -2302,7 +2304,8 @@ int usb_add_hcd(struct usb_hcd *hcd,
 					(unsigned long long)hcd->rsrc_start);
 	}
 
-	if ((retval = hcd->driver->start(hcd)) < 0) {
+	retval = hcd->driver->start(hcd);
+	if (retval < 0) {
 		dev_err(hcd->self.controller, "startup error %d\n", retval);
 		goto err_hcd_driver_start;
 	}
-- 
1.6.3.3

--

From: Sarah Sharp
Date: Thursday, December 30, 2010 - 4:22 pm

The xhci_bus_suspend() and xhci_bus_resume() functions are a bit hard to
read, because they have an ambiguously named variable "port".  Rename it
to "port_index".  Introduce a new temporary variable, "max_ports" that
holds the maximum number of roothub ports the host controller supports.
This will reduce the number of register reads, and make it easy to change
the maximum number of ports when there are two roothubs.

Signed-off-by: Sarah Sharp <sarah.a.sharp@linux.intel.com>
Cc: Andiry Xu <andiry.xu@amd.com>
---
 drivers/usb/host/xhci-hub.c  |   33 ++++++++++++++++++---------------
 drivers/usb/host/xhci-ring.c |    6 +++---
 2 files changed, 21 insertions(+), 18 deletions(-)

diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 50e250c..0261198 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -568,29 +568,30 @@ int xhci_hub_status_data(struct usb_hcd *hcd, char *buf)
 int xhci_bus_suspend(struct usb_hcd *hcd)
 {
 	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
-	int port;
+	int max_ports, port_index;
 	unsigned long flags;
 
 	xhci_dbg(xhci, "suspend root hub\n");
+	max_ports = HCS_MAX_PORTS(xhci->hcs_params1);
 
 	spin_lock_irqsave(&xhci->lock, flags);
 
 	if (hcd->self.root_hub->do_remote_wakeup) {
-		port = HCS_MAX_PORTS(xhci->hcs_params1);
-		while (port--) {
-			if (xhci->resume_done[port] != 0) {
+		port_index = max_ports;
+		while (port_index--) {
+			if (xhci->resume_done[port_index] != 0) {
 				spin_unlock_irqrestore(&xhci->lock, flags);
 				xhci_dbg(xhci, "suspend failed because "
 						"port %d is resuming\n",
-						port + 1);
+						port_index + 1);
 				return -EBUSY;
 			}
 		}
 	}
 
-	port = HCS_MAX_PORTS(xhci->hcs_params1);
+	port_index = max_ports;
 	xhci->bus_suspended = 0;
-	while (port--) {
+	while (port_index--) {
 		/* suspend the port if the port is not suspended */
 		u32 __iomem *addr;
 		u32 t1, t2;
@@ -602,8 +603,9 @@ int xhci_bus_suspend(struct usb_hcd *hcd)
 		t2 = ...
From: Sarah Sharp
Date: Thursday, December 30, 2010 - 4:22 pm

The USB core only allows up to 31 (USB_MAXCHILDREN) ports under a roothub.
The xHCI driver keeps track of which ports are suspended, which ports have
a suspend change bit set, and what time the port will be done resuming.
It keeps track of the first two by setting a bit in a u32 variable,
suspended_ports or port_c_suspend.  The xHCI driver currently assumes we
can have up to 256 ports under a roothub, so it allocates an array of 8
u32 variables for both suspended_ports and port_c_suspend.  It also
allocates a 256-element array to keep track of when the ports will be done
resuming.

Since we can only have 31 roothub ports, we only need to use one u32 for
each of the suspend state and change variables.  We simplify the bit math
that's trying to index into those arrays and set the correct bit, if we
assume wIndex never exceeds 30.  (wIndex is zero-based after it's
decremented from the value passed in from the USB core.)  Finally, we
change the resume_done array to only hold 31 elements.

Signed-off-by: Sarah Sharp <sarah.a.sharp@linux.intel.com>
Cc: Andiry Xu <andiry.xu@amd.com>
---
 drivers/usb/host/xhci-hub.c |   28 ++++++++++------------------
 drivers/usb/host/xhci-mem.c |    2 +-
 drivers/usb/host/xhci.h     |    9 +++++----
 3 files changed, 16 insertions(+), 23 deletions(-)

diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index d83c27b..50e250c 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -347,20 +347,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[wIndex >> 5] |=
-						1 << (wIndex & 31);
-				xhci->suspended_ports[wIndex >> 5] &=
-						~(1 << (wIndex & 31));
+				xhci->port_c_suspend |= 1 << wIndex;
+				xhci->suspended_ports &= ~(1 << wIndex);
 			}
 		}
 		if ((temp & PORT_PLS_MASK) == XDEV_U0
 			&& (temp & PORT_POWER)
-			&& (xhci->suspended_ports[wIndex >> 5] &
-			    (1 << (wIndex & ...
Previous thread: Re: USB3 device suspend/runtime PM issue by Alan Stern on Thursday, December 30, 2010 - 3:05 pm. (2 messages)

Next thread: [RFC v2 00/22] USB 3.0 hub support & xHCI split roothub for 2.6.38 by Sarah Sharp on Thursday, December 30, 2010 - 4:22 pm. (23 messages)