Re: [PATCH] iio: buffer-dmaengine: fix sg entry iteration when building dma_vecs

["Nuno Sá" <nuno.sa@analog.com>]

On Sun, Aug 23, 2026 at 01:16:18AM +0100, Jonathan Cameron wrote:
> On Wed, 19 Aug 2026 11:16:22 +0100
> Nuno Sá <nuno.sa@analog.com> wrote:
> 
> > On Wed, Aug 19, 2026 at 01:41:04AM +0100, Jonathan Cameron wrote:
> > > On Tue, 18 Aug 2026 17:45:29 +0100
> > > Nuno Sá <nuno.sa@analog.com> wrote:
> > >   
> > > > From: Michael Hennerich <michael.hennerich@analog.com>
> > > > 
> > > > iio_dmaengine_buffer_submit_block() counts scatterlist entries with
> > > > sg_nents_for_len(), which walks the CPU-side lengths (sg->length), but
> > > > then consumes the DMA-side fields (sg_dma_address()/sg_dma_len()).
> > > > After dma_map_sgtable() the two views may differ: an IOMMU can coalesce
> > > > the mapping so that only the first sgt->nents entries carry valid DMA
> > > > addresses, with nents < orig_nents.
> > > > 
> > > > On x86 with an IOMMU enabled, a DMABUF block backed by two 1 MiB
> > > > system-heap chunks maps to a single 2 MiB IOVA range. The CPU-side
> > > > count is 2, so the loop reads one entry past the mapped set and emits a
> > > > garbage vec ({addr = ~0, len = 0}). The DMA engine driver rejects the
> > > > vec array (prep returns NULL), the fence is signalled with -ENOMEM,
> > > > which a userspace poller cannot observe, and the block is left in
> > > > ACTIVE state so every further enqueue of it fails with -EBUSY. The
> > > > visible symptom is a stream of zero-filled blocks followed by a wedged
> > > > buffer.
> > > > 
> > > > Platforms without an IOMMU never hit this because nents == orig_nents.
> > > > 
> > > > Size the vec array with sg_nents_for_dma(), which walks the DMA-mapped
> > > > view and accounts for max-length splitting, and stop the fill loop once
> > > > bytes_used is covered - which is allowed to be smaller than the block
> > > > size - passing the reduced count to dmaengine_prep_peripheral_dma_vec().
> > > > 
> > > > Assisted-by: Claude:claude-fable-5
> > > > Fixes: 7a86d469983a ("iio: buffer-dmaengine: Support new DMABUF based userspace API")
> > > > Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
> > > > Signed-off-by: Nuno Sá <nuno.sa@analog.com>  
> > > 
> > > There are some gremlins nearby in this code...
> > > In the else just of this context seems max_size is computed again
> > > having been done just above the code seen here.
> > > 
> > > Unless I'm missing something that should be cleaned up as well.  
> > 
> > Oh yes! Something I already noticed a couple of times but never sent the
> > patch right away so I kept forgetting about it.
> > > 
> > > Been a while since I got my head into the scatterlist
> > > stuff, so I might have it wrong below, but I don't think what
> > > you have here actually works if the merging of entries is
> > > larger than the max dma entry the hardware supports.
> > >   
> > 
> > In theory might not be a bug but I'm not sure it's a claim we can fully
> > take as guarantee.
> > 
> > >   
> > > > ---
> > > > Note the Signed-off-by is just because I'm carrying Michael's patch!
> > > > ---
> > > >  drivers/iio/buffer/industrialio-buffer-dmaengine.c | 16 ++++++++++++----
> > > >  1 file changed, 12 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/drivers/iio/buffer/industrialio-buffer-dmaengine.c b/drivers/iio/buffer/industrialio-buffer-dmaengine.c
> > > > index ecc02a427b92..bece45381c8c 100644
> > > > --- a/drivers/iio/buffer/industrialio-buffer-dmaengine.c
> > > > +++ b/drivers/iio/buffer/industrialio-buffer-dmaengine.c
> > > > @@ -104,10 +104,16 @@ static int iio_dmaengine_buffer_submit_block(struct iio_dma_buffer_queue *queue,
> > > >  	if (block->sg_table) {
> > > >  		unsigned long flags;
> > > >  
> > > > +		/*
> > > > +		 * Use the DMA-mapped view of the sg_table: after mapping
> > > > +		 * (e.g. through an IOMMU) the DMA entries (sgt->nents) can be
> > > > +		 * fewer than the CPU entries, and sg_dma_address()/sg_dma_len()
> > > > +		 * are only valid for the first sgt->nents entries. Counting
> > > > +		 * with sg_nents_for_len() (CPU lengths) walks past them and
> > > > +		 * hands garbage vecs to the DMA engine.  
> > > 
> > > This feels like too much info after the fix is in place.  Talking about other
> > > stuff that would be wrong is rather unusual.
> > >  
> > 
> > Oh well, I guess LLM over commenting as usual.
> > 
> > > > +		 */
> > > >  		sgl = block->sg_table->sgl;
> > > > -		nents = sg_nents_for_len(sgl, block->bytes_used);
> > > > -		if (nents < 0)
> > > > -			return nents;
> > > > +		nents = sg_nents_for_dma(sgl, block->sg_table->nents, max_size);  
> > > 
> > > So this fun function will generally give us the number of sgl entries, but not
> > > quite always. It will give us how many chunks of up to max_size fit into
> > > a particularly large entry.
> > >   
> > > >  
> > > >  		vecs = kmalloc_array(nents, sizeof(*vecs), GFP_ATOMIC);
> > > >  		if (!vecs)
> > > > @@ -115,7 +121,7 @@ static int iio_dmaengine_buffer_submit_block(struct iio_dma_buffer_queue *queue,
> > > >  
> > > >  		len_total = block->bytes_used;
> > > >  
> > > > -		for (i = 0; i < nents; i++) {
> > > > +		for (i = 0; i < nents && len_total; i++) {  
> > > So this needs to be more clever as we aren't just iterating entrees and filling
> > > them in, some of them could at least in theory be too big to fit
> > > in a single vec - hence you need to do a loop in here that sets
> > > multiple entries if that occurs.  
> > 
> > I see! But I think that merging means that we end up with more nents
> > (sg_nents_for_len()) entries than DMA ones which was the issue we had
> > because we were left with vecs with invalid addresses and 0 sized.
> 
> You can end up with a smaller set of DMA mappings than the CPU ones that
> were typically pushed in.  That covers what happens in the IOMMU.
> However those can be 'big' and so your DMA engines own ability to handle
> a particular size of contiguous block may be the problem and result in
> you needing more entrees on the hardware side of things.
> 
> > The other
> > way around should not happen on the IOMMU path at least given the assumption
> > that a single sg  entry must not be bigger than max_len [1].
> Hmm - do I understand this right - is the question: If you original CPU side
> mappings (the input scatter gather list entries) are less than max_len bytes each

That seems to be the assumption taken in the IOMMU merge path.
And it seems that there's even some debug configs we can use that
will trigger some warning about this [1].

> then can we end up with a large set of entries for the DMA Engine after
> the merge then split?  If so I think I think the answer is no (as long
> as no other constraints like alignment make a mess for us - I gave
> up trying to reason those out on a bit of paper).
> 
> > I think in theory
> > that can actually happen (if we end using a dma provider defaulting to 64K and 
> > cma dma_bufs) but maybe we can argue that's not our problem? Given
> > the assumptions of course.
> > 
> > But even if we somehow reach this path with sg_dma_address > max_size
> > and end up stuffing all of it in say, vec[0], the DMA controller should
> > also care to make sure it splits each vec if it exceeds the descriptor
> > max size. That's what both users of (ADI axi_dmac being one them)
> > .device_prep_peripheral_dma_vec() are doing today. But yes, I agree that
> > might be a bold assumption.
> 
> Then why we are we passing in max_size to the function to work out
> how many entrees?  If splits are handled elsewhere we don't need that
> limit.
> 

I guess it was what made sense to pass to sg_nents_for_dma() at this
level.

> > 
> > > 
> > > If that can't happen for some other reason then I think you can 
> > > just use block->sgtable->nents instead of the more complex call above.
> > >   
> > 
> > I was the one suggesting the other call because it seemed what we
> > actually wanted but I'm fine with just using the above. I'm more tempted
> > to keep it simple and use 'block->sgtable->nents' rather than jumping in
> > a more complex subloop without clear evidence we really need it.
> > Thoughts?
> 
> If they are split below this layer then indeed we don't need the call
> and can use the number DMA entries.

I want to reinforce that's what happening on both controllers
implementing the vector based callback. But I do think it's a fair
assumption given that there's no way we can actually know the
channel/descriptor size a dma controller can handle. AFAIK, it's not
exported anywhere. At this level we just know about the segment max_ size a
mapping can have.

If we also look at all users of sg_nents_for_dma(), we only see dma
controllers drivers using it (with the len argument being precisely what
looks like a descriptor max size). So yes, I'm more convinced this layer
is the wrong place to use such an helper and I'll just use number of
entries we got from the mappings.

[1]: https://elixir.bootlin.com/linux/v7.2/source/kernel/dma/debug.c#L1230
- Nuno Sá

> 
> > 
> > As a fun side note this also made visible another subtle issue. Given
> > that submitting a block might only happen when enabling the buffer (so
> > async to enqueueing it) when we hit this issue, we get -ENOMEM and
> > and go ahead  to wake up poll(). But AFAIK, we can't really propagate
> > the error code up to userspace so userspace wakes up just to get
> > garbage and thinking everything is fine. Easy way out would be to treat
> > this as and hard error and return error when enabling the buffer. But
> > that also raises the question that other blocks might have been properly
> > submitted though. Not really sure how to handle this but anyways a problem
> > for another day :)
> 
> Hmm. It is always tricky to cleanly surface errors when we get a problem
> somewhere random in DMA.  No idea off the top of my head on how to solve
> this case.
> 
> Jonathan
> 
> > 
> > [1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ab2cbeb0ed301a9f0460078e91b09f39958212ef
> > 
> > - Nuno Sá
> > 
> > > >  			vecs[i].addr = sg_dma_address(sgl);
> > > >  			vecs[i].len = min(sg_dma_len(sgl), len_total);
> > > >  			len_total -= vecs[i].len;
> > > > @@ -133,6 +139,8 @@ static int iio_dmaengine_buffer_submit_block(struct iio_dma_buffer_queue *queue,
> > > >  		 * before it can run, so always set the EOT flag.
> > > >  		 */
> > > >  		flags |= DMA_PREP_LOAD_EOT;
> > > > +		nents = i;
> > > > +
> > > >  		desc = dmaengine_prep_peripheral_dma_vec(dmaengine_buffer->chan,
> > > >  							 vecs, nents, dma_dir,
> > > >  							 flags);
> > > > 
> > > > ---
> > > > base-commit: b756b143e5391151e577ae645b1378a43f93c2f5
> > > > change-id: 20260818-iio-buffer-dmabuf-iommu-fic-1b281f15e5a4
> > > > --
> > > > 
> > > > Thanks!
> > > > - Nuno Sá
> > > >   
> > >   
>