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: Don't setup private driver data until registration
Date: Sun, 22 Mar 2026 03:17:22 +1000
Message-ID: <review-patch3-20260320233645.950190-4-lyude@redhat.com>
In-Reply-To: <20260320233645.950190-4-lyude@redhat.com>
References: <20260320233645.950190-1-lyude@redhat.com> <20260320233645.950190-4-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

**Critical concern =E2=80=94 error path for data initialization failure:**
```rust
fn new(
    drm: drm::UnregisteredDevice<T>,
    data: impl PinInit<T::Data, Error>,
    flags: usize,
) -> Result<Self> {
    unsafe { data.__pinned_init(drm.0.data.get().cast()) }?;
    drm.data_is_init.store(true, Ordering::Relaxed);
    to_result(unsafe { bindings::drm_dev_register(drm.as_raw(), flags) })?;
```

If `data.__pinned_init()` succeeds but `drm_dev_register()` fails, the func=
tion returns `Err`, which drops `drm` (the `UnregisteredDevice`). This will=
 eventually call `release()`, which checks `data_is_init` and will drop the=
 initialized data =E2=80=94 so this is correct.

However, if `data.__pinned_init()` itself fails, the `?` propagates the err=
or and `drm` is dropped. At this point `data_is_init` is still `false`, so =
the data won't be double-dropped. But we need to verify that the partially-=
initialized data from a failed `__pinned_init` is properly handled. By the =
`PinInit` contract, if `__pinned_init` returns an error, the memory is left=
 in a valid-to-deallocate state (i.e., not needing drop). So this is fine.

**`pub(super)` on `UnregisteredDevice` tuple field:**
```rust
pub struct UnregisteredDevice<T: drm::Driver>(pub(super) ARef<Device<T, Uni=
nit>>, NotThreadSafe);
```
Making the inner `ARef` `pub(super)` is needed for the `drm.0.data.get().ca=
st()` access in `driver.rs`. This is acceptable since both files are in the=
 `drm` module, but it does weaken the encapsulation of `UnregisteredDevice`.

---
Generated by Claude Code Patch Reviewer