| Message ID | 1591610401-12590-1-git-send-email-haibo.chen@nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series | Revert "pinctrl: freescale: imx: Use 'devm_of_iomap()' to avoid a resource leak in case of error in 'imx_pinctrl_probe()'" | expand |
+ 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
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.
> 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
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
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
> -----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
> 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
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
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
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; } } }
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(-)