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::Object::sg_table()
Date: Thu, 04 Jun 2026 16:06:44 +1000
Message-ID: <review-patch6-20260529183702.677677-7-lyude@redhat.com>
In-Reply-To: <20260529183702.677677-7-lyude@redhat.com>
References: <20260529183702.677677-1-lyude@redhat.com> <20260529183702.677677-7-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

**Status: Looks good**

This patch adds the test for the wrong-device rejection path that was intro=
duced in patch 3. The test is well-structured:

```rust
fn fail_sg_table_on_wrong_dev() -> Result {
    let (_dev, drm) =3D create_drm_dev()?;
    let wrong_dev =3D faux::Registration::new(c"EvilKunit", None)?;

    let obj =3D Object::<KunitObject>::new(&drm, PAGE_SIZE, ObjectConfig::d=
efault(), ())?;

    assert_eq!(obj.sg_table(wrong_dev.as_ref()).err().unwrap(), EINVAL);

    // If sgt_res was not initialized mistakenly with the wrong device, thi=
s should still fail.
    assert_eq!(obj.sg_table(wrong_dev.as_ref()).err().unwrap(), EINVAL);
```

The double-call test is a good edge case check =E2=80=94 it verifies that `=
sgt_res` (the `LazyInit`) isn't accidentally initialized with the wrong dev=
ice on the first call.

The TODO comments about needing dummy dma_ops for success-path testing are =
reasonable.

---

**Summary**: The series is well-designed and mostly correct. The two action=
able items are: (1) fix the kdoc function name in patch 5 to say `__drm_gem=
_shmem_free_sgt_locked`, and (2) consider adding a NULL guard or `drm_WARN_=
ON` for `shmem->sgt` in `__drm_gem_shmem_free_sgt_locked()` since it's an e=
xported symbol. The kdoc claim about "unpin" is also inaccurate and should =
say "DMA unmap and release" instead.

---
Generated by Claude Code Patch Reviewer