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