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@xxxxxxxxxx>
> > > Signed-off-by: Danilo Krummrich <dakr@xxxxxxxxxx>
> > > ---
> > > 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()) };
