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

["Golla, Nagendra" <Nagendra.Golla@amd.com>]

On 5/25/2026 5:03 PM, sashiko-bot@kernel.org wrote:
> 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?

Hi,

Thanks for pointing this out.After reviewing the runtime PM guidelines 
and the driver behavior, I believe this is not a strict requirement for 
all IRQ handlers. The PM documentation does not mandate that 
pm_runtime_get_if_active() must be used before every register access in 
an ISR. It is mainly intended for cases where an interrupt can occur 
while the device might be runtime-suspended, such as shared IRQ or 
spurious interrupt scenarios.

In the case of the zynqmp DMA driver, the interrupt is generated only 
when the hardware is active during a transfer, and the driver ensures 
the device remains runtime-active while DMA operations are in progress. 
Because of this, the ISR is not expected to run when the device is 
suspended, and accessing registers directly should be safe.

So, in this context, adding pm_runtime_get_if_active() in the IRQ 
handler may not be necessary. That said, please let me know  if you see 
a scenario where the interrupt could be triggered while the device is 
runtime-suspended, and I can revisit accordingly.

Thanks,
Nagendra


> 
> 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?
>