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

[Zhi Wang]

On Sat, 04 Jul 2026 21:42:59 +0200
"Danilo Krummrich" <dakr@xxxxxxxxxx> wrote:

> On Mon Jun 29, 2026 at 5:01 PM CEST, Zhi Wang wrote:
> > Introduce safe Rust wrappers around struct fwctl_device and

snip

> 
> This also needs:
> 
> Signed-off-by: Danilo Krummrich <dakr@xxxxxxxxxx>
> 
> > Link: https://lore.kernel.org/r/DJJW7X4ESDSM.QCVYK2FC7ZR3@xxxxxxxxxx
> > Signed-off-by: Zhi Wang <zhiw@xxxxxxxxxx>  
> 

Thanks, will add it.

> Few remaining nits below.
> 
> Jason, would you be fine if I take this through the DRM tree, so Nova
> can build on top of it right away, or do you want to take it through
> your fwctl tree?
> 
> > ---
> >  drivers/fwctl/Kconfig           |  12 +
> >  rust/bindings/bindings_helper.h |   1 +
> >  rust/helpers/fwctl.c            |  17 +
> >  rust/helpers/helpers.c          |   3 +-
> >  rust/kernel/fwctl.rs            | 546
> > ++++++++++++++++++++++++++++++++ rust/kernel/lib.rs              |
> >  2 + 6 files changed, 580 insertions(+), 1 deletion(-)
> >  create mode 100644 rust/helpers/fwctl.c
> >  create mode 100644 rust/kernel/fwctl.rs  
> 
> 

snip

> NIT: Do we want to add all device types even though they do not (plan
> to) have a Rust driver?
>

I think they are not intended as placeholders for future Rust drivers.
`DeviceType` models the valid `fwctl_device_type` UAPI domain, so I
kept all existing non-error values while leaving
`FWCTL_DEVICE_TYPE_ERROR` and arbitrary integers unrepresentable.

`FWCTL_DEVICE_TYPE_NOVA_CORE` has not been allocated in the C UAPI
yet, so it is intentionally not included here. I plan to add
`DeviceType::NovaCore` together with the C UAPI value and the nova-core
consumer.

Do you have any better idea about this? We don't have a dummy driver as
well...


snip

> > +    /// Called when a new user context is opened.
> > +    ///
> > +    /// Returns a [`PinInit`] initializer for `Self`. The instance
> > is dropped
> > +    /// automatically when the FD is closed (after
> > [`close`](Self::close)).
> > +    fn open<'a>(
> > +        device: &'a Device<Self>,
> > +        reg_data: &'a Self::RegistrationData<'a>,
> > +    ) -> impl PinInit<Self, Error>;  
> 
> I think this can just be
> 
> 	fn open<'a>(
> 	    device: &Device<Self>,
> 	    reg_data: &Self::RegistrationData<'a>,
> 	) -> impl PinInit<Self, Error>;
> 

Agree.

> > +impl<T: Operations> Device<T> {
> > +    /// Allocate a new fwctl device.
> > +    ///
> > +    /// Returns an [`ARef`] that can be passed to
> > [`Registration::new()`]
> > +    /// to make the device visible to userspace.
> > +    pub fn new(parent: &device::Device<device::Bound>) ->
> > Result<ARef<Self>> {
> > +        const_assert!(
> > +            core::mem::offset_of!(Self, dev) == 0,
> > +            "struct fwctl_device must be at offset 0"
> > +        );
> > +
> > +        let ops =
> > core::ptr::from_ref::<bindings::fwctl_ops>(&VTable::<T>::VTABLE).cast_mut();
> > +
> > +        // SAFETY: `ops` is static, `parent` is bound, and `size`
> > covers the full `Device<T>`.
> > +        let raw = unsafe {
> > +            bindings::_fwctl_alloc_device(parent.as_raw(), ops,
> > core::mem::size_of::<Self>())
> > +        };
> > +
> > +        if raw.is_null() {
> > +            return Err(ENOMEM);
> > +        }
> > +
> > +        let this = raw.cast::<Self>();
> > +
> > +        // INVARIANT: Set `registration_data` to dangling (no
> > registration yet).
> > +        // SAFETY: `this` points to the allocation just returned
> > by fwctl.
> > +        unsafe {
> > +            core::ptr::addr_of_mut!((*this).registration_data)  
> 
> Here and in a few other places, this can use &raw mut instead.
> 
> > +/// Static vtable mapping Rust trait methods to C callbacks.
> > +pub struct VTable<T: Operations>(PhantomData<T>);  
> 
> Why does this need public visibility?
> 

Will fix them in the next re-spin.

> > +
> > +impl<T: Operations> VTable<T> {
> > +    /// The fwctl operations vtable for this driver type.
> > +    pub const VTABLE: bindings::fwctl_ops = bindings::fwctl_ops {
> > +        device_type: T::DEVICE_TYPE as u32,
> > +        uctx_size: core::mem::size_of::<UserCtx<T>>(),
> > +        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),
> > +    };  
> 
> Same here.
> 
> > +
> > +    /// # Safety
> > +    ///
> > +    /// `uctx` must be a valid `fwctl_uctx` embedded in a
> > `UserCtx<T>` with
> > +    /// sufficient allocated space for the uctx field.
> > +    unsafe extern "C" fn open_uctx_callback(uctx: *mut
> > bindings::fwctl_uctx) -> ffi::c_int {
> > +        const_assert!(
> > +            core::mem::offset_of!(UserCtx<T>, fwctl_uctx) == 0,
> > +            "struct fwctl_uctx must be at offset 0"
> > +        );
> > +
> > +        // SAFETY: fwctl sets this pointer before calling
> > `open_uctx`.
> > +        let raw_fwctl = unsafe { (*uctx).fwctl };
> > +        // SAFETY: Rust fwctl devices use the offset-0 `Device<T>`
> > layout.
> > +        let device = unsafe { Device::<T>::from_raw(raw_fwctl) };
> > +
> > +        // SAFETY: open_uctx is called under registration_lock
> > read; the device is registered.
> > +        let reg_data = unsafe {
> > device.registration_data_unchecked() }; +
> > +        let initializer = T::open(device, reg_data);
> > +
> > +        let uctx_offset = core::mem::offset_of!(UserCtx<T>, uctx);
> > +        // SAFETY: `uctx_size` reserves space for the full
> > `UserCtx<T>`.
> > +        let uctx_ptr: *mut T = unsafe {
> > uctx.cast::<u8>().add(uctx_offset).cast() };  
> 
> This can use byte_add() instead.
> 
> > +            Ok(FwRpcResponse::NewBuffer(kvec)) => {
> > +                let (ptr, len, _cap) = kvec.into_raw_parts();
> > +                // SAFETY: `out_len` is valid.
> > +                unsafe { *out_len = len };
> > +                ptr.cast::<ffi::c_void>()  
> 
> This probably needs a kvec.is_empty() check, since an empty vector
> contains a dangling pointer.

Yup. Will fix above in the next re-spin.