From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: rust: drm: gem: Introduce shmem::SGTable
Date: Thu, 23 Apr 2026 08:05:55 +1000
Message-ID: <review-patch4-20260421235346.672794-5-lyude@redhat.com>
In-Reply-To: <20260421235346.672794-5-lyude@redhat.com>
References: <20260421235346.672794-1-lyude@redhat.com> <20260421235346.672794-5-lyude@redhat.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0

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 t=
he 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 `dm=
a_resv_lock_interruptible()` in `drm_gem_shmem_get_pages_sgt()`. Using a no=
n-interruptible lock means a user process cannot be killed while waiting fo=
r this lock. The TODO at the top of the file mentions future WW mutex suppo=
rt, but it's worth noting that this is a deliberate choice. At minimum, thi=
s should be documented as intentional.

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

```rust
Err(e) =3D> {
    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 sti=
ll 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 p=
rotecting `sgt_res: UnsafeCell<Option<Devres<SGTableMap<T>>>>` with the dma=
_resv lock is reasonable but unusual for Rust. The safety invariants are do=
cumented. However, there's a subtle concern: when `get_sg_table()` returns =
the `&Devres<SGTableMap<T>>` reference, the dma_resv lock has been *release=
d*. The reference remains valid because the `Option` transitions from `None=
` to `Some` but never back (the `Devres` handles revocation internally). Th=
is reasoning should perhaps be made more explicit in a safety comment.

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

```rust
fn drop(&mut self) {
    let obj =3D 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 whethe=
r the `NonNull<Object<T>>` in `SGTableMap` is still valid at that point. Th=
e `sgt_res` field in `Object<T>` is declared *before* `obj`, which the patc=
h comment says "ensures that it is destroyed before `obj` on `Drop`". But `=
SGTableMap` holds a raw `NonNull` pointer back to the `Object<T>` =E2=80=94=
 the `Drop` for `SGTableMap` (triggered via `Devres`) would be called durin=
g driver unbind, not during `Object<T>`'s drop. So this should be fine as l=
ong 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 `A=
Ref` =E2=80=94 it holds a raw `NonNull`. This seems like a potential use-af=
ter-free risk if the `Devres` fires after the GEM object has been freed. Th=
e 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 drive=
r unbind the `Devres` fires while the object is still alive. This is worth =
double-checking.

**5. C-style comment in Rust**: Minor nit =E2=80=94 there's a C-style comme=
nt 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