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

[Alice Ryhl <aliceryhl@google.com>]

On Sun, Jun 22, 2025 at 06:40:39PM +0200, Danilo Krummrich wrote:
> 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>
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>

overall looks good
Reviewed-by: Alice Ryhl <aliceryhl@google.com>

>  rust/helpers/device.c     |  7 ++++
>  rust/kernel/cpufreq.rs    | 11 +++---
>  rust/kernel/devres.rs     | 70 +++++++++++++++++++++++++++++++++------
>  rust/kernel/drm/driver.rs | 14 ++++----
>  4 files changed, 82 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 544e50efab43..250073749279 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,61 @@ 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()) };

Nit: I usually write this
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: 'static,
> +    Error: From<E>,
> +{
> +    let data = KBox::pin_init(data, flags)?;

Wouldn't we also want to expose the ForeignOwnable version? It seems
likely someone would want to avoid the allocation.

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