Skip to contentSkip to conversation

UIC fixes

PATCH In review David Gibson 2 participants 7 messages Aug 14, 2007 – Aug 15, 2007 PowerPC

lore
Share & tools
lore
#0 lore raw

cover UIC fixes

This series fixes several small problems with the driver for the UIC interrupt controller found in PowerPC 4xx embedded CPUs.

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
#1 lore raw

PATCH 1/3 Fix setting of irq trigger type in UIC driver

The UIC (interrupt controller in 4xx embedded CPUs) driver currently missets the IRQ_lEVEL flag in desc->status, due to a thinko. This patch fixes the bug.

Currently this is only a cosmetic problem (affects the output in /proc/interrupts), however subsequent patches will use the IRQ_LEVEL flag to affect flow handling.

---
 arch/powerpc/sysdev/uic.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/powerpc/sysdev/uic.c +1 -1
Index: working-2.6/arch/powerpc/sysdev/uic.c
===================================================================
@@ -142,7 +142,7 @@ static int uic_set_irq_type(unsigned int
 
 	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);
Acked-by on v1 1/3
#2 lore raw

Re: [PATCH 1/3] Fix setting of irq trigger type in UIC driver

Quoted 10 lines · On Tue, 14 Aug 2007 13:52:42 +1000 (EST) David Gibson…

The UIC (interrupt controller in 4xx embedded CPUs) driver currently missets the IRQ_lEVEL flag in desc->status, due to a thinko. This patch fixes the bug.

Currently this is only a cosmetic problem (affects the output in /proc/interrupts), however subsequent patches will use the IRQ_LEVEL flag to affect flow handling.

josh

#3 lore raw

PATCH 2/3 Fix irq flow handler for 4xx UIC

In reply to David Gibson

At present the driver for the UIC (the embedded interrupt controller in 4xx chips) uses the handle_level_irq() flow handler. It turns out this does not correctly handle level triggered interrupts on the UIC.

Specifically, acknowledging an irq on the UIC (i.e. clearing the relevant bit in UIC_SR) will have no effect for a level interrupt which is still asserted by the external device, even if the irq is already masked. Therefore, unlike handle_level_irq() we must ack the interrupt after invoking the ISR (which should cause the device to stop asserting the irq) instead of acking it when we mask it, before the ISR.

This patch implements this change, in a new handle_uic_irq(), a customised irq flow handler for the UIC. For edge triggered interrupts, handle_uic_irq() still uses the old flow - we must ack edge triggered interrupt before the ISR not after, or we could miss a second event which occurred between invoking the ISR and acking the irq.

---
 arch/powerpc/sysdev/uic.c |   61 +++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 60 insertions(+), 1 deletion(-)
arch/powerpc/sysdev/uic.c +60 -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,64 @@ static struct irq_chip uic_irq_chip = {
 	.set_type	= uic_set_irq_type,
 };
 
+/**
+ *	handle_uic_irq - irq flow handler for UIC
+ *	@irq:	the interrupt number
+ *	@desc:	the interrupt description structure for this irq
+ *
+ * This is modified version of the generic handle_level_irq() suitable
+ * for the UIC.  On the UIC, acking (i.e. clearing the SR bit) a level
+ * irq will have no effect if the interrupt is still asserted by the
+ * device, even if the interrupt is already masked.  Therefore, unlike
+ * the standard handle_level_irq(), we must ack the interrupt *after*
+ * invoking the ISR (which should have de-asserted the interrupt in
+ * the external source).  For edge interrupts we ack at the beginning
+ * instead of the end, to keep the window in which we can miss an
+ * interrupt as small as possible.
+ */
+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);
+	if (desc->status & IRQ_LEVEL)
+		desc->chip->mask(irq);
+	else
+		desc->chip->mask_ack(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;
+	if (desc->status & IRQ_LEVEL)
+		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 +232,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);
Acked-by on v1 2/3
#4 lore raw

Re: [PATCH 2/3] Fix irq flow handler for 4xx UIC

Quoted 12 lines · On Tue, 14 Aug 2007 13:52:42 +1000 (EST) David Gibson…

At present the driver for the UIC (the embedded interrupt controller in 4xx chips) uses the handle_level_irq() flow handler. It turns out this does not correctly handle level triggered interrupts on the UIC.

Specifically, acknowledging an irq on the UIC (i.e. clearing the relevant bit in UIC_SR) will have no effect for a level interrupt which is still asserted by the external device, even if the irq is already masked. Therefore, unlike handle_level_irq() we must ack the interrupt after invoking the ISR (which should cause the device to stop asserting the irq) instead of acking it when we mask it, before the ISR.

Quoted 8 lines

This patch implements this change, in a new handle_uic_irq(), a customised irq flow handler for the UIC. For edge triggered interrupts, handle_uic_irq() still uses the old flow - we must ack edge triggered interrupt before the ISR not after, or we could miss a second event which occurred between invoking the ISR and acking the irq.

josh

#5 lore raw

PATCH 3/3 Improve robustness of the UIC cascade handler

In reply to David Gibson

At present the cascade interrupt handler for the UIC (interrupt controller on 4xx embedded chips) will misbehave badly if it is called spuriously - that is if the handler is invoked when no interrupts are asserted in the child UIC.

Although spurious interrupts shouldn't happen, it's good to behave robustly if they do. This patch does so by checking for and ignoring spurious interrupts.

---
 arch/powerpc/sysdev/uic.c |    3 +++
 1 file changed, 3 insertions(+)
arch/powerpc/sysdev/uic.c +3 -0
Index: working-2.6/arch/powerpc/sysdev/uic.c
===================================================================
@@ -266,6 +266,9 @@ irqreturn_t uic_cascade(int virq, void *
 	int subvirq;
 
 	msr = mfdcr(uic->dcrbase + UIC_MSR);
+	if (!msr) /* spurious interrupt */
+		return IRQ_HANDLED;
+
 	src = 32 - ffs(msr);
 
 	subvirq = irq_linear_revmap(uic->irqhost, src);
Acked-by on v1 3/3
#6 lore raw

Re: [PATCH 3/3] Improve robustness of the UIC cascade handler

Quoted 21 lines · On Tue, 14 Aug 2007 13:52:42 +1000 (EST) David Gibson…

At present the cascade interrupt handler for the UIC (interrupt controller on 4xx embedded chips) will misbehave badly if it is called spuriously - that is if the handler is invoked when no interrupts are asserted in the child UIC.

Although spurious interrupts shouldn't happen, it's good to behave robustly if they do. This patch does so by checking for and ignoring spurious interrupts.

---
 arch/powerpc/sysdev/uic.c |    3 +++
 1 file changed, 3 insertions(+)

Index: working-2.6/arch/powerpc/sysdev/uic.c
===================================================================

arch/powerpc/sysdev/uic.c irqreturn_t uic_cascade(int virq, void * · line 270

Quoted 5 lines
arch/powerpc/sysdev/uic.c +2 -0
@@ -266,6 +266,9 @@ irqreturn_t uic_cascade(int virq, void *
 	int subvirq;
 
 	msr = mfdcr(uic->dcrbase + UIC_MSR);
+	if (!msr) /* spurious interrupt */
+		return IRQ_HANDLED;

Hm. Is there was a way we could have this case increment ppc_spurious_interrupts so that the BAD entry in /proc/interrupts would be updated with these? Not a huge deal, but it might be nice to have. Otherwise the patch looks fine.

josh