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

On Tue, Jul 28, 2026 at 02:21:10PM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] A completed dynamically linked scatter/gather descriptor will be erroneously re-issued for execution if its interrupt is delayed, leading to double-execution and memory corruption.
> - [High] Descriptors can be erroneously marked as completed without execution if channel linking (E_LINK) is configured.
> --

please double check above two issues

Frank

>
> --- Patch [2]: [PATCH v7 2/2] dmaengine: fsl-edma: Support dynamic scatter/gather chaining ---
> commit b8502bf27e9fa2ac70190840a26c1603f5640651
> Author: Benoît Monin <[email protected]>
>
> 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
> > --- a/drivers/dma/fsl-edma-common.c
> > +++ b/drivers/dma/fsl-edma-common.c
> [ ... ]
> > @@ -55,10 +55,29 @@ void fsl_edma_tx_chan_handler(struct fsl_edma_chan *fsl_chan)
> >  	}
> >
> >  	if (!fsl_chan->edesc->iscyclic) {
> > -		list_del(&fsl_chan->edesc->vdesc.node);
> > -		vchan_cookie_complete(&fsl_chan->edesc->vdesc);
> > +		u8 link_sg_id = FIELD_GET(EDMA_TCD_CSR_LINKCH, edma_read_tcdreg(fsl_chan, csr));
>
> [Severity: High]
> Will this misinterpret a hardware channel link ID as a software SG ID if
> channel linking (EDMA_TCD_CSR_E_LINK) is configured?
>
> If a transfer uses EDMA_TCD_CSR_E_LINK to link to a non-zero channel, the
> handler appears to unconditionally extract link_sg_id from the CSR. This
> channel number would be read as the link_sg_id, causing id_match to be false
> for all pending descriptors (which have link_sg_id == 0).
>
> Does this cause the handler to iterate through the entire desc_issued list,
> prematurely deleting and completing every queued descriptor without them ever
> executing?
>
> > +		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);
> > +			bool id_match = (link_sg_id == fsl_desc->link_sg_id);
> > +
> > +			if (id_match && fsl_desc != fsl_chan->edesc)
> > +				break;
>
> [Severity: High]
> If multiple dynamically linked descriptors finish before the interrupt handler
> processes them (e.g., due to system load or interrupt latency), could a
> completed descriptor be stranded in the queue?
>
> When the hardware stops on a later descriptor and sets the DONE flag, this
> loop will break early when it matches the link_sg_id of the finished
> descriptor, assuming it is still running because fsl_desc != fsl_chan->edesc.
>
> > +
> > +			list_del(&vdesc->node);
> > +			vchan_cookie_complete(vdesc);
> > +
> > +			if (id_match)
> > +				break;
> > +		}
> > +
> >  		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);
> >  	}
> [ ... ]
> > @@ -931,14 +950,87 @@ void fsl_edma_xfer_desc(struct fsl_edma_chan *fsl_chan)
> >  	if (!vdesc)
> >  		return;
> >  	fsl_chan->edesc = to_fsl_edma_desc(vdesc);
> > -	fsl_edma_set_tcd_regs(fsl_chan, fsl_chan->edesc->tcd[0].vtcd);
> > -	fsl_edma_enable_request(fsl_chan);
> > -	fsl_chan->status = DMA_IN_PROGRESS;
> > +
> > +	if (fsl_chan->status != DMA_IN_PROGRESS) {
> > +		fsl_edma_set_tcd_regs(fsl_chan, fsl_chan->edesc->tcd[0].vtcd);
> > +		fsl_edma_enable_request(fsl_chan);
> > +		fsl_chan->status = DMA_IN_PROGRESS;
> > +	}
> > +}
>
> [Severity: High]
> Because the channel status was updated to DMA_COMPLETE in
> fsl_edma_tx_chan_handler() above, will fsl_edma_xfer_desc() then fetch the
> stranded descriptor from the queue and erroneously reprogram it to the
> hardware?
>
> This seems like it would cause the hardware to re-execute a DMA transfer that
> has already finished, potentially leading to data corruption for I/O devices
> (such as duplicated writes or dropped reads).
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2