Replace Devres::new_foreign_owned() with
devres::register_foreign_boxed().
The current implementation of Devres::new_foreign_owned() creates a full
Devres container instance, including the internal Revocable and
completion.
However, none of that is necessary for the intended use of giving full
ownership of an object to devres and getting it dropped once the given
device is unbound.
Hence, implement devres::register_foreign_boxed(), which is limited to
consume the given data, wrap it in a KBox and drop the KBox once the
given device is unbound, without any other synchronization.
Cc: Dave Airlie <airlied@redhat.com>
Cc: Simona Vetter <simona.vetter@ffwll.ch>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
rust/helpers/device.c | 7 ++++
rust/kernel/cpufreq.rs | 8 ++---
rust/kernel/devres.rs | 73 +++++++++++++++++++++++++++++++++------
rust/kernel/drm/driver.rs | 11 +++---
4 files changed, 79 insertions(+), 20 deletions(-)
diff --git a/rust/helpers/device.c b/rust/helpers/device.c
index b2135c6686b0..502fef7e9ae8 100644
--- a/rust/helpers/device.c
+++ b/rust/helpers/device.c
@@ -8,3 +8,10 @@ int rust_helper_devm_add_action(struct device *dev,
{
return devm_add_action(dev, action, data);
}
+
+int rust_helper_devm_add_action_or_reset(struct device *dev,
+ void (*action)(void *),
+ void *data)
+{
+ return devm_add_action_or_reset(dev, action, data);
+}
diff --git a/rust/kernel/cpufreq.rs b/rust/kernel/cpufreq.rs
index b0a9c6182aec..f20636079f7a 100644
--- a/rust/kernel/cpufreq.rs
+++ b/rust/kernel/cpufreq.rs
@@ -12,7 +12,7 @@
clk::Hertz,
cpumask,
device::{Bound, Device},
- devres::Devres,
+ devres,
error::{code::*, from_err_ptr, from_result, to_result, Result, VTABLE_DEFAULT_ERROR},
ffi::{c_char, c_ulong},
prelude::*,
@@ -910,7 +910,7 @@ unsafe impl<T: Driver> Sync for Registration<T> {}
/// thread.
unsafe impl<T: Driver> Send for Registration<T> {}
-impl<T: Driver> Registration<T> {
+impl<T: Driver + 'static> Registration<T> {
const VTABLE: bindings::cpufreq_driver = bindings::cpufreq_driver {
name: Self::copy_name(T::NAME),
boost_enabled: T::BOOST_ENABLED,
@@ -1044,10 +1044,10 @@ pub fn new() -> Result<Self> {
/// Same as [`Registration::new`], but does not return a [`Registration`] instance.
///
- /// Instead the [`Registration`] is owned by [`Devres`] and will be revoked / dropped, once the
+ /// Instead the [`Registration`] is owned by [`kernel::devres`] and will be dropped, once the
/// device is detached.
pub fn new_foreign_owned(dev: &Device<Bound>) -> Result {
- Devres::new_foreign_owned(dev, Self::new()?, GFP_KERNEL)
+ devres::register_foreign_boxed(dev, Self::new()?, GFP_KERNEL)
}
}
diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
index a7df9fbd724f..04435e810249 100644
--- a/rust/kernel/devres.rs
+++ b/rust/kernel/devres.rs
@@ -9,12 +9,12 @@
alloc::Flags,
bindings,
device::{Bound, Device},
- error::{Error, Result},
+ error::{to_result, Error, Result},
ffi::c_void,
prelude::*,
revocable::{Revocable, RevocableGuard},
sync::{rcu, Arc, Completion},
- types::ARef,
+ types::{ARef, ForeignOwnable},
};
#[pin_data]
@@ -182,14 +182,6 @@ pub fn new(dev: &Device<Bound>, data: T, flags: Flags) -> Result<Self> {
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<Bound>, data: T, flags: Flags) -> Result {
- let _ = DevresInner::new(dev, data, flags)?;
-
- Ok(())
- }
-
/// Obtain `&'a T`, bypassing the [`Revocable`].
///
/// This method allows to directly obtain a `&'a T`, bypassing the [`Revocable`], by presenting
@@ -259,3 +251,64 @@ fn drop(&mut self) {
}
}
}
+
+/// Consume `data` and [`Drop::drop`] `data` once `dev` is unbound.
+fn register_foreign<P: ForeignOwnable>(dev: &Device<Bound>, data: P) -> Result {
+ let ptr = data.into_foreign();
+
+ #[allow(clippy::missing_safety_doc)]
+ unsafe extern "C" fn callback<P: ForeignOwnable>(ptr: *mut kernel::ffi::c_void) {
+ // SAFETY: `ptr` is the pointer to the `ForeignOwnable` leaked above and hence valid.
+ let _ = unsafe { P::from_foreign(ptr.cast()) };
+ }
+
+ // SAFETY:
+ // - `dev.as_raw()` is a pointer to a valid and bound device.
+ // - `ptr` is a valid pointer the `ForeignOwnable` devres takes ownership of.
+ to_result(unsafe {
+ // `devm_add_action_or_reset()` also calls `callback` on failure, such that the
+ // `ForeignOwnable` is released eventually.
+ bindings::devm_add_action_or_reset(dev.as_raw(), Some(callback::<P>), ptr.cast())
+ })
+}
+
+/// Encapsulate `data` in a [`KBox`] and [`Drop::drop`] `data` once `dev` is unbound.
+///
+/// # Examples
+///
+/// ```no_run
+/// use kernel::{device::{Bound, Device}, devres};
+///
+/// struct Registration;
+///
+/// impl Registration {
+/// fn new() -> Self {
+/// // register (e.g. class device, IRQ, etc.)
+///
+/// Self
+/// }
+/// }
+///
+/// impl Drop for Registration {
+/// fn drop(&mut self) {
+/// // unregister
+/// }
+/// }
+///
+/// fn from_bound_context(dev: &Device<Bound>) -> Result {
+/// devres::register_foreign_boxed(dev, Registration::new(), GFP_KERNEL)
+/// }
+/// ```
+pub fn register_foreign_boxed<T, E>(
+ dev: &Device<Bound>,
+ data: impl PinInit<T, E>,
+ flags: Flags,
+) -> Result
+where
+ T: 'static,
+ Error: From<E>,
+{
+ let data = KBox::pin_init(data, flags)?;
+
+ register_foreign(dev, data)
+}
diff --git a/rust/kernel/drm/driver.rs b/rust/kernel/drm/driver.rs
index acb638086131..3b0cb80c1984 100644
--- a/rust/kernel/drm/driver.rs
+++ b/rust/kernel/drm/driver.rs
@@ -5,9 +5,7 @@
//! C header: [`include/linux/drm/drm_drv.h`](srctree/include/linux/drm/drm_drv.h)
use crate::{
- bindings, device,
- devres::Devres,
- drm,
+ bindings, device, devres, drm,
error::{to_result, Result},
prelude::*,
str::CStr,
@@ -120,7 +118,7 @@ pub trait Driver {
/// Once the `Registration` structure is dropped, the device is unregistered.
pub struct Registration<T: Driver>(ARef<drm::Device<T>>);
-impl<T: Driver> Registration<T> {
+impl<T: Driver + 'static> Registration<T> {
/// Creates a new [`Registration`] and registers it.
fn new(drm: &drm::Device<T>, flags: usize) -> Result<Self> {
// SAFETY: `drm.as_raw()` is valid by the invariants of `drm::Device`.
@@ -130,7 +128,7 @@ fn new(drm: &drm::Device<T>, flags: usize) -> Result<Self> {
}
/// Same as [`Registration::new`}, but transfers ownership of the [`Registration`] to
- /// [`Devres`].
+ /// [`devres::register_foreign_boxed`].
pub fn new_foreign_owned(
drm: &drm::Device<T>,
dev: &device::Device<device::Bound>,
@@ -141,7 +139,8 @@ pub fn new_foreign_owned(
}
let reg = Registration::<T>::new(drm, flags)?;
- Devres::new_foreign_owned(dev, reg, GFP_KERNEL)
+
+ devres::register_foreign_boxed(dev, reg, GFP_KERNEL)
}
/// Returns a reference to the `Device` instance for this registration.
--
2.49.0On Thu Jun 12, 2025 at 4:51 PM CEST, Danilo Krummrich wrote:
> diff --git a/rust/kernel/cpufreq.rs b/rust/kernel/cpufreq.rs
> index b0a9c6182aec..f20636079f7a 100644
> --- a/rust/kernel/cpufreq.rs
> +++ b/rust/kernel/cpufreq.rs
> @@ -12,7 +12,7 @@
> clk::Hertz,
> cpumask,
> device::{Bound, Device},
> - devres::Devres,
> + devres,
> error::{code::*, from_err_ptr, from_result, to_result, Result, VTABLE_DEFAULT_ERROR},
> ffi::{c_char, c_ulong},
> prelude::*,
> @@ -910,7 +910,7 @@ unsafe impl<T: Driver> Sync for Registration<T> {}
> /// thread.
> unsafe impl<T: Driver> Send for Registration<T> {}
>
> -impl<T: Driver> Registration<T> {
> +impl<T: Driver + 'static> Registration<T> {
This change should probably be its own patch? If not, then it should be
mentioned in the commit message.
> const VTABLE: bindings::cpufreq_driver = bindings::cpufreq_driver {
> name: Self::copy_name(T::NAME),
> boost_enabled: T::BOOST_ENABLED,
> @@ -1044,10 +1044,10 @@ pub fn new() -> Result<Self> {
>
> /// Same as [`Registration::new`], but does not return a [`Registration`] instance.
> ///
> - /// Instead the [`Registration`] is owned by [`Devres`] and will be revoked / dropped, once the
> + /// Instead the [`Registration`] is owned by [`kernel::devres`] and will be dropped, once the
> /// device is detached.
> pub fn new_foreign_owned(dev: &Device<Bound>) -> Result {
I think we can improve the names here. How about `new_attached`? See
more below.
> - Devres::new_foreign_owned(dev, Self::new()?, GFP_KERNEL)
> + devres::register_foreign_boxed(dev, Self::new()?, GFP_KERNEL)
> }
> }
> +/// Encapsulate `data` in a [`KBox`] and [`Drop::drop`] `data` once `dev` is unbound.
> +///
> +/// # Examples
> +///
> +/// ```no_run
> +/// use kernel::{device::{Bound, Device}, devres};
> +///
> +/// struct Registration;
> +///
> +/// impl Registration {
> +/// fn new() -> Self {
> +/// // register (e.g. class device, IRQ, etc.)
> +///
> +/// Self
> +/// }
> +/// }
> +///
> +/// impl Drop for Registration {
> +/// fn drop(&mut self) {
> +/// // unregister
> +/// }
> +/// }
> +///
> +/// fn from_bound_context(dev: &Device<Bound>) -> Result {
> +/// devres::register_foreign_boxed(dev, Registration::new(), GFP_KERNEL)
> +/// }
> +/// ```
> +pub fn register_foreign_boxed<T, E>(
I don't really get the name of this function. The data isn't really
foreign and that the user also shouldn't really care about the fact that
you use `KBox` under the hood.
How about we call this something like `attach_data`?
---
Cheers,
Benno
> + dev: &Device<Bound>,
> + data: impl PinInit<T, E>,
> + flags: Flags,
> +) -> Result
> +where
> + T: 'static,
> + Error: From<E>,
> +{
> + let data = KBox::pin_init(data, flags)?;
> +
> + register_foreign(dev, data)
> +}
On Sat, Jun 21, 2025 at 11:10:14PM +0200, Benno Lossin wrote:
> On Thu Jun 12, 2025 at 4:51 PM CEST, Danilo Krummrich wrote:
> > diff --git a/rust/kernel/cpufreq.rs b/rust/kernel/cpufreq.rs
> > index b0a9c6182aec..f20636079f7a 100644
> > --- a/rust/kernel/cpufreq.rs
> > +++ b/rust/kernel/cpufreq.rs
> > @@ -12,7 +12,7 @@
> > clk::Hertz,
> > cpumask,
> > device::{Bound, Device},
> > - devres::Devres,
> > + devres,
> > error::{code::*, from_err_ptr, from_result, to_result, Result, VTABLE_DEFAULT_ERROR},
> > ffi::{c_char, c_ulong},
> > prelude::*,
> > @@ -910,7 +910,7 @@ unsafe impl<T: Driver> Sync for Registration<T> {}
> > /// thread.
> > unsafe impl<T: Driver> Send for Registration<T> {}
> >
> > -impl<T: Driver> Registration<T> {
> > +impl<T: Driver + 'static> Registration<T> {
>
> This change should probably be its own patch? If not, then it should be
> mentioned in the commit message.
It's a consequence of register_foreign_boxed() requiring T: 'static.
> > const VTABLE: bindings::cpufreq_driver = bindings::cpufreq_driver {
> > name: Self::copy_name(T::NAME),
> > boost_enabled: T::BOOST_ENABLED,
> > @@ -1044,10 +1044,10 @@ pub fn new() -> Result<Self> {
> >
> > /// Same as [`Registration::new`], but does not return a [`Registration`] instance.
> > ///
> > - /// Instead the [`Registration`] is owned by [`Devres`] and will be revoked / dropped, once the
> > + /// Instead the [`Registration`] is owned by [`kernel::devres`] and will be dropped, once the
> > /// device is detached.
> > pub fn new_foreign_owned(dev: &Device<Bound>) -> Result {
>
> I think we can improve the names here. How about `new_attached`? See
> more below.
I feel like the name pretty much nails it: it's a new instance that is not
owned, by the Rust side, but by the C devres implementation (i.e. foreign
owned), which automatically drops it when the device is unbound.
Maybe Registration::new_devres_owned() instead?
> > - Devres::new_foreign_owned(dev, Self::new()?, GFP_KERNEL)
> > + devres::register_foreign_boxed(dev, Self::new()?, GFP_KERNEL)
> > }
> > }
>
> > +/// Encapsulate `data` in a [`KBox`] and [`Drop::drop`] `data` once `dev` is unbound.
> > +///
> > +/// # Examples
> > +///
> > +/// ```no_run
> > +/// use kernel::{device::{Bound, Device}, devres};
> > +///
> > +/// struct Registration;
> > +///
> > +/// impl Registration {
> > +/// fn new() -> Self {
> > +/// // register (e.g. class device, IRQ, etc.)
> > +///
> > +/// Self
> > +/// }
> > +/// }
> > +///
> > +/// impl Drop for Registration {
> > +/// fn drop(&mut self) {
> > +/// // unregister
> > +/// }
> > +/// }
> > +///
> > +/// fn from_bound_context(dev: &Device<Bound>) -> Result {
> > +/// devres::register_foreign_boxed(dev, Registration::new(), GFP_KERNEL)
> > +/// }
> > +/// ```
> > +pub fn register_foreign_boxed<T, E>(
>
> I don't really get the name of this function. The data isn't really
> foreign and that the user also shouldn't really care about the fact that
> you use `KBox` under the hood.
>
> How about we call this something like `attach_data`?
Hm, I think attach_data() doesn't quite hit the point. Maybe just
devres::register_owned() instead. I agree that 'boxed' is an unnecessary
implementation detail.
> ---
> Cheers,
> Benno
>
> > + dev: &Device<Bound>,
> > + data: impl PinInit<T, E>,
> > + flags: Flags,
> > +) -> Result
> > +where
> > + T: 'static,
> > + Error: From<E>,
> > +{
> > + let data = KBox::pin_init(data, flags)?;
> > +
> > + register_foreign(dev, data)
> > +}
On Sat Jun 21, 2025 at 11:45 PM CEST, Danilo Krummrich wrote:
> On Sat, Jun 21, 2025 at 11:10:14PM +0200, Benno Lossin wrote:
>> On Thu Jun 12, 2025 at 4:51 PM CEST, Danilo Krummrich wrote:
>> > diff --git a/rust/kernel/cpufreq.rs b/rust/kernel/cpufreq.rs
>> > index b0a9c6182aec..f20636079f7a 100644
>> > --- a/rust/kernel/cpufreq.rs
>> > +++ b/rust/kernel/cpufreq.rs
>> > @@ -12,7 +12,7 @@
>> > clk::Hertz,
>> > cpumask,
>> > device::{Bound, Device},
>> > - devres::Devres,
>> > + devres,
>> > error::{code::*, from_err_ptr, from_result, to_result, Result, VTABLE_DEFAULT_ERROR},
>> > ffi::{c_char, c_ulong},
>> > prelude::*,
>> > @@ -910,7 +910,7 @@ unsafe impl<T: Driver> Sync for Registration<T> {}
>> > /// thread.
>> > unsafe impl<T: Driver> Send for Registration<T> {}
>> >
>> > -impl<T: Driver> Registration<T> {
>> > +impl<T: Driver + 'static> Registration<T> {
>>
>> This change should probably be its own patch? If not, then it should be
>> mentioned in the commit message.
>
> It's a consequence of register_foreign_boxed() requiring T: 'static.
Then let's put the bound on that function, since putting it on the impl
block also affects `Registration::new()`.
>> > const VTABLE: bindings::cpufreq_driver = bindings::cpufreq_driver {
>> > name: Self::copy_name(T::NAME),
>> > boost_enabled: T::BOOST_ENABLED,
>> > @@ -1044,10 +1044,10 @@ pub fn new() -> Result<Self> {
>> >
>> > /// Same as [`Registration::new`], but does not return a [`Registration`] instance.
>> > ///
>> > - /// Instead the [`Registration`] is owned by [`Devres`] and will be revoked / dropped, once the
>> > + /// Instead the [`Registration`] is owned by [`kernel::devres`] and will be dropped, once the
>> > /// device is detached.
>> > pub fn new_foreign_owned(dev: &Device<Bound>) -> Result {
>>
>> I think we can improve the names here. How about `new_attached`? See
>> more below.
>
> I feel like the name pretty much nails it: it's a new instance that is not
> owned, by the Rust side, but by the C devres implementation (i.e. foreign
> owned), which automatically drops it when the device is unbound.
Yeah, but `foreign` is so unspecific... With `ForeignOwnable`, it makes
sense, since it could be anything.
> Maybe Registration::new_devres_owned() instead?
I like that one better, let's go with that.
>> > - Devres::new_foreign_owned(dev, Self::new()?, GFP_KERNEL)
>> > + devres::register_foreign_boxed(dev, Self::new()?, GFP_KERNEL)
>> > }
>> > }
>>
>> > +/// Encapsulate `data` in a [`KBox`] and [`Drop::drop`] `data` once `dev` is unbound.
>> > +///
>> > +/// # Examples
>> > +///
>> > +/// ```no_run
>> > +/// use kernel::{device::{Bound, Device}, devres};
>> > +///
>> > +/// struct Registration;
>> > +///
>> > +/// impl Registration {
>> > +/// fn new() -> Self {
>> > +/// // register (e.g. class device, IRQ, etc.)
>> > +///
>> > +/// Self
>> > +/// }
>> > +/// }
>> > +///
>> > +/// impl Drop for Registration {
>> > +/// fn drop(&mut self) {
>> > +/// // unregister
>> > +/// }
>> > +/// }
>> > +///
>> > +/// fn from_bound_context(dev: &Device<Bound>) -> Result {
>> > +/// devres::register_foreign_boxed(dev, Registration::new(), GFP_KERNEL)
>> > +/// }
>> > +/// ```
>> > +pub fn register_foreign_boxed<T, E>(
>>
>> I don't really get the name of this function. The data isn't really
>> foreign and that the user also shouldn't really care about the fact that
>> you use `KBox` under the hood.
>>
>> How about we call this something like `attach_data`?
>
> Hm, I think attach_data() doesn't quite hit the point. Maybe just
> devres::register_owned() instead. I agree that 'boxed' is an unnecessary
> implementation detail.
I like `register_owned` better, but I'm not 100% convinced by the
`owned` part... The regular devres creation is called `Devres::new`,
right? How about we just call this `register`?
---
Cheers,
Benno
On Sun, Jun 22, 2025 at 09:42:03AM +0200, Benno Lossin wrote: > On Sat Jun 21, 2025 at 11:45 PM CEST, Danilo Krummrich wrote: > > I feel like the name pretty much nails it: it's a new instance that is not > > owned, by the Rust side, but by the C devres implementation (i.e. foreign > > owned), which automatically drops it when the device is unbound. > > Yeah, but `foreign` is so unspecific... With `ForeignOwnable`, it makes > sense, since it could be anything. > > > Maybe Registration::new_devres_owned() instead? > > I like that one better, let's go with that. SGTM, but please note that this is unrelated to this patch; will create an issue for renaming those. > > Hm, I think attach_data() doesn't quite hit the point. Maybe just > > devres::register_owned() instead. I agree that 'boxed' is an unnecessary > > implementation detail. > > I like `register_owned` better, but I'm not 100% convinced by the > `owned` part... The regular devres creation is called `Devres::new`, > right? How about we just call this `register`? In general, devres::register() is fine for me too. But note that it looses a bit the indicator that the ownership of the object is entirely transferred to devres, in contrast to the Devres container type.
On Sun Jun 22, 2025 at 11:55 AM CEST, Danilo Krummrich wrote: > On Sun, Jun 22, 2025 at 09:42:03AM +0200, Benno Lossin wrote: >> On Sat Jun 21, 2025 at 11:45 PM CEST, Danilo Krummrich wrote: >> > I feel like the name pretty much nails it: it's a new instance that is not >> > owned, by the Rust side, but by the C devres implementation (i.e. foreign >> > owned), which automatically drops it when the device is unbound. >> >> Yeah, but `foreign` is so unspecific... With `ForeignOwnable`, it makes >> sense, since it could be anything. >> >> > Maybe Registration::new_devres_owned() instead? >> >> I like that one better, let's go with that. > > SGTM, but please note that this is unrelated to this patch; will create an issue > for renaming those. SGTM. >> > Hm, I think attach_data() doesn't quite hit the point. Maybe just >> > devres::register_owned() instead. I agree that 'boxed' is an unnecessary >> > implementation detail. >> >> I like `register_owned` better, but I'm not 100% convinced by the >> `owned` part... The regular devres creation is called `Devres::new`, >> right? How about we just call this `register`? > > In general, devres::register() is fine for me too. But note that it looses a bit > the indicator that the ownership of the object is entirely transferred to > devres, in contrast to the Devres container type. I'd say that is clear from the function signature & can be expanded upon in the docs. `register_owned` doesn't really carry the meaning "I take ownership of what you give me", so I don't think we lose anything here. (if we have a `register_owned`, then it begs the question what `register` would be... which doesn't make sense to exist IMO) --- Cheers, Benno
On 12-06-25, 16:51, Danilo Krummrich wrote: > Replace Devres::new_foreign_owned() with > devres::register_foreign_boxed(). > > The current implementation of Devres::new_foreign_owned() creates a full > Devres container instance, including the internal Revocable and > completion. > > However, none of that is necessary for the intended use of giving full > ownership of an object to devres and getting it dropped once the given > device is unbound. > > Hence, implement devres::register_foreign_boxed(), which is limited to > consume the given data, wrap it in a KBox and drop the KBox once the > given device is unbound, without any other synchronization. > > Cc: Dave Airlie <airlied@redhat.com> > Cc: Simona Vetter <simona.vetter@ffwll.ch> > Cc: Viresh Kumar <viresh.kumar@linaro.org> > Signed-off-by: Danilo Krummrich <dakr@kernel.org> > --- > rust/kernel/cpufreq.rs | 8 ++--- Acked-by: Viresh Kumar <viresh.kumar@linaro.org> -- viresh
© 2016 - 2025 Red Hat, Inc.