| Message ID | a940d1b1a02d99fdc80ba8d0526c35a776854cb3.1744783509.git.viresh.kumar@linaro.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Rust abstractions for clk, cpumask, cpufreq, OPP | expand |
On Wed, Apr 16, 2025 at 12:09:31PM +0530, Viresh Kumar wrote: > Extend the OPP abstractions to include support for interacting with the > cpufreq core, including the ability to retrieve frequency tables from > OPP table. > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > rust/kernel/opp.rs | 67 ++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 67 insertions(+) > > diff --git a/rust/kernel/opp.rs b/rust/kernel/opp.rs > index 44e11808793a..734be8b6d0ef 100644 > --- a/rust/kernel/opp.rs > +++ b/rust/kernel/opp.rs > @@ -19,6 +19,12 @@ > types::{ARef, AlwaysRefCounted, Opaque}, > }; > > +#[cfg(CONFIG_CPU_FREQ)] This config is needed quite often, it probably makes sense to move this code in its own Rust module, i.e.: #[cfg(CONFIG_CPU_FREQ)] pub mod freq;
On 16-04-25, 10:52, Danilo Krummrich wrote: > This config is needed quite often, it probably makes sense to move this code in > its own Rust module, i.e.: > > #[cfg(CONFIG_CPU_FREQ)] > pub mod freq; Like this ? diff --git a/rust/kernel/opp.rs b/rust/kernel/opp.rs index 734be8b6d0ef..f4cabe859c43 100644 --- a/rust/kernel/opp.rs +++ b/rust/kernel/opp.rs @@ -20,10 +20,67 @@ }; #[cfg(CONFIG_CPU_FREQ)] -use crate::cpufreq; +// Frequency table implementation. +mod freq { + use crate::cpufreq; + use core::ops::Deref; + use super::*; + + /// OPP frequency table. + /// + /// A [`cpufreq::Table`] created from [`Table`]. + pub struct FreqTable { + dev: ARef<Device>, + ptr: *mut bindings::cpufreq_frequency_table, + } + + impl FreqTable { + /// Creates a new instance of [`FreqTable`] from [`Table`]. + pub(crate) fn new(table: &Table) -> Result<Self> { + let mut ptr: *mut bindings::cpufreq_frequency_table = ptr::null_mut(); + + // SAFETY: The requirements are satisfied by the existence of [`Device`] and its safety + // requirements. + to_result(unsafe { + bindings::dev_pm_opp_init_cpufreq_table(table.dev.as_raw(), &mut ptr) + })?; + + Ok(Self { + dev: table.dev.clone(), + ptr, + }) + } + + // Returns a reference to the underlying [`cpufreq::Table`]. + #[inline] + fn table(&self) -> &cpufreq::Table { + // SAFETY: The `ptr` is guaranteed by the C code to be valid. + unsafe { cpufreq::Table::from_raw(self.ptr) } + } + } + + impl Deref for FreqTable { + type Target = cpufreq::Table; + + #[inline] + fn deref(&self) -> &Self::Target { + self.table() + } + } + + impl Drop for FreqTable { + fn drop(&mut self) { + // SAFETY: The pointer was created via `dev_pm_opp_init_cpufreq_table`, and is only + // freed here. + unsafe { + bindings::dev_pm_opp_free_cpufreq_table(self.dev.as_raw(), &mut self.as_raw()) + }; + } + } +} #[cfg(CONFIG_CPU_FREQ)] -use core::ops::Deref; +pub use freq::FreqTable; use core::{marker::PhantomData, ptr}; @@ -502,60 +559,6 @@ extern "C" fn config_regulators( } } -/// OPP frequency table. -/// -/// A [`cpufreq::Table`] created from [`Table`]. -#[cfg(CONFIG_CPU_FREQ)] -pub struct FreqTable { - dev: ARef<Device>, - ptr: *mut bindings::cpufreq_frequency_table, -} - -#[cfg(CONFIG_CPU_FREQ)] -impl FreqTable { - /// Creates a new instance of [`FreqTable`] from [`Table`]. - fn new(table: &Table) -> Result<Self> { - let mut ptr: *mut bindings::cpufreq_frequency_table = ptr::null_mut(); - - // SAFETY: The requirements are satisfied by the existence of [`Device`] and its safety - // requirements. - to_result(unsafe { - bindings::dev_pm_opp_init_cpufreq_table(table.dev.as_raw(), &mut ptr) - })?; - - Ok(Self { - dev: table.dev.clone(), - ptr, - }) - } - - // Returns a reference to the underlying [`cpufreq::Table`]. - #[inline] - fn table(&self) -> &cpufreq::Table { - // SAFETY: The `ptr` is guaranteed by the C code to be valid. - unsafe { cpufreq::Table::from_raw(self.ptr) } - } -} - -#[cfg(CONFIG_CPU_FREQ)] -impl Deref for FreqTable { - type Target = cpufreq::Table; - - #[inline] - fn deref(&self) -> &Self::Target { - self.table() - } -} - -#[cfg(CONFIG_CPU_FREQ)] -impl Drop for FreqTable { - fn drop(&mut self) { - // SAFETY: The pointer was created via `dev_pm_opp_init_cpufreq_table`, and is only freed - // here. - unsafe { bindings::dev_pm_opp_free_cpufreq_table(self.dev.as_raw(), &mut self.as_raw()) }; - } -} - /// A reference-counted OPP table. /// /// Rust abstraction for the C `struct opp_table`.
On Wed, Apr 16, 2025 at 11:59 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > +// Frequency table implementation. /// > + // Returns a reference to the underlying [`cpufreq::Table`]. Ditto. I wonder if we should have a `checkpatch.pl` warnings (not error) for this -- it is not always a mistake, but there is a fair chance it is. The likelihood increases if we notice things like ``[`...`]`` there. Added: https://github.com/Rust-for-Linux/linux/issues/1157 Cheers, Miguel
On 16-04-25, 12:31, Miguel Ojeda wrote: > On Wed, Apr 16, 2025 at 11:59 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > > +// Frequency table implementation. > > /// > > > + // Returns a reference to the underlying [`cpufreq::Table`]. > > Ditto. Hmm, I did not use /// as the comments were added to private definitions. Sorry for the dumb question, but why should we use /// in such cases ? They will never show up in documentation anyway, right ?
On Wed, Apr 16, 2025 at 12:40 PM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > Hmm, I did not use /// as the comments were added to private > definitions. > > Sorry for the dumb question, but why should we use /// in such cases ? > They will never show up in documentation anyway, right ? It is not a dumb question at all! The reason is that using `///` is not just for `rustdoc`, but intended to convey it is documentation for the item, rather than a comment that talks about implementation details or things like TODOs. So you may have both `///` or `//` even for private items, and it is a meaningful difference for the reader. Plus it makes it consistent with the public ones. Moreover, if we ever move to documenting private items, then we will want these to be correct -- `rustdoc` supports generating docs with private items (e.g. it puts a cute lock emoji on private items in the lists etc.). I think some kernel developers would appreciate it -- we could offer both versions in rust.docs.kernel.org with a toggle, for instance. Cheers, Miguel
On Wed, Apr 16, 2025 at 03:29:43PM +0530, Viresh Kumar wrote: > On 16-04-25, 10:52, Danilo Krummrich wrote: > > This config is needed quite often, it probably makes sense to move this code in > > its own Rust module, i.e.: > > > > #[cfg(CONFIG_CPU_FREQ)] > > pub mod freq; > > Like this ? Yes, I thought of a separate file, but I that should work as well. > > diff --git a/rust/kernel/opp.rs b/rust/kernel/opp.rs > index 734be8b6d0ef..f4cabe859c43 100644 > --- a/rust/kernel/opp.rs > +++ b/rust/kernel/opp.rs > @@ -20,10 +20,67 @@ > }; > > #[cfg(CONFIG_CPU_FREQ)] > -use crate::cpufreq; > +// Frequency table implementation. > +mod freq { > + use crate::cpufreq; > + use core::ops::Deref; > + use super::*; > + > + /// OPP frequency table. > + /// > + /// A [`cpufreq::Table`] created from [`Table`]. > + pub struct FreqTable { > + dev: ARef<Device>, > + ptr: *mut bindings::cpufreq_frequency_table, > + } > + > + impl FreqTable { > + /// Creates a new instance of [`FreqTable`] from [`Table`]. > + pub(crate) fn new(table: &Table) -> Result<Self> { > + let mut ptr: *mut bindings::cpufreq_frequency_table = ptr::null_mut(); > + > + // SAFETY: The requirements are satisfied by the existence of [`Device`] and its safety > + // requirements. > + to_result(unsafe { > + bindings::dev_pm_opp_init_cpufreq_table(table.dev.as_raw(), &mut ptr) > + })?; > + > + Ok(Self { > + dev: table.dev.clone(), > + ptr, > + }) > + } > + > + // Returns a reference to the underlying [`cpufreq::Table`]. > + #[inline] > + fn table(&self) -> &cpufreq::Table { > + // SAFETY: The `ptr` is guaranteed by the C code to be valid. > + unsafe { cpufreq::Table::from_raw(self.ptr) } > + } > + } > + > + impl Deref for FreqTable { > + type Target = cpufreq::Table; > + > + #[inline] > + fn deref(&self) -> &Self::Target { > + self.table() > + } > + } > + > + impl Drop for FreqTable { > + fn drop(&mut self) { > + // SAFETY: The pointer was created via `dev_pm_opp_init_cpufreq_table`, and is only > + // freed here. > + unsafe { > + bindings::dev_pm_opp_free_cpufreq_table(self.dev.as_raw(), &mut self.as_raw()) > + }; > + } > + } > +} > > #[cfg(CONFIG_CPU_FREQ)] > -use core::ops::Deref; > +pub use freq::FreqTable; > > use core::{marker::PhantomData, ptr}; > > @@ -502,60 +559,6 @@ extern "C" fn config_regulators( > } > } > > -/// OPP frequency table. > -/// > -/// A [`cpufreq::Table`] created from [`Table`]. > -#[cfg(CONFIG_CPU_FREQ)] > -pub struct FreqTable { > - dev: ARef<Device>, > - ptr: *mut bindings::cpufreq_frequency_table, > -} > - > -#[cfg(CONFIG_CPU_FREQ)] > -impl FreqTable { > - /// Creates a new instance of [`FreqTable`] from [`Table`]. > - fn new(table: &Table) -> Result<Self> { > - let mut ptr: *mut bindings::cpufreq_frequency_table = ptr::null_mut(); > - > - // SAFETY: The requirements are satisfied by the existence of [`Device`] and its safety > - // requirements. > - to_result(unsafe { > - bindings::dev_pm_opp_init_cpufreq_table(table.dev.as_raw(), &mut ptr) > - })?; > - > - Ok(Self { > - dev: table.dev.clone(), > - ptr, > - }) > - } > - > - // Returns a reference to the underlying [`cpufreq::Table`]. > - #[inline] > - fn table(&self) -> &cpufreq::Table { > - // SAFETY: The `ptr` is guaranteed by the C code to be valid. > - unsafe { cpufreq::Table::from_raw(self.ptr) } > - } > -} > - > -#[cfg(CONFIG_CPU_FREQ)] > -impl Deref for FreqTable { > - type Target = cpufreq::Table; > - > - #[inline] > - fn deref(&self) -> &Self::Target { > - self.table() > - } > -} > - > -#[cfg(CONFIG_CPU_FREQ)] > -impl Drop for FreqTable { > - fn drop(&mut self) { > - // SAFETY: The pointer was created via `dev_pm_opp_init_cpufreq_table`, and is only freed > - // here. > - unsafe { bindings::dev_pm_opp_free_cpufreq_table(self.dev.as_raw(), &mut self.as_raw()) }; > - } > -} > - > /// A reference-counted OPP table. > /// > /// Rust abstraction for the C `struct opp_table`. > > -- > viresh
diff --git a/rust/kernel/opp.rs b/rust/kernel/opp.rs index 44e11808793a..734be8b6d0ef 100644 --- a/rust/kernel/opp.rs +++ b/rust/kernel/opp.rs @@ -19,6 +19,12 @@ types::{ARef, AlwaysRefCounted, Opaque}, }; +#[cfg(CONFIG_CPU_FREQ)] +use crate::cpufreq; + +#[cfg(CONFIG_CPU_FREQ)] +use core::ops::Deref; + use core::{marker::PhantomData, ptr}; use macros::vtable; @@ -496,6 +502,60 @@ extern "C" fn config_regulators( } } +/// OPP frequency table. +/// +/// A [`cpufreq::Table`] created from [`Table`]. +#[cfg(CONFIG_CPU_FREQ)] +pub struct FreqTable { + dev: ARef<Device>, + ptr: *mut bindings::cpufreq_frequency_table, +} + +#[cfg(CONFIG_CPU_FREQ)] +impl FreqTable { + /// Creates a new instance of [`FreqTable`] from [`Table`]. + fn new(table: &Table) -> Result<Self> { + let mut ptr: *mut bindings::cpufreq_frequency_table = ptr::null_mut(); + + // SAFETY: The requirements are satisfied by the existence of [`Device`] and its safety + // requirements. + to_result(unsafe { + bindings::dev_pm_opp_init_cpufreq_table(table.dev.as_raw(), &mut ptr) + })?; + + Ok(Self { + dev: table.dev.clone(), + ptr, + }) + } + + // Returns a reference to the underlying [`cpufreq::Table`]. + #[inline] + fn table(&self) -> &cpufreq::Table { + // SAFETY: The `ptr` is guaranteed by the C code to be valid. + unsafe { cpufreq::Table::from_raw(self.ptr) } + } +} + +#[cfg(CONFIG_CPU_FREQ)] +impl Deref for FreqTable { + type Target = cpufreq::Table; + + #[inline] + fn deref(&self) -> &Self::Target { + self.table() + } +} + +#[cfg(CONFIG_CPU_FREQ)] +impl Drop for FreqTable { + fn drop(&mut self) { + // SAFETY: The pointer was created via `dev_pm_opp_init_cpufreq_table`, and is only freed + // here. + unsafe { bindings::dev_pm_opp_free_cpufreq_table(self.dev.as_raw(), &mut self.as_raw()) }; + } +} + /// A reference-counted OPP table. /// /// Rust abstraction for the C `struct opp_table`. @@ -751,6 +811,13 @@ pub fn adjust_voltage( }) } + /// Creates [`FreqTable`] from [`Table`]. + #[cfg(CONFIG_CPU_FREQ)] + #[inline] + pub fn cpufreq_table(&mut self) -> Result<FreqTable> { + FreqTable::new(self) + } + /// Configures device with [`OPP`] matching the frequency value. #[inline] pub fn set_rate(&self, freq: Hertz) -> Result<()> {
Extend the OPP abstractions to include support for interacting with the cpufreq core, including the ability to retrieve frequency tables from OPP table. Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- rust/kernel/opp.rs | 67 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 67 insertions(+)