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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The clock rate configuration methods (`rate()` and `set_rate()`) have been restricted exclusively to `Clk<Enabled>`, breaking support for clocks with `CLK_SET_RATE_GATE` and preventing pre-enablement configuration.
- [Low] Missing `// INVARIANT:` comments when constructing `Clk` instances during state transitions, and safety comments incorrectly refer to a non-existent `self.0` field.
- [Low] `Clk::<Enabled>::disable()` incorrectly returns a `Result` despite being infallible, while `unprepare()` correctly returns the struct directly.
- [Low] Typo in the `# Invariants` documentation section regarding the `Prepared` and `Enabled` states.
--

commit c6dd3e4cdd99489c0a4f27adce8b4f6ceef42c51
Author: Daniel Almeida <daniel.almeida@collabora.com>

rust: clk: use the type-state pattern

This commit switches the clock abstraction to use the type-state pattern
to enforce valid state transitions and track reference counts automatically.

Link: https://crates.io/crates/sealed [1]

> diff --git a/rust/kernel/clk.rs b/rust/kernel/clk.rs
> index 7abbd0767d8cf..a62e4c7e252e3 100644
> --- a/rust/kernel/clk.rs
> +++ b/rust/kernel/clk.rs

[ ... ]

> +    /// 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 [`Prepared`] state is associated with a
> +    /// single count of `clk_prepare()` and `clk_enable()`.

[Severity: Low]
Is the last part of this sentence a typo? It attributes the dual count
requirement to the Prepared state again instead of the Enabled state.

[ ... ]

> +        pub fn prepare(self) -> Result<Clk<Prepared>, Error<Unprepared>> {
> +            // We will be transferring the ownership of our `clk_get()` count to
> +            // `Clk<Prepared>`.
> +            let clk = ManuallyDrop::new(self);
> +
> +            // SAFETY: By the type invariants, `self.0` is a valid argument for
> +            // [`clk_prepare`].
> +            to_result(unsafe { bindings::clk_prepare(clk.as_raw()) })
> +                .map(|()| Clk {
> +                    inner: clk.inner,
> +                    _phantom: PhantomData,
> +                })

[Severity: Low]
Should there be an '// INVARIANT:' comment when constructing this new Clk,
similar to the initialization in get_unbound()? 

Also, does the safety comment reference 'self.0' which no longer exists 
after the struct was refactored to use named fields? The same stale reference
and missing invariant comment appears in unprepare(), enable(), and disable().

[ ... ]

> +        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,
> +            })
> +        }

[Severity: Low]
Since clk_disable() is infallible, does this method need to return a Result? 
The unprepare() method returns the struct directly, which avoids unnecessary
error handling for the caller.

[ ... ]

> +    impl Clk<Enabled> {
[ ... ]
>          pub fn set_rate(&self, rate: Hertz) -> Result {
>              // SAFETY: By the type invariants, self.as_raw() is a valid argument for
>              // [`clk_set_rate`].
>              to_result(unsafe { bindings::clk_set_rate(self.as_raw(), rate.as_hz()) })
>          }
>      }

[Severity: High]
Does restricting set_rate() and rate() to the Enabled state prevent configuring
clocks that have the CLK_SET_RATE_GATE flag? 

The C API requires clocks with this flag to be disabled before changing their 
rate. Additionally, is it still possible to safely configure a clock's rate 
before enabling it, which is often required to avoid running hardware at 
incorrect frequencies?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260618-clk-type-state-v4-0-8be082786080@collabora.com?part=1