[PATCHv3] dmaengine: at_hdmac: use devm APIs in probe()

[PATCHv3] dmaengine: at_hdmac: use devm APIs in probe()

[Rosen Penev]

Convert resource allocation and setup in probe() to use devm-managed APIs.
Simplify the error handling path and removes explicit cleanup code.

Assisted-by: opencode:big-pickle
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 v3: fix subject and description
 v2: remove list_del
 drivers/dma/at_hdmac.c | 50 ++++++++++--------------------------------
 1 file changed, 11 insertions(+), 39 deletions(-)

diff --git a/drivers/dma/at_hdmac.c b/drivers/dma/at_hdmac.c
index 971f8bc9ec48..49a85ac66e83 100644
--- a/drivers/dma/at_hdmac.c
+++ b/drivers/dma/at_hdmac.c
@@ -1986,40 +1986,34 @@ static int __init at_dma_probe(struct platform_device *pdev)
 	atdma->dma_device.cap_mask = plat_dat->cap_mask;
 	atdma->all_chan_mask = (1 << plat_dat->nr_channels) - 1;
 
-	atdma->clk = devm_clk_get(&pdev->dev, "dma_clk");
+	atdma->clk = devm_clk_get_enabled(&pdev->dev, "dma_clk");
 	if (IS_ERR(atdma->clk))
 		return PTR_ERR(atdma->clk);
 
-	err = clk_prepare_enable(atdma->clk);
-	if (err)
-		return err;
-
 	/* force dma off, just in case */
 	at_dma_off(atdma);
 
-	err = request_irq(irq, at_dma_interrupt, 0, "at_hdmac", atdma);
+	err = devm_request_irq(&pdev->dev, irq, at_dma_interrupt, 0, "at_hdmac", atdma);
 	if (err)
-		goto err_irq;
+		return err;
 
 	platform_set_drvdata(pdev, atdma);
 
 	/* create a pool of consistent memory blocks for hardware descriptors */
-	atdma->lli_pool = dma_pool_create("at_hdmac_lli_pool",
+	atdma->lli_pool = dmam_pool_create("at_hdmac_lli_pool",
 					  &pdev->dev, sizeof(struct at_lli),
 					  4 /* word alignment */, 0);
 	if (!atdma->lli_pool) {
 		dev_err(&pdev->dev, "Unable to allocate DMA LLI descriptor pool\n");
-		err = -ENOMEM;
-		goto err_desc_pool_create;
+		return -ENOMEM;
 	}
 
 	/* create a pool of consistent memory blocks for memset blocks */
-	atdma->memset_pool = dma_pool_create("at_hdmac_memset_pool",
+	atdma->memset_pool = dmam_pool_create("at_hdmac_memset_pool",
 					     &pdev->dev, sizeof(int), 4, 0);
 	if (!atdma->memset_pool) {
 		dev_err(&pdev->dev, "No memory for memset dma pool\n");
-		err = -ENOMEM;
-		goto err_memset_pool_create;
+		return -ENOMEM;
 	}
 
 	/* clear any pending interrupt */
@@ -2086,10 +2080,10 @@ static int __init at_dma_probe(struct platform_device *pdev)
 	  dma_has_cap(DMA_SLAVE, atdma->dma_device.cap_mask)  ? "slave " : "",
 	  plat_dat->nr_channels);
 
-	err = dma_async_device_register(&atdma->dma_device);
+	err = dmaenginem_async_device_register(&atdma->dma_device);
 	if (err) {
 		dev_err(&pdev->dev, "Unable to register: %d.\n", err);
-		goto err_dma_async_device_register;
+		return err;
 	}
 
 	/*
@@ -2098,27 +2092,15 @@ static int __init at_dma_probe(struct platform_device *pdev)
 	 * dma_request_channel().
 	 */
 	if (pdev->dev.of_node) {
-		err = of_dma_controller_register(pdev->dev.of_node,
+		err = devm_of_dma_controller_register(&pdev->dev, pdev->dev.of_node,
 						 at_dma_xlate, atdma);
 		if (err) {
 			dev_err(&pdev->dev, "could not register of_dma_controller\n");
-			goto err_of_dma_controller_register;
+			return err;
 		}
 	}
 
 	return 0;
-
-err_of_dma_controller_register:
-	dma_async_device_unregister(&atdma->dma_device);
-err_dma_async_device_register:
-	dma_pool_destroy(atdma->memset_pool);
-err_memset_pool_create:
-	dma_pool_destroy(atdma->lli_pool);
-err_desc_pool_create:
-	free_irq(platform_get_irq(pdev, 0), atdma);
-err_irq:
-	clk_disable_unprepare(atdma->clk);
-	return err;
 }
 
 static void at_dma_remove(struct platform_device *pdev)
@@ -2127,22 +2109,12 @@ static void at_dma_remove(struct platform_device *pdev)
 	struct dma_chan		*chan, *_chan;
 
 	at_dma_off(atdma);
-	if (pdev->dev.of_node)
-		of_dma_controller_free(pdev->dev.of_node);
-	dma_async_device_unregister(&atdma->dma_device);
-
-	dma_pool_destroy(atdma->memset_pool);
-	dma_pool_destroy(atdma->lli_pool);
-	free_irq(platform_get_irq(pdev, 0), atdma);
 
 	list_for_each_entry_safe(chan, _chan, &atdma->dma_device.channels,
 			device_node) {
 		/* Disable interrupts */
 		atc_disable_chan_irq(atdma, chan->chan_id);
-		list_del(&chan->device_node);
 	}
-
-	clk_disable_unprepare(atdma->clk);
 }
 
 static void at_dma_shutdown(struct platform_device *pdev)
-- 
2.55.0

Re: [PATCHv3] dmaengine: at_hdmac: use devm APIs in probe()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] The driver fails to kill `vchan` tasklets during driver removal and probe error paths, risking a use-after-free.
- [High] Devres reverse-cleanup order destroys DMA pools before freeing the IRQ, risking a use-after-free with concurrent ISRs.
--

--- Patch [1]: [PATCHv3] dmaengine: at_hdmac: use devm APIs in probe() ---
commit 100916adb459d4781232f09cf49ed4747fa5f88a
Author: Rosen Penev <rosenp@gmail.com>

dmaengine: at_hdmac: use devm APIs in probe()

Convert resource allocation and setup in probe() to use devm-managed APIs.
Simplify the error handling path and removes explicit cleanup code.

> diff --git a/drivers/dma/at_hdmac.c b/drivers/dma/at_hdmac.c
> index e5b30a57c477a..f16cf29ca5487 100644
> --- a/drivers/dma/at_hdmac.c
> +++ b/drivers/dma/at_hdmac.c
> @@ -1980,40 +1980,34 @@ static int __init at_dma_probe(struct platform_device *pdev)
>  	atdma->dma_device.cap_mask = plat_dat->cap_mask;
>  	atdma->all_chan_mask = (1 << plat_dat->nr_channels) - 1;
>  
> -	atdma->clk = devm_clk_get(&pdev->dev, "dma_clk");
> +	atdma->clk = devm_clk_get_enabled(&pdev->dev, "dma_clk");
>  	if (IS_ERR(atdma->clk))
>  		return PTR_ERR(atdma->clk);
>  
> -	err = clk_prepare_enable(atdma->clk);
> -	if (err)
> -		return err;
> -
>  	/* force dma off, just in case */
>  	at_dma_off(atdma);
>  
> -	err = request_irq(irq, at_dma_interrupt, 0, "at_hdmac", atdma);
> +	err = devm_request_irq(&pdev->dev, irq, at_dma_interrupt, 0, "at_hdmac", atdma);

[Severity: High]
This isn't a bug introduced by this patch, but requesting the IRQ here before
the DMA pools are created establishes an unsafe devres cleanup order.

Because devres destroys resources in reverse order, devm will destroy the
DMA pools before freeing the IRQ.

If an ISR is actively running on another CPU during driver unbind and enters
an error path, could it dereference the already-freed pool memory, leading to
a use-after-free?

>  	if (err)
> -		goto err_irq;
> +		return err;
>  
>  	platform_set_drvdata(pdev, atdma);
>  
>  	/* create a pool of consistent memory blocks for hardware descriptors */
> -	atdma->lli_pool = dma_pool_create("at_hdmac_lli_pool",
> +	atdma->lli_pool = dmam_pool_create("at_hdmac_lli_pool",
>  					  &pdev->dev, sizeof(struct at_lli),
>  					  4 /* word alignment */, 0);

[ ... ]

> @@ -2121,22 +2103,12 @@ static void at_dma_remove(struct platform_device *pdev)
>  	struct dma_chan		*chan, *_chan;
>  
>  	at_dma_off(atdma);
> -	if (pdev->dev.of_node)
> -		of_dma_controller_free(pdev->dev.of_node);
> -	dma_async_device_unregister(&atdma->dma_device);
> -
> -	dma_pool_destroy(atdma->memset_pool);
> -	dma_pool_destroy(atdma->lli_pool);
> -	free_irq(platform_get_irq(pdev, 0), atdma);
>  
>  	list_for_each_entry_safe(chan, _chan, &atdma->dma_device.channels,
>  			device_node) {
>  		/* Disable interrupts */
>  		atc_disable_chan_irq(atdma, chan->chan_id);
> -		list_del(&chan->device_node);
>  	}

[Severity: High]
This is a pre-existing issue, but this driver unbind path does not synchronously
kill pending vchan tasklets before returning.

If an interrupt fires during or just before unbind, the IRQ handler might
schedule the channel's tasklet.

While at_dma_remove() masks interrupts, it doesn't wait for or kill already
scheduled tasklets via tasklet_kill(). Is there a risk that devres will free
the memory while the tasklet continues executing, resulting in a use-after-free?

> -
> -	clk_disable_unprepare(atdma->clk);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260817201200.10952-1-rosenp@gmail.com?part=1

Re: [PATCHv3] dmaengine: at_hdmac: use devm APIs in probe()

[Frank Li]

On Mon, Aug 17, 2026 at 01:12:00PM -0700, Rosen Penev wrote:
> Convert resource allocation and setup in probe() to use devm-managed APIs.
> Simplify the error handling path and removes explicit cleanup code.
>
> Assisted-by: opencode:big-pickle
> Signed-off-by: Rosen Penev <rosenp@gmail.com>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>  v3: fix subject and description
>  v2: remove list_del
>  drivers/dma/at_hdmac.c | 50 ++++++++++--------------------------------
>  1 file changed, 11 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/dma/at_hdmac.c b/drivers/dma/at_hdmac.c
> index 971f8bc9ec48..49a85ac66e83 100644
> --- a/drivers/dma/at_hdmac.c
> +++ b/drivers/dma/at_hdmac.c
> @@ -1986,40 +1986,34 @@ static int __init at_dma_probe(struct platform_device *pdev)
>  	atdma->dma_device.cap_mask = plat_dat->cap_mask;
>  	atdma->all_chan_mask = (1 << plat_dat->nr_channels) - 1;
>
> -	atdma->clk = devm_clk_get(&pdev->dev, "dma_clk");
> +	atdma->clk = devm_clk_get_enabled(&pdev->dev, "dma_clk");
>  	if (IS_ERR(atdma->clk))
>  		return PTR_ERR(atdma->clk);
>
> -	err = clk_prepare_enable(atdma->clk);
> -	if (err)
> -		return err;
> -
>  	/* force dma off, just in case */
>  	at_dma_off(atdma);
>
> -	err = request_irq(irq, at_dma_interrupt, 0, "at_hdmac", atdma);
> +	err = devm_request_irq(&pdev->dev, irq, at_dma_interrupt, 0, "at_hdmac", atdma);
>  	if (err)
> -		goto err_irq;
> +		return err;
>
>  	platform_set_drvdata(pdev, atdma);
>
>  	/* create a pool of consistent memory blocks for hardware descriptors */
> -	atdma->lli_pool = dma_pool_create("at_hdmac_lli_pool",
> +	atdma->lli_pool = dmam_pool_create("at_hdmac_lli_pool",
>  					  &pdev->dev, sizeof(struct at_lli),
>  					  4 /* word alignment */, 0);
>  	if (!atdma->lli_pool) {
>  		dev_err(&pdev->dev, "Unable to allocate DMA LLI descriptor pool\n");
> -		err = -ENOMEM;
> -		goto err_desc_pool_create;
> +		return -ENOMEM;
>  	}
>
>  	/* create a pool of consistent memory blocks for memset blocks */
> -	atdma->memset_pool = dma_pool_create("at_hdmac_memset_pool",
> +	atdma->memset_pool = dmam_pool_create("at_hdmac_memset_pool",
>  					     &pdev->dev, sizeof(int), 4, 0);
>  	if (!atdma->memset_pool) {
>  		dev_err(&pdev->dev, "No memory for memset dma pool\n");
> -		err = -ENOMEM;
> -		goto err_memset_pool_create;
> +		return -ENOMEM;
>  	}
>
>  	/* clear any pending interrupt */
> @@ -2086,10 +2080,10 @@ static int __init at_dma_probe(struct platform_device *pdev)
>  	  dma_has_cap(DMA_SLAVE, atdma->dma_device.cap_mask)  ? "slave " : "",
>  	  plat_dat->nr_channels);
>
> -	err = dma_async_device_register(&atdma->dma_device);
> +	err = dmaenginem_async_device_register(&atdma->dma_device);
>  	if (err) {
>  		dev_err(&pdev->dev, "Unable to register: %d.\n", err);
> -		goto err_dma_async_device_register;
> +		return err;
>  	}
>
>  	/*
> @@ -2098,27 +2092,15 @@ static int __init at_dma_probe(struct platform_device *pdev)
>  	 * dma_request_channel().
>  	 */
>  	if (pdev->dev.of_node) {
> -		err = of_dma_controller_register(pdev->dev.of_node,
> +		err = devm_of_dma_controller_register(&pdev->dev, pdev->dev.of_node,
>  						 at_dma_xlate, atdma);
>  		if (err) {
>  			dev_err(&pdev->dev, "could not register of_dma_controller\n");
> -			goto err_of_dma_controller_register;
> +			return err;
>  		}
>  	}
>
>  	return 0;
> -
> -err_of_dma_controller_register:
> -	dma_async_device_unregister(&atdma->dma_device);
> -err_dma_async_device_register:
> -	dma_pool_destroy(atdma->memset_pool);
> -err_memset_pool_create:
> -	dma_pool_destroy(atdma->lli_pool);
> -err_desc_pool_create:
> -	free_irq(platform_get_irq(pdev, 0), atdma);
> -err_irq:
> -	clk_disable_unprepare(atdma->clk);
> -	return err;
>  }
>
>  static void at_dma_remove(struct platform_device *pdev)
> @@ -2127,22 +2109,12 @@ static void at_dma_remove(struct platform_device *pdev)
>  	struct dma_chan		*chan, *_chan;
>
>  	at_dma_off(atdma);
> -	if (pdev->dev.of_node)
> -		of_dma_controller_free(pdev->dev.of_node);
> -	dma_async_device_unregister(&atdma->dma_device);
> -
> -	dma_pool_destroy(atdma->memset_pool);
> -	dma_pool_destroy(atdma->lli_pool);
> -	free_irq(platform_get_irq(pdev, 0), atdma);
>
>  	list_for_each_entry_safe(chan, _chan, &atdma->dma_device.channels,
>  			device_node) {
>  		/* Disable interrupts */
>  		atc_disable_chan_irq(atdma, chan->chan_id);
> -		list_del(&chan->device_node);
>  	}
> -
> -	clk_disable_unprepare(atdma->clk);
>  }
>
>  static void at_dma_shutdown(struct platform_device *pdev)
> --
> 2.55.0
>