linux-kernel - Re: [PATCH V3 2/2] rust: Add initial clk abstractions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <Z8mWJox-0IyP1uUo@cassiopeiae>
Date: Thu, 6 Mar 2025 13:33:42 +0100
From: Danilo Krummrich <dakr@...nel.org>
To: Viresh Kumar <viresh.kumar@...aro.org>
Cc: Miguel Ojeda <miguel.ojeda.sandonis@...il.com>,
	Michael Turquette <mturquette@...libre.com>,
	Stephen Boyd <sboyd@...nel.org>, Miguel Ojeda <ojeda@...nel.org>,
	Alex Gaynor <alex.gaynor@...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@...nel.org>,
	Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>,
	Russell King <linux@...linux.org.uk>, linux-clk@...r.kernel.org,
	linux-kernel@...r.kernel.org, rust-for-linux@...r.kernel.org,
	Vincent Guittot <vincent.guittot@...aro.org>,
	Daniel Almeida <daniel.almeida@...labora.com>
Subject: Re: [PATCH V3 2/2] rust: Add initial clk abstractions

On Tue, Mar 04, 2025 at 02:23:51PM +0530, Viresh Kumar wrote:
> +/// This structure represents the Rust abstraction for a C [`struct clk`].
> +///
> +/// # Invariants
> +///
> +/// A [`Clk`] instance always corresponds to a valid [`struct clk`] created by the C portion of the
> +/// kernel.
> +///
> +/// Instances of this type are reference-counted. Calling `get` ensures that the allocation remains
> +/// valid for the lifetime of the [`Clk`].
> +///
> +/// ## Example
> +///
> +/// The following example demonstrates how to obtain and configure a clock for a device.
> +///
> +/// ```
> +/// use kernel::clk::{Clk, Hertz};
> +/// use kernel::device::Device;
> +/// use kernel::error::Result;
> +///
> +/// fn configure_clk(dev: &Device) -> Result {
> +///     let clk = Clk::get(dev, "apb_clk")?;
> +///
> +///     clk.prepare_enable()?;
> +///
> +///     let expected_rate = Hertz::new(1_000_000_000);
> +///
> +///     if clk.rate() != expected_rate {
> +///         clk.set_rate(expected_rate)?;
> +///     }
> +///
> +///     clk.disable_unprepare();
> +///     Ok(())
> +/// }
> +/// ```
> +///
> +/// [`struct clk`]: https://docs.kernel.org/driver-api/clk.html
> +#[repr(transparent)]
> +pub struct Clk(*mut bindings::clk);
> +
> +impl Clk {
> +    /// Gets `Clk` corresponding to a [`Device`] and a connection id.
> +    pub fn get(dev: &Device, name: Option<&CStr>) -> Result<Self> {
> +        let con_id = if let Some(name) = name {
> +            name.as_ptr() as *const _
> +        } else {
> +            ptr::null()
> +        };
> +
> +        // SAFETY: It is safe to call `clk_get()` for a valid device pointer.
> +        Ok(Self(from_err_ptr(unsafe {
> +            bindings::clk_get(dev.as_raw(), con_id)
> +        })?))
> +    }
> +
> +    /// Obtain the raw `struct clk *`.
> +    #[inline]
> +    pub fn as_raw(&self) -> *mut bindings::clk {
> +        self.0
> +    }
> +
> +    /// Enable the clock.
> +    #[inline]
> +    pub fn enable(&self) -> Result {
> +        // SAFETY: It is safe to call clk APIs of the C code for a clock pointer earlier returned
> +        // by `clk_get()`.

You may want to add an invariant for this, i.e. something along the lines of
"Clk always holds either a pointer to a valid struct clk or a NULL pointer".

In this safety comment you can then say that by the type invariant of Clk
self.as_raw() is a valid argument for $func.

Not that your type invariant needs the NULL case, since OptionalClk may set Clk
to hold a NULL pointer.

I still think that a new type MaybeNull<T> would be nice to encapsulate this
invariant, but we can also wait until we get another use-case for it.