sashiko.dev review (Re: [PATCH v4 13/17] dmaengine: sh: rz-dmac: Add cyclic DMA support)

[Claudiu Beznea <claudiu.beznea@tuxon.dev>]

 > commit cb66d54ec04fe2598560135c66410142d7295633
 > Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
 > dmaengine: sh: rz-dmac: Add cyclic DMA support
 > This commit introduces cyclic DMA support to the RZ DMAC driver. It adds a
 > per-channel status bit to mark cyclic channels and sets it during the DMA
 > prepare callback. The IRQ handler then uses this bit to determine whether
 > to invoke the cyclic callback function.
 > > diff --git a/drivers/dma/sh/rz-dmac.c b/drivers/dma/sh/rz-dmac.c
 > > index 958ee45abc700..9a10430109e59 100644
 > > --- a/drivers/dma/sh/rz-dmac.c
 > > +++ b/drivers/dma/sh/rz-dmac.c
 > [ ... ]
 > > @@ -586,6 +645,55 @@ rz_dmac_prep_slave_sg(struct dma_chan *chan, struct 
scatterlist *sgl,
 > [ ... ]
 > > +   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);
 > > +   }
 > > +
 > > +   desc->type = RZ_DMAC_DESC_CYCLIC;
 > > +   desc->sgcount = period_len;
 > Can tracking cyclic state via the global channel->status flag lead to
 > cross-descriptor state corruption?
 > If a prepared cyclic descriptor is freed via dmaengine_desc_free(), the
 > global flag might not be cleared during virt-dma desc release, leaving
 > the channel poisoned.

dmaengine_desc_free() works only for reusable descriptors. This functionality is 
not implemented by the rz-dmac driver.


 > Also, because the lock is dropped when exiting the scoped_guard before
 > vchan_tx_prep() is called, a concurrent terminate_all() could potentially
 > clear the flag before the descriptor is queued.
 > Could the driver rely on the desc->type field instead of a global state
 > flag to identify cyclic transfers?

I'm going to explore that but I think this will complicates the code further.

Vinod, Frank,

Could you please let me know if this is a valid expected usecase?

 > [ ... ]
 > > @@ -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);
 > > +           }
 > Will this loop calculate a monotonically decreasing residue for cyclic
 > transfers?

Yes, according to my testing.

 > ALSA requires the residue to be the exact, monotonically decreasing number
 > of bytes remaining in the buffer. Iterating until the end of the descriptor
 > ring and adding the lengths of subsequent periods might cause the returned
 > residue to jump wildly depending on which period is currently executing.
 > For example, in a 3-period buffer (A, B, C), executing period B might
 > return a much larger residue than executing period A, since it would add
 > the remaining bytes in the ring differently.
 > Could this non-monotonic behavior trigger 'PCM position is invalid'
 > errors in the ALSA framework, breaking audio playback?

Never see this in my testing.

 > [ ... ]
 > > @@ -924,10 +1041,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);
 > > +   } else {
 > > +           vchan_cookie_complete(&desc->vd);
 > > +           channel->desc = NULL;
 > If a non-cyclic transfer completes while the global channel->status flag
 > is set, could the IRQ handler erroneously call vchan_cyclic_callback()
 > instead of vchan_cookie_complete(), permanently hanging the channel?
 > Checking desc->type == RZ_DMAC_DESC_CYCLIC here instead might prevent
 > misidentifying non-cyclic transfers as cyclic.

Once the channel is configured as cyclic it cannot be used otherwise. The use 
has to call terminate_all(), synchornize() + re-get + re-prepare the transfers.
So, this exact scenario here is invalid according to the current use case.

If the status is set it means the channel->desc = NULL, which means the IRQ 
handler is not executed up to this point.