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

From: Manos Pitsidianakis
Date: Fri Jun 07 2024 - 07:25:12 EST


On Fri, 07 Jun 2024 13:51, Alice Ryhl <aliceryhl@xxxxxxxxxx> wrote:
On Fri, Jun 7, 2024 at 11:12 AM Viresh Kumar <viresh.kumar@xxxxxxxxxx> 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@xxxxxxxxxx>

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