Re: [PATCH v2 21/64] dmaengine: zynqmp-dma: switch completion tasklet to dma_chan BH

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] On channel error, the driver incorrectly marks the currently active descriptor as successful and silently drops all other pending descriptors, causing clients to hang indefinitely.
- [High] The DMA completion handler `zynqmp_dma_chan_desc_cleanup` drops `chan->lock` during callback execution while using `list_for_each_entry_safe`, allowing list corruption if a callback calls `dmaengine_terminate_async`.
- [High] `zynqmp_dma_device_terminate_all` fails to actually halt the hardware DMA engine, leading to hardware silently overwriting memory after the client assumes the transfer is stopped.
--

--- Patch [21]: [PATCH v2 21/64] dmaengine: zynqmp-dma: switch completion tasklet to dma_chan BH ---
commit c7b5b53ae6dbca9c51452b38046b4252492aa34c
Author: Allen Pais <allen.lkml@gmail.com>
Subject: dmaengine: zynqmp-dma: switch completion tasklet to dma_chan BH

Replace the per-channel tasklet with the shared dma_chan BH helper.
The handler continues to run in softirq context while dmaengine owns
the common scheduling and teardown mechanism.

> diff --git a/drivers/dma/xilinx/zynqmp_dma.c b/drivers/dma/xilinx/zynqmp_dma.c
> index f6a812e49ddca..6291126cdc9f8 100644
> --- a/drivers/dma/xilinx/zynqmp_dma.c
> +++ b/drivers/dma/xilinx/zynqmp_dma.c
[ ... ]
> @@ -760,11 +758,12 @@ static irqreturn_t zynqmp_dma_irq_handler(int irq, void *data)
>  
>  /**
>   * zynqmp_dma_do_tasklet - Schedule completion tasklet
> - * @t: Pointer to the ZynqMP DMA channel structure
> + * @c: Pointer to the ZynqMP DMA channel structure
>   */
> -static void zynqmp_dma_do_tasklet(struct tasklet_struct *t)
> +static void zynqmp_dma_do_tasklet(struct dma_chan *c)
>  {
> -	struct zynqmp_dma_chan *chan = from_tasklet(chan, t, tasklet);
> +	struct zynqmp_dma_chan *chan = container_of(c,
> +			struct zynqmp_dma_chan, common);
>  	u32 count;
>  	unsigned long irqflags;
>  

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a potential problem in
how channel errors are handled when this bottom half runs.

When chan->err is set, we call zynqmp_dma_reset(). Inside zynqmp_dma_reset():

drivers/dma/xilinx/zynqmp_dma.c:zynqmp_dma_reset() {
	...
	zynqmp_dma_complete_descriptor(chan);
	...
	zynqmp_dma_free_descriptors(chan);
	...
}

Does this incorrectly move the active descriptor to done_list and mark it
successful?

Also, it appears zynqmp_dma_free_descriptors() silently moves all remaining
descriptors from active_list and pending_list to the free_list without
invoking their callbacks.

Could this cause clients waiting on these DMA transfers to hang indefinitely?


[Severity: High]
This is a pre-existing issue, but could there be a list corruption risk in
zynqmp_dma_chan_desc_cleanup() which is called shortly after in this bottom
half?

drivers/dma/xilinx/zynqmp_dma.c: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);
		}
	...
}

The lock is dropped during callback execution. If the client calls
dmaengine_terminate_async() in the callback, all descriptors (including those
on done_list) are moved to free_list.

When the lock is reacquired, the loop continues using the next pointer which
is now on the free_list.

Could this lead to double-frees and traversing invalid entries?


[Severity: High]
This is a pre-existing issue, but does zynqmp_dma_device_terminate_all()
actually halt the hardware DMA engine?

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);
	...
}

It masks interrupts and frees software descriptors, but doesn't seem to clear
the ZYNQMP_DMA_ENABLE bit or issue a hardware reset.

If the hardware controller remains active, could it silently overwrite memory
after the client assumes the transfer is stopped and unmaps the DMA buffers?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1785183549.git.allen.lkml@gmail.com?part=21