Re: [PATCH v4 2/2] rust: introduce abstractions for fwctl

["Danilo Krummrich" <dakr@kernel.org>]

On Thu Jun 25, 2026 at 10:24 AM CEST, Zhi Wang wrote:
> Thanks so much. I will take a look and re-spin it today.

This also works with a GAT lifetime and doesn't need ForLt in the first place.
Please find the updated diff below.

Thanks,
Danilo

diff --git a/rust/kernel/fwctl.rs b/rust/kernel/fwctl.rs
index f5f802f5299c..80e592a21bfc 100644
--- a/rust/kernel/fwctl.rs
+++ b/rust/kernel/fwctl.rs
@@ -5,19 +5,18 @@
 //! C header: `include/linux/fwctl.h`

 use crate::{
-    bindings,
-    container_of,
-    device,
-    devres::Devres,
+    bindings, container_of, device,
     prelude::*,
     sync::aref::{
         ARef,
         AlwaysRefCounted, //
     },
-    types::Opaque, //
+    types::Opaque,
 };
 use core::{
+    cell::UnsafeCell,
     marker::PhantomData,
+    mem,
     ptr::NonNull,
     slice, //
 };
@@ -88,9 +87,15 @@ pub enum FwRpcResponse {
 /// 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 + Send + Sync {
-    /// Driver data embedded alongside the `fwctl_device` allocation.
-    type DeviceData: Send + Sync;
+pub trait Operations: Sized + Send + Sync + 'static {
+    /// Data owned by the [`Registration`] and accessible during callbacks.
+    ///
+    /// The lifetime `'a` is tied to the [`Registration`] scope (which lives within the parent bus
+    /// device binding scope). Drivers use this to store references to resources bound to this
+    /// scope, such as PCI BARs or typed bus device references.
+    type RegistrationData<'a>: Send + Sync + 'a
+    where
+        Self: 'a;

     /// fwctl device type identifier.
     const DEVICE_TYPE: DeviceType;
@@ -99,57 +104,68 @@ pub trait Operations: Sized + Send + Sync {
     ///
     /// Returns a [`PinInit`] initializer for `Self`. The instance is dropped
     /// automatically when the FD is closed (after [`close`](Self::close)).
-    fn open(device: &Device<Self>) -> impl PinInit<Self, Error>;
+    fn open<'a>(
+        device: &'a Device<Self>,
+        reg_data: &'a Self::RegistrationData<'a>,
+    ) -> impl PinInit<Self, Error>;

     /// Called when the user context is closed.
     ///
     /// The driver may perform additional cleanup here that requires access
     /// to the owning [`Device`]. `Self` is dropped automatically after this
     /// returns.
-    fn close(_this: Pin<&mut Self>, _device: &Device<Self>) {}
+    fn close<'a>(
+        _this: Pin<&mut Self>,
+        _device: &'a Device<Self>,
+        _reg_data: &'a Self::RegistrationData<'a>,
+    ) {
+    }

     /// Return device information to userspace.
     ///
     /// The default implementation returns no device-specific data.
-    fn info(_this: Pin<&Self>, _device: &Device<Self>) -> Result<KVec<u8>, Error> {
+    fn info<'a>(
+        _this: Pin<&Self>,
+        _device: &'a Device<Self>,
+        _reg_data: &'a Self::RegistrationData<'a>,
+    ) -> Result<KVec<u8>, Error> {
         Ok(KVec::new())
     }

     /// Handle a userspace RPC request.
-    fn fw_rpc(
+    fn fw_rpc<'a>(
         this: Pin<&Self>,
-        device: &Device<Self>,
+        device: &'a Device<Self>,
+        reg_data: &'a Self::RegistrationData<'a>,
         scope: RpcScope,
         rpc_in: &mut [u8],
     ) -> Result<FwRpcResponse, Error>;
 }

-/// A fwctl device with embedded driver data.
+/// A fwctl device.
 ///
-/// `#[repr(C)]` with the `fwctl_device` at offset 0, matching the C
-/// `fwctl_alloc_device()` layout convention.
+/// `#[repr(C)]` with the `fwctl_device` at offset 0, matching the C `fwctl_alloc_device()` layout
+/// convention. Contains a pointer to the [`Registration`]'s data, set at registration time and
+/// cleared on unregistration.
 ///
 /// # Invariants
 ///
 /// - `dev` is embedded at offset 0 and is initialised by fwctl.
 /// - The fwctl refcount owns the allocation lifetime.
-/// - `data` is dropped from the fwctl core release hook before `kfree()`.
+/// - `registration_data` is either `NonNull::dangling()` (before registration / after
+///   unregistration) or points to a valid `RegistrationData` owned by the [`Registration`].
 #[repr(C)]
 pub struct Device<T: Operations> {
     dev: Opaque<bindings::fwctl_device>,
-    data: T::DeviceData,
+    registration_data: UnsafeCell<NonNull<T::RegistrationData<'static>>>,
 }

 impl<T: Operations> Device<T> {
-    /// Allocate a new fwctl device with embedded driver data.
+    /// Allocate a new fwctl device.
     ///
     /// Returns an [`ARef`] that can be passed to [`Registration::new()`]
-    /// to make the device visible to userspace. The caller may inspect or
-    /// configure the device between allocation and registration.
-    pub fn new(
-        parent: &device::Device<device::Bound>,
-        data: impl PinInit<T::DeviceData, Error>,
-    ) -> Result<ARef<Self>> {
+    /// 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"
@@ -157,8 +173,7 @@ pub fn new(

         let ops = core::ptr::from_ref::<bindings::fwctl_ops>(&VTable::<T>::VTABLE).cast_mut();

-        // SAFETY: `ops` is static, `parent` is bound, and `size` includes the
-        // offset-0 `fwctl_device` plus `DeviceData`.
+        // 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>())
         };
@@ -169,42 +184,21 @@ pub fn new(

         let this = raw.cast::<Self>();

+        // INVARIANT: Set `registration_data` to dangling (no registration yet).
         // SAFETY: `this` points to the allocation just returned by fwctl.
-        let data_ptr = unsafe { core::ptr::addr_of_mut!((*this).data) };
-        // SAFETY: `data_ptr` addresses the uninitialised tail data.
-        unsafe { data.__pinned_init(data_ptr) }.inspect_err(|_| {
-            // SAFETY: `raw` still owns the initial reference.
-            unsafe { bindings::fwctl_put(raw) };
-        })?;
-
-        // SAFETY: `raw` is a live fwctl_device allocated above.
-        unsafe { (*raw).release_data = Some(Self::release_data_callback) };
-
-        // SAFETY: `raw` owns the initial reference and `DeviceData` is ready.
-        Ok(unsafe { ARef::from_raw(NonNull::new(raw.cast::<Self>()).ok_or(ENOMEM)?) })
-    }
+        unsafe {
+            core::ptr::addr_of_mut!((*this).registration_data)
+                .write(UnsafeCell::new(NonNull::dangling()));
+        };

-    /// Returns a reference to the embedded driver data.
-    pub fn data(&self) -> &T::DeviceData {
-        &self.data
+        // SAFETY: `raw` owns the initial reference.
+        Ok(unsafe { ARef::from_raw(NonNull::new_unchecked(this)) })
     }

     fn as_raw(&self) -> *mut bindings::fwctl_device {
         self.dev.get()
     }

-    /// # Safety
-    ///
-    /// `raw` must point to an offset-0 `fwctl_device` embedded in `Device<T>`.
-    /// fwctl calls this exactly once from the device release path.
-    unsafe extern "C" fn release_data_callback(raw: *mut bindings::fwctl_device) {
-        let this = raw.cast::<Self>();
-
-        // SAFETY: fwctl invokes this callback once during the final device
-        // release, before freeing the allocation.
-        unsafe { core::ptr::drop_in_place(core::ptr::addr_of_mut!((*this).data)) };
-    }
-
     /// # Safety
     ///
     /// `ptr` must point to a valid `fwctl_device` embedded in a `Device<T>`.
@@ -213,18 +207,18 @@ unsafe fn from_raw<'a>(ptr: *mut bindings::fwctl_device) -> &'a Self {
         unsafe { &*ptr.cast() }
     }

-    /// Returns the parent device.
+    /// Returns a reference to the registration data.
+    ///
+    /// # Safety
     ///
-    /// The parent is guaranteed to be bound while any fwctl callback is
-    /// running (ensured by the `registration_lock` read lock on the ioctl
-    /// path and by `Devres` on the teardown path).
-    pub fn parent(&self) -> &device::Device<device::Bound> {
-        // SAFETY: fwctl sets the parent during allocation.
-        let parent_dev = unsafe { (*self.as_raw()).dev.parent };
-        // SAFETY: The parent stays live while fwctl ops run.
-        let dev: &device::Device = unsafe { device::Device::from_raw(parent_dev) };
-        // SAFETY: Devres teardown keeps the parent bound here.
-        unsafe { dev.as_bound() }
+    /// The caller must ensure the device is registered, i.e. that this is called within a fwctl
+    /// callback protected by `registration_lock`. The pointer cast from
+    /// `RegistrationData<'static>` to `RegistrationData<'_>` is sound because lifetimes do not
+    /// affect layout, and the data is guaranteed alive for the duration of the borrow.
+    unsafe fn registration_data_unchecked(&self) -> &T::RegistrationData<'_> {
+        // SAFETY: Caller guarantees the device is registered, so the pointer is valid.
+        // Lifetimes do not affect layout, so the cast is sound.
+        unsafe { (*self.registration_data.get()).cast::<_>().as_ref() }
     }
 }

@@ -251,62 +245,94 @@ unsafe fn dec_ref(obj: NonNull<Self>) {
     }
 }

-// SAFETY: `Device<T>` is refcounted by the fwctl core and may be released from
-// any thread. The embedded driver data is `Send`.
+// SAFETY: `Device<T>` is refcounted by the fwctl core and may be released from any thread.
 unsafe impl<T: Operations> Send for Device<T> {}

-// SAFETY: Shared access to the embedded `fwctl_device` is protected by the
-// fwctl core, and the embedded driver data is `Sync`.
+// SAFETY: Shared access to the embedded `fwctl_device` is protected by the fwctl core. The
+// `registration_data` field is only mutated before registration and after unregistration (both
+// single-threaded with respect to callbacks).
 unsafe impl<T: Operations> Sync for Device<T> {}

 /// A registered fwctl device.
 ///
-/// Must live inside a [`Devres`] to guarantee that [`fwctl_unregister`] runs
-/// before the parent driver unbinds. `Devres` prevents the `Registration`
-/// from being moved to a context that could outlive the parent device.
+/// Carries the lifetime `'a` of the parent device to ensure that [`fwctl_unregister`] runs (via
+/// [`Drop`]) before the parent driver unbinds. Owns the
+/// [`RegistrationData`](Operations::RegistrationData) which is accessible during callbacks via the
+/// pointer stored in [`Device`].
 ///
-/// On drop the device is unregistered (all user contexts are closed and
-/// `ops` is set to `NULL`) and the [`ARef`] is released.
+/// On drop the device is unregistered (all user contexts are closed and `ops` is set to `NULL`)
+/// and the registration data is dropped.
 ///
 /// [`fwctl_unregister`]: srctree/drivers/fwctl/main.c
-pub struct Registration<T: Operations> {
+pub struct Registration<'a, T: Operations> {
     dev: ARef<Device<T>>,
+    _reg_data: Pin<KBox<T::RegistrationData<'a>>>,
 }

-impl<T: Operations> Registration<T> {
-    /// Register a previously allocated fwctl device.
-    pub fn new<'a>(
-        parent: &'a device::Device<device::Bound>,
-        dev: &'a Device<T>,
-    ) -> impl PinInit<Devres<Self>, Error> + 'a {
-        pin_init::pin_init_scope(move || {
-            // SAFETY: `dev` is a valid fwctl_device backed by an ARef.
-            let ret = unsafe { bindings::fwctl_register(dev.as_raw()) };
-            if ret != 0 {
-                return Err(Error::from_errno(ret));
-            }
+impl<'a, T: Operations> Registration<'a, T> {
+    /// Register a previously allocated fwctl device with the given registration data.
+    ///
+    /// The `reg_data` is owned by the registration and accessible during callbacks via
+    /// `Device::registration_data_unchecked()`.
+    ///
+    /// # Safety
+    ///
+    /// Callers must not `mem::forget()` the returned [`Registration`] or otherwise prevent its
+    /// [`Drop`] implementation from running, since `fwctl_unregister` must be called before the
+    /// parent device is unbound.
+    pub unsafe fn new(
+        _parent: &'a device::Device<device::Bound>,
+        dev: &Device<T>,
+        reg_data: impl PinInit<T::RegistrationData<'a>, Error>,
+    ) -> Result<Self> {
+        let reg_data: Pin<KBox<T::RegistrationData<'a>>> = KBox::pin_init(reg_data, GFP_KERNEL)?;
+
+        // Store the registration data pointer in the device before registration, so that it is
+        // visible once callbacks can be invoked.
+        //
+        // SAFETY: Lifetimes do not affect layout, so the pointer cast is sound.
+        let ptr: NonNull<T::RegistrationData<'static>> =
+            unsafe { mem::transmute(NonNull::from(Pin::get_ref(reg_data.as_ref()))) };
+
+        // SAFETY: No concurrent access; the device is not yet registered.
+        unsafe { *dev.registration_data.get() = ptr };
+
+        // SAFETY: `dev` is a valid fwctl_device backed by an ARef.
+        let ret = unsafe { bindings::fwctl_register(dev.as_raw()) };
+        if ret != 0 {
+            // SAFETY: No concurrent readers; registration failed.
+            unsafe { *dev.registration_data.get() = NonNull::dangling() };
+            return Err(Error::from_errno(ret));
+        }

-            Ok(Devres::new(parent, Self { dev: dev.into() }))
+        Ok(Self {
+            dev: dev.into(),
+            _reg_data: reg_data,
         })
     }
 }

-impl<T: Operations> Drop for Registration<T> {
+impl<T: Operations> Drop for Registration<'_, T> {
     fn drop(&mut self) {
-        // SAFETY: `Registration` lives inside a `Devres`, which guarantees
-        // that drop runs while the parent device is still bound.
+        // SAFETY: The Registration lifetime guarantees that the parent device is still bound.
+        // `fwctl_unregister` takes the write lock, closes all user contexts, and sets ops=NULL.
+        // After it returns, no callbacks can be running or will run.
         unsafe { bindings::fwctl_unregister(self.dev.as_raw()) };
-        // ARef<Device<T>> is dropped after this, calling fwctl_put.
+
+        // SAFETY: `fwctl_unregister` guarantees no concurrent readers.
+        unsafe { *self.dev.registration_data.get() = NonNull::dangling() };
+
+        // `self._reg_data` is dropped here — safe because no concurrent readers remain.
     }
 }

-// SAFETY: `Registration` can be sent between threads; the underlying
-// fwctl_device uses internal locking.
-unsafe impl<T: Operations> Send for Registration<T> {}
+// SAFETY: `Registration` can be sent between threads; the underlying fwctl_device uses internal
+// locking.
+unsafe impl<T: Operations> Send for Registration<'_, T> {}

-// SAFETY: `Registration` provides no mutable access; the underlying
-// fwctl_device is protected by internal locking.
-unsafe impl<T: Operations> Sync for Registration<T> {}
+// SAFETY: `Registration` provides no mutable access; the underlying fwctl_device is protected by
+// internal locking.
+unsafe impl<T: Operations> Sync for Registration<'_, T> {}

 /// Internal per-FD user context wrapping `struct fwctl_uctx` and `T`.
 ///
@@ -376,7 +402,10 @@ impl<T: Operations> VTable<T> {
         // SAFETY: Rust fwctl devices use the offset-0 `Device<T>` layout.
         let device = unsafe { Device::<T>::from_raw(raw_fwctl) };

-        let initializer = T::open(device);
+        // 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>`.
@@ -396,13 +425,17 @@ impl<T: Operations> VTable<T> {
         // SAFETY: fwctl keeps the owning device live for this callback.
         let device = unsafe { Device::<T>::from_raw((*uctx).fwctl) };

+        // SAFETY: close_uctx is called under registration_lock write (from fwctl_unregister) or
+        // under registration_lock read (from fwctl_fops_release); the device is registered.
+        let reg_data = unsafe { device.registration_data_unchecked() };
+
         // SAFETY: close is called for an opened Rust user context.
         let ctx = unsafe { UserCtx::<T>::from_raw_mut(uctx) };

         {
             // SAFETY: fwctl never moves an opened user context.
             let pinned = unsafe { Pin::new_unchecked(&mut ctx.uctx) };
-            T::close(pinned, device);
+            T::close(pinned, device, reg_data);
         }

         // SAFETY: close is the last callback before fwctl frees the allocation.
@@ -421,10 +454,13 @@ impl<T: Operations> VTable<T> {
         let ctx = unsafe { UserCtx::<T>::from_raw(uctx) };
         let device = ctx.device();

+        // SAFETY: info is called under registration_lock read; the device is registered.
+        let reg_data = unsafe { device.registration_data_unchecked() };
+
         // SAFETY: fwctl never moves an opened user context.
         let pinned = unsafe { Pin::new_unchecked(&ctx.uctx) };

-        match T::info(pinned, device) {
+        match T::info(pinned, device, reg_data) {
             Ok(kvec) if kvec.is_empty() => {
                 // SAFETY: `length` is a valid out-parameter.
                 unsafe { *length = 0 };
@@ -461,6 +497,9 @@ impl<T: Operations> VTable<T> {
         let ctx = unsafe { UserCtx::<T>::from_raw(uctx) };
         let device = ctx.device();

+        // SAFETY: fw_rpc is called under registration_lock read; the device is registered.
+        let reg_data = unsafe { device.registration_data_unchecked() };
+
         // SAFETY: fwctl passes a valid in/out buffer for this callback.
         let rpc_in_slice: &mut [u8] =
             unsafe { slice::from_raw_parts_mut(rpc_in.cast::<u8>(), in_len) };
@@ -468,7 +507,7 @@ impl<T: Operations> VTable<T> {
         // SAFETY: fwctl never moves an opened user context.
         let pinned = unsafe { Pin::new_unchecked(&ctx.uctx) };

-        match T::fw_rpc(pinned, device, scope, rpc_in_slice) {
+        match T::fw_rpc(pinned, device, reg_data, scope, rpc_in_slice) {
             Ok(FwRpcResponse::InPlace(len)) => {
                 // SAFETY: `out_len` is valid.
                 unsafe { *out_len = len };