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

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

On Wed, Jul 08, 2026 at 01:02:13AM +0100, Jonathan Cameron wrote:
> On Tue, 7 Jul 2026 11:51:38 +0100
> Nuno Sá <noname.nuno@gmail.com> wrote:
> 
> > 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.
> > 
> Then maybe cap it at one until we have that usecase.

Fair enough!

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