Message-Id: <086CDFC4-A9EE-40C7-89BB-D3B8CBFA01EA@collabora.com>
Date: Wed, 30 Jul 2025 09:13:59 -0300
From: Daniel Almeida <daniel.almeida@...labora.com>
To: Danilo Krummrich <dakr@...nel.org>
Cc: 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 <lossin@...nel.org>,
 Andreas Hindborg <a.hindborg@...nel.org>,
 Alice Ryhl <aliceryhl@...gle.com>,
 Trevor Gross <tmgross@...ch.edu>,
 "Rafael J. Wysocki" <rafael@...nel.org>,
 Viresh Kumar <viresh.kumar@...aro.org>,
 Alexandre Courbot <acourbot@...dia.com>,
 linux-clk@...r.kernel.org,
 rust-for-linux@...r.kernel.org,
 linux-kernel@...r.kernel.org,
 linux-pm@...r.kernel.org
Subject: Re: [PATCH] rust: clk: use the type-state pattern



> On 30 Jul 2025, at 05:03, Danilo Krummrich <dakr@...nel.org> wrote:
> 
> On Tue Jul 29, 2025 at 11:38 PM CEST, Daniel Almeida wrote:
>> In light of the Regulator abstraction that was recently merged, switch this
>> abstraction to use the type-state pattern instead. It solves both a) and b)
>> by establishing a number of states and the valid ways to transition between
>> them. It also automatically undoes any call to clk_get(), clk_prepare() and
>> clk_enable() as applicable on drop(), so users do not have to do anything
>> special before Clk goes out of scope.
> 
> That's a great improvement, thanks! Some questions / comments below.
> 
>>     /// A reference-counted clock.
>>     ///
>>     /// Rust abstraction for the C [`struct clk`].
>>     ///
>> +    /// A [`Clk`] instance represents a clock that can be in one of several
>> +    /// states: [`Unprepared`], [`Prepared`], or [`Enabled`].
>> +    ///
>> +    /// No action needs to be taken when a [`Clk`] is dropped. The calls to
>> +    /// `clk_unprepare()` and `clk_disable()` will be placed as applicable.
>> +    ///
>> +    /// An optional [`Clk`] is treated just like a regular [`Clk`], but its
>> +    /// inner `struct clk` pointer is `NULL`. This interfaces correctly with the
>> +    /// C API and also exposes all the methods of a regular [`Clk`] to users.
>> +    ///
>>     /// # Invariants
>>     ///
>>     /// A [`Clk`] instance holds either a pointer to a valid [`struct clk`] created by the C
>> @@ -99,20 +160,39 @@ mod common_clk {
>>     /// Instances of this type are reference-counted. Calling [`Clk::get`] ensures that the
>>     /// allocation remains valid for the lifetime of the [`Clk`].
>>     ///
>> -    /// ## Examples
>> +    /// The [`Prepared`] state is associated with a single count of
>> +    /// `clk_prepare()`, and the [`Enabled`] state is associated with a single
>> +    /// count of `clk_enable()`, and the [`Enabled`] state is associated with a
>> +    /// single count of `clk_prepare` and `clk_enable()`.
>> +    ///
>> +    /// All states are associated with a single count of `clk_get()`.
>> +    ///
>> +    /// # Examples
>>     ///
>>     /// The following example demonstrates how to obtain and configure a clock for a device.
>>     ///
>>     /// ```
>>     /// use kernel::c_str;
>> -    /// use kernel::clk::{Clk, Hertz};
>> +    /// use kernel::clk::{Clk, Enabled, Hertz, Unprepared, Prepared};
>>     /// use kernel::device::Device;
>>     /// use kernel::error::Result;
>>     ///
>>     /// fn configure_clk(dev: &Device) -> Result {
>> -    ///     let clk = Clk::get(dev, Some(c_str!("apb_clk")))?;
>> +    ///     // The fastest way is to use a version of `Clk::get` for the desired
>> +    ///     // state, i.e.:
>> +    ///     let clk: Clk<Enabled> = Clk::<Enabled>::get(dev, Some(c_str!("apb_clk")))?;
> 
> Given that this is a driver API, why do we allow obtaining and configuring
> clocks of any device, i.e. also unbound devices?
> 
> I think Clk::<T>::get() should take a &Device<Bound> instead.

Ah, this was a question I had, but that I forgot to mention here.

The same can probably be said of the regulator series? i.e.:

impl Regulator<Disabled> {
    /// Obtains a [`Regulator`] instance from the system.
    pub fn get(dev: &Device, name: &CStr) -> Result<Self> {
        Regulator::get_internal(dev, name)
    }

> 
>> -    ///     clk.prepare_enable()?;
>> +    ///     // Any other state is also possible, e.g.:
>> +    ///     let clk: Clk<Prepared> = Clk::<Prepared>::get(dev, Some(c_str!("apb_clk")))?;
>> +    ///
>> +    ///     // Later:
>> +    ///     let clk: Clk<Enabled> = clk.enable().map_err(|error| {
>> +    ///         error.error
>> +    ///     })?;
>> +    ///
>> +    ///     // Note that error.clk is the original `clk` if the operation
>> +    ///     // failed. It is provided as a convenience so that the operation may be
>> +    ///     // retried in case of errors.
>>     ///
>>     ///     let expected_rate = Hertz::from_ghz(1);
>>     ///
>> @@ -120,104 +200,172 @@ mod common_clk {
>>     ///         clk.set_rate(expected_rate)?;
>>     ///     }
>>     ///
>> -    ///     clk.disable_unprepare();
>> +    ///     // Nothing is needed here. The drop implementation will undo any
>> +    ///     // operations as appropriate.
>> +    ///     Ok(())
>> +    /// }
>> +    ///
>> +    /// fn shutdown(dev: &Device, clk: Clk<Enabled>) -> Result {
> 
> You don't need the dev argument here.
> 
>> +    ///     // The states can be traversed "in the reverse order" as well:
>> +    ///     let clk: Clk<Prepared> = clk.disable().map_err(|error| {
>> +    ///         error.error
>> +    ///     })?;
>> +    ///
>> +    ///     let clk: Clk<Unprepared> = clk.unprepare();
> 
> I know you want to showcase the type state, yet I don't know if we should
> explicitly declare the type if not necessary. People will likely just copy
> things. Maybe a comment is better to emphasize it?

Ok

> 
>> +    ///
>>     ///     Ok(())
>>     /// }
>>     /// ```
>>     ///
>>     /// [`struct clk`]: https://docs.kernel.org/driver-api/clk.html
>>     #[repr(transparent)]
>> -    pub struct Clk(*mut bindings::clk);
>> +    pub struct Clk<T: ClkState> {
>> +        inner: *mut bindings::clk,
>> +        _phantom: core::marker::PhantomData<T>,
>> +    }
> 
> <snip>
> 
>> +    impl<T: ClkState> Drop for Clk<T> {
>> +        fn drop(&mut self) {
>> +            if T::DISABLE_ON_DROP {
>> +                // SAFETY: By the type invariants, self.as_raw() is a valid argument for
>> +                // [`clk_disable`].
>> +                unsafe { bindings::clk_disable(self.as_raw()) };
>> +            }
>> +
>> +            if T::UNPREPARE_ON_DROP {
>> +                // SAFETY: By the type invariants, self.as_raw() is a valid argument for
>> +                // [`clk_unprepare`].
>> +                unsafe { bindings::clk_unprepare(self.as_raw()) };
>> +            }
> 
> Nice! I like this cleanup. However, don't you still need to call clk_put() to
> drop the reference count?

Right, clk_put() was totally forgotten.

> 
> Also, given that this is a device resource, don't we want to take it away from
> drivers once the corresponding device has been unbound, i.e. use Devres?

Do you mean to have the get() functions return Devres<Clk>?

Also, is this applicable for Regulator as well?

> 
>>         }
>>     }
>> }
> 

— Daniel

Powered by blists - more mailing lists