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.
