| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20250414084706.rjsdaoxmug4p4e7l@vireshk-i7>
Date: Mon, 14 Apr 2025 14:17:06 +0530
From: Viresh Kumar <viresh.kumar@...aro.org>
To: Danilo Krummrich <dakr@...nel.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>,
Erik Schilling <erik.schilling@...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 V9 15/17] rust: cpufreq: Extend abstractions for driver
registration
On 11-04-25, 13:58, Danilo Krummrich wrote:
> On Fri, Apr 11, 2025 at 04:25:14PM +0530, Viresh Kumar wrote:
> > +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.
Yes, only a few of them are setting it to a value other than `pdev`.
Maybe we can avoid it for the time being and come back to this when a
driver really wants it.
> If no, it seems to me that you can even avoid allocating a struct cpufreq_driver
> dynamically and make it const instead.
I am not sure if I understood your suggestion. The Registration::new()
method still updates the instance of cpufreq_driver before passing it
to the C cpufreq core.
I have tried to fix the other issues though.
--
viresh
diff --git a/drivers/cpufreq/rcpufreq_dt.rs b/drivers/cpufreq/rcpufreq_dt.rs
index 751be33c0218..831269bdeabf 100644
--- a/drivers/cpufreq/rcpufreq_dt.rs
+++ b/drivers/cpufreq/rcpufreq_dt.rs
@@ -59,7 +59,10 @@ impl opp::ConfigOps for CPUFreqDTDriver {}
#[vtable]
impl cpufreq::Driver for CPUFreqDTDriver {
- type Data = ();
+ const NAME: &CStr = c_str!("cpufreq-dt");
+ const FLAGS: u16 = cpufreq::flags::NEED_INITIAL_FREQ_CHECK | cpufreq::flags::IS_COOLING_DEV;
+ const BOOST_ENABLED: bool = true;
+
type PData = Arc<CPUFreqDTDevice>;
fn init(policy: &mut cpufreq::Policy) -> Result<Self::PData> {
@@ -213,13 +216,7 @@ fn probe(
pdev: &platform::Device<Core>,
_id_info: Option<&Self::IdInfo>,
) -> Result<Pin<KBox<Self>>> {
- cpufreq::Registration::<CPUFreqDTDriver>::new_foreign_owned(
- pdev.as_ref(),
- c_str!("cpufreq-dt"),
- (),
- cpufreq::flags::NEED_INITIAL_FREQ_CHECK | cpufreq::flags::IS_COOLING_DEV,
- true,
- )?;
+ cpufreq::Registration::<CPUFreqDTDriver>::new_foreign_owned(pdev.as_ref())?;
let drvdata = KBox::new(Self {}, GFP_KERNEL)?;
diff --git a/rust/kernel/cpufreq.rs b/rust/kernel/cpufreq.rs
index 4194b9558413..9b275d4d3eb6 100644
--- a/rust/kernel/cpufreq.rs
+++ b/rust/kernel/cpufreq.rs
@@ -685,13 +685,14 @@ fn drop(&mut self) {
/// Reference: <https://docs.kernel.org/cpu-freq/cpu-drivers.html>
#[vtable]
pub trait Driver {
- /// Driver specific data.
- ///
- /// Corresponds to the data retrieved via the kernel's `cpufreq_get_driver_data` function.
- ///
- /// Require `Data` to implement `ForeignOwnable`. We guarantee to never move the underlying
- /// wrapped data structure.
- type Data: ForeignOwnable;
+ /// Driver's name.
+ const NAME: &'static CStr;
+
+ /// Driver's flags.
+ const FLAGS: u16;
+
+ /// Boost support.
+ const BOOST_ENABLED: bool;
/// Policy specific data.
///
@@ -804,8 +805,8 @@ fn register_em(_policy: &mut Policy) {
///
/// ```
/// use kernel::{
-/// c_str,
/// cpu, cpufreq,
+/// c_str,
/// device::Device,
/// macros::vtable,
/// sync::Arc,
@@ -817,7 +818,10 @@ fn register_em(_policy: &mut Policy) {
///
/// #[vtable]
/// impl cpufreq::Driver for FooDriver {
-/// type Data = ();
+/// const NAME: &'static CStr = c_str!("cpufreq-foo");
+/// const FLAGS: u16 = cpufreq::flags::NEED_INITIAL_FREQ_CHECK | cpufreq::flags::IS_COOLING_DEV;
+/// const BOOST_ENABLED: bool = true;
+///
/// type PData = Arc<FooDevice>;
///
/// fn init(policy: &mut cpufreq::Policy) -> Result<Self::PData> {
@@ -848,13 +852,7 @@ fn register_em(_policy: &mut Policy) {
/// }
///
/// fn foo_probe(dev: &Device) {
-/// cpufreq::Registration::<FooDriver>::new_foreign_owned(
-/// dev,
-/// c_str!("cpufreq-foo"),
-/// (),
-/// cpufreq::flags::NEED_INITIAL_FREQ_CHECK | cpufreq::flags::IS_COOLING_DEV,
-/// true,
-/// ).unwrap();
+/// cpufreq::Registration::<FooDriver>::new_foreign_owned(dev).unwrap();
/// }
/// ```
pub struct Registration<T: Driver> {
@@ -868,13 +866,12 @@ 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.
+// thread.
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> {
+ 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)]
@@ -886,18 +883,18 @@ pub fn new(name: &'static CStr, data: T::Data, flags: u16, boost: bool) -> Resul
let drv_ref = drv.get_mut();
// Account for the trailing null byte.
- let len = name.len() + 1;
+ let len = T::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]) };
+ // 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 = boost;
- drv_ref.flags = flags;
+ drv_ref.boost_enabled = T::BOOST_ENABLED;
+ drv_ref.flags = T::FLAGS;
// Initialize mandatory callbacks.
drv_ref.init = Some(Self::init_callback);
@@ -995,10 +992,6 @@ pub fn new(name: &'static CStr, data: T::Data, flags: u16, boost: bool) -> Resul
None
};
- // Set driver data before registering the driver, as the cpufreq core calls 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 kernel C code.
to_result(unsafe { bindings::cpufreq_register_driver(drv_ref) })?;
@@ -1012,53 +1005,10 @@ pub fn new(name: &'static CStr, data: T::Data, flags: u16, boost: bool) -> Resul
///
/// Instead the [`Registration`] is owned by [`Devres`] and will be revoked / dropped, once the
/// device is detached.
- pub fn new_foreign_owned(
- dev: &Device,
- name: &'static CStr,
- data: T::Data,
- flags: u16,
- boost: bool,
- ) -> Result<()> {
- Devres::new_foreign_owned(dev, Self::new(name, data, flags, boost)?, GFP_KERNEL)?;
+ pub fn new_foreign_owned(dev: &Device) -> Result<()> {
+ Devres::new_foreign_owned(dev, Self::new()?, GFP_KERNEL)?;
Ok(())
}
-
- // Sets the `Data` for the CPU frequency driver.
- fn set_data(drv: &mut bindings::cpufreq_driver, data: T::Data) -> Result<()> {
- if drv.driver_data.is_null() {
- // Transfer the ownership of the data to the C code.
- drv.driver_data = <T::Data as ForeignOwnable>::into_foreign(data) as _;
- Ok(())
- } else {
- Err(EBUSY)
- }
- }
-
- /// Returns borrowed `Data` previously set for the CPU frequency driver.
- pub fn data(&mut self) -> Option<<T::Data as ForeignOwnable>::Borrowed<'static>> {
- let drv = self.drv.get_mut();
-
- if drv.driver_data.is_null() {
- None
- } else {
- // SAFETY: The data is earlier set by us from `set_data`.
- Some(unsafe { <T::Data as ForeignOwnable>::borrow(drv.driver_data) })
- }
- }
-
- // Clears and returns the `Data` for the CPU frequency driver.
- fn clear_data(&mut self) -> Option<T::Data> {
- let drv = self.drv.get_mut();
-
- if drv.driver_data.is_null() {
- None
- } else {
- // SAFETY: The data is earlier set by us from `set_data`.
- let data = Some(unsafe { <T::Data as ForeignOwnable>::from_foreign(drv.driver_data) });
- drv.driver_data = ptr::null_mut();
- data
- }
- }
}
// CPU frequency driver callbacks.
@@ -1313,8 +1263,5 @@ fn drop(&mut self) {
// SAFETY: The driver was earlier registered from `new`.
unsafe { bindings::cpufreq_unregister_driver(drv) };
-
- // Free data
- drop(self.clear_data());
}
}
Powered by blists - more mailing lists