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

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

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

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

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

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

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

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

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

?

Regards,
Boqun

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

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

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

Here's the diff for the change:

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

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

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

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

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

---
Cheers,
Benno

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

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

What's the downside?

Regards,
Boqun

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

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

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

> What's the downside?

You'll need to implement `Release` twice:

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

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

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

---
Cheers,
Benno

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

This is what you said in v3:

"""
and then `register_release` is:

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

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

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

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

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

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

Also you can do:

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

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

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

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

Regards,
Boqun

> 
> ---
> Cheers,
> Benno

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

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

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

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

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

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

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

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

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

---
Cheers,
Benno

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

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

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

Obviously, this implies that the ForeignOwnable is an Arc.

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

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

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

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

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

---
Cheers,
Benno

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

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

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

Looks good to me.

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

My bad, is it possible to do

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

? that's better IMO.

Regards,
Boqun

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

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

Sure, that works.

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

Thanks! With that feel free to add:

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

Regards,
Boqun