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

[Claudiu Beznea <claudiu.beznea@kernel.org>]

 >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?

It is expected the channel to be released before changing its mode (cyclic, sg, 
mem-to-mem) and requested again. When the channel is requested the 
channel->chcfg is properly reinitialized in rz_dmac_chan_filter() with the 
device tree data.

 >[ ... ]
 >> @@ -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?

Once a channel has been configured as cyclic channel it can be used only as 
cyclic channel. HW don't allow changing its usage like mentioned above.

 >[ ... ]
 >> @@ -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?

I did a lot of audio testing with this code and never encountered issues with 
the current form of this code.

 >> +                    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);
 >> +            }
 >>      }