Message-ID: <20260126181912.GA2131321@nvidia.com>
Date: Mon, 26 Jan 2026 14:19:12 -0400
From: Jason Gunthorpe <jgg@...dia.com>
To: Zhi Wang <zhiw@...dia.com>
Cc: rust-for-linux@...r.kernel.org, linux-pci@...r.kernel.org,
	linux-kernel@...r.kernel.org, dakr@...nel.org, aliceryhl@...gle.com,
	bhelgaas@...gle.com, kwilczynski@...nel.org, ojeda@...nel.org,
	alex.gaynor@...il.com, boqun.feng@...il.com, gary@...yguo.net,
	bjorn3_gh@...tonmail.com, lossin@...nel.org, a.hindborg@...nel.org,
	tmgross@...ch.edu, markus.probst@...teo.de, helgaas@...nel.org,
	cjia@...dia.com, smitra@...dia.com, ankita@...dia.com,
	aniketa@...dia.com, kwankhede@...dia.com, targupta@...dia.com,
	acourbot@...dia.com, joelagnelf@...dia.com, jhubbard@...dia.com,
	zhiwang@...nel.org, daniel.almeida@...labora.com
Subject: Re: [PATCH v2 1/2] rust: introduce abstractions for fwctl

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

Powered by blists - more mailing lists