Claude review: rust: gpu: Add GPU buddy allocator bindings

[Claude Code Review Bot <claude-review@example.com>]

Patch Review

Overall this is a well-written patch. Detailed comments below:

**1. Dead helper: `rust_helper_gpu_buddy_block_size` is defined but never called**

In `rust/helpers/gpu.c`:
```c
__rust_helper u64 rust_helper_gpu_buddy_block_size(struct gpu_buddy *mm,
                           struct gpu_buddy_block *block)
{
    return gpu_buddy_block_size(mm, block);
}
```

This helper is exported but the Rust code never uses it. `AllocatedBlock::size()` computes the same value inline:
```rust
pub fn size(&self) -> u64 {
    self.alloc.buddy.chunk_size << self.block.order()
}
```

Either use the C helper (for consistency, ensuring the Rust code tracks any future changes to the C computation) or remove the dead helper. The inline computation is fine in practice since `chunk_size << order` is the stable definition, but having an unused helper is confusing.

**2. `BuddyFlags` validation could check for unknown bits**

In `BuddyFlags::try_new()`:
```rust
pub fn try_new(flags: usize) -> Result<Self> {
    if flags > u32::MAX as usize {
        return Err(EINVAL);
    }
    if (flags & Self::RANGE_ALLOCATION) != 0 && (flags & Self::TOPDOWN_ALLOCATION) != 0 {
        return Err(EINVAL);
    }
    Ok(Self(flags))
}
```

This validates the range and the RANGE+TOPDOWN conflict, which is good. However, it doesn't reject unknown flag bits. If a caller passes e.g. `BIT(6) | BIT(7)` those would be silently accepted and passed to the C API. Consider adding a mask check:
```rust
const ALL_KNOWN: usize = Self::RANGE_ALLOCATION | Self::TOPDOWN_ALLOCATION
    | Self::CONTIGUOUS_ALLOCATION | Self::CLEAR_ALLOCATION
    | Self::TRIM_DISABLE | Self::CLEARED;
if flags & !ALL_KNOWN != 0 {
    return Err(EINVAL);
}
```

This prevents silent bugs if the flag constants are accidentally misused.

**3. `free_memory_bytes()` directly accesses the C struct field**

```rust
pub fn free_memory_bytes(&self) -> u64 {
    let guard = self.0.lock();
    unsafe { (*guard.as_raw()).avail }
}
```

This directly reads `gpu_buddy.avail` from the C struct. While this is fine today (the field is documented as public and static after init, except during alloc/free which are serialized by the lock), it bypasses the abstraction boundary. If the C struct layout changes, this would silently break. A C helper accessor (like the block helpers) might be more robust. This is a minor style point though.

**4. The `_:` syntax in `try_pin_init!` references fields by name**

```rust
try_pin_init!(AllocatedBlocks {
    buddy: buddy_arc,
    list <- CListHead::new(),
    flags: flags,
    _: {
        let guard = buddy.lock();
        to_result(unsafe {
            bindings::gpu_buddy_alloc_blocks(
                guard.as_raw(),
                start, end, size, min_block_size,
                list.as_raw(),
                flags.as_raw(),
            )
        })?
    }
})
```

This references `buddy`, `list`, and `flags` by their field names inside the `_:` block. This works because `try_pin_init!` makes previously-initialized fields accessible. This is a valid kernel Rust pattern, but it's worth noting that the order of field initialization matters here — `buddy` and `list` must be initialized before the `_:` block runs. The current order is correct.

One subtlety: if `gpu_buddy_alloc_blocks` fails, the `try_pin_init!` will unwind and the partially-initialized `list` and `buddy` will need to be dropped. Since `list` is a freshly initialized (empty) `CListHead` and `buddy` is an `Arc`, this should be safe — no blocks need freeing on error.

**5. `AllocatedBlocks::is_empty()` semantics**

```rust
pub fn is_empty(&self) -> bool {
    !self.list.is_linked()
}
```

The naming is somewhat confusing — `is_linked()` on a list head typically returns `true` when it has entries (is linked into some list / has elements). So `!is_linked()` means "empty." The logic appears correct, but it depends on the `CListHead` API contract. A comment clarifying this would help.

**6. `GpuBuddyInner::drop` acquires the lock defensively**

```rust
fn drop(self: Pin<&mut Self>) {
    let guard = self.lock();
    unsafe {
        bindings::gpu_buddy_fini(guard.as_raw());
    }
}
```

Since `GpuBuddyInner` is behind `Arc`, `drop` only runs when the last reference is released. At that point, no other thread can hold the lock (they'd need an `Arc` reference). The lock acquisition is therefore uncontested and defensive. This is fine — it's belt-and-suspenders safety — but a comment noting that the lock is uncontested at drop time would be helpful.

**7. `Block` `Send`/`Sync` safety comments are weak**

```rust
// SAFETY: `Block` is not modified after allocation for the lifetime
// of `AllocatedBlock`.
unsafe impl Send for Block {}
unsafe impl Sync for Block {}
```

The safety argument is that blocks aren't modified, but `Block` wraps `Opaque<gpu_buddy_block>` which contains raw pointers (`left`, `right`, `parent`, `link`). A stronger safety argument would be: "Block is only accessed through shared references (`&Block`) obtained via `AllocatedBlocks::iter()`, and the underlying data is immutable while the block is allocated. The block's link field is owned by the allocator during allocation and not modified until `gpu_buddy_free_list` is called, which requires exclusive access via the mutex."

**8. Doctest is comprehensive and well-structured**

The doctest at the top of the file is extensive and tests: basic allocation, top-down allocation, non-contiguous allocation with fragmentation, and contiguous allocation failure. This is excellent coverage for a doctest.

**9. Minor: `bindings_helper.h` include ordering**

```c
#include <linux/acpi.h>
+#include <linux/gpu_buddy.h>
 #include <drm/drm_device.h>
```

The `linux/gpu_buddy.h` include is placed between `linux/acpi.h` and `drm/drm_device.h`. This is fine alphabetically within the `linux/` group, but it's inserted outside the existing `#include` block structure. Not a real issue, just noting the placement.

**10. Minor: `GpuBuddyAllocParams` end_range_address=0 convention**

```rust
/// End of allocation range in bytes. Use 0 for entire range.
pub end_range_address: u64,
```

The "use 0 for entire range" semantic is inherited from the C API where `end=0` means "use the full size." This is documented, which is good. The doctest demonstrates this with `end_range_address: 0`.

---
Generated by Claude Code Patch Reviewer