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 <[email protected]> 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/[email protected]?part=1
