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

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

[haibo.chen]

From: Haibo Chen <haibo.chen@nxp.com>

This patch block system booting, find on imx7d-sdb board.
From the dts we can see, iomux and iomux_lpsr share the memory region
[0x30330000-0x3033ffff], so will trigger the following issue:

[    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

This reverts commit ba403242615c2c99e27af7984b1650771a2cc2c9.
---
 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

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

[BOUGH CHEN]

+ christophe.jaillet@wanadoo.fr

> -----Original Message-----
> From: haibo.chen@nxp.com [mailto:haibo.chen@nxp.com]
> Sent: 2020年6月8日 18:00
> To: Aisheng Dong <aisheng.dong@nxp.com>; festevam@gmail.com;
> shawnguo@kernel.org; stefan@agner.ch; kernel@pengutronix.de;
> linus.walleij@linaro.org; s.hauer@pengutronix.de; dl-linux-imx
> <linux-imx@nxp.com>
> Cc: linux-gpio@vger.kernel.org; BOUGH CHEN <haibo.chen@nxp.com>
> Subject: [PATCH] Revert "pinctrl: freescale: imx: Use 'devm_of_iomap()' to
> avoid a resource leak in case of error in 'imx_pinctrl_probe()'"
> 
> From: Haibo Chen <haibo.chen@nxp.com>
> 
> This patch block system booting, find on imx7d-sdb board.
> From the dts we can see, iomux and iomux_lpsr share the memory region
> [0x30330000-0x3033ffff], so will trigger the following issue:
> 
> [    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
> 
> This reverts commit ba403242615c2c99e27af7984b1650771a2cc2c9.
> ---
>  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

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

[Fabio Estevam]

Hi Haibo,

On Mon, Jun 8, 2020 at 7:10 AM <haibo.chen@nxp.com> wrote:
>
> From: Haibo Chen <haibo.chen@nxp.com>
>
> This patch block system booting, find on imx7d-sdb board.
> From the dts we can see, iomux and iomux_lpsr share the memory region
> [0x30330000-0x3033ffff], so will trigger the following issue:
>
> [    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
>
> This reverts commit ba403242615c2c99e27af7984b1650771a2cc2c9.

You missed your Signed-off-by tag.

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

[Aisheng Dong]

> From: haibo.chen@nxp.com <haibo.chen@nxp.com>
> Sent: Monday, June 8, 2020 6:00 PM
> 
> This patch block system booting, find on imx7d-sdb board.
> From the dts we can see, iomux and iomux_lpsr share the memory region
> [0x30330000-0x3033ffff], so will trigger the following issue:
> 
> [    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
> 
> This reverts commit ba403242615c2c99e27af7984b1650771a2cc2c9.

Better add your sign-off.
Otherwise:
Reviewed-by: Dong Aisheng <aisheng.dong@nxp.com>

Maybe you or Christophe could resubmit another proper fix for the original issue.

Regards
Aisheng

> ---
>  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

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

[Dan Carpenter]

On Mon, Jun 08, 2020 at 02:06:35PM +0000, Aisheng Dong wrote:
> > From: haibo.chen@nxp.com <haibo.chen@nxp.com>
> > Sent: Monday, June 8, 2020 6:00 PM
> > 
> > This patch block system booting, find on imx7d-sdb board.
> > From the dts we can see, iomux and iomux_lpsr share the memory region
> > [0x30330000-0x3033ffff], so will trigger the following issue:
> > 
> > [    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
> > 
> > This reverts commit ba403242615c2c99e27af7984b1650771a2cc2c9.
> 
> Better add your sign-off.
> Otherwise:
> Reviewed-by: Dong Aisheng <aisheng.dong@nxp.com>
> 
> Maybe you or Christophe could resubmit another proper fix for the original issue.

I'm really sorry about that.  This was largely my fault.

I still don't understand how commit ba403242615c caused a problem.

It sounds like in the original code ipctl->input_sel_base was released
somehow?  I do a `git grep input_sel_base` and it doesn't show anything.
What am I missing?

regards,
dan carpenter

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

[Dan Carpenter]

On Mon, Jun 08, 2020 at 02:06:35PM +0000, Aisheng Dong wrote:
> > From: haibo.chen@nxp.com <haibo.chen@nxp.com>
> > Sent: Monday, June 8, 2020 6:00 PM
> > 
> > This patch block system booting, find on imx7d-sdb board.
> > From the dts we can see, iomux and iomux_lpsr share the memory region
> > [0x30330000-0x3033ffff], so will trigger the following issue:
> > 
> > [    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
> > 
> > This reverts commit ba403242615c2c99e27af7984b1650771a2cc2c9.

Btw, the `git revert` command really sets you up for failure by
generating a patch in the wrong format.  You did well to write a good
commit message.  I would probably also change the subject, the From:
header and add a Fixes tag and a Signed-off by.  The Fixes tag should
be:

Fixes: ba403242615c ("pinctrl: freescale: imx: Use 'devm_of_iomap()' to avoid a resource leak in case of error in 'imx_pinctrl_probe()'")

regards,
dan carpenter

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

[BOUGH CHEN]

> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Sent: 2020年6月8日 22:45
> To: Aisheng Dong <aisheng.dong@nxp.com>
> Cc: BOUGH CHEN <haibo.chen@nxp.com>; festevam@gmail.com;
> shawnguo@kernel.org; stefan@agner.ch; kernel@pengutronix.de;
> linus.walleij@linaro.org; s.hauer@pengutronix.de; dl-linux-imx
> <linux-imx@nxp.com>; Christophe JAILLET <christophe.jaillet@wanadoo.fr>;
> linux-gpio@vger.kernel.org
> Subject: Re: [PATCH] Revert "pinctrl: freescale: imx: Use 'devm_of_iomap()' to
> avoid a resource leak in case of error in 'imx_pinctrl_probe()'"
> 
> On Mon, Jun 08, 2020 at 02:06:35PM +0000, Aisheng Dong wrote:
> > > From: haibo.chen@nxp.com <haibo.chen@nxp.com>
> > > Sent: Monday, June 8, 2020 6:00 PM
> > >
> > > This patch block system booting, find on imx7d-sdb board.
> > > From the dts we can see, iomux and iomux_lpsr share the memory
> > > region [0x30330000-0x3033ffff], so will trigger the following issue:
> > >
> > > [    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
> > >
> > > This reverts commit ba403242615c2c99e27af7984b1650771a2cc2c9.
> >
> > Better add your sign-off.
> > Otherwise:
> > Reviewed-by: Dong Aisheng <aisheng.dong@nxp.com>
> >
> > Maybe you or Christophe could resubmit another proper fix for the original
> issue.
> 
> I'm really sorry about that.  This was largely my fault.
> 
> I still don't understand how commit ba403242615c caused a problem.
> 
> It sounds like in the original code ipctl->input_sel_base was released somehow?
> I do a `git grep input_sel_base` and it doesn't show anything.
> What am I missing?
> 

Hi Dan,

I can give you a detail explain why this patch trigger issues.
Take our imx7d-sdb board as example, in dts file we can see to nodes:
440                         iomuxc_lpsr: iomuxc-lpsr@302c0000 {
441                                 compatible = "fsl,imx7d-iomuxc-lpsr";
442                                 reg = <0x302c0000 0x10000>;
443                                 fsl,input-sel = <&iomuxc>;
444                         };
And 
493                         iomuxc: pinctrl@30330000 {
494                                 compatible = "fsl,imx7d-iomuxc";
495                                 reg = <0x30330000 0x10000>;
496                         };

First time when probe the iomuxc_lpsr, due to it has "fsl,input-sel", so in the pinctrl driver, it will use API devm_of_iomap() to handle the iomuxc for the first time. Devm_of_iomap() use the API devm_request_mem_region() for the region <0x30330000-0x3033ffff>. 
Then, when probe the iomuxc, the pinctrl driver call the API devm_platform_ioremap_resource(), this API also call the devm_request_mem_region() for the same region <0x30330000-0x3033ffff>. 
So we see the log as following:
[    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

I will send a V2 patch to add my sign-off-by and fix tag.

Best Regards
Haibo Chen
> regards,
> dan carpenter

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

[Aisheng Dong]

> From: Dan Carpenter <dan.carpenter@oracle.com>
> Sent: Monday, June 8, 2020 10:49 PM
> 
> On Mon, Jun 08, 2020 at 02:06:35PM +0000, Aisheng Dong wrote:
> > > From: haibo.chen@nxp.com <haibo.chen@nxp.com>
> > > Sent: Monday, June 8, 2020 6:00 PM
> > >
> > > This patch block system booting, find on imx7d-sdb board.
> > > From the dts we can see, iomux and iomux_lpsr share the memory
> > > region [0x30330000-0x3033ffff], so will trigger the following issue:
> > >
> > > [    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
> > >
> > > This reverts commit ba403242615c2c99e27af7984b1650771a2cc2c9.
> 
> Btw, the `git revert` command really sets you up for failure by generating a patch
> in the wrong format.  You did well to write a good commit message.  I would
> probably also change the subject, the From:
> header and add a Fixes tag and a Signed-off by.  The Fixes tag should
> be:
> 
> Fixes: ba403242615c ("pinctrl: freescale: imx: Use 'devm_of_iomap()' to avoid a
> resource leak in case of error in 'imx_pinctrl_probe()'")
> 

By searching the kernel log, it seems most people didn't add Fixes tag for a Revert patch.
But anyway, I'm fine to add it.
e.g.
commit cf9c94456ebafc6d75a834e58dfdc8ae71a3acbc
Author: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Date:   Tue May 12 10:22:44 2020 +0200

    Revert "tty: hvc: Fix data abort due to race in hvc_open"
    
    This reverts commit e2bd1dcbe1aa34ff5570b3427c530e4332ecf0fe.
    
    In discussion on the mailing list, it has been determined that this is
    not the correct type of fix for this issue.  Revert it so that we can do
    this correctly.
    
    Reported-by: Jiri Slaby <jslaby@suse.cz>
    Reported-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
    Link: https://lore.kernel.org/r/20200428032601.22127-1-rananta@codeaurora.org
    Cc: Raghavendra Rao Ananta <rananta@codeaurora.org>
    Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Regards
Aisheng

> regards,
> dan carpenter

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

[Dan Carpenter]

On Tue, Jun 09, 2020 at 03:22:31AM +0000, Aisheng Dong wrote:
> > From: Dan Carpenter <dan.carpenter@oracle.com>
> > Sent: Monday, June 8, 2020 10:49 PM
> > 
> > On Mon, Jun 08, 2020 at 02:06:35PM +0000, Aisheng Dong wrote:
> > > > From: haibo.chen@nxp.com <haibo.chen@nxp.com>
> > > > Sent: Monday, June 8, 2020 6:00 PM
> > > >
> > > > This patch block system booting, find on imx7d-sdb board.
> > > > From the dts we can see, iomux and iomux_lpsr share the memory
> > > > region [0x30330000-0x3033ffff], so will trigger the following issue:
> > > >
> > > > [    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
> > > >
> > > > This reverts commit ba403242615c2c99e27af7984b1650771a2cc2c9.
> > 
> > Btw, the `git revert` command really sets you up for failure by generating a patch
> > in the wrong format.  You did well to write a good commit message.  I would
> > probably also change the subject, the From:
> > header and add a Fixes tag and a Signed-off by.  The Fixes tag should
> > be:
> > 
> > Fixes: ba403242615c ("pinctrl: freescale: imx: Use 'devm_of_iomap()' to avoid a
> > resource leak in case of error in 'imx_pinctrl_probe()'")
> > 
> 
> By searching the kernel log, it seems most people didn't add Fixes tag for a Revert patch.
> But anyway, I'm fine to add it.

Yeah.  It's really complicated to get revert patches right.  The revert
command was created 15 years ago and it doesn't match what we expect
from commits today.  Commit 40da7d9a93c8 ("NTB: Revert the change to use
the NTB device dev for DMA allocations") is an example of a well written
revert commit.

I'm sort of surprised that patches where the subject starts with Revert
don't break Greg's email sorting scripts.

regards,
dan carpenter

[PATCH] lib: devres: add a comment about the devm_of_iomap() function

[Dan Carpenter]

We recently introduced a bug when we tried to convert of_iomap() to
devm_of_iomap().  The problem was that there were two drivers mapping
the same io region.  The first driver was using of_iomap() and the
second driver was using devm_of_iomap() and the kernel booted fine.
When we converted the first drive to use devm_of_iomap() then the second
driver failed with -EBUSY and the kernel couldn't boot.

Let's add a comment to prevent this sort of mistake in the future.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 lib/devres.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/lib/devres.c b/lib/devres.c
index 6ef51f159c54b..0abe7c1cc4681 100644
--- a/lib/devres.c
+++ b/lib/devres.c
@@ -204,6 +204,12 @@ void __iomem *devm_ioremap_resource_wc(struct device *dev,
  *	base = devm_of_iomap(&pdev->dev, node, 0, NULL);
  *	if (IS_ERR(base))
  *		return PTR_ERR(base);
+ *
+ * Please Note: This is not a one-to-one replacement for of_iomap() because the
+ * of_iomap() function does not track whether the region is already mapped.  If
+ * two drivers try to map the same memory, the of_iomap() function will succeed
+ * but the the devm_of_iomap() function will return -EBUSY.
+ *
  */
 void __iomem *devm_of_iomap(struct device *dev, struct device_node *node, int index,
 			    resource_size_t *size)
-- 
2.26.2

[PATCH] lib: devres: add a comment about the devm_of_iomap() function

[Dan Carpenter]

We recently introduced a bug when we tried to convert of_iomap() to
devm_of_iomap().  The problem was that there were two drivers mapping
the same io region.  The first driver was using of_iomap() and the
second driver was using devm_of_iomap() and the kernel booted fine.
When we converted the first drive to use devm_of_iomap() then the second
driver failed with -EBUSY and the kernel couldn't boot.

Let's add a comment to prevent this sort of mistake in the future.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 lib/devres.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/lib/devres.c b/lib/devres.c
index 6ef51f159c54b..0abe7c1cc4681 100644
--- a/lib/devres.c
+++ b/lib/devres.c
@@ -204,6 +204,12 @@ void __iomem *devm_ioremap_resource_wc(struct device *dev,
  *	base = devm_of_iomap(&pdev->dev, node, 0, NULL);
  *	if (IS_ERR(base))
  *		return PTR_ERR(base);
+ *
+ * Please Note: This is not a one-to-one replacement for of_iomap() because the
+ * of_iomap() function does not track whether the region is already mapped.  If
+ * two drivers try to map the same memory, the of_iomap() function will succeed
+ * but the the devm_of_iomap() function will return -EBUSY.
+ *
  */
 void __iomem *devm_of_iomap(struct device *dev, struct device_node *node, int index,
 			    resource_size_t *size)
-- 
2.26.2

Re: [PATCH] lib: devres: add a comment about the devm_of_iomap() function

[Arnd Bergmann]

On Tue, Jun 9, 2020 at 12:47 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> We recently introduced a bug when we tried to convert of_iomap() to
> devm_of_iomap().  The problem was that there were two drivers mapping
> the same io region.  The first driver was using of_iomap() and the
> second driver was using devm_of_iomap() and the kernel booted fine.
> When we converted the first drive to use devm_of_iomap() then the second
> driver failed with -EBUSY and the kernel couldn't boot.
>
> Let's add a comment to prevent this sort of mistake in the future.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Good idea,

Acked-by: Arnd Bergmann <arnd@arndb.de>

Re: [PATCH] lib: devres: add a comment about the devm_of_iomap() function

[Arnd Bergmann]

On Tue, Jun 9, 2020 at 12:47 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> We recently introduced a bug when we tried to convert of_iomap() to
> devm_of_iomap().  The problem was that there were two drivers mapping
> the same io region.  The first driver was using of_iomap() and the
> second driver was using devm_of_iomap() and the kernel booted fine.
> When we converted the first drive to use devm_of_iomap() then the second
> driver failed with -EBUSY and the kernel couldn't boot.
>
> Let's add a comment to prevent this sort of mistake in the future.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Good idea,

Acked-by: Arnd Bergmann <arnd@arndb.de>

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

[Marion & Christophe JAILLET]

Le 08/06/2020 à 16:06, Aisheng Dong a écrit :
>> From: haibo.chen@nxp.com <haibo.chen@nxp.com>
>> Sent: Monday, June 8, 2020 6:00 PM
>>
>> This patch block system booting, find on imx7d-sdb board.
>>  From the dts we can see, iomux and iomux_lpsr share the memory region
>> [0x30330000-0x3033ffff], so will trigger the following issue:
>>
>> [    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
>>
>> This reverts commit ba403242615c2c99e27af7984b1650771a2cc2c9.
> Better add your sign-off.
> Otherwise:
> Reviewed-by: Dong Aisheng <aisheng.dong@nxp.com>

You can also add a "Apologies-From:" tag with my name :).
Sorry for this bogus patch.

CJ


> Maybe you or Christophe could resubmit another proper fix for the original issue.
>
> Regards
> Aisheng
>
>> ---
>>   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