Re: [PATCH V8 12/14] rust: Extend cpufreq bindings for driver registration

Danilo Krummrich
2025-02-06T12:04:28Z

On Thu, Feb 06, 2025 at 02:58:33PM +0530, Viresh Kumar wrote:
> This extends the cpufreq bindings with bindings for registering a
> driver.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> rust/kernel/cpufreq.rs | 475 ++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 473 insertions(+), 2 deletions(-)
>
> diff --git a/rust/kernel/cpufreq.rs b/rust/kernel/cpufreq.rs
> index 63ea816017c0..f92259d339d3 100644
> --- a/rust/kernel/cpufreq.rs
> +++ b/rust/kernel/cpufreq.rs
> @@ -9,14 +9,17 @@
> use crate::{
> bindings, clk, 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},
> prelude::*,
> types::ForeignOwnable,
> };
>
> use core::{
> + cell::UnsafeCell,
> + marker::PhantomData,
> pin::Pin,
> - ptr::self,
> + ptr::{self, addr_of_mut},
> };
>
> use macros::vtable;
> @@ -579,3 +582,471 @@ fn register_em(_policy: &mut Policy) {
> build_error!(VTABLE_DEFAULT_ERROR)
> }
> }
> +
> +/// Registration of a cpufreq driver.
> +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 cpufreq driver with the rest of the kernel.
> + pub fn new(name: &'static CStr, data: T::Data, flags: u16, boost: bool) -> Result<Self> {
> + let mut drv = KBox::new(
> + UnsafeCell::new(bindings::cpufreq_driver::default()),
> + GFP_KERNEL,
> + )?;
> + let drv_ref = drv.get_mut();
> +
> + // Account for the trailing null character.
> + 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;
> +
> + // Allocate an array of 3 pointers to be passed to the C code.
> + let mut attr = KBox::new([ptr::null_mut(); 3], GFP_KERNEL)?;
> + let mut next = 0;
> +
> + // SAFETY: The C code returns a valid pointer here, which is again passed to the C code in
> + // an array.
> + attr[next] =
> + unsafe { addr_of_mut!(bindings::cpufreq_freq_attr_scaling_available_freqs) as *mut _ };
> + next += 1;
> +
> + if boost {
> + // SAFETY: The C code returns a valid pointer here, which is again passed to the C code
> + // in an array.
> + attr[next] =
> + unsafe { addr_of_mut!(bindings::cpufreq_freq_attr_scaling_boost_freqs) as *mut _ };
> + next += 1;
> + }
> + attr[next] = ptr::null_mut();
> +
> + // Pass the ownership of the memory block to the C code. This will be freed when
> + // the [`Registration`] object goes out of scope.
> + drv_ref.attr = KBox::leak(attr) as *mut _;

I think this should be KBox::into_raw() instead.

> +
> + // Initialize mandatory callbacks.
> + drv_ref.init = Some(Self::init_callback);
> + drv_ref.verify = Some(Self::verify_callback);
> +
> + // Initialize optional callbacks.
> + 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 may call 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 C code.
> + to_result(unsafe { bindings::cpufreq_register_driver(drv_ref) })?;
> +
> + Ok(Self {
> + drv,
> + _p: PhantomData,
> + })
> + }

...

> +// cpufreq driver callbacks.
> +impl<T: Driver> Registration<T> {
> + // Policy's init callback.
> + extern "C" fn init_callback(ptr: *mut bindings::cpufreq_policy) -> core::ffi::c_int {

I suggest to convert all the ffi types to kernel::ffi::*.

> + from_result(|| {
> + // SAFETY: `ptr` is valid by the contract with the C code. `policy` is alive only for
> + // the duration of this call, so it is guaranteed to remain alive for the lifetime of
> + // `ptr`.
> + let mut policy = unsafe { Policy::from_raw_policy(ptr) };
> +
> + let data = T::init(&mut policy)?;
> + policy.set_data(data)?;
> + Ok(0)
> + })
> + }

...

> +impl<T: Driver> Drop for Registration<T> {
> + // Removes the registration from the kernel if it has completed successfully before.
> + fn drop(&mut self) {
> + pr_info!("Registration dropped\n");

This should be dropped.

> + let drv = self.drv.get_mut();
> +
> + // SAFETY: The driver was earlier registered from `new()`.
> + unsafe { bindings::cpufreq_unregister_driver(drv) };
> +
> + // Free the previously leaked memory to the C code.
> + if !drv.attr.is_null() {
> + // SAFETY: The pointer was earlier initialized from the result of `KBox::leak`.

Box::leak() returns a mutable reference, whereas Box::into_raw() returns a raw
pointer for exactly this purpose.

Now that I think of it, maybe Box::leak() should even be removed, since it
almost never makes any sense in the kernel.

> + unsafe { drop(KBox::from_raw(drv.attr)) };

This could just be

let _ = unsafe { KBox::from_raw(drv.attr) };

At least drop() should not be within the unsafe block.

> + }
> +
> + // Free data
> + drop(self.clear_data());

No need for drop(), but I also don't mind to be explicit.

> + }
> +}
> --
> 2.31.1.272.g89b43f80a514
>
>