ppc_irq_dispatch_handler and unhandled interrupts

ppc_irq_dispatch_handler and unhandled interrupts

[Hollis Blanchard]

[-- Attachment #1: Type: text/plain, Size: 1068 bytes --]

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

[-- Attachment #2: unhandled-irq.diff --]
[-- Type: text/plain, Size: 430 bytes --]

--- arch/ppc/kernel/irq.c	2001/12/18 00:18:00	1.1.1.2
+++ arch/ppc/kernel/irq.c	2002/11/21 16:13:22
@@ -513,7 +513,7 @@
 	 * The ->end() handler has to deal with interrupts which got
 	 * disabled while the handler was running.
 	 */
-	if (irq_desc[irq].handler) {
+	if (desc->handler && desc->action) {
 		if (irq_desc[irq].handler->end)
 			irq_desc[irq].handler->end(irq);
 		else if (irq_desc[irq].handler->enable)

Re: ppc_irq_dispatch_handler and unhandled interrupts

[Matt Porter]

On Thu, Nov 21, 2002 at 10:58:55AM -0600, Hollis Blanchard wrote:
> 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.

As I mentioned on IRC, I think this is a Good Thing(tm).  This cleanly
handles the case where somebody leaves an unhooked interrupt unmasked.
It's another good example of hardening the kernel by handling error
conditions smartly.  Unless anybody has serious objections to this
additional check, I'd like to push it in.

Regards,
--
Matt Porter
porter@cox.net
This is Linux Country. On a quiet night, you can hear Windows reboot.

** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

Re: ppc_irq_dispatch_handler and unhandled interrupts

[Paul Mackerras]

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/

Re: ppc_irq_dispatch_handler and unhandled interrupts

[Hollis Blanchard]

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/