[PATCH] dmaengine: switchtec-dma: preserve results for out-of-order completions

[PATCH] dmaengine: switchtec-dma: preserve results for out-of-order completions

[Hao-Qun Huang]

Switchtec completion entries can arrive out of submission order, and the
driver handles that by marking the descriptor complete and deferring its
callback until the ring tail catches up.  The problem is that the
per-transfer result (status and residue) is only kept in a stack variable
while the completion entry is being read.  Once the tail gap closes,
cleanup retires the descriptor at the tail along with any already
completed descriptors behind it, and hands the same stack result to every
one of them.

So a transfer that failed out of order can be reported as successful (or
the other way round) depending on which completion happened to close the
gap, and the residue can end up coming from the wrong transfer too.

Keep the result in each descriptor instead of on the stack, and pass the
descriptor's own saved result when it is retired.  Cookie ordering, the
CID-to-descriptor mapping and the completion locking are unchanged; only
the result handed to each callback is corrected.

Fixes: 30eba9df76ad ("dmaengine: switchtec-dma: Implement hardware initialization and cleanup")
Assisted-by: Claude:claude-fable-5
Signed-off-by: Hao-Qun Huang <alvinhuang0603@gmail.com>
---
 drivers/dma/switchtec_dma.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/dma/switchtec_dma.c b/drivers/dma/switchtec_dma.c
index 3ef928640615..681540836e73 100644
--- a/drivers/dma/switchtec_dma.c
+++ b/drivers/dma/switchtec_dma.c
@@ -246,6 +246,7 @@ struct switchtec_dma_hw_ce {
 struct switchtec_dma_desc {
 	struct dma_async_tx_descriptor txd;
 	struct switchtec_dma_hw_se_desc *hw;
+	struct dmaengine_result result;
 	u32 orig_size;
 	bool completed;
 };
@@ -409,7 +410,6 @@ switchtec_dma_cleanup_completed(struct switchtec_dma_chan *swdma_chan)
 	struct device *chan_dev = &swdma_chan->dma_chan.dev->device;
 	struct switchtec_dma_desc *desc;
 	struct switchtec_dma_hw_ce *ce;
-	struct dmaengine_result res;
 	int tail, cid, se_idx, i;
 	__le16 phase_tag;
 	u32 sts_code;
@@ -439,17 +439,17 @@ switchtec_dma_cleanup_completed(struct switchtec_dma_chan *swdma_chan)
 
 		tail = swdma_chan->tail;
 
-		res.residue = desc->orig_size - le32_to_cpu(ce->cpl_byte_cnt);
+		desc->result.residue = desc->orig_size - le32_to_cpu(ce->cpl_byte_cnt);
 
 		sts_code = le32_to_cpu(ce->sts_code);
 
 		if (!(sts_code & SWITCHTEC_CE_SC_MASK)) {
-			res.result = DMA_TRANS_NOERROR;
+			desc->result.result = DMA_TRANS_NOERROR;
 		} else {
 			if (sts_code & SWITCHTEC_CE_SC_D_RD_CTO)
-				res.result = DMA_TRANS_READ_FAILED;
+				desc->result.result = DMA_TRANS_READ_FAILED;
 			else
-				res.result = DMA_TRANS_WRITE_FAILED;
+				desc->result.result = DMA_TRANS_WRITE_FAILED;
 
 			dev_err(chan_dev, "CID 0x%04x failed, SC 0x%08x\n", cid,
 				(u32)(sts_code & SWITCHTEC_CE_SC_MASK));
@@ -488,7 +488,7 @@ switchtec_dma_cleanup_completed(struct switchtec_dma_chan *swdma_chan)
 		do {
 			dma_cookie_complete(&desc->txd);
 			dma_descriptor_unmap(&desc->txd);
-			dmaengine_desc_get_callback_invoke(&desc->txd, &res);
+			dmaengine_desc_get_callback_invoke(&desc->txd, &desc->result);
 			desc->txd.callback = NULL;
 			desc->txd.callback_result = NULL;
 			desc->completed = false;
-- 
2.43.0

Re: [PATCH] dmaengine: switchtec-dma: preserve results for out-of-order completions

[sashiko-bot]

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

Pre-existing issues:
- [High] Race condition on the first ring traversal can cause premature completion of in-flight DMA descriptors.
- [High] Descriptor hardware control flags accumulate across reuse.
- [High] Invoking client callbacks while holding `complete_lock` causes a deadlock if the client calls `dmaengine_tx_status`.
- [Medium] Memory leak of descriptor rings on error path during channel resource allocation.
--

--- Patch [1]: [PATCH] dmaengine: switchtec-dma: preserve results for out-of-order completions ---
commit 4a9da15cea6ee54c832c41e3ac56d3a47db7138d
Author: Hao-Qun Huang <alvinhuang0603@gmail.com>

dmaengine: switchtec-dma: preserve results for out-of-order completions

This patch modifies the driver to store the transfer result within each
descriptor rather than on the stack. Passing the descriptor's own saved
result prevents out-of-order completions from assigning incorrect status
and residue values when retiring a batch of descriptors.

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

[ ... ]

> @@ -488,7 +488,7 @@ switchtec_dma_cleanup_completed(struct switchtec_dma_chan *swdma_chan)
>  		do {
>  			dma_cookie_complete(&desc->txd);
>  			dma_descriptor_unmap(&desc->txd);
> -			dmaengine_desc_get_callback_invoke(&desc->txd, &res);
> +			dmaengine_desc_get_callback_invoke(&desc->txd, &desc->result);

[Severity: High]
This is a pre-existing issue, but does invoking the client callback here
while holding complete_lock cause a deadlock if the client queries status?

Looking at switchtec_dma_tx_status(), it unconditionally calls
switchtec_dma_cleanup_completed(), which attempts to acquire
complete_lock. If a client callback invokes dmaengine_tx_status(), it
would trigger a self-deadlock since the non-recursive lock is already
held.

>  			desc->txd.callback = NULL;
>  			desc->txd.callback_result = NULL;
>  			desc->completed = false;

[Severity: High]
This is a pre-existing issue, but could this loop prematurely complete
in-flight DMA descriptors due to a race condition?

In switchtec_dma_alloc_desc(), descriptors are initialized with
desc->completed = true.

Concurrently, if the submission thread calls switchtec_dma_prep_desc()
(setting desc->completed = false) and updates swdma_chan->head, could the
consumer thread evaluate the loop-exit condition without a read memory
barrier:

switchtec_dma_cleanup_completed() {
    ...
        if (!desc->completed)
            break;
    } while (CIRC_CNT(READ_ONCE(swdma_chan->head), swdma_chan->tail,
                      SWITCHTEC_DMA_SQ_SIZE));
}

and see a stale true state for desc->completed alongside an updated head
state, erroneously completing a newly submitted descriptor?

[ ... ]

[Severity: Medium]
This is a pre-existing issue, but does this error path leak the allocated
descriptor rings?

If device_alloc_chan_resources() fails, the dmaengine core does not invoke
device_free_chan_resources(). In switchtec_dma_alloc_chan_resources():

switchtec_dma_alloc_chan_resources() {
    ...
    rc = switchtec_dma_alloc_desc(swdma_chan);
    if (rc)
        return rc;

    rc = enable_channel(swdma_chan);
    if (rc)
        return rc;
    ...
}

The function returns directly without freeing the memory allocated by
switchtec_dma_alloc_desc().


[Severity: High]
This is a pre-existing issue, but do the hardware control flags accumulate
across descriptor reuse?

In switchtec_dma_prep_desc(), flags are conditionally added:

switchtec_dma_prep_desc() {
    ...
    if (flags & DMA_PREP_INTERRUPT)
        desc->hw->ctrl |= SWITCHTEC_SE_LIOF;

    if (flags & DMA_PREP_FENCE)
        desc->hw->ctrl |= SWITCHTEC_SE_BRR;
    ...
}

Because the descriptor slots are recycled and the flags are never cleared,
a new transfer prepared without DMA_PREP_INTERRUPT could inappropriately
trigger an interrupt if the recycled descriptor slot previously had it set.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260718223257.5662.alvinhuang0603@gmail.com?part=1