linux-kernel - Re: [PATCH V3 2/2] rust: Add initial clk abstractions

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: <20250305114659.k5pptszvmusblynm@vireshk-i7>
Date: Wed, 5 Mar 2025 17:16:59 +0530
From: Viresh Kumar <viresh.kumar@...aro.org>
To: Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
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 <benno.lossin@...ton.me>,
	Andreas Hindborg <a.hindborg@...nel.org>,
	Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>,
	Russell King <linux@...linux.org.uk>, linux-clk@...r.kernel.org,
	linux-kernel@...r.kernel.org, rust-for-linux@...r.kernel.org,
	Vincent Guittot <vincent.guittot@...aro.org>,
	Daniel Almeida <daniel.almeida@...labora.com>
Subject: Re: [PATCH V3 2/2] rust: Add initial clk abstractions

On 04-03-25, 10:37, Miguel Ojeda wrote:
> On Tue, Mar 4, 2025 at 9:53 AM Viresh Kumar <viresh.kumar@...aro.org> wrote:
> > +///     clk.disable_unprepare();
> 
> Looking at the example, a question that one may have is: should we
> have something like a scope guard or a closure-passing API for this,
> or does it not make sense in general?

Something like this (untested) ?

+/// Runs a cleanup function/closure when dropped.
+///
+/// The [`ClkGuard::dismiss`] function prevents the cleanup function from running.
+///
+pub type ClkGuard<'a> = ScopeGuard<&'a Clk, fn(&Clk)>;
+
 /// A reference-counted clock.
 ///
 /// This represents the Rust abstraction for the C [`struct clk`].
@@ -139,10 +146,12 @@ pub fn as_raw(&self) -> *mut bindings::clk {
     ///
     /// [`clk_enable`]: https://docs.kernel.org/core-api/kernel-api.html#c.clk_enable
     #[inline]
-    pub fn enable(&self) -> Result {
+    pub fn enable(&self) -> Result<ClkGuard<'_>> {
         // SAFETY: By the type invariants, it is safe to call clk APIs of the C code for a clock
         // pointer earlier returned by [`clk_get`].
-        to_result(unsafe { bindings::clk_enable(self.as_raw()) })
+        to_result(unsafe { bindings::clk_enable(self.as_raw()) })?;
+
+        Ok(ClkGuard::new_with_data(self, Clk::disable))
     }
 
     /// Disable the clock.
@@ -163,10 +172,12 @@ pub fn disable(&self) {
     ///
     /// [`clk_prepare`]: https://docs.kernel.org/core-api/kernel-api.html#c.clk_prepare
     #[inline]
-    pub fn prepare(&self) -> Result {
+    pub fn prepare(&self) -> Result<ClkGuard<'_>> {
         // SAFETY: By the type invariants, it is safe to call clk APIs of the C code for a clock
         // pointer earlier returned by [`clk_get`].
-        to_result(unsafe { bindings::clk_prepare(self.as_raw()) })
+        to_result(unsafe { bindings::clk_prepare(self.as_raw()) })?;
+
+        Ok(ClkGuard::new_with_data(self, Clk::unprepare))
     }
 
     /// Unprepare the clock.
@@ -185,10 +196,12 @@ pub fn unprepare(&self) {
     ///
     /// Equivalent to calling [`Clk::prepare`] followed by [`Clk::enable`].
     #[inline]
-    pub fn prepare_enable(&self) -> Result {
+    pub fn prepare_enable(&self) -> Result<ClkGuard<'_>> {
         // SAFETY: By the type invariants, it is safe to call clk APIs of the C code for a clock
         // pointer earlier returned by [`clk_get`].
-        to_result(unsafe { bindings::clk_prepare_enable(self.as_raw()) })
+        to_result(unsafe { bindings::clk_prepare_enable(self.as_raw()) })?;
+
+        Ok(ClkGuard::new_with_data(self, Clk::disable_unprepare))
     }
 
> > +///     let expected_rate = Hertz::new(1_000_000_000);
> 
> On top of that, would any other kind of operation make sense? For
> instance, `.inverse()` to/from time or things like that -- we don't
> need to do any of this now, of course, but it may be worth taking a
> minute to investigate how we could improve the type now that we have
> it.

I am not sure if we should be implementing them right away, I agree
about from_hz/khz/mhz/ghz though. Let some users come for that and
then we can consider it ?

-- 
viresh