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: <399B3E13-1ED1-49C2-B5E6-6B6FAA8019D5@collabora.com>
Date: Tue, 3 Feb 2026 10:35:22 -0300
From: Daniel Almeida <daniel.almeida@...labora.com>
To: Boris Brezillon <boris.brezillon@...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

Hi Boris,

> On 3 Feb 2026, at 06:17, Boris Brezillon <boris.brezillon@...labora.com> wrote:
> 
> 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 {
>>         }
>>     }
> 

I’m ok with this. I just assumed that these operations were only valid on enabled clks.

Will change this in the next version.

— Daniel