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/tyr: move probe resources into registration data
Date: Thu, 04 Jun 2026 11:21:43 +1000
Message-ID: <review-patch1-20260603-use_tyr_reg_data-v1-1-97f64e951cf6@collabora.com>
In-Reply-To: <20260603-use_tyr_reg_data-v1-1-97f64e951cf6@collabora.com>
References: <20260603-use_tyr_reg_data-v1-1-97f64e951cf6@collabora.com> <20260603-use_tyr_reg_data-v1-1-97f64e951cf6@collabora.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

**driver.rs =E2=80=94 TyrDrmDeviceData emptied and TyrDrmRegistrationData i=
ntroduced**

The core restructuring is clean. `TyrDrmDeviceData` becomes a unit struct a=
nd all resources move to the new `TyrDrmRegistrationData<'bound>`:

```rust
#[pin_data]
pub(crate) struct TyrDrmDeviceData;

#[pin_data]
pub(crate) struct TyrDrmRegistrationData<'bound> {
    pub(crate) pdev: ARef<platform::Device>,
    pub(crate) fw: Arc<Firmware<'bound>>,
    #[pin]
    clks: Mutex<Clocks>,
    #[pin]
    regulators: Mutex<Regulators>,
    pub(crate) iomem: Arc<IoMem<'bound>>,
    pub(crate) gpu_info: GpuInfo,
}
```

The `ForLt` implementation is the standard pattern for the Rust DRM lifetim=
e framework:

```rust
impl ForLt for TyrDrmRegistrationData<'static> {
    type Of<'bound> =3D TyrDrmRegistrationData<'bound>;
}
```

**driver.rs =E2=80=94 probe() restructuring**

The probe sequence now creates the DRM device first (unregistered), then cr=
eates auxiliary objects, then builds registration data and registers:

```rust
let dev_data =3D try_pin_init!(TyrDrmDeviceData {});
let unreg_dev =3D drm::UnregisteredDevice::<TyrDrmDriver>::new(pdev, dev_da=
ta)?;

let mmu =3D Mmu::new(iomem.as_arc_borrow(), &gpu_info)?;
let firmware =3D Firmware::new(pdev, iomem.clone(), &unreg_dev, mmu.as_arc_=
borrow(), &gpu_info)?;
```

**Concern: `Mmu` created but never stored.** The `Mmu` is constructed (whic=
h logs the address space slot count) and passed as `ArcBorrow` to `Firmware=
::new()`, but `Firmware` ignores it (`_mmu` parameter). The `Arc<Mmu>` is t=
hen dropped when `mmu` goes out of scope. This means the MMU object exists =
only transiently for its logging side effect. Consider either:
- Not creating it here at all (defer to when it's actually needed), or
- Storing it in `TyrDrmRegistrationData` if it needs to outlive probe

The current approach creates an object just to drop it, which is confusing =
even for a stub.

**driver.rs =E2=80=94 Safety comment on `new_with_lt`**

```rust
// SAFETY: `reg` is stored in the platform driver data and is not leaked or
// forgotten, so it is dropped before the `'bound` registration data can be=
come
// invalid.
```

The safety argument is correct in substance =E2=80=94 the `Registration` is=
 stored in `TyrPlatformDriverData` which is the platform driver data, dropp=
ed when the device unbinds. However, it would be slightly more precise to s=
tate that the `Registration` is dropped before `'bound` ends because the pl=
atform framework guarantees `TyrPlatformDriverData` is dropped during unbin=
d, which happens while the device is still bound.

**driver.rs =E2=80=94 IoMem wrapping change**

```rust
-        let iomem =3D request.iomap_sized::<SZ_2M>()?;
+        let iomem =3D Arc::new(request.iomap_sized::<SZ_2M>()?, GFP_KERNEL=
)?;
```

This changes from a direct value (or `Arc::pin_init` in the current tree) t=
o `Arc::new`. Since `IoMem` is now shared between `TyrDrmRegistrationData` =
and `Firmware`, wrapping in `Arc` is correct. Verify that `IoMem` doesn't r=
equire pinning =E2=80=94 if the base series' `IoMem<'bound>` is `Unpin`, `A=
rc::new` is fine; if it needs pinning, this should be `Arc::pin_init`.

**driver.rs =E2=80=94 Cosmetic blank line removal**

```rust
 pub(crate) type TyrDrmDevice<Ctx =3D drm::Registered> =3D drm::Device<TyrD=
rmDriver, Ctx>;
-
 pub(crate) struct TyrPlatformDriver;
```

Nit: the blank line between unrelated declarations is conventional and aids=
 readability. Consider keeping it.

**file.rs =E2=80=94 dev_query uses registration data**

The changes here are a straightforward consequence of moving `gpu_info` to =
registration data:

```rust
-        ddev: &TyrDrmDevice,
+        _ddev: &TyrDrmDevice,
         _pdev: &platform::Device<device::Bound>,
-        _reg_data: &(),
+        reg_data: &TyrDrmRegistrationData<'_>,
```

and:

```rust
-                    devquery.size =3D core::mem::size_of_val(&ddev.gpu_inf=
o) as u32;
+                    devquery.size =3D core::mem::size_of_val(&reg_data.gpu=
_info) as u32;
```

This is correct. The `gpu_info` accessor properly follows the data to its n=
ew home.

**fw.rs =E2=80=94 Firmware stub**

The stub is clean and clearly documented. A few observations:

```rust
pub(crate) struct Firmware<'bound> {
    _pdev: ARef<platform::Device>,
    _iomem: Arc<IoMem<'bound>>,
}
```

**Minor: redundant `_pdev` reference.** The `pdev` is already held by `TyrD=
rmRegistrationData`, and `Firmware` is owned by the registration data via `=
Arc<Firmware<'bound>>`. The firmware doesn't need its own `ARef<platform::D=
evice>` to keep the device alive =E2=80=94 the registration data already do=
es that. Unless future firmware code needs to call device APIs directly (li=
kely), in which case keeping it is forward-looking and fine.

```rust
pub(crate) fn boot(&self) -> Result {
    todo!()
}
```

This will panic at runtime if called. Fine for a stub, but ensure no code p=
ath can reach it before the implementation is filled in.

**mmu.rs =E2=80=94 MMU stub**

```rust
pub(super) struct Mmu<'bound> {
    _bound: PhantomData<&'bound ()>,
}
```

The struct carries the `'bound` lifetime via `PhantomData` even though it c=
urrently holds no `'bound`-lifetime resources. This is correct forward-look=
ing design for when MMU will hold `IoMem<'bound>` or similar.

```rust
let present =3D AS_PRESENT::from_raw(gpu_info.as_present).present().get();
let slot_count: usize =3D present.count_ones().try_into()?;
pr_info!("MMU: {} address space slots present", slot_count);
```

**Nit: `pr_info!` vs `dev_info!`.** Kernel driver messages should use `dev_=
info!` to include the device name for multi-device systems. The `Mmu::new()=
` would need to accept a device reference to do this. Consider adding `pdev=
: &platform::Device` or `dev: &Device` as a parameter.

**Nit: `try_into()` is infallible here.** `u32::count_ones()` returns `u32`=
, and `u32` =E2=86=92 `usize` never fails on any Linux-supported architectu=
re (usize =E2=89=A5 32 bits). Using `as usize` would be simpler and avoids =
an unnecessary error path, though `try_into()` is the safer Rust idiom.

**tyr.rs =E2=80=94 module declarations**

```rust
+mod fw;
 mod gem;
 mod gpu;
+mod mmu;
```

Straightforward module additions, properly sorted alphabetically.

**Overall: the patch is well-structured and correct for its stated purpose =
of establishing lifetime scaffolding.** The main actionable feedback is the=
 `Mmu` create-and-drop pattern and the `pr_info!` vs `dev_info!` issue.

---
Generated by Claude Code Patch Reviewer