Skip to contentSkip to conversation

usb: Remove broken optimisation in OHCI IRQ handler

PATCH In review Benjamin Herrenschmidt 2 participants 3 messages Nov 25, 2007 – Dec 6, 2007

Subsystems (2)
USB USB ohci driver
lore
Share & tools
lore

Benjamin Herrenschmidt

author
#0 lore raw

PATCH 1/2 usb: Remove broken optimisation in OHCI IRQ handler

The OHCI IRQ handler has an optimisation that tries to avoid reading the status register when it sees something has been put in the controller "done list".

This optimisation is broken on controllers that use edge interrupt signaling as it relies on "missed" interrupt to be re-emitted which is not the case with egde interrupts. Among others, it breaks the OHCI SoC controller in the AMCC 440EP PowerPC processor (and according to David, it also breaks the SA1111).

This patch removes the optimisation along with making the code a little bit less convoluted.

Signed-off-by Benjamin Herrenschmidt <[email protected]>

---

While there, any reason why we do the read of the interenable register and mask ? Is that actually useful in practice ? I haven't removed it but it might be a good candidate if we want to save on MMIO reads.

 drivers/usb/host/ohci-hcd.c |   23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)
drivers/usb/host/ohci-hcd.c +12 -11
Index: linux-work/drivers/usb/host/ohci-hcd.c
===================================================================
@@ -732,24 +732,25 @@ static irqreturn_t ohci_irq (struct usb_
 	struct ohci_regs __iomem *regs = ohci->regs;
 	int			ints;
 
-	/* we can eliminate a (slow) ohci_readl()
-	 * if _only_ WDH caused this irq
-	 */
-	if ((ohci->hcca->done_head != 0)
-			&& ! (hc32_to_cpup (ohci, &ohci->hcca->done_head)
-				& 0x01)) {
-		ints =  OHCI_INTR_WDH;
+	/* Read interrupt status & flush pending DMAs */
+	ints = ohci_readl (ohci, &regs->intrstatus);
 
-	/* cardbus/... hardware gone before remove() */
-	} else if ((ints = ohci_readl (ohci, &regs->intrstatus)) == ~(u32)0) {
+	/* Check for an all 1's result which is the consequence of a
+	 * dead or unplugged device
+	 */
+	if (ints == ~(u32)0) {
 		disable (ohci);
 		ohci_dbg (ohci, "device removed!\n");
 		return IRQ_HANDLED;
 
+	}
+
+	/* We only care about interrupts that are enabled */
+	ints &= ohci_readl (ohci, &regs->intrenable);
+
 	/* interrupt for some other device? */
-	} else if ((ints &= ohci_readl (ohci, &regs->intrenable)) == 0) {
+	if (ints == 0)
 		return IRQ_NOTMINE;
-	}
 
 	if (ints & OHCI_INTR_UE) {
 		// e.g. due to PCI Master/Target Abort
#1 lore raw
Quoted 3 lines · Benjamin Herrenschmidt

While there, any reason why we do the read of the interenable register and mask ? Is that actually useful in practice ? I haven't removed it but it might be a good candidate if we want to save on MMIO reads.

The code uses that register to keep track of which IRQs are enabled or disabled, and those enabled IRQs are changed from time to time.

I don't know of any good reason not to keep an in-memory copy of the resulting mask, though I'd keep an eye out for chip errata.

- Dave

#2 lore raw
In reply to Benjamin Herrenschmidt

From: Benjamin Herrenschmidt <[email protected]>
Subject: [PATCH 1/2] usb: Remove broken optimisation in OHCI IRQ handler

The OHCI IRQ handler has an optimisation that avoids reading some chip registers when the controller reports that the interrupt was triggered *only* because completed requests were written into the controller's "done list" and handed to the host.

This mechanism can't be used on some controllers. Among others, it fails for the SA1111 and the AMCC 440EP PowerPC processor.

This patch removes the optimisation and makes the code clearer.

Signed-off-by Benjamin Herrenschmidt <[email protected]>
Signed-off-by David Brownell <[email protected]>

---
Suitable IMO for 2.6.24 final.

 drivers/usb/host/ohci-hcd.c |   23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)
drivers/usb/host/ohci-hcd.c +13 -10
@@ -732,24 +732,27 @@ static irqreturn_t ohci_irq (struct usb_
 	struct ohci_regs __iomem *regs = ohci->regs;
 	int			ints;
 
-	/* we can eliminate a (slow) ohci_readl()
-	 * if _only_ WDH caused this irq
+	/* Read interrupt status (and flush pending writes).  We ignore the
+	 * optimization of checking the LSB of hcca->done_head; it doesn't
+	 * work on all systems (edge triggering for OHCI can be a factor).
 	 */
-	if ((ohci->hcca->done_head != 0)
-			&& ! (hc32_to_cpup (ohci, &ohci->hcca->done_head)
-				& 0x01)) {
-		ints =  OHCI_INTR_WDH;
+	ints = ohci_readl(ohci, &regs->intrstatus);
 
-	/* cardbus/... hardware gone before remove() */
-	} else if ((ints = ohci_readl (ohci, &regs->intrstatus)) == ~(u32)0) {
+	/* Check for an all 1's result which is a typical consequence
+	 * of dead, unclocked, or unplugged (CardBus...) devices
+	 */
+	if (ints == ~(u32)0) {
 		disable (ohci);
 		ohci_dbg (ohci, "device removed!\n");
 		return IRQ_HANDLED;
+	}
+
+	/* We only care about interrupts that are enabled */
+	ints &= ohci_readl(ohci, &regs->intrenable);
 
 	/* interrupt for some other device? */
-	} else if ((ints &= ohci_readl (ohci, &regs->intrenable)) == 0) {
+	if (ints == 0)
 		return IRQ_NOTMINE;
-	}
 
 	if (ints & OHCI_INTR_UE) {
 		// e.g. due to PCI Master/Target Abort