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

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

Patch Review

**Most complex patch — warrants careful scrutiny.**

**Drop ordering in `free_callback` is correct:**
```rust
unsafe { ManuallyDrop::drop(&mut (*this).sgt_res) };
unsafe { bindings::drm_gem_shmem_release(base) };
let _ = unsafe { KBox::from_raw(this) };
```
`sgt_res` must drop first because its `Drop` calls `__drm_gem_shmem_free_sgt_locked` which needs the GEM object alive. Then `drm_gem_shmem_release` cleans up the C-side shmem state. Then `KBox::from_raw` drops the remaining Rust fields. The `ManuallyDrop` pattern is the right choice here since `SetOnce::reset()` was removed in v18.

**Double-checked locking in `sg_table()` is correct:**
```rust
if let Some(sgt_res) = self.sgt_res.as_ref() {
    break 'out sgt_res;
}
let _guard = self.sgt_lock.lock();
if let Some(sgt_res) = self.sgt_res.as_ref() {
    break 'out sgt_res;
}
self.sgt_res.populate(Devres::new(dev, SGTableMap::new(self))?);
```
This requires that `SetOnce::as_ref()` is safe to call without holding `sgt_lock` (i.e., it provides atomic-like visibility). If `SetOnce` is implemented with proper memory ordering (which kernel `SetOnce` should be), this is correct. If not, there's a data race on the fast path.

**`SGTableMap::new` uses `from_err_ptr` with a `?` inside an `Init` returning closure:**
```rust
fn new(obj: &Object<T, C>) -> impl Init<Self, Error> {
    from_err_ptr(unsafe { bindings::drm_gem_shmem_get_pages_sgt(obj.as_raw_shmem()) })?;
    Ok(Self { obj: obj.into() })
}
```
This calls `drm_gem_shmem_get_pages_sgt` which acquires/releases `dma_resv` internally (it uses `dma_resv_lock_interruptible`). The return value (an `sg_table*`) is checked for errors via `from_err_ptr` but the actual pointer is then discarded — the implementation relies on the fact that `shmem->sgt` is set as a side effect. This is correct per the C implementation, but the discarded return value deserves a comment explaining this pattern.

**Device identity check:**
```rust
if dev.as_raw() != self.dev().as_ref().as_raw() {
    return Err(EINVAL);
}
```
Good — prevents creating a `Devres` bound to the wrong device, which would cause the revocation to fire at the wrong time or not at all.

**`SGTableMap` raw pointer storage:**
```rust
pub struct SGTableMap<T: DriverObject, C: DeviceContext> {
    obj: NonNull<Object<T, C>>,
}
```
This stores a non-refcounted pointer to the owning `Object`. The invariant is that `SGTableMap` lives inside the `Object`'s `sgt_res` field, so the object outlives it. The `ManuallyDrop` ensures `sgt_res` is dropped before the object in `free_callback`. This is sound but not self-documenting — a `// INVARIANT:` comment on the `obj` field explaining the lifetime relationship would help future maintainers.

**Test only covers the failure path:**
```rust
fn fail_sg_table_on_wrong_dev() -> Result {
```
The TODO explains that testing the success path would require dummy DMA ops. The failure test is useful — it confirms the device check prevents `sgt_res` from being populated with a wrong device. Good defensive test.

**`Send`/`Sync` for `SGTableMap`:** The safety justification references GEM object thread-safety, which is established by the existing `Send`/`Sync` impls on `Object`. This is correct.

---
Generated by Claude Code Patch Reviewer