Here's the failure mode:
On a 405LP (using ppc405_pic), an irq is becoming unmasked[1] (in
UIC0_ER). ppc_irq_dispatch_handler sees irq_desc[0].action == NULL, and
correctly complains of an unhandled interrupt and masks it off. However
at the end of the same function (label "out"), the irq is unmasked again
because irq_desc[0].handler == ppc405_pic (irq_desc[0..NR_IRQS] =
ppc405_pic; see ppc4xx_init_IRQ() ). So the irq is unmasked, occurs
again, is masked, is unmasked, occurs again...
The attached patch fixes the problem by checking desc->action as well as
desc->handler - there is no sense unmasking an interrupt if we already
know there are no drivers ready to handle it.
[1] In my particular case, UIC0_ER is being modified by BIOS when waking
from sleep (the BIOS later returns control to Linux), which could be
worked around in wakeup code. However in the interest of handling as
many unexpected hardware and software errors as possible, I believe this
patch should be applied.
-Hollis
--
PowerPC Linux
IBM Linux Technology Center
Hollis Blanchard writes:
Here's the failure mode:
On a 405LP (using ppc405_pic), an irq is becoming unmasked[1] (in
UIC0_ER). ppc_irq_dispatch_handler sees irq_desc[0].action == NULL, and
correctly complains of an unhandled interrupt and masks it off. However
at the end of the same function (label "out"), the irq is unmasked again
because irq_desc[0].handler == ppc405_pic (irq_desc[0..NR_IRQS] =
ppc405_pic; see ppc4xx_init_IRQ() ). So the irq is unmasked, occurs
again, is masked, is unmasked, occurs again...
The attached patch fixes the problem by checking desc->action as well as
desc->handler - there is no sense unmasking an interrupt if we already
know there are no drivers ready to handle it.
This is the wrong fix IMO. The situation is no different to calling
disable_irq during an interrupt handler, and as the comment says, the
->end() handler has to deal with that. The problem is that if we
don't have an ->end() handler we unconditionally call the ->enable()
handler, which (correctly) just unconditionally enables the
interrupt. So the bit of code at the end of ppc_irq_dispatch_handler
should look like this:
/*
* The ->end() handler has to deal with interrupts which got
* disabled while the handler was running.
*/
if (irq_desc[irq].handler) {
if (irq_desc[irq].handler->end)
irq_desc[irq].handler->end(irq);
else if (irq_desc[irq].handler->enable
&& !(irq_desc[irq_nr].status
& (IRQ_DISABLED|IRQ_INPROGRESS)))
irq_desc[irq].handler->enable(irq);
}
[1] In my particular case, UIC0_ER is being modified by BIOS when waking
BIOS??? What BIOS?
Paul.
** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/
On Fri, 2002-11-22 at 16:45, Paul Mackerras wrote:
This is the wrong fix IMO. The situation is no different to calling
disable_irq during an interrupt handler, and as the comment says, the
->end() handler has to deal with that. The problem is that if we
don't have an ->end() handler we unconditionally call the ->enable()
handler, which (correctly) just unconditionally enables the
interrupt.
You're right, not the right fix. I was on a obsolete tree which did not
have a ppc405_uic_end.
But why not require all handlers to have an ->end() handler? If you look
at arch/i386/kernel/irq.c do_IRQ you'll see they just assume there is an
end, simplifying code and removing at least one conditional from the irq
path. In linuxppc-2.5 there are 4 pic drivers entirely without end
handlers: adir_pic, cpc700_pic, gt64260_pic, and ppc403_pic.
-Hollis
--
PowerPC Linux
IBM Linux Technology Center
** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/