| Message ID | 20241214040113.626275-1-joe@pf.is.s.u-tokyo.ac.jp (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | dmaengine: fsl-edma: implement the cleanup path of fsl_edma3_attach_pd() | expand |
On Sat, Dec 14, 2024 at 01:01:13PM +0900, Joe Hattori wrote: > Current implementation of fsl_edma3_attach_pd() does not provide a > cleanup path, resulting in memory leak. Thus, implement an error path of > fsl_edma3_attach_pd(), which detaches PM domains, removes device links, > and unwind the runtime PM settings. Also add fsl_edma3_detach_pd() and > call it on the error path of the .probe(). > > This bug was found by an experimental static analysis tool that I am > developing. static analysis often false alarm. Descript problem itself. for example how memory leak happen when call func1(), .. func2()... > > Fixes: 72f5801a4e2b ("dmaengine: fsl-edma: integrate v3 support") > Signed-off-by: Joe Hattori <joe@pf.is.s.u-tokyo.ac.jp> > --- > drivers/dma/fsl-edma-common.h | 1 + > drivers/dma/fsl-edma-main.c | 54 ++++++++++++++++++++++++++++------- > 2 files changed, 45 insertions(+), 10 deletions(-) > > diff --git a/drivers/dma/fsl-edma-common.h b/drivers/dma/fsl-edma-common.h > index ce37e1ee9c46..fe8f103d4a63 100644 > --- a/drivers/dma/fsl-edma-common.h > +++ b/drivers/dma/fsl-edma-common.h > @@ -166,6 +166,7 @@ struct fsl_edma_chan { > struct work_struct issue_worker; > struct platform_device *pdev; > struct device *pd_dev; > + struct device_link *pd_dev_link; > u32 srcid; > struct clk *clk; > int priority; > diff --git a/drivers/dma/fsl-edma-main.c b/drivers/dma/fsl-edma-main.c > index 60de1003193a..5204c06589ea 100644 > --- a/drivers/dma/fsl-edma-main.c > +++ b/drivers/dma/fsl-edma-main.c > @@ -420,7 +420,6 @@ MODULE_DEVICE_TABLE(of, fsl_edma_dt_ids); > static int fsl_edma3_attach_pd(struct platform_device *pdev, struct fsl_edma_engine *fsl_edma) > { > struct fsl_edma_chan *fsl_chan; > - struct device_link *link; > struct device *pd_chan; > struct device *dev; > int i; > @@ -436,15 +435,16 @@ static int fsl_edma3_attach_pd(struct platform_device *pdev, struct fsl_edma_eng > pd_chan = dev_pm_domain_attach_by_id(dev, i); > if (IS_ERR_OR_NULL(pd_chan)) { > dev_err(dev, "Failed attach pd %d\n", i); > - return -EINVAL; > + goto err; > } > > - link = device_link_add(dev, pd_chan, DL_FLAG_STATELESS | > + fsl_chan->pd_dev_link = device_link_add(dev, pd_chan, DL_FLAG_STATELESS | > DL_FLAG_PM_RUNTIME | > DL_FLAG_RPM_ACTIVE); > - if (!link) { > + if (!fsl_chan->pd_dev_link) { > dev_err(dev, "Failed to add device_link to %d\n", i); > - return -EINVAL; > + dev_pm_domain_detach(pd_chan, false); > + goto err; > } > > fsl_chan->pd_dev = pd_chan; > @@ -455,6 +455,33 @@ static int fsl_edma3_attach_pd(struct platform_device *pdev, struct fsl_edma_eng > } > > return 0; > +err: > + while (--i >= 0) { > + if (fsl_edma->chan_masked & BIT(i)) > + continue; > + fsl_chan = &fsl_edma->chans[i]; > + dev_pm_domain_detach(fsl_chan->pd_dev, false); > + device_link_del(fsl_chan->pd_dev_link); > + pm_runtime_dont_use_autosuspend(fsl_chan->pd_dev); > + pm_runtime_set_suspended(fsl_chan->pd_dev); > + } can you call helper funciton fsl_edma3_detach_pd()? > + return -EINVAL; > +} > + > +static void fsl_edma3_detach_pd(struct fsl_edma_engine *fsl_edma) > +{ > + struct fsl_edma_chan *fsl_chan; > + int i; > + > + for (i = 0; i < fsl_edma->n_chans; i++) { > + if (fsl_edma->chan_masked & BIT(i)) > + continue; > + fsl_chan = &fsl_edma->chans[i]; > + dev_pm_domain_detach(fsl_chan->pd_dev, false); > + device_link_del(fsl_chan->pd_dev_link); > + pm_runtime_dont_use_autosuspend(fsl_chan->pd_dev); > + pm_runtime_set_suspended(fsl_chan->pd_dev); > + } > } > > static int fsl_edma_probe(struct platform_device *pdev) > @@ -577,8 +604,10 @@ static int fsl_edma_probe(struct platform_device *pdev) > fsl_chan->clk = devm_clk_get_enabled(&pdev->dev, > (const char *)clk_name); > > - if (IS_ERR(fsl_chan->clk)) > - return PTR_ERR(fsl_chan->clk); > + if (IS_ERR(fsl_chan->clk)) { > + ret = PTR_ERR(fsl_chan->clk); > + goto err; > + } > } > fsl_chan->pdev = pdev; > vchan_init(&fsl_chan->vchan, &fsl_edma->dma_dev); > @@ -591,7 +620,7 @@ static int fsl_edma_probe(struct platform_device *pdev) > > ret = fsl_edma->drvdata->setup_irq(pdev, fsl_edma); > if (ret) > - return ret; > + goto err; > > dma_cap_set(DMA_PRIVATE, fsl_edma->dma_dev.cap_mask); > dma_cap_set(DMA_SLAVE, fsl_edma->dma_dev.cap_mask); > @@ -642,7 +671,7 @@ static int fsl_edma_probe(struct platform_device *pdev) > if (ret) { > dev_err(&pdev->dev, > "Can't register Freescale eDMA engine. (%d)\n", ret); > - return ret; > + goto err; > } > > ret = of_dma_controller_register(np, > @@ -652,7 +681,7 @@ static int fsl_edma_probe(struct platform_device *pdev) > dev_err(&pdev->dev, > "Can't register Freescale eDMA of_dma. (%d)\n", ret); > dma_async_device_unregister(&fsl_edma->dma_dev); > - return ret; > + goto err; > } > > /* enable round robin arbitration */ > @@ -660,6 +689,11 @@ static int fsl_edma_probe(struct platform_device *pdev) > edma_writel(fsl_edma, EDMA_CR_ERGA | EDMA_CR_ERCA, regs->cr); > > return 0; > + > +err: > + if (drvdata->flags & FSL_EDMA_DRV_HAS_PD) > + fsl_edma3_detach_pd(fsl_edma); > + return ret; Use devm_add_action() to avoid goto. Frank > } > > static void fsl_edma_remove(struct platform_device *pdev) > -- > 2.34.1 >
Thank you for your review. On 12/17/24 02:14, Frank Li wrote: > On Sat, Dec 14, 2024 at 01:01:13PM +0900, Joe Hattori wrote: >> Current implementation of fsl_edma3_attach_pd() does not provide a >> cleanup path, resulting in memory leak. Thus, implement an error path of >> fsl_edma3_attach_pd(), which detaches PM domains, removes device links, >> and unwind the runtime PM settings. Also add fsl_edma3_detach_pd() and >> call it on the error path of the .probe(). >> >> This bug was found by an experimental static analysis tool that I am >> developing. > > static analysis often false alarm. Descript problem itself. for example > how memory leak happen when call func1(), .. func2()... Updated the commit message in the v2 patch. > >> >> Fixes: 72f5801a4e2b ("dmaengine: fsl-edma: integrate v3 support") >> Signed-off-by: Joe Hattori <joe@pf.is.s.u-tokyo.ac.jp> >> --- >> drivers/dma/fsl-edma-common.h | 1 + >> drivers/dma/fsl-edma-main.c | 54 ++++++++++++++++++++++++++++------- >> 2 files changed, 45 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/dma/fsl-edma-common.h b/drivers/dma/fsl-edma-common.h >> index ce37e1ee9c46..fe8f103d4a63 100644 >> --- a/drivers/dma/fsl-edma-common.h >> +++ b/drivers/dma/fsl-edma-common.h >> @@ -166,6 +166,7 @@ struct fsl_edma_chan { >> struct work_struct issue_worker; >> struct platform_device *pdev; >> struct device *pd_dev; >> + struct device_link *pd_dev_link; >> u32 srcid; >> struct clk *clk; >> int priority; >> diff --git a/drivers/dma/fsl-edma-main.c b/drivers/dma/fsl-edma-main.c >> index 60de1003193a..5204c06589ea 100644 >> --- a/drivers/dma/fsl-edma-main.c >> +++ b/drivers/dma/fsl-edma-main.c >> @@ -420,7 +420,6 @@ MODULE_DEVICE_TABLE(of, fsl_edma_dt_ids); >> static int fsl_edma3_attach_pd(struct platform_device *pdev, struct fsl_edma_engine *fsl_edma) >> { >> struct fsl_edma_chan *fsl_chan; >> - struct device_link *link; >> struct device *pd_chan; >> struct device *dev; >> int i; >> @@ -436,15 +435,16 @@ static int fsl_edma3_attach_pd(struct platform_device *pdev, struct fsl_edma_eng >> pd_chan = dev_pm_domain_attach_by_id(dev, i); >> if (IS_ERR_OR_NULL(pd_chan)) { >> dev_err(dev, "Failed attach pd %d\n", i); >> - return -EINVAL; >> + goto err; >> } >> >> - link = device_link_add(dev, pd_chan, DL_FLAG_STATELESS | >> + fsl_chan->pd_dev_link = device_link_add(dev, pd_chan, DL_FLAG_STATELESS | >> DL_FLAG_PM_RUNTIME | >> DL_FLAG_RPM_ACTIVE); >> - if (!link) { >> + if (!fsl_chan->pd_dev_link) { >> dev_err(dev, "Failed to add device_link to %d\n", i); >> - return -EINVAL; >> + dev_pm_domain_detach(pd_chan, false); >> + goto err; >> } >> >> fsl_chan->pd_dev = pd_chan; >> @@ -455,6 +455,33 @@ static int fsl_edma3_attach_pd(struct platform_device *pdev, struct fsl_edma_eng >> } >> >> return 0; >> +err: >> + while (--i >= 0) { >> + if (fsl_edma->chan_masked & BIT(i)) >> + continue; >> + fsl_chan = &fsl_edma->chans[i]; >> + dev_pm_domain_detach(fsl_chan->pd_dev, false); >> + device_link_del(fsl_chan->pd_dev_link); >> + pm_runtime_dont_use_autosuspend(fsl_chan->pd_dev); >> + pm_runtime_set_suspended(fsl_chan->pd_dev); >> + } > > can you call helper funciton fsl_edma3_detach_pd()? Makes sense, addressed in the v2 patch. > >> + return -EINVAL; >> +} >> + >> +static void fsl_edma3_detach_pd(struct fsl_edma_engine *fsl_edma) >> +{ >> + struct fsl_edma_chan *fsl_chan; >> + int i; >> + >> + for (i = 0; i < fsl_edma->n_chans; i++) { >> + if (fsl_edma->chan_masked & BIT(i)) >> + continue; >> + fsl_chan = &fsl_edma->chans[i]; >> + dev_pm_domain_detach(fsl_chan->pd_dev, false); >> + device_link_del(fsl_chan->pd_dev_link); >> + pm_runtime_dont_use_autosuspend(fsl_chan->pd_dev); >> + pm_runtime_set_suspended(fsl_chan->pd_dev); >> + } >> } >> >> static int fsl_edma_probe(struct platform_device *pdev) >> @@ -577,8 +604,10 @@ static int fsl_edma_probe(struct platform_device *pdev) >> fsl_chan->clk = devm_clk_get_enabled(&pdev->dev, >> (const char *)clk_name); >> >> - if (IS_ERR(fsl_chan->clk)) >> - return PTR_ERR(fsl_chan->clk); >> + if (IS_ERR(fsl_chan->clk)) { >> + ret = PTR_ERR(fsl_chan->clk); >> + goto err; >> + } >> } >> fsl_chan->pdev = pdev; >> vchan_init(&fsl_chan->vchan, &fsl_edma->dma_dev); >> @@ -591,7 +620,7 @@ static int fsl_edma_probe(struct platform_device *pdev) >> >> ret = fsl_edma->drvdata->setup_irq(pdev, fsl_edma); >> if (ret) >> - return ret; >> + goto err; >> >> dma_cap_set(DMA_PRIVATE, fsl_edma->dma_dev.cap_mask); >> dma_cap_set(DMA_SLAVE, fsl_edma->dma_dev.cap_mask); >> @@ -642,7 +671,7 @@ static int fsl_edma_probe(struct platform_device *pdev) >> if (ret) { >> dev_err(&pdev->dev, >> "Can't register Freescale eDMA engine. (%d)\n", ret); >> - return ret; >> + goto err; >> } >> >> ret = of_dma_controller_register(np, >> @@ -652,7 +681,7 @@ static int fsl_edma_probe(struct platform_device *pdev) >> dev_err(&pdev->dev, >> "Can't register Freescale eDMA of_dma. (%d)\n", ret); >> dma_async_device_unregister(&fsl_edma->dma_dev); >> - return ret; >> + goto err; >> } >> >> /* enable round robin arbitration */ >> @@ -660,6 +689,11 @@ static int fsl_edma_probe(struct platform_device *pdev) >> edma_writel(fsl_edma, EDMA_CR_ERGA | EDMA_CR_ERCA, regs->cr); >> >> return 0; >> + >> +err: >> + if (drvdata->flags & FSL_EDMA_DRV_HAS_PD) >> + fsl_edma3_detach_pd(fsl_edma); >> + return ret; > > Use devm_add_action() to avoid goto. Fixed in the v2 patch. > > Frank > >> } >> >> static void fsl_edma_remove(struct platform_device *pdev) >> -- >> 2.34.1 >> Best, Joe
diff --git a/drivers/dma/fsl-edma-common.h b/drivers/dma/fsl-edma-common.h index ce37e1ee9c46..fe8f103d4a63 100644 --- a/drivers/dma/fsl-edma-common.h +++ b/drivers/dma/fsl-edma-common.h @@ -166,6 +166,7 @@ struct fsl_edma_chan { struct work_struct issue_worker; struct platform_device *pdev; struct device *pd_dev; + struct device_link *pd_dev_link; u32 srcid; struct clk *clk; int priority; diff --git a/drivers/dma/fsl-edma-main.c b/drivers/dma/fsl-edma-main.c index 60de1003193a..5204c06589ea 100644 --- a/drivers/dma/fsl-edma-main.c +++ b/drivers/dma/fsl-edma-main.c @@ -420,7 +420,6 @@ MODULE_DEVICE_TABLE(of, fsl_edma_dt_ids); static int fsl_edma3_attach_pd(struct platform_device *pdev, struct fsl_edma_engine *fsl_edma) { struct fsl_edma_chan *fsl_chan; - struct device_link *link; struct device *pd_chan; struct device *dev; int i; @@ -436,15 +435,16 @@ static int fsl_edma3_attach_pd(struct platform_device *pdev, struct fsl_edma_eng pd_chan = dev_pm_domain_attach_by_id(dev, i); if (IS_ERR_OR_NULL(pd_chan)) { dev_err(dev, "Failed attach pd %d\n", i); - return -EINVAL; + goto err; } - link = device_link_add(dev, pd_chan, DL_FLAG_STATELESS | + fsl_chan->pd_dev_link = device_link_add(dev, pd_chan, DL_FLAG_STATELESS | DL_FLAG_PM_RUNTIME | DL_FLAG_RPM_ACTIVE); - if (!link) { + if (!fsl_chan->pd_dev_link) { dev_err(dev, "Failed to add device_link to %d\n", i); - return -EINVAL; + dev_pm_domain_detach(pd_chan, false); + goto err; } fsl_chan->pd_dev = pd_chan; @@ -455,6 +455,33 @@ static int fsl_edma3_attach_pd(struct platform_device *pdev, struct fsl_edma_eng } return 0; +err: + while (--i >= 0) { + if (fsl_edma->chan_masked & BIT(i)) + continue; + fsl_chan = &fsl_edma->chans[i]; + dev_pm_domain_detach(fsl_chan->pd_dev, false); + device_link_del(fsl_chan->pd_dev_link); + pm_runtime_dont_use_autosuspend(fsl_chan->pd_dev); + pm_runtime_set_suspended(fsl_chan->pd_dev); + } + return -EINVAL; +} + +static void fsl_edma3_detach_pd(struct fsl_edma_engine *fsl_edma) +{ + struct fsl_edma_chan *fsl_chan; + int i; + + for (i = 0; i < fsl_edma->n_chans; i++) { + if (fsl_edma->chan_masked & BIT(i)) + continue; + fsl_chan = &fsl_edma->chans[i]; + dev_pm_domain_detach(fsl_chan->pd_dev, false); + device_link_del(fsl_chan->pd_dev_link); + pm_runtime_dont_use_autosuspend(fsl_chan->pd_dev); + pm_runtime_set_suspended(fsl_chan->pd_dev); + } } static int fsl_edma_probe(struct platform_device *pdev) @@ -577,8 +604,10 @@ static int fsl_edma_probe(struct platform_device *pdev) fsl_chan->clk = devm_clk_get_enabled(&pdev->dev, (const char *)clk_name); - if (IS_ERR(fsl_chan->clk)) - return PTR_ERR(fsl_chan->clk); + if (IS_ERR(fsl_chan->clk)) { + ret = PTR_ERR(fsl_chan->clk); + goto err; + } } fsl_chan->pdev = pdev; vchan_init(&fsl_chan->vchan, &fsl_edma->dma_dev); @@ -591,7 +620,7 @@ static int fsl_edma_probe(struct platform_device *pdev) ret = fsl_edma->drvdata->setup_irq(pdev, fsl_edma); if (ret) - return ret; + goto err; dma_cap_set(DMA_PRIVATE, fsl_edma->dma_dev.cap_mask); dma_cap_set(DMA_SLAVE, fsl_edma->dma_dev.cap_mask); @@ -642,7 +671,7 @@ static int fsl_edma_probe(struct platform_device *pdev) if (ret) { dev_err(&pdev->dev, "Can't register Freescale eDMA engine. (%d)\n", ret); - return ret; + goto err; } ret = of_dma_controller_register(np, @@ -652,7 +681,7 @@ static int fsl_edma_probe(struct platform_device *pdev) dev_err(&pdev->dev, "Can't register Freescale eDMA of_dma. (%d)\n", ret); dma_async_device_unregister(&fsl_edma->dma_dev); - return ret; + goto err; } /* enable round robin arbitration */ @@ -660,6 +689,11 @@ static int fsl_edma_probe(struct platform_device *pdev) edma_writel(fsl_edma, EDMA_CR_ERGA | EDMA_CR_ERCA, regs->cr); return 0; + +err: + if (drvdata->flags & FSL_EDMA_DRV_HAS_PD) + fsl_edma3_detach_pd(fsl_edma); + return ret; } static void fsl_edma_remove(struct platform_device *pdev)
Current implementation of fsl_edma3_attach_pd() does not provide a cleanup path, resulting in memory leak. Thus, implement an error path of fsl_edma3_attach_pd(), which detaches PM domains, removes device links, and unwind the runtime PM settings. Also add fsl_edma3_detach_pd() and call it on the error path of the .probe(). This bug was found by an experimental static analysis tool that I am developing. Fixes: 72f5801a4e2b ("dmaengine: fsl-edma: integrate v3 support") Signed-off-by: Joe Hattori <joe@pf.is.s.u-tokyo.ac.jp> --- drivers/dma/fsl-edma-common.h | 1 + drivers/dma/fsl-edma-main.c | 54 ++++++++++++++++++++++++++++------- 2 files changed, 45 insertions(+), 10 deletions(-)