linux-kernel - Re: [PATCH 1/2] rust: Add initial interconnect framework 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: <CANiq72nPLn+3V_DhN9_dmKnRrb5mfjzQ67Utz7HdtOY3McpweA@mail.gmail.com>
Date: Wed, 23 Jul 2025 12:42:27 +0200
From: Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
To: Konrad Dybcio <konradybcio@...nel.org>
Cc: 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 <lossin@...nel.org>, Andreas Hindborg <a.hindborg@...nel.org>, 
	Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>, 
	Danilo Krummrich <dakr@...nel.org>, Georgi Djakov <djakov@...nel.org>, 
	Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>, 
	Bjorn Andersson <bjorn.andersson@....qualcomm.com>, 
	Marijn Suijten <marijn.suijten@...ainline.org>, linux-kernel@...r.kernel.org, 
	rust-for-linux@...r.kernel.org, linux-pm@...r.kernel.org, 
	Konrad Dybcio <konrad.dybcio@....qualcomm.com>
Subject: Re: [PATCH 1/2] rust: Add initial interconnect framework abstractions

Hi Konrad,

Some quick mostly doc-related comments...

On Tue, Jul 22, 2025 at 11:14 PM Konrad Dybcio <konradybcio@...nel.org> wrote:
>
> +//! Interconnect abstractions

Please follow the usual style, i.e. ending sentences with a period.

> +//! (based on clk.rs)

Is there a reason to mention this in the documentation? If not, I
would probably mention it in the commit message instead.

> +//! C headers:
> +//! [`include/linux/interconnect.h`](srctree/include/linux/interconnect.h)
> +//! [`include/linux/interconnect-provider.h`](srctree/include/linux/interconnect-provider.h)

Please see if this looks as expected when rendered -- you may want an
" and " or a comma or similar.

> +/// The interconnect framework bandidth unit.

Typo.

> +/// Represents a bus bandwidth request in kBps, wrapping a [`u32`] value.
> +#[derive(Copy, Clone, PartialEq, Eq, Debug)]
> +pub struct IccBwUnit(pub u32);

Since there are accessors below, do the internal details need to be public?

> +    /// Create a new instance from gigabytes (GB) per second
> +    pub const fn from_gigabytes_per_sec(gbps: u32) -> Self {
> +        Self(gbps * 1000 * 1000)
> +    }

I guess this means callers must call this with reasonable numbers and
otherwise it is considered a bug, right? i.e. this could overflow, and
thus panic under `CONFIG_RUST_OVERFLOW_CHECKS=y`.

> +impl From<IccBwUnit> for u32 {
> +    fn from(bw: IccBwUnit) -> Self {
> +        bw.0
> +    }
> +}

Is this needed since there are more explicit accessors?

> +#[cfg(CONFIG_INTERCONNECT)]
> +mod icc_path {

Maybe a different file?

> +    /// Rust abstraction for the C [`struct icc_path`]

This intra-doc link is probably broken, since it refers to C --
normally you need an explicit link for this. Please check the docs via
`make .... rustdoc`.

> +    /// The following example demonstrates hwo to obtain and configure an interconnect path for

Typo.

> +    ///     // bus_path goes out of scope and self-disables if there are no other users

Please follow the usual formatting for comments, i.e. Markdown and
ending with a period.

> +            // SAFETY: It's always safe to call [`of_icc_get`]

Comments don't need intra-doc links, since they do not get rendered
(sadly -- a long-term wish of mine is having `rustdoc` link those in
the source view and thus checked too).

> +            // SAFETY: By the type invariants, self.as_raw() is a valid argument for `icc_enable`].

That seems like half of an intra-doc link :)

> +// SAFETY: An `IccPath` is always reference-counted and can be released from any thread.
> +unsafe impl Send for IccPath {}

This gives an error, right? Was it meant to be inside the other Rust module?

Also, please also run `make .... rustfmt`.

Finally, the examples in the docs are converted automatically into
KUnit tests (under `CONFIG_RUST_KERNEL_DOCTESTS=y`) -- the examples
currently have build errors.

We have some extra notes at:

    https://rust-for-linux.com/contributing#submit-checklist-addendum

on things that are useful to test/check.

I hope that helps!

Cheers,
Miguel