Re: [ PATCH ] PowerPC cascade UIC IRQ handler fix.

[David Gibson <david@gibson.dropbear.id.au>]

On Fri, Aug 03, 2007 at 11:18:09AM +1000, Benjamin Herrenschmidt wrote:
> On Thu, 2007-08-02 at 13:48 +1000, David Gibson wrote:
> > On Mon, Jul 30, 2007 at 08:35:17PM +0400, Valentine Barshak wrote:
> > > 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 <vbarshak@ru.mvista.com>
> > 
> > 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.

> 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.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson