* [PATCH 0/3] ARM: at91/tclib: fix segmentation fault @ 2014-08-19 22:07 ` Gaël PORTAY 0 siblings, 0 replies; 34+ messages in thread From: Gaël PORTAY @ 2014-08-19 22:07 UTC (permalink / raw) To: Arnd Bergmann, Daniel Lezcano, Greg Kroah-Hartman, linux-arm-kernel, linux-kernel, linux-pwm, Nicolas Ferre, Thierry Reding, Thomas Gleixner Cc: Boris Brezillon, Alexandre Belloni, Jean-Christophe PLAGNIOL-VILLARD, Gaël PORTAY Hi every one, This set of patches fix a segmentation fault happening when kexec-ing kernel on an at91 platform (see backtrace below). While the previous kernel shuts down, the tcb_clksrc driver leaves its interruptions unmasked. When the new kernel initiliazes any tclib making use of a TC block, an interruption may happen before the interrupt handler is set, causing a kernel segmentation fault. To prevent from such cases from happening, the last patch sets up the shutdown callback which masks interruptions when the machine is shutdown. Furthermore, it also masks the interruptions at probe to make sure no interruption happens before the handler is set. This will prevent freshly kexec-ed kernel from crashing when launched from a kernel which does not properly mask interruptions at shutdown. Unable to handle kernel NULL pointer dereference at virtual address 00000000 pgd = c0004000 [00000000] *pgd=00000000 Internal error: Oops: 80000005 [#1] ARM Modules linked in: CPU: 0 PID: 1 Comm: swapper Not tainted 3.16.0+ #144 task: c1828aa0 ti: c182a000 task.ti: c182a000 PC is at 0x0 LR is at ch2_irq+0x28/0x30 pc : [<00000000>] lr : [<c01db904>] psr: 000000d3 sp : c182bd38 ip : c182bd48 fp : c182bd44 r10: c0373390 r9 : c1825b00 r8 : 60000053 r7 : 00000000 r6 : 00000000 r5 : 00000013 r4 : c036e800 r3 : 00000000 r2 : 00002004 r1 : c036e760 r0 : c036e760 Flags: nzcv IRQs off FIQs off Mode SVC_32 ISA ARM Segment kernel Control: 0005317f Table: 20004000 DAC: 00000017 Process swapper (pid: 1, stack limit = 0xc182a1c0) Stack: (0xc182bd38 to 0xc182c000) bd20: c182bd7c c182bd48 bd40: c0045430 c01db8ec 00000000 c18c6f40 c182bd74 c1825b00 c035cec4 00000000 bd60: c182be2c 60000053 c1825b34 00000000 c182bd94 c182bd80 c0045570 c0045408 bd80: 00000000 c1825b00 c182bdac c182bd98 c0047f34 c0045550 00000013 c036619c bda0: c182bdc4 c182bdb0 c0044da4 c0047e98 0000007f 00000013 c182bde4 c182bdc8 bdc0: c0009e34 c0044d8c fefff000 c0046728 60000053 ffffffff c182bdf4 c182bde8 bde0: c00086a8 c0009ddc c182be74 c182bdf8 c000cb80 c0008674 00000000 00000013 be00: 00000000 00014200 c1825b00 c036e800 00000013 c035ed98 60000053 c1825b34 be20: 00000000 c182be74 c182be20 c182be40 c0047994 c0046728 60000053 ffffffff be40: 00000013 c036e800 c182be64 c1825b00 00000013 c036e800 c035ed98 c03874bc be60: 00000004 c036e700 c182be94 c182be78 c004689c c0046398 c036e760 c18c6080 be80: 00000000 c035ed10 c182bedc c182be98 c0348b08 c004684c 0000000c c034dac8 bea0: 004c4b3f c028c338 c036e760 00000013 c014ecc8 c18e67e0 c035b9c0 c0348884 bec0: c035b9c0 c182a020 00000000 00000000 c182bf54 c182bee0 c00089fc c0348894 bee0: c00da51c c1ffcc78 c182bf0c c182bef8 c002d100 c002d09c c1ffcc78 00000000 bf00: c182bf54 c182bf10 c002d308 c0336570 c182bf3c c0334e44 00000003 00000003 bf20: 00000030 c0334b44 c0044d74 00000003 00000003 c034dac8 c0350a94 c0373440 bf40: c0373440 00000030 c182bf94 c182bf58 c0336d24 c000890c 00000003 00000003 bf60: c0336560 c182bf64 c182bf64 6e616e0d 00000000 c0272fc8 00000000 00000000 bf80: 00000000 00000000 c182bfac c182bf98 c0272fd8 c0336bd8 c182a000 00000000 bfa0: 00000000 c182bfb0 c00095d0 c0272fd8 00000000 00000000 00000000 00000000 bfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 bfe0: 00000000 00000000 00000000 00000000 00000013 00000000 374d27cd 33cc33e4 Backtrace: [<c01db8dc>] (ch2_irq) from [<c0045430>] (handle_irq_event_percpu+0x38/0x148) [<c00453f8>] (handle_irq_event_percpu) from [<c0045570>] (handle_irq_event+0x30/0x40) r10:00000000 r9:c1825b34 r8:60000053 r7:c182be2c r6:00000000 r5:c035cec4 r4:c1825b00 [<c0045540>] (handle_irq_event) from [<c0047f34>] (handle_fasteoi_irq+0xac/0x11c) r4:c1825b00 r3:00000000 [<c0047e88>] (handle_fasteoi_irq) from [<c0044da4>] (generic_handle_irq+0x28/0x38) r5:c036619c r4:00000013 [<c0044d7c>] (generic_handle_irq) from [<c0009e34>] (handle_IRQ+0x68/0x88) r4:00000013 r3:0000007f [<c0009dcc>] (handle_IRQ) from [<c00086a8>] (at91_aic_handle_irq+0x44/0x4c) r6:ffffffff r5:60000053 r4:c0046728 r3:fefff000 [<c0008664>] (at91_aic_handle_irq) from [<c000cb80>] (__irq_svc+0x40/0x4c) Exception stack(0xc182bdf8 to 0xc182be40) bde0: 00000000 00000013 be00: 00000000 00014200 c1825b00 c036e800 00000013 c035ed98 60000053 c1825b34 be20: 00000000 c182be74 c182be20 c182be40 c0047994 c0046728 60000053 ffffffff [<c0046388>] (__setup_irq) from [<c004689c>] (setup_irq+0x60/0x8c) r10:c036e700 r9:00000004 r8:c03874bc r7:c035ed98 r6:c036e800 r5:00000013 r4:c1825b00 [<c004683c>] (setup_irq) from [<c0348b08>] (tcb_clksrc_init+0x284/0x31c) r6:c035ed10 r5:00000000 r4:c18c6080 r3:c036e760 [<c0348884>] (tcb_clksrc_init) from [<c00089fc>] (do_one_initcall+0x100/0x1b4) r10:00000000 r9:00000000 r8:c182a020 r7:c035b9c0 r6:c0348884 r5:c035b9c0 r4:c18e67e0 [<c00088fc>] (do_one_initcall) from [<c0336d24>] (kernel_init_freeable+0x15c/0x224) r9:00000030 r8:c0373440 r7:c0373440 r6:c0350a94 r5:c034dac8 r4:00000003 [<c0336bc8>] (kernel_init_freeable) from [<c0272fd8>] (kernel_init+0x10/0xec) r9:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c0272fc8 r4:00000000 [<c0272fc8>] (kernel_init) from [<c00095d0>] (ret_from_fork+0x14/0x24) r4:00000000 r3:c182a000 Code: bad PC value ---[ end trace 5b30f0017e282e47 ]--- Kernel panic - not syncing: Fatal exception in interrupt Your sincerly, Gaël PORTAY Gaël PORTAY (3): ARM: at91/tclib: prefer using of devm_* functions ARM: at91/tclib: move initialization from alloc to probe ARM: at91/tclib: mask interruptions at shutdown and probe drivers/clocksource/tcb_clksrc.c | 2 +- drivers/misc/atmel_tclib.c | 101 +++++++++++++++++---------------------- drivers/pwm/pwm-atmel-tcb.c | 2 +- include/linux/atmel_tc.h | 8 ++-- 4 files changed, 51 insertions(+), 62 deletions(-) -- 1.9.3 ^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 0/3] ARM: at91/tclib: fix segmentation fault @ 2014-08-19 22:07 ` Gaël PORTAY 0 siblings, 0 replies; 34+ messages in thread From: Gaël PORTAY @ 2014-08-19 22:07 UTC (permalink / raw) To: linux-arm-kernel Hi every one, This set of patches fix a segmentation fault happening when kexec-ing kernel on an at91 platform (see backtrace below). While the previous kernel shuts down, the tcb_clksrc driver leaves its interruptions unmasked. When the new kernel initiliazes any tclib making use of a TC block, an interruption may happen before the interrupt handler is set, causing a kernel segmentation fault. To prevent from such cases from happening, the last patch sets up the shutdown callback which masks interruptions when the machine is shutdown. Furthermore, it also masks the interruptions at probe to make sure no interruption happens before the handler is set. This will prevent freshly kexec-ed kernel from crashing when launched from a kernel which does not properly mask interruptions at shutdown. Unable to handle kernel NULL pointer dereference at virtual address 00000000 pgd = c0004000 [00000000] *pgd=00000000 Internal error: Oops: 80000005 [#1] ARM Modules linked in: CPU: 0 PID: 1 Comm: swapper Not tainted 3.16.0+ #144 task: c1828aa0 ti: c182a000 task.ti: c182a000 PC is at 0x0 LR is at ch2_irq+0x28/0x30 pc : [<00000000>] lr : [<c01db904>] psr: 000000d3 sp : c182bd38 ip : c182bd48 fp : c182bd44 r10: c0373390 r9 : c1825b00 r8 : 60000053 r7 : 00000000 r6 : 00000000 r5 : 00000013 r4 : c036e800 r3 : 00000000 r2 : 00002004 r1 : c036e760 r0 : c036e760 Flags: nzcv IRQs off FIQs off Mode SVC_32 ISA ARM Segment kernel Control: 0005317f Table: 20004000 DAC: 00000017 Process swapper (pid: 1, stack limit = 0xc182a1c0) Stack: (0xc182bd38 to 0xc182c000) bd20: c182bd7c c182bd48 bd40: c0045430 c01db8ec 00000000 c18c6f40 c182bd74 c1825b00 c035cec4 00000000 bd60: c182be2c 60000053 c1825b34 00000000 c182bd94 c182bd80 c0045570 c0045408 bd80: 00000000 c1825b00 c182bdac c182bd98 c0047f34 c0045550 00000013 c036619c bda0: c182bdc4 c182bdb0 c0044da4 c0047e98 0000007f 00000013 c182bde4 c182bdc8 bdc0: c0009e34 c0044d8c fefff000 c0046728 60000053 ffffffff c182bdf4 c182bde8 bde0: c00086a8 c0009ddc c182be74 c182bdf8 c000cb80 c0008674 00000000 00000013 be00: 00000000 00014200 c1825b00 c036e800 00000013 c035ed98 60000053 c1825b34 be20: 00000000 c182be74 c182be20 c182be40 c0047994 c0046728 60000053 ffffffff be40: 00000013 c036e800 c182be64 c1825b00 00000013 c036e800 c035ed98 c03874bc be60: 00000004 c036e700 c182be94 c182be78 c004689c c0046398 c036e760 c18c6080 be80: 00000000 c035ed10 c182bedc c182be98 c0348b08 c004684c 0000000c c034dac8 bea0: 004c4b3f c028c338 c036e760 00000013 c014ecc8 c18e67e0 c035b9c0 c0348884 bec0: c035b9c0 c182a020 00000000 00000000 c182bf54 c182bee0 c00089fc c0348894 bee0: c00da51c c1ffcc78 c182bf0c c182bef8 c002d100 c002d09c c1ffcc78 00000000 bf00: c182bf54 c182bf10 c002d308 c0336570 c182bf3c c0334e44 00000003 00000003 bf20: 00000030 c0334b44 c0044d74 00000003 00000003 c034dac8 c0350a94 c0373440 bf40: c0373440 00000030 c182bf94 c182bf58 c0336d24 c000890c 00000003 00000003 bf60: c0336560 c182bf64 c182bf64 6e616e0d 00000000 c0272fc8 00000000 00000000 bf80: 00000000 00000000 c182bfac c182bf98 c0272fd8 c0336bd8 c182a000 00000000 bfa0: 00000000 c182bfb0 c00095d0 c0272fd8 00000000 00000000 00000000 00000000 bfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 bfe0: 00000000 00000000 00000000 00000000 00000013 00000000 374d27cd 33cc33e4 Backtrace: [<c01db8dc>] (ch2_irq) from [<c0045430>] (handle_irq_event_percpu+0x38/0x148) [<c00453f8>] (handle_irq_event_percpu) from [<c0045570>] (handle_irq_event+0x30/0x40) r10:00000000 r9:c1825b34 r8:60000053 r7:c182be2c r6:00000000 r5:c035cec4 r4:c1825b00 [<c0045540>] (handle_irq_event) from [<c0047f34>] (handle_fasteoi_irq+0xac/0x11c) r4:c1825b00 r3:00000000 [<c0047e88>] (handle_fasteoi_irq) from [<c0044da4>] (generic_handle_irq+0x28/0x38) r5:c036619c r4:00000013 [<c0044d7c>] (generic_handle_irq) from [<c0009e34>] (handle_IRQ+0x68/0x88) r4:00000013 r3:0000007f [<c0009dcc>] (handle_IRQ) from [<c00086a8>] (at91_aic_handle_irq+0x44/0x4c) r6:ffffffff r5:60000053 r4:c0046728 r3:fefff000 [<c0008664>] (at91_aic_handle_irq) from [<c000cb80>] (__irq_svc+0x40/0x4c) Exception stack(0xc182bdf8 to 0xc182be40) bde0: 00000000 00000013 be00: 00000000 00014200 c1825b00 c036e800 00000013 c035ed98 60000053 c1825b34 be20: 00000000 c182be74 c182be20 c182be40 c0047994 c0046728 60000053 ffffffff [<c0046388>] (__setup_irq) from [<c004689c>] (setup_irq+0x60/0x8c) r10:c036e700 r9:00000004 r8:c03874bc r7:c035ed98 r6:c036e800 r5:00000013 r4:c1825b00 [<c004683c>] (setup_irq) from [<c0348b08>] (tcb_clksrc_init+0x284/0x31c) r6:c035ed10 r5:00000000 r4:c18c6080 r3:c036e760 [<c0348884>] (tcb_clksrc_init) from [<c00089fc>] (do_one_initcall+0x100/0x1b4) r10:00000000 r9:00000000 r8:c182a020 r7:c035b9c0 r6:c0348884 r5:c035b9c0 r4:c18e67e0 [<c00088fc>] (do_one_initcall) from [<c0336d24>] (kernel_init_freeable+0x15c/0x224) r9:00000030 r8:c0373440 r7:c0373440 r6:c0350a94 r5:c034dac8 r4:00000003 [<c0336bc8>] (kernel_init_freeable) from [<c0272fd8>] (kernel_init+0x10/0xec) r9:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c0272fc8 r4:00000000 [<c0272fc8>] (kernel_init) from [<c00095d0>] (ret_from_fork+0x14/0x24) r4:00000000 r3:c182a000 Code: bad PC value ---[ end trace 5b30f0017e282e47 ]--- Kernel panic - not syncing: Fatal exception in interrupt Your sincerly, Ga?l PORTAY Ga?l PORTAY (3): ARM: at91/tclib: prefer using of devm_* functions ARM: at91/tclib: move initialization from alloc to probe ARM: at91/tclib: mask interruptions at shutdown and probe drivers/clocksource/tcb_clksrc.c | 2 +- drivers/misc/atmel_tclib.c | 101 +++++++++++++++++---------------------- drivers/pwm/pwm-atmel-tcb.c | 2 +- include/linux/atmel_tc.h | 8 ++-- 4 files changed, 51 insertions(+), 62 deletions(-) -- 1.9.3 ^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 1/3] ARM: at91/tclib: prefer using of devm_* functions 2014-08-19 22:07 ` Gaël PORTAY @ 2014-08-19 22:07 ` Gaël PORTAY -1 siblings, 0 replies; 34+ messages in thread From: Gaël PORTAY @ 2014-08-19 22:07 UTC (permalink / raw) To: Arnd Bergmann, Daniel Lezcano, Greg Kroah-Hartman, linux-arm-kernel, linux-kernel, linux-pwm, Nicolas Ferre, Thierry Reding, Thomas Gleixner Cc: Boris Brezillon, Alexandre Belloni, Jean-Christophe PLAGNIOL-VILLARD, Gaël PORTAY Signed-off-by: Gaël PORTAY <gael.portay@gmail.com> --- drivers/misc/atmel_tclib.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/drivers/misc/atmel_tclib.c b/drivers/misc/atmel_tclib.c index c8d8e38..b514a2d 100644 --- a/drivers/misc/atmel_tclib.c +++ b/drivers/misc/atmel_tclib.c @@ -150,17 +150,15 @@ static int __init tc_probe(struct platform_device *pdev) if (irq < 0) return -EINVAL; - tc = kzalloc(sizeof(struct atmel_tc), GFP_KERNEL); + tc = devm_kzalloc(&pdev->dev, sizeof(struct atmel_tc), GFP_KERNEL); if (!tc) return -ENOMEM; tc->pdev = pdev; - clk = clk_get(&pdev->dev, "t0_clk"); - if (IS_ERR(clk)) { - kfree(tc); - return -EINVAL; - } + clk = devm_clk_get(&pdev->dev, "t0_clk"); + if (IS_ERR(clk)) + return PTR_ERR(clk); /* Now take SoC information if available */ if (pdev->dev.of_node) { @@ -171,10 +169,10 @@ static int __init tc_probe(struct platform_device *pdev) } tc->clk[0] = clk; - tc->clk[1] = clk_get(&pdev->dev, "t1_clk"); + tc->clk[1] = devm_clk_get(&pdev->dev, "t1_clk"); if (IS_ERR(tc->clk[1])) tc->clk[1] = clk; - tc->clk[2] = clk_get(&pdev->dev, "t2_clk"); + tc->clk[2] = devm_clk_get(&pdev->dev, "t2_clk"); if (IS_ERR(tc->clk[2])) tc->clk[2] = clk; -- 1.9.3 ^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH 1/3] ARM: at91/tclib: prefer using of devm_* functions @ 2014-08-19 22:07 ` Gaël PORTAY 0 siblings, 0 replies; 34+ messages in thread From: Gaël PORTAY @ 2014-08-19 22:07 UTC (permalink / raw) To: linux-arm-kernel Signed-off-by: Ga?l PORTAY <gael.portay@gmail.com> --- drivers/misc/atmel_tclib.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/drivers/misc/atmel_tclib.c b/drivers/misc/atmel_tclib.c index c8d8e38..b514a2d 100644 --- a/drivers/misc/atmel_tclib.c +++ b/drivers/misc/atmel_tclib.c @@ -150,17 +150,15 @@ static int __init tc_probe(struct platform_device *pdev) if (irq < 0) return -EINVAL; - tc = kzalloc(sizeof(struct atmel_tc), GFP_KERNEL); + tc = devm_kzalloc(&pdev->dev, sizeof(struct atmel_tc), GFP_KERNEL); if (!tc) return -ENOMEM; tc->pdev = pdev; - clk = clk_get(&pdev->dev, "t0_clk"); - if (IS_ERR(clk)) { - kfree(tc); - return -EINVAL; - } + clk = devm_clk_get(&pdev->dev, "t0_clk"); + if (IS_ERR(clk)) + return PTR_ERR(clk); /* Now take SoC information if available */ if (pdev->dev.of_node) { @@ -171,10 +169,10 @@ static int __init tc_probe(struct platform_device *pdev) } tc->clk[0] = clk; - tc->clk[1] = clk_get(&pdev->dev, "t1_clk"); + tc->clk[1] = devm_clk_get(&pdev->dev, "t1_clk"); if (IS_ERR(tc->clk[1])) tc->clk[1] = clk; - tc->clk[2] = clk_get(&pdev->dev, "t2_clk"); + tc->clk[2] = devm_clk_get(&pdev->dev, "t2_clk"); if (IS_ERR(tc->clk[2])) tc->clk[2] = clk; -- 1.9.3 ^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH 1/3] ARM: at91/tclib: prefer using of devm_* functions 2014-08-19 22:07 ` Gaël PORTAY @ 2014-08-20 8:19 ` Boris BREZILLON -1 siblings, 0 replies; 34+ messages in thread From: Boris BREZILLON @ 2014-08-20 8:19 UTC (permalink / raw) To: Gaël PORTAY Cc: Arnd Bergmann, Daniel Lezcano, Greg Kroah-Hartman, linux-arm-kernel, linux-kernel, linux-pwm, Nicolas Ferre, Thierry Reding, Thomas Gleixner, Alexandre Belloni, Jean-Christophe PLAGNIOL-VILLARD On Wed, 20 Aug 2014 00:07:50 +0200 Gaël PORTAY <gael.portay@gmail.com> wrote: > Signed-off-by: Gaël PORTAY <gael.portay@gmail.com> Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com> > --- > drivers/misc/atmel_tclib.c | 14 ++++++-------- > 1 file changed, 6 insertions(+), 8 deletions(-) > > diff --git a/drivers/misc/atmel_tclib.c b/drivers/misc/atmel_tclib.c > index c8d8e38..b514a2d 100644 > --- a/drivers/misc/atmel_tclib.c > +++ b/drivers/misc/atmel_tclib.c > @@ -150,17 +150,15 @@ static int __init tc_probe(struct platform_device *pdev) > if (irq < 0) > return -EINVAL; > > - tc = kzalloc(sizeof(struct atmel_tc), GFP_KERNEL); > + tc = devm_kzalloc(&pdev->dev, sizeof(struct atmel_tc), GFP_KERNEL); > if (!tc) > return -ENOMEM; > > tc->pdev = pdev; > > - clk = clk_get(&pdev->dev, "t0_clk"); > - if (IS_ERR(clk)) { > - kfree(tc); > - return -EINVAL; > - } > + clk = devm_clk_get(&pdev->dev, "t0_clk"); > + if (IS_ERR(clk)) > + return PTR_ERR(clk); > > /* Now take SoC information if available */ > if (pdev->dev.of_node) { > @@ -171,10 +169,10 @@ static int __init tc_probe(struct platform_device *pdev) > } > > tc->clk[0] = clk; > - tc->clk[1] = clk_get(&pdev->dev, "t1_clk"); > + tc->clk[1] = devm_clk_get(&pdev->dev, "t1_clk"); > if (IS_ERR(tc->clk[1])) > tc->clk[1] = clk; > - tc->clk[2] = clk_get(&pdev->dev, "t2_clk"); > + tc->clk[2] = devm_clk_get(&pdev->dev, "t2_clk"); > if (IS_ERR(tc->clk[2])) > tc->clk[2] = clk; > -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com ^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 1/3] ARM: at91/tclib: prefer using of devm_* functions @ 2014-08-20 8:19 ` Boris BREZILLON 0 siblings, 0 replies; 34+ messages in thread From: Boris BREZILLON @ 2014-08-20 8:19 UTC (permalink / raw) To: linux-arm-kernel On Wed, 20 Aug 2014 00:07:50 +0200 Ga?l PORTAY <gael.portay@gmail.com> wrote: > Signed-off-by: Ga?l PORTAY <gael.portay@gmail.com> Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com> > --- > drivers/misc/atmel_tclib.c | 14 ++++++-------- > 1 file changed, 6 insertions(+), 8 deletions(-) > > diff --git a/drivers/misc/atmel_tclib.c b/drivers/misc/atmel_tclib.c > index c8d8e38..b514a2d 100644 > --- a/drivers/misc/atmel_tclib.c > +++ b/drivers/misc/atmel_tclib.c > @@ -150,17 +150,15 @@ static int __init tc_probe(struct platform_device *pdev) > if (irq < 0) > return -EINVAL; > > - tc = kzalloc(sizeof(struct atmel_tc), GFP_KERNEL); > + tc = devm_kzalloc(&pdev->dev, sizeof(struct atmel_tc), GFP_KERNEL); > if (!tc) > return -ENOMEM; > > tc->pdev = pdev; > > - clk = clk_get(&pdev->dev, "t0_clk"); > - if (IS_ERR(clk)) { > - kfree(tc); > - return -EINVAL; > - } > + clk = devm_clk_get(&pdev->dev, "t0_clk"); > + if (IS_ERR(clk)) > + return PTR_ERR(clk); > > /* Now take SoC information if available */ > if (pdev->dev.of_node) { > @@ -171,10 +169,10 @@ static int __init tc_probe(struct platform_device *pdev) > } > > tc->clk[0] = clk; > - tc->clk[1] = clk_get(&pdev->dev, "t1_clk"); > + tc->clk[1] = devm_clk_get(&pdev->dev, "t1_clk"); > if (IS_ERR(tc->clk[1])) > tc->clk[1] = clk; > - tc->clk[2] = clk_get(&pdev->dev, "t2_clk"); > + tc->clk[2] = devm_clk_get(&pdev->dev, "t2_clk"); > if (IS_ERR(tc->clk[2])) > tc->clk[2] = clk; > -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com ^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 2/3] ARM: at91/tclib: move initialization from alloc to probe 2014-08-19 22:07 ` Gaël PORTAY @ 2014-08-19 22:07 ` Gaël PORTAY -1 siblings, 0 replies; 34+ messages in thread From: Gaël PORTAY @ 2014-08-19 22:07 UTC (permalink / raw) To: Arnd Bergmann, Daniel Lezcano, Greg Kroah-Hartman, linux-arm-kernel, linux-kernel, linux-pwm, Nicolas Ferre, Thierry Reding, Thomas Gleixner Cc: Boris Brezillon, Alexandre Belloni, Jean-Christophe PLAGNIOL-VILLARD, Gaël PORTAY Move resource retrieval from atmel_tc_alloc to tc_probe to avoid lately reporting resource related issues when a TC block user request a TC block. Moreover, resources retrieval are usually done in the probe function, thus moving them add some consistency with other drivers. Initialization is done once, ie not every time a tc block is requested. If it fails, the device is not appended to the list of tc blocks. Furhermore, the device id is retrieved at probe as well, avoiding parsing DT every time the user requests of tc block. Signed-off-by: Gaël PORTAY <gael.portay@gmail.com> --- drivers/clocksource/tcb_clksrc.c | 2 +- drivers/misc/atmel_tclib.c | 71 +++++++++++++--------------------------- drivers/pwm/pwm-atmel-tcb.c | 2 +- include/linux/atmel_tc.h | 8 +++-- 4 files changed, 29 insertions(+), 54 deletions(-) diff --git a/drivers/clocksource/tcb_clksrc.c b/drivers/clocksource/tcb_clksrc.c index a8d7ea1..f922e81 100644 --- a/drivers/clocksource/tcb_clksrc.c +++ b/drivers/clocksource/tcb_clksrc.c @@ -279,7 +279,7 @@ static int __init tcb_clksrc_init(void) int i; int ret; - tc = atmel_tc_alloc(CONFIG_ATMEL_TCB_CLKSRC_BLOCK, clksrc.name); + tc = atmel_tc_alloc(CONFIG_ATMEL_TCB_CLKSRC_BLOCK); if (!tc) { pr_debug("can't alloc TC for clocksource\n"); return -ENODEV; diff --git a/drivers/misc/atmel_tclib.c b/drivers/misc/atmel_tclib.c index b514a2d..d505d1e 100644 --- a/drivers/misc/atmel_tclib.c +++ b/drivers/misc/atmel_tclib.c @@ -35,60 +35,31 @@ static LIST_HEAD(tc_list); /** * atmel_tc_alloc - allocate a specified TC block * @block: which block to allocate - * @name: name to be associated with the iomem resource * * Caller allocates a block. If it is available, a pointer to a * pre-initialized struct atmel_tc is returned. The caller can access * the registers directly through the "regs" field. */ -struct atmel_tc *atmel_tc_alloc(unsigned block, const char *name) +struct atmel_tc *atmel_tc_alloc(unsigned block) { struct atmel_tc *tc; struct platform_device *pdev = NULL; - struct resource *r; - size_t size; spin_lock(&tc_list_lock); list_for_each_entry(tc, &tc_list, node) { - if (tc->pdev->dev.of_node) { - if (of_alias_get_id(tc->pdev->dev.of_node, "tcb") - == block) { - pdev = tc->pdev; - break; - } - } else if (tc->pdev->id == block) { + if (tc->allocated) + continue; + + if ((tc->pdev->dev.of_node && tc->id == block) || + (tc->pdev->id == block)) { pdev = tc->pdev; + tc->allocated = true; break; } } - - if (!pdev || tc->iomem) - goto fail; - - r = platform_get_resource(pdev, IORESOURCE_MEM, 0); - if (!r) - goto fail; - - size = resource_size(r); - r = request_mem_region(r->start, size, name); - if (!r) - goto fail; - - tc->regs = ioremap(r->start, size); - if (!tc->regs) - goto fail_ioremap; - - tc->iomem = r; - -out: spin_unlock(&tc_list_lock); - return tc; -fail_ioremap: - release_mem_region(r->start, size); -fail: - tc = NULL; - goto out; + return pdev ? tc : NULL; } EXPORT_SYMBOL_GPL(atmel_tc_alloc); @@ -96,19 +67,14 @@ EXPORT_SYMBOL_GPL(atmel_tc_alloc); * atmel_tc_free - release a specified TC block * @tc: Timer/counter block that was returned by atmel_tc_alloc() * - * This reverses the effect of atmel_tc_alloc(), unmapping the I/O - * registers, invalidating the resource returned by that routine and - * making the TC available to other drivers. + * This reverses the effect of atmel_tc_alloc(), invalidating the resource + * returned by that routine and making the TC available to other drivers. */ void atmel_tc_free(struct atmel_tc *tc) { spin_lock(&tc_list_lock); - if (tc->regs) { - iounmap(tc->regs); - release_mem_region(tc->iomem->start, resource_size(tc->iomem)); - tc->regs = NULL; - tc->iomem = NULL; - } + if (tc->allocated) + tc->allocated = false; spin_unlock(&tc_list_lock); } EXPORT_SYMBOL_GPL(atmel_tc_free); @@ -142,9 +108,7 @@ static int __init tc_probe(struct platform_device *pdev) struct atmel_tc *tc; struct clk *clk; int irq; - - if (!platform_get_resource(pdev, IORESOURCE_MEM, 0)) - return -EINVAL; + struct resource *r; irq = platform_get_irq(pdev, 0); if (irq < 0) @@ -160,12 +124,21 @@ static int __init tc_probe(struct platform_device *pdev) if (IS_ERR(clk)) return PTR_ERR(clk); + r = platform_get_resource(pdev, IORESOURCE_MEM, 0); + tc->regs = devm_ioremap_resource(&pdev->dev, r); + if (IS_ERR(tc->regs)) + return PTR_ERR(tc->regs); + /* Now take SoC information if available */ if (pdev->dev.of_node) { const struct of_device_id *match; match = of_match_node(atmel_tcb_dt_ids, pdev->dev.of_node); if (match) tc->tcb_config = match->data; + + tc->id = of_alias_get_id(tc->pdev->dev.of_node, "tcb"); + } else { + tc->id = pdev->id; } tc->clk[0] = clk; diff --git a/drivers/pwm/pwm-atmel-tcb.c b/drivers/pwm/pwm-atmel-tcb.c index f3dcd02..d56e5b7 100644 --- a/drivers/pwm/pwm-atmel-tcb.c +++ b/drivers/pwm/pwm-atmel-tcb.c @@ -379,7 +379,7 @@ static int atmel_tcb_pwm_probe(struct platform_device *pdev) return err; } - tc = atmel_tc_alloc(tcblock, "tcb-pwm"); + tc = atmel_tc_alloc(tcblock); if (tc == NULL) { dev_err(&pdev->dev, "failed to allocate Timer Counter Block\n"); return -ENOMEM; diff --git a/include/linux/atmel_tc.h b/include/linux/atmel_tc.h index 89a931b..d8aa884 100644 --- a/include/linux/atmel_tc.h +++ b/include/linux/atmel_tc.h @@ -44,12 +44,13 @@ struct atmel_tcb_config { /** * struct atmel_tc - information about a Timer/Counter Block * @pdev: physical device - * @iomem: resource associated with the I/O register * @regs: mapping through which the I/O registers can be accessed + * @id: block id * @tcb_config: configuration data from SoC * @irq: irq for each of the three channels * @clk: internal clock source for each of the three channels * @node: list node, for tclib internal use + * @allocated: if already used, for tclib internal use * * On some platforms, each TC channel has its own clocks and IRQs, * while on others, all TC channels share the same clock and IRQ. @@ -61,15 +62,16 @@ struct atmel_tcb_config { */ struct atmel_tc { struct platform_device *pdev; - struct resource *iomem; void __iomem *regs; + int id; const struct atmel_tcb_config *tcb_config; int irq[3]; struct clk *clk[3]; struct list_head node; + bool allocated; }; -extern struct atmel_tc *atmel_tc_alloc(unsigned block, const char *name); +extern struct atmel_tc *atmel_tc_alloc(unsigned block); extern void atmel_tc_free(struct atmel_tc *tc); /* platform-specific ATMEL_TC_TIMER_CLOCKx divisors (0 means 32KiHz) */ -- 1.9.3 ^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH 2/3] ARM: at91/tclib: move initialization from alloc to probe @ 2014-08-19 22:07 ` Gaël PORTAY 0 siblings, 0 replies; 34+ messages in thread From: Gaël PORTAY @ 2014-08-19 22:07 UTC (permalink / raw) To: linux-arm-kernel Move resource retrieval from atmel_tc_alloc to tc_probe to avoid lately reporting resource related issues when a TC block user request a TC block. Moreover, resources retrieval are usually done in the probe function, thus moving them add some consistency with other drivers. Initialization is done once, ie not every time a tc block is requested. If it fails, the device is not appended to the list of tc blocks. Furhermore, the device id is retrieved at probe as well, avoiding parsing DT every time the user requests of tc block. Signed-off-by: Ga?l PORTAY <gael.portay@gmail.com> --- drivers/clocksource/tcb_clksrc.c | 2 +- drivers/misc/atmel_tclib.c | 71 +++++++++++++--------------------------- drivers/pwm/pwm-atmel-tcb.c | 2 +- include/linux/atmel_tc.h | 8 +++-- 4 files changed, 29 insertions(+), 54 deletions(-) diff --git a/drivers/clocksource/tcb_clksrc.c b/drivers/clocksource/tcb_clksrc.c index a8d7ea1..f922e81 100644 --- a/drivers/clocksource/tcb_clksrc.c +++ b/drivers/clocksource/tcb_clksrc.c @@ -279,7 +279,7 @@ static int __init tcb_clksrc_init(void) int i; int ret; - tc = atmel_tc_alloc(CONFIG_ATMEL_TCB_CLKSRC_BLOCK, clksrc.name); + tc = atmel_tc_alloc(CONFIG_ATMEL_TCB_CLKSRC_BLOCK); if (!tc) { pr_debug("can't alloc TC for clocksource\n"); return -ENODEV; diff --git a/drivers/misc/atmel_tclib.c b/drivers/misc/atmel_tclib.c index b514a2d..d505d1e 100644 --- a/drivers/misc/atmel_tclib.c +++ b/drivers/misc/atmel_tclib.c @@ -35,60 +35,31 @@ static LIST_HEAD(tc_list); /** * atmel_tc_alloc - allocate a specified TC block * @block: which block to allocate - * @name: name to be associated with the iomem resource * * Caller allocates a block. If it is available, a pointer to a * pre-initialized struct atmel_tc is returned. The caller can access * the registers directly through the "regs" field. */ -struct atmel_tc *atmel_tc_alloc(unsigned block, const char *name) +struct atmel_tc *atmel_tc_alloc(unsigned block) { struct atmel_tc *tc; struct platform_device *pdev = NULL; - struct resource *r; - size_t size; spin_lock(&tc_list_lock); list_for_each_entry(tc, &tc_list, node) { - if (tc->pdev->dev.of_node) { - if (of_alias_get_id(tc->pdev->dev.of_node, "tcb") - == block) { - pdev = tc->pdev; - break; - } - } else if (tc->pdev->id == block) { + if (tc->allocated) + continue; + + if ((tc->pdev->dev.of_node && tc->id == block) || + (tc->pdev->id == block)) { pdev = tc->pdev; + tc->allocated = true; break; } } - - if (!pdev || tc->iomem) - goto fail; - - r = platform_get_resource(pdev, IORESOURCE_MEM, 0); - if (!r) - goto fail; - - size = resource_size(r); - r = request_mem_region(r->start, size, name); - if (!r) - goto fail; - - tc->regs = ioremap(r->start, size); - if (!tc->regs) - goto fail_ioremap; - - tc->iomem = r; - -out: spin_unlock(&tc_list_lock); - return tc; -fail_ioremap: - release_mem_region(r->start, size); -fail: - tc = NULL; - goto out; + return pdev ? tc : NULL; } EXPORT_SYMBOL_GPL(atmel_tc_alloc); @@ -96,19 +67,14 @@ EXPORT_SYMBOL_GPL(atmel_tc_alloc); * atmel_tc_free - release a specified TC block * @tc: Timer/counter block that was returned by atmel_tc_alloc() * - * This reverses the effect of atmel_tc_alloc(), unmapping the I/O - * registers, invalidating the resource returned by that routine and - * making the TC available to other drivers. + * This reverses the effect of atmel_tc_alloc(), invalidating the resource + * returned by that routine and making the TC available to other drivers. */ void atmel_tc_free(struct atmel_tc *tc) { spin_lock(&tc_list_lock); - if (tc->regs) { - iounmap(tc->regs); - release_mem_region(tc->iomem->start, resource_size(tc->iomem)); - tc->regs = NULL; - tc->iomem = NULL; - } + if (tc->allocated) + tc->allocated = false; spin_unlock(&tc_list_lock); } EXPORT_SYMBOL_GPL(atmel_tc_free); @@ -142,9 +108,7 @@ static int __init tc_probe(struct platform_device *pdev) struct atmel_tc *tc; struct clk *clk; int irq; - - if (!platform_get_resource(pdev, IORESOURCE_MEM, 0)) - return -EINVAL; + struct resource *r; irq = platform_get_irq(pdev, 0); if (irq < 0) @@ -160,12 +124,21 @@ static int __init tc_probe(struct platform_device *pdev) if (IS_ERR(clk)) return PTR_ERR(clk); + r = platform_get_resource(pdev, IORESOURCE_MEM, 0); + tc->regs = devm_ioremap_resource(&pdev->dev, r); + if (IS_ERR(tc->regs)) + return PTR_ERR(tc->regs); + /* Now take SoC information if available */ if (pdev->dev.of_node) { const struct of_device_id *match; match = of_match_node(atmel_tcb_dt_ids, pdev->dev.of_node); if (match) tc->tcb_config = match->data; + + tc->id = of_alias_get_id(tc->pdev->dev.of_node, "tcb"); + } else { + tc->id = pdev->id; } tc->clk[0] = clk; diff --git a/drivers/pwm/pwm-atmel-tcb.c b/drivers/pwm/pwm-atmel-tcb.c index f3dcd02..d56e5b7 100644 --- a/drivers/pwm/pwm-atmel-tcb.c +++ b/drivers/pwm/pwm-atmel-tcb.c @@ -379,7 +379,7 @@ static int atmel_tcb_pwm_probe(struct platform_device *pdev) return err; } - tc = atmel_tc_alloc(tcblock, "tcb-pwm"); + tc = atmel_tc_alloc(tcblock); if (tc == NULL) { dev_err(&pdev->dev, "failed to allocate Timer Counter Block\n"); return -ENOMEM; diff --git a/include/linux/atmel_tc.h b/include/linux/atmel_tc.h index 89a931b..d8aa884 100644 --- a/include/linux/atmel_tc.h +++ b/include/linux/atmel_tc.h @@ -44,12 +44,13 @@ struct atmel_tcb_config { /** * struct atmel_tc - information about a Timer/Counter Block * @pdev: physical device - * @iomem: resource associated with the I/O register * @regs: mapping through which the I/O registers can be accessed + * @id: block id * @tcb_config: configuration data from SoC * @irq: irq for each of the three channels * @clk: internal clock source for each of the three channels * @node: list node, for tclib internal use + * @allocated: if already used, for tclib internal use * * On some platforms, each TC channel has its own clocks and IRQs, * while on others, all TC channels share the same clock and IRQ. @@ -61,15 +62,16 @@ struct atmel_tcb_config { */ struct atmel_tc { struct platform_device *pdev; - struct resource *iomem; void __iomem *regs; + int id; const struct atmel_tcb_config *tcb_config; int irq[3]; struct clk *clk[3]; struct list_head node; + bool allocated; }; -extern struct atmel_tc *atmel_tc_alloc(unsigned block, const char *name); +extern struct atmel_tc *atmel_tc_alloc(unsigned block); extern void atmel_tc_free(struct atmel_tc *tc); /* platform-specific ATMEL_TC_TIMER_CLOCKx divisors (0 means 32KiHz) */ -- 1.9.3 ^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH 2/3] ARM: at91/tclib: move initialization from alloc to probe 2014-08-19 22:07 ` Gaël PORTAY @ 2014-08-20 7:39 ` Thierry Reding -1 siblings, 0 replies; 34+ messages in thread From: Thierry Reding @ 2014-08-20 7:39 UTC (permalink / raw) To: Gaël PORTAY Cc: Arnd Bergmann, Daniel Lezcano, Greg Kroah-Hartman, linux-arm-kernel, linux-kernel, linux-pwm, Nicolas Ferre, Thomas Gleixner, Boris Brezillon, Alexandre Belloni, Jean-Christophe PLAGNIOL-VILLARD [-- Attachment #1: Type: text/plain, Size: 1056 bytes --] On Wed, Aug 20, 2014 at 12:07:51AM +0200, Gaël PORTAY wrote: > Move resource retrieval from atmel_tc_alloc to tc_probe to avoid lately > reporting resource related issues when a TC block user request a TC block. > > Moreover, resources retrieval are usually done in the probe function, > thus moving them add some consistency with other drivers. > > Initialization is done once, ie not every time a tc block is requested. > If it fails, the device is not appended to the list of tc blocks. > > Furhermore, the device id is retrieved at probe as well, avoiding parsing > DT every time the user requests of tc block. > > Signed-off-by: Gaël PORTAY <gael.portay@gmail.com> > --- > drivers/clocksource/tcb_clksrc.c | 2 +- > drivers/misc/atmel_tclib.c | 71 +++++++++++++--------------------------- > drivers/pwm/pwm-atmel-tcb.c | 2 +- > include/linux/atmel_tc.h | 8 +++-- > 4 files changed, 29 insertions(+), 54 deletions(-) For the PWM driver part: Acked-by: Thierry Reding <thierry.reding@gmail.com> [-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 2/3] ARM: at91/tclib: move initialization from alloc to probe @ 2014-08-20 7:39 ` Thierry Reding 0 siblings, 0 replies; 34+ messages in thread From: Thierry Reding @ 2014-08-20 7:39 UTC (permalink / raw) To: linux-arm-kernel On Wed, Aug 20, 2014 at 12:07:51AM +0200, Ga?l PORTAY wrote: > Move resource retrieval from atmel_tc_alloc to tc_probe to avoid lately > reporting resource related issues when a TC block user request a TC block. > > Moreover, resources retrieval are usually done in the probe function, > thus moving them add some consistency with other drivers. > > Initialization is done once, ie not every time a tc block is requested. > If it fails, the device is not appended to the list of tc blocks. > > Furhermore, the device id is retrieved at probe as well, avoiding parsing > DT every time the user requests of tc block. > > Signed-off-by: Ga?l PORTAY <gael.portay@gmail.com> > --- > drivers/clocksource/tcb_clksrc.c | 2 +- > drivers/misc/atmel_tclib.c | 71 +++++++++++++--------------------------- > drivers/pwm/pwm-atmel-tcb.c | 2 +- > include/linux/atmel_tc.h | 8 +++-- > 4 files changed, 29 insertions(+), 54 deletions(-) For the PWM driver part: Acked-by: Thierry Reding <thierry.reding@gmail.com> -------------- next part -------------- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 819 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140820/16a4665b/attachment.sig> ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 2/3] ARM: at91/tclib: move initialization from alloc to probe 2014-08-19 22:07 ` Gaël PORTAY @ 2014-08-20 8:16 ` Boris BREZILLON -1 siblings, 0 replies; 34+ messages in thread From: Boris BREZILLON @ 2014-08-20 8:16 UTC (permalink / raw) To: Gaël PORTAY Cc: Arnd Bergmann, Daniel Lezcano, Greg Kroah-Hartman, linux-arm-kernel, linux-kernel, linux-pwm, Nicolas Ferre, Thierry Reding, Thomas Gleixner, Alexandre Belloni, Jean-Christophe PLAGNIOL-VILLARD Hi Gael, On Wed, 20 Aug 2014 00:07:51 +0200 Gaël PORTAY <gael.portay@gmail.com> wrote: > Move resource retrieval from atmel_tc_alloc to tc_probe to avoid lately > reporting resource related issues when a TC block user request a TC block. > > Moreover, resources retrieval are usually done in the probe function, > thus moving them add some consistency with other drivers. > > Initialization is done once, ie not every time a tc block is requested. > If it fails, the device is not appended to the list of tc blocks. > > Furhermore, the device id is retrieved at probe as well, avoiding parsing > DT every time the user requests of tc block. > > Signed-off-by: Gaël PORTAY <gael.portay@gmail.com> Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com> > --- > drivers/clocksource/tcb_clksrc.c | 2 +- > drivers/misc/atmel_tclib.c | 71 +++++++++++++--------------------------- > drivers/pwm/pwm-atmel-tcb.c | 2 +- > include/linux/atmel_tc.h | 8 +++-- > 4 files changed, 29 insertions(+), 54 deletions(-) > > diff --git a/drivers/clocksource/tcb_clksrc.c b/drivers/clocksource/tcb_clksrc.c > index a8d7ea1..f922e81 100644 > --- a/drivers/clocksource/tcb_clksrc.c > +++ b/drivers/clocksource/tcb_clksrc.c > @@ -279,7 +279,7 @@ static int __init tcb_clksrc_init(void) > int i; > int ret; > > - tc = atmel_tc_alloc(CONFIG_ATMEL_TCB_CLKSRC_BLOCK, clksrc.name); > + tc = atmel_tc_alloc(CONFIG_ATMEL_TCB_CLKSRC_BLOCK); > if (!tc) { > pr_debug("can't alloc TC for clocksource\n"); > return -ENODEV; > diff --git a/drivers/misc/atmel_tclib.c b/drivers/misc/atmel_tclib.c > index b514a2d..d505d1e 100644 > --- a/drivers/misc/atmel_tclib.c > +++ b/drivers/misc/atmel_tclib.c > @@ -35,60 +35,31 @@ static LIST_HEAD(tc_list); > /** > * atmel_tc_alloc - allocate a specified TC block > * @block: which block to allocate > - * @name: name to be associated with the iomem resource > * > * Caller allocates a block. If it is available, a pointer to a > * pre-initialized struct atmel_tc is returned. The caller can access > * the registers directly through the "regs" field. > */ > -struct atmel_tc *atmel_tc_alloc(unsigned block, const char *name) > +struct atmel_tc *atmel_tc_alloc(unsigned block) > { > struct atmel_tc *tc; > struct platform_device *pdev = NULL; > - struct resource *r; > - size_t size; > > spin_lock(&tc_list_lock); > list_for_each_entry(tc, &tc_list, node) { > - if (tc->pdev->dev.of_node) { > - if (of_alias_get_id(tc->pdev->dev.of_node, "tcb") > - == block) { > - pdev = tc->pdev; > - break; > - } > - } else if (tc->pdev->id == block) { > + if (tc->allocated) > + continue; > + > + if ((tc->pdev->dev.of_node && tc->id == block) || > + (tc->pdev->id == block)) { > pdev = tc->pdev; > + tc->allocated = true; > break; > } > } > - > - if (!pdev || tc->iomem) > - goto fail; > - > - r = platform_get_resource(pdev, IORESOURCE_MEM, 0); > - if (!r) > - goto fail; > - > - size = resource_size(r); > - r = request_mem_region(r->start, size, name); > - if (!r) > - goto fail; > - > - tc->regs = ioremap(r->start, size); > - if (!tc->regs) > - goto fail_ioremap; > - > - tc->iomem = r; > - > -out: > spin_unlock(&tc_list_lock); > - return tc; > > -fail_ioremap: > - release_mem_region(r->start, size); > -fail: > - tc = NULL; > - goto out; > + return pdev ? tc : NULL; > } > EXPORT_SYMBOL_GPL(atmel_tc_alloc); > > @@ -96,19 +67,14 @@ EXPORT_SYMBOL_GPL(atmel_tc_alloc); > * atmel_tc_free - release a specified TC block > * @tc: Timer/counter block that was returned by atmel_tc_alloc() > * > - * This reverses the effect of atmel_tc_alloc(), unmapping the I/O > - * registers, invalidating the resource returned by that routine and > - * making the TC available to other drivers. > + * This reverses the effect of atmel_tc_alloc(), invalidating the resource > + * returned by that routine and making the TC available to other drivers. > */ > void atmel_tc_free(struct atmel_tc *tc) > { > spin_lock(&tc_list_lock); > - if (tc->regs) { > - iounmap(tc->regs); > - release_mem_region(tc->iomem->start, resource_size(tc->iomem)); > - tc->regs = NULL; > - tc->iomem = NULL; > - } > + if (tc->allocated) > + tc->allocated = false; > spin_unlock(&tc_list_lock); > } > EXPORT_SYMBOL_GPL(atmel_tc_free); > @@ -142,9 +108,7 @@ static int __init tc_probe(struct platform_device *pdev) > struct atmel_tc *tc; > struct clk *clk; > int irq; > - > - if (!platform_get_resource(pdev, IORESOURCE_MEM, 0)) > - return -EINVAL; > + struct resource *r; > > irq = platform_get_irq(pdev, 0); > if (irq < 0) > @@ -160,12 +124,21 @@ static int __init tc_probe(struct platform_device *pdev) > if (IS_ERR(clk)) > return PTR_ERR(clk); > > + r = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + tc->regs = devm_ioremap_resource(&pdev->dev, r); > + if (IS_ERR(tc->regs)) > + return PTR_ERR(tc->regs); > + > /* Now take SoC information if available */ > if (pdev->dev.of_node) { > const struct of_device_id *match; > match = of_match_node(atmel_tcb_dt_ids, pdev->dev.of_node); > if (match) > tc->tcb_config = match->data; > + > + tc->id = of_alias_get_id(tc->pdev->dev.of_node, "tcb"); > + } else { > + tc->id = pdev->id; > } > > tc->clk[0] = clk; > diff --git a/drivers/pwm/pwm-atmel-tcb.c b/drivers/pwm/pwm-atmel-tcb.c > index f3dcd02..d56e5b7 100644 > --- a/drivers/pwm/pwm-atmel-tcb.c > +++ b/drivers/pwm/pwm-atmel-tcb.c > @@ -379,7 +379,7 @@ static int atmel_tcb_pwm_probe(struct platform_device *pdev) > return err; > } > > - tc = atmel_tc_alloc(tcblock, "tcb-pwm"); > + tc = atmel_tc_alloc(tcblock); > if (tc == NULL) { > dev_err(&pdev->dev, "failed to allocate Timer Counter Block\n"); > return -ENOMEM; > diff --git a/include/linux/atmel_tc.h b/include/linux/atmel_tc.h > index 89a931b..d8aa884 100644 > --- a/include/linux/atmel_tc.h > +++ b/include/linux/atmel_tc.h > @@ -44,12 +44,13 @@ struct atmel_tcb_config { > /** > * struct atmel_tc - information about a Timer/Counter Block > * @pdev: physical device > - * @iomem: resource associated with the I/O register > * @regs: mapping through which the I/O registers can be accessed > + * @id: block id > * @tcb_config: configuration data from SoC > * @irq: irq for each of the three channels > * @clk: internal clock source for each of the three channels > * @node: list node, for tclib internal use > + * @allocated: if already used, for tclib internal use > * > * On some platforms, each TC channel has its own clocks and IRQs, > * while on others, all TC channels share the same clock and IRQ. > @@ -61,15 +62,16 @@ struct atmel_tcb_config { > */ > struct atmel_tc { > struct platform_device *pdev; > - struct resource *iomem; > void __iomem *regs; > + int id; > const struct atmel_tcb_config *tcb_config; > int irq[3]; > struct clk *clk[3]; > struct list_head node; > + bool allocated; > }; > > -extern struct atmel_tc *atmel_tc_alloc(unsigned block, const char *name); > +extern struct atmel_tc *atmel_tc_alloc(unsigned block); > extern void atmel_tc_free(struct atmel_tc *tc); > > /* platform-specific ATMEL_TC_TIMER_CLOCKx divisors (0 means 32KiHz) */ -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com ^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 2/3] ARM: at91/tclib: move initialization from alloc to probe @ 2014-08-20 8:16 ` Boris BREZILLON 0 siblings, 0 replies; 34+ messages in thread From: Boris BREZILLON @ 2014-08-20 8:16 UTC (permalink / raw) To: linux-arm-kernel Hi Gael, On Wed, 20 Aug 2014 00:07:51 +0200 Ga?l PORTAY <gael.portay@gmail.com> wrote: > Move resource retrieval from atmel_tc_alloc to tc_probe to avoid lately > reporting resource related issues when a TC block user request a TC block. > > Moreover, resources retrieval are usually done in the probe function, > thus moving them add some consistency with other drivers. > > Initialization is done once, ie not every time a tc block is requested. > If it fails, the device is not appended to the list of tc blocks. > > Furhermore, the device id is retrieved at probe as well, avoiding parsing > DT every time the user requests of tc block. > > Signed-off-by: Ga?l PORTAY <gael.portay@gmail.com> Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com> > --- > drivers/clocksource/tcb_clksrc.c | 2 +- > drivers/misc/atmel_tclib.c | 71 +++++++++++++--------------------------- > drivers/pwm/pwm-atmel-tcb.c | 2 +- > include/linux/atmel_tc.h | 8 +++-- > 4 files changed, 29 insertions(+), 54 deletions(-) > > diff --git a/drivers/clocksource/tcb_clksrc.c b/drivers/clocksource/tcb_clksrc.c > index a8d7ea1..f922e81 100644 > --- a/drivers/clocksource/tcb_clksrc.c > +++ b/drivers/clocksource/tcb_clksrc.c > @@ -279,7 +279,7 @@ static int __init tcb_clksrc_init(void) > int i; > int ret; > > - tc = atmel_tc_alloc(CONFIG_ATMEL_TCB_CLKSRC_BLOCK, clksrc.name); > + tc = atmel_tc_alloc(CONFIG_ATMEL_TCB_CLKSRC_BLOCK); > if (!tc) { > pr_debug("can't alloc TC for clocksource\n"); > return -ENODEV; > diff --git a/drivers/misc/atmel_tclib.c b/drivers/misc/atmel_tclib.c > index b514a2d..d505d1e 100644 > --- a/drivers/misc/atmel_tclib.c > +++ b/drivers/misc/atmel_tclib.c > @@ -35,60 +35,31 @@ static LIST_HEAD(tc_list); > /** > * atmel_tc_alloc - allocate a specified TC block > * @block: which block to allocate > - * @name: name to be associated with the iomem resource > * > * Caller allocates a block. If it is available, a pointer to a > * pre-initialized struct atmel_tc is returned. The caller can access > * the registers directly through the "regs" field. > */ > -struct atmel_tc *atmel_tc_alloc(unsigned block, const char *name) > +struct atmel_tc *atmel_tc_alloc(unsigned block) > { > struct atmel_tc *tc; > struct platform_device *pdev = NULL; > - struct resource *r; > - size_t size; > > spin_lock(&tc_list_lock); > list_for_each_entry(tc, &tc_list, node) { > - if (tc->pdev->dev.of_node) { > - if (of_alias_get_id(tc->pdev->dev.of_node, "tcb") > - == block) { > - pdev = tc->pdev; > - break; > - } > - } else if (tc->pdev->id == block) { > + if (tc->allocated) > + continue; > + > + if ((tc->pdev->dev.of_node && tc->id == block) || > + (tc->pdev->id == block)) { > pdev = tc->pdev; > + tc->allocated = true; > break; > } > } > - > - if (!pdev || tc->iomem) > - goto fail; > - > - r = platform_get_resource(pdev, IORESOURCE_MEM, 0); > - if (!r) > - goto fail; > - > - size = resource_size(r); > - r = request_mem_region(r->start, size, name); > - if (!r) > - goto fail; > - > - tc->regs = ioremap(r->start, size); > - if (!tc->regs) > - goto fail_ioremap; > - > - tc->iomem = r; > - > -out: > spin_unlock(&tc_list_lock); > - return tc; > > -fail_ioremap: > - release_mem_region(r->start, size); > -fail: > - tc = NULL; > - goto out; > + return pdev ? tc : NULL; > } > EXPORT_SYMBOL_GPL(atmel_tc_alloc); > > @@ -96,19 +67,14 @@ EXPORT_SYMBOL_GPL(atmel_tc_alloc); > * atmel_tc_free - release a specified TC block > * @tc: Timer/counter block that was returned by atmel_tc_alloc() > * > - * This reverses the effect of atmel_tc_alloc(), unmapping the I/O > - * registers, invalidating the resource returned by that routine and > - * making the TC available to other drivers. > + * This reverses the effect of atmel_tc_alloc(), invalidating the resource > + * returned by that routine and making the TC available to other drivers. > */ > void atmel_tc_free(struct atmel_tc *tc) > { > spin_lock(&tc_list_lock); > - if (tc->regs) { > - iounmap(tc->regs); > - release_mem_region(tc->iomem->start, resource_size(tc->iomem)); > - tc->regs = NULL; > - tc->iomem = NULL; > - } > + if (tc->allocated) > + tc->allocated = false; > spin_unlock(&tc_list_lock); > } > EXPORT_SYMBOL_GPL(atmel_tc_free); > @@ -142,9 +108,7 @@ static int __init tc_probe(struct platform_device *pdev) > struct atmel_tc *tc; > struct clk *clk; > int irq; > - > - if (!platform_get_resource(pdev, IORESOURCE_MEM, 0)) > - return -EINVAL; > + struct resource *r; > > irq = platform_get_irq(pdev, 0); > if (irq < 0) > @@ -160,12 +124,21 @@ static int __init tc_probe(struct platform_device *pdev) > if (IS_ERR(clk)) > return PTR_ERR(clk); > > + r = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + tc->regs = devm_ioremap_resource(&pdev->dev, r); > + if (IS_ERR(tc->regs)) > + return PTR_ERR(tc->regs); > + > /* Now take SoC information if available */ > if (pdev->dev.of_node) { > const struct of_device_id *match; > match = of_match_node(atmel_tcb_dt_ids, pdev->dev.of_node); > if (match) > tc->tcb_config = match->data; > + > + tc->id = of_alias_get_id(tc->pdev->dev.of_node, "tcb"); > + } else { > + tc->id = pdev->id; > } > > tc->clk[0] = clk; > diff --git a/drivers/pwm/pwm-atmel-tcb.c b/drivers/pwm/pwm-atmel-tcb.c > index f3dcd02..d56e5b7 100644 > --- a/drivers/pwm/pwm-atmel-tcb.c > +++ b/drivers/pwm/pwm-atmel-tcb.c > @@ -379,7 +379,7 @@ static int atmel_tcb_pwm_probe(struct platform_device *pdev) > return err; > } > > - tc = atmel_tc_alloc(tcblock, "tcb-pwm"); > + tc = atmel_tc_alloc(tcblock); > if (tc == NULL) { > dev_err(&pdev->dev, "failed to allocate Timer Counter Block\n"); > return -ENOMEM; > diff --git a/include/linux/atmel_tc.h b/include/linux/atmel_tc.h > index 89a931b..d8aa884 100644 > --- a/include/linux/atmel_tc.h > +++ b/include/linux/atmel_tc.h > @@ -44,12 +44,13 @@ struct atmel_tcb_config { > /** > * struct atmel_tc - information about a Timer/Counter Block > * @pdev: physical device > - * @iomem: resource associated with the I/O register > * @regs: mapping through which the I/O registers can be accessed > + * @id: block id > * @tcb_config: configuration data from SoC > * @irq: irq for each of the three channels > * @clk: internal clock source for each of the three channels > * @node: list node, for tclib internal use > + * @allocated: if already used, for tclib internal use > * > * On some platforms, each TC channel has its own clocks and IRQs, > * while on others, all TC channels share the same clock and IRQ. > @@ -61,15 +62,16 @@ struct atmel_tcb_config { > */ > struct atmel_tc { > struct platform_device *pdev; > - struct resource *iomem; > void __iomem *regs; > + int id; > const struct atmel_tcb_config *tcb_config; > int irq[3]; > struct clk *clk[3]; > struct list_head node; > + bool allocated; > }; > > -extern struct atmel_tc *atmel_tc_alloc(unsigned block, const char *name); > +extern struct atmel_tc *atmel_tc_alloc(unsigned block); > extern void atmel_tc_free(struct atmel_tc *tc); > > /* platform-specific ATMEL_TC_TIMER_CLOCKx divisors (0 means 32KiHz) */ -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com ^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 3/3] ARM: at91/tclib: mask interruptions at shutdown and probe 2014-08-19 22:07 ` Gaël PORTAY @ 2014-08-19 22:07 ` Gaël PORTAY -1 siblings, 0 replies; 34+ messages in thread From: Gaël PORTAY @ 2014-08-19 22:07 UTC (permalink / raw) To: Arnd Bergmann, Daniel Lezcano, Greg Kroah-Hartman, linux-arm-kernel, linux-kernel, linux-pwm, Nicolas Ferre, Thierry Reding, Thomas Gleixner Cc: Boris Brezillon, Alexandre Belloni, Jean-Christophe PLAGNIOL-VILLARD, Gaël PORTAY Shutdown properly the timer counter block by masking interruptions. Otherwise, a segmentation may happen when kexec-ing a new kernel (see backtrace below). An interruption may happen before the handler is set, leading to a kernel segmentation fault. Furthermore, we make sure the interruptions are masked when the driver is initialized. This will prevent freshly kexec-ed kernel from crashing when launched from a kernel which does not properly mask interruptions at shutdown. The backtrace below happened after kexec-ing a new kernel, from a kernel that did not shut down properly leaving interruptions unmasked. Unable to handle kernel NULL pointer dereference at virtual address 00000000 pgd = c0004000 [00000000] *pgd=00000000 Internal error: Oops: 80000005 [#1] ARM Modules linked in: CPU: 0 PID: 1 Comm: swapper Not tainted 3.16.0+ #144 task: c1828aa0 ti: c182a000 task.ti: c182a000 PC is at 0x0 LR is at ch2_irq+0x28/0x30 pc : [<00000000>] lr : [<c01db904>] psr: 000000d3 sp : c182bd38 ip : c182bd48 fp : c182bd44 r10: c0373390 r9 : c1825b00 r8 : 60000053 r7 : 00000000 r6 : 00000000 r5 : 00000013 r4 : c036e800 r3 : 00000000 r2 : 00002004 r1 : c036e760 r0 : c036e760 Flags: nzcv IRQs off FIQs off Mode SVC_32 ISA ARM Segment kernel Control: 0005317f Table: 20004000 DAC: 00000017 Process swapper (pid: 1, stack limit = 0xc182a1c0) Stack: (0xc182bd38 to 0xc182c000) bd20: c182bd7c c182bd48 bd40: c0045430 c01db8ec 00000000 c18c6f40 c182bd74 c1825b00 c035cec4 00000000 bd60: c182be2c 60000053 c1825b34 00000000 c182bd94 c182bd80 c0045570 c0045408 bd80: 00000000 c1825b00 c182bdac c182bd98 c0047f34 c0045550 00000013 c036619c bda0: c182bdc4 c182bdb0 c0044da4 c0047e98 0000007f 00000013 c182bde4 c182bdc8 bdc0: c0009e34 c0044d8c fefff000 c0046728 60000053 ffffffff c182bdf4 c182bde8 bde0: c00086a8 c0009ddc c182be74 c182bdf8 c000cb80 c0008674 00000000 00000013 be00: 00000000 00014200 c1825b00 c036e800 00000013 c035ed98 60000053 c1825b34 be20: 00000000 c182be74 c182be20 c182be40 c0047994 c0046728 60000053 ffffffff be40: 00000013 c036e800 c182be64 c1825b00 00000013 c036e800 c035ed98 c03874bc be60: 00000004 c036e700 c182be94 c182be78 c004689c c0046398 c036e760 c18c6080 be80: 00000000 c035ed10 c182bedc c182be98 c0348b08 c004684c 0000000c c034dac8 bea0: 004c4b3f c028c338 c036e760 00000013 c014ecc8 c18e67e0 c035b9c0 c0348884 bec0: c035b9c0 c182a020 00000000 00000000 c182bf54 c182bee0 c00089fc c0348894 bee0: c00da51c c1ffcc78 c182bf0c c182bef8 c002d100 c002d09c c1ffcc78 00000000 bf00: c182bf54 c182bf10 c002d308 c0336570 c182bf3c c0334e44 00000003 00000003 bf20: 00000030 c0334b44 c0044d74 00000003 00000003 c034dac8 c0350a94 c0373440 bf40: c0373440 00000030 c182bf94 c182bf58 c0336d24 c000890c 00000003 00000003 bf60: c0336560 c182bf64 c182bf64 6e616e0d 00000000 c0272fc8 00000000 00000000 bf80: 00000000 00000000 c182bfac c182bf98 c0272fd8 c0336bd8 c182a000 00000000 bfa0: 00000000 c182bfb0 c00095d0 c0272fd8 00000000 00000000 00000000 00000000 bfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 bfe0: 00000000 00000000 00000000 00000000 00000013 00000000 374d27cd 33cc33e4 Backtrace: [<c01db8dc>] (ch2_irq) from [<c0045430>] (handle_irq_event_percpu+0x38/0x148) [<c00453f8>] (handle_irq_event_percpu) from [<c0045570>] (handle_irq_event+0x30/0x40) r10:00000000 r9:c1825b34 r8:60000053 r7:c182be2c r6:00000000 r5:c035cec4 r4:c1825b00 [<c0045540>] (handle_irq_event) from [<c0047f34>] (handle_fasteoi_irq+0xac/0x11c) r4:c1825b00 r3:00000000 [<c0047e88>] (handle_fasteoi_irq) from [<c0044da4>] (generic_handle_irq+0x28/0x38) r5:c036619c r4:00000013 [<c0044d7c>] (generic_handle_irq) from [<c0009e34>] (handle_IRQ+0x68/0x88) r4:00000013 r3:0000007f [<c0009dcc>] (handle_IRQ) from [<c00086a8>] (at91_aic_handle_irq+0x44/0x4c) r6:ffffffff r5:60000053 r4:c0046728 r3:fefff000 [<c0008664>] (at91_aic_handle_irq) from [<c000cb80>] (__irq_svc+0x40/0x4c) Exception stack(0xc182bdf8 to 0xc182be40) bde0: 00000000 00000013 be00: 00000000 00014200 c1825b00 c036e800 00000013 c035ed98 60000053 c1825b34 be20: 00000000 c182be74 c182be20 c182be40 c0047994 c0046728 60000053 ffffffff [<c0046388>] (__setup_irq) from [<c004689c>] (setup_irq+0x60/0x8c) r10:c036e700 r9:00000004 r8:c03874bc r7:c035ed98 r6:c036e800 r5:00000013 r4:c1825b00 [<c004683c>] (setup_irq) from [<c0348b08>] (tcb_clksrc_init+0x284/0x31c) r6:c035ed10 r5:00000000 r4:c18c6080 r3:c036e760 [<c0348884>] (tcb_clksrc_init) from [<c00089fc>] (do_one_initcall+0x100/0x1b4) r10:00000000 r9:00000000 r8:c182a020 r7:c035b9c0 r6:c0348884 r5:c035b9c0 r4:c18e67e0 [<c00088fc>] (do_one_initcall) from [<c0336d24>] (kernel_init_freeable+0x15c/0x224) r9:00000030 r8:c0373440 r7:c0373440 r6:c0350a94 r5:c034dac8 r4:00000003 [<c0336bc8>] (kernel_init_freeable) from [<c0272fd8>] (kernel_init+0x10/0xec) r9:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c0272fc8 r4:00000000 [<c0272fc8>] (kernel_init) from [<c00095d0>] (ret_from_fork+0x14/0x24) r4:00000000 r3:c182a000 Code: bad PC value ---[ end trace 5b30f0017e282e47 ]--- Kernel panic - not syncing: Fatal exception in interrupt Signed-off-by: Gaël PORTAY <gael.portay@gmail.com> --- drivers/misc/atmel_tclib.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/drivers/misc/atmel_tclib.c b/drivers/misc/atmel_tclib.c index d505d1e..a680151 100644 --- a/drivers/misc/atmel_tclib.c +++ b/drivers/misc/atmel_tclib.c @@ -109,6 +109,7 @@ static int __init tc_probe(struct platform_device *pdev) struct clk *clk; int irq; struct resource *r; + unsigned int i; irq = platform_get_irq(pdev, 0); if (irq < 0) @@ -157,18 +158,33 @@ static int __init tc_probe(struct platform_device *pdev) if (tc->irq[2] < 0) tc->irq[2] = irq; + for (i = 0; i < 3; i++) + __raw_writel(0xff, tc->regs + ATMEL_TC_REG(i, IDR)); + spin_lock(&tc_list_lock); list_add_tail(&tc->node, &tc_list); spin_unlock(&tc_list_lock); + platform_set_drvdata(pdev, tc); + return 0; } +static void tc_shutdown (struct platform_device *pdev) +{ + int i; + struct atmel_tc *tc = platform_get_drvdata(pdev); + + for (i = 0; i < 3; i++) + __raw_writel(0xff, tc->regs + ATMEL_TC_REG(i, IDR)); +} + static struct platform_driver tc_driver = { .driver = { .name = "atmel_tcb", .of_match_table = of_match_ptr(atmel_tcb_dt_ids), }, + .shutdown = tc_shutdown, }; static int __init tc_init(void) -- 1.9.3 ^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH 3/3] ARM: at91/tclib: mask interruptions at shutdown and probe @ 2014-08-19 22:07 ` Gaël PORTAY 0 siblings, 0 replies; 34+ messages in thread From: Gaël PORTAY @ 2014-08-19 22:07 UTC (permalink / raw) To: linux-arm-kernel Shutdown properly the timer counter block by masking interruptions. Otherwise, a segmentation may happen when kexec-ing a new kernel (see backtrace below). An interruption may happen before the handler is set, leading to a kernel segmentation fault. Furthermore, we make sure the interruptions are masked when the driver is initialized. This will prevent freshly kexec-ed kernel from crashing when launched from a kernel which does not properly mask interruptions at shutdown. The backtrace below happened after kexec-ing a new kernel, from a kernel that did not shut down properly leaving interruptions unmasked. Unable to handle kernel NULL pointer dereference at virtual address 00000000 pgd = c0004000 [00000000] *pgd=00000000 Internal error: Oops: 80000005 [#1] ARM Modules linked in: CPU: 0 PID: 1 Comm: swapper Not tainted 3.16.0+ #144 task: c1828aa0 ti: c182a000 task.ti: c182a000 PC is at 0x0 LR is at ch2_irq+0x28/0x30 pc : [<00000000>] lr : [<c01db904>] psr: 000000d3 sp : c182bd38 ip : c182bd48 fp : c182bd44 r10: c0373390 r9 : c1825b00 r8 : 60000053 r7 : 00000000 r6 : 00000000 r5 : 00000013 r4 : c036e800 r3 : 00000000 r2 : 00002004 r1 : c036e760 r0 : c036e760 Flags: nzcv IRQs off FIQs off Mode SVC_32 ISA ARM Segment kernel Control: 0005317f Table: 20004000 DAC: 00000017 Process swapper (pid: 1, stack limit = 0xc182a1c0) Stack: (0xc182bd38 to 0xc182c000) bd20: c182bd7c c182bd48 bd40: c0045430 c01db8ec 00000000 c18c6f40 c182bd74 c1825b00 c035cec4 00000000 bd60: c182be2c 60000053 c1825b34 00000000 c182bd94 c182bd80 c0045570 c0045408 bd80: 00000000 c1825b00 c182bdac c182bd98 c0047f34 c0045550 00000013 c036619c bda0: c182bdc4 c182bdb0 c0044da4 c0047e98 0000007f 00000013 c182bde4 c182bdc8 bdc0: c0009e34 c0044d8c fefff000 c0046728 60000053 ffffffff c182bdf4 c182bde8 bde0: c00086a8 c0009ddc c182be74 c182bdf8 c000cb80 c0008674 00000000 00000013 be00: 00000000 00014200 c1825b00 c036e800 00000013 c035ed98 60000053 c1825b34 be20: 00000000 c182be74 c182be20 c182be40 c0047994 c0046728 60000053 ffffffff be40: 00000013 c036e800 c182be64 c1825b00 00000013 c036e800 c035ed98 c03874bc be60: 00000004 c036e700 c182be94 c182be78 c004689c c0046398 c036e760 c18c6080 be80: 00000000 c035ed10 c182bedc c182be98 c0348b08 c004684c 0000000c c034dac8 bea0: 004c4b3f c028c338 c036e760 00000013 c014ecc8 c18e67e0 c035b9c0 c0348884 bec0: c035b9c0 c182a020 00000000 00000000 c182bf54 c182bee0 c00089fc c0348894 bee0: c00da51c c1ffcc78 c182bf0c c182bef8 c002d100 c002d09c c1ffcc78 00000000 bf00: c182bf54 c182bf10 c002d308 c0336570 c182bf3c c0334e44 00000003 00000003 bf20: 00000030 c0334b44 c0044d74 00000003 00000003 c034dac8 c0350a94 c0373440 bf40: c0373440 00000030 c182bf94 c182bf58 c0336d24 c000890c 00000003 00000003 bf60: c0336560 c182bf64 c182bf64 6e616e0d 00000000 c0272fc8 00000000 00000000 bf80: 00000000 00000000 c182bfac c182bf98 c0272fd8 c0336bd8 c182a000 00000000 bfa0: 00000000 c182bfb0 c00095d0 c0272fd8 00000000 00000000 00000000 00000000 bfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 bfe0: 00000000 00000000 00000000 00000000 00000013 00000000 374d27cd 33cc33e4 Backtrace: [<c01db8dc>] (ch2_irq) from [<c0045430>] (handle_irq_event_percpu+0x38/0x148) [<c00453f8>] (handle_irq_event_percpu) from [<c0045570>] (handle_irq_event+0x30/0x40) r10:00000000 r9:c1825b34 r8:60000053 r7:c182be2c r6:00000000 r5:c035cec4 r4:c1825b00 [<c0045540>] (handle_irq_event) from [<c0047f34>] (handle_fasteoi_irq+0xac/0x11c) r4:c1825b00 r3:00000000 [<c0047e88>] (handle_fasteoi_irq) from [<c0044da4>] (generic_handle_irq+0x28/0x38) r5:c036619c r4:00000013 [<c0044d7c>] (generic_handle_irq) from [<c0009e34>] (handle_IRQ+0x68/0x88) r4:00000013 r3:0000007f [<c0009dcc>] (handle_IRQ) from [<c00086a8>] (at91_aic_handle_irq+0x44/0x4c) r6:ffffffff r5:60000053 r4:c0046728 r3:fefff000 [<c0008664>] (at91_aic_handle_irq) from [<c000cb80>] (__irq_svc+0x40/0x4c) Exception stack(0xc182bdf8 to 0xc182be40) bde0: 00000000 00000013 be00: 00000000 00014200 c1825b00 c036e800 00000013 c035ed98 60000053 c1825b34 be20: 00000000 c182be74 c182be20 c182be40 c0047994 c0046728 60000053 ffffffff [<c0046388>] (__setup_irq) from [<c004689c>] (setup_irq+0x60/0x8c) r10:c036e700 r9:00000004 r8:c03874bc r7:c035ed98 r6:c036e800 r5:00000013 r4:c1825b00 [<c004683c>] (setup_irq) from [<c0348b08>] (tcb_clksrc_init+0x284/0x31c) r6:c035ed10 r5:00000000 r4:c18c6080 r3:c036e760 [<c0348884>] (tcb_clksrc_init) from [<c00089fc>] (do_one_initcall+0x100/0x1b4) r10:00000000 r9:00000000 r8:c182a020 r7:c035b9c0 r6:c0348884 r5:c035b9c0 r4:c18e67e0 [<c00088fc>] (do_one_initcall) from [<c0336d24>] (kernel_init_freeable+0x15c/0x224) r9:00000030 r8:c0373440 r7:c0373440 r6:c0350a94 r5:c034dac8 r4:00000003 [<c0336bc8>] (kernel_init_freeable) from [<c0272fd8>] (kernel_init+0x10/0xec) r9:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c0272fc8 r4:00000000 [<c0272fc8>] (kernel_init) from [<c00095d0>] (ret_from_fork+0x14/0x24) r4:00000000 r3:c182a000 Code: bad PC value ---[ end trace 5b30f0017e282e47 ]--- Kernel panic - not syncing: Fatal exception in interrupt Signed-off-by: Ga?l PORTAY <gael.portay@gmail.com> --- drivers/misc/atmel_tclib.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/drivers/misc/atmel_tclib.c b/drivers/misc/atmel_tclib.c index d505d1e..a680151 100644 --- a/drivers/misc/atmel_tclib.c +++ b/drivers/misc/atmel_tclib.c @@ -109,6 +109,7 @@ static int __init tc_probe(struct platform_device *pdev) struct clk *clk; int irq; struct resource *r; + unsigned int i; irq = platform_get_irq(pdev, 0); if (irq < 0) @@ -157,18 +158,33 @@ static int __init tc_probe(struct platform_device *pdev) if (tc->irq[2] < 0) tc->irq[2] = irq; + for (i = 0; i < 3; i++) + __raw_writel(0xff, tc->regs + ATMEL_TC_REG(i, IDR)); + spin_lock(&tc_list_lock); list_add_tail(&tc->node, &tc_list); spin_unlock(&tc_list_lock); + platform_set_drvdata(pdev, tc); + return 0; } +static void tc_shutdown (struct platform_device *pdev) +{ + int i; + struct atmel_tc *tc = platform_get_drvdata(pdev); + + for (i = 0; i < 3; i++) + __raw_writel(0xff, tc->regs + ATMEL_TC_REG(i, IDR)); +} + static struct platform_driver tc_driver = { .driver = { .name = "atmel_tcb", .of_match_table = of_match_ptr(atmel_tcb_dt_ids), }, + .shutdown = tc_shutdown, }; static int __init tc_init(void) -- 1.9.3 ^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH 3/3] ARM: at91/tclib: mask interruptions at shutdown and probe 2014-08-19 22:07 ` Gaël PORTAY @ 2014-08-19 22:11 ` Jean-Christophe PLAGNIOL-VILLARD -1 siblings, 0 replies; 34+ messages in thread From: Jean-Christophe PLAGNIOL-VILLARD @ 2014-08-19 22:11 UTC (permalink / raw) To: Gaël PORTAY Cc: Jean-Christophe PLAGNIOL-VILLARD, Arnd Bergmann, Daniel Lezcano, Greg Kroah-Hartman, linux-arm-kernel, linux-kernel, linux-pwm, Nicolas FERRE, Thierry Reding, Thomas Gleixner, Boris Brezillon, Alexandre Belloni Hi, This is a bit weird as the clock of the TC should be off and the irq free so this should never happened we need to investigate more why this append Best Regards, J. On Aug 20, 2014, at 6:07 AM, Gaël PORTAY <gael.portay@gmail.com> wrote: > > Shutdown properly the timer counter block by masking interruptions. Otherwise, > a segmentation may happen when kexec-ing a new kernel (see backtrace below). > An interruption may happen before the handler is set, leading to a kernel > segmentation fault. > > Furthermore, we make sure the interruptions are masked when the driver is > initialized. This will prevent freshly kexec-ed kernel from crashing when > launched from a kernel which does not properly mask interruptions at shutdown. > > The backtrace below happened after kexec-ing a new kernel, from a kernel > that did not shut down properly leaving interruptions unmasked. > > Unable to handle kernel NULL pointer dereference at virtual address 00000000 > pgd = c0004000 > [00000000] *pgd=00000000 > Internal error: Oops: 80000005 [#1] ARM > Modules linked in: > CPU: 0 PID: 1 Comm: swapper Not tainted 3.16.0+ #144 > task: c1828aa0 ti: c182a000 task.ti: c182a000 > PC is at 0x0 > LR is at ch2_irq+0x28/0x30 > pc : [<00000000>] lr : [<c01db904>] psr: 000000d3 > sp : c182bd38 ip : c182bd48 fp : c182bd44 > r10: c0373390 r9 : c1825b00 r8 : 60000053 > r7 : 00000000 r6 : 00000000 r5 : 00000013 r4 : c036e800 > r3 : 00000000 r2 : 00002004 r1 : c036e760 r0 : c036e760 > Flags: nzcv IRQs off FIQs off Mode SVC_32 ISA ARM Segment kernel > Control: 0005317f Table: 20004000 DAC: 00000017 > Process swapper (pid: 1, stack limit = 0xc182a1c0) > Stack: (0xc182bd38 to 0xc182c000) > bd20: c182bd7c c182bd48 > bd40: c0045430 c01db8ec 00000000 c18c6f40 c182bd74 c1825b00 c035cec4 00000000 > bd60: c182be2c 60000053 c1825b34 00000000 c182bd94 c182bd80 c0045570 c0045408 > bd80: 00000000 c1825b00 c182bdac c182bd98 c0047f34 c0045550 00000013 c036619c > bda0: c182bdc4 c182bdb0 c0044da4 c0047e98 0000007f 00000013 c182bde4 c182bdc8 > bdc0: c0009e34 c0044d8c fefff000 c0046728 60000053 ffffffff c182bdf4 c182bde8 > bde0: c00086a8 c0009ddc c182be74 c182bdf8 c000cb80 c0008674 00000000 00000013 > be00: 00000000 00014200 c1825b00 c036e800 00000013 c035ed98 60000053 c1825b34 > be20: 00000000 c182be74 c182be20 c182be40 c0047994 c0046728 60000053 ffffffff > be40: 00000013 c036e800 c182be64 c1825b00 00000013 c036e800 c035ed98 c03874bc > be60: 00000004 c036e700 c182be94 c182be78 c004689c c0046398 c036e760 c18c6080 > be80: 00000000 c035ed10 c182bedc c182be98 c0348b08 c004684c 0000000c c034dac8 > bea0: 004c4b3f c028c338 c036e760 00000013 c014ecc8 c18e67e0 c035b9c0 c0348884 > bec0: c035b9c0 c182a020 00000000 00000000 c182bf54 c182bee0 c00089fc c0348894 > bee0: c00da51c c1ffcc78 c182bf0c c182bef8 c002d100 c002d09c c1ffcc78 00000000 > bf00: c182bf54 c182bf10 c002d308 c0336570 c182bf3c c0334e44 00000003 00000003 > bf20: 00000030 c0334b44 c0044d74 00000003 00000003 c034dac8 c0350a94 c0373440 > bf40: c0373440 00000030 c182bf94 c182bf58 c0336d24 c000890c 00000003 00000003 > bf60: c0336560 c182bf64 c182bf64 6e616e0d 00000000 c0272fc8 00000000 00000000 > bf80: 00000000 00000000 c182bfac c182bf98 c0272fd8 c0336bd8 c182a000 00000000 > bfa0: 00000000 c182bfb0 c00095d0 c0272fd8 00000000 00000000 00000000 00000000 > bfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 > bfe0: 00000000 00000000 00000000 00000000 00000013 00000000 374d27cd 33cc33e4 > Backtrace: > [<c01db8dc>] (ch2_irq) from [<c0045430>] (handle_irq_event_percpu+0x38/0x148) > [<c00453f8>] (handle_irq_event_percpu) from [<c0045570>] (handle_irq_event+0x30/0x40) > r10:00000000 r9:c1825b34 r8:60000053 r7:c182be2c r6:00000000 r5:c035cec4 > r4:c1825b00 > [<c0045540>] (handle_irq_event) from [<c0047f34>] (handle_fasteoi_irq+0xac/0x11c) > r4:c1825b00 r3:00000000 > [<c0047e88>] (handle_fasteoi_irq) from [<c0044da4>] (generic_handle_irq+0x28/0x38) > r5:c036619c r4:00000013 > [<c0044d7c>] (generic_handle_irq) from [<c0009e34>] (handle_IRQ+0x68/0x88) > r4:00000013 r3:0000007f > [<c0009dcc>] (handle_IRQ) from [<c00086a8>] (at91_aic_handle_irq+0x44/0x4c) > r6:ffffffff r5:60000053 r4:c0046728 r3:fefff000 > [<c0008664>] (at91_aic_handle_irq) from [<c000cb80>] (__irq_svc+0x40/0x4c) > Exception stack(0xc182bdf8 to 0xc182be40) > bde0: 00000000 00000013 > be00: 00000000 00014200 c1825b00 c036e800 00000013 c035ed98 60000053 c1825b34 > be20: 00000000 c182be74 c182be20 c182be40 c0047994 c0046728 60000053 ffffffff > [<c0046388>] (__setup_irq) from [<c004689c>] (setup_irq+0x60/0x8c) > r10:c036e700 r9:00000004 r8:c03874bc r7:c035ed98 r6:c036e800 r5:00000013 > r4:c1825b00 > [<c004683c>] (setup_irq) from [<c0348b08>] (tcb_clksrc_init+0x284/0x31c) > r6:c035ed10 r5:00000000 r4:c18c6080 r3:c036e760 > [<c0348884>] (tcb_clksrc_init) from [<c00089fc>] (do_one_initcall+0x100/0x1b4) > r10:00000000 r9:00000000 r8:c182a020 r7:c035b9c0 r6:c0348884 r5:c035b9c0 > r4:c18e67e0 > [<c00088fc>] (do_one_initcall) from [<c0336d24>] (kernel_init_freeable+0x15c/0x224) > r9:00000030 r8:c0373440 r7:c0373440 r6:c0350a94 r5:c034dac8 r4:00000003 > [<c0336bc8>] (kernel_init_freeable) from [<c0272fd8>] (kernel_init+0x10/0xec) > r9:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c0272fc8 r4:00000000 > [<c0272fc8>] (kernel_init) from [<c00095d0>] (ret_from_fork+0x14/0x24) > r4:00000000 r3:c182a000 > Code: bad PC value > ---[ end trace 5b30f0017e282e47 ]--- > Kernel panic - not syncing: Fatal exception in interrupt > > Signed-off-by: Gaël PORTAY <gael.portay@gmail.com> > --- > drivers/misc/atmel_tclib.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/drivers/misc/atmel_tclib.c b/drivers/misc/atmel_tclib.c > index d505d1e..a680151 100644 > --- a/drivers/misc/atmel_tclib.c > +++ b/drivers/misc/atmel_tclib.c > @@ -109,6 +109,7 @@ static int __init tc_probe(struct platform_device *pdev) > struct clk *clk; > int irq; > struct resource *r; > + unsigned int i; > > irq = platform_get_irq(pdev, 0); > if (irq < 0) > @@ -157,18 +158,33 @@ static int __init tc_probe(struct platform_device *pdev) > if (tc->irq[2] < 0) > tc->irq[2] = irq; > > + for (i = 0; i < 3; i++) > + __raw_writel(0xff, tc->regs + ATMEL_TC_REG(i, IDR)); > + > spin_lock(&tc_list_lock); > list_add_tail(&tc->node, &tc_list); > spin_unlock(&tc_list_lock); > > + platform_set_drvdata(pdev, tc); > + > return 0; > } > > +static void tc_shutdown (struct platform_device *pdev) > +{ > + int i; > + struct atmel_tc *tc = platform_get_drvdata(pdev); > + > + for (i = 0; i < 3; i++) > + __raw_writel(0xff, tc->regs + ATMEL_TC_REG(i, IDR)); > +} > + > static struct platform_driver tc_driver = { > .driver = { > .name = "atmel_tcb", > .of_match_table = of_match_ptr(atmel_tcb_dt_ids), > }, > + .shutdown = tc_shutdown, > }; > > static int __init tc_init(void) > -- > 1.9.3 > ^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 3/3] ARM: at91/tclib: mask interruptions at shutdown and probe @ 2014-08-19 22:11 ` Jean-Christophe PLAGNIOL-VILLARD 0 siblings, 0 replies; 34+ messages in thread From: Jean-Christophe PLAGNIOL-VILLARD @ 2014-08-19 22:11 UTC (permalink / raw) To: linux-arm-kernel Hi, This is a bit weird as the clock of the TC should be off and the irq free so this should never happened we need to investigate more why this append Best Regards, J. On Aug 20, 2014, at 6:07 AM, Ga?l PORTAY <gael.portay@gmail.com> wrote: > > Shutdown properly the timer counter block by masking interruptions. Otherwise, > a segmentation may happen when kexec-ing a new kernel (see backtrace below). > An interruption may happen before the handler is set, leading to a kernel > segmentation fault. > > Furthermore, we make sure the interruptions are masked when the driver is > initialized. This will prevent freshly kexec-ed kernel from crashing when > launched from a kernel which does not properly mask interruptions at shutdown. > > The backtrace below happened after kexec-ing a new kernel, from a kernel > that did not shut down properly leaving interruptions unmasked. > > Unable to handle kernel NULL pointer dereference at virtual address 00000000 > pgd = c0004000 > [00000000] *pgd=00000000 > Internal error: Oops: 80000005 [#1] ARM > Modules linked in: > CPU: 0 PID: 1 Comm: swapper Not tainted 3.16.0+ #144 > task: c1828aa0 ti: c182a000 task.ti: c182a000 > PC is at 0x0 > LR is at ch2_irq+0x28/0x30 > pc : [<00000000>] lr : [<c01db904>] psr: 000000d3 > sp : c182bd38 ip : c182bd48 fp : c182bd44 > r10: c0373390 r9 : c1825b00 r8 : 60000053 > r7 : 00000000 r6 : 00000000 r5 : 00000013 r4 : c036e800 > r3 : 00000000 r2 : 00002004 r1 : c036e760 r0 : c036e760 > Flags: nzcv IRQs off FIQs off Mode SVC_32 ISA ARM Segment kernel > Control: 0005317f Table: 20004000 DAC: 00000017 > Process swapper (pid: 1, stack limit = 0xc182a1c0) > Stack: (0xc182bd38 to 0xc182c000) > bd20: c182bd7c c182bd48 > bd40: c0045430 c01db8ec 00000000 c18c6f40 c182bd74 c1825b00 c035cec4 00000000 > bd60: c182be2c 60000053 c1825b34 00000000 c182bd94 c182bd80 c0045570 c0045408 > bd80: 00000000 c1825b00 c182bdac c182bd98 c0047f34 c0045550 00000013 c036619c > bda0: c182bdc4 c182bdb0 c0044da4 c0047e98 0000007f 00000013 c182bde4 c182bdc8 > bdc0: c0009e34 c0044d8c fefff000 c0046728 60000053 ffffffff c182bdf4 c182bde8 > bde0: c00086a8 c0009ddc c182be74 c182bdf8 c000cb80 c0008674 00000000 00000013 > be00: 00000000 00014200 c1825b00 c036e800 00000013 c035ed98 60000053 c1825b34 > be20: 00000000 c182be74 c182be20 c182be40 c0047994 c0046728 60000053 ffffffff > be40: 00000013 c036e800 c182be64 c1825b00 00000013 c036e800 c035ed98 c03874bc > be60: 00000004 c036e700 c182be94 c182be78 c004689c c0046398 c036e760 c18c6080 > be80: 00000000 c035ed10 c182bedc c182be98 c0348b08 c004684c 0000000c c034dac8 > bea0: 004c4b3f c028c338 c036e760 00000013 c014ecc8 c18e67e0 c035b9c0 c0348884 > bec0: c035b9c0 c182a020 00000000 00000000 c182bf54 c182bee0 c00089fc c0348894 > bee0: c00da51c c1ffcc78 c182bf0c c182bef8 c002d100 c002d09c c1ffcc78 00000000 > bf00: c182bf54 c182bf10 c002d308 c0336570 c182bf3c c0334e44 00000003 00000003 > bf20: 00000030 c0334b44 c0044d74 00000003 00000003 c034dac8 c0350a94 c0373440 > bf40: c0373440 00000030 c182bf94 c182bf58 c0336d24 c000890c 00000003 00000003 > bf60: c0336560 c182bf64 c182bf64 6e616e0d 00000000 c0272fc8 00000000 00000000 > bf80: 00000000 00000000 c182bfac c182bf98 c0272fd8 c0336bd8 c182a000 00000000 > bfa0: 00000000 c182bfb0 c00095d0 c0272fd8 00000000 00000000 00000000 00000000 > bfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 > bfe0: 00000000 00000000 00000000 00000000 00000013 00000000 374d27cd 33cc33e4 > Backtrace: > [<c01db8dc>] (ch2_irq) from [<c0045430>] (handle_irq_event_percpu+0x38/0x148) > [<c00453f8>] (handle_irq_event_percpu) from [<c0045570>] (handle_irq_event+0x30/0x40) > r10:00000000 r9:c1825b34 r8:60000053 r7:c182be2c r6:00000000 r5:c035cec4 > r4:c1825b00 > [<c0045540>] (handle_irq_event) from [<c0047f34>] (handle_fasteoi_irq+0xac/0x11c) > r4:c1825b00 r3:00000000 > [<c0047e88>] (handle_fasteoi_irq) from [<c0044da4>] (generic_handle_irq+0x28/0x38) > r5:c036619c r4:00000013 > [<c0044d7c>] (generic_handle_irq) from [<c0009e34>] (handle_IRQ+0x68/0x88) > r4:00000013 r3:0000007f > [<c0009dcc>] (handle_IRQ) from [<c00086a8>] (at91_aic_handle_irq+0x44/0x4c) > r6:ffffffff r5:60000053 r4:c0046728 r3:fefff000 > [<c0008664>] (at91_aic_handle_irq) from [<c000cb80>] (__irq_svc+0x40/0x4c) > Exception stack(0xc182bdf8 to 0xc182be40) > bde0: 00000000 00000013 > be00: 00000000 00014200 c1825b00 c036e800 00000013 c035ed98 60000053 c1825b34 > be20: 00000000 c182be74 c182be20 c182be40 c0047994 c0046728 60000053 ffffffff > [<c0046388>] (__setup_irq) from [<c004689c>] (setup_irq+0x60/0x8c) > r10:c036e700 r9:00000004 r8:c03874bc r7:c035ed98 r6:c036e800 r5:00000013 > r4:c1825b00 > [<c004683c>] (setup_irq) from [<c0348b08>] (tcb_clksrc_init+0x284/0x31c) > r6:c035ed10 r5:00000000 r4:c18c6080 r3:c036e760 > [<c0348884>] (tcb_clksrc_init) from [<c00089fc>] (do_one_initcall+0x100/0x1b4) > r10:00000000 r9:00000000 r8:c182a020 r7:c035b9c0 r6:c0348884 r5:c035b9c0 > r4:c18e67e0 > [<c00088fc>] (do_one_initcall) from [<c0336d24>] (kernel_init_freeable+0x15c/0x224) > r9:00000030 r8:c0373440 r7:c0373440 r6:c0350a94 r5:c034dac8 r4:00000003 > [<c0336bc8>] (kernel_init_freeable) from [<c0272fd8>] (kernel_init+0x10/0xec) > r9:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c0272fc8 r4:00000000 > [<c0272fc8>] (kernel_init) from [<c00095d0>] (ret_from_fork+0x14/0x24) > r4:00000000 r3:c182a000 > Code: bad PC value > ---[ end trace 5b30f0017e282e47 ]--- > Kernel panic - not syncing: Fatal exception in interrupt > > Signed-off-by: Ga?l PORTAY <gael.portay@gmail.com> > --- > drivers/misc/atmel_tclib.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/drivers/misc/atmel_tclib.c b/drivers/misc/atmel_tclib.c > index d505d1e..a680151 100644 > --- a/drivers/misc/atmel_tclib.c > +++ b/drivers/misc/atmel_tclib.c > @@ -109,6 +109,7 @@ static int __init tc_probe(struct platform_device *pdev) > struct clk *clk; > int irq; > struct resource *r; > + unsigned int i; > > irq = platform_get_irq(pdev, 0); > if (irq < 0) > @@ -157,18 +158,33 @@ static int __init tc_probe(struct platform_device *pdev) > if (tc->irq[2] < 0) > tc->irq[2] = irq; > > + for (i = 0; i < 3; i++) > + __raw_writel(0xff, tc->regs + ATMEL_TC_REG(i, IDR)); > + > spin_lock(&tc_list_lock); > list_add_tail(&tc->node, &tc_list); > spin_unlock(&tc_list_lock); > > + platform_set_drvdata(pdev, tc); > + > return 0; > } > > +static void tc_shutdown (struct platform_device *pdev) > +{ > + int i; > + struct atmel_tc *tc = platform_get_drvdata(pdev); > + > + for (i = 0; i < 3; i++) > + __raw_writel(0xff, tc->regs + ATMEL_TC_REG(i, IDR)); > +} > + > static struct platform_driver tc_driver = { > .driver = { > .name = "atmel_tcb", > .of_match_table = of_match_ptr(atmel_tcb_dt_ids), > }, > + .shutdown = tc_shutdown, > }; > > static int __init tc_init(void) > -- > 1.9.3 > ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 3/3] ARM: at91/tclib: mask interruptions at shutdown and probe 2014-08-19 22:11 ` Jean-Christophe PLAGNIOL-VILLARD @ 2014-08-19 23:01 ` Boris BREZILLON -1 siblings, 0 replies; 34+ messages in thread From: Boris BREZILLON @ 2014-08-19 23:01 UTC (permalink / raw) To: Jean-Christophe PLAGNIOL-VILLARD Cc: Gaël PORTAY, Arnd Bergmann, Daniel Lezcano, Greg Kroah-Hartman, linux-arm-kernel, linux-kernel, linux-pwm, Nicolas FERRE, Thierry Reding, Thomas Gleixner, Alexandre Belloni Hi Jean-Christophe, On Wed, 20 Aug 2014 06:11:17 +0800 Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> wrote: > Hi, > > This is a bit weird as the clock of the TC should be off and the irq free > > so this should never happened we need to investigate more why this append I may have found the source of this bug. As Gael stated, when you're kexec-ing a new kernel your previous kernel could be using the tbc_clksrc driver (and especially the clkevent device). Thus the kernel might have planned a timer event and then been asked to shutdown the machine (requested by the kexec code). In this case the AIC interrupt connected to the TC Block is disabled but not the interrupts within the TCB IP (IDR registers), possibly leaving a pending interrupt before booting the new kernel. When the tcb_clksrc driver is loaded by the new kernel it enables the interrupt line by calling setup_irq [1] while the clockevent device is not registered yet [2]. Thus the event_handler is still NULL when the AIC line connected to the TCB is unmasked. Remember that an interrupt is still pending on this HW block, which will lead to an immediate call to the ch2_irq handler, which tries to call the event_handler, which in turns is NULL because clkevent device registration has not taken place at this moment => Kernel panic. ITOH, we can't register the clkevent device before the irq handler is set up, because we should be ready to handle clkevent request at the time clockevents_config_and_register is called. This leaves two solution: 1) disable the TCB irqs (using TCB IDR registers) before calling setup_irq in the tcb_clksrc driver 2) disable the TCB irqs at the tclib level (as proposed by Gael) I prefer solution #2 because it fixes the bug for all TCB users (not just the tcb_clksrc driver). Best Regards, Boris [1]http://lxr.free-electrons.com/source/drivers/clocksource/tcb_clksrc.c#L207 [2]http://lxr.free-electrons.com/source/drivers/clocksource/tcb_clksrc.c#L211 > > Best Regards, > J. > On Aug 20, 2014, at 6:07 AM, Gaël PORTAY <gael.portay@gmail.com> wrote: > > > > > Shutdown properly the timer counter block by masking interruptions. Otherwise, > > a segmentation may happen when kexec-ing a new kernel (see backtrace below). > > An interruption may happen before the handler is set, leading to a kernel > > segmentation fault. > > > > Furthermore, we make sure the interruptions are masked when the driver is > > initialized. This will prevent freshly kexec-ed kernel from crashing when > > launched from a kernel which does not properly mask interruptions at shutdown. > > > > The backtrace below happened after kexec-ing a new kernel, from a kernel > > that did not shut down properly leaving interruptions unmasked. > > > > Unable to handle kernel NULL pointer dereference at virtual address 00000000 > > pgd = c0004000 > > [00000000] *pgd=00000000 > > Internal error: Oops: 80000005 [#1] ARM > > Modules linked in: > > CPU: 0 PID: 1 Comm: swapper Not tainted 3.16.0+ #144 > > task: c1828aa0 ti: c182a000 task.ti: c182a000 > > PC is at 0x0 > > LR is at ch2_irq+0x28/0x30 > > pc : [<00000000>] lr : [<c01db904>] psr: 000000d3 > > sp : c182bd38 ip : c182bd48 fp : c182bd44 > > r10: c0373390 r9 : c1825b00 r8 : 60000053 > > r7 : 00000000 r6 : 00000000 r5 : 00000013 r4 : c036e800 > > r3 : 00000000 r2 : 00002004 r1 : c036e760 r0 : c036e760 > > Flags: nzcv IRQs off FIQs off Mode SVC_32 ISA ARM Segment kernel > > Control: 0005317f Table: 20004000 DAC: 00000017 > > Process swapper (pid: 1, stack limit = 0xc182a1c0) > > Stack: (0xc182bd38 to 0xc182c000) > > bd20: c182bd7c c182bd48 > > bd40: c0045430 c01db8ec 00000000 c18c6f40 c182bd74 c1825b00 c035cec4 00000000 > > bd60: c182be2c 60000053 c1825b34 00000000 c182bd94 c182bd80 c0045570 c0045408 > > bd80: 00000000 c1825b00 c182bdac c182bd98 c0047f34 c0045550 00000013 c036619c > > bda0: c182bdc4 c182bdb0 c0044da4 c0047e98 0000007f 00000013 c182bde4 c182bdc8 > > bdc0: c0009e34 c0044d8c fefff000 c0046728 60000053 ffffffff c182bdf4 c182bde8 > > bde0: c00086a8 c0009ddc c182be74 c182bdf8 c000cb80 c0008674 00000000 00000013 > > be00: 00000000 00014200 c1825b00 c036e800 00000013 c035ed98 60000053 c1825b34 > > be20: 00000000 c182be74 c182be20 c182be40 c0047994 c0046728 60000053 ffffffff > > be40: 00000013 c036e800 c182be64 c1825b00 00000013 c036e800 c035ed98 c03874bc > > be60: 00000004 c036e700 c182be94 c182be78 c004689c c0046398 c036e760 c18c6080 > > be80: 00000000 c035ed10 c182bedc c182be98 c0348b08 c004684c 0000000c c034dac8 > > bea0: 004c4b3f c028c338 c036e760 00000013 c014ecc8 c18e67e0 c035b9c0 c0348884 > > bec0: c035b9c0 c182a020 00000000 00000000 c182bf54 c182bee0 c00089fc c0348894 > > bee0: c00da51c c1ffcc78 c182bf0c c182bef8 c002d100 c002d09c c1ffcc78 00000000 > > bf00: c182bf54 c182bf10 c002d308 c0336570 c182bf3c c0334e44 00000003 00000003 > > bf20: 00000030 c0334b44 c0044d74 00000003 00000003 c034dac8 c0350a94 c0373440 > > bf40: c0373440 00000030 c182bf94 c182bf58 c0336d24 c000890c 00000003 00000003 > > bf60: c0336560 c182bf64 c182bf64 6e616e0d 00000000 c0272fc8 00000000 00000000 > > bf80: 00000000 00000000 c182bfac c182bf98 c0272fd8 c0336bd8 c182a000 00000000 > > bfa0: 00000000 c182bfb0 c00095d0 c0272fd8 00000000 00000000 00000000 00000000 > > bfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 > > bfe0: 00000000 00000000 00000000 00000000 00000013 00000000 374d27cd 33cc33e4 > > Backtrace: > > [<c01db8dc>] (ch2_irq) from [<c0045430>] (handle_irq_event_percpu+0x38/0x148) > > [<c00453f8>] (handle_irq_event_percpu) from [<c0045570>] (handle_irq_event+0x30/0x40) > > r10:00000000 r9:c1825b34 r8:60000053 r7:c182be2c r6:00000000 r5:c035cec4 > > r4:c1825b00 > > [<c0045540>] (handle_irq_event) from [<c0047f34>] (handle_fasteoi_irq+0xac/0x11c) > > r4:c1825b00 r3:00000000 > > [<c0047e88>] (handle_fasteoi_irq) from [<c0044da4>] (generic_handle_irq+0x28/0x38) > > r5:c036619c r4:00000013 > > [<c0044d7c>] (generic_handle_irq) from [<c0009e34>] (handle_IRQ+0x68/0x88) > > r4:00000013 r3:0000007f > > [<c0009dcc>] (handle_IRQ) from [<c00086a8>] (at91_aic_handle_irq+0x44/0x4c) > > r6:ffffffff r5:60000053 r4:c0046728 r3:fefff000 > > [<c0008664>] (at91_aic_handle_irq) from [<c000cb80>] (__irq_svc+0x40/0x4c) > > Exception stack(0xc182bdf8 to 0xc182be40) > > bde0: 00000000 00000013 > > be00: 00000000 00014200 c1825b00 c036e800 00000013 c035ed98 60000053 c1825b34 > > be20: 00000000 c182be74 c182be20 c182be40 c0047994 c0046728 60000053 ffffffff > > [<c0046388>] (__setup_irq) from [<c004689c>] (setup_irq+0x60/0x8c) > > r10:c036e700 r9:00000004 r8:c03874bc r7:c035ed98 r6:c036e800 r5:00000013 > > r4:c1825b00 > > [<c004683c>] (setup_irq) from [<c0348b08>] (tcb_clksrc_init+0x284/0x31c) > > r6:c035ed10 r5:00000000 r4:c18c6080 r3:c036e760 > > [<c0348884>] (tcb_clksrc_init) from [<c00089fc>] (do_one_initcall+0x100/0x1b4) > > r10:00000000 r9:00000000 r8:c182a020 r7:c035b9c0 r6:c0348884 r5:c035b9c0 > > r4:c18e67e0 > > [<c00088fc>] (do_one_initcall) from [<c0336d24>] (kernel_init_freeable+0x15c/0x224) > > r9:00000030 r8:c0373440 r7:c0373440 r6:c0350a94 r5:c034dac8 r4:00000003 > > [<c0336bc8>] (kernel_init_freeable) from [<c0272fd8>] (kernel_init+0x10/0xec) > > r9:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c0272fc8 r4:00000000 > > [<c0272fc8>] (kernel_init) from [<c00095d0>] (ret_from_fork+0x14/0x24) > > r4:00000000 r3:c182a000 > > Code: bad PC value > > ---[ end trace 5b30f0017e282e47 ]--- > > Kernel panic - not syncing: Fatal exception in interrupt > > > > Signed-off-by: Gaël PORTAY <gael.portay@gmail.com> > > --- > > drivers/misc/atmel_tclib.c | 16 ++++++++++++++++ > > 1 file changed, 16 insertions(+) > > > > diff --git a/drivers/misc/atmel_tclib.c b/drivers/misc/atmel_tclib.c > > index d505d1e..a680151 100644 > > --- a/drivers/misc/atmel_tclib.c > > +++ b/drivers/misc/atmel_tclib.c > > @@ -109,6 +109,7 @@ static int __init tc_probe(struct platform_device *pdev) > > struct clk *clk; > > int irq; > > struct resource *r; > > + unsigned int i; > > > > irq = platform_get_irq(pdev, 0); > > if (irq < 0) > > @@ -157,18 +158,33 @@ static int __init tc_probe(struct platform_device *pdev) > > if (tc->irq[2] < 0) > > tc->irq[2] = irq; > > > > + for (i = 0; i < 3; i++) > > + __raw_writel(0xff, tc->regs + ATMEL_TC_REG(i, IDR)); > > + > > spin_lock(&tc_list_lock); > > list_add_tail(&tc->node, &tc_list); > > spin_unlock(&tc_list_lock); > > > > + platform_set_drvdata(pdev, tc); > > + > > return 0; > > } > > > > +static void tc_shutdown (struct platform_device *pdev) > > +{ > > + int i; > > + struct atmel_tc *tc = platform_get_drvdata(pdev); > > + > > + for (i = 0; i < 3; i++) > > + __raw_writel(0xff, tc->regs + ATMEL_TC_REG(i, IDR)); > > +} > > + > > static struct platform_driver tc_driver = { > > .driver = { > > .name = "atmel_tcb", > > .of_match_table = of_match_ptr(atmel_tcb_dt_ids), > > }, > > + .shutdown = tc_shutdown, > > }; > > > > static int __init tc_init(void) > > -- > > 1.9.3 > > > -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com ^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 3/3] ARM: at91/tclib: mask interruptions at shutdown and probe @ 2014-08-19 23:01 ` Boris BREZILLON 0 siblings, 0 replies; 34+ messages in thread From: Boris BREZILLON @ 2014-08-19 23:01 UTC (permalink / raw) To: linux-arm-kernel Hi Jean-Christophe, On Wed, 20 Aug 2014 06:11:17 +0800 Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> wrote: > Hi, > > This is a bit weird as the clock of the TC should be off and the irq free > > so this should never happened we need to investigate more why this append I may have found the source of this bug. As Gael stated, when you're kexec-ing a new kernel your previous kernel could be using the tbc_clksrc driver (and especially the clkevent device). Thus the kernel might have planned a timer event and then been asked to shutdown the machine (requested by the kexec code). In this case the AIC interrupt connected to the TC Block is disabled but not the interrupts within the TCB IP (IDR registers), possibly leaving a pending interrupt before booting the new kernel. When the tcb_clksrc driver is loaded by the new kernel it enables the interrupt line by calling setup_irq [1] while the clockevent device is not registered yet [2]. Thus the event_handler is still NULL when the AIC line connected to the TCB is unmasked. Remember that an interrupt is still pending on this HW block, which will lead to an immediate call to the ch2_irq handler, which tries to call the event_handler, which in turns is NULL because clkevent device registration has not taken place at this moment => Kernel panic. ITOH, we can't register the clkevent device before the irq handler is set up, because we should be ready to handle clkevent request at the time clockevents_config_and_register is called. This leaves two solution: 1) disable the TCB irqs (using TCB IDR registers) before calling setup_irq in the tcb_clksrc driver 2) disable the TCB irqs at the tclib level (as proposed by Gael) I prefer solution #2 because it fixes the bug for all TCB users (not just the tcb_clksrc driver). Best Regards, Boris [1]http://lxr.free-electrons.com/source/drivers/clocksource/tcb_clksrc.c#L207 [2]http://lxr.free-electrons.com/source/drivers/clocksource/tcb_clksrc.c#L211 > > Best Regards, > J. > On Aug 20, 2014, at 6:07 AM, Ga?l PORTAY <gael.portay@gmail.com> wrote: > > > > > Shutdown properly the timer counter block by masking interruptions. Otherwise, > > a segmentation may happen when kexec-ing a new kernel (see backtrace below). > > An interruption may happen before the handler is set, leading to a kernel > > segmentation fault. > > > > Furthermore, we make sure the interruptions are masked when the driver is > > initialized. This will prevent freshly kexec-ed kernel from crashing when > > launched from a kernel which does not properly mask interruptions at shutdown. > > > > The backtrace below happened after kexec-ing a new kernel, from a kernel > > that did not shut down properly leaving interruptions unmasked. > > > > Unable to handle kernel NULL pointer dereference at virtual address 00000000 > > pgd = c0004000 > > [00000000] *pgd=00000000 > > Internal error: Oops: 80000005 [#1] ARM > > Modules linked in: > > CPU: 0 PID: 1 Comm: swapper Not tainted 3.16.0+ #144 > > task: c1828aa0 ti: c182a000 task.ti: c182a000 > > PC is at 0x0 > > LR is at ch2_irq+0x28/0x30 > > pc : [<00000000>] lr : [<c01db904>] psr: 000000d3 > > sp : c182bd38 ip : c182bd48 fp : c182bd44 > > r10: c0373390 r9 : c1825b00 r8 : 60000053 > > r7 : 00000000 r6 : 00000000 r5 : 00000013 r4 : c036e800 > > r3 : 00000000 r2 : 00002004 r1 : c036e760 r0 : c036e760 > > Flags: nzcv IRQs off FIQs off Mode SVC_32 ISA ARM Segment kernel > > Control: 0005317f Table: 20004000 DAC: 00000017 > > Process swapper (pid: 1, stack limit = 0xc182a1c0) > > Stack: (0xc182bd38 to 0xc182c000) > > bd20: c182bd7c c182bd48 > > bd40: c0045430 c01db8ec 00000000 c18c6f40 c182bd74 c1825b00 c035cec4 00000000 > > bd60: c182be2c 60000053 c1825b34 00000000 c182bd94 c182bd80 c0045570 c0045408 > > bd80: 00000000 c1825b00 c182bdac c182bd98 c0047f34 c0045550 00000013 c036619c > > bda0: c182bdc4 c182bdb0 c0044da4 c0047e98 0000007f 00000013 c182bde4 c182bdc8 > > bdc0: c0009e34 c0044d8c fefff000 c0046728 60000053 ffffffff c182bdf4 c182bde8 > > bde0: c00086a8 c0009ddc c182be74 c182bdf8 c000cb80 c0008674 00000000 00000013 > > be00: 00000000 00014200 c1825b00 c036e800 00000013 c035ed98 60000053 c1825b34 > > be20: 00000000 c182be74 c182be20 c182be40 c0047994 c0046728 60000053 ffffffff > > be40: 00000013 c036e800 c182be64 c1825b00 00000013 c036e800 c035ed98 c03874bc > > be60: 00000004 c036e700 c182be94 c182be78 c004689c c0046398 c036e760 c18c6080 > > be80: 00000000 c035ed10 c182bedc c182be98 c0348b08 c004684c 0000000c c034dac8 > > bea0: 004c4b3f c028c338 c036e760 00000013 c014ecc8 c18e67e0 c035b9c0 c0348884 > > bec0: c035b9c0 c182a020 00000000 00000000 c182bf54 c182bee0 c00089fc c0348894 > > bee0: c00da51c c1ffcc78 c182bf0c c182bef8 c002d100 c002d09c c1ffcc78 00000000 > > bf00: c182bf54 c182bf10 c002d308 c0336570 c182bf3c c0334e44 00000003 00000003 > > bf20: 00000030 c0334b44 c0044d74 00000003 00000003 c034dac8 c0350a94 c0373440 > > bf40: c0373440 00000030 c182bf94 c182bf58 c0336d24 c000890c 00000003 00000003 > > bf60: c0336560 c182bf64 c182bf64 6e616e0d 00000000 c0272fc8 00000000 00000000 > > bf80: 00000000 00000000 c182bfac c182bf98 c0272fd8 c0336bd8 c182a000 00000000 > > bfa0: 00000000 c182bfb0 c00095d0 c0272fd8 00000000 00000000 00000000 00000000 > > bfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 > > bfe0: 00000000 00000000 00000000 00000000 00000013 00000000 374d27cd 33cc33e4 > > Backtrace: > > [<c01db8dc>] (ch2_irq) from [<c0045430>] (handle_irq_event_percpu+0x38/0x148) > > [<c00453f8>] (handle_irq_event_percpu) from [<c0045570>] (handle_irq_event+0x30/0x40) > > r10:00000000 r9:c1825b34 r8:60000053 r7:c182be2c r6:00000000 r5:c035cec4 > > r4:c1825b00 > > [<c0045540>] (handle_irq_event) from [<c0047f34>] (handle_fasteoi_irq+0xac/0x11c) > > r4:c1825b00 r3:00000000 > > [<c0047e88>] (handle_fasteoi_irq) from [<c0044da4>] (generic_handle_irq+0x28/0x38) > > r5:c036619c r4:00000013 > > [<c0044d7c>] (generic_handle_irq) from [<c0009e34>] (handle_IRQ+0x68/0x88) > > r4:00000013 r3:0000007f > > [<c0009dcc>] (handle_IRQ) from [<c00086a8>] (at91_aic_handle_irq+0x44/0x4c) > > r6:ffffffff r5:60000053 r4:c0046728 r3:fefff000 > > [<c0008664>] (at91_aic_handle_irq) from [<c000cb80>] (__irq_svc+0x40/0x4c) > > Exception stack(0xc182bdf8 to 0xc182be40) > > bde0: 00000000 00000013 > > be00: 00000000 00014200 c1825b00 c036e800 00000013 c035ed98 60000053 c1825b34 > > be20: 00000000 c182be74 c182be20 c182be40 c0047994 c0046728 60000053 ffffffff > > [<c0046388>] (__setup_irq) from [<c004689c>] (setup_irq+0x60/0x8c) > > r10:c036e700 r9:00000004 r8:c03874bc r7:c035ed98 r6:c036e800 r5:00000013 > > r4:c1825b00 > > [<c004683c>] (setup_irq) from [<c0348b08>] (tcb_clksrc_init+0x284/0x31c) > > r6:c035ed10 r5:00000000 r4:c18c6080 r3:c036e760 > > [<c0348884>] (tcb_clksrc_init) from [<c00089fc>] (do_one_initcall+0x100/0x1b4) > > r10:00000000 r9:00000000 r8:c182a020 r7:c035b9c0 r6:c0348884 r5:c035b9c0 > > r4:c18e67e0 > > [<c00088fc>] (do_one_initcall) from [<c0336d24>] (kernel_init_freeable+0x15c/0x224) > > r9:00000030 r8:c0373440 r7:c0373440 r6:c0350a94 r5:c034dac8 r4:00000003 > > [<c0336bc8>] (kernel_init_freeable) from [<c0272fd8>] (kernel_init+0x10/0xec) > > r9:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c0272fc8 r4:00000000 > > [<c0272fc8>] (kernel_init) from [<c00095d0>] (ret_from_fork+0x14/0x24) > > r4:00000000 r3:c182a000 > > Code: bad PC value > > ---[ end trace 5b30f0017e282e47 ]--- > > Kernel panic - not syncing: Fatal exception in interrupt > > > > Signed-off-by: Ga?l PORTAY <gael.portay@gmail.com> > > --- > > drivers/misc/atmel_tclib.c | 16 ++++++++++++++++ > > 1 file changed, 16 insertions(+) > > > > diff --git a/drivers/misc/atmel_tclib.c b/drivers/misc/atmel_tclib.c > > index d505d1e..a680151 100644 > > --- a/drivers/misc/atmel_tclib.c > > +++ b/drivers/misc/atmel_tclib.c > > @@ -109,6 +109,7 @@ static int __init tc_probe(struct platform_device *pdev) > > struct clk *clk; > > int irq; > > struct resource *r; > > + unsigned int i; > > > > irq = platform_get_irq(pdev, 0); > > if (irq < 0) > > @@ -157,18 +158,33 @@ static int __init tc_probe(struct platform_device *pdev) > > if (tc->irq[2] < 0) > > tc->irq[2] = irq; > > > > + for (i = 0; i < 3; i++) > > + __raw_writel(0xff, tc->regs + ATMEL_TC_REG(i, IDR)); > > + > > spin_lock(&tc_list_lock); > > list_add_tail(&tc->node, &tc_list); > > spin_unlock(&tc_list_lock); > > > > + platform_set_drvdata(pdev, tc); > > + > > return 0; > > } > > > > +static void tc_shutdown (struct platform_device *pdev) > > +{ > > + int i; > > + struct atmel_tc *tc = platform_get_drvdata(pdev); > > + > > + for (i = 0; i < 3; i++) > > + __raw_writel(0xff, tc->regs + ATMEL_TC_REG(i, IDR)); > > +} > > + > > static struct platform_driver tc_driver = { > > .driver = { > > .name = "atmel_tcb", > > .of_match_table = of_match_ptr(atmel_tcb_dt_ids), > > }, > > + .shutdown = tc_shutdown, > > }; > > > > static int __init tc_init(void) > > -- > > 1.9.3 > > > -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 3/3] ARM: at91/tclib: mask interruptions at shutdown and probe 2014-08-19 23:01 ` Boris BREZILLON @ 2014-08-20 7:31 ` Thierry Reding -1 siblings, 0 replies; 34+ messages in thread From: Thierry Reding @ 2014-08-20 7:31 UTC (permalink / raw) To: Boris BREZILLON Cc: Jean-Christophe PLAGNIOL-VILLARD, Gaël PORTAY, Arnd Bergmann, Daniel Lezcano, Greg Kroah-Hartman, linux-arm-kernel, linux-kernel, linux-pwm, Nicolas FERRE, Thomas Gleixner, Alexandre Belloni [-- Attachment #1: Type: text/plain, Size: 2436 bytes --] On Wed, Aug 20, 2014 at 01:01:30AM +0200, Boris BREZILLON wrote: > Hi Jean-Christophe, > > On Wed, 20 Aug 2014 06:11:17 +0800 > Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> wrote: > > > Hi, > > > > This is a bit weird as the clock of the TC should be off and the irq free > > > > so this should never happened we need to investigate more why this append > > I may have found the source of this bug. > > As Gael stated, when you're kexec-ing a new kernel your previous kernel > could be using the tbc_clksrc driver (and especially the clkevent > device). Thus the kernel might have planned a timer event and then been > asked to shutdown the machine (requested by the kexec code). > In this case the AIC interrupt connected to the TC Block is disabled > but not the interrupts within the TCB IP (IDR registers), possibly > leaving a pending interrupt before booting the new kernel. > > When the tcb_clksrc driver is loaded by the new kernel it enables the > interrupt line by calling setup_irq [1] while the clockevent device is > not registered yet [2]. Thus the event_handler is still NULL when the > AIC line connected to the TCB is unmasked. Remember that an interrupt > is still pending on this HW block, which will lead to an immediate call > to the ch2_irq handler, which tries to call the event_handler, which in > turns is NULL because clkevent device registration has not taken place > at this moment => Kernel panic. > ITOH, we can't register the clkevent device before the irq handler is > set up, because we should be ready to handle clkevent request at the > time clockevents_config_and_register is called. > > This leaves two solution: > 1) disable the TCB irqs (using TCB IDR registers) before calling > setup_irq in the tcb_clksrc driver > 2) disable the TCB irqs at the tclib level (as proposed by Gael) > > I prefer solution #2 because it fixes the bug for all TCB users (not > just the tcb_clksrc driver). Wouldn't a more proper fix be to only enable the IRQ (setup_irq()) once everything has properly been set up? That's certainly how all other drivers are doing this. Generally I think it's best to assume that an interrupt can fire at any point after it's been enabled, so everything should be set up prior to enabling it. Also, does anyone know why this driver uses setup_irq() rather than the more idiomatic request_irq()? Thierry [-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 3/3] ARM: at91/tclib: mask interruptions at shutdown and probe @ 2014-08-20 7:31 ` Thierry Reding 0 siblings, 0 replies; 34+ messages in thread From: Thierry Reding @ 2014-08-20 7:31 UTC (permalink / raw) To: linux-arm-kernel On Wed, Aug 20, 2014 at 01:01:30AM +0200, Boris BREZILLON wrote: > Hi Jean-Christophe, > > On Wed, 20 Aug 2014 06:11:17 +0800 > Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> wrote: > > > Hi, > > > > This is a bit weird as the clock of the TC should be off and the irq free > > > > so this should never happened we need to investigate more why this append > > I may have found the source of this bug. > > As Gael stated, when you're kexec-ing a new kernel your previous kernel > could be using the tbc_clksrc driver (and especially the clkevent > device). Thus the kernel might have planned a timer event and then been > asked to shutdown the machine (requested by the kexec code). > In this case the AIC interrupt connected to the TC Block is disabled > but not the interrupts within the TCB IP (IDR registers), possibly > leaving a pending interrupt before booting the new kernel. > > When the tcb_clksrc driver is loaded by the new kernel it enables the > interrupt line by calling setup_irq [1] while the clockevent device is > not registered yet [2]. Thus the event_handler is still NULL when the > AIC line connected to the TCB is unmasked. Remember that an interrupt > is still pending on this HW block, which will lead to an immediate call > to the ch2_irq handler, which tries to call the event_handler, which in > turns is NULL because clkevent device registration has not taken place > at this moment => Kernel panic. > ITOH, we can't register the clkevent device before the irq handler is > set up, because we should be ready to handle clkevent request at the > time clockevents_config_and_register is called. > > This leaves two solution: > 1) disable the TCB irqs (using TCB IDR registers) before calling > setup_irq in the tcb_clksrc driver > 2) disable the TCB irqs at the tclib level (as proposed by Gael) > > I prefer solution #2 because it fixes the bug for all TCB users (not > just the tcb_clksrc driver). Wouldn't a more proper fix be to only enable the IRQ (setup_irq()) once everything has properly been set up? That's certainly how all other drivers are doing this. Generally I think it's best to assume that an interrupt can fire at any point after it's been enabled, so everything should be set up prior to enabling it. Also, does anyone know why this driver uses setup_irq() rather than the more idiomatic request_irq()? Thierry -------------- next part -------------- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 819 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140820/67449f7a/attachment.sig> ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 3/3] ARM: at91/tclib: mask interruptions at shutdown and probe 2014-08-20 7:31 ` Thierry Reding @ 2014-08-20 8:14 ` Boris BREZILLON -1 siblings, 0 replies; 34+ messages in thread From: Boris BREZILLON @ 2014-08-20 8:14 UTC (permalink / raw) To: Thierry Reding Cc: Jean-Christophe PLAGNIOL-VILLARD, Gaël PORTAY, Arnd Bergmann, Daniel Lezcano, Greg Kroah-Hartman, linux-arm-kernel, linux-kernel, linux-pwm, Nicolas FERRE, Thomas Gleixner, Alexandre Belloni Hi Thierry, On Wed, 20 Aug 2014 09:31:13 +0200 Thierry Reding <thierry.reding@gmail.com> wrote: > On Wed, Aug 20, 2014 at 01:01:30AM +0200, Boris BREZILLON wrote: > > Hi Jean-Christophe, > > > > On Wed, 20 Aug 2014 06:11:17 +0800 > > Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> wrote: > > > > > Hi, > > > > > > This is a bit weird as the clock of the TC should be off and the irq free > > > > > > so this should never happened we need to investigate more why this append > > > > I may have found the source of this bug. > > > > As Gael stated, when you're kexec-ing a new kernel your previous kernel > > could be using the tbc_clksrc driver (and especially the clkevent > > device). Thus the kernel might have planned a timer event and then been > > asked to shutdown the machine (requested by the kexec code). > > In this case the AIC interrupt connected to the TC Block is disabled > > but not the interrupts within the TCB IP (IDR registers), possibly > > leaving a pending interrupt before booting the new kernel. > > > > When the tcb_clksrc driver is loaded by the new kernel it enables the > > interrupt line by calling setup_irq [1] while the clockevent device is > > not registered yet [2]. Thus the event_handler is still NULL when the > > AIC line connected to the TCB is unmasked. Remember that an interrupt > > is still pending on this HW block, which will lead to an immediate call > > to the ch2_irq handler, which tries to call the event_handler, which in > > turns is NULL because clkevent device registration has not taken place > > at this moment => Kernel panic. > > ITOH, we can't register the clkevent device before the irq handler is > > set up, because we should be ready to handle clkevent request at the > > time clockevents_config_and_register is called. > > > > This leaves two solution: > > 1) disable the TCB irqs (using TCB IDR registers) before calling > > setup_irq in the tcb_clksrc driver > > 2) disable the TCB irqs at the tclib level (as proposed by Gael) > > > > I prefer solution #2 because it fixes the bug for all TCB users (not > > just the tcb_clksrc driver). > > Wouldn't a more proper fix be to only enable the IRQ (setup_irq()) once > everything has properly been set up? That's certainly how all other > drivers are doing this. Generally I think it's best to assume that an > interrupt can fire at any point after it's been enabled, so everything > should be set up prior to enabling it. Sure. And, AFAIK, another common practice is to disable all interrupts and acknowledge all pending interrupts before registering a new irq handler to avoid inheriting peripheral dirty state from previous usage (either the bootloader, or the previous kernel when using kexec). This being said, I really think we should leave the HW in a clean state when shutdown is called. And disabling interrupts at the tclib level (in a shutdown callback) ensure that. > > Also, does anyone know why this driver uses setup_irq() rather than the > more idiomatic request_irq()? Because nobody has sanitized this driver yet ;-). Best Regards, Boris -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com ^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 3/3] ARM: at91/tclib: mask interruptions at shutdown and probe @ 2014-08-20 8:14 ` Boris BREZILLON 0 siblings, 0 replies; 34+ messages in thread From: Boris BREZILLON @ 2014-08-20 8:14 UTC (permalink / raw) To: linux-arm-kernel Hi Thierry, On Wed, 20 Aug 2014 09:31:13 +0200 Thierry Reding <thierry.reding@gmail.com> wrote: > On Wed, Aug 20, 2014 at 01:01:30AM +0200, Boris BREZILLON wrote: > > Hi Jean-Christophe, > > > > On Wed, 20 Aug 2014 06:11:17 +0800 > > Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> wrote: > > > > > Hi, > > > > > > This is a bit weird as the clock of the TC should be off and the irq free > > > > > > so this should never happened we need to investigate more why this append > > > > I may have found the source of this bug. > > > > As Gael stated, when you're kexec-ing a new kernel your previous kernel > > could be using the tbc_clksrc driver (and especially the clkevent > > device). Thus the kernel might have planned a timer event and then been > > asked to shutdown the machine (requested by the kexec code). > > In this case the AIC interrupt connected to the TC Block is disabled > > but not the interrupts within the TCB IP (IDR registers), possibly > > leaving a pending interrupt before booting the new kernel. > > > > When the tcb_clksrc driver is loaded by the new kernel it enables the > > interrupt line by calling setup_irq [1] while the clockevent device is > > not registered yet [2]. Thus the event_handler is still NULL when the > > AIC line connected to the TCB is unmasked. Remember that an interrupt > > is still pending on this HW block, which will lead to an immediate call > > to the ch2_irq handler, which tries to call the event_handler, which in > > turns is NULL because clkevent device registration has not taken place > > at this moment => Kernel panic. > > ITOH, we can't register the clkevent device before the irq handler is > > set up, because we should be ready to handle clkevent request at the > > time clockevents_config_and_register is called. > > > > This leaves two solution: > > 1) disable the TCB irqs (using TCB IDR registers) before calling > > setup_irq in the tcb_clksrc driver > > 2) disable the TCB irqs at the tclib level (as proposed by Gael) > > > > I prefer solution #2 because it fixes the bug for all TCB users (not > > just the tcb_clksrc driver). > > Wouldn't a more proper fix be to only enable the IRQ (setup_irq()) once > everything has properly been set up? That's certainly how all other > drivers are doing this. Generally I think it's best to assume that an > interrupt can fire at any point after it's been enabled, so everything > should be set up prior to enabling it. Sure. And, AFAIK, another common practice is to disable all interrupts and acknowledge all pending interrupts before registering a new irq handler to avoid inheriting peripheral dirty state from previous usage (either the bootloader, or the previous kernel when using kexec). This being said, I really think we should leave the HW in a clean state when shutdown is called. And disabling interrupts at the tclib level (in a shutdown callback) ensure that. > > Also, does anyone know why this driver uses setup_irq() rather than the > more idiomatic request_irq()? Because nobody has sanitized this driver yet ;-). Best Regards, Boris -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 3/3] ARM: at91/tclib: mask interruptions at shutdown and probe 2014-08-20 8:14 ` Boris BREZILLON @ 2014-08-20 8:28 ` Thierry Reding -1 siblings, 0 replies; 34+ messages in thread From: Thierry Reding @ 2014-08-20 8:28 UTC (permalink / raw) To: Boris BREZILLON Cc: Jean-Christophe PLAGNIOL-VILLARD, Gaël PORTAY, Arnd Bergmann, Daniel Lezcano, Greg Kroah-Hartman, linux-arm-kernel, linux-kernel, linux-pwm, Nicolas FERRE, Thomas Gleixner, Alexandre Belloni [-- Attachment #1: Type: text/plain, Size: 3558 bytes --] On Wed, Aug 20, 2014 at 10:14:22AM +0200, Boris BREZILLON wrote: > Hi Thierry, > > On Wed, 20 Aug 2014 09:31:13 +0200 > Thierry Reding <thierry.reding@gmail.com> wrote: > > > On Wed, Aug 20, 2014 at 01:01:30AM +0200, Boris BREZILLON wrote: > > > Hi Jean-Christophe, > > > > > > On Wed, 20 Aug 2014 06:11:17 +0800 > > > Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> wrote: > > > > > > > Hi, > > > > > > > > This is a bit weird as the clock of the TC should be off and the irq free > > > > > > > > so this should never happened we need to investigate more why this append > > > > > > I may have found the source of this bug. > > > > > > As Gael stated, when you're kexec-ing a new kernel your previous kernel > > > could be using the tbc_clksrc driver (and especially the clkevent > > > device). Thus the kernel might have planned a timer event and then been > > > asked to shutdown the machine (requested by the kexec code). > > > In this case the AIC interrupt connected to the TC Block is disabled > > > but not the interrupts within the TCB IP (IDR registers), possibly > > > leaving a pending interrupt before booting the new kernel. > > > > > > When the tcb_clksrc driver is loaded by the new kernel it enables the > > > interrupt line by calling setup_irq [1] while the clockevent device is > > > not registered yet [2]. Thus the event_handler is still NULL when the > > > AIC line connected to the TCB is unmasked. Remember that an interrupt > > > is still pending on this HW block, which will lead to an immediate call > > > to the ch2_irq handler, which tries to call the event_handler, which in > > > turns is NULL because clkevent device registration has not taken place > > > at this moment => Kernel panic. > > > ITOH, we can't register the clkevent device before the irq handler is > > > set up, because we should be ready to handle clkevent request at the > > > time clockevents_config_and_register is called. > > > > > > This leaves two solution: > > > 1) disable the TCB irqs (using TCB IDR registers) before calling > > > setup_irq in the tcb_clksrc driver > > > 2) disable the TCB irqs at the tclib level (as proposed by Gael) > > > > > > I prefer solution #2 because it fixes the bug for all TCB users (not > > > just the tcb_clksrc driver). > > > > Wouldn't a more proper fix be to only enable the IRQ (setup_irq()) once > > everything has properly been set up? That's certainly how all other > > drivers are doing this. Generally I think it's best to assume that an > > interrupt can fire at any point after it's been enabled, so everything > > should be set up prior to enabling it. > > Sure. And, AFAIK, another common practice is to disable all interrupts > and acknowledge all pending interrupts before registering a new irq > handler to avoid inheriting peripheral dirty state from previous usage > (either the bootloader, or the previous kernel when using kexec). Discarding all pending interrupts may not always be what we want. And masking interrupts prior to registering the handler isn't always going to work either (shared interrupts), so device drivers should always set things up in the correct order. > This being said, I really think we should leave the HW in a clean state > when shutdown is called. And disabling interrupts at the tclib level > (in a shutdown callback) ensure that. Yes, cleaning up the hardware and disabling interrupts upon shutdown seems like a good idea to me in addition to the above. Thierry [-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 3/3] ARM: at91/tclib: mask interruptions at shutdown and probe @ 2014-08-20 8:28 ` Thierry Reding 0 siblings, 0 replies; 34+ messages in thread From: Thierry Reding @ 2014-08-20 8:28 UTC (permalink / raw) To: linux-arm-kernel On Wed, Aug 20, 2014 at 10:14:22AM +0200, Boris BREZILLON wrote: > Hi Thierry, > > On Wed, 20 Aug 2014 09:31:13 +0200 > Thierry Reding <thierry.reding@gmail.com> wrote: > > > On Wed, Aug 20, 2014 at 01:01:30AM +0200, Boris BREZILLON wrote: > > > Hi Jean-Christophe, > > > > > > On Wed, 20 Aug 2014 06:11:17 +0800 > > > Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> wrote: > > > > > > > Hi, > > > > > > > > This is a bit weird as the clock of the TC should be off and the irq free > > > > > > > > so this should never happened we need to investigate more why this append > > > > > > I may have found the source of this bug. > > > > > > As Gael stated, when you're kexec-ing a new kernel your previous kernel > > > could be using the tbc_clksrc driver (and especially the clkevent > > > device). Thus the kernel might have planned a timer event and then been > > > asked to shutdown the machine (requested by the kexec code). > > > In this case the AIC interrupt connected to the TC Block is disabled > > > but not the interrupts within the TCB IP (IDR registers), possibly > > > leaving a pending interrupt before booting the new kernel. > > > > > > When the tcb_clksrc driver is loaded by the new kernel it enables the > > > interrupt line by calling setup_irq [1] while the clockevent device is > > > not registered yet [2]. Thus the event_handler is still NULL when the > > > AIC line connected to the TCB is unmasked. Remember that an interrupt > > > is still pending on this HW block, which will lead to an immediate call > > > to the ch2_irq handler, which tries to call the event_handler, which in > > > turns is NULL because clkevent device registration has not taken place > > > at this moment => Kernel panic. > > > ITOH, we can't register the clkevent device before the irq handler is > > > set up, because we should be ready to handle clkevent request at the > > > time clockevents_config_and_register is called. > > > > > > This leaves two solution: > > > 1) disable the TCB irqs (using TCB IDR registers) before calling > > > setup_irq in the tcb_clksrc driver > > > 2) disable the TCB irqs at the tclib level (as proposed by Gael) > > > > > > I prefer solution #2 because it fixes the bug for all TCB users (not > > > just the tcb_clksrc driver). > > > > Wouldn't a more proper fix be to only enable the IRQ (setup_irq()) once > > everything has properly been set up? That's certainly how all other > > drivers are doing this. Generally I think it's best to assume that an > > interrupt can fire at any point after it's been enabled, so everything > > should be set up prior to enabling it. > > Sure. And, AFAIK, another common practice is to disable all interrupts > and acknowledge all pending interrupts before registering a new irq > handler to avoid inheriting peripheral dirty state from previous usage > (either the bootloader, or the previous kernel when using kexec). Discarding all pending interrupts may not always be what we want. And masking interrupts prior to registering the handler isn't always going to work either (shared interrupts), so device drivers should always set things up in the correct order. > This being said, I really think we should leave the HW in a clean state > when shutdown is called. And disabling interrupts at the tclib level > (in a shutdown callback) ensure that. Yes, cleaning up the hardware and disabling interrupts upon shutdown seems like a good idea to me in addition to the above. Thierry -------------- next part -------------- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 819 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140820/d2002450/attachment-0001.sig> ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 3/3] ARM: at91/tclib: mask interruptions at shutdown and probe 2014-08-20 8:28 ` Thierry Reding @ 2014-08-20 9:06 ` Boris BREZILLON -1 siblings, 0 replies; 34+ messages in thread From: Boris BREZILLON @ 2014-08-20 9:06 UTC (permalink / raw) To: Thierry Reding Cc: Jean-Christophe PLAGNIOL-VILLARD, Gaël PORTAY, Arnd Bergmann, Daniel Lezcano, Greg Kroah-Hartman, linux-arm-kernel, linux-kernel, linux-pwm, Nicolas FERRE, Thomas Gleixner, Alexandre Belloni On Wed, 20 Aug 2014 10:28:20 +0200 Thierry Reding <thierry.reding@gmail.com> wrote: > On Wed, Aug 20, 2014 at 10:14:22AM +0200, Boris BREZILLON wrote: > > Hi Thierry, > > > > On Wed, 20 Aug 2014 09:31:13 +0200 > > Thierry Reding <thierry.reding@gmail.com> wrote: > > > > > On Wed, Aug 20, 2014 at 01:01:30AM +0200, Boris BREZILLON wrote: > > > > Hi Jean-Christophe, > > > > > > > > On Wed, 20 Aug 2014 06:11:17 +0800 > > > > Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> wrote: > > > > > > > > > Hi, > > > > > > > > > > This is a bit weird as the clock of the TC should be off and the irq free > > > > > > > > > > so this should never happened we need to investigate more why this append > > > > > > > > I may have found the source of this bug. > > > > > > > > As Gael stated, when you're kexec-ing a new kernel your previous kernel > > > > could be using the tbc_clksrc driver (and especially the clkevent > > > > device). Thus the kernel might have planned a timer event and then been > > > > asked to shutdown the machine (requested by the kexec code). > > > > In this case the AIC interrupt connected to the TC Block is disabled > > > > but not the interrupts within the TCB IP (IDR registers), possibly > > > > leaving a pending interrupt before booting the new kernel. > > > > > > > > When the tcb_clksrc driver is loaded by the new kernel it enables the > > > > interrupt line by calling setup_irq [1] while the clockevent device is > > > > not registered yet [2]. Thus the event_handler is still NULL when the > > > > AIC line connected to the TCB is unmasked. Remember that an interrupt > > > > is still pending on this HW block, which will lead to an immediate call > > > > to the ch2_irq handler, which tries to call the event_handler, which in > > > > turns is NULL because clkevent device registration has not taken place > > > > at this moment => Kernel panic. > > > > ITOH, we can't register the clkevent device before the irq handler is > > > > set up, because we should be ready to handle clkevent request at the > > > > time clockevents_config_and_register is called. > > > > > > > > This leaves two solution: > > > > 1) disable the TCB irqs (using TCB IDR registers) before calling > > > > setup_irq in the tcb_clksrc driver > > > > 2) disable the TCB irqs at the tclib level (as proposed by Gael) > > > > > > > > I prefer solution #2 because it fixes the bug for all TCB users (not > > > > just the tcb_clksrc driver). > > > > > > Wouldn't a more proper fix be to only enable the IRQ (setup_irq()) once > > > everything has properly been set up? That's certainly how all other > > > drivers are doing this. Generally I think it's best to assume that an > > > interrupt can fire at any point after it's been enabled, so everything > > > should be set up prior to enabling it. > > > > Sure. And, AFAIK, another common practice is to disable all interrupts > > and acknowledge all pending interrupts before registering a new irq > > handler to avoid inheriting peripheral dirty state from previous usage > > (either the bootloader, or the previous kernel when using kexec). > > Discarding all pending interrupts may not always be what we want. And > masking interrupts prior to registering the handler isn't always going > to work either (shared interrupts), so device drivers should always set > things up in the correct order. > I meant disabling/acknowledging interrupts within the HW block not the interrupt line connected to the interrupt controller (which indeed can be shared among several peripherals). The TCB IP provides SR (Status Register) to acknowledge interrupts at the TCB level and IER/IDR/ISR (Interrupt Enable/Disable/Status Register) to manipulate TCB interrupts. > > This being said, I really think we should leave the HW in a clean state > > when shutdown is called. And disabling interrupts at the tclib level > > (in a shutdown callback) ensure that. > > Yes, cleaning up the hardware and disabling interrupts upon shutdown > seems like a good idea to me in addition to the above. > > Thierry -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com ^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 3/3] ARM: at91/tclib: mask interruptions at shutdown and probe @ 2014-08-20 9:06 ` Boris BREZILLON 0 siblings, 0 replies; 34+ messages in thread From: Boris BREZILLON @ 2014-08-20 9:06 UTC (permalink / raw) To: linux-arm-kernel On Wed, 20 Aug 2014 10:28:20 +0200 Thierry Reding <thierry.reding@gmail.com> wrote: > On Wed, Aug 20, 2014 at 10:14:22AM +0200, Boris BREZILLON wrote: > > Hi Thierry, > > > > On Wed, 20 Aug 2014 09:31:13 +0200 > > Thierry Reding <thierry.reding@gmail.com> wrote: > > > > > On Wed, Aug 20, 2014 at 01:01:30AM +0200, Boris BREZILLON wrote: > > > > Hi Jean-Christophe, > > > > > > > > On Wed, 20 Aug 2014 06:11:17 +0800 > > > > Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> wrote: > > > > > > > > > Hi, > > > > > > > > > > This is a bit weird as the clock of the TC should be off and the irq free > > > > > > > > > > so this should never happened we need to investigate more why this append > > > > > > > > I may have found the source of this bug. > > > > > > > > As Gael stated, when you're kexec-ing a new kernel your previous kernel > > > > could be using the tbc_clksrc driver (and especially the clkevent > > > > device). Thus the kernel might have planned a timer event and then been > > > > asked to shutdown the machine (requested by the kexec code). > > > > In this case the AIC interrupt connected to the TC Block is disabled > > > > but not the interrupts within the TCB IP (IDR registers), possibly > > > > leaving a pending interrupt before booting the new kernel. > > > > > > > > When the tcb_clksrc driver is loaded by the new kernel it enables the > > > > interrupt line by calling setup_irq [1] while the clockevent device is > > > > not registered yet [2]. Thus the event_handler is still NULL when the > > > > AIC line connected to the TCB is unmasked. Remember that an interrupt > > > > is still pending on this HW block, which will lead to an immediate call > > > > to the ch2_irq handler, which tries to call the event_handler, which in > > > > turns is NULL because clkevent device registration has not taken place > > > > at this moment => Kernel panic. > > > > ITOH, we can't register the clkevent device before the irq handler is > > > > set up, because we should be ready to handle clkevent request at the > > > > time clockevents_config_and_register is called. > > > > > > > > This leaves two solution: > > > > 1) disable the TCB irqs (using TCB IDR registers) before calling > > > > setup_irq in the tcb_clksrc driver > > > > 2) disable the TCB irqs at the tclib level (as proposed by Gael) > > > > > > > > I prefer solution #2 because it fixes the bug for all TCB users (not > > > > just the tcb_clksrc driver). > > > > > > Wouldn't a more proper fix be to only enable the IRQ (setup_irq()) once > > > everything has properly been set up? That's certainly how all other > > > drivers are doing this. Generally I think it's best to assume that an > > > interrupt can fire at any point after it's been enabled, so everything > > > should be set up prior to enabling it. > > > > Sure. And, AFAIK, another common practice is to disable all interrupts > > and acknowledge all pending interrupts before registering a new irq > > handler to avoid inheriting peripheral dirty state from previous usage > > (either the bootloader, or the previous kernel when using kexec). > > Discarding all pending interrupts may not always be what we want. And > masking interrupts prior to registering the handler isn't always going > to work either (shared interrupts), so device drivers should always set > things up in the correct order. > I meant disabling/acknowledging interrupts within the HW block not the interrupt line connected to the interrupt controller (which indeed can be shared among several peripherals). The TCB IP provides SR (Status Register) to acknowledge interrupts at the TCB level and IER/IDR/ISR (Interrupt Enable/Disable/Status Register) to manipulate TCB interrupts. > > This being said, I really think we should leave the HW in a clean state > > when shutdown is called. And disabling interrupts at the tclib level > > (in a shutdown callback) ensure that. > > Yes, cleaning up the hardware and disabling interrupts upon shutdown > seems like a good idea to me in addition to the above. > > Thierry -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 3/3] ARM: at91/tclib: mask interruptions at shutdown and probe 2014-08-20 9:06 ` Boris BREZILLON @ 2014-08-20 9:48 ` Thierry Reding -1 siblings, 0 replies; 34+ messages in thread From: Thierry Reding @ 2014-08-20 9:48 UTC (permalink / raw) To: Boris BREZILLON Cc: Jean-Christophe PLAGNIOL-VILLARD, Gaël PORTAY, Arnd Bergmann, Daniel Lezcano, Greg Kroah-Hartman, linux-arm-kernel, linux-kernel, linux-pwm, Nicolas FERRE, Thomas Gleixner, Alexandre Belloni [-- Attachment #1: Type: text/plain, Size: 4180 bytes --] On Wed, Aug 20, 2014 at 11:06:25AM +0200, Boris BREZILLON wrote: > On Wed, 20 Aug 2014 10:28:20 +0200 > Thierry Reding <thierry.reding@gmail.com> wrote: > > > On Wed, Aug 20, 2014 at 10:14:22AM +0200, Boris BREZILLON wrote: > > > Hi Thierry, > > > > > > On Wed, 20 Aug 2014 09:31:13 +0200 > > > Thierry Reding <thierry.reding@gmail.com> wrote: > > > > > > > On Wed, Aug 20, 2014 at 01:01:30AM +0200, Boris BREZILLON wrote: > > > > > Hi Jean-Christophe, > > > > > > > > > > On Wed, 20 Aug 2014 06:11:17 +0800 > > > > > Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> wrote: > > > > > > > > > > > Hi, > > > > > > > > > > > > This is a bit weird as the clock of the TC should be off and the irq free > > > > > > > > > > > > so this should never happened we need to investigate more why this append > > > > > > > > > > I may have found the source of this bug. > > > > > > > > > > As Gael stated, when you're kexec-ing a new kernel your previous kernel > > > > > could be using the tbc_clksrc driver (and especially the clkevent > > > > > device). Thus the kernel might have planned a timer event and then been > > > > > asked to shutdown the machine (requested by the kexec code). > > > > > In this case the AIC interrupt connected to the TC Block is disabled > > > > > but not the interrupts within the TCB IP (IDR registers), possibly > > > > > leaving a pending interrupt before booting the new kernel. > > > > > > > > > > When the tcb_clksrc driver is loaded by the new kernel it enables the > > > > > interrupt line by calling setup_irq [1] while the clockevent device is > > > > > not registered yet [2]. Thus the event_handler is still NULL when the > > > > > AIC line connected to the TCB is unmasked. Remember that an interrupt > > > > > is still pending on this HW block, which will lead to an immediate call > > > > > to the ch2_irq handler, which tries to call the event_handler, which in > > > > > turns is NULL because clkevent device registration has not taken place > > > > > at this moment => Kernel panic. > > > > > ITOH, we can't register the clkevent device before the irq handler is > > > > > set up, because we should be ready to handle clkevent request at the > > > > > time clockevents_config_and_register is called. > > > > > > > > > > This leaves two solution: > > > > > 1) disable the TCB irqs (using TCB IDR registers) before calling > > > > > setup_irq in the tcb_clksrc driver > > > > > 2) disable the TCB irqs at the tclib level (as proposed by Gael) > > > > > > > > > > I prefer solution #2 because it fixes the bug for all TCB users (not > > > > > just the tcb_clksrc driver). > > > > > > > > Wouldn't a more proper fix be to only enable the IRQ (setup_irq()) once > > > > everything has properly been set up? That's certainly how all other > > > > drivers are doing this. Generally I think it's best to assume that an > > > > interrupt can fire at any point after it's been enabled, so everything > > > > should be set up prior to enabling it. > > > > > > Sure. And, AFAIK, another common practice is to disable all interrupts > > > and acknowledge all pending interrupts before registering a new irq > > > handler to avoid inheriting peripheral dirty state from previous usage > > > (either the bootloader, or the previous kernel when using kexec). > > > > Discarding all pending interrupts may not always be what we want. And > > masking interrupts prior to registering the handler isn't always going > > to work either (shared interrupts), so device drivers should always set > > things up in the correct order. > > > > I meant disabling/acknowledging interrupts within the HW block not > the interrupt line connected to the interrupt controller (which indeed > can be shared among several peripherals). > The TCB IP provides SR (Status Register) to acknowledge interrupts at > the TCB level and IER/IDR/ISR (Interrupt Enable/Disable/Status > Register) to manipulate TCB interrupts. But when you share interrupts, then when an incoming interrupt will cause all handlers to be called, so you still need to set it up properly. Thierry [-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 3/3] ARM: at91/tclib: mask interruptions at shutdown and probe @ 2014-08-20 9:48 ` Thierry Reding 0 siblings, 0 replies; 34+ messages in thread From: Thierry Reding @ 2014-08-20 9:48 UTC (permalink / raw) To: linux-arm-kernel On Wed, Aug 20, 2014 at 11:06:25AM +0200, Boris BREZILLON wrote: > On Wed, 20 Aug 2014 10:28:20 +0200 > Thierry Reding <thierry.reding@gmail.com> wrote: > > > On Wed, Aug 20, 2014 at 10:14:22AM +0200, Boris BREZILLON wrote: > > > Hi Thierry, > > > > > > On Wed, 20 Aug 2014 09:31:13 +0200 > > > Thierry Reding <thierry.reding@gmail.com> wrote: > > > > > > > On Wed, Aug 20, 2014 at 01:01:30AM +0200, Boris BREZILLON wrote: > > > > > Hi Jean-Christophe, > > > > > > > > > > On Wed, 20 Aug 2014 06:11:17 +0800 > > > > > Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> wrote: > > > > > > > > > > > Hi, > > > > > > > > > > > > This is a bit weird as the clock of the TC should be off and the irq free > > > > > > > > > > > > so this should never happened we need to investigate more why this append > > > > > > > > > > I may have found the source of this bug. > > > > > > > > > > As Gael stated, when you're kexec-ing a new kernel your previous kernel > > > > > could be using the tbc_clksrc driver (and especially the clkevent > > > > > device). Thus the kernel might have planned a timer event and then been > > > > > asked to shutdown the machine (requested by the kexec code). > > > > > In this case the AIC interrupt connected to the TC Block is disabled > > > > > but not the interrupts within the TCB IP (IDR registers), possibly > > > > > leaving a pending interrupt before booting the new kernel. > > > > > > > > > > When the tcb_clksrc driver is loaded by the new kernel it enables the > > > > > interrupt line by calling setup_irq [1] while the clockevent device is > > > > > not registered yet [2]. Thus the event_handler is still NULL when the > > > > > AIC line connected to the TCB is unmasked. Remember that an interrupt > > > > > is still pending on this HW block, which will lead to an immediate call > > > > > to the ch2_irq handler, which tries to call the event_handler, which in > > > > > turns is NULL because clkevent device registration has not taken place > > > > > at this moment => Kernel panic. > > > > > ITOH, we can't register the clkevent device before the irq handler is > > > > > set up, because we should be ready to handle clkevent request at the > > > > > time clockevents_config_and_register is called. > > > > > > > > > > This leaves two solution: > > > > > 1) disable the TCB irqs (using TCB IDR registers) before calling > > > > > setup_irq in the tcb_clksrc driver > > > > > 2) disable the TCB irqs at the tclib level (as proposed by Gael) > > > > > > > > > > I prefer solution #2 because it fixes the bug for all TCB users (not > > > > > just the tcb_clksrc driver). > > > > > > > > Wouldn't a more proper fix be to only enable the IRQ (setup_irq()) once > > > > everything has properly been set up? That's certainly how all other > > > > drivers are doing this. Generally I think it's best to assume that an > > > > interrupt can fire at any point after it's been enabled, so everything > > > > should be set up prior to enabling it. > > > > > > Sure. And, AFAIK, another common practice is to disable all interrupts > > > and acknowledge all pending interrupts before registering a new irq > > > handler to avoid inheriting peripheral dirty state from previous usage > > > (either the bootloader, or the previous kernel when using kexec). > > > > Discarding all pending interrupts may not always be what we want. And > > masking interrupts prior to registering the handler isn't always going > > to work either (shared interrupts), so device drivers should always set > > things up in the correct order. > > > > I meant disabling/acknowledging interrupts within the HW block not > the interrupt line connected to the interrupt controller (which indeed > can be shared among several peripherals). > The TCB IP provides SR (Status Register) to acknowledge interrupts at > the TCB level and IER/IDR/ISR (Interrupt Enable/Disable/Status > Register) to manipulate TCB interrupts. But when you share interrupts, then when an incoming interrupt will cause all handlers to be called, so you still need to set it up properly. Thierry -------------- next part -------------- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 819 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140820/bf5af537/attachment-0001.sig> ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 3/3] ARM: at91/tclib: mask interruptions at shutdown and probe 2014-08-20 9:48 ` Thierry Reding @ 2014-08-20 10:07 ` Boris BREZILLON -1 siblings, 0 replies; 34+ messages in thread From: Boris BREZILLON @ 2014-08-20 10:07 UTC (permalink / raw) To: Thierry Reding Cc: Jean-Christophe PLAGNIOL-VILLARD, Gaël PORTAY, Arnd Bergmann, Daniel Lezcano, Greg Kroah-Hartman, linux-arm-kernel, linux-kernel, linux-pwm, Nicolas FERRE, Thomas Gleixner, Alexandre Belloni On Wed, 20 Aug 2014 11:48:08 +0200 Thierry Reding <thierry.reding@gmail.com> wrote: > On Wed, Aug 20, 2014 at 11:06:25AM +0200, Boris BREZILLON wrote: > > On Wed, 20 Aug 2014 10:28:20 +0200 > > Thierry Reding <thierry.reding@gmail.com> wrote: > > > > > On Wed, Aug 20, 2014 at 10:14:22AM +0200, Boris BREZILLON wrote: > > > > Hi Thierry, > > > > > > > > On Wed, 20 Aug 2014 09:31:13 +0200 > > > > Thierry Reding <thierry.reding@gmail.com> wrote: > > > > > > > > > On Wed, Aug 20, 2014 at 01:01:30AM +0200, Boris BREZILLON wrote: > > > > > > Hi Jean-Christophe, > > > > > > > > > > > > On Wed, 20 Aug 2014 06:11:17 +0800 > > > > > > Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> wrote: > > > > > > > > > > > > > Hi, > > > > > > > > > > > > > > This is a bit weird as the clock of the TC should be off and the irq free > > > > > > > > > > > > > > so this should never happened we need to investigate more why this append > > > > > > > > > > > > I may have found the source of this bug. > > > > > > > > > > > > As Gael stated, when you're kexec-ing a new kernel your previous kernel > > > > > > could be using the tbc_clksrc driver (and especially the clkevent > > > > > > device). Thus the kernel might have planned a timer event and then been > > > > > > asked to shutdown the machine (requested by the kexec code). > > > > > > In this case the AIC interrupt connected to the TC Block is disabled > > > > > > but not the interrupts within the TCB IP (IDR registers), possibly > > > > > > leaving a pending interrupt before booting the new kernel. > > > > > > > > > > > > When the tcb_clksrc driver is loaded by the new kernel it enables the > > > > > > interrupt line by calling setup_irq [1] while the clockevent device is > > > > > > not registered yet [2]. Thus the event_handler is still NULL when the > > > > > > AIC line connected to the TCB is unmasked. Remember that an interrupt > > > > > > is still pending on this HW block, which will lead to an immediate call > > > > > > to the ch2_irq handler, which tries to call the event_handler, which in > > > > > > turns is NULL because clkevent device registration has not taken place > > > > > > at this moment => Kernel panic. > > > > > > ITOH, we can't register the clkevent device before the irq handler is > > > > > > set up, because we should be ready to handle clkevent request at the > > > > > > time clockevents_config_and_register is called. > > > > > > > > > > > > This leaves two solution: > > > > > > 1) disable the TCB irqs (using TCB IDR registers) before calling > > > > > > setup_irq in the tcb_clksrc driver > > > > > > 2) disable the TCB irqs at the tclib level (as proposed by Gael) > > > > > > > > > > > > I prefer solution #2 because it fixes the bug for all TCB users (not > > > > > > just the tcb_clksrc driver). > > > > > > > > > > Wouldn't a more proper fix be to only enable the IRQ (setup_irq()) once > > > > > everything has properly been set up? That's certainly how all other > > > > > drivers are doing this. Generally I think it's best to assume that an > > > > > interrupt can fire at any point after it's been enabled, so everything > > > > > should be set up prior to enabling it. > > > > > > > > Sure. And, AFAIK, another common practice is to disable all interrupts > > > > and acknowledge all pending interrupts before registering a new irq > > > > handler to avoid inheriting peripheral dirty state from previous usage > > > > (either the bootloader, or the previous kernel when using kexec). > > > > > > Discarding all pending interrupts may not always be what we want. And > > > masking interrupts prior to registering the handler isn't always going > > > to work either (shared interrupts), so device drivers should always set > > > things up in the correct order. > > > > > > > I meant disabling/acknowledging interrupts within the HW block not > > the interrupt line connected to the interrupt controller (which indeed > > can be shared among several peripherals). > > The TCB IP provides SR (Status Register) to acknowledge interrupts at > > the TCB level and IER/IDR/ISR (Interrupt Enable/Disable/Status > > Register) to manipulate TCB interrupts. > > But when you share interrupts, then when an incoming interrupt will > cause all handlers to be called, so you still need to set it up > properly. Right, I forgot about that one (even if we could mask the status register with the interrupt status register to avoid calling the event_handler when the interrupt is not enabled). Anyway I agree with you on this point: everything should be ready when calling request_irq. -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com ^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 3/3] ARM: at91/tclib: mask interruptions at shutdown and probe @ 2014-08-20 10:07 ` Boris BREZILLON 0 siblings, 0 replies; 34+ messages in thread From: Boris BREZILLON @ 2014-08-20 10:07 UTC (permalink / raw) To: linux-arm-kernel On Wed, 20 Aug 2014 11:48:08 +0200 Thierry Reding <thierry.reding@gmail.com> wrote: > On Wed, Aug 20, 2014 at 11:06:25AM +0200, Boris BREZILLON wrote: > > On Wed, 20 Aug 2014 10:28:20 +0200 > > Thierry Reding <thierry.reding@gmail.com> wrote: > > > > > On Wed, Aug 20, 2014 at 10:14:22AM +0200, Boris BREZILLON wrote: > > > > Hi Thierry, > > > > > > > > On Wed, 20 Aug 2014 09:31:13 +0200 > > > > Thierry Reding <thierry.reding@gmail.com> wrote: > > > > > > > > > On Wed, Aug 20, 2014 at 01:01:30AM +0200, Boris BREZILLON wrote: > > > > > > Hi Jean-Christophe, > > > > > > > > > > > > On Wed, 20 Aug 2014 06:11:17 +0800 > > > > > > Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> wrote: > > > > > > > > > > > > > Hi, > > > > > > > > > > > > > > This is a bit weird as the clock of the TC should be off and the irq free > > > > > > > > > > > > > > so this should never happened we need to investigate more why this append > > > > > > > > > > > > I may have found the source of this bug. > > > > > > > > > > > > As Gael stated, when you're kexec-ing a new kernel your previous kernel > > > > > > could be using the tbc_clksrc driver (and especially the clkevent > > > > > > device). Thus the kernel might have planned a timer event and then been > > > > > > asked to shutdown the machine (requested by the kexec code). > > > > > > In this case the AIC interrupt connected to the TC Block is disabled > > > > > > but not the interrupts within the TCB IP (IDR registers), possibly > > > > > > leaving a pending interrupt before booting the new kernel. > > > > > > > > > > > > When the tcb_clksrc driver is loaded by the new kernel it enables the > > > > > > interrupt line by calling setup_irq [1] while the clockevent device is > > > > > > not registered yet [2]. Thus the event_handler is still NULL when the > > > > > > AIC line connected to the TCB is unmasked. Remember that an interrupt > > > > > > is still pending on this HW block, which will lead to an immediate call > > > > > > to the ch2_irq handler, which tries to call the event_handler, which in > > > > > > turns is NULL because clkevent device registration has not taken place > > > > > > at this moment => Kernel panic. > > > > > > ITOH, we can't register the clkevent device before the irq handler is > > > > > > set up, because we should be ready to handle clkevent request at the > > > > > > time clockevents_config_and_register is called. > > > > > > > > > > > > This leaves two solution: > > > > > > 1) disable the TCB irqs (using TCB IDR registers) before calling > > > > > > setup_irq in the tcb_clksrc driver > > > > > > 2) disable the TCB irqs at the tclib level (as proposed by Gael) > > > > > > > > > > > > I prefer solution #2 because it fixes the bug for all TCB users (not > > > > > > just the tcb_clksrc driver). > > > > > > > > > > Wouldn't a more proper fix be to only enable the IRQ (setup_irq()) once > > > > > everything has properly been set up? That's certainly how all other > > > > > drivers are doing this. Generally I think it's best to assume that an > > > > > interrupt can fire at any point after it's been enabled, so everything > > > > > should be set up prior to enabling it. > > > > > > > > Sure. And, AFAIK, another common practice is to disable all interrupts > > > > and acknowledge all pending interrupts before registering a new irq > > > > handler to avoid inheriting peripheral dirty state from previous usage > > > > (either the bootloader, or the previous kernel when using kexec). > > > > > > Discarding all pending interrupts may not always be what we want. And > > > masking interrupts prior to registering the handler isn't always going > > > to work either (shared interrupts), so device drivers should always set > > > things up in the correct order. > > > > > > > I meant disabling/acknowledging interrupts within the HW block not > > the interrupt line connected to the interrupt controller (which indeed > > can be shared among several peripherals). > > The TCB IP provides SR (Status Register) to acknowledge interrupts at > > the TCB level and IER/IDR/ISR (Interrupt Enable/Disable/Status > > Register) to manipulate TCB interrupts. > > But when you share interrupts, then when an incoming interrupt will > cause all handlers to be called, so you still need to set it up > properly. Right, I forgot about that one (even if we could mask the status register with the interrupt status register to avoid calling the event_handler when the interrupt is not enabled). Anyway I agree with you on this point: everything should be ready when calling request_irq. -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 3/3] ARM: at91/tclib: mask interruptions at shutdown and probe 2014-08-20 7:31 ` Thierry Reding (?) (?) @ 2014-08-27 15:27 ` Gaël PORTAY -1 siblings, 0 replies; 34+ messages in thread From: Gaël PORTAY @ 2014-08-27 15:27 UTC (permalink / raw) To: Thierry Reding Cc: Boris BREZILLON, Jean-Christophe PLAGNIOL-VILLARD, Arnd Bergmann, Daniel Lezcano, Greg Kroah-Hartman, linux-arm-kernel, linux-kernel, linux-pwm, Nicolas FERRE, Thomas Gleixner, Alexandre Belloni [-- Attachment #1: Type: text/plain, Size: 1306 bytes --] On 20 August 2014 09:31, Thierry Reding <thierry.reding@gmail.com> wrote: > Wouldn't a more proper fix be to only enable the IRQ (setup_irq()) once > everything has properly been set up? That's certainly how all other > drivers are doing this. Generally I think it's best to assume that an > interrupt can fire at any point after it's been enabled, so everything > should be set up prior to enabling it. > I think we cannot move the IRQ request after the clock registration... I did not find any clockevents_DEconfig_and_UNregister function... so if the IRQ request fails, it will not be possible to unregister from the clock events. It would be preferable to leave the IRQ request before the clock registration, is not it ? > Also, does anyone know why this driver uses setup_irq() rather than the > more idiomatic request_irq()? > I took care of that... By the way, if case of request failure, would it be cleaner to process a clk_disable_unprepare(t2_clk) before returning ? clkevt.clkevt.cpumask = cpumask_of(0); ret = request_irq(irq, ch2_irq, IRQF_TIMER, "tc_clkevt", &clkevt); if (ret) { + clk_disable_unprepare(t2_clk); return ret; } clockevents_config_and_register(&clkevt.clkevt, 32768, 1, 0xffff); Gaël [-- Attachment #2: Type: text/html, Size: 1948 bytes --] ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 3/3] ARM: at91/tclib: mask interruptions at shutdown and probe 2014-08-19 22:07 ` Gaël PORTAY @ 2014-08-21 3:32 ` Arnd Bergmann -1 siblings, 0 replies; 34+ messages in thread From: Arnd Bergmann @ 2014-08-21 3:32 UTC (permalink / raw) To: Gaël PORTAY Cc: Daniel Lezcano, Greg Kroah-Hartman, linux-arm-kernel, linux-kernel, linux-pwm, Nicolas Ferre, Thierry Reding, Thomas Gleixner, Boris Brezillon, Alexandre Belloni, Jean-Christophe PLAGNIOL-VILLARD On Wednesday 20 August 2014, Gaël PORTAY wrote: > +static void tc_shutdown (struct platform_device *pdev) > +{ > + int i; > + struct atmel_tc *tc = platform_get_drvdata(pdev); > + > + for (i = 0; i < 3; i++) > + __raw_writel(0xff, tc->regs + ATMEL_TC_REG(i, IDR)); > +} In general, __raw_readl/__raw_writel are not meant to be called by device drivers. Just use readl/writel by default, or readl_relaxed/writel_relaxed if the code is performance critical and you are sure it is safe to use them. Arnd ^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 3/3] ARM: at91/tclib: mask interruptions at shutdown and probe @ 2014-08-21 3:32 ` Arnd Bergmann 0 siblings, 0 replies; 34+ messages in thread From: Arnd Bergmann @ 2014-08-21 3:32 UTC (permalink / raw) To: linux-arm-kernel On Wednesday 20 August 2014, Ga?l PORTAY wrote: > +static void tc_shutdown (struct platform_device *pdev) > +{ > + int i; > + struct atmel_tc *tc = platform_get_drvdata(pdev); > + > + for (i = 0; i < 3; i++) > + __raw_writel(0xff, tc->regs + ATMEL_TC_REG(i, IDR)); > +} In general, __raw_readl/__raw_writel are not meant to be called by device drivers. Just use readl/writel by default, or readl_relaxed/writel_relaxed if the code is performance critical and you are sure it is safe to use them. Arnd ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH 3/3] ARM: at91/tclib: mask interruptions at shutdown and probe 2014-08-21 3:32 ` Arnd Bergmann (?) @ 2014-08-21 9:43 ` Gaël PORTAY -1 siblings, 0 replies; 34+ messages in thread From: Gaël PORTAY @ 2014-08-21 9:43 UTC (permalink / raw) To: Arnd Bergmann Cc: Daniel Lezcano, Greg Kroah-Hartman, linux-arm-kernel, linux-kernel, linux-pwm, Nicolas Ferre, Thierry Reding, Thomas Gleixner, Boris Brezillon, Alexandre Belloni, Jean-Christophe PLAGNIOL-VILLARD [-- Attachment #1: Type: text/plain, Size: 857 bytes --] On 21 August 2014 05:32, Arnd Bergmann <arnd@arndb.de> wrote: > On Wednesday 20 August 2014, Gaël PORTAY wrote: > > +static void tc_shutdown (struct platform_device *pdev) > > +{ > > + int i; > > + struct atmel_tc *tc = platform_get_drvdata(pdev); > > + > > + for (i = 0; i < 3; i++) > > + __raw_writel(0xff, tc->regs + ATMEL_TC_REG(i, IDR)); > > +} > > In general, __raw_readl/__raw_writel are not meant to be called by device > drivers. > > Just use readl/writel by default, or readl_relaxed/writel_relaxed if the > code is > performance critical and you are sure it is safe to use them. > > Arnd > Ok, I will thix this. I used __raw_readl/__raw_writel, because those functions are used in the tcb_clksrc driver. By the way... thanks every one for your review :) -- Gaël PORTAY [-- Attachment #2: Type: text/html, Size: 1440 bytes --] ^ permalink raw reply [flat|nested] 34+ messages in thread
end of thread, other threads:[~2014-08-27 15:27 UTC | newest] Thread overview: 34+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-08-19 22:07 [PATCH 0/3] ARM: at91/tclib: fix segmentation fault Gaël PORTAY 2014-08-19 22:07 ` Gaël PORTAY 2014-08-19 22:07 ` [PATCH 1/3] ARM: at91/tclib: prefer using of devm_* functions Gaël PORTAY 2014-08-19 22:07 ` Gaël PORTAY 2014-08-20 8:19 ` Boris BREZILLON 2014-08-20 8:19 ` Boris BREZILLON 2014-08-19 22:07 ` [PATCH 2/3] ARM: at91/tclib: move initialization from alloc to probe Gaël PORTAY 2014-08-19 22:07 ` Gaël PORTAY 2014-08-20 7:39 ` Thierry Reding 2014-08-20 7:39 ` Thierry Reding 2014-08-20 8:16 ` Boris BREZILLON 2014-08-20 8:16 ` Boris BREZILLON 2014-08-19 22:07 ` [PATCH 3/3] ARM: at91/tclib: mask interruptions at shutdown and probe Gaël PORTAY 2014-08-19 22:07 ` Gaël PORTAY 2014-08-19 22:11 ` Jean-Christophe PLAGNIOL-VILLARD 2014-08-19 22:11 ` Jean-Christophe PLAGNIOL-VILLARD 2014-08-19 23:01 ` Boris BREZILLON 2014-08-19 23:01 ` Boris BREZILLON 2014-08-20 7:31 ` Thierry Reding 2014-08-20 7:31 ` Thierry Reding 2014-08-20 8:14 ` Boris BREZILLON 2014-08-20 8:14 ` Boris BREZILLON 2014-08-20 8:28 ` Thierry Reding 2014-08-20 8:28 ` Thierry Reding 2014-08-20 9:06 ` Boris BREZILLON 2014-08-20 9:06 ` Boris BREZILLON 2014-08-20 9:48 ` Thierry Reding 2014-08-20 9:48 ` Thierry Reding 2014-08-20 10:07 ` Boris BREZILLON 2014-08-20 10:07 ` Boris BREZILLON 2014-08-27 15:27 ` Gaël PORTAY 2014-08-21 3:32 ` Arnd Bergmann 2014-08-21 3:32 ` Arnd Bergmann 2014-08-21 9:43 ` Gaël PORTAY
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.