Message-ID: <38d708b5-7a73-4736-909c-eefd3c938769@kernel.org>
Date: Wed, 27 Nov 2024 14:19:02 +0100
From: Danilo Krummrich <dakr@...nel.org>
To: Alice Ryhl <aliceryhl@...gle.com>
Cc: gregkh@...uxfoundation.org, rafael@...nel.org, bhelgaas@...gle.com,
 ojeda@...nel.org, alex.gaynor@...il.com, boqun.feng@...il.com,
 gary@...yguo.net, bjorn3_gh@...tonmail.com, benno.lossin@...ton.me,
 tmgross@...ch.edu, a.hindborg@...sung.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,
 rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-pci@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH v3 10/16] rust: add devres abstraction

On 11/27/24 1:21 PM, Alice Ryhl wrote:
> On Tue, Oct 22, 2024 at 11:33 PM 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  | 180 +++++++++++++++++++++++++++++++++++++++++
>>   rust/kernel/lib.rs     |   1 +
>>   5 files changed, 193 insertions(+)
>>   create mode 100644 rust/helpers/device.c
>>   create mode 100644 rust/kernel/devres.rs
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 0a8882252257..97914d0752fb 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -6983,6 +6983,7 @@ F:        include/linux/property.h
>>   F:     lib/kobj*
>>   F:     rust/kernel/device.rs
>>   F:     rust/kernel/device_id.rs
>> +F:     rust/kernel/devres.rs
>>   F:     rust/kernel/driver.rs
>>
>>   DRIVERS FOR OMAP ADAPTIVE VOLTAGE SCALING (AVS)
>> diff --git a/rust/helpers/device.c b/rust/helpers/device.c
>> new file mode 100644
>> index 000000000000..b2135c6686b0
>> --- /dev/null
>> +++ b/rust/helpers/device.c
>> @@ -0,0 +1,10 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +#include <linux/device.h>
>> +
>> +int rust_helper_devm_add_action(struct device *dev,
>> +                               void (*action)(void *),
>> +                               void *data)
>> +{
>> +       return devm_add_action(dev, action, data);
>> +}
>> diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
>> index e2f6b2197061..3acb2b9e52ec 100644
>> --- a/rust/helpers/helpers.c
>> +++ b/rust/helpers/helpers.c
>> @@ -11,6 +11,7 @@
>>   #include "bug.c"
>>   #include "build_assert.c"
>>   #include "build_bug.c"
>> +#include "device.c"
>>   #include "err.c"
>>   #include "io.c"
>>   #include "kunit.c"
>> diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
>> new file mode 100644
>> index 000000000000..b23559f55214
>> --- /dev/null
>> +++ b/rust/kernel/devres.rs
>> @@ -0,0 +1,180 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +//! Devres abstraction
>> +//!
>> +//! [`Devres`] represents an abstraction for the kernel devres (device resource management)
>> +//! implementation.
>> +
>> +use crate::{
>> +    alloc::Flags,
>> +    bindings,
>> +    device::Device,
>> +    error::{Error, Result},
>> +    prelude::*,
>> +    revocable::Revocable,
>> +    sync::Arc,
>> +};
>> +
>> +use core::ffi::c_void;
>> +use core::ops::Deref;
>> +
>> +#[pin_data]
>> +struct DevresInner<T> {
>> +    #[pin]
>> +    data: Revocable<T>,
>> +}
>> +
>> +/// This abstraction is meant to be used by subsystems to containerize [`Device`] bound resources to
>> +/// manage their lifetime.
>> +///
>> +/// [`Device`] bound resources should be freed when either the resource goes out of scope or the
>> +/// [`Device`] is unbound respectively, depending on what happens first.
>> +///
>> +/// To achieve that [`Devres`] registers a devres callback on creation, which is called once the
>> +/// [`Device`] is unbound, revoking access to the encapsulated resource (see also [`Revocable`]).
>> +///
>> +/// After the [`Devres`] has been unbound it is not possible to access the encapsulated resource
>> +/// anymore.
>> +///
>> +/// [`Devres`] users should make sure to simply free the corresponding backing resource in `T`'s
>> +/// [`Drop`] implementation.
>> +///
>> +/// # Example
>> +///
>> +/// ```no_run
>> +/// # use kernel::{bindings, c_str, device::Device, devres::Devres, io::Io};
>> +/// # use core::ops::Deref;
>> +///
>> +/// // See also [`pci::Bar`] for a real example.
>> +/// struct IoMem<const SIZE: usize>(Io<SIZE>);
>> +///
>> +/// impl<const SIZE: usize> IoMem<SIZE> {
>> +///     /// # Safety
>> +///     ///
>> +///     /// [`paddr`, `paddr` + `SIZE`) must be a valid MMIO region that is mappable into the CPUs
>> +///     /// virtual address space.
>> +///     unsafe fn new(paddr: usize) -> Result<Self>{
>> +///
>> +///         // SAFETY: By the safety requirements of this function [`paddr`, `paddr` + `SIZE`) is
>> +///         // valid for `ioremap`.
>> +///         let addr = unsafe { bindings::ioremap(paddr as _, SIZE.try_into().unwrap()) };
>> +///         if addr.is_null() {
>> +///             return Err(ENOMEM);
>> +///         }
>> +///
>> +///         // SAFETY: `addr` is guaranteed to be the start of a valid I/O mapped memory region of
>> +///         // size `SIZE`.
>> +///         let io = unsafe { Io::new(addr as _, SIZE)? };
>> +///
>> +///         Ok(IoMem(io))
>> +///     }
>> +/// }
>> +///
>> +/// impl<const SIZE: usize> Drop for IoMem<SIZE> {
>> +///     fn drop(&mut self) {
>> +///         // SAFETY: Safe as by the invariant of `Io`.
>> +///         unsafe { bindings::iounmap(self.0.base_addr() as _); };
>> +///     }
>> +/// }
>> +///
>> +/// impl<const SIZE: usize> Deref for IoMem<SIZE> {
>> +///    type Target = Io<SIZE>;
>> +///
>> +///    fn deref(&self) -> &Self::Target {
>> +///        &self.0
>> +///    }
>> +/// }
>> +///
>> +/// # fn no_run() -> Result<(), Error> {
>> +/// # // SAFETY: Invalid usage; just for the example to get an `ARef<Device>` instance.
>> +/// # let dev = unsafe { Device::from_raw(core::ptr::null_mut()) };
>> +///
>> +/// // 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 res = devres.try_access().ok_or(ENXIO)?;
>> +/// res.writel(0x42, 0x0);
>> +/// # Ok(())
>> +/// # }
>> +/// ```
>> +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 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()`.
>> +        self.revoke();
> 
> When the destructor runs, it's guaranteed that nobody is accessing the
> inner resource since the only way to do that is through the Devres
> handle, but its destructor is running. Therefore, you can skip the
> synchronize_rcu() call in this case.

Yeah, I think this optimization should be possible.

We'd require `Revocable` to have a `revoke_nosync` method for that I guess...

> 
> Alice
> 

Powered by blists - more mailing lists