Re: [PATCH V10 11/15] rust: cpufreq: Add initial abstractions for cpufreq framework

[Danilo Krummrich]

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?