Claude review: rust: drm: gem: Introduce shmem::SGTable

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

Patch Review

This is the most complex and consequential patch. Several observations:

**1. Non-interruptible dma_resv_lock**: In `get_sg_table()` (line ~821 of the mbox):

```rust
unsafe { bindings::dma_resv_lock(self.raw_dma_resv(), ptr::null_mut()) };
```

This uses `dma_resv_lock()` which is non-interruptible. The C side uses `dma_resv_lock_interruptible()` in `drm_gem_shmem_get_pages_sgt()`. Using a non-interruptible lock means a user process cannot be killed while waiting for this lock. The TODO at the top of the file mentions future WW mutex support, but it's worth noting that this is a deliberate choice. At minimum, this should be documented as intentional.

**2. Lock not released on Devres::new failure before __drm_gem_shmem_free_sgt_locked**: Looking at the error path in `get_sg_table()`:

```rust
Err(e) => {
    unsafe { bindings::__drm_gem_shmem_free_sgt_locked(self.as_raw_shmem()) };
    Err(e)
}
```

This calls `__drm_gem_shmem_free_sgt_locked` while the dma_resv lock is still held (good, as the function requires it), and then the lock is released at the end of the function. This is correct.

**3. UnsafeCell + dma_resv lock for interior mutability**: The pattern of protecting `sgt_res: UnsafeCell<Option<Devres<SGTableMap<T>>>>` with the dma_resv lock is reasonable but unusual for Rust. The safety invariants are documented. However, there's a subtle concern: when `get_sg_table()` returns the `&Devres<SGTableMap<T>>` reference, the dma_resv lock has been *released*. The reference remains valid because the `Option` transitions from `None` to `Some` but never back (the `Devres` handles revocation internally). This reasoning should perhaps be made more explicit in a safety comment.

**4. SGTableMap::Drop acquires dma_resv_lock**: The `Drop` impl for `SGTableMap` acquires the dma_resv lock:

```rust
fn drop(&mut self) {
    let obj = unsafe { self.obj.as_ref() };
    unsafe { bindings::dma_resv_lock(obj.raw_dma_resv(), ptr::null_mut()) };
    unsafe { bindings::__drm_gem_shmem_free_sgt_locked(obj.as_raw_shmem()) };
    unsafe { bindings::dma_resv_unlock(obj.raw_dma_resv()) };
}
```

This is called from `Devres` on driver unbind. There's a question of whether the `NonNull<Object<T>>` in `SGTableMap` is still valid at that point. The `sgt_res` field in `Object<T>` is declared *before* `obj`, which the patch comment says "ensures that it is destroyed before `obj` on `Drop`". But `SGTableMap` holds a raw `NonNull` pointer back to the `Object<T>` — the `Drop` for `SGTableMap` (triggered via `Devres`) would be called during driver unbind, not during `Object<T>`'s drop. So this should be fine as long as the GEM object outlives the devres teardown, which it should via the `ARef` stored in `SGTable`. But the `SGTableMap` itself doesn't hold an `ARef` — it holds a raw `NonNull`. This seems like a potential use-after-free risk if the `Devres` fires after the GEM object has been freed. The safety argument needs to be that the GEM object's refcount keeps it alive as long as anyone holds an `SGTable` (which holds an `ARef`), and on driver unbind the `Devres` fires while the object is still alive. This is worth double-checking.

**5. C-style comment in Rust**: Minor nit — there's a C-style comment marker `*/` at the end of a Rust safety comment:

```rust
// SAFETY: We grabbed the lock required for calling this function above */
```

---
Generated by Claude Code Patch Reviewer