Re: [PATCH V9 15/17] rust: cpufreq: Extend abstractions for driver registration

[Danilo Krummrich <dakr@kernel.org>]

On Mon, Apr 14, 2025 at 04:22:12PM +0530, Viresh Kumar wrote:
> On 14-04-25, 11:39, Danilo Krummrich wrote:
> > 	const VTABLE: bindings::cpufreq_driver = bindings::cpufreq_driver {
> > 	   name: Self::copy_name(T::NAME),
> > 	   boost_enabled: T::BOOST_ENABLED,
> > 	   flags: T::FLAGS,
> > 	   [...]
> > 	}
> 
> Ahh, thanks for this.

The diff below looks good!

One nit below.

> diff --git a/rust/kernel/cpufreq.rs b/rust/kernel/cpufreq.rs
> index 9b275d4d3eb6..a6e660d46304 100644
> --- a/rust/kernel/cpufreq.rs
> +++ b/rust/kernel/cpufreq.rs
> @@ -9,28 +9,32 @@
>  //! Reference: <https://docs.kernel.org/admin-guide/pm/cpufreq.html>
>  
>  use crate::{
> +    alloc::AllocError,
>      bindings,
>      clk::{Clk, Hertz},
>      cpumask,
>      device::Device,
>      devres::Devres,
>      error::{code::*, from_err_ptr, from_result, to_result, Result, VTABLE_DEFAULT_ERROR},
> -    ffi::c_ulong,
> +    ffi::{c_char, c_ulong},
>      prelude::*,
>      types::ForeignOwnable,
>      types::Opaque,
>  };
>  
>  use core::{
> -    cell::UnsafeCell,
>      marker::PhantomData,
> +    mem::MaybeUninit,
>      ops::{Deref, DerefMut},
>      pin::Pin,
> -    ptr,
> +    ptr::{self, NonNull},
>  };
>  
>  use macros::vtable;
>  
> +// 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.
>  pub const ETERNAL_LATENCY_NS: u32 = bindings::CPUFREQ_ETERNAL as u32;
>  
> @@ -855,10 +859,8 @@ fn register_em(_policy: &mut Policy) {
>  ///     cpufreq::Registration::<FooDriver>::new_foreign_owned(dev).unwrap();
>  /// }
>  /// ```
> -pub struct Registration<T: Driver> {
> -    drv: KBox<UnsafeCell<bindings::cpufreq_driver>>,
> -    _p: PhantomData<T>,
> -}
> +#[repr(transparent)]
> +pub struct Registration<T: Driver>(NonNull<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.
> @@ -870,135 +872,136 @@ unsafe impl<T: Driver> Sync for Registration<T> {}
>  unsafe impl<T: Driver> Send for Registration<T> {}
>  
>  impl<T: Driver> Registration<T> {
> -    /// Registers a CPU frequency driver with the cpufreq core.
> -    pub fn new() -> Result<Self> {
> -        // Required due to Rust 1.82's stricter handling of `unsafe` in mutable statics. The
> -        // `unsafe` blocks aren't required anymore with later versions.
> -        #![allow(unused_unsafe)]
> -
> -        let mut drv = KBox::new(
> -            UnsafeCell::new(bindings::cpufreq_driver::default()),
> -            GFP_KERNEL,
> -        )?;
> -        let drv_ref = drv.get_mut();
> -
> -        // Account for the trailing null byte.
> -        let len = T::NAME.len() + 1;
> -        if len > drv_ref.name.len() {
> -            return Err(EINVAL);
> -        };
> -
> -        // SAFETY: `T::NAME` is a valid `CStr`, and we are copying it to an array of equal or
> -        // larger size.
> -        let name = unsafe { &*(T::NAME.as_bytes_with_nul() as *const [u8]) };
> -        drv_ref.name[..len].copy_from_slice(name);
> -
> -        drv_ref.boost_enabled = T::BOOST_ENABLED;
> -        drv_ref.flags = T::FLAGS;
> +    const VTABLE: bindings::cpufreq_driver = bindings::cpufreq_driver {
> +        name: Self::copy_name(T::NAME),
> +        boost_enabled: T::BOOST_ENABLED,
> +        flags: T::FLAGS,
>  
>          // Initialize mandatory callbacks.
> -        drv_ref.init = Some(Self::init_callback);
> -        drv_ref.verify = Some(Self::verify_callback);
> +        init: Some(Self::init_callback),
> +        verify: Some(Self::verify_callback),
>  
>          // Initialize optional callbacks based on the traits of `T`.
> -        drv_ref.setpolicy = if T::HAS_SETPOLICY {
> +        setpolicy: if T::HAS_SETPOLICY {
>              Some(Self::setpolicy_callback)
>          } else {
>              None
> -        };
> -        drv_ref.target = if T::HAS_TARGET {
> +        },
> +        target: if T::HAS_TARGET {
>              Some(Self::target_callback)
>          } else {
>              None
> -        };
> -        drv_ref.target_index = if T::HAS_TARGET_INDEX {
> +        },
> +        target_index: if T::HAS_TARGET_INDEX {
>              Some(Self::target_index_callback)
>          } else {
>              None
> -        };
> -        drv_ref.fast_switch = if T::HAS_FAST_SWITCH {
> +        },
> +        fast_switch: if T::HAS_FAST_SWITCH {
>              Some(Self::fast_switch_callback)
>          } else {
>              None
> -        };
> -        drv_ref.adjust_perf = if T::HAS_ADJUST_PERF {
> +        },
> +        adjust_perf: if T::HAS_ADJUST_PERF {
>              Some(Self::adjust_perf_callback)
>          } else {
>              None
> -        };
> -        drv_ref.get_intermediate = if T::HAS_GET_INTERMEDIATE {
> +        },
> +        get_intermediate: if T::HAS_GET_INTERMEDIATE {
>              Some(Self::get_intermediate_callback)
>          } else {
>              None
> -        };
> -        drv_ref.target_intermediate = if T::HAS_TARGET_INTERMEDIATE {
> +        },
> +        target_intermediate: if T::HAS_TARGET_INTERMEDIATE {
>              Some(Self::target_intermediate_callback)
>          } else {
>              None
> -        };
> -        drv_ref.get = if T::HAS_GET {
> +        },
> +        get: if T::HAS_GET {
>              Some(Self::get_callback)
>          } else {
>              None
> -        };
> -        drv_ref.update_limits = if T::HAS_UPDATE_LIMITS {
> +        },
> +        update_limits: if T::HAS_UPDATE_LIMITS {
>              Some(Self::update_limits_callback)
>          } else {
>              None
> -        };
> -        drv_ref.bios_limit = if T::HAS_BIOS_LIMIT {
> +        },
> +        bios_limit: if T::HAS_BIOS_LIMIT {
>              Some(Self::bios_limit_callback)
>          } else {
>              None
> -        };
> -        drv_ref.online = if T::HAS_ONLINE {
> +        },
> +        online: if T::HAS_ONLINE {
>              Some(Self::online_callback)
>          } else {
>              None
> -        };
> -        drv_ref.offline = if T::HAS_OFFLINE {
> +        },
> +        offline: if T::HAS_OFFLINE {
>              Some(Self::offline_callback)
>          } else {
>              None
> -        };
> -        drv_ref.exit = if T::HAS_EXIT {
> +        },
> +        exit: if T::HAS_EXIT {
>              Some(Self::exit_callback)
>          } else {
>              None
> -        };
> -        drv_ref.suspend = if T::HAS_SUSPEND {
> +        },
> +        suspend: if T::HAS_SUSPEND {
>              Some(Self::suspend_callback)
>          } else {
>              None
> -        };
> -        drv_ref.resume = if T::HAS_RESUME {
> +        },
> +        resume: if T::HAS_RESUME {
>              Some(Self::resume_callback)
>          } else {
>              None
> -        };
> -        drv_ref.ready = if T::HAS_READY {
> +        },
> +        ready: if T::HAS_READY {
>              Some(Self::ready_callback)
>          } else {
>              None
> -        };
> -        drv_ref.set_boost = if T::HAS_SET_BOOST {
> +        },
> +        set_boost: if T::HAS_SET_BOOST {
>              Some(Self::set_boost_callback)
>          } else {
>              None
> -        };
> -        drv_ref.register_em = if T::HAS_REGISTER_EM {
> +        },
> +        register_em: if T::HAS_REGISTER_EM {
>              Some(Self::register_em_callback)
>          } else {
>              None
> -        };
> +        },
> +        // SAFETY: All zeros is a valid value for `bindings::cpufreq_driver`.
> +        ..unsafe { MaybeUninit::zeroed().assume_init() }
> +    };
> +
> +    const fn copy_name(name: &'static CStr) -> [c_char; CPUFREQ_NAME_LEN] {
> +        let src = name.as_bytes_with_nul();
> +        let mut dst = [0; CPUFREQ_NAME_LEN];
> +
> +        build_assert!(src.len() <= CPUFREQ_NAME_LEN);
> +
> +        let mut i = 0;
> +        while i < src.len() {
> +            dst[i] = src[i];
> +            i += 1;
> +        }
> +
> +        dst
> +    }
> +
> +    /// Registers a CPU frequency driver with the cpufreq core.
> +    pub fn new() -> Result<Self> {
> +        let drv = &Self::VTABLE as *const _ as *mut _;

Hm, I didn't think of that, maybe it's better to store a *const directly instead
of a NonNull, given that VTABLE can't be altered.

Tamir is trying to clean up 'as' casts in [1]. So, I'd write this as

	let drv: *const bindings::cpufreq_driver = &Self::VTABLE;

[1] https://lore.kernel.org/rust-for-linux/20250409-ptr-as-ptr-v8-0-3738061534ef@gmail.com/

>  
>          // 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_ref) })?;
> +        to_result(unsafe { bindings::cpufreq_register_driver(drv) })?;
>  
> -        Ok(Self {
> -            drv,
> -            _p: PhantomData,
> -        })
> +        Ok(Self(
> +            NonNull::new(drv.cast()).ok_or(AllocError)?,
> +            PhantomData,
> +        ))
>      }
>  
>      /// Same as [`Registration::new`], but does not return a [`Registration`] instance.
> @@ -1259,9 +1262,7 @@ 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.
>      fn drop(&mut self) {
> -        let drv = self.drv.get_mut();
> -
>          // SAFETY: The driver was earlier registered from `new`.
> -        unsafe { bindings::cpufreq_unregister_driver(drv) };
> +        unsafe { bindings::cpufreq_unregister_driver(self.0.as_ptr()) };
>      }
>  }