| 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: <20250520055906.fxdbow2646gpyax2@vireshk-i7>
Date: Tue, 20 May 2025 11:29: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>,
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>,
Andrew Ballance <andrewjballance@...il.com>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH V12 13/15] rust: cpufreq: Extend abstractions for driver
registration
On 19-05-25, 13:06, Danilo Krummrich wrote:
> Sorry, I didn't catch the above in my previous review -- fine for me if you do
> those improvements in a subsequent patch.
That's fine. Thanks a lot for reviewing the series.
--
viresh
diff --git a/drivers/cpufreq/rcpufreq_dt.rs b/drivers/cpufreq/rcpufreq_dt.rs
index d0e60b7db81f..94ed81644fe1 100644
--- a/drivers/cpufreq/rcpufreq_dt.rs
+++ b/drivers/cpufreq/rcpufreq_dt.rs
@@ -152,30 +152,30 @@ fn init(policy: &mut cpufreq::Policy) -> Result<Self::PData> {
)?)
}
- fn exit(_policy: &mut cpufreq::Policy, _data: Option<Self::PData>) -> Result<()> {
+ fn exit(_policy: &mut cpufreq::Policy, _data: Option<Self::PData>) -> Result {
Ok(())
}
- fn online(_policy: &mut cpufreq::Policy) -> Result<()> {
+ fn online(_policy: &mut cpufreq::Policy) -> Result {
// We did light-weight tear down earlier, nothing to do here.
Ok(())
}
- fn offline(_policy: &mut cpufreq::Policy) -> Result<()> {
+ fn offline(_policy: &mut cpufreq::Policy) -> Result {
// Preserve policy->data and don't free resources on light-weight
// tear down.
Ok(())
}
- fn suspend(policy: &mut cpufreq::Policy) -> Result<()> {
+ fn suspend(policy: &mut cpufreq::Policy) -> Result {
policy.generic_suspend()
}
- fn verify(data: &mut cpufreq::PolicyData) -> Result<()> {
+ fn verify(data: &mut cpufreq::PolicyData) -> Result {
data.generic_verify()
}
- fn target_index(policy: &mut cpufreq::Policy, index: cpufreq::TableIndex) -> Result<()> {
+ fn target_index(policy: &mut cpufreq::Policy, index: cpufreq::TableIndex) -> Result {
let Some(data) = policy.data::<Self::PData>() else {
return Err(ENOENT);
};
@@ -188,7 +188,7 @@ fn get(policy: &mut cpufreq::Policy) -> Result<u32> {
policy.generic_get()
}
- fn set_boost(_policy: &mut cpufreq::Policy, _state: i32) -> Result<()> {
+ fn set_boost(_policy: &mut cpufreq::Policy, _state: i32) -> Result {
Ok(())
}
@@ -213,10 +213,7 @@ fn probe(
_id_info: Option<&Self::IdInfo>,
) -> Result<Pin<KBox<Self>>> {
cpufreq::Registration::<CPUFreqDTDriver>::new_foreign_owned(pdev.as_ref())?;
-
- let drvdata = KBox::new(Self {}, GFP_KERNEL)?;
-
- Ok(drvdata.into())
+ Ok(KBox::new(Self {}, GFP_KERNEL)?.into())
}
}
diff --git a/rust/kernel/cpufreq.rs b/rust/kernel/cpufreq.rs
index 826710c4f4b0..09b856bb297b 100644
--- a/rust/kernel/cpufreq.rs
+++ b/rust/kernel/cpufreq.rs
@@ -154,7 +154,7 @@ pub fn as_raw(&self) -> *mut bindings::cpufreq_policy_data {
/// Wrapper for `cpufreq_generic_frequency_table_verify`.
#[inline]
- pub fn generic_verify(&self) -> Result<()> {
+ pub fn generic_verify(&self) -> Result {
// SAFETY: By the type invariant, the pointer stored in `self` is valid.
to_result(unsafe { bindings::cpufreq_generic_frequency_table_verify(self.as_raw()) })
}
@@ -208,15 +208,16 @@ fn from(index: TableIndex) -> Self {
/// ```
/// use kernel::cpufreq::{Policy, TableIndex};
///
-/// fn show_freq(policy: &Policy) {
-/// let table = policy.freq_table().unwrap();
+/// fn show_freq(policy: &Policy) -> Result {
+/// let table = policy.freq_table()?;
///
/// // SAFETY: Index is a valid entry in the table.
/// let index = unsafe { TableIndex::new(0) };
///
-/// pr_info!("The frequency at index 0 is: {:?}\n", table.freq(index).unwrap());
+/// pr_info!("The frequency at index 0 is: {:?}\n", table.freq(index)?);
/// pr_info!("The flags at index 0 is: {}\n", table.flags(index));
/// pr_info!("The data at index 0 is: {}\n", table.data(index));
+/// Ok(())
/// }
/// ```
#[repr(transparent)]
@@ -361,7 +362,7 @@ pub fn new() -> Self {
}
/// Adds a new entry to the table.
- pub fn add(&mut self, freq: Hertz, flags: u32, driver_data: u32) -> Result<()> {
+ pub fn add(&mut self, freq: Hertz, flags: u32, driver_data: u32) -> Result {
// Adds the new entry at the end of the vector.
Ok(self.entries.push(
bindings::cpufreq_frequency_table {
@@ -515,7 +516,7 @@ pub fn set_suspend_freq(&mut self, freq: Hertz) -> &mut Self {
/// Provides a wrapper to the generic suspend routine.
#[inline]
- pub fn generic_suspend(&mut self) -> Result<()> {
+ pub fn generic_suspend(&mut self) -> Result {
// SAFETY: By the type invariant, the pointer stored in `self` is valid.
to_result(unsafe { bindings::cpufreq_generic_suspend(self.as_mut_ref()) })
}
@@ -643,7 +644,7 @@ pub fn data<T: ForeignOwnable>(&mut self) -> Option<<T>::Borrowed<'_>> {
/// # Errors
///
/// Returns `EBUSY` if private data is already set.
- fn set_data<T: ForeignOwnable>(&mut self, data: T) -> Result<()> {
+ fn set_data<T: ForeignOwnable>(&mut self, data: T) -> Result {
if self.as_ref().driver_data.is_null() {
// Transfer the ownership of the data to the foreign interface.
self.as_mut_ref().driver_data = <T as ForeignOwnable>::into_foreign(data) as _;
@@ -736,27 +737,27 @@ pub trait Driver {
fn init(policy: &mut Policy) -> Result<Self::PData>;
/// Driver's `exit` callback.
- fn exit(_policy: &mut Policy, _data: Option<Self::PData>) -> Result<()> {
+ fn exit(_policy: &mut Policy, _data: Option<Self::PData>) -> Result {
build_error!(VTABLE_DEFAULT_ERROR)
}
/// Driver's `online` callback.
- fn online(_policy: &mut Policy) -> Result<()> {
+ fn online(_policy: &mut Policy) -> Result {
build_error!(VTABLE_DEFAULT_ERROR)
}
/// Driver's `offline` callback.
- fn offline(_policy: &mut Policy) -> Result<()> {
+ fn offline(_policy: &mut Policy) -> Result {
build_error!(VTABLE_DEFAULT_ERROR)
}
/// Driver's `suspend` callback.
- fn suspend(_policy: &mut Policy) -> Result<()> {
+ fn suspend(_policy: &mut Policy) -> Result {
build_error!(VTABLE_DEFAULT_ERROR)
}
/// Driver's `resume` callback.
- fn resume(_policy: &mut Policy) -> Result<()> {
+ fn resume(_policy: &mut Policy) -> Result {
build_error!(VTABLE_DEFAULT_ERROR)
}
@@ -766,20 +767,20 @@ fn ready(_policy: &mut Policy) {
}
/// Driver's `verify` callback.
- fn verify(data: &mut PolicyData) -> Result<()>;
+ fn verify(data: &mut PolicyData) -> Result;
/// Driver's `setpolicy` callback.
- fn setpolicy(_policy: &mut Policy) -> Result<()> {
+ fn setpolicy(_policy: &mut Policy) -> Result {
build_error!(VTABLE_DEFAULT_ERROR)
}
/// Driver's `target` callback.
- fn target(_policy: &mut Policy, _target_freq: u32, _relation: Relation) -> Result<()> {
+ fn target(_policy: &mut Policy, _target_freq: u32, _relation: Relation) -> Result {
build_error!(VTABLE_DEFAULT_ERROR)
}
/// Driver's `target_index` callback.
- fn target_index(_policy: &mut Policy, _index: TableIndex) -> Result<()> {
+ fn target_index(_policy: &mut Policy, _index: TableIndex) -> Result {
build_error!(VTABLE_DEFAULT_ERROR)
}
@@ -799,7 +800,7 @@ fn get_intermediate(_policy: &mut Policy, _index: TableIndex) -> u32 {
}
/// Driver's `target_intermediate` callback.
- fn target_intermediate(_policy: &mut Policy, _index: TableIndex) -> Result<()> {
+ fn target_intermediate(_policy: &mut Policy, _index: TableIndex) -> Result {
build_error!(VTABLE_DEFAULT_ERROR)
}
@@ -814,12 +815,12 @@ fn update_limits(_policy: &mut Policy) {
}
/// Driver's `bios_limit` callback.
- fn bios_limit(_policy: &mut Policy, _limit: &mut u32) -> Result<()> {
+ fn bios_limit(_policy: &mut Policy, _limit: &mut u32) -> Result {
build_error!(VTABLE_DEFAULT_ERROR)
}
/// Driver's `set_boost` callback.
- fn set_boost(_policy: &mut Policy, _state: i32) -> Result<()> {
+ fn set_boost(_policy: &mut Policy, _state: i32) -> Result {
build_error!(VTABLE_DEFAULT_ERROR)
}
@@ -837,43 +838,44 @@ fn register_em(_policy: &mut Policy) {
///
/// ```
/// use kernel::{
-/// cpu, cpufreq,
+/// cpufreq,
/// c_str,
-/// device::{Bound, Device},
+/// device::{Core, Device},
/// macros::vtable,
+/// of, platform,
/// sync::Arc,
/// };
-/// struct FooDevice;
+/// struct SampleDevice;
///
/// #[derive(Default)]
-/// struct FooDriver;
+/// struct SampleDriver;
///
/// #[vtable]
-/// impl cpufreq::Driver for FooDriver {
-/// const NAME: &'static CStr = c_str!("cpufreq-foo");
+/// impl cpufreq::Driver for SampleDriver {
+/// const NAME: &'static CStr = c_str!("cpufreq-sample");
/// const FLAGS: u16 = cpufreq::flags::NEED_INITIAL_FREQ_CHECK | cpufreq::flags::IS_COOLING_DEV;
/// const BOOST_ENABLED: bool = true;
///
-/// type PData = Arc<FooDevice>;
+/// type PData = Arc<SampleDevice>;
///
/// fn init(policy: &mut cpufreq::Policy) -> Result<Self::PData> {
/// // Initialize here
-/// Ok(Arc::new(FooDevice, GFP_KERNEL)?)
+/// Ok(Arc::new(SampleDevice, GFP_KERNEL)?)
/// }
///
-/// fn exit(_policy: &mut cpufreq::Policy, _data: Option<Self::PData>) -> Result<()> {
+/// fn exit(_policy: &mut cpufreq::Policy, _data: Option<Self::PData>) -> Result {
/// Ok(())
/// }
///
-/// fn suspend(policy: &mut cpufreq::Policy) -> Result<()> {
+/// fn suspend(policy: &mut cpufreq::Policy) -> Result {
/// policy.generic_suspend()
/// }
///
-/// fn verify(data: &mut cpufreq::PolicyData) -> Result<()> {
+/// fn verify(data: &mut cpufreq::PolicyData) -> Result {
/// data.generic_verify()
/// }
///
-/// fn target_index(policy: &mut cpufreq::Policy, index: cpufreq::TableIndex) -> Result<()> {
+/// fn target_index(policy: &mut cpufreq::Policy, index: cpufreq::TableIndex) -> Result {
/// // Update CPU frequency
/// Ok(())
/// }
@@ -883,8 +885,17 @@ fn register_em(_policy: &mut Policy) {
/// }
/// }
///
-/// fn foo_probe(dev: &Device<Bound>) {
-/// cpufreq::Registration::<FooDriver>::new_foreign_owned(dev).unwrap();
+/// impl platform::Driver for SampleDriver {
+/// type IdInfo = ();
+/// const OF_ID_TABLE: Option<of::IdTable<Self::IdInfo>> = None;
+///
+/// fn probe(
+/// pdev: &platform::Device<Core>,
+/// _id_info: Option<&Self::IdInfo>,
+/// ) -> Result<Pin<KBox<Self>>> {
+/// cpufreq::Registration::<SampleDriver>::new_foreign_owned(pdev.as_ref())?;
+/// Ok(KBox::new(Self {}, GFP_KERNEL)?.into())
+/// }
/// }
/// ```
#[repr(transparent)]
@@ -1035,7 +1046,7 @@ pub fn new() -> Result<Self> {
///
/// Instead the [`Registration`] is owned by [`Devres`] and will be revoked / dropped, once the
/// device is detached.
- pub fn new_foreign_owned(dev: &Device<Bound>) -> Result<()> {
+ pub fn new_foreign_owned(dev: &Device<Bound>) -> Result {
Devres::new_foreign_owned(dev, Self::new()?, GFP_KERNEL)
}
}
diff --git a/rust/kernel/opp.rs b/rust/kernel/opp.rs
index 1e5fd9887b3a..212555dacd45 100644
--- a/rust/kernel/opp.rs
+++ b/rust/kernel/opp.rs
@@ -292,7 +292,7 @@ pub enum SearchType {
pub trait ConfigOps {
/// This is typically used to scale clocks when transitioning between OPPs.
#[inline]
- fn config_clks(_dev: &Device, _table: &Table, _opp: &OPP, _scaling_down: bool) -> Result<()> {
+ fn config_clks(_dev: &Device, _table: &Table, _opp: &OPP, _scaling_down: bool) -> Result {
build_error!(VTABLE_DEFAULT_ERROR)
}
@@ -304,7 +304,7 @@ fn config_regulators(
_opp_new: &OPP,
_data: *mut *mut bindings::regulator,
_count: u32,
- ) -> Result<()> {
+ ) -> Result {
build_error!(VTABLE_DEFAULT_ERROR)
}
}
@@ -753,7 +753,7 @@ pub fn suspend_freq(&self) -> Hertz {
/// Synchronizes regulators used by the [`Table`].
#[inline]
- pub fn sync_regulators(&self) -> Result<()> {
+ pub fn sync_regulators(&self) -> Result {
// SAFETY: The requirements are satisfied by the existence of [`Device`] and its safety
// requirements.
to_result(unsafe { bindings::dev_pm_opp_sync_regulators(self.dev.as_raw()) })
@@ -761,14 +761,14 @@ pub fn sync_regulators(&self) -> Result<()> {
/// Gets sharing CPUs.
#[inline]
- pub fn sharing_cpus(dev: &Device, cpumask: &mut Cpumask) -> Result<()> {
+ pub fn sharing_cpus(dev: &Device, cpumask: &mut Cpumask) -> Result {
// SAFETY: The requirements are satisfied by the existence of [`Device`] and its safety
// requirements.
to_result(unsafe { bindings::dev_pm_opp_get_sharing_cpus(dev.as_raw(), cpumask.as_raw()) })
}
/// Sets sharing CPUs.
- pub fn set_sharing_cpus(&mut self, cpumask: &mut Cpumask) -> Result<()> {
+ pub fn set_sharing_cpus(&mut self, cpumask: &mut Cpumask) -> Result {
// SAFETY: The requirements are satisfied by the existence of [`Device`] and its safety
// requirements.
to_result(unsafe {
@@ -786,7 +786,7 @@ pub fn set_sharing_cpus(&mut self, cpumask: &mut Cpumask) -> Result<()> {
/// Gets sharing CPUs from device tree.
#[cfg(CONFIG_OF)]
#[inline]
- pub fn of_sharing_cpus(dev: &Device, cpumask: &mut Cpumask) -> Result<()> {
+ pub fn of_sharing_cpus(dev: &Device, cpumask: &mut Cpumask) -> Result {
// SAFETY: The requirements are satisfied by the existence of [`Device`] and its safety
// requirements.
to_result(unsafe {
@@ -802,7 +802,7 @@ pub fn adjust_voltage(
volt: MicroVolt,
volt_min: MicroVolt,
volt_max: MicroVolt,
- ) -> Result<()> {
+ ) -> Result {
// SAFETY: The requirements are satisfied by the existence of [`Device`] and its safety
// requirements.
to_result(unsafe {
@@ -825,7 +825,7 @@ pub fn cpufreq_table(&mut self) -> Result<FreqTable> {
/// Configures device with [`OPP`] matching the frequency value.
#[inline]
- pub fn set_rate(&self, freq: Hertz) -> Result<()> {
+ pub fn set_rate(&self, freq: Hertz) -> Result {
// SAFETY: The requirements are satisfied by the existence of [`Device`] and its safety
// requirements.
to_result(unsafe { bindings::dev_pm_opp_set_rate(self.dev.as_raw(), freq.into()) })
@@ -833,7 +833,7 @@ pub fn set_rate(&self, freq: Hertz) -> Result<()> {
/// Configures device with [`OPP`].
#[inline]
- pub fn set_opp(&self, opp: &OPP) -> Result<()> {
+ pub fn set_opp(&self, opp: &OPP) -> Result {
// SAFETY: The requirements are satisfied by the existence of [`Device`] and its safety
// requirements.
to_result(unsafe { bindings::dev_pm_opp_set_opp(self.dev.as_raw(), opp.as_raw()) })
@@ -937,7 +937,7 @@ pub fn opp_from_bw(&self, mut bw: u32, index: i32, stype: SearchType) -> Result<
/// Enables the [`OPP`].
#[inline]
- pub fn enable_opp(&self, freq: Hertz) -> Result<()> {
+ pub fn enable_opp(&self, freq: Hertz) -> Result {
// SAFETY: The requirements are satisfied by the existence of [`Device`] and its safety
// requirements.
to_result(unsafe { bindings::dev_pm_opp_enable(self.dev.as_raw(), freq.into()) })
@@ -945,7 +945,7 @@ pub fn enable_opp(&self, freq: Hertz) -> Result<()> {
/// Disables the [`OPP`].
#[inline]
- pub fn disable_opp(&self, freq: Hertz) -> Result<()> {
+ pub fn disable_opp(&self, freq: Hertz) -> Result {
// SAFETY: The requirements are satisfied by the existence of [`Device`] and its safety
// requirements.
to_result(unsafe { bindings::dev_pm_opp_disable(self.dev.as_raw(), freq.into()) })
@@ -953,7 +953,7 @@ pub fn disable_opp(&self, freq: Hertz) -> Result<()> {
/// Registers with the Energy model.
#[cfg(CONFIG_OF)]
- pub fn of_register_em(&mut self, cpumask: &mut Cpumask) -> Result<()> {
+ pub fn of_register_em(&mut self, cpumask: &mut Cpumask) -> Result {
// SAFETY: The requirements are satisfied by the existence of [`Device`] and its safety
// requirements.
to_result(unsafe {
diff --git a/rust/macros/module.rs b/rust/macros/module.rs
index 27cc72d474f0..6ff34096d7ee 100644
--- a/rust/macros/module.rs
+++ b/rust/macros/module.rs
@@ -185,9 +185,9 @@ pub(crate) fn module(ts: TokenStream) -> TokenStream {
let info = ModuleInfo::parse(&mut it);
- /* Rust does not allow hyphens in identifiers, use underscore instead */
- let name_identifier = info.name.replace('-', "_");
- let mut modinfo = ModInfoBuilder::new(name_identifier.as_ref());
+ // Rust does not allow hyphens in identifiers, use underscore instead.
+ let ident = info.name.replace('-', "_");
+ let mut modinfo = ModInfoBuilder::new(ident.as_ref());
if let Some(author) = info.author {
modinfo.emit("author", &author);
}
@@ -312,15 +312,15 @@ mod __module_init {{
#[doc(hidden)]
#[link_section = \"{initcall_section}\"]
#[used]
- pub static __{name_identifier}_initcall: extern \"C\" fn() ->
- kernel::ffi::c_int = __{name_identifier}_init;
+ pub static __{ident}_initcall: extern \"C\" fn() ->
+ kernel::ffi::c_int = __{ident}_init;
#[cfg(not(MODULE))]
#[cfg(CONFIG_HAVE_ARCH_PREL32_RELOCATIONS)]
core::arch::global_asm!(
r#\".section \"{initcall_section}\", \"a\"
- __{name_identifier}_initcall:
- .long __{name_identifier}_init - .
+ __{ident}_initcall:
+ .long __{ident}_init - .
.previous
\"#
);
@@ -328,7 +328,7 @@ mod __module_init {{
#[cfg(not(MODULE))]
#[doc(hidden)]
#[no_mangle]
- pub extern \"C\" fn __{name_identifier}_init() -> kernel::ffi::c_int {{
+ pub extern \"C\" fn __{ident}_init() -> kernel::ffi::c_int {{
// SAFETY: This function is inaccessible to the outside due to the double
// module wrapping it. It is called exactly once by the C side via its
// placement above in the initcall section.
@@ -338,12 +338,12 @@ mod __module_init {{
#[cfg(not(MODULE))]
#[doc(hidden)]
#[no_mangle]
- pub extern \"C\" fn __{name_identifier}_exit() {{
+ pub extern \"C\" fn __{ident}_exit() {{
// SAFETY:
// - This function is inaccessible to the outside due to the double
// module wrapping it. It is called exactly once by the C side via its
// unique name,
- // - furthermore it is only called after `__{name_identifier}_init` has
+ // - furthermore it is only called after `__{ident}_init` has
// returned `0` (which delegates to `__init`).
unsafe {{ __exit() }}
}}
@@ -384,7 +384,7 @@ unsafe fn __exit() {{
",
type_ = info.type_,
name = info.name,
- name_identifier = name_identifier,
+ ident = ident,
modinfo = modinfo.buffer,
initcall_section = ".initcall6.init"
)
Powered by blists - more mailing lists