diff mbox

ARM: imx: irq: fix buggy usage of irq_data irq field

Message ID 1417451109-30276-1-git-send-email-marc.zyngier@arm.com
State New
Headers show

Commit Message

Marc Zyngier Dec. 1, 2014, 4:25 p.m. UTC 
mach-imx directly references to the irq field in
struct irq_data, and uses this to directly poke hardware register.

But irq is the *virtual* irq number, something that has nothing
to do with the actual HW irq (stored in the hwirq field). And once
we put the stacked domain code in action, the whole thing explodes,
as these two values are *very* different.

Just replacing all instances of irq with hwirq fixes the issue.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm/mach-imx/gpc.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

Comments

Fabio Estevam Dec. 1, 2014, 5 p.m. UTC | #1 
Hi Marc,

On Mon, Dec 1, 2014 at 2:25 PM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> mach-imx directly references to the irq field in
> struct irq_data, and uses this to directly poke hardware register.
>
> But irq is the *virtual* irq number, something that has nothing
> to do with the actual HW irq (stored in the hwirq field). And once
> we put the stacked domain code in action, the whole thing explodes,
> as these two values are *very* different.
>
> Just replacing all instances of irq with hwirq fixes the issue.
>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>

I tested your patch and I still have the following problem on a mx6q:

[    0.000000] Unable to handle kernel NULL pointer dereference at
virtual address 00000008
[    0.000000] pgd = 80004000
[    0.000000] [00000008] *pgd=00000000
[    0.000000] Internal error: Oops: 5 [#1] SMP ARM
[    0.000000] Modules linked in:
[    0.000000] CPU: 0 PID: 0 Comm: swapper/0 Not tainted
3.18.0-rc6-next-20141201-dirty #341
[    0.000000] Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
[    0.000000] task: 8097c2b0 ti: 80970000 task.ti: 80970000
[    0.000000] PC is at imx_gpc_irq_unmask+0xc/0x4c
[    0.000000] LR is at imx6q_set_lpm+0x80/0x10c
[    0.000000] pc : [<80026598>]    lr : [<8002757c>]    psr: 800001d3
[    0.000000] sp : 80971ea8  ip : 80971eb8  fp : 80971eb4
[    0.000000] r10: c0818078  r9 : 809d8524  r8 : c0818074
[    0.000000] r7 : 00000000  r6 : 809d8fec  r5 : 00000000  r4 : 00000078
[    0.000000] r3 : c0818000  r2 : 00000079  r1 : 00000020  r0 : 00000000
[    0.000000] Flags: Nzcv  IRQs off  FIQs off  Mode SVC_32  ISA ARM
Segment kernel
[    0.000000] Control: 10c5387d  Table: 1000404a  DAC: 00000015
[    0.000000] Process swapper/0 (pid: 0, stack limit = 0x80970238)
[    0.000000] Stack: (0x80971ea8 to 0x80972000)
[    0.000000] 1ea0:                   80971ed4 80971eb8 8002757c
80026598 809d8524 809d8524
[    0.000000] 1ec0: 80968a18 809d8524 80971f64 80971ed8 809293f4
80027508 c0818060 00000018
[    0.000000] 1ee0: 00000000 8097f348 00000000 8097f348 0000001a
be7cfe84 c081803c c0818020
[    0.000000] 1f00: c0818024 c0818028 c0818034 c0818048 c0818060
c0818038 c081802c c0818068
[    0.000000] 1f20: c0818030 c0818014 c081801c c081806c c0818018
c0818080 016e3600 00000001
[    0.000000] 1f40: be7cfe84 be0027c0 809c7e2c 00000000 809c7d18
809c7e34 80971fa4 80971f68
[    0.000000] 1f60: 80950984 80925cac 00000008 00000001 00000000
00000000 00000000 00000000
[    0.000000] 1f80: 00000001 ffffffff 809d7a00 8095e9d8 412fc09a
befffa40 80971fb4 80971fa8
[    0.000000] 1fa0: 809175fc 809508bc 80971ff4 80971fb8 80913b9c
809175dc ffffffff ffffffff
[    0.000000] 1fc0: 809136d4 00000000 00000000 8095e9d8 809d7c94
80978968 8095e9d4 8097d9fc
[    0.000000] 1fe0: 1000406a 00000000 00000000 80971ff8 10008074
8091395c 00000000 00000000
[    0.000000] Backtrace:
[    0.000000] [<8002658c>] (imx_gpc_irq_unmask) from [<8002757c>]
(imx6q_set_lpm+0x80/0x10c)
[    0.000000] [<800274fc>] (imx6q_set_lpm) from [<809293f4>]
(imx6q_clocks_init+0x3754/0x375c)
[    0.000000]  r7:809d8524 r6:80968a18 r5:809d8524 r4:809d8524
[    0.000000] [<80925ca0>] (imx6q_clocks_init) from [<80950984>]
(of_clk_init+0xd4/0x1a0)
[    0.000000]  r10:809c7e34 r9:809c7d18 r8:00000000 r7:809c7e2c
r6:be0027c0 r5:be7cfe84
[    0.000000]  r4:00000001
[    0.000000] [<809508b0>] (of_clk_init) from [<809175fc>]
(time_init+0x2c/0x38)
[    0.000000]  r10:befffa40 r9:412fc09a r8:8095e9d8 r7:809d7a00
r6:ffffffff r5:00000001
[    0.000000]  r4:00000000
[    0.000000] [<809175d0>] (time_init) from [<80913b9c>]
(start_kernel+0x24c/0x3d8)
[    0.000000] [<80913950>] (start_kernel) from [<10008074>] (0x10008074)
[    0.000000]  r10:00000000 r8:1000406a r7:8097d9fc r6:8095e9d4
r5:80978968 r4:809d7c94
[    0.000000] Code: 809d84f0 e1a0c00d e92dd800 e24cb004 (e5903008)
[    0.000000] ---[ end trace cb88537fdc8fa200 ]---
[    0.000000] Kernel panic - not syncing: Attempted to kill the idle task!
[    0.000000] ---[ end Kernel panic - not syncing: Attempted to kill
the idle task!

This issue does not happen on linux-next 20141126, but it stats at 201411267.

I haven't bisect it yet, but if you have any ideas, please let me know. Thanks
Marc Zyngier Dec. 1, 2014, 5:03 p.m. UTC | #2 
On 01/12/14 17:00, Fabio Estevam wrote:
> Hi Marc,
> 
> On Mon, Dec 1, 2014 at 2:25 PM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>> mach-imx directly references to the irq field in
>> struct irq_data, and uses this to directly poke hardware register.
>>
>> But irq is the *virtual* irq number, something that has nothing
>> to do with the actual HW irq (stored in the hwirq field). And once
>> we put the stacked domain code in action, the whole thing explodes,
>> as these two values are *very* different.
>>
>> Just replacing all instances of irq with hwirq fixes the issue.
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> 
> I tested your patch and I still have the following problem on a mx6q:

[...]

> This issue does not happen on linux-next 20141126, but it stats at 201411267.
> 
> I haven't bisect it yet, but if you have any ideas, please let me know. Thanks

I do have an idea indeed, as well as a patch for this. I'll put you on
Cc, watch that space.

Thanks,

	M.
Philipp Zabel Dec. 1, 2014, 5:12 p.m. UTC | #3 
Hi Marc,

Am Montag, den 01.12.2014, 16:25 +0000 schrieb Marc Zyngier:
> mach-imx directly references to the irq field in
> struct irq_data, and uses this to directly poke hardware register.
> 
> But irq is the *virtual* irq number, something that has nothing
> to do with the actual HW irq (stored in the hwirq field). And once
> we put the stacked domain code in action, the whole thing explodes,
> as these two values are *very* different.
> 
> Just replacing all instances of irq with hwirq fixes the issue.

I have tried this on next-20141128, but due to 
	struct irq_data *iomuxc_irq_data = irq_get_irq_data(32); /* now returns NULL */
in arch/arm/mach-imx/pm-imx6q.c it still explodes:

Unable to handle kernel NULL pointer dereference at virtual address 00000008
pgd = 80004000
[00000008] *pgd=00000000
Internal error: Oops: 5 [#1] SMP ARM
Modules linked in:
CPU: 0 PID: 0 Comm: swapper/0 Not tainted 3.18.0-rc6-next-20141128+ #8437
Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
task: 808520e8 ti: 80846000 task.ti: 80846000
PC is at imx_gpc_irq_unmask+0x14/0x54
LR is at imx6q_set_lpm+0x6c/0xe0
pc : [<8001ec90>]    lr : [<8001fb6c>]    psr: 800001d3
sp : 80847ea0  ip : 8001ec90  fp : 80847eac
r10: 00000000  r9 : 80891c4c  r8 : c0818080
r7 : 00000000  r6 : 00000000  r5 : 00000078  r4 : 80892068
r3 : c0818000  r2 : 00000079  r1 : 00000020  r0 : 00000000
Flags: Nzcv  IRQs off  FIQs off  Mode SVC_32  ISA ARM  Segment kernel
Control: 10c5387d  Table: 1000404a  DAC: 00000015
Process swapper/0 (pid: 0, stack limit = 0x80846240)
Stack: (0x80847ea0 to 0x80848000)
7ea0: 80847ecc 80847eb0 8001fb6c 8001ec88 80891c94 af01e080 8083f9e8 80891c94
7ec0: 80847f54 80847ed0 807dddd0 8001fb0c c0818060 00000018 00000000 80891c4c
7ee0: 00000000 80891c4c c0818024 af6644c0 c081803c c0818020 c0818060 c0818038
7f00: c0818028 c0818034 c0818048 c0818024 c081802c c0818014 8001e3dc c081801c
7f20: 80891c4c c0818000 016e3600 00000001 af6644c0 af002740 808801e4 00200200
7f40: 00100100 00000000 80847f94 80847f58 80807710 807da4bc 80880180 00000001
7f60: 00000000 00000000 00000000 00000000 808917c0 00000001 ffffffff 80820808
7f80: 8084e480 808917c0 80847fa4 80847f98 807d57b8 80807608 80847ff4 80847fa8
7fa0: 807d1bd4 807d5794 ffffffff ffffffff 807d16d0 00000000 00000000 affff9c0
7fc0: 00000000 80820808 00000000 80891a54 8084e4fc 80820804 808531c4 1000406a
7fe0: 412fc09a 00000000 00000000 80847ff8 10008074 807d1960 00000000 00000000
Backtrace: 
[<8001ec7c>] (imx_gpc_irq_unmask) from [<8001fb6c>] (imx6q_set_lpm+0x6c/0xe0)
[<8001fb00>] (imx6q_set_lpm) from [<807dddd0>] (imx6q_clocks_init+0x3920/0x392c)
 r7:80891c94 r6:8083f9e8 r5:af01e080 r4:80891c94
[<807da4b0>] (imx6q_clocks_init) from [<80807710>] (of_clk_init+0x114/0x1a4)
 r10:00000000 r9:00100100 r8:00200200 r7:808801e4 r6:af002740 r5:af6644c0
 r4:00000001
[<808075fc>] (of_clk_init) from [<807d57b8>] (time_init+0x30/0x38)
 r10:808917c0 r9:8084e480 r8:80820808 r7:ffffffff r6:00000001 r5:808917c0
 r4:00000000
[<807d5788>] (time_init) from [<807d1bd4>] (start_kernel+0x280/0x3dc)
[<807d1954>] (start_kernel) from [<10008074>] (0x10008074)
 r10:00000000 r9:412fc09a r8:1000406a r7:808531c4 r6:80820804 r5:8084e4fc
 r4:80891a54
Code: e92dd800 e24cb004 e52de004 ebffdbbb (e5903008) 
---[ end trace cb88537fdc8fa200 ]---

> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  arch/arm/mach-imx/gpc.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/arm/mach-imx/gpc.c b/arch/arm/mach-imx/gpc.c
> index 82ea74e..1455829 100644
> --- a/arch/arm/mach-imx/gpc.c
> +++ b/arch/arm/mach-imx/gpc.c
> @@ -56,14 +56,14 @@ void imx_gpc_post_resume(void)
>  
>  static int imx_gpc_irq_set_wake(struct irq_data *d, unsigned int on)
>  {
> -	unsigned int idx = d->irq / 32 - 1;
> +	unsigned int idx = d->hwirq / 32 - 1;
>  	u32 mask;
>  
>  	/* Sanity check for SPI irq */
> -	if (d->irq < 32)
> +	if (d->hwirq < 32)
>  		return -EINVAL;
>  
> -	mask = 1 << d->irq % 32;
> +	mask = 1 << d->hwirq % 32;
>  	gpc_wake_irqs[idx] = on ? gpc_wake_irqs[idx] | mask :
>  				  gpc_wake_irqs[idx] & ~mask;
>  
> @@ -97,12 +97,12 @@ void imx_gpc_irq_unmask(struct irq_data *d)
>  	u32 val;
>  
>  	/* Sanity check for SPI irq */
> -	if (d->irq < 32)
> +	if (d->hwirq < 32)
>  		return;
>  
> -	reg = gpc_base + GPC_IMR1 + (d->irq / 32 - 1) * 4;
> +	reg = gpc_base + GPC_IMR1 + (d->hwirq / 32 - 1) * 4;
>  	val = readl_relaxed(reg);
> -	val &= ~(1 << d->irq % 32);
> +	val &= ~(1 << d->hwirq % 32);
>  	writel_relaxed(val, reg);
>  }
>  
> @@ -112,12 +112,12 @@ void imx_gpc_irq_mask(struct irq_data *d)
>  	u32 val;
>  
>  	/* Sanity check for SPI irq */
> -	if (d->irq < 32)
> +	if (d->hwirq < 32)
>  		return;
>  
> -	reg = gpc_base + GPC_IMR1 + (d->irq / 32 - 1) * 4;
> +	reg = gpc_base + GPC_IMR1 + (d->hwirq / 32 - 1) * 4;
>  	val = readl_relaxed(reg);
> -	val |= 1 << (d->irq % 32);
> +	val |= 1 << (d->hwirq % 32);
>  	writel_relaxed(val, reg);
>  }
>
Fabio Estevam Dec. 1, 2014, 5:14 p.m. UTC | #4 
On Mon, Dec 1, 2014 at 2:25 PM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> mach-imx directly references to the irq field in
> struct irq_data, and uses this to directly poke hardware register.
>
> But irq is the *virtual* irq number, something that has nothing
> to do with the actual HW irq (stored in the hwirq field). And once
> we put the stacked domain code in action, the whole thing explodes,
> as these two values are *very* different.
>
> Just replacing all instances of irq with hwirq fixes the issue.
>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>

Tested-by: Fabio Estevam <fabio.estevam@freescale.com>
diff mbox

Patch

diff --git a/arch/arm/mach-imx/gpc.c b/arch/arm/mach-imx/gpc.c
index 82ea74e..1455829 100644
--- a/arch/arm/mach-imx/gpc.c
+++ b/arch/arm/mach-imx/gpc.c
@@ -56,14 +56,14 @@  void imx_gpc_post_resume(void)
 
 static int imx_gpc_irq_set_wake(struct irq_data *d, unsigned int on)
 {
-	unsigned int idx = d->irq / 32 - 1;
+	unsigned int idx = d->hwirq / 32 - 1;
 	u32 mask;
 
 	/* Sanity check for SPI irq */
-	if (d->irq < 32)
+	if (d->hwirq < 32)
 		return -EINVAL;
 
-	mask = 1 << d->irq % 32;
+	mask = 1 << d->hwirq % 32;
 	gpc_wake_irqs[idx] = on ? gpc_wake_irqs[idx] | mask :
 				  gpc_wake_irqs[idx] & ~mask;
 
@@ -97,12 +97,12 @@  void imx_gpc_irq_unmask(struct irq_data *d)
 	u32 val;
 
 	/* Sanity check for SPI irq */
-	if (d->irq < 32)
+	if (d->hwirq < 32)
 		return;
 
-	reg = gpc_base + GPC_IMR1 + (d->irq / 32 - 1) * 4;
+	reg = gpc_base + GPC_IMR1 + (d->hwirq / 32 - 1) * 4;
 	val = readl_relaxed(reg);
-	val &= ~(1 << d->irq % 32);
+	val &= ~(1 << d->hwirq % 32);
 	writel_relaxed(val, reg);
 }
 
@@ -112,12 +112,12 @@  void imx_gpc_irq_mask(struct irq_data *d)
 	u32 val;
 
 	/* Sanity check for SPI irq */
-	if (d->irq < 32)
+	if (d->hwirq < 32)
 		return;
 
-	reg = gpc_base + GPC_IMR1 + (d->irq / 32 - 1) * 4;
+	reg = gpc_base + GPC_IMR1 + (d->hwirq / 32 - 1) * 4;
 	val = readl_relaxed(reg);
-	val |= 1 << (d->irq % 32);
+	val |= 1 << (d->hwirq % 32);
 	writel_relaxed(val, reg);
 }