[PATCH 0/1] fix MVEBU GPIO driver bug causing kernel hang

[PATCH 0/1] fix MVEBU GPIO driver bug causing kernel hang

[Gerlando Falauto]

The MVEBU GPIO driver (like the old orion-gpio) is affected by a subtle
bug, where requesting (and triggering) both EDGE- and LEVEL- based IRQs
causes the kernel to hang.
Part of the reworking necessary to fix this bug (i.e. handling two
separate mask registers, one per chip type) was first submitted by me,
then reworked by Thomas Gleixner, then mainlined, finally landing in 3.11.

The final bits to actually fix this are still missing though.
This patch will fix this.
I also marked the necessary prerequisites to apply this to linux-stable.

SIDE NOTE: If I understand correctly, a brand new driver from
Sebastian Hesselbarth (drivers/irqchip/irq-orion.c) may replace this driver
in the future, but it still misses the IRQ_GC_MASK_CACHE_PER_TYPE bit:

static int __init orion_bridge_irq_init(struct device_node *np,
                                       struct device_node *parent)
{
[...]
       ret = irq_alloc_domain_generic_chips(domain, nrirqs, 1, np->name,
                            handle_level_irq, clr, 0, IRQ_GC_INIT_MASK_CACHE);

Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: Linus Walleij <linus.walleij@stericsson.com>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Jason Cooper <jason@lakedaemon.net>
Cc: Gregory Clement <gregory.clement@free-electrons.com>
Cc: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: linux-arm-kernel at lists.infradead.org
#Cc: <stable@vger.kernel.org> # 3.7.x cfeaa93 genirq: Generic chip: Remove the local cur_regs() function
#Cc: <stable@vger.kernel.org> # 3.7.x 899f0e6 genirq: Generic chip: Add support for per chip type mask cache
#Cc: <stable@vger.kernel.org> # 3.7.x af80b0f genirq: Generic chip: Handle separate mask registers

Gerlando Falauto (1):
  gpio: mvebu: enable and use IRQ_GC_MASK_CACHE_PER_TYPE

 drivers/gpio/gpio-mvebu.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

-- 
1.8.0.1

[PATCH 1/1] gpio: mvebu: enable and use IRQ_GC_MASK_CACHE_PER_TYPE

[Gerlando Falauto]

Since we have now introduced mask_cache within irq_chip_type to also
handle per-chip-type mask registers, convert gpio-mvebu driver to use
this new pointer.

Also enable IRQ_GC_MASK_CACHE_PER_TYPE to actually handle separate mask
registers for all three SoC variants handled by this driver.

This wll fix a bug where requesting (and triggering) both EDGE- and
LEVEL- based IRQs causes the kernel to hang.

Signed-off-by: Gerlando Falauto <gerlando.falauto@keymile.com>
Cc: Simon Guinot <sguinot@lacie.com>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: Linus Walleij <linus.walleij@stericsson.com>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Jason Cooper <jason@lakedaemon.net>
Cc: Gregory Clement <gregory.clement@free-electrons.com>
Cc: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: linux-arm-kernel at lists.infradead.org
---
 drivers/gpio/gpio-mvebu.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c
index 3a4816a..ce425b3 100644
--- a/drivers/gpio/gpio-mvebu.c
+++ b/drivers/gpio/gpio-mvebu.c
@@ -303,48 +303,52 @@ static void mvebu_gpio_irq_ack(struct irq_data *d)
 static void mvebu_gpio_edge_irq_mask(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);
 	struct mvebu_gpio_chip *mvchip = gc->private;
 	u32 mask = 1 << (d->irq - gc->irq_base);
 
 	irq_gc_lock(gc);
-	gc->mask_cache &= ~mask;
-	writel_relaxed(gc->mask_cache, mvebu_gpioreg_edge_mask(mvchip));
+	*ct->mask_cache &= ~mask;
+	writel_relaxed(*ct->mask_cache, mvebu_gpioreg_edge_mask(mvchip));
 	irq_gc_unlock(gc);
 }
 
 static void mvebu_gpio_edge_irq_unmask(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);
 	struct mvebu_gpio_chip *mvchip = gc->private;
 	u32 mask = 1 << (d->irq - gc->irq_base);
 
 	irq_gc_lock(gc);
-	gc->mask_cache |= mask;
-	writel_relaxed(gc->mask_cache, mvebu_gpioreg_edge_mask(mvchip));
+	*ct->mask_cache |= mask;
+	writel_relaxed(*ct->mask_cache, mvebu_gpioreg_edge_mask(mvchip));
 	irq_gc_unlock(gc);
 }
 
 static void mvebu_gpio_level_irq_mask(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);
 	struct mvebu_gpio_chip *mvchip = gc->private;
 	u32 mask = 1 << (d->irq - gc->irq_base);
 
 	irq_gc_lock(gc);
-	gc->mask_cache &= ~mask;
-	writel_relaxed(gc->mask_cache, mvebu_gpioreg_level_mask(mvchip));
+	*ct->mask_cache &= ~mask;
+	writel_relaxed(*ct->mask_cache, mvebu_gpioreg_level_mask(mvchip));
 	irq_gc_unlock(gc);
 }
 
 static void mvebu_gpio_level_irq_unmask(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);
 	struct mvebu_gpio_chip *mvchip = gc->private;
 	u32 mask = 1 << (d->irq - gc->irq_base);
 
 	irq_gc_lock(gc);
-	gc->mask_cache |= mask;
-	writel_relaxed(gc->mask_cache, mvebu_gpioreg_level_mask(mvchip));
+	*ct->mask_cache |= mask;
+	writel_relaxed(*ct->mask_cache, mvebu_gpioreg_level_mask(mvchip));
 	irq_gc_unlock(gc);
 }
 
@@ -708,7 +712,8 @@ static int mvebu_gpio_probe(struct platform_device *pdev)
 	ct->handler = handle_edge_irq;
 	ct->chip.name = mvchip->chip.label;
 
-	irq_setup_generic_chip(gc, IRQ_MSK(ngpios), 0,
+	irq_setup_generic_chip(gc, IRQ_MSK(ngpios),
+			       IRQ_GC_MASK_CACHE_PER_TYPE,
 			       IRQ_NOREQUEST, IRQ_LEVEL | IRQ_NOPROBE);
 
 	/* Setup irq domain on top of the generic chip. */
-- 
1.8.0.1

[PATCH 0/1] fix MVEBU GPIO driver bug causing kernel hang

[Thomas Petazzoni]

Dear Gerlando Falauto,

On Wed,  2 Oct 2013 14:31:58 +0200, Gerlando Falauto wrote:

> SIDE NOTE: If I understand correctly, a brand new driver from
> Sebastian Hesselbarth (drivers/irqchip/irq-orion.c) may replace this driver
> in the future, but it still misses the IRQ_GC_MASK_CACHE_PER_TYPE bit:

Hum? The driver you're touching is a GPIO driver, while
drivers/irqchip/irq-orion.c is an interrupt controller driver. The
gpio-mvebu driver can be used on all mvebu platforms that are DT
capable, but the main interrupt controller is different from one
platform to another: we already have irq-armada-370-xp.c for Armada
370/XP, while Sebastian's irq-orion.c is for older families of SoCs.

Or maybe I'm missing something?

Thanks!

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

[PATCH 0/1] fix MVEBU GPIO driver bug causing kernel hang

[Gerlando Falauto]

Hi Thomas,

On 10/02/2013 02:45 PM, Thomas Petazzoni wrote:
> Dear Gerlando Falauto,
>
> On Wed,  2 Oct 2013 14:31:58 +0200, Gerlando Falauto wrote:
>
>> SIDE NOTE: If I understand correctly, a brand new driver from
>> Sebastian Hesselbarth (drivers/irqchip/irq-orion.c) may replace this driver
>> in the future, but it still misses the IRQ_GC_MASK_CACHE_PER_TYPE bit:
>
> Hum? The driver you're touching is a GPIO driver, while
> drivers/irqchip/irq-orion.c is an interrupt controller driver. The
> gpio-mvebu driver can be used on all mvebu platforms that are DT
> capable, but the main interrupt controller is different from one
> platform to another: we already have irq-armada-370-xp.c for Armada
> 370/XP, while Sebastian's irq-orion.c is for older families of SoCs.

My foreword was "if I understand correctly", which I clearly didn't. ;-)

OK, so Sebastian's rework irq-orion.c is about the MAIN interrupt 
controller (replaces arch/arm/plat-orion/irq.c), which also cascades 
interrupts from GPIOs.
These are then either handled by gpio-mvebu.c (for all platforms which 
are DT capable) or by arch/arm/plat-orion/gpio.c (legacy).
Is that right?

> Or maybe I'm missing something?

Sorry, I'm the one missing something here... perhaps too much.

Thanks!
Gerlando

[PATCH 0/1] fix MVEBU GPIO driver bug causing kernel hang

[Thomas Petazzoni]

Dear Gerlando Falauto,

On Wed, 02 Oct 2013 15:14:28 +0200, Gerlando Falauto wrote:

> > Hum? The driver you're touching is a GPIO driver, while
> > drivers/irqchip/irq-orion.c is an interrupt controller driver. The
> > gpio-mvebu driver can be used on all mvebu platforms that are DT
> > capable, but the main interrupt controller is different from one
> > platform to another: we already have irq-armada-370-xp.c for Armada
> > 370/XP, while Sebastian's irq-orion.c is for older families of SoCs.
> 
> My foreword was "if I understand correctly", which I clearly didn't. ;-)
> 
> OK, so Sebastian's rework irq-orion.c is about the MAIN interrupt 
> controller (replaces arch/arm/plat-orion/irq.c), which also cascades 
> interrupts from GPIOs.
> These are then either handled by gpio-mvebu.c (for all platforms which 
> are DT capable) or by arch/arm/plat-orion/gpio.c (legacy).
> Is that right?

Yes, that's right.

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

[PATCH 1/1] gpio: mvebu: enable and use IRQ_GC_MASK_CACHE_PER_TYPE

[Linus Walleij]

On Wed, Oct 2, 2013 at 2:34 PM, Gerlando Falauto
<gerlando.falauto@keymile.com> wrote:

> Since we have now introduced mask_cache within irq_chip_type to also
> handle per-chip-type mask registers, convert gpio-mvebu driver to use
> this new pointer.
>
> Also enable IRQ_GC_MASK_CACHE_PER_TYPE to actually handle separate mask
> registers for all three SoC variants handled by this driver.
>
> This wll fix a bug where requesting (and triggering) both EDGE- and
> LEVEL- based IRQs causes the kernel to hang.
>
> Signed-off-by: Gerlando Falauto <gerlando.falauto@keymile.com>
> Cc: Simon Guinot <sguinot@lacie.com>
> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Cc: Linus Walleij <linus.walleij@stericsson.com>
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: Jason Cooper <jason@lakedaemon.net>
> Cc: Gregory Clement <gregory.clement@free-electrons.com>
> Cc: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: linux-arm-kernel at lists.infradead.org

Thomas/Gregory: can you comment on this patch?

Yours,
Linus Walleij

[PATCH 0/1] fix MVEBU GPIO driver bug causing kernel hang

[Linus Walleij]

OK I see these comments on patch [0/1] now (how confusing)
forget what I just said. Patch is dropped.

Yours,
Linus Walleij

[PATCH 0/1] fix MVEBU GPIO driver bug causing kernel hang

[Gerlando Falauto]

Hi LinusW,

what do you mean?
My statement on patch [0/1] contained speculation about a driver which 
is clearly unrelated (the main, generic, orion IRQ controller, which is 
*NOT* what this patch is about). Our discussion with Thomas was about 
this and him explaining how I got the two things confused in my mind.
But the patch, in its real context (the GPIO chip and its interrupt 
handling) should still be sound and needed...
[Patch 0/1] will not be in the commit message anyway...

Thanks,
Gerlando


On 10/11/2013 01:15 PM, Linus Walleij wrote:
> OK I see these comments on patch [0/1] now (how confusing)
> forget what I just said. Patch is dropped.
>
> Yours,
> Linus Walleij
>

[PATCH 0/1] fix MVEBU GPIO driver bug causing kernel hang

[Linus Walleij]

On Fri, Oct 11, 2013 at 1:21 PM, Gerlando Falauto
<gerlando.falauto@keymile.com> wrote:

> My statement on patch [0/1] contained speculation about a driver which is
> clearly unrelated (the main, generic, orion IRQ controller, which is *NOT*
> what this patch is about).

OK I got it all wrong then, I'll wait for an ACK on patch 1/1 before
applying though.

Yours,
Linus Walleij