| Message ID | cover.1719990273.git.viresh.kumar@linaro.org |
|---|---|
| Headers | show |
| Series | Rust bindings for cpufreq and OPP core + sample driver | expand |
On 03-07-24, 08:34, Boqun Feng wrote: > On Wed, Jul 03, 2024 at 12:44:26PM +0530, Viresh Kumar wrote: > > +// SAFETY: `OPP` only holds a pointer to a C OPP, which is safe to be used from any thread. > > +unsafe impl Send for OPP {} > > + > > +// SAFETY: `OPP` only holds a pointer to a C OPP, references to which are safe to be used from any > > +// thread. > > +unsafe impl Sync for OPP {} > > + > > Same for the above safety comments, as they are still based on the old > implementation. Do I still need to change these ? Since we aren't always using ARef now.
On Tue, Jul 09, 2024 at 04:32:45PM +0530, Viresh Kumar wrote: > On 03-07-24, 08:34, Boqun Feng wrote: > > On Wed, Jul 03, 2024 at 12:44:26PM +0530, Viresh Kumar wrote: > > > +// SAFETY: `OPP` only holds a pointer to a C OPP, which is safe to be used from any thread. > > > +unsafe impl Send for OPP {} > > > + > > > +// SAFETY: `OPP` only holds a pointer to a C OPP, references to which are safe to be used from any > > > +// thread. > > > +unsafe impl Sync for OPP {} > > > + > > > > Same for the above safety comments, as they are still based on the old > > implementation. > > Do I still need to change these ? Since we aren't always using ARef > now. > Correct, you will still need to change these. You're welcome to submit a draft version here and I can help take a look before you send out a whole new version, if you prefer that way. Regards, Boqun > -- > viresh
On 09-07-24, 10:45, Boqun Feng wrote: > On Tue, Jul 09, 2024 at 04:32:45PM +0530, Viresh Kumar wrote: > > On 03-07-24, 08:34, Boqun Feng wrote: > > > On Wed, Jul 03, 2024 at 12:44:26PM +0530, Viresh Kumar wrote: > > > > +// SAFETY: `OPP` only holds a pointer to a C OPP, which is safe to be used from any thread. > > > > +unsafe impl Send for OPP {} > > > > + > > > > +// SAFETY: `OPP` only holds a pointer to a C OPP, references to which are safe to be used from any > > > > +// thread. > > > > +unsafe impl Sync for OPP {} > > > > + > > > > > > Same for the above safety comments, as they are still based on the old > > > implementation. > > > > Do I still need to change these ? Since we aren't always using ARef > > now. > > > > Correct, you will still need to change these. You're welcome to submit > a draft version here and I can help take a look before you send out a > whole new version, if you prefer that way. I am not entirely sure what the change must be like that :)
On 10-07-24, 13:06, Viresh Kumar wrote:
> I am not entirely sure what the change must be like that :)
Anyway, I have looked around and made some changes. Please see how it
looks now. Thanks Boqun.
diff --git a/rust/kernel/opp.rs b/rust/kernel/opp.rs
new file mode 100644
index 000000000000..2ef262c4640a
--- /dev/null
+++ b/rust/kernel/opp.rs
@@ -0,0 +1,188 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Operating performance points.
+//!
+//! This module provides bindings for interacting with the OPP subsystem.
+//!
+//! C header: [`include/linux/pm_opp.h`](srctree/include/linux/pm_opp.h)
+
+use crate::{
+ bindings,
+ device::Device,
+ error::{code::*, to_result, Result},
+ types::{ARef, AlwaysRefCounted, Opaque},
+};
+
+use core::ptr;
+
+/// Dynamically created Operating performance point (OPP).
+pub struct Token {
+ dev: ARef<Device>,
+ freq: u64,
+}
+
+impl Token {
+ /// Adds an OPP dynamically.
+ pub fn new(dev: &ARef<Device>, mut data: Data) -> Result<Self> {
+ // SAFETY: The requirements are satisfied by the existence of `Device` and its safety
+ // requirements.
+ to_result(unsafe { bindings::dev_pm_opp_add_dynamic(dev.as_raw(), &mut data.0) })?;
+ Ok(Self {
+ dev: dev.clone(),
+ freq: data.freq(),
+ })
+ }
+}
+
+impl Drop for Token {
+ fn drop(&mut self) {
+ // SAFETY: The requirements are satisfied by the existence of `Device` and its safety
+ // requirements.
+ unsafe { bindings::dev_pm_opp_remove(self.dev.as_raw(), self.freq) };
+ }
+}
+
+/// Equivalent to `struct dev_pm_opp_data` in the C Code.
+#[repr(transparent)]
+pub struct Data(bindings::dev_pm_opp_data);
+
+impl Data {
+ /// Creates new instance of [`Data`].
+ pub fn new(freq: u64, u_volt: u64, level: u32, turbo: bool) -> Self {
+ Self(bindings::dev_pm_opp_data {
+ turbo,
+ freq,
+ u_volt,
+ level,
+ })
+ }
+
+ /// Adds an OPP dynamically. The OPP is freed once the [`Token`] gets freed.
+ pub fn add_opp(self, dev: &ARef<Device>) -> Result<Token> {
+ Token::new(dev, self)
+ }
+
+ fn freq(&self) -> u64 {
+ self.0.freq
+ }
+}
+
+/// Operating performance point (OPP).
+///
+/// Wraps the kernel's `struct dev_pm_opp`.
+///
+/// The pointer stored in `Self` is non-null and valid for the lifetime of the `OPP` instance.
+///
+/// # Refcounting
+///
+/// Instances of this type are reference-counted. The reference count is incremented by the
+/// `dev_pm_opp_get()` function and decremented by `dev_pm_opp_put`. The Rust type `ARef<OPP>`
+/// represents a pointer that owns a reference count on the OPP.
+///
+/// A reference to the `OPP`, `&OPP` isn't refcounted by the Rust code. The C code guarantees that
+/// the pointer stored in `OPP` is is valid for the lifetime of the reference and hence refcounting
+/// isn't required.
+
+#[repr(transparent)]
+pub struct OPP(Opaque<bindings::dev_pm_opp>);
+
+// SAFETY: It is okay to send ownership of `OPP` across thread boundaries and `OPP::dec_ref` can be
+// called from any thread.
+unsafe impl Send for OPP {}
+
+// SAFETY: It's OK to access `OPP` through shared references from other threads because we're
+// either accessing properties that don't change or that are properly synchronised by C code.
+unsafe impl Sync for OPP {}
+
+// SAFETY: The type invariants guarantee that [`OPP`] is always refcounted.
+unsafe impl AlwaysRefCounted for OPP {
+ fn inc_ref(&self) {
+ // SAFETY: The existence of a shared reference means that the refcount is nonzero.
+ unsafe { bindings::dev_pm_opp_get(self.0.get()) };
+ }
+
+ unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
+ // SAFETY: The safety requirements guarantee that the refcount is nonzero.
+ unsafe { bindings::dev_pm_opp_put(obj.cast().as_ptr()) }
+ }
+}
+
+impl OPP {
+ /// Creates an owned reference to a [`OPP`] from a valid pointer.
+ ///
+ /// The refcount is incremented by the C code and will be decremented by `dec_ref()` when the
+ /// ARef object is dropped.
+ ///
+ /// # Safety
+ ///
+ /// The caller must ensure that `ptr` is valid and OPP's refcount is incremented.
+ pub unsafe fn from_raw_opp_owned(ptr: *mut bindings::dev_pm_opp) -> Result<ARef<Self>> {
+ let ptr = ptr::NonNull::new(ptr).ok_or(ENODEV)?;
+
+ // SAFETY: The safety requirements guarantee the validity of the pointer.
+ Ok(unsafe { ARef::from_raw(ptr.cast()) })
+ }
+
+ /// Creates a reference to a [`OPP`] from a valid pointer.
+ ///
+ /// The refcount is not updated by the Rust API unless the returned reference is converted to
+ /// an ARef object.
+ ///
+ /// # Safety
+ ///
+ /// The caller must ensure that `ptr` is valid and remains valid for the duration of 'a.
+ pub unsafe fn from_raw_opp<'a>(ptr: *mut bindings::dev_pm_opp) -> Result<&'a Self> {
+ // SAFETY: The caller guarantees that the pointer is not dangling and stays valid for the
+ // duration of 'a. The cast is okay because `OPP` is `repr(transparent)`.
+ Ok(unsafe { &*ptr.cast() })
+ }
+
+ #[inline]
+ fn as_raw(&self) -> *mut bindings::dev_pm_opp {
+ self.0.get()
+ }
+
+ /// Returns the frequency of an OPP.
+ pub fn freq(&self, index: Option<u32>) -> u64 {
+ let index = index.unwrap_or(0);
+
+ // SAFETY: By the type invariants, we know that `self` owns a reference, so it is safe to
+ // use it.
+ unsafe { bindings::dev_pm_opp_get_freq_indexed(self.as_raw(), index) }
+ }
+
+ /// Returns the voltage of an OPP.
+ pub fn voltage(&self) -> u64 {
+ // SAFETY: By the type invariants, we know that `self` owns a reference, so it is safe to
+ // use it.
+ unsafe { bindings::dev_pm_opp_get_voltage(self.as_raw()) }
+ }
+
+ /// Returns the level of an OPP.
+ pub fn level(&self) -> u32 {
+ // SAFETY: By the type invariants, we know that `self` owns a reference, so it is safe to
+ // use it.
+ unsafe { bindings::dev_pm_opp_get_level(self.as_raw()) }
+ }
+
+ /// Returns the power of an OPP.
+ pub fn power(&self) -> u64 {
+ // SAFETY: By the type invariants, we know that `self` owns a reference, so it is safe to
+ // use it.
+ unsafe { bindings::dev_pm_opp_get_power(self.as_raw()) }
+ }
+
+ /// Returns the required pstate of an OPP.
+ pub fn required_pstate(&self, index: u32) -> u32 {
+ // SAFETY: By the type invariants, we know that `self` owns a reference, so it is safe to
+ // use it.
+ unsafe { bindings::dev_pm_opp_get_required_pstate(self.as_raw(), index) }
+ }
+
+ /// Returns true if the OPP is turbo.
+ pub fn is_turbo(&self) -> bool {
+ // SAFETY: By the type invariants, we know that `self` owns a reference, so it is safe to
+ // use it.
+ unsafe { bindings::dev_pm_opp_is_turbo(self.as_raw()) }
+ }
+}
On Wed, Jul 10, 2024 at 04:36:07PM +0530, Viresh Kumar wrote: > On 10-07-24, 13:06, Viresh Kumar wrote: > > I am not entirely sure what the change must be like that :) > > Anyway, I have looked around and made some changes. Please see how it > looks now. Thanks Boqun. > > diff --git a/rust/kernel/opp.rs b/rust/kernel/opp.rs > new file mode 100644 > index 000000000000..2ef262c4640a > --- /dev/null > +++ b/rust/kernel/opp.rs > @@ -0,0 +1,188 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +//! Operating performance points. > +//! > +//! This module provides bindings for interacting with the OPP subsystem. > +//! > +//! C header: [`include/linux/pm_opp.h`](srctree/include/linux/pm_opp.h) > + > +use crate::{ > + bindings, > + device::Device, > + error::{code::*, to_result, Result}, > + types::{ARef, AlwaysRefCounted, Opaque}, > +}; > + > +use core::ptr; > + > +/// Dynamically created Operating performance point (OPP). > +pub struct Token { > + dev: ARef<Device>, > + freq: u64, > +} > + > +impl Token { > + /// Adds an OPP dynamically. > + pub fn new(dev: &ARef<Device>, mut data: Data) -> Result<Self> { > + // SAFETY: The requirements are satisfied by the existence of `Device` and its safety > + // requirements. > + to_result(unsafe { bindings::dev_pm_opp_add_dynamic(dev.as_raw(), &mut data.0) })?; > + Ok(Self { > + dev: dev.clone(), > + freq: data.freq(), > + }) > + } > +} > + > +impl Drop for Token { > + fn drop(&mut self) { > + // SAFETY: The requirements are satisfied by the existence of `Device` and its safety > + // requirements. > + unsafe { bindings::dev_pm_opp_remove(self.dev.as_raw(), self.freq) }; > + } > +} > + > +/// Equivalent to `struct dev_pm_opp_data` in the C Code. > +#[repr(transparent)] > +pub struct Data(bindings::dev_pm_opp_data); > + > +impl Data { > + /// Creates new instance of [`Data`]. > + pub fn new(freq: u64, u_volt: u64, level: u32, turbo: bool) -> Self { > + Self(bindings::dev_pm_opp_data { > + turbo, > + freq, > + u_volt, > + level, > + }) > + } > + > + /// Adds an OPP dynamically. The OPP is freed once the [`Token`] gets freed. > + pub fn add_opp(self, dev: &ARef<Device>) -> Result<Token> { > + Token::new(dev, self) > + } > + > + fn freq(&self) -> u64 { > + self.0.freq > + } > +} > + > +/// Operating performance point (OPP). > +/// > +/// Wraps the kernel's `struct dev_pm_opp`. > +/// > +/// The pointer stored in `Self` is non-null and valid for the lifetime of the `OPP` instance. > +/// > +/// # Refcounting > +/// > +/// Instances of this type are reference-counted. The reference count is incremented by the > +/// `dev_pm_opp_get()` function and decremented by `dev_pm_opp_put`. The Rust type `ARef<OPP>` > +/// represents a pointer that owns a reference count on the OPP. > +/// > +/// A reference to the `OPP`, `&OPP` isn't refcounted by the Rust code. The C code guarantees that > +/// the pointer stored in `OPP` is is valid for the lifetime of the reference and hence refcounting "the pointer stored in `OPP`" is incorrect, I think what you tried to say here is "the C code guarantees a pointer to `OPP` is valid with at lease one reference count for the lifetime of the `&OPP`". But this comment can be avoided at all since it's generally true for most references. It's OK if you want to write this here as a special note. > +/// isn't required. > + > +#[repr(transparent)] > +pub struct OPP(Opaque<bindings::dev_pm_opp>); > + > +// SAFETY: It is okay to send ownership of `OPP` across thread boundaries and `OPP::dec_ref` can be > +// called from any thread. Whether `OPP::dec_ref` can be called from any thread is unrelated to whether `OPP` is `Send` or not. `Send` means you could own an `OPP` (instead of an `ARef<OPP>`) that's created by other thread/context, as long as `Opp::drop` is safe to call from a different thread (other than the one that creates it), it should be safe to send. > +unsafe impl Send for OPP {} > + > +// SAFETY: It's OK to access `OPP` through shared references from other threads because we're > +// either accessing properties that don't change or that are properly synchronised by C code. LGTM. > +unsafe impl Sync for OPP {} > + > +// SAFETY: The type invariants guarantee that [`OPP`] is always refcounted. Since you use "type invariants" here, you should rename the "# Refcounting" section before "OPP" as "# Invariants". > +unsafe impl AlwaysRefCounted for OPP { > + fn inc_ref(&self) { > + // SAFETY: The existence of a shared reference means that the refcount is nonzero. > + unsafe { bindings::dev_pm_opp_get(self.0.get()) }; > + } > + > + unsafe fn dec_ref(obj: ptr::NonNull<Self>) { > + // SAFETY: The safety requirements guarantee that the refcount is nonzero. > + unsafe { bindings::dev_pm_opp_put(obj.cast().as_ptr()) } > + } > +} > + > +impl OPP { > + /// Creates an owned reference to a [`OPP`] from a valid pointer. > + /// > + /// The refcount is incremented by the C code and will be decremented by `dec_ref()` when the > + /// ARef object is dropped. > + /// > + /// # Safety > + /// > + /// The caller must ensure that `ptr` is valid and OPP's refcount is incremented. One part is missing in this safety requirement, the caller needs to guarantee "forget"ing one reference count of the object, because that's owned by the returned value, see the safety requirement of `ARef::from_raw()` for more informatoin. Regards, Boqun > + pub unsafe fn from_raw_opp_owned(ptr: *mut bindings::dev_pm_opp) -> Result<ARef<Self>> { > + let ptr = ptr::NonNull::new(ptr).ok_or(ENODEV)?; > + > + // SAFETY: The safety requirements guarantee the validity of the pointer. > + Ok(unsafe { ARef::from_raw(ptr.cast()) }) > + } > + > + /// Creates a reference to a [`OPP`] from a valid pointer. > + /// > + /// The refcount is not updated by the Rust API unless the returned reference is converted to > + /// an ARef object. > + /// > + /// # Safety > + /// > + /// The caller must ensure that `ptr` is valid and remains valid for the duration of 'a. > + pub unsafe fn from_raw_opp<'a>(ptr: *mut bindings::dev_pm_opp) -> Result<&'a Self> { > + // SAFETY: The caller guarantees that the pointer is not dangling and stays valid for the > + // duration of 'a. The cast is okay because `OPP` is `repr(transparent)`. > + Ok(unsafe { &*ptr.cast() }) > + } > + > + #[inline] > + fn as_raw(&self) -> *mut bindings::dev_pm_opp { > + self.0.get() > + } > + > + /// Returns the frequency of an OPP. > + pub fn freq(&self, index: Option<u32>) -> u64 { > + let index = index.unwrap_or(0); > + > + // SAFETY: By the type invariants, we know that `self` owns a reference, so it is safe to > + // use it. > + unsafe { bindings::dev_pm_opp_get_freq_indexed(self.as_raw(), index) } > + } > + > + /// Returns the voltage of an OPP. > + pub fn voltage(&self) -> u64 { > + // SAFETY: By the type invariants, we know that `self` owns a reference, so it is safe to > + // use it. > + unsafe { bindings::dev_pm_opp_get_voltage(self.as_raw()) } > + } > + > + /// Returns the level of an OPP. > + pub fn level(&self) -> u32 { > + // SAFETY: By the type invariants, we know that `self` owns a reference, so it is safe to > + // use it. > + unsafe { bindings::dev_pm_opp_get_level(self.as_raw()) } > + } > + > + /// Returns the power of an OPP. > + pub fn power(&self) -> u64 { > + // SAFETY: By the type invariants, we know that `self` owns a reference, so it is safe to > + // use it. > + unsafe { bindings::dev_pm_opp_get_power(self.as_raw()) } > + } > + > + /// Returns the required pstate of an OPP. > + pub fn required_pstate(&self, index: u32) -> u32 { > + // SAFETY: By the type invariants, we know that `self` owns a reference, so it is safe to > + // use it. > + unsafe { bindings::dev_pm_opp_get_required_pstate(self.as_raw(), index) } > + } > + > + /// Returns true if the OPP is turbo. > + pub fn is_turbo(&self) -> bool { > + // SAFETY: By the type invariants, we know that `self` owns a reference, so it is safe to > + // use it. > + unsafe { bindings::dev_pm_opp_is_turbo(self.as_raw()) } > + } > +}