David Gibson wrote:

On Fri, Aug 03, 2007 at 04:23:46PM +1000, David Gibson wrote:

quoted

On Fri, Aug 03, 2007 at 02:57:05PM +1000, David Gibson wrote:

quoted

On Fri, Aug 03, 2007 at 11:18:09AM +1000, Benjamin Herrenschmidt wrote:

quoted

On Thu, 2007-08-02 at 13:48 +1000, David Gibson wrote:

quoted

On Mon, Jul 30, 2007 at 08:35:17PM +0400, Valentine Barshak wrote:

quoted

PPC44x cascade UIC irq handler fix.

According to PPC44x UM, if an interrupt is configured as level-sensitive,
and a clear is attempted on the UIC_SR, the UIC_SR field is not
cleared if the incoming interrupt signal is at the asserted polarity.
This causes us to enter a cascade handler twice, since we first ack
parent UIC interrupt and ack child UIC one after that.
The patch checks child UIC msr value and returns IRQ_HANDLED
if there're no pending interrupts. Otherwise we get a kernel panic
with a "Fatal exception in interrupt" (illegal vector).
The patch also fixes status flags.

Signed-off-by: Valentine Barshak <redacted>

Hrm... This doesn't seem like the right fix to me.  Instead, I think
the cascaded IRQ handler should ack the interrupt on the child first.
I'm a little surprised it doesn't at the moment.

Well, we certainly do also need to make the code more solid vs.
spurrious interrupts.

Actually that's true.  The suggested patch is a good improvement for
general robustness, but doesn't actually address the real problem.

quoted

The main thing is, if the cascade is a level interrupt, it should
probably use a smarter cascade handler that masks it, handle the child
interrupts, then unmasks it.

We already have that, since I just use setup_irq() to set up a cascade
handler, rather than a custom flow handler.

The problem is that the standard handle_level_irq() flow handler acks
before the ISR is called, whereas because of this UIC behaviour, we
need to ack after the ISR has cleared the interrupt in the source.
This is not specific to cascades, but is a problem for all
level-triggered interrupts (i.e. basically everything).

I think it means we must currently be getting a whole lot of spurious
interrupts - will do some investigation in a moment.

To fix this either we'll need a custom flow handler for UIC, or we can
use the standard one, but clear the UIC_SR bits from the ->unmask()
callback for level interrupts.  I'm not entirely sure if the latter
approach is safe - I *think* it is, but I could do with more
convincing.

Ok, here's a patch which fixes up the flow handling on the UIC.  It
needs some polish yet, but should be ok to test.  Valentine, can you
test this on your setup, *without* your original proposed patch.
Eventually, for robustness, we'll want something like your original
patch as well for robustness, but in the meantime leaving it out
should tell us if my patch is actually having the intended effect.

Valentine, it would be really helpful if you could test this on the
problem you observed with the cascade interrupt.  Any word on this?

Thanks David,
the patch works fine here (without the original one).
Don't think we really need a "fastcall" here on a powerpc though.
The original patch also fixes a minor issue with /proc/interrupts
(the the "if (trigger)" stuff).
Currently level-triggered interrupts are displayed as edge-triggered 
ones and vice versa.

Thanks,
Valentine.