Extend the cpufreq abstractions to support driver registration from
Rust.
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
rust/kernel/cpufreq.rs | 530 ++++++++++++++++++++++++++++++++++++++++-
1 file changed, 528 insertions(+), 2 deletions(-)
diff --git a/rust/kernel/cpufreq.rs b/rust/kernel/cpufreq.rs
index 3e9ded655d46..4194b9558413 100644
--- a/rust/kernel/cpufreq.rs
+++ b/rust/kernel/cpufreq.rs
@@ -13,7 +13,8 @@
clk::{Clk, Hertz},
cpumask,
device::Device,
- error::{code::*, from_err_ptr, to_result, Result, VTABLE_DEFAULT_ERROR},
+ devres::Devres,
+ error::{code::*, from_err_ptr, from_result, to_result, Result, VTABLE_DEFAULT_ERROR},
ffi::c_ulong,
prelude::*,
types::ForeignOwnable,
@@ -21,9 +22,11 @@
};
use core::{
+ cell::UnsafeCell,
+ marker::PhantomData,
ops::{Deref, DerefMut},
pin::Pin,
- ptr::self,
+ ptr,
};
use macros::vtable;
@@ -792,3 +795,526 @@ fn register_em(_policy: &mut Policy) {
build_error!(VTABLE_DEFAULT_ERROR)
}
}
+
+/// CPU frequency driver Registration.
+///
+/// ## Examples
+///
+/// The following example demonstrates how to register a cpufreq driver.
+///
+/// ```
+/// use kernel::{
+/// c_str,
+/// cpu, cpufreq,
+/// device::Device,
+/// macros::vtable,
+/// sync::Arc,
+/// };
+/// struct FooDevice;
+///
+/// #[derive(Default)]
+/// struct FooDriver;
+///
+/// #[vtable]
+/// impl cpufreq::Driver for FooDriver {
+/// type Data = ();
+/// type PData = Arc<FooDevice>;
+///
+/// fn init(policy: &mut cpufreq::Policy) -> Result<Self::PData> {
+/// // Initialize here
+/// Ok(Arc::new(FooDevice, GFP_KERNEL)?)
+/// }
+///
+/// fn exit(_policy: &mut cpufreq::Policy, _data: Option<Self::PData>) -> Result<()> {
+/// Ok(())
+/// }
+///
+/// fn suspend(policy: &mut cpufreq::Policy) -> Result<()> {
+/// policy.generic_suspend()
+/// }
+///
+/// fn verify(data: &mut cpufreq::PolicyData) -> Result<()> {
+/// data.generic_verify()
+/// }
+///
+/// fn target_index(policy: &mut cpufreq::Policy, index: u32) -> Result<()> {
+/// // Update CPU frequency
+/// Ok(())
+/// }
+///
+/// fn get(policy: &mut cpufreq::Policy) -> Result<u32> {
+/// policy.generic_get()
+/// }
+/// }
+///
+/// fn foo_probe(dev: &Device) {
+/// cpufreq::Registration::<FooDriver>::new_foreign_owned(
+/// dev,
+/// c_str!("cpufreq-foo"),
+/// (),
+/// cpufreq::flags::NEED_INITIAL_FREQ_CHECK | cpufreq::flags::IS_COOLING_DEV,
+/// true,
+/// ).unwrap();
+/// }
+/// ```
+pub struct Registration<T: Driver> {
+ drv: KBox<UnsafeCell<bindings::cpufreq_driver>>,
+ _p: PhantomData<T>,
+}
+
+// SAFETY: `Registration` doesn't offer any methods or access to fields when shared between threads
+// or CPUs, so it is safe to share it.
+unsafe impl<T: Driver> Sync for Registration<T> {}
+
+#[allow(clippy::non_send_fields_in_send_ty)]
+// SAFETY: Registration with and unregistration from the cpufreq subsystem can happen from any
+// thread. Additionally, `T::Data` (which is dropped during unregistration) is `Send`, so it is
+// okay to move `Registration` to different threads.
+unsafe impl<T: Driver> Send for Registration<T> {}
+
+impl<T: Driver> Registration<T> {
+ /// Registers a CPU frequency driver with the cpufreq core.
+ pub fn new(name: &'static CStr, data: T::Data, flags: u16, boost: bool) -> Result<Self> {
+ // Required due to Rust 1.82's stricter handling of `unsafe` in mutable statics. The
+ // `unsafe` blocks aren't required anymore with later versions.
+ #![allow(unused_unsafe)]
+
+ let mut drv = KBox::new(
+ UnsafeCell::new(bindings::cpufreq_driver::default()),
+ GFP_KERNEL,
+ )?;
+ let drv_ref = drv.get_mut();
+
+ // Account for the trailing null byte.
+ let len = name.len() + 1;
+ if len > drv_ref.name.len() {
+ return Err(EINVAL);
+ };
+
+ // SAFETY: `name` is a valid `CStr`, and we are copying it to an array of equal or larger
+ // size.
+ let name = unsafe { &*(name.as_bytes_with_nul() as *const [u8]) };
+ drv_ref.name[..len].copy_from_slice(name);
+
+ drv_ref.boost_enabled = boost;
+ drv_ref.flags = flags;
+
+ // Initialize mandatory callbacks.
+ drv_ref.init = Some(Self::init_callback);
+ drv_ref.verify = Some(Self::verify_callback);
+
+ // Initialize optional callbacks based on the traits of `T`.
+ drv_ref.setpolicy = if T::HAS_SETPOLICY {
+ Some(Self::setpolicy_callback)
+ } else {
+ None
+ };
+ drv_ref.target = if T::HAS_TARGET {
+ Some(Self::target_callback)
+ } else {
+ None
+ };
+ drv_ref.target_index = if T::HAS_TARGET_INDEX {
+ Some(Self::target_index_callback)
+ } else {
+ None
+ };
+ drv_ref.fast_switch = if T::HAS_FAST_SWITCH {
+ Some(Self::fast_switch_callback)
+ } else {
+ None
+ };
+ drv_ref.adjust_perf = if T::HAS_ADJUST_PERF {
+ Some(Self::adjust_perf_callback)
+ } else {
+ None
+ };
+ drv_ref.get_intermediate = if T::HAS_GET_INTERMEDIATE {
+ Some(Self::get_intermediate_callback)
+ } else {
+ None
+ };
+ drv_ref.target_intermediate = if T::HAS_TARGET_INTERMEDIATE {
+ Some(Self::target_intermediate_callback)
+ } else {
+ None
+ };
+ drv_ref.get = if T::HAS_GET {
+ Some(Self::get_callback)
+ } else {
+ None
+ };
+ drv_ref.update_limits = if T::HAS_UPDATE_LIMITS {
+ Some(Self::update_limits_callback)
+ } else {
+ None
+ };
+ drv_ref.bios_limit = if T::HAS_BIOS_LIMIT {
+ Some(Self::bios_limit_callback)
+ } else {
+ None
+ };
+ drv_ref.online = if T::HAS_ONLINE {
+ Some(Self::online_callback)
+ } else {
+ None
+ };
+ drv_ref.offline = if T::HAS_OFFLINE {
+ Some(Self::offline_callback)
+ } else {
+ None
+ };
+ drv_ref.exit = if T::HAS_EXIT {
+ Some(Self::exit_callback)
+ } else {
+ None
+ };
+ drv_ref.suspend = if T::HAS_SUSPEND {
+ Some(Self::suspend_callback)
+ } else {
+ None
+ };
+ drv_ref.resume = if T::HAS_RESUME {
+ Some(Self::resume_callback)
+ } else {
+ None
+ };
+ drv_ref.ready = if T::HAS_READY {
+ Some(Self::ready_callback)
+ } else {
+ None
+ };
+ drv_ref.set_boost = if T::HAS_SET_BOOST {
+ Some(Self::set_boost_callback)
+ } else {
+ None
+ };
+ drv_ref.register_em = if T::HAS_REGISTER_EM {
+ Some(Self::register_em_callback)
+ } else {
+ None
+ };
+
+ // Set driver data before registering the driver, as the cpufreq core calls few callbacks
+ // before `cpufreq_register_driver` returns.
+ Self::set_data(drv_ref, data)?;
+
+ // SAFETY: It is safe to register the driver with the cpufreq core in the kernel C code.
+ to_result(unsafe { bindings::cpufreq_register_driver(drv_ref) })?;
+
+ Ok(Self {
+ drv,
+ _p: PhantomData,
+ })
+ }
+
+ /// 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
+ /// device is detached.
+ pub fn new_foreign_owned(
+ dev: &Device,
+ name: &'static CStr,
+ data: T::Data,
+ flags: u16,
+ boost: bool,
+ ) -> Result<()> {
+ Devres::new_foreign_owned(dev, Self::new(name, data, flags, boost)?, GFP_KERNEL)?;
+ Ok(())
+ }
+
+ // Sets the `Data` for the CPU frequency driver.
+ fn set_data(drv: &mut bindings::cpufreq_driver, data: T::Data) -> Result<()> {
+ if drv.driver_data.is_null() {
+ // Transfer the ownership of the data to the C code.
+ drv.driver_data = <T::Data as ForeignOwnable>::into_foreign(data) as _;
+ Ok(())
+ } else {
+ Err(EBUSY)
+ }
+ }
+
+ /// Returns borrowed `Data` previously set for the CPU frequency driver.
+ pub fn data(&mut self) -> Option<<T::Data as ForeignOwnable>::Borrowed<'static>> {
+ let drv = self.drv.get_mut();
+
+ if drv.driver_data.is_null() {
+ None
+ } else {
+ // SAFETY: The data is earlier set by us from `set_data`.
+ Some(unsafe { <T::Data as ForeignOwnable>::borrow(drv.driver_data) })
+ }
+ }
+
+ // Clears and returns the `Data` for the CPU frequency driver.
+ fn clear_data(&mut self) -> Option<T::Data> {
+ let drv = self.drv.get_mut();
+
+ if drv.driver_data.is_null() {
+ None
+ } else {
+ // SAFETY: The data is earlier set by us from `set_data`.
+ let data = Some(unsafe { <T::Data as ForeignOwnable>::from_foreign(drv.driver_data) });
+ drv.driver_data = ptr::null_mut();
+ data
+ }
+ }
+}
+
+// CPU frequency driver callbacks.
+impl<T: Driver> Registration<T> {
+ // Driver's `init` callback.
+ //
+ // SAFETY: Called from C. Inputs must be valid pointers.
+ extern "C" fn init_callback(ptr: *mut bindings::cpufreq_policy) -> kernel::ffi::c_int {
+ from_result(|| {
+ // SAFETY: The `ptr` is guaranteed to be valid by the contract with the C code for the
+ // lifetime of `policy`.
+ let policy = unsafe { Policy::from_raw_mut(ptr) };
+
+ let data = T::init(policy)?;
+ policy.set_data(data)?;
+ Ok(0)
+ })
+ }
+
+ // Driver's `exit` callback.
+ //
+ // SAFETY: Called from C. Inputs must be valid pointers.
+ extern "C" fn exit_callback(ptr: *mut bindings::cpufreq_policy) {
+ // SAFETY: The `ptr` is guaranteed to be valid by the contract with the C code for the
+ // lifetime of `policy`.
+ let policy = unsafe { Policy::from_raw_mut(ptr) };
+
+ let data = policy.clear_data();
+ let _ = T::exit(policy, data);
+ }
+
+ // Driver's `online` callback.
+ //
+ // SAFETY: Called from C. Inputs must be valid pointers.
+ extern "C" fn online_callback(ptr: *mut bindings::cpufreq_policy) -> kernel::ffi::c_int {
+ from_result(|| {
+ // SAFETY: The `ptr` is guaranteed to be valid by the contract with the C code for the
+ // lifetime of `policy`.
+ let policy = unsafe { Policy::from_raw_mut(ptr) };
+ T::online(policy).map(|()| 0)
+ })
+ }
+
+ // Driver's `offline` callback.
+ //
+ // SAFETY: Called from C. Inputs must be valid pointers.
+ extern "C" fn offline_callback(ptr: *mut bindings::cpufreq_policy) -> kernel::ffi::c_int {
+ from_result(|| {
+ // SAFETY: The `ptr` is guaranteed to be valid by the contract with the C code for the
+ // lifetime of `policy`.
+ let policy = unsafe { Policy::from_raw_mut(ptr) };
+ T::offline(policy).map(|()| 0)
+ })
+ }
+
+ // Driver's `suspend` callback.
+ //
+ // SAFETY: Called from C. Inputs must be valid pointers.
+ extern "C" fn suspend_callback(ptr: *mut bindings::cpufreq_policy) -> kernel::ffi::c_int {
+ from_result(|| {
+ // SAFETY: The `ptr` is guaranteed to be valid by the contract with the C code for the
+ // lifetime of `policy`.
+ let policy = unsafe { Policy::from_raw_mut(ptr) };
+ T::suspend(policy).map(|()| 0)
+ })
+ }
+
+ // Driver's `resume` callback.
+ //
+ // SAFETY: Called from C. Inputs must be valid pointers.
+ extern "C" fn resume_callback(ptr: *mut bindings::cpufreq_policy) -> kernel::ffi::c_int {
+ from_result(|| {
+ // SAFETY: The `ptr` is guaranteed to be valid by the contract with the C code for the
+ // lifetime of `policy`.
+ let policy = unsafe { Policy::from_raw_mut(ptr) };
+ T::resume(policy).map(|()| 0)
+ })
+ }
+
+ // Driver's `ready` callback.
+ //
+ // SAFETY: Called from C. Inputs must be valid pointers.
+ extern "C" fn ready_callback(ptr: *mut bindings::cpufreq_policy) {
+ // SAFETY: The `ptr` is guaranteed to be valid by the contract with the C code for the
+ // lifetime of `policy`.
+ let policy = unsafe { Policy::from_raw_mut(ptr) };
+ T::ready(policy);
+ }
+
+ // Driver's `verify` callback.
+ //
+ // SAFETY: Called from C. Inputs must be valid pointers.
+ extern "C" fn verify_callback(ptr: *mut bindings::cpufreq_policy_data) -> kernel::ffi::c_int {
+ from_result(|| {
+ // SAFETY: The `ptr` is guaranteed to be valid by the contract with the C code for the
+ // lifetime of `policy`.
+ let data = unsafe { PolicyData::from_raw_mut(ptr) };
+ T::verify(data).map(|()| 0)
+ })
+ }
+
+ // Driver's `setpolicy` callback.
+ //
+ // SAFETY: Called from C. Inputs must be valid pointers.
+ extern "C" fn setpolicy_callback(ptr: *mut bindings::cpufreq_policy) -> kernel::ffi::c_int {
+ from_result(|| {
+ // SAFETY: The `ptr` is guaranteed to be valid by the contract with the C code for the
+ // lifetime of `policy`.
+ let policy = unsafe { Policy::from_raw_mut(ptr) };
+ T::setpolicy(policy).map(|()| 0)
+ })
+ }
+
+ // Driver's `target` callback.
+ //
+ // SAFETY: Called from C. Inputs must be valid pointers.
+ extern "C" fn target_callback(
+ ptr: *mut bindings::cpufreq_policy,
+ target_freq: u32,
+ relation: u32,
+ ) -> kernel::ffi::c_int {
+ from_result(|| {
+ // SAFETY: The `ptr` is guaranteed to be valid by the contract with the C code for the
+ // lifetime of `policy`.
+ let policy = unsafe { Policy::from_raw_mut(ptr) };
+ T::target(policy, target_freq, Relation::new(relation)?).map(|()| 0)
+ })
+ }
+
+ // Driver's `target_index` callback.
+ //
+ // SAFETY: Called from C. Inputs must be valid pointers.
+ extern "C" fn target_index_callback(
+ ptr: *mut bindings::cpufreq_policy,
+ index: u32,
+ ) -> kernel::ffi::c_int {
+ from_result(|| {
+ // SAFETY: The `ptr` is guaranteed to be valid by the contract with the C code for the
+ // lifetime of `policy`.
+ let policy = unsafe { Policy::from_raw_mut(ptr) };
+ T::target_index(policy, index).map(|()| 0)
+ })
+ }
+
+ // Driver's `fast_switch` callback.
+ //
+ // SAFETY: Called from C. Inputs must be valid pointers.
+ extern "C" fn fast_switch_callback(
+ ptr: *mut bindings::cpufreq_policy,
+ target_freq: u32,
+ ) -> kernel::ffi::c_uint {
+ // SAFETY: The `ptr` is guaranteed to be valid by the contract with the C code for the
+ // lifetime of `policy`.
+ let policy = unsafe { Policy::from_raw_mut(ptr) };
+ T::fast_switch(policy, target_freq)
+ }
+
+ // Driver's `adjust_perf` callback.
+ extern "C" fn adjust_perf_callback(
+ cpu: u32,
+ min_perf: usize,
+ target_perf: usize,
+ capacity: usize,
+ ) {
+ if let Ok(mut policy) = PolicyCpu::from_cpu(cpu) {
+ T::adjust_perf(&mut policy, min_perf, target_perf, capacity);
+ }
+ }
+
+ // Driver's `get_intermediate` callback.
+ //
+ // SAFETY: Called from C. Inputs must be valid pointers.
+ extern "C" fn get_intermediate_callback(
+ ptr: *mut bindings::cpufreq_policy,
+ index: u32,
+ ) -> kernel::ffi::c_uint {
+ // SAFETY: The `ptr` is guaranteed to be valid by the contract with the C code for the
+ // lifetime of `policy`.
+ let policy = unsafe { Policy::from_raw_mut(ptr) };
+ T::get_intermediate(policy, index)
+ }
+
+ // Driver's `target_intermediate` callback.
+ //
+ // SAFETY: Called from C. Inputs must be valid pointers.
+ extern "C" fn target_intermediate_callback(
+ ptr: *mut bindings::cpufreq_policy,
+ index: u32,
+ ) -> kernel::ffi::c_int {
+ from_result(|| {
+ // SAFETY: The `ptr` is guaranteed to be valid by the contract with the C code for the
+ // lifetime of `policy`.
+ let policy = unsafe { Policy::from_raw_mut(ptr) };
+ T::target_intermediate(policy, index).map(|()| 0)
+ })
+ }
+
+ // Driver's `get` callback.
+ extern "C" fn get_callback(cpu: u32) -> kernel::ffi::c_uint {
+ PolicyCpu::from_cpu(cpu).map_or(0, |mut policy| T::get(&mut policy).map_or(0, |f| f))
+ }
+
+ // Driver's `update_limit` callback.
+ extern "C" fn update_limits_callback(cpu: u32) {
+ if let Ok(mut policy) = PolicyCpu::from_cpu(cpu) {
+ T::update_limits(&mut policy);
+ }
+ }
+
+ // Driver's `bios_limit` callback.
+ //
+ // SAFETY: Called from C. Inputs must be valid pointers.
+ extern "C" fn bios_limit_callback(cpu: i32, limit: *mut u32) -> kernel::ffi::c_int {
+ from_result(|| {
+ let mut policy = PolicyCpu::from_cpu(cpu as u32)?;
+
+ // SAFETY: `limit` is guaranteed by the C code to be valid.
+ T::bios_limit(&mut policy, &mut (unsafe { *limit })).map(|()| 0)
+ })
+ }
+
+ // Driver's `set_boost` callback.
+ //
+ // SAFETY: Called from C. Inputs must be valid pointers.
+ extern "C" fn set_boost_callback(
+ ptr: *mut bindings::cpufreq_policy,
+ state: i32,
+ ) -> kernel::ffi::c_int {
+ from_result(|| {
+ // SAFETY: The `ptr` is guaranteed to be valid by the contract with the C code for the
+ // lifetime of `policy`.
+ let policy = unsafe { Policy::from_raw_mut(ptr) };
+ T::set_boost(policy, state).map(|()| 0)
+ })
+ }
+
+ // Driver's `register_em` callback.
+ //
+ // SAFETY: Called from C. Inputs must be valid pointers.
+ extern "C" fn register_em_callback(ptr: *mut bindings::cpufreq_policy) {
+ // SAFETY: The `ptr` is guaranteed to be valid by the contract with the C code for the
+ // lifetime of `policy`.
+ let policy = unsafe { Policy::from_raw_mut(ptr) };
+ T::register_em(policy);
+ }
+}
+
+impl<T: Driver> Drop for Registration<T> {
+ // Removes the `Registration` from the kernel, if it has initialized successfully earlier.
+ fn drop(&mut self) {
+ let drv = self.drv.get_mut();
+
+ // SAFETY: The driver was earlier registered from `new`.
+ unsafe { bindings::cpufreq_unregister_driver(drv) };
+
+ // Free data
+ drop(self.clear_data());
+ }
+}
--
2.31.1.272.g89b43f80a514On Fri, Apr 11, 2025 at 04:25:14PM +0530, Viresh Kumar wrote:
> +pub struct Registration<T: Driver> {
> + drv: KBox<UnsafeCell<bindings::cpufreq_driver>>,
> + _p: PhantomData<T>,
> +}
> +
> +// SAFETY: `Registration` doesn't offer any methods or access to fields when shared between threads
> +// or CPUs, so it is safe to share it.
> +unsafe impl<T: Driver> Sync for Registration<T> {}
> +
> +#[allow(clippy::non_send_fields_in_send_ty)]
> +// SAFETY: Registration with and unregistration from the cpufreq subsystem can happen from any
> +// thread. Additionally, `T::Data` (which is dropped during unregistration) is `Send`, so it is
> +// okay to move `Registration` to different threads.
> +unsafe impl<T: Driver> Send for Registration<T> {}
> +
> +impl<T: Driver> Registration<T> {
> + /// Registers a CPU frequency driver with the cpufreq core.
> + pub fn new(name: &'static CStr, data: T::Data, flags: u16, boost: bool) -> Result<Self> {
Do you really need the private data? It seems to be used by only very few
drivers in C either.
If yes, I think you have to split this up into a CpufreqDriver structure and a
separate Registration. Otherwise you won't ever get access to the private data
again, after Registration::new_foreign_owned().
Note that we typically want Registration::new_foreign_owned(), because it
implies the guaranteed "fence" for where we can safely consider a device to be
bound. If we'd give out a Registration instance instead, the driver can
arbitrarily extend its lifetime across the remove() boundary.
If no, it seems to me that you can even avoid allocating a struct cpufreq_driver
dynamically and make it const instead. The fields name, boost_enabled and flags
could be required consts in your cpufreq::Driver trait.
On 11-04-25, 13:58, Danilo Krummrich wrote:
> On Fri, Apr 11, 2025 at 04:25:14PM +0530, Viresh Kumar wrote:
> > +impl<T: Driver> Registration<T> {
> > + /// Registers a CPU frequency driver with the cpufreq core.
> > + pub fn new(name: &'static CStr, data: T::Data, flags: u16, boost: bool) -> Result<Self> {
>
> Do you really need the private data? It seems to be used by only very few
> drivers in C either.
Yes, only a few of them are setting it to a value other than `pdev`.
Maybe we can avoid it for the time being and come back to this when a
driver really wants it.
> If no, it seems to me that you can even avoid allocating a struct cpufreq_driver
> dynamically and make it const instead.
I am not sure if I understood your suggestion. The Registration::new()
method still updates the instance of cpufreq_driver before passing it
to the C cpufreq core.
I have tried to fix the other issues though.
--
viresh
diff --git a/drivers/cpufreq/rcpufreq_dt.rs b/drivers/cpufreq/rcpufreq_dt.rs
index 751be33c0218..831269bdeabf 100644
--- a/drivers/cpufreq/rcpufreq_dt.rs
+++ b/drivers/cpufreq/rcpufreq_dt.rs
@@ -59,7 +59,10 @@ impl opp::ConfigOps for CPUFreqDTDriver {}
#[vtable]
impl cpufreq::Driver for CPUFreqDTDriver {
- type Data = ();
+ const NAME: &CStr = c_str!("cpufreq-dt");
+ const FLAGS: u16 = cpufreq::flags::NEED_INITIAL_FREQ_CHECK | cpufreq::flags::IS_COOLING_DEV;
+ const BOOST_ENABLED: bool = true;
+
type PData = Arc<CPUFreqDTDevice>;
fn init(policy: &mut cpufreq::Policy) -> Result<Self::PData> {
@@ -213,13 +216,7 @@ fn probe(
pdev: &platform::Device<Core>,
_id_info: Option<&Self::IdInfo>,
) -> Result<Pin<KBox<Self>>> {
- cpufreq::Registration::<CPUFreqDTDriver>::new_foreign_owned(
- pdev.as_ref(),
- c_str!("cpufreq-dt"),
- (),
- cpufreq::flags::NEED_INITIAL_FREQ_CHECK | cpufreq::flags::IS_COOLING_DEV,
- true,
- )?;
+ cpufreq::Registration::<CPUFreqDTDriver>::new_foreign_owned(pdev.as_ref())?;
let drvdata = KBox::new(Self {}, GFP_KERNEL)?;
diff --git a/rust/kernel/cpufreq.rs b/rust/kernel/cpufreq.rs
index 4194b9558413..9b275d4d3eb6 100644
--- a/rust/kernel/cpufreq.rs
+++ b/rust/kernel/cpufreq.rs
@@ -685,13 +685,14 @@ fn drop(&mut self) {
/// Reference: <https://docs.kernel.org/cpu-freq/cpu-drivers.html>
#[vtable]
pub trait Driver {
- /// Driver specific data.
- ///
- /// Corresponds to the data retrieved via the kernel's `cpufreq_get_driver_data` function.
- ///
- /// Require `Data` to implement `ForeignOwnable`. We guarantee to never move the underlying
- /// wrapped data structure.
- type Data: ForeignOwnable;
+ /// Driver's name.
+ const NAME: &'static CStr;
+
+ /// Driver's flags.
+ const FLAGS: u16;
+
+ /// Boost support.
+ const BOOST_ENABLED: bool;
/// Policy specific data.
///
@@ -804,8 +805,8 @@ fn register_em(_policy: &mut Policy) {
///
/// ```
/// use kernel::{
-/// c_str,
/// cpu, cpufreq,
+/// c_str,
/// device::Device,
/// macros::vtable,
/// sync::Arc,
@@ -817,7 +818,10 @@ fn register_em(_policy: &mut Policy) {
///
/// #[vtable]
/// impl cpufreq::Driver for FooDriver {
-/// type Data = ();
+/// const NAME: &'static CStr = c_str!("cpufreq-foo");
+/// const FLAGS: u16 = cpufreq::flags::NEED_INITIAL_FREQ_CHECK | cpufreq::flags::IS_COOLING_DEV;
+/// const BOOST_ENABLED: bool = true;
+///
/// type PData = Arc<FooDevice>;
///
/// fn init(policy: &mut cpufreq::Policy) -> Result<Self::PData> {
@@ -848,13 +852,7 @@ fn register_em(_policy: &mut Policy) {
/// }
///
/// fn foo_probe(dev: &Device) {
-/// cpufreq::Registration::<FooDriver>::new_foreign_owned(
-/// dev,
-/// c_str!("cpufreq-foo"),
-/// (),
-/// cpufreq::flags::NEED_INITIAL_FREQ_CHECK | cpufreq::flags::IS_COOLING_DEV,
-/// true,
-/// ).unwrap();
+/// cpufreq::Registration::<FooDriver>::new_foreign_owned(dev).unwrap();
/// }
/// ```
pub struct Registration<T: Driver> {
@@ -868,13 +866,12 @@ unsafe impl<T: Driver> Sync for Registration<T> {}
#[allow(clippy::non_send_fields_in_send_ty)]
// SAFETY: Registration with and unregistration from the cpufreq subsystem can happen from any
-// thread. Additionally, `T::Data` (which is dropped during unregistration) is `Send`, so it is
-// okay to move `Registration` to different threads.
+// thread.
unsafe impl<T: Driver> Send for Registration<T> {}
impl<T: Driver> Registration<T> {
/// Registers a CPU frequency driver with the cpufreq core.
- pub fn new(name: &'static CStr, data: T::Data, flags: u16, boost: bool) -> Result<Self> {
+ pub fn new() -> Result<Self> {
// Required due to Rust 1.82's stricter handling of `unsafe` in mutable statics. The
// `unsafe` blocks aren't required anymore with later versions.
#![allow(unused_unsafe)]
@@ -886,18 +883,18 @@ pub fn new(name: &'static CStr, data: T::Data, flags: u16, boost: bool) -> Resul
let drv_ref = drv.get_mut();
// Account for the trailing null byte.
- let len = name.len() + 1;
+ let len = T::NAME.len() + 1;
if len > drv_ref.name.len() {
return Err(EINVAL);
};
- // SAFETY: `name` is a valid `CStr`, and we are copying it to an array of equal or larger
- // size.
- let name = unsafe { &*(name.as_bytes_with_nul() as *const [u8]) };
+ // SAFETY: `T::NAME` is a valid `CStr`, and we are copying it to an array of equal or
+ // larger size.
+ let name = unsafe { &*(T::NAME.as_bytes_with_nul() as *const [u8]) };
drv_ref.name[..len].copy_from_slice(name);
- drv_ref.boost_enabled = boost;
- drv_ref.flags = flags;
+ drv_ref.boost_enabled = T::BOOST_ENABLED;
+ drv_ref.flags = T::FLAGS;
// Initialize mandatory callbacks.
drv_ref.init = Some(Self::init_callback);
@@ -995,10 +992,6 @@ pub fn new(name: &'static CStr, data: T::Data, flags: u16, boost: bool) -> Resul
None
};
- // Set driver data before registering the driver, as the cpufreq core calls few callbacks
- // before `cpufreq_register_driver` returns.
- Self::set_data(drv_ref, data)?;
-
// SAFETY: It is safe to register the driver with the cpufreq core in the kernel C code.
to_result(unsafe { bindings::cpufreq_register_driver(drv_ref) })?;
@@ -1012,53 +1005,10 @@ pub fn new(name: &'static CStr, data: T::Data, flags: u16, boost: bool) -> Resul
///
/// Instead the [`Registration`] is owned by [`Devres`] and will be revoked / dropped, once the
/// device is detached.
- pub fn new_foreign_owned(
- dev: &Device,
- name: &'static CStr,
- data: T::Data,
- flags: u16,
- boost: bool,
- ) -> Result<()> {
- Devres::new_foreign_owned(dev, Self::new(name, data, flags, boost)?, GFP_KERNEL)?;
+ pub fn new_foreign_owned(dev: &Device) -> Result<()> {
+ Devres::new_foreign_owned(dev, Self::new()?, GFP_KERNEL)?;
Ok(())
}
-
- // Sets the `Data` for the CPU frequency driver.
- fn set_data(drv: &mut bindings::cpufreq_driver, data: T::Data) -> Result<()> {
- if drv.driver_data.is_null() {
- // Transfer the ownership of the data to the C code.
- drv.driver_data = <T::Data as ForeignOwnable>::into_foreign(data) as _;
- Ok(())
- } else {
- Err(EBUSY)
- }
- }
-
- /// Returns borrowed `Data` previously set for the CPU frequency driver.
- pub fn data(&mut self) -> Option<<T::Data as ForeignOwnable>::Borrowed<'static>> {
- let drv = self.drv.get_mut();
-
- if drv.driver_data.is_null() {
- None
- } else {
- // SAFETY: The data is earlier set by us from `set_data`.
- Some(unsafe { <T::Data as ForeignOwnable>::borrow(drv.driver_data) })
- }
- }
-
- // Clears and returns the `Data` for the CPU frequency driver.
- fn clear_data(&mut self) -> Option<T::Data> {
- let drv = self.drv.get_mut();
-
- if drv.driver_data.is_null() {
- None
- } else {
- // SAFETY: The data is earlier set by us from `set_data`.
- let data = Some(unsafe { <T::Data as ForeignOwnable>::from_foreign(drv.driver_data) });
- drv.driver_data = ptr::null_mut();
- data
- }
- }
}
// CPU frequency driver callbacks.
@@ -1313,8 +1263,5 @@ fn drop(&mut self) {
// SAFETY: The driver was earlier registered from `new`.
unsafe { bindings::cpufreq_unregister_driver(drv) };
-
- // Free data
- drop(self.clear_data());
}
}
On Mon, Apr 14, 2025 at 02:17:06PM +0530, Viresh Kumar wrote:
> On 11-04-25, 13:58, Danilo Krummrich wrote:
> > On Fri, Apr 11, 2025 at 04:25:14PM +0530, Viresh Kumar wrote:
>
> > If no, it seems to me that you can even avoid allocating a struct cpufreq_driver
> > dynamically and make it const instead.
>
> I am not sure if I understood your suggestion. The Registration::new()
> method still updates the instance of cpufreq_driver before passing it
> to the C cpufreq core.
See comment in the diff below.
> diff --git a/rust/kernel/cpufreq.rs b/rust/kernel/cpufreq.rs
> index 4194b9558413..9b275d4d3eb6 100644
> --- a/rust/kernel/cpufreq.rs
> +++ b/rust/kernel/cpufreq.rs
> @@ -685,13 +685,14 @@ fn drop(&mut self) {
> /// Reference: <https://docs.kernel.org/cpu-freq/cpu-drivers.html>
> #[vtable]
> pub trait Driver {
> - /// Driver specific data.
> - ///
> - /// Corresponds to the data retrieved via the kernel's `cpufreq_get_driver_data` function.
> - ///
> - /// Require `Data` to implement `ForeignOwnable`. We guarantee to never move the underlying
> - /// wrapped data structure.
> - type Data: ForeignOwnable;
> + /// Driver's name.
> + const NAME: &'static CStr;
> +
> + /// Driver's flags.
> + const FLAGS: u16;
> +
> + /// Boost support.
> + const BOOST_ENABLED: bool;
>
> /// Policy specific data.
> ///
> @@ -804,8 +805,8 @@ fn register_em(_policy: &mut Policy) {
> ///
> /// ```
> /// use kernel::{
> -/// c_str,
> /// cpu, cpufreq,
> +/// c_str,
> /// device::Device,
> /// macros::vtable,
> /// sync::Arc,
> @@ -817,7 +818,10 @@ fn register_em(_policy: &mut Policy) {
> ///
> /// #[vtable]
> /// impl cpufreq::Driver for FooDriver {
> -/// type Data = ();
> +/// const NAME: &'static CStr = c_str!("cpufreq-foo");
> +/// const FLAGS: u16 = cpufreq::flags::NEED_INITIAL_FREQ_CHECK | cpufreq::flags::IS_COOLING_DEV;
> +/// const BOOST_ENABLED: bool = true;
> +///
> /// type PData = Arc<FooDevice>;
> ///
> /// fn init(policy: &mut cpufreq::Policy) -> Result<Self::PData> {
> @@ -848,13 +852,7 @@ fn register_em(_policy: &mut Policy) {
> /// }
> ///
> /// fn foo_probe(dev: &Device) {
> -/// cpufreq::Registration::<FooDriver>::new_foreign_owned(
> -/// dev,
> -/// c_str!("cpufreq-foo"),
> -/// (),
> -/// cpufreq::flags::NEED_INITIAL_FREQ_CHECK | cpufreq::flags::IS_COOLING_DEV,
> -/// true,
> -/// ).unwrap();
> +/// cpufreq::Registration::<FooDriver>::new_foreign_owned(dev).unwrap();
> /// }
> /// ```
> pub struct Registration<T: Driver> {
You could define Registration<T: Driver> as
pub struct Registration<T: Driver>(
NonNull<bindings::cpufreq_driver>,
PhantomData<T>
);
and subsequently...
> @@ -868,13 +866,12 @@ unsafe impl<T: Driver> Sync for Registration<T> {}
>
> #[allow(clippy::non_send_fields_in_send_ty)]
> // SAFETY: Registration with and unregistration from the cpufreq subsystem can happen from any
> -// thread. Additionally, `T::Data` (which is dropped during unregistration) is `Send`, so it is
> -// okay to move `Registration` to different threads.
> +// thread.
> unsafe impl<T: Driver> Send for Registration<T> {}
>
> impl<T: Driver> Registration<T> {
...add a new const of type bindings::cpufreq_driver, i.e.
const VTABLE: bindings::cpufreq_driver = bindings::cpufreq_driver {
name: Self::copy_name(T::NAME),
boost_enabled: T::BOOST_ENABLED,
flags: T::FLAGS,
[...]
}
const fn copy_name(name: &'static CStr) -> [kernel::ffi::c_char; CPUFREQ_NAME_LEN] {
let src name.as_bytes_with_nul();
let mut dst = [0; CPUFREQ_NAME_LEN];
build_assert!(name.len() <= dst.len());
let mut i = 0;
while i < dst.len() {
dst[i] = src[i];
i += 1;
}
dst
}
You should then be able to store a pointer of Self::VTABLE in your Registration
and and hence avoid dynamic allocation of struct cpufreq_driver.
> /// Registers a CPU frequency driver with the cpufreq core.
> - pub fn new(name: &'static CStr, data: T::Data, flags: u16, boost: bool) -> Result<Self> {
> + pub fn new() -> Result<Self> {
> // Required due to Rust 1.82's stricter handling of `unsafe` in mutable statics. The
> // `unsafe` blocks aren't required anymore with later versions.
> #![allow(unused_unsafe)]
> @@ -886,18 +883,18 @@ pub fn new(name: &'static CStr, data: T::Data, flags: u16, boost: bool) -> Resul
> let drv_ref = drv.get_mut();
>
> // Account for the trailing null byte.
> - let len = name.len() + 1;
> + let len = T::NAME.len() + 1;
> if len > drv_ref.name.len() {
> return Err(EINVAL);
> };
>
> - // SAFETY: `name` is a valid `CStr`, and we are copying it to an array of equal or larger
> - // size.
> - let name = unsafe { &*(name.as_bytes_with_nul() as *const [u8]) };
> + // SAFETY: `T::NAME` is a valid `CStr`, and we are copying it to an array of equal or
> + // larger size.
> + let name = unsafe { &*(T::NAME.as_bytes_with_nul() as *const [u8]) };
> drv_ref.name[..len].copy_from_slice(name);
>
> - drv_ref.boost_enabled = boost;
> - drv_ref.flags = flags;
> + drv_ref.boost_enabled = T::BOOST_ENABLED;
> + drv_ref.flags = T::FLAGS;
>
> // Initialize mandatory callbacks.
> drv_ref.init = Some(Self::init_callback);
> @@ -995,10 +992,6 @@ pub fn new(name: &'static CStr, data: T::Data, flags: u16, boost: bool) -> Resul
> None
> };
>
> - // Set driver data before registering the driver, as the cpufreq core calls few callbacks
> - // before `cpufreq_register_driver` returns.
> - Self::set_data(drv_ref, data)?;
> -
> // SAFETY: It is safe to register the driver with the cpufreq core in the kernel C code.
> to_result(unsafe { bindings::cpufreq_register_driver(drv_ref) })?;
>
> @@ -1012,53 +1005,10 @@ pub fn new(name: &'static CStr, data: T::Data, flags: u16, boost: bool) -> Resul
> ///
> /// Instead the [`Registration`] is owned by [`Devres`] and will be revoked / dropped, once the
> /// device is detached.
> - pub fn new_foreign_owned(
> - dev: &Device,
> - name: &'static CStr,
> - data: T::Data,
> - flags: u16,
> - boost: bool,
> - ) -> Result<()> {
> - Devres::new_foreign_owned(dev, Self::new(name, data, flags, boost)?, GFP_KERNEL)?;
> + pub fn new_foreign_owned(dev: &Device) -> Result<()> {
> + Devres::new_foreign_owned(dev, Self::new()?, GFP_KERNEL)?;
> Ok(())
> }
> -
> - // Sets the `Data` for the CPU frequency driver.
> - fn set_data(drv: &mut bindings::cpufreq_driver, data: T::Data) -> Result<()> {
> - if drv.driver_data.is_null() {
> - // Transfer the ownership of the data to the C code.
> - drv.driver_data = <T::Data as ForeignOwnable>::into_foreign(data) as _;
> - Ok(())
> - } else {
> - Err(EBUSY)
> - }
> - }
> -
> - /// Returns borrowed `Data` previously set for the CPU frequency driver.
> - pub fn data(&mut self) -> Option<<T::Data as ForeignOwnable>::Borrowed<'static>> {
> - let drv = self.drv.get_mut();
> -
> - if drv.driver_data.is_null() {
> - None
> - } else {
> - // SAFETY: The data is earlier set by us from `set_data`.
> - Some(unsafe { <T::Data as ForeignOwnable>::borrow(drv.driver_data) })
> - }
> - }
> -
> - // Clears and returns the `Data` for the CPU frequency driver.
> - fn clear_data(&mut self) -> Option<T::Data> {
> - let drv = self.drv.get_mut();
> -
> - if drv.driver_data.is_null() {
> - None
> - } else {
> - // SAFETY: The data is earlier set by us from `set_data`.
> - let data = Some(unsafe { <T::Data as ForeignOwnable>::from_foreign(drv.driver_data) });
> - drv.driver_data = ptr::null_mut();
> - data
> - }
> - }
> }
>
> // CPU frequency driver callbacks.
> @@ -1313,8 +1263,5 @@ fn drop(&mut self) {
>
> // SAFETY: The driver was earlier registered from `new`.
> unsafe { bindings::cpufreq_unregister_driver(drv) };
> -
> - // Free data
> - drop(self.clear_data());
> }
> }
On 14-04-25, 11:39, Danilo Krummrich wrote:
> const VTABLE: bindings::cpufreq_driver = bindings::cpufreq_driver {
> name: Self::copy_name(T::NAME),
> boost_enabled: T::BOOST_ENABLED,
> flags: T::FLAGS,
> [...]
> }
Ahh, thanks for this.
--
viresh
diff --git a/rust/kernel/cpufreq.rs b/rust/kernel/cpufreq.rs
index 9b275d4d3eb6..a6e660d46304 100644
--- a/rust/kernel/cpufreq.rs
+++ b/rust/kernel/cpufreq.rs
@@ -9,28 +9,32 @@
//! Reference: <https://docs.kernel.org/admin-guide/pm/cpufreq.html>
use crate::{
+ alloc::AllocError,
bindings,
clk::{Clk, Hertz},
cpumask,
device::Device,
devres::Devres,
error::{code::*, from_err_ptr, from_result, to_result, Result, VTABLE_DEFAULT_ERROR},
- ffi::c_ulong,
+ ffi::{c_char, c_ulong},
prelude::*,
types::ForeignOwnable,
types::Opaque,
};
use core::{
- cell::UnsafeCell,
marker::PhantomData,
+ mem::MaybeUninit,
ops::{Deref, DerefMut},
pin::Pin,
- ptr,
+ ptr::{self, NonNull},
};
use macros::vtable;
+// Maximum length of CPU frequency driver's name.
+const CPUFREQ_NAME_LEN: usize = bindings::CPUFREQ_NAME_LEN as usize;
+
/// Default transition latency value in nanoseconds.
pub const ETERNAL_LATENCY_NS: u32 = bindings::CPUFREQ_ETERNAL as u32;
@@ -855,10 +859,8 @@ fn register_em(_policy: &mut Policy) {
/// cpufreq::Registration::<FooDriver>::new_foreign_owned(dev).unwrap();
/// }
/// ```
-pub struct Registration<T: Driver> {
- drv: KBox<UnsafeCell<bindings::cpufreq_driver>>,
- _p: PhantomData<T>,
-}
+#[repr(transparent)]
+pub struct Registration<T: Driver>(NonNull<bindings::cpufreq_driver>, PhantomData<T>);
// SAFETY: `Registration` doesn't offer any methods or access to fields when shared between threads
// or CPUs, so it is safe to share it.
@@ -870,135 +872,136 @@ unsafe impl<T: Driver> Sync for Registration<T> {}
unsafe impl<T: Driver> Send for Registration<T> {}
impl<T: Driver> Registration<T> {
- /// Registers a CPU frequency driver with the cpufreq core.
- pub fn new() -> Result<Self> {
- // Required due to Rust 1.82's stricter handling of `unsafe` in mutable statics. The
- // `unsafe` blocks aren't required anymore with later versions.
- #![allow(unused_unsafe)]
-
- let mut drv = KBox::new(
- UnsafeCell::new(bindings::cpufreq_driver::default()),
- GFP_KERNEL,
- )?;
- let drv_ref = drv.get_mut();
-
- // Account for the trailing null byte.
- let len = T::NAME.len() + 1;
- if len > drv_ref.name.len() {
- return Err(EINVAL);
- };
-
- // SAFETY: `T::NAME` is a valid `CStr`, and we are copying it to an array of equal or
- // larger size.
- let name = unsafe { &*(T::NAME.as_bytes_with_nul() as *const [u8]) };
- drv_ref.name[..len].copy_from_slice(name);
-
- drv_ref.boost_enabled = T::BOOST_ENABLED;
- drv_ref.flags = T::FLAGS;
+ const VTABLE: bindings::cpufreq_driver = bindings::cpufreq_driver {
+ name: Self::copy_name(T::NAME),
+ boost_enabled: T::BOOST_ENABLED,
+ flags: T::FLAGS,
// Initialize mandatory callbacks.
- drv_ref.init = Some(Self::init_callback);
- drv_ref.verify = Some(Self::verify_callback);
+ init: Some(Self::init_callback),
+ verify: Some(Self::verify_callback),
// Initialize optional callbacks based on the traits of `T`.
- drv_ref.setpolicy = if T::HAS_SETPOLICY {
+ setpolicy: if T::HAS_SETPOLICY {
Some(Self::setpolicy_callback)
} else {
None
- };
- drv_ref.target = if T::HAS_TARGET {
+ },
+ target: if T::HAS_TARGET {
Some(Self::target_callback)
} else {
None
- };
- drv_ref.target_index = if T::HAS_TARGET_INDEX {
+ },
+ target_index: if T::HAS_TARGET_INDEX {
Some(Self::target_index_callback)
} else {
None
- };
- drv_ref.fast_switch = if T::HAS_FAST_SWITCH {
+ },
+ fast_switch: if T::HAS_FAST_SWITCH {
Some(Self::fast_switch_callback)
} else {
None
- };
- drv_ref.adjust_perf = if T::HAS_ADJUST_PERF {
+ },
+ adjust_perf: if T::HAS_ADJUST_PERF {
Some(Self::adjust_perf_callback)
} else {
None
- };
- drv_ref.get_intermediate = if T::HAS_GET_INTERMEDIATE {
+ },
+ get_intermediate: if T::HAS_GET_INTERMEDIATE {
Some(Self::get_intermediate_callback)
} else {
None
- };
- drv_ref.target_intermediate = if T::HAS_TARGET_INTERMEDIATE {
+ },
+ target_intermediate: if T::HAS_TARGET_INTERMEDIATE {
Some(Self::target_intermediate_callback)
} else {
None
- };
- drv_ref.get = if T::HAS_GET {
+ },
+ get: if T::HAS_GET {
Some(Self::get_callback)
} else {
None
- };
- drv_ref.update_limits = if T::HAS_UPDATE_LIMITS {
+ },
+ update_limits: if T::HAS_UPDATE_LIMITS {
Some(Self::update_limits_callback)
} else {
None
- };
- drv_ref.bios_limit = if T::HAS_BIOS_LIMIT {
+ },
+ bios_limit: if T::HAS_BIOS_LIMIT {
Some(Self::bios_limit_callback)
} else {
None
- };
- drv_ref.online = if T::HAS_ONLINE {
+ },
+ online: if T::HAS_ONLINE {
Some(Self::online_callback)
} else {
None
- };
- drv_ref.offline = if T::HAS_OFFLINE {
+ },
+ offline: if T::HAS_OFFLINE {
Some(Self::offline_callback)
} else {
None
- };
- drv_ref.exit = if T::HAS_EXIT {
+ },
+ exit: if T::HAS_EXIT {
Some(Self::exit_callback)
} else {
None
- };
- drv_ref.suspend = if T::HAS_SUSPEND {
+ },
+ suspend: if T::HAS_SUSPEND {
Some(Self::suspend_callback)
} else {
None
- };
- drv_ref.resume = if T::HAS_RESUME {
+ },
+ resume: if T::HAS_RESUME {
Some(Self::resume_callback)
} else {
None
- };
- drv_ref.ready = if T::HAS_READY {
+ },
+ ready: if T::HAS_READY {
Some(Self::ready_callback)
} else {
None
- };
- drv_ref.set_boost = if T::HAS_SET_BOOST {
+ },
+ set_boost: if T::HAS_SET_BOOST {
Some(Self::set_boost_callback)
} else {
None
- };
- drv_ref.register_em = if T::HAS_REGISTER_EM {
+ },
+ register_em: if T::HAS_REGISTER_EM {
Some(Self::register_em_callback)
} else {
None
- };
+ },
+ // SAFETY: All zeros is a valid value for `bindings::cpufreq_driver`.
+ ..unsafe { MaybeUninit::zeroed().assume_init() }
+ };
+
+ const fn copy_name(name: &'static CStr) -> [c_char; CPUFREQ_NAME_LEN] {
+ let src = name.as_bytes_with_nul();
+ let mut dst = [0; CPUFREQ_NAME_LEN];
+
+ build_assert!(src.len() <= CPUFREQ_NAME_LEN);
+
+ let mut i = 0;
+ while i < src.len() {
+ dst[i] = src[i];
+ i += 1;
+ }
+
+ dst
+ }
+
+ /// Registers a CPU frequency driver with the cpufreq core.
+ pub fn new() -> Result<Self> {
+ let drv = &Self::VTABLE as *const _ as *mut _;
// SAFETY: It is safe to register the driver with the cpufreq core in the kernel C code.
- to_result(unsafe { bindings::cpufreq_register_driver(drv_ref) })?;
+ to_result(unsafe { bindings::cpufreq_register_driver(drv) })?;
- Ok(Self {
- drv,
- _p: PhantomData,
- })
+ Ok(Self(
+ NonNull::new(drv.cast()).ok_or(AllocError)?,
+ PhantomData,
+ ))
}
/// Same as [`Registration::new`], but does not return a [`Registration`] instance.
@@ -1259,9 +1262,7 @@ extern "C" fn register_em_callback(ptr: *mut bindings::cpufreq_policy) {
impl<T: Driver> Drop for Registration<T> {
// Removes the `Registration` from the kernel, if it has initialized successfully earlier.
fn drop(&mut self) {
- let drv = self.drv.get_mut();
-
// SAFETY: The driver was earlier registered from `new`.
- unsafe { bindings::cpufreq_unregister_driver(drv) };
+ unsafe { bindings::cpufreq_unregister_driver(self.0.as_ptr()) };
}
}
On Mon, Apr 14, 2025 at 04:22:12PM +0530, Viresh Kumar wrote:
> On 14-04-25, 11:39, Danilo Krummrich wrote:
> > const VTABLE: bindings::cpufreq_driver = bindings::cpufreq_driver {
> > name: Self::copy_name(T::NAME),
> > boost_enabled: T::BOOST_ENABLED,
> > flags: T::FLAGS,
> > [...]
> > }
>
> Ahh, thanks for this.
The diff below looks good!
One nit below.
> diff --git a/rust/kernel/cpufreq.rs b/rust/kernel/cpufreq.rs
> index 9b275d4d3eb6..a6e660d46304 100644
> --- a/rust/kernel/cpufreq.rs
> +++ b/rust/kernel/cpufreq.rs
> @@ -9,28 +9,32 @@
> //! Reference: <https://docs.kernel.org/admin-guide/pm/cpufreq.html>
>
> use crate::{
> + alloc::AllocError,
> bindings,
> clk::{Clk, Hertz},
> cpumask,
> device::Device,
> devres::Devres,
> error::{code::*, from_err_ptr, from_result, to_result, Result, VTABLE_DEFAULT_ERROR},
> - ffi::c_ulong,
> + ffi::{c_char, c_ulong},
> prelude::*,
> types::ForeignOwnable,
> types::Opaque,
> };
>
> use core::{
> - cell::UnsafeCell,
> marker::PhantomData,
> + mem::MaybeUninit,
> ops::{Deref, DerefMut},
> pin::Pin,
> - ptr,
> + ptr::{self, NonNull},
> };
>
> use macros::vtable;
>
> +// Maximum length of CPU frequency driver's name.
> +const CPUFREQ_NAME_LEN: usize = bindings::CPUFREQ_NAME_LEN as usize;
> +
> /// Default transition latency value in nanoseconds.
> pub const ETERNAL_LATENCY_NS: u32 = bindings::CPUFREQ_ETERNAL as u32;
>
> @@ -855,10 +859,8 @@ fn register_em(_policy: &mut Policy) {
> /// cpufreq::Registration::<FooDriver>::new_foreign_owned(dev).unwrap();
> /// }
> /// ```
> -pub struct Registration<T: Driver> {
> - drv: KBox<UnsafeCell<bindings::cpufreq_driver>>,
> - _p: PhantomData<T>,
> -}
> +#[repr(transparent)]
> +pub struct Registration<T: Driver>(NonNull<bindings::cpufreq_driver>, PhantomData<T>);
>
> // SAFETY: `Registration` doesn't offer any methods or access to fields when shared between threads
> // or CPUs, so it is safe to share it.
> @@ -870,135 +872,136 @@ unsafe impl<T: Driver> Sync for Registration<T> {}
> unsafe impl<T: Driver> Send for Registration<T> {}
>
> impl<T: Driver> Registration<T> {
> - /// Registers a CPU frequency driver with the cpufreq core.
> - pub fn new() -> Result<Self> {
> - // Required due to Rust 1.82's stricter handling of `unsafe` in mutable statics. The
> - // `unsafe` blocks aren't required anymore with later versions.
> - #![allow(unused_unsafe)]
> -
> - let mut drv = KBox::new(
> - UnsafeCell::new(bindings::cpufreq_driver::default()),
> - GFP_KERNEL,
> - )?;
> - let drv_ref = drv.get_mut();
> -
> - // Account for the trailing null byte.
> - let len = T::NAME.len() + 1;
> - if len > drv_ref.name.len() {
> - return Err(EINVAL);
> - };
> -
> - // SAFETY: `T::NAME` is a valid `CStr`, and we are copying it to an array of equal or
> - // larger size.
> - let name = unsafe { &*(T::NAME.as_bytes_with_nul() as *const [u8]) };
> - drv_ref.name[..len].copy_from_slice(name);
> -
> - drv_ref.boost_enabled = T::BOOST_ENABLED;
> - drv_ref.flags = T::FLAGS;
> + const VTABLE: bindings::cpufreq_driver = bindings::cpufreq_driver {
> + name: Self::copy_name(T::NAME),
> + boost_enabled: T::BOOST_ENABLED,
> + flags: T::FLAGS,
>
> // Initialize mandatory callbacks.
> - drv_ref.init = Some(Self::init_callback);
> - drv_ref.verify = Some(Self::verify_callback);
> + init: Some(Self::init_callback),
> + verify: Some(Self::verify_callback),
>
> // Initialize optional callbacks based on the traits of `T`.
> - drv_ref.setpolicy = if T::HAS_SETPOLICY {
> + setpolicy: if T::HAS_SETPOLICY {
> Some(Self::setpolicy_callback)
> } else {
> None
> - };
> - drv_ref.target = if T::HAS_TARGET {
> + },
> + target: if T::HAS_TARGET {
> Some(Self::target_callback)
> } else {
> None
> - };
> - drv_ref.target_index = if T::HAS_TARGET_INDEX {
> + },
> + target_index: if T::HAS_TARGET_INDEX {
> Some(Self::target_index_callback)
> } else {
> None
> - };
> - drv_ref.fast_switch = if T::HAS_FAST_SWITCH {
> + },
> + fast_switch: if T::HAS_FAST_SWITCH {
> Some(Self::fast_switch_callback)
> } else {
> None
> - };
> - drv_ref.adjust_perf = if T::HAS_ADJUST_PERF {
> + },
> + adjust_perf: if T::HAS_ADJUST_PERF {
> Some(Self::adjust_perf_callback)
> } else {
> None
> - };
> - drv_ref.get_intermediate = if T::HAS_GET_INTERMEDIATE {
> + },
> + get_intermediate: if T::HAS_GET_INTERMEDIATE {
> Some(Self::get_intermediate_callback)
> } else {
> None
> - };
> - drv_ref.target_intermediate = if T::HAS_TARGET_INTERMEDIATE {
> + },
> + target_intermediate: if T::HAS_TARGET_INTERMEDIATE {
> Some(Self::target_intermediate_callback)
> } else {
> None
> - };
> - drv_ref.get = if T::HAS_GET {
> + },
> + get: if T::HAS_GET {
> Some(Self::get_callback)
> } else {
> None
> - };
> - drv_ref.update_limits = if T::HAS_UPDATE_LIMITS {
> + },
> + update_limits: if T::HAS_UPDATE_LIMITS {
> Some(Self::update_limits_callback)
> } else {
> None
> - };
> - drv_ref.bios_limit = if T::HAS_BIOS_LIMIT {
> + },
> + bios_limit: if T::HAS_BIOS_LIMIT {
> Some(Self::bios_limit_callback)
> } else {
> None
> - };
> - drv_ref.online = if T::HAS_ONLINE {
> + },
> + online: if T::HAS_ONLINE {
> Some(Self::online_callback)
> } else {
> None
> - };
> - drv_ref.offline = if T::HAS_OFFLINE {
> + },
> + offline: if T::HAS_OFFLINE {
> Some(Self::offline_callback)
> } else {
> None
> - };
> - drv_ref.exit = if T::HAS_EXIT {
> + },
> + exit: if T::HAS_EXIT {
> Some(Self::exit_callback)
> } else {
> None
> - };
> - drv_ref.suspend = if T::HAS_SUSPEND {
> + },
> + suspend: if T::HAS_SUSPEND {
> Some(Self::suspend_callback)
> } else {
> None
> - };
> - drv_ref.resume = if T::HAS_RESUME {
> + },
> + resume: if T::HAS_RESUME {
> Some(Self::resume_callback)
> } else {
> None
> - };
> - drv_ref.ready = if T::HAS_READY {
> + },
> + ready: if T::HAS_READY {
> Some(Self::ready_callback)
> } else {
> None
> - };
> - drv_ref.set_boost = if T::HAS_SET_BOOST {
> + },
> + set_boost: if T::HAS_SET_BOOST {
> Some(Self::set_boost_callback)
> } else {
> None
> - };
> - drv_ref.register_em = if T::HAS_REGISTER_EM {
> + },
> + register_em: if T::HAS_REGISTER_EM {
> Some(Self::register_em_callback)
> } else {
> None
> - };
> + },
> + // SAFETY: All zeros is a valid value for `bindings::cpufreq_driver`.
> + ..unsafe { MaybeUninit::zeroed().assume_init() }
> + };
> +
> + const fn copy_name(name: &'static CStr) -> [c_char; CPUFREQ_NAME_LEN] {
> + let src = name.as_bytes_with_nul();
> + let mut dst = [0; CPUFREQ_NAME_LEN];
> +
> + build_assert!(src.len() <= CPUFREQ_NAME_LEN);
> +
> + let mut i = 0;
> + while i < src.len() {
> + dst[i] = src[i];
> + i += 1;
> + }
> +
> + dst
> + }
> +
> + /// Registers a CPU frequency driver with the cpufreq core.
> + pub fn new() -> Result<Self> {
> + let drv = &Self::VTABLE as *const _ as *mut _;
Hm, I didn't think of that, maybe it's better to store a *const directly instead
of a NonNull, given that VTABLE can't be altered.
Tamir is trying to clean up 'as' casts in [1]. So, I'd write this as
let drv: *const bindings::cpufreq_driver = &Self::VTABLE;
[1] https://lore.kernel.org/rust-for-linux/20250409-ptr-as-ptr-v8-0-3738061534ef@gmail.com/
>
> // SAFETY: It is safe to register the driver with the cpufreq core in the kernel C code.
> - to_result(unsafe { bindings::cpufreq_register_driver(drv_ref) })?;
> + to_result(unsafe { bindings::cpufreq_register_driver(drv) })?;
>
> - Ok(Self {
> - drv,
> - _p: PhantomData,
> - })
> + Ok(Self(
> + NonNull::new(drv.cast()).ok_or(AllocError)?,
> + PhantomData,
> + ))
> }
>
> /// Same as [`Registration::new`], but does not return a [`Registration`] instance.
> @@ -1259,9 +1262,7 @@ extern "C" fn register_em_callback(ptr: *mut bindings::cpufreq_policy) {
> impl<T: Driver> Drop for Registration<T> {
> // Removes the `Registration` from the kernel, if it has initialized successfully earlier.
> fn drop(&mut self) {
> - let drv = self.drv.get_mut();
> -
> // SAFETY: The driver was earlier registered from `new`.
> - unsafe { bindings::cpufreq_unregister_driver(drv) };
> + unsafe { bindings::cpufreq_unregister_driver(self.0.as_ptr()) };
> }
> }
© 2016 - 2026 Red Hat, Inc.