Message-ID: <20250417083440.q27nwcizlxb76vzn@vireshk-i7>
Date: Thu, 17 Apr 2025 14:04:40 +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 11/15] rust: cpufreq: Add initial abstractions for
 cpufreq framework

On 16-04-25, 14:25, Danilo Krummrich wrote:
> This sounds like you could just abstract the index passed through the callback
> in some trusted type (e.g. cpufreq::TableIndex) and let the cpufreq::Table
> methods take this trusted index type, rather than a raw usize, which would also
> make the methods safe.

diff --git a/rust/kernel/cpufreq.rs b/rust/kernel/cpufreq.rs
index 87a54a8af198..4de7fea7bf3f 100644
--- a/rust/kernel/cpufreq.rs
+++ b/rust/kernel/cpufreq.rs
@@ -61,12 +61,12 @@ pub mod flags {
     pub const NO_AUTO_DYNAMIC_SWITCHING: u16 = 1 << 6;
 }
 
-// Relations from the C code.
+/// Relations from the C code.
 const CPUFREQ_RELATION_L: u32 = 0;
 const CPUFREQ_RELATION_H: u32 = 1;
 const CPUFREQ_RELATION_C: u32 = 2;
 
-// Can be used with any of the above values.
+/// Can be used with any of the above values.
 const CPUFREQ_RELATION_E: u32 = 1 << 2;
 
 /// CPU frequency selection relations.
@@ -157,6 +157,36 @@ pub fn generic_verify(&self) -> Result<()> {
     }
 }
 
+/// The frequency table index.
+///
+/// Represents index with a frequency table.
+///
+/// # Invariants
+///
+/// The index must correspond to a valid entry in the [`Table`] it is used for.
+#[derive(Copy, Clone, PartialEq, Eq, Debug)]
+pub struct TableIndex(usize);
+
+impl TableIndex {
+    /// Creates an instance of [`TableIndex`].
+    ///
+    /// # Safety
+    ///
+    /// The caller must ensure that `index` correspond to a valid entry in the [`Table`] it is used
+    /// for.
+    pub unsafe fn new(index: usize) -> Self {
+        // INVARIANT: The caller ensures that `index` correspond to a valid entry in the [`Table`].
+        Self(index)
+    }
+}
+
+impl From<TableIndex> for usize {
+    #[inline]
+    fn from(index: TableIndex) -> Self {
+        index.0
+    }
+}
+
 /// CPU frequency table.
 ///
 /// Rust abstraction for the C `struct cpufreq_frequency_table`.
@@ -173,20 +203,19 @@ pub fn generic_verify(&self) -> Result<()> {
 /// The following example demonstrates how to read a frequency value from [`Table`].
 ///
 /// ```
-/// use kernel::cpufreq::Policy;
+/// use kernel::cpufreq::{Policy, TableIndex};
 ///
 /// fn show_freq(policy: &Policy) {
 ///     let table = policy.freq_table().unwrap();
 ///
-///     // SAFETY: The index values passed are correct.
-///     unsafe {
-///         pr_info!("The frequency at index 0 is: {:?}\n", table.freq(0).unwrap());
-///         pr_info!("The flags at index 0 is: {}\n", table.flags(0));
-///         pr_info!("The data at index 0 is: {}\n", table.data(0));
-///     }
+///     // SAFETY: Index is a valid entry in the table.
+///     let index = unsafe { TableIndex::new(0) };
+///
+///     pr_info!("The frequency at index 0 is: {:?}\n", table.freq(index).unwrap());
+///     pr_info!("The flags at index 0 is: {}\n", table.flags(index));
+///     pr_info!("The data at index 0 is: {}\n", table.data(index));
 /// }
 /// ```
-#[allow(dead_code)]
 #[repr(transparent)]
 pub struct Table(Opaque<bindings::cpufreq_frequency_table>);
 
@@ -214,47 +243,34 @@ pub fn as_raw(&self) -> *mut bindings::cpufreq_frequency_table {
     }
 
     /// Returns frequency at `index` in the [`Table`].
-    ///
-    /// # Safety
-    ///
-    /// The caller must ensure that `index` corresponds to a valid table entry.
     #[inline]
-    pub unsafe fn freq(&self, index: usize) -> Result<Hertz> {
+    pub fn freq(&self, index: TableIndex) -> Result<Hertz> {
         // SAFETY: By the type invariant, the pointer stored in `self` is valid and `index` is
-        // guaranteed to be valid by the safety requirements of the function.
+        // guaranteed to be valid by its safety requirements.
         Ok(Hertz::from_khz(unsafe {
-            (*self.as_raw().add(index)).frequency.try_into()?
+            (*self.as_raw().add(index.into())).frequency.try_into()?
         }))
     }
 
     /// Returns flags at `index` in the [`Table`].
-    ///
-    /// # Safety
-    ///
-    /// The caller must ensure that `index` corresponds to a valid table entry.
     #[inline]
-    pub unsafe fn flags(&self, index: usize) -> u32 {
+    pub fn flags(&self, index: TableIndex) -> u32 {
         // SAFETY: By the type invariant, the pointer stored in `self` is valid and `index` is
-        // guaranteed to be valid by the safety requirements of the function.
-        unsafe { (*self.as_raw().add(index)).flags }
+        // guaranteed to be valid by its safety requirements.
+        unsafe { (*self.as_raw().add(index.into())).flags }
     }
 
     /// Returns data at `index` in the [`Table`].
-    ///
-    /// # Safety
-    ///
-    /// The caller must ensure that `index` corresponds to a valid table entry.
     #[inline]
-    pub unsafe fn data(&self, index: usize) -> u32 {
+    pub fn data(&self, index: TableIndex) -> u32 {
         // SAFETY: By the type invariant, the pointer stored in `self` is valid and `index` is
-        // guaranteed to be valid by the safety requirements of the function.
-        unsafe { (*self.as_raw().add(index)).driver_data }
+        // guaranteed to be valid by its safety requirements.
+        unsafe { (*self.as_raw().add(index.into())).driver_data }
     }
 }
 
 /// CPU frequency table owned and pinned in memory, created from a [`TableBuilder`].
 pub struct TableBox {
-    #[allow(dead_code)]
     entries: Pin<KVec<bindings::cpufreq_frequency_table>>,
 }
 
@@ -302,7 +318,7 @@ fn deref(&self) -> &Self::Target {
 /// The following example demonstrates how to create a CPU frequency table.
 ///
 /// ```
-/// use kernel::cpufreq::TableBuilder;
+/// use kernel::cpufreq::{TableBuilder, TableIndex};
 /// use kernel::clk::Hertz;
 ///
 /// let mut builder = TableBuilder::new();
@@ -315,15 +331,18 @@ fn deref(&self) -> &Self::Target {
 ///
 /// let table = builder.to_table().unwrap();
 ///
-/// // SAFETY: The index values passed are correct.
-/// unsafe {
-///     assert_eq!(table.freq(0), Ok(Hertz::from_mhz(700)));
-///     assert_eq!(table.flags(0), 0);
-///     assert_eq!(table.data(0), 1);
+/// // SAFETY: Index values correspond to valid entries in the table.
+/// let (index0, index2) = unsafe { (TableIndex::new(0), TableIndex::new(2)) };
 ///
-///     assert_eq!(table.freq(2), Ok(Hertz::from_mhz(900)));
-///     assert_eq!(table.flags(2), 4);
-///     assert_eq!(table.data(2), 5);
-/// }
+/// assert_eq!(table.freq(index0), Ok(Hertz::from_mhz(700)));
+/// assert_eq!(table.flags(index0), 0);
+/// assert_eq!(table.data(index0), 1);
+///
+/// assert_eq!(table.freq(index2), Ok(Hertz::from_mhz(900)));
+/// assert_eq!(table.flags(index2), 4);
+/// assert_eq!(table.data(index2), 5);
 /// ```
 #[derive(Default)]

-- 
viresh

Powered by blists - more mailing lists