linux-kernel - Re: [PATCH] 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: <BEDA91F1-B6FB-4A28-8B34-4811CE683514@collabora.com>
Date: Wed, 30 Jul 2025 09:25:53 -0300
From: Daniel Almeida <daniel.almeida@...labora.com>
To: Daniel Sedlak <daniel@...lak.dev>
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>,
 Danilo Krummrich <dakr@...nel.org>,
 "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

Hi Daniel,

> On 30 Jul 2025, at 04:29, Daniel Sedlak <daniel@...lak.dev> wrote:
> 
> Hi Daniel,
> 
> On 7/29/25 11:38 PM, Daniel Almeida wrote:
>> +    mod private {
>> +        pub trait Sealed {}
>> +
>> +        impl Sealed for super::Unprepared {}
>> +        impl Sealed for super::Prepared {}
>> +        impl Sealed for super::Enabled {}
>> +    }
> 
> I just noticed we have plenty of Sealed traits scattered across rust/ folder. Do you think we would benefit from unifying it to a single location to prevent duplication?

Benno replied to this below.

> 
>> +
>> +    /// A trait representing the different states that a [`Clk`] can be in.
>> +    pub trait ClkState: private::Sealed {
>> +        /// Whether the clock should be disabled when dropped.
>> +        const DISABLE_ON_DROP: bool;
>> +
>> +        /// Whether the clock should be unprepared when dropped.
>> +        const UNPREPARE_ON_DROP: bool;
>> +    }
>> +
>> +    /// A state where the [`Clk`] is not prepared and not enabled.
>> +    pub struct Unprepared;
>> +
>> +    /// A state where the [`Clk`] is prepared but not enabled.
>> +    pub struct Prepared;
>> +
>> +    /// A state where the [`Clk`] is both prepared and enabled.
>> +    pub struct Enabled;
> 
> I would put a private member into the structs so the user of this API cannot construct it themself without using your abstractions.
> 
> pub struct Unprepared(());
> pub struct Prepared(());
> pub struct Enabled(());

I don’t think it’s a problem if they construct these. They can
construct e.g.: Unprepared, Prepared, etc, but not Clk<Unprepared>, or
Clk<Prepared> and others without going through the right API.

> 
>> +
>> +    impl ClkState for Unprepared {
>> +        const DISABLE_ON_DROP: bool = false;
>> +        const UNPREPARE_ON_DROP: bool = false;
>> +    }
>> +
>> +    impl ClkState for Prepared {
>> +        const DISABLE_ON_DROP: bool = false;
>> +        const UNPREPARE_ON_DROP: bool = true;
>> +    }
>> +
>> +    impl ClkState for Enabled {
>> +        const DISABLE_ON_DROP: bool = true;
>> +        const UNPREPARE_ON_DROP: bool = true;
>> +    }
>> +
>> +    /// An error that can occur when trying to convert a [`Clk`] between states.
>> +    pub struct Error<State: ClkState> {
> 
> Nit: IMO we mostly use the `where` variant instead of the colon.
> 
> pub struct Error<State>
> where State: ClkState
> 
> But does it make sense to put the bounds on the structs? Shouldn't be enough but but the bounds on the impl block?

This does have some implications in general, but in practical terms, I don’t think it
matters here.

Also, the bound on Clk<T> is somewhat warranted even though we could probably
get away with having it only on the impl block. It correctly encodes that a
Clk<T> should only _ever_ exist if T is one of the states.

From the above, it follows that Error<T> also needs the bound on the struct,
otherwise this will likely not compile.

> 
>> +        /// The error that occurred.
>> +        pub error: kernel::error::Error,
>> +
>> +        /// The [`Clk`] that caused the error, so that the operation may be
>> +        /// retried.
>> +        pub clk: Clk<State>,
>> +    }
>>  
> 
> Thanks!
> Daniel