Re: [PATCH] dma-mapping: don't trace the DMA address when the allocation fails

[Sean Anderson <sean.anderson@linux.dev>]

On 9/5/26 03:19, Donggeun Yoo wrote:
> dma_alloc_attrs() passes *dma_handle to trace_dma_alloc() and
> debug_dma_alloc_coherent() without checking whether the allocation
> succeeded.

I don't understand what you mean. We are literally in an if-statement
that checks whether the allocation succeeded.

--Sean

> No backend writes it on failure: dma_direct_alloc(),
> iommu_dma_alloc() and the dma_map_ops instances assign it only on the
> path that returns a buffer. Callers usually pass an uninitialized
> automatic variable, so with the tracepoint enabled a failed allocation
> records whatever the stack held, next to the virt_addr=(null) that marks
> the record as an error.
> 
> The device coherent pool path is the same: a non-zero return from
> dma_alloc_from_dev_coherent() means the request was handled, not that it
> succeeded, so cpu_addr is NULL and dma_handle untouched once the pool
> runs out.
> 
> Split both sites on cpu_addr, as dma_alloc_pages() and
> dma_alloc_noncontiguous() do further down the file, and pass 0 for the
> failure case like the two error paths already in this function.
> 
> Fixes: 038eb433dc14 ("dma-mapping: add tracing for dma-mapping API calls")
> Fixes: 68b6dbf1f441 ("dma-mapping: trace more error paths")
> Signed-off-by: Donggeun Yoo <donggeunyoo.kernel@gmail.com>
> ---
> Compile-tested only, with CONFIG_DMA_API_DEBUG=y and CONFIG_TRACEPOINTS=y
> so that both changed calls are built. The claim that no backend writes
> *dma_handle on failure was checked against dma_direct_alloc() and its
> helpers, iommu_dma_alloc(), iommu_dma_alloc_remap() and every in-tree
> dma_map_ops .alloc implementation.
> 
>   kernel/dma/mapping.c | 20 +++++++++++++++-----
>   1 file changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/kernel/dma/mapping.c b/kernel/dma/mapping.c
> index bf2651a70b7c..098cd57e1157 100644
> --- a/kernel/dma/mapping.c
> +++ b/kernel/dma/mapping.c
> @@ -656,8 +656,12 @@ void *dma_alloc_attrs(struct device *dev, size_t size, dma_addr_t *dma_handle,
>   		attrs |= __DMA_ATTR_ALLOC_CC_SHARED;
>   
>   	if (dma_alloc_from_dev_coherent(dev, size, dma_handle, &cpu_addr)) {
> -		trace_dma_alloc(dev, cpu_addr, *dma_handle, size,
> -				DMA_BIDIRECTIONAL, flag, attrs);
> +		if (cpu_addr)
> +			trace_dma_alloc(dev, cpu_addr, *dma_handle, size,
> +					DMA_BIDIRECTIONAL, flag, attrs);
> +		else
> +			trace_dma_alloc(dev, NULL, 0, size, DMA_BIDIRECTIONAL,
> +					flag, attrs);
>   		return cpu_addr;
>   	}
>   
> @@ -676,9 +680,15 @@ void *dma_alloc_attrs(struct device *dev, size_t size, dma_addr_t *dma_handle,
>   		return NULL;
>   	}
>   
> -	trace_dma_alloc(dev, cpu_addr, *dma_handle, size, DMA_BIDIRECTIONAL,
> -			flag, attrs);
> -	debug_dma_alloc_coherent(dev, size, *dma_handle, cpu_addr, attrs);
> +	if (cpu_addr) {
> +		trace_dma_alloc(dev, cpu_addr, *dma_handle, size,
> +				DMA_BIDIRECTIONAL, flag, attrs);
> +		debug_dma_alloc_coherent(dev, size, *dma_handle, cpu_addr,
> +					 attrs);
> +	} else {
> +		trace_dma_alloc(dev, NULL, 0, size, DMA_BIDIRECTIONAL, flag,
> +				attrs);
> +	}
>   	return cpu_addr;
>   }
>   EXPORT_SYMBOL(dma_alloc_attrs);