[PATCH] Alchemy: fix edge irq handling

[PATCH] Alchemy: fix edge irq handling

[Manuel Lauss]

Introduce separate mack_ack callbacks which really do shut up the
edge-triggered irqs when called.  Without this change, high-frequency
edge interrupts can result in an endless irq storm, hanging the system.

This can be easily triggered for example by setting an irq to falling
edge type and manually connecting the associated pin to ground.

Signed-off-by: Manuel Lauss <mano@roarinelk.homelinux.net>
---
 arch/mips/alchemy/common/irq.c |   32 ++++++++++++++++++++++++--------
 1 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/arch/mips/alchemy/common/irq.c b/arch/mips/alchemy/common/irq.c
index c88c821..60da581 100644
--- a/arch/mips/alchemy/common/irq.c
+++ b/arch/mips/alchemy/common/irq.c
@@ -320,6 +320,16 @@ static void au1x_ic0_mask(unsigned int irq_nr)
 	au_sync();
 }
 
+static void au1x_ic0_maskack(unsigned int irq_nr)
+{
+	unsigned int bit = irq_nr - AU1000_INTC0_INT_BASE;
+	au_writel(1 << bit, IC0_MASKCLR);
+	au_writel(1 << bit, IC0_WAKECLR);
+	au_writel(1 << bit, IC0_FALLINGCLR);
+	au_writel(1 << bit, IC0_RISINGCLR);
+	au_sync();
+}
+
 static void au1x_ic1_mask(unsigned int irq_nr)
 {
 	unsigned int bit = irq_nr - AU1000_INTC1_INT_BASE;
@@ -328,6 +338,16 @@ static void au1x_ic1_mask(unsigned int irq_nr)
 	au_sync();
 }
 
+static void au1x_ic1_maskack(unsigned int irq_nr)
+{
+	unsigned int bit = irq_nr - AU1000_INTC1_INT_BASE;
+	au_writel(1 << bit, IC1_MASKCLR);
+	au_writel(1 << bit, IC1_WAKECLR);
+	au_writel(1 << bit, IC1_FALLINGCLR);
+	au_writel(1 << bit, IC1_RISINGCLR);
+	au_sync();
+}
+
 static void au1x_ic0_ack(unsigned int irq_nr)
 {
 	unsigned int bit = irq_nr - AU1000_INTC0_INT_BASE;
@@ -379,25 +399,21 @@ static int au1x_ic1_setwake(unsigned int irq, unsigned int on)
 /*
  * irq_chips for both ICs; this way the mask handlers can be
  * as short as possible.
- *
- * NOTE: the ->ack() callback is used by the handle_edge_irq
- *	 flowhandler only, the ->mask_ack() one by handle_level_irq,
- *	 so no need for an irq_chip for each type of irq (level/edge).
  */
 static struct irq_chip au1x_ic0_chip = {
 	.name		= "Alchemy-IC0",
-	.ack		= au1x_ic0_ack,		/* edge */
+	.ack		= au1x_ic0_ack,
 	.mask		= au1x_ic0_mask,
-	.mask_ack	= au1x_ic0_mask,	/* level */
+	.mask_ack	= au1x_ic0_maskack,
 	.unmask		= au1x_ic0_unmask,
 	.set_type	= au1x_ic_settype,
 };
 
 static struct irq_chip au1x_ic1_chip = {
 	.name		= "Alchemy-IC1",
-	.ack		= au1x_ic1_ack,		/* edge */
+	.ack		= au1x_ic1_ack,
 	.mask		= au1x_ic1_mask,
-	.mask_ack	= au1x_ic1_mask,	/* level */
+	.mask_ack	= au1x_ic1_maskack,
 	.unmask		= au1x_ic1_unmask,
 	.set_type	= au1x_ic_settype,
 	.set_wake	= au1x_ic1_setwake,
-- 
1.6.1

Re: [PATCH] Alchemy: fix edge irq handling

[Kevin Hickey]

Manuel,

Have you actually seen this happen (outside of inducing it manually)?  I
have some concern that by doing this we may either miss interrupts on
devices that send a lot (by design) or miss a design bug in a system
because we are masking out some interrupts.  I know that system
stability is important, but I don't like hiding problems.

=Kevin

On Tue, 2009-01-20 at 11:03 +0100, Manuel Lauss wrote:
> Introduce separate mack_ack callbacks which really do shut up the
> edge-triggered irqs when called.  Without this change, high-frequency
> edge interrupts can result in an endless irq storm, hanging the system.
> 
> This can be easily triggered for example by setting an irq to falling
> edge type and manually connecting the associated pin to ground.
> 
> Signed-off-by: Manuel Lauss <mano@roarinelk.homelinux.net>
> ---
>  arch/mips/alchemy/common/irq.c |   32 ++++++++++++++++++++++++--------
>  1 files changed, 24 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/mips/alchemy/common/irq.c b/arch/mips/alchemy/common/irq.c
> index c88c821..60da581 100644
> --- a/arch/mips/alchemy/common/irq.c
> +++ b/arch/mips/alchemy/common/irq.c
> @@ -320,6 +320,16 @@ static void au1x_ic0_mask(unsigned int irq_nr)
>  	au_sync();
>  }
>  
> +static void au1x_ic0_maskack(unsigned int irq_nr)
> +{
> +	unsigned int bit = irq_nr - AU1000_INTC0_INT_BASE;
> +	au_writel(1 << bit, IC0_MASKCLR);
> +	au_writel(1 << bit, IC0_WAKECLR);
> +	au_writel(1 << bit, IC0_FALLINGCLR);
> +	au_writel(1 << bit, IC0_RISINGCLR);
> +	au_sync();
> +}
> +
>  static void au1x_ic1_mask(unsigned int irq_nr)
>  {
>  	unsigned int bit = irq_nr - AU1000_INTC1_INT_BASE;
> @@ -328,6 +338,16 @@ static void au1x_ic1_mask(unsigned int irq_nr)
>  	au_sync();
>  }
>  
> +static void au1x_ic1_maskack(unsigned int irq_nr)
> +{
> +	unsigned int bit = irq_nr - AU1000_INTC1_INT_BASE;
> +	au_writel(1 << bit, IC1_MASKCLR);
> +	au_writel(1 << bit, IC1_WAKECLR);
> +	au_writel(1 << bit, IC1_FALLINGCLR);
> +	au_writel(1 << bit, IC1_RISINGCLR);
> +	au_sync();
> +}
> +
>  static void au1x_ic0_ack(unsigned int irq_nr)
>  {
>  	unsigned int bit = irq_nr - AU1000_INTC0_INT_BASE;
> @@ -379,25 +399,21 @@ static int au1x_ic1_setwake(unsigned int irq, unsigned int on)
>  /*
>   * irq_chips for both ICs; this way the mask handlers can be
>   * as short as possible.
> - *
> - * NOTE: the ->ack() callback is used by the handle_edge_irq
> - *	 flowhandler only, the ->mask_ack() one by handle_level_irq,
> - *	 so no need for an irq_chip for each type of irq (level/edge).
>   */
>  static struct irq_chip au1x_ic0_chip = {
>  	.name		= "Alchemy-IC0",
> -	.ack		= au1x_ic0_ack,		/* edge */
> +	.ack		= au1x_ic0_ack,
>  	.mask		= au1x_ic0_mask,
> -	.mask_ack	= au1x_ic0_mask,	/* level */
> +	.mask_ack	= au1x_ic0_maskack,
>  	.unmask		= au1x_ic0_unmask,
>  	.set_type	= au1x_ic_settype,
>  };
>  
>  static struct irq_chip au1x_ic1_chip = {
>  	.name		= "Alchemy-IC1",
> -	.ack		= au1x_ic1_ack,		/* edge */
> +	.ack		= au1x_ic1_ack,
>  	.mask		= au1x_ic1_mask,
> -	.mask_ack	= au1x_ic1_mask,	/* level */
> +	.mask_ack	= au1x_ic1_maskack,
>  	.unmask		= au1x_ic1_unmask,
>  	.set_type	= au1x_ic_settype,
>  	.set_wake	= au1x_ic1_setwake,
-- 
=Kevin

Re: [PATCH] Alchemy: fix edge irq handling

[Manuel Lauss]

Hi Kevin,

> Have you actually seen this happen (outside of inducing it manually)?  I
> have some concern that by doing this we may either miss interrupts on
> devices that send a lot (by design) or miss a design bug in a system
> because we are masking out some interrupts.  I know that system
> stability is important, but I don't like hiding problems.

Yes, in a customer project.  A simple pushbutton which connects a pulled-up
gpio pin to ground.  Push it, instant hang (handler called over and over
again) when it is not debounced.  With a single edge and a much lower
edge-frequency it obviously works fine (see timer).

(And, handle_edge_irq() _does_ call mask_ack() after all).


Best regards,
	Manuel Lauss

 
> =Kevin
> 
> On Tue, 2009-01-20 at 11:03 +0100, Manuel Lauss wrote:
> > Introduce separate mack_ack callbacks which really do shut up the
> > edge-triggered irqs when called.  Without this change, high-frequency
> > edge interrupts can result in an endless irq storm, hanging the system.
> > 
> > This can be easily triggered for example by setting an irq to falling
> > edge type and manually connecting the associated pin to ground.
> > 

Re: [PATCH] Alchemy: fix edge irq handling

[Kevin Hickey]

I am still concerned about this patch.  Just last week we encountered
similar behavior that turned out to be a board design error.  I had a
similar patch in that kernel that allowed me to run without error.  Our
Windows CE developer, however, did not and ended up finding the board
bug.  Fixing the board improved performance and system stability - had
we not been running CE we probably would not have found the issue until
much later and with greater effort.

Your example below is similar - debouncing the switch in hardware seems
a better solution (albeit likely an expensive one) than patching the
mainline kernel.  And I reiterate: some devices send a lot of interrupts
by design; we should honor their requests, not mask them out.

=Kevin

On Wed, 2009-01-21 at 07:48 +0100, Manuel Lauss wrote:
> Hi Kevin,
> 
> > Have you actually seen this happen (outside of inducing it manually)?  I
> > have some concern that by doing this we may either miss interrupts on
> > devices that send a lot (by design) or miss a design bug in a system
> > because we are masking out some interrupts.  I know that system
> > stability is important, but I don't like hiding problems.
> 
> Yes, in a customer project.  A simple pushbutton which connects a pulled-up
> gpio pin to ground.  Push it, instant hang (handler called over and over
> again) when it is not debounced.  With a single edge and a much lower
> edge-frequency it obviously works fine (see timer).
> 
> (And, handle_edge_irq() _does_ call mask_ack() after all).

-- 
Kevin Hickey
Alchemy Solutions
RMI Corporation
khickey@rmicorp.com
P:  512.691.8044

Re: [PATCH] Alchemy: fix edge irq handling

[Manuel Lauss]

Kevin,

On Mon, Jan 26, 2009 at 08:18:45PM -0600, Kevin Hickey wrote:
> Your example below is similar - debouncing the switch in hardware seems
> a better solution (albeit likely an expensive one) than patching the
> mainline kernel.  And I reiterate: some devices send a lot of interrupts
> by design; we should honor their requests, not mask them out.

I agree in principle, but what's the point of honoring the requests if they
come in faster than the cpu can handle them?  I think that's why the
handle_edge_irq() flowhandler masks the interrupt when another edge comes in
while the handler for the previous one is still running.  This is also the
problem I'm running into:  the second (and following) edges don't get acked
when the flowhandler tries to mask them, resulting in the irq storm.  If I
explicitly ack it in the irq handler itself, all is well.

The current in-tree irq code bahaves differently than in <=2.6.28;  this
patch restores this behaviour, and I believe it is the way the mask_ack()
callback is supposed to work.  It affects only edge interrupts which come in
faster than the cpu can handle them;  for all others there's no change (other
than 2 more stores in the mask fastpath).

(Or maybe it's a logic bug in handle_edge_irq(); I don't know.)

Thanks,
	Manuel Lauss