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

[Jonathan Cameron <jic23@kernel.org>]

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.

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.


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

> +		 */
>  		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.

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.

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