Re: [PATCH 4/4] dmaengine: loongson: loongson2-apb-cmc-dma: Add pause/resume support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] dmaengine: loongson: loongson2_cmc_dma_start_transfer() clears pause state
- [Medium] dmaengine: loongson: loongson2_cmc_dma_tx_status() ignores pause if !state
--

commit 8e161a43d78d90e76eaddc39b1b96c7a28b1dd6c
Author: Binbin Zhou <zhoubinbin@loongson.cn>

dmaengine: loongson: loongson2-apb-cmc-dma: Add pause/resume support

This commit adds pause and resume support to the Loongson-2 APB CMC DMA
driver by clearing and restoring LOONGSON2_CMCDMA_CCR_EN while preserving
the hardware transfer position. It also updates status reporting and
cyclic scatterlist advancement to honor the paused state.

> diff --git a/drivers/dma/loongson/loongson2-apb-cmc-dma.c b/drivers/dma/loongson/loongson2-apb-cmc-dma.c
> index c654e05729024..a4153ea854623 100644
> --- a/drivers/dma/loongson/loongson2-apb-cmc-dma.c
> +++ b/drivers/dma/loongson/loongson2-apb-cmc-dma.c

[ ... ]

> @@ -184,6 +185,8 @@ static void loongson2_cmc_dma_stop(struct loongson2_cmc_dma_chan *lchan)
>  	loongson2_cmc_dma_write(lddev, LOONGSON2_CMCDMA_CCR, lchan->id, ccr);
>  
>  	loongson2_cmc_dma_irq_clear(lchan, LOONGSON2_CMCDMA_MASKI);
> +
> +	lchan->paused = false;
>  }

[Severity: Medium]
Can this reset lchan->paused and restart DMA when a segment completion
interrupt races with loongson2_cmc_dma_pause() on a non-cyclic transfer?

loongson2_cmc_dma_pause() allows pausing any channel with an active
lchan->desc, including non-cyclic slave_sg transfers. If a scatterlist
segment finishes and latches LOONGSON2_CMCDMA_TCI just before
loongson2_cmc_dma_pause() acquires lchan->vchan.lock and sets
lchan->paused = true, the interrupt handler will subsequently run:

  loongson2_cmc_dma_chan_irq()
    -> loongson2_cmc_dma_handle_chan_done()
      -> loongson2_cmc_dma_start_transfer()

Unlike the cyclic path in loongson2_cmc_dma_configure_next_sg(), the
non-cyclic path in loongson2_cmc_dma_handle_chan_done() calls
loongson2_cmc_dma_start_transfer():

	loongson2_cmc_dma_stop(lchan);
	...
	/* Start DMA */
	reg->ccr |= LOONGSON2_CMCDMA_CCR_EN;
	loongson2_cmc_dma_write(lddev, LOONGSON2_CMCDMA_CCR, lchan->id, reg->ccr);

This clears lchan->paused = false in loongson2_cmc_dma_stop() and
unconditionally sets LOONGSON2_CMCDMA_CCR_EN for the next scatterlist
entry or next queued descriptor. Should non-cyclic scatterlist
advancement also preserve lchan->paused and avoid re-enabling the channel
while paused?

[ ... ]

> @@ -534,6 +545,13 @@ static enum dma_status loongson2_cmc_dma_tx_status(struct dma_chan *chan, dma_co
>  			/* Active descriptor: compute residue from hardware progress */
>  			state->residue = loongson2_cmc_dma_desc_residue(lchan, lchan->desc,
>  									lchan->next_sg);
> +			/*
> +			 * A paused channel still has an active descriptor but is not making
> +			 * progress. Report DMA_PAUSED so clients can distinguish it from a
> +			 * running transfer.
> +			 */
> +			if (lchan->paused && status == DMA_IN_PROGRESS)
> +				status = DMA_PAUSED;
>  		} else if (vdesc) {
>  			/* Queued descriptor not yet started: residue is the total length */
>  			struct loongson2_cmc_dma_desc *ldesc = to_lmdma_desc(vdesc);

[Severity: Medium]
What happens if a client queries transfer status on a paused channel with
a NULL state pointer, such as via dmaengine_tx_status(chan, cookie, NULL)
or dma_async_is_tx_complete()?

Earlier in loongson2_cmc_dma_tx_status():

	status = dma_cookie_status(chan, cookie, state);
	if (status == DMA_COMPLETE || !state)
		return status;

When state is NULL, loongson2_cmc_dma_tx_status() returns early before
acquiring lchan->vchan.lock and checking lchan->paused, so it will return
DMA_IN_PROGRESS instead of DMA_PAUSED for the active paused descriptor.
Should the lchan->paused check still be performed when state is NULL?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1792988985.git.zhoubinbin@loongson.cn?part=4