Re: [PATCH v3 5/5] dmaengine: mcf-edma: Use devm for per-channel IRQ registration

[sashiko-bot@kernel.org]

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

New issues:
- [High] Replacing explicit `free_irq()` with `devm_request_irq()` introduces a Use-After-Free race condition by deferring IRQ unregistration until after the virtual channels are torn down.

Pre-existing issues:
- [High] dma_async_device_unregister() fails to clean up channels because fsl_edma_cleanup_vchan() improperly empties the channel list beforehand, leading to a resource leak.
--

commit 3174cb73d4f28e4270bffd2960b8600186152672
Author: Jean-Michel Hautbois <jeanmichel.hautbois@yoseli.org>

dmaengine: mcf-edma: Use devm for per-channel IRQ registration

This patch converts the eDMA transfer interrupt registration to use the
managed devm_request_irq() API with per-channel names. It also adds logic
in the remove path to manually quiesce the hardware since the interrupts
are now freed later by devres.

> diff --git a/drivers/dma/mcf-edma-main.c b/drivers/dma/mcf-edma-main.c
> --- a/drivers/dma/mcf-edma-main.c
> +++ b/drivers/dma/mcf-edma-main.c

[ ... ]

> @@ -249,8 +234,21 @@ static int mcf_edma_probe(struct platform_device *pdev)
>  static void mcf_edma_remove(struct platform_device *pdev)
>  {
>  	struct fsl_edma_engine *mcf_edma = platform_get_drvdata(pdev);
> +	struct edma_regs *regs = &mcf_edma->regs;
> +	int i;
> +
> +	/*
> +	 * The per-channel interrupts are requested with devm and are only
> +	 * freed after this function returns.  Quiesce the controller first so
> +	 * that no interrupt can fire while the virtual channels are torn down:
> +	 * disable every channel's request and acknowledge any pending
> +	 * interrupt.
> +	 */
> +	for (i = 0; i < mcf_edma->n_chans; i++)
> +		fsl_edma_disable_request(&mcf_edma->chans[i]);
> +	iowrite32(~0, regs->inth);
> +	iowrite32(~0, regs->intl);
>  
> -	mcf_edma_irq_free(pdev, mcf_edma);
>  	fsl_edma_cleanup_vchan(&mcf_edma->dma_dev);

[Severity: High]
Does deferring the IRQ unregistration until after the virtual channels are
torn down introduce a use-after-free race?

Since devm_request_irq() defers freeing the IRQ until after mcf_edma_remove()
returns, the driver loses the synchronization barrier that previously waited
for in-flight handlers. The manual hardware quiescence loop does not
synchronize with already executing handlers.

If an interrupt handler executes on another CPU, could it complete the transfer
and call vchan_cookie_complete(), which executes tasklet_schedule()?

If this happens after fsl_edma_cleanup_vchan() kills the tasklet:

fsl_edma_cleanup_vchan()
        tasklet_kill(&chan->vchan.task);

the tasklet could be placed back on the runqueue. After remove() completes,
devres frees the IRQ and device memory, meaning the scheduled tasklet would
execute on freed memory.

>  	dma_async_device_unregister(&mcf_edma->dma_dev);

[Severity: High]
This isn't a bug introduced by this patch, but does the order of operations
here leave dma_async_device_unregister() operating on an empty list?

Looking at fsl_edma_cleanup_vchan(), it iterates over dmadev->channels and
removes every channel from the list:

fsl_edma_cleanup_vchan() {
        list_for_each_entry_safe(...) {
                list_del(&chan->vchan.chan.device_node);
                ...
        }
}

Immediately after, dma_async_device_unregister() is called, which relies on
iterating over the exact same device->channels list to unregister the
channels.

Because the list was just emptied, the iteration does zero passes, skipping
the core channel unregistration and permanently leaking sysfs entries and
per-cpu memory.

>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260625-b4-edma-dmaengine-v3-0-44be00ace37d@yoseli.org?part=5