Re: Interrupt handling documentation

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

On Mon, 2008-03-17 at 17:13 +0100, Laurent Pinchart wrote:

> The PIC I am working with is linked to a falling-edge external irq on the 
> CPM2. When the first PIC interrupt was generated the kernel called the PIC 
> chained irq handler endlessly.
> 
> After some investigation it turned out the external interrupt bit in the CPM2 
> interrupt pending register never got cleared. set_irq_chained_handler() 
> registers the chained irq handler at the lowest level in the irq stack, 
> bypassing all the interrupt acknowledgement/masking logic.

Yes, exactly. To answer a previous question in the thread, the reason
there are two approaches to cascades is just that:

 - The "easy" approach using setup_irq(). The normal interrupt handling
is done for the cascade, it's masked/acked/whatever-is-needed as any
other interrupt before the second interrupt is fetched. This results
is slightly more kernel stack usage and overhead in getting to the
second interrupt, among other things, but is easier.

 - The "fast" approach using a chained handler. This, as you noticed,
bypass pretty much the whole stack and calls the chain handler directly.
That means that your chain handler is responsible to perform all the
necessary things to ensure the cascade interrupt is properly ack'ed
etc...
 
> The fix was easy, all I had to do was to call desc->chip->ack(irq) at the 
> beginning on the chained irq handler and desc->chip->eoi(irq) at the end. 

For an edge cascade, that would do, I suppose. But beware that if you
are only calling ack() and not mask(), then a subsequent chain interrupt
from the same cascade can (and will) potentially happen while you are
calling the handler as the cascade itself has been ack'ed and not
masked. In the case of cpm2, that also probably means you don't need to
call end().

That might be fine though, but it increases the chances of having of
stack overflows caused by interrupts stacking up.

> However, I'm wondering if this really belongs in the PIC code, or if PICs 
> shouldn't be registered at a higher level (setup_irq or even request_irq) so 
> that they would reuse the handle_*_irq handlers. Any opinion on this ?

They can. The chain handling mechanism is an optimisation. It avoids a
spinlock and other bits & pieces which improve performance & latency of
handling cascaded interrupts, at the expense of that added complexity.

Ben.