Improvements for Devres

rust/kernel/devres.rs +4 -0

+    #[pin]
+    inner: Opaque<Inner<T>>,
+}
+

rust/kernel/devres.rs +21 -5

+
+#[pinned_drop]
+impl<T> PinnedDrop for Devres<T> {
+    fn drop(self: Pin<&mut Self>) {
         // SAFETY: When `drop` runs, it is guaranteed that nobody is accessing the revocable data
         // anymore, hence it is safe not to wait for the grace period to finish.
-        if unsafe { self.0.data.revoke_nosync() } {
-            // We revoked `self.0.data` before the devres action did, hence try to remove it.
-            if !DevresInner::remove_action(&self.0) {
+        if unsafe { self.data().revoke_nosync() } {
+            // We revoked `self.data` before the devres action did, hence try to remove it.
+            if !self.remove_action() {
                 // We could not remove the devres action, which means that it now runs concurrently,
-                // hence signal that `self.0.data` has been revoked successfully.
-                self.0.revoke.complete_all();
+                // hence signal that `self.data` has been revoked by us successfully.
+                self.inner().revoke.complete_all();
+
+                // Wait for `Self::devres_callback` to be done using this object.
+                self.inner().devm.wait_for_completion();
             }
+        } else {
+            // `Self::devres_callback` revokes `self.data` for us, hence wait for it to be done
+            // using this object.
+            self.inner().devm.wait_for_completion();
         }
+
+        // INVARIANT: At this point it is guaranteed that `inner` can't be accessed any more.
+        //
+        // SAFETY: `inner` is valid for dropping.
+        unsafe { core::ptr::drop_in_place(self.inner.get()) };
     }
 }

On Tue, Jun 24, 2025 at 11:54:01PM +0200, Danilo Krummrich wrote:
[...]

rust/kernel/devres.rs struct DevresInner<T> { · line 108

Quoted 2 lines

rust/kernel/devres.rs +2 -0

+#[pin_data(PinnedDrop)]
+pub struct Devres<T> {

It makes me realize: I think we need to make T being Send? Because the devm callback can happen on a different thread other than Devres::new() and the callback may drop T because of revoke(), so we are essientially sending T. Alternatively we can make Devres::new() and its friend require T being Send.

If it's true, we need a separate patch that "Fixes" this.

(Imagine a Devres<MutexGuard>)

rust/kernel/devres.rs struct DevresInner<T> { · line 116

Quoted 8 lines

rust/kernel/devres.rs +8 -0

+    dev: ARef<Device>,
+    /// Pointer to [`Self::devres_callback`].
+    ///
+    /// Has to be stored, since Rust does not guarantee to always return the same address for a
+    /// function. However, the C API uses the address as a key.
+    callback: unsafe extern "C" fn(*mut c_void),
+    /// Contains all the fields shared with [`Self::callback`].
+    // TODO: Replace with `UnsafePinned`, once available.

nit: Maybe also reference the drop_in_place() in Devres::drop() as well, because once we use UnsafePinned, we don't need that drop_in_place(). But not a big deal, just trying to help the people who would handle that "TODO" ;-)

Quoted 11 lines · Boqun Feng

On Tue, Jun 24, 2025 at 11:54:01PM +0200, Danilo Krummrich wrote:
[...]

rust/kernel/devres.rs struct DevresInner<T> { · line 108

Quoted 2 lines

rust/kernel/devres.rs +2 -0

+#[pin_data(PinnedDrop)]
+pub struct Devres<T> {

It makes me realize: I think we need to make T being Send? Because the devm callback can happen on a different thread other than Devres::new() and the callback may drop T because of revoke(), so we are essientially sending T. Alternatively we can make Devres::new() and its friend require T being Send.

Quoted 1 line

If it's true, we need a separate patch that "Fixes" this.

Indeed, that needs a fix.

Quoted 12 lines · Danilo Krummrich

Quoted 11 lines · Boqun Feng

On Tue, Jun 24, 2025 at 11:54:01PM +0200, Danilo Krummrich wrote:
[...]

rust/kernel/devres.rs struct DevresInner<T> { · line 108

Quoted 2 lines

rust/kernel/devres.rs +2 -0

+#[pin_data(PinnedDrop)]
+pub struct Devres<T> {

It makes me realize: I think we need to make T being Send? Because the devm callback can happen on a different thread other than Devres::new() and the callback may drop T because of revoke(), so we are essientially sending T. Alternatively we can make Devres::new() and its friend require T being Send.

Quoted 3 lines

Quoted 1 line

If it's true, we need a separate patch that "Fixes" this.

Indeed, that needs a fix.

Oh and we have no 'static bound on T either... We should require that as well.

Quoted 13 lines · Benno Lossin

Quoted 12 lines · Danilo Krummrich

Quoted 11 lines · Boqun Feng

On Tue, Jun 24, 2025 at 11:54:01PM +0200, Danilo Krummrich wrote:
[...]

rust/kernel/devres.rs struct DevresInner<T> { · line 108

Quoted 2 lines

rust/kernel/devres.rs +2 -0

+#[pin_data(PinnedDrop)]
+pub struct Devres<T> {

It makes me realize: I think we need to make T being Send? Because the devm callback can happen on a different thread other than Devres::new() and the callback may drop T because of revoke(), so we are essientially sending T. Alternatively we can make Devres::new() and its friend require T being Send.

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

If it's true, we need a separate patch that "Fixes" this.

Indeed, that needs a fix.

Oh and we have no 'static bound on T either... We should require that as well.

I don't think we actually need that, The Devres instance can't out-live a &T passed into it. And the &T can't out-live the &Device<Bound>, hence we're guaranteed that devres_callback() is never called because Devres::drop() will be able successfully unregister the callback given that we're still in the &Device<Bound> scope.

The only thing that could technically out-live the &Device<Bound> would be &'static T, but that would obviously be fine.

Do I miss anything?

Quoted 27 lines · Danilo Krummrich

Quoted 13 lines · Benno Lossin

Quoted 12 lines · Danilo Krummrich

Quoted 11 lines · Boqun Feng

On Tue, Jun 24, 2025 at 11:54:01PM +0200, Danilo Krummrich wrote:
[...]

rust/kernel/devres.rs struct DevresInner<T> { · line 108

Quoted 2 lines

rust/kernel/devres.rs +2 -0

+#[pin_data(PinnedDrop)]
+pub struct Devres<T> {

It makes me realize: I think we need to make T being Send? Because the devm callback can happen on a different thread other than Devres::new() and the callback may drop T because of revoke(), so we are essientially sending T. Alternatively we can make Devres::new() and its friend require T being Send.

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

If it's true, we need a separate patch that "Fixes" this.

Indeed, that needs a fix.

Oh and we have no 'static bound on T either... We should require that as well.

I don't think we actually need that, The Devres instance can't out-live a &T passed into it. And the &T can't out-live the &Device<Bound>, hence we're guaranteed that devres_callback() is never called because Devres::drop() will be able successfully unregister the callback given that we're still in the &Device<Bound> scope.

Quoted 4 lines

The only thing that could technically out-live the &Device<Bound> would be &'static T, but that would obviously be fine.

Do I miss anything?

Thinking a bit more about it, a similar argumentation is true for not needing
T: Send. The only way to leave the &Device<Bound> scope and hence the thread would be to stuff the Devres into a ForeignOwnable container, no?

Analogous to Benno asking for ForeignOwnable: 'static, should we also require
ForeignOwnable: Send + Sync?

Alternatively, the safety requirements of ForeignOwnable:::from_foreign() and ForeignOwnable::borrow() would need to cover this, which they currently they are not.

Quoted 17 lines · Benno Lossin

Quoted 15 lines · Danilo Krummrich

Quoted 11 lines · Boqun Feng

On Tue, Jun 24, 2025 at 11:54:01PM +0200, Danilo Krummrich wrote:
[...]

rust/kernel/devres.rs struct DevresInner<T> { · line 108

Quoted 2 lines

rust/kernel/devres.rs +2 -0

+#[pin_data(PinnedDrop)]
+pub struct Devres<T> {

It makes me realize: I think we need to make T being Send? Because the devm callback can happen on a different thread other than Devres::new() and the callback may drop T because of revoke(), so we are essientially sending T. Alternatively we can make Devres::new() and its friend require T being Send.

Quoted 1 line

If it's true, we need a separate patch that "Fixes" this.

Indeed, that needs a fix.

Quoted 2 lines

Oh and we have no 'static bound on T either... We should require that as well.

I don't think we actually need that, The Devres instance can't out-live a &T passed into it. And the &T can't out-live the &Device<Bound>, hence we're guaranteed that devres_callback() is never called because Devres::drop() will be able successfully unregister the callback given that we're still in the &Device<Bound> scope.

The only thing that could technically out-live the &Device<Bound> would be &'static T, but that would obviously be fine.

Do I miss anything?

On Tue, Jun 24, 2025 at 11:54:01PM +0200, Danilo Krummrich wrote:
T also need to be Send for Devres<T> to be Sync because that's what Revocable<T> requires.

(Unless we want T always being Send
because of the issue I mentioned above)

The rest looks good to me.

Quoted 3 lines · Boqun Feng

On Tue, Jun 24, 2025 at 11:54:01PM +0200, Danilo Krummrich wrote:
T also need to be Send for Devres<T> to be Sync because that's what Revocable<T> requires.

I think that's because of Revocable::revoke(), however you can't call Devres::revoke().

Quoted 3 lines

(Unless we want T always being Send
because of the issue I mentioned above)

Yes, we do.

Quoted 1 line

The rest looks good to me.

Great! :)

rust/kernel/devres.rs +8 -0

+pub fn register_release<P>(dev: &Device<Bound>, data: P) -> Result
+where
+    P: ForeignOwnable + 'static,
+    for<'a> P::Borrowed<'a>: Release,
+{
+    let ptr = data.into_foreign();
+
+    #[allow(clippy::missing_safety_doc)]

rust/kernel/devres.rs +11 -0

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

Or, we could change Release to be:

    pub trait Release {
        type Ptr: ForeignOwnable;

        fn release(this: Self::Ptr);
    }

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.

Related questions:

* should we implement ForeignOwnable for &'static T?

* should we require 'static in ForeignOwnable? At the moment we only have those kinds supported and it only makes sense, a foreign owned object can be owned for any amount of time (so it must stay valid indefinitely).

Quoted 8 lines · Benno Lossin

Or, we could change Release to be:

    pub trait Release {
        type Ptr: ForeignOwnable;

        fn release(this: Self::Ptr);
    }

Quoted 5 lines

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.

I think this was also the case before? Well, it was P::Borrowed instead.

Quoted 3 lines

Related questions:

* should we implement ForeignOwnable for &'static T?

There's already a patch on the list doing this in the context of DebugFS [1].

[1] https://lore.kernel.org/lkml/[email protected]/

Quoted 12 lines · Benno Lossin

Or, we could change Release to be:

    pub trait Release {
        type Ptr: ForeignOwnable;

        fn release(this: Self::Ptr);
    }

and then register_release is:

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

Quoted 1 line

This way, one can store a Box<T> and get access to the T at the end.

I think this was also the case before? Well, it was P::Borrowed instead.

Quoted 2 lines

Or if they store the value in an Arc<T>, they have the option to clone it and give it to somewhere else.

Anyways, I really like this proposal of implementing the Release trait.

Quoted 17 lines · Danilo Krummrich

Quoted 8 lines · Benno Lossin

Or, we could change Release to be:

    pub trait Release {
        type Ptr: ForeignOwnable;

        fn release(this: Self::Ptr);
    }

Quoted 5 lines

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.

I think this was also the case before? Well, it was P::Borrowed instead.

Quoted 4 lines

Quoted 2 lines

Or if they store the value in an Arc<T>, they have the option to clone it and give it to somewhere else.

Anyways, I really like this proposal of implementing the Release trait.

One downside seems to be that the compiler cannot infer T anymore with this function signature.

Quoted 23 lines · Danilo Krummrich

Quoted 21 lines · Danilo Krummrich

Quoted 8 lines · Benno Lossin

Or, we could change Release to be:

    pub trait Release {
        type Ptr: ForeignOwnable;

        fn release(this: Self::Ptr);
    }

Quoted 5 lines

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.

I think this was also the case before? Well, it was P::Borrowed instead.

Quoted 2 lines

Or if they store the value in an Arc<T>, they have the option to clone it and give it to somewhere else.

Anyways, I really like this proposal of implementing the Release trait.

Quoted 2 lines

One downside seems to be that the compiler cannot infer T anymore with this function signature.

Yeah... That's a bit annoying.

We might be able to add an associated type to ForeignOwnable like Target or Inner or whatever.

Then we could do:

    pub fn register_release<P>(dev: &Device<Bound>, data: P) -> Result
    where
        P: ForeignOwnable,
        P::Inner: Release<Ptr = P>,

---
Cheers,
Benno

Quoted 27 lines · Benno Lossin

Quoted 25 lines · Danilo Krummrich

Quoted 13 lines · Danilo Krummrich

Quoted 12 lines · Benno Lossin

Or, we could change Release to be:

    pub trait Release {
        type Ptr: ForeignOwnable;

        fn release(this: Self::Ptr);
    }

and then register_release is:

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

Quoted 8 lines

Quoted 1 line

This way, one can store a Box<T> and get access to the T at the end.

I think this was also the case before? Well, it was P::Borrowed instead.

Quoted 2 lines

Or if they store the value in an Arc<T>, they have the option to clone it and give it to somewhere else.

Anyways, I really like this proposal of implementing the Release trait.

One downside seems to be that the compiler cannot infer T anymore with this function signature.

Quoted 4 lines

Yeah... That's a bit annoying.

We might be able to add an associated type to ForeignOwnable like Target or Inner or whatever.

I think we already have PointedTo [1]? But I remember that I've seen a patch to remove it again [2].

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/rust/kernel/types.rs#n32
[2] https://lore.kernel.org/all/[email protected]/

Quoted 10 lines

Then we could do:

    pub fn register_release<P>(dev: &Device<Bound>, data: P) -> Result
    where
        P: ForeignOwnable,
        P::Inner: Release<Ptr = P>,

---
Cheers,
Benno

Quoted 30 lines · Danilo Krummrich

Quoted 29 lines · Benno Lossin

Quoted 18 lines · Danilo Krummrich

Quoted 17 lines · Danilo Krummrich

Quoted 8 lines · Benno Lossin

Or, we could change Release to be:

    pub trait Release {
        type Ptr: ForeignOwnable;

        fn release(this: Self::Ptr);
    }

Quoted 5 lines

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.

I think this was also the case before? Well, it was P::Borrowed instead.

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

Or if they store the value in an Arc<T>, they have the option to clone it and give it to somewhere else.

Anyways, I really like this proposal of implementing the Release trait.

One downside seems to be that the compiler cannot infer T anymore with this function signature.

Yeah... That's a bit annoying.

Quoted 5 lines

Quoted 2 lines

We might be able to add an associated type to ForeignOwnable like Target or Inner or whatever.

I think we already have PointedTo [1]? But I remember that I've seen a patch to remove it again [2].

Well, not exactly, I think. Arc, for instance, defines PointedTo as ArcInner<T>. So, I think we indeed want something different.