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: drm: nova: convert to use DRM registration data
Date: Thu, 04 Jun 2026 12:03:18 +1000
Message-ID: <review-patch7-20260603011711.2077361-8-dakr@kernel.org>
In-Reply-To: <20260603011711.2077361-8-dakr@kernel.org>
References: <20260603011711.2077361-1-dakr@kernel.org> <20260603011711.2077361-8-dakr@kernel.org>
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 demonstration patch. Nova moves the auxiliary device reference =
from `drm::Device` data into `RegistrationData`:

```rust
-pub(crate) struct NovaData {
-    pub(crate) adev: ARef<auxiliary::Device>,
+pub(crate) struct NovaData<'bound> {
+    pub(crate) adev: &'bound auxiliary::Device<Bound>,
 }
```

And the driver trait:
```rust
-    type Data =3D NovaData;
-    type RegistrationData =3D ForLt!(());
+    type Data =3D ();
+    type RegistrationData =3D CovariantForLt!(NovaData<'_>);
```

**Key change**: `Data` becomes `()` (no per-device data accessible without =
UnbindGuard) and `RegistrationData` becomes `CovariantForLt!(NovaData<'_>)`=
 containing the borrowed `&'bound auxiliary::Device<Bound>`.

**`new_with_lt()` usage**:
```rust
+        let drm =3D drm::UnregisteredDevice::<Self>::new(adev, Ok(()))?;
+        let reg_data =3D NovaData { adev };
+        // SAFETY: We never bypass the destructor of `reg`.
+        let reg =3D unsafe { drm::Registration::new_with_lt(adev.as_ref(),=
 drm, reg_data, 0)? };
```

The `new_with_lt()` is unsafe because the caller must not `mem::forget()` t=
he Registration (which would cause use-after-free of the borrowed `adev` re=
ference after the bus driver unbinds). The safety comment "We never bypass =
the destructor of `reg`" is correct =E2=80=94 `reg` is stored in `Nova<'bou=
nd>` which is the bus driver's `Data<'bound>`, and the bus framework guaran=
tees drop is called.

Note that `drm::UnregisteredDevice::new(adev, Ok(()))` passes `Ok(())` as t=
he data init =E2=80=94 this works because `PinInit<(), Error>` is implement=
ed for `Result<()>`. Slightly unusual but correct.

**In `get_param()`**:
```rust
-        let adev =3D &dev.adev;
-        let parent =3D adev.parent();
-        let pdev: &pci::Device =3D parent.try_into()?;
+        let parent =3D reg_data.adev.parent();
+        let pdev: &pci::Device<Bound> =3D parent.try_into()?;
```

Now the PCI device reference is `&pci::Device<Bound>` instead of `&pci::Dev=
ice` (unbound). This is a type-level improvement =E2=80=94 the bound contex=
t guarantees the PCI device is still bound, which is correct within the `dr=
m_dev_enter()` critical section.

Overall: this is a well-structured, carefully thought-out series that addre=
sses a genuine soundness concern. The SRCU-based safety argument is correct=
 (verified against the C `drm_dev_unplug()` implementation), and the HRTB c=
losure pattern for lifetime-erased data access is the standard Rust approac=
h for this kind of problem. The incremental patch structure makes it easy t=
o review each step.

---
Generated by Claude Code Patch Reviewer