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

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

Patch Review

This is the core Rust abstraction patch. The design — `SGTableMap` as a Devres-managed resource, `SGTable` as an owned handle — is well thought out. However:

**Object destruction ordering (potential use-after-free):** This is the main concern. Consider the normal buffer teardown path where a GEM object is freed while the driver is still bound:

1. `free_callback` calls `drm_gem_shmem_release(shmem)` → frees sgt (sets `shmem->sgt = NULL`), calls `drm_gem_object_release()` (destroys the `dma_resv`)
2. `KBox::from_raw(this)` drops the `Object<T>`, which drops `sgt_res`
3. `Devres<SGTableMap>` is dropped, deregistering the devres action
4. If the `SGTableMap` inside the `Revocable` hasn't been revoked, its `Drop` runs:

```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()) };
    }
}
```

At step 4, `dma_resv` has already been destroyed by `drm_gem_object_release()`, so `dma_resv_lock` is UB. And `shmem->sgt` is NULL, so `__drm_gem_shmem_free_sgt_locked` would dereference NULL.

The driver-unbind case (Case 1) is fine: Devres revokes `SGTableMap` before the object is freed. But the normal-teardown case (Case 2) needs to be addressed. Possible fixes: clear/revoke `sgt_res` in `free_callback` before calling `drm_gem_shmem_release`, or add a NULL guard to `__drm_gem_shmem_free_sgt_locked`.

**`dma_resv_lock` return value ignored:** In `create_sg_table`:

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

With `ctx=NULL` this effectively can't fail (it's a plain mutex lock), but the return value is silently discarded. A `// SAFETY` comment should note that with ctx=NULL, dma_resv_lock cannot return EDEADLK and thus cannot fail, or use `to_result()` to be explicit.

**`NonNull<Object<T>>` in SGTableMap is a raw pointer without refcounting:**

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

The soundness of this depends entirely on the Devres lifecycle guaranteeing that `SGTableMap` is revoked before the Object is freed. Per the destruction-ordering issue above, this invariant can be violated.

**C-style comment syntax in safety comment:**

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

There's a trailing `*/` from a C-style comment.

---

---
Generated by Claude Code Patch Reviewer