Message-ID: <Zo8D86qiZ7aYdZhY@boqun-archlinux>
Date: Wed, 10 Jul 2024 14:58:11 -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 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()) }
> +    }
> +}

Powered by blists - more mailing lists