Re: [PATCH V3 5/6] dmaengine: zynqmp_dma: Add new compatible string for Versal Net

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

Hi,
On 8/10/2026 3:50 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
> 
> New issues:
> - [High] The `has_reset` field is introduced but completely unused, missing the required hardware reset logic.

Thanks for the review

The has_reset field is intentionally introduced in this patch without 
usage. It is consumed in the next patch (6/6) where 
zynqmp_dma_chan_probe() checks match_data->has_reset and conditionally 
calls device_reset(). The series is split this way to keep the data 
structure change separate from the reset logic.

Thanks,
Nagendra
> 
> Pre-existing issues:
> - [High] `zynqmp_dma_chan_probe()` hardcodes the `platform_get_irq()` error return to `-ENXIO`, breaking probe deferral.
> - [High] `zynqmp_dma_reset()` silently drops active and pending DMA descriptors without invoking their callbacks, causing deadlocks for client drivers.
> - [High] `zynqmp_dma_device_terminate_all()` does not command the hardware to stop, leading to potential memory corruption.
> --
> 
> --- Patch [5]: [PATCH V3 5/6] dmaengine: zynqmp_dma: Add new compatible string for Versal Net ---
> commit 93c6838fba5eee0dddf024579d8474fed6b7dee5
> Author: Golla Nagendra <nagendra.golla@amd.com>
> 
> dmaengine: zynqmp_dma: Add new compatible string for Versal Net
> 
> This patch introduces the amd,versal-net-dma-1.0 compatible string and its
> SoC-specific match data. It also adds a has_reset field to zynqmp_dma_config
> to indicate whether the compatible requires a per-channel reset during probe.
> 
>> diff --git a/drivers/dma/xilinx/zynqmp_dma.c b/drivers/dma/xilinx/zynqmp_dma.c
>> index b03171e37e278..7561416b8e626 100644
>> --- a/drivers/dma/xilinx/zynqmp_dma.c
>> +++ b/drivers/dma/xilinx/zynqmp_dma.c
>> @@ -259,13 +259,20 @@ struct zynqmp_dma_device {
>>   };
>>   
>>   struct zynqmp_dma_config {
>> +	bool has_reset;
>>   	u32 offset;
>>   };
>>   
>>   static const struct zynqmp_dma_config versal2_dma_config = {
>> +	.has_reset = true,
>>   	.offset = IRQ_REG_OFFSET,
>>   };
>>   
>> +/* offset = 0: Versal Net uses base IRQ register address */
>> +static const struct zynqmp_dma_config versal_net_dma_config = {
>> +	.has_reset = true,
>> +};
>> +
> 
> [Severity: High]
> Does this configuration actually perform a hardware reset?
> 
> The patch introduces the has_reset flag and sets it to true for versal2 and
> versal-net, but looking at zynqmp_dma_chan_probe(), the match data is
> retrieved without ever checking this flag or invoking any reset functions:
> 
> drivers/dma/xilinx/zynqmp_dma.c:zynqmp_dma_chan_probe() {
>      ...
> 	match_data = of_device_get_match_data(&pdev->dev);
> 	if (match_data)
> 		chan->irq_offset = match_data->offset;
>      ...
> }
> 
> Does the omission of the reset implementation leave the DMA hardware in an
> uninitialized state during probe, potentially causing back-to-back usage
> failures?
> 
> While reviewing this patch, I also noticed a few pre-existing issues in the
> same file:
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but zynqmp_dma_chan_probe()
> hardcodes the platform_get_irq() error return to -ENXIO, which discards
> probe deferral errors like -EPROBE_DEFER:
> 
> drivers/dma/xilinx/zynqmp_dma.c:zynqmp_dma_chan_probe() {
>      ...
> 	chan->irq = platform_get_irq(pdev, 0);
> 	if (chan->irq < 0)
> 		return -ENXIO;
>      ...
> }
> 
> Does this permanent probe failure prevent the DMA controller from working when
> the interrupt controller is not yet ready during boot?
> 
> [Severity: High]
> This is a pre-existing issue, but zynqmp_dma_reset() silently drops active and
> pending DMA descriptors without invoking their callbacks:
> 
> drivers/dma/xilinx/zynqmp_dma.c:zynqmp_dma_reset() {
>      ...
> 	spin_lock_irqsave(&chan->lock, irqflags);
> 	zynqmp_dma_complete_descriptor(chan);
> 	spin_unlock_irqrestore(&chan->lock, irqflags);
> 	zynqmp_dma_chan_desc_cleanup(chan);
> 	zynqmp_dma_free_descriptors(chan);
>      ...
> }
> 
> Because zynqmp_dma_complete_descriptor() only completes the first descriptor in
> the active_list, the remaining active and pending descriptors are silently
> recycled by zynqmp_dma_free_descriptors(). Could this strand waiting client
> drivers and cause deadlocks when a hardware error occurs?
> 
> [Severity: High]
> This is a pre-existing issue, but zynqmp_dma_device_terminate_all() does not
> command the hardware to stop:
> 
> drivers/dma/xilinx/zynqmp_dma.c:zynqmp_dma_device_terminate_all() {
>      ...
> 	writel(ZYNQMP_DMA_IDS_DEFAULT_MASK, chan->regs + ZYNQMP_DMA_IDS);
> 	zynqmp_dma_free_descriptors(chan);
> 
> 	return 0;
> }
> 
> The function merely masks interrupts and frees software descriptors, but never
> clears the ZYNQMP_DMA_ENABLE bit or issues a hardware stop command. If a client
> driver calls dmaengine_terminate_all() and frees the DMA buffers, could the DMA
> controller continue to read/write the in-flight physical memory in the
> background and corrupt memory?
>