Re: [PATCH 1/2] rust: Add initial interconnect framework abstractions

[Miguel Ojeda]

Hi Konrad,

Some quick mostly doc-related comments...

On Tue, Jul 22, 2025 at 11:14 PM Konrad Dybcio <konradybcio@xxxxxxxxxx> 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