[PATCH] fix for certain sequnce of request_irq can cause irq storm

[PATCH] fix for certain sequnce of request_irq can cause irq storm

[Evgeniy Dushistov]

The problem is that hardware handled by arm/plat-orion/gpio.c,
require ack for edge irq, and no ack for level irq.

The code handle this issue, by two "struct irq_chip_type" per
one "struct irq_chip_generic". For one "struct irq_chip_generic"
irq_ack pointer is setted, for another it is NULL.

But we have only one mask_cache per two "struct irq_chip_type".
So if we 
1)unmask interrupt A for "edge type" trigger,
2)unmask interrupt B for "level type" trigger,
3)unmask interrupt C for "edge type",

we, because of usage of generic irq_gc_mask_clr_bit/irq_gc_mask_set_bit,
have hardware configured to trigger interrupt B on "edge type",
because of shared mask_cache. But kernel think that B is "level type",
so when interrupt B occur via "edge" reason, we don't ack it,
and B triggered again and again.

Signed-off-by: Evgeniy A. Dushistov <dushistov@mail.ru>
---
 arch/arm/plat-orion/gpio.c | 36 ++++++++++++++++++++++++++++++++----
 1 file changed, 32 insertions(+), 4 deletions(-)

diff --git a/arch/arm/plat-orion/gpio.c b/arch/arm/plat-orion/gpio.c
index b61a3bc..e048f61 100644
--- a/arch/arm/plat-orion/gpio.c
+++ b/arch/arm/plat-orion/gpio.c
@@ -497,6 +497,34 @@ static void orion_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip)
 #define orion_gpio_dbg_show NULL
 #endif
 
+static void orion_gpio_unmask_irq(struct irq_data *d)
+{
+	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+	struct irq_chip_type *ct = irq_data_get_chip_type(d);
+	u32 reg_val;
+	u32 mask = d->mask;
+
+	irq_gc_lock(gc);
+	reg_val = irq_reg_readl(gc->reg_base + ct->regs.mask);
+	reg_val |= mask;
+	irq_reg_writel(reg_val, gc->reg_base + ct->regs.mask);
+	irq_gc_unlock(gc);
+}
+
+static void orion_gpio_mask_irq(struct irq_data *d)
+{
+	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+	struct irq_chip_type *ct = irq_data_get_chip_type(d);
+	u32 mask = d->mask;
+	u32 reg_val;
+
+	irq_gc_lock(gc);
+	reg_val = irq_reg_readl(gc->reg_base + ct->regs.mask);
+	reg_val &= ~mask;
+	irq_reg_writel(reg_val, gc->reg_base + ct->regs.mask);
+	irq_gc_unlock(gc);
+}
+
 void __init orion_gpio_init(struct device_node *np,
 			    int gpio_base, int ngpio,
 			    void __iomem *base, int mask_offset,
@@ -565,8 +593,8 @@ void __init orion_gpio_init(struct device_node *np,
 	ct = gc->chip_types;
 	ct->regs.mask = ochip->mask_offset + GPIO_LEVEL_MASK_OFF;
 	ct->type = IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW;
-	ct->chip.irq_mask = irq_gc_mask_clr_bit;
-	ct->chip.irq_unmask = irq_gc_mask_set_bit;
+	ct->chip.irq_mask = orion_gpio_mask_irq;
+	ct->chip.irq_unmask = orion_gpio_unmask_irq;
 	ct->chip.irq_set_type = gpio_irq_set_type;
 	ct->chip.name = ochip->chip.label;
 
@@ -575,8 +603,8 @@ void __init orion_gpio_init(struct device_node *np,
 	ct->regs.ack = GPIO_EDGE_CAUSE_OFF;
 	ct->type = IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING;
 	ct->chip.irq_ack = irq_gc_ack_clr_bit;
-	ct->chip.irq_mask = irq_gc_mask_clr_bit;
-	ct->chip.irq_unmask = irq_gc_mask_set_bit;
+	ct->chip.irq_mask = orion_gpio_mask_irq;
+	ct->chip.irq_unmask = orion_gpio_unmask_irq;
 	ct->chip.irq_set_type = gpio_irq_set_type;
 	ct->handler = handle_edge_irq;
 	ct->chip.name = ochip->chip.label;
-- 
1.8.5.5

-- 
/Evgeniy

[PATCH] fix for certain sequnce of request_irq can cause irq storm

[Andrew Lunn]

On Sat, Jul 26, 2014 at 07:56:59PM +0400, Evgeniy Dushistov wrote:
> The problem is that hardware handled by arm/plat-orion/gpio.c,
> require ack for edge irq, and no ack for level irq.

Hi Evgeniy

Thanks for the problem report and patch. What device are you using?
An Orion5x based board or mv78xx0? Those are the only two left using
this code.

Thanks
	Andrew

[PATCH] fix for certain sequnce of request_irq can cause irq storm

[Evgeniy Dushistov]

On Sat, Jul 26, 2014 at 06:56:20PM +0200, Andrew Lunn wrote:
> On Sat, Jul 26, 2014 at 07:56:59PM +0400, Evgeniy Dushistov wrote:
> > The problem is that hardware handled by arm/plat-orion/gpio.c,
> > require ack for edge irq, and no ack for level irq.
> 
> Hi Evgeniy
> 
> Thanks for the problem report and patch. What device are you using?
> An Orion5x based board or mv78xx0? Those are the only two left using
> this code.
> 

I used custom board with mv78200 CPU.

-- 
/Evgeniy

[PATCH] fix for certain sequnce of request_irq can cause irq storm

[Andrew Lunn]

On Sat, Jul 26, 2014 at 07:56:59PM +0400, Evgeniy Dushistov wrote:
> The problem is that hardware handled by arm/plat-orion/gpio.c,
> require ack for edge irq, and no ack for level irq.
> 
> The code handle this issue, by two "struct irq_chip_type" per
> one "struct irq_chip_generic". For one "struct irq_chip_generic"
> irq_ack pointer is setted, for another it is NULL.
> 
> But we have only one mask_cache per two "struct irq_chip_type".
> So if we 
> 1)unmask interrupt A for "edge type" trigger,
> 2)unmask interrupt B for "level type" trigger,
> 3)unmask interrupt C for "edge type",
> 
> we, because of usage of generic irq_gc_mask_clr_bit/irq_gc_mask_set_bit,
> have hardware configured to trigger interrupt B on "edge type",
> because of shared mask_cache. But kernel think that B is "level type",
> so when interrupt B occur via "edge" reason, we don't ack it,
> and B triggered again and again.

Hi Evgeniy

Did you look at the way gpio-mvebu.c handles this? It is a little bit
different from your solution. I'm wondering if gpio-mvebu.c is just
different, or wrong? It does seem to be using one mask_cache for both
edge and level trigger.

Thanks
	Andrew


> 
> Signed-off-by: Evgeniy A. Dushistov <dushistov@mail.ru>
> ---
>  arch/arm/plat-orion/gpio.c | 36 ++++++++++++++++++++++++++++++++----
>  1 file changed, 32 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/plat-orion/gpio.c b/arch/arm/plat-orion/gpio.c
> index b61a3bc..e048f61 100644
> --- a/arch/arm/plat-orion/gpio.c
> +++ b/arch/arm/plat-orion/gpio.c
> @@ -497,6 +497,34 @@ static void orion_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip)
>  #define orion_gpio_dbg_show NULL
>  #endif
>  
> +static void orion_gpio_unmask_irq(struct irq_data *d)
> +{
> +	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
> +	struct irq_chip_type *ct = irq_data_get_chip_type(d);
> +	u32 reg_val;
> +	u32 mask = d->mask;
> +
> +	irq_gc_lock(gc);
> +	reg_val = irq_reg_readl(gc->reg_base + ct->regs.mask);
> +	reg_val |= mask;
> +	irq_reg_writel(reg_val, gc->reg_base + ct->regs.mask);
> +	irq_gc_unlock(gc);
> +}
> +
> +static void orion_gpio_mask_irq(struct irq_data *d)
> +{
> +	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
> +	struct irq_chip_type *ct = irq_data_get_chip_type(d);
> +	u32 mask = d->mask;
> +	u32 reg_val;
> +
> +	irq_gc_lock(gc);
> +	reg_val = irq_reg_readl(gc->reg_base + ct->regs.mask);
> +	reg_val &= ~mask;
> +	irq_reg_writel(reg_val, gc->reg_base + ct->regs.mask);
> +	irq_gc_unlock(gc);
> +}
> +
>  void __init orion_gpio_init(struct device_node *np,
>  			    int gpio_base, int ngpio,
>  			    void __iomem *base, int mask_offset,
> @@ -565,8 +593,8 @@ void __init orion_gpio_init(struct device_node *np,
>  	ct = gc->chip_types;
>  	ct->regs.mask = ochip->mask_offset + GPIO_LEVEL_MASK_OFF;
>  	ct->type = IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW;
> -	ct->chip.irq_mask = irq_gc_mask_clr_bit;
> -	ct->chip.irq_unmask = irq_gc_mask_set_bit;
> +	ct->chip.irq_mask = orion_gpio_mask_irq;
> +	ct->chip.irq_unmask = orion_gpio_unmask_irq;
>  	ct->chip.irq_set_type = gpio_irq_set_type;
>  	ct->chip.name = ochip->chip.label;
>  
> @@ -575,8 +603,8 @@ void __init orion_gpio_init(struct device_node *np,
>  	ct->regs.ack = GPIO_EDGE_CAUSE_OFF;
>  	ct->type = IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING;
>  	ct->chip.irq_ack = irq_gc_ack_clr_bit;
> -	ct->chip.irq_mask = irq_gc_mask_clr_bit;
> -	ct->chip.irq_unmask = irq_gc_mask_set_bit;
> +	ct->chip.irq_mask = orion_gpio_mask_irq;
> +	ct->chip.irq_unmask = orion_gpio_unmask_irq;
>  	ct->chip.irq_set_type = gpio_irq_set_type;
>  	ct->handler = handle_edge_irq;
>  	ct->chip.name = ochip->chip.label;
> -- 
> 1.8.5.5
> 
> -- 
> /Evgeniy

[PATCH] fix for certain sequnce of request_irq can cause irq storm

[Evgeniy Dushistov]

On Sat, Jul 26, 2014 at 07:45:06PM +0200, Andrew Lunn wrote:
> 
> Did you look at the way gpio-mvebu.c handles this? It is a little bit
> different from your solution. I'm wondering if gpio-mvebu.c is just
> different, or wrong? It does seem to be using one mask_cache for both
> edge and level trigger.
> 

Looks wrong to me. I don't look at irq handling, just unmask.
Consider the same scenario.
1)unmask interrupt A for "edge type" trigger
call mvebu_gpio_edge_irq_unmask and now CPU trigger interrupt A
if edge changed

2)unmask interupt B for "level type" trigger
call mvebu_gpio_level_irq_unmask,
gc->mask_cache have interrupt A,
so we unmask A and B for "level type" trigger.

So  expected result:
A edge type interrupt
B level type iterrupt

Actual result:
A edge type interrupt
A and B level type interrupt.

This may cause many funny things,
for example drivers may works or not works depending
on loading order.

-- 
/Evgeniy

[PATCH] fix for certain sequnce of request_irq can cause irq storm

[Evgeniy Dushistov]

On Sat, Jul 26, 2014 at 07:56:59PM +0400, Evgeniy Dushistov wrote:
> The problem is that hardware handled by arm/plat-orion/gpio.c,
> require ack for edge irq, and no ack for level irq.
> 
> The code handle this issue, by two "struct irq_chip_type" per
> one "struct irq_chip_generic". For one "struct irq_chip_generic"
> irq_ack pointer is setted, for another it is NULL.
> 
> But we have only one mask_cache per two "struct irq_chip_type".
> So if we 
> 1)unmask interrupt A for "edge type" trigger,
> 2)unmask interrupt B for "level type" trigger,
> 3)unmask interrupt C for "edge type",
> 
> we, because of usage of generic irq_gc_mask_clr_bit/irq_gc_mask_set_bit,
> have hardware configured to trigger interrupt B on "edge type",
> because of shared mask_cache. But kernel think that B is "level type",
> so when interrupt B occur via "edge" reason, we don't ack it,
> and B triggered again and again.
> 

Any news about mergin this patch?


-- 
/Evgeniy

[PATCH] fix for certain sequnce of request_irq can cause irq storm

[Jason Cooper]

On Sat, Jul 26, 2014 at 07:56:59PM +0400, Evgeniy Dushistov wrote:
> The problem is that hardware handled by arm/plat-orion/gpio.c,
> require ack for edge irq, and no ack for level irq.
> 
> The code handle this issue, by two "struct irq_chip_type" per
> one "struct irq_chip_generic". For one "struct irq_chip_generic"
> irq_ack pointer is setted, for another it is NULL.
> 
> But we have only one mask_cache per two "struct irq_chip_type".
> So if we 
> 1)unmask interrupt A for "edge type" trigger,
> 2)unmask interrupt B for "level type" trigger,
> 3)unmask interrupt C for "edge type",
> 
> we, because of usage of generic irq_gc_mask_clr_bit/irq_gc_mask_set_bit,
> have hardware configured to trigger interrupt B on "edge type",
> because of shared mask_cache. But kernel think that B is "level type",
> so when interrupt B occur via "edge" reason, we don't ack it,
> and B triggered again and again.
> 
> Signed-off-by: Evgeniy A. Dushistov <dushistov@mail.ru>
> ---
>  arch/arm/plat-orion/gpio.c | 36 ++++++++++++++++++++++++++++++++----
>  1 file changed, 32 insertions(+), 4 deletions(-)

Applied to mvebu/fixes with a minor subject line tweak.  Thanks for the
ping!

thx,

Jason.

[PATCH] fix for certain sequnce of request_irq can cause irq storm

[Thomas Petazzoni]

Dear Evgeniy Dushistov,

On Sat, 26 Jul 2014 21:12:53 +0400, Evgeniy Dushistov wrote:
> On Sat, Jul 26, 2014 at 06:56:20PM +0200, Andrew Lunn wrote:
> > On Sat, Jul 26, 2014 at 07:56:59PM +0400, Evgeniy Dushistov wrote:
> > > The problem is that hardware handled by arm/plat-orion/gpio.c,
> > > require ack for edge irq, and no ack for level irq.
> > 
> > Hi Evgeniy
> > 
> > Thanks for the problem report and patch. What device are you using?
> > An Orion5x based board or mv78xx0? Those are the only two left using
> > this code.
> 
> I used custom board with mv78200 CPU.

So you're still using the mainline kernel on this system? We were
actually starting to consider getting rid of the support for this
platform, since it doesn't receive much attention, and there are almost
no systems supported in mainline that use this CPU.

Are you actually using the two CPUs on mv78200 ? If so, in what
configurations ?

Would you be willing to put some effort into moving mv78xx0 to the
Device Tree, and all other modern subsystems (clock, pinmux, etc.) ?

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH] fix for certain sequnce of request_irq can cause irq storm

[Evgeniy Dushistov]

On Tue, Oct 21, 2014 at 04:15:51PM +0200, Thomas Petazzoni wrote:
> > I used custom board with mv78200 CPU.
> 
> So you're still using the mainline kernel on this system?

Yes, we reuse old hardware design with new enougth (3.12) kernel.

>We were
> actually starting to consider getting rid of the support for this
> platform, since it doesn't receive much attention, and there are almost
> no systems supported in mainline that use this CPU.
> 
> Are you actually using the two CPUs on mv78200 ? If so, in what
> configurations ?
> 

We use several configurations, on some of them the second cpu is disabled,
on others the second CPU connected to the first via gigabit ethernet 
(eth2<->eth3 in loopback like mode [I fixed one bug several years ago
about usage of such config, it should be in mainline]).


> Would you be willing to put some effort into moving mv78xx0 to the
> Device Tree, and all other modern subsystems (clock, pinmux, etc.) ?
> 

At now I have several git branches for slightly different device
configurations, so I suppose support of Device Tree should simplify
things for me.

But I can only test it on mv78200 CPU, and only
compiled into the kernel, because of u-boot from marvell old enough(no
Device Tree support),
and not sure that mainstream u-boot support the usage of two cores.


-- 
/Evgeniy

[PATCH] fix for certain sequnce of request_irq can cause irq storm

[Thomas Petazzoni]

Dear Evgeniy Dushistov,

On Tue, 21 Oct 2014 19:20:34 +0400, Evgeniy Dushistov wrote:

> > So you're still using the mainline kernel on this system?
> 
> Yes, we reuse old hardware design with new enougth (3.12) kernel.

Ok. And are you interested in seeing the mv78xx0 support being kept in
the mainline kernel for the foreseeable future?

> >We were
> > actually starting to consider getting rid of the support for this
> > platform, since it doesn't receive much attention, and there are almost
> > no systems supported in mainline that use this CPU.
> > 
> > Are you actually using the two CPUs on mv78200 ? If so, in what
> > configurations ?
> > 
> 
> We use several configurations, on some of them the second cpu is disabled,
> on others the second CPU connected to the first via gigabit ethernet 
> (eth2<->eth3 in loopback like mode [I fixed one bug several years ago
> about usage of such config, it should be in mainline]).

Ok.

> > Would you be willing to put some effort into moving mv78xx0 to the
> > Device Tree, and all other modern subsystems (clock, pinmux, etc.) ?
> > 
> 
> At now I have several git branches for slightly different device
> configurations, so I suppose support of Device Tree should simplify
> things for me.
> 
> But I can only test it on mv78200 CPU, and only
> compiled into the kernel, because of u-boot from marvell old enough(no
> Device Tree support),
> and not sure that mainstream u-boot support the usage of two cores.

Right, you probably have no other choice but keeping the original
U-Boot. But that's fine, with appended DTB, you can use this old U-Boot
and boot modern kernels.

But are you interested in helping doing this migration, but sending
some patches, doing some testing and so on?

Thanks,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH] fix for certain sequnce of request_irq can cause irq storm

[Evgeniy Dushistov]

On Tue, Oct 21, 2014 at 05:37:00PM +0200, Thomas Petazzoni wrote:
> Ok. And are you interested in seeing the mv78xx0 support being kept in
> the mainline kernel for the foreseeable future?
> 

Yes

> Right, you probably have no other choice but keeping the original
> U-Boot. But that's fine, with appended DTB, you can use this old U-Boot
> and boot modern kernels.
> 
> But are you interested in helping doing this migration, but sending
> some patches, doing some testing and so on?
> 

Yes, I am interesting. I am rather busy right now, and will be busy
in the next a couple of months, but after that I can write code to use
Device Tree or test already existing patches.

-- 
/Evgeniy