diff mbox series

[v2] Revert "pinctrl: freescale: imx: Use 'devm_of_iomap()' to avoid a resource leak in case of error in 'imx_pinctrl_probe()'"

Message ID 1591673223-1680-1-git-send-email-haibo.chen@nxp.com
State New
Headers show
Series [v2] Revert "pinctrl: freescale: imx: Use 'devm_of_iomap()' to avoid a resource leak in case of error in 'imx_pinctrl_probe()'" | expand

Commit Message

Bough Chen June 9, 2020, 3:27 a.m. UTC 
From: Haibo Chen <haibo.chen@nxp.com>

This reverts commit ba403242615c2c99e27af7984b1650771a2cc2c9.

After commit 26d8cde5260b ("pinctrl: freescale: imx: add shared
input select reg support"). i.MX7D has two iomux controllers
iomuxc and iomuxc-lpsr which share select_input register for
daisy chain settings.
If use 'devm_of_iomap()', when probe the iomuxc-lpsr, will call
devm_request_mem_region() for the region <0x30330000-0x3033ffff>
for the first time. Then, next time when probe the iomuxc, API
devm_platform_ioremap_resource() will also use the API
devm_request_mem_region() for the share region <0x30330000-0x3033ffff>
again, then cause issue, log like below:

[    0.179561] imx7d-pinctrl 302c0000.iomuxc-lpsr: initialized IMX pinctrl driver
[    0.191742] imx7d-pinctrl 30330000.pinctrl: can't request region for resource [mem 0x30330000-0x3033ffff]
[    0.191842] imx7d-pinctrl: probe of 30330000.pinctrl failed with error -16

Fixes: ba403242615c ("pinctrl: freescale: imx: Use 'devm_of_iomap()' to avoid a resource leak in case of error in 'imx_pinctrl_probe()'")
Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
Reviewed-by: Dong Aisheng <aisheng.dong@nxp.com>
---
 drivers/pinctrl/freescale/pinctrl-imx.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

Comments

Aisheng Dong June 16, 2020, 2:53 a.m. UTC | #1 
Hi Linus,

Could you help apply this patch as it blocked MX7D booting for a while?

Regards
Aisheng

> From: haibo.chen@nxp.com <haibo.chen@nxp.com>
> Sent: Tuesday, June 9, 2020 11:27 AM
> 
> From: Haibo Chen <haibo.chen@nxp.com>
> 
> This reverts commit ba403242615c2c99e27af7984b1650771a2cc2c9.
> 
> After commit 26d8cde5260b ("pinctrl: freescale: imx: add shared input select
> reg support"). i.MX7D has two iomux controllers iomuxc and iomuxc-lpsr which
> share select_input register for daisy chain settings.
> If use 'devm_of_iomap()', when probe the iomuxc-lpsr, will call
> devm_request_mem_region() for the region <0x30330000-0x3033ffff> for the
> first time. Then, next time when probe the iomuxc, API
> devm_platform_ioremap_resource() will also use the API
> devm_request_mem_region() for the share region <0x30330000-0x3033ffff>
> again, then cause issue, log like below:
> 
> [    0.179561] imx7d-pinctrl 302c0000.iomuxc-lpsr: initialized IMX pinctrl
> driver
> [    0.191742] imx7d-pinctrl 30330000.pinctrl: can't request region for
> resource [mem 0x30330000-0x3033ffff]
> [    0.191842] imx7d-pinctrl: probe of 30330000.pinctrl failed with error -16
> 
> Fixes: ba403242615c ("pinctrl: freescale: imx: Use 'devm_of_iomap()' to avoid a
> resource leak in case of error in 'imx_pinctrl_probe()'")
> Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
> Reviewed-by: Dong Aisheng <aisheng.dong@nxp.com>

> ---
>  drivers/pinctrl/freescale/pinctrl-imx.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c
> b/drivers/pinctrl/freescale/pinctrl-imx.c
> index cb7e0f08d2cf..1f81569c7ae3 100644
> --- a/drivers/pinctrl/freescale/pinctrl-imx.c
> +++ b/drivers/pinctrl/freescale/pinctrl-imx.c
> @@ -824,13 +824,12 @@ int imx_pinctrl_probe(struct platform_device *pdev,
>  				return -EINVAL;
>  			}
> 
> -			ipctl->input_sel_base = devm_of_iomap(&pdev->dev, np,
> -							      0, NULL);
> +			ipctl->input_sel_base = of_iomap(np, 0);
>  			of_node_put(np);
> -			if (IS_ERR(ipctl->input_sel_base)) {
> +			if (!ipctl->input_sel_base) {
>  				dev_err(&pdev->dev,
>  					"iomuxc input select base address not found\n");
> -				return PTR_ERR(ipctl->input_sel_base);
> +				return -ENOMEM;
>  			}
>  		}
>  	}
> --
> 2.17.1
Linus Walleij June 16, 2020, 8:21 a.m. UTC | #2 
On Tue, Jun 9, 2020 at 5:38 AM <haibo.chen@nxp.com> wrote:

> From: Haibo Chen <haibo.chen@nxp.com>
>
> This reverts commit ba403242615c2c99e27af7984b1650771a2cc2c9.
>
> After commit 26d8cde5260b ("pinctrl: freescale: imx: add shared
> input select reg support"). i.MX7D has two iomux controllers
> iomuxc and iomuxc-lpsr which share select_input register for
> daisy chain settings.
> If use 'devm_of_iomap()', when probe the iomuxc-lpsr, will call
> devm_request_mem_region() for the region <0x30330000-0x3033ffff>
> for the first time. Then, next time when probe the iomuxc, API
> devm_platform_ioremap_resource() will also use the API
> devm_request_mem_region() for the share region <0x30330000-0x3033ffff>
> again, then cause issue, log like below:
>
> [    0.179561] imx7d-pinctrl 302c0000.iomuxc-lpsr: initialized IMX pinctrl driver
> [    0.191742] imx7d-pinctrl 30330000.pinctrl: can't request region for resource [mem 0x30330000-0x3033ffff]
> [    0.191842] imx7d-pinctrl: probe of 30330000.pinctrl failed with error -16
>
> Fixes: ba403242615c ("pinctrl: freescale: imx: Use 'devm_of_iomap()' to avoid a resource leak in case of error in 'imx_pinctrl_probe()'")
> Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
> Reviewed-by: Dong Aisheng <aisheng.dong@nxp.com>

Patch applied for fixes.

Yours,
Linus Walleij
Andy Shevchenko June 16, 2020, 6:15 p.m. UTC | #3 
On Tue, Jun 16, 2020 at 5:54 AM Aisheng Dong <aisheng.dong@nxp.com> wrote:

> Could you help apply this patch as it blocked MX7D booting for a while?

> > This reverts commit ba403242615c2c99e27af7984b1650771a2cc2c9.
> >
> > After commit 26d8cde5260b ("pinctrl: freescale: imx: add shared input select
> > reg support"). i.MX7D has two iomux controllers iomuxc and iomuxc-lpsr which
> > share select_input register for daisy chain settings.
> > If use 'devm_of_iomap()', when probe the iomuxc-lpsr, will call
> > devm_request_mem_region() for the region <0x30330000-0x3033ffff> for the
> > first time. Then, next time when probe the iomuxc, API
> > devm_platform_ioremap_resource() will also use the API
> > devm_request_mem_region() for the share region <0x30330000-0x3033ffff>
> > again, then cause issue, log like below:
> >
> > [    0.179561] imx7d-pinctrl 302c0000.iomuxc-lpsr: initialized IMX pinctrl
> > driver
> > [    0.191742] imx7d-pinctrl 30330000.pinctrl: can't request region for
> > resource [mem 0x30330000-0x3033ffff]
> > [    0.191842] imx7d-pinctrl: probe of 30330000.pinctrl failed with error -16

It means that shared support took a wrong approach. If something has
shared resources, another schema should be used, something like MFD
(parent device which keeps only shared resources). Easiest way is to
switch to the regmap API.

P.S. The revert is okay as a quick fix but... see above.
Peng Fan June 17, 2020, 1:14 a.m. UTC | #4 
> Subject: Re: [PATCH v2] Revert "pinctrl: freescale: imx: Use 'devm_of_iomap()'
> to avoid a resource leak in case of error in 'imx_pinctrl_probe()'"
> 
> On Tue, Jun 16, 2020 at 5:54 AM Aisheng Dong <aisheng.dong@nxp.com>
> wrote:
> 
> > Could you help apply this patch as it blocked MX7D booting for a while?
> 
> > > This reverts commit ba403242615c2c99e27af7984b1650771a2cc2c9.
> > >
> > > After commit 26d8cde5260b ("pinctrl: freescale: imx: add shared
> > > input select reg support"). i.MX7D has two iomux controllers iomuxc
> > > and iomuxc-lpsr which share select_input register for daisy chain settings.
> > > If use 'devm_of_iomap()', when probe the iomuxc-lpsr, will call
> > > devm_request_mem_region() for the region <0x30330000-0x3033ffff> for
> > > the first time. Then, next time when probe the iomuxc, API
> > > devm_platform_ioremap_resource() will also use the API
> > > devm_request_mem_region() for the share region
> > > <0x30330000-0x3033ffff> again, then cause issue, log like below:
> > >
> > > [    0.179561] imx7d-pinctrl 302c0000.iomuxc-lpsr: initialized IMX
> pinctrl
> > > driver
> > > [    0.191742] imx7d-pinctrl 30330000.pinctrl: can't request region for
> > > resource [mem 0x30330000-0x3033ffff]
> > > [    0.191842] imx7d-pinctrl: probe of 30330000.pinctrl failed with error
> -16
> 
> It means that shared support took a wrong approach. If something has shared
> resources, another schema should be used, something like MFD (parent
> device which keeps only shared resources). Easiest way is to switch to the
> regmap API.

Actually not MFD, it is the dts use wrong address region.

                        iomuxc_lpsr: iomuxc-lpsr@302c0000 {
                                compatible = "fsl,imx7d-iomuxc-lpsr";
                                reg = <0x302c0000 0x10000>;
                                fsl,input-sel = <&iomuxc>;
                        };
                        iomuxc: pinctrl@30330000 {
                                compatible = "fsl,imx7d-iomuxc";
                                reg = <0x30330000 0x10000>;
                        };

There is overlap between the two.

The iomuxc_lpsr using 0x10000 as size is wrong. It not have such large area.

With the size fix, devm_of_ioremap could be used I think.

Regards,
Peng.

> 
> P.S. The revert is okay as a quick fix but... see above.
> 
> --
> With Best Regards,
> Andy Shevchenko
Aisheng Dong June 17, 2020, 2:43 a.m. UTC | #5 
> From: Andy Shevchenko <andy.shevchenko@gmail.com>
> Sent: Wednesday, June 17, 2020 2:16 AM
> 
> On Tue, Jun 16, 2020 at 5:54 AM Aisheng Dong <aisheng.dong@nxp.com>
> wrote:
> 
> > Could you help apply this patch as it blocked MX7D booting for a while?
> 
> > > This reverts commit ba403242615c2c99e27af7984b1650771a2cc2c9.
> > >
> > > After commit 26d8cde5260b ("pinctrl: freescale: imx: add shared
> > > input select reg support"). i.MX7D has two iomux controllers iomuxc
> > > and iomuxc-lpsr which share select_input register for daisy chain settings.
> > > If use 'devm_of_iomap()', when probe the iomuxc-lpsr, will call
> > > devm_request_mem_region() for the region <0x30330000-0x3033ffff> for
> > > the first time. Then, next time when probe the iomuxc, API
> > > devm_platform_ioremap_resource() will also use the API
> > > devm_request_mem_region() for the share region
> > > <0x30330000-0x3033ffff> again, then cause issue, log like below:
> > >
> > > [    0.179561] imx7d-pinctrl 302c0000.iomuxc-lpsr: initialized IMX pinctrl
> > > driver
> > > [    0.191742] imx7d-pinctrl 30330000.pinctrl: can't request region for
> > > resource [mem 0x30330000-0x3033ffff]
> > > [    0.191842] imx7d-pinctrl: probe of 30330000.pinctrl failed with error
> -16
> 
> It means that shared support took a wrong approach. If something has shared
> resources, another schema should be used, something like MFD (parent device
> which keeps only shared resources). Easiest way is to switch to the regmap API.
> 

I think a bit more, probably it's not worth to do it in order to not break current driver design a lot.
LPSR iomux case is bit complicated that only sel input is using the shared memory within another iomuxc.
Current approach seems already the easiest and most clean way.

Regards
Aisheng

> P.S. The revert is okay as a quick fix but... see above.
> 
> --
> With Best Regards,
> Andy Shevchenko
Aisheng Dong June 17, 2020, 2:53 a.m. UTC | #6 
> From: Peng Fan <peng.fan@nxp.com>
> Sent: Wednesday, June 17, 2020 9:15 AM
> 
> > Subject: Re: [PATCH v2] Revert "pinctrl: freescale: imx: Use 'devm_of_iomap()'
> > to avoid a resource leak in case of error in 'imx_pinctrl_probe()'"
> >
> > On Tue, Jun 16, 2020 at 5:54 AM Aisheng Dong <aisheng.dong@nxp.com>
> > wrote:
> >
> > > Could you help apply this patch as it blocked MX7D booting for a while?
> >
> > > > This reverts commit ba403242615c2c99e27af7984b1650771a2cc2c9.
> > > >
> > > > After commit 26d8cde5260b ("pinctrl: freescale: imx: add shared
> > > > input select reg support"). i.MX7D has two iomux controllers
> > > > iomuxc and iomuxc-lpsr which share select_input register for daisy chain
> settings.
> > > > If use 'devm_of_iomap()', when probe the iomuxc-lpsr, will call
> > > > devm_request_mem_region() for the region <0x30330000-0x3033ffff>
> > > > for the first time. Then, next time when probe the iomuxc, API
> > > > devm_platform_ioremap_resource() will also use the API
> > > > devm_request_mem_region() for the share region
> > > > <0x30330000-0x3033ffff> again, then cause issue, log like below:
> > > >
> > > > [    0.179561] imx7d-pinctrl 302c0000.iomuxc-lpsr: initialized IMX
> > pinctrl
> > > > driver
> > > > [    0.191742] imx7d-pinctrl 30330000.pinctrl: can't request region for
> > > > resource [mem 0x30330000-0x3033ffff]
> > > > [    0.191842] imx7d-pinctrl: probe of 30330000.pinctrl failed with error
> > -16
> >
> > It means that shared support took a wrong approach. If something has
> > shared resources, another schema should be used, something like MFD
> > (parent device which keeps only shared resources). Easiest way is to
> > switch to the regmap API.
> 
> Actually not MFD, it is the dts use wrong address region.
> 
>                         iomuxc_lpsr: iomuxc-lpsr@302c0000 {
>                                 compatible = "fsl,imx7d-iomuxc-lpsr";
>                                 reg = <0x302c0000 0x10000>;
>                                 fsl,input-sel = <&iomuxc>;
>                         };
>                         iomuxc: pinctrl@30330000 {
>                                 compatible = "fsl,imx7d-iomuxc";
>                                 reg = <0x30330000 0x10000>;
>                         };
> 
> There is overlap between the two.
> 
> The iomuxc_lpsr using 0x10000 as size is wrong. It not have such large area.
> 
> With the size fix, devm_of_ioremap could be used I think.
> 

Really overlapped?

I think the real issue is LPSR Sel Input part using the same memory region as iomux.

Regards
Aisheng

> Regards,
> Peng.
> 
> >
> > P.S. The revert is okay as a quick fix but... see above.
> >
> > --
> > With Best Regards,
> > Andy Shevchenko
Peng Fan June 17, 2020, 2:56 a.m. UTC | #7 
> Subject: RE: [PATCH v2] Revert "pinctrl: freescale: imx: Use
> 'devm_of_iomap()' to avoid a resource leak in case of error in
> 'imx_pinctrl_probe()'"
> 
> > From: Peng Fan <peng.fan@nxp.com>
> > Sent: Wednesday, June 17, 2020 9:15 AM
> >
> > > Subject: Re: [PATCH v2] Revert "pinctrl: freescale: imx: Use
> 'devm_of_iomap()'
> > > to avoid a resource leak in case of error in 'imx_pinctrl_probe()'"
> > >
> > > On Tue, Jun 16, 2020 at 5:54 AM Aisheng Dong <aisheng.dong@nxp.com>
> > > wrote:
> > >
> > > > Could you help apply this patch as it blocked MX7D booting for a while?
> > >
> > > > > This reverts commit ba403242615c2c99e27af7984b1650771a2cc2c9.
> > > > >
> > > > > After commit 26d8cde5260b ("pinctrl: freescale: imx: add shared
> > > > > input select reg support"). i.MX7D has two iomux controllers
> > > > > iomuxc and iomuxc-lpsr which share select_input register for
> > > > > daisy chain
> > settings.
> > > > > If use 'devm_of_iomap()', when probe the iomuxc-lpsr, will call
> > > > > devm_request_mem_region() for the region <0x30330000-0x3033ffff>
> > > > > for the first time. Then, next time when probe the iomuxc, API
> > > > > devm_platform_ioremap_resource() will also use the API
> > > > > devm_request_mem_region() for the share region
> > > > > <0x30330000-0x3033ffff> again, then cause issue, log like below:
> > > > >
> > > > > [    0.179561] imx7d-pinctrl 302c0000.iomuxc-lpsr: initialized IMX
> > > pinctrl
> > > > > driver
> > > > > [    0.191742] imx7d-pinctrl 30330000.pinctrl: can't request region
> for
> > > > > resource [mem 0x30330000-0x3033ffff]
> > > > > [    0.191842] imx7d-pinctrl: probe of 30330000.pinctrl failed with
> error
> > > -16
> > >
> > > It means that shared support took a wrong approach. If something has
> > > shared resources, another schema should be used, something like MFD
> > > (parent device which keeps only shared resources). Easiest way is to
> > > switch to the regmap API.
> >
> > Actually not MFD, it is the dts use wrong address region.
> >
> >                         iomuxc_lpsr: iomuxc-lpsr@302c0000 {
> >                                 compatible =
> "fsl,imx7d-iomuxc-lpsr";
> >                                 reg = <0x302c0000 0x10000>;
> >                                 fsl,input-sel = <&iomuxc>;
> >                         };
> >                         iomuxc: pinctrl@30330000 {
> >                                 compatible = "fsl,imx7d-iomuxc";
> >                                 reg = <0x30330000 0x10000>;
> >                         };
> >
> > There is overlap between the two.
> >
> > The iomuxc_lpsr using 0x10000 as size is wrong. It not have such large area.
> >
> > With the size fix, devm_of_ioremap could be used I think.
> >
> 
> Really overlapped?

Oops.
> 
> I think the real issue is LPSR Sel Input part using the same memory region as
> iomux.

Indeed. Then mfd might be better to be used here.

Regards,
Peng.

> 
> Regards
> Aisheng
> 
> > Regards,
> > Peng.
> >
> > >
> > > P.S. The revert is okay as a quick fix but... see above.
> > >
> > > --
> > > With Best Regards,
> > > Andy Shevchenko
diff mbox series

Patch

diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c b/drivers/pinctrl/freescale/pinctrl-imx.c
index cb7e0f08d2cf..1f81569c7ae3 100644
--- a/drivers/pinctrl/freescale/pinctrl-imx.c
+++ b/drivers/pinctrl/freescale/pinctrl-imx.c
@@ -824,13 +824,12 @@  int imx_pinctrl_probe(struct platform_device *pdev,
 				return -EINVAL;
 			}
 
-			ipctl->input_sel_base = devm_of_iomap(&pdev->dev, np,
-							      0, NULL);
+			ipctl->input_sel_base = of_iomap(np, 0);
 			of_node_put(np);
-			if (IS_ERR(ipctl->input_sel_base)) {
+			if (!ipctl->input_sel_base) {
 				dev_err(&pdev->dev,
 					"iomuxc input select base address not found\n");
-				return PTR_ERR(ipctl->input_sel_base);
+				return -ENOMEM;
 			}
 		}
 	}