[PATCH] USB: Fix USB suspend/resume crasher (#2)
This patch closes the IRQ race and makes various other OHCI & EHCI code
path safer vs. suspend/resume.
I've been able to (finally !) successfully suspend and resume various
Mac models, with or without USB mouse plugged, or plugging while asleep,
or unplugging while asleep etc... all without a crash.
Alan, please verify the UHCI bit I did, I only verified that it builds.
It's very simple so I wouldn't expect any issue there. If you aren't
confident, then just drop the hunks that change uhci-hcd.c
I also made the patch a little bit more "safer" by making sure the store
to the interrupt register that disables interrupts is not posted before
I set the flag and drop the spinlock.
Without this patch, you cannot reliably sleep/wakeup any recent Mac, and
I suspect PCs have some more sneaky issues too (they don't frankly crash
with machine checks because x86 tend to silently swallow PCI errors but
that won't last afaik, at least PCI Express will blow up in those
situations, but the USB code may still misbehave).
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>
diff --git a/drivers/usb/core/hcd-pci.c b/drivers/usb/core/hcd-pci.c
index 5131d88e..29b5b2a 100644
--- a/drivers/usb/core/hcd-pci.c
+++ b/drivers/usb/core/hcd-pci.c
@@ -219,6 +219,7 @@
goto done;
}
}
+ synchronize_irq(dev->irq);
/* FIXME until the generic PM interfaces change a lot more, this
* can't use PCI D1 and D2 states. For example, the confusion
@@ -392,7 +393,7 @@
dev->dev.power.power_state = PMSG_ON;
- hcd->saw_irq = 0;
+ clear_bit(HCD_FLAG_SAW_IRQ, &hcd->flags);
if (hcd->driver->resume) {
retval = hcd->driver->resume(hcd);
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 5e5f65a..da24c31 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -1315,11 +1315,12 @@
* finish unlinking the initial failed usb_set_address()
* or device descriptor fetch.
*/
- if (!hcd->saw_irq && hcd->self.root_hub != urb->dev) {
+ if (!test_bit(HCD_FLAG_SAW_IRQ, &hcd->flags)
+ && hcd->self.root_hub != urb->dev) {
dev_warn (hcd->self.controller, "Unlink after no-IRQ? "
"Controller is probably using the wrong IRQ."
"\n");
- hcd->saw_irq = 1;
+ set_bit(HCD_FLAG_SAW_IRQ, &hcd->flags);
}
urb->status = status;
@@ -1649,13 +1650,15 @@
struct usb_hcd *hcd = __hcd;
int start = hcd->state;
- if (start == HC_STATE_HALT)
+ if (unlikely(start == HC_STATE_HALT ||
+ !test_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags)))
return IRQ_NONE;
if (hcd->driver->irq (hcd, r) == IRQ_NONE)
return IRQ_NONE;
- hcd->saw_irq = 1;
- if (hcd->state == HC_STATE_HALT)
+ set_bit(HCD_FLAG_SAW_IRQ, &hcd->flags);
+
+ if (unlikely(hcd->state == HC_STATE_HALT))
usb_hc_died (hcd);
return IRQ_HANDLED;
}
@@ -1768,6 +1771,8 @@
dev_info(hcd->self.controller, "%s\n", hcd->product_desc);
+ set_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
+
/* till now HC has been in an indeterminate state ... */
if (hcd->driver->reset && (retval = hcd->driver->reset(hcd)) < 0) {
dev_err(hcd->self.controller, "can't reset\n");
diff --git a/drivers/usb/core/hcd.h b/drivers/usb/core/hcd.h
index 24a62a2..c8a1b35 100644
--- a/drivers/usb/core/hcd.h
+++ b/drivers/usb/core/hcd.h
@@ -72,7 +72,12 @@
* hardware info/state
*/
const struct hc_driver *driver; /* hw-specific hooks */
- unsigned saw_irq : 1;
+
+ /* Flags that need to be manipulated atomically */
+ unsigned long flags;
+#define HCD_FLAG_HW_ACCESSIBLE 0x00000001
+#define HCD_FLAG_SAW_IRQ 0x00000002
+
unsigned can_wakeup:1; /* hw supports wakeup? */
unsigned remote_wakeup:1;/* sw should use wakeup? */
unsigned rh_registered:1;/* is root hub registered? */
diff --git a/drivers/usb/host/ehci-pci.c b/drivers/usb/host/ehci-pci.c
index 441c260..14fff57 100644
--- a/drivers/usb/host/ehci-pci.c
+++ b/drivers/usb/host/ehci-pci.c
@@ -228,14 +228,36 @@
static int ehci_pci_suspend(struct usb_hcd *hcd, pm_message_t message)
{
struct ehci_hcd *ehci = hcd_to_ehci(hcd);
+ unsigned long flags;
+ int rc = 0;
if (time_before(jiffies, ehci->next_statechange))
msleep(10);
+ /* Root hub was already suspended. Disable irq emission and
+ * mark HW unaccessible, bail out if RH has been resumed. Use
+ * the spinlock to properly synchronize with possible pending
+ * RH suspend or resume activity.
+ *
+ * This is still racy as hcd->state is manipulated outside of
+ * any locks =P But that will be a different fix.
+ */
+ spin_lock_irqsave (&ehci->lock, flags);
+ if (hcd->state != HC_STATE_SUSPENDED) {
+ rc = -EINVAL;
+ goto bail;
+ }
+ writel (0, &ehci->regs->intr_enable);
+ (void)readl(&ehci->regs->intr_enable);
+
+ clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
+ bail:
+ spin_unlock_irqrestore (&ehci->lock, flags);
+
// could save FLADJ in case of Vaux power loss
// ... we'd only use it to handle clock skew
- return 0;
+ return rc;
}
static int ehci_pci_resume(struct usb_hcd *hcd)
@@ -251,6 +273,9 @@
if (time_before(jiffies, ehci->next_statechange))
msleep(100);
+ /* Mark hardware accessible again as we are out of D3 state by now */
+ set_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
+
/* If CF is clear, we lost PCI Vaux power and need to restart. */
if (readl(&ehci->regs->configured_flag) != FLAG_CF)
goto restart;
diff --git a/drivers/usb/host/ehci-q.c b/drivers/usb/host/ehci-q.c
index 5bb872c..bf03ec0 100644
--- a/drivers/usb/host/ehci-q.c
+++ b/drivers/usb/host/ehci-q.c
@@ -912,6 +912,7 @@
int epnum;
unsigned long flags;
struct ehci_qh *qh = NULL;
+ int rc = 0;
qtd = list_entry (qtd_list->next, struct ehci_qtd, qtd_list);
epnum = ep->desc.bEndpointAddress;
@@ -926,21 +927,28 @@
#endif
spin_lock_irqsave (&ehci->lock, flags);
+ if (unlikely(!test_bit(HCD_FLAG_HW_ACCESSIBLE,
+ &ehci_to_hcd(ehci)->flags))) {
+ rc = -ESHUTDOWN;
+ goto done;
+ }
+
qh = qh_append_tds (ehci, urb, qtd_list, epnum, &ep->hcpriv);
+ if (unlikely(qh == NULL)) {
+ rc = -ENOMEM;
+ goto done;
+ }
/* Control/bulk operations through TTs don't need scheduling,
* the HC and TT handle it when the TT has a buffer ready.
*/
- if (likely (qh != NULL)) {
- if (likely (qh->qh_state == QH_STATE_IDLE))
- qh_link_async (ehci, qh_get (qh));
- }
+ if (likely (qh->qh_state == QH_STATE_IDLE))
+ qh_link_async (ehci, qh_get (qh));
+ done:
spin_unlock_irqrestore (&ehci->lock, flags);
- if (unlikely (qh == NULL)) {
+ if (unlikely (qh == NULL))
qtd_list_free (ehci, urb, qtd_list);
- return -ENOMEM;
- }
- return 0;
+ return rc;
}
/*-------------------------------------------------------------------------*/
diff --git a/drivers/usb/host/ehci-sched.c b/drivers/usb/host/ehci-sched.c
index f0c8aa1..57e7737 100644
--- a/drivers/usb/host/ehci-sched.c
+++ b/drivers/usb/host/ehci-sched.c
@@ -602,6 +602,12 @@
spin_lock_irqsave (&ehci->lock, flags);
+ if (unlikely(!test_bit(HCD_FLAG_HW_ACCESSIBLE,
+ &ehci_to_hcd(ehci)->flags))) {
+ status = -ESHUTDOWN;
+ goto done;
+ }
+
/* get qh and force any scheduling errors */
INIT_LIST_HEAD (&empty);
qh = qh_append_tds (ehci, urb, &empty, epnum, &ep->hcpriv);
@@ -1456,7 +1462,11 @@
/* schedule ... need to lock */
spin_lock_irqsave (&ehci->lock, flags);
- status = iso_stream_schedule (ehci, urb, stream);
+ if (unlikely(!test_bit(HCD_FLAG_HW_ACCESSIBLE,
+ &ehci_to_hcd(ehci)->flags)))
+ status = -ESHUTDOWN;
+ else
+ status = iso_stream_schedule (ehci, urb, stream);
if (likely (status == 0))
itd_link_urb (ehci, urb, ehci->periodic_size << 3, stream);
spin_unlock_irqrestore (&ehci->lock, flags);
@@ -1815,7 +1825,11 @@
/* schedule ... need to lock */
spin_lock_irqsave (&ehci->lock, flags);
- status = iso_stream_schedule (ehci, urb, stream);
+ if (unlikely(!test_bit(HCD_FLAG_HW_ACCESSIBLE,
+ &ehci_to_hcd(ehci)->flags)))
+ status = -ESHUTDOWN;
+ else
+ status = iso_stream_schedule (ehci, urb, stream);
if (status == 0)
sitd_link_urb (ehci, urb, ehci->periodic_size << 3, stream);
spin_unlock_irqrestore (&ehci->lock, flags);
diff --git a/drivers/usb/host/ohci-hcd.c b/drivers/usb/host/ohci-hcd.c
index 5c0c6c8..bf1d9ab 100644
--- a/drivers/usb/host/ohci-hcd.c
+++ b/drivers/usb/host/ohci-hcd.c
@@ -115,7 +115,7 @@
/*-------------------------------------------------------------------------*/
-// #define OHCI_VERBOSE_DEBUG /* not always helpful */
+#undef OHCI_VERBOSE_DEBUG /* not always helpful */
/* For initializing controller (mask in an HCFS mode too) */
#define OHCI_CONTROL_INIT OHCI_CTRL_CBSR
@@ -253,6 +253,10 @@
spin_lock_irqsave (&ohci->lock, flags);
/* don't submit to a dead HC */
+ if (!test_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags)) {
+ retval = -ENODEV;
+ goto fail;
+ }
if (!HC_IS_RUNNING(hcd->state)) {
retval = -ENODEV;
goto fail;
diff --git a/drivers/usb/host/ohci-hub.c b/drivers/usb/host/ohci-hub.c
index e01e77b..72e3b12 100644
--- a/drivers/usb/host/ohci-hub.c
+++ b/drivers/usb/host/ohci-hub.c
@@ -53,6 +53,11 @@
spin_lock_irqsave (&ohci->lock, flags);
+ if (unlikely(!test_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags))) {
+ spin_unlock_irqrestore (&ohci->lock, flags);
+ return -ESHUTDOWN;
+ }
+
ohci->hc_control = ohci_readl (ohci, &ohci->regs->control);
switch (ohci->hc_control & OHCI_CTRL_HCFS) {
case OHCI_USB_RESUME:
@@ -140,11 +145,19 @@
struct ohci_hcd *ohci = hcd_to_ohci (hcd);
u32 temp, enables;
int status = -EINPROGRESS;
+ unsigned long flags;
if (time_before (jiffies, ohci->next_statechange))
msleep(5);
- spin_lock_irq (&ohci->lock);
+ spin_lock_irqsave (&ohci->lock, flags);
+
+ if (unlikely(!test_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags))) {
+ spin_unlock_irqrestore (&ohci->lock, flags);
+ return -ESHUTDOWN;
+ }
+
+
ohci->hc_control = ohci_readl (ohci, &ohci->regs->control);
if (ohci->hc_control & (OHCI_CTRL_IR | OHCI_SCHED_ENABLES)) {
@@ -179,7 +192,7 @@
ohci_dbg (ohci, "lost power\n");
status = -EBUSY;
}
- spin_unlock_irq (&ohci->lock);
+ spin_unlock_irqrestore (&ohci->lock, flags);
if (status == -EBUSY) {
(void) ohci_init (ohci);
return ohci_restart (ohci);
@@ -297,8 +310,8 @@
/* handle autosuspended root: finish resuming before
* letting khubd or root hub timer see state changes.
*/
- if ((ohci->hc_control & OHCI_CTRL_HCFS) != OHCI_USB_OPER
- || !HC_IS_RUNNING(hcd->state)) {
+ if (unlikely((ohci->hc_control & OHCI_CTRL_HCFS) != OHCI_USB_OPER
+ || !HC_IS_RUNNING(hcd->state))) {
can_suspend = 0;
goto done;
}
@@ -508,6 +521,9 @@
u32 temp;
int retval = 0;
+ if (unlikely(!test_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags)))
+ return -ESHUTDOWN;
+
switch (typeReq) {
case ClearHubFeature:
switch (wValue) {
diff --git a/drivers/usb/host/ohci-pci.c b/drivers/usb/host/ohci-pci.c
index 5f22e65..1b09dde 100644
--- a/drivers/usb/host/ohci-pci.c
+++ b/drivers/usb/host/ohci-pci.c
@@ -105,13 +105,36 @@
static int ohci_pci_suspend (struct usb_hcd *hcd, pm_message_t message)
{
- /* root hub was already suspended */
- return 0;
+ struct ohci_hcd *ohci = hcd_to_ohci (hcd);
+ unsigned long flags;
+ int rc = 0;
+
+ /* Root hub was already suspended. Disable irq emission and
+ * mark HW unaccessible, bail out if RH has been resumed. Use
+ * the spinlock to properly synchronize with possible pending
+ * RH suspend or resume activity.
+ *
+ * This is still racy as hcd->state is manipulated outside of
+ * any locks =P But that will be a different fix.
+ */
+ spin_lock_irqsave (&ohci->lock, flags);
+ if (hcd->state != HC_STATE_SUSPENDED) {
+ rc = -EINVAL;
+ goto bail;
+ }
+ ohci_writel(ohci, OHCI_INTR_MIE, &ohci->regs->intrdisable);
+ (void)ohci_readl(ohci, &ohci->regs->intrdisable);
+ clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
+ bail:
+ spin_unlock_irqrestore (&ohci->lock, flags);
+
+ return rc;
}
static int ohci_pci_resume (struct usb_hcd *hcd)
{
+ set_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
usb_hcd_resume_root_hub(hcd);
return 0;
}
diff --git a/drivers/usb/host/uhci-hcd.c b/drivers/usb/host/uhci-hcd.c
index d33ce39..ed55013 100644
--- a/drivers/usb/host/uhci-hcd.c
+++ b/drivers/usb/host/uhci-hcd.c
@@ -717,6 +717,7 @@
* at the source, so we must turn off PIRQ.
*/
pci_write_config_word(to_pci_dev(uhci_dev(uhci)), USBLEGSUP, 0);
+ clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
uhci->hc_inaccessible = 1;
hcd->poll_rh = 0;
@@ -733,6 +734,11 @@
dev_dbg(uhci_dev(uhci), "%s\n", __FUNCTION__);
+ /* We aren't in D3 state anymore, we do that even if dead as I
+ * really don't want to keep a stale HCD_FLAG_HW_ACCESSIBLE=0
+ */
+ set_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
+
if (uhci->rh_state == UHCI_RH_RESET) /* Dead */
return 0;
spin_lock_irq(&uhci->lock);