[PATCH 0/2] Add initial interconnect (icc_path) Rust abstractions

[PATCH 0/2] Add initial interconnect (icc_path) Rust abstractions

[Konrad Dybcio]

icc_path is in essence very similar to `struct clk`, so the newly
propsed bindings are understandably based on the corresponding
common_clk module.
This is the interconnect consumer part, with the corresponding ICC
provider changes coming in some near future.

I attached a sample driver making use of these, to ease any testing
or CI work (as the title says, please don't merge it though).

First contribution to kernel-rs, open to any and all suggestions.

Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
---
Konrad Dybcio (2):
      rust: Add initial interconnect framework abstractions
      [DNM] interconnect: Add a test Rust consumer driver

 MAINTAINERS                     |   1 +
 drivers/interconnect/Makefile   |   1 +
 drivers/interconnect/test.rs    |  47 +++++++++
 rust/bindings/bindings_helper.h |   2 +
 rust/kernel/icc.rs              | 225 ++++++++++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs              |   1 +
 6 files changed, 277 insertions(+)
---
base-commit: 05adbee3ad528100ab0285c15c91100e19e10138
change-id: 20250722-topic-icc_rs-fdbc135c57ae

Best regards,
-- 
Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

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

[Konrad Dybcio]

From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

Add abstractions for icc_path handling, laying the groundwork for more
work on the subsystem.

Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.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@pengutronix.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.
+
+//! 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
+    }
+}
+
+#[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.
+    ///
+    /// Instances of this type are reference-counted. Calling [`IccPath::of_get`] ensures that the
+    /// allocation remains valid for the lifetime of the [`IccPath`].
+    ///
+    /// # Examples
+    ///
+    /// The following example demonstrates hwo to obtain and configure an interconnect path for
+    /// a device.
+    ///
+    /// ```
+    /// 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.
+        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)
+            })?))
+        }
+
+        /// Obtain the raw [`struct icc_path`] pointer.
+        #[inline]
+        pub fn as_raw(&self) -> *mut bindings::icc_path {
+            self.0
+        }
+
+        /// 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()) })
+        }
+
+        /// Disable the path.
+        ///
+        /// Equivalent to the kernel's icc_disable() API.
+        #[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

[PATCH DNM 2/2] interconnect: Add a test Rust consumer driver

[Konrad Dybcio]

From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

Do not merge, this is for illustration / CI purposes only.

Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
---
 drivers/interconnect/Makefile |  1 +
 drivers/interconnect/test.rs  | 47 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+)

diff --git a/drivers/interconnect/Makefile b/drivers/interconnect/Makefile
index b0a9a6753b9dc30083781163ccc01dafcfcd0485..913b92080cc0b79846b74c239e14959b45b5450c 100644
--- a/drivers/interconnect/Makefile
+++ b/drivers/interconnect/Makefile
@@ -2,6 +2,7 @@
 
 CFLAGS_core.o				:= -I$(src)
 icc-core-objs				:= core.o bulk.o debugfs-client.o
+icc-core-$(CONFIG_RUST)			+= test.o
 
 obj-$(CONFIG_INTERCONNECT)		+= icc-core.o
 obj-$(CONFIG_INTERCONNECT_IMX)		+= imx/
diff --git a/drivers/interconnect/test.rs b/drivers/interconnect/test.rs
new file mode 100644
index 0000000000000000000000000000000000000000..f4ba2000d0f1fd2d91aedf8aace0b0b54bfd48f2
--- /dev/null
+++ b/drivers/interconnect/test.rs
@@ -0,0 +1,47 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
+
+//! Test interconnect consumer driver
+use kernel::{
+    c_str, device::Core, icc::*, module_platform_driver, of, of::DeviceId, platform, prelude::*,
+};
+
+#[pin_data]
+struct IccTestConsumerDriver {
+    #[pin]
+    path: IccPath,
+}
+
+kernel::of_device_table!(
+    OF_TABLE,
+    MODULE_OF_TABLE,
+    <IccTestConsumerDriver as platform::Driver>::IdInfo,
+    [(DeviceId::new(c_str!("linux,icc-consumer-test")), ())]
+);
+
+impl platform::Driver for IccTestConsumerDriver {
+    type IdInfo = ();
+    const OF_ID_TABLE: Option<of::IdTable<Self::IdInfo>> = Some(&OF_TABLE);
+
+    fn probe(
+        pdev: &platform::Device<Core>,
+        _id_info: Option<&Self::IdInfo>,
+    ) -> Result<Pin<KBox<Self>>> {
+        let path = IccPath::of_get(pdev.as_ref(), None)?;
+
+        path.set_bw(
+            IccBwUnit::from_megabits_per_sec(400),
+            IccBwUnit::from_megabits_per_sec(800),
+        )?;
+
+        Ok(KBox::pin_init(Self { path }, GFP_KERNEL)?.into())
+    }
+}
+
+module_platform_driver! {
+    type: IccTestConsumerDriver,
+    name: "icc-test-consumer",
+    authors: ["Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>"],
+    description: "Test interconnect consumer driver",
+    license: "GPL",
+}

-- 
2.50.1

Re: [PATCH 0/2] Add initial interconnect (icc_path) Rust abstractions

[Miguel Ojeda]

On Tue, Jul 22, 2025 at 11:14 PM Konrad Dybcio <konradybcio@kernel.org> wrote:
>
> icc_path is in essence very similar to `struct clk`, so the newly
> propsed bindings are understandably based on the corresponding
> common_clk module.
> This is the interconnect consumer part, with the corresponding ICC
> provider changes coming in some near future.
>
> I attached a sample driver making use of these, to ease any testing
> or CI work (as the title says, please don't merge it though).

Thanks!

The usual two main questions for new abstractions are whether the
maintainers of the C side want to see this happen (and how will it be
maintained etc.) and what users of the abstractions are expected
upstream.

For the first part, some subsystems prefer to maintain it themselves,
others prefer to have someone else lead a separate sub-entry in
`MAINTAINERS` (e.g. "... [RUST]"), possibly with its own branch too.

Cheers,
Miguel

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@kernel.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

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

[Daniel Sedlak]

Hi Konrad,

On 7/22/25 11:14 PM, Konrad Dybcio wrote:

> +//! 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);

IMO this is bit of an anti pattern for the newtype. Wouldn't be better 
to have the inner type private instead of public to keep the u32 as an 
implementation detail?

> +
> +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)
> +    }

This is a very shady API. If I were to call

	let bw = IccBwUnit::from_bits_per_sec(16);

I would expect.

	assert_eq!(bw.as_bytes_per_sec(), 2);

But it would fail (assuming that 8 bits = 1 byte), since 
IccBwUnit::from_bits_per_sec(), always assigns 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
> +    }
> +}
> +
> +#[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.
> +    ///
> +    /// Instances of this type are reference-counted. Calling [`IccPath::of_get`] ensures that the
> +    /// allocation remains valid for the lifetime of the [`IccPath`].
> +    ///
> +    /// # Examples
> +    ///
> +    /// The following example demonstrates hwo to obtain and configure an interconnect path for
> +    /// a device.
> +    ///
> +    /// ```
> +    /// 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.
> +        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)
> +            })?))
> +        }
> +
> +        /// Obtain the raw [`struct icc_path`] pointer.
> +        #[inline]
> +        pub fn as_raw(&self) -> *mut bindings::icc_path {
> +            self.0
> +        }
> +
> +        /// 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()) })
> +        }
> +
> +        /// Disable the path.
> +        ///
> +        /// Equivalent to the kernel's icc_disable() API.
> +        #[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;

Nit: formatting/missing space?

>   pub mod io;
>   pub mod ioctl;
>   pub mod jump_label;
> 

Thanks!
Daniel

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

[Konrad Dybcio]

On 7/23/25 12:42 PM, Miguel Ojeda wrote:
> Hi Konrad,
> 
> Some quick mostly doc-related comments...

[...]

>> +    /// 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`.

The C framework makes no effort to check for that, so panicking is at
least something.. That said, what would you suggest to do here?

[...]

>> +#[cfg(CONFIG_INTERCONNECT)]
>> +mod icc_path {
> 
> Maybe a different file?

I was debating that. icc_path represents the interconnect consumer part
(i.e. used in device drivers that just need to toggle a bus endpoint),
whereas the corresponding provider part (which manages said bus) is not
yet abstracted.

It would make logical sense to split these two.. with the latter going
to icc_provider.rs, perhaps?

[...]

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

No, it compiles fine here.. Strangely, I didn't get any warnings or 
errors with this patch. Maybe because the struct is pub and within the
same file?

Should I move it into the module scope for sanity?

> 
> 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.

I was missing this config, yeah..

> 
> We have some extra notes at:
> 
>     https://rust-for-linux.com/contributing#submit-checklist-addendum
> 
> on things that are useful to test/check.

I almost wanna say `make rustfmt` produced slightly different results
(one or two lines of difference) than make rust-analyzer + vscode
extension.. hmm.. Perhaps PEBKAC..

Konrad

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

[kernel test robot]

Hi Konrad,

kernel test robot noticed the following build errors:

[auto build test ERROR on 05adbee3ad528100ab0285c15c91100e19e10138]

url:    https://github.com/intel-lab-lkp/linux/commits/Konrad-Dybcio/rust-Add-initial-interconnect-framework-abstractions/20250723-051723
base:   05adbee3ad528100ab0285c15c91100e19e10138
patch link:    https://lore.kernel.org/r/20250722-topic-icc_rs-v1-1-9da731c14603%40oss.qualcomm.com
patch subject: [PATCH 1/2] rust: Add initial interconnect framework abstractions
config: x86_64-rhel-9.4-rust (https://download.01.org/0day-ci/archive/20250723/202507231918.JGWa9fgH-lkp@intel.com/config)
compiler: clang version 20.1.8 (https://github.com/llvm/llvm-project 87f0227cb60147a26a1eeb4fb06e3b505e9c7261)
rustc: rustc 1.88.0 (6b00bc388 2025-06-23)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250723/202507231918.JGWa9fgH-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202507231918.JGWa9fgH-lkp@intel.com/

All errors (new ones prefixed by >>):

>> error[E0412]: cannot find type `IccPath` in this scope
   --> rust/kernel/icc.rs:222:22
   |
   222 | unsafe impl Send for IccPath {}
   |                      ^^^^^^^ not found in this scope

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[Konrad Dybcio]

On 7/23/25 12:44 PM, Daniel Sedlak wrote:
> Hi Konrad,

[...]

>> +    /// Create a new instance from bits (b) per second
>> +    pub const fn from_bits_per_sec(_bps: u32) -> Self {
>> +        Self(1)
>> +    }
> 
> This is a very shady API. If I were to call
> 
>     let bw = IccBwUnit::from_bits_per_sec(16);
> 
> I would expect.
> 
>     assert_eq!(bw.as_bytes_per_sec(), 2);
> 
> But it would fail (assuming that 8 bits = 1 byte), since IccBwUnit::from_bits_per_sec(), always assigns 1.

I followed the C api:

include/linux/interconnect.h
18:#define bps_to_icc(x)        (1)

But indeed, something like bw.div_ceil(8) makes more logical sense


>> 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;
> 
> Nit: formatting/missing space?

No, it's actually fine.. git decided that context lines shall begin
with a space

Konrad

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

[Miguel Ojeda]

On Wed, Jul 23, 2025 at 1:32 PM Konrad Dybcio
<konrad.dybcio@oss.qualcomm.com> wrote:
>
> The C framework makes no effort to check for that, so panicking is at
> least something.. That said, what would you suggest to do here?

If you want to mimic the C side, then you will need to use one of the
non-panicking operations, such as e.g. `wrapping_mul()`.

Otherwise, you could make it a fallible method, i.e. return `Result`.

Otherwise, I think the panic should be documented in the docs of the
methods (because callers then really need to be careful).

Which option to takes depends a bit on the use case and what C
maintainers consider best for a particular operation.

For instance, sometimes people have used `build_assert!` because they
expect that the value is always known at compile-time (after
optimizations).

> I was debating that. icc_path represents the interconnect consumer part
> (i.e. used in device drivers that just need to toggle a bus endpoint),
> whereas the corresponding provider part (which manages said bus) is not
> yet abstracted.
>
> It would make logical sense to split these two.. with the latter going
> to icc_provider.rs, perhaps?

Ah, so I just meant that you could have the `icc_path` as a `mod
icc_path;`, and move it to its own file, rather than inline. Other
reorganizations makes sense, but I was only suggesting that :)

> No, it compiles fine here.. Strangely, I didn't get any warnings or
> errors with this patch. Maybe because the struct is pub and within the
> same file?

It likely happens if `CONFIG_INTERCONNECT` is not set, because then
the entire module above is gone.

> I almost wanna say `make rustfmt` produced slightly different results
> (one or two lines of difference) than make rust-analyzer + vscode
> extension.. hmm.. Perhaps PEBKAC..

In mainline we currently enforce that code is formatted with `make ...
rustfmt` (there is `make ... rustfmtcheck` to check, too), so if some
extension gives you different formatting, please double-check before
submitting the code.

Cheers,
Miguel

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

[Miguel Ojeda]

On Wed, Jul 23, 2025 at 1:41 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> reorganizations makes sense, but I was only suggesting that :)

s/makes/may make/

i.e. I didn't mean to suggest anything deeper than that (how to
organize is really up to you and the C maintainers etc.).

I hope that helps!

Cheers,
Miguel

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

[Konrad Dybcio]

On 7/23/25 1:41 PM, Miguel Ojeda wrote:
> On Wed, Jul 23, 2025 at 1:32 PM Konrad Dybcio
> <konrad.dybcio@oss.qualcomm.com> wrote:
>>
>> The C framework makes no effort to check for that, so panicking is at
>> least something.. That said, what would you suggest to do here?
> 
> If you want to mimic the C side, then you will need to use one of the
> non-panicking operations, such as e.g. `wrapping_mul()`.
> 
> Otherwise, you could make it a fallible method, i.e. return `Result`.
> 
> Otherwise, I think the panic should be documented in the docs of the
> methods (because callers then really need to be careful).
> 
> Which option to takes depends a bit on the use case and what C
> maintainers consider best for a particular operation.
> 
> For instance, sometimes people have used `build_assert!` because they
> expect that the value is always known at compile-time (after
> optimizations).

build_assert sounds nice, but the value is often retrieved dynamically
(e.g. from the device tree itself). Result seems like the best option.
I'll also add a verse in the docs about the return variants.

> 
>> I was debating that. icc_path represents the interconnect consumer part
>> (i.e. used in device drivers that just need to toggle a bus endpoint),
>> whereas the corresponding provider part (which manages said bus) is not
>> yet abstracted.
>>
>> It would make logical sense to split these two.. with the latter going
>> to icc_provider.rs, perhaps?
> 
> Ah, so I just meant that you could have the `icc_path` as a `mod
> icc_path;`, and move it to its own file, rather than inline. Other
> reorganizations makes sense, but I was only suggesting that :)

So, IccBwUnit stays in icc.rs and icc_path -> icc_path.rs?

> 
>> No, it compiles fine here.. Strangely, I didn't get any warnings or
>> errors with this patch. Maybe because the struct is pub and within the
>> same file?
> 
> It likely happens if `CONFIG_INTERCONNECT` is not set, because then
> the entire module above is gone.

OK that makes sense!

> 
>> I almost wanna say `make rustfmt` produced slightly different results
>> (one or two lines of difference) than make rust-analyzer + vscode
>> extension.. hmm.. Perhaps PEBKAC..
> 
> In mainline we currently enforce that code is formatted with `make ...
> rustfmt` (there is `make ... rustfmtcheck` to check, too), so if some
> extension gives you different formatting, please double-check before
> submitting the code.

I assumed that since the extension name is the same as the make target,
they were meant to be ;)

Konrad

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

[Daniel Almeida]

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@kernel.org> wrote:
> 
> From: Konrad Dybcio <konrad.dybcio@oss.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@oss.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@pengutronix.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/

Re: [PATCH DNM 2/2] interconnect: Add a test Rust consumer driver

[Daniel Almeida]

Hi Konrad, commenting in case this driver goes forward.

> On 22 Jul 2025, at 18:14, Konrad Dybcio <konradybcio@kernel.org> wrote:
> 
> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> 
> Do not merge, this is for illustration / CI purposes only.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> ---
> drivers/interconnect/Makefile |  1 +
> drivers/interconnect/test.rs  | 47 +++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 48 insertions(+)
> 
> diff --git a/drivers/interconnect/Makefile b/drivers/interconnect/Makefile
> index b0a9a6753b9dc30083781163ccc01dafcfcd0485..913b92080cc0b79846b74c239e14959b45b5450c 100644
> --- a/drivers/interconnect/Makefile
> +++ b/drivers/interconnect/Makefile
> @@ -2,6 +2,7 @@
> 
> CFLAGS_core.o := -I$(src)
> icc-core-objs := core.o bulk.o debugfs-client.o
> +icc-core-$(CONFIG_RUST) += test.o
> 
> obj-$(CONFIG_INTERCONNECT) += icc-core.o
> obj-$(CONFIG_INTERCONNECT_IMX) += imx/
> diff --git a/drivers/interconnect/test.rs b/drivers/interconnect/test.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..f4ba2000d0f1fd2d91aedf8aace0b0b54bfd48f2
> --- /dev/null
> +++ b/drivers/interconnect/test.rs
> @@ -0,0 +1,47 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
> +
> +//! Test interconnect consumer driver
> +use kernel::{
> +    c_str, device::Core, icc::*, module_platform_driver, of, of::DeviceId, platform, prelude::*,
> +};
> +
> +#[pin_data]
> +struct IccTestConsumerDriver {
> +    #[pin]
> +    path: IccPath,
> +}

I don’t think this does anything useful without PhantomPinned, but Benno is
the right person to chime in here.

More importantly though, why do you have #[pin] on IccPath?

> +
> +kernel::of_device_table!(
> +    OF_TABLE,
> +    MODULE_OF_TABLE,
> +    <IccTestConsumerDriver as platform::Driver>::IdInfo,
> +    [(DeviceId::new(c_str!("linux,icc-consumer-test")), ())]
> +);
> +
> +impl platform::Driver for IccTestConsumerDriver {
> +    type IdInfo = ();
> +    const OF_ID_TABLE: Option<of::IdTable<Self::IdInfo>> = Some(&OF_TABLE);
> +
> +    fn probe(
> +        pdev: &platform::Device<Core>,
> +        _id_info: Option<&Self::IdInfo>,
> +    ) -> Result<Pin<KBox<Self>>> {
> +        let path = IccPath::of_get(pdev.as_ref(), None)?;
> +
> +        path.set_bw(
> +            IccBwUnit::from_megabits_per_sec(400),
> +            IccBwUnit::from_megabits_per_sec(800),
> +        )?;
> +
> +        Ok(KBox::pin_init(Self { path }, GFP_KERNEL)?.into())
> +    }
> +}
> +
> +module_platform_driver! {
> +    type: IccTestConsumerDriver,
> +    name: "icc-test-consumer",
> +    authors: ["Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>"],
> +    description: "Test interconnect consumer driver",
> +    license: "GPL",
> +}
> 
> -- 
> 2.50.1
> 
> 

— Daniel

Re: [PATCH DNM 2/2] interconnect: Add a test Rust consumer driver

[Benno Lossin]

On Wed Jul 23, 2025 at 3:10 PM CEST, Daniel Almeida wrote:
> On 22 Jul 2025, at 18:14, Konrad Dybcio <konradybcio@kernel.org> wrote:
>> +#[pin_data]
>> +struct IccTestConsumerDriver {
>> +    #[pin]
>> +    path: IccPath,
>> +}
>
> I don’t think this does anything useful without PhantomPinned, but Benno is
> the right person to chime in here.

It does do something useful, there just has to be one type marked with
`#[pin]` that is `!Unpin` (so for example `PhantomPinned`, `Opaque<T>`
etc.).

In this case however, `IccPath` is a newtype of `*mut bindings::icc_path`
which isn't `PhantomPinned`, so this doesn't ensure that the
`IccTestConsumerDriver` will stay pinned after initializing.

> More importantly though, why do you have #[pin] on IccPath?

Another question is: why is `IccPath` not a newtype of
`Opaque<bindings::icc_path>`? And then one can use `&IccPath`.

---
Cheers,
Benno

Re: [PATCH 0/2] Add initial interconnect (icc_path) Rust abstractions

[Konrad Dybcio]

On 7/23/25 12:22 PM, Miguel Ojeda wrote:
> On Tue, Jul 22, 2025 at 11:14 PM Konrad Dybcio <konradybcio@kernel.org> wrote:
>>
>> icc_path is in essence very similar to `struct clk`, so the newly
>> propsed bindings are understandably based on the corresponding
>> common_clk module.
>> This is the interconnect consumer part, with the corresponding ICC
>> provider changes coming in some near future.
>>
>> I attached a sample driver making use of these, to ease any testing
>> or CI work (as the title says, please don't merge it though).
> 
> Thanks!
> 
> The usual two main questions for new abstractions are whether the
> maintainers of the C side want to see this happen (and how will it be
> maintained etc.) and what users of the abstractions are expected
> upstream.

I haven't talked to Georgi about this. I can volunteer for
code-janitoring, but as you can tell I'll still need your oversight

Regarding the users, I don't have any specific promises on a consumer
of these abstractions in a short term, although the ICC API is rather
common (especially across the major arm-based SoCs), so it shouldn't be
long before someone needs it.

Konrad

> 
> For the first part, some subsystems prefer to maintain it themselves,
> others prefer to have someone else lead a separate sub-entry in
> `MAINTAINERS` (e.g. "... [RUST]"), possibly with its own branch too.
> 
> Cheers,
> Miguel

Re: [PATCH 0/2] Add initial interconnect (icc_path) Rust abstractions

[Miguel Ojeda]

On Thu, Jul 24, 2025 at 2:36 PM Konrad Dybcio
<konrad.dybcio@oss.qualcomm.com> wrote:
>
> Regarding the users, I don't have any specific promises on a consumer
> of these abstractions in a short term, although the ICC API is rather
> common (especially across the major arm-based SoCs), so it shouldn't be
> long before someone needs it.

In general, abstractions cannot be added unless there is a known user
that is coming upstream (or developed in-tree over time, like Nova and
Tyr).

There is also the "Rust reference driver" approach/exception to help C
maintainers bootstrap the Rust side, which you may be able to take
advantage of:

    https://rust-for-linux.com/rust-reference-drivers

At the end of the day, it depends on the particular case and the
maintainers, of course.

I hope that helps.

Cheers,
Miguel