[PATCH v3 0/4] Improvements for Devres

[PATCH v3 0/4] Improvements for Devres

[Danilo Krummrich]

This patch series provides some optimizations for Devres:

  1) Provide a more lightweight replacement for Devres::new_foreign_owned().

  2) Get rid of Devres' inner Arc and instead consume and provide an
     impl PinInit instead.

     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.

  3) An optimization for when we never need to access the resource or release
     it manually.

Thanks to Alice and Benno for some great offline discussions on this topic.

This patch series depends on the Opaque patch in [1] and the pin-init patch in
[2], which Benno will provide a signed tag for. A branch containing the patches
can be found in [3].

[1] https://lore.kernel.org/lkml/20250610-b4-rust_miscdevice_registrationdata-v6-1-b03f5dfce998@gmail.com/
[2] https://lore.kernel.org/rust-for-linux/20250529081027.297648-2-lossin@kernel.org/
[3] https://git.kernel.org/pub/scm/linux/kernel/git/dakr/linux.git/log/?h=rust/devres

Changes in v3:
  - devres::register:
    - add 'static bound for ForeignOwnable
    - use drop() instead of `let _ =`
  - Devres:
    - use ptr::from_ref() instead of Inner::as_ptr()
    - use &raw mut instead of addr_of_mut!()
    - use SAFETY comment proposal from Benno
    - add invariant for `Devres::inner`
    - use ScopeGuard in devres_callback()
  - devres::register_release():
    - add impl<T: Release> Release for &T
    - add 'static bound for ForeignOwnable
    - use drop() instead of `let _ =`

Changes in v2:
  - Revocable:
    - remove Error: From<E> bound
  - devres::register:
    - rename devres::register_foreign_boxed() to just devres::register()
    - move T: 'static bound to the function rather than the impl block
  - Devres:
    - Fix aliasing issue by using an Opaque<Inner>; should be
      UnsafePinned<Inner> once available.
    - Add doc-comments for a couple of private fields.
    - Link Revocable on 'revoke' in Devres::new().
  - devres::register_release():
    - expand documentation of Release
    - rename devres::register_foreign_release() for devres::register_release()

Danilo Krummrich (4):
  rust: revocable: support fallible PinInit types
  rust: devres: replace Devres::new_foreign_owned()
  rust: devres: get rid of Devres' inner Arc
  rust: devres: implement register_release()

 drivers/gpu/nova-core/driver.rs |   7 +-
 drivers/gpu/nova-core/gpu.rs    |   6 +-
 rust/helpers/device.c           |   7 +
 rust/kernel/cpufreq.rs          |  11 +-
 rust/kernel/devres.rs           | 372 +++++++++++++++++++++++---------
 rust/kernel/drm/driver.rs       |  14 +-
 rust/kernel/pci.rs              |  20 +-
 rust/kernel/revocable.rs        |   6 +-
 samples/rust/rust_driver_pci.rs |  19 +-
 9 files changed, 327 insertions(+), 135 deletions(-)


base-commit: d752224c81ec2e5c0e86635c4b1cc835f9ca4151
-- 
2.49.0

[PATCH v3 1/4] rust: revocable: support fallible PinInit types

[Danilo Krummrich]

Currently, Revocable::new() only supports infallible PinInit
implementations, i.e. impl PinInit<T, Infallible>.

This has been sufficient so far, since users such as Devres do not
support fallibility.

Since this is about to change, make Revocable::new() generic over the
error type E.

Reviewed-by: Benno Lossin <lossin@kernel.org>
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Acked-by: Miguel Ojeda <ojeda@kernel.org>
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 rust/kernel/devres.rs    | 2 +-
 rust/kernel/revocable.rs | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
index 57502534d985..544e50efab43 100644
--- a/rust/kernel/devres.rs
+++ b/rust/kernel/devres.rs
@@ -100,7 +100,7 @@ struct DevresInner<T> {
 impl<T> DevresInner<T> {
     fn new(dev: &Device<Bound>, data: T, flags: Flags) -> Result<Arc<DevresInner<T>>> {
         let inner = Arc::pin_init(
-            pin_init!( DevresInner {
+            try_pin_init!( DevresInner {
                 dev: dev.into(),
                 callback: Self::devres_callback,
                 data <- Revocable::new(data),
diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
index fa1fd70efa27..46768b374656 100644
--- a/rust/kernel/revocable.rs
+++ b/rust/kernel/revocable.rs
@@ -82,11 +82,11 @@ unsafe impl<T: Sync + Send> Sync for Revocable<T> {}
 
 impl<T> Revocable<T> {
     /// Creates a new revocable instance of the given data.
-    pub fn new(data: impl PinInit<T>) -> impl PinInit<Self> {
-        pin_init!(Self {
+    pub fn new<E>(data: impl PinInit<T, E>) -> impl PinInit<Self, E> {
+        try_pin_init!(Self {
             is_available: AtomicBool::new(true),
             data <- Opaque::pin_init(data),
-        })
+        }? E)
     }
 
     /// Tries to access the revocable wrapped object.
-- 
2.49.0

[PATCH v3 2/4] rust: devres: replace Devres::new_foreign_owned()

[Danilo Krummrich]

Replace Devres::new_foreign_owned() with devres::register().

The current implementation of Devres::new_foreign_owned() creates a full
Devres container instance, including the internal Revocable and
completion.

However, none of that is necessary for the intended use of giving full
ownership of an object to devres and getting it dropped once the given
device is unbound.

Hence, implement devres::register(), which is limited to consume the
given data, wrap it in a KBox and drop the KBox once the given device is
unbound, without any other synchronization.

Cc: Dave Airlie <airlied@redhat.com>
Cc: Simona Vetter <simona.vetter@ffwll.ch>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
Reviewed-by: Benno Lossin <lossin@kernel.org>
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 rust/helpers/device.c     |  7 ++++
 rust/kernel/cpufreq.rs    | 11 +++---
 rust/kernel/devres.rs     | 70 +++++++++++++++++++++++++++++++++------
 rust/kernel/drm/driver.rs | 14 ++++----
 4 files changed, 82 insertions(+), 20 deletions(-)

diff --git a/rust/helpers/device.c b/rust/helpers/device.c
index b2135c6686b0..502fef7e9ae8 100644
--- a/rust/helpers/device.c
+++ b/rust/helpers/device.c
@@ -8,3 +8,10 @@ int rust_helper_devm_add_action(struct device *dev,
 {
 	return devm_add_action(dev, action, data);
 }
+
+int rust_helper_devm_add_action_or_reset(struct device *dev,
+					 void (*action)(void *),
+					 void *data)
+{
+	return devm_add_action_or_reset(dev, action, data);
+}
diff --git a/rust/kernel/cpufreq.rs b/rust/kernel/cpufreq.rs
index 11b03e9d7e89..dd84e2b4d7ae 100644
--- a/rust/kernel/cpufreq.rs
+++ b/rust/kernel/cpufreq.rs
@@ -13,7 +13,7 @@
     cpu::CpuId,
     cpumask,
     device::{Bound, Device},
-    devres::Devres,
+    devres,
     error::{code::*, from_err_ptr, from_result, to_result, Result, VTABLE_DEFAULT_ERROR},
     ffi::{c_char, c_ulong},
     prelude::*,
@@ -1046,10 +1046,13 @@ pub fn new() -> Result<Self> {
 
     /// Same as [`Registration::new`], but does not return a [`Registration`] instance.
     ///
-    /// Instead the [`Registration`] is owned by [`Devres`] and will be revoked / dropped, once the
+    /// Instead the [`Registration`] is owned by [`devres::register`] and will be dropped, once the
     /// device is detached.
-    pub fn new_foreign_owned(dev: &Device<Bound>) -> Result {
-        Devres::new_foreign_owned(dev, Self::new()?, GFP_KERNEL)
+    pub fn new_foreign_owned(dev: &Device<Bound>) -> Result
+    where
+        T: 'static,
+    {
+        devres::register(dev, Self::new()?, GFP_KERNEL)
     }
 }
 
diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
index 544e50efab43..47ead37faf4c 100644
--- a/rust/kernel/devres.rs
+++ b/rust/kernel/devres.rs
@@ -9,12 +9,12 @@
     alloc::Flags,
     bindings,
     device::{Bound, Device},
-    error::{Error, Result},
+    error::{to_result, Error, Result},
     ffi::c_void,
     prelude::*,
     revocable::{Revocable, RevocableGuard},
     sync::{rcu, Arc, Completion},
-    types::ARef,
+    types::{ARef, ForeignOwnable},
 };
 
 #[pin_data]
@@ -184,14 +184,6 @@ pub fn new(dev: &Device<Bound>, data: T, flags: Flags) -> Result<Self> {
         Ok(Devres(inner))
     }
 
-    /// Same as [`Devres::new`], but does not return a `Devres` instance. Instead the given `data`
-    /// is owned by devres and will be revoked / dropped, once the device is detached.
-    pub fn new_foreign_owned(dev: &Device<Bound>, data: T, flags: Flags) -> Result {
-        let _ = DevresInner::new(dev, data, flags)?;
-
-        Ok(())
-    }
-
     /// Obtain `&'a T`, bypassing the [`Revocable`].
     ///
     /// This method allows to directly obtain a `&'a T`, bypassing the [`Revocable`], by presenting
@@ -261,3 +253,61 @@ fn drop(&mut self) {
         }
     }
 }
+
+/// Consume `data` and [`Drop::drop`] `data` once `dev` is unbound.
+fn register_foreign<P: ForeignOwnable + 'static>(dev: &Device<Bound>, data: P) -> Result {
+    let ptr = data.into_foreign();
+
+    #[allow(clippy::missing_safety_doc)]
+    unsafe extern "C" fn callback<P: ForeignOwnable>(ptr: *mut kernel::ffi::c_void) {
+        // SAFETY: `ptr` is the pointer to the `ForeignOwnable` leaked above and hence valid.
+        drop(unsafe { P::from_foreign(ptr.cast()) });
+    }
+
+    // SAFETY:
+    // - `dev.as_raw()` is a pointer to a valid and bound device.
+    // - `ptr` is a valid pointer the `ForeignOwnable` devres takes ownership of.
+    to_result(unsafe {
+        // `devm_add_action_or_reset()` also calls `callback` on failure, such that the
+        // `ForeignOwnable` is released eventually.
+        bindings::devm_add_action_or_reset(dev.as_raw(), Some(callback::<P>), ptr.cast())
+    })
+}
+
+/// Encapsulate `data` in a [`KBox`] and [`Drop::drop`] `data` once `dev` is unbound.
+///
+/// # Examples
+///
+/// ```no_run
+/// use kernel::{device::{Bound, Device}, devres};
+///
+/// /// Registration of e.g. a class device, IRQ, etc.
+/// struct Registration;
+///
+/// impl Registration {
+///     fn new() -> Self {
+///         // register
+///
+///         Self
+///     }
+/// }
+///
+/// impl Drop for Registration {
+///     fn drop(&mut self) {
+///        // unregister
+///     }
+/// }
+///
+/// fn from_bound_context(dev: &Device<Bound>) -> Result {
+///     devres::register(dev, Registration::new(), GFP_KERNEL)
+/// }
+/// ```
+pub fn register<T, E>(dev: &Device<Bound>, data: impl PinInit<T, E>, flags: Flags) -> Result
+where
+    T: 'static,
+    Error: From<E>,
+{
+    let data = KBox::pin_init(data, flags)?;
+
+    register_foreign(dev, data)
+}
diff --git a/rust/kernel/drm/driver.rs b/rust/kernel/drm/driver.rs
index acb638086131..f63addaf7235 100644
--- a/rust/kernel/drm/driver.rs
+++ b/rust/kernel/drm/driver.rs
@@ -5,9 +5,7 @@
 //! C header: [`include/linux/drm/drm_drv.h`](srctree/include/linux/drm/drm_drv.h)
 
 use crate::{
-    bindings, device,
-    devres::Devres,
-    drm,
+    bindings, device, devres, drm,
     error::{to_result, Result},
     prelude::*,
     str::CStr,
@@ -130,18 +128,22 @@ fn new(drm: &drm::Device<T>, flags: usize) -> Result<Self> {
     }
 
     /// Same as [`Registration::new`}, but transfers ownership of the [`Registration`] to
-    /// [`Devres`].
+    /// [`devres::register`].
     pub fn new_foreign_owned(
         drm: &drm::Device<T>,
         dev: &device::Device<device::Bound>,
         flags: usize,
-    ) -> Result {
+    ) -> Result
+    where
+        T: 'static,
+    {
         if drm.as_ref().as_raw() != dev.as_raw() {
             return Err(EINVAL);
         }
 
         let reg = Registration::<T>::new(drm, flags)?;
-        Devres::new_foreign_owned(dev, reg, GFP_KERNEL)
+
+        devres::register(dev, reg, GFP_KERNEL)
     }
 
     /// Returns a reference to the `Device` instance for this registration.
-- 
2.49.0

[PATCH v3 3/4] rust: devres: get rid of Devres' inner Arc

[Danilo Krummrich]

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 <dakr@kernel.org>
---
 drivers/gpu/nova-core/driver.rs |   7 +-
 drivers/gpu/nova-core/gpu.rs    |   6 +-
 rust/kernel/devres.rs           | 210 +++++++++++++++++++-------------
 rust/kernel/pci.rs              |  20 +--
 samples/rust/rust_driver_pci.rs |  19 +--
 5 files changed, 151 insertions(+), 111 deletions(-)

diff --git a/drivers/gpu/nova-core/driver.rs b/drivers/gpu/nova-core/driver.rs
index 8c86101c26cb..110f2b355db4 100644
--- a/drivers/gpu/nova-core/driver.rs
+++ b/drivers/gpu/nova-core/driver.rs
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 
-use kernel::{auxiliary, bindings, c_str, device::Core, pci, prelude::*};
+use kernel::{auxiliary, bindings, c_str, device::Core, pci, prelude::*, sync::Arc};
 
 use crate::gpu::Gpu;
 
@@ -34,7 +34,10 @@ fn probe(pdev: &pci::Device<Core>, _info: &Self::IdInfo) -> Result<Pin<KBox<Self
         pdev.enable_device_mem()?;
         pdev.set_master();
 
-        let bar = pdev.iomap_region_sized::<BAR0_SIZE>(0, c_str!("nova-core/bar0"))?;
+        let bar = Arc::pin_init(
+            pdev.iomap_region_sized::<BAR0_SIZE>(0, c_str!("nova-core/bar0")),
+            GFP_KERNEL,
+        )?;
 
         let this = KBox::pin_init(
             try_pin_init!(Self {
diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
index 60b86f370284..47653c14838b 100644
--- a/drivers/gpu/nova-core/gpu.rs
+++ b/drivers/gpu/nova-core/gpu.rs
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 
-use kernel::{device, devres::Devres, error::code::*, pci, prelude::*};
+use kernel::{device, devres::Devres, error::code::*, pci, prelude::*, sync::Arc};
 
 use crate::driver::Bar0;
 use crate::firmware::{Firmware, FIRMWARE_VERSION};
@@ -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>>,
     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>> {
         let bar = devres_bar.access(pdev.as_ref())?;
         let spec = Spec::new(bar)?;
diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
index 47ead37faf4c..eeffdc8115aa 100644
--- a/rust/kernel/devres.rs
+++ b/rust/kernel/devres.rs
@@ -13,16 +13,21 @@
     ffi::c_void,
     prelude::*,
     revocable::{Revocable, RevocableGuard},
-    sync::{rcu, Arc, Completion},
-    types::{ARef, ForeignOwnable},
+    sync::{rcu, Completion},
+    types::{ARef, ForeignOwnable, ScopeGuard, Opaque},
 };
 
+use pin_init::Wrapper;
+
+/// [`Devres`] inner data accessed from [`Devres::callback`].
 #[pin_data]
-struct DevresInner<T> {
-    dev: ARef<Device>,
-    callback: unsafe extern "C" fn(*mut c_void),
+struct Inner<T> {
     #[pin]
     data: Revocable<T>,
+    /// Tracks whether [`Devres::callback`] has been completed.
+    #[pin]
+    devm: Completion,
+    /// Tracks whether revoking [`Self::data`] has been completed.
     #[pin]
     revoke: Completion,
 }
@@ -44,6 +49,10 @@ struct DevresInner<T> {
 /// [`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.
+///
 /// # Example
 ///
 /// ```no_run
@@ -88,100 +97,108 @@ struct DevresInner<T> {
 /// # fn no_run(dev: &Device<Bound>) -> Result<(), Error> {
 /// // SAFETY: Invalid usage for example purposes.
 /// let iomem = unsafe { IoMem::<{ core::mem::size_of::<u32>() }>::new(0xBAAAAAAD)? };
-/// let devres = Devres::new(dev, iomem, GFP_KERNEL)?;
+/// let devres = KBox::pin_init(Devres::new(dev, iomem), GFP_KERNEL)?;
 ///
 /// let res = devres.try_access().ok_or(ENXIO)?;
 /// res.write8(0x42, 0x0);
 /// # Ok(())
 /// # }
 /// ```
-pub struct Devres<T>(Arc<DevresInner<T>>);
+#[pin_data(PinnedDrop)]
+pub struct Devres<T> {
+    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.
+    #[pin]
+    inner: Opaque<Inner<T>>,
+}
+
+impl<T> Devres<T> {
+    /// Creates a new [`Devres`] instance of the given `data`.
+    ///
+    /// The `data` encapsulated within the returned `Devres` instance' `data` will be
+    /// (revoked)[`Revocable`] once the device is detached.
+    pub fn new<'a, E>(
+        dev: &'a Device<Bound>,
+        data: impl PinInit<T, E> + 'a,
+    ) -> impl PinInit<Self, Error> + 'a
+    where
+        T: 'a,
+        Error: From<E>,
+    {
+        let callback = Self::devres_callback;
 
-impl<T> DevresInner<T> {
-    fn new(dev: &Device<Bound>, data: T, flags: Flags) -> Result<Arc<DevresInner<T>>> {
-        let inner = Arc::pin_init(
-            try_pin_init!( DevresInner {
-                dev: dev.into(),
-                callback: Self::devres_callback,
+        try_pin_init!(&this in Self {
+            // INVARIANT: `inner` is properly initialized.
+            inner <- Opaque::pin_init(try_pin_init!(Inner {
                 data <- Revocable::new(data),
+                devm <- Completion::new(),
                 revoke <- Completion::new(),
-            }),
-            flags,
-        )?;
-
-        // Convert `Arc<DevresInner>` into a raw pointer and make devres own this reference until
-        // `Self::devres_callback` is called.
-        let data = inner.clone().into_raw();
+            })),
+            callback,
+            dev: {
+                // SAFETY: `this` is a valid pointer to uninitialized memory.
+                let inner = unsafe { &raw mut (*this.as_ptr()).inner };
 
-        // SAFETY: `devm_add_action` guarantees to call `Self::devres_callback` once `dev` is
-        // detached.
-        let ret =
-            unsafe { bindings::devm_add_action(dev.as_raw(), Some(inner.callback), data as _) };
-
-        if ret != 0 {
-            // SAFETY: We just created another reference to `inner` in order to pass it to
-            // `bindings::devm_add_action`. If `bindings::devm_add_action` fails, we have to drop
-            // this reference accordingly.
-            let _ = unsafe { Arc::from_raw(data) };
-            return Err(Error::from_errno(ret));
-        }
+                // SAFETY:
+                // - `dev.as_raw()` is a pointer to a valid bound device.
+                // - `inner` is guaranteed to be a valid for the duration of the lifetime of `Self`.
+                // - `devm_add_action()` is guaranteed not to call `callback` until `this` has been
+                //    properly initialized, because we require `dev` (i.e. the *bound* device) to
+                //    live at least as long as the returned `impl PinInit<Self, Error>`.
+                to_result(unsafe {
+                    bindings::devm_add_action(dev.as_raw(), Some(callback), inner.cast())
+                })?;
 
-        Ok(inner)
+                dev.into()
+            },
+        })
     }
 
-    fn as_ptr(&self) -> *const Self {
-        self as _
+    fn inner(&self) -> &Inner<T> {
+        // SAFETY: By the type invairants of `Self`, `inner` is properly initialized and always
+        // accessed read-only.
+        unsafe { &*self.inner.get() }
     }
 
-    fn remove_action(this: &Arc<Self>) -> bool {
-        // SAFETY:
-        // - `self.inner.dev` is a valid `Device`,
-        // - the `action` and `data` pointers are the exact same ones as given to devm_add_action()
-        //   previously,
-        // - `self` is always valid, even if the action has been released already.
-        let success = unsafe {
-            bindings::devm_remove_action_nowarn(
-                this.dev.as_raw(),
-                Some(this.callback),
-                this.as_ptr() as _,
-            )
-        } == 0;
-
-        if success {
-            // SAFETY: We leaked an `Arc` reference to devm_add_action() in `DevresInner::new`; if
-            // devm_remove_action_nowarn() was successful we can (and have to) claim back ownership
-            // of this reference.
-            let _ = unsafe { Arc::from_raw(this.as_ptr()) };
-        }
-
-        success
+    fn data(&self) -> &Revocable<T> {
+        &self.inner().data
     }
 
     #[allow(clippy::missing_safety_doc)]
     unsafe extern "C" fn devres_callback(ptr: *mut kernel::ffi::c_void) {
-        let ptr = ptr as *mut DevresInner<T>;
-        // Devres owned this memory; now that we received the callback, drop the `Arc` and hence the
-        // reference.
-        // SAFETY: Safe, since we leaked an `Arc` reference to devm_add_action() in
-        //         `DevresInner::new`.
-        let inner = unsafe { Arc::from_raw(ptr) };
+        // SAFETY: In `Self::new` we've passed a valid pointer to `Inner` to `devm_add_action()`,
+        // hence `ptr` must be a valid pointer to `Inner`.
+        let inner = unsafe { &*ptr.cast::<Inner<T>>() };
+
+        // Ensure that `inner` can't be used anymore after we signal completion of this callback.
+        let inner = ScopeGuard::new_with_data(inner, |inner| inner.devm.complete_all());
 
         if !inner.data.revoke() {
             // If `revoke()` returns false, it means that `Devres::drop` already started revoking
-            // `inner.data` for us. Hence we have to wait until `Devres::drop()` signals that it
-            // completed revoking `inner.data`.
+            // `data` for us. Hence we have to wait until `Devres::drop` signals that it
+            // completed revoking `data`.
             inner.revoke.wait_for_completion();
         }
     }
-}
 
-impl<T> Devres<T> {
-    /// Creates a new [`Devres`] instance of the given `data`. The `data` encapsulated within the
-    /// returned `Devres` instance' `data` will be revoked once the device is detached.
-    pub fn new(dev: &Device<Bound>, data: T, flags: Flags) -> Result<Self> {
-        let inner = DevresInner::new(dev, data, flags)?;
-
-        Ok(Devres(inner))
+    fn remove_action(&self) -> bool {
+        // SAFETY:
+        // - `self.dev` is a valid `Device`,
+        // - the `action` and `data` pointers are the exact same ones as given to
+        //   `devm_add_action()` previously,
+        (unsafe {
+            bindings::devm_remove_action_nowarn(
+                self.dev.as_raw(),
+                Some(self.callback),
+                core::ptr::from_ref(self.inner()).cast_mut().cast(),
+            )
+        } == 0)
     }
 
     /// Obtain `&'a T`, bypassing the [`Revocable`].
@@ -213,44 +230,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);
         }
 
         // 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`.
+        Ok(unsafe { self.data().access() })
     }
 
     /// [`Devres`] accessor for [`Revocable::try_access`].
     pub fn try_access(&self) -> Option<RevocableGuard<'_, T>> {
-        self.0.data.try_access()
+        self.data().try_access()
     }
 
     /// [`Devres`] accessor for [`Revocable::try_access_with`].
     pub fn try_access_with<R, F: FnOnce(&T) -> R>(&self, f: F) -> Option<R> {
-        self.0.data.try_access_with(f)
+        self.data().try_access_with(f)
     }
 
     /// [`Devres`] accessor for [`Revocable::try_access_with_guard`].
     pub fn try_access_with_guard<'a>(&'a self, guard: &'a rcu::Guard) -> Option<&'a T> {
-        self.0.data.try_access_with_guard(guard)
+        self.data().try_access_with_guard(guard)
     }
 }
 
-impl<T> Drop for Devres<T> {
-    fn drop(&mut self) {
+// SAFETY: `Devres` can be send to any task, if `T: Send`.
+unsafe impl<T: Send> Send for Devres<T> {}
+
+// SAFETY: `Devres` can be shared with any task, if `T: Sync`.
+unsafe impl<T: Sync> Sync for Devres<T> {}
+
+#[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()) };
     }
 }
 
diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
index 8435f8132e38..db0eb7eaf9b1 100644
--- a/rust/kernel/pci.rs
+++ b/rust/kernel/pci.rs
@@ -5,7 +5,6 @@
 //! C header: [`include/linux/pci.h`](srctree/include/linux/pci.h)
 
 use crate::{
-    alloc::flags::*,
     bindings, container_of, device,
     device_id::RawDeviceId,
     devres::Devres,
@@ -398,19 +397,20 @@ pub fn resource_len(&self, bar: u32) -> Result<bindings::resource_size_t> {
 impl Device<device::Bound> {
     /// Mapps an entire PCI-BAR after performing a region-request on it. I/O operation bound checks
     /// can be performed on compile time for offsets (plus the requested type size) < SIZE.
-    pub fn iomap_region_sized<const SIZE: usize>(
-        &self,
+    pub fn iomap_region_sized<'a, const SIZE: usize>(
+        &'a self,
         bar: u32,
-        name: &CStr,
-    ) -> Result<Devres<Bar<SIZE>>> {
-        let bar = Bar::<SIZE>::new(self, bar, name)?;
-        let devres = Devres::new(self.as_ref(), bar, GFP_KERNEL)?;
-
-        Ok(devres)
+        name: &'a CStr,
+    ) -> impl PinInit<Devres<Bar<SIZE>>, Error> + 'a {
+        Devres::new(self.as_ref(), Bar::<SIZE>::new(self, bar, name))
     }
 
     /// Mapps an entire PCI-BAR after performing a region-request on it.
-    pub fn iomap_region(&self, bar: u32, name: &CStr) -> Result<Devres<Bar>> {
+    pub fn iomap_region<'a>(
+        &'a self,
+        bar: u32,
+        name: &'a CStr,
+    ) -> impl PinInit<Devres<Bar>, Error> + 'a {
         self.iomap_region_sized::<0>(bar, name)
     }
 }
diff --git a/samples/rust/rust_driver_pci.rs b/samples/rust/rust_driver_pci.rs
index 15147e4401b2..5c35f1414172 100644
--- a/samples/rust/rust_driver_pci.rs
+++ b/samples/rust/rust_driver_pci.rs
@@ -25,8 +25,10 @@ impl TestIndex {
     const NO_EVENTFD: Self = Self(0);
 }
 
+#[pin_data(PinnedDrop)]
 struct SampleDriver {
     pdev: ARef<pci::Device>,
+    #[pin]
     bar: Devres<Bar0>,
 }
 
@@ -73,13 +75,11 @@ fn probe(pdev: &pci::Device<Core>, info: &Self::IdInfo) -> Result<Pin<KBox<Self>
         pdev.enable_device_mem()?;
         pdev.set_master();
 
-        let bar = pdev.iomap_region_sized::<{ Regs::END }>(0, c_str!("rust_driver_pci"))?;
-
-        let drvdata = KBox::new(
-            Self {
+        let drvdata = KBox::pin_init(
+            try_pin_init!(Self {
                 pdev: pdev.into(),
-                bar,
-            },
+                bar <- pdev.iomap_region_sized::<{ Regs::END }>(0, c_str!("rust_driver_pci")),
+            }),
             GFP_KERNEL,
         )?;
 
@@ -90,12 +90,13 @@ fn probe(pdev: &pci::Device<Core>, info: &Self::IdInfo) -> Result<Pin<KBox<Self>
             Self::testdev(info, bar)?
         );
 
-        Ok(drvdata.into())
+        Ok(drvdata)
     }
 }
 
-impl Drop for SampleDriver {
-    fn drop(&mut self) {
+#[pinned_drop]
+impl PinnedDrop for SampleDriver {
+    fn drop(self: Pin<&mut Self>) {
         dev_dbg!(self.pdev.as_ref(), "Remove Rust PCI driver sample.\n");
     }
 }
-- 
2.49.0

[PATCH v3 4/4] rust: devres: implement register_release()

[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 <aliceryhl@google.com>
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 rust/kernel/devres.rs | 90 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 90 insertions(+)

diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
index eeffdc8115aa..ff33e835c44f 100644
--- a/rust/kernel/devres.rs
+++ b/rust/kernel/devres.rs
@@ -16,6 +16,7 @@
     sync::{rcu, Completion},
     types::{ARef, ForeignOwnable, ScopeGuard, Opaque},
 };
+use core::ops::Deref;
 
 use pin_init::Wrapper;
 
@@ -347,3 +348,92 @@ 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 {
+    /// Called once the [`Device`] given to [`register_release`] is unbound.
+    fn release(&self);
+}
+
+impl<T: Release> Release for crate::sync::ArcBorrow<'_, T> {
+    fn release(&self) {
+        self.deref().release();
+    }
+}
+
+impl<T: Release> Release for Pin<&'_ T> {
+    fn release(&self) {
+        self.deref().release();
+    }
+}
+
+impl<T: Release> Release for &'_ T {
+    fn release(&self) {
+        (*self).release();
+    }
+}
+
+/// 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<T> {
+///     data: T,
+/// }
+///
+/// impl<T> Registration<T> {
+///     fn new(data: T) -> Result<Arc<Self>> {
+///         // register
+///
+///         Ok(Arc::new(Self { data }, GFP_KERNEL)?)
+///     }
+/// }
+///
+/// impl<T> Release for Registration<T> {
+///     fn release(&self) {
+///        // unregister
+///     }
+/// }
+///
+/// fn from_bound_context(dev: &Device<Bound>) -> Result {
+///     let reg = Registration::new(0x42)?;
+///
+///     devres::register_release(dev, reg.clone())
+/// }
+/// ```
+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)]
+    unsafe extern "C" fn callback<P>(ptr: *mut kernel::ffi::c_void)
+    where
+        P: ForeignOwnable,
+        for<'a> P::Borrowed<'a>: Release,
+    {
+        // SAFETY: `ptr` is the pointer to the `ForeignOwnable` leaked above and hence valid.
+        unsafe { P::borrow(ptr.cast()) }.release();
+
+        // SAFETY: `ptr` is the pointer to the `ForeignOwnable` leaked above and hence valid.
+        drop(unsafe { P::from_foreign(ptr.cast()) });
+    }
+
+    // SAFETY:
+    // - `dev.as_raw()` is a pointer to a valid and bound device.
+    // - `ptr` is a valid pointer the `ForeignOwnable` devres takes ownership of.
+    to_result(unsafe {
+        // `devm_add_action_or_reset()` also calls `callback` on failure, such that the
+        // `ForeignOwnable` is released eventually.
+        bindings::devm_add_action_or_reset(dev.as_raw(), Some(callback::<P>), ptr.cast())
+    })
+}
-- 
2.49.0

Re: [PATCH v3 3/4] rust: devres: get rid of Devres' inner Arc

[Boqun Feng]

On Tue, Jun 24, 2025 at 11:54:01PM +0200, Danilo Krummrich wrote:
[...]
> +#[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>)

> +    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" ;-)

> +    #[pin]
> +    inner: Opaque<Inner<T>>,
> +}
> +
[...]
> +// SAFETY: `Devres` can be send to any task, if `T: Send`.
> +unsafe impl<T: Send> Send for Devres<T> {}
> +
> +// SAFETY: `Devres` can be shared with any task, if `T: Sync`.
> +unsafe impl<T: Sync> Sync for Devres<T> {}

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

Regards,
Boqun

> +
> +#[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()) };
>      }
>  }
>  
[...]

Re: [PATCH v3 3/4] rust: devres: get rid of Devres' inner Arc

[Danilo Krummrich]

On Wed, Jun 25, 2025 at 09:13:24PM -0700, Boqun Feng wrote:
> On Tue, Jun 24, 2025 at 11:54:01PM +0200, Danilo Krummrich wrote:
> [...]
> > +#[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.

Indeed, that needs a fix.

> (Imagine a Devres<MutexGuard>)
> 
> > +    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" ;-)

Makes sense, the same is true for the Send and Sync impls, I think. AFAIK,
UnsafePinned should cover them automatically.

Re: [PATCH v3 3/4] rust: devres: get rid of Devres' inner Arc

[Benno Lossin]

On Thu Jun 26, 2025 at 12:01 PM CEST, Danilo Krummrich wrote:
> On Wed, Jun 25, 2025 at 09:13:24PM -0700, Boqun Feng wrote:
>> On Tue, Jun 24, 2025 at 11:54:01PM +0200, Danilo Krummrich wrote:
>> [...]
>> > +#[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.
>
> Indeed, that needs a fix.

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

---
Cheers,
Benno

Re: [PATCH v3 4/4] rust: devres: implement register_release()

[Benno Lossin]

On Tue Jun 24, 2025 at 11:54 PM CEST, Danilo Krummrich wrote:
> +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)]
> +    unsafe extern "C" fn callback<P>(ptr: *mut kernel::ffi::c_void)
> +    where
> +        P: ForeignOwnable,
> +        for<'a> P::Borrowed<'a>: Release,
> +    {
> +        // SAFETY: `ptr` is the pointer to the `ForeignOwnable` leaked above and hence valid.
> +        unsafe { P::borrow(ptr.cast()) }.release();
> +
> +        // SAFETY: `ptr` is the pointer to the `ForeignOwnable` leaked above and hence valid.
> +        drop(unsafe { P::from_foreign(ptr.cast()) });

Maybe this function should just be:

    let p = unsafe { P::from_foreign(ptr.cast()) };
    p.release();

And we require `P: ForeignOwnable + Release + 'static`?

We then need these impls instead:

    impl<T: Release, A: Allocator> Release for Pin<Box<T, A>>;
    impl<T: Release, A: Allocator> Release for Box<T, A>;
    impl<T: Release> Release for Arc<T>;

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

---
Cheers,
Benno

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

Re: [PATCH v3 3/4] rust: devres: get rid of Devres' inner Arc

[Danilo Krummrich]

On Thu, Jun 26, 2025 at 12:27:18PM +0200, Benno Lossin wrote:
> On Thu Jun 26, 2025 at 12:01 PM CEST, Danilo Krummrich wrote:
> > On Wed, Jun 25, 2025 at 09:13:24PM -0700, Boqun Feng wrote:
> >> On Tue, Jun 24, 2025 at 11:54:01PM +0200, Danilo Krummrich wrote:
> >> [...]
> >> > +#[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.
> >
> > 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?

Re: [PATCH v3 4/4] rust: devres: implement register_release()

[Danilo Krummrich]

On Thu, Jun 26, 2025 at 12:36:23PM +0200, Benno Lossin wrote:
> 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.

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

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

> 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/20250624-debugfs-rust-v7-3-9c8835a7a20f@google.com/

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

Sounds reasonable to me.

Re: [PATCH v3 3/4] rust: devres: get rid of Devres' inner Arc

[Danilo Krummrich]

On Thu, Jun 26, 2025 at 01:07:25PM +0200, Danilo Krummrich wrote:
> On Thu, Jun 26, 2025 at 12:27:18PM +0200, Benno Lossin wrote:
> > On Thu Jun 26, 2025 at 12:01 PM CEST, Danilo Krummrich wrote:
> > > On Wed, Jun 25, 2025 at 09:13:24PM -0700, Boqun Feng wrote:
> > >> On Tue, Jun 24, 2025 at 11:54:01PM +0200, Danilo Krummrich wrote:
> > >> [...]
> > >> > +#[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.
> > >
> > > 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?

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.

Re: [PATCH v3 3/4] rust: devres: get rid of Devres' inner Arc

[Benno Lossin]

On Thu Jun 26, 2025 at 1:07 PM CEST, Danilo Krummrich wrote:
> On Thu, Jun 26, 2025 at 12:27:18PM +0200, Benno Lossin wrote:
>> On Thu Jun 26, 2025 at 12:01 PM CEST, Danilo Krummrich wrote:
>> > On Wed, Jun 25, 2025 at 09:13:24PM -0700, Boqun Feng wrote:
>> >> On Tue, Jun 24, 2025 at 11:54:01PM +0200, Danilo Krummrich wrote:
>> >> [...]
>> >> > +#[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.
>> >
>> > 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.

Yeah that's correct, I got confused.

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

Nope :)

---
Cheers,
Benno

Re: [PATCH v3 3/4] rust: devres: get rid of Devres' inner Arc

[Benno Lossin]

On Thu Jun 26, 2025 at 1:40 PM CEST, Danilo Krummrich wrote:
> On Thu, Jun 26, 2025 at 01:07:25PM +0200, Danilo Krummrich wrote:
>> On Thu, Jun 26, 2025 at 12:27:18PM +0200, Benno Lossin wrote:
>> > On Thu Jun 26, 2025 at 12:01 PM CEST, Danilo Krummrich wrote:
>> > > On Wed, Jun 25, 2025 at 09:13:24PM -0700, Boqun Feng wrote:
>> > >> On Tue, Jun 24, 2025 at 11:54:01PM +0200, Danilo Krummrich wrote:
>> > >> [...]
>> > >> > +#[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.
>> > >
>> > > 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?
>
> 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?

I think `T: Send` is required, since we drop the `T` in the other thread
when `devres_callback` is called from the device unbinding.

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

I don't think so, you could have a type that stores the pointer in C,
but only ever allows access from the same thread.

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

Oh right, yeah they should cover that.

---
Cheers,
Benno

Re: [PATCH v3 4/4] rust: devres: implement register_release()

[Benno Lossin]

On Thu Jun 26, 2025 at 1:15 PM CEST, Danilo Krummrich wrote:
> On Thu, Jun 26, 2025 at 12:36:23PM +0200, Benno Lossin wrote:
>> 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.
>
> I think this was also the case before? Well, it was P::Borrowed instead.

Well you had access to a `&T`, but not the `T` directly.

>> 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/20250624-debugfs-rust-v7-3-9c8835a7a20f@google.com/

Ah, I'm not following that series at the moment.

---
Cheers,
Benno

Re: [PATCH v3 4/4] rust: devres: implement register_release()

[Danilo Krummrich]

On Thu, Jun 26, 2025 at 01:15:34PM +0200, Danilo Krummrich wrote:
> On Thu, Jun 26, 2025 at 12:36:23PM +0200, Benno Lossin wrote:
> > 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.
> 
> I think this was also the case before? Well, it was P::Borrowed instead.
> 
> > 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.

Re: [PATCH v3 4/4] rust: devres: implement register_release()

[Benno Lossin]

On Thu Jun 26, 2025 at 4:02 PM CEST, Danilo Krummrich wrote:
> On Thu, Jun 26, 2025 at 01:15:34PM +0200, Danilo Krummrich wrote:
>> On Thu, Jun 26, 2025 at 12:36:23PM +0200, Benno Lossin wrote:
>> > 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.
>> 
>> I think this was also the case before? Well, it was P::Borrowed instead.
>> 
>> > 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.

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

Re: [PATCH v3 4/4] rust: devres: implement register_release()

[Danilo Krummrich]

On Thu, Jun 26, 2025 at 04:41:55PM +0200, Benno Lossin wrote:
> On Thu Jun 26, 2025 at 4:02 PM CEST, Danilo Krummrich wrote:
> > On Thu, Jun 26, 2025 at 01:15:34PM +0200, Danilo Krummrich wrote:
> >> On Thu, Jun 26, 2025 at 12:36:23PM +0200, Benno Lossin wrote:
> >> > 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.
> >> 
> >> I think this was also the case before? Well, it was P::Borrowed instead.
> >> 
> >> > 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.
> 
> 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/20250612-pointed-to-v3-1-b009006d86a1@kernel.org/

> 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

Re: [PATCH v3 4/4] rust: devres: implement register_release()

[Danilo Krummrich]

On Thu, Jun 26, 2025 at 05:32:25PM +0200, Danilo Krummrich wrote:
> On Thu, Jun 26, 2025 at 04:41:55PM +0200, Benno Lossin wrote:
> > On Thu Jun 26, 2025 at 4:02 PM CEST, Danilo Krummrich wrote:
> > > On Thu, Jun 26, 2025 at 01:15:34PM +0200, Danilo Krummrich wrote:
> > >> On Thu, Jun 26, 2025 at 12:36:23PM +0200, Benno Lossin wrote:
> > >> > 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.
> > >> 
> > >> I think this was also the case before? Well, it was P::Borrowed instead.
> > >> 
> > >> > 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.
> > 
> > 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.

> [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/20250612-pointed-to-v3-1-b009006d86a1@kernel.org/
> 
> > 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

Re: [PATCH v3 3/4] rust: devres: get rid of Devres' inner Arc

[Danilo Krummrich]

On Wed, Jun 25, 2025 at 09:13:24PM -0700, Boqun Feng wrote:
> 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().

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

Yes, we do.

> The rest looks good to me.

Great! :)

Re: [PATCH v3 3/4] rust: devres: get rid of Devres' inner Arc

[Boqun Feng]

On Thu, Jun 26, 2025 at 09:47:18PM +0200, Danilo Krummrich wrote:
> On Wed, Jun 25, 2025 at 09:13:24PM -0700, Boqun Feng wrote:
> > 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().

Well, what could happen is we create a `Arc<Devres<T>>` on one thread,
and because it's `Sync`, we can have a clone of it in another thread,
and if that's the last one call `Arc::drop()`, the `Revocable` will be
`revoke()`d in a different thread than it's created.

But with `T` always being `Send`, this is not a problem now.

Regards,
Boqun

> 
> > (Unless we want `T` always being `Send`
> > because of the issue I mentioned above)
> 
> Yes, we do.
> 
> > The rest looks good to me.
> 
> Great! :)

Re: [PATCH v3 4/4] rust: devres: implement register_release()

[Benno Lossin]

On Thu Jun 26, 2025 at 5:49 PM CEST, Danilo Krummrich wrote:
> On Thu, Jun 26, 2025 at 05:32:25PM +0200, Danilo Krummrich wrote:
>> On Thu, Jun 26, 2025 at 04:41:55PM +0200, Benno Lossin wrote:
>> > On Thu Jun 26, 2025 at 4:02 PM CEST, Danilo Krummrich wrote:
>> > > On Thu, Jun 26, 2025 at 01:15:34PM +0200, Danilo Krummrich wrote:
>> > >> On Thu, Jun 26, 2025 at 12:36:23PM +0200, Benno Lossin wrote:
>> > >> > 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.
>> > >> 
>> > >> I think this was also the case before? Well, it was P::Borrowed instead.
>> > >> 
>> > >> > 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.
>> > 
>> > 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.

Yeah `PointedTo` is used to specify the type of the foreign pointer and
should go away soon.

---
Cheers,
Benno