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

[Claudiu Beznea]

>commit 206116040d966fa7859b2dc81e454bdbed7acd43
>Author: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
>    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);
>> +            }
>>      }