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

[Claudiu Beznea]

> commit cb66d54ec04fe2598560135c66410142d7295633
> Author: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
> 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.