Claude review: rust: drm: gem: Introduce shmem::Object::sg_table()

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

Patch Review

The most complex patch. Several observations:

**Missing C-side function**: `SGTableMap::drop` calls:
```rust
unsafe { bindings::__drm_gem_shmem_free_sgt_locked(obj.as_raw_shmem()) };
```
This function does not exist in the current drm-next tree. It presumably lives in the "first half" of the series already pushed to drm-rust-next. If it's missing from the base, this won't compile. This should be verified against the actual base tree.

**Import restructuring**: The patch changes `device` from `crate::drm::device` to `crate::device`:
```rust
+    device::{
+        self,
+        Bound, //
+    },
     drm::{
+        driver,
+        gem,
+        private::Sealed,
         Device, //
     },
```
This changes the meaning of `device::Device<T::Driver>` in the existing `dev()` method. On the actual drm-rust-next base, this may already be reconciled. Worth verifying.

**Double-checked locking in `sg_table()`**: The pattern is correct:
```rust
if let Some(sgt_res) = self.sgt_res.as_ref() {  // fast path (Acquire)
    break 'out sgt_res;
}
let _guard = self.sgt_lock.lock();               // slow path
if let Some(sgt_res) = self.sgt_res.as_ref() {  // re-check
    break 'out sgt_res;
}
self.sgt_res.populate(...);                       // Release
```
The `SetOnce` uses `Acquire` on read and `Release` on write, which provides the necessary happens-before guarantees for the double-checked locking pattern.

**SGTableMap lifetime safety**: `SGTableMap` holds `NonNull<Object<T>>` without an `ARef`:
```rust
pub struct SGTableMap<T: DriverObject> {
    obj: NonNull<Object<T>>,
}
```
This is safe because `SGTableMap` is stored inside the `Object<T>` itself (via `sgt_res`), so the object outlives the mapping. The `free_callback` explicitly resets `sgt_res` before releasing the GEM object:
```rust
unsafe { &mut (*this).sgt_res }.reset();   // drops SGTableMap first
unsafe { bindings::drm_gem_shmem_release(base) };  // then releases GEM
let _ = unsafe { KBox::from_raw(this) };   // then frees memory
```
This ordering is correct and critical.

**Device identity check**:
```rust
if dev.as_raw() != self.dev().as_ref().as_raw() {
    return Err(EINVAL);
}
```
Comparing raw device pointers to verify the caller passed the correct device is a good defensive check. The test `fail_sg_table_on_wrong_dev` verifies this works.

**`SGTableMap::new` discards the sg_table pointer**:
```rust
fn new(obj: &Object<T>) -> impl Init<Self, Error> {
    from_err_ptr(unsafe { bindings::drm_gem_shmem_get_pages_sgt(obj.as_raw_shmem()) })?;
    Ok(Self { obj: obj.into() })
}
```
The returned `sg_table*` is discarded. This is correct because `drm_gem_shmem_get_pages_sgt` stores the SGT internally in `shmem->sgt`, which is what `SGTableMap::deref` reads later.

**Minor style**: The `#[pin]` attribute appears before the doc comment on `sgt_lock`:
```rust
+    #[pin]
+    /// Lock for protecting initialization of `sgt_res`.
+    sgt_lock: Mutex<()>,
```
Convention is doc comments before attributes, though both orderings are valid Rust.

**Test coverage**: The `fail_sg_table_on_wrong_dev` test is good but limited — as the TODOs note, testing the success path requires mock DMA ops. The comment is honest about this limitation, which is appreciated.

---
Generated by Claude Code Patch Reviewer