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: auxiliary: add registration data to auxiliary devices
Date: Tue, 28 Apr 2026 13:51:40 +1000
Message-ID: <review-patch1-20260427221002.2143861-2-dakr@kernel.org>
In-Reply-To: <20260427221002.2143861-2-dakr@kernel.org>
References: <20260427221002.2143861-1-dakr@kernel.org> <20260427221002.2143861-2-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

**C header change** (`include/linux/auxiliary_bus.h`):

The `void *registration_data_rust` field added to `struct auxiliary_device`=
 is well-placed. The `_rust` suffix is an appropriate convention making cle=
ar that C code should not touch this field. However, this grows every `stru=
ct auxiliary_device` by a pointer even when no Rust driver is involved:

```c
void *registration_data_rust;
```

Minor nit: the kernel community may push back on adding a Rust-specific fie=
ld to a core C structure that affects all users. It might be worth noting i=
n the commit message or cover letter that this is a single pointer and the =
auxiliary_device is already not a high-frequency allocation.

**`RegistrationData<T>` wrapper** (`rust/kernel/auxiliary.rs`):

The `#[repr(C)]` layout with `type_id` at offset 0 enables the safe type-ch=
ecking pattern in `registration_data()`:

```rust
#[repr(C)]
#[pin_data]
struct RegistrationData<T> {
    type_id: TypeId,
    #[pin]
    data: T,
}
```

This is sound: `#[repr(C)]` guarantees `type_id` is at offset 0 regardless =
of `T`, so the `ptr.cast::<TypeId>().read()` in `registration_data()` is va=
lid even before the actual type `T` is confirmed. The alignment is also gua=
ranteed since `RegistrationData<T>`'s alignment is at least `align_of::<Typ=
eId>()`.

**`Registration::new` API change**:

The shift from returning `impl PinInit<Devres<Self>>` (used with `<-` in `p=
in_init!`) to returning `Result<Devres<Self>>` (used with `=3D` and `?`) si=
mplifies the call sites. This works because `Devres::new` already accepts `=
impl PinInit<T, E>`, and a value of type `Self` satisfies `PinInit<Self, In=
fallible>`.

**Error path in `Registration::new`**:

```rust
if ret !=3D 0 {
    let _ =3D unsafe {
        Pin::<KBox<RegistrationData<T>>>::from_foreign((*adev).registration=
_data_rust)
    };
    unsafe { bindings::auxiliary_device_uninit(adev) };
    return Err(Error::from_errno(ret));
}
```

Correct: registration data is reclaimed before `auxiliary_device_uninit`, w=
hich will eventually trigger `Device::release` to free the device memory. T=
he data cleanup doesn't depend on device validity, just on the pointer stor=
ed in the field.

**`Registration::drop` ordering**:

```rust
unsafe { bindings::auxiliary_device_delete(self.adev.as_ptr()) };
// then free registration data
// then auxiliary_device_uninit
```

This is the critical ordering and it's correct. `auxiliary_device_delete()`=
 is synchronous =E2=80=94 it unbinds the child driver and waits for `remove=
_callback` to complete. Only after that is the registration data freed. Sin=
ce `registration_data()` is only available on `Device<Bound>`, and the chil=
d is no longer bound after `auxiliary_device_delete()`, there's no race.

**Invariant comment nit**: The invariant says "`registration_data` points t=
o a valid `Pin<KBox<RegistrationData<T>>>`", but there is no field named `r=
egistration_data` in the struct =E2=80=94 it refers to the `registration_da=
ta_rust` field of the pointed-to C struct. Consider: "the `registration_dat=
a_rust` field of the `auxiliary_device` pointed to by `self.adev`..."

**`Sync` bound**:

```rust
unsafe impl<T: Send> Sync for Registration<T> {}
```

This requires `T: Send` (not `T: Sync`) for `Registration<T>: Sync`. This i=
s appropriate because `Registration` doesn't hand out `&T` through `&Regist=
ration<T>` =E2=80=94 the data is accessed via a separate path through the d=
evice.

**Nova-core update** (`drivers/gpu/nova-core/driver.rs`):

The change from `#[pin]` + `<-` init to non-pinned `=3D` init with `?` is c=
lean. The `()` registration data is the right choice when no inter-driver c=
ommunication is needed. If `_reg1` creation fails, Rust's drop semantics wi=
ll correctly clean up the already-constructed `_reg0`.

**Sample driver update** (`samples/rust/rust_driver_auxiliary.rs`):

Good demonstration of the new API. The `Data { index: u32 }` struct makes t=
he sample more meaningful than the old `TypeId::of::<Self>()` approach. The=
 `#[pin_data]` attribute is correctly removed from `ParentDriver` since no =
fields need pin-initialization anymore.

---
Generated by Claude Code Patch Reviewer