Message-Id: <DATCT1EG0Z70.3ON7LFZEEZ93M@kernel.org>
Date: Sun, 22 Jun 2025 22:45:02 +0200
From: "Benno Lossin" <lossin@...nel.org>
To: "Danilo Krummrich" <dakr@...nel.org>, <gregkh@...uxfoundation.org>,
 <rafael@...nel.org>, <ojeda@...nel.org>, <alex.gaynor@...il.com>,
 <boqun.feng@...il.com>, <gary@...yguo.net>, <bjorn3_gh@...tonmail.com>,
 <a.hindborg@...nel.org>, <aliceryhl@...gle.com>, <tmgross@...ch.edu>,
 <david.m.ertman@...el.com>, <ira.weiny@...el.com>, <leon@...nel.org>,
 <kwilczynski@...nel.org>, <bhelgaas@...gle.com>
Cc: <rust-for-linux@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
 <linux-pci@...r.kernel.org>
Subject: Re: [PATCH v2 3/4] rust: devres: get rid of Devres' inner Arc

On Sun Jun 22, 2025 at 6:40 PM CEST, Danilo Krummrich wrote:
> diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
> index 250073749279..15a0a94e992b 100644
> --- a/rust/kernel/devres.rs
> +++ b/rust/kernel/devres.rs
> @@ -13,20 +13,31 @@
>      ffi::c_void,
>      prelude::*,
>      revocable::{Revocable, RevocableGuard},
> -    sync::{rcu, Arc, Completion},
> -    types::{ARef, ForeignOwnable},
> +    sync::{rcu, Completion},
> +    types::{ARef, ForeignOwnable, 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,
>  }
>  
> +impl<T> Inner<T> {
> +    fn as_ptr(&self) -> *const Self {
> +        self
> +    }
> +}

Instead of creating this function, you can use `ptr::from_ref`.

> +
>  /// This abstraction is meant to be used by subsystems to containerize [`Device`] bound resources to
>  /// manage their lifetime.
>  ///
> @@ -88,100 +99,106 @@ 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> 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,
> +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;
> +
> +        try_pin_init!(&this in Self {
> +            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: It is valid to dereference `this` to find the address of `inner`.

    // SAFETY: `this` is a valid pointer to uninitialized memory.

> +                let inner = unsafe { core::ptr::addr_of_mut!((*this.as_ptr()).inner) };

We can use `raw` instead of `addr_of_mut!`:

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

Just wanted to highlight that this is a very cool application of the
borrow checker :)

> +                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: `inner` is valid and properly initialized.

We should have an invariant here that `inner` is always accessed
read-only and that it is initialized.

---
Cheers,
Benno

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

Powered by blists - more mailing lists