[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAH5fLgjChZCtTUnHVHJat-sXFyLVE+MgDXrNDiUD0LNsUndpBQ@mail.gmail.com>
Date: Fri, 7 Jun 2024 12:51:33 +0200
From: Alice Ryhl <aliceryhl@...gle.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>, Boqun Feng <boqun.feng@...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>,
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 V2 1/8] rust: Add initial bindings for OPP framework
On Fri, Jun 7, 2024 at 11:12 AM Viresh Kumar <viresh.kumar@...aro.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@...aro.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
Powered by blists - more mailing lists