[PATCH] gpio: pl061: move irqdomain initialization

[PATCH] gpio: pl061: move irqdomain initialization

[Linus Walleij]

The PL061 driver had the irqdomain initialization in an unfortunate
place: when used with device tree (and thus passing the base IRQ
0) the driver would work, as this registers an irqdomain and waits
for mappings to be done dynamically as the devices request their
IRQs, whereas when booting using platform data the irqdomain core
would attempt to allocate IRQ descriptors dynamically (which works
fine) but also to associate the irq_domain_associate_many() on all
IRQs, which in turn will call the mapping function which at this
point will try to set the type of the IRQ and then tries to acquire
a non-initialized spinlock yielding a backtrace like this:

CPU: 0 PID: 1 Comm: swapper Not tainted 3.13.0-rc1+ #652
Backtrace:
[<c0016f0c>] (dump_backtrace) from [<c00172ac>] (show_stack+0x18/0x1c)
 r6:c798ace0 r5:00000000 r4:c78257e0 r3:00200140
[<c0017294>] (show_stack) from [<c0329ea0>] (dump_stack+0x20/0x28)
[<c0329e80>] (dump_stack) from [<c004fa80>] (__lock_acquire+0x1c0/0x1b80)
[<c004f8c0>] (__lock_acquire) from [<c0051970>] (lock_acquire+0x6c/0x80)
 r10:00000000 r9:c0455234 r8:00000060 r7:c047d798 r6:600000d3 r5:00000000
 r4:c782c000
[<c0051904>] (lock_acquire) from [<c032e484>] (_raw_spin_lock_irqsave+0x60/0x74)
 r6:c01a1100 r5:800000d3 r4:c798acd0
[<c032e424>] (_raw_spin_lock_irqsave) from [<c01a1100>] (pl061_irq_type+0x28/0x)
 r6:00000000 r5:00000000 r4:c798acd0
[<c01a10d8>] (pl061_irq_type) from [<c0059ef4>] (__irq_set_trigger+0x70/0x104)
 r6:00000000 r5:c01a10d8 r4:c046da1c r3:c01a10d8
[<c0059e84>] (__irq_set_trigger) from [<c005b348>] (irq_set_irq_type+0x40/0x60)
 r10:c043240c r8:00000060 r7:00000000 r6:c046da1c r5:00000060 r4:00000000
[<c005b308>] (irq_set_irq_type) from [<c01a1208>] (pl061_irq_map+0x40/0x54)
 r6:c79693c0 r5:c798acd0 r4:00000060
[<c01a11c8>] (pl061_irq_map) from [<c005d27c>] (irq_domain_associate+0xc0/0x190)
 r5:00000060 r4:c046da1c
[<c005d1bc>] (irq_domain_associate) from [<c005d604>] (irq_domain_associate_man)
 r8:00000008 r7:00000000 r6:c79693c0 r5:00000060 r4:00000000
[<c005d5d0>] (irq_domain_associate_many) from [<c005d864>] (irq_domain_add_simp)
 r8:c046578c r7:c035b72c r6:c79693c0 r5:00000060 r4:00000008 r3:00000008
[<c005d814>] (irq_domain_add_simple) from [<c01a1380>] (pl061_probe+0xc4/0x22c)
 r6:00000060 r5:c0464380 r4:c798acd0
[<c01a12bc>] (pl061_probe) from [<c01c0450>] (amba_probe+0x74/0xe0)
 r10:c043240c r9:c0455234 r8:00000000 r7:c047d7f8 r6:c047d744 r5:00000000
 r4:c0464380

This moves the irqdomain initialization to a point where the spinlock
and GPIO chip are both fully propulated, so the callbacks can be used
without crashes.

I had some problem reproducing the crash, as the devm_kzalloc():ed
zeroed memory would seemingly mask the spinlock as something OK,
but by poisoning the lock like this:

u32 *dum;
dum = (u32 *) &chip->lock;
*dum = 0xaaaaaaaaU;

I could reproduce, fix and test the patch.

Reported-by: Russell King <linux@arm.linux.org.uk>
Cc: Rob Herring <robherring2@gmail.com>
Cc: Haojian Zhuang <haojian.zhuang@linaro.org>
Cc: Baruch Siach <baruch@tkos.co.il>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpio/gpio-pl061.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpio/gpio-pl061.c b/drivers/gpio/gpio-pl061.c
index f22f7f3e2e53..b4d42112d02d 100644
--- a/drivers/gpio/gpio-pl061.c
+++ b/drivers/gpio/gpio-pl061.c
@@ -286,11 +286,6 @@ static int pl061_probe(struct amba_device *adev, const struct amba_id *id)
 	if (!chip->base)
 		return -ENOMEM;
 
-	chip->domain = irq_domain_add_simple(adev->dev.of_node, PL061_GPIO_NR,
-					     irq_base, &pl061_domain_ops, chip);
-	if (!chip->domain)
-		return -ENODEV;
-
 	spin_lock_init(&chip->lock);
 
 	chip->gc.request = pl061_gpio_request;
@@ -320,6 +315,11 @@ static int pl061_probe(struct amba_device *adev, const struct amba_id *id)
 	irq_set_chained_handler(irq, pl061_irq_handler);
 	irq_set_handler_data(irq, chip);
 
+	chip->domain = irq_domain_add_simple(adev->dev.of_node, PL061_GPIO_NR,
+					     irq_base, &pl061_domain_ops, chip);
+	if (!chip->domain)
+		return -ENODEV;
+
 	for (i = 0; i < PL061_GPIO_NR; i++) {
 		if (pdata) {
 			if (pdata->directions & (1 << i))
-- 
1.8.3.1

Re: [PATCH] gpio: pl061: move irqdomain initialization

[Baruch Siach]

Hi Linus,

On Wed, Nov 27, 2013 at 02:11:21PM +0100, Linus Walleij wrote:
> The PL061 driver had the irqdomain initialization in an unfortunate
> place: when used with device tree (and thus passing the base IRQ
> 0) the driver would work, as this registers an irqdomain and waits
> for mappings to be done dynamically as the devices request their
> IRQs, whereas when booting using platform data the irqdomain core
> would attempt to allocate IRQ descriptors dynamically (which works
> fine) but also to associate the irq_domain_associate_many() on all
> IRQs, which in turn will call the mapping function which at this
> point will try to set the type of the IRQ and then tries to acquire
> a non-initialized spinlock yielding a backtrace like this:
> 
> CPU: 0 PID: 1 Comm: swapper Not tainted 3.13.0-rc1+ #652
> Backtrace:
> [<c0016f0c>] (dump_backtrace) from [<c00172ac>] (show_stack+0x18/0x1c)
>  r6:c798ace0 r5:00000000 r4:c78257e0 r3:00200140
> [<c0017294>] (show_stack) from [<c0329ea0>] (dump_stack+0x20/0x28)
> [<c0329e80>] (dump_stack) from [<c004fa80>] (__lock_acquire+0x1c0/0x1b80)
> [<c004f8c0>] (__lock_acquire) from [<c0051970>] (lock_acquire+0x6c/0x80)
>  r10:00000000 r9:c0455234 r8:00000060 r7:c047d798 r6:600000d3 r5:00000000
>  r4:c782c000
> [<c0051904>] (lock_acquire) from [<c032e484>] (_raw_spin_lock_irqsave+0x60/0x74)
>  r6:c01a1100 r5:800000d3 r4:c798acd0
> [<c032e424>] (_raw_spin_lock_irqsave) from [<c01a1100>] (pl061_irq_type+0x28/0x)
>  r6:00000000 r5:00000000 r4:c798acd0
> [<c01a10d8>] (pl061_irq_type) from [<c0059ef4>] (__irq_set_trigger+0x70/0x104)
>  r6:00000000 r5:c01a10d8 r4:c046da1c r3:c01a10d8
> [<c0059e84>] (__irq_set_trigger) from [<c005b348>] (irq_set_irq_type+0x40/0x60)
>  r10:c043240c r8:00000060 r7:00000000 r6:c046da1c r5:00000060 r4:00000000
> [<c005b308>] (irq_set_irq_type) from [<c01a1208>] (pl061_irq_map+0x40/0x54)
>  r6:c79693c0 r5:c798acd0 r4:00000060
> [<c01a11c8>] (pl061_irq_map) from [<c005d27c>] (irq_domain_associate+0xc0/0x190)
>  r5:00000060 r4:c046da1c
> [<c005d1bc>] (irq_domain_associate) from [<c005d604>] (irq_domain_associate_man)
>  r8:00000008 r7:00000000 r6:c79693c0 r5:00000060 r4:00000000
> [<c005d5d0>] (irq_domain_associate_many) from [<c005d864>] (irq_domain_add_simp)
>  r8:c046578c r7:c035b72c r6:c79693c0 r5:00000060 r4:00000008 r3:00000008
> [<c005d814>] (irq_domain_add_simple) from [<c01a1380>] (pl061_probe+0xc4/0x22c)
>  r6:00000060 r5:c0464380 r4:c798acd0
> [<c01a12bc>] (pl061_probe) from [<c01c0450>] (amba_probe+0x74/0xe0)
>  r10:c043240c r9:c0455234 r8:00000000 r7:c047d7f8 r6:c047d744 r5:00000000
>  r4:c0464380
> 
> This moves the irqdomain initialization to a point where the spinlock
> and GPIO chip are both fully propulated, so the callbacks can be used
> without crashes.
> 
> I had some problem reproducing the crash, as the devm_kzalloc():ed
> zeroed memory would seemingly mask the spinlock as something OK,
> but by poisoning the lock like this:
> 
> u32 *dum;
> dum = (u32 *) &chip->lock;
> *dum = 0xaaaaaaaaU;
> 
> I could reproduce, fix and test the patch.
> 
> Reported-by: Russell King <linux@arm.linux.org.uk>
> Cc: Rob Herring <robherring2@gmail.com>
> Cc: Haojian Zhuang <haojian.zhuang@linaro.org>
> Cc: Baruch Siach <baruch@tkos.co.il>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

Isn't this a matter for -stable (v3.10+)?

baruch

> ---
>  drivers/gpio/gpio-pl061.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-pl061.c b/drivers/gpio/gpio-pl061.c
> index f22f7f3e2e53..b4d42112d02d 100644
> --- a/drivers/gpio/gpio-pl061.c
> +++ b/drivers/gpio/gpio-pl061.c
> @@ -286,11 +286,6 @@ static int pl061_probe(struct amba_device *adev, const struct amba_id *id)
>  	if (!chip->base)
>  		return -ENOMEM;
>  
> -	chip->domain = irq_domain_add_simple(adev->dev.of_node, PL061_GPIO_NR,
> -					     irq_base, &pl061_domain_ops, chip);
> -	if (!chip->domain)
> -		return -ENODEV;
> -
>  	spin_lock_init(&chip->lock);
>  
>  	chip->gc.request = pl061_gpio_request;
> @@ -320,6 +315,11 @@ static int pl061_probe(struct amba_device *adev, const struct amba_id *id)
>  	irq_set_chained_handler(irq, pl061_irq_handler);
>  	irq_set_handler_data(irq, chip);
>  
> +	chip->domain = irq_domain_add_simple(adev->dev.of_node, PL061_GPIO_NR,
> +					     irq_base, &pl061_domain_ops, chip);
> +	if (!chip->domain)
> +		return -ENODEV;
> +
>  	for (i = 0; i < PL061_GPIO_NR; i++) {
>  		if (pdata) {
>  			if (pdata->directions & (1 << i))

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

Re: [PATCH] gpio: pl061: move irqdomain initialization

[Linus Walleij]

On Wed, Nov 27, 2013 at 2:41 PM, Baruch Siach <baruch@tkos.co.il> wrote:

> Isn't this a matter for -stable (v3.10+)?

Sure I just missed to add that tag. I'll send a separate ping to Greg
on stable@vger once this is upstream, I've put a reminder into my
calendar...

Yours,
Linus Walleij