Re: [PATCHv4] dmaengine: at_hdmac: use devm APIs in at_dma_probe()

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

Pre-existing issues:
- [High] Pending virtual channel tasklets are not explicitly killed before freeing driver resources during module unload or error paths.
- [High] Incorrect teardown order destroys DMA pools before freeing the IRQ, risking a use-after-free during concurrent IRQ handling.
--

--- Patch [1]: [PATCHv4] dmaengine: at_hdmac: use devm APIs in at_dma_probe() ---
commit 5e07523a5a1740589f78ad9b3692760b7a57f76e
Author: Rosen Penev <[email protected]>

dmaengine: at_hdmac: use devm APIs in at_dma_probe()

Convert resource allocation and setup in probe() to use devm-managed APIs.
Simplify the error handling path and remove 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);
>  	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;
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but converting to devres APIs
highlights a pre-existing issue with the teardown order.

In at_dma_probe(), devm_request_irq() is called before dmam_pool_create().
Because devres actions execute in reverse order, dmam_pool_destroy() will be
invoked before the IRQ is freed during unbind.

While at_dma_remove() calls at_dma_off() to disable hardware interrupts,
this doesn't synchronize with an already-executing IRQ handler on another
CPU. If an IRQ handler is actively running when unbind is initiated, devres
will destroy the descriptor pools while the handler might still attempt to
access them.

Should the IRQ be requested after the pools are created, or perhaps use a
manual devm_add_action() to ensure the IRQ is freed before the pools are
destroyed?

[ ... ]

> @@ -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);
>  	}
> -
> -	clk_disable_unprepare(atdma->clk);
>  }

[Severity: High]
This is a pre-existing issue, but does this function need to kill the virtual
channel tasklets before driver teardown completes?

The driver uses vchan_init(), which sets up a completion tasklet. During
module unload or unbind, neither the explicit remove path nor the devres
cleanup calls tasklet_kill() (e.g., via vchan_free()).

If a DMA interrupt fires and schedules a tasklet just before at_dma_remove()
executes (or before the IRQ is finally freed by devres), the tasklet can
remain pending. When the devres cleanup ultimately frees the atdma memory
structure, the pending tasklet could execute and access the freed memory,
leading to a use-after-free.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1