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@kernel.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
diff --git a/MAINTAINERS b/MAINTAINERS
index 0cc69e282889..a9ae2d69f961 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7034,6 +7034,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 63f9b1da179f..a3b52aa021de 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c
@@ -12,6 +12,7 @@
#include "build_assert.c"
#include "build_bug.c"
#include "cred.c"
+#include "device.c"
#include "err.c"
#include "fs.c"
#include "io.c"
diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
new file mode 100644
index 000000000000..9c9dd39584eb
--- /dev/null
+++ b/rust/kernel/devres.rs
@@ -0,0 +1,178 @@
+// 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::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, IoRaw}};
+/// # use core::ops::Deref;
+///
+/// // See also [`pci::Bar`] for a real example.
+/// struct IoMem<const SIZE: usize>(IoRaw<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 as _) };
+/// if addr.is_null() {
+/// return Err(ENOMEM);
+/// }
+///
+/// Ok(IoMem(IoRaw::new(addr as _, SIZE)?))
+/// }
+/// }
+///
+/// impl<const SIZE: usize> Drop for IoMem<SIZE> {
+/// fn drop(&mut self) {
+/// // SAFETY: `self.0.addr()` is guaranteed to be properly mapped by `Self::new`.
+/// unsafe { bindings::iounmap(self.0.addr() as _); };
+/// }
+/// }
+///
+/// impl<const SIZE: usize> Deref for IoMem<SIZE> {
+/// type Target = Io<SIZE>;
+///
+/// fn deref(&self) -> &Self::Target {
+/// // SAFETY: The memory range stored in `self` has been properly mapped in `Self::new`.
+/// unsafe { Io::from_raw(&self.0) }
+/// }
+/// }
+/// # fn no_run() -> Result<(), Error> {
+/// # // SAFETY: Invalid usage; just for the example to get an `ARef<Device>` instance.
+/// # let dev = unsafe { Device::get_device(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 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()`.
+ //
+ // 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)]
--
2.47.1On Thu, 19 Dec 2024 18:04:10 +0100
Danilo Krummrich <dakr@kernel.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@kernel.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?
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)]
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@kernel.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@kernel.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?
>
> 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.
Better late than never, I now remember what's the *actual* race I was preventing
here:
| Task 0 | Task 1
--|----------------------------------------------------------------------------
1 | Devres::drop() { | devres_release_all() {
2 | DevresInner::remove_action() { | spin_lock_irqsave();
3 | | remove_nodes(&todo);
4 | | spin_unlock_irqrestore();
5 | devm_remove_action_nowarn(); |
6 | let _ = Arc::from_raw(); |
7 | } |
8 | } |
9 | | release_nodes(&todo);
10| | }
So, if devres_release_all() wins the race it stores the devres action within the
temporary todo list. Which means that in 9 we enter
DevresInner::devres_callback() even though DevresInner has been freed already.
Unfortunately, this race can happen with [1], but not with this original version
of Devres.
I see two ways to fix it:
1) Just revert [1] and stick with the original version.
2) Use devm_release_action() instead of devm_remove_action() and don't drop the
reference in DevresInner::remove_action() (6). This way the reference is
always dropped from the callback.
With 2) we still have an unnecessary call to revoke() if Devres is dropped
before the device is detached from the driver, but that's still better than
keeping DevresInner alive until the device is detached from the driver, which
the original version did. Hence, I'll go ahead and prepare a patch for this,
unless there are any concerns.
- Danilo
[1] https://lore.kernel.org/rust-for-linux/20250107122609.8135-2-dakr@kernel.org/
>
> > + //
> > + // 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)]
>
On Tue, Jan 14, 2025 at 05:33:45PM +0100, Danilo Krummrich wrote:
> > > +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?
> >
> > 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.
>
> Better late than never, I now remember what's the *actual* race I was preventing
> here:
>
> | Task 0 | Task 1
> --|----------------------------------------------------------------------------
> 1 | Devres::drop() { | devres_release_all() {
> 2 | DevresInner::remove_action() { | spin_lock_irqsave();
> 3 | | remove_nodes(&todo);
> 4 | | spin_unlock_irqrestore();
> 5 | devm_remove_action_nowarn(); |
> 6 | let _ = Arc::from_raw(); |
> 7 | } |
> 8 | } |
> 9 | | release_nodes(&todo);
> 10| | }
>
> So, if devres_release_all() wins the race it stores the devres action within the
> temporary todo list. Which means that in 9 we enter
> DevresInner::devres_callback() even though DevresInner has been freed already.
>
> Unfortunately, this race can happen with [1], but not with this original version
> of Devres.
Well, actually this can't happen, since obviously we know whether Devres:drop
removed it or whether it will be released by devres_release_all() and we only
free DevresInner conditionally.
So, the code in [1] is fine; I somehow managed to confuse myself.
Sorry for the noise.
- Danilo
>
> I see two ways to fix it:
>
> 1) Just revert [1] and stick with the original version.
>
> 2) Use devm_release_action() instead of devm_remove_action() and don't drop the
> reference in DevresInner::remove_action() (6). This way the reference is
> always dropped from the callback.
>
> With 2) we still have an unnecessary call to revoke() if Devres is dropped
> before the device is detached from the driver, but that's still better than
> keeping DevresInner alive until the device is detached from the driver, which
> the original version did. Hence, I'll go ahead and prepare a patch for this,
> unless there are any concerns.
>
> - Danilo
>
> [1] https://lore.kernel.org/rust-for-linux/20250107122609.8135-2-dakr@kernel.org/
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@kernel.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@kernel.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)]
>
On Thu, Jan 02, 2025 at 11:30:11AM +0100, Danilo Krummrich wrote:
> 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@kernel.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@kernel.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.
Ok, I double checked and this should indeed be the only issue.
I can reproduce the race and can confirm that a devm_remove_action_nowarn()
works, as long as we make it return the integer that indicates whether the
action has been removed already, such that we can handle it from the Rust side
accordingly.
Generally, I don't think it's a big deal, drivers usually hold the resources
they request anyways until remove(). But it's still an improvement, so I'll send
a patch for that.
>
> >
> > 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)]
> >
© 2016 - 2025 Red Hat, Inc.