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: coherent: store reserved memory coherent regions Date: Mon, 09 Mar 2026 09:02:40 +1000 Message-ID: In-Reply-To: <20260306-b4-dmabuf-heap-coherent-rmem-v3-4-3d00d36c9bc4@redhat.com> References: <20260306-b4-dmabuf-heap-coherent-rmem-v3-0-3d00d36c9bc4@redhat.com> <20260306-b4-dmabuf-heap-coherent-rmem-v3-4-3d00d36c9bc4@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 **Issue: Static array with hardcoded limit** ```c #define MAX_COHERENT_REGIONS 64 static struct reserved_mem *rmem_coherent_areas[MAX_COHERENT_REGIONS]; static unsigned int rmem_coherent_areas_num; ``` 64 regions is probably sufficient for practical purposes, but using a static array feels fragile. Since `rmem_dma_setup()` is called early (from `__reservedmem_of_table`), dynamic allocation may not be available, so this is acceptable. However, the error return value should be `-ENOSPC` rather than `-EINVAL`: ```c if (rmem_coherent_areas_num >= MAX_COHERENT_REGIONS) { ... return -EINVAL; } ``` `-ENOSPC` or `-ENOMEM` would be more descriptive. **Issue: `#include ` added to coherent.c but not used** ```c +#include ``` This header is added but nothing from it is used in this file. The `IS_ENABLED(CONFIG_DMABUF_HEAPS_COHERENT)` check doesn't require it. This include should be dropped. **Design concern: `IS_ENABLED()` check couples DMA core to heap config** ```c if (IS_ENABLED(CONFIG_DMABUF_HEAPS_COHERENT)) { int ret = rmem_coherent_insert_area(rmem); ``` This is noted in the commit message. The `IS_ENABLED()` approach means the storage array and insertion code are always compiled in but guarded at runtime. Since this is `__init` code, it's not a runtime concern, but it does add code to `kernel/dma/coherent.c` that only matters when the heap module is loaded. An alternative would be to have the heap module iterate reserved memory regions itself, avoiding coupling. However, since `rmem_dma_setup()` is an `__init` function and the rmem structs are available post-init, the module could potentially walk the reserved_mem array directly. That said, this approach works. **Thread safety**: The array is populated at `__init` time (single-threaded) and read later by the module init. No races possible. Fine. --- --- Generated by Claude Code Patch Reviewer