Re: [PATCH v6 1/1] rust: introduce abstractions for fwctl

[Danilo Krummrich]

On Mon Jun 29, 2026 at 5:01 PM CEST, Zhi Wang wrote:
> Introduce safe Rust wrappers around struct fwctl_device and
> struct fwctl_uctx. This lets Rust drivers register fwctl devices and
> implement firmware RPC callbacks through a typed trait interface.
>
> The abstraction keeps lifetime and reference-count handling inside the
> wrapper, exposes pinned per-FD user contexts to drivers, and validates the
> layout assumptions required by the C fwctl allocation model.
>
> Registration owns driver private data with a lifetime tied to the bound
> parent device. fwctl callbacks access that data through the Registration
> lifetime, while Device remains only the refcounted fwctl object. This
> avoids requiring Rust drop glue from the fwctl_device release path after
> unregister or module teardown.
>
> Co-developed-by: Danilo Krummrich <dakr@xxxxxxxxxx>

This also needs:

Signed-off-by: Danilo Krummrich <dakr@xxxxxxxxxx>

> Link: https://lore.kernel.org/r/DJJW7X4ESDSM.QCVYK2FC7ZR3@xxxxxxxxxx
> Signed-off-by: Zhi Wang <zhiw@xxxxxxxxxx>

Few remaining nits below.

Jason, would you be fine if I take this through the DRM tree, so Nova can build
on top of it right away, or do you want to take it through your fwctl tree?

> ---
>  drivers/fwctl/Kconfig           |  12 +
>  rust/bindings/bindings_helper.h |   1 +
>  rust/helpers/fwctl.c            |  17 +
>  rust/helpers/helpers.c          |   3 +-
>  rust/kernel/fwctl.rs            | 546 ++++++++++++++++++++++++++++++++
>  rust/kernel/lib.rs              |   2 +
>  6 files changed, 580 insertions(+), 1 deletion(-)
>  create mode 100644 rust/helpers/fwctl.c
>  create mode 100644 rust/kernel/fwctl.rs


The MAINTAINERS file needs an update.

> +/// Represents a fwctl device type.
> +///
> +/// Corresponds to the C `enum fwctl_device_type`.
> +#[repr(u32)]
> +#[derive(Copy, Clone, Debug, Eq, PartialEq)]
> +pub enum DeviceType {
> +    /// Mellanox ConnectX (mlx5) device.
> +    Mlx5 = bindings::fwctl_device_type_FWCTL_DEVICE_TYPE_MLX5,
> +    /// CXL (Compute Express Link) device.
> +    Cxl = bindings::fwctl_device_type_FWCTL_DEVICE_TYPE_CXL,
> +    /// AMD/Pensando PDS device.
> +    Pds = bindings::fwctl_device_type_FWCTL_DEVICE_TYPE_PDS,
> +    /// Broadcom NetXtreme (bnxt) device.
> +    Bnxt = bindings::fwctl_device_type_FWCTL_DEVICE_TYPE_BNXT,
> +}

NIT: Do we want to add all device types even though they do not (plan to) have a
Rust driver?

> +/// Trait implemented by each Rust driver that integrates with the fwctl subsystem.
> +///
> +/// The implementing type **is** the per-FD user context: one instance is
> +/// created for each `open()` call and dropped when the FD is closed.
> +///
> +/// Each implementation corresponds to a specific device type and provides the
> +/// vtable used by the core `fwctl` layer to manage per-FD user contexts and
> +/// handle RPC requests.
> +pub trait Operations: Sized + Send + Sync + 'static {
> +    /// Data owned by the [`Registration`] and accessible during callbacks.
> +    ///
> +    /// The lifetime `'a` is tied to the [`Registration`] scope (which lives within the parent bus
> +    /// device binding scope). Drivers use it to store references to resources bound to this scope,
> +    /// such as PCI BARs or typed bus device references.
> +    type RegistrationData<'a>: Send + Sync + 'a
> +    where
> +        Self: 'a;
> +
> +    /// fwctl device type identifier.
> +    const DEVICE_TYPE: DeviceType;
> +
> +    /// Called when a new user context is opened.
> +    ///
> +    /// Returns a [`PinInit`] initializer for `Self`. The instance is dropped
> +    /// automatically when the FD is closed (after [`close`](Self::close)).
> +    fn open<'a>(
> +        device: &'a Device<Self>,
> +        reg_data: &'a Self::RegistrationData<'a>,
> +    ) -> impl PinInit<Self, Error>;

I think this can just be

	fn open<'a>(
	    device: &Device<Self>,
	    reg_data: &Self::RegistrationData<'a>,
	) -> impl PinInit<Self, Error>;

> +impl<T: Operations> Device<T> {
> +    /// Allocate a new fwctl device.
> +    ///
> +    /// Returns an [`ARef`] that can be passed to [`Registration::new()`]
> +    /// to make the device visible to userspace.
> +    pub fn new(parent: &device::Device<device::Bound>) -> Result<ARef<Self>> {
> +        const_assert!(
> +            core::mem::offset_of!(Self, dev) == 0,
> +            "struct fwctl_device must be at offset 0"
> +        );
> +
> +        let ops = core::ptr::from_ref::<bindings::fwctl_ops>(&VTable::<T>::VTABLE).cast_mut();
> +
> +        // SAFETY: `ops` is static, `parent` is bound, and `size` covers the full `Device<T>`.
> +        let raw = unsafe {
> +            bindings::_fwctl_alloc_device(parent.as_raw(), ops, core::mem::size_of::<Self>())
> +        };
> +
> +        if raw.is_null() {
> +            return Err(ENOMEM);
> +        }
> +
> +        let this = raw.cast::<Self>();
> +
> +        // INVARIANT: Set `registration_data` to dangling (no registration yet).
> +        // SAFETY: `this` points to the allocation just returned by fwctl.
> +        unsafe {
> +            core::ptr::addr_of_mut!((*this).registration_data)

Here and in a few other places, this can use &raw mut instead.

> +/// Static vtable mapping Rust trait methods to C callbacks.
> +pub struct VTable<T: Operations>(PhantomData<T>);

Why does this need public visibility?

> +
> +impl<T: Operations> VTable<T> {
> +    /// The fwctl operations vtable for this driver type.
> +    pub const VTABLE: bindings::fwctl_ops = bindings::fwctl_ops {
> +        device_type: T::DEVICE_TYPE as u32,
> +        uctx_size: core::mem::size_of::<UserCtx<T>>(),
> +        open_uctx: Some(Self::open_uctx_callback),
> +        close_uctx: Some(Self::close_uctx_callback),
> +        info: Some(Self::info_callback),
> +        fw_rpc: Some(Self::fw_rpc_callback),
> +    };

Same here.

> +
> +    /// # Safety
> +    ///
> +    /// `uctx` must be a valid `fwctl_uctx` embedded in a `UserCtx<T>` with
> +    /// sufficient allocated space for the uctx field.
> +    unsafe extern "C" fn open_uctx_callback(uctx: *mut bindings::fwctl_uctx) -> ffi::c_int {
> +        const_assert!(
> +            core::mem::offset_of!(UserCtx<T>, fwctl_uctx) == 0,
> +            "struct fwctl_uctx must be at offset 0"
> +        );
> +
> +        // SAFETY: fwctl sets this pointer before calling `open_uctx`.
> +        let raw_fwctl = unsafe { (*uctx).fwctl };
> +        // SAFETY: Rust fwctl devices use the offset-0 `Device<T>` layout.
> +        let device = unsafe { Device::<T>::from_raw(raw_fwctl) };
> +
> +        // SAFETY: open_uctx is called under registration_lock read; the device is registered.
> +        let reg_data = unsafe { device.registration_data_unchecked() };
> +
> +        let initializer = T::open(device, reg_data);
> +
> +        let uctx_offset = core::mem::offset_of!(UserCtx<T>, uctx);
> +        // SAFETY: `uctx_size` reserves space for the full `UserCtx<T>`.
> +        let uctx_ptr: *mut T = unsafe { uctx.cast::<u8>().add(uctx_offset).cast() };

This can use byte_add() instead.

> +            Ok(FwRpcResponse::NewBuffer(kvec)) => {
> +                let (ptr, len, _cap) = kvec.into_raw_parts();
> +                // SAFETY: `out_len` is valid.
> +                unsafe { *out_len = len };
> +                ptr.cast::<ffi::c_void>()

This probably needs a kvec.is_empty() check, since an empty vector contains a
dangling pointer.