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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The completion handler erroneously completes and drops pending descriptors if hardware channel linking (E_LINK) is used.
--

commit 1703b2e67fb35f2931ebc1bf4444955c605a74fd
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 allow
chaining multiple DMA descriptors without stopping the channel. It adds
fsl_edma_link_sg() to link descriptors and updates the transfer completion
handler to reap issued descriptors using the EDMA_TCD_CSR_LINKCH field.

> diff --git a/drivers/dma/fsl-edma-common.c b/drivers/dma/fsl-edma-common.c
> index c5f5951c988be..76fbd2bc5154f 100644
> --- a/drivers/dma/fsl-edma-common.c
> +++ b/drivers/dma/fsl-edma-common.c
> @@ -55,10 +55,36 @@ 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);
> +		u16 csr = edma_read_tcdreg(fsl_chan, csr);
> +		u8 link_sg_id = FIELD_GET(EDMA_TCD_CSR_LINKCH, csr);

[Severity: High]
When hardware channel linking is used (the E_LINK bit is set in the CSR),
doesn't the EDMA_TCD_CSR_LINKCH field represent the MAJORLINKCH target
channel rather than a software scatter/gather ID?

If fsl_edma_link_sg() explicitly skips assigning dynamic SG IDs to
descriptors with E_LINK set, their software fsl_desc->link_sg_id will
remain 0. If this interrupt handler extracts a target channel number
(e.g., 15) into link_sg_id here, it will fail to match the software ID of 0.

> +		struct virt_dma_desc *vdesc, *tmp;
> +
> +		/* Channel is DONE when a TCD with D_REQ set completes */
> +		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;
> +		}
> +
> +		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 the transfer is still running,
> +			 * don't mark as complete the current descriptor
> +			 */
> +			if (id_match && fsl_chan->status != DMA_COMPLETE)
> +				break;
> +
> +			list_del(&vdesc->node);
> +			vchan_cookie_complete(vdesc);
> +
> +			if (id_match)
> +				break;
> +		}

[Severity: High]
Following from the E_LINK mismatch above, if id_match evaluates to false
for a descriptor using hardware channel linking, will this loop erroneously
continue deleting and completing all subsequent pending descriptors in the
desc_issued list? 

Since their software IDs will also fail to match the extracted hardware
channel number, it looks like these pending descriptors will be silently
dropped and completed without ever being submitted to the hardware.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260911-fsl-edma-dyn-sg-v10-0-700eb72def57@bootlin.com?part=2