[PATCH 0/4] Improvements for Devres

[PATCH 0/4] 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 for some great offline discussions on this topic.

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

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

Danilo Krummrich (4):
  rust: revocable: support fallible PinInit types
  rust: devres: replace Devres::new_foreign_owned()
  rust: devres: get rid of Devres' inner Arc
  rust: devres: implement register_foreign_release()

 drivers/gpu/nova-core/driver.rs |   7 +-
 drivers/gpu/nova-core/gpu.rs    |   6 +-
 rust/helpers/device.c           |   7 +
 rust/kernel/cpufreq.rs          |   8 +-
 rust/kernel/devres.rs           | 338 ++++++++++++++++++++++----------
 rust/kernel/drm/driver.rs       |  11 +-
 rust/kernel/pci.rs              |  20 +-
 rust/kernel/revocable.rs        |   7 +-
 samples/rust/rust_driver_pci.rs |  19 +-
 9 files changed, 280 insertions(+), 143 deletions(-)


base-commit: e15a5b4301ec42990448b5b023e3439315b821ce
-- 
2.49.0

[PATCH 1/4] 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.

Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 rust/kernel/devres.rs    | 2 +-
 rust/kernel/revocable.rs | 7 +++++--
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
index d8bdf2bdb879..a7df9fbd724f 100644
--- a/rust/kernel/devres.rs
+++ b/rust/kernel/devres.rs
@@ -98,7 +98,7 @@ struct DevresInner<T> {
 impl<T> 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..41b8fe374af6 100644
--- a/rust/kernel/revocable.rs
+++ b/rust/kernel/revocable.rs
@@ -82,8 +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, Error>
+    where
+        Error: From<E>,
+    {
+        try_pin_init!(Self {
             is_available: AtomicBool::new(true),
             data <- Opaque::pin_init(data),
         })
-- 
2.49.0

Re: [PATCH 1/4] rust: revocable: support fallible PinInit types

[Benno Lossin]

On Thu Jun 12, 2025 at 4:51 PM CEST, Danilo Krummrich wrote:
> 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.
>
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> ---
>  rust/kernel/devres.rs    | 2 +-
>  rust/kernel/revocable.rs | 7 +++++--
>  2 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
> index d8bdf2bdb879..a7df9fbd724f 100644
> --- a/rust/kernel/devres.rs
> +++ b/rust/kernel/devres.rs
> @@ -98,7 +98,7 @@ struct DevresInner<T> {
>  impl<T> 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..41b8fe374af6 100644
> --- a/rust/kernel/revocable.rs
> +++ b/rust/kernel/revocable.rs
> @@ -82,8 +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, Error>
> +    where
> +        Error: From<E>,

I don't think we need this bound as you don't use it in the function
body.

---
Cheers,
Benno

> +    {
> +        try_pin_init!(Self {
>              is_available: AtomicBool::new(true),
>              data <- Opaque::pin_init(data),
>          })

Re: [PATCH 1/4] rust: revocable: support fallible PinInit types

[Danilo Krummrich]

On Thu, Jun 12, 2025 at 05:48:36PM +0200, Benno Lossin wrote:
> On Thu Jun 12, 2025 at 4:51 PM CEST, Danilo Krummrich wrote:
> > diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
> > index fa1fd70efa27..41b8fe374af6 100644
> > --- a/rust/kernel/revocable.rs
> > +++ b/rust/kernel/revocable.rs
> > @@ -82,8 +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, Error>
> > +    where
> > +        Error: From<E>,
> 
> I don't think we need this bound as you don't use it in the function
> body.

I think it's needed by try_pin_init!() below, no?

Without it I get the compilation error in [1].

> > +    {
> > +        try_pin_init!(Self {
> >              is_available: AtomicBool::new(true),
> >              data <- Opaque::pin_init(data),
> >          })
> 

[1]

error[E0277]: `?` couldn't convert the error to `error::Error`
  --> rust/kernel/revocable.rs:87:9
   |
87 | /         try_pin_init!(Self {
88 | |             is_available: AtomicBool::new(true),
89 | |             data <- Opaque::pin_init(data),
90 | |         })
   | |          ^
   | |          |
   | |__________this can't be annotated with `?` because it has type `Result<_, E>`
   |            the trait `core::convert::From<E>` is not implemented for `error::Error`
   |
   = note: the question mark operation (`?`) implicitly performs a conversion on the error value using the `From` trait
   = note: required for `core::result::Result<revocable::Revocable<T>::new::__InitOk, error::Error>` to implement `core::ops::FromResidual<core::result::Result<core::convert::Infallible, E>>`
   = note: this error originates in the macro `$crate::__init_internal` which comes from the expansion of the macro `try_pin_init` (in Nightly builds, run with -Z macro-backtrace for more info)
help: consider introducing a `where` clause, but there might be an alternative better way to express this requirement
   |
83 | impl<T> Revocable<T> where error::Error: core::convert::From<E> {
   |                      ++++++++++++++++++++++++++++++++++++++++++

error[E0277]: the trait bound `impl PinInit<Revocable<T>, Error>: PinInit<Revocable<T>, E>` is not satisfied
  --> rust/kernel/revocable.rs:85:48
   |
85 |     pub fn new<E>(data: impl PinInit<T, E>) -> impl PinInit<Self, E>
   |                                                ^^^^^^^^^^^^^^^^^^^^^ the trait `pin_init::PinInit<revocable::Revocable<T>, E>` is not implemented for `impl pin_init::PinInit<revocable::Revocable<T>, error::Error>`
   |
   = help: the following other types implement trait `pin_init::PinInit<T, E>`:
             `core::result::Result<T, E>` implements `pin_init::PinInit<T, E>`
             `pin_init::ChainInit<I, F, T, E>` implements `pin_init::PinInit<T, E>`
             `pin_init::ChainPinInit<I, F, T, E>` implements `pin_init::PinInit<T, E>`
             `pin_init::__internal::AlwaysFail<T>` implements `pin_init::PinInit<T, ()>`
   = note: the full name for the type has been written to 'kernel.long-type-441004638990533407.txt'
   = note: consider using `--verbose` to print the full type name to the console

Re: [PATCH 1/4] rust: revocable: support fallible PinInit types

[Benno Lossin]

On Thu Jun 12, 2025 at 5:58 PM CEST, Danilo Krummrich wrote:
> On Thu, Jun 12, 2025 at 05:48:36PM +0200, Benno Lossin wrote:
>> On Thu Jun 12, 2025 at 4:51 PM CEST, Danilo Krummrich wrote:
>> > diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
>> > index fa1fd70efa27..41b8fe374af6 100644
>> > --- a/rust/kernel/revocable.rs
>> > +++ b/rust/kernel/revocable.rs
>> > @@ -82,8 +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, Error>
>> > +    where
>> > +        Error: From<E>,
>> 
>> I don't think we need this bound as you don't use it in the function
>> body.
>
> I think it's needed by try_pin_init!() below, no?
>
> Without it I get the compilation error in [1].
>
>> > +    {
>> > +        try_pin_init!(Self {
>> >              is_available: AtomicBool::new(true),
>> >              data <- Opaque::pin_init(data),
>> >          })

Does it work with this?

    try_pin_init!(Self {
        is_available: AtomicBool::new(true),
        data <- Opaque::pin_init(data),
    }? E)

---
Cheers,
Benno

>> 
>
> [1]
>
> error[E0277]: `?` couldn't convert the error to `error::Error`
>   --> rust/kernel/revocable.rs:87:9
>    |
> 87 | /         try_pin_init!(Self {
> 88 | |             is_available: AtomicBool::new(true),
> 89 | |             data <- Opaque::pin_init(data),
> 90 | |         })
>    | |          ^
>    | |          |
>    | |__________this can't be annotated with `?` because it has type `Result<_, E>`
>    |            the trait `core::convert::From<E>` is not implemented for `error::Error`
>    |
>    = note: the question mark operation (`?`) implicitly performs a conversion on the error value using the `From` trait
>    = note: required for `core::result::Result<revocable::Revocable<T>::new::__InitOk, error::Error>` to implement `core::ops::FromResidual<core::result::Result<core::convert::Infallible, E>>`
>    = note: this error originates in the macro `$crate::__init_internal` which comes from the expansion of the macro `try_pin_init` (in Nightly builds, run with -Z macro-backtrace for more info)
> help: consider introducing a `where` clause, but there might be an alternative better way to express this requirement
>    |
> 83 | impl<T> Revocable<T> where error::Error: core::convert::From<E> {
>    |                      ++++++++++++++++++++++++++++++++++++++++++
>
> error[E0277]: the trait bound `impl PinInit<Revocable<T>, Error>: PinInit<Revocable<T>, E>` is not satisfied
>   --> rust/kernel/revocable.rs:85:48
>    |
> 85 |     pub fn new<E>(data: impl PinInit<T, E>) -> impl PinInit<Self, E>
>    |                                                ^^^^^^^^^^^^^^^^^^^^^ the trait `pin_init::PinInit<revocable::Revocable<T>, E>` is not implemented for `impl pin_init::PinInit<revocable::Revocable<T>, error::Error>`
>    |
>    = help: the following other types implement trait `pin_init::PinInit<T, E>`:
>              `core::result::Result<T, E>` implements `pin_init::PinInit<T, E>`
>              `pin_init::ChainInit<I, F, T, E>` implements `pin_init::PinInit<T, E>`
>              `pin_init::ChainPinInit<I, F, T, E>` implements `pin_init::PinInit<T, E>`
>              `pin_init::__internal::AlwaysFail<T>` implements `pin_init::PinInit<T, ()>`
>    = note: the full name for the type has been written to 'kernel.long-type-441004638990533407.txt'
>    = note: consider using `--verbose` to print the full type name to the console

Re: [PATCH 1/4] rust: revocable: support fallible PinInit types

[Danilo Krummrich]

On Thu, Jun 12, 2025 at 06:17:40PM +0200, Benno Lossin wrote:
> On Thu Jun 12, 2025 at 5:58 PM CEST, Danilo Krummrich wrote:
> > On Thu, Jun 12, 2025 at 05:48:36PM +0200, Benno Lossin wrote:
> >> On Thu Jun 12, 2025 at 4:51 PM CEST, Danilo Krummrich wrote:
> >> > diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
> >> > index fa1fd70efa27..41b8fe374af6 100644
> >> > --- a/rust/kernel/revocable.rs
> >> > +++ b/rust/kernel/revocable.rs
> >> > @@ -82,8 +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, Error>
> >> > +    where
> >> > +        Error: From<E>,
> >> 
> >> I don't think we need this bound as you don't use it in the function
> >> body.
> >
> > I think it's needed by try_pin_init!() below, no?
> >
> > Without it I get the compilation error in [1].
> >
> >> > +    {
> >> > +        try_pin_init!(Self {
> >> >              is_available: AtomicBool::new(true),
> >> >              data <- Opaque::pin_init(data),
> >> >          })
> 
> Does it work with this?
> 
>     try_pin_init!(Self {
>         is_available: AtomicBool::new(true),
>         data <- Opaque::pin_init(data),
>     }? E)

Yes, it does -- thanks!

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

[Danilo Krummrich]

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

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_foreign_boxed(), 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>
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 rust/helpers/device.c     |  7 ++++
 rust/kernel/cpufreq.rs    |  8 ++---
 rust/kernel/devres.rs     | 73 +++++++++++++++++++++++++++++++++------
 rust/kernel/drm/driver.rs | 11 +++---
 4 files changed, 79 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 b0a9c6182aec..f20636079f7a 100644
--- a/rust/kernel/cpufreq.rs
+++ b/rust/kernel/cpufreq.rs
@@ -12,7 +12,7 @@
     clk::Hertz,
     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::*,
@@ -910,7 +910,7 @@ unsafe impl<T: Driver> Sync for Registration<T> {}
 /// thread.
 unsafe impl<T: Driver> Send for Registration<T> {}
 
-impl<T: Driver> Registration<T> {
+impl<T: Driver + 'static> Registration<T> {
     const VTABLE: bindings::cpufreq_driver = bindings::cpufreq_driver {
         name: Self::copy_name(T::NAME),
         boost_enabled: T::BOOST_ENABLED,
@@ -1044,10 +1044,10 @@ 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 [`kernel::devres`] 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)
+        devres::register_foreign_boxed(dev, Self::new()?, GFP_KERNEL)
     }
 }
 
diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
index a7df9fbd724f..04435e810249 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]
@@ -182,14 +182,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
@@ -259,3 +251,64 @@ fn drop(&mut self) {
         }
     }
 }
+
+/// Consume `data` and [`Drop::drop`] `data` once `dev` is unbound.
+fn register_foreign<P: ForeignOwnable>(dev: &Device<Bound>, data: P) -> Result {
+    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.
+        let _ = 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};
+///
+/// struct Registration;
+///
+/// impl Registration {
+///     fn new() -> Self {
+///         // register (e.g. class device, IRQ, etc.)
+///
+///         Self
+///     }
+/// }
+///
+/// impl Drop for Registration {
+///     fn drop(&mut self) {
+///        // unregister
+///     }
+/// }
+///
+/// fn from_bound_context(dev: &Device<Bound>) -> Result {
+///     devres::register_foreign_boxed(dev, Registration::new(), GFP_KERNEL)
+/// }
+/// ```
+pub fn register_foreign_boxed<T, E>(
+    dev: &Device<Bound>,
+    data: impl PinInit<T, E>,
+    flags: Flags,
+) -> Result
+where
+    T: '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..3b0cb80c1984 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,
@@ -120,7 +118,7 @@ pub trait Driver {
 /// Once the `Registration` structure is dropped, the device is unregistered.
 pub struct Registration<T: Driver>(ARef<drm::Device<T>>);
 
-impl<T: Driver> Registration<T> {
+impl<T: Driver + 'static> Registration<T> {
     /// Creates a new [`Registration`] and registers it.
     fn new(drm: &drm::Device<T>, flags: usize) -> Result<Self> {
         // SAFETY: `drm.as_raw()` is valid by the invariants of `drm::Device`.
@@ -130,7 +128,7 @@ fn new(drm: &drm::Device<T>, flags: usize) -> Result<Self> {
     }
 
     /// Same as [`Registration::new`}, but transfers ownership of the [`Registration`] to
-    /// [`Devres`].
+    /// [`devres::register_foreign_boxed`].
     pub fn new_foreign_owned(
         drm: &drm::Device<T>,
         dev: &device::Device<device::Bound>,
@@ -141,7 +139,8 @@ pub fn new_foreign_owned(
         }
 
         let reg = Registration::<T>::new(drm, flags)?;
-        Devres::new_foreign_owned(dev, reg, GFP_KERNEL)
+
+        devres::register_foreign_boxed(dev, reg, GFP_KERNEL)
     }
 
     /// Returns a reference to the `Device` instance for this registration.
-- 
2.49.0

Re: [PATCH 2/4] rust: devres: replace Devres::new_foreign_owned()

[Viresh Kumar]

On 12-06-25, 16:51, Danilo Krummrich wrote:
> Replace Devres::new_foreign_owned() with
> devres::register_foreign_boxed().
> 
> 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_foreign_boxed(), 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>
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> ---
>  rust/kernel/cpufreq.rs    |  8 ++---

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

Re: [PATCH 2/4] rust: devres: replace Devres::new_foreign_owned()

[Benno Lossin]

On Thu Jun 12, 2025 at 4:51 PM CEST, Danilo Krummrich wrote:
> diff --git a/rust/kernel/cpufreq.rs b/rust/kernel/cpufreq.rs
> index b0a9c6182aec..f20636079f7a 100644
> --- a/rust/kernel/cpufreq.rs
> +++ b/rust/kernel/cpufreq.rs
> @@ -12,7 +12,7 @@
>      clk::Hertz,
>      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::*,
> @@ -910,7 +910,7 @@ unsafe impl<T: Driver> Sync for Registration<T> {}
>  /// thread.
>  unsafe impl<T: Driver> Send for Registration<T> {}
>  
> -impl<T: Driver> Registration<T> {
> +impl<T: Driver + 'static> Registration<T> {

This change should probably be its own patch? If not, then it should be
mentioned in the commit message.

>      const VTABLE: bindings::cpufreq_driver = bindings::cpufreq_driver {
>          name: Self::copy_name(T::NAME),
>          boost_enabled: T::BOOST_ENABLED,
> @@ -1044,10 +1044,10 @@ 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 [`kernel::devres`] and will be dropped, once the
>      /// device is detached.
>      pub fn new_foreign_owned(dev: &Device<Bound>) -> Result {

I think we can improve the names here. How about `new_attached`? See
more below.

> -        Devres::new_foreign_owned(dev, Self::new()?, GFP_KERNEL)
> +        devres::register_foreign_boxed(dev, Self::new()?, GFP_KERNEL)
>      }
>  }

> +/// Encapsulate `data` in a [`KBox`] and [`Drop::drop`] `data` once `dev` is unbound.
> +///
> +/// # Examples
> +///
> +/// ```no_run
> +/// use kernel::{device::{Bound, Device}, devres};
> +///
> +/// struct Registration;
> +///
> +/// impl Registration {
> +///     fn new() -> Self {
> +///         // register (e.g. class device, IRQ, etc.)
> +///
> +///         Self
> +///     }
> +/// }
> +///
> +/// impl Drop for Registration {
> +///     fn drop(&mut self) {
> +///        // unregister
> +///     }
> +/// }
> +///
> +/// fn from_bound_context(dev: &Device<Bound>) -> Result {
> +///     devres::register_foreign_boxed(dev, Registration::new(), GFP_KERNEL)
> +/// }
> +/// ```
> +pub fn register_foreign_boxed<T, E>(

I don't really get the name of this function. The data isn't really
foreign and that the user also shouldn't really care about the fact that
you use `KBox` under the hood.

How about we call this something like `attach_data`? 

---
Cheers,
Benno

> +    dev: &Device<Bound>,
> +    data: impl PinInit<T, E>,
> +    flags: Flags,
> +) -> Result
> +where
> +    T: 'static,
> +    Error: From<E>,
> +{
> +    let data = KBox::pin_init(data, flags)?;
> +
> +    register_foreign(dev, data)
> +}

Re: [PATCH 2/4] rust: devres: replace Devres::new_foreign_owned()

[Danilo Krummrich]

On Sat, Jun 21, 2025 at 11:10:14PM +0200, Benno Lossin wrote:
> On Thu Jun 12, 2025 at 4:51 PM CEST, Danilo Krummrich wrote:
> > diff --git a/rust/kernel/cpufreq.rs b/rust/kernel/cpufreq.rs
> > index b0a9c6182aec..f20636079f7a 100644
> > --- a/rust/kernel/cpufreq.rs
> > +++ b/rust/kernel/cpufreq.rs
> > @@ -12,7 +12,7 @@
> >      clk::Hertz,
> >      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::*,
> > @@ -910,7 +910,7 @@ unsafe impl<T: Driver> Sync for Registration<T> {}
> >  /// thread.
> >  unsafe impl<T: Driver> Send for Registration<T> {}
> >  
> > -impl<T: Driver> Registration<T> {
> > +impl<T: Driver + 'static> Registration<T> {
> 
> This change should probably be its own patch? If not, then it should be
> mentioned in the commit message.

It's a consequence of register_foreign_boxed() requiring T: 'static.

> >      const VTABLE: bindings::cpufreq_driver = bindings::cpufreq_driver {
> >          name: Self::copy_name(T::NAME),
> >          boost_enabled: T::BOOST_ENABLED,
> > @@ -1044,10 +1044,10 @@ 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 [`kernel::devres`] and will be dropped, once the
> >      /// device is detached.
> >      pub fn new_foreign_owned(dev: &Device<Bound>) -> Result {
> 
> I think we can improve the names here. How about `new_attached`? See
> more below.

I feel like the name pretty much nails it: it's a new instance that is not
owned, by the Rust side, but by the C devres implementation (i.e. foreign
owned), which automatically drops it when the device is unbound.

Maybe Registration::new_devres_owned() instead?

> > -        Devres::new_foreign_owned(dev, Self::new()?, GFP_KERNEL)
> > +        devres::register_foreign_boxed(dev, Self::new()?, GFP_KERNEL)
> >      }
> >  }
> 
> > +/// Encapsulate `data` in a [`KBox`] and [`Drop::drop`] `data` once `dev` is unbound.
> > +///
> > +/// # Examples
> > +///
> > +/// ```no_run
> > +/// use kernel::{device::{Bound, Device}, devres};
> > +///
> > +/// struct Registration;
> > +///
> > +/// impl Registration {
> > +///     fn new() -> Self {
> > +///         // register (e.g. class device, IRQ, etc.)
> > +///
> > +///         Self
> > +///     }
> > +/// }
> > +///
> > +/// impl Drop for Registration {
> > +///     fn drop(&mut self) {
> > +///        // unregister
> > +///     }
> > +/// }
> > +///
> > +/// fn from_bound_context(dev: &Device<Bound>) -> Result {
> > +///     devres::register_foreign_boxed(dev, Registration::new(), GFP_KERNEL)
> > +/// }
> > +/// ```
> > +pub fn register_foreign_boxed<T, E>(
> 
> I don't really get the name of this function. The data isn't really
> foreign and that the user also shouldn't really care about the fact that
> you use `KBox` under the hood.
> 
> How about we call this something like `attach_data`?

Hm, I think attach_data() doesn't quite hit the point. Maybe just
devres::register_owned() instead. I agree that 'boxed' is an unnecessary
implementation detail.

> ---
> Cheers,
> Benno
> 
> > +    dev: &Device<Bound>,
> > +    data: impl PinInit<T, E>,
> > +    flags: Flags,
> > +) -> Result
> > +where
> > +    T: 'static,
> > +    Error: From<E>,
> > +{
> > +    let data = KBox::pin_init(data, flags)?;
> > +
> > +    register_foreign(dev, data)
> > +}

Re: [PATCH 2/4] rust: devres: replace Devres::new_foreign_owned()

[Benno Lossin]

On Sat Jun 21, 2025 at 11:45 PM CEST, Danilo Krummrich wrote:
> On Sat, Jun 21, 2025 at 11:10:14PM +0200, Benno Lossin wrote:
>> On Thu Jun 12, 2025 at 4:51 PM CEST, Danilo Krummrich wrote:
>> > diff --git a/rust/kernel/cpufreq.rs b/rust/kernel/cpufreq.rs
>> > index b0a9c6182aec..f20636079f7a 100644
>> > --- a/rust/kernel/cpufreq.rs
>> > +++ b/rust/kernel/cpufreq.rs
>> > @@ -12,7 +12,7 @@
>> >      clk::Hertz,
>> >      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::*,
>> > @@ -910,7 +910,7 @@ unsafe impl<T: Driver> Sync for Registration<T> {}
>> >  /// thread.
>> >  unsafe impl<T: Driver> Send for Registration<T> {}
>> >  
>> > -impl<T: Driver> Registration<T> {
>> > +impl<T: Driver + 'static> Registration<T> {
>> 
>> This change should probably be its own patch? If not, then it should be
>> mentioned in the commit message.
>
> It's a consequence of register_foreign_boxed() requiring T: 'static.

Then let's put the bound on that function, since putting it on the impl
block also affects `Registration::new()`.

>> >      const VTABLE: bindings::cpufreq_driver = bindings::cpufreq_driver {
>> >          name: Self::copy_name(T::NAME),
>> >          boost_enabled: T::BOOST_ENABLED,
>> > @@ -1044,10 +1044,10 @@ 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 [`kernel::devres`] and will be dropped, once the
>> >      /// device is detached.
>> >      pub fn new_foreign_owned(dev: &Device<Bound>) -> Result {
>> 
>> I think we can improve the names here. How about `new_attached`? See
>> more below.
>
> I feel like the name pretty much nails it: it's a new instance that is not
> owned, by the Rust side, but by the C devres implementation (i.e. foreign
> owned), which automatically drops it when the device is unbound.

Yeah, but `foreign` is so unspecific... With `ForeignOwnable`, it makes
sense, since it could be anything.

> Maybe Registration::new_devres_owned() instead?

I like that one better, let's go with that.

>> > -        Devres::new_foreign_owned(dev, Self::new()?, GFP_KERNEL)
>> > +        devres::register_foreign_boxed(dev, Self::new()?, GFP_KERNEL)
>> >      }
>> >  }
>> 
>> > +/// Encapsulate `data` in a [`KBox`] and [`Drop::drop`] `data` once `dev` is unbound.
>> > +///
>> > +/// # Examples
>> > +///
>> > +/// ```no_run
>> > +/// use kernel::{device::{Bound, Device}, devres};
>> > +///
>> > +/// struct Registration;
>> > +///
>> > +/// impl Registration {
>> > +///     fn new() -> Self {
>> > +///         // register (e.g. class device, IRQ, etc.)
>> > +///
>> > +///         Self
>> > +///     }
>> > +/// }
>> > +///
>> > +/// impl Drop for Registration {
>> > +///     fn drop(&mut self) {
>> > +///        // unregister
>> > +///     }
>> > +/// }
>> > +///
>> > +/// fn from_bound_context(dev: &Device<Bound>) -> Result {
>> > +///     devres::register_foreign_boxed(dev, Registration::new(), GFP_KERNEL)
>> > +/// }
>> > +/// ```
>> > +pub fn register_foreign_boxed<T, E>(
>> 
>> I don't really get the name of this function. The data isn't really
>> foreign and that the user also shouldn't really care about the fact that
>> you use `KBox` under the hood.
>> 
>> How about we call this something like `attach_data`?
>
> Hm, I think attach_data() doesn't quite hit the point. Maybe just
> devres::register_owned() instead. I agree that 'boxed' is an unnecessary
> implementation detail.

I like `register_owned` better, but I'm not 100% convinced by the
`owned` part... The regular devres creation is called `Devres::new`,
right? How about we just call this `register`?

---
Cheers,
Benno

Re: [PATCH 2/4] rust: devres: replace Devres::new_foreign_owned()

[Danilo Krummrich]

On Sun, Jun 22, 2025 at 09:42:03AM +0200, Benno Lossin wrote:
> On Sat Jun 21, 2025 at 11:45 PM CEST, Danilo Krummrich wrote:
> > I feel like the name pretty much nails it: it's a new instance that is not
> > owned, by the Rust side, but by the C devres implementation (i.e. foreign
> > owned), which automatically drops it when the device is unbound.
> 
> Yeah, but `foreign` is so unspecific... With `ForeignOwnable`, it makes
> sense, since it could be anything.
> 
> > Maybe Registration::new_devres_owned() instead?
> 
> I like that one better, let's go with that.

SGTM, but please note that this is unrelated to this patch; will create an issue
for renaming those.

> > Hm, I think attach_data() doesn't quite hit the point. Maybe just
> > devres::register_owned() instead. I agree that 'boxed' is an unnecessary
> > implementation detail.
> 
> I like `register_owned` better, but I'm not 100% convinced by the
> `owned` part... The regular devres creation is called `Devres::new`,
> right? How about we just call this `register`?

In general, devres::register() is fine for me too. But note that it looses a bit
the indicator that the ownership of the object is entirely transferred to
devres, in contrast to the Devres container type.

Re: [PATCH 2/4] rust: devres: replace Devres::new_foreign_owned()

[Benno Lossin]

On Sun Jun 22, 2025 at 11:55 AM CEST, Danilo Krummrich wrote:
> On Sun, Jun 22, 2025 at 09:42:03AM +0200, Benno Lossin wrote:
>> On Sat Jun 21, 2025 at 11:45 PM CEST, Danilo Krummrich wrote:
>> > I feel like the name pretty much nails it: it's a new instance that is not
>> > owned, by the Rust side, but by the C devres implementation (i.e. foreign
>> > owned), which automatically drops it when the device is unbound.
>> 
>> Yeah, but `foreign` is so unspecific... With `ForeignOwnable`, it makes
>> sense, since it could be anything.
>> 
>> > Maybe Registration::new_devres_owned() instead?
>> 
>> I like that one better, let's go with that.
>
> SGTM, but please note that this is unrelated to this patch; will create an issue
> for renaming those.

SGTM.

>> > Hm, I think attach_data() doesn't quite hit the point. Maybe just
>> > devres::register_owned() instead. I agree that 'boxed' is an unnecessary
>> > implementation detail.
>> 
>> I like `register_owned` better, but I'm not 100% convinced by the
>> `owned` part... The regular devres creation is called `Devres::new`,
>> right? How about we just call this `register`?
>
> In general, devres::register() is fine for me too. But note that it looses a bit
> the indicator that the ownership of the object is entirely transferred to
> devres, in contrast to the Devres container type.

I'd say that is clear from the function signature & can be expanded upon
in the docs. `register_owned` doesn't really carry the meaning "I take
ownership of what you give me", so I don't think we lose anything here.

(if we have a `register_owned`, then it begs the question what
`register` would be... which doesn't make sense to exist IMO)

---
Cheers,
Benno

[PATCH 3/4] 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           | 187 +++++++++++++++-----------------
 rust/kernel/pci.rs              |  20 ++--
 samples/rust/rust_driver_pci.rs |  19 ++--
 5 files changed, 117 insertions(+), 122 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 04435e810249..4ee9037f4ad4 100644
--- a/rust/kernel/devres.rs
+++ b/rust/kernel/devres.rs
@@ -13,20 +13,10 @@
     ffi::c_void,
     prelude::*,
     revocable::{Revocable, RevocableGuard},
-    sync::{rcu, Arc, Completion},
+    sync::{rcu, Completion},
     types::{ARef, ForeignOwnable},
 };
 
-#[pin_data]
-struct DevresInner<T> {
-    dev: ARef<Device>,
-    callback: unsafe extern "C" fn(*mut c_void),
-    #[pin]
-    data: Revocable<T>,
-    #[pin]
-    revoke: Completion,
-}
-
 /// This abstraction is meant to be used by subsystems to containerize [`Device`] bound resources to
 /// manage their lifetime.
 ///
@@ -86,100 +76,93 @@ struct DevresInner<T> {
 /// # 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>(Arc<DevresInner<T>>);
-
-impl<T> 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,
-                data <- Revocable::new(data),
-                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();
+#[pin_data(PinnedDrop)]
+pub struct Devres<T> {
+    dev: ARef<Device>,
+    callback: unsafe extern "C" fn(*mut c_void),
+    #[pin]
+    data: Revocable<T>,
+    #[pin]
+    devm: Completion,
+    #[pin]
+    revoke: Completion,
+}
 
-        // 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 _) };
+impl<T> 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<'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;
 
-        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));
-        }
+        try_pin_init!(&this in Self {
+            data <- Revocable::new(data),
+            devm <- Completion::new(),
+            revoke <- Completion::new(),
+            callback,
+            dev: {
+                // SAFETY:
+                // - `dev.as_raw()` is a pointer to a valid bound device.
+                // - `this` 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), this.as_ptr().cast())
+                })?;
 
-        Ok(inner)
+                dev.into()
+            },
+        })
     }
 
     fn as_ptr(&self) -> *const Self {
-        self as _
-    }
-
-    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
+        self
     }
 
     #[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 `Self` to `devm_add_action()`,
+        // hence `ptr` must be a valid pointer to `Self`.
+        let this = unsafe { &*ptr.cast::<Self>() };
 
-        if !inner.data.revoke() {
+        if !this.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`.
-            inner.revoke.wait_for_completion();
+            // `data` for us. Hence we have to wait until `Devres::drop` signals that it
+            // completed revoking `data`.
+            this.revoke.wait_for_completion();
         }
-    }
-}
 
-impl<T> 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)?;
+        // Signal that we're done using `this`.
+        this.devm.complete_all();
+    }
 
-        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,
+        // - `self` is always valid, even if the action has been released already.
+        (unsafe {
+            bindings::devm_remove_action_nowarn(
+                self.dev.as_raw(),
+                Some(self.callback),
+                self.as_ptr().cast_mut().cast(),
+            )
+        } == 0)
     }
 
     /// Obtain `&'a T`, bypassing the [`Revocable`].
@@ -211,43 +194,51 @@ 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> Drop for Devres<T> {
-    fn drop(&mut self) {
+#[pinned_drop]
+impl<T> 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.revoke.complete_all();
+
+                // Wait for `Self::devres_callback` to be done using this object.
+                self.devm.wait_for_completion();
             }
+        } else {
+            // `Self::devres_callback` revokes `self.data` for us, hence wait for it to be done
+            // using this object.
+            self.devm.wait_for_completion();
         }
     }
 }
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

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

[Benno Lossin]

On Thu Jun 12, 2025 at 4:51 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>

This is really nice, good to see the extra allocations gone :)

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

> @@ -86,100 +76,93 @@ struct DevresInner<T> {
>  /// # 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>(Arc<DevresInner<T>>);
> -
> -impl<T> 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,
> -                data <- Revocable::new(data),
> -                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();
> +#[pin_data(PinnedDrop)]
> +pub struct Devres<T> {
> +    dev: ARef<Device>,
> +    callback: unsafe extern "C" fn(*mut c_void),

Do I remember correctly that we at some point talked about adding a
comment here for why this is needed? (ie it's needed, because
`Self::callback` might return different addresses?)

> +    #[pin]
> +    data: Revocable<T>,
> +    #[pin]
> +    devm: Completion,
> +    #[pin]
> +    revoke: Completion,

Probably a good idea to add some doc comments explaining what these two
completions track.

(feel free to do these in another patch or in a follow-up)

> +}
>  
> -        // 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 _) };
> +impl<T> Devres<T> {
> +    /// Creates a new [`Devres`] instance of the given `data`. The `data` encapsulated within the

Missing double newline after the first sentence.

> +    /// returned `Devres` instance' `data` will be revoked once the device is detached.

Maybe we should link to `Revocable` on the word `revoked`?

> +    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;

> -        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,
> +        // - `self` is always valid, even if the action has been released already.
> +        (unsafe {
> +            bindings::devm_remove_action_nowarn(
> +                self.dev.as_raw(),
> +                Some(self.callback),
> +                self.as_ptr().cast_mut().cast(),
> +            )
> +        } == 0)

I don't think the parenthesis are required?

>      }
>  
>      /// Obtain `&'a T`, bypassing the [`Revocable`].

> -impl<T> Drop for Devres<T> {
> -    fn drop(&mut self) {
> +#[pinned_drop]
> +impl<T> 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.revoke.complete_all();
> +
> +                // Wait for `Self::devres_callback` to be done using this object.
> +                self.devm.wait_for_completion();
>              }
> +        } else {
> +            // `Self::devres_callback` revokes `self.data` for us, hence wait for it to be done
> +            // using this object.
> +            self.devm.wait_for_completion();

I don't understand this change, maybe it's best to move that into a
separate commit?

---
Cheers,
Benno

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

[Danilo Krummrich]

On Sun, Jun 22, 2025 at 09:05:51AM +0200, Benno Lossin wrote:
> On Thu Jun 12, 2025 at 4:51 PM CEST, Danilo Krummrich wrote:
> > +#[pin_data(PinnedDrop)]
> > +pub struct Devres<T> {
> > +    dev: ARef<Device>,
> > +    callback: unsafe extern "C" fn(*mut c_void),
> 
> Do I remember correctly that we at some point talked about adding a
> comment here for why this is needed? (ie it's needed, because
> `Self::callback` might return different addresses?)

Correct -- thanks for reminding me of that. Will add the corresponding comment.

> > +    #[pin]
> > +    data: Revocable<T>,
> > +    #[pin]
> > +    devm: Completion,
> > +    #[pin]
> > +    revoke: Completion,
> 
> Probably a good idea to add some doc comments explaining what these two
> completions track.
> 
> (feel free to do these in another patch or in a follow-up)

No, I think it'd be good to do it right away -- will add them.

> > +#[pinned_drop]
> > +impl<T> 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.revoke.complete_all();
> > +
> > +                // Wait for `Self::devres_callback` to be done using this object.
> > +                self.devm.wait_for_completion();
> >              }
> > +        } else {
> > +            // `Self::devres_callback` revokes `self.data` for us, hence wait for it to be done
> > +            // using this object.
> > +            self.devm.wait_for_completion();
> 
> I don't understand this change, maybe it's best to move that into a
> separate commit?

We can't do that, without this change the code would be incorrect.

What happens here is that, if drop() races with devres_callback() we have to
make drop() wait until devres_callback() is completed, because otherwise
devres_callback() might experience a use-after-free.

Previoulsly this has been taken care of by Arc<DevresInner>, which C devres held
a reference of.

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

[Benno Lossin]

On Sun Jun 22, 2025 at 2:08 PM CEST, Danilo Krummrich wrote:
> On Sun, Jun 22, 2025 at 09:05:51AM +0200, Benno Lossin wrote:
>> On Thu Jun 12, 2025 at 4:51 PM CEST, Danilo Krummrich wrote:
>> > +#[pinned_drop]
>> > +impl<T> 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.revoke.complete_all();
>> > +
>> > +                // Wait for `Self::devres_callback` to be done using this object.
>> > +                self.devm.wait_for_completion();
>> >              }
>> > +        } else {
>> > +            // `Self::devres_callback` revokes `self.data` for us, hence wait for it to be done
>> > +            // using this object.
>> > +            self.devm.wait_for_completion();
>> 
>> I don't understand this change, maybe it's best to move that into a
>> separate commit?
>
> We can't do that, without this change the code would be incorrect.
>
> What happens here is that, if drop() races with devres_callback() we have to
> make drop() wait until devres_callback() is completed, because otherwise
> devres_callback() might experience a use-after-free.
>
> Previoulsly this has been taken care of by Arc<DevresInner>, which C devres held
> a reference of.

Yeah I understand it now, the diff was adding too much noise and looking
at it directly was helpful :)

Theoretically, you could add it in a commit before removing the Arc, but
probably not worth it.

---
Cheers,
Benno

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

[Danilo Krummrich]

On Sun, Jun 22, 2025 at 09:05:51AM +0200, Benno Lossin wrote:
> On Thu Jun 12, 2025 at 4:51 PM CEST, Danilo Krummrich wrote:
> > +    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,
> > +        // - `self` is always valid, even if the action has been released already.
> > +        (unsafe {
> > +            bindings::devm_remove_action_nowarn(
> > +                self.dev.as_raw(),
> > +                Some(self.callback),
> > +                self.as_ptr().cast_mut().cast(),
> > +            )
> > +        } == 0)
> 
> I don't think the parenthesis are required?

At least the compiler doesn't seem to be happy about removing them:

error: expected expression, found `==`
   --> rust/kernel/devres.rs:199:11
    |
199 |         } == 0
    |           ^^ expected expression

error[E0308]: mismatched types
   --> rust/kernel/devres.rs:194:13
    |
194 | /             bindings::devm_remove_action_nowarn(
195 | |                 self.dev.as_raw(),
196 | |                 Some(self.callback),
197 | |                 self.inner().as_ptr().cast_mut().cast(),
198 | |             )
    | |             ^- help: consider using a semicolon here: `;`
    | |_____________|
    |               expected `()`, found `i32`

error: aborting due to 2 previous errors

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

[Benno Lossin]

On Sun Jun 22, 2025 at 5:45 PM CEST, Danilo Krummrich wrote:
> On Sun, Jun 22, 2025 at 09:05:51AM +0200, Benno Lossin wrote:
>> On Thu Jun 12, 2025 at 4:51 PM CEST, Danilo Krummrich wrote:
>> > +    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,
>> > +        // - `self` is always valid, even if the action has been released already.
>> > +        (unsafe {
>> > +            bindings::devm_remove_action_nowarn(
>> > +                self.dev.as_raw(),
>> > +                Some(self.callback),
>> > +                self.as_ptr().cast_mut().cast(),
>> > +            )
>> > +        } == 0)
>> 
>> I don't think the parenthesis are required?
>
> At least the compiler doesn't seem to be happy about removing them:
>
> error: expected expression, found `==`
>    --> rust/kernel/devres.rs:199:11
>     |
> 199 |         } == 0
>     |           ^^ expected expression
>
> error[E0308]: mismatched types
>    --> rust/kernel/devres.rs:194:13
>     |
> 194 | /             bindings::devm_remove_action_nowarn(
> 195 | |                 self.dev.as_raw(),
> 196 | |                 Some(self.callback),
> 197 | |                 self.inner().as_ptr().cast_mut().cast(),
> 198 | |             )
>     | |             ^- help: consider using a semicolon here: `;`
>     | |_____________|
>     |               expected `()`, found `i32`
>
> error: aborting due to 2 previous errors

Huh... Do you like this better?:

    let res = unsafe {
        bindings::devm_remove_action_nowarn(
            /*
             * ...
             */
        )
    };
    res == 0

Maybe it's more readable, but I'm not sure what is more idiomatic in
this case.

---
Cheers,
Benno

[PATCH 4/4] rust: devres: implement register_foreign_release()

[Danilo Krummrich]

register_foreign_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_foreign_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 | 80 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 80 insertions(+)

diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
index 4ee9037f4ad4..495dca6240fc 100644
--- a/rust/kernel/devres.rs
+++ b/rust/kernel/devres.rs
@@ -16,6 +16,7 @@
     sync::{rcu, Completion},
     types::{ARef, ForeignOwnable},
 };
+use core::ops::Deref;
 
 /// This abstraction is meant to be used by subsystems to containerize [`Device`] bound resources to
 /// manage their lifetime.
@@ -303,3 +304,82 @@ pub fn register_foreign_boxed<T, E>(
 
     register_foreign(dev, data)
 }
+
+/// To be implemented by an object passed to [`register_foreign_release`].
+pub trait Release {
+    /// Called once the [`Device`] given to [`register_foreign_release`] is unbound.
+    fn release(&self);
+}
+
+impl<T: Release> Release for crate::sync::ArcBorrow<'_, T> {
+    fn release(&self) {
+        self.deref().release();
+    }
+}
+
+impl<T: Release> Release for Pin<&'_ T> {
+    fn release(&self) {
+        self.deref().release();
+    }
+}
+
+/// 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};
+///
+/// struct Registration<T> {
+///     data: T,
+/// }
+///
+/// impl<T> Registration<T> {
+///     fn new(data: T) -> Result<Arc<Self>> {
+///         // register (e.g. class device, IRQ, etc.)
+///
+///         Ok(Arc::new(Self { data }, GFP_KERNEL)?)
+///     }
+/// }
+///
+/// impl<T> Release for Registration<T> {
+///     fn release(&self) {
+///        // unregister
+///     }
+/// }
+///
+/// fn from_bound_context(dev: &Device<Bound>) -> Result {
+///     let reg = Registration::new(0x42)?;
+///
+///     devres::register_foreign_release(dev, reg.clone())
+/// }
+/// ```
+pub fn register_foreign_release<P>(dev: &Device<Bound>, data: P) -> Result
+where
+    P: ForeignOwnable,
+    for<'a> P::Borrowed<'a>: Release,
+{
+    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,
+        for<'a> P::Borrowed<'a>: Release,
+    {
+        // SAFETY: `ptr` is the pointer to the `ForeignOwnable` leaked above and hence valid.
+        unsafe { P::borrow(ptr.cast()) }.release();
+
+        // SAFETY: `ptr` is the pointer to the `ForeignOwnable` leaked above and hence valid.
+        let _ = 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())
+    })
+}
-- 
2.49.0

Re: [PATCH 4/4] rust: devres: implement register_foreign_release()

[Benno Lossin]

On Thu Jun 12, 2025 at 4:51 PM CEST, Danilo Krummrich wrote:
> register_foreign_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_foreign_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 | 80 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 80 insertions(+)

Idea & implementation look good, I'm just a little unsatisfied with the
names & docs...

> diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
> index 4ee9037f4ad4..495dca6240fc 100644
> --- a/rust/kernel/devres.rs
> +++ b/rust/kernel/devres.rs
> @@ -16,6 +16,7 @@
>      sync::{rcu, Completion},
>      types::{ARef, ForeignOwnable},
>  };
> +use core::ops::Deref;
>  
>  /// This abstraction is meant to be used by subsystems to containerize [`Device`] bound resources to
>  /// manage their lifetime.
> @@ -303,3 +304,82 @@ pub fn register_foreign_boxed<T, E>(
>  
>      register_foreign(dev, data)
>  }
> +
> +/// To be implemented by an object passed to [`register_foreign_release`].

    /// [`Devres`]-releaseable resource.
    ///
    /// Register an object implementing this trait with [`register_foreign_release`]. It's `release`
    /// function will be called once the device unbinds.

> +pub trait Release {
> +    /// Called once the [`Device`] given to [`register_foreign_release`] is unbound.
> +    fn release(&self);

Would it make sense to also supply the `Device` that this is attached
to? In case you have one object in multiple `register_foreign_release`
calls with different devices, or is that something that doesn't happen?

> +}
> +
> +impl<T: Release> Release for crate::sync::ArcBorrow<'_, T> {
> +    fn release(&self) {
> +        self.deref().release();
> +    }
> +}
> +
> +impl<T: Release> Release for Pin<&'_ T> {
> +    fn release(&self) {
> +        self.deref().release();
> +    }
> +}

We should also implement it for `&T`, since that is `Box`'s `Borrowed`.

> +
> +/// 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};
> +///
> +/// struct Registration<T> {

Maybe add some explanation above/below this example. It looks like a new
bus registration?

---
Cheers,
Benno

> +///     data: T,
> +/// }
> +///
> +/// impl<T> Registration<T> {
> +///     fn new(data: T) -> Result<Arc<Self>> {
> +///         // register (e.g. class device, IRQ, etc.)
> +///
> +///         Ok(Arc::new(Self { data }, GFP_KERNEL)?)
> +///     }
> +/// }
> +///
> +/// impl<T> Release for Registration<T> {
> +///     fn release(&self) {
> +///        // unregister
> +///     }
> +/// }
> +///
> +/// fn from_bound_context(dev: &Device<Bound>) -> Result {
> +///     let reg = Registration::new(0x42)?;
> +///
> +///     devres::register_foreign_release(dev, reg.clone())
> +/// }
> +/// ```
> +pub fn register_foreign_release<P>(dev: &Device<Bound>, data: P) -> Result
> +where
> +    P: ForeignOwnable,
> +    for<'a> P::Borrowed<'a>: Release,
> +{
> +    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,
> +        for<'a> P::Borrowed<'a>: Release,
> +    {
> +        // SAFETY: `ptr` is the pointer to the `ForeignOwnable` leaked above and hence valid.
> +        unsafe { P::borrow(ptr.cast()) }.release();
> +
> +        // SAFETY: `ptr` is the pointer to the `ForeignOwnable` leaked above and hence valid.
> +        let _ = 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())
> +    })
> +}

Re: [PATCH 4/4] rust: devres: implement register_foreign_release()

[Danilo Krummrich]

On Sun, Jun 22, 2025 at 09:26:33AM +0200, Benno Lossin wrote:
> On Thu Jun 12, 2025 at 4:51 PM CEST, Danilo Krummrich wrote:
> > +/// To be implemented by an object passed to [`register_foreign_release`].
> 
>     /// [`Devres`]-releaseable resource.
>     ///
>     /// Register an object implementing this trait with [`register_foreign_release`]. It's `release`
>     /// function will be called once the device unbinds.

Sounds good, thanks!

> > +pub trait Release {
> > +    /// Called once the [`Device`] given to [`register_foreign_release`] is unbound.
> > +    fn release(&self);
> 
> Would it make sense to also supply the `Device` that this is attached
> to? In case you have one object in multiple `register_foreign_release`
> calls with different devices, or is that something that doesn't happen?

No, doing that wouldn't make any sense. A resource should only be bound to the
lifetime of a single device.

> > +}
> > +
> > +impl<T: Release> Release for crate::sync::ArcBorrow<'_, T> {
> > +    fn release(&self) {
> > +        self.deref().release();
> > +    }
> > +}
> > +
> > +impl<T: Release> Release for Pin<&'_ T> {
> > +    fn release(&self) {
> > +        self.deref().release();
> > +    }
> > +}
> 
> We should also implement it for `&T`, since that is `Box`'s `Borrowed`.

That should implicitly be the case when T: Release, where T is P<T>.

> > +
> > +/// 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};
> > +///
> > +/// struct Registration<T> {
> 
> Maybe add some explanation above/below this example. It looks like a new
> bus registration?

*class device registration, see Registration::new() below. But I can also add a
brief comment to the struct. It's indeed a bit subtly this way. :)

> > +///     data: T,
> > +/// }
> > +///
> > +/// impl<T> Registration<T> {
> > +///     fn new(data: T) -> Result<Arc<Self>> {
> > +///         // register (e.g. class device, IRQ, etc.)
> > +///
> > +///         Ok(Arc::new(Self { data }, GFP_KERNEL)?)
> > +///     }
> > +/// }
> > +///
> > +/// impl<T> Release for Registration<T> {
> > +///     fn release(&self) {
> > +///        // unregister
> > +///     }
> > +/// }
> > +///
> > +/// fn from_bound_context(dev: &Device<Bound>) -> Result {
> > +///     let reg = Registration::new(0x42)?;
> > +///
> > +///     devres::register_foreign_release(dev, reg.clone())
> > +/// }
> > +/// ```
> > +pub fn register_foreign_release<P>(dev: &Device<Bound>, data: P) -> Result
> > +where
> > +    P: ForeignOwnable,
> > +    for<'a> P::Borrowed<'a>: Release,
> > +{
> > +    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,
> > +        for<'a> P::Borrowed<'a>: Release,
> > +    {
> > +        // SAFETY: `ptr` is the pointer to the `ForeignOwnable` leaked above and hence valid.
> > +        unsafe { P::borrow(ptr.cast()) }.release();
> > +
> > +        // SAFETY: `ptr` is the pointer to the `ForeignOwnable` leaked above and hence valid.
> > +        let _ = 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())
> > +    })
> > +}
> 

Re: [PATCH 4/4] rust: devres: implement register_foreign_release()

[Benno Lossin]

On Sun Jun 22, 2025 at 2:46 PM CEST, Danilo Krummrich wrote:
> On Sun, Jun 22, 2025 at 09:26:33AM +0200, Benno Lossin wrote:
>> On Thu Jun 12, 2025 at 4:51 PM CEST, Danilo Krummrich wrote:
>> > +pub trait Release {
>> > +    /// Called once the [`Device`] given to [`register_foreign_release`] is unbound.
>> > +    fn release(&self);
>> 
>> Would it make sense to also supply the `Device` that this is attached
>> to? In case you have one object in multiple `register_foreign_release`
>> calls with different devices, or is that something that doesn't happen?
>
> No, doing that wouldn't make any sense. A resource should only be bound to the
> lifetime of a single device.

But the API doesn't prevent it... Does it make more sense to instead ask
the user to provide a closure?

>> > +}
>> > +
>> > +impl<T: Release> Release for crate::sync::ArcBorrow<'_, T> {
>> > +    fn release(&self) {
>> > +        self.deref().release();
>> > +    }
>> > +}
>> > +
>> > +impl<T: Release> Release for Pin<&'_ T> {
>> > +    fn release(&self) {
>> > +        self.deref().release();
>> > +    }
>> > +}
>> 
>> We should also implement it for `&T`, since that is `Box`'s `Borrowed`.
>
> That should implicitly be the case when T: Release, where T is P<T>.

I don't understand? When `P` is set to `Box<T>` in the
`register_foreign_release` below, then `P::Borrow<'_> == &'_ T` and the
bound of `&T: Release` is not satisfied.

>> > +
>> > +/// 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};
>> > +///
>> > +/// struct Registration<T> {
>> 
>> Maybe add some explanation above/below this example. It looks like a new
>> bus registration?
>
> *class device registration, see Registration::new() below. But I can also add a
> brief comment to the struct. It's indeed a bit subtly this way. :)

I'd just add a small paragraph explaining what it does :)

---
Cheers,
Benno

Re: [PATCH 4/4] rust: devres: implement register_foreign_release()

[Danilo Krummrich]

On Sun, Jun 22, 2025 at 10:14:00PM +0200, Benno Lossin wrote:
> I don't understand? When `P` is set to `Box<T>` in the
> `register_foreign_release` below, then `P::Borrow<'_> == &'_ T` and the
> bound of `&T: Release` is not satisfied.

Yeah, you're right, we have to add `&T` too.