[PATCH] USB: fix interrupt disabling for HCDs with shared interrupt handlers

[Stefan Becker <Stefan.Becker@nokia.com>]

[-- Attachment #1: Type: text/plain, Size: 562 bytes --]

Hi,

ext David Brownell wrote:
> On Monday 30 June 2008, Alan Stern wrote:
>> Don't bother with this extra stuff.  All USB host controller drivers
>> want to have interrupts disabled when their IRQ handlers run.
> 
> How about this one instead?  I think it's probably almost midnight
> where Stefan is, so I'd not expect Stefan to have an updated
> patch very soon ... :)

It is, put the rain pouring down on my house is keeping me awake :-)

Looks like you posted the same changes though...

Regards,

	Stefan

---
Stefan Becker
E-Mail: Stefan.Becker@nokia.com

[-- Attachment #2: usb-fix-shared-interrupts-disabling.patch --]
[-- Type: text/plain, Size: 2916 bytes --]

From 36027c10330bdf08d4ba594c37913edda6df3983 Mon Sep 17 00:00:00 2001
From: Stefan Becker <stefan.becker@nokia.com>
Date: Mon, 30 Jun 2008 21:18:29 +0300
Subject: [PATCH] USB: fix interrupt disabling for HCDs with shared interrupt handlers

As has been discussed several times on LKML, IRQF_SHARED | IRQF_DISABLED
doesn't work reliably, i.e. a shared interrupt handler CAN'T be certain to
be called with interrupts disabled. Most USB HCD handlers use IRQF_DISABLED
and therefore havoc can break out if they share their interrupt with a
handler that doesn't use it.

On my test machine the yenta_socket interrupt handler (no IRQF_DISABLED)
was registered before ehci_hcd and one uhci_hcd instance. Therefore all
usb_hcd_irq() invocations for ehci_hcd and for one uhci_hcd instance
happened with interrupts enabled. That led to random lockups as USB core
HCD functions that acquire the same spinlock could be called twice
from interrupt handlers.

This patch updates usb_hcd_irq() to always disable/restore interrupts.
usb_add_hcd() will silently remove any IRQF_DISABLED requested from HCD code.

Signed-off-by: Stefan Becker <stefan.becker@nokia.com>

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 09a53e7..3d356bd 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -1683,20 +1683,27 @@ EXPORT_SYMBOL_GPL(usb_bus_start_enum);
  */
 irqreturn_t usb_hcd_irq (int irq, void *__hcd)
 {
-	struct usb_hcd		*hcd = __hcd;
-	int			start = hcd->state;
+	struct usb_hcd		*hcd  = __hcd;
+	unsigned int            flags = 0;
+	irqreturn_t             rc;
 
-	if (unlikely(start == HC_STATE_HALT ||
-	    !test_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags)))
-		return IRQ_NONE;
-	if (hcd->driver->irq (hcd) == IRQ_NONE)
-		return IRQ_NONE;
+	local_irq_save(flags);
 
-	set_bit(HCD_FLAG_SAW_IRQ, &hcd->flags);
+	if (unlikely(hcd->state == HC_STATE_HALT ||
+		     !test_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags))) {
+		rc = IRQ_NONE;
+	} else if (hcd->driver->irq (hcd) == IRQ_NONE) {
+		rc = IRQ_NONE;
+	} else {
+		set_bit(HCD_FLAG_SAW_IRQ, &hcd->flags);
+
+		if (unlikely(hcd->state == HC_STATE_HALT))
+			usb_hc_died (hcd);
+		rc = IRQ_HANDLED;
+	}
 
-	if (unlikely(hcd->state == HC_STATE_HALT))
-		usb_hc_died (hcd);
-	return IRQ_HANDLED;
+	local_irq_restore(flags);
+	return rc;
 }
 
 /*-------------------------------------------------------------------------*/
@@ -1860,6 +1867,13 @@ int usb_add_hcd(struct usb_hcd *hcd,
 
 	/* 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.
+		 */
+		irqflags &= ~IRQF_DISABLED;
+
 		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,