Re: [PATCHv3 01/15] dmaengine: fsldma: kill tasklet before removing channel

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Unhandled Error Return from dma_async_device_register() leads to silent probe failure and resource leaks.
- [High] Off-by-one error in the fsldma_request_irqs() error unwind path attempts to free the IRQ that just failed to be requested.
- [High] Double unlock in fsldma_suspend_late() error path causes unbalanced spinlocks.
- [High] Client callback is invoked with the channel descriptor spinlock held, which can lead to a self-deadlock.
--

commit 89849d3f67e5b5c4ccce67310ff8f24602b221dc
Author: Rosen Penev <rosenp@gmail.com>

dmaengine: fsldma: kill tasklet before removing channel
    
Add tasklet_kill() in fsl_dma_chan_remove() to prevent a race
where the tasklet, scheduled by the IRQ handler, runs after
the channel has been freed.

> diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c
> index 22d62d958abd9..0e2f84862261c 100644
> --- a/drivers/dma/fsldma.c
> +++ b/drivers/dma/fsldma.c
> @@ -1205,6 +1205,7 @@ static int fsl_dma_chan_probe(struct fsldma_device *fdev,
>  
>  static void fsl_dma_chan_remove(struct fsldma_chan *chan)
>  {
> +	tasklet_kill(&chan->tasklet);
>  	irq_dispose_mapping(chan->irq);
>  	list_del(&chan->common.device_node);
>  	iounmap(chan->regs);

[Severity: High]
This is a pre-existing issue, but does fsldma_of_probe() ignore the return
value of dma_async_device_register()?

If registration fails due to an IDA allocation error, the probe function
unconditionally returns 0, falsely reporting success to the driver core.
Later, when the device is removed, fsldma_of_remove() will call
dma_async_device_unregister() on a device that was never fully registered.

Can this corrupt IDA states or trigger assertions and kernel panics?

Will allocated resources like memory, IRQs, and iomappings leak because the
error cleanup path is bypassed?

drivers/dma/fsldma.c:fsldma_of_probe() {
    ...
	dma_async_device_register(&fdev->common);
	return 0;
}

[Severity: High]
This is a pre-existing issue, but could the rollback loop in
fsldma_request_irqs() be missing a decrement of 'i' before the first
iteration?

If request_irq() fails for a channel at index 'i', the code jumps to
out_unwind and begins the cleanup loop starting at the exact same index 'i'.
It unconditionally calls free_irq() for this channel. Because request_irq()
just failed, the IRQ was never successfully allocated.

Will calling free_irq() on an unrequested IRQ trigger a kernel warning?

drivers/dma/fsldma.c:fsldma_request_irqs() {
    ...
out_unwind:
	for (/* none */; i >= 0; i--) {
		chan = fdev->chan[i];
		if (!chan)
			continue;

		if (!chan->irq)
			continue;

		free_irq(chan->irq, chan);
	}
}

[Severity: High]
This is a pre-existing issue, but are we double unlocking chan->desc_lock
in the error path of fsldma_suspend_late()?

In fsldma_suspend_late(), the forward loop acquires the lock, updates state,
and releases it. If a subsequent channel is not idle, it jumps to out: and
iterates backward, unlocking all previously processed channels:

drivers/dma/fsldma.c:fsldma_suspend_late() {
    ...
out:
	for (; i >= 0; i--) {
		chan = fdev->chan[i];
		if (!chan)
			continue;
		chan->pm_state = RUNNING;
		spin_unlock_bh(&chan->desc_lock);
	}
}

Since the locks for these earlier channels were already released during the
forward iteration, might this corrupt the lock state and local_bh preemption
count, leading to warnings and potential crashes?

[Severity: High]
This is a pre-existing issue, but is the client callback invoked with the
channel descriptor spinlock held?

In dma_do_tasklet(), chan->desc_lock is acquired before cleaning up completed
descriptors:

drivers/dma/fsldma.c:dma_do_tasklet() {
    ...
	spin_lock(&chan->desc_lock);

	/* the hardware is now idle and ready for more */
	chan->idle = true;

	/* Run all cleanup for descriptors which have been completed */
	fsldma_cleanup_descriptors(chan);
    ...
}

The cleanup chain eventually calls dmaengine_desc_get_callback_invoke()
without dropping the spinlock:

drivers/dma/fsldma.c:fsldma_run_tx_complete_actions() {
    ...
		/* Run the link descriptor callback function */
		dmaengine_desc_get_callback_invoke(txd, NULL);
    ...
}

If the client callback attempts to submit new transfers, it reaches
fsl_dma_tx_submit() which attempts to acquire the same non-recursive lock:

drivers/dma/fsldma.c:fsl_dma_tx_submit() {
    ...
	spin_lock_bh(&chan->desc_lock);
    ...
}

Could this lead to a self-deadlock?

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