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

[sashiko-bot@kernel.org]

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.
--

--- 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