* [PATCH v2 0/2] rust: introduce abstractions for fwctl
@ 2026-01-22 20:42 Zhi Wang
2026-01-22 20:42 ` [PATCH v2 1/2] " Zhi Wang
` (2 more replies)
0 siblings, 3 replies; 33+ messages in thread
From: Zhi Wang @ 2026-01-22 20:42 UTC (permalink / raw)
To: rust-for-linux, linux-pci, linux-kernel
Cc: dakr, aliceryhl, bhelgaas, kwilczynski, ojeda, alex.gaynor,
boqun.feng, gary, bjorn3_gh, lossin, a.hindborg, tmgross,
markus.probst, helgaas, cjia, smitra, ankita, aniketa, kwankhede,
targupta, acourbot, joelagnelf, jhubbard, zhiwang, daniel.almeida,
Zhi Wang
In the NVIDIA vGPU RFC [1], the vGPU type blobs must be provided to the GSP
before userspace can enumerate available vGPU types and create vGPU
instances. The original design relied on the firmware loading interface,
but fwctl is a more natural fit for this use case, as it is designed for
uploading configuration or firmware data required before the device becomes
operational.
This patch introduces a Rust abstraction over the fwctl subsystem,
providing safe and idiomatic bindings.
The new `fwctl` module allows Rust drivers to integrate with the existing
C-side fwctl core through a typed trait interface. It provides:
- `Operations` trait — defines driver-specific operations such as
`open_uctx()`, `close_uctx()`, `info()`, and `fw_rpc()`.
Each Rust driver implements this trait to describe its own per-FD
user-context behavior and RPC handling.
- `UserCtx<T>` — a generic wrapper around `struct fwctl_uctx`
embedding driver-specific context data, providing safe conversion
from raw C pointers and access to the parent device.
- `Registration<T>` — safe registration and automatic unregistration
of `struct fwctl_device` objects using the kernel's device model.
- `VTable<T>` — a static vtable bridging C callbacks and Rust
trait methods, ensuring type safety across the FFI boundary.
`rust/kernel/lib.rs` is updated to conditionally include this module
under `CONFIG_FWCTL`.
The repo with the patches can be found at [2].
v2:
- Don't open fwctl_put(). Add a rust helper. (Jason/Danilo)
- Wrap Registration with Devres to guarantee proper lifetime management.
(Jason/Danilo)
- Rename FwctlOps to Operations, FwctlUserCtx to UserCtx, FwctlDevice to
Device. (Danilo)
- Use fwctl::DeviceType enum instead of raw u32 for DEVICE_TYPE. (Danilo)
- Change fwctl_uctx field in UserCtx to Opaque<bindings::fwctl_uctx> and
make it private. (Danilo)
- Provide Deref and DerefMut implementations for UserCtx::uctx. (Danilo)
- Add UserCtx::parent_device_from_raw() to simplify parent device access.
- Use cast() and cast_mut() instead of manual pointer casts. (Danilo)
- Implement AlwaysRefCounted for Device and use ARef<Device> in
Registration. (Danilo)
- Add rust_helper_fwctl_get() for reference counting.
- Improve safety comments for slice::from_raw_parts_mut() in
fw_rpc_callback. (Danilo)
- Convert imports to vertical style.
- Fix all clippy warnings.
v1:
- Initial submission introducing fwctl Rust abstractions.
[1] https://lore.kernel.org/all/20250903221111.3866249-1-zhiw@nvidia.com/
[2] https://github.com/zhiwang-nvidia/driver-core/tree/fwctl-rust-abstraction-v2
Zhi Wang (2):
rust: introduce abstractions for fwctl
samples: rust: fwctl: add sample code for fwctl
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 +
samples/rust/Kconfig | 11 +
samples/rust/Makefile | 1 +
samples/rust/rust_driver_fwctl.rs | 136 +++++++++
10 files changed, 639 insertions(+), 1 deletion(-)
create mode 100644 rust/helpers/fwctl.c
create mode 100644 rust/kernel/fwctl.rs
create mode 100644 samples/rust/rust_driver_fwctl.rs
--
2.51.0
^ permalink raw reply [flat|nested] 33+ messages in thread* [PATCH v2 1/2] rust: introduce abstractions for fwctl 2026-01-22 20:42 [PATCH v2 0/2] rust: introduce abstractions for fwctl Zhi Wang @ 2026-01-22 20:42 ` Zhi Wang 2026-01-22 21:17 ` Joel Fernandes ` (2 more replies) 2026-01-22 20:42 ` [PATCH v2 2/2] samples: rust: fwctl: add sample code " Zhi Wang 2026-01-22 21:32 ` [PATCH v2 0/2] rust: introduce abstractions " Danilo Krummrich 2 siblings, 3 replies; 33+ messages in thread From: Zhi Wang @ 2026-01-22 20:42 UTC (permalink / raw) To: rust-for-linux, linux-pci, linux-kernel Cc: dakr, aliceryhl, bhelgaas, kwilczynski, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh, lossin, a.hindborg, tmgross, markus.probst, helgaas, cjia, smitra, ankita, aniketa, kwankhede, targupta, acourbot, joelagnelf, jhubbard, zhiwang, daniel.almeida, Zhi Wang, Jason Gunthorpe 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.0 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH v2 1/2] rust: introduce abstractions for fwctl 2026-01-22 20:42 ` [PATCH v2 1/2] " Zhi Wang @ 2026-01-22 21:17 ` Joel Fernandes 2026-01-23 10:25 ` Zhi Wang 2026-01-26 17:48 ` Gary Guo 2026-01-26 18:19 ` Jason Gunthorpe 2 siblings, 1 reply; 33+ messages in thread From: Joel Fernandes @ 2026-01-22 21:17 UTC (permalink / raw) To: Zhi Wang; +Cc: rust-for-linux, linux-pci, linux-kernel, dakr, aliceryhl, jgg 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 1/2] rust: introduce abstractions for fwctl 2026-01-22 21:17 ` Joel Fernandes @ 2026-01-23 10:25 ` Zhi Wang 0 siblings, 0 replies; 33+ messages in thread From: Zhi Wang @ 2026-01-23 10:25 UTC (permalink / raw) To: Joel Fernandes Cc: rust-for-linux, linux-pci, linux-kernel, dakr, aliceryhl, jgg 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 > ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 1/2] rust: introduce abstractions for fwctl 2026-01-22 20:42 ` [PATCH v2 1/2] " Zhi Wang 2026-01-22 21:17 ` Joel Fernandes @ 2026-01-26 17:48 ` Gary Guo 2026-01-27 19:59 ` Zhi Wang 2026-01-26 18:19 ` Jason Gunthorpe 2 siblings, 1 reply; 33+ messages in thread From: Gary Guo @ 2026-01-26 17:48 UTC (permalink / raw) To: Zhi Wang, rust-for-linux, linux-pci, linux-kernel Cc: dakr, aliceryhl, bhelgaas, kwilczynski, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh, lossin, a.hindborg, tmgross, markus.probst, helgaas, cjia, smitra, ankita, aniketa, kwankhede, targupta, acourbot, joelagnelf, jhubbard, zhiwang, daniel.almeida, Jason Gunthorpe 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 > + } > +} ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 1/2] rust: introduce abstractions for fwctl 2026-01-26 17:48 ` Gary Guo @ 2026-01-27 19:59 ` Zhi Wang 0 siblings, 0 replies; 33+ messages in thread From: Zhi Wang @ 2026-01-27 19:59 UTC (permalink / raw) To: Gary Guo Cc: rust-for-linux, linux-pci, linux-kernel, dakr, aliceryhl, bhelgaas, kwilczynski, ojeda, alex.gaynor, boqun.feng, bjorn3_gh, lossin, a.hindborg, tmgross, markus.probst, helgaas, cjia, smitra, ankita, aniketa, kwankhede, targupta, acourbot, joelagnelf, jhubbard, zhiwang, daniel.almeida, Jason Gunthorpe 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 > > + } > > +} > ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 1/2] rust: introduce abstractions for fwctl 2026-01-22 20:42 ` [PATCH v2 1/2] " Zhi Wang 2026-01-22 21:17 ` Joel Fernandes 2026-01-26 17:48 ` Gary Guo @ 2026-01-26 18:19 ` Jason Gunthorpe 2026-01-27 19:57 ` Zhi Wang 2 siblings, 1 reply; 33+ messages in thread From: Jason Gunthorpe @ 2026-01-26 18:19 UTC (permalink / raw) To: Zhi Wang Cc: rust-for-linux, linux-pci, linux-kernel, dakr, aliceryhl, bhelgaas, kwilczynski, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh, lossin, a.hindborg, tmgross, markus.probst, helgaas, cjia, smitra, ankita, aniketa, kwankhede, targupta, acourbot, joelagnelf, jhubbard, zhiwang, daniel.almeida 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 1/2] rust: introduce abstractions for fwctl 2026-01-26 18:19 ` Jason Gunthorpe @ 2026-01-27 19:57 ` Zhi Wang 2026-01-27 20:07 ` Danilo Krummrich 2026-01-27 20:09 ` Danilo Krummrich 0 siblings, 2 replies; 33+ messages in thread From: Zhi Wang @ 2026-01-27 19:57 UTC (permalink / raw) To: Jason Gunthorpe Cc: rust-for-linux, linux-pci, linux-kernel, dakr, aliceryhl, bhelgaas, kwilczynski, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh, lossin, a.hindborg, tmgross, markus.probst, helgaas, cjia, smitra, ankita, aniketa, kwankhede, targupta, acourbot, joelagnelf, jhubbard, zhiwang, daniel.almeida 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 > ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 1/2] rust: introduce abstractions for fwctl 2026-01-27 19:57 ` Zhi Wang @ 2026-01-27 20:07 ` Danilo Krummrich 2026-01-28 0:04 ` Jason Gunthorpe 2026-01-28 11:36 ` [PATCH v2 1/2] rust: introduce abstractions for fwctl Zhi Wang 2026-01-27 20:09 ` Danilo Krummrich 1 sibling, 2 replies; 33+ messages in thread From: Danilo Krummrich @ 2026-01-27 20:07 UTC (permalink / raw) To: Zhi Wang Cc: Jason Gunthorpe, rust-for-linux, linux-pci, linux-kernel, aliceryhl, bhelgaas, kwilczynski, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh, lossin, a.hindborg, tmgross, markus.probst, helgaas, cjia, smitra, ankita, aniketa, kwankhede, targupta, acourbot, joelagnelf, jhubbard, zhiwang, daniel.almeida 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. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 1/2] rust: introduce abstractions for fwctl 2026-01-27 20:07 ` Danilo Krummrich @ 2026-01-28 0:04 ` Jason Gunthorpe 2026-01-28 1:21 ` Danilo Krummrich 2026-01-28 11:36 ` [PATCH v2 1/2] rust: introduce abstractions for fwctl Zhi Wang 1 sibling, 1 reply; 33+ messages in thread From: Jason Gunthorpe @ 2026-01-28 0:04 UTC (permalink / raw) To: Danilo Krummrich Cc: Zhi Wang, rust-for-linux, linux-pci, linux-kernel, aliceryhl, bhelgaas, kwilczynski, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh, lossin, a.hindborg, tmgross, markus.probst, helgaas, cjia, smitra, ankita, aniketa, kwankhede, targupta, acourbot, joelagnelf, jhubbard, zhiwang, daniel.almeida 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 1/2] rust: introduce abstractions for fwctl 2026-01-28 0:04 ` Jason Gunthorpe @ 2026-01-28 1:21 ` Danilo Krummrich 2026-01-28 13:20 ` [PATCH v2 1/2] rust: introduce abstractions for fwctlg Jason Gunthorpe 0 siblings, 1 reply; 33+ messages in thread From: Danilo Krummrich @ 2026-01-28 1:21 UTC (permalink / raw) To: Jason Gunthorpe Cc: Zhi Wang, rust-for-linux, linux-pci, linux-kernel, aliceryhl, bhelgaas, kwilczynski, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh, lossin, a.hindborg, tmgross, markus.probst, helgaas, cjia, smitra, ankita, aniketa, kwankhede, targupta, acourbot, joelagnelf, jhubbard, zhiwang, daniel.almeida 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. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 1/2] rust: introduce abstractions for fwctlg 2026-01-28 1:21 ` Danilo Krummrich @ 2026-01-28 13:20 ` Jason Gunthorpe 2026-01-28 14:01 ` Danilo Krummrich 0 siblings, 1 reply; 33+ messages in thread From: Jason Gunthorpe @ 2026-01-28 13:20 UTC (permalink / raw) To: Danilo Krummrich Cc: Zhi Wang, rust-for-linux, linux-pci, linux-kernel, aliceryhl, bhelgaas, kwilczynski, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh, lossin, a.hindborg, tmgross, markus.probst, helgaas, cjia, smitra, ankita, aniketa, kwankhede, targupta, acourbot, joelagnelf, jhubbard, zhiwang, daniel.almeida 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 1/2] rust: introduce abstractions for fwctlg 2026-01-28 13:20 ` [PATCH v2 1/2] rust: introduce abstractions for fwctlg Jason Gunthorpe @ 2026-01-28 14:01 ` Danilo Krummrich 2026-01-28 14:59 ` Jason Gunthorpe 0 siblings, 1 reply; 33+ messages in thread From: Danilo Krummrich @ 2026-01-28 14:01 UTC (permalink / raw) To: Jason Gunthorpe Cc: Zhi Wang, rust-for-linux, linux-pci, linux-kernel, aliceryhl, bhelgaas, kwilczynski, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh, lossin, a.hindborg, tmgross, markus.probst, helgaas, cjia, smitra, ankita, aniketa, kwankhede, targupta, acourbot, joelagnelf, jhubbard, zhiwang, daniel.almeida 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 1/2] rust: introduce abstractions for fwctlg 2026-01-28 14:01 ` Danilo Krummrich @ 2026-01-28 14:59 ` Jason Gunthorpe 2026-01-28 15:49 ` Danilo Krummrich 0 siblings, 1 reply; 33+ messages in thread From: Jason Gunthorpe @ 2026-01-28 14:59 UTC (permalink / raw) To: Danilo Krummrich Cc: Zhi Wang, rust-for-linux, linux-pci, linux-kernel, aliceryhl, bhelgaas, kwilczynski, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh, lossin, a.hindborg, tmgross, markus.probst, helgaas, cjia, smitra, ankita, aniketa, kwankhede, targupta, acourbot, joelagnelf, jhubbard, zhiwang, daniel.almeida 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 1/2] rust: introduce abstractions for fwctlg 2026-01-28 14:59 ` Jason Gunthorpe @ 2026-01-28 15:49 ` Danilo Krummrich 2026-01-28 15:56 ` Jason Gunthorpe 0 siblings, 1 reply; 33+ messages in thread From: Danilo Krummrich @ 2026-01-28 15:49 UTC (permalink / raw) To: Jason Gunthorpe Cc: Zhi Wang, rust-for-linux, linux-pci, linux-kernel, aliceryhl, bhelgaas, kwilczynski, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh, lossin, a.hindborg, tmgross, markus.probst, helgaas, cjia, smitra, ankita, aniketa, kwankhede, targupta, acourbot, joelagnelf, jhubbard, zhiwang, daniel.almeida 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(). ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 1/2] rust: introduce abstractions for fwctlg 2026-01-28 15:49 ` Danilo Krummrich @ 2026-01-28 15:56 ` Jason Gunthorpe 2026-01-28 16:35 ` Danilo Krummrich 0 siblings, 1 reply; 33+ messages in thread From: Jason Gunthorpe @ 2026-01-28 15:56 UTC (permalink / raw) To: Danilo Krummrich Cc: Zhi Wang, rust-for-linux, linux-pci, linux-kernel, aliceryhl, bhelgaas, kwilczynski, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh, lossin, a.hindborg, tmgross, markus.probst, helgaas, cjia, smitra, ankita, aniketa, kwankhede, targupta, acourbot, joelagnelf, jhubbard, zhiwang, daniel.almeida 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 1/2] rust: introduce abstractions for fwctlg 2026-01-28 15:56 ` Jason Gunthorpe @ 2026-01-28 16:35 ` Danilo Krummrich 2026-01-28 16:39 ` Jason Gunthorpe 2026-01-28 17:30 ` Zhi Wang 0 siblings, 2 replies; 33+ messages in thread From: Danilo Krummrich @ 2026-01-28 16:35 UTC (permalink / raw) To: Jason Gunthorpe Cc: Zhi Wang, rust-for-linux, linux-pci, linux-kernel, aliceryhl, bhelgaas, kwilczynski, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh, lossin, a.hindborg, tmgross, markus.probst, helgaas, cjia, smitra, ankita, aniketa, kwankhede, targupta, acourbot, joelagnelf, jhubbard, zhiwang, daniel.almeida 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. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 1/2] rust: introduce abstractions for fwctlg 2026-01-28 16:35 ` Danilo Krummrich @ 2026-01-28 16:39 ` Jason Gunthorpe 2026-01-28 17:26 ` Zhi Wang 2026-01-28 17:30 ` Zhi Wang 1 sibling, 1 reply; 33+ messages in thread From: Jason Gunthorpe @ 2026-01-28 16:39 UTC (permalink / raw) To: Danilo Krummrich Cc: Zhi Wang, rust-for-linux, linux-pci, linux-kernel, aliceryhl, bhelgaas, kwilczynski, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh, lossin, a.hindborg, tmgross, markus.probst, helgaas, cjia, smitra, ankita, aniketa, kwankhede, targupta, acourbot, joelagnelf, jhubbard, zhiwang, daniel.almeida 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 1/2] rust: introduce abstractions for fwctlg 2026-01-28 16:39 ` Jason Gunthorpe @ 2026-01-28 17:26 ` Zhi Wang 0 siblings, 0 replies; 33+ messages in thread From: Zhi Wang @ 2026-01-28 17:26 UTC (permalink / raw) To: Jason Gunthorpe Cc: Danilo Krummrich, rust-for-linux, linux-pci, linux-kernel, aliceryhl, bhelgaas, kwilczynski, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh, lossin, a.hindborg, tmgross, markus.probst, helgaas, cjia, smitra, ankita, aniketa, kwankhede, targupta, acourbot, joelagnelf, jhubbard, zhiwang, daniel.almeida 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 1/2] rust: introduce abstractions for fwctlg 2026-01-28 16:35 ` Danilo Krummrich 2026-01-28 16:39 ` Jason Gunthorpe @ 2026-01-28 17:30 ` Zhi Wang 2026-01-28 17:39 ` Jason Gunthorpe 2026-01-28 17:40 ` Danilo Krummrich 1 sibling, 2 replies; 33+ messages in thread From: Zhi Wang @ 2026-01-28 17:30 UTC (permalink / raw) To: Danilo Krummrich Cc: Jason Gunthorpe, rust-for-linux, linux-pci, linux-kernel, aliceryhl, bhelgaas, kwilczynski, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh, lossin, a.hindborg, tmgross, markus.probst, helgaas, cjia, smitra, ankita, aniketa, kwankhede, targupta, acourbot, joelagnelf, jhubbard, zhiwang, daniel.almeida 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. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 1/2] rust: introduce abstractions for fwctlg 2026-01-28 17:30 ` Zhi Wang @ 2026-01-28 17:39 ` Jason Gunthorpe 2026-01-28 17:40 ` Danilo Krummrich 1 sibling, 0 replies; 33+ messages in thread From: Jason Gunthorpe @ 2026-01-28 17:39 UTC (permalink / raw) To: Zhi Wang Cc: Danilo Krummrich, rust-for-linux, linux-pci, linux-kernel, aliceryhl, bhelgaas, kwilczynski, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh, lossin, a.hindborg, tmgross, markus.probst, helgaas, cjia, smitra, ankita, aniketa, kwankhede, targupta, acourbot, joelagnelf, jhubbard, zhiwang, daniel.almeida 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 1/2] rust: introduce abstractions for fwctlg 2026-01-28 17:30 ` Zhi Wang 2026-01-28 17:39 ` Jason Gunthorpe @ 2026-01-28 17:40 ` Danilo Krummrich 1 sibling, 0 replies; 33+ messages in thread From: Danilo Krummrich @ 2026-01-28 17:40 UTC (permalink / raw) To: Zhi Wang Cc: Jason Gunthorpe, rust-for-linux, linux-pci, linux-kernel, aliceryhl, bhelgaas, kwilczynski, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh, lossin, a.hindborg, tmgross, markus.probst, helgaas, cjia, smitra, ankita, aniketa, kwankhede, targupta, acourbot, joelagnelf, jhubbard, zhiwang, daniel.almeida 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). ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 1/2] rust: introduce abstractions for fwctl 2026-01-27 20:07 ` Danilo Krummrich 2026-01-28 0:04 ` Jason Gunthorpe @ 2026-01-28 11:36 ` Zhi Wang 2026-01-28 11:41 ` Danilo Krummrich 1 sibling, 1 reply; 33+ messages in thread From: Zhi Wang @ 2026-01-28 11:36 UTC (permalink / raw) To: Danilo Krummrich Cc: Jason Gunthorpe, rust-for-linux, linux-pci, linux-kernel, aliceryhl, bhelgaas, kwilczynski, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh, lossin, a.hindborg, tmgross, markus.probst, helgaas, cjia, smitra, ankita, aniketa, kwankhede, targupta, acourbot, joelagnelf, jhubbard, zhiwang, daniel.almeida 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. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 1/2] rust: introduce abstractions for fwctl 2026-01-28 11:36 ` [PATCH v2 1/2] rust: introduce abstractions for fwctl Zhi Wang @ 2026-01-28 11:41 ` Danilo Krummrich 0 siblings, 0 replies; 33+ messages in thread From: Danilo Krummrich @ 2026-01-28 11:41 UTC (permalink / raw) To: Zhi Wang Cc: Jason Gunthorpe, rust-for-linux, linux-pci, linux-kernel, aliceryhl, bhelgaas, kwilczynski, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh, lossin, a.hindborg, tmgross, markus.probst, helgaas, cjia, smitra, ankita, aniketa, kwankhede, targupta, acourbot, joelagnelf, jhubbard, zhiwang, daniel.almeida 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 1/2] rust: introduce abstractions for fwctl 2026-01-27 19:57 ` Zhi Wang 2026-01-27 20:07 ` Danilo Krummrich @ 2026-01-27 20:09 ` Danilo Krummrich 1 sibling, 0 replies; 33+ messages in thread From: Danilo Krummrich @ 2026-01-27 20:09 UTC (permalink / raw) To: Zhi Wang Cc: Jason Gunthorpe, rust-for-linux, linux-pci, linux-kernel, aliceryhl, bhelgaas, kwilczynski, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh, lossin, a.hindborg, tmgross, markus.probst, helgaas, cjia, smitra, ankita, aniketa, kwankhede, targupta, acourbot, joelagnelf, jhubbard, zhiwang, daniel.almeida 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. :) ^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH v2 2/2] samples: rust: fwctl: add sample code for fwctl 2026-01-22 20:42 [PATCH v2 0/2] rust: introduce abstractions for fwctl Zhi Wang 2026-01-22 20:42 ` [PATCH v2 1/2] " Zhi Wang @ 2026-01-22 20:42 ` Zhi Wang 2026-01-22 20:58 ` Jason Gunthorpe 2026-01-26 17:59 ` Gary Guo 2026-01-22 21:32 ` [PATCH v2 0/2] rust: introduce abstractions " Danilo Krummrich 2 siblings, 2 replies; 33+ messages in thread From: Zhi Wang @ 2026-01-22 20:42 UTC (permalink / raw) To: rust-for-linux, linux-pci, linux-kernel Cc: dakr, aliceryhl, bhelgaas, kwilczynski, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh, lossin, a.hindborg, tmgross, markus.probst, helgaas, cjia, smitra, ankita, aniketa, kwankhede, targupta, acourbot, joelagnelf, jhubbard, zhiwang, daniel.almeida, Zhi Wang, Jason Gunthorpe This patch adds a sample Rust driver demonstrating the usage of the fwctl Rust abstractions. Add sample code for creating a FwCtl device, getting device info and issuing an RPC. Cc: Danilo Krummrich <dakr@kernel.org> Cc: Jason Gunthorpe <jgg@nvidia.com> Signed-off-by: Zhi Wang <zhiw@nvidia.com> --- samples/rust/Kconfig | 11 +++ samples/rust/Makefile | 1 + samples/rust/rust_driver_fwctl.rs | 136 ++++++++++++++++++++++++++++++ 3 files changed, 148 insertions(+) create mode 100644 samples/rust/rust_driver_fwctl.rs diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig index c49ab9106345..3d0b223caa89 100644 --- a/samples/rust/Kconfig +++ b/samples/rust/Kconfig @@ -172,6 +172,17 @@ config SAMPLE_RUST_SOC If unsure, say N. +config SAMPLE_RUST_DRIVER_FWCTL + tristate "Fwctl Driver" + depends on FWCTL + help + This option builds the Rust Fwctl driver sample. + + To compile this as a module, choose M here: + the module will be called rust_driver_fwctl. + + If unsure, say N. + config SAMPLE_RUST_HOSTPROGS bool "Host programs" help diff --git a/samples/rust/Makefile b/samples/rust/Makefile index 6c0aaa58cccc..6f6030e64727 100644 --- a/samples/rust/Makefile +++ b/samples/rust/Makefile @@ -14,6 +14,7 @@ obj-$(CONFIG_SAMPLE_RUST_DRIVER_PLATFORM) += rust_driver_platform.o obj-$(CONFIG_SAMPLE_RUST_DRIVER_USB) += rust_driver_usb.o obj-$(CONFIG_SAMPLE_RUST_DRIVER_FAUX) += rust_driver_faux.o obj-$(CONFIG_SAMPLE_RUST_DRIVER_AUXILIARY) += rust_driver_auxiliary.o +obj-$(CONFIG_SAMPLE_RUST_DRIVER_FWCTL) += rust_driver_fwctl.o obj-$(CONFIG_SAMPLE_RUST_CONFIGFS) += rust_configfs.o obj-$(CONFIG_SAMPLE_RUST_SOC) += rust_soc.o diff --git a/samples/rust/rust_driver_fwctl.rs b/samples/rust/rust_driver_fwctl.rs new file mode 100644 index 000000000000..ac5f979fd73b --- /dev/null +++ b/samples/rust/rust_driver_fwctl.rs @@ -0,0 +1,136 @@ +// SPDX-License-Identifier: GPL-2.0 + +//! Rust fwctl API test (based on QEMU's `pci-testdev`). +//! +//! To make this driver probe, QEMU must be run with `-device pci-testdev`. + +use kernel::{ + bindings, + device, + device::Core, + devres::Devres, + fwctl, + pci, + prelude::*, + sync::aref::ARef, + types, +}; + +struct FwctlSampleUserCtx { + _drvdata: u32, +} + +struct FwctlSampleOps; + +impl fwctl::Operations for FwctlSampleOps { + type UserCtx = FwctlSampleUserCtx; + + const DEVICE_TYPE: fwctl::DeviceType = fwctl::DeviceType::RustFwctlTest; + + fn open( + fwctl_uctx: &types::Opaque<bindings::fwctl_uctx> + ) -> Result<impl PinInit<Self::UserCtx, Error>, Error> { + let dev = fwctl::UserCtx::<Self::UserCtx>::parent_device_from_raw(fwctl_uctx); + + dev_info!(dev, "fwctl test driver: open_uctx()"); + + // Return an initializer for the user context. + // The framework will initialize this in-place in the C-allocated memory. + Ok(try_init!(FwctlSampleUserCtx { + _drvdata: 0, + })) + } + + fn close(uctx: &mut fwctl::UserCtx<FwctlSampleUserCtx>) { + let dev = uctx.get_parent_device(); + + dev_info!(dev, "fwctl test driver: close_uctx()"); + } + + fn info(uctx: &mut fwctl::UserCtx<FwctlSampleUserCtx>) -> Result<KVec<u8>, Error> { + let dev = uctx.get_parent_device(); + + dev_info!(dev, "fwctl test driver: info()"); + + let mut infobuf = KVec::<u8>::new(); + infobuf.push(0xef, GFP_KERNEL)?; + infobuf.push(0xbe, GFP_KERNEL)?; + infobuf.push(0xad, GFP_KERNEL)?; + infobuf.push(0xde, GFP_KERNEL)?; + + Ok(infobuf) + } + + fn fw_rpc( + uctx: &mut fwctl::UserCtx<FwctlSampleUserCtx>, + scope: u32, + rpc_in: &mut [u8], + _out_len: *mut usize, + ) -> Result<Option<KVec<u8>>, Error> { + let dev = uctx.get_parent_device(); + + dev_info!(dev, "fwctl test driver: fw_rpc() scope {}", scope); + + if rpc_in.len() != 4 { + return Err(EINVAL); + } + + dev_info!( + dev, + "fwctl test driver: inbuf len{} bytes[0-3] {:x} {:x} {:x} {:x}", + rpc_in.len(), + rpc_in[0], + rpc_in[1], + rpc_in[2], + rpc_in[3] + ); + + let mut outbuf = KVec::<u8>::new(); + outbuf.push(0xef, GFP_KERNEL)?; + outbuf.push(0xbe, GFP_KERNEL)?; + outbuf.push(0xad, GFP_KERNEL)?; + outbuf.push(0xde, GFP_KERNEL)?; + + Ok(Some(outbuf)) + } +} + +#[pin_data] +struct FwctlSampleDriver { + pdev: ARef<pci::Device>, + #[pin] + fwctl: Devres<fwctl::Registration<FwctlSampleOps>>, +} + +kernel::pci_device_table!( + PCI_TABLE, + MODULE_PCI_TABLE, + <FwctlSampleDriver as pci::Driver>::IdInfo, + [(pci::DeviceId::from_id(pci::Vendor::REDHAT, 0x5), ())] +); + +impl pci::Driver for FwctlSampleDriver { + type IdInfo = (); + const ID_TABLE: pci::IdTable<Self::IdInfo> = &PCI_TABLE; + + fn probe(pdev: &pci::Device<Core>, _info: &Self::IdInfo) -> impl PinInit<Self, Error> { + dev_info!(pdev.as_ref(), "Probe fwctl test driver"); + + // `pdev` is `Device<Core>`, which derefs to `Device<Bound>` during probe. + let pdev_bound: &pci::Device<device::Bound> = pdev; + + try_pin_init!(Self { + pdev: pdev.into(), + fwctl <- fwctl::Registration::<FwctlSampleOps>::new(pdev_bound.as_ref()), + }) + } +} + +kernel::module_pci_driver! { + type: FwctlSampleDriver, + name: "rust_driver_fwctl", + authors: ["Zhi Wang"], + description: "Rust fwctl test", + license: "GPL v2", + imports_ns: ["FWCTL"], +} -- 2.51.0 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH v2 2/2] samples: rust: fwctl: add sample code for fwctl 2026-01-22 20:42 ` [PATCH v2 2/2] samples: rust: fwctl: add sample code " Zhi Wang @ 2026-01-22 20:58 ` Jason Gunthorpe 2026-01-22 21:06 ` Danilo Krummrich 2026-01-26 17:59 ` Gary Guo 1 sibling, 1 reply; 33+ messages in thread From: Jason Gunthorpe @ 2026-01-22 20:58 UTC (permalink / raw) To: Zhi Wang Cc: rust-for-linux, linux-pci, linux-kernel, dakr, aliceryhl, bhelgaas, kwilczynski, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh, lossin, a.hindborg, tmgross, markus.probst, helgaas, cjia, smitra, ankita, aniketa, kwankhede, targupta, acourbot, joelagnelf, jhubbard, zhiwang, daniel.almeida On Thu, Jan 22, 2026 at 10:42:31PM +0200, Zhi Wang wrote: > This patch adds a sample Rust driver demonstrating the usage of the fwctl > Rust abstractions. Add sample code for creating a FwCtl device, getting > device info and issuing an RPC. > > Cc: Danilo Krummrich <dakr@kernel.org> > Cc: Jason Gunthorpe <jgg@nvidia.com> > Signed-off-by: Zhi Wang <zhiw@nvidia.com> > --- > samples/rust/Kconfig | 11 +++ > samples/rust/Makefile | 1 + > samples/rust/rust_driver_fwctl.rs | 136 ++++++++++++++++++++++++++++++ > 3 files changed, 148 insertions(+) > create mode 100644 samples/rust/rust_driver_fwctl.rs Is this normal for Rust? Can we delete it when a real driver is merged? I'm not so keen on sample drivers, I've spent far too much time lately touching sample drivers that nobody ever uses :( Jason ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 2/2] samples: rust: fwctl: add sample code for fwctl 2026-01-22 20:58 ` Jason Gunthorpe @ 2026-01-22 21:06 ` Danilo Krummrich 2026-01-22 21:16 ` John Hubbard 0 siblings, 1 reply; 33+ messages in thread From: Danilo Krummrich @ 2026-01-22 21:06 UTC (permalink / raw) To: Jason Gunthorpe Cc: Zhi Wang, rust-for-linux, linux-pci, linux-kernel, aliceryhl, bhelgaas, kwilczynski, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh, lossin, a.hindborg, tmgross, markus.probst, helgaas, cjia, smitra, ankita, aniketa, kwankhede, targupta, acourbot, joelagnelf, jhubbard, zhiwang, daniel.almeida On Thu Jan 22, 2026 at 9:58 PM CET, Jason Gunthorpe wrote: > On Thu, Jan 22, 2026 at 10:42:31PM +0200, Zhi Wang wrote: >> This patch adds a sample Rust driver demonstrating the usage of the fwctl >> Rust abstractions. Add sample code for creating a FwCtl device, getting >> device info and issuing an RPC. >> >> Cc: Danilo Krummrich <dakr@kernel.org> >> Cc: Jason Gunthorpe <jgg@nvidia.com> >> Signed-off-by: Zhi Wang <zhiw@nvidia.com> >> --- >> samples/rust/Kconfig | 11 +++ >> samples/rust/Makefile | 1 + >> samples/rust/rust_driver_fwctl.rs | 136 ++++++++++++++++++++++++++++++ >> 3 files changed, 148 insertions(+) >> create mode 100644 samples/rust/rust_driver_fwctl.rs > > Is this normal for Rust? Can we delete it when a real driver is > merged? > > I'm not so keen on sample drivers, I've spent far too much time lately > touching sample drivers that nobody ever uses :( I think it is pretty useful for review purposes (at least as long as the first real user is not yet ready) to see how it turns out from a user perspective. It may also be pretty useful for people starting out with Rust in the kernel, as the samples have a very limited scope compared to real drivers. But there is no policy or anything, i.e. no need to keep it. There's also no need to even land it if you prefer not to do so. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 2/2] samples: rust: fwctl: add sample code for fwctl 2026-01-22 21:06 ` Danilo Krummrich @ 2026-01-22 21:16 ` John Hubbard 2026-01-23 10:23 ` Zhi Wang 0 siblings, 1 reply; 33+ messages in thread From: John Hubbard @ 2026-01-22 21:16 UTC (permalink / raw) To: Danilo Krummrich, Jason Gunthorpe Cc: Zhi Wang, rust-for-linux, linux-pci, linux-kernel, aliceryhl, bhelgaas, kwilczynski, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh, lossin, a.hindborg, tmgross, markus.probst, helgaas, cjia, smitra, ankita, aniketa, kwankhede, targupta, acourbot, joelagnelf, zhiwang, daniel.almeida On 1/22/26 1:06 PM, Danilo Krummrich wrote: > On Thu Jan 22, 2026 at 9:58 PM CET, Jason Gunthorpe wrote: >> On Thu, Jan 22, 2026 at 10:42:31PM +0200, Zhi Wang wrote: >>> This patch adds a sample Rust driver demonstrating the usage of the fwctl >>> Rust abstractions. Add sample code for creating a FwCtl device, getting >>> device info and issuing an RPC. >>> >>> Cc: Danilo Krummrich <dakr@kernel.org> >>> Cc: Jason Gunthorpe <jgg@nvidia.com> >>> Signed-off-by: Zhi Wang <zhiw@nvidia.com> >>> --- >>> samples/rust/Kconfig | 11 +++ >>> samples/rust/Makefile | 1 + >>> samples/rust/rust_driver_fwctl.rs | 136 ++++++++++++++++++++++++++++++ >>> 3 files changed, 148 insertions(+) >>> create mode 100644 samples/rust/rust_driver_fwctl.rs >> >> Is this normal for Rust? Can we delete it when a real driver is >> merged? >> >> I'm not so keen on sample drivers, I've spent far too much time lately >> touching sample drivers that nobody ever uses :( > > I think it is pretty useful for review purposes (at least as long as the first > real user is not yet ready) to see how it turns out from a user perspective. > > It may also be pretty useful for people starting out with Rust in the kernel, as > the samples have a very limited scope compared to real drivers. > > But there is no policy or anything, i.e. no need to keep it. There's also no > need to even land it if you prefer not to do so. Zhi, the real code that uses this for vGPU is going to be posted soon-ish, right? So we could use this patch as a review assist, like Danilo mentions, but not merge it, because the real code will suffice to exercise the new fwctl functionality. thanks, -- John Hubbard ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 2/2] samples: rust: fwctl: add sample code for fwctl 2026-01-22 21:16 ` John Hubbard @ 2026-01-23 10:23 ` Zhi Wang 0 siblings, 0 replies; 33+ messages in thread From: Zhi Wang @ 2026-01-23 10:23 UTC (permalink / raw) To: John Hubbard Cc: Danilo Krummrich, Jason Gunthorpe, rust-for-linux, linux-pci, linux-kernel, aliceryhl, bhelgaas, kwilczynski, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh, lossin, a.hindborg, tmgross, markus.probst, helgaas, cjia, smitra, ankita, aniketa, kwankhede, targupta, acourbot, joelagnelf, zhiwang, daniel.almeida On Thu, 22 Jan 2026 13:16:29 -0800 John Hubbard <jhubbard@nvidia.com> wrote: > On 1/22/26 1:06 PM, Danilo Krummrich wrote: > > On Thu Jan 22, 2026 at 9:58 PM CET, Jason Gunthorpe wrote: > >> On Thu, Jan 22, 2026 at 10:42:31PM +0200, Zhi Wang wrote: > >>> This patch adds a sample Rust driver demonstrating the usage of the > >>> fwctl Rust abstractions. Add sample code for creating a FwCtl > >>> device, getting device info and issuing an RPC. > >>> > >>> Cc: Danilo Krummrich <dakr@kernel.org> > >>> Cc: Jason Gunthorpe <jgg@nvidia.com> > >>> Signed-off-by: Zhi Wang <zhiw@nvidia.com> > >>> --- > >>> samples/rust/Kconfig | 11 +++ > >>> samples/rust/Makefile | 1 + > >>> samples/rust/rust_driver_fwctl.rs | 136 > >>> ++++++++++++++++++++++++++++++ 3 files changed, 148 insertions(+) > >>> create mode 100644 samples/rust/rust_driver_fwctl.rs > >> > >> Is this normal for Rust? Can we delete it when a real driver is > >> merged? > >> > >> I'm not so keen on sample drivers, I've spent far too much time lately > >> touching sample drivers that nobody ever uses :( > > > > I think it is pretty useful for review purposes (at least as long as > > the first real user is not yet ready) to see how it turns out from a > > user perspective. > > > > It may also be pretty useful for people starting out with Rust in the > > kernel, as the samples have a very limited scope compared to real > > drivers. > > > > But there is no policy or anything, i.e. no need to keep it. There's > > also no need to even land it if you prefer not to do so. > > Zhi, the real code that uses this for vGPU is going to be posted > soon-ish, right? So we could use this patch as a review assist, like > Danilo mentions, but not merge it, because the real code will suffice to > exercise the new fwctl functionality. > I am OK with any solutions. My motivation of updating this one is because of many changes and improvements in this re-spin comparing to the RFC. As Jason/Danilo gave quite some feedback on RFC (Thanks for the review efforts!), I would like to start the review before the vGPU patches. Z. > thanks, ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 2/2] samples: rust: fwctl: add sample code for fwctl 2026-01-22 20:42 ` [PATCH v2 2/2] samples: rust: fwctl: add sample code " Zhi Wang 2026-01-22 20:58 ` Jason Gunthorpe @ 2026-01-26 17:59 ` Gary Guo 1 sibling, 0 replies; 33+ messages in thread From: Gary Guo @ 2026-01-26 17:59 UTC (permalink / raw) To: Zhi Wang, rust-for-linux, linux-pci, linux-kernel Cc: dakr, aliceryhl, bhelgaas, kwilczynski, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh, lossin, a.hindborg, tmgross, markus.probst, helgaas, cjia, smitra, ankita, aniketa, kwankhede, targupta, acourbot, joelagnelf, jhubbard, zhiwang, daniel.almeida, Jason Gunthorpe On Thu Jan 22, 2026 at 8:42 PM GMT, Zhi Wang wrote: > This patch adds a sample Rust driver demonstrating the usage of the fwctl > Rust abstractions. Add sample code for creating a FwCtl device, getting > device info and issuing an RPC. > > Cc: Danilo Krummrich <dakr@kernel.org> > Cc: Jason Gunthorpe <jgg@nvidia.com> > Signed-off-by: Zhi Wang <zhiw@nvidia.com> > --- > samples/rust/Kconfig | 11 +++ > samples/rust/Makefile | 1 + > samples/rust/rust_driver_fwctl.rs | 136 ++++++++++++++++++++++++++++++ > 3 files changed, 148 insertions(+) > create mode 100644 samples/rust/rust_driver_fwctl.rs > > diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig > index c49ab9106345..3d0b223caa89 100644 > --- a/samples/rust/Kconfig > +++ b/samples/rust/Kconfig > @@ -172,6 +172,17 @@ config SAMPLE_RUST_SOC > > If unsure, say N. > > +config SAMPLE_RUST_DRIVER_FWCTL > + tristate "Fwctl Driver" > + depends on FWCTL > + help > + This option builds the Rust Fwctl driver sample. > + > + To compile this as a module, choose M here: > + the module will be called rust_driver_fwctl. > + > + If unsure, say N. > + > config SAMPLE_RUST_HOSTPROGS > bool "Host programs" > help > diff --git a/samples/rust/Makefile b/samples/rust/Makefile > index 6c0aaa58cccc..6f6030e64727 100644 > --- a/samples/rust/Makefile > +++ b/samples/rust/Makefile > @@ -14,6 +14,7 @@ obj-$(CONFIG_SAMPLE_RUST_DRIVER_PLATFORM) += rust_driver_platform.o > obj-$(CONFIG_SAMPLE_RUST_DRIVER_USB) += rust_driver_usb.o > obj-$(CONFIG_SAMPLE_RUST_DRIVER_FAUX) += rust_driver_faux.o > obj-$(CONFIG_SAMPLE_RUST_DRIVER_AUXILIARY) += rust_driver_auxiliary.o > +obj-$(CONFIG_SAMPLE_RUST_DRIVER_FWCTL) += rust_driver_fwctl.o > obj-$(CONFIG_SAMPLE_RUST_CONFIGFS) += rust_configfs.o > obj-$(CONFIG_SAMPLE_RUST_SOC) += rust_soc.o > > diff --git a/samples/rust/rust_driver_fwctl.rs b/samples/rust/rust_driver_fwctl.rs > new file mode 100644 > index 000000000000..ac5f979fd73b > --- /dev/null > +++ b/samples/rust/rust_driver_fwctl.rs > @@ -0,0 +1,136 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +//! Rust fwctl API test (based on QEMU's `pci-testdev`). > +//! > +//! To make this driver probe, QEMU must be run with `-device pci-testdev`. > + > +use kernel::{ > + bindings, > + device, > + device::Core, > + devres::Devres, > + fwctl, > + pci, > + prelude::*, > + sync::aref::ARef, > + types, > +}; > + > +struct FwctlSampleUserCtx { > + _drvdata: u32, > +} > + > +struct FwctlSampleOps; > + > +impl fwctl::Operations for FwctlSampleOps { > + type UserCtx = FwctlSampleUserCtx; > + > + const DEVICE_TYPE: fwctl::DeviceType = fwctl::DeviceType::RustFwctlTest; > + > + fn open( > + fwctl_uctx: &types::Opaque<bindings::fwctl_uctx> > + ) -> Result<impl PinInit<Self::UserCtx, Error>, Error> { > + let dev = fwctl::UserCtx::<Self::UserCtx>::parent_device_from_raw(fwctl_uctx); Why does this raw binding need to be handled by the user? > + > + dev_info!(dev, "fwctl test driver: open_uctx()"); > + > + // Return an initializer for the user context. > + // The framework will initialize this in-place in the C-allocated memory. > + Ok(try_init!(FwctlSampleUserCtx { > + _drvdata: 0, > + })) > + } > + > + fn close(uctx: &mut fwctl::UserCtx<FwctlSampleUserCtx>) { > + let dev = uctx.get_parent_device(); > + > + dev_info!(dev, "fwctl test driver: close_uctx()"); > + } > + > + fn info(uctx: &mut fwctl::UserCtx<FwctlSampleUserCtx>) -> Result<KVec<u8>, Error> { This can just be `Result<KVec<u8>>`. Best, Gary > + let dev = uctx.get_parent_device(); > + > + dev_info!(dev, "fwctl test driver: info()"); > + > + let mut infobuf = KVec::<u8>::new(); > + infobuf.push(0xef, GFP_KERNEL)?; > + infobuf.push(0xbe, GFP_KERNEL)?; > + infobuf.push(0xad, GFP_KERNEL)?; > + infobuf.push(0xde, GFP_KERNEL)?; > + > + Ok(infobuf) > + } > + > + fn fw_rpc( > + uctx: &mut fwctl::UserCtx<FwctlSampleUserCtx>, > + scope: u32, > + rpc_in: &mut [u8], > + _out_len: *mut usize, > + ) -> Result<Option<KVec<u8>>, Error> { > + let dev = uctx.get_parent_device(); > + > + dev_info!(dev, "fwctl test driver: fw_rpc() scope {}", scope); > + > + if rpc_in.len() != 4 { > + return Err(EINVAL); > + } > + > + dev_info!( > + dev, > + "fwctl test driver: inbuf len{} bytes[0-3] {:x} {:x} {:x} {:x}", > + rpc_in.len(), > + rpc_in[0], > + rpc_in[1], > + rpc_in[2], > + rpc_in[3] > + ); > + > + let mut outbuf = KVec::<u8>::new(); > + outbuf.push(0xef, GFP_KERNEL)?; > + outbuf.push(0xbe, GFP_KERNEL)?; > + outbuf.push(0xad, GFP_KERNEL)?; > + outbuf.push(0xde, GFP_KERNEL)?; > + > + Ok(Some(outbuf)) > + } > +} > + > +#[pin_data] > +struct FwctlSampleDriver { > + pdev: ARef<pci::Device>, > + #[pin] > + fwctl: Devres<fwctl::Registration<FwctlSampleOps>>, > +} > + > +kernel::pci_device_table!( > + PCI_TABLE, > + MODULE_PCI_TABLE, > + <FwctlSampleDriver as pci::Driver>::IdInfo, > + [(pci::DeviceId::from_id(pci::Vendor::REDHAT, 0x5), ())] > +); > + > +impl pci::Driver for FwctlSampleDriver { > + type IdInfo = (); > + const ID_TABLE: pci::IdTable<Self::IdInfo> = &PCI_TABLE; > + > + fn probe(pdev: &pci::Device<Core>, _info: &Self::IdInfo) -> impl PinInit<Self, Error> { > + dev_info!(pdev.as_ref(), "Probe fwctl test driver"); > + > + // `pdev` is `Device<Core>`, which derefs to `Device<Bound>` during probe. > + let pdev_bound: &pci::Device<device::Bound> = pdev; > + > + try_pin_init!(Self { > + pdev: pdev.into(), > + fwctl <- fwctl::Registration::<FwctlSampleOps>::new(pdev_bound.as_ref()), > + }) > + } > +} > + > +kernel::module_pci_driver! { > + type: FwctlSampleDriver, > + name: "rust_driver_fwctl", > + authors: ["Zhi Wang"], > + description: "Rust fwctl test", > + license: "GPL v2", > + imports_ns: ["FWCTL"], > +} ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 0/2] rust: introduce abstractions for fwctl 2026-01-22 20:42 [PATCH v2 0/2] rust: introduce abstractions for fwctl Zhi Wang 2026-01-22 20:42 ` [PATCH v2 1/2] " Zhi Wang 2026-01-22 20:42 ` [PATCH v2 2/2] samples: rust: fwctl: add sample code " Zhi Wang @ 2026-01-22 21:32 ` Danilo Krummrich 2026-01-23 10:14 ` Zhi Wang 2 siblings, 1 reply; 33+ messages in thread From: Danilo Krummrich @ 2026-01-22 21:32 UTC (permalink / raw) To: Zhi Wang Cc: rust-for-linux, linux-pci, linux-kernel, aliceryhl, bhelgaas, kwilczynski, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh, lossin, a.hindborg, tmgross, markus.probst, helgaas, cjia, smitra, ankita, aniketa, kwankhede, targupta, acourbot, joelagnelf, jhubbard, zhiwang, daniel.almeida, Jason Gunthorpe (Cc: Jason) Looks like you used the recipient list of another patch series by accident. :) On Thu Jan 22, 2026 at 9:42 PM CET, Zhi Wang wrote: > In the NVIDIA vGPU RFC [1], the vGPU type blobs must be provided to the GSP > before userspace can enumerate available vGPU types and create vGPU > instances. The original design relied on the firmware loading interface, > but fwctl is a more natural fit for this use case, as it is designed for > uploading configuration or firmware data required before the device becomes > operational. > > This patch introduces a Rust abstraction over the fwctl subsystem, > providing safe and idiomatic bindings. > > The new `fwctl` module allows Rust drivers to integrate with the existing > C-side fwctl core through a typed trait interface. It provides: > > - `Operations` trait — defines driver-specific operations such as > `open_uctx()`, `close_uctx()`, `info()`, and `fw_rpc()`. > Each Rust driver implements this trait to describe its own per-FD > user-context behavior and RPC handling. > > - `UserCtx<T>` — a generic wrapper around `struct fwctl_uctx` > embedding driver-specific context data, providing safe conversion > from raw C pointers and access to the parent device. > > - `Registration<T>` — safe registration and automatic unregistration > of `struct fwctl_device` objects using the kernel's device model. > > - `VTable<T>` — a static vtable bridging C callbacks and Rust > trait methods, ensuring type safety across the FFI boundary. > > `rust/kernel/lib.rs` is updated to conditionally include this module > under `CONFIG_FWCTL`. > > The repo with the patches can be found at [2]. > > v2: > > - Don't open fwctl_put(). Add a rust helper. (Jason/Danilo) > - Wrap Registration with Devres to guarantee proper lifetime management. > (Jason/Danilo) > - Rename FwctlOps to Operations, FwctlUserCtx to UserCtx, FwctlDevice to > Device. (Danilo) > - Use fwctl::DeviceType enum instead of raw u32 for DEVICE_TYPE. (Danilo) > - Change fwctl_uctx field in UserCtx to Opaque<bindings::fwctl_uctx> and > make it private. (Danilo) > - Provide Deref and DerefMut implementations for UserCtx::uctx. (Danilo) > - Add UserCtx::parent_device_from_raw() to simplify parent device access. > - Use cast() and cast_mut() instead of manual pointer casts. (Danilo) > - Implement AlwaysRefCounted for Device and use ARef<Device> in > Registration. (Danilo) > - Add rust_helper_fwctl_get() for reference counting. > - Improve safety comments for slice::from_raw_parts_mut() in > fw_rpc_callback. (Danilo) > - Convert imports to vertical style. > - Fix all clippy warnings. > > v1: > > - Initial submission introducing fwctl Rust abstractions. > > [1] https://lore.kernel.org/all/20250903221111.3866249-1-zhiw@nvidia.com/ > [2] https://github.com/zhiwang-nvidia/driver-core/tree/fwctl-rust-abstraction-v2 > > Zhi Wang (2): > rust: introduce abstractions for fwctl > samples: rust: fwctl: add sample code for fwctl > > 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 + > samples/rust/Kconfig | 11 + > samples/rust/Makefile | 1 + > samples/rust/rust_driver_fwctl.rs | 136 +++++++++ > 10 files changed, 639 insertions(+), 1 deletion(-) > create mode 100644 rust/helpers/fwctl.c > create mode 100644 rust/kernel/fwctl.rs > create mode 100644 samples/rust/rust_driver_fwctl.rs > > -- > 2.51.0 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 0/2] rust: introduce abstractions for fwctl 2026-01-22 21:32 ` [PATCH v2 0/2] rust: introduce abstractions " Danilo Krummrich @ 2026-01-23 10:14 ` Zhi Wang 0 siblings, 0 replies; 33+ messages in thread From: Zhi Wang @ 2026-01-23 10:14 UTC (permalink / raw) To: Danilo Krummrich Cc: rust-for-linux, linux-pci, linux-kernel, aliceryhl, bhelgaas, kwilczynski, ojeda, alex.gaynor, boqun.feng, gary, bjorn3_gh, lossin, a.hindborg, tmgross, markus.probst, helgaas, cjia, smitra, ankita, aniketa, kwankhede, targupta, acourbot, joelagnelf, jhubbard, zhiwang, daniel.almeida, Jason Gunthorpe On Thu, 22 Jan 2026 22:32:03 +0100 "Danilo Krummrich" <dakr@kernel.org> wrote: > (Cc: Jason) > > Looks like you used the recipient list of another patch series by > accident. :) > Sorry my bad. I would prepare a new script for this series. Z. > On Thu Jan 22, 2026 at 9:42 PM CET, Zhi Wang wrote: > > In the NVIDIA vGPU RFC [1], the vGPU type blobs must be provided to > > the GSP before userspace can enumerate available vGPU types and create > > vGPU instances. The original design relied on the firmware loading > > interface, but fwctl is a more natural fit for this use case, as it is > > designed for uploading configuration or firmware data required before > > the device becomes operational. > > > > This patch introduces a Rust abstraction over the fwctl subsystem, > > providing safe and idiomatic bindings. > > > > The new `fwctl` module allows Rust drivers to integrate with the > > existing C-side fwctl core through a typed trait interface. It > > provides: > > > > - `Operations` trait — defines driver-specific operations such as > > `open_uctx()`, `close_uctx()`, `info()`, and `fw_rpc()`. > > Each Rust driver implements this trait to describe its own per-FD > > user-context behavior and RPC handling. > > > > - `UserCtx<T>` — a generic wrapper around `struct fwctl_uctx` > > embedding driver-specific context data, providing safe conversion > > from raw C pointers and access to the parent device. > > > > - `Registration<T>` — safe registration and automatic unregistration > > of `struct fwctl_device` objects using the kernel's device model. > > > > - `VTable<T>` — a static vtable bridging C callbacks and Rust > > trait methods, ensuring type safety across the FFI boundary. > > > > `rust/kernel/lib.rs` is updated to conditionally include this module > > under `CONFIG_FWCTL`. > > > > The repo with the patches can be found at [2]. > > > > v2: > > > > - Don't open fwctl_put(). Add a rust helper. (Jason/Danilo) > > - Wrap Registration with Devres to guarantee proper lifetime > > management. (Jason/Danilo) > > - Rename FwctlOps to Operations, FwctlUserCtx to UserCtx, FwctlDevice > > to Device. (Danilo) > > - Use fwctl::DeviceType enum instead of raw u32 for DEVICE_TYPE. > > (Danilo) > > - Change fwctl_uctx field in UserCtx to Opaque<bindings::fwctl_uctx> > > and make it private. (Danilo) > > - Provide Deref and DerefMut implementations for UserCtx::uctx. > > (Danilo) > > - Add UserCtx::parent_device_from_raw() to simplify parent device > > access. > > - Use cast() and cast_mut() instead of manual pointer casts. (Danilo) > > - Implement AlwaysRefCounted for Device and use ARef<Device> in > > Registration. (Danilo) > > - Add rust_helper_fwctl_get() for reference counting. > > - Improve safety comments for slice::from_raw_parts_mut() in > > fw_rpc_callback. (Danilo) > > - Convert imports to vertical style. > > - Fix all clippy warnings. > > > > v1: > > > > - Initial submission introducing fwctl Rust abstractions. > > > > [1] > > https://lore.kernel.org/all/20250903221111.3866249-1-zhiw@nvidia.com/ > > [2] > > https://github.com/zhiwang-nvidia/driver-core/tree/fwctl-rust-abstraction-v2 > > > > Zhi Wang (2): > > rust: introduce abstractions for fwctl > > samples: rust: fwctl: add sample code for fwctl > > > > 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 + > > samples/rust/Kconfig | 11 + > > samples/rust/Makefile | 1 + > > samples/rust/rust_driver_fwctl.rs | 136 +++++++++ > > 10 files changed, 639 insertions(+), 1 deletion(-) > > create mode 100644 rust/helpers/fwctl.c > > create mode 100644 rust/kernel/fwctl.rs > > create mode 100644 samples/rust/rust_driver_fwctl.rs > > > > -- > > 2.51.0 > ^ permalink raw reply [flat|nested] 33+ messages in thread
end of thread, other threads:[~2026-01-28 17:40 UTC | newest] Thread overview: 33+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-01-22 20:42 [PATCH v2 0/2] rust: introduce abstractions for fwctl Zhi Wang 2026-01-22 20:42 ` [PATCH v2 1/2] " Zhi Wang 2026-01-22 21:17 ` Joel Fernandes 2026-01-23 10:25 ` Zhi Wang 2026-01-26 17:48 ` Gary Guo 2026-01-27 19:59 ` Zhi Wang 2026-01-26 18:19 ` Jason Gunthorpe 2026-01-27 19:57 ` Zhi Wang 2026-01-27 20:07 ` Danilo Krummrich 2026-01-28 0:04 ` Jason Gunthorpe 2026-01-28 1:21 ` Danilo Krummrich 2026-01-28 13:20 ` [PATCH v2 1/2] rust: introduce abstractions for fwctlg Jason Gunthorpe 2026-01-28 14:01 ` Danilo Krummrich 2026-01-28 14:59 ` Jason Gunthorpe 2026-01-28 15:49 ` Danilo Krummrich 2026-01-28 15:56 ` Jason Gunthorpe 2026-01-28 16:35 ` Danilo Krummrich 2026-01-28 16:39 ` Jason Gunthorpe 2026-01-28 17:26 ` Zhi Wang 2026-01-28 17:30 ` Zhi Wang 2026-01-28 17:39 ` Jason Gunthorpe 2026-01-28 17:40 ` Danilo Krummrich 2026-01-28 11:36 ` [PATCH v2 1/2] rust: introduce abstractions for fwctl Zhi Wang 2026-01-28 11:41 ` Danilo Krummrich 2026-01-27 20:09 ` Danilo Krummrich 2026-01-22 20:42 ` [PATCH v2 2/2] samples: rust: fwctl: add sample code " Zhi Wang 2026-01-22 20:58 ` Jason Gunthorpe 2026-01-22 21:06 ` Danilo Krummrich 2026-01-22 21:16 ` John Hubbard 2026-01-23 10:23 ` Zhi Wang 2026-01-26 17:59 ` Gary Guo 2026-01-22 21:32 ` [PATCH v2 0/2] rust: introduce abstractions " Danilo Krummrich 2026-01-23 10:14 ` Zhi Wang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox