Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 8de98402 authored by Benjamin Herrenschmidt's avatar Benjamin Herrenschmidt Committed by Greg Kroah-Hartman
Browse files

[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: default avatarBenjamin Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: default avatarGreg Kroah-Hartman <gregkh@suse.de>
Signed-off-by: default avatarGreg Kroah-Hartman <gregkh@suse.de>
parent d3420ba4

drivers/usb/core/hcd-pci.c

+2 −1
Original line number Diff line number Diff line
@@ -219,6 +219,7 @@ int usb_hcd_pci_suspend (struct pci_dev *dev, pm_message_t message) 
			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 @@ int usb_hcd_pci_resume (struct pci_dev *dev) 


	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);

drivers/usb/core/hcd.c

+10 −5
Original line number Diff line number Diff line
@@ -1315,11 +1315,12 @@ static int hcd_unlink_urb (struct urb *urb, int status) 
	 * 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 @@ irqreturn_t usb_hcd_irq (int irq, void *__hcd, struct pt_regs * r) 
	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 @@ int usb_add_hcd(struct usb_hcd *hcd, 


	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");

drivers/usb/core/hcd.h

+6 −1
Original line number Diff line number Diff line
@@ -72,7 +72,12 @@ struct usb_hcd { /* usb_bus.hcpriv points to this */ 
	 * 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? */

drivers/usb/host/ehci-pci.c

+26 −1
Original line number Diff line number Diff line
@@ -228,14 +228,36 @@ static int ehci_pci_reset(struct usb_hcd *hcd) 
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 @@ static int ehci_pci_resume(struct usb_hcd *hcd) 
	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;

drivers/usb/host/ehci-q.c

+16 −8
Original line number Diff line number Diff line
@@ -912,6 +912,7 @@ submit_async ( 
	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 @@ submit_async ( 
#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));

	}

 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;

}



/*-------------------------------------------------------------------------*/