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

[Danilo Krummrich <dakr@kernel.org>]

On Sun, Jun 22, 2025 at 10:45:02PM +0200, Benno Lossin wrote:
> On Sun Jun 22, 2025 at 6:40 PM CEST, Danilo Krummrich wrote:
> > +impl<T> Inner<T> {
> > +    fn as_ptr(&self) -> *const Self {
> > +        self
> > +    }
> > +}
> 
> Instead of creating this function, you can use `ptr::from_ref`.

Yeah, I like this much better.

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

Indeed, forgot we finally have raw_ref_op since v6.15. :)

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

It is indeed -- I really like this one!

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

Makes sense, gonna add it.