Message-ID: <20250417082906.dfiproihig7egkp2@vireshk-i7>
Date: Thu, 17 Apr 2025 13:59:06 +0530
From: Viresh Kumar <viresh.kumar@...aro.org>
To: Danilo Krummrich <dakr@...nel.org>
Cc: "Rafael J. Wysocki" <rafael@...nel.org>,
	Miguel Ojeda <miguel.ojeda.sandonis@...il.com>,
	Danilo Krummrich <dakr@...hat.com>, Miguel Ojeda <ojeda@...nel.org>,
	Alex Gaynor <alex.gaynor@...il.com>,
	Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
	Björn Roy Baron <bjorn3_gh@...tonmail.com>,
	Benno Lossin <benno.lossin@...ton.me>,
	Andreas Hindborg <a.hindborg@...nel.org>,
	Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>,
	linux-pm@...r.kernel.org,
	Vincent Guittot <vincent.guittot@...aro.org>,
	Stephen Boyd <sboyd@...nel.org>, Nishanth Menon <nm@...com>,
	rust-for-linux@...r.kernel.org,
	Manos Pitsidianakis <manos.pitsidianakis@...aro.org>,
	Alex Bennée <alex.bennee@...aro.org>,
	Joakim Bech <joakim.bech@...aro.org>, Rob Herring <robh@...nel.org>,
	Yury Norov <yury.norov@...il.com>, Burak Emir <bqe@...gle.com>,
	Rasmus Villemoes <linux@...musvillemoes.dk>,
	Russell King <linux@...linux.org.uk>, linux-clk@...r.kernel.org,
	Michael Turquette <mturquette@...libre.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH V10 13/15] rust: cpufreq: Extend abstractions for driver
 registration

On 16-04-25, 12:59, Danilo Krummrich wrote:
> Anyways, that doesn't help for now. Unfortunately, I think you actually need to
> dynamically allocate it. There's no need to revert everything though. You can
> just allocate a new KBox from VTABLE, i.e.
> 
> 	let vtable = KBox::new(Self::VTABLE, GFP_KERNEL)?;

Thanks. Here is the diff for this patch:

diff --git a/rust/kernel/cpufreq.rs b/rust/kernel/cpufreq.rs
index e8f4e18002c9..8cb62641c64b 100644
--- a/rust/kernel/cpufreq.rs
+++ b/rust/kernel/cpufreq.rs
@@ -9,7 +9,6 @@
 //! Reference: <https://docs.kernel.org/admin-guide/pm/cpufreq.html>
 
 use crate::{
-    alloc::AllocError,
     clk::{Clk, Hertz},
     cpumask,
     device::Device,
@@ -22,16 +21,17 @@
 };
 
 use core::{
+    cell::UnsafeCell,
     marker::PhantomData,
     mem::MaybeUninit,
     ops::{Deref, DerefMut},
     pin::Pin,
-    ptr::{self, NonNull},
+    ptr,
 };
 
 use macros::vtable;
 
-// Maximum length of CPU frequency driver's name.
+/// 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.
@@ -872,7 +872,7 @@ fn register_em(_policy: &mut Policy) {
 ///         data.generic_verify()
 ///     }
 ///
-///     fn target_index(policy: &mut cpufreq::Policy, index: u32) -> Result<()> {
+///     fn target_index(policy: &mut cpufreq::Policy, index: cpufreq::TableIndex) -> Result<()> {
 ///         // Update CPU frequency
 ///         Ok(())
 ///     }
@@ -887,15 +887,15 @@ fn register_em(_policy: &mut Policy) {
 /// }
 /// ```
 #[repr(transparent)]
-pub struct Registration<T: Driver>(NonNull<bindings::cpufreq_driver>, PhantomData<T>);
+pub struct Registration<T: Driver>(KBox<UnsafeCell<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.
+/// 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.
+/// SAFETY: Registration with and unregistration from the cpufreq subsystem can happen from any
+/// thread.
 unsafe impl<T: Driver> Send for Registration<T> {}
 
 impl<T: Driver> Registration<T> {
@@ -1020,16 +1020,14 @@ impl<T: Driver> Registration<T> {
 
     /// Registers a CPU frequency driver with the cpufreq core.
     pub fn new() -> Result<Self> {
-        let drv: *const bindings::cpufreq_driver = &Self::VTABLE;
-        let drv = drv.cast_mut();
+        // We can't use `&Self::VTABLE` directly because the cpufreq core modifies some fields in
+        // the C `struct cpufreq_driver`, which requires a mutable reference.
+        let mut drv = KBox::new(UnsafeCell::new(Self::VTABLE), GFP_KERNEL)?;
 
-        // 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) })?;
+        // SAFETY: `drv` is guaranteed to be valid for the lifetime of `Registration`.
+        to_result(unsafe { bindings::cpufreq_register_driver(drv.get_mut()) })?;
 
-        Ok(Self(
-            NonNull::new(drv.cast()).ok_or(AllocError)?,
-            PhantomData,
-        ))
+        Ok(Self(drv, PhantomData))
     }
 
     /// Same as [`Registration::new`], but does not return a [`Registration`] instance.
@@ -1037,16 +1035,15 @@ pub fn new() -> Result<Self> {
     /// Instead the [`Registration`] is owned by [`Devres`] and will be revoked / dropped, once the
     /// device is detached.
     pub fn new_foreign_owned(dev: &Device) -> Result<()> {
-        Devres::new_foreign_owned(dev, Self::new()?, GFP_KERNEL)?;
-        Ok(())
+        Devres::new_foreign_owned(dev, Self::new()?, GFP_KERNEL)
     }
 }
 
-// CPU frequency driver callbacks.
+/// CPU frequency driver callbacks.
 impl<T: Driver> Registration<T> {
-    // Driver's `init` callback.
-    //
-    // SAFETY: Called from C. Inputs must be valid pointers.
+    /// 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
@@ -1059,9 +1056,9 @@ extern "C" fn init_callback(ptr: *mut bindings::cpufreq_policy) -> kernel::ffi::
         })
     }
 
-    // Driver's `exit` callback.
-    //
-    // SAFETY: Called from C. Inputs must be valid pointers.
+    /// 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`.
@@ -1071,9 +1068,9 @@ extern "C" fn exit_callback(ptr: *mut bindings::cpufreq_policy) {
         let _ = T::exit(policy, data);
     }
 
-    // Driver's `online` callback.
-    //
-    // SAFETY: Called from C. Inputs must be valid pointers.
+    /// 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
@@ -1083,9 +1080,9 @@ extern "C" fn online_callback(ptr: *mut bindings::cpufreq_policy) -> kernel::ffi
         })
     }
 
-    // Driver's `offline` callback.
-    //
-    // SAFETY: Called from C. Inputs must be valid pointers.
+    /// 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
@@ -1095,9 +1092,9 @@ extern "C" fn offline_callback(ptr: *mut bindings::cpufreq_policy) -> kernel::ff
         })
     }
 
-    // Driver's `suspend` callback.
-    //
-    // SAFETY: Called from C. Inputs must be valid pointers.
+    /// 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
@@ -1107,9 +1104,9 @@ extern "C" fn suspend_callback(ptr: *mut bindings::cpufreq_policy) -> kernel::ff
         })
     }
 
-    // Driver's `resume` callback.
-    //
-    // SAFETY: Called from C. Inputs must be valid pointers.
+    /// 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
@@ -1119,9 +1116,9 @@ extern "C" fn resume_callback(ptr: *mut bindings::cpufreq_policy) -> kernel::ffi
         })
     }
 
-    // Driver's `ready` callback.
-    //
-    // SAFETY: Called from C. Inputs must be valid pointers.
+    /// 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`.
@@ -1129,9 +1126,9 @@ extern "C" fn ready_callback(ptr: *mut bindings::cpufreq_policy) {
         T::ready(policy);
     }
 
-    // Driver's `verify` callback.
-    //
-    // SAFETY: Called from C. Inputs must be valid pointers.
+    /// 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
@@ -1141,9 +1138,9 @@ extern "C" fn verify_callback(ptr: *mut bindings::cpufreq_policy_data) -> kernel
         })
     }
 
-    // Driver's `setpolicy` callback.
-    //
-    // SAFETY: Called from C. Inputs must be valid pointers.
+    /// 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
@@ -1153,9 +1150,9 @@ extern "C" fn setpolicy_callback(ptr: *mut bindings::cpufreq_policy) -> kernel::
         })
     }
 
-    // Driver's `target` callback.
-    //
-    // SAFETY: Called from C. Inputs must be valid pointers.
+    /// 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,
@@ -1169,9 +1166,9 @@ extern "C" fn target_callback(
         })
     }
 
-    // Driver's `target_index` callback.
-    //
-    // SAFETY: Called from C. Inputs must be valid pointers.
+    /// 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,
@@ -1180,13 +1177,18 @@ extern "C" fn target_index_callback(
             // 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) };
+
+            // SAFETY: The C code guarantees that `index` corresponds to a valid entry in the
+            // frequency table.
+            let index = unsafe { TableIndex::new(index as usize) };
+
             T::target_index(policy, index).map(|()| 0)
         })
     }
 
-    // Driver's `fast_switch` callback.
-    //
-    // SAFETY: Called from C. Inputs must be valid pointers.
+    /// 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,
@@ -1197,7 +1199,7 @@ extern "C" fn fast_switch_callback(
         T::fast_switch(policy, target_freq)
     }
 
-    // Driver's `adjust_perf` callback.
+    /// Driver's `adjust_perf` callback.
     extern "C" fn adjust_perf_callback(
         cpu: u32,
         min_perf: usize,
@@ -1209,9 +1211,9 @@ extern "C" fn adjust_perf_callback(
         }
     }
 
-    // Driver's `get_intermediate` callback.
-    //
-    // SAFETY: Called from C. Inputs must be valid pointers.
+    /// 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,
@@ -1219,12 +1221,17 @@ extern "C" fn get_intermediate_callback(
         // 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) };
+
+        // SAFETY: The C code guarantees that `index` corresponds to a valid entry in the
+        // frequency table.
+        let index = unsafe { TableIndex::new(index as usize) };
+
         T::get_intermediate(policy, index)
     }
 
-    // Driver's `target_intermediate` callback.
-    //
-    // SAFETY: Called from C. Inputs must be valid pointers.
+    /// 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,
@@ -1233,25 +1240,30 @@ extern "C" fn target_intermediate_callback(
             // 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) };
+
+            // SAFETY: The C code guarantees that `index` corresponds to a valid entry in the
+            // frequency table.
+            let index = unsafe { TableIndex::new(index as usize) };
+
             T::target_intermediate(policy, index).map(|()| 0)
         })
     }
 
-    // Driver's `get` callback.
+    /// 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.
+    /// 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.
+    /// 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)?;
@@ -1261,9 +1273,9 @@ extern "C" fn bios_limit_callback(cpu: i32, limit: *mut u32) -> kernel::ffi::c_i
         })
     }
 
-    // Driver's `set_boost` callback.
-    //
-    // SAFETY: Called from C. Inputs must be valid pointers.
+    /// 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,
@@ -1276,9 +1288,9 @@ extern "C" fn set_boost_callback(
         })
     }
 
-    // Driver's `register_em` callback.
-    //
-    // SAFETY: Called from C. Inputs must be valid pointers.
+    /// 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`.
@@ -1288,9 +1300,9 @@ 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.
+    /// Unregisters with the cpufreq core.
     fn drop(&mut self) {
-        // SAFETY: The driver was earlier registered from `new`.
-        unsafe { bindings::cpufreq_unregister_driver(self.0.as_ptr()) };
+        // SAFETY: `self.0` is guaranteed to be valid for the lifetime of `Registration`.
+        unsafe { bindings::cpufreq_unregister_driver(self.0.get_mut()) };
     }
 }

-- 
viresh

Powered by blists - more mailing lists