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

Jason Gunthorpe
2026-01-26T18:19:20Z

On Thu, Jan 22, 2026 at 10:42:30PM +0200, Zhi Wang wrote:
> --- a/drivers/fwctl/Kconfig
> +++ b/drivers/fwctl/Kconfig
> @@ -8,6 +8,18 @@ menuconfig FWCTL
> manipulating device FLASH, debugging, and other activities that don't
> fit neatly into an existing subsystem.
>
> +config RUST_FWCTL_ABSTRACTIONS
> + bool "Rust fwctl abstractions"
> + depends on RUST
> + select FWCTL
> + help
> + This enables the Rust abstractions for the fwctl device firmware
> + access framework. It provides safe wrappers around struct fwctl_device
> + and struct fwctl_uctx, allowing Rust drivers to register fwctl devices
> + and implement their control and RPC logic in safe Rust.
> +
> + If unsure, say N.
> +
> if FWCTL
> config FWCTL_MLX5

It should be below the if and not use "select FWCTL"

> --- a/include/uapi/fwctl/fwctl.h
> +++ b/include/uapi/fwctl/fwctl.h
> @@ -45,6 +45,7 @@ enum fwctl_device_type {
> FWCTL_DEVICE_TYPE_MLX5 = 1,
> FWCTL_DEVICE_TYPE_CXL = 2,
> FWCTL_DEVICE_TYPE_PDS = 4,
> + FWCTL_DEVICE_TYPE_RUST_FWCTL_TEST = 8,
> };

Put this in the patch adding the test and maybe this is a reason not
to merge it..

> +/// Represents a fwctl device type.
> +///
> +/// This enum corresponds to the C `enum fwctl_device_type` and is used to identify
> +/// the specific firmware control interface implemented by a device.
> +#[repr(u32)]
> +#[derive(Copy, Clone, Debug, Eq, PartialEq)]
> +pub enum DeviceType {
> + /// Error/invalid device type.
> + Error = bindings::fwctl_device_type_FWCTL_DEVICE_TYPE_ERROR,
> + /// MLX5 device type.
> + Mlx5 = bindings::fwctl_device_type_FWCTL_DEVICE_TYPE_MLX5,
> + /// CXL device type.
> + Cxl = bindings::fwctl_device_type_FWCTL_DEVICE_TYPE_CXL,
> + /// PDS device type.
> + Pds = bindings::fwctl_device_type_FWCTL_DEVICE_TYPE_PDS,
> + /// Rust fwctl test device type.
> + RustFwctlTest = bindings::fwctl_device_type_FWCTL_DEVICE_TYPE_RUST_FWCTL_TEST,
> +}

Do we really need these contentless comments?

> +impl Device {
> + /// # Safety
> + ///
> + /// `ptr` must be a valid pointer to a `struct fwctl_device`.
> + unsafe fn from_raw<'a>(ptr: *mut bindings::fwctl_device) -> &'a Self {
> + // CAST: `Self` is a transparent wrapper around `bindings::fwctl_device`.
> + // SAFETY: By the safety requirement, `ptr` is valid.
> + unsafe { &*ptr.cast() }
> + }
> +
> + fn as_raw(&self) -> *mut bindings::fwctl_device {
> + self.0.get()
> + }
> +
> + /// Returns the parent device.
> + pub fn parent(&self) -> &device::Device {
> + // SAFETY: By the type invariant, `self.as_raw()` is a valid pointer to a
> + // `struct fwctl_device`, which always has a parent device.
> + let parent_dev = unsafe { (*self.as_raw()).dev.parent };
> + // SAFETY: `parent_dev` points to a valid `struct device`. The parent device
> + // is guaranteed to be valid for the lifetime of the fwctl_device.
> + unsafe { device::Device::from_raw(parent_dev) }
> + }
> +}
> +
> +impl AsRef<device::Device> for Device {
> + fn as_ref(&self) -> &device::Device {
> + // SAFETY: By the type invariant of `Self`, `self.as_raw()` is a pointer to a valid
> + // `struct fwctl_device`.
> + let dev = unsafe { core::ptr::addr_of_mut!((*self.as_raw()).dev) };
> +
> + // SAFETY: `dev` points to a valid `struct device`.
> + unsafe { device::Device::from_raw(dev) }
> + }
> +}
> +
> +// SAFETY: The fwctl_device is reference counted through the embedded `struct device`,
> +// and inc_ref/dec_ref use fwctl_get/fwctl_put to manage its lifetime.
> +unsafe impl crate::sync::aref::AlwaysRefCounted for Device {
> + fn inc_ref(&self) {
> + // SAFETY: The existence of a shared reference guarantees that the refcount is non-zero.
> + // `self.as_raw()` is a valid pointer to a `struct fwctl_device`.
> + unsafe { bindings::fwctl_get(self.as_raw()) };
> + }
> +
> + unsafe fn dec_ref(obj: NonNull<Self>) {
> + // CAST: `Self` is a transparent wrapper of `bindings::fwctl_device`.
> + let fwctl: *mut bindings::fwctl_device = obj.cast().as_ptr();
> +
> + // SAFETY: By the type invariant, `fwctl` is a valid pointer to a `struct fwctl_device`.
> + unsafe { bindings::fwctl_put(fwctl) };
> + }
> +}
> +
> +// SAFETY: A `Device` is always reference-counted and can be released from any thread.
> +unsafe impl Send for Device {}
> +
> +// SAFETY: `Device` can be shared among threads because all methods are thread-safe.
> +unsafe impl Sync for Device {}
> +
> +/// The registration of a fwctl device.
> +///
> +/// This type represents the registration of a [`struct fwctl_device`]. It should always be
> +/// used within a [`Devres`] wrapper to ensure proper lifetime management. When dropped,
> +/// the fwctl device will be unregistered and freed.
> +///
> +/// [`Devres`] guarantees that the device is unregistered before the parent device is unbound.
> +///
> +/// [`struct fwctl_device`]: srctree/include/linux/device/fwctl.h
> +pub struct Registration<T: Operations> {
> + device: ARef<Device>,
> + _marker: PhantomData<T>,
> +}
> +
> +impl<T: Operations> Registration<T> {
> + /// Allocate and register a new fwctl device under the given parent device.
> + ///
> + /// The returned [`Devres`] wrapper ensures that the fwctl device is unregistered
> + /// before the parent device is unbound.
> + pub fn new<'a>(
> + parent: &'a device::Device<device::Bound>,
> + ) -> impl PinInit<Devres<Self>, Error> + 'a
> + where
> + T: 'a,
> + {
> + pin_init::pin_init_scope(move || {
> + let ops = core::ptr::from_ref::<bindings::fwctl_ops>(&VTable::<T>::VTABLE).cast_mut();
> +
> + // SAFETY: `_fwctl_alloc_device()` allocates a new `fwctl_device`
> + // and initializes its embedded `struct device`. The `ops` pointer
> + // points to a static VTABLE that outlives the device. The parent
> + // device is guaranteed to be bound to a driver (Device<Bound>),
> + // ensuring it remains valid during allocation.
> + let dev = unsafe {
> + bindings::_fwctl_alloc_device(
> + parent.as_raw(),
> + ops,
> + core::mem::size_of::<bindings::fwctl_device>(),
> + )
> + };
> +
> + if dev.is_null() {
> + return Err(ENOMEM);
> + }
> +
> + // SAFETY: dev is guaranteed to be a valid pointer from `_fwctl_alloc_device()`.
> + let ret = unsafe { bindings::fwctl_register(dev) };
> + if ret != 0 {
> + // SAFETY: dev is guaranteed to be a valid pointer from `_fwctl_alloc_device()`.
> + unsafe {
> + bindings::fwctl_put(dev);
> + }
> + return Err(Error::from_errno(ret));
> + }

This looks weirdly sequenced, the driver's object has to be fully
initialized before you can call register, so it is quite strange to
see a wrapper that does both alloc and register in one function.

> +// SAFETY: `Registration` can be sent to other threads because:
> +// - It only contains a `NonNull<fwctl_device>` pointer and a PhantomData marker
> +// - The underlying C fwctl_device is thread-safe with internal locking
> +// - `Drop` calls `fwctl_unregister()/fwctl_put()` which are safe from any sleepable context

fwctl_unregister is not safe from any context, it must be called
while the Device is still bound.

Jason