Re: OHCI IRQ issues

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

On Friday 23 November 2007, Benjamin Herrenschmidt wrote:
> I have a problem with the current ohci_irq() implementation:

I count more than one issue you raise.  :)

Most of these behaviors date from the old 2.4 version of the
driver, and have been carried forward more out of paranoia
than anything else.

> - The optimisation below breaks if the controller has an edge triggered
> interrupt which is the case on some embedded controllers such as the
> AMCC PowerPC 440EP:

I hope you mean the "test low bit of hcca->done_head" optimization...
FWIW, that also breaks the SA-1111 OHCI controller.  I'm not sure that
it's worth avoiding the register read here, especially since it does
cause problems now and then. 

Seeing that optimization called out in the OHCI spec was actually a
surprise to me ... it wouldn't seem to play well with the usual
implementation flexibility goals, and I've not seen similar things
done with other drivers.  (Which is why for example neither EHCI
nor UHCI could ever try such a thing!)

> 	/* 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;
> 	/* cardbus/... hardware gone before remove() */
> 	} else if ((ints = ohci_readl (ohci, &regs->intrstatus)) == ~(u32)0) {
> 		disable (ohci);
> 		ohci_dbg (ohci, "device removed!\n");
> 		return IRQ_HANDLED;
> We must not do that on edge interrupts because the interrupts we "miss"
> will not be re-triggered as they don't stay asserted.

If so, why aren't you arguing that this should be looping until
no IRQ status bits are set?

I'm not sure what exactly you mean by edge triggering here.  Which
edge(s)?  I've seen individual interrupt status bits act as if they
used rising-edge triggers, but never the primary OHCI interrupt.

Even in SOCs, that primary interrupt has been level triggered.
I think that's at least implicit in the IRQ handling model, in the
way that IRQs need to be acked.

> - There may also be issues with edge interrupts here:
> 	if (ints & OHCI_INTR_RHSC) {
> 	.../...
> 	}
> 	else if (ints & OHCI_INTR_RD) {

Notice the CodingStyle violation ... "else" should be snugged
up against the bracket!

> If for some reason both RHSC and RD are set, we'll never notice RD, is
> that expected ?

That's newish 2.6 behavior, and Alan's answer is right.
The 2.4 code didn't use either interrupt.  Switching over
to IRQs for root hub management made the dynamic tick
stuff work better on ARM.  (That's the NO_HZ precursor.)

Those hub events can be either level or edge triggered,
confusingly enough...

> - I can't understand why we do that masking/unmasking later on:
> 	if (ints & OHCI_INTR_WDH) {
> 		if (HC_IS_RUNNING(hcd->state))
> 			ohci_writel (ohci, OHCI_INTR_WDH, &regs->intrdisable);
> 		spin_lock (&ohci->lock);
> 		dl_done_list (ohci);
> 		spin_unlock (&ohci->lock);
> 		if (HC_IS_RUNNING(hcd->state))
> 			ohci_writel (ohci, OHCI_INTR_WDH, &regs->intrenable);
> 	}
> Those 2 writes to intrdisable look totally useless to me. First they are
> posted so they won't actually protect anything, then, we -are- in the
> interrupt handler for the OHCI, so we cannot take that interrupt again
> recursively anyway. So they are just a waste of cycles as far as I'm
> concerned.

That's stuff that was carried forward from the 2.4 code, and
I've had the same puzzlement.  However, never when I actually
had time to explore the issue (i.e. test it on a wide variety
of OHCI implementations).

The thing is, the 2.4 code tended to just throw lots of oddball
logic in there to work around some chip quirk ... without so
much as a comment about the issue being worked around.  It's
no fun to find out via bug reports that some odd construct was
actually an undocumented workaround for a chip bug.

This one does look silly enough that it'd be worth seeing if
removing it hurts anything.  Let it cook for a while.

> Now, regarding the first problem, my proposal would be to just remove
> the optimisation, I can't see it being very useful in practice. 

OK by me.  I won't worry too much about the cost of one PCI
cycle vs one (uncached) memory access.

> As for the two others, what do you guys think ? I believe the masking in
> WDH handling can just be removed,

I'd hope that's fine...

> and I'm not sure about the else around RD handling.

It does appear to need that CodingStyle fix.

- Dave

This email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
To unsubscribe, use the last form field at:

[Home]     [Video for Linux]     [Photo]     [Yosemite Forum]     [Yosemite Photos]    [Video Projectors]     [PDAs]     [Hacking TiVo]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Devices]     [Big List of Linux Books]     [Free Dating]

  Powered by Linux