Re: [PATCH v2 64/64] dmaengine: switchtec: use channel BH helpers

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

Pre-existing issues:
- [High] dmaengine_synchronize() incorrectly aborts pending descriptors instead of waiting, violating the DMA API contract.
- [High] Lost completion results for out-of-order descriptors leads to incorrect callback data.
- [High] Channel reset in `switchtec_dma_synchronize` fails to zero host memory, causing permanent desynchronization.
--

--- Patch [64]: [PATCH v2 64/64] dmaengine: switchtec: use channel BH helpers ---
commit ae138df18f2b5dc2645b9652b5d982dbf7b2fde2
Author: Allen Pais <[email protected]>

dmaengine: switchtec: use channel BH helpers

Replace the per-channel descriptor completion tasklet with the shared
dma_chan BH helpers. Client callbacks remain in softirq context while
using the common scheduling and teardown mechanism.

> diff --git a/drivers/dma/switchtec_dma.c b/drivers/dma/switchtec_dma.c
> index 3ef9286406159..f3459799e05d9 100644
> --- a/drivers/dma/switchtec_dma.c
> +++ b/drivers/dma/switchtec_dma.c

[ ... ]

> @@ -584,6 +582,7 @@ static void switchtec_dma_synchronize(struct dma_chan *chan)
>  
>  	int rc;
>  
> +	dma_chan_kill_bh(chan);
>  	switchtec_dma_abort_desc(swdma_chan, 1);

[Severity: High]
This is a pre-existing issue, but does switchtec_dma_synchronize() violate
the DMA API contract here by actively destroying the channel's state?

The DMA API requires dmaengine_synchronize() to merely wait for the
completion of previously terminated or submitted transfers. Instead, this
forcefully aborts all pending descriptors and skips their callbacks, which
would drop pending transfers and leave clients with missing completions.

[Severity: High]
This is also a pre-existing issue, but further down in
switchtec_dma_synchronize(), the software ring state is reset:

	spin_lock_bh(&swdma_chan->complete_lock);
	swdma_chan->comp_ring_active = true;
	swdma_chan->phase_tag = 0;
	swdma_chan->tail = 0;
	swdma_chan->cq_tail = 0;

Does this channel reset fail to zero the backing hardware memory (hw_cq)?

Since the memory retains previous phase tags, the software might incorrectly
interpret stale data at index 0 as a new hardware completion, causing it to
read garbage and permanently desynchronize from the hardware state machine.

>  
>  	rc = enable_channel(swdma_chan);

[ ... ]

> @@ -820,9 +819,10 @@ static int switchtec_dma_resume(struct dma_chan *chan)
>  	return ret;
>  }
>  
> -static void switchtec_dma_desc_task(unsigned long data)
> +static void switchtec_dma_desc_task(struct dma_chan *chan)
>  {
> -	struct switchtec_dma_chan *swdma_chan = (void *)data;
> +	struct switchtec_dma_chan *swdma_chan =
> +		container_of(chan, struct switchtec_dma_chan, dma_chan);
>  
>  	switchtec_dma_cleanup_completed(swdma_chan);

[Severity: High]
This isn't a bug introduced by this patch, but does
switchtec_dma_cleanup_completed() lose the completion results for
out-of-order descriptors?

When the hardware completes a descriptor out-of-order, res is computed but
immediately discarded by a continue:

		/*  Out of order CE */
		if (se_idx != tail) {
			spin_unlock_bh(&swdma_chan->complete_lock);
			continue;
		}

When se_idx == tail is eventually met, the catch-up loop reuses the exact
same local res variable for all queued completions:

		do {
			dma_cookie_complete(&desc->txd);
			dma_descriptor_unmap(&desc->txd);
			dmaengine_desc_get_callback_invoke(&desc->txd, &res);
			...

Could out-of-order DMA completions lose their actual result status and
residue, causing them to incorrectly inherit the status of the last
sequentially completed descriptor?

>  }
>

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