Message-ID: <Zog5NYptZRaqbUBz@boqun-archlinux>
Date: Fri, 5 Jul 2024 11:19:33 -0700
From: Boqun Feng <boqun.feng@...il.com>
To: Viresh Kumar <viresh.kumar@...aro.org>
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

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

Powered by blists - more mailing lists