[ PATCH ] PowerPC cascade UIC IRQ handler fix.

STALE6929d

10 messages, 4 authors, 2007-08-14 · open the first message on its own page

[ PATCH ] PowerPC cascade UIC IRQ handler fix.

From: Valentine Barshak <hidden>
Date: 2007-07-30 16:36:20 

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

--- linux.orig/arch/powerpc/sysdev/uic.c	2007-07-27 20:37:11.000000000 +0400
+++ linux/arch/powerpc/sysdev/uic.c	2007-07-30 20:26:48.000000000 +0400
@@ -142,7 +142,7 @@
 
 	desc->status &= ~(IRQ_TYPE_SENSE_MASK | IRQ_LEVEL);
 	desc->status |= flow_type & IRQ_TYPE_SENSE_MASK;
-	if (trigger)
+	if (!trigger)
 		desc->status |= IRQ_LEVEL;
 
 	spin_unlock_irqrestore(&uic->lock, flags);
@@ -207,6 +207,9 @@
 	int subvirq;
 
 	msr = mfdcr(uic->dcrbase + UIC_MSR);
+	if (!msr)
+		return IRQ_HANDLED;
+
 	src = 32 - ffs(msr);
 
 	subvirq = irq_linear_revmap(uic->irqhost, src);

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

From: David Gibson <hidden>
Date: 2007-08-02 03:48:48 

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

--- linux.orig/arch/powerpc/sysdev/uic.c	2007-07-27 20:37:11.000000000 +0400
+++ linux/arch/powerpc/sysdev/uic.c	2007-07-30 20:26:48.000000000 +0400
@@ -142,7 +142,7 @@
 
 	desc->status &= ~(IRQ_TYPE_SENSE_MASK | IRQ_LEVEL);
 	desc->status |= flow_type & IRQ_TYPE_SENSE_MASK;
-	if (trigger)
+	if (!trigger)
 		desc->status |= IRQ_LEVEL;
 
 	spin_unlock_irqrestore(&uic->lock, flags);
@@ -207,6 +207,9 @@
 	int subvirq;
 
 	msr = mfdcr(uic->dcrbase + UIC_MSR);
+	if (!msr)
+		return IRQ_HANDLED;
+
 	src = 32 - ffs(msr);
 
 	subvirq = irq_linear_revmap(uic->irqhost, src);
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
-- 
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

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

From: Josh Boyer <hidden>
Date: 2007-08-02 20:10:30 

On Thu, 2 Aug 2007 13:48:48 +1000
David Gibson [off-list ref] wrote:

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.
Agreed.  Anyone going to hack up a patch for that?

josh

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

From: Valentine Barshak <hidden>
Date: 2007-08-03 11:10:28 

Josh Boyer wrote:
On Thu, 2 Aug 2007 13:48:48 +1000
David Gibson [off-list ref] 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.
Agreed.  Anyone going to hack up a patch for that?

josh
Anyways, I don't see why kernel should panic if we get a spurious interrupt.

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

From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Date: 2007-08-03 01:18:09 

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

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.

Ben.

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

From: David Gibson <hidden>
Date: 2007-08-03 04:57:05 

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

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

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

From: David Gibson <hidden>
Date: 2007-08-03 06:23:46 

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

Index: working-2.6/arch/powerpc/sysdev/uic.c
===================================================================
--- working-2.6.orig/arch/powerpc/sysdev/uic.c	2007-08-03 16:09:48.000000000 +1000
+++ working-2.6/arch/powerpc/sysdev/uic.c	2007-08-03 16:09:49.000000000 +1000
@@ -24,6 +24,7 @@
 #include <linux/spinlock.h>
 #include <linux/irq.h>
 #include <linux/interrupt.h>
+#include <linux/kernel_stat.h>
 #include <asm/irq.h>
 #include <asm/io.h>
 #include <asm/prom.h>
@@ -159,6 +160,56 @@ static struct irq_chip uic_irq_chip = {
 	.set_type	= uic_set_irq_type,
 };
 
+/**
+ *	handle_uic_irq - Level type irq handler
+ *	@irq:	the interrupt number
+ *	@desc:	the interrupt description structure for this irq
+ *
+ *	Level type interrupts are active as long as the hardware line has
+ *	the active level. This may require to mask the interrupt and unmask
+ *	it after the associated handler has acknowledged the device, so the
+ *	interrupt line is back to inactive.
+ */
+void fastcall
+handle_uic_irq(unsigned int irq, struct irq_desc *desc)
+{
+	unsigned int cpu = smp_processor_id();
+	struct irqaction *action;
+	irqreturn_t action_ret;
+
+	spin_lock(&desc->lock);
+	desc->chip->mask(irq);
+
+	if (unlikely(desc->status & IRQ_INPROGRESS))
+		goto out_unlock;
+	desc->status &= ~(IRQ_REPLAY | IRQ_WAITING);
+	kstat_cpu(cpu).irqs[irq]++;
+
+	/*
+	 * If its disabled or no action available
+	 * keep it masked and get out of here
+	 */
+	action = desc->action;
+	if (unlikely(!action || (desc->status & IRQ_DISABLED))) {
+		desc->status |= IRQ_PENDING;
+		goto out_unlock;
+	}
+
+	desc->status |= IRQ_INPROGRESS;
+	desc->status &= ~IRQ_PENDING;
+	spin_unlock(&desc->lock);
+
+	action_ret = handle_IRQ_event(irq, action);
+
+	spin_lock(&desc->lock);
+	desc->status &= ~IRQ_INPROGRESS;
+	desc->chip->ack(irq);
+	if (!(desc->status & IRQ_DISABLED) && desc->chip->unmask)
+		desc->chip->unmask(irq);
+out_unlock:
+	spin_unlock(&desc->lock);
+}
+
 static int uic_host_match(struct irq_host *h, struct device_node *node)
 {
 	struct uic *uic = h->host_data;
@@ -173,7 +224,7 @@ static int uic_host_map(struct irq_host 
 	set_irq_chip_data(virq, uic);
 	/* Despite the name, handle_level_irq() works for both level
 	 * and edge irqs on UIC.  FIXME: check this is correct */
-	set_irq_chip_and_handler(virq, &uic_irq_chip, handle_level_irq);
+	set_irq_chip_and_handler(virq, &uic_irq_chip, handle_uic_irq);
 
 	/* Set default irq type */
 	set_irq_type(virq, IRQ_TYPE_NONE);


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

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

From: David Gibson <hidden>
Date: 2007-08-13 01:08:48 

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:
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?

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

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

From: Valentine Barshak <hidden>
Date: 2007-08-13 12:32:06 

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.

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

From: David Gibson <hidden>
Date: 2007-08-14 01:35:10 

On Mon, Aug 13, 2007 at 04:31:03PM +0400, Valentine Barshak wrote:
[snip]
quoted
quoted
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).
Ok, great.

Don't think we really need a "fastcall" here on a powerpc though.
Oh, yeah, that's just copied from the generic handle_level_irq().

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.
Yes, we'll still want two patches similar to your original: one to fix
the cosmetic /proc/interrupts problem, the other to make the cascade
handler more robust against spurious interrupts.  I just wanted to see
if this flow handler change fixed the basic problem.

-- 
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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help