* [PATCH] pinctrl: freescale: imx: Use 'devm_of_iomap()' to avoid a resource leak in case of error in 'imx_pinctrl_probe()'
@ 2020-06-02 20:06 Christophe JAILLET
2020-06-03 10:52 ` Dan Carpenter
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Christophe JAILLET @ 2020-06-02 20:06 UTC (permalink / raw)
To: aisheng.dong, festevam, shawnguo, stefan, kernel, linus.walleij,
s.hauer, linux-imx, aalonso
Cc: linux-gpio, linux-arm-kernel, Christophe JAILLET, Dan Carpenter
Use 'devm_of_iomap()' instead 'of_iomap()' to avoid a resource leak in
case of error.
Update the error handling code accordingly.
Fixes: 26d8cde5260b ("pinctrl: freescale: imx: add shared input select reg support")
Suggested-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
drivers/pinctrl/freescale/pinctrl-imx.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c b/drivers/pinctrl/freescale/pinctrl-imx.c
index 1f81569c7ae3..cb7e0f08d2cf 100644
--- a/drivers/pinctrl/freescale/pinctrl-imx.c
+++ b/drivers/pinctrl/freescale/pinctrl-imx.c
@@ -824,12 +824,13 @@ int imx_pinctrl_probe(struct platform_device *pdev,
return -EINVAL;
}
- ipctl->input_sel_base = of_iomap(np, 0);
+ ipctl->input_sel_base = devm_of_iomap(&pdev->dev, np,
+ 0, NULL);
of_node_put(np);
- if (!ipctl->input_sel_base) {
+ if (IS_ERR(ipctl->input_sel_base)) {
dev_err(&pdev->dev,
"iomuxc input select base address not found\n");
- return -ENOMEM;
+ return PTR_ERR(ipctl->input_sel_base);
}
}
}
--
2.25.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] pinctrl: freescale: imx: Use 'devm_of_iomap()' to avoid a resource leak in case of error in 'imx_pinctrl_probe()'
2020-06-02 20:06 [PATCH] pinctrl: freescale: imx: Use 'devm_of_iomap()' to avoid a resource leak in case of error in 'imx_pinctrl_probe()' Christophe JAILLET
@ 2020-06-03 10:52 ` Dan Carpenter
2020-06-03 12:36 ` Linus Walleij
2020-06-13 15:49 ` Guenter Roeck
2 siblings, 0 replies; 5+ messages in thread
From: Dan Carpenter @ 2020-06-03 10:52 UTC (permalink / raw)
To: Christophe JAILLET
Cc: aisheng.dong, festevam, shawnguo, stefan, kernel, linus.walleij,
s.hauer, linux-imx, aalonso, linux-gpio, linux-arm-kernel
On Tue, Jun 02, 2020 at 10:06:26PM +0200, Christophe JAILLET wrote:
> Use 'devm_of_iomap()' instead 'of_iomap()' to avoid a resource leak in
> case of error.
>
> Update the error handling code accordingly.
>
> Fixes: 26d8cde5260b ("pinctrl: freescale: imx: add shared input select reg support")
> Suggested-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
Thanks! You are a hero!
Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] pinctrl: freescale: imx: Use 'devm_of_iomap()' to avoid a resource leak in case of error in 'imx_pinctrl_probe()'
2020-06-02 20:06 [PATCH] pinctrl: freescale: imx: Use 'devm_of_iomap()' to avoid a resource leak in case of error in 'imx_pinctrl_probe()' Christophe JAILLET
2020-06-03 10:52 ` Dan Carpenter
@ 2020-06-03 12:36 ` Linus Walleij
2020-06-13 15:49 ` Guenter Roeck
2 siblings, 0 replies; 5+ messages in thread
From: Linus Walleij @ 2020-06-03 12:36 UTC (permalink / raw)
To: Christophe JAILLET
Cc: Dong Aisheng, Fabio Estevam, Shawn Guo, Stefan Agner,
Sascha Hauer, Sascha Hauer, NXP Linux Team, Adrian Alonso,
open list:GPIO SUBSYSTEM, Linux ARM, Dan Carpenter
On Tue, Jun 2, 2020 at 10:06 PM Christophe JAILLET
<christophe.jaillet@wanadoo.fr> wrote:
> Use 'devm_of_iomap()' instead 'of_iomap()' to avoid a resource leak in
> case of error.
>
> Update the error handling code accordingly.
>
> Fixes: 26d8cde5260b ("pinctrl: freescale: imx: add shared input select reg support")
> Suggested-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
Patch applied!
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] pinctrl: freescale: imx: Use 'devm_of_iomap()' to avoid a resource leak in case of error in 'imx_pinctrl_probe()'
2020-06-02 20:06 [PATCH] pinctrl: freescale: imx: Use 'devm_of_iomap()' to avoid a resource leak in case of error in 'imx_pinctrl_probe()' Christophe JAILLET
2020-06-03 10:52 ` Dan Carpenter
2020-06-03 12:36 ` Linus Walleij
@ 2020-06-13 15:49 ` Guenter Roeck
2020-06-15 12:15 ` Dan Carpenter
2 siblings, 1 reply; 5+ messages in thread
From: Guenter Roeck @ 2020-06-13 15:49 UTC (permalink / raw)
To: Christophe JAILLET
Cc: aisheng.dong, festevam, shawnguo, stefan, kernel, linus.walleij,
s.hauer, linux-imx, aalonso, linux-gpio, Dan Carpenter,
linux-arm-kernel
On Tue, Jun 02, 2020 at 10:06:26PM +0200, Christophe JAILLET wrote:
> Use 'devm_of_iomap()' instead 'of_iomap()' to avoid a resource leak in
> case of error.
>
> Update the error handling code accordingly.
>
> Fixes: 26d8cde5260b ("pinctrl: freescale: imx: add shared input select reg support")
> Suggested-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
When booting mcimx7d-sabre in qemu, his patch results in:
[ 1.835341] imx7d-pinctrl 302c0000.iomuxc-lpsr: initialized IMX pinctrl driver
[ 1.839702] imx7d-pinctrl 30330000.pinctrl: can't request region for resource [mem 0x30330000-0x3033ffff]
[ 1.840261] imx7d-pinctrl: probe of 30330000.pinctrl failed with error -16
This is followed by:
[ 21.589169] sdhci-esdhc-imx 30b40000.usdhc: could not get pinctrl
[ 21.589569] 8<--- cut here ---
[ 21.589683] Unable to handle kernel paging request at virtual address ffffff9e
[ 21.589830] pgd = (ptrval)
[ 21.589905] [ffffff9e] *pgd=8bfde841, *pte=00000000, *ppte=00000000
[ 21.590404] Internal error: Oops: 27 [#1] SMP ARM
[ 21.590595] Modules linked in:
[ 21.590792] CPU: 0 PID: 5 Comm: kworker/0:0 Not tainted 5.7.0-14530-g464c47191c90-dirty #4
[ 21.590926] Hardware name: Freescale i.MX7 Dual (Device Tree)
[ 21.591402] Workqueue: events deferred_probe_work_func
[ 21.591547] PC is at pinctrl_lookup_state+0x8/0x78
[ 21.591626] LR is at sdhci_esdhc_imx_probe+0x5e4/0x6c4
[ 21.591731] pc : [<c091e884>] lr : [<c0f9c84c>] psr: 20000013
[ 21.591874] sp : cb1b5df8 ip : 00000001 fp : c1fee723
[ 21.591993] r10: 00000009 r9 : 00000000 r8 : 00000000
[ 21.592120] r7 : cb4c5810 r6 : cb4c5800 r5 : ffffff92 r4 : cbdd338c
[ 21.592263] r3 : 00014368 r2 : d8f46c35 r1 : c1a1e710 r0 : ffffff92
[ 21.592441] Flags: nzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none
[ 21.592610] Control: 10c5387d Table: 8020406a DAC: 00000051
[ 21.592762] Process kworker/0:0 (pid: 5, stack limit = 0x(ptrval))
[ 21.592935] Stack: (0xcb1b5df8 to 0xcb1b6000)
[ 21.593129] 5de0: cbdd338c c9744540
[ 21.593404] 5e00: cb4c5800 cb4c5810 00000000 c0f9c84c 00000000 d8f46c35 cb4c5810 00000000
[ 21.593649] 5e20: cb4c5810 c1fc55b4 c2652ab0 00000000 c1fc55b4 c0bb98a4 cb4c5810 c2652aa8
[ 21.593937] 5e40: 00000000 c2652ab0 00000000 c0bb73f8 cb4c5810 c1fc55b4 c0bb776c cb1b4000
[ 21.594209] 5e60: c1f8b2e8 cb1b4000 c1e08eec c0bb766c 00000000 cb1b5eac c0bb776c c0bb55d0
[ 21.594460] 5e80: cb4a7124 cb0bb8e4 c94af4d8 d8f46c35 00000004 cb4c5810 cb1b4000 00000001
[ 21.594702] 5ea0: cb4c58a0 c0bb71a4 cb1b4000 cb4c5810 00000001 d8f46c35 00000000 cb4bbaf4
[ 21.594973] 5ec0: cb4c5810 c1f8b750 c1f8b29c c0bb6470 cb4bbaf4 cb4c5810 c1f8b29c c0bb692c
[ 21.595187] 5ee0: c1f8b340 cb0b4c80 cbd95c00 cbd98f00 c1fefbd0 c036885c 00000001 00000000
[ 21.595457] 5f00: c03687a8 c120d364 00000000 00000000 00000000 cbd95c00 c1f8b340 c24673d8
[ 21.595727] 5f20: 00000000 c19af744 00000000 d8f46c35 cbd95c00 cb0b4c80 cbd95c00 cb0b4c94
[ 21.596023] 5f40: cbd95c38 c1e05d00 00000008 cb1b4000 cbd95c00 c0368f94 cb19a164 c1fef34a
[ 21.596284] 5f60: cb1b4000 cb0b5280 cb17a7c0 cb1b4000 00000000 c0368d7c cb0b4c80 cb183e7c
[ 21.596519] 5f80: cb0b52c4 c03714c8 cb1b4000 cb17a7c0 c0371388 00000000 00000000 00000000
[ 21.596742] 5fa0: 00000000 00000000 00000000 c0300174 00000000 00000000 00000000 00000000
[ 21.596976] 5fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[ 21.597196] 5fe0: 00000000 00000000 00000000 00000000 00000013 00000000 00000000 00000000
[ 21.597442] [<c091e884>] (pinctrl_lookup_state) from [<c0f9c84c>] (sdhci_esdhc_imx_probe+0x5e4/0x6c4)
[ 21.597651] [<c0f9c84c>] (sdhci_esdhc_imx_probe) from [<c0bb98a4>] (platform_drv_probe+0x48/0x98)
[ 21.597838] [<c0bb98a4>] (platform_drv_probe) from [<c0bb73f8>] (really_probe+0x1e0/0x348)
[ 21.598016] [<c0bb73f8>] (really_probe) from [<c0bb766c>] (driver_probe_device+0x5c/0xb4)
[ 21.598188] [<c0bb766c>] (driver_probe_device) from [<c0bb55d0>] (bus_for_each_drv+0x84/0xc8)
[ 21.598367] [<c0bb55d0>] (bus_for_each_drv) from [<c0bb71a4>] (__device_attach+0xdc/0x148)
[ 21.598545] [<c0bb71a4>] (__device_attach) from [<c0bb6470>] (bus_probe_device+0x88/0x90)
[ 21.598720] [<c0bb6470>] (bus_probe_device) from [<c0bb692c>] (deferred_probe_work_func+0x74/0xa4)
[ 21.598906] [<c0bb692c>] (deferred_probe_work_func) from [<c036885c>] (process_one_work+0x274/0x794)
[ 21.599105] [<c036885c>] (process_one_work) from [<c0368f94>] (worker_thread+0x218/0x4f4)
[ 21.599275] [<c0368f94>] (worker_thread) from [<c03714c8>] (kthread+0x140/0x174)
[ 21.599441] [<c03714c8>] (kthread) from [<c0300174>] (ret_from_fork+0x14/0x20)
[ 21.599606] Exception stack(0xcb1b5fb0 to 0xcb1b5ff8)
[ 21.599782] 5fa0: 00000000 00000000 00000000 00000000
[ 21.600042] 5fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[ 21.600290] 5fe0: 00000000 00000000 00000000 00000000 00000013 00000000
[ 21.600582] Code: e8bd81f0 c138c66c e92d41f0 e1a05000 (e5b5400c)
[ 21.601140] ---[ end trace a236f1b24b2698e6 ]---
After reverting the patch, the log is:
[ 1.829261] imx7d-pinctrl 302c0000.iomuxc-lpsr: initialized IMX pinctrl driver
[ 1.841358] imx7d-pinctrl 30330000.pinctrl: initialized IMX pinctrl driver
and there is no subsequent error/crash.
Guenter
---
bisect log:
# bad: [af7b4801030c07637840191c69eb666917e4135d] Merge git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net
# good: [3b69e8b4571125bec1f77f886174fe6cab6b9d75] Merge tag 'sh-for-5.8' of git://git.libc.org/linux-sh
git bisect start 'af7b4801030c' '3b69e8b45711'
# good: [77f55d1305c11fb729b88f2c3f7881ba0831fa6f] staging: rtl8723bs: Use common packet header constants
git bisect good 77f55d1305c11fb729b88f2c3f7881ba0831fa6f
# good: [f558b8364e19f9222e7976c64e9367f66bab02cc] Merge tag 'driver-core-5.8-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core
git bisect good f558b8364e19f9222e7976c64e9367f66bab02cc
# good: [57343d51613227373759f5b0f2eede257fd4b82e] misc: xilinx-sdfec: improve get_user_pages_fast() error handling
git bisect good 57343d51613227373759f5b0f2eede257fd4b82e
# good: [e8dff03aef6a76c5c9184ed1dd3c770d4ce9c885] Merge tag 'rtc-5.8' of git://git.kernel.org/pub/scm/linux/kernel/git/abelloni/linux
git bisect good e8dff03aef6a76c5c9184ed1dd3c770d4ce9c885
# good: [920fecc1aa4591da27ef9dcb338fc5da86b404d7] pinctrl: bcm2835: Add support for wake-up interrupts
git bisect good 920fecc1aa4591da27ef9dcb338fc5da86b404d7
# good: [4e2905adac9f0fdc25154ac83719a986c2367a14] net: dp83869: Reset return variable if PHY strap is read
git bisect good 4e2905adac9f0fdc25154ac83719a986c2367a14
# good: [9049a40c858f49c141e12924b77b91cce4c46617] Merge branch 'for-davem' of git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs
git bisect good 9049a40c858f49c141e12924b77b91cce4c46617
# bad: [cf0c97f148e9e50aa5a7ddd1984a604dd2bde4af] Merge tag 'pinctrl-v5.8-1' of git://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl
git bisect bad cf0c97f148e9e50aa5a7ddd1984a604dd2bde4af
# good: [11d8da5cabf7c6c3263ba2cd9c00260395867048] pinctrl: freescale: imx: Fix an error handling path in 'imx_pinctrl_probe()'
git bisect good 11d8da5cabf7c6c3263ba2cd9c00260395867048
# bad: [08acc963190a3a0eb491efa9cc466b2c18d59f22] pinctrl: pxa: pxa2xx: Remove 'pxa2xx_pinctrl_exit()' which is unused and broken
git bisect bad 08acc963190a3a0eb491efa9cc466b2c18d59f22
# bad: [ba403242615c2c99e27af7984b1650771a2cc2c9] pinctrl: freescale: imx: Use 'devm_of_iomap()' to avoid a resource leak in case of error in 'imx_pinctrl_probe()'
git bisect bad ba403242615c2c99e27af7984b1650771a2cc2c9
# first bad commit: [ba403242615c2c99e27af7984b1650771a2cc2c9] pinctrl: freescale: imx: Use 'devm_of_iomap()' to avoid a resource leak in case of error in 'imx_pinctrl_probe()'
---
qemu command line:
qemu-system-arm -M mcimx7d-sabre \
-kernel arch/arm/boot/zImage -no-reboot \
-initrd rootfs-armv7a.cpio \
-m 256 -display none \
--append "rdinit=/sbin/init earlycon=ec_imx6q,mmio,0x30860000,115200n8 console=ttymxc0,115200" \
-dtb arch/arm/boot/dts/imx7d-sdb.dtb \
-nographic -monitor null -serial stdio
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] pinctrl: freescale: imx: Use 'devm_of_iomap()' to avoid a resource leak in case of error in 'imx_pinctrl_probe()'
2020-06-13 15:49 ` Guenter Roeck
@ 2020-06-15 12:15 ` Dan Carpenter
0 siblings, 0 replies; 5+ messages in thread
From: Dan Carpenter @ 2020-06-15 12:15 UTC (permalink / raw)
To: Guenter Roeck
Cc: Christophe JAILLET, aisheng.dong, festevam, shawnguo, stefan,
kernel, linus.walleij, s.hauer, linux-imx, aalonso, linux-gpio,
linux-arm-kernel
On Sat, Jun 13, 2020 at 08:49:54AM -0700, Guenter Roeck wrote:
> On Tue, Jun 02, 2020 at 10:06:26PM +0200, Christophe JAILLET wrote:
> > Use 'devm_of_iomap()' instead 'of_iomap()' to avoid a resource leak in
> > case of error.
> >
> > Update the error handling code accordingly.
> >
> > Fixes: 26d8cde5260b ("pinctrl: freescale: imx: add shared input select reg support")
> > Suggested-by: Dan Carpenter <dan.carpenter@oracle.com>
> > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>
> When booting mcimx7d-sabre in qemu, his patch results in:
>
> [ 1.835341] imx7d-pinctrl 302c0000.iomuxc-lpsr: initialized IMX pinctrl driver
> [ 1.839702] imx7d-pinctrl 30330000.pinctrl: can't request region for resource [mem 0x30330000-0x3033ffff]
> [ 1.840261] imx7d-pinctrl: probe of 30330000.pinctrl failed with error -16
Yeah. Sorry about that. We had to revert that patch.
The problem is that that devm_of_iomap() tracks if the regions are
already used and of_iomap() does not. In this case there were two
places mapping the same memory. I added a comment about that to the
devm_of_iomap() so hopefully we won't introduce bugs like this in the
future.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-06-15 12:18 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-02 20:06 [PATCH] pinctrl: freescale: imx: Use 'devm_of_iomap()' to avoid a resource leak in case of error in 'imx_pinctrl_probe()' Christophe JAILLET
2020-06-03 10:52 ` Dan Carpenter
2020-06-03 12:36 ` Linus Walleij
2020-06-13 15:49 ` Guenter Roeck
2020-06-15 12:15 ` Dan Carpenter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).