[48/87] USB: move usbcore away from hcd->state
From: Greg KH
Date: Mon Mar 21 2011 - 19:42:05 EST
2.6.37-stable review patch. If anyone has any objections, please let us know.
------------------
From: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
commit 9b37596a2e860404503a3f2a6513db60c296bfdc upstream.
The hcd->state variable is a disaster. It's not clearly owned by
either usbcore or the host controller drivers, and they both change it
from time to time, potentially stepping on each other's toes. It's
not protected by any locks. And there's no mechanism to prevent it
from going through an invalid transition.
This patch (as1451) takes a first step toward fixing these problems.
As it turns out, usbcore uses hcd->state for essentially only two
things: checking whether the controller's root hub is running and
checking whether the controller has died. Therefore the patch adds
two new atomic bitflags to the hcd structure, to store these pieces of
information. The new flags are used only by usbcore, and a private
spinlock prevents invalid combinations (a dead controller's root hub
cannot be running).
The patch does not change the places where usbcore sets hcd->state,
since HCDs may depend on them. Furthermore, there is one place in
usb_hcd_irq() where usbcore still must use hcd->state: An HCD's
interrupt handler can implicitly indicate that the controller died by
setting hcd->state to HC_STATE_HALT. Nevertheless, the new code is a
big improvement over the current code.
The patch makes one other change. The hcd_bus_suspend() and
hcd_bus_resume() routines now check first whether the host controller
has died; if it has then they return immediately without calling the
HCD's bus_suspend or bus_resume methods.
This fixes the major problem reported in Bugzilla #29902: The system
fails to suspend after a host controller dies during system resume.
Signed-off-by: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
Tested-by: Alex Terekhov <a.terekhov@xxxxxxxxx>
Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxx>
---
drivers/usb/core/hcd-pci.c | 13 ++++------
drivers/usb/core/hcd.c | 55 +++++++++++++++++++++++++++++++++------------
include/linux/usb/hcd.h | 4 +++
3 files changed, 51 insertions(+), 21 deletions(-)
--- a/drivers/usb/core/hcd-pci.c
+++ b/drivers/usb/core/hcd-pci.c
@@ -364,8 +364,7 @@ static int check_root_hub_suspended(stru
struct pci_dev *pci_dev = to_pci_dev(dev);
struct usb_hcd *hcd = pci_get_drvdata(pci_dev);
- if (!(hcd->state == HC_STATE_SUSPENDED ||
- hcd->state == HC_STATE_HALT)) {
+ if (HCD_RH_RUNNING(hcd)) {
dev_warn(dev, "Root hub is not suspended\n");
return -EBUSY;
}
@@ -387,7 +386,7 @@ static int suspend_common(struct device
if (retval)
return retval;
- if (hcd->driver->pci_suspend) {
+ if (hcd->driver->pci_suspend && !HCD_DEAD(hcd)) {
/* Optimization: Don't suspend if a root-hub wakeup is
* pending and it would cause the HCD to wake up anyway.
*/
@@ -428,7 +427,7 @@ static int resume_common(struct device *
struct usb_hcd *hcd = pci_get_drvdata(pci_dev);
int retval;
- if (hcd->state != HC_STATE_SUSPENDED) {
+ if (HCD_RH_RUNNING(hcd)) {
dev_dbg(dev, "can't resume, not suspended!\n");
return 0;
}
@@ -443,7 +442,7 @@ static int resume_common(struct device *
clear_bit(HCD_FLAG_SAW_IRQ, &hcd->flags);
- if (hcd->driver->pci_resume) {
+ if (hcd->driver->pci_resume && !HCD_DEAD(hcd)) {
if (event != PM_EVENT_AUTO_RESUME)
wait_for_companions(pci_dev, hcd);
@@ -476,10 +475,10 @@ static int hcd_pci_suspend_noirq(struct
pci_save_state(pci_dev);
- /* If the root hub is HALTed rather than SUSPENDed,
+ /* If the root hub is dead rather than suspended,
* disallow remote wakeup.
*/
- if (hcd->state == HC_STATE_HALT)
+ if (HCD_DEAD(hcd))
device_set_wakeup_enable(dev, 0);
dev_dbg(dev, "wakeup: %d\n", device_may_wakeup(dev));
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -984,7 +984,7 @@ static int register_root_hub(struct usb_
spin_unlock_irq (&hcd_root_hub_lock);
/* Did the HC die before the root hub was registered? */
- if (hcd->state == HC_STATE_HALT)
+ if (HCD_DEAD(hcd) || hcd->state == HC_STATE_HALT)
usb_hc_died (hcd); /* This time clean up */
}
@@ -1090,13 +1090,10 @@ int usb_hcd_link_urb_to_ep(struct usb_hc
* Check the host controller's state and add the URB to the
* endpoint's queue.
*/
- switch (hcd->state) {
- case HC_STATE_RUNNING:
- case HC_STATE_RESUMING:
+ if (HCD_RH_RUNNING(hcd)) {
urb->unlinked = 0;
list_add_tail(&urb->urb_list, &urb->ep->urb_list);
- break;
- default:
+ } else {
rc = -ESHUTDOWN;
goto done;
}
@@ -1914,7 +1911,7 @@ int usb_hcd_get_frame_number (struct usb
{
struct usb_hcd *hcd = bus_to_hcd(udev->bus);
- if (!HC_IS_RUNNING (hcd->state))
+ if (!HCD_RH_RUNNING(hcd))
return -ESHUTDOWN;
return hcd->driver->get_frame_number (hcd);
}
@@ -1931,9 +1928,15 @@ int hcd_bus_suspend(struct usb_device *r
dev_dbg(&rhdev->dev, "bus %s%s\n",
(msg.event & PM_EVENT_AUTO ? "auto-" : ""), "suspend");
+ if (HCD_DEAD(hcd)) {
+ dev_dbg(&rhdev->dev, "skipped %s of dead bus\n", "suspend");
+ return 0;
+ }
+
if (!hcd->driver->bus_suspend) {
status = -ENOENT;
} else {
+ clear_bit(HCD_FLAG_RH_RUNNING, &hcd->flags);
hcd->state = HC_STATE_QUIESCING;
status = hcd->driver->bus_suspend(hcd);
}
@@ -1941,7 +1944,12 @@ int hcd_bus_suspend(struct usb_device *r
usb_set_device_state(rhdev, USB_STATE_SUSPENDED);
hcd->state = HC_STATE_SUSPENDED;
} else {
- hcd->state = old_state;
+ spin_lock_irq(&hcd_root_hub_lock);
+ if (!HCD_DEAD(hcd)) {
+ set_bit(HCD_FLAG_RH_RUNNING, &hcd->flags);
+ hcd->state = old_state;
+ }
+ spin_unlock_irq(&hcd_root_hub_lock);
dev_dbg(&rhdev->dev, "bus %s fail, err %d\n",
"suspend", status);
}
@@ -1956,9 +1964,13 @@ int hcd_bus_resume(struct usb_device *rh
dev_dbg(&rhdev->dev, "usb %s%s\n",
(msg.event & PM_EVENT_AUTO ? "auto-" : ""), "resume");
+ if (HCD_DEAD(hcd)) {
+ dev_dbg(&rhdev->dev, "skipped %s of dead bus\n", "resume");
+ return 0;
+ }
if (!hcd->driver->bus_resume)
return -ENOENT;
- if (hcd->state == HC_STATE_RUNNING)
+ if (HCD_RH_RUNNING(hcd))
return 0;
hcd->state = HC_STATE_RESUMING;
@@ -1967,10 +1979,15 @@ int hcd_bus_resume(struct usb_device *rh
if (status == 0) {
/* TRSMRCY = 10 msec */
msleep(10);
- usb_set_device_state(rhdev, rhdev->actconfig
- ? USB_STATE_CONFIGURED
- : USB_STATE_ADDRESS);
- hcd->state = HC_STATE_RUNNING;
+ spin_lock_irq(&hcd_root_hub_lock);
+ if (!HCD_DEAD(hcd)) {
+ usb_set_device_state(rhdev, rhdev->actconfig
+ ? USB_STATE_CONFIGURED
+ : USB_STATE_ADDRESS);
+ set_bit(HCD_FLAG_RH_RUNNING, &hcd->flags);
+ hcd->state = HC_STATE_RUNNING;
+ }
+ spin_unlock_irq(&hcd_root_hub_lock);
} else {
hcd->state = old_state;
dev_dbg(&rhdev->dev, "bus %s fail, err %d\n",
@@ -2081,7 +2098,7 @@ irqreturn_t usb_hcd_irq (int irq, void *
*/
local_irq_save(flags);
- if (unlikely(hcd->state == HC_STATE_HALT || !HCD_HW_ACCESSIBLE(hcd))) {
+ if (unlikely(HCD_DEAD(hcd) || !HCD_HW_ACCESSIBLE(hcd))) {
rc = IRQ_NONE;
} else if (hcd->driver->irq(hcd) == IRQ_NONE) {
rc = IRQ_NONE;
@@ -2115,6 +2132,8 @@ void usb_hc_died (struct usb_hcd *hcd)
dev_err (hcd->self.controller, "HC died; cleaning up\n");
spin_lock_irqsave (&hcd_root_hub_lock, flags);
+ clear_bit(HCD_FLAG_RH_RUNNING, &hcd->flags);
+ set_bit(HCD_FLAG_DEAD, &hcd->flags);
if (hcd->rh_registered) {
clear_bit(HCD_FLAG_POLL_RH, &hcd->flags);
@@ -2257,6 +2276,12 @@ int usb_add_hcd(struct usb_hcd *hcd,
*/
device_init_wakeup(&rhdev->dev, 1);
+ /* HCD_FLAG_RH_RUNNING doesn't matter until the root hub is
+ * registered. But since the controller can die at any time,
+ * let's initialize the flag before touching the hardware.
+ */
+ set_bit(HCD_FLAG_RH_RUNNING, &hcd->flags);
+
/* "reset" is misnamed; its role is now one-time init. the controller
* should already have been reset (and boot firmware kicked off etc).
*/
@@ -2324,6 +2349,7 @@ int usb_add_hcd(struct usb_hcd *hcd,
return retval;
error_create_attr_group:
+ clear_bit(HCD_FLAG_RH_RUNNING, &hcd->flags);
if (HC_IS_RUNNING(hcd->state))
hcd->state = HC_STATE_QUIESCING;
spin_lock_irq(&hcd_root_hub_lock);
@@ -2376,6 +2402,7 @@ void usb_remove_hcd(struct usb_hcd *hcd)
usb_get_dev(rhdev);
sysfs_remove_group(&rhdev->dev.kobj, &usb_bus_attr_group);
+ clear_bit(HCD_FLAG_RH_RUNNING, &hcd->flags);
if (HC_IS_RUNNING (hcd->state))
hcd->state = HC_STATE_QUIESCING;
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -99,6 +99,8 @@ struct usb_hcd {
#define HCD_FLAG_POLL_RH 2 /* poll for rh status? */
#define HCD_FLAG_POLL_PENDING 3 /* status has changed? */
#define HCD_FLAG_WAKEUP_PENDING 4 /* root hub is resuming? */
+#define HCD_FLAG_RH_RUNNING 5 /* root hub is running? */
+#define HCD_FLAG_DEAD 6 /* controller has died? */
/* The flags can be tested using these macros; they are likely to
* be slightly faster than test_bit().
@@ -108,6 +110,8 @@ struct usb_hcd {
#define HCD_POLL_RH(hcd) ((hcd)->flags & (1U << HCD_FLAG_POLL_RH))
#define HCD_POLL_PENDING(hcd) ((hcd)->flags & (1U << HCD_FLAG_POLL_PENDING))
#define HCD_WAKEUP_PENDING(hcd) ((hcd)->flags & (1U << HCD_FLAG_WAKEUP_PENDING))
+#define HCD_RH_RUNNING(hcd) ((hcd)->flags & (1U << HCD_FLAG_RH_RUNNING))
+#define HCD_DEAD(hcd) ((hcd)->flags & (1U << HCD_FLAG_DEAD))
/* Flags that get set only during HCD registration or removal. */
unsigned rh_registered:1;/* is root hub registered? */
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/