Message-Id: <DASVTOJU2OE8.GU1NH3MZ3SX@kernel.org>
Date: Sun, 22 Jun 2025 09:26:33 +0200
From: "Benno Lossin" <lossin@...nel.org>
To: "Danilo Krummrich" <dakr@...nel.org>, <gregkh@...uxfoundation.org>,
 <rafael@...nel.org>, <ojeda@...nel.org>, <alex.gaynor@...il.com>,
 <boqun.feng@...il.com>, <gary@...yguo.net>, <bjorn3_gh@...tonmail.com>,
 <benno.lossin@...ton.me>, <a.hindborg@...nel.org>, <aliceryhl@...gle.com>,
 <tmgross@...ch.edu>
Cc: <rust-for-linux@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 4/4] rust: devres: implement register_foreign_release()

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@...gle.com>
> Signed-off-by: Danilo Krummrich <dakr@...nel.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())
> +    })
> +}

Powered by blists - more mailing lists