Re: [PATCH 1/2] rust: auxiliary: add registration data to auxiliary devices

Alice Ryhl
2026-04-30T08:59:48Z

On Tue, Apr 28, 2026 at 12:09:40AM +0200, Danilo Krummrich wrote:
> Add a registration_data pointer to struct auxiliary_device, allowing the
> registering (parent) driver to attach private data to the device at
> registration time and retrieve it later when called back by the
> auxiliary (child) driver.
>
> By tying the data to the device's registration, Rust drivers can bind
> the lifetime of device resources to it, since the auxiliary bus
> guarantees that the parent driver remains bound while the auxiliary
> device is bound.
>
> On the Rust side, Registration<T> takes ownership of the data via
> ForeignOwnable. A TypeId is stored alongside the data for runtime type
> checking, making Device::registration_data<T>() a safe method.
>
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>

So overall I think this patch makes sense. A few comments below.

> diff --git a/include/linux/auxiliary_bus.h b/include/linux/auxiliary_bus.h
> index bc09b55e3682..4e1ad8ccbcdd 100644
> --- a/include/linux/auxiliary_bus.h
> +++ b/include/linux/auxiliary_bus.h
> @@ -62,6 +62,9 @@
> * @sysfs.irqs: irqs xarray contains irq indices which are used by the device,
> * @sysfs.lock: Synchronize irq sysfs creation,
> * @sysfs.irq_dir_exists: whether "irqs" directory exists,
> + * @registration_data_rust: private data owned by the registering (parent)
> + * driver; valid for as long as the device is
> + * registered with the driver core,
> *
> * An auxiliary_device represents a part of its parent device's functionality.
> * It is given a name that, combined with the registering drivers
> @@ -148,6 +151,7 @@ struct auxiliary_device {
> struct mutex lock; /* Synchronize irq sysfs creation */
> bool irq_dir_exists;
> } sysfs;
> + void *registration_data_rust;

Is this really Rust-specific? Would you not want C drivers with the same
pattern to do the same thing?

> + // SAFETY: `ptr` is non-null and was set via `into_foreign()` in `Registration::new()`;
> + // `RegistrationData` is `#[repr(C)]` with `type_id` at offset 0, so reading a `TypeId`
> + // at the start of the allocation is valid regardless of `T`.
> + let type_id = unsafe { ptr.cast::<TypeId>().read() };
> + if type_id != TypeId::of::<T>() {
> + return Err(EINVAL);
> + }

Right, okay, so if you put C stuff there, we need the layout to be
compatible with Rust type ids.

Still, we could have Rust expose a couple methods to allow C code to use
the same field with a null type id.

But I guess this is all future work.

> + let data = KBox::pin_init::<Error>(
> + try_pin_init!(RegistrationData {
> + type_id: TypeId::of::<T>(),
> + data <- data,
> + }),
> + GFP_KERNEL,
> + )?;
> +
> + let boxed = KBox::new(Opaque::<bindings::auxiliary_device>::zeroed(), GFP_KERNEL)?;

Use __GFP_ZERO here instead?

> + // SAFETY:
> + // - `adev` is guaranteed to be a valid pointer to a `struct auxiliary_device`, which
> + // has been initialized,
> + // - `modname.as_char_ptr()` is a NULL terminated string.
> + let ret = unsafe { bindings::__auxiliary_device_add(adev, modname.as_char_ptr()) };
> + if ret != 0 {
> + // SAFETY: `registration_data` was set above via `into_foreign()`.
> + let _ = unsafe {
> + Pin::<KBox<RegistrationData<T>>>::from_foreign((*adev).registration_data_rust)
> + };

Nit: Please use `drop(unsafe { ... })` to explicitly drop.

Alice