Improvements for Devres

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 <[email protected]>

drivers/gpu/nova-core/gpu.rs +0 -1

 /// # Example
 ///
 /// ```no_run

@@ -213,44 +233,63 @@ 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() {

drivers/gpu/nova-core/gpu.rs +1 -0

+        Ok(unsafe { self.data().access() })
     }

Quoted 4 lines · Danilo Krummrich

drivers/gpu/nova-core/gpu.rs +0 -0

index 60b86f370284..47653c14838b 100644

drivers/gpu/nova-core/gpu.rs fn new(bar: &Bar0) -> Result<Spec> { · line 164

Quoted 6 lines

drivers/gpu/nova-core/gpu.rs +1 -1

@@ -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>>,

Can't you store it inline, given that you return an impl PinInit<Self> below?

drivers/gpu/nova-core/gpu.rs fn new(bar: &Bar0) -> Result<Spec> { · line 172

Quoted 9 lines

drivers/gpu/nova-core/gpu.rs +1 -1

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

While I see this code, is it really necessary to return Result wrapping the initializer here? I think it's probably better to return impl PinInit<Self, Error> instead. (of course in a different patch/an issue)

Quoted 3 lines

drivers/gpu/nova-core/gpu.rs +0 -0

         let bar = devres_bar.access(pdev.as_ref())?;
         let spec = Spec::new(bar)?;

drivers/gpu/nova-core/gpu.rs struct DevresInner<T: Send> { · line 55

Quoted 8 lines

drivers/gpu/nova-core/gpu.rs +4 -0

@@ -44,6 +49,10 @@ struct DevresInner<T: Send> {
 /// [`Devres`] users should make sure to simply free the corresponding backing resource in `T`'s
 /// [`Drop`] implementation.
 ///
+/// # Invariants
+///
+/// [`Self::inner`] is guaranteed to be initialized and is always accessed read-only.
+///

Let's put this section below the examples, I really ought to write the safety docs one day and let everyone vote on this kind of stuff...

Quoted 13 lines

drivers/gpu/nova-core/gpu.rs +1 -1

 /// # Example
 ///
 /// ```no_run

@@ -213,44 +233,63 @@ 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);
         }
 

drivers/gpu/nova-core/gpu.rs pub fn new(dev: &Device<Bound>, data: T, flags: Flags) -> Result<Self> { · line 242

Quoted 6 lines

drivers/gpu/nova-core/gpu.rs +2 -3

         // 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`.

What if the device has been unbound and a new device has been allocated in the exact same memory?

Quoted 6 lines · Benno Lossin

Quoted 4 lines · Danilo Krummrich

drivers/gpu/nova-core/gpu.rs +0 -0

index 60b86f370284..47653c14838b 100644

Quoted 9 lines

drivers/gpu/nova-core/gpu.rs fn new(bar: &Bar0) -> Result<Spec> { · line 164

Quoted 6 lines

drivers/gpu/nova-core/gpu.rs +1 -1

@@ -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>>,

Can't you store it inline, given that you return an impl PinInit<Self> below?

I could, but I already know that we'll have to share bar later on.

Quoted 10 lines

Quoted 9 lines

drivers/gpu/nova-core/gpu.rs +1 -1

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

Quoted 4 lines

While I see this code, is it really necessary to return Result wrapping the initializer here? I think it's probably better to return impl PinInit<Self, Error> instead. (of course in a different patch/an issue)

I will double check, but it's rather unlikely it makes sense. There's a lot of initialization going on in Gpu::new(), the try_pin_init! call would probably get too crazy.

Quoted 13 lines

Quoted 13 lines

drivers/gpu/nova-core/gpu.rs +0 -0

 /// # Example
 ///

/// ```no_run

drivers/gpu/nova-core/gpu.rs +1 -1

@@ -213,44 +233,63 @@ 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);
         }
 

Quoted 9 lines

drivers/gpu/nova-core/gpu.rs pub fn new(dev: &Device<Bound>, data: T, flags: Flags) -> Result<Self> { · line 242

Quoted 6 lines

drivers/gpu/nova-core/gpu.rs +2 -3

         // 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`.

What if the device has been unbound and a new device has been allocated in the exact same memory?

Unbound doesn't mean freed. Devres holds a reference of the device is was created with, so it is impossible that it has been freed.

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 <[email protected]>

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

ForeignOwnable::Target defines the payload data of a ForeignOwnable. For Arc<T> for instance, ForeignOwnable::Target would just be T.

This is useful for cases where a trait bound is required on the target type of the ForeignOwnable. For instance:

	fn example<P>(data: P)
	   where
	      P: ForeignOwnable,
	      P::Target: MyTrait,
	{}

Suggested-by Benno Lossin <[email protected]>

Signed-off-by Danilo Krummrich <[email protected]>

---
 rust/kernel/alloc/kbox.rs | 2 ++
 rust/kernel/sync/arc.rs   | 1 +
 rust/kernel/types.rs      | 4 ++++
 3 files changed, 7 insertions(+)

rust/kernel/alloc/kbox.rs +2 -0

index c386ff771d50..66fad9777567 100644
@@ -403,6 +403,7 @@ unsafe impl<T: 'static, A> ForeignOwnable for Box<T, A>
 where
     A: Allocator,
 {
+    type Target = T;
     type PointedTo = T;
     type Borrowed<'a> = &'a T;
     type BorrowedMut<'a> = &'a mut T;

@@ -435,6 +436,7 @@ unsafe impl<T: 'static, A> ForeignOwnable for Pin<Box<T, A>>
 where
     A: Allocator,
 {
+    type Target = T;
     type PointedTo = T;
     type Borrowed<'a> = Pin<&'a T>;
     type BorrowedMut<'a> = Pin<&'a mut T>;

rust/kernel/sync/arc.rs +1 -0

index c7af0aa48a0a..24fb63597d35 100644
@@ -374,6 +374,7 @@ pub fn into_unique_or_drop(self) -> Option<Pin<UniqueArc<T>>> {
 
 // SAFETY: The `into_foreign` function returns a pointer that is well-aligned.
 unsafe impl<T: 'static> ForeignOwnable for Arc<T> {
+    type Target = T;
     type PointedTo = ArcInner<T>;
     type Borrowed<'a> = ArcBorrow<'a, T>;
     type BorrowedMut<'a> = Self::Borrowed<'a>;

rust/kernel/types.rs +0 -0

index 3958a5f44d56..74c787b352a9 100644

rust/kernel/types.rs +2 -0

+
     /// Type used when the value is foreign-owned. In practical terms only defines the alignment of
     /// the pointer.
     type PointedTo;

@@ -128,6 +131,7 @@ unsafe fn try_from_foreign(ptr: *mut Self::PointedTo) -> Option<Self> {
 
 // SAFETY: The `into_foreign` function returns a pointer that is dangling, but well-aligned.
 unsafe impl ForeignOwnable for () {
+    type Target = ();
     type PointedTo = ();
     type Borrowed<'a> = ();
     type BorrowedMut<'a> = ();

Signature · 1 line

2.49.0

rust/kernel/types.rs line 31

Quoted 10 lines · Danilo Krummrich

rust/kernel/types.rs +2 -0

index 3958a5f44d56..74c787b352a9 100644
@@ -27,6 +27,9 @@
 /// [`into_foreign`]: Self::into_foreign
 /// [`PointedTo`]: Self::PointedTo
 pub unsafe trait ForeignOwnable: Sized {
+    /// The payload type of the foreign-owned value.
+    type Target;

I think ForeignOwnable also implies a T maybe get dropped via a pointer from into_foreign(). Not sure it's worth mentioning though.

Quoted 14 lines · Boqun Feng

rust/kernel/types.rs line 31

Quoted 10 lines · Danilo Krummrich

rust/kernel/types.rs +2 -0

index 3958a5f44d56..74c787b352a9 100644
@@ -27,6 +27,9 @@
 /// [`into_foreign`]: Self::into_foreign
 /// [`PointedTo`]: Self::PointedTo
 pub unsafe trait ForeignOwnable: Sized {
+    /// The payload type of the foreign-owned value.
+    type Target;

I think ForeignOwnable also implies a T maybe get dropped via a pointer from into_foreign(). Not sure it's worth mentioning though.

What? How would that happen?

---
Cheers,
Benno

Quoted 16 lines · Benno Lossin

Quoted 14 lines · Boqun Feng

rust/kernel/types.rs line 31

Quoted 10 lines · Danilo Krummrich

rust/kernel/types.rs +2 -0

index 3958a5f44d56..74c787b352a9 100644
@@ -27,6 +27,9 @@
 /// [`into_foreign`]: Self::into_foreign
 /// [`PointedTo`]: Self::PointedTo
 pub unsafe trait ForeignOwnable: Sized {
+    /// The payload type of the foreign-owned value.
+    type Target;

I think ForeignOwnable also implies a T maybe get dropped via a pointer from into_foreign(). Not sure it's worth mentioning though.

Quoted 1 line

What? How would that happen?

The owner of the pointer can do from_foreign() and eventually drop the ForeignOwnable, hence dropping T.

Quoted 17 lines · Boqun Feng

Quoted 16 lines · Benno Lossin

Quoted 14 lines · Boqun Feng

rust/kernel/types.rs line 31

Quoted 10 lines · Danilo Krummrich

rust/kernel/types.rs +2 -0

index 3958a5f44d56..74c787b352a9 100644
@@ -27,6 +27,9 @@
 /// [`into_foreign`]: Self::into_foreign
 /// [`PointedTo`]: Self::PointedTo
 pub unsafe trait ForeignOwnable: Sized {
+    /// The payload type of the foreign-owned value.
+    type Target;

I think ForeignOwnable also implies a T maybe get dropped via a pointer from into_foreign(). Not sure it's worth mentioning though.

Quoted 4 lines

Quoted 1 line

What? How would that happen?

The owner of the pointer can do from_foreign() and eventually drop the ForeignOwnable, hence dropping T.

I'm confused, you said into_foreign above. I don't think any sensible ForeignOwnable implementation will drop a T in any of its functions.

---
Cheers,
Benno

Quoted 18 lines · Benno Lossin

Quoted 17 lines · Boqun Feng

Quoted 16 lines · Benno Lossin

Quoted 14 lines · Boqun Feng

rust/kernel/types.rs line 31

Quoted 10 lines · Danilo Krummrich

rust/kernel/types.rs +2 -0

index 3958a5f44d56..74c787b352a9 100644
@@ -27,6 +27,9 @@
 /// [`into_foreign`]: Self::into_foreign
 /// [`PointedTo`]: Self::PointedTo
 pub unsafe trait ForeignOwnable: Sized {
+    /// The payload type of the foreign-owned value.
+    type Target;

I think ForeignOwnable also implies a T maybe get dropped via a pointer from into_foreign(). Not sure it's worth mentioning though.

Quoted 8 lines

Quoted 4 lines

Quoted 1 line

What? How would that happen?

The owner of the pointer can do from_foreign() and eventually drop the ForeignOwnable, hence dropping T.

I'm confused, you said into_foreign above. I don't think any sensible ForeignOwnable implementation will drop a T in any of its functions.

A KBox<T> would drop T when it gets dropped, no?
A Arc<T> would drop T when it gets dropped if it’s the last refcount.

I was trying to say “ForeignOwnable::drop() may implies Target::drop()”, that’s what a “payload” means. Maybe that I used “T” instead of “Target” in the original message caused confusion?

Regards,
Boqun

Quoted 3 lines

---
Cheers,
Benno

Quoted 28 lines · Boqun Feng

Quoted 26 lines · Benno Lossin

Quoted 17 lines · Boqun Feng

Quoted 16 lines · Benno Lossin

Quoted 14 lines · Boqun Feng

rust/kernel/types.rs line 31

Quoted 10 lines · Danilo Krummrich

rust/kernel/types.rs +2 -0

index 3958a5f44d56..74c787b352a9 100644
@@ -27,6 +27,9 @@
 /// [`into_foreign`]: Self::into_foreign
 /// [`PointedTo`]: Self::PointedTo
 pub unsafe trait ForeignOwnable: Sized {
+    /// The payload type of the foreign-owned value.
+    type Target;

I think ForeignOwnable also implies a T maybe get dropped via a pointer from into_foreign(). Not sure it's worth mentioning though.

Quoted 4 lines

Quoted 1 line

What? How would that happen?

The owner of the pointer can do from_foreign() and eventually drop the ForeignOwnable, hence dropping T.

I'm confused, you said into_foreign above. I don't think any sensible ForeignOwnable implementation will drop a T in any of its functions.

Quoted 6 lines

A KBox<T> would drop T when it gets dropped, no?
A Arc<T> would drop T when it gets dropped if it’s the last refcount.

I was trying to say “ForeignOwnable::drop() may implies Target::drop()”, that’s what a “payload” means. Maybe that I used “T” instead of “Target” in the original message caused confusion?

The point is whichever receives the pointer from a into_foreign() would owns the Target, because it can from_foreign() and drop the ForeignOwnable. So for example, if the pointer can be passed across threads, that means Target needs to be Send.

ForeignOwnable::Target defines the payload data of a ForeignOwnable. For Arc<T> for instance, ForeignOwnable::Target would just be T.

This is useful for cases where a trait bound is required on the target type of the ForeignOwnable. For instance:

	fn example<P>(data: P)
	   where
	      P: ForeignOwnable,
	      P::Target: MyTrait,
	{}

Suggested-by Benno Lossin <[email protected]>

Signed-off-by Danilo Krummrich <[email protected]>

---
 rust/kernel/alloc/kbox.rs | 2 ++
 rust/kernel/sync/arc.rs   | 1 +
 rust/kernel/types.rs      | 4 ++++
 3 files changed, 7 insertions(+)

ForeignOwnable::Target defines the payload data of a ForeignOwnable. For Arc<T> for instance, ForeignOwnable::Target would just be T.

This is useful for cases where a trait bound is required on the target type of the ForeignOwnable. For instance:

fn example<P>(data: P)
where
P: ForeignOwnable,
P::Target: MyTrait,
{}

Suggested-by Benno Lossin <[email protected]>

Signed-off-by Danilo Krummrich <[email protected]>

ForeignOwnable::Target defines the payload data of a ForeignOwnable. For Arc<T> for instance, ForeignOwnable::Target would just be T.

This is useful for cases where a trait bound is required on the target type of the ForeignOwnable. For instance:

fn example<P>(data: P)
where
P: ForeignOwnable,
P::Target: MyTrait,
{}

Suggested-by Benno Lossin <[email protected]>

Signed-off-by Danilo Krummrich <[email protected]>

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 <[email protected]>

Signed-off-by Danilo Krummrich <[email protected]>

---
 rust/kernel/devres.rs | 73 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 73 insertions(+)

rust/kernel/devres.rs +3 -0

index 3ce8d6161778..92aca78874ff 100644
@@ -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.
+///

rust/kernel/devres.rs +60 -0

+/// 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())
+    })
+}

Signature · 1 line

2.49.0

Quoted 34 lines · Danilo Krummrich

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 <[email protected]>

Signed-off-by Danilo Krummrich <[email protected]>

---
 rust/kernel/devres.rs | 73 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 73 insertions(+)

rust/kernel/devres.rs +3 -0

index 3ce8d6161778..92aca78874ff 100644
@@ -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.
+///

rust/kernel/devres.rs pub fn register<T, E>(dev: &Device<Bound>, data: impl PinInit<T, E>, flags: Flag · line 368

Quoted 10 lines

rust/kernel/devres.rs +10 -0

+/// 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);
    }

Quoted 51 lines · Boqun Feng

Quoted 34 lines · Danilo Krummrich

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 <[email protected]>

Signed-off-by Danilo Krummrich <[email protected]>

---
 rust/kernel/devres.rs | 73 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 73 insertions(+)

rust/kernel/devres.rs +3 -0

index 3ce8d6161778..92aca78874ff 100644
@@ -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.
+///

rust/kernel/devres.rs pub fn register<T, E>(dev: &Device<Bound>, data: impl PinInit<T, E>, flags: Flag · line 368

Quoted 10 lines

rust/kernel/devres.rs +10 -0

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

Quoted 3 lines

    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:

rust/kernel/devres.rs +4 -6

     /// 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()) };

Quoted 52 lines · Danilo Krummrich

Quoted 51 lines · Boqun Feng

Quoted 34 lines · Danilo Krummrich

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 <[email protected]>

Signed-off-by Danilo Krummrich <[email protected]>

---
  rust/kernel/devres.rs | 73 +++++++++++++++++++++++++++++++++++++++++++
  1 file changed, 73 insertions(+)

rust/kernel/devres.rs +3 -0

index 3ce8d6161778..92aca78874ff 100644
@@ -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.
+///

rust/kernel/devres.rs pub fn register<T, E>(dev: &Device<Bound>, data: impl PinInit<T, E>, flags: Flag · line 368

Quoted 10 lines

rust/kernel/devres.rs +10 -0

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

Quoted 8 lines

Quoted 3 lines

     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.

rust/kernel/devres.rs pub fn register<T, E>(dev: &Device<Bound>, data: impl PinInit<T, E>, flags: Flag · line 361

Quoted 13 lines

rust/kernel/devres.rs +1 -4

index 92aca78874ff..42a9cd2812d8 100644
@@ -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.

Quoted 7 lines

Quoted 1 line

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

Quoted 19 lines · Boqun Feng

Review

Quoted 12 lines · Danilo Krummrich

(quoted diff) +12 -0

+/// [`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:

Quoted 3 lines

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

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

Quoted 20 lines · Danilo Krummrich

Quoted 19 lines · Boqun Feng

Review

Quoted 12 lines · Danilo Krummrich

(quoted diff) +12 -0

+/// [`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:

Quoted 5 lines

Quoted 3 lines

    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

Quoted 27 lines · Benno Lossin

Quoted 25 lines · Danilo Krummrich

Quoted 19 lines · Boqun Feng

Review

Quoted 12 lines · Danilo Krummrich

(quoted diff) +12 -0

+/// [`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:

Quoted 3 lines

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

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

Quoted 1 line

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?

Quoted 30 lines · Boqun Feng

Quoted 28 lines · Benno Lossin

Quoted 20 lines · Danilo Krummrich

Quoted 19 lines · Boqun Feng

Review

Quoted 12 lines · Danilo Krummrich

(quoted diff) +12 -0

+/// [`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:

Quoted 5 lines

Quoted 3 lines

    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

Quoted 5 lines

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

Quoted 1 line

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

Quoted 31 lines · Benno Lossin

Quoted 30 lines · Boqun Feng

Quoted 28 lines · Benno Lossin

Quoted 20 lines · Danilo Krummrich

Quoted 19 lines · Boqun Feng

Review

Quoted 12 lines · Danilo Krummrich

(quoted diff) +12 -0

+/// [`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:

Quoted 5 lines

Quoted 3 lines

    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

Quoted 8 lines

Quoted 5 lines

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.

Quoted 4 lines

Quoted 1 line

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?

Quoted 12 lines

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

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

Quoted 2 lines

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.

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

Quoted 1 line · Benno Lossin

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.

Quoted 20 lines · Danilo Krummrich

Quoted 1 line · Benno Lossin

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.

Quoted 2 lines

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.

rust: revocable: support fallible PinInit types
rust: devres: replace Devres::new_foreign_owned()
rust: devres: get rid of Devres' inner Arc

rust: types: ForeignOwnable: Add type Target
rust: devres: implement register_release()