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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] Hardware executes dynamically linked descriptors before issue_pending() is called, causing missed interrupts and double execution.
- [Critical] TOCTOU race condition with the channel DONE bit in the interrupt handler causes double execution of chained descriptors.
- [High] Incorrect list check order in fsl_edma_link_sg() drops intermediate descriptors from the hardware scatter-gather chain.
- [High] Clearing the channel DONE bit during dynamic linking causes a permanent DMA channel stall.
- [High] Missing memory barrier when updating in-memory scatter-gather links risks torn hardware reads and system bus errors.
--

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

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

This commit implements dynamic linking of scatter/gather transfers to enable
chaining multiple DMA descriptors without stopping the channel. It adds
fsl_edma_link_sg() which is called during tx_submit() to link the new
descriptor to the previously submitted one.

> diff --git a/drivers/dma/fsl-edma-common.c b/drivers/dma/fsl-edma-common.c
> index c10190164926a..b83d1b91dca2b 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;
> +		}

If the hardware dynamically chains from descriptor A to descriptor B, it
still raises an interrupt for A. If the CPU is delayed and the hardware
finishes executing B and halts, setting the DONE bit, won't the ISR for A
see the DONE bit set here, set the status to DMA_COMPLETE, and then later
explicitly re-enable the channel to execute B again, causing memory
corruption?

>  	} else {
>  		vchan_cyclic_callback(&fsl_chan->edesc->vdesc);
>  	}

[ ... ]

> @@ -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_issued,
> +					struct virt_dma_desc, node);
> +	if (!vdesc)
> +		vdesc = list_last_entry_or_null(&fsl_chan->vchan.desc_submitted,
> +						struct virt_dma_desc, node);

If desc_issued is not empty but there are also descriptors in
desc_submitted, this will link against the tail of desc_issued instead of
the newest descriptor at the tail of desc_submitted. Will this cause the
hardware to skip intermediate submissions and corrupt the transfer sequence?

> +	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 write memory barrier like dma_wmb() needed between updating dlast_sga
and setting the E_SG bit in the csr? If the CPU or compiler reorders these
writes, could the actively running eDMA controller observe the E_SG bit set
with a stale dlast_sga pointer and trigger a 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);

If the hardware has just finished executing the transfer and halted before
this register access, the DONE bit is already set. Writing it back clears
it. Will the subsequent completion interrupt ISR then wrongly assume the
channel chained successfully, leaving the state as DMA_IN_PROGRESS and
causing a permanent channel stall?

> +
> +		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);

By dynamically chaining the new descriptor to the currently running hardware
here, the hardware could start executing it before dma_async_issue_pending()
is called.

If the hardware completes the new descriptor while it is still in the
desc_submitted list, could the interrupt handler ignore its completion
because it only processes desc_issued?

If so, would this result in double execution later when issue_pending() is
finally called and the software explicitly restarts the already-completed
descriptor?

> +	cookie = dma_cookie_assign(tx);
> +	list_move_tail(&vd->node, &vc->desc_submitted);
> +
> +	return cookie;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260511-fsl-edma-dyn-sg-v3-0-98a181775dae@bootlin.com?part=2