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.
The core of the abstraction is the `FwCtlOps` trait, which defines the
driver callbacks (`open_uctx`, `close_uctx`, `info`, and `fw_rpc`).
`FwCtlVTable` bridges these trait methods to the C fwctl core through a
static vtable.
`rust/kernel/lib.rs` is updated to conditionally build this module when
`CONFIG_FWCTL` is enabled.
Signed-off-by: Zhi Wang <zhiw@nvidia.com>
---
rust/bindings/bindings_helper.h | 1 +
rust/kernel/fwctl.rs | 254 ++++++++++++++++++++++++++++++++
rust/kernel/lib.rs | 2 +
3 files changed, 257 insertions(+)
create mode 100644 rust/kernel/fwctl.rs
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 2e43c66635a2..5c374965f0f1 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/ioport.h>
diff --git a/rust/kernel/fwctl.rs b/rust/kernel/fwctl.rs
new file mode 100644
index 000000000000..21f8f7d11d6f
--- /dev/null
+++ b/rust/kernel/fwctl.rs
@@ -0,0 +1,254 @@
+// 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::device::Device;
+use crate::types::ARef;
+use crate::{bindings, container_of, device, error::code::*, prelude::*};
+
+use core::marker::PhantomData;
+use core::ptr::NonNull;
+use core::slice;
+
+/// The registration of a fwctl device.
+///
+/// This type represents the registration of a [`struct fwctl_device`]. When an instance of this
+/// type is dropped, its respective fwctl device will be unregistered and freed.
+///
+/// [`struct fwctl_device`]: srctree/include/linux/device/fwctl.h
+pub struct Registration<T: FwCtlOps> {
+ fwctl_dev: NonNull<bindings::fwctl_device>,
+ _marker: PhantomData<T>,
+}
+
+impl<T: FwCtlOps> Registration<T> {
+ /// Allocate and register a new fwctl device under the given parent device.
+ pub fn new(parent: &device::Device) -> Result<Self> {
+ let ops = &FwCtlVTable::<T>::VTABLE as *const _ as *mut _;
+
+ // SAFETY: `_fwctl_alloc_device()` allocates a new `fwctl_device`
+ // and initializes its embedded `struct device`.
+ let dev = unsafe {
+ bindings::_fwctl_alloc_device(
+ parent.as_raw(),
+ ops,
+ core::mem::size_of::<bindings::fwctl_device>(),
+ )
+ };
+
+ let dev = NonNull::new(dev).ok_or(ENOMEM)?;
+
+ // SAFETY: `fwctl_register()` expects a valid device from `_fwctl_alloc_device()`.
+ let ret = unsafe { bindings::fwctl_register(dev.as_ptr()) };
+ if ret != 0 {
+ // SAFETY: If registration fails, release the allocated fwctl_device().
+ unsafe {
+ bindings::put_device(core::ptr::addr_of_mut!((*dev.as_ptr()).dev));
+ }
+ return Err(Error::from_errno(ret));
+ }
+
+ Ok(Self {
+ fwctl_dev: dev,
+ _marker: PhantomData,
+ })
+ }
+
+ fn as_raw(&self) -> *mut bindings::fwctl_device {
+ self.fwctl_dev.as_ptr()
+ }
+}
+
+impl<T: FwCtlOps> Drop for Registration<T> {
+ fn drop(&mut self) {
+ // SAFETY: `fwctl_unregister()` expects a valid device from `_fwctl_alloc_device()`.
+ unsafe {
+ bindings::fwctl_unregister(self.as_raw());
+ bindings::put_device(core::ptr::addr_of_mut!((*self.as_raw()).dev));
+ }
+ }
+}
+
+// SAFETY: The only action allowed in a `Registration` instance is dropping it, which is safe to do
+// from any thread because `fwctl_unregister()/put_device()` can be called from any sleepible
+// context.
+unsafe impl<T: FwCtlOps> Send 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 FwCtlOps: Sized {
+ /// Driver UCtx type.
+ type UCtx;
+
+ /// fwctl device type, matching the C enum `fwctl_device_type`.
+ const DEVICE_TYPE: u32;
+
+ /// Called when a new user context is opened by userspace.
+ fn open_uctx(uctx: &mut FwCtlUCtx<Self::UCtx>) -> Result<(), Error>;
+
+ /// Called when the user context is being closed.
+ fn close_uctx(uctx: &mut FwCtlUCtx<Self::UCtx>);
+
+ /// Return device or context information to userspace.
+ fn info(uctx: &mut FwCtlUCtx<Self::UCtx>) -> Result<KVec<u8>, Error>;
+
+ /// Called when a userspace RPC request is received.
+ fn fw_rpc(
+ uctx: &mut FwCtlUCtx<Self::UCtx>,
+ scope: u32,
+ rpc_in: &mut [u8],
+ out_len: *mut usize,
+ ) -> Result<Option<KVec<u8>>, Error>;
+}
+
+/// Represents a per-FD user context (`struct fwctl_uctx`).
+///
+/// Each driver embeds `struct fwctl_uctx` as the first field of its own
+/// context type and uses this wrapper to access driver-specific data.
+#[repr(C)]
+#[pin_data]
+pub struct FwCtlUCtx<T> {
+ /// The core fwctl user context shared with the C implementation.
+ #[pin]
+ pub fwctl_uctx: bindings::fwctl_uctx,
+ /// Driver-specific data associated with this user context.
+ pub uctx: T,
+}
+
+impl<T> FwCtlUCtx<T> {
+ /// Converts a raw C pointer to `struct fwctl_uctx` into a reference to the
+ /// enclosing `FwCtlUCtx<T>`.
+ ///
+ /// # Safety
+ /// * `ptr` must be a valid pointer to a `fwctl_uctx` that is embedded
+ /// inside an existing `FwCtlUCtx<T>` instance.
+ /// * The caller must ensure that the lifetime of the returned reference
+ /// does not outlive the underlying object managed on the C side.
+ pub unsafe fn from_raw<'a>(ptr: *mut bindings::fwctl_uctx) -> &'a mut Self {
+ // SAFETY: `ptr` was originally created from a valid `FwCtlUCtx<T>`.
+ unsafe { &mut *container_of!(ptr, FwCtlUCtx<T>, fwctl_uctx) }
+ }
+
+ /// Returns the parent device of this user context.
+ ///
+ /// # Safety
+ /// The `fwctl_device` pointer inside `fwctl_uctx` must be valid.
+ pub fn get_parent_device(&self) -> ARef<Device> {
+ // SAFETY: `self.fwctl_uctx.fwctl` is initialized by the fwctl subsystem and guaranteed
+ // to remain valid for the lifetime of this `FwCtlUCtx`.
+ let raw_dev =
+ unsafe { (*(self.fwctl_uctx.fwctl)).dev.parent as *mut kernel::bindings::device };
+ // SAFETY: `raw_dev` points to a live device object.
+ unsafe { Device::get_device(raw_dev) }
+ }
+
+ /// Returns a mutable reference to the driver-specific context.
+ pub fn to_driver_uctx_mut(&mut self) -> &mut T {
+ &mut self.uctx
+ }
+}
+
+/// Static vtable mapping Rust trait methods to C callbacks.
+pub struct FwCtlVTable<T: FwCtlOps>(PhantomData<T>);
+
+impl<T: FwCtlOps> FwCtlVTable<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,
+ uctx_size: core::mem::size_of::<FwCtlUCtx<T::UCtx>>(),
+ 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.
+ unsafe extern "C" fn open_uctx_callback(uctx: *mut bindings::fwctl_uctx) -> ffi::c_int {
+ // SAFETY: `uctx` is guaranteed by the fwctl subsystem to be a valid pointer.
+ let ctx = unsafe { FwCtlUCtx::<T::UCtx>::from_raw(uctx) };
+ match T::open_uctx(ctx) {
+ Ok(()) => 0,
+ Err(e) => e.to_errno(),
+ }
+ }
+
+ /// Called when the user context is being closed.
+ 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 { FwCtlUCtx::<T::UCtx>::from_raw(uctx) };
+ T::close_uctx(ctx);
+ }
+
+ /// Returns device or context information.
+ 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 { FwCtlUCtx::<T::UCtx>::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.
+ 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 { FwCtlUCtx::<T::UCtx>::from_raw(uctx) };
+
+ // SAFETY: `rpc_in` points to a valid input buffer of size `in_len`
+ // provided by fwctl subsystem.
+ let rpc_in_slice: &mut [u8] =
+ unsafe { slice::from_raw_parts_mut(rpc_in as *mut 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,
+
+ // Return an ERR_PTR-style encoded error pointer.
+ Err(e) => Error::to_ptr(e),
+ }
+ }
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 3dd7bebe7888..8ddad8ae211a 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -94,6 +94,8 @@
pub mod firmware;
pub mod fmt;
pub mod fs;
+#[cfg(CONFIG_FWCTL)]
+pub mod fwctl;
pub mod id_pool;
pub mod init;
pub mod io;
--
2.47.3On Thu Oct 30, 2025 at 5:03 PM CET, Zhi Wang wrote:
> +/// Static vtable mapping Rust trait methods to C callbacks.
> +pub struct FwCtlVTable<T: FwCtlOps>(PhantomData<T>);
> +
> +impl<T: FwCtlOps> FwCtlVTable<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,
> + uctx_size: core::mem::size_of::<FwCtlUCtx<T::UCtx>>(),
The fwctl code uses this size to allocate memory for both, struct fwctl_uctx and
the driver's private data at the end of the allocation.
This means that it is not enough to just consider the size of T::UCtx, you also
have to consider its required alignment, and, if required, allocate more memory.
> + 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.
> + unsafe extern "C" fn open_uctx_callback(uctx: *mut bindings::fwctl_uctx) -> ffi::c_int {
> + // SAFETY: `uctx` is guaranteed by the fwctl subsystem to be a valid pointer.
> + let ctx = unsafe { FwCtlUCtx::<T::UCtx>::from_raw(uctx) };
Considering the above, this is incorrect for two reasons.
(1) FwCtlUCtx::uctx might not be aligned correctly.
(2) FwCtlUCtx::uctx is not initialized, hence creating a reference might be
undefined behavior.
I think the correct way to fix (2) is to only provide an abstraction of struct
fwctl_uctx as argument to T::open_uctx() and let T::open_uctx() return an
initializer for FwCtlUCtx::uctx, i.e. impl PinInit<T::UCtx, Error>.
All other callbacks should be correct as they are once the alignment is
considered.
> + match T::open_uctx(ctx) {
> + Ok(()) => 0,
> + Err(e) => e.to_errno(),
> + }
> + }
On Sun, 02 Nov 2025 19:33:10 +0100
"Danilo Krummrich" <dakr@kernel.org> wrote:
> On Thu Oct 30, 2025 at 5:03 PM CET, Zhi Wang wrote:
> > +/// Static vtable mapping Rust trait methods to C callbacks.
> > +pub struct FwCtlVTable<T: FwCtlOps>(PhantomData<T>);
> > +
> > +impl<T: FwCtlOps> FwCtlVTable<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,
> > + uctx_size: core::mem::size_of::<FwCtlUCtx<T::UCtx>>(),
>
> The fwctl code uses this size to allocate memory for both, struct
> fwctl_uctx and the driver's private data at the end of the allocation.
>
> This means that it is not enough to just consider the size of
> T::UCtx, you also have to consider its required alignment, and, if
> required, allocate more memory.
>
FwCtlUCtx is defined as below:
+#[repr(C)]
+#[pin_data]
+pub struct FwCtlUCtx<T> {
+ /// The core fwctl user context shared with the C implementation.
+ #[pin]
+ pub fwctl_uctx: bindings::fwctl_uctx,
+ /// Driver-specific data associated with this user context.
+ pub uctx: T,
+}
I assume it should be equal to C structure as below and with #[repr(C)],
the handling of layout and the alignment should be as the same as C
structure.
struct FwCtlUCtx {
struct fwctl_uctx base;
struct my_driver_data data;
};
uctx_size: core::mem::size_of::<FwCtlUCtx<T::UCtx>>() should be:
sizeof(FWCtlUCtx).
Do we need anything extra for alignment? Or some parts of the flow
doesn't respect the #[repr(C)]?
> > + 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.
> > + unsafe extern "C" fn open_uctx_callback(uctx: *mut
> > bindings::fwctl_uctx) -> ffi::c_int {
> > + // SAFETY: `uctx` is guaranteed by the fwctl subsystem to
> > be a valid pointer.
> > + let ctx = unsafe { FwCtlUCtx::<T::UCtx>::from_raw(uctx) };
>
> Considering the above, this is incorrect for two reasons.
>
> (1) FwCtlUCtx::uctx might not be aligned correctly.
>
> (2) FwCtlUCtx::uctx is not initialized, hence creating a reference
> might be undefined behavior.
>
> I think the correct way to fix (2) is to only provide an abstraction
> of struct fwctl_uctx as argument to T::open_uctx() and let
> T::open_uctx() return an initializer for FwCtlUCtx::uctx, i.e. impl
> PinInit<T::UCtx, Error>.
>
Nice catch. With this approach, we can force the driver user context to
be explicitly intialized and start to be tracked in the safe world.
> All other callbacks should be correct as they are once the alignment
> is considered.
>
> > + match T::open_uctx(ctx) {
> > + Ok(()) => 0,
> > + Err(e) => e.to_errno(),
> > + }
> > + }
On Mon Nov 3, 2025 at 10:55 AM CET, Zhi Wang wrote:
> On Sun, 02 Nov 2025 19:33:10 +0100
> "Danilo Krummrich" <dakr@kernel.org> wrote:
>
>> On Thu Oct 30, 2025 at 5:03 PM CET, Zhi Wang wrote:
>> > +/// Static vtable mapping Rust trait methods to C callbacks.
>> > +pub struct FwCtlVTable<T: FwCtlOps>(PhantomData<T>);
>> > +
>> > +impl<T: FwCtlOps> FwCtlVTable<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,
>> > + uctx_size: core::mem::size_of::<FwCtlUCtx<T::UCtx>>(),
>>
>> The fwctl code uses this size to allocate memory for both, struct
>> fwctl_uctx and the driver's private data at the end of the allocation.
>>
>> This means that it is not enough to just consider the size of
>> T::UCtx, you also have to consider its required alignment, and, if
>> required, allocate more memory.
>>
>
> FwCtlUCtx is defined as below:
>
> +#[repr(C)]
> +#[pin_data]
> +pub struct FwCtlUCtx<T> {
> + /// The core fwctl user context shared with the C implementation.
> + #[pin]
> + pub fwctl_uctx: bindings::fwctl_uctx,
> + /// Driver-specific data associated with this user context.
> + pub uctx: T,
> +}
>
> I assume it should be equal to C structure as below and with #[repr(C)],
> the handling of layout and the alignment should be as the same as C
> structure.
>
> struct FwCtlUCtx {
> struct fwctl_uctx base;
> struct my_driver_data data;
> };
>
> uctx_size: core::mem::size_of::<FwCtlUCtx<T::UCtx>>() should be:
That's indeed correct then, I think I read uctx_size as the size of T::UCtx
only. (I've recently come across subsystems that do exacly that and hence run
into the problem in [2].)
Anyways, I suggest to not give out a FwCtlUCtx<T> to the driver at all, since in
open() we can't (for the discussed reasons) and in the other callbacks it
doesn't seem very useful either.
Instead, I think we should have the following callback arguments:
impl fwctl::Operations for MyDriverContext {
fn open(
fdev: &fwctl::Device,
parent: &Device<Bound>
) -> impl PinInit<Self, Error>;
fn close(
this: Pin<&mut Self>,
fdev: &fwctl::Device,
parent: &Device<Bound>
) -> impl PinInit<Self, Error>;
}
with
#[repr(transparent)]
struct Device(Opaque<bindings::fwctl_device);
Note this also gets us rid of the Self::UCtx type alias, which seems
unnecessary.
You could still keep a FwCtlUCtx<T> type internally since it might make the
implementation easier.
[2] https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=806c1a9a53a174ef39acff8ae5bb0e68
On Sun, Nov 02, 2025 at 07:33:10PM +0100, Danilo Krummrich wrote:
> On Thu Oct 30, 2025 at 5:03 PM CET, Zhi Wang wrote:
> > +/// Static vtable mapping Rust trait methods to C callbacks.
> > +pub struct FwCtlVTable<T: FwCtlOps>(PhantomData<T>);
> > +
> > +impl<T: FwCtlOps> FwCtlVTable<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,
> > + uctx_size: core::mem::size_of::<FwCtlUCtx<T::UCtx>>(),
>
> The fwctl code uses this size to allocate memory for both, struct fwctl_uctx and
> the driver's private data at the end of the allocation.
>
> This means that it is not enough to just consider the size of T::UCtx, you also
> have to consider its required alignment, and, if required, allocate more memory.
Yes, the container_of() relationship must be such that the core struct
is first in the memory and any driver data is trailing. The C version
has a static_assertion to constrain this.
> All other callbacks should be correct as they are once the alignment is
> considered.
Yes, only alloc and put/free make this assumption.
Jason
Hi Zhi,
Additional to my other comments, some thoughts about naming.
On Thu Oct 30, 2025 at 5:03 PM CET, Zhi Wang wrote:
> +/// 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 FwCtlOps: Sized {
Up to Jason, but I usually recommend to take the Rust module name into account,
i.e a user of the API can refer to this as fwctl::FwCtlOps.
Hence, I suggest to name this Ops or even Operations, such that users can refer
to this as fwctl::Ops or fwctl::Operations.
It would also be more consistent with existing code, e.g. pci::Device,
platform::Device, etc., but also the fwctl::Registration from this patch. :)
> + /// Driver UCtx type.
> + type UCtx;
I think we can affort to be a bit more verbose here, maybe just spell it out as
UserContext.
> + /// fwctl device type, matching the C enum `fwctl_device_type`.
> + const DEVICE_TYPE: u32;
I think it would make sense to have a new (Rust enum) type (fwctl::DeviceType)
for this, rather than using a raw u32, see [1] for reference.
[1] https://lore.kernel.org/all/20250828133323.53311-2-dakr@kernel.org/
> +/// Represents a per-FD user context (`struct fwctl_uctx`).
> +///
> +/// Each driver embeds `struct fwctl_uctx` as the first field of its own
> +/// context type and uses this wrapper to access driver-specific data.
> +#[repr(C)]
> +#[pin_data]
> +pub struct FwCtlUCtx<T> {
What about UserContext<T>? A driver would refer to this as fwctl::UserContext.
On Sun, Nov 02, 2025 at 06:26:58PM +0100, Danilo Krummrich wrote:
> Hi Zhi,
>
> Additional to my other comments, some thoughts about naming.
>
> On Thu Oct 30, 2025 at 5:03 PM CET, Zhi Wang wrote:
> > +/// 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 FwCtlOps: Sized {
>
> Up to Jason, but I usually recommend to take the Rust module name into account,
> i.e a user of the API can refer to this as fwctl::FwCtlOps.
I don't have any knowledge to have a preference, I trust you to guide
Zhi to whatever is the most common and appropriate thing here..
Thanks,
Jason
On Thu Oct 30, 2025 at 5:03 PM CET, Zhi Wang wrote:
> diff --git a/rust/kernel/fwctl.rs b/rust/kernel/fwctl.rs
> new file mode 100644
> index 000000000000..21f8f7d11d6f
> --- /dev/null
> +++ b/rust/kernel/fwctl.rs
> @@ -0,0 +1,254 @@
> +// 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::device::Device;
> +use crate::types::ARef;
> +use crate::{bindings, container_of, device, error::code::*, prelude::*};
> +
> +use core::marker::PhantomData;
> +use core::ptr::NonNull;
> +use core::slice;
Please use the import scheme as documented in [1].
[1] https://docs.kernel.org/rust/coding-guidelines.html#imports
> +/// The registration of a fwctl device.
> +///
> +/// This type represents the registration of a [`struct fwctl_device`]. When an instance of this
> +/// type is dropped, its respective fwctl device will be unregistered and freed.
> +///
> +/// [`struct fwctl_device`]: srctree/include/linux/device/fwctl.h
> +pub struct Registration<T: FwCtlOps> {
> + fwctl_dev: NonNull<bindings::fwctl_device>,
Given that this structure has to keep a reference count of the fwctl_device, I'd
prefer to have an abstraction of struct fwctl_device (fwctl::Device) which
implements AlwaysRefCounted.
This way the Registration can store an ARef<fwctl::Device> rather than a raw
pointer.
However, I wonder if we really need a reference count? Does fwctl_register() not
take a reference count itself?
> + _marker: PhantomData<T>,
> +}
> +
> +impl<T: FwCtlOps> Registration<T> {
> + /// Allocate and register a new fwctl device under the given parent device.
> + pub fn new(parent: &device::Device) -> Result<Self> {
AFAIK, fwctl_unregister() is synchronized against IOCTLs. Hence, if we guarantee
that a fwctl::Registration can not out-live parent device unbind, we can provide
a &Device<Bound> in the FwCtlOps callbacks, which allows us to do zero-cost
accesses of device resources with Devres::access().
In order to provide this guarantee, this function should return
impl PinInit<Devres<Self>, Error>.
> + let ops = &FwCtlVTable::<T>::VTABLE as *const _ as *mut _;
Please use cast() and cast_mut() when possible.
> +
> + // SAFETY: `_fwctl_alloc_device()` allocates a new `fwctl_device`
> + // and initializes its embedded `struct device`.
This safety comment should justify how you guarantee that the arguments you pass
in are valid, instead of describing what the called function does.
> + let dev = unsafe {
> + bindings::_fwctl_alloc_device(
> + parent.as_raw(),
> + ops,
> + core::mem::size_of::<bindings::fwctl_device>(),
> + )
> + };
> +
> + let dev = NonNull::new(dev).ok_or(ENOMEM)?;
> +
> + // SAFETY: `fwctl_register()` expects a valid device from `_fwctl_alloc_device()`.
> + let ret = unsafe { bindings::fwctl_register(dev.as_ptr()) };
> + if ret != 0 {
> + // SAFETY: If registration fails, release the allocated fwctl_device().
> + unsafe {
> + bindings::put_device(core::ptr::addr_of_mut!((*dev.as_ptr()).dev));
> + }
> + return Err(Error::from_errno(ret));
> + }
> +
> + Ok(Self {
> + fwctl_dev: dev,
> + _marker: PhantomData,
> + })
> + }
> +
> + fn as_raw(&self) -> *mut bindings::fwctl_device {
> + self.fwctl_dev.as_ptr()
> + }
> +}
> +
> +impl<T: FwCtlOps> Drop for Registration<T> {
> + fn drop(&mut self) {
> + // SAFETY: `fwctl_unregister()` expects a valid device from `_fwctl_alloc_device()`.
> + unsafe {
> + bindings::fwctl_unregister(self.as_raw());
> + bindings::put_device(core::ptr::addr_of_mut!((*self.as_raw()).dev));
> + }
> + }
> +}
> +
> +// SAFETY: The only action allowed in a `Registration` instance is dropping it, which is safe to do
> +// from any thread because `fwctl_unregister()/put_device()` can be called from any sleepible
> +// context.
> +unsafe impl<T: FwCtlOps> Send 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 FwCtlOps: Sized {
> + /// Driver UCtx type.
> + type UCtx;
> +
> + /// fwctl device type, matching the C enum `fwctl_device_type`.
> + const DEVICE_TYPE: u32;
> +
> + /// Called when a new user context is opened by userspace.
> + fn open_uctx(uctx: &mut FwCtlUCtx<Self::UCtx>) -> Result<(), Error>;
> +
> + /// Called when the user context is being closed.
> + fn close_uctx(uctx: &mut FwCtlUCtx<Self::UCtx>);
Why not just open() and close()?
> + /// Return device or context information to userspace.
> + fn info(uctx: &mut FwCtlUCtx<Self::UCtx>) -> Result<KVec<u8>, Error>;
> +
> + /// Called when a userspace RPC request is received.
> + fn fw_rpc(
> + uctx: &mut FwCtlUCtx<Self::UCtx>,
> + scope: u32,
> + rpc_in: &mut [u8],
> + out_len: *mut usize,
> + ) -> Result<Option<KVec<u8>>, Error>;
As mentioned above, if we ensure that a fwctl::Registration cannot out-live the
parent device being bound, we can provide a &Device<Bound> in those callbacks
for zero-cost accesses of device resources with Devres::access().
> +}
> +
> +/// Represents a per-FD user context (`struct fwctl_uctx`).
> +///
> +/// Each driver embeds `struct fwctl_uctx` as the first field of its own
> +/// context type and uses this wrapper to access driver-specific data.
> +#[repr(C)]
> +#[pin_data]
> +pub struct FwCtlUCtx<T> {
> + /// The core fwctl user context shared with the C implementation.
> + #[pin]
> + pub fwctl_uctx: bindings::fwctl_uctx,
This should be Opaque<bindings::fwctl_uctx> and should not be a public field.
> + /// Driver-specific data associated with this user context.
> + pub uctx: T,
I'd rather provide a Deref and DerefMut implementation for this.
> +}
> +
> +impl<T> FwCtlUCtx<T> {
> + /// Converts a raw C pointer to `struct fwctl_uctx` into a reference to the
> + /// enclosing `FwCtlUCtx<T>`.
> + ///
> + /// # Safety
> + /// * `ptr` must be a valid pointer to a `fwctl_uctx` that is embedded
> + /// inside an existing `FwCtlUCtx<T>` instance.
> + /// * The caller must ensure that the lifetime of the returned reference
> + /// does not outlive the underlying object managed on the C side.
> + pub unsafe fn from_raw<'a>(ptr: *mut bindings::fwctl_uctx) -> &'a mut Self {
Why does this need to be public?
> + // SAFETY: `ptr` was originally created from a valid `FwCtlUCtx<T>`.
> + unsafe { &mut *container_of!(ptr, FwCtlUCtx<T>, fwctl_uctx) }
> + }
> +
> + /// Returns the parent device of this user context.
> + ///
> + /// # Safety
> + /// The `fwctl_device` pointer inside `fwctl_uctx` must be valid.
> + pub fn get_parent_device(&self) -> ARef<Device> {
We the fwctl::Registration changes suggested above, this should return a
&Device<Bound>.
Regardless of this, it's better to return a &Device than an ARef<Device>. The
caller can always obtain a reference count, i.e. ARef<Device> from a &Device (or
a &Device<Bound>).
> + // SAFETY: `self.fwctl_uctx.fwctl` is initialized by the fwctl subsystem and guaranteed
> + // to remain valid for the lifetime of this `FwCtlUCtx`.
> + let raw_dev =
> + unsafe { (*(self.fwctl_uctx.fwctl)).dev.parent as *mut kernel::bindings::device };
> + // SAFETY: `raw_dev` points to a live device object.
> + unsafe { Device::get_device(raw_dev) }
> + }
> +
> + /// Returns a mutable reference to the driver-specific context.
> + pub fn to_driver_uctx_mut(&mut self) -> &mut T {
> + &mut self.uctx
> + }
As mentioned, I think Deref and DerefMut are a better fit for this.
> +}
> +
> +/// Static vtable mapping Rust trait methods to C callbacks.
> +pub struct FwCtlVTable<T: FwCtlOps>(PhantomData<T>);
> +
> +impl<T: FwCtlOps> FwCtlVTable<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,
> + uctx_size: core::mem::size_of::<FwCtlUCtx<T::UCtx>>(),
> + 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.
> + unsafe extern "C" fn open_uctx_callback(uctx: *mut bindings::fwctl_uctx) -> ffi::c_int {
> + // SAFETY: `uctx` is guaranteed by the fwctl subsystem to be a valid pointer.
> + let ctx = unsafe { FwCtlUCtx::<T::UCtx>::from_raw(uctx) };
> + match T::open_uctx(ctx) {
> + Ok(()) => 0,
> + Err(e) => e.to_errno(),
> + }
> + }
> +
> + /// Called when the user context is being closed.
> + 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 { FwCtlUCtx::<T::UCtx>::from_raw(uctx) };
> + T::close_uctx(ctx);
> + }
> +
> + /// Returns device or context information.
> + 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 { FwCtlUCtx::<T::UCtx>::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.
> + 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 { FwCtlUCtx::<T::UCtx>::from_raw(uctx) };
> +
> + // SAFETY: `rpc_in` points to a valid input buffer of size `in_len`
> + // provided by fwctl subsystem.
Please see the safety requirements of slice::from_raw_parts_mut() and justify
all of them.
> + let rpc_in_slice: &mut [u8] =
> + unsafe { slice::from_raw_parts_mut(rpc_in as *mut 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;
> + }
NIT: If you move the semicolon at the end of the unsafe block, this is formatted
in a single line.
> +
> + ptr.cast::<ffi::c_void>()
> + }
> +
> + // Driver re-uses the existing input buffer and writes the out_len.
> + Ok(None) => rpc_in,
> +
> + // Return an ERR_PTR-style encoded error pointer.
> + Err(e) => Error::to_ptr(e),
> + }
> + }
> +}
On Thu, Oct 30, 2025 at 04:03:12PM +0000, Zhi Wang wrote:
> +impl<T: FwCtlOps> Registration<T> {
> + /// Allocate and register a new fwctl device under the given parent device.
> + pub fn new(parent: &device::Device) -> Result<Self> {
> + let ops = &FwCtlVTable::<T>::VTABLE as *const _ as *mut _;
> +
> + // SAFETY: `_fwctl_alloc_device()` allocates a new `fwctl_device`
> + // and initializes its embedded `struct device`.
> + let dev = unsafe {
> + bindings::_fwctl_alloc_device(
> + parent.as_raw(),
> + ops,
> + core::mem::size_of::<bindings::fwctl_device>(),
> + )
> + };
> +
> + let dev = NonNull::new(dev).ok_or(ENOMEM)?;
> +
> + // SAFETY: `fwctl_register()` expects a valid device from `_fwctl_alloc_device()`.
> + let ret = unsafe { bindings::fwctl_register(dev.as_ptr()) };
This is a Bound device, not just any device.
> + if ret != 0 {
> + // SAFETY: If registration fails, release the allocated fwctl_device().
> + unsafe {
> + bindings::put_device(core::ptr::addr_of_mut!((*dev.as_ptr()).dev));
?? Don't open code fwctl_put() - it should be called directly?
> + }
> + return Err(Error::from_errno(ret));
> + }
> +
> + Ok(Self {
> + fwctl_dev: dev,
> + _marker: PhantomData,
> + })
> + }
> +
> + fn as_raw(&self) -> *mut bindings::fwctl_device {
> + self.fwctl_dev.as_ptr()
> + }
> +}
> +
> +impl<T: FwCtlOps> Drop for Registration<T> {
> + fn drop(&mut self) {
> + // SAFETY: `fwctl_unregister()` expects a valid device from `_fwctl_alloc_device()`.
Incomplete safety statement, the device passed to fwctl_alloc_device must
still be bound prior to calling fwctl_unregister
> + unsafe {
> + bindings::fwctl_unregister(self.as_raw());
> + bindings::put_device(core::ptr::addr_of_mut!((*self.as_raw()).dev));
There for Drop can only do fwctl_put() since otherwise there is no way
to guarantee a Bound device.
unregister has to happen before remove() completes, Danilo had some
approach to this I think he told me?
Jason
On Thu Oct 30, 2025 at 5:22 PM CET, Jason Gunthorpe wrote:
> On Thu, Oct 30, 2025 at 04:03:12PM +0000, Zhi Wang wrote:
>> +impl<T: FwCtlOps> Registration<T> {
>> + /// Allocate and register a new fwctl device under the given parent device.
>> + pub fn new(parent: &device::Device) -> Result<Self> {
>> + let ops = &FwCtlVTable::<T>::VTABLE as *const _ as *mut _;
>> +
>> + // SAFETY: `_fwctl_alloc_device()` allocates a new `fwctl_device`
>> + // and initializes its embedded `struct device`.
>> + let dev = unsafe {
>> + bindings::_fwctl_alloc_device(
>> + parent.as_raw(),
>> + ops,
>> + core::mem::size_of::<bindings::fwctl_device>(),
>> + )
>> + };
>> +
>> + let dev = NonNull::new(dev).ok_or(ENOMEM)?;
>> +
>> + // SAFETY: `fwctl_register()` expects a valid device from `_fwctl_alloc_device()`.
>> + let ret = unsafe { bindings::fwctl_register(dev.as_ptr()) };
>
> This is a Bound device, not just any device.
Indeed, the safety comment should mention this. And if it would it could not
justify it with the current code, since the function takes a &Device instead of
the required &Device<Bound> argument.
>> + if ret != 0 {
>> + // SAFETY: If registration fails, release the allocated fwctl_device().
>> + unsafe {
>> + bindings::put_device(core::ptr::addr_of_mut!((*dev.as_ptr()).dev));
>
> ?? Don't open code fwctl_put() - it should be called directly?
>
>> + }
>> + return Err(Error::from_errno(ret));
>> + }
>> +
>> + Ok(Self {
>> + fwctl_dev: dev,
>> + _marker: PhantomData,
>> + })
>> + }
>> +
>> + fn as_raw(&self) -> *mut bindings::fwctl_device {
>> + self.fwctl_dev.as_ptr()
>> + }
>> +}
>> +
>> +impl<T: FwCtlOps> Drop for Registration<T> {
>> + fn drop(&mut self) {
>> + // SAFETY: `fwctl_unregister()` expects a valid device from `_fwctl_alloc_device()`.
>
> Incomplete safety statement, the device passed to fwctl_alloc_device must
> still be bound prior to calling fwctl_unregister
>
>> + unsafe {
>> + bindings::fwctl_unregister(self.as_raw());
>> + bindings::put_device(core::ptr::addr_of_mut!((*self.as_raw()).dev));
>
> There for Drop can only do fwctl_put() since otherwise there is no way
> to guarantee a Bound device.
>
> unregister has to happen before remove() completes, Danilo had some
> approach to this I think he told me?
Yeah, such Registration structures of (class) devices should be wrapped into a
Devres container (i.e. Devres<fwctl::Registration>)to be able to provide this
guarantee. See also my other reply to this patch [1].
While not a class device, the auxiliary bus with its auxiliary::Registration
[2], is a good example.
Alternatively (or additionally), it can also be implemented in a way that the
driver does not get control over a Registration object at all, but once created
it is not accessible anymore and automatically dropped on parent device unbind.
This approach is used by cpufreq [3].
It always depends on whether a driver might want to drop the Registration
manually before device unbind.
[1] https://lore.kernel.org/lkml/DDVT5YA564C6.3HN9WCMQX49PC@kernel.org/
[2] https://git.kernel.org/pub/scm/linux/kernel/git/driver-core/driver-core.git/tree/rust/kernel/auxiliary.rs?id=b0b7301b004301afe920b3d08caa6171dd3f4011#n304
[3] https://rust.docs.kernel.org/kernel/cpufreq/struct.Registration.html#method.new_foreign_owned
On 30.10.2025 18.22, Jason Gunthorpe wrote:
> On Thu, Oct 30, 2025 at 04:03:12PM +0000, Zhi Wang wrote:
>> +impl<T: FwCtlOps> Registration<T> {
>> + /// Allocate and register a new fwctl device under the given parent device.
>> + pub fn new(parent: &device::Device) -> Result<Self> {
>> + let ops = &FwCtlVTable::<T>::VTABLE as *const _ as *mut _;
>> +
>> + // SAFETY: `_fwctl_alloc_device()` allocates a new `fwctl_device`
>> + // and initializes its embedded `struct device`.
>> + let dev = unsafe {
>> + bindings::_fwctl_alloc_device(
>> + parent.as_raw(),
>> + ops,
>> + core::mem::size_of::<bindings::fwctl_device>(),
>> + )
>> + };
>> +
>> + let dev = NonNull::new(dev).ok_or(ENOMEM)?;
>> +
>> + // SAFETY: `fwctl_register()` expects a valid device from `_fwctl_alloc_device()`.
>> + let ret = unsafe { bindings::fwctl_register(dev.as_ptr()) };
>
> This is a Bound device, not just any device.
>
Will do this in the next re-spin. It needs extra abstraction around fwctl_device.
>> + if ret != 0 {
>> + // SAFETY: If registration fails, release the allocated fwctl_device().
>> + unsafe {
>> + bindings::put_device(core::ptr::addr_of_mut!((*dev.as_ptr()).dev));
>
> ?? Don't open code fwctl_put() - it should be called directly?
fwctl_put() is a inline function and it seem opened by the compiler when bindings are
generated.
$ grep -R fwctl_put *
$ pwd
/home/inno/drm-rust/rust/bindings
$ grep -R put_device *
bindings_generated.rs: pub fn put_device(dev: *mut device);
Hehe. I am open to options.
Z.
>
>> + }
>> + return Err(Error::from_errno(ret));
>> + }
>> +
>> + Ok(Self {
>> + fwctl_dev: dev,
>> + _marker: PhantomData,
>> + })
>> + }
>> +
>> + fn as_raw(&self) -> *mut bindings::fwctl_device {
>> + self.fwctl_dev.as_ptr()
>> + }
>> +}
>> +
>> +impl<T: FwCtlOps> Drop for Registration<T> {
>> + fn drop(&mut self) {
>> + // SAFETY: `fwctl_unregister()` expects a valid device from `_fwctl_alloc_device()`.
>
> Incomplete safety statement, the device passed to fwctl_alloc_device must
> still be bound prior to calling fwctl_unregister
>
>> + unsafe {
>> + bindings::fwctl_unregister(self.as_raw());
>> + bindings::put_device(core::ptr::addr_of_mut!((*self.as_raw()).dev));
>
> There for Drop can only do fwctl_put() since otherwise there is no way
> to guarantee a Bound device.
>
> unregister has to happen before remove() completes, Danilo had some
> approach to this I think he told me?
>
> Jason
On Thu Oct 30, 2025 at 6:19 PM CET, Zhi Wang wrote: > fwctl_put() is a inline function and it seem opened by the compiler when bindings are > generated. > > $ grep -R fwctl_put * > $ pwd > /home/inno/drm-rust/rust/bindings > $ grep -R put_device * > bindings_generated.rs: pub fn put_device(dev: *mut device); > > Hehe. I am open to options. You can add them to rust/helpers/.
© 2016 - 2026 Red Hat, Inc.