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

2008-08-06 Thread miltonm 
- Original Message Follows -
From: Michael Ellerman [EMAIL PROTECTED]
To: [EMAIL PROTECTED]
Cc: Paul Mackerras [EMAIL PROTECTED],
linuxppc-dev@ozlabs.org, Segher Boessenkool
[EMAIL PROTECTED]
Subject: Re: [PATCH] powerpc: EOI spurious irqs during boot
so they can be reenabled later
Date: Wed, 06 Aug 2008 14:53:54 +1000

 On Tue, 2008-08-05 at 20:55 -0600, miltonm wrote:
  - Original Message Follows -
  From: Michael Ellerman [EMAIL PROTECTED]
  To: Paul Mackerras [EMAIL PROTECTED]
  Cc: linuxppc-dev@ozlabs.org, Milton Miller
  [EMAIL PROTECTED], Segher Boessenkool
  [EMAIL PROTECTED]
  Subject: [PATCH] powerpc: EOI spurious irqs during boot
  so they can be reenabled later
  Date: Wed,  6 Aug 2008 12:03:37 +1000 (EST)
   



  
  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.
 
 It's not pretty, but the alternative is worse I think:
 

Well, it needs a bit of refinement.

 From 0a908825c8de6cd4c26288aba8c5b7fe3ed0a69f Mon Sep 17
 00:00:00 2001 From: Michael Ellerman
 [EMAIL PROTECTED] 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 [EMAIL PROTECTED]
 ---
  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) 
 0x00ff;
  }
  
  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  0x00ff;
  }
 

No, don't put the  0x00FF here.
(1) this is not get_vector
(2) we can take advantage of the full value

Instead we need a macro to extract this out.

The value returned is the value that is supposed to be used
to EOI.  The top byte is the  old priority of the CPPR and
the bottom 3 is the vector.   xics has priority levels
even though linux does not, and hence can allow limited
nesting.  we use that to allow
only ipis to nest hardware irqs, but once we take an eoi we
actually drop and let
any new hwirq in.   that's a seperate bug/hole.
 
  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);
  }

so if we undo this part

  
  static inline unsigned int xics_remap_irq(unsigned int
 vec)
  {
 -unsigned int irq;
 -
 -vec = 0x00ff;
 -
 -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

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

2008-08-05 Thread Michael Ellerman 
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 later
in boot, the console is non-functional because we're receiving no console
interrupts.

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

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


Probably 27 material unless there are objections ..


diff --git a/arch/powerpc/platforms/pseries/xics.c 
b/arch/powerpc/platforms/pseries/xics.c
index 0fc830f..dc007f4 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 vec)
@@ -350,9 +355,15 @@ static inline unsigned int xics_remap_irq(unsigned int vec)
if (likely(irq != NO_IRQ))
return irq;
 
-   printk(KERN_ERR Interrupt %u (real) is invalid,
-   disabling it.\n, vec);
+   pr_err(%s: no mapping for hwirq %u, disabling it.\n, __func__, vec);
+
+   if (firmware_has_feature(FW_FEATURE_LPAR))
+   xics_eoi_hwirq_lpar(vec);
+   else
+   xics_eoi_hwirq_direct(vec);
+
xics_mask_real_irq(vec);
+
return NO_IRQ;
 }
 
-- 
1.5.5

___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev

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

2008-08-05 Thread Segher Boessenkool 

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


What happens when a new IRQ arrives on the interrupt controller
between these EOI and mask calls?  Should you instead first mask
it, and only then EOI it?  Or doesn't that work on XICS?


Segher

___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev

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

2008-08-05 Thread Michael Ellerman 
On Tue, 2008-08-05 at 18:48 +0200, Segher Boessenkool wrote:
  So when we receive a spurious irq, EOI it, and then mask it.
 
 What happens when a new IRQ arrives on the interrupt controller
 between these EOI and mask calls?  Should you instead first mask
 it, and only then EOI it?  Or doesn't that work on XICS?

Yeah right, in which case we'd have not EOI'ed that one and we're back
at square one.

It seems to work with a mask then EOI, so unless Milton says otherwise
I'll do it that way - new patch coming.

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person


signature.asc
Description: This is a digitally signed message part
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev

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

2008-08-05 Thread Michael Ellerman 
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 [EMAIL PROTECTED]
---
 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.

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 vec)
@@ -350,9 +355,15 @@ static inline unsigned int xics_remap_irq(unsigned int vec)
if (likely(irq != NO_IRQ))
return irq;
 
-   printk(KERN_ERR Interrupt %u (real) is invalid,
-   disabling it.\n, vec);
+   pr_err(%s: no mapping for hwirq %u, disabling it.\n, __func__, vec);
+
xics_mask_real_irq(vec);
+
+   if (firmware_has_feature(FW_FEATURE_LPAR))
+   xics_eoi_hwirq_lpar(vec);
+   else
+   xics_eoi_hwirq_direct(vec);
+
return NO_IRQ;
 }
 
-- 
1.5.5

___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev

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

2008-08-05 Thread miltonm 
- Original Message Follows -
From: Michael Ellerman [EMAIL PROTECTED]
To: Paul Mackerras [EMAIL PROTECTED]
Cc: linuxppc-dev@ozlabs.org, Milton Miller
[EMAIL PROTECTED], Segher Boessenkool
[EMAIL PROTECTED]
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 [EMAIL PROTECTED]
 ---
  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...
 
 
 
 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
 vec) @@ -350,9 +355,15 @@ static inline unsigned int
 xics_remap_irq(unsigned int vec)
  if (likely(irq != NO_IRQ))
  return irq;
  
 -printk(KERN_ERR Interrupt %u (real) is invalid,
 -disabling it.\n, vec);
 +pr_err(%s: no mapping for hwirq %u, disabling it.\n
 , __func__, vec); +
  xics_mask_real_irq(vec);
 +
 +if (firmware_has_feature(FW_FEATURE_LPAR))
 +xics_eoi_hwirq_lpar(vec);
 +else
 +xics_eoi_hwirq_direct(vec);
 +
  return NO_IRQ;
  }
 
 
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
 
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev

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

2008-08-05 Thread Michael Ellerman 
On Tue, 2008-08-05 at 20:55 -0600, miltonm wrote:
 - Original Message Follows -
 From: Michael Ellerman [EMAIL PROTECTED]
 To: Paul Mackerras [EMAIL PROTECTED]
 Cc: linuxppc-dev@ozlabs.org, Milton Miller
 [EMAIL PROTECTED], Segher Boessenkool
 [EMAIL PROTECTED]
 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.

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

  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
  vec) @@ -350,9 +355,15 @@ static inline unsigned int
  xics_remap_irq(unsigned int vec)
   if (likely(irq != NO_IRQ))
   return irq;
   
  -printk(KERN_ERR Interrupt %u (real) is invalid,
  -disabling it.\n, vec);
  +pr_err(%s: no mapping for hwirq %u, disabling it.\n
  , __func__, vec); +
   xics_mask_real_irq(vec);
  +
  +if (firmware_has_feature(FW_FEATURE_LPAR))
  +xics_eoi_hwirq_lpar(vec);
  +else
  +xics_eoi_hwirq_direct(vec);
  +
   return NO_IRQ;
   }
  
 
 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.

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

From 0a908825c8de6cd4c26288aba8c5b7fe3ed0a69f Mon Sep 17 00:00:00 2001
From: Michael Ellerman [EMAIL PROTECTED]
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 [EMAIL PROTECTED]
---
 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)  0x00ff;
 }
 
 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