From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: dma-buf: dma-heap: Keep track of the heap device struct Date: Wed, 04 Mar 2026 07:19:20 +1000 Message-ID: In-Reply-To: <20260303-b4-dmabuf-heap-coherent-rmem-v2-1-65a4653b3378@redhat.com> References: <20260303-b4-dmabuf-heap-coherent-rmem-v2-0-65a4653b3378@redhat.com> <20260303-b4-dmabuf-heap-coherent-rmem-v2-1-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 **From John Stultz, carried by Albert Esteve.** This patch stores the `device *` returned by `device_create()` in `heap->heap_dev` instead of a local variable, and adds a `dma_heap_get_dev()` accessor. **Issues:** 1. **Spurious `get_device()` and extra blank line.** The patch adds: ```c + /* Make sure it doesn't disappear on us */ + heap->heap_dev = get_device(heap->heap_dev); + + /* Add heap to the list */ ``` There's a double blank line here (minor style). More importantly, this `get_device()` takes an extra reference on a device that was just created by `device_create()` -- the heap already holds a reference from creation, and there is no corresponding `put_device()` in any error or teardown path in this patch. This leaks a refcount. Patch 2 restructures things and removes this `get_device()`, but in the meantime patch 1 is broken in isolation. Ideally each patch should be correct standalone (bisectability). 2. **The `heap_dev` pointer is not cleaned up on the `err3` path** (duplicate name). If `device_create()` succeeds but the name check fails, `heap_dev` is stored in the struct but `device_destroy()` cleans up by devt. The field is then freed with `kfree(heap)`. This is fine functionally, but the extra `get_device()` above means the device won't actually be freed. Again, this is resolved in patch 2, but patch 1 in isolation has a refcount leak. --- Generated by Claude Code Patch Reviewer