[PATCH v4 0/5] Improvements for Devres

[PATCH v4 0/5] Improvements for Devres

[Danilo Krummrich]

This patch series provides some optimizations for Devres:

  1) Provide a more lightweight replacement for Devres::new_foreign_owned().

  2) Get rid of Devres' inner Arc and instead consume and provide an
     impl PinInit instead.

     Additionally, having the resulting explicit synchronization in
     Devres::drop() prevents potential subtle undesired side effects of the
     devres callback dropping the final Arc reference asynchronously within
     the devres callback.

  3) An optimization for when we never need to access the resource or release
     it manually.

Thanks to Alice and Benno for some great offline discussions on this topic.

This patch series depends on the Opaque patch in [1] and the pin-init patch in
[2], which Benno will provide a signed tag for. A branch containing the patches
can be found in [3].

[1] https://lore.kernel.org/lkml/20250610-b4-rust_miscdevice_registrationdata-v6-1-b03f5dfce998@gmail.com/
[2] https://lore.kernel.org/rust-for-linux/20250529081027.297648-2-lossin@kernel.org/
[3] https://git.kernel.org/pub/scm/linux/kernel/git/dakr/linux.git/log/?h=rust/devres

Changes in v4:
  - devres::register:
    - require T: Send and ForeignOwnable: Send
  - Devres:
    - require T: Send
    - document possible changes when switching from Opaque to UnsafePinned, once
      possible
  - devres::register_release():
    - rework to Benno's proposal of Release::Ptr
    - require P::Target: Send
  - add a patch adding ForeignOwnable::Target, i.e. the type of the payload data
    (required for devres::register_release())

Changes in v3:
  - devres::register:
    - add 'static bound for ForeignOwnable
    - use drop() instead of `let _ =`
  - Devres:
    - use ptr::from_ref() instead of Inner::as_ptr()
    - use &raw mut instead of addr_of_mut!()
    - use SAFETY comment proposal from Benno
    - add invariant for `Devres::inner`
    - use ScopeGuard in devres_callback()
  - devres::register_release():
    - add impl<T: Release> Release for &T
    - add 'static bound for ForeignOwnable
    - use drop() instead of `let _ =`

Changes in v2:
  - Revocable:
    - remove Error: From<E> bound
  - devres::register:
    - rename devres::register_foreign_boxed() to just devres::register()
    - move T: 'static bound to the function rather than the impl block
  - Devres:
    - Fix aliasing issue by using an Opaque<Inner>; should be
      UnsafePinned<Inner> once available.
    - Add doc-comments for a couple of private fields.
    - Link Revocable on 'revoke' in Devres::new().
  - devres::register_release():
    - expand documentation of Release
    - rename devres::register_foreign_release() for devres::register_release()

Danilo Krummrich (5):
  rust: revocable: support fallible PinInit types
  rust: devres: replace Devres::new_foreign_owned()
  rust: devres: get rid of Devres' inner Arc
  rust: types: ForeignOwnable: Add type Target
  rust: devres: implement register_release()

 drivers/gpu/nova-core/driver.rs |   7 +-
 drivers/gpu/nova-core/gpu.rs    |   6 +-
 rust/helpers/device.c           |   7 +
 rust/kernel/alloc/kbox.rs       |   2 +
 rust/kernel/cpufreq.rs          |  11 +-
 rust/kernel/devres.rs           | 361 +++++++++++++++++++++++---------
 rust/kernel/drm/driver.rs       |  14 +-
 rust/kernel/pci.rs              |  20 +-
 rust/kernel/revocable.rs        |   6 +-
 rust/kernel/sync/arc.rs         |   1 +
 rust/kernel/types.rs            |   4 +
 samples/rust/rust_driver_pci.rs |  19 +-
 12 files changed, 323 insertions(+), 135 deletions(-)


base-commit: 7be11f5d927a548254be5c07291b32a6aa50917f
-- 
2.49.0

[PATCH v4 1/5] rust: revocable: support fallible PinInit types

[Danilo Krummrich]

Currently, Revocable::new() only supports infallible PinInit
implementations, i.e. impl PinInit<T, Infallible>.

This has been sufficient so far, since users such as Devres do not
support fallibility.

Since this is about to change, make Revocable::new() generic over the
error type E.

Reviewed-by: Benno Lossin <lossin@kernel.org>
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Acked-by: Miguel Ojeda <ojeda@kernel.org>
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 rust/kernel/devres.rs    | 2 +-
 rust/kernel/revocable.rs | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
index 8ede607414fd..fd8b75aa03bc 100644
--- a/rust/kernel/devres.rs
+++ b/rust/kernel/devres.rs
@@ -100,7 +100,7 @@ struct DevresInner<T: Send> {
 impl<T: Send> DevresInner<T> {
     fn new(dev: &Device<Bound>, data: T, flags: Flags) -> Result<Arc<DevresInner<T>>> {
         let inner = Arc::pin_init(
-            pin_init!( DevresInner {
+            try_pin_init!( DevresInner {
                 dev: dev.into(),
                 callback: Self::devres_callback,
                 data <- Revocable::new(data),
diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
index fa1fd70efa27..46768b374656 100644
--- a/rust/kernel/revocable.rs
+++ b/rust/kernel/revocable.rs
@@ -82,11 +82,11 @@ unsafe impl<T: Sync + Send> Sync for Revocable<T> {}
 
 impl<T> Revocable<T> {
     /// Creates a new revocable instance of the given data.
-    pub fn new(data: impl PinInit<T>) -> impl PinInit<Self> {
-        pin_init!(Self {
+    pub fn new<E>(data: impl PinInit<T, E>) -> impl PinInit<Self, E> {
+        try_pin_init!(Self {
             is_available: AtomicBool::new(true),
             data <- Opaque::pin_init(data),
-        })
+        }? E)
     }
 
     /// Tries to access the revocable wrapped object.
-- 
2.49.0

[PATCH v4 2/5] rust: devres: replace Devres::new_foreign_owned()

[Danilo Krummrich]

Replace Devres::new_foreign_owned() with devres::register().

The current implementation of Devres::new_foreign_owned() creates a full
Devres container instance, including the internal Revocable and
completion.

However, none of that is necessary for the intended use of giving full
ownership of an object to devres and getting it dropped once the given
device is unbound.

Hence, implement devres::register(), which is limited to consume the
given data, wrap it in a KBox and drop the KBox once the given device is
unbound, without any other synchronization.

Cc: Dave Airlie <airlied@redhat.com>
Cc: Simona Vetter <simona.vetter@ffwll.ch>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
Reviewed-by: Benno Lossin <lossin@kernel.org>
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 rust/helpers/device.c     |  7 ++++
 rust/kernel/cpufreq.rs    | 11 +++---
 rust/kernel/devres.rs     | 73 +++++++++++++++++++++++++++++++++------
 rust/kernel/drm/driver.rs | 14 ++++----
 4 files changed, 85 insertions(+), 20 deletions(-)

diff --git a/rust/helpers/device.c b/rust/helpers/device.c
index b2135c6686b0..502fef7e9ae8 100644
--- a/rust/helpers/device.c
+++ b/rust/helpers/device.c
@@ -8,3 +8,10 @@ int rust_helper_devm_add_action(struct device *dev,
 {
 	return devm_add_action(dev, action, data);
 }
+
+int rust_helper_devm_add_action_or_reset(struct device *dev,
+					 void (*action)(void *),
+					 void *data)
+{
+	return devm_add_action_or_reset(dev, action, data);
+}
diff --git a/rust/kernel/cpufreq.rs b/rust/kernel/cpufreq.rs
index 11b03e9d7e89..dd84e2b4d7ae 100644
--- a/rust/kernel/cpufreq.rs
+++ b/rust/kernel/cpufreq.rs
@@ -13,7 +13,7 @@
     cpu::CpuId,
     cpumask,
     device::{Bound, Device},
-    devres::Devres,
+    devres,
     error::{code::*, from_err_ptr, from_result, to_result, Result, VTABLE_DEFAULT_ERROR},
     ffi::{c_char, c_ulong},
     prelude::*,
@@ -1046,10 +1046,13 @@ pub fn new() -> Result<Self> {
 
     /// Same as [`Registration::new`], but does not return a [`Registration`] instance.
     ///
-    /// Instead the [`Registration`] is owned by [`Devres`] and will be revoked / dropped, once the
+    /// Instead the [`Registration`] is owned by [`devres::register`] and will be dropped, once the
     /// device is detached.
-    pub fn new_foreign_owned(dev: &Device<Bound>) -> Result {
-        Devres::new_foreign_owned(dev, Self::new()?, GFP_KERNEL)
+    pub fn new_foreign_owned(dev: &Device<Bound>) -> Result
+    where
+        T: 'static,
+    {
+        devres::register(dev, Self::new()?, GFP_KERNEL)
     }
 }
 
diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
index fd8b75aa03bc..64458ca3d69f 100644
--- a/rust/kernel/devres.rs
+++ b/rust/kernel/devres.rs
@@ -9,12 +9,12 @@
     alloc::Flags,
     bindings,
     device::{Bound, Device},
-    error::{Error, Result},
+    error::{to_result, Error, Result},
     ffi::c_void,
     prelude::*,
     revocable::{Revocable, RevocableGuard},
     sync::{rcu, Arc, Completion},
-    types::ARef,
+    types::{ARef, ForeignOwnable},
 };
 
 #[pin_data]
@@ -184,14 +184,6 @@ pub fn new(dev: &Device<Bound>, data: T, flags: Flags) -> Result<Self> {
         Ok(Devres(inner))
     }
 
-    /// Same as [`Devres::new`], but does not return a `Devres` instance. Instead the given `data`
-    /// is owned by devres and will be revoked / dropped, once the device is detached.
-    pub fn new_foreign_owned(dev: &Device<Bound>, data: T, flags: Flags) -> Result {
-        let _ = DevresInner::new(dev, data, flags)?;
-
-        Ok(())
-    }
-
     /// Obtain `&'a T`, bypassing the [`Revocable`].
     ///
     /// This method allows to directly obtain a `&'a T`, bypassing the [`Revocable`], by presenting
@@ -261,3 +253,64 @@ fn drop(&mut self) {
         }
     }
 }
+
+/// Consume `data` and [`Drop::drop`] `data` once `dev` is unbound.
+fn register_foreign<P>(dev: &Device<Bound>, data: P) -> Result
+where
+    P: ForeignOwnable + Send + 'static,
+{
+    let ptr = data.into_foreign();
+
+    #[allow(clippy::missing_safety_doc)]
+    unsafe extern "C" fn callback<P: ForeignOwnable>(ptr: *mut kernel::ffi::c_void) {
+        // SAFETY: `ptr` is the pointer to the `ForeignOwnable` leaked above and hence valid.
+        drop(unsafe { P::from_foreign(ptr.cast()) });
+    }
+
+    // SAFETY:
+    // - `dev.as_raw()` is a pointer to a valid and bound device.
+    // - `ptr` is a valid pointer the `ForeignOwnable` devres takes ownership of.
+    to_result(unsafe {
+        // `devm_add_action_or_reset()` also calls `callback` on failure, such that the
+        // `ForeignOwnable` is released eventually.
+        bindings::devm_add_action_or_reset(dev.as_raw(), Some(callback::<P>), ptr.cast())
+    })
+}
+
+/// Encapsulate `data` in a [`KBox`] and [`Drop::drop`] `data` once `dev` is unbound.
+///
+/// # Examples
+///
+/// ```no_run
+/// use kernel::{device::{Bound, Device}, devres};
+///
+/// /// Registration of e.g. a class device, IRQ, etc.
+/// struct Registration;
+///
+/// impl Registration {
+///     fn new() -> Self {
+///         // register
+///
+///         Self
+///     }
+/// }
+///
+/// impl Drop for Registration {
+///     fn drop(&mut self) {
+///        // unregister
+///     }
+/// }
+///
+/// fn from_bound_context(dev: &Device<Bound>) -> Result {
+///     devres::register(dev, Registration::new(), GFP_KERNEL)
+/// }
+/// ```
+pub fn register<T, E>(dev: &Device<Bound>, data: impl PinInit<T, E>, flags: Flags) -> Result
+where
+    T: Send + 'static,
+    Error: From<E>,
+{
+    let data = KBox::pin_init(data, flags)?;
+
+    register_foreign(dev, data)
+}
diff --git a/rust/kernel/drm/driver.rs b/rust/kernel/drm/driver.rs
index acb638086131..f63addaf7235 100644
--- a/rust/kernel/drm/driver.rs
+++ b/rust/kernel/drm/driver.rs
@@ -5,9 +5,7 @@
 //! C header: [`include/linux/drm/drm_drv.h`](srctree/include/linux/drm/drm_drv.h)
 
 use crate::{
-    bindings, device,
-    devres::Devres,
-    drm,
+    bindings, device, devres, drm,
     error::{to_result, Result},
     prelude::*,
     str::CStr,
@@ -130,18 +128,22 @@ fn new(drm: &drm::Device<T>, flags: usize) -> Result<Self> {
     }
 
     /// Same as [`Registration::new`}, but transfers ownership of the [`Registration`] to
-    /// [`Devres`].
+    /// [`devres::register`].
     pub fn new_foreign_owned(
         drm: &drm::Device<T>,
         dev: &device::Device<device::Bound>,
         flags: usize,
-    ) -> Result {
+    ) -> Result
+    where
+        T: 'static,
+    {
         if drm.as_ref().as_raw() != dev.as_raw() {
             return Err(EINVAL);
         }
 
         let reg = Registration::<T>::new(drm, flags)?;
-        Devres::new_foreign_owned(dev, reg, GFP_KERNEL)
+
+        devres::register(dev, reg, GFP_KERNEL)
     }
 
     /// Returns a reference to the `Device` instance for this registration.
-- 
2.49.0

[PATCH v4 3/5] rust: devres: get rid of Devres' inner Arc

[Danilo Krummrich]

So far Devres uses an inner memory allocation and reference count, i.e.
an inner Arc, in order to ensure that the devres callback can't run into
a use-after-free in case where the Devres object is dropped while the
devres callback runs concurrently.

Instead, use a completion in order to avoid a potential UAF: In
Devres::drop(), if we detect that we can't remove the devres action
anymore, we wait for the completion that is completed from the devres
callback. If, in turn, we were able to successfully remove the devres
action, we can just go ahead.

This, again, allows us to get rid of the internal Arc, and instead let
Devres consume an `impl PinInit<T, E>` in order to return an
`impl PinInit<Devres<T>, E>`, which enables us to get away with less
memory allocations.

Additionally, having the resulting explicit synchronization in
Devres::drop() prevents potential subtle undesired side effects of the
devres callback dropping the final Arc reference asynchronously within
the devres callback.

Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 drivers/gpu/nova-core/driver.rs |   7 +-
 drivers/gpu/nova-core/gpu.rs    |   6 +-
 rust/kernel/devres.rs           | 213 +++++++++++++++++++-------------
 rust/kernel/pci.rs              |  20 +--
 samples/rust/rust_driver_pci.rs |  19 +--
 5 files changed, 154 insertions(+), 111 deletions(-)

diff --git a/drivers/gpu/nova-core/driver.rs b/drivers/gpu/nova-core/driver.rs
index 8c86101c26cb..110f2b355db4 100644
--- a/drivers/gpu/nova-core/driver.rs
+++ b/drivers/gpu/nova-core/driver.rs
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 
-use kernel::{auxiliary, bindings, c_str, device::Core, pci, prelude::*};
+use kernel::{auxiliary, bindings, c_str, device::Core, pci, prelude::*, sync::Arc};
 
 use crate::gpu::Gpu;
 
@@ -34,7 +34,10 @@ fn probe(pdev: &pci::Device<Core>, _info: &Self::IdInfo) -> Result<Pin<KBox<Self
         pdev.enable_device_mem()?;
         pdev.set_master();
 
-        let bar = pdev.iomap_region_sized::<BAR0_SIZE>(0, c_str!("nova-core/bar0"))?;
+        let bar = Arc::pin_init(
+            pdev.iomap_region_sized::<BAR0_SIZE>(0, c_str!("nova-core/bar0")),
+            GFP_KERNEL,
+        )?;
 
         let this = KBox::pin_init(
             try_pin_init!(Self {
diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
index 60b86f370284..47653c14838b 100644
--- a/drivers/gpu/nova-core/gpu.rs
+++ b/drivers/gpu/nova-core/gpu.rs
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 
-use kernel::{device, devres::Devres, error::code::*, pci, prelude::*};
+use kernel::{device, devres::Devres, error::code::*, pci, prelude::*, sync::Arc};
 
 use crate::driver::Bar0;
 use crate::firmware::{Firmware, FIRMWARE_VERSION};
@@ -161,14 +161,14 @@ fn new(bar: &Bar0) -> Result<Spec> {
 pub(crate) struct Gpu {
     spec: Spec,
     /// MMIO mapping of PCI BAR 0
-    bar: Devres<Bar0>,
+    bar: Arc<Devres<Bar0>>,
     fw: Firmware,
 }
 
 impl Gpu {
     pub(crate) fn new(
         pdev: &pci::Device<device::Bound>,
-        devres_bar: Devres<Bar0>,
+        devres_bar: Arc<Devres<Bar0>>,
     ) -> Result<impl PinInit<Self>> {
         let bar = devres_bar.access(pdev.as_ref())?;
         let spec = Spec::new(bar)?;
diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
index 64458ca3d69f..3ce8d6161778 100644
--- a/rust/kernel/devres.rs
+++ b/rust/kernel/devres.rs
@@ -13,16 +13,21 @@
     ffi::c_void,
     prelude::*,
     revocable::{Revocable, RevocableGuard},
-    sync::{rcu, Arc, Completion},
-    types::{ARef, ForeignOwnable},
+    sync::{rcu, Completion},
+    types::{ARef, ForeignOwnable, Opaque, ScopeGuard},
 };
 
+use pin_init::Wrapper;
+
+/// [`Devres`] inner data accessed from [`Devres::callback`].
 #[pin_data]
-struct DevresInner<T: Send> {
-    dev: ARef<Device>,
-    callback: unsafe extern "C" fn(*mut c_void),
+struct Inner<T: Send> {
     #[pin]
     data: Revocable<T>,
+    /// Tracks whether [`Devres::callback`] has been completed.
+    #[pin]
+    devm: Completion,
+    /// Tracks whether revoking [`Self::data`] has been completed.
     #[pin]
     revoke: Completion,
 }
@@ -44,6 +49,10 @@ struct DevresInner<T: Send> {
 /// [`Devres`] users should make sure to simply free the corresponding backing resource in `T`'s
 /// [`Drop`] implementation.
 ///
+/// # Invariants
+///
+/// [`Self::inner`] is guaranteed to be initialized and is always accessed read-only.
+///
 /// # Example
 ///
 /// ```no_run
@@ -88,100 +97,111 @@ struct DevresInner<T: Send> {
 /// # fn no_run(dev: &Device<Bound>) -> Result<(), Error> {
 /// // SAFETY: Invalid usage for example purposes.
 /// let iomem = unsafe { IoMem::<{ core::mem::size_of::<u32>() }>::new(0xBAAAAAAD)? };
-/// let devres = Devres::new(dev, iomem, GFP_KERNEL)?;
+/// let devres = KBox::pin_init(Devres::new(dev, iomem), GFP_KERNEL)?;
 ///
 /// let res = devres.try_access().ok_or(ENXIO)?;
 /// res.write8(0x42, 0x0);
 /// # Ok(())
 /// # }
 /// ```
-pub struct Devres<T: Send>(Arc<DevresInner<T>>);
+#[pin_data(PinnedDrop)]
+pub struct Devres<T: Send> {
+    dev: ARef<Device>,
+    /// Pointer to [`Self::devres_callback`].
+    ///
+    /// Has to be stored, since Rust does not guarantee to always return the same address for a
+    /// function. However, the C API uses the address as a key.
+    callback: unsafe extern "C" fn(*mut c_void),
+    /// Contains all the fields shared with [`Self::callback`].
+    // TODO: Replace with `UnsafePinned`, once available.
+    //
+    // Subsequently, the `drop_in_place()` in `Devres::drop` and the explicit `Send` and `Sync'
+    // impls can be removed.
+    #[pin]
+    inner: Opaque<Inner<T>>,
+}
+
+impl<T: Send> Devres<T> {
+    /// Creates a new [`Devres`] instance of the given `data`.
+    ///
+    /// The `data` encapsulated within the returned `Devres` instance' `data` will be
+    /// (revoked)[`Revocable`] once the device is detached.
+    pub fn new<'a, E>(
+        dev: &'a Device<Bound>,
+        data: impl PinInit<T, E> + 'a,
+    ) -> impl PinInit<Self, Error> + 'a
+    where
+        T: 'a,
+        Error: From<E>,
+    {
+        let callback = Self::devres_callback;
 
-impl<T: Send> DevresInner<T> {
-    fn new(dev: &Device<Bound>, data: T, flags: Flags) -> Result<Arc<DevresInner<T>>> {
-        let inner = Arc::pin_init(
-            try_pin_init!( DevresInner {
-                dev: dev.into(),
-                callback: Self::devres_callback,
+        try_pin_init!(&this in Self {
+            // INVARIANT: `inner` is properly initialized.
+            inner <- Opaque::pin_init(try_pin_init!(Inner {
                 data <- Revocable::new(data),
+                devm <- Completion::new(),
                 revoke <- Completion::new(),
-            }),
-            flags,
-        )?;
-
-        // Convert `Arc<DevresInner>` into a raw pointer and make devres own this reference until
-        // `Self::devres_callback` is called.
-        let data = inner.clone().into_raw();
+            })),
+            callback,
+            dev: {
+                // SAFETY: `this` is a valid pointer to uninitialized memory.
+                let inner = unsafe { &raw mut (*this.as_ptr()).inner };
 
-        // SAFETY: `devm_add_action` guarantees to call `Self::devres_callback` once `dev` is
-        // detached.
-        let ret =
-            unsafe { bindings::devm_add_action(dev.as_raw(), Some(inner.callback), data as _) };
-
-        if ret != 0 {
-            // SAFETY: We just created another reference to `inner` in order to pass it to
-            // `bindings::devm_add_action`. If `bindings::devm_add_action` fails, we have to drop
-            // this reference accordingly.
-            let _ = unsafe { Arc::from_raw(data) };
-            return Err(Error::from_errno(ret));
-        }
+                // SAFETY:
+                // - `dev.as_raw()` is a pointer to a valid bound device.
+                // - `inner` is guaranteed to be a valid for the duration of the lifetime of `Self`.
+                // - `devm_add_action()` is guaranteed not to call `callback` until `this` has been
+                //    properly initialized, because we require `dev` (i.e. the *bound* device) to
+                //    live at least as long as the returned `impl PinInit<Self, Error>`.
+                to_result(unsafe {
+                    bindings::devm_add_action(dev.as_raw(), Some(callback), inner.cast())
+                })?;
 
-        Ok(inner)
+                dev.into()
+            },
+        })
     }
 
-    fn as_ptr(&self) -> *const Self {
-        self as _
+    fn inner(&self) -> &Inner<T> {
+        // SAFETY: By the type invairants of `Self`, `inner` is properly initialized and always
+        // accessed read-only.
+        unsafe { &*self.inner.get() }
     }
 
-    fn remove_action(this: &Arc<Self>) -> bool {
-        // SAFETY:
-        // - `self.inner.dev` is a valid `Device`,
-        // - the `action` and `data` pointers are the exact same ones as given to devm_add_action()
-        //   previously,
-        // - `self` is always valid, even if the action has been released already.
-        let success = unsafe {
-            bindings::devm_remove_action_nowarn(
-                this.dev.as_raw(),
-                Some(this.callback),
-                this.as_ptr() as _,
-            )
-        } == 0;
-
-        if success {
-            // SAFETY: We leaked an `Arc` reference to devm_add_action() in `DevresInner::new`; if
-            // devm_remove_action_nowarn() was successful we can (and have to) claim back ownership
-            // of this reference.
-            let _ = unsafe { Arc::from_raw(this.as_ptr()) };
-        }
-
-        success
+    fn data(&self) -> &Revocable<T> {
+        &self.inner().data
     }
 
     #[allow(clippy::missing_safety_doc)]
     unsafe extern "C" fn devres_callback(ptr: *mut kernel::ffi::c_void) {
-        let ptr = ptr as *mut DevresInner<T>;
-        // Devres owned this memory; now that we received the callback, drop the `Arc` and hence the
-        // reference.
-        // SAFETY: Safe, since we leaked an `Arc` reference to devm_add_action() in
-        //         `DevresInner::new`.
-        let inner = unsafe { Arc::from_raw(ptr) };
+        // SAFETY: In `Self::new` we've passed a valid pointer to `Inner` to `devm_add_action()`,
+        // hence `ptr` must be a valid pointer to `Inner`.
+        let inner = unsafe { &*ptr.cast::<Inner<T>>() };
+
+        // Ensure that `inner` can't be used anymore after we signal completion of this callback.
+        let inner = ScopeGuard::new_with_data(inner, |inner| inner.devm.complete_all());
 
         if !inner.data.revoke() {
             // If `revoke()` returns false, it means that `Devres::drop` already started revoking
-            // `inner.data` for us. Hence we have to wait until `Devres::drop()` signals that it
-            // completed revoking `inner.data`.
+            // `data` for us. Hence we have to wait until `Devres::drop` signals that it
+            // completed revoking `data`.
             inner.revoke.wait_for_completion();
         }
     }
-}
 
-impl<T: Send> Devres<T> {
-    /// Creates a new [`Devres`] instance of the given `data`. The `data` encapsulated within the
-    /// returned `Devres` instance' `data` will be revoked once the device is detached.
-    pub fn new(dev: &Device<Bound>, data: T, flags: Flags) -> Result<Self> {
-        let inner = DevresInner::new(dev, data, flags)?;
-
-        Ok(Devres(inner))
+    fn remove_action(&self) -> bool {
+        // SAFETY:
+        // - `self.dev` is a valid `Device`,
+        // - the `action` and `data` pointers are the exact same ones as given to
+        //   `devm_add_action()` previously,
+        (unsafe {
+            bindings::devm_remove_action_nowarn(
+                self.dev.as_raw(),
+                Some(self.callback),
+                core::ptr::from_ref(self.inner()).cast_mut().cast(),
+            )
+        } == 0)
     }
 
     /// Obtain `&'a T`, bypassing the [`Revocable`].
@@ -213,44 +233,63 @@ pub fn new(dev: &Device<Bound>, data: T, flags: Flags) -> Result<Self> {
     /// }
     /// ```
     pub fn access<'a>(&'a self, dev: &'a Device<Bound>) -> Result<&'a T> {
-        if self.0.dev.as_raw() != dev.as_raw() {
+        if self.dev.as_raw() != dev.as_raw() {
             return Err(EINVAL);
         }
 
         // SAFETY: `dev` being the same device as the device this `Devres` has been created for
-        // proves that `self.0.data` hasn't been revoked and is guaranteed to not be revoked as
-        // long as `dev` lives; `dev` lives at least as long as `self`.
-        Ok(unsafe { self.0.data.access() })
+        // proves that `self.data` hasn't been revoked and is guaranteed to not be revoked as long
+        // as `dev` lives; `dev` lives at least as long as `self`.
+        Ok(unsafe { self.data().access() })
     }
 
     /// [`Devres`] accessor for [`Revocable::try_access`].
     pub fn try_access(&self) -> Option<RevocableGuard<'_, T>> {
-        self.0.data.try_access()
+        self.data().try_access()
     }
 
     /// [`Devres`] accessor for [`Revocable::try_access_with`].
     pub fn try_access_with<R, F: FnOnce(&T) -> R>(&self, f: F) -> Option<R> {
-        self.0.data.try_access_with(f)
+        self.data().try_access_with(f)
     }
 
     /// [`Devres`] accessor for [`Revocable::try_access_with_guard`].
     pub fn try_access_with_guard<'a>(&'a self, guard: &'a rcu::Guard) -> Option<&'a T> {
-        self.0.data.try_access_with_guard(guard)
+        self.data().try_access_with_guard(guard)
     }
 }
 
-impl<T: Send> Drop for Devres<T> {
-    fn drop(&mut self) {
+// SAFETY: `Devres` can be send to any task, if `T: Send`.
+unsafe impl<T: Send> Send for Devres<T> {}
+
+// SAFETY: `Devres` can be shared with any task, if `T: Sync`.
+unsafe impl<T: Send + Sync> Sync for Devres<T> {}
+
+#[pinned_drop]
+impl<T: Send> PinnedDrop for Devres<T> {
+    fn drop(self: Pin<&mut Self>) {
         // SAFETY: When `drop` runs, it is guaranteed that nobody is accessing the revocable data
         // anymore, hence it is safe not to wait for the grace period to finish.
-        if unsafe { self.0.data.revoke_nosync() } {
-            // We revoked `self.0.data` before the devres action did, hence try to remove it.
-            if !DevresInner::remove_action(&self.0) {
+        if unsafe { self.data().revoke_nosync() } {
+            // We revoked `self.data` before the devres action did, hence try to remove it.
+            if !self.remove_action() {
                 // We could not remove the devres action, which means that it now runs concurrently,
-                // hence signal that `self.0.data` has been revoked successfully.
-                self.0.revoke.complete_all();
+                // hence signal that `self.data` has been revoked by us successfully.
+                self.inner().revoke.complete_all();
+
+                // Wait for `Self::devres_callback` to be done using this object.
+                self.inner().devm.wait_for_completion();
             }
+        } else {
+            // `Self::devres_callback` revokes `self.data` for us, hence wait for it to be done
+            // using this object.
+            self.inner().devm.wait_for_completion();
         }
+
+        // INVARIANT: At this point it is guaranteed that `inner` can't be accessed any more.
+        //
+        // SAFETY: `inner` is valid for dropping.
+        unsafe { core::ptr::drop_in_place(self.inner.get()) };
     }
 }
 
diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
index 8435f8132e38..db0eb7eaf9b1 100644
--- a/rust/kernel/pci.rs
+++ b/rust/kernel/pci.rs
@@ -5,7 +5,6 @@
 //! C header: [`include/linux/pci.h`](srctree/include/linux/pci.h)
 
 use crate::{
-    alloc::flags::*,
     bindings, container_of, device,
     device_id::RawDeviceId,
     devres::Devres,
@@ -398,19 +397,20 @@ pub fn resource_len(&self, bar: u32) -> Result<bindings::resource_size_t> {
 impl Device<device::Bound> {
     /// Mapps an entire PCI-BAR after performing a region-request on it. I/O operation bound checks
     /// can be performed on compile time for offsets (plus the requested type size) < SIZE.
-    pub fn iomap_region_sized<const SIZE: usize>(
-        &self,
+    pub fn iomap_region_sized<'a, const SIZE: usize>(
+        &'a self,
         bar: u32,
-        name: &CStr,
-    ) -> Result<Devres<Bar<SIZE>>> {
-        let bar = Bar::<SIZE>::new(self, bar, name)?;
-        let devres = Devres::new(self.as_ref(), bar, GFP_KERNEL)?;
-
-        Ok(devres)
+        name: &'a CStr,
+    ) -> impl PinInit<Devres<Bar<SIZE>>, Error> + 'a {
+        Devres::new(self.as_ref(), Bar::<SIZE>::new(self, bar, name))
     }
 
     /// Mapps an entire PCI-BAR after performing a region-request on it.
-    pub fn iomap_region(&self, bar: u32, name: &CStr) -> Result<Devres<Bar>> {
+    pub fn iomap_region<'a>(
+        &'a self,
+        bar: u32,
+        name: &'a CStr,
+    ) -> impl PinInit<Devres<Bar>, Error> + 'a {
         self.iomap_region_sized::<0>(bar, name)
     }
 }
diff --git a/samples/rust/rust_driver_pci.rs b/samples/rust/rust_driver_pci.rs
index 15147e4401b2..5c35f1414172 100644
--- a/samples/rust/rust_driver_pci.rs
+++ b/samples/rust/rust_driver_pci.rs
@@ -25,8 +25,10 @@ impl TestIndex {
     const NO_EVENTFD: Self = Self(0);
 }
 
+#[pin_data(PinnedDrop)]
 struct SampleDriver {
     pdev: ARef<pci::Device>,
+    #[pin]
     bar: Devres<Bar0>,
 }
 
@@ -73,13 +75,11 @@ fn probe(pdev: &pci::Device<Core>, info: &Self::IdInfo) -> Result<Pin<KBox<Self>
         pdev.enable_device_mem()?;
         pdev.set_master();
 
-        let bar = pdev.iomap_region_sized::<{ Regs::END }>(0, c_str!("rust_driver_pci"))?;
-
-        let drvdata = KBox::new(
-            Self {
+        let drvdata = KBox::pin_init(
+            try_pin_init!(Self {
                 pdev: pdev.into(),
-                bar,
-            },
+                bar <- pdev.iomap_region_sized::<{ Regs::END }>(0, c_str!("rust_driver_pci")),
+            }),
             GFP_KERNEL,
         )?;
 
@@ -90,12 +90,13 @@ fn probe(pdev: &pci::Device<Core>, info: &Self::IdInfo) -> Result<Pin<KBox<Self>
             Self::testdev(info, bar)?
         );
 
-        Ok(drvdata.into())
+        Ok(drvdata)
     }
 }
 
-impl Drop for SampleDriver {
-    fn drop(&mut self) {
+#[pinned_drop]
+impl PinnedDrop for SampleDriver {
+    fn drop(self: Pin<&mut Self>) {
         dev_dbg!(self.pdev.as_ref(), "Remove Rust PCI driver sample.\n");
     }
 }
-- 
2.49.0

[PATCH v4 4/5] rust: types: ForeignOwnable: Add type Target

[Danilo Krummrich]

ForeignOwnable::Target defines the payload data of a ForeignOwnable. For
Arc<T> for instance, ForeignOwnable::Target would just be T.

This is useful for cases where a trait bound is required on the target
type of the ForeignOwnable. For instance:

	fn example<P>(data: P)
	   where
	      P: ForeignOwnable,
	      P::Target: MyTrait,
	{}

Suggested-by: Benno Lossin <lossin@kernel.org>
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 rust/kernel/alloc/kbox.rs | 2 ++
 rust/kernel/sync/arc.rs   | 1 +
 rust/kernel/types.rs      | 4 ++++
 3 files changed, 7 insertions(+)

diff --git a/rust/kernel/alloc/kbox.rs b/rust/kernel/alloc/kbox.rs
index c386ff771d50..66fad9777567 100644
--- a/rust/kernel/alloc/kbox.rs
+++ b/rust/kernel/alloc/kbox.rs
@@ -403,6 +403,7 @@ unsafe impl<T: 'static, A> ForeignOwnable for Box<T, A>
 where
     A: Allocator,
 {
+    type Target = T;
     type PointedTo = T;
     type Borrowed<'a> = &'a T;
     type BorrowedMut<'a> = &'a mut T;
@@ -435,6 +436,7 @@ unsafe impl<T: 'static, A> ForeignOwnable for Pin<Box<T, A>>
 where
     A: Allocator,
 {
+    type Target = T;
     type PointedTo = T;
     type Borrowed<'a> = Pin<&'a T>;
     type BorrowedMut<'a> = Pin<&'a mut T>;
diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
index c7af0aa48a0a..24fb63597d35 100644
--- a/rust/kernel/sync/arc.rs
+++ b/rust/kernel/sync/arc.rs
@@ -374,6 +374,7 @@ pub fn into_unique_or_drop(self) -> Option<Pin<UniqueArc<T>>> {
 
 // SAFETY: The `into_foreign` function returns a pointer that is well-aligned.
 unsafe impl<T: 'static> ForeignOwnable for Arc<T> {
+    type Target = T;
     type PointedTo = ArcInner<T>;
     type Borrowed<'a> = ArcBorrow<'a, T>;
     type BorrowedMut<'a> = Self::Borrowed<'a>;
diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
index 3958a5f44d56..74c787b352a9 100644
--- a/rust/kernel/types.rs
+++ b/rust/kernel/types.rs
@@ -27,6 +27,9 @@
 /// [`into_foreign`]: Self::into_foreign
 /// [`PointedTo`]: Self::PointedTo
 pub unsafe trait ForeignOwnable: Sized {
+    /// The payload type of the foreign-owned value.
+    type Target;
+
     /// Type used when the value is foreign-owned. In practical terms only defines the alignment of
     /// the pointer.
     type PointedTo;
@@ -128,6 +131,7 @@ unsafe fn try_from_foreign(ptr: *mut Self::PointedTo) -> Option<Self> {
 
 // SAFETY: The `into_foreign` function returns a pointer that is dangling, but well-aligned.
 unsafe impl ForeignOwnable for () {
+    type Target = ();
     type PointedTo = ();
     type Borrowed<'a> = ();
     type BorrowedMut<'a> = ();
-- 
2.49.0

[PATCH v4 5/5] rust: devres: implement register_release()

[Danilo Krummrich]

register_release() is useful when a device resource has associated data,
but does not require the capability of accessing it or manually releasing
it.

If we would want to be able to access the device resource and release the
device resource manually before the device is unbound, but still keep
access to the associated data, we could implement it as follows.

	struct Registration<T> {
	   inner: Devres<RegistrationInner>,
	   data: T,
	}

However, if we never need to access the resource or release it manually,
register_release() is great optimization for the above, since it does not
require the synchronization of the Devres type.

Suggested-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 rust/kernel/devres.rs | 73 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 73 insertions(+)

diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
index 3ce8d6161778..92aca78874ff 100644
--- a/rust/kernel/devres.rs
+++ b/rust/kernel/devres.rs
@@ -353,3 +353,76 @@ pub fn register<T, E>(dev: &Device<Bound>, data: impl PinInit<T, E>, flags: Flag
 
     register_foreign(dev, data)
 }
+
+/// [`Devres`]-releaseable resource.
+///
+/// Register an object implementing this trait with [`register_release`]. Its `release`
+/// function will be called once the device is being unbound.
+pub trait Release {
+    /// The [`ForeignOwnable`] pointer type consumed by [`register_release`].
+    type Ptr: ForeignOwnable;
+
+    /// Called once the [`Device`] given to [`register_release`] is unbound.
+    fn release(this: Self::Ptr);
+}
+
+/// Consume the `data`, [`Release::release`] and [`Drop::drop`] `data` once `dev` is unbound.
+///
+/// # Examples
+///
+/// ```no_run
+/// use kernel::{device::{Bound, Device}, devres, devres::Release, sync::Arc};
+///
+/// /// Registration of e.g. a class device, IRQ, etc.
+/// struct Registration;
+///
+/// impl Registration {
+///     fn new() -> Result<Arc<Self>> {
+///         // register
+///
+///         Ok(Arc::new(Self, GFP_KERNEL)?)
+///     }
+/// }
+///
+/// impl Release for Registration {
+///     type Ptr = Arc<Self>;
+///
+///     fn release(this: Arc<Self>) {
+///        // unregister
+///     }
+/// }
+///
+/// fn from_bound_context(dev: &Device<Bound>) -> Result {
+///     let reg = Registration::new()?;
+///
+///     devres::register_release(dev, reg.clone())
+/// }
+/// ```
+pub fn register_release<P>(dev: &Device<Bound>, data: P) -> Result
+where
+    P: ForeignOwnable,
+    P::Target: Release<Ptr = P> + Send,
+{
+    let ptr = data.into_foreign();
+
+    #[allow(clippy::missing_safety_doc)]
+    unsafe extern "C" fn callback<P>(ptr: *mut kernel::ffi::c_void)
+    where
+        P: ForeignOwnable,
+        P::Target: Release<Ptr = P>,
+    {
+        // SAFETY: `ptr` is the pointer to the `ForeignOwnable` leaked above and hence valid.
+        let data = unsafe { P::from_foreign(ptr.cast()) };
+
+        P::Target::release(data);
+    }
+
+    // SAFETY:
+    // - `dev.as_raw()` is a pointer to a valid and bound device.
+    // - `ptr` is a valid pointer the `ForeignOwnable` devres takes ownership of.
+    to_result(unsafe {
+        // `devm_add_action_or_reset()` also calls `callback` on failure, such that the
+        // `ForeignOwnable` is released eventually.
+        bindings::devm_add_action_or_reset(dev.as_raw(), Some(callback::<P>), ptr.cast())
+    })
+}
-- 
2.49.0

Re: [PATCH v4 3/5] rust: devres: get rid of Devres' inner Arc

[Boqun Feng]

On Thu, Jun 26, 2025 at 10:00:41PM +0200, Danilo Krummrich wrote:
> So far Devres uses an inner memory allocation and reference count, i.e.
> an inner Arc, in order to ensure that the devres callback can't run into
> a use-after-free in case where the Devres object is dropped while the
> devres callback runs concurrently.
> 
> Instead, use a completion in order to avoid a potential UAF: In
> Devres::drop(), if we detect that we can't remove the devres action
> anymore, we wait for the completion that is completed from the devres
> callback. If, in turn, we were able to successfully remove the devres
> action, we can just go ahead.
> 
> This, again, allows us to get rid of the internal Arc, and instead let
> Devres consume an `impl PinInit<T, E>` in order to return an
> `impl PinInit<Devres<T>, E>`, which enables us to get away with less
> memory allocations.
> 
> Additionally, having the resulting explicit synchronization in
> Devres::drop() prevents potential subtle undesired side effects of the
> devres callback dropping the final Arc reference asynchronously within
> the devres callback.
> 
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>

Reviewed-by: Boqun Feng <boqun.feng@gmail.com>

Regards,
Boqun

Re: [PATCH v4 4/5] rust: types: ForeignOwnable: Add type Target

[Boqun Feng]

On Thu, Jun 26, 2025 at 10:00:42PM +0200, Danilo Krummrich wrote:
> ForeignOwnable::Target defines the payload data of a ForeignOwnable. For
> Arc<T> for instance, ForeignOwnable::Target would just be T.
> 
> This is useful for cases where a trait bound is required on the target
> type of the ForeignOwnable. For instance:
> 
> 	fn example<P>(data: P)
> 	   where
> 	      P: ForeignOwnable,
> 	      P::Target: MyTrait,
> 	{}
> 
> Suggested-by: Benno Lossin <lossin@kernel.org>
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>

Reviewed-by: Boqun Feng <boqun.feng@gmail.com>

One nit below:

> ---
>  rust/kernel/alloc/kbox.rs | 2 ++
>  rust/kernel/sync/arc.rs   | 1 +
>  rust/kernel/types.rs      | 4 ++++
>  3 files changed, 7 insertions(+)
> 
> diff --git a/rust/kernel/alloc/kbox.rs b/rust/kernel/alloc/kbox.rs
> index c386ff771d50..66fad9777567 100644
> --- a/rust/kernel/alloc/kbox.rs
> +++ b/rust/kernel/alloc/kbox.rs
> @@ -403,6 +403,7 @@ unsafe impl<T: 'static, A> ForeignOwnable for Box<T, A>
>  where
>      A: Allocator,
>  {
> +    type Target = T;
>      type PointedTo = T;
>      type Borrowed<'a> = &'a T;
>      type BorrowedMut<'a> = &'a mut T;
> @@ -435,6 +436,7 @@ unsafe impl<T: 'static, A> ForeignOwnable for Pin<Box<T, A>>
>  where
>      A: Allocator,
>  {
> +    type Target = T;
>      type PointedTo = T;
>      type Borrowed<'a> = Pin<&'a T>;
>      type BorrowedMut<'a> = Pin<&'a mut T>;
> diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
> index c7af0aa48a0a..24fb63597d35 100644
> --- a/rust/kernel/sync/arc.rs
> +++ b/rust/kernel/sync/arc.rs
> @@ -374,6 +374,7 @@ pub fn into_unique_or_drop(self) -> Option<Pin<UniqueArc<T>>> {
>  
>  // SAFETY: The `into_foreign` function returns a pointer that is well-aligned.
>  unsafe impl<T: 'static> ForeignOwnable for Arc<T> {
> +    type Target = T;
>      type PointedTo = ArcInner<T>;
>      type Borrowed<'a> = ArcBorrow<'a, T>;
>      type BorrowedMut<'a> = Self::Borrowed<'a>;
> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> index 3958a5f44d56..74c787b352a9 100644
> --- a/rust/kernel/types.rs
> +++ b/rust/kernel/types.rs
> @@ -27,6 +27,9 @@
>  /// [`into_foreign`]: Self::into_foreign
>  /// [`PointedTo`]: Self::PointedTo
>  pub unsafe trait ForeignOwnable: Sized {
> +    /// The payload type of the foreign-owned value.
> +    type Target;

I think `ForeignOwnable` also implies a `T` maybe get dropped via a
pointer from `into_foreign()`. Not sure it's worth mentioning though.

Regards,
Boqun

> +
>      /// Type used when the value is foreign-owned. In practical terms only defines the alignment of
>      /// the pointer.
>      type PointedTo;
> @@ -128,6 +131,7 @@ unsafe fn try_from_foreign(ptr: *mut Self::PointedTo) -> Option<Self> {
>  
>  // SAFETY: The `into_foreign` function returns a pointer that is dangling, but well-aligned.
>  unsafe impl ForeignOwnable for () {
> +    type Target = ();
>      type PointedTo = ();
>      type Borrowed<'a> = ();
>      type BorrowedMut<'a> = ();
> -- 
> 2.49.0
> 

Re: [PATCH v4 5/5] rust: devres: implement register_release()

[Boqun Feng]

On Thu, Jun 26, 2025 at 10:00:43PM +0200, Danilo Krummrich wrote:
> register_release() is useful when a device resource has associated data,
> but does not require the capability of accessing it or manually releasing
> it.
> 
> If we would want to be able to access the device resource and release the
> device resource manually before the device is unbound, but still keep
> access to the associated data, we could implement it as follows.
> 
> 	struct Registration<T> {
> 	   inner: Devres<RegistrationInner>,
> 	   data: T,
> 	}
> 
> However, if we never need to access the resource or release it manually,
> register_release() is great optimization for the above, since it does not
> require the synchronization of the Devres type.
> 
> Suggested-by: Alice Ryhl <aliceryhl@google.com>
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> ---
>  rust/kernel/devres.rs | 73 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 73 insertions(+)
> 
> diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
> index 3ce8d6161778..92aca78874ff 100644
> --- a/rust/kernel/devres.rs
> +++ b/rust/kernel/devres.rs
> @@ -353,3 +353,76 @@ pub fn register<T, E>(dev: &Device<Bound>, data: impl PinInit<T, E>, flags: Flag
>  
>      register_foreign(dev, data)
>  }
> +
> +/// [`Devres`]-releaseable resource.
> +///
> +/// Register an object implementing this trait with [`register_release`]. Its `release`
> +/// function will be called once the device is being unbound.
> +pub trait Release {
> +    /// The [`ForeignOwnable`] pointer type consumed by [`register_release`].
> +    type Ptr: ForeignOwnable;
> +
> +    /// Called once the [`Device`] given to [`register_release`] is unbound.
> +    fn release(this: Self::Ptr);
> +}
> +

I would like to point out the limitation of this design, say you have a
`Foo` that can ipml `Release`, with this, I think you could only support
either `Arc<Foo>` or `KBox<Foo>`. You cannot support both as the input
for `register_release()`. Maybe we want:

    pub trait Release<Ptr: ForeignOwnable> {
        fn release(this: Ptr);
    }

?

Regards,
Boqun

> +/// Consume the `data`, [`Release::release`] and [`Drop::drop`] `data` once `dev` is unbound.
> +///
> +/// # Examples
> +///
> +/// ```no_run
> +/// use kernel::{device::{Bound, Device}, devres, devres::Release, sync::Arc};
> +///
> +/// /// Registration of e.g. a class device, IRQ, etc.
> +/// struct Registration;
> +///
> +/// impl Registration {
> +///     fn new() -> Result<Arc<Self>> {
> +///         // register
> +///
> +///         Ok(Arc::new(Self, GFP_KERNEL)?)
> +///     }
> +/// }
> +///
> +/// impl Release for Registration {
> +///     type Ptr = Arc<Self>;
> +///
> +///     fn release(this: Arc<Self>) {
> +///        // unregister
> +///     }
> +/// }
> +///
> +/// fn from_bound_context(dev: &Device<Bound>) -> Result {
> +///     let reg = Registration::new()?;
> +///
> +///     devres::register_release(dev, reg.clone())
> +/// }
> +/// ```
> +pub fn register_release<P>(dev: &Device<Bound>, data: P) -> Result
> +where
> +    P: ForeignOwnable,
> +    P::Target: Release<Ptr = P> + Send,
> +{
> +    let ptr = data.into_foreign();
> +
> +    #[allow(clippy::missing_safety_doc)]
> +    unsafe extern "C" fn callback<P>(ptr: *mut kernel::ffi::c_void)
> +    where
> +        P: ForeignOwnable,
> +        P::Target: Release<Ptr = P>,
> +    {
> +        // SAFETY: `ptr` is the pointer to the `ForeignOwnable` leaked above and hence valid.
> +        let data = unsafe { P::from_foreign(ptr.cast()) };
> +
> +        P::Target::release(data);
> +    }
> +
> +    // SAFETY:
> +    // - `dev.as_raw()` is a pointer to a valid and bound device.
> +    // - `ptr` is a valid pointer the `ForeignOwnable` devres takes ownership of.
> +    to_result(unsafe {
> +        // `devm_add_action_or_reset()` also calls `callback` on failure, such that the
> +        // `ForeignOwnable` is released eventually.
> +        bindings::devm_add_action_or_reset(dev.as_raw(), Some(callback::<P>), ptr.cast())
> +    })
> +}
> -- 
> 2.49.0
> 

Re: [PATCH v4 5/5] rust: devres: implement register_release()

[Danilo Krummrich]

On Thu, Jun 26, 2025 at 01:37:22PM -0700, Boqun Feng wrote:
> On Thu, Jun 26, 2025 at 10:00:43PM +0200, Danilo Krummrich wrote:
> > register_release() is useful when a device resource has associated data,
> > but does not require the capability of accessing it or manually releasing
> > it.
> > 
> > If we would want to be able to access the device resource and release the
> > device resource manually before the device is unbound, but still keep
> > access to the associated data, we could implement it as follows.
> > 
> > 	struct Registration<T> {
> > 	   inner: Devres<RegistrationInner>,
> > 	   data: T,
> > 	}
> > 
> > However, if we never need to access the resource or release it manually,
> > register_release() is great optimization for the above, since it does not
> > require the synchronization of the Devres type.
> > 
> > Suggested-by: Alice Ryhl <aliceryhl@google.com>
> > Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> > ---
> >  rust/kernel/devres.rs | 73 +++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 73 insertions(+)
> > 
> > diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
> > index 3ce8d6161778..92aca78874ff 100644
> > --- a/rust/kernel/devres.rs
> > +++ b/rust/kernel/devres.rs
> > @@ -353,3 +353,76 @@ pub fn register<T, E>(dev: &Device<Bound>, data: impl PinInit<T, E>, flags: Flag
> >  
> >      register_foreign(dev, data)
> >  }
> > +
> > +/// [`Devres`]-releaseable resource.
> > +///
> > +/// Register an object implementing this trait with [`register_release`]. Its `release`
> > +/// function will be called once the device is being unbound.
> > +pub trait Release {
> > +    /// The [`ForeignOwnable`] pointer type consumed by [`register_release`].
> > +    type Ptr: ForeignOwnable;
> > +
> > +    /// Called once the [`Device`] given to [`register_release`] is unbound.
> > +    fn release(this: Self::Ptr);
> > +}
> > +
> 
> I would like to point out the limitation of this design, say you have a
> `Foo` that can ipml `Release`, with this, I think you could only support
> either `Arc<Foo>` or `KBox<Foo>`. You cannot support both as the input
> for `register_release()`. Maybe we want:
> 
>     pub trait Release<Ptr: ForeignOwnable> {
>         fn release(this: Ptr);
>     }

Good catch! I think this wasn't possible without ForeignOwnable::Target.

Here's the diff for the change:

diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
index 92aca78874ff..42a9cd2812d8 100644
--- a/rust/kernel/devres.rs
+++ b/rust/kernel/devres.rs
@@ -358,12 +358,9 @@ pub fn register<T, E>(dev: &Device<Bound>, data: impl PinInit<T, E>, flags: Flag
 ///
 /// Register an object implementing this trait with [`register_release`]. Its `release`
 /// function will be called once the device is being unbound.
-pub trait Release {
-    /// The [`ForeignOwnable`] pointer type consumed by [`register_release`].
-    type Ptr: ForeignOwnable;
-
+pub trait Release<Ptr: ForeignOwnable> {
     /// Called once the [`Device`] given to [`register_release`] is unbound.
-    fn release(this: Self::Ptr);
+    fn release(this: Ptr);
 }

 /// Consume the `data`, [`Release::release`] and [`Drop::drop`] `data` once `dev` is unbound.
@@ -384,9 +381,7 @@ pub trait Release {
 ///     }
 /// }
 ///
-/// impl Release for Registration {
-///     type Ptr = Arc<Self>;
-///
+/// impl Release<Arc<Self>> for Registration {
 ///     fn release(this: Arc<Self>) {
 ///        // unregister
 ///     }
@@ -401,7 +396,7 @@ pub trait Release {
 pub fn register_release<P>(dev: &Device<Bound>, data: P) -> Result
 where
     P: ForeignOwnable,
-    P::Target: Release<Ptr = P> + Send,
+    P::Target: Release<P> + Send,
 {
     let ptr = data.into_foreign();

@@ -409,7 +404,7 @@ pub fn register_release<P>(dev: &Device<Bound>, data: P) -> Result
     unsafe extern "C" fn callback<P>(ptr: *mut kernel::ffi::c_void)
     where
         P: ForeignOwnable,
-        P::Target: Release<Ptr = P>,
+        P::Target: Release<P>,
     {
         // SAFETY: `ptr` is the pointer to the `ForeignOwnable` leaked above and hence valid.
         let data = unsafe { P::from_foreign(ptr.cast()) };

Re: [PATCH v4 5/5] rust: devres: implement register_release()

[Boqun Feng]

On Thu, Jun 26, 2025 at 10:48:03PM +0200, Danilo Krummrich wrote:
> On Thu, Jun 26, 2025 at 01:37:22PM -0700, Boqun Feng wrote:
> > On Thu, Jun 26, 2025 at 10:00:43PM +0200, Danilo Krummrich wrote:
> > > register_release() is useful when a device resource has associated data,
> > > but does not require the capability of accessing it or manually releasing
> > > it.
> > > 
> > > If we would want to be able to access the device resource and release the
> > > device resource manually before the device is unbound, but still keep
> > > access to the associated data, we could implement it as follows.
> > > 
> > > 	struct Registration<T> {
> > > 	   inner: Devres<RegistrationInner>,
> > > 	   data: T,
> > > 	}
> > > 
> > > However, if we never need to access the resource or release it manually,
> > > register_release() is great optimization for the above, since it does not
> > > require the synchronization of the Devres type.
> > > 
> > > Suggested-by: Alice Ryhl <aliceryhl@google.com>
> > > Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> > > ---
> > >  rust/kernel/devres.rs | 73 +++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 73 insertions(+)
> > > 
> > > diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
> > > index 3ce8d6161778..92aca78874ff 100644
> > > --- a/rust/kernel/devres.rs
> > > +++ b/rust/kernel/devres.rs
> > > @@ -353,3 +353,76 @@ pub fn register<T, E>(dev: &Device<Bound>, data: impl PinInit<T, E>, flags: Flag
> > >  
> > >      register_foreign(dev, data)
> > >  }
> > > +
> > > +/// [`Devres`]-releaseable resource.
> > > +///
> > > +/// Register an object implementing this trait with [`register_release`]. Its `release`
> > > +/// function will be called once the device is being unbound.
> > > +pub trait Release {
> > > +    /// The [`ForeignOwnable`] pointer type consumed by [`register_release`].
> > > +    type Ptr: ForeignOwnable;
> > > +
> > > +    /// Called once the [`Device`] given to [`register_release`] is unbound.
> > > +    fn release(this: Self::Ptr);
> > > +}
> > > +
> > 
> > I would like to point out the limitation of this design, say you have a
> > `Foo` that can ipml `Release`, with this, I think you could only support
> > either `Arc<Foo>` or `KBox<Foo>`. You cannot support both as the input
> > for `register_release()`. Maybe we want:
> > 
> >     pub trait Release<Ptr: ForeignOwnable> {
> >         fn release(this: Ptr);
> >     }
> 
> Good catch! I think this wasn't possible without ForeignOwnable::Target.
> 
> Here's the diff for the change:
> 

Looks good to me.

> diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
> index 92aca78874ff..42a9cd2812d8 100644
> --- a/rust/kernel/devres.rs
> +++ b/rust/kernel/devres.rs
> @@ -358,12 +358,9 @@ pub fn register<T, E>(dev: &Device<Bound>, data: impl PinInit<T, E>, flags: Flag
>  ///
>  /// Register an object implementing this trait with [`register_release`]. Its `release`
>  /// function will be called once the device is being unbound.
> -pub trait Release {
> -    /// The [`ForeignOwnable`] pointer type consumed by [`register_release`].
> -    type Ptr: ForeignOwnable;
> -
> +pub trait Release<Ptr: ForeignOwnable> {

My bad, is it possible to do

	pub trait Release<Ptr: ForeignOwnable<Target=Self>> {

? that's better IMO.

Regards,
Boqun

>      /// Called once the [`Device`] given to [`register_release`] is unbound.
> -    fn release(this: Self::Ptr);
> +    fn release(this: Ptr);
>  }
> 
>  /// Consume the `data`, [`Release::release`] and [`Drop::drop`] `data` once `dev` is unbound.
> @@ -384,9 +381,7 @@ pub trait Release {
>  ///     }
>  /// }
>  ///
> -/// impl Release for Registration {
> -///     type Ptr = Arc<Self>;
> -///
> +/// impl Release<Arc<Self>> for Registration {
>  ///     fn release(this: Arc<Self>) {
>  ///        // unregister
>  ///     }
> @@ -401,7 +396,7 @@ pub trait Release {
>  pub fn register_release<P>(dev: &Device<Bound>, data: P) -> Result
>  where
>      P: ForeignOwnable,
> -    P::Target: Release<Ptr = P> + Send,
> +    P::Target: Release<P> + Send,
>  {
>      let ptr = data.into_foreign();
> 
> @@ -409,7 +404,7 @@ pub fn register_release<P>(dev: &Device<Bound>, data: P) -> Result
>      unsafe extern "C" fn callback<P>(ptr: *mut kernel::ffi::c_void)
>      where
>          P: ForeignOwnable,
> -        P::Target: Release<Ptr = P>,
> +        P::Target: Release<P>,
>      {
>          // SAFETY: `ptr` is the pointer to the `ForeignOwnable` leaked above and hence valid.
>          let data = unsafe { P::from_foreign(ptr.cast()) };

Re: [PATCH v4 5/5] rust: devres: implement register_release()

[Danilo Krummrich]

On 6/26/25 11:16 PM, Boqun Feng wrote:
> On Thu, Jun 26, 2025 at 10:48:03PM +0200, Danilo Krummrich wrote:
>> On Thu, Jun 26, 2025 at 01:37:22PM -0700, Boqun Feng wrote:
>>> On Thu, Jun 26, 2025 at 10:00:43PM +0200, Danilo Krummrich wrote:
>>>> register_release() is useful when a device resource has associated data,
>>>> but does not require the capability of accessing it or manually releasing
>>>> it.
>>>>
>>>> If we would want to be able to access the device resource and release the
>>>> device resource manually before the device is unbound, but still keep
>>>> access to the associated data, we could implement it as follows.
>>>>
>>>> 	struct Registration<T> {
>>>> 	   inner: Devres<RegistrationInner>,
>>>> 	   data: T,
>>>> 	}
>>>>
>>>> However, if we never need to access the resource or release it manually,
>>>> register_release() is great optimization for the above, since it does not
>>>> require the synchronization of the Devres type.
>>>>
>>>> Suggested-by: Alice Ryhl <aliceryhl@google.com>
>>>> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
>>>> ---
>>>>   rust/kernel/devres.rs | 73 +++++++++++++++++++++++++++++++++++++++++++
>>>>   1 file changed, 73 insertions(+)
>>>>
>>>> diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
>>>> index 3ce8d6161778..92aca78874ff 100644
>>>> --- a/rust/kernel/devres.rs
>>>> +++ b/rust/kernel/devres.rs
>>>> @@ -353,3 +353,76 @@ pub fn register<T, E>(dev: &Device<Bound>, data: impl PinInit<T, E>, flags: Flag
>>>>   
>>>>       register_foreign(dev, data)
>>>>   }
>>>> +
>>>> +/// [`Devres`]-releaseable resource.
>>>> +///
>>>> +/// Register an object implementing this trait with [`register_release`]. Its `release`
>>>> +/// function will be called once the device is being unbound.
>>>> +pub trait Release {
>>>> +    /// The [`ForeignOwnable`] pointer type consumed by [`register_release`].
>>>> +    type Ptr: ForeignOwnable;
>>>> +
>>>> +    /// Called once the [`Device`] given to [`register_release`] is unbound.
>>>> +    fn release(this: Self::Ptr);
>>>> +}
>>>> +
>>>
>>> I would like to point out the limitation of this design, say you have a
>>> `Foo` that can ipml `Release`, with this, I think you could only support
>>> either `Arc<Foo>` or `KBox<Foo>`. You cannot support both as the input
>>> for `register_release()`. Maybe we want:
>>>
>>>      pub trait Release<Ptr: ForeignOwnable> {
>>>          fn release(this: Ptr);
>>>      }
>>
>> Good catch! I think this wasn't possible without ForeignOwnable::Target.
>>
>> Here's the diff for the change:
>>
> 
> Looks good to me.
> 
>> diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
>> index 92aca78874ff..42a9cd2812d8 100644
>> --- a/rust/kernel/devres.rs
>> +++ b/rust/kernel/devres.rs
>> @@ -358,12 +358,9 @@ pub fn register<T, E>(dev: &Device<Bound>, data: impl PinInit<T, E>, flags: Flag
>>   ///
>>   /// Register an object implementing this trait with [`register_release`]. Its `release`
>>   /// function will be called once the device is being unbound.
>> -pub trait Release {
>> -    /// The [`ForeignOwnable`] pointer type consumed by [`register_release`].
>> -    type Ptr: ForeignOwnable;
>> -
>> +pub trait Release<Ptr: ForeignOwnable> {
> 
> My bad, is it possible to do
> 
> 	pub trait Release<Ptr: ForeignOwnable<Target=Self>> {
> 
> ? that's better IMO.

Sure, that works.

Re: [PATCH v4 5/5] rust: devres: implement register_release()

[Boqun Feng]

On Thu, Jun 26, 2025 at 11:20:21PM +0200, Danilo Krummrich wrote:
[...]
> > > +pub trait Release<Ptr: ForeignOwnable> {
> > 
> > My bad, is it possible to do
> > 
> > 	pub trait Release<Ptr: ForeignOwnable<Target=Self>> {
> > 
> > ? that's better IMO.
> 
> Sure, that works.

Thanks! With that feel free to add:

Reviewed-by: Boqun Feng <boqun.feng@gmail.com>

Regards,
Boqun

Re: [PATCH v4 4/5] rust: types: ForeignOwnable: Add type Target

[Benno Lossin]

On Thu Jun 26, 2025 at 10:20 PM CEST, Boqun Feng wrote:
> On Thu, Jun 26, 2025 at 10:00:42PM +0200, Danilo Krummrich wrote:
>> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
>> index 3958a5f44d56..74c787b352a9 100644
>> --- a/rust/kernel/types.rs
>> +++ b/rust/kernel/types.rs
>> @@ -27,6 +27,9 @@
>>  /// [`into_foreign`]: Self::into_foreign
>>  /// [`PointedTo`]: Self::PointedTo
>>  pub unsafe trait ForeignOwnable: Sized {
>> +    /// The payload type of the foreign-owned value.
>> +    type Target;
>
> I think `ForeignOwnable` also implies a `T` maybe get dropped via a
> pointer from `into_foreign()`. Not sure it's worth mentioning though.

What? How would that happen?

---
Cheers,
Benno

Re: [PATCH v4 5/5] rust: devres: implement register_release()

[Benno Lossin]

On Thu Jun 26, 2025 at 10:48 PM CEST, Danilo Krummrich wrote:
> On Thu, Jun 26, 2025 at 01:37:22PM -0700, Boqun Feng wrote:
>> On Thu, Jun 26, 2025 at 10:00:43PM +0200, Danilo Krummrich wrote:
>> > +/// [`Devres`]-releaseable resource.
>> > +///
>> > +/// Register an object implementing this trait with [`register_release`]. Its `release`
>> > +/// function will be called once the device is being unbound.
>> > +pub trait Release {
>> > +    /// The [`ForeignOwnable`] pointer type consumed by [`register_release`].
>> > +    type Ptr: ForeignOwnable;
>> > +
>> > +    /// Called once the [`Device`] given to [`register_release`] is unbound.
>> > +    fn release(this: Self::Ptr);
>> > +}
>> > +
>> 
>> I would like to point out the limitation of this design, say you have a
>> `Foo` that can ipml `Release`, with this, I think you could only support
>> either `Arc<Foo>` or `KBox<Foo>`. You cannot support both as the input
>> for `register_release()`. Maybe we want:
>> 
>>     pub trait Release<Ptr: ForeignOwnable> {
>>         fn release(this: Ptr);
>>     }
>
> Good catch! I think this wasn't possible without ForeignOwnable::Target.

Hmm do we really need that? Normally you either store a type in a shared
or a non-shared manner and not both...

---
Cheers,
Benno

Re: [PATCH v4 4/5] rust: types: ForeignOwnable: Add type Target

[Boqun Feng]

On Thu, Jun 26, 2025, at 4:17 PM, Benno Lossin wrote:
> On Thu Jun 26, 2025 at 10:20 PM CEST, Boqun Feng wrote:
>> On Thu, Jun 26, 2025 at 10:00:42PM +0200, Danilo Krummrich wrote:
>>> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
>>> index 3958a5f44d56..74c787b352a9 100644
>>> --- a/rust/kernel/types.rs
>>> +++ b/rust/kernel/types.rs
>>> @@ -27,6 +27,9 @@
>>>  /// [`into_foreign`]: Self::into_foreign
>>>  /// [`PointedTo`]: Self::PointedTo
>>>  pub unsafe trait ForeignOwnable: Sized {
>>> +    /// The payload type of the foreign-owned value.
>>> +    type Target;
>>
>> I think `ForeignOwnable` also implies a `T` maybe get dropped via a
>> pointer from `into_foreign()`. Not sure it's worth mentioning though.
>
> What? How would that happen?
>

The owner of the pointer can do from_foreign() and eventually drop
the ForeignOwnable, hence dropping T.

Regards,
Boqun

> ---
> Cheers,
> Benno

Re: [PATCH v4 4/5] rust: types: ForeignOwnable: Add type Target

[Benno Lossin]

On Thu Jun 26, 2025 at 10:00 PM CEST, Danilo Krummrich wrote:
> ForeignOwnable::Target defines the payload data of a ForeignOwnable. For
> Arc<T> for instance, ForeignOwnable::Target would just be T.
>
> This is useful for cases where a trait bound is required on the target
> type of the ForeignOwnable. For instance:
>
> 	fn example<P>(data: P)
> 	   where
> 	      P: ForeignOwnable,
> 	      P::Target: MyTrait,
> 	{}
>
> Suggested-by: Benno Lossin <lossin@kernel.org>
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>

Reviewed-by: Benno Lossin <lossin@kernel.org>

We might also be able to add a `Deref<Target = Self::Target>` bound on
`Borrowed` and `BorrowedMut`, but we should only do that when necessary.

---
Cheers,
Benno

> ---
>  rust/kernel/alloc/kbox.rs | 2 ++
>  rust/kernel/sync/arc.rs   | 1 +
>  rust/kernel/types.rs      | 4 ++++
>  3 files changed, 7 insertions(+)

Re: [PATCH v4 3/5] rust: devres: get rid of Devres' inner Arc

[Benno Lossin]

On Thu Jun 26, 2025 at 10:00 PM CEST, Danilo Krummrich wrote:
> diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
> index 60b86f370284..47653c14838b 100644
> --- a/drivers/gpu/nova-core/gpu.rs
> +++ b/drivers/gpu/nova-core/gpu.rs

> @@ -161,14 +161,14 @@ fn new(bar: &Bar0) -> Result<Spec> {
>  pub(crate) struct Gpu {
>      spec: Spec,
>      /// MMIO mapping of PCI BAR 0
> -    bar: Devres<Bar0>,
> +    bar: Arc<Devres<Bar0>>,

Can't you store it inline, given that you return an `impl PinInit<Self>`
below?

>      fw: Firmware,
>  }
>  
>  impl Gpu {
>      pub(crate) fn new(
>          pdev: &pci::Device<device::Bound>,
> -        devres_bar: Devres<Bar0>,
> +        devres_bar: Arc<Devres<Bar0>>,
>      ) -> Result<impl PinInit<Self>> {

While I see this code, is it really necessary to return `Result`
wrapping the initializer here? I think it's probably better to return
`impl PinInit<Self, Error>` instead. (of course in a different patch/an
issue)

>          let bar = devres_bar.access(pdev.as_ref())?;
>          let spec = Spec::new(bar)?;

> @@ -44,6 +49,10 @@ struct DevresInner<T: Send> {
>  /// [`Devres`] users should make sure to simply free the corresponding backing resource in `T`'s
>  /// [`Drop`] implementation.
>  ///
> +/// # Invariants
> +///
> +/// [`Self::inner`] is guaranteed to be initialized and is always accessed read-only.
> +///

Let's put this section below the examples, I really ought to write the
safety docs one day and let everyone vote on this kind of stuff...

>  /// # Example
>  ///
>  /// ```no_run

> @@ -213,44 +233,63 @@ pub fn new(dev: &Device<Bound>, data: T, flags: Flags) -> Result<Self> {
>      /// }
>      /// ```
>      pub fn access<'a>(&'a self, dev: &'a Device<Bound>) -> Result<&'a T> {
> -        if self.0.dev.as_raw() != dev.as_raw() {
> +        if self.dev.as_raw() != dev.as_raw() {
>              return Err(EINVAL);
>          }
>  
>          // SAFETY: `dev` being the same device as the device this `Devres` has been created for
> -        // proves that `self.0.data` hasn't been revoked and is guaranteed to not be revoked as
> -        // long as `dev` lives; `dev` lives at least as long as `self`.
> -        Ok(unsafe { self.0.data.access() })
> +        // proves that `self.data` hasn't been revoked and is guaranteed to not be revoked as long
> +        // as `dev` lives; `dev` lives at least as long as `self`.

What if the device has been unbound and a new device has been allocated
in the exact same memory?

---
Cheers,
Benno

> +        Ok(unsafe { self.data().access() })
>      }

Re: [PATCH v4 4/5] rust: types: ForeignOwnable: Add type Target

[Benno Lossin]

On Fri Jun 27, 2025 at 1:21 AM CEST, Boqun Feng wrote:
> On Thu, Jun 26, 2025, at 4:17 PM, Benno Lossin wrote:
>> On Thu Jun 26, 2025 at 10:20 PM CEST, Boqun Feng wrote:
>>> On Thu, Jun 26, 2025 at 10:00:42PM +0200, Danilo Krummrich wrote:
>>>> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
>>>> index 3958a5f44d56..74c787b352a9 100644
>>>> --- a/rust/kernel/types.rs
>>>> +++ b/rust/kernel/types.rs
>>>> @@ -27,6 +27,9 @@
>>>>  /// [`into_foreign`]: Self::into_foreign
>>>>  /// [`PointedTo`]: Self::PointedTo
>>>>  pub unsafe trait ForeignOwnable: Sized {
>>>> +    /// The payload type of the foreign-owned value.
>>>> +    type Target;
>>>
>>> I think `ForeignOwnable` also implies a `T` maybe get dropped via a
>>> pointer from `into_foreign()`. Not sure it's worth mentioning though.
>>
>> What? How would that happen?
>
> The owner of the pointer can do from_foreign() and eventually drop
> the ForeignOwnable, hence dropping T.

I'm confused, you said `into_foreign` above. I don't think any sensible
ForeignOwnable implementation will drop a T in any of its functions.

---
Cheers,
Benno

Re: [PATCH v4 4/5] rust: types: ForeignOwnable: Add type Target

[Boqun Feng]

On Thu, Jun 26, 2025, at 4:36 PM, Benno Lossin wrote:
> On Fri Jun 27, 2025 at 1:21 AM CEST, Boqun Feng wrote:
>> On Thu, Jun 26, 2025, at 4:17 PM, Benno Lossin wrote:
>>> On Thu Jun 26, 2025 at 10:20 PM CEST, Boqun Feng wrote:
>>>> On Thu, Jun 26, 2025 at 10:00:42PM +0200, Danilo Krummrich wrote:
>>>>> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
>>>>> index 3958a5f44d56..74c787b352a9 100644
>>>>> --- a/rust/kernel/types.rs
>>>>> +++ b/rust/kernel/types.rs
>>>>> @@ -27,6 +27,9 @@
>>>>>  /// [`into_foreign`]: Self::into_foreign
>>>>>  /// [`PointedTo`]: Self::PointedTo
>>>>>  pub unsafe trait ForeignOwnable: Sized {
>>>>> +    /// The payload type of the foreign-owned value.
>>>>> +    type Target;
>>>>
>>>> I think `ForeignOwnable` also implies a `T` maybe get dropped via a
>>>> pointer from `into_foreign()`. Not sure it's worth mentioning though.
>>>
>>> What? How would that happen?
>>
>> The owner of the pointer can do from_foreign() and eventually drop
>> the ForeignOwnable, hence dropping T.
>
> I'm confused, you said `into_foreign` above. I don't think any sensible
> ForeignOwnable implementation will drop a T in any of its functions.
>

A KBox<T> would drop T when it gets dropped, no?
A Arc<T> would drop T when it gets dropped if it’s the last refcount.

I was trying to say “ForeignOwnable::drop() may implies Target::drop()”,
that’s what a “payload” means. Maybe that I used “T” instead of “Target”
in the original message caused confusion?

Regards,
Boqun

> ---
> Cheers,
> Benno

Re: [PATCH v4 3/5] rust: devres: get rid of Devres' inner Arc

[Danilo Krummrich]

On Fri, Jun 27, 2025 at 01:33:41AM +0200, Benno Lossin wrote:
> On Thu Jun 26, 2025 at 10:00 PM CEST, Danilo Krummrich wrote:
> > diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
> > index 60b86f370284..47653c14838b 100644
> > --- a/drivers/gpu/nova-core/gpu.rs
> > +++ b/drivers/gpu/nova-core/gpu.rs
> 
> > @@ -161,14 +161,14 @@ fn new(bar: &Bar0) -> Result<Spec> {
> >  pub(crate) struct Gpu {
> >      spec: Spec,
> >      /// MMIO mapping of PCI BAR 0
> > -    bar: Devres<Bar0>,
> > +    bar: Arc<Devres<Bar0>>,
> 
> Can't you store it inline, given that you return an `impl PinInit<Self>`
> below?

I could, but I already know that we'll have to share bar later on.

> >      fw: Firmware,
> >  }
> >  
> >  impl Gpu {
> >      pub(crate) fn new(
> >          pdev: &pci::Device<device::Bound>,
> > -        devres_bar: Devres<Bar0>,
> > +        devres_bar: Arc<Devres<Bar0>>,
> >      ) -> Result<impl PinInit<Self>> {
> 
> While I see this code, is it really necessary to return `Result`
> wrapping the initializer here? I think it's probably better to return
> `impl PinInit<Self, Error>` instead. (of course in a different patch/an
> issue)

I will double check, but it's rather unlikely it makes sense. There's a lot of
initialization going on in Gpu::new(), the try_pin_init! call would probably get
too crazy.

> 
> >          let bar = devres_bar.access(pdev.as_ref())?;
> >          let spec = Spec::new(bar)?;
> 
> > @@ -44,6 +49,10 @@ struct DevresInner<T: Send> {
> >  /// [`Devres`] users should make sure to simply free the corresponding backing resource in `T`'s
> >  /// [`Drop`] implementation.
> >  ///
> > +/// # Invariants
> > +///
> > +/// [`Self::inner`] is guaranteed to be initialized and is always accessed read-only.
> > +///
> 
> Let's put this section below the examples, I really ought to write the
> safety docs one day and let everyone vote on this kind of stuff...

Sure!

> >  /// # Example
> >  ///
> >  /// ```no_run
> 
> > @@ -213,44 +233,63 @@ pub fn new(dev: &Device<Bound>, data: T, flags: Flags) -> Result<Self> {
> >      /// }
> >      /// ```
> >      pub fn access<'a>(&'a self, dev: &'a Device<Bound>) -> Result<&'a T> {
> > -        if self.0.dev.as_raw() != dev.as_raw() {
> > +        if self.dev.as_raw() != dev.as_raw() {
> >              return Err(EINVAL);
> >          }
> >  
> >          // SAFETY: `dev` being the same device as the device this `Devres` has been created for
> > -        // proves that `self.0.data` hasn't been revoked and is guaranteed to not be revoked as
> > -        // long as `dev` lives; `dev` lives at least as long as `self`.
> > -        Ok(unsafe { self.0.data.access() })
> > +        // proves that `self.data` hasn't been revoked and is guaranteed to not be revoked as long
> > +        // as `dev` lives; `dev` lives at least as long as `self`.
> 
> What if the device has been unbound and a new device has been allocated
> in the exact same memory?

Unbound doesn't mean freed. Devres holds a reference of the device is was
created with, so it is impossible that it has been freed.

Re: [PATCH v4 4/5] rust: types: ForeignOwnable: Add type Target

[Boqun Feng]

On Thu, Jun 26, 2025, at 4:45 PM, Boqun Feng wrote:
> On Thu, Jun 26, 2025, at 4:36 PM, Benno Lossin wrote:
>> On Fri Jun 27, 2025 at 1:21 AM CEST, Boqun Feng wrote:
>>> On Thu, Jun 26, 2025, at 4:17 PM, Benno Lossin wrote:
>>>> On Thu Jun 26, 2025 at 10:20 PM CEST, Boqun Feng wrote:
>>>>> On Thu, Jun 26, 2025 at 10:00:42PM +0200, Danilo Krummrich wrote:
>>>>>> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
>>>>>> index 3958a5f44d56..74c787b352a9 100644
>>>>>> --- a/rust/kernel/types.rs
>>>>>> +++ b/rust/kernel/types.rs
>>>>>> @@ -27,6 +27,9 @@
>>>>>>  /// [`into_foreign`]: Self::into_foreign
>>>>>>  /// [`PointedTo`]: Self::PointedTo
>>>>>>  pub unsafe trait ForeignOwnable: Sized {
>>>>>> +    /// The payload type of the foreign-owned value.
>>>>>> +    type Target;
>>>>>
>>>>> I think `ForeignOwnable` also implies a `T` maybe get dropped via a
>>>>> pointer from `into_foreign()`. Not sure it's worth mentioning though.
>>>>
>>>> What? How would that happen?
>>>
>>> The owner of the pointer can do from_foreign() and eventually drop
>>> the ForeignOwnable, hence dropping T.
>>
>> I'm confused, you said `into_foreign` above. I don't think any sensible
>> ForeignOwnable implementation will drop a T in any of its functions.
>>
>
> A KBox<T> would drop T when it gets dropped, no?
> A Arc<T> would drop T when it gets dropped if it’s the last refcount.
>
> I was trying to say “ForeignOwnable::drop() may implies Target::drop()”,
> that’s what a “payload” means. Maybe that I used “T” instead of “Target”
> in the original message caused confusion?
>

The point is whichever receives the pointer from a into_foreign()
would owns the Target, because it can from_foreign() and
drop the ForeignOwnable. So for example, if the pointer can
be passed across threads, that means Target needs to be Send.

Regards,
Boqun

> Regards,
> Boqun
>
>> ---
>> Cheers,
>> Benno

Re: [PATCH v4 3/5] rust: devres: get rid of Devres' inner Arc

[Benno Lossin]

On Fri Jun 27, 2025 at 1:53 AM CEST, Danilo Krummrich wrote:
> On Fri, Jun 27, 2025 at 01:33:41AM +0200, Benno Lossin wrote:
>> On Thu Jun 26, 2025 at 10:00 PM CEST, Danilo Krummrich wrote:
>> > diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
>> > index 60b86f370284..47653c14838b 100644
>> > --- a/drivers/gpu/nova-core/gpu.rs
>> > +++ b/drivers/gpu/nova-core/gpu.rs
>> 
>> > @@ -161,14 +161,14 @@ fn new(bar: &Bar0) -> Result<Spec> {
>> >  pub(crate) struct Gpu {
>> >      spec: Spec,
>> >      /// MMIO mapping of PCI BAR 0
>> > -    bar: Devres<Bar0>,
>> > +    bar: Arc<Devres<Bar0>>,
>> 
>> Can't you store it inline, given that you return an `impl PinInit<Self>`
>> below?
>
> I could, but I already know that we'll have to share bar later on.

Ahh, planning ahead :)

How would you have shared it if you didn't do the devres rework? Or is
this one of the reasons to do that?

>> >      fw: Firmware,
>> >  }
>> >  
>> >  impl Gpu {
>> >      pub(crate) fn new(
>> >          pdev: &pci::Device<device::Bound>,
>> > -        devres_bar: Devres<Bar0>,
>> > +        devres_bar: Arc<Devres<Bar0>>,
>> >      ) -> Result<impl PinInit<Self>> {
>> 
>> While I see this code, is it really necessary to return `Result`
>> wrapping the initializer here? I think it's probably better to return
>> `impl PinInit<Self, Error>` instead. (of course in a different patch/an
>> issue)
>
> I will double check, but it's rather unlikely it makes sense. There's a lot of
> initialization going on in Gpu::new(), the try_pin_init! call would probably get
> too crazy.

Makes sense, I don't have too much data on where to place the error,
since I only have had rather simple uses of pin-init. So you could have
a case where it makes sense to put the error outside of the initializer.

>> >  /// # Example
>> >  ///
>> >  /// ```no_run
>> 
>> > @@ -213,44 +233,63 @@ pub fn new(dev: &Device<Bound>, data: T, flags: Flags) -> Result<Self> {
>> >      /// }
>> >      /// ```
>> >      pub fn access<'a>(&'a self, dev: &'a Device<Bound>) -> Result<&'a T> {
>> > -        if self.0.dev.as_raw() != dev.as_raw() {
>> > +        if self.dev.as_raw() != dev.as_raw() {
>> >              return Err(EINVAL);
>> >          }
>> >  
>> >          // SAFETY: `dev` being the same device as the device this `Devres` has been created for
>> > -        // proves that `self.0.data` hasn't been revoked and is guaranteed to not be revoked as
>> > -        // long as `dev` lives; `dev` lives at least as long as `self`.
>> > -        Ok(unsafe { self.0.data.access() })
>> > +        // proves that `self.data` hasn't been revoked and is guaranteed to not be revoked as long
>> > +        // as `dev` lives; `dev` lives at least as long as `self`.
>> 
>> What if the device has been unbound and a new device has been allocated
>> in the exact same memory?
>
> Unbound doesn't mean freed. Devres holds a reference of the device is was
> created with, so it is impossible that it has been freed.

Ahh right, I thought I was missing something! This also should be
mentioned in the safety comment though! Feel free to do it in some later
patch or create a good-first-issue :)

---
Cheers,
Benno

Re: [PATCH v4 4/5] rust: types: ForeignOwnable: Add type Target

[Benno Lossin]

On Fri Jun 27, 2025 at 1:55 AM CEST, Boqun Feng wrote:
> On Thu, Jun 26, 2025, at 4:45 PM, Boqun Feng wrote:
>> On Thu, Jun 26, 2025, at 4:36 PM, Benno Lossin wrote:
>>> On Fri Jun 27, 2025 at 1:21 AM CEST, Boqun Feng wrote:
>>>> On Thu, Jun 26, 2025, at 4:17 PM, Benno Lossin wrote:
>>>>> On Thu Jun 26, 2025 at 10:20 PM CEST, Boqun Feng wrote:
>>>>>> On Thu, Jun 26, 2025 at 10:00:42PM +0200, Danilo Krummrich wrote:
>>>>>>> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
>>>>>>> index 3958a5f44d56..74c787b352a9 100644
>>>>>>> --- a/rust/kernel/types.rs
>>>>>>> +++ b/rust/kernel/types.rs
>>>>>>> @@ -27,6 +27,9 @@
>>>>>>>  /// [`into_foreign`]: Self::into_foreign
>>>>>>>  /// [`PointedTo`]: Self::PointedTo
>>>>>>>  pub unsafe trait ForeignOwnable: Sized {
>>>>>>> +    /// The payload type of the foreign-owned value.
>>>>>>> +    type Target;
>>>>>>
>>>>>> I think `ForeignOwnable` also implies a `T` maybe get dropped via a
>>>>>> pointer from `into_foreign()`. Not sure it's worth mentioning though.
>>>>>
>>>>> What? How would that happen?
>>>>
>>>> The owner of the pointer can do from_foreign() and eventually drop
>>>> the ForeignOwnable, hence dropping T.
>>>
>>> I'm confused, you said `into_foreign` above. I don't think any sensible
>>> ForeignOwnable implementation will drop a T in any of its functions.
>>>
>>
>> A KBox<T> would drop T when it gets dropped, no?
>> A Arc<T> would drop T when it gets dropped if it’s the last refcount.
>>
>> I was trying to say “ForeignOwnable::drop() may implies Target::drop()”,
>> that’s what a “payload” means. Maybe that I used “T” instead of “Target”
>> in the original message caused confusion?

Ah now I understand what you are saying. Your mentioning of
`into_foreign` and `from_foreign` confused me. Yes a `ForeignOwnable`
may drop a `T`/`Target` in its own drop function. But I don't think we
need to document that.

> The point is whichever receives the pointer from a into_foreign()
> would owns the Target, because it can from_foreign() and
> drop the ForeignOwnable. So for example, if the pointer can
> be passed across threads, that means Target needs to be Send.

We should solve this in a different manner. Document the `Send` & `Sync`
requirements on `into_foreign`. So when you turn a `P: ForeignOwnable`
that is `!Send` into a raw pointer, you are not allowed to call
`from_foreign` on a different thread.

If `P: !Sync` then you're not allowed to call `borrow[_mut]` on the
pointer from two different threads (ie only the one that is currently
owning the value is allowed to call that).

---
Cheers,
Benno

Re: [PATCH v4 4/5] rust: types: ForeignOwnable: Add type Target

[Miguel Ojeda]

On Thu, Jun 26, 2025 at 10:01 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> ForeignOwnable::Target defines the payload data of a ForeignOwnable. For
> Arc<T> for instance, ForeignOwnable::Target would just be T.
>
> This is useful for cases where a trait bound is required on the target
> type of the ForeignOwnable. For instance:
>
>         fn example<P>(data: P)
>            where
>               P: ForeignOwnable,
>               P::Target: MyTrait,
>         {}
>
> Suggested-by: Benno Lossin <lossin@kernel.org>
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>

Acked-by: Miguel Ojeda <ojeda@kernel.org>

Cheers,
Miguel

Re: [PATCH v4 3/5] rust: devres: get rid of Devres' inner Arc

[Benno Lossin]

On Thu Jun 26, 2025 at 10:00 PM CEST, Danilo Krummrich wrote:
> So far Devres uses an inner memory allocation and reference count, i.e.
> an inner Arc, in order to ensure that the devres callback can't run into
> a use-after-free in case where the Devres object is dropped while the
> devres callback runs concurrently.
>
> Instead, use a completion in order to avoid a potential UAF: In
> Devres::drop(), if we detect that we can't remove the devres action
> anymore, we wait for the completion that is completed from the devres
> callback. If, in turn, we were able to successfully remove the devres
> action, we can just go ahead.
>
> This, again, allows us to get rid of the internal Arc, and instead let
> Devres consume an `impl PinInit<T, E>` in order to return an
> `impl PinInit<Devres<T>, E>`, which enables us to get away with less
> memory allocations.
>
> Additionally, having the resulting explicit synchronization in
> Devres::drop() prevents potential subtle undesired side effects of the
> devres callback dropping the final Arc reference asynchronously within
> the devres callback.
>
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>

With the invariants section moved:

Reviewed-by: Benno Lossin <lossin@kernel.org>

---
Cheers,
Benno

> ---
>  drivers/gpu/nova-core/driver.rs |   7 +-
>  drivers/gpu/nova-core/gpu.rs    |   6 +-
>  rust/kernel/devres.rs           | 213 +++++++++++++++++++-------------
>  rust/kernel/pci.rs              |  20 +--
>  samples/rust/rust_driver_pci.rs |  19 +--
>  5 files changed, 154 insertions(+), 111 deletions(-)

Re: [PATCH v4 5/5] rust: devres: implement register_release()

[Boqun Feng]

On Fri, Jun 27, 2025 at 01:19:53AM +0200, Benno Lossin wrote:
> On Thu Jun 26, 2025 at 10:48 PM CEST, Danilo Krummrich wrote:
> > On Thu, Jun 26, 2025 at 01:37:22PM -0700, Boqun Feng wrote:
> >> On Thu, Jun 26, 2025 at 10:00:43PM +0200, Danilo Krummrich wrote:
> >> > +/// [`Devres`]-releaseable resource.
> >> > +///
> >> > +/// Register an object implementing this trait with [`register_release`]. Its `release`
> >> > +/// function will be called once the device is being unbound.
> >> > +pub trait Release {
> >> > +    /// The [`ForeignOwnable`] pointer type consumed by [`register_release`].
> >> > +    type Ptr: ForeignOwnable;
> >> > +
> >> > +    /// Called once the [`Device`] given to [`register_release`] is unbound.
> >> > +    fn release(this: Self::Ptr);
> >> > +}
> >> > +
> >> 
> >> I would like to point out the limitation of this design, say you have a
> >> `Foo` that can ipml `Release`, with this, I think you could only support
> >> either `Arc<Foo>` or `KBox<Foo>`. You cannot support both as the input
> >> for `register_release()`. Maybe we want:
> >> 
> >>     pub trait Release<Ptr: ForeignOwnable> {
> >>         fn release(this: Ptr);
> >>     }
> >
> > Good catch! I think this wasn't possible without ForeignOwnable::Target.
> 
> Hmm do we really need that? Normally you either store a type in a shared

I think it might be quite common, for example, `Foo` may be a general
watchdog for a subsystem, for one driver, there might be multiple
devices that could feed the dog, for another driver, there might be only
one. For the first case we need Arc<Watchdog> or the second we can do
Box<Watchdog>.

What's the downside?

Regards,
Boqun

> or a non-shared manner and not both...
> 
> ---
> Cheers,
> Benno
> 

Re: [PATCH v4 5/5] rust: devres: implement register_release()

[Benno Lossin]

On Sat Jun 28, 2025 at 12:06 AM CEST, Boqun Feng wrote:
> On Fri, Jun 27, 2025 at 01:19:53AM +0200, Benno Lossin wrote:
>> On Thu Jun 26, 2025 at 10:48 PM CEST, Danilo Krummrich wrote:
>> > On Thu, Jun 26, 2025 at 01:37:22PM -0700, Boqun Feng wrote:
>> >> On Thu, Jun 26, 2025 at 10:00:43PM +0200, Danilo Krummrich wrote:
>> >> > +/// [`Devres`]-releaseable resource.
>> >> > +///
>> >> > +/// Register an object implementing this trait with [`register_release`]. Its `release`
>> >> > +/// function will be called once the device is being unbound.
>> >> > +pub trait Release {
>> >> > +    /// The [`ForeignOwnable`] pointer type consumed by [`register_release`].
>> >> > +    type Ptr: ForeignOwnable;
>> >> > +
>> >> > +    /// Called once the [`Device`] given to [`register_release`] is unbound.
>> >> > +    fn release(this: Self::Ptr);
>> >> > +}
>> >> > +
>> >> 
>> >> I would like to point out the limitation of this design, say you have a
>> >> `Foo` that can ipml `Release`, with this, I think you could only support
>> >> either `Arc<Foo>` or `KBox<Foo>`. You cannot support both as the input
>> >> for `register_release()`. Maybe we want:
>> >> 
>> >>     pub trait Release<Ptr: ForeignOwnable> {
>> >>         fn release(this: Ptr);
>> >>     }
>> >
>> > Good catch! I think this wasn't possible without ForeignOwnable::Target.
>> 
>> Hmm do we really need that? Normally you either store a type in a shared
>
> I think it might be quite common, for example, `Foo` may be a general
> watchdog for a subsystem, for one driver, there might be multiple
> devices that could feed the dog, for another driver, there might be only
> one. For the first case we need Arc<Watchdog> or the second we can do
> Box<Watchdog>.

I guess then the original `&self` design is better? Not sure...

> What's the downside?

You'll need to implement `Release` twice:

    impl Release<Box<Self>> for Foo {
        fn release(this: Box<Self>) {
            /* ... */
        }
    }

    impl Release<Arc<Self>> for Foo {
        fn release(this: Arc<Self>) {
            /* ... */
        }
    }

This also means that you can have different behavior for `Box` and
`Arc`...

---
Cheers,
Benno

Re: [PATCH v4 5/5] rust: devres: implement register_release()

[Boqun Feng]

On Sat, Jun 28, 2025 at 08:06:52AM +0200, Benno Lossin wrote:
> On Sat Jun 28, 2025 at 12:06 AM CEST, Boqun Feng wrote:
> > On Fri, Jun 27, 2025 at 01:19:53AM +0200, Benno Lossin wrote:
> >> On Thu Jun 26, 2025 at 10:48 PM CEST, Danilo Krummrich wrote:
> >> > On Thu, Jun 26, 2025 at 01:37:22PM -0700, Boqun Feng wrote:
> >> >> On Thu, Jun 26, 2025 at 10:00:43PM +0200, Danilo Krummrich wrote:
> >> >> > +/// [`Devres`]-releaseable resource.
> >> >> > +///
> >> >> > +/// Register an object implementing this trait with [`register_release`]. Its `release`
> >> >> > +/// function will be called once the device is being unbound.
> >> >> > +pub trait Release {
> >> >> > +    /// The [`ForeignOwnable`] pointer type consumed by [`register_release`].
> >> >> > +    type Ptr: ForeignOwnable;
> >> >> > +
> >> >> > +    /// Called once the [`Device`] given to [`register_release`] is unbound.
> >> >> > +    fn release(this: Self::Ptr);
> >> >> > +}
> >> >> > +
> >> >> 
> >> >> I would like to point out the limitation of this design, say you have a
> >> >> `Foo` that can ipml `Release`, with this, I think you could only support
> >> >> either `Arc<Foo>` or `KBox<Foo>`. You cannot support both as the input
> >> >> for `register_release()`. Maybe we want:
> >> >> 
> >> >>     pub trait Release<Ptr: ForeignOwnable> {
> >> >>         fn release(this: Ptr);
> >> >>     }
> >> >
> >> > Good catch! I think this wasn't possible without ForeignOwnable::Target.
> >> 
> >> Hmm do we really need that? Normally you either store a type in a shared
> >
> > I think it might be quite common, for example, `Foo` may be a general
> > watchdog for a subsystem, for one driver, there might be multiple
> > devices that could feed the dog, for another driver, there might be only
> > one. For the first case we need Arc<Watchdog> or the second we can do
> > Box<Watchdog>.
> 
> I guess then the original `&self` design is better? Not sure...
> 

This is what you said in v3:

"""
and then `register_release` is:

    pub fn register_release<T: Release>(dev: &Device<Bound>, data: T::Ptr) -> Result

This way, one can store a `Box<T>` and get access to the `T` at the end.
Or if they store the value in an `Arc<T>`, they have the option to clone
it and give it to somewhere else.
"""

I think that's the reason why we think the current version (the
associate type design) is better than `&self`?

The generic type design (i.e. Release<P: ForeignOwnable>) just further
allows this "different behaviors between Box and Arc" for the same type
T. I think it's a natural extension of the current design and provides
some better flexibility.

> > What's the downside?
> 
> You'll need to implement `Release` twice:
> 

Only if you need to support both for `Foo`, right? You can impl only one
if you only need one.

Also you can do:

    impl<P: ForeignOwnable<Target=Foo> + Deref<Target=Foo>> Release<P> for Foo {
        fn release(this: P) {
	    this.deref().do_sth();
	}
    }

if you want Box and Arc case share the similar behavior, right?

>     impl Release<Box<Self>> for Foo {
>         fn release(this: Box<Self>) {
>             /* ... */
>         }
>     }
> 
>     impl Release<Arc<Self>> for Foo {
>         fn release(this: Arc<Self>) {
>             /* ... */
>         }
>     }
> 
> This also means that you can have different behavior for `Box` and
> `Arc`...

That's the point, as one of the benefits you mentioned above for the
associate type design, just extending it to the same type.

Regards,
Boqun

> 
> ---
> Cheers,
> Benno

Re: [PATCH v4 5/5] rust: devres: implement register_release()

[Benno Lossin]

On Sat Jun 28, 2025 at 8:38 AM CEST, Boqun Feng wrote:
> On Sat, Jun 28, 2025 at 08:06:52AM +0200, Benno Lossin wrote:
>> On Sat Jun 28, 2025 at 12:06 AM CEST, Boqun Feng wrote:
>> > On Fri, Jun 27, 2025 at 01:19:53AM +0200, Benno Lossin wrote:
>> >> On Thu Jun 26, 2025 at 10:48 PM CEST, Danilo Krummrich wrote:
>> >> > On Thu, Jun 26, 2025 at 01:37:22PM -0700, Boqun Feng wrote:
>> >> >> On Thu, Jun 26, 2025 at 10:00:43PM +0200, Danilo Krummrich wrote:
>> >> >> > +/// [`Devres`]-releaseable resource.
>> >> >> > +///
>> >> >> > +/// Register an object implementing this trait with [`register_release`]. Its `release`
>> >> >> > +/// function will be called once the device is being unbound.
>> >> >> > +pub trait Release {
>> >> >> > +    /// The [`ForeignOwnable`] pointer type consumed by [`register_release`].
>> >> >> > +    type Ptr: ForeignOwnable;
>> >> >> > +
>> >> >> > +    /// Called once the [`Device`] given to [`register_release`] is unbound.
>> >> >> > +    fn release(this: Self::Ptr);
>> >> >> > +}
>> >> >> > +
>> >> >> 
>> >> >> I would like to point out the limitation of this design, say you have a
>> >> >> `Foo` that can ipml `Release`, with this, I think you could only support
>> >> >> either `Arc<Foo>` or `KBox<Foo>`. You cannot support both as the input
>> >> >> for `register_release()`. Maybe we want:
>> >> >> 
>> >> >>     pub trait Release<Ptr: ForeignOwnable> {
>> >> >>         fn release(this: Ptr);
>> >> >>     }
>> >> >
>> >> > Good catch! I think this wasn't possible without ForeignOwnable::Target.
>> >> 
>> >> Hmm do we really need that? Normally you either store a type in a shared
>> >
>> > I think it might be quite common, for example, `Foo` may be a general
>> > watchdog for a subsystem, for one driver, there might be multiple
>> > devices that could feed the dog, for another driver, there might be only
>> > one. For the first case we need Arc<Watchdog> or the second we can do
>> > Box<Watchdog>.
>> 
>> I guess then the original `&self` design is better? Not sure...
>> 
>
> This is what you said in v3:
>
> """
> and then `register_release` is:
>
>     pub fn register_release<T: Release>(dev: &Device<Bound>, data: T::Ptr) -> Result
>
> This way, one can store a `Box<T>` and get access to the `T` at the end.
> Or if they store the value in an `Arc<T>`, they have the option to clone
> it and give it to somewhere else.
> """
>
> I think that's the reason why we think the current version (the
> associate type design) is better than `&self`?

Yeah and I'd still say that that statement is true.

> The generic type design (i.e. Release<P: ForeignOwnable>) just further
> allows this "different behaviors between Box and Arc" for the same type
> T. I think it's a natural extension of the current design and provides
> some better flexibility.

I think that extension is going to end up being too verbose.

>> > What's the downside?
>> 
>> You'll need to implement `Release` twice:
>> 
>
> Only if you need to support both for `Foo`, right? You can impl only one
> if you only need one.
>
> Also you can do:
>
>     impl<P: ForeignOwnable<Target=Foo> + Deref<Target=Foo>> Release<P> for Foo {
>         fn release(this: P) {
> 	    this.deref().do_sth();
> 	}
>     }

Please no. If this is a regular pattern, then let's go back to `&self`.
You lose all benefits of the generic design if you do it like this,
because you don't know the concrete type of the foreign ownable.

> if you want Box and Arc case share the similar behavior, right?
>
>>     impl Release<Box<Self>> for Foo {
>>         fn release(this: Box<Self>) {
>>             /* ... */
>>         }
>>     }
>> 
>>     impl Release<Arc<Self>> for Foo {
>>         fn release(this: Arc<Self>) {
>>             /* ... */
>>         }
>>     }
>> 
>> This also means that you can have different behavior for `Box` and
>> `Arc`...
>
> That's the point, as one of the benefits you mentioned above for the
> associate type design, just extending it to the same type.

I'd say that's too verbose for something that's rather supposed to be
simple.

Hmm @Danilo, do you have any use-cases in mind or already done?

---
Cheers,
Benno

Re: [PATCH v4 5/5] rust: devres: implement register_release()

[Danilo Krummrich]

On Sat, Jun 28, 2025 at 09:53:06AM +0200, Benno Lossin wrote:
> Hmm @Danilo, do you have any use-cases in mind or already done?

There may be other use-cases, but the one that I could forsee is very specific:

A Registration type that carries additional reference-counted data, where the
Registration should be released exactly when the device is unbound, independent
of the lifetime of the data.

Obviously, this implies that the ForeignOwnable is an Arc.

With KBox, Release and Drop are pretty much identical, so using
devres::release() instead, is much simpler and hence what we do for all simple
class device registrations.

Besides that, the use-case described above can also be covered by Devres with
the pin-init rework, by having the Registration  embed a Devres<Inner>, which
is what irq::Registration does and I also do in the MiscDeviceRegistration
patches.

Hence, I already considered dropping this patch -- and I think we should do this
for now.

Re: [PATCH v4 5/5] rust: devres: implement register_release()

[Benno Lossin]

On Sat Jun 28, 2025 at 11:58 AM CEST, Danilo Krummrich wrote:
> On Sat, Jun 28, 2025 at 09:53:06AM +0200, Benno Lossin wrote:
>> Hmm @Danilo, do you have any use-cases in mind or already done?
>
> There may be other use-cases, but the one that I could forsee is very specific:
>
> A Registration type that carries additional reference-counted data, where the
> Registration should be released exactly when the device is unbound, independent
> of the lifetime of the data.
>
> Obviously, this implies that the ForeignOwnable is an Arc.
>
> With KBox, Release and Drop are pretty much identical, so using
> devres::release() instead, is much simpler and hence what we do for all simple
> class device registrations.
>
> Besides that, the use-case described above can also be covered by Devres with
> the pin-init rework, by having the Registration  embed a Devres<Inner>, which
> is what irq::Registration does and I also do in the MiscDeviceRegistration
> patches.
>
> Hence, I already considered dropping this patch -- and I think we should do this
> for now.

Sounds good! We can always pick it up again when needed.

---
Cheers,
Benno

Re: [PATCH v4 5/5] rust: devres: implement register_release()

[Danilo Krummrich]

On Sat, Jun 28, 2025 at 02:13:08PM +0200, Benno Lossin wrote:
> On Sat Jun 28, 2025 at 11:58 AM CEST, Danilo Krummrich wrote:
> > On Sat, Jun 28, 2025 at 09:53:06AM +0200, Benno Lossin wrote:
> >> Hmm @Danilo, do you have any use-cases in mind or already done?
> >
> > There may be other use-cases, but the one that I could forsee is very specific:
> >
> > A Registration type that carries additional reference-counted data, where the
> > Registration should be released exactly when the device is unbound, independent
> > of the lifetime of the data.
> >
> > Obviously, this implies that the ForeignOwnable is an Arc.
> >
> > With KBox, Release and Drop are pretty much identical, so using
> > devres::release() instead, is much simpler and hence what we do for all simple
> > class device registrations.
> >
> > Besides that, the use-case described above can also be covered by Devres with
> > the pin-init rework, by having the Registration  embed a Devres<Inner>, which
> > is what irq::Registration does and I also do in the MiscDeviceRegistration
> > patches.
> >
> > Hence, I already considered dropping this patch -- and I think we should do this
> > for now.
> 
> Sounds good! We can always pick it up again when needed.

Exactly -- thanks Benno and Boqun! The discussion of the design of the Release
trait seems also relevant for other use-cases with ForeignOwnable types where we
require trait bounds for their target types.

Re: [PATCH v4 0/5] Improvements for Devres

[Danilo Krummrich]

On Thu, Jun 26, 2025 at 10:00:38PM +0200, Danilo Krummrich wrote:
>   rust: revocable: support fallible PinInit types
>   rust: devres: replace Devres::new_foreign_owned()
>   rust: devres: get rid of Devres' inner Arc

Applied to driver-core-testing, thanks!

>   rust: types: ForeignOwnable: Add type Target
>   rust: devres: implement register_release()

Dropped those two for now.

Re: [PATCH v4 4/5] rust: types: ForeignOwnable: Add type Target

[Alice Ryhl]

On Thu, Jun 26, 2025 at 10:01 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> ForeignOwnable::Target defines the payload data of a ForeignOwnable. For
> Arc<T> for instance, ForeignOwnable::Target would just be T.
>
> This is useful for cases where a trait bound is required on the target
> type of the ForeignOwnable. For instance:
>
>         fn example<P>(data: P)
>            where
>               P: ForeignOwnable,
>               P::Target: MyTrait,
>         {}
>
> Suggested-by: Benno Lossin <lossin@kernel.org>
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>

I am skeptical about this patch. I think that most of the time, you
should just place the trait bound on `P` instead of having a Target
type like this.

Alice