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

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

Patch Review

This is the most complex patch. Several observations:

**1. `SGTableMap` holds a raw `NonNull` without a refcount:**

```rust
pub struct SGTableMap<T: DriverObject> {
    obj: NonNull<Object<T>>,
}
```

This is initialized from `self.into()` which for `&Object<T>` creates a `NonNull` from the reference. The `SGTableMap` is stored inside a `Devres` which is stored in an `UnsafeCell` on the `Object<T>` itself. So the `SGTableMap` points back to its owning object. This creates a self-referential structure. It should be safe because:
- The `Devres<SGTableMap<T>>` is stored in the `Object<T>` itself via `sgt_res`
- The `Devres` will be dropped before `obj` (field ordering guarantees this as of patch 5)

However, in patch 4, the field ordering is:

```rust
pub struct Object<T: DriverObject> {
    obj: Opaque<bindings::drm_gem_shmem_object>,
    parent_resv_obj: Option<ARef<Object<T>>>,
    sgt_res: UnsafeCell<Option<Devres<SGTableMap<T>>>>,
    inner: T,
}
```

Here `sgt_res` is *after* `obj`, meaning `obj` would be dropped first, which could be problematic. This is fixed in patch 5 which moves `sgt_res` to the top. **This means patches 4 and 5 must not be applied independently** — patch 4 in isolation has incorrect field drop ordering.

**2. Lock/unlock pattern in `get_sg_table` doesn't use RAII guards:**

```rust
unsafe { bindings::dma_resv_lock(self.raw_dma_resv(), ptr::null_mut()) };
// ... lots of code with multiple return paths ...
unsafe { bindings::dma_resv_unlock(self.raw_dma_resv()) };
```

The code between lock and unlock assigns to `ret` and then unlocks after. This is correct because `ret` captures all exit paths, but it's fragile — any early return would deadlock. The TODO at the top of the file acknowledges this limitation.

**3. `get_sg_table` returns a reference to a Devres stored in the UnsafeCell:**

```rust
fn get_sg_table<'a>(
    &'a self,
    dev: &'a device::Device<Bound>,
) -> Result<&'a Devres<SGTableMap<T>>> {
```

The returned reference's lifetime is tied to `&'a self`, which is correct. The `UnsafeCell` access is protected by the dma_resv lock during the initialization phase, and subsequent reads return the already-initialized value. This is sound because the `Option<Devres<...>>` goes from `None` to `Some` exactly once and never goes back.

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

```rust
impl<T: DriverObject> Drop for SGTableMap<T> {
    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 will be called from the devres callback on driver unbind. At that point, taking the dma_resv lock should be fine since the driver is being unbound. However, it could also be dropped during normal Object cleanup when the Object is freed. In `free_callback`, `drm_gem_shmem_release()` is called first (which already handles the sgt under lock), then the `KBox::from_raw` drops the Rust fields including `sgt_res`. If `drm_gem_shmem_release()` already freed the sgt (setting `shmem->sgt = NULL`), then the devres is still `Some(...)` but the `SGTableMap::drop` will call `__drm_gem_shmem_free_sgt_locked` on a NULL sgt — **linking back to the concern in patch 2 about the missing NULL check**.

Actually, looking more carefully: the `Devres` drop path would revoke the `Revocable<T>`, which may or may not call `SGTableMap::drop`. If the devres callback already fired (device unbind happened), the `Revocable` is already revoked, so dropping the `Devres` won't drop the inner `SGTableMap` again. But if device unbind hasn't happened yet, the `Devres::drop` would call `devm_remove_action_nowarn` and then drop the `Arc<Revocable<T>>`. This needs careful analysis of the ordering between `drm_gem_shmem_release` (called in `free_callback`) and the devres/`sgt_res` drop.

This is a subtle interaction that deserves either a safety comment or a restructuring to make the ordering explicit.

---

---
Generated by Claude Code Patch Reviewer