David Gibson wrote: > On Fri, Aug 03, 2007 at 04:23:46PM +1000, David Gibson wrote: >> On Fri, Aug 03, 2007 at 02:57:05PM +1000, David Gibson wrote: >>> 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 <[EMAIL PROTECTED]> >>>>> 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. >> 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. _______________________________________________ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
