Message-ID: <Z_904KuBhKbO738_@pollux>
Date: Wed, 16 Apr 2025 11:14:08 +0200
From: Danilo Krummrich <dakr@...nel.org>
To: Viresh Kumar <viresh.kumar@...aro.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 Wed, Apr 16, 2025 at 12:09:28PM +0530, Viresh Kumar wrote:
> +/// CPU frequency table.
> +///
> +/// Rust abstraction for the C `struct cpufreq_frequency_table`.
> +///
> +/// # Invariants
> +///
> +/// A [`Table`] instance always corresponds to a valid C `struct cpufreq_frequency_table`.
> +///
> +/// The callers must ensure that the `struct cpufreq_frequency_table` is valid for access and
> +/// remains valid for the lifetime of the returned reference.
> +///
> +/// ## Examples
> +///
> +/// The following example demonstrates how to read a frequency value from [`Table`].
> +///
> +/// ```
> +/// use kernel::cpufreq::Policy;
> +///
> +/// 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));
> +///     }
> +/// }
> +/// ```
> +#[allow(dead_code)]

Why is this needed?

> +#[repr(transparent)]
> +pub struct Table(Opaque<bindings::cpufreq_frequency_table>);
> +
> +impl Table {
> +    /// Creates a reference to an existing C `struct cpufreq_frequency_table` pointer.
> +    ///
> +    /// # Safety
> +    ///
> +    /// The caller must ensure that `ptr` is valid for reading and remains valid for the lifetime
> +    /// of the returned reference.
> +    #[inline]
> +    pub unsafe fn from_raw<'a>(ptr: *const bindings::cpufreq_frequency_table) -> &'a Self {
> +        // SAFETY: Guaranteed by the safety requirements of the function.
> +        //
> +        // INVARIANT: The caller ensures that `ptr` is valid for reading and remains valid for the
> +        // lifetime of the returned reference.
> +        unsafe { &*ptr.cast() }
> +    }
> +
> +    /// Returns the raw mutable pointer to the C `struct cpufreq_frequency_table`.
> +    #[inline]
> +    pub fn as_raw(&self) -> *mut bindings::cpufreq_frequency_table {
> +        let this: *const Self = self;
> +        this.cast_mut().cast()
> +    }
> +
> +    /// 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> {
> +        // 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.
> +        Ok(Hertz::from_khz(unsafe {
> +            (*self.as_raw().add(index)).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 {
> +        // 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 }
> +    }
> +
> +    /// 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 {
> +        // 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 }
> +    }

Those three functions above look like they're supposed to be used directly by
drivers, but are unsafe. :(

It looks like the reason for them being unsafe is that with only the pointer to
the struct cpufreq_frequency_table array we don't know the length of the array.

However, a Table instance seems to come from TableBox, which *does* know the
length of the KVec<bindings::cpufreq_frequency_table>. Why can't we just preserve the
length and provide a safe API?

> +}
> +
> +/// CPU frequency table owned and pinned in memory, created from a [`TableBuilder`].
> +pub struct TableBox {
> +    #[allow(dead_code)]

Why?

Powered by blists - more mailing lists