Re: [PATCH v3 21/34] dmaengine: zynqmp-dma: switch completion tasklet to dmaengine BH

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

Pre-existing issues:
- [Critical] Dropping the channel lock during list iteration allows concurrent list modifications, leading to an infinite loop and severe memory corruption.
- [High] Pre-existing PM runtime reference count leak in the probe error path.
--

--- Patch [21]: [PATCH v3 21/34] dmaengine: zynqmp-dma: switch completion tasklet to dmaengine BH ---
commit 31456cd82dd8bdc7d99216cfcc194b9aecb47fb3
Author: Allen Pais <[email protected]>

dmaengine: zynqmp-dma: switch completion tasklet to dmaengine BH

This commit replaces the per-channel tasklet with the shared dmaengine BH
helper. The handler continues to run in softirq context while dmaengine
owns the common scheduling and teardown mechanism.

> diff --git a/drivers/dma/xilinx/zynqmp_dma.c b/drivers/dma/xilinx/zynqmp_dma.c
> index f6a812e49ddca..e396a35f0e0d5 100644
> --- a/drivers/dma/xilinx/zynqmp_dma.c
> +++ b/drivers/dma/xilinx/zynqmp_dma.c

[ ... ]

[Severity: High]
This is a pre-existing issue, but I noticed a potential regression in the
probe error path.

In zynqmp_dma_probe(), if pm_runtime_resume_and_get() succeeds, it
increments the usage count. If a subsequent initialization step like
zynqmp_dma_chan_probe() fails, the execution jumps to err_disable_pm:

drivers/dma/xilinx/zynqmp_dma.c:zynqmp_dma_probe() {
    ...
	ret = pm_runtime_resume_and_get(zdev->dev);
    ...
	ret = zynqmp_dma_chan_probe(zdev, pdev);
	if (ret) {
		dev_err_probe(&pdev->dev, ret, "Probing channel failed\n");
		goto err_disable_pm;
	}
    ...
err_disable_pm:
	if (!pm_runtime_enabled(zdev->dev))
		zynqmp_dma_runtime_suspend(zdev->dev);
	pm_runtime_disable(zdev->dev);
	return ret;
}

Because pm_runtime_enabled(zdev->dev) is true, it skips calling
zynqmp_dma_runtime_suspend(zdev->dev). The function then calls
pm_runtime_disable() and returns the error.

Does this regression miss calling pm_runtime_put_noidle() to balance the
usage count or taking any action to power off the device?


[Severity: Critical]
This is a pre-existing issue, but I noticed a regression when iterating
over the done list.

In zynqmp_dma_chan_desc_cleanup(), the code iterates over chan->done_list
using list_for_each_entry_safe(). For each descriptor, it temporarily
drops chan->lock to invoke the client callback:

drivers/dma/xilinx/zynqmp_dma.c:zynqmp_dma_chan_desc_cleanup() {
    ...
	list_for_each_entry_safe(desc, next, &chan->done_list, node) {
		struct dmaengine_desc_callback cb;

		dmaengine_desc_get_callback(&desc->async_tx, &cb);
		if (dmaengine_desc_callback_valid(&cb)) {
			spin_unlock_irqrestore(&chan->lock, irqflags);
			dmaengine_desc_callback_invoke(&cb, NULL);
			spin_lock_irqsave(&chan->lock, irqflags);
		}
    ...
}

If a concurrent thread or the callback itself calls
dmaengine_terminate_all(), zynqmp_dma_free_descriptors() will execute.
This acquires the lock and unconditionally moves all descriptors from
done_list (including the cached next descriptor) to free_list.

When the callback completes, the loop resumes with desc = next. Because
next is now linked in free_list, the loop termination condition
&desc->node != &chan->done_list will never be met.

Could this regression cause an infinite loop and arbitrary memory
corruption as it iterates off the end of free_list?

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