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

[Danilo Krummrich]

On Fri, Apr 11, 2025 at 04:25:14PM +0530, Viresh Kumar wrote:
> +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 CPU frequency driver with the cpufreq core.
> +    pub fn new(name: &'static CStr, data: T::Data, flags: u16, boost: bool) -> Result<Self> {

Do you really need the private data? It seems to be used by only very few
drivers in C either.

If yes, I think you have to split this up into a CpufreqDriver structure and a
separate Registration. Otherwise you won't ever get access to the private data
again, after Registration::new_foreign_owned().

Note that we typically want Registration::new_foreign_owned(), because it
implies the guaranteed "fence" for where we can safely consider a device to be
bound. If we'd give out a Registration instance instead, the driver can
arbitrarily extend its lifetime across the remove() boundary.

If no, it seems to me that you can even avoid allocating a struct cpufreq_driver
dynamically and make it const instead. The fields name, boost_enabled and flags
could be required consts in your cpufreq::Driver trait.