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] [day] [month] [year] [list]

Message-ID: <CANiq72=sU1sHvamC5REFPEC1aOVdZw9EKdxOgkUYESTR2yh3iQ@mail.gmail.com>
Date: Tue, 4 Mar 2025 10:37:50 +0100
From: Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
To: Viresh Kumar <viresh.kumar@...aro.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 <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 Tue, Mar 4, 2025 at 9:53 AM Viresh Kumar <viresh.kumar@...aro.org> wrote:
>
> I have tried some improvements based on your (and Alice's comments), please see
> if it looks any better now.

That looks much, much better, thanks!

> +/// Frequency unit.
> +#[derive(Copy, Clone, PartialEq, Eq, Debug)]
> +pub struct Hertz(c_ulong);

Please add a quick example for this one, e.g. constructing it and
comparing the value with an `assert_eq!` and another line comparing
two different `Hertz` objects for instance. After all, this one we can
even run it easily!

> +/// This structure represents the Rust abstraction for a C [`struct clk`].

Nit: the usual style is e.g.:

    /// An instance of a PHY device.
    ///
    /// Wraps the kernel's [`struct phy_device`].

i.e. the first line does not need to say "This structure" ... "Rust
abstraction" etc.

> +/// Instances of this type are reference-counted. Calling `get` ensures that the allocation remains

Please use intra-doc links (also for `OptionalClk` etc.).

> +/// ## Example

Nit: plural (even if there is a single example).

> +///     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?

> +    /// Enable the clock.
> +    #[inline]
> +    pub fn enable(&self) -> Result {

Should the users of these methods consult the C API side for the
comments/docs? e.g. should they read
https://docs.kernel.org/driver-api/clk.html#locking?

If so, please at least provide a link to the C API or the relevant
docs. e.g. https://docs.kernel.org/core-api/kernel-api.html#c.clk_enable.
Otherwise, if there is something there that should be mentioned here
directly, please do so.

In other words, in general, the goal is that you can find everything
you need in the Rust docs, even if those docs may sometimes rely on a
link there to the C side or a Doc/ document to avoid duplication. But
the information or the way to find that information should be there,
if that makes sense.

> +        // SAFETY: It is safe to call clk APIs of the C code for a clock pointer earlier returned
> +        // by `clk_get()`.

We should probably say why we know that, i.e. due to the invariant,
unless I am missing something.

By the way, in the constructor, you should add/use an `// INVARIANT:`
comment (please grep to see how others do it).

> +///     let expected_rate = Hertz::new(1_000_000_000);

Would it be useful for users to have constructors for a few SI
prefixes, e.g. `Hertz::from_giga`? I see some big constants used for
e.g. `set_rate` in the C side, so I guess it could.

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.

Thanks!

Cheers,
Miguel