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

Commit Message

Viresh Kumar July 3, 2024, 7:14 a.m. UTC

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.

Reviewed-by: Manos Pitsidianakis <[e-mail]>
Signed-off-by: Viresh Kumar <[e-mail]>
---
 rust/bindings/bindings_helper.h |   1 +
 rust/kernel/lib.rs              |   2 +
 rust/kernel/opp.rs              | 182 ++++++++++++++++++++++++++++++++
 3 files changed, 185 insertions(+)
 create mode 100644 rust/kernel/opp.rs

Comments

Boqun Feng July 3, 2024, 3:34 p.m. UTC | #1

Hi Viresh,

On Wed, Jul 03, 2024 at 12:44:26PM +0530, Viresh Kumar 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.
> 
> Reviewed-by: Manos Pitsidianakis <[e-mail]>
> Signed-off-by: Viresh Kumar <[e-mail]>
> ---
[...]
> +
> +/// 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.

Since you use `ARef` pattern now, you may want to rewrite this
"invariants".

> +#[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 {}
> +

Same for the above safety comments, as they are still based on the old
implementation.

> +// 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 {
[...]
> +
> +impl Drop for OPP {

I don't think you need the `drop` implementation here, since it should
be already handled by `impl AlwaysRefCounted`, could you try to a doc
test for this? Something like:

	let opp: ARef<OPP> = <from a raw dev_pm_opp ponter whose refcount is 1>
	drop(opp);

IIUC, this will result double-free with the current implementation.

Overall, `OPP` is now representing to the actual device instead of the
pointer to the device, so the `drop` function won't need to handle the
refcounting.

Regards,
Boqun

> +    fn drop(&mut self) {
> +        // SAFETY: The safety requirements guarantee that the refcount is nonzero.
> +        unsafe { bindings::dev_pm_opp_put(self.as_raw()) }
> +    }
> +}
> -- 
> 2.31.1.272.g89b43f80a514
>

Viresh Kumar July 5, 2024, 11:02 a.m. UTC | #2

Hi Boqun,

On 03-07-24, 08:34, Boqun Feng wrote:
> On Wed, Jul 03, 2024 at 12:44:26PM +0530, Viresh Kumar wrote:
> > +/// 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.
> 
> Since you use `ARef` pattern now, you may want to rewrite this
> "invariants".

I copied it from the device's documentation. What all details should I
be writing here ? A link to some other implementation would be useful.

> > +impl Drop for OPP {
> 
> I don't think you need the `drop` implementation here, since it should
> be already handled by `impl AlwaysRefCounted`,

Right.

> could you try to a doc
> test for this? Something like:
> 
> 	let opp: ARef<OPP> = <from a raw dev_pm_opp ponter whose refcount is 1>
> 	drop(opp);

I now tested it with a kernel test to see what's going on internally

> IIUC, this will result double-free with the current implementation.

Quite the opposite actually. I am getting double get and a single put :)

Thanks a lot for pointing me to this direction as I have found that my
implementation was incorrect. This is how I understand it, I can be
wrong since I am okayish with Rust:

- What's getting returned from `from_raw_opp/from_raw_opp_owned` is a
  reference: `<&'a Self>`.

- Since this is a reference, when it gets out of scope, nothing
  happens. i.e. the `drop()` fn of `struct OPP` never gets called for
  the OPP object, as there is no real OPP object, but just a
  reference.

- When this gets converted to an `ARef` object (implicit typecasting),
  we increment the count. And when that gets dropped, we decrement it.
  But Apart from an `ARef` object, only the reference to the OPP gets
  dropped and hence again, drop() doesn't get called.

- The important part here is that `from_raw_opp()` shouldn't be
  incrementing the refcount, as drop() will never get called. And
  since we reach here from the C implementation, the OPP will remain
  valid for the function call.

- On the other hand, I can't return <&'a Self> from
  from_raw_opp_owned() anymore. In this case the OPP core has already
  incremented the refcount of the OPP (while it searched the OPP on
  behalf of the Rust code). Whatever is returned here, must drop the
  refcount when it goes out of scope. Also the returned OPP reference
  can live for a longer period of time in this case, since the call
  originates from the Rust side. So, it needs to be an explicit
  conversion to ARef which won't increment the refcount, but just
  decrement when the ARef gets out of scope.

Here is the diff that I need:

diff --git a/rust/kernel/opp.rs b/rust/kernel/opp.rs
index aaf220e6aeac..a99950b4d835 100644
--- a/rust/kernel/opp.rs
+++ b/rust/kernel/opp.rs
@@ -692,7 +692,7 @@ pub fn opp_from_freq(
         })?;
 
         // SAFETY: The `ptr` is guaranteed by the C code to be valid.
-        Ok(unsafe { OPP::from_raw_opp_owned(ptr)?.into() })
+        unsafe { OPP::from_raw_opp_owned(ptr) }
     }
 
     /// Finds OPP based on level.
@@ -718,7 +718,7 @@ pub fn opp_from_level(&self, mut level: u32, stype: SearchType) -> Result<ARef<O
         })?;
 
         // SAFETY: The `ptr` is guaranteed by the C code to be valid.
-        Ok(unsafe { OPP::from_raw_opp_owned(ptr)?.into() })
+        unsafe { OPP::from_raw_opp_owned(ptr) }
     }
 
     /// Finds OPP based on bandwidth.
@@ -743,7 +743,7 @@ pub fn opp_from_bw(&self, mut bw: u32, index: i32, stype: SearchType) -> Result<
         })?;
 
         // SAFETY: The `ptr` is guaranteed by the C code to be valid.
-        Ok(unsafe { OPP::from_raw_opp_owned(ptr)?.into() })
+        unsafe { OPP::from_raw_opp_owned(ptr) }
     }
 
     /// Enable the OPP.
@@ -834,31 +834,33 @@ unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
 }
 
 impl OPP {
-    /// Creates a reference to a [`OPP`] from a valid pointer.
+    /// Creates an owned 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() })
+    /// The caller must ensure that `ptr` is valid and OPP's refcount is incremented. The refcount
+    /// will be decremented by `dec_ref` when the ARef object is dropped.
+    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.
+        //
+        // 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.
+    /// The caller must ensure that `ptr` is valid and remains valid for the duration of 'a. The
+    /// refcount is not updated by the Rust API unless the returned reference is converted to an
+    /// ARef object.
     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)
+        // 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]
@@ -910,10 +912,3 @@ pub fn is_turbo(&self) -> bool {
         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()) }
-    }
-}

Makes sense ?

Boqun Feng July 5, 2024, 6:19 p.m. UTC | #3

On Fri, Jul 05, 2024 at 04:32:28PM +0530, Viresh Kumar wrote:
> Hi Boqun,
> 
> On 03-07-24, 08:34, Boqun Feng wrote:
> > On Wed, Jul 03, 2024 at 12:44:26PM +0530, Viresh Kumar wrote:
> > > +/// 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.
> > 
> > Since you use `ARef` pattern now, you may want to rewrite this
> > "invariants".
> 
> I copied it from the device's documentation. What all details should I
> be writing here ? A link to some other implementation would be useful.
> 

"Invariants" defines "what's a valid instance of a type", so here I
think you could drop the "# Invariants" section at all, unless you need
to use some of the invariants of fields in `dev_mp_opp` as a
justification for safety. For example if you have a pointer in
`dev_mp_opp`, and it's always pointing to a valid `T`, and in one method
of `OPP`, you need to dereference the pointer.

> > > +impl Drop for OPP {
> > 
> > I don't think you need the `drop` implementation here, since it should
> > be already handled by `impl AlwaysRefCounted`,
> 
> Right.
> 
> > could you try to a doc
> > test for this? Something like:
> > 
> > 	let opp: ARef<OPP> = <from a raw dev_pm_opp ponter whose refcount is 1>
> > 	drop(opp);
> 
> I now tested it with a kernel test to see what's going on internally
> 
> > IIUC, this will result double-free with the current implementation.
> 
> Quite the opposite actually. I am getting double get and a single put :)
> 
> Thanks a lot for pointing me to this direction as I have found that my
> implementation was incorrect. This is how I understand it, I can be
> wrong since I am okayish with Rust:
> 
> - What's getting returned from `from_raw_opp/from_raw_opp_owned` is a
>   reference: `<&'a Self>`.
> 
> - Since this is a reference, when it gets out of scope, nothing
>   happens. i.e. the `drop()` fn of `struct OPP` never gets called for
>   the OPP object, as there is no real OPP object, but just a
>   reference.
> 
> - When this gets converted to an `ARef` object (implicit typecasting),
>   we increment the count. And when that gets dropped, we decrement it.
>   But Apart from an `ARef` object, only the reference to the OPP gets
>   dropped and hence again, drop() doesn't get called.
> 
> - The important part here is that `from_raw_opp()` shouldn't be
>   incrementing the refcount, as drop() will never get called. And
>   since we reach here from the C implementation, the OPP will remain
>   valid for the function call.
> 

Right. I wasn't aware that you didn't return a `ARef<OPP>`, which mean
the return value won't handle the refcounting automatically (and because
of that, the refcount shouldn't be increased.)

> - On the other hand, I can't return <&'a Self> from
>   from_raw_opp_owned() anymore. In this case the OPP core has already
>   incremented the refcount of the OPP (while it searched the OPP on
>   behalf of the Rust code). Whatever is returned here, must drop the
>   refcount when it goes out of scope. Also the returned OPP reference
>   can live for a longer period of time in this case, since the call
>   originates from the Rust side. So, it needs to be an explicit
>   conversion to ARef which won't increment the refcount, but just
>   decrement when the ARef gets out of scope.
> 

I think you got it correct ;-) The takeaway is: there are different
types of pointers/references, 1) if users only use a `struct dev_pm_opp
*` shortly and the scope can be told at compile time, you can provide a
`&OPP`, 2) if the scope of usage is somewhat dynamic, and the users
should descrease the refcount after use it, the API should return a
`ARef<OPP>`. And since the refcount inc/dec are already maintained in
`ARef<_>`, `OPP::drop` shouldn't touch the refcount anymore.

Also as you already noticed, calling `into` on a `&OPP` will give a
`ARef<OPP>`, which includes an increment of the refcount, and usually
should be used when the users want to switch to long-term usage after a
quick short-term use (e.g. do a quick check of the status and decide
some more work is needed, and maybe need to transfer the ownership of
the pointer to a workqueue or something).

> Here is the diff that I need:
> 
> diff --git a/rust/kernel/opp.rs b/rust/kernel/opp.rs
> index aaf220e6aeac..a99950b4d835 100644
> --- a/rust/kernel/opp.rs
> +++ b/rust/kernel/opp.rs
> @@ -692,7 +692,7 @@ pub fn opp_from_freq(
>          })?;
>  
>          // SAFETY: The `ptr` is guaranteed by the C code to be valid.
> -        Ok(unsafe { OPP::from_raw_opp_owned(ptr)?.into() })
> +        unsafe { OPP::from_raw_opp_owned(ptr) }
>      }
>  
>      /// Finds OPP based on level.
> @@ -718,7 +718,7 @@ pub fn opp_from_level(&self, mut level: u32, stype: SearchType) -> Result<ARef<O
>          })?;
>  
>          // SAFETY: The `ptr` is guaranteed by the C code to be valid.
> -        Ok(unsafe { OPP::from_raw_opp_owned(ptr)?.into() })
> +        unsafe { OPP::from_raw_opp_owned(ptr) }
>      }
>  
>      /// Finds OPP based on bandwidth.
> @@ -743,7 +743,7 @@ pub fn opp_from_bw(&self, mut bw: u32, index: i32, stype: SearchType) -> Result<
>          })?;
>  
>          // SAFETY: The `ptr` is guaranteed by the C code to be valid.
> -        Ok(unsafe { OPP::from_raw_opp_owned(ptr)?.into() })
> +        unsafe { OPP::from_raw_opp_owned(ptr) }
>      }
>  
>      /// Enable the OPP.
> @@ -834,31 +834,33 @@ unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
>  }
>  
>  impl OPP {
> -    /// Creates a reference to a [`OPP`] from a valid pointer.
> +    /// Creates an owned 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() })
> +    /// The caller must ensure that `ptr` is valid and OPP's refcount is incremented. The refcount
> +    /// will be decremented by `dec_ref` when the ARef object is dropped.

Usually the second sentense "The refcount ..." won't need to be put in
the safety requirement, as it just describes how ARef works.

> +    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.
> +        //
> +        // 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>`.

You can probably drop the "INVARIANT", as it's an invariant of `ARef`
which already guarantees since the safety requirement of
`ARef::from_raw()` meets. At least you can write them as "normal"
comments.

> +        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.
> +    /// The caller must ensure that `ptr` is valid and remains valid for the duration of 'a. The
> +    /// refcount is not updated by the Rust API unless the returned reference is converted to an
> +    /// ARef object.

Again you could drop the second sentence, or you can put it somewhere
outside the "Safety" section, if you want to.

>      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)
> +        // 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]
> @@ -910,10 +912,3 @@ pub fn is_turbo(&self) -> bool {
>          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()) }
> -    }
> -}
> 
> Makes sense ?
> 

Yep, looks good to me!

Regards,
Boqun

> -- 
> viresh

Viresh Kumar July 9, 2024, 11:02 a.m. UTC | #4

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.

Boqun Feng July 9, 2024, 5:45 p.m. UTC | #5

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

Viresh Kumar July 10, 2024, 7:36 a.m. UTC | #6

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 :)

Viresh Kumar July 10, 2024, 11:06 a.m. UTC | #7

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()) }
+    }
+}

Boqun Feng July 10, 2024, 9:58 p.m. UTC | #8

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()) }
> +    }
> +}

Patch

diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index d8b54b9fa4d0..1bf8e053c8f4 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -15,6 +15,7 @@ 
 #include <linux/of.h>
 #include <linux/phy.h>
 #include <linux/platform_device.h>
+#include <linux/pm_opp.h>
 #include <linux/refcount.h>
 #include <linux/sched.h>
 #include <linux/slab.h>
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 3bf1089b87a3..e309d7774cbd 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -45,6 +45,8 @@ 
 #[cfg(CONFIG_NET)]
 pub mod net;
 pub mod of;
+#[cfg(CONFIG_PM_OPP)]
+pub mod opp;
 pub mod platform;
 pub mod prelude;
 pub mod print;
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()) }
+    }
+}