linux-kernel - Re: [PATCH v3 1/3] rust: clk: use the type-state pattern

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20260203101726.2cec1050@fedora>
Date: Tue, 3 Feb 2026 10:17:26 +0100
From: Boris Brezillon <boris.brezillon@...labora.com>
To: Daniel Almeida <daniel.almeida@...labora.com>
Cc: "Rafael J. Wysocki" <rafael@...nel.org>, Viresh Kumar
 <viresh.kumar@...aro.org>, Danilo Krummrich <dakr@...nel.org>, Alice Ryhl
 <aliceryhl@...gle.com>, Maarten Lankhorst
 <maarten.lankhorst@...ux.intel.com>, Maxime Ripard <mripard@...nel.org>,
 Thomas Zimmermann <tzimmermann@...e.de>, David Airlie <airlied@...il.com>,
 Simona Vetter <simona@...ll.ch>, Drew Fustini <fustini@...nel.org>, Guo Ren
 <guoren@...nel.org>, Fu Wei <wefu@...hat.com>, Uwe Kleine-König <ukleinek@...nel.org>, Michael Turquette
 <mturquette@...libre.com>, Stephen Boyd <sboyd@...nel.org>, Miguel Ojeda
 <ojeda@...nel.org>, Boqun Feng <boqun.feng@...il.com>, Gary Guo
 <gary@...yguo.net>, Björn Roy Baron
 <bjorn3_gh@...tonmail.com>, Benno Lossin <lossin@...nel.org>, Andreas
 Hindborg <a.hindborg@...nel.org>, Trevor Gross <tmgross@...ch.edu>,
 linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org,
 dri-devel@...ts.freedesktop.org, linux-riscv@...ts.infradead.org,
 linux-pwm@...r.kernel.org, linux-clk@...r.kernel.org,
 rust-for-linux@...r.kernel.org
Subject: Re: [PATCH v3 1/3] rust: clk: use the type-state pattern

Hello Daniel,

On Wed, 07 Jan 2026 12:09:52 -0300
Daniel Almeida <daniel.almeida@...labora.com> wrote:

> -        /// Disable and unprepare the clock.
> +    impl Clk<Enabled> {
> +        /// Gets [`Clk`] corresponding to a bound [`Device`] and a connection id
> +        /// and then prepares and enables it.
>          ///
> -        /// Equivalent to calling [`Clk::disable`] followed by [`Clk::unprepare`].
> +        /// Equivalent to calling [`Clk::get`], followed by [`Clk::prepare`],
> +        /// followed by [`Clk::enable`].
>          #[inline]
> -        pub fn disable_unprepare(&self) {
> -            // SAFETY: By the type invariants, self.as_raw() is a valid argument for
> -            // [`clk_disable_unprepare`].
> -            unsafe { bindings::clk_disable_unprepare(self.as_raw()) };
> +        pub fn get(dev: &Device<Bound>, name: Option<&CStr>) -> Result<Clk<Enabled>> {
> +            Clk::<Prepared>::get(dev, name)?
> +                .enable()
> +                .map_err(|error| error.error)
> +        }
> +
> +        /// Behaves the same as [`Self::get`], except when there is no clock
> +        /// producer. In this case, instead of returning [`ENOENT`], it returns
> +        /// a dummy [`Clk`].
> +        #[inline]
> +        pub fn get_optional(dev: &Device<Bound>, name: Option<&CStr>) -> Result<Clk<Enabled>> {
> +            Clk::<Prepared>::get_optional(dev, name)?
> +                .enable()
> +                .map_err(|error| error.error)
> +        }
> +
> +        /// Attempts to disable the [`Clk`] and convert it to the [`Prepared`]
> +        /// state.
> +        #[inline]
> +        pub fn disable(self) -> Result<Clk<Prepared>, Error<Enabled>> {
> +            // We will be transferring the ownership of our `clk_get()` and
> +            // `clk_enable()` counts to `Clk<Prepared>`.
> +            let clk = ManuallyDrop::new(self);
> +
> +            // SAFETY: By the type invariants, `self.0` is a valid argument for
> +            // [`clk_disable`].
> +            unsafe { bindings::clk_disable(clk.as_raw()) };
> +
> +            Ok(Clk {
> +                inner: clk.inner,
> +                _phantom: PhantomData,
> +            })
>          }
>  
>          /// Get clock's rate.

Dunno if this has been mentioned already, but I belive the rate
getter/setter should be in the generic implementation. Indeed, it's
quite common for clock users to change the rate when the clk is
disabled to avoid unstable transitional state. The usual pattern for
that is:

	- clk_set_parent(my_clk, secondary_parent)
	- clk_disable[_unprepare](primary_parent) // (usually a PLL)
	- clk_set_rate(primary_parent)
	- clk[_prepare]_enable(primary_parent)
	- clk_set_parent(my_clk, primary_parent)

The case where the clk rate is changed while the clk is active is also
valid (usually fine when it's just a divider that's changed, because
there's no stabilization period).

> @@ -252,83 +429,31 @@ pub fn set_rate(&self, rate: Hertz) -> Result {
>          }
>      }