Re: [PATCH] iio: buffer-dmaengine: Add support for cyclic DMA transfers

["Nuno Sá" <noname.nuno@gmail.com>]

On Mon, Jul 06, 2026 at 04:47:19PM +0100, Jonathan Cameron wrote:
> On Mon, 6 Jul 2026 11:07:58 +0100
> Nuno Sá <noname.nuno@gmail.com> wrote:
> 
> > On Fri, Jul 03, 2026 at 07:52:38PM +0100, Jonathan Cameron wrote:
> > > On Thu, 11 Jun 2026 16:28:57 +0100
> > > Nuno Sá via B4 Relay <devnull+nuno.sa.analog.com@kernel.org> wrote:
> > >   
> > > > From: Nuno Sá <nuno.sa@analog.com>
> > > > 
> > > > Allow buffer blocks flagged as cyclic to be submitted as repeating DMA
> > > > transfers. For cyclic blocks, use DMA_PREP_REPEAT so the engine keeps
> > > > replaying the descriptor.
> > > > 
> > > > Skip installing the completion callback for cyclic blocks. Since the
> > > > transfer is continuously replayed, the callback would fire on every
> > > > period, throwing off the block refcount.
> > > > 
> > > > Because nothing prevents a new cyclic transfer from replacing an
> > > > already active cyclic one, always set DMA_PREP_LOAD_EOT so the engine
> > > > correctly terminates the active transfer before loading the new
> > > > descriptor.
> > > >   
> > > I'd like a little more use case stuff in here.  Basically I had
> > > same question David did on when this was useful.  
> > 
> > Alright! I'll re-spin with an improved commit message.
> > 
> > > 
> > > Otherwise, seems fine to me. Sashiko raised a concern about a particular
> > > sequence of blocks being added.  
> > > https://sashiko.dev/#/patchset/20260611-iio-dma-cyclic-buffer-support-v1-1-bcf00e8d802c%40analog.com
> > > I don't really understand this code well enough to be sure if the issue
> > > is real or not!  
> > 
> > Hmm I mean, for the same block it's not a problem as we return -EBUSY.
> > Now for the below:
> > 
> > "Could an unprivileged user repeatedly attach and enqueue new DMABUFs as
> > cyclic"
> > 
> > Don't think the "unprivileged" part is true but a privileged one could
> > in theory just keep the list growing until we disable the buffer. But
> > this is part of the choice done in the patch note and AFAIU,
> > dmabuf-fences also have this kind of "don't unref me right away"
> > behavior.
> Should we perhaps set a defensive cap given we think this is unreasonable
> behaviour?  So fail to queue a new one if the backlog is 16 long or something
> like that?

I guess we can add that for some piece of mind. In reality I'm not even
aware of any usecase where we enqueue more than one cyclic transfer so a
backlog of 16 should be more than enough! I just allowed one cyclic to
end another one because I don't see any technical reason for not allowing it.

- Nuno Sá

> 
> J
> > 
> > - Nuno Sá
> > 
> > >   
> > > > Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> > > > ---
> > > > There's one subtle choice in here. Given that the termination callback
> > > > is not set. We will never give the block refcount. That means cyclic
> > > > blocks are only completely freed when we disable the buffer and
> > > > iio_dmaengine_buffer_abort() get's called. So no leak, we just defer it
> > > > as it makes it more simple to handle. I also think this a fair
> > > > expectation from a cyclic transfer. We set it up and let it run until we
> > > > disable the buffer.  
> > > 
> > > To me that seems fine
> > >   
> > > > 
> > > > Alternatively, we can give in the refcount as soon as we give the block
> > > > to the DMA layer with dma_async_issue_pending(). But we also need to
> > > > make sure that the block is not added to the dmaengine_buffer->active list.
> > > > As said, I feel that the current approach is just simpler.
> > > > ---
> > > >  drivers/iio/buffer/industrialio-buffer-dmaengine.c | 19 ++++++++++++++++---
> > > >  1 file changed, 16 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/iio/buffer/industrialio-buffer-dmaengine.c b/drivers/iio/buffer/industrialio-buffer-dmaengine.c
> > > > index 98acce909854..4a78cd3e7c7d 100644
> > > > --- a/drivers/iio/buffer/industrialio-buffer-dmaengine.c
> > > > +++ b/drivers/iio/buffer/industrialio-buffer-dmaengine.c
> > > > @@ -80,6 +80,8 @@ static int iio_dmaengine_buffer_submit_block(struct iio_dma_buffer_queue *queue,
> > > >  		dma_dir = DMA_MEM_TO_DEV;
> > > >  
> > > >  	if (block->sg_table) {
> > > > +		unsigned long flags;
> > > > +
> > > >  		sgl = block->sg_table->sgl;
> > > >  		nents = sg_nents_for_len(sgl, block->bytes_used);
> > > >  		if (nents < 0)
> > > > @@ -99,9 +101,18 @@ static int iio_dmaengine_buffer_submit_block(struct iio_dma_buffer_queue *queue,
> > > >  			sgl = sg_next(sgl);
> > > >  		}
> > > >  
> > > > +		if (block->cyclic)
> > > > +			flags = DMA_PREP_REPEAT;
> > > > +		else
> > > > +			flags = DMA_PREP_INTERRUPT;
> > > > +
> > > > +		/*
> > > > +		 * There's nothing preventing a cyclic transfer to replace an active
> > > > +		 * cyclic one. So always set the EOT flag.
> > > > +		 */
> > > >  		desc = dmaengine_prep_peripheral_dma_vec(dmaengine_buffer->chan,
> > > >  							 vecs, nents, dma_dir,
> > > > -							 DMA_PREP_INTERRUPT);
> > > > +							 flags | DMA_PREP_LOAD_EOT);
> > > >  		kfree(vecs);
> > > >  	} else {
> > > >  		max_size = min(block->size, dmaengine_buffer->max_size);
> > > > @@ -122,8 +133,10 @@ static int iio_dmaengine_buffer_submit_block(struct iio_dma_buffer_queue *queue,
> > > >  	if (!desc)
> > > >  		return -ENOMEM;
> > > >  
> > > > -	desc->callback_result = iio_dmaengine_buffer_block_done;
> > > > -	desc->callback_param = block;
> > > > +	if (!block->cyclic) {
> > > > +		desc->callback_result = iio_dmaengine_buffer_block_done;
> > > > +		desc->callback_param = block;
> > > > +	}
> > > >  
> > > >  	cookie = dmaengine_submit(desc);
> > > >  	if (dma_submit_error(cookie))
> > > > 
> > > > ---
> > > > base-commit: ae696dfa47c30016cd429b9db5e70b259b8f509e
> > > > change-id: 20260609-iio-dma-cyclic-buffer-support-f18034f8f34c
> > > > --
> > > > 
> > > > Thanks!
> > > > - Nuno Sá
> > > > 
> > > >   
> > >   
>