Re: [PATCHv2] dmaengine: fsldma: convert to platform_get_irq_optional()

[Frank Li <Frank.li@oss.nxp.com>]

On Thu, Sep 10, 2026 at 08:58:44PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
>
> New issues:
> - [Low] The patch introduces an unused label `out_iounmap:`, which will cause a compiler warning and break kernel builds configured with -Werror.

Did you build it?

Frank

>
> Pre-existing issues:
> - [Critical] Missing `tasklet_kill()` during channel removal can lead to a use-after-free of the channel structure memory.
> - [High] The driver ignores the return value of `dma_async_device_register()`, leading to a silent failure state and potential kernel oops on module removal.
> - [High] The error path in `fsldma_suspend_late` unlocks `desc_lock` for channels that were already unlocked, leading to a double-unlock kernel panic.
> --
>
> commit 8cf71591d829b94cb515a6922619751103de2f0d
> Author: Rosen Penev <rosenp@gmail.com>
>
> dmaengine: fsldma: convert to platform_get_irq_optional()
>
> Replace the per-controller irq_of_parse_and_map() call with
> platform_get_irq_optional(). The controller IRQ is optional when absent
> and the driver falls back to per-channel IRQs. The corresponding
> irq_dispose_mapping() calls in the probe error path and remove function
> are removed.
>
> > diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c
> > index 22d62d958abd9..0637a7b2e14d0 100644
> > --- a/drivers/dma/fsldma.c
> > +++ b/drivers/dma/fsldma.c
>
> [ ... ]
>
> > @@ -1300,7 +1307,7 @@ static int fsldma_of_probe(struct platform_device *op)
> >  		if (fdev->chan[i])
> >  			fsl_dma_chan_remove(fdev->chan[i]);
>
> [Severity: Critical]
> This is a pre-existing issue, but does fsl_dma_chan_remove() need to call
> tasklet_kill() before freeing the channel memory?
>
> drivers/dma/fsldma.c:fsl_dma_chan_remove() {
> 	irq_dispose_mapping(chan->irq);
> 	list_del(&chan->common.device_node);
> 	iounmap(chan->regs);
> 	kfree(chan);
> }
>
> A tasklet scheduled by the device's IRQ handler can execute after this frees
> the channel memory via kfree(chan). The free_irq() function waits for
> executing IRQ handlers but does not wait for or kill scheduled tasklets,
> potentially leading to a use-after-free.
>
> >  	}
> > -	irq_dispose_mapping(fdev->irq);
> > +out_iounmap:
>
> [Severity: Low]
> Does this introduce an unused label?
>
> The patch introduces out_iounmap:, but nothing seems to jump to it, which
> will cause a compiler warning and break kernel builds configured with -Werror.
>
> >  	iounmap(fdev->regs);
> >  out_free:
> >  	kfree(fdev);
>
> [ ... ]
>
>
> [Severity: High]
> This is a pre-existing issue, but does the driver ignore the return value of
> dma_async_device_register() in fsldma_of_probe()?
>
> drivers/dma/fsldma.c:fsldma_of_probe() {
>     ...
> 	dma_async_device_register(&fdev->common);
> 	return 0;
>
> out_free_fdev:
>     ...
> }
>
> If dma_async_device_register() fails, it returns a negative error code,
> leaving fields like the device IDA and refcount uninitialized or zeroed.
> Because the driver ignores this failure, it will complete the probe
> successfully.
>
> During module removal, dma_async_device_unregister() will execute on these
> uninitialized internal fields, potentially triggering an oops.
>
>
> [Severity: High]
> This is a pre-existing issue, but does fsldma_suspend_late() unlock
> desc_lock for channels that were already unlocked?
>
> drivers/dma/fsldma.c:fsldma_suspend_late() {
>     ...
> 	for (i = 0; i < FSL_DMA_MAX_CHANS_PER_DEVICE; i++) {
>         ...
> 		spin_lock_bh(&chan->desc_lock);
> 		if (unlikely(!chan->idle))
> 			goto out;
>         ...
> 		spin_unlock_bh(&chan->desc_lock);
> 	}
> 	return 0;
>
> out:
> 	for (; i >= 0; i--) {
> 		chan = fdev->chan[i];
> 		if (!chan)
> 			continue;
> 		chan->pm_state = RUNNING;
> 		spin_unlock_bh(&chan->desc_lock);
> 	}
> 	return -EBUSY;
> }
>
> In the forward loop, spin_unlock_bh(&chan->desc_lock) is correctly called on
> successful iterations.
>
> If a subsequent channel is not idle, the code jumps to out and iterates
> backwards from the failing channel down to 0, unconditionally calling
> spin_unlock_bh(&chan->desc_lock) for all previously processed channels.
>
> Can this unlock locks that were already released during the forward loop's
> successful iterations, leading to a double-unlock panic?
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260910204625.55367-1-rosenp@gmail.com?part=1