Re: [PATCH] fix IRQ reentrancy

[Chris Gilbert <chris%dokein.co.uk@localhost>]

Mikko Rapeli wrote:
> Current locks up hard when under load like compiling a kernel. Following
> patch fixes this on yesterdays current. Compilation of kernel on OMAP
> 2420 fails for some other reason but the lockups don't happen anymore.
>
> Please apply.
>
>   

I've taken a quick look over the patch, and I'm not so sure that the
changes to make things a two dimensional array are actually needed.

In theory (although I admit I'm as intimately  familiar with the pic
code)  one set of  should be enough, as when the IRQ handler returns it
should leave it in the state is was in on entry to the handler.

I might take a stab at porting another platform to the PIC stuff and see
if the issues are the same.  My concern is that the irq code should be
as simple and quick as we can make it, adding 2d arrays and other things


> -Mikko
>
> Author: Imre Deak <imre.deak%teleca.com@localhost>
> Date:   Wed Jun 18 13:35:12 2008 +0300
>
>     ARM: IRQ: fix IRQ reentrancy
>     
>     - Fix the case when an IRQ with a higher IPL preempts another
>       IRQ handler.
>     
>       Currently the blocked PIC mask is global and the blocked IRQ
>       mask is per PIC, so that each pending PIC/IRQ is marked in
>       these masks regardless of their IPL. After the higher IPL
>       preempts the lower one and the corresponding handlers are run,
>       processed but still unblocked lower IPL IRQs might be cleared
>       from the blocked masks without actually getting unblocked, thus
>       future IRQs will be lost.
>     
>       The patch changes the blocked PIC/IRQ masks into a per IPL
>       array, so IRQs can be unblocked on a per IPL manner.
>     
>     - Remove the unused irq_count from pic_deliver_irqs. This value
>       is anyway the same as irq_base.
>     
>     - Fix the case when an IRQ is triggered during the execution of
>       another IRQ's handler on the same IPL / PIC. This could happen
>       if the new IRQ is in a group that has already been processed
>       in pic_deliver_irqs and not rechecked again.
>     
>       To fix this do the IPL unmarking (and if necessary that of the PIC)
>       before IRQs are reenabled, so that any new IRQs on the same IPL / PIC
>       will be marked correctly. Start over to check the first IRQ group
>       if there has been any new IRQs during the previous round
Sounds necessary.

>     
>     - Change the progress KASSERT to one which checks if the IPL was indeed
>       unmarked as the result of processing this PIC / IPL.
>     
>     - Don't recalculate pending IRQs in the delivery loop. There won't
>       be any new one until IRQ unblocking, which happens only after the
>       whole IPL is processed.
>   

Unless it works differently, I thought we lazy blocked IRQs IE only
masked them out if they occur, to avoid hammering on the hardware more
than we need to?  So we need to check after every delivery loop...

IE we block the pending interrupts, if any other interrupt occurs (lower
or higher IPL) it's noted and either processed (if higher) of marked as
pending, and the IRQ handler exited, eg:

IRQ 1 (IPL_VM) happens
IRQ dispatch entered
mask out IRQ 1
unblock interrupts
Process IRQ 1

IRQ2 (IPL_VM) happens.
IRQ dispatch entered
mask out IRQ 2
find we're at IPL_VM, so flag as pending
IRQ dispatch exitted

Complete IRQ 1 processing
Block interrupts
unmask IRQ 1
find pending IRQ 2
unblock interrupts
process IRQ 2

IRQ 3 (IPL_SCHED) happens
IRQ dispatch entered
mask out IRQ 3
IPL_SCHED > IPL_VM
unblock interrupts
process IRQ 3
block interrupts
check pending, find none.
unmask IRQ 3
IRQ dispatch exits

complete IRQ 2
block interrupts
check finding, find none
unmask IRQ 2
IRQ dispatch exits back to userland/idle

>     
>     - Add a pending flag to interrupts sources so we can KASSERT on
>       IRQ handlers not beeing reentered.

Shouldn't that be a bool, as it's used like one?

>     
>     - In case of chained IRQ handlers, like the OMAP gpio IRQ,
>       pic_handle_intr is called to mark pending interrupts on the
>       chained PIC. This should be done with interrupts disabled,
>       to protect global data like pic_pending_ipls.
>
>   

Yes, anything playing with pic_pending_ipls should be doing so with IRQs
blocked :)

Thanks,
Chris