diff mbox series

dmaengine: fsl-edma: implement the cleanup path of fsl_edma3_attach_pd()

Message ID 20241214040113.626275-1-joe@pf.is.s.u-tokyo.ac.jp (mailing list archive)
State New
Headers show
Series dmaengine: fsl-edma: implement the cleanup path of fsl_edma3_attach_pd() | expand

Commit Message

Joe Hattori Dec. 14, 2024, 4:01 a.m. UTC 
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(-)

Comments

Frank Li Dec. 16, 2024, 5:14 p.m. UTC | #1 
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
>
diff mbox series

Patch

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)