Re: [PATCH 2/2] dmaengine: zynqmp_dma: Add per-channel reset support

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The probe error path leaks a runtime PM usage counter reference, permanently breaking runtime PM and leaving hardware clocks enabled.
- [High] Dropping the spinlock inside `list_for_each_entry_safe` allows concurrent modification of the list, leading to memory corruption.
- [High] Hardware registers are accessed in the IRQ handler without verifying if the device is active, risking a kernel panic on spurious interrupts.
--

commit b2d2a678f48614c7188f8d81cbc70143b2bdfc0f
Author: Golla Nagendra <nagendra.golla@amd.com>
Subject: dmaengine: zynqmp_dma: Add per-channel reset support

This commit introduces per-channel reset support for Versal Gen 2 and
Versal Net SoCs. It adds reset handling in the channel probe path using
device_reset_optional() to trigger a reset pulse during initialization.
Platforms without per-channel reset are unaffected.

> diff --git a/drivers/dma/xilinx/zynqmp_dma.c b/drivers/dma/xilinx/zynqmp_dma.c
> index f6a812e49ddca..51c831760372c 100644
> --- a/drivers/dma/xilinx/zynqmp_dma.c
> +++ b/drivers/dma/xilinx/zynqmp_dma.c
[ ... ]
> @@ -916,6 +917,11 @@ static int zynqmp_dma_chan_probe(struct zynqmp_dma_device *zdev,
>  	if (IS_ERR(chan->regs))
>  		return PTR_ERR(chan->regs);
>  
> +	err = device_reset_optional(&pdev->dev);
> +	if (err)
> +		return dev_err_probe(&pdev->dev, err,
> +				     "failed to reset channel\n");
> +

[Severity: High]
This isn't a bug introduced by this patch, but does this error path leak a
runtime PM usage counter reference?

In zynqmp_dma_probe(), pm_runtime_resume_and_get() is called to increment
the runtime PM usage counter and enable the device clocks. If a subsequent
initialization step fails, such as the newly added device_reset_optional()
returning -EPROBE_DEFER, the code jumps to the err_disable_pm label:

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;

Since the device is still runtime PM enabled, it skips
zynqmp_dma_runtime_suspend() and fails to drop the usage counter with
pm_runtime_put_noidle().

Because -EPROBE_DEFER is a normal occurrence during boot, this path can
be hit multiple times. Will this permanently leave the usage counter above
zero, preventing the device from ever entering runtime suspend and leaving
its clocks enabled indefinitely?

>  	chan->bus_width = ZYNQMP_DMA_BUS_WIDTH_64;
>  	chan->dst_burst_len = ZYNQMP_DMA_MAX_DST_BURST_LEN;

[Severity: High]
This is a pre-existing issue, but does dropping the spinlock inside
list_for_each_entry_safe() in zynqmp_dma_chan_desc_cleanup() allow concurrent
modification of the list, leading to memory corruption?

When zynqmp_dma_chan_desc_cleanup() temporarily drops the lock to invoke
the DMA callback:

zynqmp_dma_chan_desc_cleanup()
    ...
	list_for_each_entry_safe(desc, next, &chan->done_list, node) {
        ...
		if (dmaengine_desc_callback_valid(&cb)) {
			spin_unlock_irqrestore(&chan->lock, irqflags);
			dmaengine_desc_callback_invoke(&cb, NULL);
			spin_lock_irqsave(&chan->lock, irqflags);
		}

A client driver can concurrently invoke dmaengine_terminate_sync(), which
calls zynqmp_dma_device_terminate_all() and executes
zynqmp_dma_free_descriptors(). This moves all remaining elements, including
the prefetched next pointer, from done_list to free_list.

When zynqmp_dma_chan_desc_cleanup() reacquires the lock, won't it continue
iterating using the corrupted next pointer that now resides in free_list,
eventually interpreting the list head &chan->free_list as a struct
zynqmp_dma_desc_sw and resulting in out-of-bounds memory accesses?

[Severity: High]
This is a pre-existing issue, but are hardware registers accessed in the
IRQ handler without verifying if the device is active, risking a kernel
panic on spurious interrupts?

The IRQ handler zynqmp_dma_irq_handler() directly reads the ISR and IMR
hardware registers:

zynqmp_dma_irq_handler()
    ...
	isr = readl(chan->regs + ZYNQMP_DMA_ISR);
	imr = readl(chan->regs + ZYNQMP_DMA_IMR);

According to the Power Management Subsystem guidelines, IRQ handlers must
use pm_runtime_get_if_active() before accessing hardware registers.

Although this IRQ is not shared, spurious interrupts can still occur. If a
spurious interrupt fires while the device is in a suspended state with its
clocks disabled via zynqmp_dma_runtime_suspend(), could this unprotected
register read lead to a synchronous external abort and crash the kernel?

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