* [PATCH] memory: fsl_ifc: fix leak of irq and nand_irq in fsl_ifc_ctrl_probe
@ 2021-08-03 7:51 Dongliang Mu
2021-08-03 7:57 ` Krzysztof Kozlowski
0 siblings, 1 reply; 5+ messages in thread
From: Dongliang Mu @ 2021-08-03 7:51 UTC (permalink / raw)
To: Krzysztof Kozlowski; +Cc: Dongliang Mu, linux-kernel
In fsl_ifc_ctrl_probe, if fsl_ifc_ctrl_init fails, we should free the
resources allocated by irq_of_parse_and_map.
Fix this by adjusting the error handling code.
Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
---
drivers/memory/fsl_ifc.c | 19 ++++++++++++-------
1 file changed, 12 insertions(+), 7 deletions(-)
diff --git a/drivers/memory/fsl_ifc.c b/drivers/memory/fsl_ifc.c
index d062c2f8250f..391390dd3dcb 100644
--- a/drivers/memory/fsl_ifc.c
+++ b/drivers/memory/fsl_ifc.c
@@ -258,12 +258,17 @@ static int fsl_ifc_ctrl_probe(struct platform_device *dev)
/* get the nand machine irq */
fsl_ifc_ctrl_dev->nand_irq =
irq_of_parse_and_map(dev->dev.of_node, 1);
+ if (fsl_ifc_ctrl_dev->nand_irq == 0) {
+ dev_err(&dev->dev, "failed to get nand_irq resource for IFC\n");
+ ret = -ENODEV;
+ goto err_unmap_irq;
+ }
fsl_ifc_ctrl_dev->dev = &dev->dev;
ret = fsl_ifc_ctrl_init(fsl_ifc_ctrl_dev);
if (ret < 0)
- goto err;
+ goto err_unmap_nandirq;
init_waitqueue_head(&fsl_ifc_ctrl_dev->nand_wait);
@@ -272,7 +277,7 @@ static int fsl_ifc_ctrl_probe(struct platform_device *dev)
if (ret != 0) {
dev_err(&dev->dev, "failed to install irq (%d)\n",
fsl_ifc_ctrl_dev->irq);
- goto err_irq;
+ goto err_unmap_nandirq;
}
if (fsl_ifc_ctrl_dev->nand_irq) {
@@ -281,17 +286,17 @@ static int fsl_ifc_ctrl_probe(struct platform_device *dev)
if (ret != 0) {
dev_err(&dev->dev, "failed to install irq (%d)\n",
fsl_ifc_ctrl_dev->nand_irq);
- goto err_nandirq;
+ goto err_free_irq;
}
}
return 0;
-err_nandirq:
- free_irq(fsl_ifc_ctrl_dev->nand_irq, fsl_ifc_ctrl_dev);
- irq_dispose_mapping(fsl_ifc_ctrl_dev->nand_irq);
-err_irq:
+err_free_irq:
free_irq(fsl_ifc_ctrl_dev->irq, fsl_ifc_ctrl_dev);
+err_unmap_nandirq:
+ irq_dispose_mapping(fsl_ifc_ctrl_dev->nand_irq);
+err_unmap_irq:
irq_dispose_mapping(fsl_ifc_ctrl_dev->irq);
err:
iounmap(fsl_ifc_ctrl_dev->gregs);
--
2.25.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] memory: fsl_ifc: fix leak of irq and nand_irq in fsl_ifc_ctrl_probe
2021-08-03 7:51 [PATCH] memory: fsl_ifc: fix leak of irq and nand_irq in fsl_ifc_ctrl_probe Dongliang Mu
@ 2021-08-03 7:57 ` Krzysztof Kozlowski
2021-08-03 9:28 ` Dongliang Mu
0 siblings, 1 reply; 5+ messages in thread
From: Krzysztof Kozlowski @ 2021-08-03 7:57 UTC (permalink / raw)
To: Dongliang Mu; +Cc: linux-kernel
On 03/08/2021 09:51, Dongliang Mu wrote:
> In fsl_ifc_ctrl_probe, if fsl_ifc_ctrl_init fails, we should free the
> resources allocated by irq_of_parse_and_map.
Your code is doing much more. You also touch nand_irq, not only
fsl_ifc_ctrl_init(). This looks incorrect as IRQ is optional, isn't it?
The problem is entirely different than you described here - the error
paths of fsl_ifc_ctrl_init() and request_irq() are wrong. They do not
release resources in proper paths.
Best regards,
Krzysztof
>
> Fix this by adjusting the error handling code.
>
> Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
> ---
> drivers/memory/fsl_ifc.c | 19 ++++++++++++-------
> 1 file changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/memory/fsl_ifc.c b/drivers/memory/fsl_ifc.c
> index d062c2f8250f..391390dd3dcb 100644
> --- a/drivers/memory/fsl_ifc.c
> +++ b/drivers/memory/fsl_ifc.c
> @@ -258,12 +258,17 @@ static int fsl_ifc_ctrl_probe(struct platform_device *dev)
> /* get the nand machine irq */
> fsl_ifc_ctrl_dev->nand_irq =
> irq_of_parse_and_map(dev->dev.of_node, 1);
> + if (fsl_ifc_ctrl_dev->nand_irq == 0) {
> + dev_err(&dev->dev, "failed to get nand_irq resource for IFC\n");
> + ret = -ENODEV;
> + goto err_unmap_irq;
> + }
>
> fsl_ifc_ctrl_dev->dev = &dev->dev;
>
> ret = fsl_ifc_ctrl_init(fsl_ifc_ctrl_dev);
> if (ret < 0)
> - goto err;
> + goto err_unmap_nandirq;
>
> init_waitqueue_head(&fsl_ifc_ctrl_dev->nand_wait);
>
> @@ -272,7 +277,7 @@ static int fsl_ifc_ctrl_probe(struct platform_device *dev)
> if (ret != 0) {
> dev_err(&dev->dev, "failed to install irq (%d)\n",
> fsl_ifc_ctrl_dev->irq);
> - goto err_irq;
> + goto err_unmap_nandirq;
> }
>
> if (fsl_ifc_ctrl_dev->nand_irq) {
> @@ -281,17 +286,17 @@ static int fsl_ifc_ctrl_probe(struct platform_device *dev)
> if (ret != 0) {
> dev_err(&dev->dev, "failed to install irq (%d)\n",
> fsl_ifc_ctrl_dev->nand_irq);
> - goto err_nandirq;
> + goto err_free_irq;
> }
> }
>
> return 0;
>
> -err_nandirq:
> - free_irq(fsl_ifc_ctrl_dev->nand_irq, fsl_ifc_ctrl_dev);
> - irq_dispose_mapping(fsl_ifc_ctrl_dev->nand_irq);
> -err_irq:
> +err_free_irq:
> free_irq(fsl_ifc_ctrl_dev->irq, fsl_ifc_ctrl_dev);
> +err_unmap_nandirq:
> + irq_dispose_mapping(fsl_ifc_ctrl_dev->nand_irq);
> +err_unmap_irq:
> irq_dispose_mapping(fsl_ifc_ctrl_dev->irq);
> err:
> iounmap(fsl_ifc_ctrl_dev->gregs);
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] memory: fsl_ifc: fix leak of irq and nand_irq in fsl_ifc_ctrl_probe
2021-08-03 7:57 ` Krzysztof Kozlowski
@ 2021-08-03 9:28 ` Dongliang Mu
2021-08-03 9:51 ` Krzysztof Kozlowski
0 siblings, 1 reply; 5+ messages in thread
From: Dongliang Mu @ 2021-08-03 9:28 UTC (permalink / raw)
To: Krzysztof Kozlowski; +Cc: linux-kernel
On Tue, Aug 3, 2021 at 3:57 PM Krzysztof Kozlowski
<krzysztof.kozlowski@canonical.com> wrote:
>
> On 03/08/2021 09:51, Dongliang Mu wrote:
> > In fsl_ifc_ctrl_probe, if fsl_ifc_ctrl_init fails, we should free the
> > resources allocated by irq_of_parse_and_map.
>
> Your code is doing much more. You also touch nand_irq, not only
> fsl_ifc_ctrl_init(). This looks incorrect as IRQ is optional, isn't it?
>
> The problem is entirely different than you described here - the error
> paths of fsl_ifc_ctrl_init() and request_irq() are wrong. They do not
> release resources in proper paths.
Yes, you're right :). This patch rewrites the whole error handling
code. Any comment on the code changes?
I will rewrite the commit message in the patch v2.
BTW, there is a minor issue: if request_irq fails, we should not
invoke free_irq.
>
>
>
> Best regards,
> Krzysztof
>
> >
> > Fix this by adjusting the error handling code.
> >
> > Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com>
> > ---
> > drivers/memory/fsl_ifc.c | 19 ++++++++++++-------
> > 1 file changed, 12 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/memory/fsl_ifc.c b/drivers/memory/fsl_ifc.c
> > index d062c2f8250f..391390dd3dcb 100644
> > --- a/drivers/memory/fsl_ifc.c
> > +++ b/drivers/memory/fsl_ifc.c
> > @@ -258,12 +258,17 @@ static int fsl_ifc_ctrl_probe(struct platform_device *dev)
> > /* get the nand machine irq */
> > fsl_ifc_ctrl_dev->nand_irq =
> > irq_of_parse_and_map(dev->dev.of_node, 1);
> > + if (fsl_ifc_ctrl_dev->nand_irq == 0) {
> > + dev_err(&dev->dev, "failed to get nand_irq resource for IFC\n");
> > + ret = -ENODEV;
> > + goto err_unmap_irq;
> > + }
> >
> > fsl_ifc_ctrl_dev->dev = &dev->dev;
> >
> > ret = fsl_ifc_ctrl_init(fsl_ifc_ctrl_dev);
> > if (ret < 0)
> > - goto err;
> > + goto err_unmap_nandirq;
> >
> > init_waitqueue_head(&fsl_ifc_ctrl_dev->nand_wait);
> >
> > @@ -272,7 +277,7 @@ static int fsl_ifc_ctrl_probe(struct platform_device *dev)
> > if (ret != 0) {
> > dev_err(&dev->dev, "failed to install irq (%d)\n",
> > fsl_ifc_ctrl_dev->irq);
> > - goto err_irq;
> > + goto err_unmap_nandirq;
> > }
> >
> > if (fsl_ifc_ctrl_dev->nand_irq) {
> > @@ -281,17 +286,17 @@ static int fsl_ifc_ctrl_probe(struct platform_device *dev)
> > if (ret != 0) {
> > dev_err(&dev->dev, "failed to install irq (%d)\n",
> > fsl_ifc_ctrl_dev->nand_irq);
> > - goto err_nandirq;
> > + goto err_free_irq;
> > }
> > }
> >
> > return 0;
> >
> > -err_nandirq:
> > - free_irq(fsl_ifc_ctrl_dev->nand_irq, fsl_ifc_ctrl_dev);
> > - irq_dispose_mapping(fsl_ifc_ctrl_dev->nand_irq);
> > -err_irq:
> > +err_free_irq:
> > free_irq(fsl_ifc_ctrl_dev->irq, fsl_ifc_ctrl_dev);
> > +err_unmap_nandirq:
> > + irq_dispose_mapping(fsl_ifc_ctrl_dev->nand_irq);
> > +err_unmap_irq:
> > irq_dispose_mapping(fsl_ifc_ctrl_dev->irq);
> > err:
> > iounmap(fsl_ifc_ctrl_dev->gregs);
> >
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] memory: fsl_ifc: fix leak of irq and nand_irq in fsl_ifc_ctrl_probe
2021-08-03 9:28 ` Dongliang Mu
@ 2021-08-03 9:51 ` Krzysztof Kozlowski
2021-08-03 10:55 ` Dongliang Mu
0 siblings, 1 reply; 5+ messages in thread
From: Krzysztof Kozlowski @ 2021-08-03 9:51 UTC (permalink / raw)
To: Dongliang Mu; +Cc: linux-kernel
On 03/08/2021 11:28, Dongliang Mu wrote:
> On Tue, Aug 3, 2021 at 3:57 PM Krzysztof Kozlowski
> <krzysztof.kozlowski@canonical.com> wrote:
>>
>> On 03/08/2021 09:51, Dongliang Mu wrote:
>>> In fsl_ifc_ctrl_probe, if fsl_ifc_ctrl_init fails, we should free the
>>> resources allocated by irq_of_parse_and_map.
>>
>> Your code is doing much more. You also touch nand_irq, not only
>> fsl_ifc_ctrl_init(). This looks incorrect as IRQ is optional, isn't it?
>>
>> The problem is entirely different than you described here - the error
>> paths of fsl_ifc_ctrl_init() and request_irq() are wrong. They do not
>> release resources in proper paths.
>
> Yes, you're right :). This patch rewrites the whole error handling
> code. Any comment on the code changes?
I did not check the exact error paths, I assume you are going to make
them correct. Therefore only the nand_irq which looks optional and your
code makes it required.
>
> I will rewrite the commit message in the patch v2.
>
> BTW, there is a minor issue: if request_irq fails, we should not
> invoke free_irq.
>
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] memory: fsl_ifc: fix leak of irq and nand_irq in fsl_ifc_ctrl_probe
2021-08-03 9:51 ` Krzysztof Kozlowski
@ 2021-08-03 10:55 ` Dongliang Mu
0 siblings, 0 replies; 5+ messages in thread
From: Dongliang Mu @ 2021-08-03 10:55 UTC (permalink / raw)
To: Krzysztof Kozlowski; +Cc: linux-kernel
On Tue, Aug 3, 2021 at 5:51 PM Krzysztof Kozlowski
<krzysztof.kozlowski@canonical.com> wrote:
>
> On 03/08/2021 11:28, Dongliang Mu wrote:
> > On Tue, Aug 3, 2021 at 3:57 PM Krzysztof Kozlowski
> > <krzysztof.kozlowski@canonical.com> wrote:
> >>
> >> On 03/08/2021 09:51, Dongliang Mu wrote:
> >>> In fsl_ifc_ctrl_probe, if fsl_ifc_ctrl_init fails, we should free the
> >>> resources allocated by irq_of_parse_and_map.
> >>
> >> Your code is doing much more. You also touch nand_irq, not only
> >> fsl_ifc_ctrl_init(). This looks incorrect as IRQ is optional, isn't it?
> >>
> >> The problem is entirely different than you described here - the error
> >> paths of fsl_ifc_ctrl_init() and request_irq() are wrong. They do not
> >> release resources in proper paths.
> >
> > Yes, you're right :). This patch rewrites the whole error handling
> > code. Any comment on the code changes?
>
> I did not check the exact error paths, I assume you are going to make
> them correct. Therefore only the nand_irq which looks optional and your
> code makes it required.
Well, yes. It seems unnecessary. I will revert this part of the code
changes and send a v2 patch.
>
> >
> > I will rewrite the commit message in the patch v2.
> >
> > BTW, there is a minor issue: if request_irq fails, we should not
> > invoke free_irq.
> >
>
>
> Best regards,
> Krzysztof
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-08-03 10:56 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-03 7:51 [PATCH] memory: fsl_ifc: fix leak of irq and nand_irq in fsl_ifc_ctrl_probe Dongliang Mu
2021-08-03 7:57 ` Krzysztof Kozlowski
2021-08-03 9:28 ` Dongliang Mu
2021-08-03 9:51 ` Krzysztof Kozlowski
2021-08-03 10:55 ` Dongliang Mu
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).