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

[Frank Li <Frank.li@oss.nxp.com>]

On Fri, Jul 10, 2026 at 12:22:06PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Critical] Dynamically linked descriptors are incorrectly re-executed if the hardware completes the entire scatter/gather chain before the interrupt handler runs.
> - [Critical] Dynamic scatter/gather chaining loses the `link_sg_id` on multi-TCD descriptors, causing the interrupt handler to incorrectly reap actively executing descriptors.
> --

Overall is good, please check above sashiko report issues

Frank

>
> --- Patch [2]: [PATCH v6 2/2] dmaengine: fsl-edma: Support dynamic scatter/gather chaining ---
> commit 7b2e56fb9781985df561a9463fc3de1f384eab25
> 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.
> This avoids waiting for the channel to go idle if there is another
> transaction already issued.
>
> > diff --git a/drivers/dma/fsl-edma-common.c b/drivers/dma/fsl-edma-common.c
> > index c10190164926a..6cca5dca8d60a 100644
> > --- a/drivers/dma/fsl-edma-common.c
> > +++ b/drivers/dma/fsl-edma-common.c
> > @@ -58,11 +61,29 @@ 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;
> > +		}
> >  	} else {
> >  		vchan_cyclic_callback(&fsl_chan->edesc->vdesc);
> >  	}
> >
> > +	if (!fsl_chan->edesc && fsl_chan->status != DMA_COMPLETE) {
>
> [Severity: Critical]
> If the hardware completes the entire scatter/gather chain before the interrupt
> handler runs, fsl_chan->status will be set to DMA_COMPLETE above.
>
> Does this condition then cause the handler to entirely skip reaping the
> completed intermediate descriptors?
>
> > +		u8 link_sg_id = FIELD_GET(EDMA_TCD_CSR_LINKCH, edma_read_tcdreg(fsl_chan, csr));
> > +		struct virt_dma_desc *vdesc, *tmp;
> > +
> > +		list_for_each_entry_safe(vdesc, tmp, &fsl_chan->vchan.desc_issued, node) {
> > +			struct fsl_edma_desc *fsl_desc = to_fsl_edma_desc(vdesc);
> > +
> > +			if (link_sg_id == fsl_desc->link_sg_id)
> > +				break;
> > +
> > +			list_del(&vdesc->node);
> > +			vchan_cookie_complete(vdesc);
> > +		}
> > +	}
> > +
> >  	if (!fsl_chan->edesc)
> >  		fsl_edma_xfer_desc(fsl_chan);
>
> [Severity: Critical]
> If the intermediate descriptors are not reaped because fsl_chan->status is
> DMA_COMPLETE, will fsl_edma_xfer_desc() pop those already-finished descriptors
> from the desc_issued list and re-submit them, causing duplicate I/O execution?
>
> [ ... ]
>
> > @@ -924,14 +945,74 @@ void fsl_edma_xfer_desc(struct fsl_edma_chan *fsl_chan)
> [ ... ]
> > +static void fsl_edma_link_sg(struct fsl_edma_chan *fsl_chan, struct fsl_edma_desc *fsl_desc)
> > +{
> [ ... ]
> > +	first_csr |= FIELD_PREP(EDMA_TCD_CSR_LINKCH, fsl_chan->link_sg_id++);
> > +	fsl_edma_set_tcd_to_le(fsl_chan, first_tcd, first_csr, csr);
> > +	fsl_desc->link_sg_id = FIELD_GET(EDMA_TCD_CSR_LINKCH, first_csr);
>
> [Severity: Critical]
> This embedded link_sg_id appears to only be set on the first TCD of the
> descriptor.
>
> If fsl_desc has multiple TCDs, when the hardware progresses to the second TCD,
> will it load a link_sg_id of 0?
>
> If an interrupt fires while the second TCD is active, would
> fsl_edma_tx_chan_handler() read the hardware CSR as 0 and mistakenly reap the
> actively executing descriptor since it won't match fsl_desc->link_sg_id?
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260710-fsl-edma-dyn-sg-v6-0-831b96be3f31@bootlin.com?part=2