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: <DC3B4FA6-5938-49CE-A75C-D354562593B3@collabora.com>
Date: Fri, 7 Mar 2025 23:03:15 -0300
From: Daniel Almeida <daniel.almeida@...labora.com>
To: Viresh Kumar <viresh.kumar@...aro.org>
Cc: Miguel Ojeda <miguel.ojeda.sandonis@...il.com>,
 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>
Subject: Re: [PATCH V3 2/2] rust: Add initial clk abstractions

Hi Viresh,

> On 4 Mar 2025, at 05:53, Viresh Kumar <viresh.kumar@...aro.org> wrote:
> 
> On 03-03-25, 11:16, Miguel Ojeda wrote:
>> On Mon, Mar 3, 2025 at 11:00 AM Viresh Kumar <viresh.kumar@...aro.org> wrote:
>>> 
>>> +/// Frequency unit.
>>> +pub type Hertz = crate::ffi::c_ulong;
>> 
>> Do we want this to be an alias or would it make sense to take the
>> chance to make this a newtype?
> 
> I have tried some improvements based on your (and Alice's comments), please see
> if it looks any better now.
> 
> -- 
> viresh
> 
> -------------------------8<-------------------------
> 
> diff --git a/rust/kernel/clk.rs b/rust/kernel/clk.rs
> new file mode 100644
> index 000000000000..fc3cb0f5f332
> --- /dev/null
> +++ b/rust/kernel/clk.rs
> @@ -0,0 +1,232 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Clock abstractions.
> +//!
> +//! C header: [`include/linux/clk.h`](srctree/include/linux/clk.h)
> +//!
> +//! Reference: <https://docs.kernel.org/driver-api/clk.html>
> +
> +use crate::{
> +    bindings,
> +    device::Device,
> +    error::{from_err_ptr, to_result, Result},
> +    ffi::c_ulong,
> +    prelude::*,
> +};
> +
> +use core::{ops::Deref, ptr};
> +
> +/// Frequency unit.
> +#[derive(Copy, Clone, PartialEq, Eq, Debug)]
> +pub struct Hertz(c_ulong);

Maybe make self.0 pub too?

> +
> +impl Hertz {
> +    /// Creates a new `Hertz` value.
> +    pub fn new(freq: c_ulong) -> Self {
> +        Hertz(freq)

I don’t think we need a `new` function.  IMHO, the only thing that matters is
that the name Hertz shows up in the calling code, i.e.:

```
fn foo() {
    let clk = …;
    let some_val = …;
    clk.set_rate(Hertz(some_val)); // Ok: crystal clear this is Hertz
}
```

A  impl From<Hertz> for c_ulong would also be helpful, so that we don’t have to
manually define all the arithmetic operations on this.

```
fn foo() {
    let clk = …;
    let double = u32::from(clk.rate()) * 2;
    clk.set_rate(Hertz(double)); // Ok: crystal clear this is Hertz
}
```

I need more time to look at the rest of the patch, so feel free to carry on with the
feedback from others. Sorry for the delay!

— Daniel