Introduce 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.
Cc: Danilo Krummrich <dakr@kernel.org>
Cc: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Zhi Wang <zhiw@nvidia.com>
---
drivers/fwctl/Kconfig | 12 +
include/uapi/fwctl/fwctl.h | 1 +
rust/bindings/bindings_helper.h | 1 +
rust/helpers/fwctl.c | 17 ++
rust/helpers/helpers.c | 3 +-
rust/kernel/fwctl.rs | 456 ++++++++++++++++++++++++++++++++
rust/kernel/lib.rs | 2 +
7 files changed, 491 insertions(+), 1 deletion(-)
create mode 100644 rust/helpers/fwctl.c
create mode 100644 rust/kernel/fwctl.rs
diff --git a/drivers/fwctl/Kconfig b/drivers/fwctl/Kconfig
index b5583b12a011..d8538249f3ae 100644
--- 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
tristate "mlx5 ConnectX control fwctl driver"
diff --git a/include/uapi/fwctl/fwctl.h b/include/uapi/fwctl/fwctl.h
index 716ac0eee42d..eea1020ad180 100644
--- 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,
};
/**
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 9fdf76ca630e..2c50d5bab0cf 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -56,6 +56,7 @@
#include <linux/fdtable.h>
#include <linux/file.h>
#include <linux/firmware.h>
+#include <linux/fwctl.h>
#include <linux/interrupt.h>
#include <linux/fs.h>
#include <linux/i2c.h>
diff --git a/rust/helpers/fwctl.c b/rust/helpers/fwctl.c
new file mode 100644
index 000000000000..bb4a028e7afb
--- /dev/null
+++ b/rust/helpers/fwctl.c
@@ -0,0 +1,17 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/fwctl.h>
+
+#if IS_ENABLED(CONFIG_RUST_FWCTL_ABSTRACTIONS)
+
+struct fwctl_device *rust_helper_fwctl_get(struct fwctl_device *fwctl)
+{
+ return fwctl_get(fwctl);
+}
+
+void rust_helper_fwctl_put(struct fwctl_device *fwctl)
+{
+ fwctl_put(fwctl);
+}
+
+#endif
diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
index 79c72762ad9c..19a505473bef 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c
@@ -27,8 +27,9 @@
#include "dma.c"
#include "drm.c"
#include "err.c"
-#include "irq.c"
#include "fs.c"
+#include "fwctl.c"
+#include "irq.c"
#include "io.c"
#include "jump_label.c"
#include "kunit.c"
diff --git a/rust/kernel/fwctl.rs b/rust/kernel/fwctl.rs
new file mode 100644
index 000000000000..4065c948784d
--- /dev/null
+++ b/rust/kernel/fwctl.rs
@@ -0,0 +1,456 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+//! Abstractions for the fwctl.
+//!
+//! This module provides bindings for working with fwctl devices in kernel modules.
+//!
+//! C header: [`include/linux/fwctl.h`]
+
+use crate::{
+ bindings,
+ container_of,
+ device,
+ devres::Devres,
+ prelude::*,
+ types::{
+ ARef,
+ Opaque, //
+ }, //
+};
+use core::{
+ marker::PhantomData,
+ ptr::NonNull,
+ slice, //
+};
+
+/// 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,
+}
+
+impl From<DeviceType> for u32 {
+ fn from(device_type: DeviceType) -> Self {
+ device_type as u32
+ }
+}
+
+/// A fwctl device.
+///
+/// Wraps the C `struct fwctl_device` and manages its reference count.
+///
+/// # Invariants
+///
+/// Instances of this type represent a valid `struct fwctl_device` created by the C portion
+/// of the kernel.
+#[repr(transparent)]
+pub struct Device(Opaque<bindings::fwctl_device>);
+
+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));
+ }
+
+ // SAFETY: dev is guaranteed to be a valid pointer from `_fwctl_alloc_device()`.
+ let device = unsafe {
+ let dev_ref = Device::from_raw(dev);
+ // SAFETY: We just verified dev is non-null above, and Device::from_raw
+ // returns a reference, so NonNull::new_unchecked is safe.
+ ARef::from_raw(NonNull::new_unchecked(
+ core::ptr::from_ref(dev_ref).cast_mut(),
+ ))
+ };
+
+ Ok(Devres::new(
+ parent,
+ Self {
+ device,
+ _marker: PhantomData,
+ },
+ ))
+ })
+ }
+
+ fn as_raw(&self) -> *mut bindings::fwctl_device {
+ self.device.as_raw()
+ }
+}
+
+impl<T: Operations> Drop for Registration<T> {
+ fn drop(&mut self) {
+ // SAFETY: `fwctl_unregister()` expects a valid registered device.
+ // By the type invariant, `self.device` holds a valid fwctl_device.
+ unsafe {
+ bindings::fwctl_unregister(self.as_raw());
+ }
+ // The ARef<Device> will automatically call fwctl_put() when dropped.
+ }
+}
+
+// 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
+unsafe impl<T: Operations> Send for Registration<T> {}
+
+// SAFETY: `Registration` can be shared between threads because:
+// - It provides no methods for mutation (except Drop, which takes &mut self)
+// - The underlying C fwctl_device is protected by internal locking (registration_lock)
+// - Multiple threads can safely hold immutable references to the same Registration
+unsafe impl<T: Operations> Sync for Registration<T> {}
+
+/// Trait implemented by each Rust driver that integrates with the fwctl subsystem.
+///
+/// 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 {
+ /// Driver user context type.
+ type UserCtx;
+
+ /// fwctl device type.
+ const DEVICE_TYPE: DeviceType;
+
+ /// Called when a new user context is opened by userspace.
+ fn open(
+ fwctl_uctx: &Opaque<bindings::fwctl_uctx>,
+ ) -> Result<impl PinInit<Self::UserCtx, Error>, Error>;
+
+ /// Called when the user context is being closed.
+ fn close(uctx: &mut UserCtx<Self::UserCtx>);
+
+ /// Return device or context information to userspace.
+ fn info(uctx: &mut UserCtx<Self::UserCtx>) -> Result<KVec<u8>, Error>;
+
+ /// Called when a userspace RPC request is received.
+ fn fw_rpc(
+ uctx: &mut UserCtx<Self::UserCtx>,
+ scope: u32,
+ rpc_in: &mut [u8],
+ out_len: *mut usize,
+ ) -> Result<Option<KVec<u8>>, Error>;
+}
+
+/// Represents a per-FD user context (`struct fwctl_uctx`).
+#[repr(C)]
+#[pin_data]
+pub struct UserCtx<T> {
+ /// The core fwctl user context shared with the C implementation.
+ #[pin]
+ fwctl_uctx: Opaque<bindings::fwctl_uctx>,
+
+ /// Driver-specific data associated with this user context.
+ #[pin]
+ uctx: T,
+}
+
+impl<T> UserCtx<T> {
+ /// Converts a raw C pointer to `struct fwctl_uctx` into a reference to the
+ /// enclosing `UserCtx<T>`.
+ ///
+ /// # Safety
+ /// * `ptr` must be a valid pointer to a `fwctl_uctx` that is embedded
+ /// inside an existing `UserCtx<T>` instance.
+ /// * The caller must ensure that the lifetime of the returned reference
+ /// does not outlive the underlying object managed on the C side.
+ unsafe fn from_raw<'a>(ptr: *mut bindings::fwctl_uctx) -> &'a mut Self {
+ // SAFETY: `ptr` was originally created from a valid `UserCtx<T>`.
+ // We cast through `Opaque` since `fwctl_uctx` is wrapped in `Opaque`.
+ unsafe { &mut *container_of!(Opaque::cast_from(ptr), UserCtx<T>, fwctl_uctx).cast_mut() }
+ }
+
+ /// Returns a reference to the parent device from a raw `fwctl_uctx` pointer.
+ pub fn parent_device_from_raw(
+ fwctl_uctx: &Opaque<bindings::fwctl_uctx>,
+ ) -> &device::Device<device::Bound> {
+ // SAFETY: `fwctl_uctx` is initialized by the fwctl subsystem
+ // and guaranteed to remain valid.
+ let raw_fwctl = unsafe { (*fwctl_uctx.get()).fwctl };
+ // SAFETY: `raw_fwctl` is a valid pointer to a `fwctl_device`, and its `dev.parent`
+ // field points to a valid parent device.
+ let raw_dev = unsafe { (*raw_fwctl).dev.parent };
+
+ // SAFETY: `raw_dev` points to a live device object.
+ let dev: &device::Device = unsafe { device::Device::from_raw(raw_dev) };
+
+ // SAFETY: The device is guaranteed to be bound.
+ unsafe { dev.as_bound() }
+ }
+
+ /// Returns a reference to the parent device of this user context.
+ pub fn get_parent_device(&self) -> &device::Device<device::Bound> {
+ Self::parent_device_from_raw(&self.fwctl_uctx)
+ }
+}
+
+impl<T> core::ops::Deref for UserCtx<T> {
+ type Target = T;
+
+ fn deref(&self) -> &Self::Target {
+ &self.uctx
+ }
+}
+
+impl<T> core::ops::DerefMut for UserCtx<T> {
+ fn deref_mut(&mut self) -> &mut Self::Target {
+ &mut self.uctx
+ }
+}
+
+/// Static vtable mapping Rust trait methods to C callbacks.
+pub struct VTable<T: Operations>(PhantomData<T>);
+
+impl<T: Operations> VTable<T> {
+ /// Static instance of `fwctl_ops` used by the C core to call into Rust.
+ pub const VTABLE: bindings::fwctl_ops = bindings::fwctl_ops {
+ device_type: T::DEVICE_TYPE as u32,
+ uctx_size: core::mem::size_of::<UserCtx<T::UserCtx>>(),
+ 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),
+ };
+
+ /// Called when a new user context is opened by userspace.
+ /// # Safety
+ ///
+ /// `uctx` must be a valid pointer to an initialized `fwctl_uctx` structure,
+ /// embedded within a C-allocated `UserCtx<T::UserCtx>` with sufficient space.
+ unsafe extern "C" fn open_uctx_callback(uctx: *mut bindings::fwctl_uctx) -> ffi::c_int {
+ // SAFETY: `uctx` points to valid, initialized `fwctl_uctx` structure.
+ let fwctl_uctx_ref = unsafe { &*Opaque::cast_from(uctx) };
+
+ let initializer = match T::open(fwctl_uctx_ref) {
+ Ok(init) => init,
+ Err(e) => return e.to_errno(),
+ };
+
+ let uctx_offset = core::mem::offset_of!(UserCtx<T::UserCtx>, uctx);
+
+ // SAFETY: The C side allocated enough space for the entire UserCtx.
+ let uctx_ptr: *mut T::UserCtx = unsafe { uctx.cast::<u8>().add(uctx_offset).cast() };
+
+ // Initialize the uctx field in-place using the pin initializer.
+ // SAFETY:
+ // - uctx_ptr points to valid allocated memory
+ // - The memory is properly aligned (guaranteed by #[repr(C)] and our compile-time check)
+ // - The memory is uninitialized, which is what PinInit expects
+ // - After this call, the memory will be properly initialized
+ match unsafe { initializer.__pinned_init(uctx_ptr.cast()) } {
+ Ok(()) => 0,
+ Err(e) => e.to_errno(),
+ }
+ }
+
+ /// Called when the user context is being closed.
+ /// # Safety
+ ///
+ /// `uctx` must be a valid pointer to an initialized `fwctl_uctx` structure,
+ /// embedded within a fully initialized `UserCtx<T::UserCtx>`.
+ unsafe extern "C" fn close_uctx_callback(uctx: *mut bindings::fwctl_uctx) {
+ // SAFETY: `uctx` is guaranteed by the fwctl subsystem to be a valid pointer.
+ let ctx = unsafe { UserCtx::<T::UserCtx>::from_raw(uctx) };
+ T::close(ctx);
+ }
+
+ /// Returns device or context information.
+ /// # Safety
+ ///
+ /// - `uctx` must be a valid pointer to an initialized `fwctl_uctx` structure,
+ /// embedded within a fully initialized `UserCtx<T::UserCtx>`.
+ /// - `length` must be a valid pointer to write the output length.
+ unsafe extern "C" fn info_callback(
+ uctx: *mut bindings::fwctl_uctx,
+ length: *mut usize,
+ ) -> *mut ffi::c_void {
+ // SAFETY: `uctx` is guaranteed by the fwctl subsystem to be a valid pointer.
+ let ctx = unsafe { UserCtx::<T::UserCtx>::from_raw(uctx) };
+
+ match T::info(ctx) {
+ Ok(kvec) => {
+ // The ownership of the buffer is now transferred to the foreign
+ // caller. It must eventually be released by fwctl framework.
+ let (ptr, len, _cap) = kvec.into_raw_parts();
+
+ // SAFETY: `length` is a valid out-parameter provided by the C
+ // caller. Write the number of bytes in the returned buffer.
+ unsafe {
+ *length = len;
+ }
+
+ ptr.cast::<ffi::c_void>()
+ }
+
+ Err(e) => Error::to_ptr(e),
+ }
+ }
+
+ /// Called when a user-space RPC request is received.
+ /// # Safety
+ ///
+ /// - `uctx` must be a valid pointer to an initialized `fwctl_uctx` structure,
+ /// embedded within a fully initialized `UserCtx<T::UserCtx>`.
+ /// - `rpc_in` must be a valid pointer to `in_len` bytes of readable/writable memory.
+ /// - `out_len` must be a valid pointer to write the output length.
+ unsafe extern "C" fn fw_rpc_callback(
+ uctx: *mut bindings::fwctl_uctx,
+ scope: u32,
+ rpc_in: *mut ffi::c_void,
+ in_len: usize,
+ out_len: *mut usize,
+ ) -> *mut ffi::c_void {
+ // SAFETY: `uctx` is guaranteed by the fwctl framework to be a valid pointer.
+ let ctx = unsafe { UserCtx::<T::UserCtx>::from_raw(uctx) };
+
+ // SAFETY: Creating a mutable slice from `rpc_in`:
+ // - `rpc_in` is non-null and properly aligned: guaranteed by the fwctl subsystem
+ // - `rpc_in` points to `in_len` consecutive properly initialized bytes
+ // - The memory is valid for reads and writes for the lifetime of the returned slice
+ // - The total size `in_len` does not exceed `isize::MAX`: checked by the fwctl subsystem
+ // - No other references to this memory exist during this callback
+ let rpc_in_slice: &mut [u8] =
+ unsafe { slice::from_raw_parts_mut(rpc_in.cast::<u8>(), in_len) };
+
+ match T::fw_rpc(ctx, scope, rpc_in_slice, out_len) {
+ // Driver allocates a new output buffer.
+ Ok(Some(kvec)) => {
+ // The ownership of the buffer is now transferred to the foreign
+ // caller. It must eventually be released by fwctl subsystem.
+ let (ptr, len, _cap) = kvec.into_raw_parts();
+
+ // SAFETY: `out_len` is a valid writable pointer provided by the C caller.
+ unsafe { *out_len = len };
+
+ ptr.cast::<ffi::c_void>()
+ }
+
+ // Driver re-uses the existing input buffer and writes the out_len.
+ Ok(None) => rpc_in,
+
+ Err(e) => Error::to_ptr(e),
+ }
+ }
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 6d637e2fed1b..956e865c17ea 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -97,6 +97,8 @@
pub mod firmware;
pub mod fmt;
pub mod fs;
+#[cfg(CONFIG_RUST_FWCTL_ABSTRACTIONS)]
+pub mod fwctl;
#[cfg(CONFIG_I2C = "y")]
pub mod i2c;
pub mod id_pool;
--
2.51.0On 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
On Mon, 26 Jan 2026 14:19:12 -0400
Jason Gunthorpe <jgg@nvidia.com> wrote:
> 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.
> >
snip
> > +/// 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?
>
I will try to remove them if the compiler allows me to do that in the next
re-spin.
> > +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() }
> > + }
snip
> > + // 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.
>
The fwctl_alloc_device() helper allocates a raw struct fwctl_device
without private driver data here. The Rust driver object should be
already allocated and initialized separately before reaching this
point.
We rely on the standard dev->parent chain to access the rust driver
object from the fwctl callbacks.
> > + bindings::_fwctl_alloc_device(
> > + parent.as_raw(),
> > + ops,
> > + core::mem::size_of::<bindings::fwctl_device>(),
> > +// 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.
>
The registration is wrapped in Devres<> in the sample driver, which
guarantees that drop is called while the Device is still bound.
I agree that the current abstraction itself does not strictly enforce this
(e.g., if the object is moved out of Devres). I will investigate an
approach to enforce this requirement in the next re-spin.
Z.
> Jason
>
On Tue Jan 27, 2026 at 8:57 PM CET, Zhi Wang wrote: > On Mon, 26 Jan 2026 14:19:12 -0400 > Jason Gunthorpe <jgg@nvidia.com> wrote: >> fwctl_unregister is not safe from any context, it must be called >> while the Device is still bound. >> > > The registration is wrapped in Devres<> in the sample driver, which > guarantees that drop is called while the Device is still bound. > > I agree that the current abstraction itself does not strictly enforce this > (e.g., if the object is moved out of Devres). I will investigate an > approach to enforce this requirement in the next re-spin. The code is fine, the Registration object can't be moved out of devres safely. You just need to update the safety comment accordingly. :)
On Tue Jan 27, 2026 at 8:57 PM CET, Zhi Wang wrote: > The fwctl_alloc_device() helper allocates a raw struct fwctl_device > without private driver data here. The Rust driver object should be > already allocated and initialized separately before reaching this > point. > > We rely on the standard dev->parent chain to access the rust driver > object from the fwctl callbacks. (I will go for a thorough review soon, but for now a quick drive-by comment.) IIUC, you are saying that the user is supposed to use the private data of the parent device in fwctl callbacks. Let's not make this a design choice please. Instead, allow the user pass in separate private data for the fwctl device as well. This serves the purpose of clear ownership and lifetime of the data. E.g. the fwctl device does not necessarily exist as long as the parent device is bound. It is a good thing if driver authors are forced to take a decision about which object owns the data and what's the scope of the data.
On Tue, 27 Jan 2026 21:07:37 +0100 "Danilo Krummrich" <dakr@kernel.org> wrote: > On Tue Jan 27, 2026 at 8:57 PM CET, Zhi Wang wrote: > > The fwctl_alloc_device() helper allocates a raw struct fwctl_device > > without private driver data here. The Rust driver object should be > > already allocated and initialized separately before reaching this > > point. > > > > We rely on the standard dev->parent chain to access the rust driver > > object from the fwctl callbacks. > > (I will go for a thorough review soon, but for now a quick drive-by > comment.) > > IIUC, you are saying that the user is supposed to use the private data > of the parent device in fwctl callbacks. Let's not make this a design > choice please. Instead, allow the user pass in separate private data for > the fwctl device as well. > > This serves the purpose of clear ownership and lifetime of the data. > E.g. the fwctl device does not necessarily exist as long as the parent > device is bound. > > It is a good thing if driver authors are forced to take a decision about > which object owns the data and what's the scope of the data. I wrote a version like this before. My initial concern of mixing Rust objects together with C objecs within C-allocated memory was about potential memory alignment issues when rust side doing CAST on the memory. I agree that providing a way to attach private data directly to the fwctl_device also has quite some benetifs. IMO, if we go this way, the private data from rust side needs to have #[repr(C)] to address the above issue all the time? Z.
On Wed Jan 28, 2026 at 12:36 PM CET, Zhi Wang wrote: > On Tue, 27 Jan 2026 21:07:37 +0100 > "Danilo Krummrich" <dakr@kernel.org> wrote: > >> On Tue Jan 27, 2026 at 8:57 PM CET, Zhi Wang wrote: >> > The fwctl_alloc_device() helper allocates a raw struct fwctl_device >> > without private driver data here. The Rust driver object should be >> > already allocated and initialized separately before reaching this >> > point. >> > >> > We rely on the standard dev->parent chain to access the rust driver >> > object from the fwctl callbacks. >> >> (I will go for a thorough review soon, but for now a quick drive-by >> comment.) >> >> IIUC, you are saying that the user is supposed to use the private data >> of the parent device in fwctl callbacks. Let's not make this a design >> choice please. Instead, allow the user pass in separate private data for >> the fwctl device as well. >> >> This serves the purpose of clear ownership and lifetime of the data. >> E.g. the fwctl device does not necessarily exist as long as the parent >> device is bound. >> >> It is a good thing if driver authors are forced to take a decision about >> which object owns the data and what's the scope of the data. > > I wrote a version like this before. My initial concern of mixing Rust > objects together with C objecs within C-allocated memory was about > potential memory alignment issues when rust side doing CAST on the memory. > > I agree that providing a way to attach private data directly to the > fwctl_device also has quite some benetifs. > > IMO, if we go this way, the private data from rust side needs to have > #[repr(C)] to address the above issue all the time? No that's not necessary. Please have a look at what I did in drm::Device::new() [1], this should be the exact same case. [1] https://rust.docs.kernel.org/src/kernel/drm/device.rs.html#98
On Tue, Jan 27, 2026 at 09:07:37PM +0100, Danilo Krummrich wrote: > On Tue Jan 27, 2026 at 8:57 PM CET, Zhi Wang wrote: > > The fwctl_alloc_device() helper allocates a raw struct fwctl_device > > without private driver data here. The Rust driver object should be > > already allocated and initialized separately before reaching this > > point. > > > > We rely on the standard dev->parent chain to access the rust driver > > object from the fwctl callbacks. > > (I will go for a thorough review soon, but for now a quick drive-by comment.) > > IIUC, you are saying that the user is supposed to use the private data of the > parent device in fwctl callbacks. Let's not make this a design choice please. > Instead, allow the user pass in separate private data for the fwctl device as > well. > > This serves the purpose of clear ownership and lifetime of the data. E.g. the > fwctl device does not necessarily exist as long as the parent device is bound. > > It is a good thing if driver authors are forced to take a decision about which > object owns the data and what's the scope of the data. I'm not sure what the model is in rust, but the expection here is for the driver to have a window between alloc and register where the driver can fill in any information into the core structures that is needed before calling registration. Think like the driver core - the end user needs to be able to call dev_name() (and lots of others!) after device_initialize() but before calling register/add. I don't think we use this right now in fwctl, but I'd rather it not be erased from the rust bindings entirely. That sounds hard to unwind someday. This is pretty common subsystem design so I'd think there should be a general pattern in rust as well? Jason
On Wed Jan 28, 2026 at 1:04 AM CET, Jason Gunthorpe wrote:
> On Tue, Jan 27, 2026 at 09:07:37PM +0100, Danilo Krummrich wrote:
>> On Tue Jan 27, 2026 at 8:57 PM CET, Zhi Wang wrote:
>> > The fwctl_alloc_device() helper allocates a raw struct fwctl_device
>> > without private driver data here. The Rust driver object should be
>> > already allocated and initialized separately before reaching this
>> > point.
>> >
>> > We rely on the standard dev->parent chain to access the rust driver
>> > object from the fwctl callbacks.
>>
>> (I will go for a thorough review soon, but for now a quick drive-by comment.)
>>
>> IIUC, you are saying that the user is supposed to use the private data of the
>> parent device in fwctl callbacks. Let's not make this a design choice please.
>> Instead, allow the user pass in separate private data for the fwctl device as
>> well.
>>
>> This serves the purpose of clear ownership and lifetime of the data. E.g. the
>> fwctl device does not necessarily exist as long as the parent device is bound.
>>
>> It is a good thing if driver authors are forced to take a decision about which
>> object owns the data and what's the scope of the data.
I think we were talking about different things. :)
Zhi mentioned the private data of the parent device being used in fwctl
callbacks, but we should use private data stored in fwctl_device::dev instead.
> I'm not sure what the model is in rust, but the expection here is for
> the driver to have a window between alloc and register where the
> driver can fill in any information into the core structures that is
> needed before calling registration.
Yes, in Rust you can't have an instance of a structure without initializing it
(unless you put it in a MaybeUninit container, etc.).
For dynamic allocations Rust solves this with initializers. For instance, where
C does:
struct foo *foo = kzalloc(sizeof(*foo), GFP_KERNEL);
/* Maybe broken if foo is used here already. */
foo_initialize(foo, ...);
you'd have something like this in Rust:
struct Foo {
data: Mutex<Data>,
index: u32,
}
impl Foo {
fn new(index: u32, args: Args) -> impl PinInit<Self, Error> {
try_pin_init!(Self {
data <- new_mutex!(args.to_data()?),
index.
})
}
}
The returned object is not an instance of struct Foo yet, it is an initializer,
i.e. impl PinInit<Foo, Error>; no memory has been allocated yet. Note that this
initializer can also fail if executed, since args.to_data() can fail.
Let's allocate the memory now:
let foo = KBox::pin_init(Foo::new(...), GFP_KERNEL)?;
This allocates the memory (i.e. KBox::pin_init() calls kmalloc() eventually) and
executes the initializer. (The call fails if either the memory allocation fails,
or the initializer fails.) This way there is no way for the caller to use foo
before it has been initialized.
(What about the "pin" thing? Let's assume Data contains something
self-referential, in this case pin-init does prevent the user from moving data
out of Foo, which would be a bug since it is a self-referential structure.
Actually, the mutex must not be moved either.)
Back to this abstraction. :)
In this case Registration::new() returns an initializer, but also allocates the
C struct fwctl_device within the initializer.
AFAICS, _fwctl_alloc_device() already initializes the structure properly, so it
seems there is nothing to be done. But let's assume there is. In this case it
wouldn't help much if we would create a separate fwctl::Device structure before,
as this one also has to be properly initialized once created. So the constructor
of fwctl::Device has to take the corresponding arguments.
Though, sometimes there are cases where we have to defer some initialization.
This is where we usually use separate types or type states. Let's assume
something in the device only ever gets initialized after registration for some
reason. In this case you could have a fwctl::Device<Unregistred> and a
fwctl::Device<Registered> and correspondingly treat the inner data as partially
uninitialized (which requires unsafe code).
Either way, I think it would be cleaner if fwctl::Device has a constructor
impl Device<T> {
fn new(
parent: &Device<Bound>,
data: impl PinInit<T, Error>
) -> Result<ARef<Self>>;
}
where T is the driver's private data for the struct fwctl_device.
And Registration::new() can take a &fwctl::Device<T> and the parent
&Device<Bound> of course. This would also be in line with what we do in other
class device abstractions.
On Wed, Jan 28, 2026 at 02:21:38AM +0100, Danilo Krummrich wrote:
> On Wed Jan 28, 2026 at 1:04 AM CET, Jason Gunthorpe wrote:
> > On Tue, Jan 27, 2026 at 09:07:37PM +0100, Danilo Krummrich wrote:
> >> On Tue Jan 27, 2026 at 8:57 PM CET, Zhi Wang wrote:
> >> > The fwctl_alloc_device() helper allocates a raw struct fwctl_device
> >> > without private driver data here. The Rust driver object should be
> >> > already allocated and initialized separately before reaching this
> >> > point.
> >> >
> >> > We rely on the standard dev->parent chain to access the rust driver
> >> > object from the fwctl callbacks.
> >>
> >> (I will go for a thorough review soon, but for now a quick drive-by comment.)
> >>
> >> IIUC, you are saying that the user is supposed to use the private data of the
> >> parent device in fwctl callbacks. Let's not make this a design choice please.
> >> Instead, allow the user pass in separate private data for the fwctl device as
> >> well.
> >>
> >> This serves the purpose of clear ownership and lifetime of the data. E.g. the
> >> fwctl device does not necessarily exist as long as the parent device is bound.
> >>
> >> It is a good thing if driver authors are forced to take a decision about which
> >> object owns the data and what's the scope of the data.
>
> I think we were talking about different things. :)
Well, I've always been talking about this :)
> In this case Registration::new() returns an initializer, but also allocates the
> C struct fwctl_device within the initializer.
In a normal C implementation this would allocate both the core and
driver struct using one memory and a container_of() relationship.
> AFAICS, _fwctl_alloc_device() already initializes the structure properly, so it
> seems there is nothing to be done.
It does the first part of a three step sequence
1) Allocate memory and initialize core code
2) Steup driver related data
3) "register" to make the device live and begin concurrent access
I don't think 1 and 3 can be in the same function. The driver must have
the opportunity to do its #2 step in this sequence.
> Though, sometimes there are cases where we have to defer some initialization.
> This is where we usually use separate types or type states. Let's assume
> something in the device only ever gets initialized after registration for some
> reason. In this case you could have a fwctl::Device<Unregistred> and a
> fwctl::Device<Registered> and correspondingly treat the inner data as partially
> uninitialized (which requires unsafe code).
Maybe this is what is needed here then.
> Either way, I think it would be cleaner if fwctl::Device has a constructor
>
> impl Device<T> {
> fn new(
> parent: &Device<Bound>,
> data: impl PinInit<T, Error>
> ) -> Result<ARef<Self>>;
> }
>
> where T is the driver's private data for the struct fwctl_device.
>
> And Registration::new() can take a &fwctl::Device<T> and the parent
> &Device<Bound> of course. This would also be in line with what we do in other
> class device abstractions.
If it goes like this is there some way rust can retain the
container_of layout and avoid a second memory allocation?
Jason
On Wed Jan 28, 2026 at 2:20 PM CET, Jason Gunthorpe wrote:
> On Wed, Jan 28, 2026 at 02:21:38AM +0100, Danilo Krummrich wrote:
>> On Wed Jan 28, 2026 at 1:04 AM CET, Jason Gunthorpe wrote:
>> > On Tue, Jan 27, 2026 at 09:07:37PM +0100, Danilo Krummrich wrote:
>> >> On Tue Jan 27, 2026 at 8:57 PM CET, Zhi Wang wrote:
>> >> > The fwctl_alloc_device() helper allocates a raw struct fwctl_device
>> >> > without private driver data here. The Rust driver object should be
>> >> > already allocated and initialized separately before reaching this
>> >> > point.
>> >> >
>> >> > We rely on the standard dev->parent chain to access the rust driver
>> >> > object from the fwctl callbacks.
>> >>
>> >> (I will go for a thorough review soon, but for now a quick drive-by comment.)
>> >>
>> >> IIUC, you are saying that the user is supposed to use the private data of the
>> >> parent device in fwctl callbacks. Let's not make this a design choice please.
>> >> Instead, allow the user pass in separate private data for the fwctl device as
>> >> well.
>> >>
>> >> This serves the purpose of clear ownership and lifetime of the data. E.g. the
>> >> fwctl device does not necessarily exist as long as the parent device is bound.
>> >>
>> >> It is a good thing if driver authors are forced to take a decision about which
>> >> object owns the data and what's the scope of the data.
>>
>> I think we were talking about different things. :)
>
> Well, I've always been talking about this :)
Fair enough. :)
>> In this case Registration::new() returns an initializer, but also allocates the
>> C struct fwctl_device within the initializer.
>
> In a normal C implementation this would allocate both the core and
> driver struct using one memory and a container_of() relationship.
That's what happens here as well, see below.
>> AFAICS, _fwctl_alloc_device() already initializes the structure properly, so it
>> seems there is nothing to be done.
>
> It does the first part of a three step sequence
>
> 1) Allocate memory and initialize core code
> 2) Steup driver related data
> 3) "register" to make the device live and begin concurrent access
>
> I don't think 1 and 3 can be in the same function. The driver must have
> the opportunity to do its #2 step in this sequence.
Yes, the driver has this opportunity, again see below.
>> Either way, I think it would be cleaner if fwctl::Device has a constructor
>>
>> impl Device<T> {
>> fn new(
>> parent: &Device<Bound>,
>> data: impl PinInit<T, Error>
>> ) -> Result<ARef<Self>>;
>> }
>>
>> where T is the driver's private data for the struct fwctl_device.
>>
>> And Registration::new() can take a &fwctl::Device<T> and the parent
>> &Device<Bound> of course. This would also be in line with what we do in other
>> class device abstractions.
>
> If it goes like this is there some way rust can retain the
> container_of layout and avoid a second memory allocation?
There is no second memory allocation. In the implementation of
fwctl::Device::new() above we call _fwctl_alloc_device() with a size (and
layout) such that this allocation is suitable to initialize the second argument
(i.e. data: impl PinInit<T, Error>) within this allocation.
This is the equivalent to the subclassing pattern as you would implement it in C
with container_of(). See also the drm::Device implementation [1], which does the
exact same thing.
When it comes to the Registration object (which returns an initializer as well),
this would go into the initializer (initializer can be nested) of the driver's
device private data of the parent device, i.e. no separate allocation, it just
lives in the driver's device private data of the parent device.
Alternatively, we can also just not return a Registration object at all and
leave it up to devres to entirely, i.e. equivalent to a
devm_fwctl_register_device() function. The Registration object is only needed if
we intend to unregister the fwctl device before the parent device unbind.
[1] https://rust.docs.kernel.org/src/kernel/drm/device.rs.html#98
On Wed, Jan 28, 2026 at 03:01:25PM +0100, Danilo Krummrich wrote: > There is no second memory allocation. In the implementation of > fwctl::Device::new() above we call _fwctl_alloc_device() with a size (and > layout) such that this allocation is suitable to initialize the second argument > (i.e. data: impl PinInit<T, Error>) within this allocation. You are talking about your suggestions now right? Because what I see in Zi's patch doesn't match any of this? + bindings::_fwctl_alloc_device( + parent.as_raw(), + ops, + core::mem::size_of::<bindings::fwctl_device>(), + ) That is not allocating any memory for driver use ... What you are explaining sounds good to me, though I don't quite get the PinInit<> flow but I trust you on that. :) Jason
On Wed Jan 28, 2026 at 3:59 PM CET, Jason Gunthorpe wrote:
> On Wed, Jan 28, 2026 at 03:01:25PM +0100, Danilo Krummrich wrote:
>> There is no second memory allocation. In the implementation of
>> fwctl::Device::new() above we call _fwctl_alloc_device() with a size (and
>> layout) such that this allocation is suitable to initialize the second argument
>> (i.e. data: impl PinInit<T, Error>) within this allocation.
>
> You are talking about your suggestions now right?
Correct, I'm talking about my proposal.
> Because what I see in Zi's patch doesn't match any of this?
>
> + bindings::_fwctl_alloc_device(
> + parent.as_raw(),
> + ops,
> + core::mem::size_of::<bindings::fwctl_device>(),
> + )
>
> That is not allocating any memory for driver use ...
Yes, the patch doesn't do any of that.
> What you are explaining sounds good to me, though I don't quite get
> the PinInit<> flow but I trust you on that. :)
So, it would basically look like this:
pub struct Device<T> {
dev: Opaque<bindings::fwctl_device>,
data: T,
}
Where T is the type of the driver specific fwctl device data.
impl<T> Device<T> {
pub fn new(
parent: &device::Device,
data: impl PinInit<T::Data, Error>
) -> Result<ARef<Self>> {
let layout = Kmalloc::aligned_layout(Layout::new::<Self>());
// SAFETY: ...
let raw_fwctl: *mut Self = unsafe {
bindings::_fwctl_alloc_device(
parent.as_raw(),
&Self::VTABLE,
layout.size(),
)
}
.cast();
let raw_fwctl = NonNull::new(from_err_ptr(raw_fwctl)?).ok_or(ENOMEM)?;
// Get the pointer to `Self::data`.
let raw_data = unsafe { &raw mut (*raw_fwctl.as_ptr().data) };
// Initialize the `data` initializer within the memory pointed
// to by `raw_data`.
unsafe { data.__pinned_init(raw_data) }.inspect_err(|_| {
// Find the `struct fwctl_device` pointer from the `Self`
// pointer.
let fwctl_dev = unsafe { Self::into_fwcl_device(raw_fwctl) };
// Since we failed to execute the initializer of `data`,
// unwind, i.e. drop our reference to `fwctl_dev`.
unsafe { bindings::fwctl_put(fwctl_dev) };
})?;
// The initializer was successful, hence return `Self` as
// `ARef<Self>`.
Ok(unsafe { ARef::from_raw(raw_fwctl) })
}
}
So, essentially the driver passes an initializer of its private data and we
"write" this initializer into the extra memory allocated with
_fwctl_alloc_device().
On Wed, Jan 28, 2026 at 04:49:07PM +0100, Danilo Krummrich wrote:
> // Initialize the `data` initializer within the memory pointed
> // to by `raw_data`.
> unsafe { data.__pinned_init(raw_data) }.inspect_err(|_| {
> So, essentially the driver passes an initializer of its private data and we
> "write" this initializer into the extra memory allocated with
> _fwctl_alloc_device().
This all seems like the right way to do it!
My only remark it that it still doesn't give an opportunity to call a
function between init and register.
__pinned_init() is taking the T type without access to the initialized
fwctl_device
So if I add some function drivers need to call between init and register:
fwctl_XYZ(fwctl, ..)
It is not possible?
Or would you some container_of in rust within pinned_init()?
Or is it needed to add the typestate?
Jason
On Wed Jan 28, 2026 at 4:56 PM CET, Jason Gunthorpe wrote:
> On Wed, Jan 28, 2026 at 04:49:07PM +0100, Danilo Krummrich wrote:
>
>> // Initialize the `data` initializer within the memory pointed
>> // to by `raw_data`.
>> unsafe { data.__pinned_init(raw_data) }.inspect_err(|_| {
>
>> So, essentially the driver passes an initializer of its private data and we
>> "write" this initializer into the extra memory allocated with
>> _fwctl_alloc_device().
>
> This all seems like the right way to do it!
>
> My only remark it that it still doesn't give an opportunity to call a
> function between init and register.
You would, from a driver side it would look like this:
#[pin_data]
struct MyDriver {
#[pin]
_reg: Devres<fwctl::Registration>,
fwctl: ARef<fwctl::Device>,
}
#[pin_data]
struct FwctlData {
#[pin]
foo: Mutex<Foo>,
...,
}
impl pci::Driver for MyDriver {
fn probe(
pdev: &pci::Device<Core>,
_info: &Self::IdInfo,
) -> impl PinInit<Self, Error> {
let fwctl = fwctl::Device::new(
pdev.as_ref(),
try_pin_init!(FwctlData {
foo <- mutex_new!(Foo::new()),
...,
}),
)?;
// Let's do something with the `fwctl::Device` before we
// register it.
fwctl.do_stuff();
try_pin_init!(Self {
// We could omit this and instead provide
// `fwctl::Registration::register()`, which does only return
// a `Result` and keeps the `fwct::Device` registered until
// driver unbind.
_reg <- fwctl::Registration::new(pdev.as_ref(), fwctl);
fwctl,
})
}
}
> __pinned_init() is taking the T type without access to the initialized
> fwctl_device
If a driver, in order to come up with its private data (FwctlData in the example
above), needs a struct fwctl_device, we could make this happen as well. For
instance, fwctl::Device::new() could take a closure that has a valid
representation of a struct fwctl_device as argument. But I think that's not
necessary.
> So if I add some function drivers need to call between init and register:
>
> fwctl_XYZ(fwctl, ..)
>
> It is not possible?
As you can see from the example above, that's possible.
> Or is it needed to add the typestate?
This is something we should consider when a fwctl::Device would have different
states it can be in, where calling certain methods of a fwctl::Device is only
valid for a certain state and would cause undefined behavior if called from the
wrong state.
On Wed, 28 Jan 2026 17:35:04 +0100 "Danilo Krummrich" <dakr@kernel.org> wrote: > On Wed Jan 28, 2026 at 4:56 PM CET, Jason Gunthorpe wrote: > > On Wed, Jan 28, 2026 at 04:49:07PM +0100, Danilo Krummrich wrote: snip > > Or is it needed to add the typestate? > > This is something we should consider when a fwctl::Device would have > different states it can be in, where calling certain methods of a > fwctl::Device is only valid for a certain state and would cause > undefined behavior if called from the wrong state. Are you saying we should define typestate like Device<Bound/Core> also for fwctl device? I haven't thought about this and some design consideration would be helpful, as the rust PCI subsystem has it, while some of other abstractions don't have it. I could start to picture about this if this is necessary. Z.
On Wed Jan 28, 2026 at 6:30 PM CET, Zhi Wang wrote:
> On Wed, 28 Jan 2026 17:35:04 +0100
> "Danilo Krummrich" <dakr@kernel.org> wrote:
>
>> On Wed Jan 28, 2026 at 4:56 PM CET, Jason Gunthorpe wrote:
>> > On Wed, Jan 28, 2026 at 04:49:07PM +0100, Danilo Krummrich wrote:
>
> snip
>
>> > Or is it needed to add the typestate?
>>
>> This is something we should consider when a fwctl::Device would have
>> different states it can be in, where calling certain methods of a
>> fwctl::Device is only valid for a certain state and would cause
>> undefined behavior if called from the wrong state.
>
> Are you saying we should define typestate like Device<Bound/Core> also for
> fwctl device? I haven't thought about this and some design consideration
> would be helpful, as the rust PCI subsystem has it, while some of other
> abstractions don't have it. I could start to picture about this if this
> is necessary.
No, I don't see a need for a type state for fwctl::Device.
The DeviceContext type states, such as Core, Bound, etc. are type states for bus
devices only. If you find other device structures that don't have this, then
those are not bus devices, but class devices (such as {pwm,drm,fwctl}::Device).
On Wed, Jan 28, 2026 at 07:30:11PM +0200, Zhi Wang wrote: > On Wed, 28 Jan 2026 17:35:04 +0100 > "Danilo Krummrich" <dakr@kernel.org> wrote: > > > On Wed Jan 28, 2026 at 4:56 PM CET, Jason Gunthorpe wrote: > > > On Wed, Jan 28, 2026 at 04:49:07PM +0100, Danilo Krummrich wrote: > > snip > > > > Or is it needed to add the typestate? > > > > This is something we should consider when a fwctl::Device would have > > different states it can be in, where calling certain methods of a > > fwctl::Device is only valid for a certain state and would cause > > undefined behavior if called from the wrong state. > > Are you saying we should define typestate like Device<Bound/Core> also for > fwctl device? I haven't thought about this and some design consideration > would be helpful, as the rust PCI subsystem has it, while some of other > abstractions don't have it. I could start to picture about this if this > is necessary. I think it is not necessary right now Jason
On Wed, Jan 28, 2026 at 05:35:04PM +0100, Danilo Krummrich wrote:
> On Wed Jan 28, 2026 at 4:56 PM CET, Jason Gunthorpe wrote:
> > On Wed, Jan 28, 2026 at 04:49:07PM +0100, Danilo Krummrich wrote:
> >
> >> // Initialize the `data` initializer within the memory pointed
> >> // to by `raw_data`.
> >> unsafe { data.__pinned_init(raw_data) }.inspect_err(|_| {
> >
> >> So, essentially the driver passes an initializer of its private data and we
> >> "write" this initializer into the extra memory allocated with
> >> _fwctl_alloc_device().
> >
> > This all seems like the right way to do it!
> >
> > My only remark it that it still doesn't give an opportunity to call a
> > function between init and register.
>
> You would, from a driver side it would look like this:
>
> #[pin_data]
> struct MyDriver {
> #[pin]
> _reg: Devres<fwctl::Registration>,
> fwctl: ARef<fwctl::Device>,
> }
>
> #[pin_data]
> struct FwctlData {
> #[pin]
> foo: Mutex<Foo>,
> ...,
> }
>
> impl pci::Driver for MyDriver {
> fn probe(
> pdev: &pci::Device<Core>,
> _info: &Self::IdInfo,
> ) -> impl PinInit<Self, Error> {
> let fwctl = fwctl::Device::new(
> pdev.as_ref(),
> try_pin_init!(FwctlData {
> foo <- mutex_new!(Foo::new()),
> ...,
> }),
> )?;
>
> // Let's do something with the `fwctl::Device` before we
> // register it.
> fwctl.do_stuff();
>
> try_pin_init!(Self {
> // We could omit this and instead provide
> // `fwctl::Registration::register()`, which does only return
> // a `Result` and keeps the `fwct::Device` registered until
> // driver unbind.
> _reg <- fwctl::Registration::new(pdev.as_ref(), fwctl);
> fwctl,
> })
> }
> }
Okay it is fine by me, Zi did you get all this?
Jason
On Wed, 28 Jan 2026 12:39:27 -0400
Jason Gunthorpe <jgg@nvidia.com> wrote:
> On Wed, Jan 28, 2026 at 05:35:04PM +0100, Danilo Krummrich wrote:
> > On Wed Jan 28, 2026 at 4:56 PM CET, Jason Gunthorpe wrote:
> > > On Wed, Jan 28, 2026 at 04:49:07PM +0100, Danilo Krummrich wrote:
> > >
> > >> // Initialize the `data` initializer within the
> > >> memory pointed // to by `raw_data`.
> > >> unsafe { data.__pinned_init(raw_data)
> > >> }.inspect_err(|_| {
> > >
> > >> So, essentially the driver passes an initializer of its private
> > >> data and we "write" this initializer into the extra memory
> > >> allocated with _fwctl_alloc_device().
> > >
> > > This all seems like the right way to do it!
> > >
> > > My only remark it that it still doesn't give an opportunity to call a
> > > function between init and register.
> >
> > You would, from a driver side it would look like this:
> >
> > #[pin_data]
> > struct MyDriver {
> > #[pin]
> > _reg: Devres<fwctl::Registration>,
> > fwctl: ARef<fwctl::Device>,
> > }
> >
> > #[pin_data]
> > struct FwctlData {
> > #[pin]
> > foo: Mutex<Foo>,
> > ...,
> > }
> >
> > impl pci::Driver for MyDriver {
> > fn probe(
> > pdev: &pci::Device<Core>,
> > _info: &Self::IdInfo,
> > ) -> impl PinInit<Self, Error> {
> > let fwctl = fwctl::Device::new(
> > pdev.as_ref(),
> > try_pin_init!(FwctlData {
> > foo <- mutex_new!(Foo::new()),
> > ...,
> > }),
> > )?;
> >
> > // Let's do something with the `fwctl::Device` before
> > we // register it.
> > fwctl.do_stuff();
> >
> > try_pin_init!(Self {
> > // We could omit this and instead provide
> > // `fwctl::Registration::register()`, which does
> > only return // a `Result` and keeps the `fwct::Device` registered until
> > // driver unbind.
> > _reg <- fwctl::Registration::new(pdev.as_ref(),
> > fwctl); fwctl,
> > })
> > }
> > }
>
> Okay it is fine by me, Zi did you get all this?
>
Yes. These will be addressed in the next re-spin.
> Jason
On Thu Jan 22, 2026 at 8:42 PM GMT, Zhi Wang wrote:
> Introduce 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.
>
> Cc: Danilo Krummrich <dakr@kernel.org>
> Cc: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Zhi Wang <zhiw@nvidia.com>
> ---
> drivers/fwctl/Kconfig | 12 +
> include/uapi/fwctl/fwctl.h | 1 +
> rust/bindings/bindings_helper.h | 1 +
> rust/helpers/fwctl.c | 17 ++
> rust/helpers/helpers.c | 3 +-
> rust/kernel/fwctl.rs | 456 ++++++++++++++++++++++++++++++++
> rust/kernel/lib.rs | 2 +
> 7 files changed, 491 insertions(+), 1 deletion(-)
> create mode 100644 rust/helpers/fwctl.c
> create mode 100644 rust/kernel/fwctl.rs
>
> diff --git a/drivers/fwctl/Kconfig b/drivers/fwctl/Kconfig
> index b5583b12a011..d8538249f3ae 100644
> --- 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
> tristate "mlx5 ConnectX control fwctl driver"
> diff --git a/include/uapi/fwctl/fwctl.h b/include/uapi/fwctl/fwctl.h
> index 716ac0eee42d..eea1020ad180 100644
> --- 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,
This is UAPI, adding new device type just for testing is not ideal.
> };
>
> /**
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index 9fdf76ca630e..2c50d5bab0cf 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -56,6 +56,7 @@
> #include <linux/fdtable.h>
> #include <linux/file.h>
> #include <linux/firmware.h>
> +#include <linux/fwctl.h>
> #include <linux/interrupt.h>
> #include <linux/fs.h>
> #include <linux/i2c.h>
> diff --git a/rust/helpers/fwctl.c b/rust/helpers/fwctl.c
> new file mode 100644
> index 000000000000..bb4a028e7afb
> --- /dev/null
> +++ b/rust/helpers/fwctl.c
> @@ -0,0 +1,17 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/fwctl.h>
> +
> +#if IS_ENABLED(CONFIG_RUST_FWCTL_ABSTRACTIONS)
> +
> +struct fwctl_device *rust_helper_fwctl_get(struct fwctl_device *fwctl)
Helpers need to have __rust_helper.
> +{
> + return fwctl_get(fwctl);
> +}
> +
> +void rust_helper_fwctl_put(struct fwctl_device *fwctl)
> +{
> + fwctl_put(fwctl);
> +}
> +
> +#endif
> diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
> index 79c72762ad9c..19a505473bef 100644
> --- a/rust/helpers/helpers.c
> +++ b/rust/helpers/helpers.c
> @@ -27,8 +27,9 @@
> #include "dma.c"
> #include "drm.c"
> #include "err.c"
> -#include "irq.c"
> #include "fs.c"
> +#include "fwctl.c"
> +#include "irq.c"
> #include "io.c"
> #include "jump_label.c"
> #include "kunit.c"
> diff --git a/rust/kernel/fwctl.rs b/rust/kernel/fwctl.rs
> new file mode 100644
> index 000000000000..4065c948784d
> --- /dev/null
> +++ b/rust/kernel/fwctl.rs
> @@ -0,0 +1,456 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +//! Abstractions for the fwctl.
> +//!
> +//! This module provides bindings for working with fwctl devices in kernel modules.
> +//!
> +//! C header: [`include/linux/fwctl.h`]
> +
> +use crate::{
> + bindings,
> + container_of,
> + device,
> + devres::Devres,
> + prelude::*,
> + types::{
> + ARef,
> + Opaque, //
> + }, //
> +};
> +use core::{
> + marker::PhantomData,
> + ptr::NonNull,
> + slice, //
> +};
> +
> +/// 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,
Does this need to be present? I took a look at the C side, this isn't used at
all. It'll be a bug if `Operations` impl uses this value as their `DEVICE_TYPE`.
Best,
Gary
> + /// 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,
> +}
> +
> +impl From<DeviceType> for u32 {
> + fn from(device_type: DeviceType) -> Self {
> + device_type as u32
> + }
> +}
On Mon, 26 Jan 2026 17:48:09 +0000
"Gary Guo" <gary@garyguo.net> wrote:
> On Thu Jan 22, 2026 at 8:42 PM GMT, Zhi Wang wrote:
Many thanks for the comments. Will integrate them in the next re-spin.
Z.
> > Introduce 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.
> >
> > Cc: Danilo Krummrich <dakr@kernel.org>
> > Cc: Jason Gunthorpe <jgg@nvidia.com>
> > Signed-off-by: Zhi Wang <zhiw@nvidia.com>
> > ---
> > drivers/fwctl/Kconfig | 12 +
> > include/uapi/fwctl/fwctl.h | 1 +
> > rust/bindings/bindings_helper.h | 1 +
> > rust/helpers/fwctl.c | 17 ++
> > rust/helpers/helpers.c | 3 +-
> > rust/kernel/fwctl.rs | 456 ++++++++++++++++++++++++++++++++
> > rust/kernel/lib.rs | 2 +
> > 7 files changed, 491 insertions(+), 1 deletion(-)
> > create mode 100644 rust/helpers/fwctl.c
> > create mode 100644 rust/kernel/fwctl.rs
> >
> > diff --git a/drivers/fwctl/Kconfig b/drivers/fwctl/Kconfig
> > index b5583b12a011..d8538249f3ae 100644
> > --- 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
> > tristate "mlx5 ConnectX control fwctl driver"
> > diff --git a/include/uapi/fwctl/fwctl.h b/include/uapi/fwctl/fwctl.h
> > index 716ac0eee42d..eea1020ad180 100644
> > --- 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,
>
> This is UAPI, adding new device type just for testing is not ideal.
>
> > };
> >
> > /**
> > diff --git a/rust/bindings/bindings_helper.h
> > b/rust/bindings/bindings_helper.h index 9fdf76ca630e..2c50d5bab0cf
> > 100644 --- a/rust/bindings/bindings_helper.h
> > +++ b/rust/bindings/bindings_helper.h
> > @@ -56,6 +56,7 @@
> > #include <linux/fdtable.h>
> > #include <linux/file.h>
> > #include <linux/firmware.h>
> > +#include <linux/fwctl.h>
> > #include <linux/interrupt.h>
> > #include <linux/fs.h>
> > #include <linux/i2c.h>
> > diff --git a/rust/helpers/fwctl.c b/rust/helpers/fwctl.c
> > new file mode 100644
> > index 000000000000..bb4a028e7afb
> > --- /dev/null
> > +++ b/rust/helpers/fwctl.c
> > @@ -0,0 +1,17 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +#include <linux/fwctl.h>
> > +
> > +#if IS_ENABLED(CONFIG_RUST_FWCTL_ABSTRACTIONS)
> > +
> > +struct fwctl_device *rust_helper_fwctl_get(struct fwctl_device
> > *fwctl)
>
> Helpers need to have __rust_helper.
>
> > +{
> > + return fwctl_get(fwctl);
> > +}
> > +
> > +void rust_helper_fwctl_put(struct fwctl_device *fwctl)
> > +{
> > + fwctl_put(fwctl);
> > +}
> > +
> > +#endif
> > diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
> > index 79c72762ad9c..19a505473bef 100644
> > --- a/rust/helpers/helpers.c
> > +++ b/rust/helpers/helpers.c
> > @@ -27,8 +27,9 @@
> > #include "dma.c"
> > #include "drm.c"
> > #include "err.c"
> > -#include "irq.c"
> > #include "fs.c"
> > +#include "fwctl.c"
> > +#include "irq.c"
> > #include "io.c"
> > #include "jump_label.c"
> > #include "kunit.c"
> > diff --git a/rust/kernel/fwctl.rs b/rust/kernel/fwctl.rs
> > new file mode 100644
> > index 000000000000..4065c948784d
> > --- /dev/null
> > +++ b/rust/kernel/fwctl.rs
> > @@ -0,0 +1,456 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +
> > +//! Abstractions for the fwctl.
> > +//!
> > +//! This module provides bindings for working with fwctl devices in
> > kernel modules. +//!
> > +//! C header: [`include/linux/fwctl.h`]
> > +
> > +use crate::{
> > + bindings,
> > + container_of,
> > + device,
> > + devres::Devres,
> > + prelude::*,
> > + types::{
> > + ARef,
> > + Opaque, //
> > + }, //
> > +};
> > +use core::{
> > + marker::PhantomData,
> > + ptr::NonNull,
> > + slice, //
> > +};
> > +
> > +/// 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,
>
> Does this need to be present? I took a look at the C side, this isn't
> used at all. It'll be a bug if `Operations` impl uses this value as
> their `DEVICE_TYPE`.
>
> Best,
> Gary
>
> > + /// 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, +}
> > +
> > +impl From<DeviceType> for u32 {
> > + fn from(device_type: DeviceType) -> Self {
> > + device_type as u32
> > + }
> > +}
>
On Thu, Jan 22, 2026 at 10:42:30PM +0200, Zhi Wang wrote:
> + /// Called when a userspace RPC request is received.
> + fn fw_rpc(
> + uctx: &mut UserCtx<Self::UserCtx>,
> + scope: u32,
> + rpc_in: &mut [u8],
> + out_len: *mut usize,
> + ) -> Result<Option<KVec<u8>>, Error>;
Exposing a raw pointer in the trait API means drivers that want to "reuse input
buffer" need to write unsafe code right?
unsafe { *out_len = ... };
I believe the unsafe code should be confined to the abstraction layer instead,
not in the driver.
How about using using an enum return type instead to properly wrap inplace
versus driver allocated outputs?
pub enum RpcOutput {
Allocated(KVec<u8>),
InPlace(usize),
}
fn fw_rpc(
uctx: &mut UserCtx<Self::UserCtx>,
scope: u32,
rpc_in: &mut [u8],
) -> Result<RpcOutput, Error>;
Then the abstraction handles the pointer write:
match T::fw_rpc(ctx, scope, rpc_in_slice) {
Ok(RpcOutput::Allocated(kvec)) => {
...
}
Ok(RpcOutput::InPlace(len)) => {
unsafe { *out_len = len };
...
}
}
fw_rpc() as a bonus also gets 1 less function parameter and cleaner return
signature.
--
Joel Fernandes
On Thu, 22 Jan 2026 16:17:55 -0500
Joel Fernandes <joelagnelf@nvidia.com> wrote:
> On Thu, Jan 22, 2026 at 10:42:30PM +0200, Zhi Wang wrote:
> > + /// Called when a userspace RPC request is received.
> > + fn fw_rpc(
> > + uctx: &mut UserCtx<Self::UserCtx>,
> > + scope: u32,
> > + rpc_in: &mut [u8],
> > + out_len: *mut usize,
> > + ) -> Result<Option<KVec<u8>>, Error>;
>
> Exposing a raw pointer in the trait API means drivers that want to
> "reuse input buffer" need to write unsafe code right?
>
> unsafe { *out_len = ... };
>
> I believe the unsafe code should be confined to the abstraction layer
> instead, not in the driver.
>
> How about using using an enum return type instead to properly wrap
> inplace versus driver allocated outputs?
>
> pub enum RpcOutput {
> Allocated(KVec<u8>),
> InPlace(usize),
> }
>
> fn fw_rpc(
> uctx: &mut UserCtx<Self::UserCtx>,
> scope: u32,
> rpc_in: &mut [u8],
> ) -> Result<RpcOutput, Error>;
>
> Then the abstraction handles the pointer write:
>
> match T::fw_rpc(ctx, scope, rpc_in_slice) {
> Ok(RpcOutput::Allocated(kvec)) => {
> ...
> }
> Ok(RpcOutput::InPlace(len)) => {
> unsafe { *out_len = len };
> ...
> }
> }
>
> fw_rpc() as a bonus also gets 1 less function parameter and cleaner
> return signature.
>
This is a good idea. Noted. :)
> --
> Joel Fernandes
>
© 2016 - 2026 Red Hat, Inc.