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

[David Brownell]

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 ... :)

- dave


====== CUT HERE
Add a workaround for the way the IRQ framework can disregard
IRQF_DISABLED when IRQF_SHARED is set and lockdep isn't in use.

This leaves IRQF_DISABLED active for unshared IRQs, but clears
that flag otherwise (since the genirq code should eventually
start warning folk about that bad combination of flags).

(Thanks to Stefan Becker <Stefan.Becker@xxxxxxxxx> for tracking
down the source of hard lockups on his hardware; on some systems
this problem could lead to oopsing.)

Signed-off-by: David Brownell <dbrownell@xxxxxxxxxxxxxxxxxxxxx>
---
UNTESTED but "obviously right" yes?  Candidate for 2.6.26-rc8+ ...

 drivers/usb/core/hcd.c |   22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

--- a/drivers/usb/core/hcd.c	2008-06-30 13:09:00.000000000 -0700
+++ b/drivers/usb/core/hcd.c	2008-06-30 13:28:04.000000000 -0700
@@ -1687,19 +1687,31 @@ 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;
+	int			start;
+	unsigned long		flags;
+	irqreturn_t		value = IRQ_NONE;
+
+	/* IRQF_DISABLED doesn't work correctly with shared IRQs
+	 * when the first handler doesn't use it.  So let's just
+	 * assume it's never used.
+	 */
+	local_irq_save(flags);
 
+	start = hcd->state;
 	if (unlikely(start == HC_STATE_HALT ||
 	    !test_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags)))
-		return IRQ_NONE;
+		goto done;
 	if (hcd->driver->irq (hcd) == IRQ_NONE)
-		return IRQ_NONE;
+		goto done;
 
+	value = IRQ_HANDLED;
 	set_bit(HCD_FLAG_SAW_IRQ, &hcd->flags);
 
 	if (unlikely(hcd->state == HC_STATE_HALT))
 		usb_hc_died (hcd);
-	return IRQ_HANDLED;
+done:
+	local_irq_restore(flags);
+	return value;
 }
 
 /*-------------------------------------------------------------------------*/
@@ -1865,6 +1877,8 @@ int usb_add_hcd(struct usb_hcd *hcd,
 	if (hcd->driver->irq) {
 		snprintf(hcd->irq_descr, sizeof(hcd->irq_descr), "%s:usb%d",
 				hcd->driver->description, hcd->self.busnum);
+		/* work around (IRQF_SHARED|IRQF_DISABLED) misbehavior.. */
+		irqflags &= ~IRQF_DISABLED;
 		if ((retval = request_irq(irqnum, &usb_hcd_irq, irqflags,
 				hcd->irq_descr, hcd)) != 0) {
 			dev_err(hcd->self.controller,
--
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/