Message-ID: <Z3Zqq5qLht2j9Qqq@cassiopeiae>
Date: Thu, 2 Jan 2025 11:30:03 +0100
From: Danilo Krummrich <dakr@...nel.org>
To: Gary Guo <gary@...yguo.net>
Cc: gregkh@...uxfoundation.org, rafael@...nel.org, bhelgaas@...gle.com,
	ojeda@...nel.org, alex.gaynor@...il.com, boqun.feng@...il.com,
	bjorn3_gh@...tonmail.com, benno.lossin@...ton.me, tmgross@...ch.edu,
	a.hindborg@...sung.com, aliceryhl@...gle.com, airlied@...il.com,
	fujita.tomonori@...il.com, lina@...hilina.net, pstanner@...hat.com,
	ajanulgu@...hat.com, lyude@...hat.com, robh@...nel.org,
	daniel.almeida@...labora.com, saravanak@...gle.com,
	dirk.behme@...bosch.com, j@...nau.net, fabien.parent@...aro.org,
	chrisi.schrefl@...il.com, paulmck@...nel.org,
	rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-pci@...r.kernel.org, devicetree@...r.kernel.org,
	rcu@...r.kernel.org
Subject: Re: [PATCH v7 08/16] rust: add devres abstraction

On Tue, Dec 24, 2024 at 09:53:23PM +0000, Gary Guo wrote:
> On Thu, 19 Dec 2024 18:04:10 +0100
> Danilo Krummrich <dakr@...nel.org> wrote:
> 
> > Add a Rust abstraction for the kernel's devres (device resource
> > management) implementation.
> > 
> > The Devres type acts as a container to manage the lifetime and
> > accessibility of device bound resources. Therefore it registers a
> > devres callback and revokes access to the resource on invocation.
> > 
> > Users of the Devres abstraction can simply free the corresponding
> > resources in their Drop implementation, which is invoked when either the
> > Devres instance goes out of scope or the devres callback leads to the
> > resource being revoked, which implies a call to drop_in_place().
> > 
> > Signed-off-by: Danilo Krummrich <dakr@...nel.org>
> > ---
> >  MAINTAINERS            |   1 +
> >  rust/helpers/device.c  |  10 +++
> >  rust/helpers/helpers.c |   1 +
> >  rust/kernel/devres.rs  | 178 +++++++++++++++++++++++++++++++++++++++++
> >  rust/kernel/lib.rs     |   1 +
> >  5 files changed, 191 insertions(+)
> >  create mode 100644 rust/helpers/device.c
> >  create mode 100644 rust/kernel/devres.rs
> > 
> > <snip>
> >
> > +pub struct Devres<T>(Arc<DevresInner<T>>);
> > +
> > +impl<T> DevresInner<T> {
> > +    fn new(dev: &Device, data: T, flags: Flags) -> Result<Arc<DevresInner<T>>> {
> > +        let inner = Arc::pin_init(
> > +            pin_init!( DevresInner {
> > +                data <- Revocable::new(data),
> > +            }),
> > +            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();
> > +
> > +        // 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(Self::devres_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));
> > +        }
> > +
> > +        Ok(inner)
> > +    }
> > +
> > +    #[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) };
> > +
> > +        inner.data.revoke();
> > +    }
> > +}
> > +
> > +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, data: T, flags: Flags) -> Result<Self> {
> > +        let inner = DevresInner::new(dev, data, flags)?;
> > +
> > +        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, data: T, flags: Flags) -> Result {
> > +        let _ = DevresInner::new(dev, data, flags)?;
> > +
> > +        Ok(())
> > +    }
> > +}
> > +
> > +impl<T> Deref for Devres<T> {
> > +    type Target = Revocable<T>;
> > +
> > +    fn deref(&self) -> &Self::Target {
> > +        &self.0.data
> > +    }
> > +}
> > +
> > +impl<T> Drop for Devres<T> {
> > +    fn drop(&mut self) {
> > +        // Revoke the data, such that it gets dropped already and the actual resource is freed.
> > +        //
> > +        // `DevresInner` has to stay alive until the devres callback has been called. This is
> > +        // necessary since we don't know when `Devres` is dropped and calling
> > +        // `devm_remove_action()` instead could race with `devres_release_all()`.
> 
> IIUC, the outcome of that race is the `WARN` if
> devres_release_all takes the spinlock first and has already remvoed the
> action?

Yes, this was one issue. But I think there was another when you have a class
`Registration` that is owned by a `Devres`, which holds private data that is
encapsulated in a `Devres` too.

I have this case in Nova where the DRM device' private data holds the PCI bar,
and the DRM device registration has a reference of the corresponding DRM device.

But maybe this also was something else. I will double check and if I can confirm
that the WARN_ON() in devm_remove_action() is the only issue, we can certainly
change this.

> 
> Could you do a custom devres_release here that mimick
> `devm_remove_action` but omit the `WARN`? This way it allows the memory
> behind DevresInner to be freed early without keeping it allocated until
> the end of device lifetime.
> 
> > +        //
> > +        // SAFETY: When `drop` runs, it's guaranteed that nobody is accessing the revocable data
> > +        // anymore, hence it is safe not to wait for the grace period to finish.
> > +        unsafe { self.revoke_nosync() };
> > +    }
> > +}
> > diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> > index 6c836ab73771..2b61bf99d1ee 100644
> > --- a/rust/kernel/lib.rs
> > +++ b/rust/kernel/lib.rs
> > @@ -41,6 +41,7 @@
> >  pub mod cred;
> >  pub mod device;
> >  pub mod device_id;
> > +pub mod devres;
> >  pub mod driver;
> >  pub mod error;
> >  #[cfg(CONFIG_RUST_FW_LOADER_ABSTRACTIONS)]
> 

Powered by blists - more mailing lists