dma-mapping: don't trace the DMA address when the allocation fails

dma_alloc_attrs() passes *dma_handle to trace_dma_alloc() and debug_dma_alloc_coherent() without checking whether the allocation succeeded.

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 <[email protected]>

---
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(-)

kernel/dma/mapping.c +15 -5

index bf2651a70b7c..098cd57e1157 100644
@@ -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);

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

Quoted 2 lines · Sean Anderson

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

That if checks whether the device has a coherent pool, not whether the allocation out of it worked. dma_alloc_from_dev_coherent():

* Returns 0 if dma_alloc_coherent should continue with allocating from
* generic memory areas, or !0 if dma_alloc_coherent should return @ret.

It returns 1 as soon as it finds a pool; __dma_alloc_from_coherent() stores NULL in @ret and leaves *dma_handle alone when the request is larger than the pool or the pool is full. The third paragraph of the commit message says this.

The second hunk is under no condition at all: dma_direct_alloc(), iommu_dma_alloc() and ops->alloc() return NULL on failure, and the trace below the if/else chain runs either way.

Ah, I see what you mean. TBH I would just do something like

cpu_handle ? *dma_handle : NULL

to make it clear what we're doing

dma_alloc_attrs() passes *dma_handle to trace_dma_alloc() and debug_dma_alloc_coherent() without checking whether the allocation succeeded. 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 <[email protected]>

---
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(-)

kernel/dma/mapping.c +0 -0

index bf2651a70b7c..098cd57e1157 100644
@@ -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;

kernel/dma/mapping.c +0 -2

 		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);

Frankly speaking I would prefer to fix this in include/trace/events/dma.h with __entry->dma_addr = virt_addr ? dma_addr : 0;

Here You remove the debug_dma_alloc call for the failed case. Right, it is a noop now for the allocation failure, but it might be extended with some kind of debugging in the future, so I prefer to keep the current code here.