From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: dma-buf: dma-heap: split dma_heap_add
Date: Wed, 04 Mar 2026 07:19:20 +1000
Message-ID: <review-patch2-20260303-b4-dmabuf-heap-coherent-rmem-v2-2-65a4653b3378@redhat.com>
In-Reply-To: <20260303-b4-dmabuf-heap-coherent-rmem-v2-2-65a4653b3378@redhat.com>
References: <20260303-b4-dmabuf-heap-coherent-rmem-v2-0-65a4653b3378@redhat.com> <20260303-b4-dmabuf-heap-coherent-rmem-v2-2-65a4653b3378@redhat.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0

Patch Review

This splits `dma_heap_add()` into `dma_heap_create()` + `dma_heap_register()`, with a `dma_heap_destroy()` that calls `put_device()`, and a `dma_heap_dev_release()` callback.

**Issues:**

1. **Type error / wrong return in refactored `dma_heap_add()`.** The refactored `dma_heap_add()` has:
```c
+	heap = dma_heap_create(exp_info);
+	if (IS_ERR(heap)) {
+		pr_err("dma_heap: failed to create heap (%d)\n", PTR_ERR(heap));
+		return PTR_ERR(heap);
+	}
```
`dma_heap_add()` returns `struct dma_heap *`, but `PTR_ERR(heap)` returns `long`. This is a type mismatch -- it should be `return ERR_CAST(heap)` or just `return heap`. Note: Patch 4 fixes this exact bug by changing it to `ERR_CAST(heap)` and the format specifier to `%ld`, but that means patch 2 is broken in isolation (it will produce a compiler warning and return a truncated pointer, not an error-encoded pointer, on 64-bit).

2. **`dma_heap_register()` error path calls `dma_heap_destroy()` after `device_destroy()`.** In `dma_heap_register()`:
```c
err3:
	device_destroy(dma_heap_class, heap->heap_devt);
...
err0:
-	kfree(heap);
-	return err_ret;
+	dma_heap_destroy(heap);
+	return ret;
```
`dma_heap_destroy()` calls `put_device(heap->heap_dev)`, which may trigger `dma_heap_dev_release()` and free the device. But `device_destroy()` on `err3` already destroys the device by class+devt. If `device_add()` succeeded (err3 path), calling both `device_destroy()` and then `put_device()` would be a double-free/use-after-free. The err3 path should probably just call `device_del()` (remove from sysfs) and let the `put_device()` in `dma_heap_destroy()` handle the final free, or the destroy should skip the put if device_destroy already handled it.

   Actually, looking more carefully: `device_destroy()` calls `device_unregister()` which is `device_del()` + `put_device()`. So after `device_destroy()`, the device reference from `device_add()` is dropped. Then `dma_heap_destroy()` -> `put_device()` drops the reference from `device_initialize()`. Since `device_add()` takes an extra ref, and `device_initialize()` sets refcount to 1, this sequence might actually be correct (the initialize ref + the add ref = 2, device_destroy drops 1, dma_heap_destroy drops 1). But this is fragile and non-obvious -- it relies on `device_add()` taking a get_device. This deserves a comment at minimum.

3. **`dma_heap_dev_release()` frees `heap->name` and `heap`.** In `dma_heap_create()`, `heap->name = exp_info->name` just copies the pointer. If the caller allocated the name (as coherent_heap does with `kstrdup`), freeing it in the device release is correct. But for existing callers that pass string literals (e.g., the system heap passes `"system"`), `kfree(heap->name)` on a string literal would be undefined behavior. The existing `dma_heap_add()` wrapper doesn't copy the name either. This is a latent bug for any code path that triggers device release on a heap created via `dma_heap_add()`.

4. **Minor:** `dma_heap_create()` uses `kzalloc_obj(*heap->heap_dev)` which allocates a bare `struct device`. This works but is unusual -- most kernel subsystems embed the device in a larger struct or use specialized device allocation. The comment in the release function `kfree(dev)` confirms this is a standalone allocation.

---
Generated by Claude Code Patch Reviewer