Re: [PATCH] powerpc: EOI spurious irqs during boot so they can be reenabled later

----- Original Message Follows -----
From: Michael Ellerman <redacted>
To: Paul Mackerras <redacted>
Cc: <redacted>, Milton Miller
[off-list ref], Segher Boessenkool
[off-list ref]
Subject: [PATCH] powerpc: EOI spurious irqs during boot so
they can be reenabled later
Date: Wed,  6 Aug 2008 12:03:37 +1000 (EST)
 

In the xics code, if we receive an irq during boot that
hasn't been setup yet - ie. we have no reverse mapping for
it - we mask the irq so we don't hear from it again.

Later on if someone request_irq()'s that irq, we'll unmask
it but it will still never fire. This is because we never
EOI'ed the irq when we originally received it - when it
was spurious.

This can be reproduced trivially by banging the keyboard
while kexec'ing on a P5 LPAR, even though the hvc_console
driver request's the console irq, the console is
non-functional because we're receiving no console
interrupts.

 
which means some driver didn't disable interrupts on its
shutdown, but I digress.
 

So when we receive a spurious irq mask it and then EOI it.

Signed-off-by: Michael Ellerman <redacted>
---
 arch/powerpc/platforms/pseries/xics.c |   29
++++++++++++++++++++---------
 1 files changed, 20 insertions(+), 9 deletions(-)


Updated to mask then EOI, thanks to Segher for whacking me
with the clue stick.

 
Actually, just sending the EOI would likely result in the
exact same interrupt comming back, as the mask will set
a bit near the source but the interrupt would have already
been missed.  (most irqs in xics systems are level).
 
Thanks to segher for noticing the order.   However...
 
 
 

18 more quoted lines

 
 
I really dislike this great big if in the middle of a
function.
we called this function from two different call sites where
the
choice of which to call was based on their environment.
 
Please move the call to eoi the hwirq to the callers, who
know
which path to take.
 
I guess it might make sense to put the printk in a 
mask_invalid_real_irq wrapper, and then perhaps just
duplicate 
the small remaining code?   Or split the return of spurious
from NO_IRQ (ie should spurious be NO_IRQ_IGNORE?)
 

 
-- 
1.5.5

On Tue, 2008-08-05 at 20:55 -0600, miltonm wrote:

----- Original Message Follows -----
From: Michael Ellerman <redacted>
To: Paul Mackerras <redacted>
Cc: <redacted>, Milton Miller
[off-list ref], Segher Boessenkool
[off-list ref]
Subject: [PATCH] powerpc: EOI spurious irqs during boot so
they can be reenabled later
Date: Wed,  6 Aug 2008 12:03:37 +1000 (EST)
 

quoted

In the xics code, if we receive an irq during boot that
hasn't been setup yet - ie. we have no reverse mapping for
it - we mask the irq so we don't hear from it again.

Later on if someone request_irq()'s that irq, we'll unmask
it but it will still never fire. This is because we never
EOI'ed the irq when we originally received it - when it
was spurious.

This can be reproduced trivially by banging the keyboard
while kexec'ing on a P5 LPAR, even though the hvc_console
driver request's the console irq, the console is
non-functional because we're receiving no console
interrupts.

 
which means some driver didn't disable interrupts on its
shutdown, but I digress.

But in the case of kdump the driver never gets a chance to shutdown its
irq, but I digress too :)

quoted

diff --git a/arch/powerpc/platforms/pseries/xics.c

b/arch/powerpc/platforms/pseries/xics.c index
0fc830f..4c692b2 100644 ---
a/arch/powerpc/platforms/pseries/xics.c +++
b/arch/powerpc/platforms/pseries/xics.c @@ -321,21 +321,26

@@ static unsigned int xics_startup(unsigned int virq)
     return 0;
 }
 
+static void xics_eoi_hwirq_direct(unsigned int hwirq)
+{
+    iosync();
+    direct_xirr_info_set((0xff << 24) | hwirq);
+}
+
 static void xics_eoi_direct(unsigned int virq)
 {
-    unsigned int irq = (unsigned int)irq_map[virq].hwirq;
+    xics_eoi_hwirq_direct((unsigned

int)irq_map[virq].hwirq); +}
 
+static void xics_eoi_hwirq_lpar(unsigned int hwirq)
+{
     iosync();
-    direct_xirr_info_set((0xff << 24) | irq);
+    lpar_xirr_info_set((0xff << 24) | hwirq);
 }
 
-
 static void xics_eoi_lpar(unsigned int virq)
 {
-    unsigned int irq = (unsigned int)irq_map[virq].hwirq;
-
-    iosync();
-    lpar_xirr_info_set((0xff << 24) | irq);
+    xics_eoi_hwirq_lpar((unsigned
int)irq_map[virq].hwirq);
 }
 
 static inline unsigned int xics_remap_irq(unsigned int

28 more quoted lines

It's not pretty, but the alternative is worse I think:

From 0a908825c8de6cd4c26288aba8c5b7fe3ed0a69f Mon Sep 17 00:00:00 2001

From: Michael Ellerman <redacted>
Date: Tue, 5 Aug 2008 22:34:48 +1000
Subject: [PATCH] powerpc: EOI spurious irqs during boot so they can be reenabled later

In the xics code, if we receive an irq during boot that hasn't been setup
yet - ie. we have no reverse mapping for it - we mask the irq so we don't
hear from it again.

Later on if someone request_irq()'s that irq, we'll unmask it but it will
still never fire. This is because we never EOI'ed the irq when we originally
received it - when it was spurious.

This can be reproduced trivially by banging the keyboard while kexec'ing on
a P5 LPAR, even though the hvc_console driver request's the console irq, the
console is non-functional because we're receiving no console interrupts.

So when we receive a spurious irq mask it and then EOI it.

Signed-off-by: Michael Ellerman <redacted>
---
 arch/powerpc/platforms/pseries/xics.c |   74 ++++++++++++++++++++++-----------
 1 files changed, 50 insertions(+), 24 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/xics.c b/arch/powerpc/platforms/pseries/xics.c
index 0fc830f..754706c 100644
--- a/arch/powerpc/platforms/pseries/xics.c
+++ b/arch/powerpc/platforms/pseries/xics.c

@@ -90,7 +90,7 @@ static inline unsigned int direct_xirr_info_get(void)
 {
 	int cpu = smp_processor_id();
 
-	return in_be32(&xics_per_cpu[cpu]->xirr.word);
+	return in_be32(&xics_per_cpu[cpu]->xirr.word) & 0x00ffffff;
 }
 
 static inline void direct_xirr_info_set(int value)

@@ -124,7 +124,8 @@ static inline unsigned int lpar_xirr_info_get(void)
 	lpar_rc = plpar_xirr(&return_value);
 	if (lpar_rc != H_SUCCESS)
 		panic(" bad return code xirr - rc = %lx \n", lpar_rc);
-	return (unsigned int)return_value;
+
+	return (unsigned int)return_value & 0x00ffffff;
 }
 
 static inline void lpar_xirr_info_set(int value)

@@ -321,49 +322,74 @@ static unsigned int xics_startup(unsigned int virq)
 	return 0;
 }
 
+static void xics_eoi_hwirq_direct(unsigned int hwirq)
+{
+	iosync();
+	direct_xirr_info_set((0xff << 24) | hwirq);
+}
+
 static void xics_eoi_direct(unsigned int virq)
 {
-	unsigned int irq = (unsigned int)irq_map[virq].hwirq;
+	xics_eoi_hwirq_direct((unsigned int)irq_map[virq].hwirq);
+}
 
+static void xics_eoi_hwirq_lpar(unsigned int hwirq)
+{
 	iosync();
-	direct_xirr_info_set((0xff << 24) | irq);
+	lpar_xirr_info_set((0xff << 24) | hwirq);
 }
 
-
 static void xics_eoi_lpar(unsigned int virq)
 {
-	unsigned int irq = (unsigned int)irq_map[virq].hwirq;
-
-	iosync();
-	lpar_xirr_info_set((0xff << 24) | irq);
+	xics_eoi_hwirq_lpar((unsigned int)irq_map[virq].hwirq);
 }
 
 static inline unsigned int xics_remap_irq(unsigned int vec)
 {
-	unsigned int irq;
-
-	vec &= 0x00ffffff;
-
-	if (vec == XICS_IRQ_SPURIOUS)
-		return NO_IRQ;
-	irq = irq_radix_revmap(xics_host, vec);
-	if (likely(irq != NO_IRQ))
-		return irq;
+	return irq_radix_revmap(xics_host, vec);
+}
 
-	printk(KERN_ERR "Interrupt %u (real) is invalid,"
-	       " disabling it.\n", vec);
-	xics_mask_real_irq(vec);
-	return NO_IRQ;
+static void xics_report_bad_irq(unsigned int hwirq)
+{
+	pr_err("%s: no mapping for hwirq %u, disabling it.\n", __func__, hwirq);
 }
 
 static unsigned int xics_get_irq_direct(void)
 {
-	return xics_remap_irq(direct_xirr_info_get());
+	unsigned int virq, hwirq;
+
+	hwirq = direct_xirr_info_get();
+	if (hwirq == XICS_IRQ_SPURIOUS)
+		return NO_IRQ;
+
+	virq = xics_remap_irq(hwirq);
+
+	if (unlikely(virq == NO_IRQ)) {
+		xics_report_bad_irq(hwirq);
+		xics_mask_real_irq(hwirq);
+		xics_eoi_hwirq_direct(hwirq);
+	}
+
+	return virq;
 }
 
 static unsigned int xics_get_irq_lpar(void)
 {
-	return xics_remap_irq(lpar_xirr_info_get());
+	unsigned int virq, hwirq;
+
+	hwirq = lpar_xirr_info_get();
+	if (hwirq == XICS_IRQ_SPURIOUS)
+		return NO_IRQ;
+
+	virq = xics_remap_irq(hwirq);
+
+	if (unlikely(virq == NO_IRQ)) {
+		xics_report_bad_irq(hwirq);
+		xics_mask_real_irq(hwirq);
+		xics_eoi_hwirq_lpar(hwirq);
+	}
+
+	return virq;
 }
 
 #ifdef CONFIG_SMP

-- 
1.5.5