Message-Id: <3133FE9C-473D-4142-AF38-CA447265C02E@collabora.com>
Date: Wed, 23 Jul 2025 09:36:32 -0300
From: Daniel Almeida <daniel.almeida@...labora.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,

I will be skipping the feedback that was given by others to not sound
repetitive.

> On 22 Jul 2025, at 18:14, Konrad Dybcio <konradybcio@...nel.org> wrote:
> 
> From: Konrad Dybcio <konrad.dybcio@....qualcomm.com>
> 
> Add abstractions for icc_path handling, laying the groundwork for more
> work on the subsystem.

For the sake of reviewers, I think you should add a brief overview of what this
code does and what you plan to use it for.

E.g.: can you write a sentence or two about the interconnect subsystem, for
example? We can then derive more information from the C documentation if
needed. Also, why do we need icc_path specifically? Why does it have to
come first? 


> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@....qualcomm.com>
> ---
> MAINTAINERS                     |   1 +
> rust/bindings/bindings_helper.h |   2 +
> rust/kernel/icc.rs              | 225 ++++++++++++++++++++++++++++++++++++++++
> rust/kernel/lib.rs              |   1 +
> 4 files changed, 229 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index ffb35359f1e2d4c286c5afef691f10421a3542a6..fbdbaa3c401d3705974f43bbd47e5a83632d33ef 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12735,6 +12735,7 @@ F: drivers/interconnect/
> F: include/dt-bindings/interconnect/
> F: include/linux/interconnect-provider.h
> F: include/linux/interconnect.h
> +F: rust/kernel/icc.rs
> 
> INTERRUPT COUNTER DRIVER
> M: Oleksij Rempel <o.rempel@...gutronix.de>
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index 84d60635e8a9baef1f1a1b2752dc0fa044f8542f..becfce3fa4794a51d817927376f77df7b8b0434d 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -53,6 +53,8 @@
> #include <linux/file.h>
> #include <linux/firmware.h>
> #include <linux/fs.h>
> +#include <linux/interconnect-provider.h>
> +#include <linux/interconnect.h>
> #include <linux/ioport.h>
> #include <linux/jiffies.h>
> #include <linux/jump_label.h>
> diff --git a/rust/kernel/icc.rs b/rust/kernel/icc.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..3674632866954613749e78bc24b8db6f1f3c0369
> --- /dev/null
> +++ b/rust/kernel/icc.rs
> @@ -0,0 +1,225 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +// Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.

Perhaps this instead [0].

> +
> +//! Interconnect abstractions
> +//!
> +//! (based on clk.rs)
> +//!
> +//! C headers:
> +//! [`include/linux/interconnect.h`](srctree/include/linux/interconnect.h)
> +//! [`include/linux/interconnect-provider.h`](srctree/include/linux/interconnect-provider.h)
> +//!
> +//! Reference: <https://docs.kernel.org/driver-api/interconnect.html>
> +
> +/// The interconnect framework bandidth unit.
> +///
> +/// Represents a bus bandwidth request in kBps, wrapping a [`u32`] value.
> +#[derive(Copy, Clone, PartialEq, Eq, Debug)]
> +pub struct IccBwUnit(pub u32);
> +
> +impl IccBwUnit {
> +    /// Create a new instance from bytes (B)
> +    pub const fn from_bytes_per_sec(bps: u32) -> Self {
> +        Self(bps / 1000)
> +    }
> +
> +    /// Create a new instance from kilobytes (kB) per second
> +    pub const fn from_kilobytes_per_sec(kbps: u32) -> Self {
> +        Self(kbps)
> +    }
> +
> +    /// Create a new instance from megabytes (MB) per second
> +    pub const fn from_megabytes_per_sec(mbps: u32) -> Self {
> +        Self(mbps * 1000)
> +    }
> +
> +    /// Create a new instance from gigabytes (GB) per second
> +    pub const fn from_gigabytes_per_sec(gbps: u32) -> Self {
> +        Self(gbps * 1000 * 1000)
> +    }
> +
> +    /// Create a new instance from bits (b) per second
> +    pub const fn from_bits_per_sec(_bps: u32) -> Self {
> +        Self(1)
> +    }
> +
> +    /// Create a new instance from kilobits (kb) per second
> +    pub const fn from_kilobits_per_sec(kbps: u32) -> Self {
> +        Self(kbps.div_ceil(8))
> +    }
> +
> +    /// Create a new instance from megabits (Mb) per second
> +    pub const fn from_megabits_per_sec(mbps: u32) -> Self {
> +        Self(mbps * 1000 / 8)
> +    }
> +
> +    /// Create a new instance from gigabits (Gb) per second
> +    pub const fn from_gigabits_per_sec(mbps: u32) -> Self {
> +        Self(mbps * 1000 * 1000 / 8)
> +    }
> +
> +    /// Get the bandwidth in bytes (B) per second
> +    pub const fn as_bytes_per_sec(self) -> u32 {
> +        self.0 * 1000
> +    }
> +
> +    /// Get the bandwidth in kilobytes (kB) per second
> +    pub const fn as_kilobytes_per_sec(self) -> u32 {
> +        self.0
> +    }
> +
> +    /// Get the bandwidth in megabytes (MB) per second
> +    pub const fn as_megabytes_per_sec(self) -> u32 {
> +        self.0 / 1000
> +    }
> +
> +    /// Get the bandwidth in gigabytes (GB) per second
> +    pub const fn as_gigabytes_per_sec(self) -> u32 {
> +        self.0 / 1000 / 1000
> +    }
> +
> +    /// Get the bandwidth in bits (b) per second
> +    pub const fn as_bits_per_sec(self) -> u32 {
> +        self.0 * 8 / 1000
> +    }
> +
> +    /// Get the bandwidth in kilobits (kb) per second
> +    pub const fn as_kilobits_per_sec(self) -> u32 {
> +        self.0 * 8
> +    }
> +
> +    /// Get the bandwidth in megabits (Mb) per second
> +    pub const fn as_megabits_per_sec(self) -> u32 {
> +        self.0 * 8 * 1000
> +    }
> +
> +    /// Get the bandwidth in gigabits (Gb) per second
> +    pub const fn as_gigabits_per_sec(self) -> u32 {
> +        self.0 * 8 * 1000 * 1000
> +    }
> +}
> +
> +impl From<IccBwUnit> for u32 {
> +    fn from(bw: IccBwUnit) -> Self {
> +        bw.0
> +    }
> +}
> +

Why is this under CONFIG_INTERCONNECT, but not the code above it?

> +#[cfg(CONFIG_INTERCONNECT)]
> +mod icc_path {
> +    use super::IccBwUnit;
> +    use crate::{
> +        device::Device,
> +        error::{Result, from_err_ptr, to_result},
> +        prelude::*,
> +    };
> +
> +    use core::ptr;
> +
> +    /// A reference-counted interconnect path.
> +    ///
> +    /// Rust abstraction for the C [`struct icc_path`]
> +    ///
> +    /// # Invariants
> +    ///
> +    /// An [`IccPath`] instance holds either a pointer to a valid [`struct icc_path`] created by
> +    /// the C portion of the kernel, or a NULL pointer.

Why should this ever be NULL? Can you expand on that in the docs? Otherwise
look into NonNull.

> +    ///
> +    /// Instances of this type are reference-counted. Calling [`IccPath::of_get`] ensures that the
> +    /// allocation remains valid for the lifetime of the [`IccPath`].

Should this implement AlwaysRefCounted?

> +    ///
> +    /// # Examples
> +    ///
> +    /// The following example demonstrates hwo to obtain and configure an interconnect path for
> +    /// a device.

Typo

> +    ///
> +    /// ```
> +    /// use kernel::icc_path::{IccPath, IccBwUnit};
> +    /// use kernel::device::Device;
> +    /// use kernel::error::Result;
> +    ///
> +    /// fn poke_at_bus(dev: &Device) -> Result {
> +    ///     let bus_path = IccPath::of_get(dev, Some(c_str!("bus")))?;
> +    ///
> +    ///     bus_path.set_bw(
> +    ///         IccBwUnit::from_megabits_per_sec(400),
> +    ///         IccBwUnit::from_megabits_per_sec(800)
> +    ///     )?;
> +    ///
> +    ///     // bus_path goes out of scope and self-disables if there are no other users
> +    ///
> +    ///     Ok(())
> +    /// }
> +    /// ```
> +    #[repr(transparent)]
> +    pub struct IccPath(*mut bindings::icc_path);
> +
> +    impl IccPath {
> +        /// Get [`IccPath`] corresponding to a [`Device`]
> +        ///
> +        /// Equivalent to the kernel's of_icc_get() API.

Please either use backticks or provide a link if appropriate.

> +        pub fn of_get(dev: &Device, name: Option<&CStr>) -> Result<Self> {
> +            let id = name.map_or(ptr::null(), |n| n.as_ptr());
> +
> +            // SAFETY: It's always safe to call [`of_icc_get`]
> +            //
> +            // INVARIANT: The reference count is decremented when [`IccPath`] goes out of scope
> +            Ok(Self(from_err_ptr(unsafe {
> +                bindings::of_icc_get(dev.as_raw(), id)
> +            })?))

There’s a lot going on here at once. Can you break this into multiple lines?

> +        }
> +
> +        /// Obtain the raw [`struct icc_path`] pointer.
> +        #[inline]
> +        pub fn as_raw(&self) -> *mut bindings::icc_path {
> +            self.0
> +        }

Why should this be needed in drivers?

> +
> +        /// Enable the path.
> +        ///
> +        /// Equivalent to the kernel's icc_enable() API.
> +        #[inline]
> +        pub fn enable(&self) -> Result {
> +            // SAFETY: By the type invariants, self.as_raw() is a valid argument for `icc_enable`].
> +            to_result(unsafe { bindings::icc_enable(self.as_raw()) })
> +        }

Should this take &mut? Same for all other functions below.

Note that using &mut may help you implement Sync later.

> +
> +        /// Disable the path.
> +        ///
> +        /// Equivalent to the kernel's icc_disable() API.

Same comment here. You need to ensure that this looks nice in the rendered docs.

> +        #[inline]
> +        pub fn disable(&self) -> Result {
> +            // SAFETY: By the type invariants, self.as_raw() is a valid argument for `icc_disable`].
> +            to_result(unsafe { bindings::icc_disable(self.as_raw()) })
> +        }
> +
> +        /// Set the bandwidth on a path
> +        ///
> +        /// Equivalent to the kernel's icc_set_bw() API.
> +        #[inline]
> +        pub fn set_bw(&self, avg_bw: IccBwUnit, peak_bw: IccBwUnit) -> Result {
> +            // SAFETY: By the type invariants, self.as_raw() is a valid argument for [`icc_set_bw`].
> +            to_result(unsafe {
> +                bindings::icc_set_bw(
> +                    self.as_raw(),
> +                    avg_bw.as_kilobytes_per_sec(),
> +                    peak_bw.as_kilobytes_per_sec(),
> +                )
> +            })
> +        }
> +    }
> +
> +    impl Drop for IccPath {
> +        fn drop(&mut self) {
> +            // SAFETY: By the type invariants, self.as_raw() is a valid argument for [`icc_put`].
> +            unsafe { bindings::icc_put(self.as_raw()) }
> +        }
> +    }
> +}
> +
> +// SAFETY: An `IccPath` is always reference-counted and can be released from any thread.
> +unsafe impl Send for IccPath {}
> +
> +#[cfg(CONFIG_INTERCONNECT)]
> +pub use icc_path::*;
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 87bcaa1c6b8a6291e71905e8dde60d945b654b98..60f6ac6e79cce57a8538ea0ad48f34f51af91731 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -89,6 +89,7 @@
> pub mod fmt;
> pub mod fs;
> pub mod init;
> +pub mod icc;
> pub mod io;
> pub mod ioctl;
> pub mod jump_label;
> 
> -- 
> 2.50.1
> 
> 

— Daniel

[0] https://spdx.github.io/spdx-spec/v3.0.1/model/Software/Properties/copyrightText/

Powered by blists - more mailing lists