Re: [PATCH 1/2] dmaengine: zynqmp_dma: fix race between runtime PM and device removal

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The probe error path (`err_disable_pm`) fails to drop the PM runtime usage counter and leaves hardware clocks enabled if PM runtime is active.
- [High] Memory and PM runtime usage counter leak in `zynqmp_dma_alloc_chan_resources()` on error paths.
- [High] Device removal and shutdown paths disable clocks without first halting the DMA hardware, which can cause an AXI bus hang.
- [High] In `zynqmp_dma_prep_memcpy`, a 0-length transfer results in an invalid descriptor allocation count leading to free_list memory corruption.
- [High] Missing dma_wmb() before clearing the STOP bit when appending DMA descriptors.
--

commit 14cd36b7d6a47ddc5e88925238c330623d079cf9
Author: Golla Nagendra <nagendra.golla@amd.com>
Subject: dmaengine: zynqmp_dma: fix race between runtime PM and device removal

This commit changes zynqmp_dma_remove() to disable runtime PM before
attempting to manually suspend the device. This prevents a potential race
condition where runtime PM transitions could happen concurrently during the
driver's unbind path.

> diff --git a/drivers/dma/xilinx/zynqmp_dma.c b/drivers/dma/xilinx/zynqmp_dma.c
> index f6a812e49ddca..ca2dee0012c87 100644
> --- a/drivers/dma/xilinx/zynqmp_dma.c
> +++ b/drivers/dma/xilinx/zynqmp_dma.c
> @@ -1170,9 +1170,9 @@ static void zynqmp_dma_remove(struct platform_device *pdev)
>  	dma_async_device_unregister(&zdev->common);
>  
>  	zynqmp_dma_chan_remove(zdev->chan);
> -	if (pm_runtime_active(zdev->dev))
> -		zynqmp_dma_runtime_suspend(zdev->dev);
>  	pm_runtime_disable(zdev->dev);
> +	if (!pm_runtime_status_suspended(zdev->dev))
> +		zynqmp_dma_runtime_suspend(zdev->dev);
>  }

[Severity: High]
This isn't a bug introduced by this patch, but does zynqmp_dma_remove() need
to halt the DMA hardware before disabling clocks?

The function disables device clocks via zynqmp_dma_runtime_suspend() but never
writes to the hardware to halt active transfers or waits for idle.

Could stopping clocks while an AXI master is mid-transaction violate the bus
protocol and cause an interconnect hang?


[Severity: High]
This isn't a bug introduced by this patch, but does the probe error path fail
to drop the PM runtime usage counter?

If zynqmp_dma_chan_probe() or subsequent initialization steps fail, the
err_disable_pm path is taken:

drivers/dma/xilinx/zynqmp_dma.c:zynqmp_dma_probe() {
...
err_disable_pm:
	if (!pm_runtime_enabled(zdev->dev))
		zynqmp_dma_runtime_suspend(zdev->dev);
	pm_runtime_disable(zdev->dev);
	return ret;
}

When CONFIG_PM is enabled, pm_runtime_enabled() evaluates to true, so it
skips calling zynqmp_dma_runtime_suspend().

Doesn't this also fail to call pm_runtime_put_noidle() to balance the
earlier pm_runtime_resume_and_get(), permanently leaking the usage
counter and leaving hardware clocks running?


[Severity: High]
This isn't a bug introduced by this patch, but do the error paths in
zynqmp_dma_alloc_chan_resources() leak memory and PM usage counters?

If memory allocation fails:

drivers/dma/xilinx/zynqmp_dma.c:zynqmp_dma_alloc_chan_resources() {
...
	chan->sw_desc_pool = kzalloc_objs(*desc, ZYNQMP_DMA_NUM_DESCS);
	if (!chan->sw_desc_pool)
		return -ENOMEM;
...
}

This returns -ENOMEM directly without calling pm_runtime_put() to balance
the earlier pm_runtime_resume_and_get(), and without freeing previously
allocated structures.

Since the dmaengine core doesn't automatically invoke free_chan_resources()
when alloc_chan_resources() fails, does this permanently leak the usage
counter and descriptor memory?


[Severity: High]
This isn't a bug introduced by this patch, but can a 0-length transfer cause
memory corruption in zynqmp_dma_prep_memcpy()?

If a client driver requests a DMA memcpy with len == 0:

drivers/dma/xilinx/zynqmp_dma.c:zynqmp_dma_prep_memcpy() {
...
	desc_cnt = DIV_ROUND_UP(len, ZYNQMP_DMA_MAX_TRANS_LEN);
...
	chan->desc_free_cnt = chan->desc_free_cnt - desc_cnt;
...
	do {
		new = zynqmp_dma_get_descriptor(chan);
...
	} while (len);
...
}

DIV_ROUND_UP(0, ...) is 0, so desc_cnt is 0. The availability check passes
and desc_free_cnt is decremented by 0.

However, the subsequent do-while loop executes once, consuming one descriptor
from free_list.

When the descriptor is later freed, desc_free_cnt is incremented by 1,
inflating the available count.

Could subsequent transfers then bypass the free count check and extract a
descriptor from an empty list?


[Severity: High]
This isn't a bug introduced by this patch, but is a memory barrier missing
before clearing the STOP bit when appending descriptors?

In zynqmp_dma_tx_submit():

drivers/dma/xilinx/zynqmp_dma.c:zynqmp_dma_tx_submit() {
...
		desc->src_v->nxtdscraddr = new->src_p;
		desc->src_v->ctrl &= ~ZYNQMP_DMA_DESC_CTRL_STOP;
		desc->dst_v->nxtdscraddr = new->dst_p;
		desc->dst_v->ctrl &= ~ZYNQMP_DMA_DESC_CTRL_STOP;
...
}

The CPU updates the previous descriptor's nxtdscraddr and then clears its
ZYNQMP_DMA_DESC_CTRL_STOP bit.

Without a dma_wmb() between these stores to coherent memory, could the CPU
or memory controller reorder them?

If the DMA controller fetches the cleared ctrl word before the nxtdscraddr
update is globally visible, might it jump to a stale or invalid address?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260630064844.705173-1-nagendra.golla@amd.com?part=1