Skip to contentSkip to conversation

PowerPC cascade UIC IRQ handler fix.

PATCH In review Valentine Barshak 4 participants 10 messages Jul 30, 2007 – Aug 14, 2007 PowerPC

lore
Share & tools
lore

Valentine Barshak

author
#0 lore raw

PATCH 1/1 PowerPC cascade UIC IRQ handler fix.

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

---

arch/powerpc/sysdev/uic.c +4 -1
@@ -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);
#1 lore raw
Quoted 12 lines · Valentine Barshak

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.

Quoted 1 line
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.

Quoted 28 lines

---

arch/powerpc/sysdev/uic.c +4 -1
@@ -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
[email protected]
https://ozlabs.org/mailman/listinfo/linuxppc-dev

Signature · 4 lines
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
#2 lore raw
Quoted 13 lines · On Thu, 2 Aug 2007 13:48:48 +1000 David Gibson <[email protected]>…
Quoted 12 lines · Valentine Barshak

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.

Quoted 5 lines
Quoted 1 line
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.

Agreed. Anyone going to hack up a patch for that?

josh

author
#3 lore raw
Quoted 16 lines · Josh Boyer
Quoted 13 lines · On Thu, 2 Aug 2007 13:48:48 +1000 David Gibson <[email protected]>…
Quoted 12 lines · Valentine Barshak

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.

Quoted 8 lines
Quoted 4 lines
Quoted 1 line
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.

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.

#4 lore raw
In reply to David Gibson
Quoted 13 lines · David Gibson
Quoted 12 lines · Valentine Barshak

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.

Quoted 5 lines
Quoted 1 line
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.

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.

#5 lore raw
Quoted 14 lines · Benjamin Herrenschmidt
Quoted 13 lines · David Gibson
Quoted 12 lines · Valentine Barshak

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.

Quoted 8 lines
Quoted 5 lines
Quoted 1 line
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.

Quoted 3 lines

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.

Signature · 4 lines
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
#6 lore raw
Quoted 40 lines · David Gibson
Quoted 14 lines · Benjamin Herrenschmidt
Quoted 13 lines · David Gibson
Quoted 12 lines · Valentine Barshak

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.

Quoted 8 lines
Quoted 5 lines
Quoted 1 line
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.

Quoted 3 lines

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

Quoted 8 lines

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.

arch/powerpc/sysdev/uic.c +52 -1
Index: working-2.6/arch/powerpc/sysdev/uic.c
===================================================================
@@ -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);
Signature · 4 lines
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
#7 lore raw
Quoted 50 lines · David Gibson
Quoted 40 lines · David Gibson
Quoted 14 lines · Benjamin Herrenschmidt
Quoted 13 lines · David Gibson
Quoted 12 lines · Valentine Barshak

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.

Quoted 8 lines
Quoted 5 lines
Quoted 1 line
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.

Quoted 3 lines

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

Quoted 8 lines

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.

Quoted 6 lines

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?

Signature · 4 lines
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
author
#8 lore raw
Quoted 53 lines · David Gibson
Quoted 45 lines · David Gibson
Quoted 36 lines · David Gibson
Quoted 14 lines · Benjamin Herrenschmidt
Quoted 13 lines · David Gibson
Quoted 12 lines · Valentine Barshak

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.

Quoted 6 lines
Quoted 4 lines
Quoted 1 line
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.

Quoted 3 lines

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

Quoted 8 lines

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.

Quoted 6 lines

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.

Quoted 3 lines

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.

#9 lore raw

On Mon, Aug 13, 2007 at 04:31:03PM +0400, Valentine Barshak wrote:
[snip]

Quoted 7 lines
Quoted 7 lines
Quoted 6 lines

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.

Quoted 6 lines
Quoted 3 lines

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.

Quoted 1 line

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

Quoted 4 lines

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.

Signature · 4 lines
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