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

[Jonathan Cameron <jic23@kernel.org>]

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?

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