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: rust: drm: gem: shmem: Add vmap functions
Date: Fri, 05 Jun 2026 06:03:38 +1000
Message-ID: <review-patch2-20260604192740.659240-3-lyude@redhat.com>
In-Reply-To: <20260604192740.659240-3-lyude@redhat.com>
References: <20260604192740.659240-1-lyude@redhat.com> <20260604192740.659240-3-lyude@redhat.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0

Patch Review

**Generally well-structured.** The `make_vmap` =E2=86=92 `vmap`/`owned_vmap=
` pattern with the generic `R: Deref<Target =3D Self> + From<&'a Self>` is =
clean and avoids code duplication.

**Lock/unlock sequencing in `make_vmap` looks correct:**
```rust
let guard =3D DmaResvGuard::new(self);
to_result(unsafe {
    bindings::drm_gem_shmem_vmap_locked(self.as_raw_shmem(), map.as_mut_ptr=
())
})?;
drop(guard);
```
The explicit `drop(guard)` before the iomem check + potential `raw_vunmap` =
is necessary since `raw_vunmap` re-acquires the lock. Good.

**`ENOTSUPP` for iomem:** The XXX comment acknowledges this limitation. `EN=
OTSUPP` vs `EOPNOTSUPP` =E2=80=94 kernel convention typically uses `EOPNOTS=
UPP` for userspace-facing errors and `ENOTSUPP` for internal use. Since thi=
s is an internal Rust API, `ENOTSUPP` is acceptable but worth confirming in=
tent.

**`IoCapable` macro =E2=80=94 raw pointer casts from `usize`:**
```rust
unsafe fn io_read(&self, address: usize) -> $ty {
    let ptr =3D address as *mut $ty;
    unsafe { ptr::read(ptr) }
}
```
The `address` is a raw kernel virtual address cast from the `Io` trait's `a=
ddr()`. The safety contract is documented via `Io` trait bounds. This follo=
ws the existing kernel `Io` pattern so it's fine.

**Test coverage is good.** The `compile_time_vmap_sizes` test validates the=
 SIZE invariant, and `vmap_io` exercises read/write paths including cross-t=
ype verification (write u32 then read individual u8 bytes).

**Byte-order assumption in test:**
```rust
vmap.write32(0xFFFFFFFF, 0x20);
assert_eq!(vmap.read8(0x20), 0xFF);
```
This works for `0xFFFFFFFF` regardless of endianness, but the pattern would=
 be fragile for other values. Since all bytes are `0xFF` here, it's fine.

---
Generated by Claude Code Patch Reviewer