Message-ID: <20240705110228.qqhhynbwwuwpcdeo@vireshk-i7>
Date: Fri, 5 Jul 2024 16:32:28 +0530
From: Viresh Kumar <viresh.kumar@...aro.org>
To: Boqun Feng <boqun.feng@...il.com>
Cc: "Rafael J. Wysocki" <rafael@...nel.org>,
	Miguel Ojeda <miguel.ojeda.sandonis@...il.com>,
	Miguel Ojeda <ojeda@...nel.org>,
	Alex Gaynor <alex.gaynor@...il.com>,
	Wedson Almeida Filho <wedsonaf@...il.com>,
	Gary Guo <gary@...yguo.net>,
	Björn Roy Baron <bjorn3_gh@...tonmail.com>,
	Benno Lossin <benno.lossin@...ton.me>,
	Andreas Hindborg <a.hindborg@...sung.com>,
	Alice Ryhl <aliceryhl@...gle.com>, linux-pm@...r.kernel.org,
	Vincent Guittot <vincent.guittot@...aro.org>,
	Stephen Boyd <sboyd@...nel.org>, Nishanth Menon <nm@...com>,
	rust-for-linux@...r.kernel.org,
	Manos Pitsidianakis <manos.pitsidianakis@...aro.org>,
	Erik Schilling <erik.schilling@...aro.org>,
	Alex Bennée <alex.bennee@...aro.org>,
	Joakim Bech <joakim.bech@...aro.org>, Rob Herring <robh@...nel.org>,
	linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH V3 1/8] rust: Add initial bindings for OPP framework

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 ?

-- 
viresh

Powered by blists - more mailing lists