[RFC,V2,1/8] rust: Add initial bindings for OPP framework

This commit adds initial Rust bindings for the Operating performance
points (OPP) core. This adds bindings for `struct dev_pm_opp` and
`struct dev_pm_opp_data` to begin with.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 rust/bindings/bindings_helper.h |   1 +
 rust/kernel/lib.rs              |   2 +
 rust/kernel/opp.rs              | 156 ++++++++++++++++++++++++++++++++
 3 files changed, 159 insertions(+)
 create mode 100644 rust/kernel/opp.rs

On Fri, Jun 7, 2024 at 11:12 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> This commit adds initial Rust bindings for the Operating performance
> points (OPP) core. This adds bindings for `struct dev_pm_opp` and
> `struct dev_pm_opp_data` to begin with.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

> +//! Operating performance points.
> +//!
> +//! This module provides bindings for interacting with the OPP subsystem.
> +//!
> +//! C header: [`include/linux/pm_opp.h`](../../../../../../include/linux/pm_opp.h)

Please use srctree links instead.

C header: [`include/linux/pm_opp.h`](srctree/include/linux/pm_opp.h)

> +impl OPP {
> +    /// Creates a reference to a [`OPP`] from a valid pointer.
> +    ///
> +    /// # Safety
> +    ///
> +    /// The caller must ensure that `ptr` is valid and remains valid for the lifetime of the
> +    /// returned [`OPP`] reference.
> +    pub unsafe fn from_ptr_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.
> +        //
> +        // INVARIANT: The refcount is already incremented by the C API that returned the pointer,
> +        // and we pass ownership of the refcount to the new `ARef<OPP>`.
> +        Ok(unsafe { ARef::from_raw(ptr.cast()) })
> +    }
> +
> +    /// Creates a reference to a [`OPP`] from a valid pointer.
> +    ///
> +    /// # Safety
> +    ///
> +    /// The caller must ensure that `ptr` is valid and remains valid for the lifetime of the
> +    /// returned [`OPP`] reference.
> +    pub unsafe fn from_ptr(ptr: *mut bindings::dev_pm_opp) -> Result<ARef<Self>> {
> +        let opp = unsafe { Self::from_ptr_owned(ptr) }?;
> +
> +        // Take an extra reference to the OPP since the caller didn't take it.
> +        opp.inc_ref();
> +
> +        Ok(opp)
> +    }

I would recommend a slightly different approach here. You can provide
a method called `from_raw_opp` that takes a *mut bindings::dev_pm_opp
and returns a &Self. The ARef type provides a method that converts
&Self to ARef<Self> by taking a refcount. This way, users would also
be able to call OPP methods without giving Rust any refcounts. You can
compare to my file patchset, where I am going to rename the equivalent
method to `from_raw_file` in the next version.

As for `from_ptr_owned`, I would probably rename it to
`from_raw_opp_owned` or similar. It's often nice to use a more
descriptive name than just "ptr".

> +    fn as_mut_ptr(&self) -> *mut bindings::dev_pm_opp {
> +        self.0.get()
> +    }

I think most existing examples call this `as_raw` and mark it `#[inline]`.

> +    /// Adds an OPP dynamically.
> +    pub fn add(dev: ARef<Device>, mut data: Data) -> Result<()> {
> +        // 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) })
> +    }
> +
> +    /// Removes a dynamically added OPP.
> +    pub fn remove(dev: ARef<Device>, freq: u64) {
> +        // SAFETY: The requirements are satisfied by the existence of `Device` and its safety
> +        // requirements.
> +        unsafe { bindings::dev_pm_opp_remove(dev.as_raw(), freq) };
> +    }

Is it intentional that these methods take ownership of a refcount to
the device that it then drops after calling the C function?

Also, why are these methods defined on OPP when they appear to be
methods on Device and don't take any OPP argument?

Alice

On Fri, 07 Jun 2024 13:51, Alice Ryhl <aliceryhl@google.com> wrote:
>On Fri, Jun 7, 2024 at 11:12 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>>
>> This commit adds initial Rust bindings for the Operating performance
>> points (OPP) core. This adds bindings for `struct dev_pm_opp` and
>> `struct dev_pm_opp_data` to begin with.
>>
>> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
>
>> +//! Operating performance points.
>> +//!
>> +//! This module provides bindings for interacting with the OPP subsystem.
>> +//!
>> +//! C header: [`include/linux/pm_opp.h`](../../../../../../include/linux/pm_opp.h)
>
>Please use srctree links instead.
>
>C header: [`include/linux/pm_opp.h`](srctree/include/linux/pm_opp.h)
>
>> +impl OPP {
>> +    /// Creates a reference to a [`OPP`] from a valid pointer.
>> +    ///
>> +    /// # Safety
>> +    ///
>> +    /// The caller must ensure that `ptr` is valid and remains valid for the lifetime of the
>> +    /// returned [`OPP`] reference.
>> +    pub unsafe fn from_ptr_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.
>> +        //
>> +        // INVARIANT: The refcount is already incremented by the C API that returned the pointer,
>> +        // and we pass ownership of the refcount to the new `ARef<OPP>`.
>> +        Ok(unsafe { ARef::from_raw(ptr.cast()) })
>> +    }
>> +
>> +    /// Creates a reference to a [`OPP`] from a valid pointer.
>> +    ///
>> +    /// # Safety
>> +    ///
>> +    /// The caller must ensure that `ptr` is valid and remains valid for the lifetime of the
>> +    /// returned [`OPP`] reference.
>> +    pub unsafe fn from_ptr(ptr: *mut bindings::dev_pm_opp) -> Result<ARef<Self>> {
>> +        let opp = unsafe { Self::from_ptr_owned(ptr) }?;
>> +
>> +        // Take an extra reference to the OPP since the caller didn't take it.
>> +        opp.inc_ref();
>> +
>> +        Ok(opp)
>> +    }
>
>I would recommend a slightly different approach here. You can provide
>a method called `from_raw_opp` that takes a *mut bindings::dev_pm_opp
>and returns a &Self. The ARef type provides a method that converts
>&Self to ARef<Self> by taking a refcount. This way, users would also
>be able to call OPP methods without giving Rust any refcounts. You can

Wouldn't this allow for use-after-free? What if the refcount drops to 0 
before the method is called?

>As for `from_ptr_owned`, I would probably rename it to
>`from_raw_opp_owned` or similar. It's often nice to use a more
>descriptive name than just "ptr".
>I think most existing examples call this `as_raw` and mark it 
>`#[inline]`.

I think `ptr` is more idiomatic to Rust users, not that your suggestion 
is wrong. from_ptr_owned also implies the function signature.


>
>> +    /// Adds an OPP dynamically.
>> +    pub fn add(dev: ARef<Device>, mut data: Data) -> Result<()> {
>> +        // 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) })
>> +    }
>> +
>> +    /// Removes a dynamically added OPP.
>> +    pub fn remove(dev: ARef<Device>, freq: u64) {
>> +        // SAFETY: The requirements are satisfied by the existence of `Device` and its safety
>> +        // requirements.
>> +        unsafe { bindings::dev_pm_opp_remove(dev.as_raw(), freq) };
>> +    }
>
>Is it intentional that these methods take ownership of a refcount to
>the device that it then drops after calling the C function?

use-after-free again? Though I'm suggesting this without actually 
examining if it can happen.

Thanks Alice for reviewing.

On 07-06-24, 12:51, Alice Ryhl wrote:
> > +    /// Removes a dynamically added OPP.
> > +    pub fn remove(dev: ARef<Device>, freq: u64) {
> > +        // SAFETY: The requirements are satisfied by the existence of `Device` and its safety
> > +        // requirements.
> > +        unsafe { bindings::dev_pm_opp_remove(dev.as_raw(), freq) };
> > +    }
> 
> Also, why are these methods defined on OPP when they appear to be
> methods on Device and don't take any OPP argument?

I have changed them slightly to match what they are supposed to look
like and implemented them as method on the data itself.

All other comments are incorporated in the following diff:

diff --git a/rust/kernel/opp.rs b/rust/kernel/opp.rs
new file mode 100644
index 000000000000..b26e39a74635
--- /dev/null
+++ b/rust/kernel/opp.rs
@@ -0,0 +1,182 @@
+// 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).
+///
+/// # Invariants
+///
+/// The pointer stored in `Self` is non-null and valid for the lifetime of the ARef instance. In
+/// particular, the ARef instance owns an increment on underlying object’s reference count.
+#[repr(transparent)]
+pub struct OPP(Opaque<bindings::dev_pm_opp>);
+
+// 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 {}
+
+// 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 a reference to a [`OPP`] from a valid pointer.
+    ///
+    /// # Safety
+    ///
+    /// The caller must ensure that `ptr` is valid and remains valid for the lifetime of the
+    /// returned [`OPP`] reference.
+    pub unsafe fn from_raw_opp_owned<'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() })
+    }
+
+    /// Creates a reference to a [`OPP`] from a valid pointer.
+    ///
+    /// # Safety
+    ///
+    /// The caller must ensure that `ptr` is valid and remains valid for the lifetime of the
+    /// returned [`OPP`] reference.
+    pub unsafe fn from_raw_opp<'a>(ptr: *mut bindings::dev_pm_opp) -> Result<&'a Self> {
+        let opp = unsafe { Self::from_raw_opp_owned(ptr) }?;
+
+        // Take an extra reference to the OPP since the caller didn't take it.
+        opp.inc_ref();
+        Ok(opp)
+    }
+
+    #[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()) }
+    }
+}
+
+impl Drop for OPP {
+    fn drop(&mut self) {
+        // SAFETY: The safety requirements guarantee that the refcount is nonzero.
+        unsafe { bindings::dev_pm_opp_put(self.as_raw()) }
+    }
+}