Re: [PATCH v6 12/18] dmaengine: sh: rz-dmac: Add cyclic DMA support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Using a global channel status flag to track cyclic transfer state causes race conditions, incorrect handling of concurrent transfers, and state leaks.
- [High] The calculation of residue for cyclic transfers evaluates to erratic, mathematically incorrect values.
--

commit 0cfb7df401a4633fd8fdbdc75fec6ad11e0803e3
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

dmaengine: sh: rz-dmac: Add cyclic DMA support

Add cyclic DMA support to the RZ DMAC driver. A per-channel status bit is
introduced to mark cyclic channels and is set during the DMA prepare
callback. The IRQ handler checks this status bit and calls
vchan_cyclic_callback() accordingly.

> diff --git a/drivers/dma/sh/rz-dmac.c b/drivers/dma/sh/rz-dmac.c
> index c9c00650ddd5d..8fd8a4bd9cc90 100644
> --- a/drivers/dma/sh/rz-dmac.c
> +++ b/drivers/dma/sh/rz-dmac.c

[ ... ]

> @@ -733,9 +841,18 @@ static u32 rz_dmac_calculate_residue_bytes_in_vd(struct rz_dmac_chan *channel,
>  	}
>  
>  	/* Calculate residue from next lmdesc to end of virtual desc */
> -	while (lmdesc->chcfg & CHCFG_DEM) {
> -		residue += lmdesc->tb;
> -		lmdesc = rz_dmac_get_next_lmdesc(channel->lmdesc.base, lmdesc);
> +	if (channel->status & BIT(RZ_DMAC_CHAN_STATUS_CYCLIC)) {
> +		u32 start_lmdesc_addr = rz_dmac_lmdesc_addr(channel, desc->start_lmdesc);
> +
> +		while (lmdesc->nxla != start_lmdesc_addr) {
> +			residue += lmdesc->tb;
> +			lmdesc = rz_dmac_get_next_lmdesc(channel->lmdesc.base, lmdesc);
> +		}

[Severity: High]
Does this loop calculate the remaining bytes correctly for cyclic
transfers?

The earlier loop leaves lmdesc pointing to the descriptor preceding the
currently executing one. If the loop here sums lmdesc->tb from this
preceding descriptor up to (but excluding) the last descriptor, won't the
returned residue values be cyclically shifted by one period?

For example, in a 3-period buffer (A, B, C), if the hardware is executing
period B, summing the transfer bytes of A and B would return
crtb + 2 * period_len instead of the correct crtb + period_len. Could this
cause audio cyclic pointers to jump erratically backwards and forwards?

> +	} else {
> +		while (lmdesc->chcfg & CHCFG_DEM) {
> +			residue += lmdesc->tb;
> +			lmdesc = rz_dmac_get_next_lmdesc(channel->lmdesc.base, lmdesc);
> +		}
>  	}

[ ... ]

> @@ -928,10 +1045,14 @@ static irqreturn_t rz_dmac_irq_handler_thread(int irq, void *dev_id)
>  	if (!desc)
>  		return IRQ_HANDLED;
>  
> -	vchan_cookie_complete(&desc->vd);
> -	channel->desc = NULL;
> +	if (channel->status & BIT(RZ_DMAC_CHAN_STATUS_CYCLIC)) {
> +		vchan_cyclic_callback(&desc->vd);

[Severity: High]
Should this check desc->type == RZ_DMAC_DESC_CYCLIC instead of the global
channel status?

If a cyclic transfer is prepared while a non-cyclic transfer is currently
executing, the global channel status will be updated immediately. When the
active non-cyclic transfer finishes, won't this logic incorrectly call
vchan_cyclic_callback() for the non-cyclic descriptor?

This could skip vchan_cookie_complete() and rz_dmac_xfer_desc(), which
would leak the descriptor and stall the DMA channel.

> +	} else {
> +		vchan_cookie_complete(&desc->vd);
> +		channel->desc = NULL;
>  
> -	rz_dmac_xfer_desc(channel);
> +		rz_dmac_xfer_desc(channel);
> +	}
>  
>  	return IRQ_HANDLED;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260526084710.3491480-1-claudiu.beznea@kernel.org?part=12