Re: [PATCH v5 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] Double-execution of DMA descriptors due to state machine desync when interrupts coalesce.
- [Critical] Channel deadlock due to dynamic descriptor linking bypassing `issue_pending()`.
--

commit a67c20eb8f20e15fe2cadb092322588069fe328e
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
> --- 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;
> +		}

[Severity: Critical]
Does checking the channel-global DONE flag here cause chained descriptors
to execute twice if interrupts coalesce?

For example, if descriptors A and B are chained and the hardware finishes
both before the interrupt handler runs, won't the handler complete A,
see DONE is set, and mark the channel DMA_COMPLETE?

If that happens, wouldn't fsl_edma_xfer_desc() then fetch B from the
issued list and restart the hardware, executing B a second time?

[ ... ]

> @@ -673,6 +676,68 @@ struct dma_async_tx_descriptor *fsl_edma_prep_dma_cyclic(
> +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);
> +	cookie = dma_cookie_assign(tx);
> +	list_move_tail(&vd->node, &vc->desc_submitted);
> +
> +	return cookie;
> +}

[Severity: Critical]
Can this cause a channel deadlock by executing unissued descriptors?

Since hardware linking happens in fsl_edma_tx_submit() rather than waiting
for the issue_pending() callback, if the hardware completes the newly linked
descriptor before the client calls issue_pending(), won't the interrupt
handler fire but find edesc is NULL?

In fsl_edma_tx_chan_handler():

    if (!fsl_chan->edesc) {
        /* terminate_all called before */
        spin_unlock(&fsl_chan->vchan.lock);
        return;
    }

Wouldn't this drop the completion and leave the channel stuck when
issue_pending() is eventually called, since the channel status is still left
as DMA_IN_PROGRESS?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260702-fsl-edma-dyn-sg-v5-0-16787185be49@bootlin.com?part=2