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

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.

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 <[email protected]>

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?

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