* Re: [PATCHv2] dmaengine: fsldma: kill tasklet before removing channel
2026-09-10 20:28 [PATCHv2] dmaengine: fsldma: kill tasklet before removing channel Rosen Penev
@ 2026-09-10 20:43 ` sashiko-bot
0 siblings, 0 replies; 2+ messages in thread
From: sashiko-bot @ 2026-09-10 20:43 UTC (permalink / raw)
To: Rosen Penev; +Cc: vkoul, Frank.Li, dmaengine
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
Pre-existing issues:
- [High] The error path in fsldma_request_irqs() calls free_irq() on an IRQ that just failed to be requested, triggering a kernel WARNING.
- [High] fsldma_of_probe() completely ignores the return value of dma_async_device_register(), leading to silent failures and potential list corruption.
- [High] Recursive deadlock when invoking client callbacks or resolving DMA dependencies in fsldma_run_tx_complete_actions().
- [High] Unbalanced spin_unlock_bh() on the error path in fsldma_suspend_late(), causing a double unlock.
- [High] DMA engine is not halted during channel removal, potentially causing memory corruption.
--
commit 6e64fb2982240feb5c9b3c6e8b290c5fad43ac6d
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 is scheduled by the IRQ handler and 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);
[Severity: High]
This is a pre-existing issue, but does this function neglect to halt the
hardware DMA controller before tearing down the software channel? If the driver
is unbound while a DMA transfer is in progress, couldn't the hardware continue
executing transactions and corrupt memory after the descriptors and IO
registers are destroyed?
> irq_dispose_mapping(chan->irq);
> list_del(&chan->common.device_node);
> iounmap(chan->regs);
[ ... ]
drivers/dma/fsldma.c:fsldma_request_irqs() {
...
ret = request_irq(chan->irq, fsldma_chan_irq, IRQF_SHARED,
"fsldma-chan", chan);
if (ret) {
chan_err(chan, "unable to request per-channel IRQ\n");
goto out_unwind;
}
}
return 0;
out_unwind:
for (/* none */; i >= 0; i--) {
chan = fdev->chan[i];
if (!chan)
continue;
if (!chan->irq)
continue;
free_irq(chan->irq, chan);
}
return ret;
}
[Severity: High]
This is a pre-existing issue, but since the index 'i' is not decremented
before entering the out_unwind loop, could this call free_irq() on the channel
that just failed initialization? This might trigger a kernel warning for trying
to free an already-free IRQ.
drivers/dma/fsldma.c:fsldma_of_probe() {
...
err = fsldma_request_irqs(fdev);
if (err) {
dev_err(fdev->dev, "unable to request IRQs\n");
goto out_free_fdev;
}
dma_async_device_register(&fdev->common);
return 0;
}
[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, returning success
could lead to a NULL pointer dereference later during device removal when
dma_async_device_unregister() is called on an uninitialized device.
drivers/dma/fsldma.c:fsldma_run_tx_complete_actions() {
...
if (txd->cookie > 0) {
ret = txd->cookie;
dma_descriptor_unmap(txd);
/* Run the link descriptor callback function */
dmaengine_desc_get_callback_invoke(txd, NULL);
}
/* Run any dependencies */
dma_run_dependencies(txd);
...
}
[Severity: High]
This is a pre-existing issue, but is there a potential recursive deadlock
when invoking client callbacks? fsldma_cleanup_descriptors() holds
chan->desc_lock when calling fsldma_run_tx_complete_actions(). If a client
callback or dependency submits a new transaction to the same channel,
fsl_dma_tx_submit() will try to acquire the same chan->desc_lock.
drivers/dma/fsldma.c:fsldma_suspend_late() {
...
for (i = 0; i < FSL_DMA_MAX_CHANS_PER_DEVICE; i++) {
chan = fdev->chan[i];
if (!chan)
continue;
spin_lock_bh(&chan->desc_lock);
if (unlikely(!chan->idle))
goto out;
chan->regs_save.mr = get_mr(chan);
chan->pm_state = SUSPENDED;
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);
}
...
}
[Severity: High]
This is a pre-existing issue, but does the error path in fsldma_suspend_late()
perform a double-unlock? For channels prior to the failing index 'i', the lock
was already correctly released in the first loop. Does the unconditional
spin_unlock_bh() in the out loop cause a double-unlock for those previously
processed channels?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260910202846.48941-1-rosenp@gmail.com?part=1
^ permalink raw reply [flat|nested] 2+ messages in thread