Re: [PATCH v4 2/2] dmaengine: fsl-edma: Support dynamic scatter/gather chaining

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] Dynamically linking descriptors in the .tx_submit() callback causes premature execution, lost interrupts, and permanent channel stalls.
- [Critical] Relying on the asynchronous hardware DONE flag in the completion handler creates a race condition leading to double-execution of DMA transfers.
- [High] Clearing the hardware DONE bit during active descriptor linking causes a Time-of-Check to Time-of-Use (TOCTOU) race that permanently deadlocks the DMA channel.
- [High] Modifying scatter/gather fields of an active multi-TCD descriptor in coherent memory without a memory barrier causes fatal DMA bus errors.
--

commit c846f40d8bb27c4bedc449b94402c8b84f93b59c
Author: Benoît Monin <benoit.monin@bootlin.com>

dmaengine: fsl-edma: Support dynamic scatter/gather chaining

Implement dynamic linking of scatter/gather transfers to enable chaining
multiple DMA descriptors without stopping the channel.

> diff --git a/drivers/dma/fsl-edma-common.c b/drivers/dma/fsl-edma-common.c
> index c10190164926a..6e5820051f292 100644
> --- a/drivers/dma/fsl-edma-common.c
> +++ b/drivers/dma/fsl-edma-common.c
> @@ -58,7 +58,10 @@ void fsl_edma_tx_chan_handler(struct fsl_edma_chan *fsl_chan)
>  		list_del(&fsl_chan->edesc->vdesc.node);
>  		vchan_cookie_complete(&fsl_chan->edesc->vdesc);
>  		fsl_chan->edesc = NULL;
> -		fsl_chan->status = DMA_COMPLETE;
> +		if (!(fsl_edma_drvflags(fsl_chan) & FSL_EDMA_DRV_SPLIT_REG) ||
> +		    (edma_readl_chreg(fsl_chan, ch_csr) & EDMA_V3_CH_CSR_DONE)) {
> +			fsl_chan->status = DMA_COMPLETE;
> +		}

Could this DONE flag check lead to a race condition where a chained DMA 
transfer is executed twice?

If descriptor A is chained to descriptor B, and both A and B complete in 
hardware before the CPU services A's completion interrupt, the hardware 
will halt and set the DONE flag.

The interrupt handler for A would then see the DONE flag set, assume the 
channel cleanly stopped after A, and set the status to DMA_COMPLETE. Later 
in the same handler, fsl_edma_xfer_desc() will fetch descriptor B from the 
virtual channel and, seeing the DMA_COMPLETE status, unconditionally write 
B's TCD into the hardware and restart the channel, causing B to execute 
a second time.

[ ... ]

> @@ -673,6 +676,68 @@ struct dma_async_tx_descriptor *fsl_edma_prep_dma_cyclic(
>  	return vchan_tx_prep(&fsl_chan->vchan, &fsl_desc->vdesc, flags);
>  }
>  
> +static void fsl_edma_link_sg(struct fsl_edma_chan *fsl_chan, struct fsl_edma_desc *fsl_desc)
> +{
> +	u32 flags = fsl_edma_drvflags(fsl_chan);
> +	struct fsl_edma_hw_tcd *last_tcd;
> +	struct fsl_edma_desc *prev_desc;
> +	struct virt_dma_desc *vdesc;
> +	u16 csr;
> +
> +	lockdep_assert_held(&fsl_chan->vchan.lock);
> +
> +	if (!(flags & FSL_EDMA_DRV_SPLIT_REG))
> +		return;
> +
> +	vdesc = list_last_entry_or_null(&fsl_chan->vchan.desc_submitted,
> +					struct virt_dma_desc, node);
> +	if (!vdesc)
> +		vdesc = list_last_entry_or_null(&fsl_chan->vchan.desc_issued,
> +						struct virt_dma_desc, node);
> +	if (!vdesc)
> +		return;
> +
> +	prev_desc = to_fsl_edma_desc(vdesc);
> +	last_tcd = prev_desc->tcd[prev_desc->n_tcds - 1].vtcd;
> +
> +	csr = fsl_edma_get_tcd_to_cpu(fsl_chan, last_tcd, csr);
> +	if (!(csr & EDMA_TCD_CSR_D_REQ))
> +		return;
> +
> +	fsl_edma_set_tcd_to_le(fsl_chan, last_tcd, fsl_desc->tcd[0].ptcd, dlast_sga);
> +
> +	csr &= ~EDMA_TCD_CSR_D_REQ;
> +	csr |= EDMA_TCD_CSR_E_SG;
> +	fsl_edma_set_tcd_to_le(fsl_chan, last_tcd, csr, csr);

Is a memory barrier needed between updating dlast_sga and updating the csr 
to set EDMA_TCD_CSR_E_SG?

Since this descriptor is in DMA coherent memory and actively visible to the 
hardware, the CPU or memory subsystem might reorder these writes. If the eDMA 
engine fetches the TCD while E_SG is set but dlast_sga is still stale or 
uninitialized, it might fetch the next descriptor from an invalid physical 
address and trigger a fatal bus error.

> +
> +	if (prev_desc == fsl_chan->edesc && prev_desc->n_tcds == 1) {
> +		if (flags & FSL_EDMA_DRV_CLEAR_DONE_E_SG)
> +			edma_writel_chreg(fsl_chan, edma_readl_chreg(fsl_chan, ch_csr), ch_csr);

Does this read and write-back of ch_csr introduce a race condition that can 
deadlock the channel?

If the hardware transfer completes just before the channel lock is acquired, 
the channel becomes idle and the DONE bit is set. The driver then implicitly 
clears the write-1-to-clear DONE bit and sets E_SG, but setting E_SG on an 
already halted channel will not restart it.

When the delayed completion interrupt runs, it will read ch_csr, see that 
DONE is 0, incorrectly assume the channel seamlessly continued, and leave 
the status as DMA_IN_PROGRESS. This would prevent fsl_edma_xfer_desc() from
enabling the hardware request, leaving the channel stopped indefinitely.

> +
> +		edma_cp_tcd_to_reg(fsl_chan, last_tcd, dlast_sga);
> +		edma_cp_tcd_to_reg(fsl_chan, last_tcd, csr);
> +	}
> +
> +	trace_edma_link_sg(fsl_chan, last_tcd);
> +}
> +
> +static dma_cookie_t fsl_edma_tx_submit(struct dma_async_tx_descriptor *tx)
> +{
> +	struct virt_dma_desc *vd = container_of(tx, struct virt_dma_desc, tx);
> +	struct fsl_edma_chan *fsl_chan = to_fsl_edma_chan(tx->chan);
> +	struct fsl_edma_desc *fsl_desc = to_fsl_edma_desc(vd);
> +	struct virt_dma_chan *vc = to_virt_chan(tx->chan);
> +	dma_cookie_t cookie;
> +
> +	guard(spinlock_irqsave)(&fsl_chan->vchan.lock);
> +
> +	fsl_edma_link_sg(fsl_chan, fsl_desc);
> +	cookie = dma_cookie_assign(tx);
> +	list_move_tail(&vd->node, &vc->desc_submitted);
> +
> +	return cookie;
> +}

Can calling fsl_edma_link_sg() directly from the tx_submit callback lead to 
premature execution and lost interrupts?

By modifying the active hardware TCD to link the new descriptor before the 
client calls dma_async_issue_pending(), the hardware can transition to and 
start executing the newly submitted descriptor automatically.

If the completion interrupt for the previous descriptor fires before 
issue_pending() is called, vchan_next_desc() won't find the new descriptor 
because it is still sitting in the desc_submitted list rather than the 
desc_issued list.

This would leave fsl_chan->edesc as NULL, meaning when the new descriptor 
finishes, its completion interrupt is dropped. When issue_pending() is 
finally called, the channel may either permanently stall or mistakenly 
restart the descriptor and execute it a second time.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260518-fsl-edma-dyn-sg-v4-0-8ce7d95b1ce9@bootlin.com?part=2