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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Modifying global channel state during cyclic descriptor preparation causes race conditions and state corruption.
- [High] Cyclic residue calculation omits the last descriptor, resulting in under-reported residue.
- [High] Stale hardware configuration fields leak from previous transfers into cyclic transfers.
--

commit 206116040d966fa7859b2dc81e454bdbed7acd43
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

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

This commit adds 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.

> diff --git a/drivers/dma/sh/rz-dmac.c b/drivers/dma/sh/rz-dmac.c
> index 2de519b581b60..d6ad070be705c 100644
> --- a/drivers/dma/sh/rz-dmac.c
> +++ b/drivers/dma/sh/rz-dmac.c

[ ... ]

> @@ -431,6 +435,57 @@ static void rz_dmac_prepare_descs_for_slave_sg(struct rz_dmac_chan *channel)
>  	channel->chctrl = 0;
>  }
>  
> +static void rz_dmac_prepare_descs_for_cyclic(struct rz_dmac_chan *channel)
> +{
> +	struct dma_chan *chan = &channel->vc.chan;
> +	struct rz_dmac *dmac = to_rz_dmac(chan->device);
> +	struct rz_dmac_desc *d = channel->desc;
> +	size_t period_len = d->sgcount;
> +	struct rz_lmdesc *lmdesc;
> +	size_t buf_len = d->len;
> +	size_t periods = buf_len / period_len;
> +
> +	lockdep_assert_held(&channel->vc.lock);
> +
> +	channel->chcfg |= CHCFG_SEL(channel->index) | CHCFG_DMS;

Are stale hardware configuration fields leaking from previous transfers here?

If a channel was previously used for a memory-to-memory transfer,
channel->chctrl might retain the CHCTRL_STG (Software Trigger Generation) bit.
If it was used for slave_sg, channel->chcfg might retain the CHCFG_DEM
(Descriptor End Mask) bit.

Because channel->chcfg is modified via OR-assignment and channel->chctrl
is not initialized to 0, could the stale CHCTRL_STG bit cause the peripheral
transfer to continuously trigger via software rather than synchronizing with
the hardware DREQ signal? Additionally, could a stale CHCFG_DEM bit mask
every period interrupt and prevent vchan_cyclic_callback() from being invoked?

[ ... ]

> @@ -586,6 +645,55 @@ rz_dmac_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
>  	return vchan_tx_prep(&channel->vc, &desc->vd, flags);
>  }
>  
> +static struct dma_async_tx_descriptor *
> +rz_dmac_prep_dma_cyclic(struct dma_chan *chan, dma_addr_t buf_addr,
> +			size_t buf_len, size_t period_len,
> +			enum dma_transfer_direction direction,
> +			unsigned long flags)
> +{
> +	struct rz_dmac_chan *channel = to_rz_dmac_chan(chan);
> +	struct rz_dmac_desc *desc;
> +	size_t periods;
> +
> +	if (!is_slave_direction(direction))
> +		return NULL;
> +
> +	if (!period_len || !buf_len)
> +		return NULL;
> +
> +	periods = buf_len / period_len;
> +	if (!periods || periods > DMAC_NR_LMDESC)
> +		return NULL;
> +
> +	scoped_guard(spinlock_irqsave, &channel->vc.lock) {
> +		if (channel->status & BIT(RZ_DMAC_CHAN_STATUS_CYCLIC))
> +			return NULL;
> +
> +		desc = list_first_entry_or_null(&channel->ld_free, struct rz_dmac_desc, node);
> +		if (!desc)
> +			return NULL;
> +
> +		list_del(&desc->node);
> +
> +		channel->status |= BIT(RZ_DMAC_CHAN_STATUS_CYCLIC);

Can this global channel state modification during descriptor preparation cause
race conditions?

The DMA engine API permits descriptors to be prepared concurrently with active
transfers, or to be freed without being issued. If a slave_sg transfer is
running when prep_dma_cyclic() is called, the global channel status immediately
becomes CYCLIC.

When the active slave_sg transfer triggers an interrupt, wouldn't
rz_dmac_irq_handler_thread() see this flag and incorrectly execute the cyclic
path (vchan_cyclic_callback) instead of vchan_cookie_complete()?

Would it be safer to determine the execution mode by checking the currently
active descriptor's type (e.g., desc->type == RZ_DMAC_DESC_CYCLIC) instead of
updating a global channel flag during preparation?

[ ... ]

> @@ -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) {

Does this omit the residue from the last descriptor in the cyclic ring?

The last descriptor is configured such that its nxla field points to
start_lmdesc_addr in order to loop back. Because the loop condition evaluates
to false exactly when lmdesc reaches this last descriptor, it terminates
before adding lmdesc->tb (the transfer byte count) to the residue.

Could this cause the residue to be permanently under-reported by one full
period length, affecting subsystems like ALSA that rely on strict monotonic
residue reporting?

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260512121219.216159-1-claudiu.beznea.uj@bp.renesas.com?part=12