[PATCH V8 00/14] Rust bindings for cpufreq and OPP core + sample driver

[PATCH V8 00/14] Rust bindings for cpufreq and OPP core + sample driver

[Viresh Kumar]

Hello,

I am seeking a few Acks for this patch series before merging it into the PM tree
for the 6.15 merge window, unless there are any objections.

This series introduces initial Rust bindings for two subsystems: cpufreq and
Operating Performance Points (OPP). The bindings cover most of the interfaces
exposed by these subsystems. It also includes minimal bindings for the clk and
cpumask frameworks, which are required by the cpufreq bindings.

Additionally, a sample cpufreq driver, rcpufreq-dt, is included. This is a
duplicate of the existing cpufreq-dt driver, which is a platform-agnostic,
device-tree-based driver commonly used on ARM platforms.

The implementation has been tested using QEMU, ensuring that frequency
transitions, various configurations, and driver binding/unbinding work as
expected. However, performance measurements have not been conducted yet.

For those interested in testing these patches, they can be found at:

git://git.kernel.org/pub/scm/linux/kernel/git/vireshk/linux.git rust/cpufreq-dt

This version is rebased on v6.14-rc1.

--
Viresh

V7->V8:
- Updated cpumask bindings to work with !CONFIG_CPUMASK_OFFSTACK case.
- Dropped few patches (property_present() and opp helpers), as they are already
  merged.
- from_cpu() is marked unsafe.
- Included a patch by Anisse Astier, to solve a long standing issue with this
  series.
- Dropped: "DO-NOT_MERGE: cpufreq: Rename cpufreq-dt platdev."
- Updated MAINTAINERS for new files.
- Other minor changes / cleanups.

V6->V7:
- from_cpu() is moved to cpu.rs and doesn't return ARef anymore, but just a
  reference.
- Dropped cpufreq_table_len() and related validation in cpufreq core.
- Solved the issue with BIT() macro differently, using an enum now.
- Few patches are broken into smaller / independent patches.
- Improved Commit logs and SAFETY comments at few places.
- Removed print message from cpufreq driver.
- Rebased over linux-next/master.
- Few other minor changes.

V5->V6:
- Rebase over latest rust/dev branch, which changed few interfaces that the
  patches were using.
- Included all other patches, which weren't included until now to focus only on
  core APIs.
- Other minor cleanups, additions.

V4->V5:
- Rename Registration::register() as new().
- Provide a new API: Registration::new_foreign_owned() and use it for
  rcpufreq_dt driver.
- Update MAINTAINERS file.

V3->V4:
- Fix bugs with freeing of OPP structure. Dropped the Drop routine and fixed
  reference counting.
- Registration object of the cpufreq core is modified a bit to remove the
  registered field, and few other cleanups.
- Use Devres for instead of platform data.
- Improve SAFETY comments.

V2->V3:
- Rebased on latest rust-device changes, which removed `Data` and so few changes
  were required to make it work.
- use srctree links (Alice Ryhl).
- Various changes the OPP creation APIs, new APIs: from_raw_opp() and
  from_raw_opp_owned() (Alice Ryhl).
- Inline as_raw() helpers (Alice Ryhl).
- Add new interface (`OPP::Token`) for dynamically created OPPs.
- Add Reviewed-by tag from Manos.
- Modified/simplified cpufreq registration structure / method a bit.

V1->V2:
- Create and use separate bindings for OF, clk, cpumask, etc (not included in
  this patchset but pushed to the above branch). This helped removing direct
  calls from the driver.
- Fix wrong usage of Pinning + Vec.
- Use Token for OPP Config.
- Use Opaque, transparent and Aref for few structures.
- Broken down into smaller patches to make it easy for reviewers.
- Based over staging/rust-device.

Thanks.

Anisse Astier (1):
  rust: macros: enable use of hyphens in module names

Viresh Kumar (13):
  cpufreq: Use enum for cpufreq flags that use BIT()
  rust: cpu: Add from_cpu()
  rust: Add cpumask helpers
  rust: Add bindings for cpumask
  rust: Add bare minimal bindings for clk framework
  rust: Add initial bindings for OPP framework
  rust: Extend OPP bindings for the OPP table
  rust: Extend OPP bindings for the configuration options
  rust: Add initial bindings for cpufreq framework
  rust: Extend cpufreq bindings for policy and driver ops
  rust: Extend cpufreq bindings for driver registration
  rust: Extend OPP bindings with CPU frequency table
  cpufreq: Add Rust based cpufreq-dt driver

 MAINTAINERS                     |    6 +
 drivers/cpufreq/Kconfig         |   12 +
 drivers/cpufreq/Makefile        |    1 +
 drivers/cpufreq/rcpufreq_dt.rs  |  238 +++++++
 include/linux/cpufreq.h         |   96 +--
 rust/bindings/bindings_helper.h |    5 +
 rust/helpers/cpufreq.c          |   10 +
 rust/helpers/cpumask.c          |   40 ++
 rust/helpers/helpers.c          |    2 +
 rust/kernel/clk.rs              |   48 ++
 rust/kernel/cpu.rs              |   31 +
 rust/kernel/cpufreq.rs          | 1054 +++++++++++++++++++++++++++++++
 rust/kernel/cpumask.rs          |  138 ++++
 rust/kernel/lib.rs              |    8 +
 rust/kernel/opp.rs              |  889 ++++++++++++++++++++++++++
 rust/macros/module.rs           |   17 +-
 16 files changed, 2543 insertions(+), 52 deletions(-)
 create mode 100644 drivers/cpufreq/rcpufreq_dt.rs
 create mode 100644 rust/helpers/cpufreq.c
 create mode 100644 rust/helpers/cpumask.c
 create mode 100644 rust/kernel/clk.rs
 create mode 100644 rust/kernel/cpu.rs
 create mode 100644 rust/kernel/cpufreq.rs
 create mode 100644 rust/kernel/cpumask.rs
 create mode 100644 rust/kernel/opp.rs

-- 
2.31.1.272.g89b43f80a514

[PATCH V8 01/14] rust: macros: enable use of hyphens in module names

[Viresh Kumar]

From: Anisse Astier <anisse@astier.eu>

Some modules might need naming that contains hyphens "-" to match the
auto-probing by name in the platform devices that comes from the device
tree.

But rust identifiers cannot contain hyphens, so replace the module name
by an underscore anywhere we'd use it as an identifier.

Signed-off-by: Anisse Astier <anisse@astier.eu>
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
[Viresh: Replace "-" with '-']
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 rust/macros/module.rs | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/rust/macros/module.rs b/rust/macros/module.rs
index cdf94f4982df..2e740bbdb598 100644
--- a/rust/macros/module.rs
+++ b/rust/macros/module.rs
@@ -182,7 +182,9 @@ pub(crate) fn module(ts: TokenStream) -> TokenStream {
 
     let info = ModuleInfo::parse(&mut it);
 
-    let mut modinfo = ModInfoBuilder::new(info.name.as_ref());
+    /* Rust does not allow hyphens in identifiers, use underscore instead */
+    let name_identifier = info.name.replace('-', "_");
+    let mut modinfo = ModInfoBuilder::new(name_identifier.as_ref());
     if let Some(author) = info.author {
         modinfo.emit("author", &author);
     }
@@ -298,14 +300,14 @@ mod __module_init {{
                     #[doc(hidden)]
                     #[link_section = \"{initcall_section}\"]
                     #[used]
-                    pub static __{name}_initcall: extern \"C\" fn() -> kernel::ffi::c_int = __{name}_init;
+                    pub static __{name_identifier}_initcall: extern \"C\" fn() -> kernel::ffi::c_int = __{name_identifier}_init;
 
                     #[cfg(not(MODULE))]
                     #[cfg(CONFIG_HAVE_ARCH_PREL32_RELOCATIONS)]
                     core::arch::global_asm!(
                         r#\".section \"{initcall_section}\", \"a\"
-                        __{name}_initcall:
-                            .long   __{name}_init - .
+                        __{name_identifier}_initcall:
+                            .long   __{name_identifier}_init - .
                             .previous
                         \"#
                     );
@@ -313,7 +315,7 @@ mod __module_init {{
                     #[cfg(not(MODULE))]
                     #[doc(hidden)]
                     #[no_mangle]
-                    pub extern \"C\" fn __{name}_init() -> kernel::ffi::c_int {{
+                    pub extern \"C\" fn __{name_identifier}_init() -> kernel::ffi::c_int {{
                         // SAFETY: This function is inaccessible to the outside due to the double
                         // module wrapping it. It is called exactly once by the C side via its
                         // placement above in the initcall section.
@@ -323,12 +325,12 @@ mod __module_init {{
                     #[cfg(not(MODULE))]
                     #[doc(hidden)]
                     #[no_mangle]
-                    pub extern \"C\" fn __{name}_exit() {{
+                    pub extern \"C\" fn __{name_identifier}_exit() {{
                         // SAFETY:
                         // - This function is inaccessible to the outside due to the double
                         //   module wrapping it. It is called exactly once by the C side via its
                         //   unique name,
-                        // - furthermore it is only called after `__{name}_init` has returned `0`
+                        // - furthermore it is only called after `__{name_identifier}_init` has returned `0`
                         //   (which delegates to `__init`).
                         unsafe {{ __exit() }}
                     }}
@@ -369,6 +371,7 @@ unsafe fn __exit() {{
         ",
         type_ = info.type_,
         name = info.name,
+        name_identifier = name_identifier,
         modinfo = modinfo.buffer,
         initcall_section = ".initcall6.init"
     )
-- 
2.31.1.272.g89b43f80a514

[PATCH V8 02/14] cpufreq: Use enum for cpufreq flags that use BIT()

[Viresh Kumar]

The BIT() macro is too complex for Rust's bindgen to interpret as
integer constants. This results in many of the cpufreq macros being
undefined in Rust auto-generated bindings. By replacing the "#define"
macros with an "enum", we ensure that bindgen can properly evaluate
these values, enabling their seamless use in Rust code.

No intentional functional impact.

Suggested-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 include/linux/cpufreq.h | 96 ++++++++++++++++++++++-------------------
 1 file changed, 51 insertions(+), 45 deletions(-)

diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index 7fe0981a7e46..bd67728081ad 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -292,11 +292,12 @@ static inline void cpufreq_stats_record_transition(struct cpufreq_policy *policy
  *                      CPUFREQ DRIVER INTERFACE                     *
  *********************************************************************/
 
-#define CPUFREQ_RELATION_L 0  /* lowest frequency at or above target */
-#define CPUFREQ_RELATION_H 1  /* highest frequency below or at target */
-#define CPUFREQ_RELATION_C 2  /* closest frequency to target */
-/* relation flags */
-#define CPUFREQ_RELATION_E BIT(2) /* Get if possible an efficient frequency */
+enum {
+	CPUFREQ_RELATION_L = 0, /* lowest frequency at or above target */
+	CPUFREQ_RELATION_H = BIT(0), /* highest frequency below or at target */
+	CPUFREQ_RELATION_C = BIT(1), /* closest frequency to target */
+	CPUFREQ_RELATION_E = BIT(2), /* Get if possible an efficient frequency */
+};
 
 #define CPUFREQ_RELATION_LE (CPUFREQ_RELATION_L | CPUFREQ_RELATION_E)
 #define CPUFREQ_RELATION_HE (CPUFREQ_RELATION_H | CPUFREQ_RELATION_E)
@@ -418,52 +419,57 @@ struct cpufreq_driver {
 
 /* flags */
 
-/*
- * Set by drivers that need to update internal upper and lower boundaries along
- * with the target frequency and so the core and governors should also invoke
- * the diver if the target frequency does not change, but the policy min or max
- * may have changed.
- */
-#define CPUFREQ_NEED_UPDATE_LIMITS		BIT(0)
+enum {
+	/*
+	 * Set by drivers that need to update internal upper and lower
+	 * boundaries along with the target frequency and so the core and
+	 * governors should also invoke the diver if the target frequency does
+	 * not change, but the policy min or max may have changed.
+	 */
+	CPUFREQ_NEED_UPDATE_LIMITS		= BIT(0),
 
-/* loops_per_jiffy or other kernel "constants" aren't affected by frequency transitions */
-#define CPUFREQ_CONST_LOOPS			BIT(1)
+	/*
+	 * loops_per_jiffy or other kernel "constants" aren't affected by
+	 * frequency transitions.
+	 */
+	CPUFREQ_CONST_LOOPS			= BIT(1),
 
-/*
- * Set by drivers that want the core to automatically register the cpufreq
- * driver as a thermal cooling device.
- */
-#define CPUFREQ_IS_COOLING_DEV			BIT(2)
+	/*
+	 * Set by drivers that want the core to automatically register the
+	 * cpufreq driver as a thermal cooling device.
+	 */
+	CPUFREQ_IS_COOLING_DEV			= BIT(2),
 
-/*
- * This should be set by platforms having multiple clock-domains, i.e.
- * supporting multiple policies. With this sysfs directories of governor would
- * be created in cpu/cpu<num>/cpufreq/ directory and so they can use the same
- * governor with different tunables for different clusters.
- */
-#define CPUFREQ_HAVE_GOVERNOR_PER_POLICY	BIT(3)
+	/*
+	 * This should be set by platforms having multiple clock-domains, i.e.
+	 * supporting multiple policies. With this sysfs directories of governor
+	 * would be created in cpu/cpu<num>/cpufreq/ directory and so they can
+	 * use the same governor with different tunables for different clusters.
+	 */
+	CPUFREQ_HAVE_GOVERNOR_PER_POLICY	= BIT(3),
 
-/*
- * Driver will do POSTCHANGE notifications from outside of their ->target()
- * routine and so must set cpufreq_driver->flags with this flag, so that core
- * can handle them specially.
- */
-#define CPUFREQ_ASYNC_NOTIFICATION		BIT(4)
+	/*
+	 * Driver will do POSTCHANGE notifications from outside of their
+	 * ->target() routine and so must set cpufreq_driver->flags with this
+	 *  flag, so that core can handle them specially.
+	 */
+	CPUFREQ_ASYNC_NOTIFICATION		= BIT(4),
 
-/*
- * Set by drivers which want cpufreq core to check if CPU is running at a
- * frequency present in freq-table exposed by the driver. For these drivers if
- * CPU is found running at an out of table freq, we will try to set it to a freq
- * from the table. And if that fails, we will stop further boot process by
- * issuing a BUG_ON().
- */
-#define CPUFREQ_NEED_INITIAL_FREQ_CHECK	BIT(5)
+	/*
+	 * Set by drivers which want cpufreq core to check if CPU is running at
+	 * a frequency present in freq-table exposed by the driver. For these
+	 * drivers if CPU is found running at an out of table freq, we will try
+	 * to set it to a freq from the table. And if that fails, we will stop
+	 * further boot process by issuing a BUG_ON().
+	 */
+	CPUFREQ_NEED_INITIAL_FREQ_CHECK		= BIT(5),
 
-/*
- * Set by drivers to disallow use of governors with "dynamic_switching" flag
- * set.
- */
-#define CPUFREQ_NO_AUTO_DYNAMIC_SWITCHING	BIT(6)
+	/*
+	 * Set by drivers to disallow use of governors with "dynamic_switching"
+	 * flag set.
+	 */
+	CPUFREQ_NO_AUTO_DYNAMIC_SWITCHING	= BIT(6),
+};
 
 int cpufreq_register_driver(struct cpufreq_driver *driver_data);
 void cpufreq_unregister_driver(struct cpufreq_driver *driver_data);
-- 
2.31.1.272.g89b43f80a514

[PATCH V8 03/14] rust: cpu: Add from_cpu()

[Viresh Kumar]

This implements cpu::from_cpu(), which returns a reference to
Device for a CPU. The C struct is created at initialization time for
CPUs and is never freed and so ARef isn't returned from this function.

The new helper will be used by Rust based cpufreq drivers.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 MAINTAINERS                     |  1 +
 rust/bindings/bindings_helper.h |  1 +
 rust/kernel/cpu.rs              | 31 +++++++++++++++++++++++++++++++
 rust/kernel/lib.rs              |  1 +
 4 files changed, 34 insertions(+)
 create mode 100644 rust/kernel/cpu.rs

diff --git a/MAINTAINERS b/MAINTAINERS
index 896a307fa065..ee6709599df5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6034,6 +6034,7 @@ F:	include/linux/cpuhotplug.h
 F:	include/linux/smpboot.h
 F:	kernel/cpu.c
 F:	kernel/smpboot.*
+F:	rust/kernel/cpu.rs
 
 CPU IDLE TIME MANAGEMENT FRAMEWORK
 M:	"Rafael J. Wysocki" <rafael@kernel.org>
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 55354e4dec14..fda1e0d8f376 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -10,6 +10,7 @@
 #include <linux/blk-mq.h>
 #include <linux/blk_types.h>
 #include <linux/blkdev.h>
+#include <linux/cpu.h>
 #include <linux/cred.h>
 #include <linux/errname.h>
 #include <linux/ethtool.h>
diff --git a/rust/kernel/cpu.rs b/rust/kernel/cpu.rs
new file mode 100644
index 000000000000..3054165d3818
--- /dev/null
+++ b/rust/kernel/cpu.rs
@@ -0,0 +1,31 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Generic CPU definitions.
+//!
+//! C header: [`include/linux/cpu.h`](srctree/include/linux/cpu.h)
+
+use crate::{bindings, device::Device, error::Result, prelude::ENODEV};
+
+/// Creates a new instance of CPU's device.
+///
+/// # Safety
+///
+/// Reference counting is not implemented for the CPU device in the C code. When a CPU is
+/// hot-unplugged, the corresponding CPU device is unregistered, but its associated memory
+/// is not freed.
+///
+/// Callers must ensure that the CPU device is not used after it has been unregistered.
+/// This can be achieved, for example, by registering a CPU hotplug notifier and removing
+/// any references to the CPU device within the notifier's callback.
+pub unsafe fn from_cpu(cpu: u32) -> Result<&'static Device> {
+    // SAFETY: The pointer returned by `get_cpu_device()`, if not `NULL`, is a valid pointer to
+    // a `struct device` and is never freed by the C code.
+    let ptr = unsafe { bindings::get_cpu_device(cpu) };
+    if ptr.is_null() {
+        return Err(ENODEV);
+    }
+
+    // SAFETY: The pointer returned by `get_cpu_device()`, if not `NULL`, is a valid pointer to
+    // a `struct device` and is never freed by the C code.
+    Ok(unsafe { Device::as_ref(ptr) })
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 496ed32b0911..415c500212dd 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -40,6 +40,7 @@
 pub mod block;
 #[doc(hidden)]
 pub mod build_assert;
+pub mod cpu;
 pub mod cred;
 pub mod device;
 pub mod device_id;
-- 
2.31.1.272.g89b43f80a514

[PATCH V8 04/14] rust: Add cpumask helpers

[Viresh Kumar]

In order to prepare for adding Rust abstractions for cpumask, this patch
adds cpumask helpers.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 MAINTAINERS                     |  1 +
 rust/bindings/bindings_helper.h |  1 +
 rust/helpers/cpumask.c          | 40 +++++++++++++++++++++++++++++++++
 rust/helpers/helpers.c          |  1 +
 4 files changed, 43 insertions(+)
 create mode 100644 rust/helpers/cpumask.c

diff --git a/MAINTAINERS b/MAINTAINERS
index ee6709599df5..bfc1bf2ebd77 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4021,6 +4021,7 @@ F:	lib/cpumask_kunit.c
 F:	lib/find_bit.c
 F:	lib/find_bit_benchmark.c
 F:	lib/test_bitmap.c
+F:	rust/helpers/cpumask.c
 F:	tools/include/linux/bitfield.h
 F:	tools/include/linux/bitmap.h
 F:	tools/include/linux/bits.h
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index fda1e0d8f376..59b4bc49d039 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -11,6 +11,7 @@
 #include <linux/blk_types.h>
 #include <linux/blkdev.h>
 #include <linux/cpu.h>
+#include <linux/cpumask.h>
 #include <linux/cred.h>
 #include <linux/errname.h>
 #include <linux/ethtool.h>
diff --git a/rust/helpers/cpumask.c b/rust/helpers/cpumask.c
new file mode 100644
index 000000000000..49267ad33b3c
--- /dev/null
+++ b/rust/helpers/cpumask.c
@@ -0,0 +1,40 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/cpumask.h>
+
+void rust_helper_cpumask_set_cpu(unsigned int cpu, struct cpumask *dstp)
+{
+	cpumask_set_cpu(cpu, dstp);
+}
+
+void rust_helper_cpumask_clear_cpu(int cpu, struct cpumask *dstp)
+{
+	cpumask_clear_cpu(cpu, dstp);
+}
+
+void rust_helper_cpumask_setall(struct cpumask *dstp)
+{
+	cpumask_setall(dstp);
+}
+
+unsigned int rust_helper_cpumask_weight(struct cpumask *srcp)
+{
+	return cpumask_weight(srcp);
+}
+
+void rust_helper_cpumask_copy(struct cpumask *dstp, const struct cpumask *srcp)
+{
+	cpumask_copy(dstp, srcp);
+}
+
+bool rust_helper_zalloc_cpumask_var(cpumask_var_t *mask, gfp_t flags)
+{
+	return zalloc_cpumask_var(mask, flags);
+}
+
+#ifndef CONFIG_CPUMASK_OFFSTACK
+void rust_helper_free_cpumask_var(cpumask_var_t mask)
+{
+	free_cpumask_var(mask);
+}
+#endif
diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
index 0640b7e115be..de2341cfd917 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c
@@ -11,6 +11,7 @@
 #include "bug.c"
 #include "build_assert.c"
 #include "build_bug.c"
+#include "cpumask.c"
 #include "cred.c"
 #include "device.c"
 #include "err.c"
-- 
2.31.1.272.g89b43f80a514

Re: [PATCH V8 04/14] rust: Add cpumask helpers

[Yury Norov]

On Thu, Feb 06, 2025 at 02:58:25PM +0530, Viresh Kumar wrote:
> In order to prepare for adding Rust abstractions for cpumask, this patch
> adds cpumask helpers.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  MAINTAINERS                     |  1 +
>  rust/bindings/bindings_helper.h |  1 +
>  rust/helpers/cpumask.c          | 40 +++++++++++++++++++++++++++++++++
>  rust/helpers/helpers.c          |  1 +
>  4 files changed, 43 insertions(+)
>  create mode 100644 rust/helpers/cpumask.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index ee6709599df5..bfc1bf2ebd77 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4021,6 +4021,7 @@ F:	lib/cpumask_kunit.c
>  F:	lib/find_bit.c
>  F:	lib/find_bit_benchmark.c
>  F:	lib/test_bitmap.c
> +F:	rust/helpers/cpumask.c

Sorry what? I never committed to maintain this thing, and will
definitely not do it for free.

NAK.

>  F:	tools/include/linux/bitfield.h
>  F:	tools/include/linux/bitmap.h
>  F:	tools/include/linux/bits.h
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index fda1e0d8f376..59b4bc49d039 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -11,6 +11,7 @@
>  #include <linux/blk_types.h>
>  #include <linux/blkdev.h>
>  #include <linux/cpu.h>
> +#include <linux/cpumask.h>
>  #include <linux/cred.h>
>  #include <linux/errname.h>
>  #include <linux/ethtool.h>
> diff --git a/rust/helpers/cpumask.c b/rust/helpers/cpumask.c
> new file mode 100644
> index 000000000000..49267ad33b3c
> --- /dev/null
> +++ b/rust/helpers/cpumask.c
> @@ -0,0 +1,40 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/cpumask.h>
> +
> +void rust_helper_cpumask_set_cpu(unsigned int cpu, struct cpumask *dstp)
> +{
> +	cpumask_set_cpu(cpu, dstp);
> +}
> +
> +void rust_helper_cpumask_clear_cpu(int cpu, struct cpumask *dstp)
> +{
> +	cpumask_clear_cpu(cpu, dstp);
> +}
> +
> +void rust_helper_cpumask_setall(struct cpumask *dstp)
> +{
> +	cpumask_setall(dstp);
> +}
> +
> +unsigned int rust_helper_cpumask_weight(struct cpumask *srcp)
> +{
> +	return cpumask_weight(srcp);
> +}
> +
> +void rust_helper_cpumask_copy(struct cpumask *dstp, const struct cpumask *srcp)
> +{
> +	cpumask_copy(dstp, srcp);
> +}
> +
> +bool rust_helper_zalloc_cpumask_var(cpumask_var_t *mask, gfp_t flags)
> +{
> +	return zalloc_cpumask_var(mask, flags);
> +}
> +
> +#ifndef CONFIG_CPUMASK_OFFSTACK
> +void rust_helper_free_cpumask_var(cpumask_var_t mask)
> +{
> +	free_cpumask_var(mask);
> +}
> +#endif

This is most likely wrong because free_cpumask_var() is declared
unconditionally, and I suspect the rust helper should be as well.

> diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
> index 0640b7e115be..de2341cfd917 100644
> --- a/rust/helpers/helpers.c
> +++ b/rust/helpers/helpers.c
> @@ -11,6 +11,7 @@
>  #include "bug.c"
>  #include "build_assert.c"
>  #include "build_bug.c"
> +#include "cpumask.c"
>  #include "cred.c"
>  #include "device.c"
>  #include "err.c"
> -- 
> 2.31.1.272.g89b43f80a514

Please for the next iteration keep me CCed for the whole series. I
want to make sure you'll not make me a rust maintainer accidentally.

Thanks,
Yury

Re: [PATCH V8 04/14] rust: Add cpumask helpers

[Viresh Kumar]

On 10-02-25, 19:02, Yury Norov wrote:
> On Thu, Feb 06, 2025 at 02:58:25PM +0530, Viresh Kumar wrote:
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index ee6709599df5..bfc1bf2ebd77 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -4021,6 +4021,7 @@ F:	lib/cpumask_kunit.c
> >  F:	lib/find_bit.c
> >  F:	lib/find_bit_benchmark.c
> >  F:	lib/test_bitmap.c
> > +F:	rust/helpers/cpumask.c
> 
> Sorry what? I never committed to maintain this thing, and will
> definitely not do it for free.
> 
> NAK.

Okay. I will add a separate entry for Rust cpumask stuff.

> > +#ifndef CONFIG_CPUMASK_OFFSTACK
> > +void rust_helper_free_cpumask_var(cpumask_var_t mask)
> > +{
> > +	free_cpumask_var(mask);
> > +}
> > +#endif
> 
> This is most likely wrong because free_cpumask_var() is declared
> unconditionally, and I suspect the rust helper should be as well.

Non-trivial C macros and inlined C functions cannot be used directly
in the Rust code and are used via functions ("helpers") that wrap
those so that they can be called from Rust.

The free_cpumask_var() function is defined as inline only for the
CONFIG_CPUMASK_OFFSTACK=n case and that's where we need this wrapper.
For the other case (CONFIG_CPUMASK_OFFSTACK=y), Rust code can directly
call free_cpumask_var() from the C code and we don't need this helper.

> Please for the next iteration keep me CCed for the whole series. I
> want to make sure you'll not make me a rust maintainer accidentally.

Sure.

-- 
viresh

Re: [PATCH V8 04/14] rust: Add cpumask helpers

[Yury Norov]

+ Jason, Christoph

On Tue, Feb 11, 2025 at 09:59:08AM +0530, Viresh Kumar wrote:
> On 10-02-25, 19:02, Yury Norov wrote:
> > On Thu, Feb 06, 2025 at 02:58:25PM +0530, Viresh Kumar wrote:
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index ee6709599df5..bfc1bf2ebd77 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -4021,6 +4021,7 @@ F:	lib/cpumask_kunit.c
> > >  F:	lib/find_bit.c
> > >  F:	lib/find_bit_benchmark.c
> > >  F:	lib/test_bitmap.c
> > > +F:	rust/helpers/cpumask.c
> > 
> > Sorry what? I never committed to maintain this thing, and will
> > definitely not do it for free.
> > 
> > NAK.
> 
> Okay.

No, not Okay.

To begin with, this is the 8th version of the same patch, but you
only now bothered to CC someone who is listed in MAINTAINERS. This is
not how the community works.

You also made it a patch bomb that touches multiple critical and very
sensitive subsystems. You link them to an experimental and unstable
project, and do it in a way that makes it really easy to slip through
maintainers' attention.

Not speaking for others, but please for cpumasks create a separate
series and start thorough discussion.

> I will add a separate entry for Rust cpumask stuff.

That would make things even worse. Before you wanted me to maintain
rust linkage. Now you want me to get approval from someone else who
maintains rust linkage. In case I need to change something, I want to
be able to just change.

I went deeper into the subject, and found that rust team has similar
problems with other maintainers:

https://lore.kernel.org/lkml/20250108122825.136021-3-abdiel.janulgue@gmail.com/

I'm particularly concerned with the following comment from Jason
Gunthorpe:

  All PRs to Linus must not break the rust build and the responsibilty
  for that falls to all the maintainers. If the Rust team is not quick
  enough to resolve any issues during the development window then
  patches must be dropped before sending PRs, or Linus will refuse the
  PR.

https://lore.kernel.org/lkml/20250130154646.GA2298732@nvidia.com/

And that happened at least once, right?

I don't expect that the functions you export now will get changed
anytime soon but it happens from time to time. cpumask_next_wrap()
is one recent example.

https://lore.kernel.org/netdev/20250128164646.4009-7-yury.norov@gmail.com/T/

The more drivers you write, the more functionality you will inevitably
pull. And the risk that something will get broken one day will grow
exponentially. So before we move forward, please explain in very details
how would you act in a scenario described by Jason?

Do you proactively run builds against development branches? If so,
please add my bitmap-for-next.

https://github.com/norov/linux/tree/bitmap-for-next

Have you considered creating a conftest, so that if rust fails to build
against the current kernel, it will get disabled until you fix the
issue?

Maybe you will just teach your language to understand inlines, and
that's it? Does it understand macros?

Thanks,
Yury

> > > +#ifndef CONFIG_CPUMASK_OFFSTACK
> > > +void rust_helper_free_cpumask_var(cpumask_var_t mask)
> > > +{
> > > +	free_cpumask_var(mask);
> > > +}
> > > +#endif
> > 
> > This is most likely wrong because free_cpumask_var() is declared
> > unconditionally, and I suspect the rust helper should be as well.
> 
> Non-trivial C macros and inlined C functions cannot be used directly
> in the Rust code and are used via functions ("helpers") that wrap
> those so that they can be called from Rust.
> 
> The free_cpumask_var() function is defined as inline only for the
> CONFIG_CPUMASK_OFFSTACK=n case and that's where we need this wrapper.
> For the other case (CONFIG_CPUMASK_OFFSTACK=y), Rust code can directly
> call free_cpumask_var() from the C code and we don't need this helper.
> 
> > Please for the next iteration keep me CCed for the whole series. I
> > want to make sure you'll not make me a rust maintainer accidentally.
> 
> Sure.
> 
> -- 
> viresh

Re: [PATCH V8 04/14] rust: Add cpumask helpers

[Jason Gunthorpe]

On Tue, Feb 11, 2025 at 11:24:55AM -0500, Yury Norov wrote:
> I'm particularly concerned with the following comment from Jason
> Gunthorpe:
> 
>   All PRs to Linus must not break the rust build and the responsibilty
>   for that falls to all the maintainers. If the Rust team is not quick
>   enough to resolve any issues during the development window then
>   patches must be dropped before sending PRs, or Linus will refuse the
>   PR.
> 
> https://lore.kernel.org/lkml/20250130154646.GA2298732@nvidia.com/
> 
> And that happened at least once, right?

Yes, 6 patches were dropped from the last merge window by Andrew and
Linus because of rust build breakage.

There has since been some clarification posted:

 https://lore.kernel.org/all/CANiq72m-R0tOakf=j7BZ78jDHdy=9-fvZbAT8j91Je2Bxy0sFg@mail.gmail.com/

Quoting:

   Who is responsible if a C change breaks a build with Rust enabled?

    The usual kernel policy applies. So, by default, changes should not
    be introduced if they are known to break the build, including Rust.

    However, exceptionally, for Rust, a subsystem may allow to
    temporarily break Rust code. The intention is to facilitate friendly
    adoption of Rust in a subsystem without introducing a burden to
    existing maintainers who may be working on urgent fixes for the C
    side. The breakage should nevertheless be fixed as soon as possible,
    ideally before the breakage reaches Linus.

    For instance, this approach was chosen by the block laye
    they called it "stage 1" in their Rust integration plan.

    We believe this approach is reasonable as long as the kernel does not
    have way too many subsystems doing that (because otherwise it would be
    very hard to build e.g. linux-next).

However, it is unclear how this "temporarily break Rust code" will
work in practice. We do not know under what conditions Linus will
accept a PR that forces him to go to CONFIG_RUST=n, or even if Linus
has agreed to this exception policy.

I suggest the safe thing for any maintainer is to not send Linus PRs
that break rust, otherwise they get to be a test case to see what
happens..

Jason

Re: [PATCH V8 04/14] rust: Add cpumask helpers

[Danilo Krummrich]

Hi Yury,

On Tue, Feb 11, 2025 at 11:24:55AM -0500, Yury Norov wrote:
> + Jason, Christoph
> 
> On Tue, Feb 11, 2025 at 09:59:08AM +0530, Viresh Kumar wrote:
> > On 10-02-25, 19:02, Yury Norov wrote:
> > > On Thu, Feb 06, 2025 at 02:58:25PM +0530, Viresh Kumar wrote:
> > > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > > index ee6709599df5..bfc1bf2ebd77 100644
> > > > --- a/MAINTAINERS
> > > > +++ b/MAINTAINERS
> > > > @@ -4021,6 +4021,7 @@ F:	lib/cpumask_kunit.c
> > > >  F:	lib/find_bit.c
> > > >  F:	lib/find_bit_benchmark.c
> > > >  F:	lib/test_bitmap.c
> > > > +F:	rust/helpers/cpumask.c
> > > 
> > > Sorry what? I never committed to maintain this thing, and will
> > > definitely not do it for free.
> > > 
> > > NAK.
> > 
> > Okay.
> 
> No, not Okay.
> 
> To begin with, this is the 8th version of the same patch, but you
> only now bothered to CC someone who is listed in MAINTAINERS. This is
> not how the community works.

I'm answering, since I was involved in the discussion you refer to below, but
please also let me add a few other thoughts from my side.

First of all, I think you are right here.

AFAICS, the cpumask abstraction was added to this series in v6, and you were
CC'd in v8, which is *not* OK.

I also agree that this definitely deserves it's own series for you to be
properly involved.

> > I will add a separate entry for Rust cpumask stuff.
> 
> That would make things even worse. Before you wanted me to maintain
> rust linkage. Now you want me to get approval from someone else who
> maintains rust linkage. In case I need to change something, I want to
> be able to just change.

I think the decision is up to you, whether

1) You want to maintain it on your own.
2) You want a co-maintainer / reviewer that takes care of the Rust parts.
3) You want nothing to do with it and have it maintained as a separate
   component.

In case you prefer option 3), please do *not* see it as "you need to get
approval from someone else for code changes in your subsystem", because no ones
wants to impose this on you.

Please see it as just another driver or another component that makes use of the
API you maintain.

If you are running into API changes that affect the Rust abstraction, that's
where you can refer to the maintainer of the Rust abstraction to help out. Just
like with every other driver or component that uses a core API, which isn't
trivial to adjust.

> 
> I went deeper into the subject, and found that rust team has similar
> problems with other maintainers:
> 
> https://lore.kernel.org/lkml/20250108122825.136021-3-abdiel.janulgue@gmail.com/

I don't think that this case is similar to the one you linked in. I think you
were indeed a bit bypassed here, plus you seem to have a real concern with
maintainership, which I think is fair to be addressed and resolved.

I hope my reply already helps with that.

- Danilo

Re: [PATCH V8 04/14] rust: Add cpumask helpers

[Miguel Ojeda]

On Tue, Feb 11, 2025 at 5:24 PM Yury Norov <yury.norov@gmail.com> wrote:
>
> No, not Okay.
>
> To begin with, this is the 8th version of the same patch, but you
> only now bothered to CC someone who is listed in MAINTAINERS. This is
> not how the community works.

Yeah, that is not good.

For what it is worth, we try to make this very clear:

    https://rust-for-linux.com/contributing#submitting-new-abstractions-and-modules

i.e. that maintainers need to be Cc'd and contacted as soon as
possible (possibly even before writing the code).

> You also made it a patch bomb that touches multiple critical and very
> sensitive subsystems. You link them to an experimental and unstable
> project, and do it in a way that makes it really easy to slip through
> maintainers' attention.

Not sure what you mean by "unstable project", but I agree that the
patch series, unless Viresh is the maintainer of the C side of
everything added, it should be discussed and maintenance discussed
accordingly before merging anything.

This is what we have done for everything else, and that has not changed.

I try to spot cases where this is not done, which is why I Cc'd you in
v7 and told Viresh to please do so, and he did -- I don't think he was
trying to bypass on purpose:

    https://lore.kernel.org/rust-for-linux/CANiq72=o+uc3ZnNrdkuoSGSL8apNE4z4QwpvsiLfGzXFywSLrQ@mail.gmail.com/

> That would make things even worse. Before you wanted me to maintain
> rust linkage. Now you want me to get approval from someone else who
> maintains rust linkage. In case I need to change something, I want to
> be able to just change.

Like Danilo mentions, there are several ways to go forward here. For
some ideas/context, please see:

    https://rust-for-linux.com/rust-kernel-policy#how-is-rust-introduced-in-a-subsystem

(Thanks Jason for linking the page)

And, yeah, whoever ends up maintaining this, then they should of
course be testing it properly with Rust enabled with a proper config
for that and so on, just like one would do for anything else. By the
way, it is possible to request Intel's 0day to build with Rust
enabled.

(Side-note: to clarify, there are different parties involved here --
"Rust team" is fairly ambiguous.)

Cheers,
Miguel

Re: [PATCH V8 04/14] rust: Add cpumask helpers

[Yury Norov]

Hi Miguel,

On Tue, Feb 11, 2025 at 10:37:26PM +0100, Miguel Ojeda wrote:
> On Tue, Feb 11, 2025 at 5:24 PM Yury Norov <yury.norov@gmail.com> wrote:
> >
> > No, not Okay.
> >
> > To begin with, this is the 8th version of the same patch, but you
> > only now bothered to CC someone who is listed in MAINTAINERS. This is
> > not how the community works.
> 
> Yeah, that is not good.
> 
> For what it is worth, we try to make this very clear:
> 
>     https://rust-for-linux.com/contributing#submitting-new-abstractions-and-modules
> 
> i.e. that maintainers need to be Cc'd and contacted as soon as
> possible (possibly even before writing the code).
> 
> > You also made it a patch bomb that touches multiple critical and very
> > sensitive subsystems. You link them to an experimental and unstable
> > project, and do it in a way that makes it really easy to slip through
> > maintainers' attention.
> 
> Not sure what you mean by "unstable project", but I agree that the

I mean that Andrew's branch got broken because of it.

> patch series, unless Viresh is the maintainer of the C side of
> everything added, it should be discussed and maintenance discussed
> accordingly before merging anything.
> 
> This is what we have done for everything else, and that has not changed.
> 
> I try to spot cases where this is not done, which is why I Cc'd you in
> v7 and told Viresh to please do so, and he did -- I don't think he was
> trying to bypass on purpose:
> 
>     https://lore.kernel.org/rust-for-linux/CANiq72=o+uc3ZnNrdkuoSGSL8apNE4z4QwpvsiLfGzXFywSLrQ@mail.gmail.com/

Yes, I see. Viresh didn't do that on purpose. Let's move forward. I'm
more concerned about lack of testing on rust side that ended up with
the patches withdrawal.
 
> > That would make things even worse. Before you wanted me to maintain
> > rust linkage. Now you want me to get approval from someone else who
> > maintains rust linkage. In case I need to change something, I want to
> > be able to just change.
> 
> Like Danilo mentions, there are several ways to go forward here. For
> some ideas/context, please see:
> 
>     https://rust-for-linux.com/rust-kernel-policy#how-is-rust-introduced-in-a-subsystem
> 
> (Thanks Jason for linking the page)
> 
> And, yeah, whoever ends up maintaining this, then they should of
> course be testing it properly with Rust enabled with a proper config
> for that and so on, just like one would do for anything else. By the
> way, it is possible to request Intel's 0day to build with Rust
> enabled.

Yes, this should be done. 0-day is already testing everything in my 
https://github.com/norov/linux repo. If you know right people, please
ask them to test bitmap-for-next branch with rust enabled. If there's
enough resources, please cover all the repo.

> (Side-note: to clarify, there are different parties involved here --
> "Rust team" is fairly ambiguous.)
>
> Cheers,
> Miguel

I'm the right person to look after rust bindings for bitops, inevitably.
I will take over patch 4/14 and submit it separately together with a
new maintenance entry.

I will not maintain any rust code. For 5/14, after I'll send my series,
Viresh, can you submit 5/14 separately and create a separate entry in
MAINTAINERS. Please make me a reviewer there.

I'll think about details over the weekend and will submit everything
early next week.

Rasmus, if you would like to help me reviewing this thing, please let
me know.

Thanks,
Yury

Re: [PATCH V8 04/14] rust: Add cpumask helpers

[Viresh Kumar]

On 13-02-25, 21:20, Yury Norov wrote:
> I'm the right person to look after rust bindings for bitops, inevitably.
> I will take over patch 4/14 and submit it separately together with a
> new maintenance entry.
> 
> I will not maintain any rust code. For 5/14, after I'll send my series,
> Viresh, can you submit 5/14 separately and create a separate entry in
> MAINTAINERS. Please make me a reviewer there.

Sounds good. Thanks.

-- 
viresh

Re: [PATCH V8 04/14] rust: Add cpumask helpers

[Miguel Ojeda]

On Fri, Feb 14, 2025 at 3:20 AM Yury Norov <yury.norov@gmail.com> wrote:
>
> I mean that Andrew's branch got broken because of it.

(This response is extra verbose to be extra clear, given the recent
discussions these last days -- I don't mean you said or implied any of
this, sorry)

To be clear: it was not a patch we introduced that broke it. It was a
patch series from someone else that got a Linaro build report and
that, nevertheless, was sent to Linus.

The build report reached our mailing list, but we were not Cc'd
directly about it (i.e. as individuals), we were not pinged about it
(i.e. we never had any further questions about that after that single
day), and we didn't realize the series was kept in a queue targeting
Linus.

Moreover, the build failure happened with a configuration that is
best-effort and is documented as very experimental (GCC mixed builds),
and that we didn't know Linus was building (we knew he built with
Clang+Rust, which is what we are focused on testing), so we didn't
spot that either on our side.

By the way, we have not been Cc'd in the new version, either... Nor
the mailing list.

To be clear, I am not blaming anyone for the breakage, and I already
apologized that we didn't notice that report. But it is not our fault
either.

We also never promised we would fix every single Rust issue spotted
across the entire kernel. We try to do our best to help, though.

    https://rust-for-linux.com/rust-kernel-policy#who-is-responsible-if-a-c-change-breaks-a-build-with-rust-enabled
    https://rust-for-linux.com/rust-kernel-policy#didnt-you-promise-rust-wouldnt-be-extra-work-for-maintainers

> Yes, I see. Viresh didn't do that on purpose. Let's move forward. I'm
> more concerned about lack of testing on rust side that ended up with
> the patches withdrawal.

Please see above.

I regularly test different combinations (branches, configs, compiler
versions, and so on) to catch mainly toolchain issues and so on, and
keep things as clean as I can. Others use regularly the Rust support
for their different use cases, thus more testing happens on different
environments. In other words, things generally work just fine.

However, our testing is not meant to catch every issue everywhere.
Like for anything else in the kernel, whoever maintains a branch with
a particular Rust feature needs to set up proper testing for that
particular feature and relevant configs.

Regarding the GCC mixed builds: worst case, if we see there is no
bandwidth for it (be it ourselves or other maintainers testing their
own branches), we could limit the testing required (e.g. limiting GCC
versions when Rust is enabled), or we could request Linus to skip Rust
with GCC in his builds, so that at least PRs are not blocked on that.

> Yes, this should be done. 0-day is already testing everything in my
> https://github.com/norov/linux repo. If you know right people, please
> ask them to test bitmap-for-next branch with rust enabled. If there's
> enough resources, please cover all the repo.

Definitely! I will do that and Cc you.

I hope that helps.

> I'm the right person to look after rust bindings for bitops, inevitably.
> I will take over patch 4/14 and submit it separately together with a
> new maintenance entry.
>
> I will not maintain any rust code. For 5/14, after I'll send my series,
> Viresh, can you submit 5/14 separately and create a separate entry in
> MAINTAINERS. Please make me a reviewer there.
>
> I'll think about details over the weekend and will submit everything
> early next week.

Thanks a lot Yury, that sounds very reasonable.

Cheers,
Miguel

Re: [PATCH V8 04/14] rust: Add cpumask helpers

[Jason Gunthorpe]

On Fri, Feb 14, 2025 at 06:56:43PM +0100, Miguel Ojeda wrote:

> > I mean that Andrew's branch got broken because of it.

> We also never promised we would fix every single Rust issue spotted
> across the entire kernel. We try to do our best to help, though.

Sure, but it was said, by many people, many times, that "Rust is
allowed to break".

This is not just my incorrect impression. For instance read Philipp's
note to Christoph:

https://lore.kernel.org/all/293df3d54bad446e8fd527f204c6dc301354e340.camel@mailbox.org/

 > reassure you that the burden of keeping Rust abstractions working with
 > any changes on the C side rests entirely on the Rust side's shoulders?
 > (because that's what the statements made by the latter seem to mean to
 > me)

And Greg's version:

https://lore.kernel.org/all/2025013030-gummy-cosmic-7927@gregkh/

 > So the claim remains the same here.  It's just like staging, api changes
 > to subsystems are allowed to break staging, and rust code, and
 > maintainers do NOT have to fix them up there, that's up to the staging
 > and rust maintainers/developers to do so.

I've heard the same statements at conferences and in other coverages
like LWN. Frankly, I never much believed in this story as workable,
but it was advanced by many people to smooth the adoption of Rust
bindings.

>    https://rust-for-linux.com/rust-kernel-policy#didnt-you-promise-rust-wouldnt-be-extra-work-for-maintainers

I do not agree with "Didn't you promise Rust wouldn't be extra work
for maintainers?" in your document. Clearly there is a widespread
belief this kind of promise was made, even if it was never made by
you. "Rust is allowed to break" is understood to be the same as saying
it won't cause extra work.

However, I am glad we are seeing a more realistic understanding of
what Rust requires of the community over the long term.

Jason

Re: [PATCH V8 04/14] rust: Add cpumask helpers

[Miguel Ojeda]

On Fri, Feb 14, 2025 at 8:11 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
>
> Sure, but it was said, by many people, many times, that "Rust is
> allowed to break".

A lot of people have said many things (especially in online fora), and
many of those things are contradictory, but that does not really mean
anything.

> This is not just my incorrect impression. For instance read Philipp's
> note to Christoph:
>
> https://lore.kernel.org/all/293df3d54bad446e8fd527f204c6dc301354e340.camel@mailbox.org/

Philipp probably sent that message with his best intentions, but he is
not part of the Rust subsystem nor speaks on our behalf.

He may have been speaking for other groups, or maybe just his own
opinion -- I do not know.

Moreover, sometimes suggestions like his may be referring to
particular subsystems (like the one that was discussed in that
thread), e.g. like block decided to take an approach where Rust is
allowed to break, rather than speaking globally.

Finally, ambiguous terms are used in many cases to refer to different
parties: "Rust community", "Rust people", "Rust team", "Rust
maintainers"... I have started to ask people to avoid doing that (at
least in the LKML), please, and be concrete if possible.

> And Greg's version:
>
> https://lore.kernel.org/all/2025013030-gummy-cosmic-7927@gregkh/

I cannot speak for Greg, sorry.

I can read his message in the following ways:

  - I can read it as a general ability of subsystems to potentially
agree to treat Rust code as something like staging, like block's plan.

  - I can read it within the context of those patches, where, as far
as I know, Danilo et al. stepped up to maintain it, like Andreas did
for block.

  - I can read it as the fact that the Rust subsystem will help,
best-effort, to bootstrap Rust and help with integration where
possible.

We need maintainers' help and expertise from other subsystems to
succeed. And we do not want to force other subsystems into dealing
with Rust. That is why the deal was that we would contact and wait for
other subsystems to handle Rust and so on. It is also why I asked, in
the very meeting where it was decided to merge Rust, that in exchange,
we would eventually need some flexibility by maintainers that may not
want Rust in their subsystem but that nevertheless may be the owners
of core APIs that other users of Rust in the kernel need. I did so
because I knew the day would come we would be in the situation we are
in that email thread.

> I've heard the same statements at conferences and in other coverages
> like LWN. Frankly, I never much believed in this story as workable,
> but it was advanced by many people to smooth the adoption of Rust
> bindings.

Again, people may make statements, but they may be local to their
subsystem, or just their opinion, or it may be a misunderstanding, and
so on and so forth.

It is very hard to keep hundreds of maintainers on the same page.

> I do not agree with "Didn't you promise Rust wouldn't be extra work
> for maintainers?" in your document. Clearly there is a widespread
> belief this kind of promise was made, even if it was never made by
> you. "Rust is allowed to break" is understood to be the same as saying
> it won't cause extra work.

Sorry, but I have to strongly push back against this paragraph.

Are you really saying that, because people out there may think
something, we cannot claim anymore that we did not promise something?

Furthermore, I don't agree with your assessment in your last sentence
at all. Even if it was decided to allow Rust to break globally and at
all times, it does not mean Rust is not extra work. It is, because
collaboration would be still needed with different subsystems and so
on. The plan has always been to not have Rust be something hidden in a
corner. For instance, see from 2020:

    https://lore.kernel.org/all/CAHk-=wipXqemHbVnK1kQsFzGOOZ8FUXn3PKrZb5WC=KkgAjRRw@mail.gmail.com/

> However, I am glad we are seeing a more realistic understanding of
> what Rust requires of the community over the long term.

That is good, but to be clear, from my point of view, the approach
mentioned in the document I wrote is what we have always said.

Cheers,
Miguel

Re: [PATCH V8 04/14] rust: Add cpumask helpers

[Jason Gunthorpe]

On Fri, Feb 14, 2025 at 09:24:31PM +0100, Miguel Ojeda wrote:
> On Fri, Feb 14, 2025 at 8:11 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
> >
> > Sure, but it was said, by many people, many times, that "Rust is
> > allowed to break".
> 
> A lot of people have said many things (especially in online fora), and
> many of those things are contradictory, but that does not really mean
> anything.

We don't have a community where there is a clear authority
structure. If lots of people are repeating a thing, that thing usually
becomes the consensus and common viewpoint regardless of who
originated it. The repetition reflects community agreement and buy in
of the idea.

At the end of the day, only the policy adopted by the people merging
patches really matters.

The assumption being if respected people speak with authority on a
policy they have also got buy in from the people responsible to
execute it.

I also think you should merge your policy document to the tree, not
keep it on a web page. That seems to be a big part of getting
community agreed policy adopted.

> Finally, ambiguous terms are used in many cases to refer to different
> parties: "Rust community", "Rust people", "Rust team", "Rust
> maintainers"... I have started to ask people to avoid doing that (at
> least in the LKML), please, and be concrete if possible.

Okay, that makes lots of seense. Please don't use "we" as well.. I
have no idea who is included in your "we", or what to even call it.

> > And Greg's version:
> >
> > https://lore.kernel.org/all/2025013030-gummy-cosmic-7927@gregkh/
> 
> I cannot speak for Greg, sorry.

Yet he does seems to be speaking with authority on this topic. His
message was quoted on LWN and likely was read by a large number of
maintainers.

Is he not part of your "we"?

> I can read his message in the following ways:

I think it was very clear, sorry, I don't read it your many ways at
all.

>   - I can read it as a general ability of subsystems to potentially
> agree to treat Rust code as something like staging, like block's plan.

As a side note, I don't see how anyone can enact this plan without the
support of Linus to do CONFIG_RUST=n builds and put out a non-working
rc1. IMHO it is yet unclear if this is real thing or an unproven idea
block has that will run into problems.

> It is very hard to keep hundreds of maintainers on the same page.

It is, but also I think you need to take this challenge to succeed.
 
> > I do not agree with "Didn't you promise Rust wouldn't be extra work
> > for maintainers?" in your document. Clearly there is a widespread
> > belief this kind of promise was made, even if it was never made by
> > you. "Rust is allowed to break" is understood to be the same as saying
> > it won't cause extra work.
> 
> Sorry, but I have to strongly push back against this paragraph.
> 
> Are you really saying that, because people out there may think
> something, we cannot claim anymore that we did not promise something?

Again "we" ?

I'm not concerned about "people out there". Greg said it. Others who I
would think are part of your "we" have posted it on LKML.

It is not clear at all. If you said Miguel never claimed it, then I
would not complain. You said it correctly above, be concrete. Ideally
acknowledge there were different policy ideas in wide circulation, but
they did not get adopted.

> Furthermore, I don't agree with your assessment in your last sentence
> at all. Even if it was decided to allow Rust to break globally and at
> all times, it does not mean Rust is not extra work. 

I appreciate this point is realistically true, but look at how Philipp
presented this 'no break' concept to Christoph as a no-work option for
him.

My main point here is that I'm glad things are getting straightened
out, some of the conflicting policy ideas shot down, and the demands
on maintainers are being articulated more clearly.

> That is good, but to be clear, from my point of view, the approach
> mentioned in the document I wrote is what we have always said.

There is another we :)

Jason

Re: [PATCH V8 04/14] rust: Add cpumask helpers

[Miguel Ojeda]

On Fri, Feb 14, 2025 at 10:06 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
>
> We don't have a community where there is a clear authority
> structure. If lots of people are repeating a thing, that thing usually
> becomes the consensus and common viewpoint regardless of who
> originated it. The repetition reflects community agreement and buy in
> of the idea.

We cannot be expected to chase down every maintainer in all online fora.

Not just because it is not viable, but because hopefully this is not
about who shouts the loudest (== more often).

Moreover, we do not have the authority to set policy ourselves, and we
are not in the business of forcing things into other subsystems.

Now, if you are a maintainer, and you want to deal with Rust in the
kernel, then please do any of the following: send us an email, join
our talks, read the LWN articles about our talks (but do not assume
LWN comments are "consensus" somehow), join our MC in LPC, join
Kangrejos, and so on and so forth.

We have been open to talk to every single maintainer that wanted to
use Rust or that we needed to interface with, and other maintainers
can tell you that we have successfully worked with them.

> At the end of the day, only the policy adopted by the people merging
> patches really matters.
>
> The assumption being if respected people speak with authority on a
> policy they have also got buy in from the people responsible to
> execute it.

Sure, and that is why we talked to the actual, relevant, maintainers
and subsystems that are taking patches that are related to Rust.

We may not have talked to you directly or explain things to you, but
that does not mean we didn't talk to others.

> I also think you should merge your policy document to the tree, not
> keep it on a web page. That seems to be a big part of getting
> community agreed policy adopted.

Very happy to do so if others are happy with it.

I published it in the website because it is not a document the overall
kernel community signed on so far. Again, we do not have that
authority as far as I understand.

The idea was to clarify the main points, and gather consensus. The
FOSDEM 2025 keynote quotes were also intended in a similar way:

    https://fosdem.org/2025/events/attachments/fosdem-2025-6507-rust-for-linux/slides/236835/2025-02-0_iwSaMYM.pdf

> Okay, that makes lots of seense. Please don't use "we" as well.. I
> have no idea who is included in your "we", or what to even call it.

In my emails in this thread, "we" means the Rust subsystem entry.

> Yet he does seems to be speaking with authority on this topic. His
> message was quoted on LWN and likely was read by a large number of
> maintainers.
>
> Is he not part of your "we"?

No, he is not part of the Rust subsystem entry, as you can easily
verify in the `MAINTAINERS` file.

And I can not speak for him either.

He, of course, has helped us a lot, like other kernel maintainers have.

By the way, the document, at the top, mentions:

    Like most things in the kernel, these points are not hard rules
and can change over time depending on the situation and what key
maintainers and the kernel community discuss.

> I think it was very clear, sorry, I don't read it your many ways at
> all.

Well, then please ask Greg, not us, and remember to Cc him, by the way.

> As a side note, I don't see how anyone can enact this plan without the
> support of Linus to do CONFIG_RUST=n builds and put out a non-working
> rc1. IMHO it is yet unclear if this is real thing or an unproven idea
> block has that will run into problems.

Please ask Jens and the block layer -- Cc'ing Jens (Andreas and Boqun
are already Cc'd):

    https://lore.kernel.org/all/593a98c9-baaa-496b-a9a7-c886463722e1@kernel.dk/

Having said that, I am not sure what you mean by -rc1. It is in the
context of a friendly collaboration -- I assume the intention is that
Andreas et al. are given enough lead time on new features to fix them
before the merge window. For fixes, it may be harder, of course. Other
ideas: they may be able to config out certain parts too; or in the
worst case, in an emergency, Linus may decide to break Rust. They may
be able to tell you the details of their plan.

> It is, but also I think you need to take this challenge to succeed.

In fact, we are, and I explained above all the ways we are engaging
with maintainers and so on.

Please note that the fact that we may not tackle the challenge in the
way you would like does not mean we are not doing it.

> It is not clear at all. If you said Miguel never claimed it, then I
> would not complain. You said it correctly above, be concrete. Ideally
> acknowledge there were different policy ideas in wide circulation, but
> they did not get adopted.

We have never (intentionally) claimed that. We may have spoken
unclearly, we may have been misinterpreted by others, and we have
always been open for clarification.

Please see my previous email.

> I appreciate this point is realistically true, but look at how Philipp
> presented this 'no break' concept to Christoph as a no-work option for
> him.
>
> My main point here is that I'm glad things are getting straightened
> out, some of the conflicting policy ideas shot down, and the demands
> on maintainers are being articulated more clearly.

Again, to be clear: the approach we have followed has been quite clear
if you actually spoke to us, instead of taking for granted what
unrelated people may have posted.

> There is another we :)

I am not sure what you are trying to achieve here.

Cheers,
Miguel

Re: [PATCH V8 04/14] rust: Add cpumask helpers

[Andreas Hindborg]

"Miguel Ojeda" <miguel.ojeda.sandonis@gmail.com> writes:

> On Fri, Feb 14, 2025 at 10:06 PM Jason Gunthorpe <jgg@nvidia.com> wrote:

[...]

>> As a side note, I don't see how anyone can enact this plan without the
>> support of Linus to do CONFIG_RUST=n builds and put out a non-working
>> rc1. IMHO it is yet unclear if this is real thing or an unproven idea
>> block has that will run into problems.
>
> Please ask Jens and the block layer -- Cc'ing Jens (Andreas and Boqun
> are already Cc'd):
>
>     https://lore.kernel.org/all/593a98c9-baaa-496b-a9a7-c886463722e1@kernel.dk/
>
> Having said that, I am not sure what you mean by -rc1. It is in the
> context of a friendly collaboration -- I assume the intention is that
> Andreas et al. are given enough lead time on new features to fix them
> before the merge window. For fixes, it may be harder, of course. Other
> ideas: they may be able to config out certain parts too; or in the
> worst case, in an emergency, Linus may decide to break Rust. They may
> be able to tell you the details of their plan.

Maybe I can help move the discussion forward by describing how we do
things in block.

In block we (the block subsystem community) currently have the rule that
rust code should not delay shipping a PR. I am not sure how Jens will
enforce this, but I could imagine that if builds start failing by the
time a PR has to be submitted, Jens would just yank rust block code. And
so no fallout of this would reach Linus.

Of course, there may be situations where problems do not surface until
Linux is merging things, but for this to happen without these issues
first appearing in linux-next would be extremely unlikely.

Maybe having a config option to disable rust block might be a good idea,
to prevent the yanking if it ever comes to that.

In practice, we never had any issues. Things have broken a handful of
times, but I usually see it within 24 ours and then I am able to send a
fix.

I can't imagine that Jens has been forced to spend a lot of cycles on
this, outside of applying a few fixes now and again. It would be
interesting to know how much the workload has actually been for him.

Anyways, it is my hope that within a few years, rust will become a fully
qualified citizen in block, and the special rule can be dropped. This of
course requires Jens becoming able and willing to handle rust related
issues himself, or him becoming confident that the current arrangement
will suffice for solving any rust related issues.


Best regards,
Andreas Hindborg

Re: [PATCH V8 04/14] rust: Add cpumask helpers

[Philipp Stanner]

On Fri, 2025-02-14 at 17:06 -0400, Jason Gunthorpe wrote:
> On Fri, Feb 14, 2025 at 09:24:31PM +0100, Miguel Ojeda wrote:
> > On Fri, Feb 14, 2025 at 8:11 PM Jason Gunthorpe <jgg@nvidia.com>
> > wrote:
> > > 
> > > Sure, but it was said, by many people, many times, that "Rust is
> > > allowed to break".
> > 
> > A lot of people have said many things (especially in online fora),
> > and
> > many of those things are contradictory, but that does not really
> > mean
> > anything.
> 
> We don't have a community where there is a clear authority
> structure. If lots of people are repeating a thing, that thing
> usually
> becomes the consensus and common viewpoint regardless of who
> originated it. The repetition reflects community agreement and buy in
> of the idea.
> 
> At the end of the day, only the policy adopted by the people merging
> patches really matters.
> 
> The assumption being if respected people speak with authority on a
> policy they have also got buy in from the people responsible to
> execute it.
> 
> I also think you should merge your policy document to the tree, not
> keep it on a web page. That seems to be a big part of getting
> community agreed policy adopted.
> 
> > Finally, ambiguous terms are used in many cases to refer to
> > different
> > parties: "Rust community", "Rust people", "Rust team", "Rust
> > maintainers"... I have started to ask people to avoid doing that
> > (at
> > least in the LKML), please, and be concrete if possible.
> 
> Okay, that makes lots of seense. Please don't use "we" as well.. I
> have no idea who is included in your "we", or what to even call it.
> 
> > > And Greg's version:
> > > 
> > > https://lore.kernel.org/all/2025013030-gummy-cosmic-7927@gregkh/
> > 
> > I cannot speak for Greg, sorry.
> 
> Yet he does seems to be speaking with authority on this topic. His
> message was quoted on LWN and likely was read by a large number of
> maintainers.
> 
> Is he not part of your "we"?
> 
> > I can read his message in the following ways:
> 
> I think it was very clear, sorry, I don't read it your many ways at
> all.
> 
> >   - I can read it as a general ability of subsystems to potentially
> > agree to treat Rust code as something like staging, like block's
> > plan.
> 
> As a side note, I don't see how anyone can enact this plan without
> the
> support of Linus to do CONFIG_RUST=n builds and put out a non-working
> rc1. IMHO it is yet unclear if this is real thing or an unproven idea
> block has that will run into problems.
> 
> > It is very hard to keep hundreds of maintainers on the same page.
> 
> It is, but also I think you need to take this challenge to succeed.
>  
> > > I do not agree with "Didn't you promise Rust wouldn't be extra
> > > work
> > > for maintainers?" in your document. Clearly there is a widespread
> > > belief this kind of promise was made, even if it was never made
> > > by
> > > you. "Rust is allowed to break" is understood to be the same as
> > > saying
> > > it won't cause extra work.
> > 
> > Sorry, but I have to strongly push back against this paragraph.
> > 
> > Are you really saying that, because people out there may think
> > something, we cannot claim anymore that we did not promise
> > something?
> 
> Again "we" ?
> 
> I'm not concerned about "people out there". Greg said it. Others who
> I
> would think are part of your "we" have posted it on LKML.
> 
> It is not clear at all. If you said Miguel never claimed it, then I
> would not complain. You said it correctly above, be concrete. Ideally
> acknowledge there were different policy ideas in wide circulation,
> but
> they did not get adopted.
> 
> > Furthermore, I don't agree with your assessment in your last
> > sentence
> > at all. Even if it was decided to allow Rust to break globally and
> > at
> > all times, it does not mean Rust is not extra work. 
> 
> I appreciate this point is realistically true, but look at how
> Philipp
> presented this 'no break' concept to Christoph as a no-work option
> for
> him.

Hello,

so to clarify that: I'm (currently) not writing or maintaining any Rust
abstractions. So I'm not representing official projects, so take the
statement for what it's worth. I mainly wanted to probe how an
agreement with Christoph could have been worked out.

My main point still holds up, though:
The rules should be written down. In tree, in linux/

As you, Jason, also state, this would enforce a discussion leading to
more clarity.


> 
> My main point here is that I'm glad things are getting straightened
> out, some of the conflicting policy ideas shot down, and the demands
> on maintainers are being articulated more clearly.
> 
> > That is good, but to be clear, from my point of view, the approach
> > mentioned in the document I wrote is what we have always said.
> 
> There is another we :)

The current, apparent, distinction between Rust and non-Rust kernel
engineers might indeed be a symbol for an underlying big question. 10
years down the road, will there still be "Rust people" who take care
of… the compiler? The abstractions and bindings? Are there "Clang
people and GCC people" currently, too?

That question goes deeper than one might think, because it's ultimately
about who maintains what and, as we already discussed, who can break
what. If, in the mid-future, subsystem maintainers uninvolved with Rust
couldn't merge changes that break Rust anymore, that *might* de facto
indeed lead to them having to learn that language. Or everyone would
need a Rust-proficient co-maintainer who can deal with those breakages;
or similar solutions.

So that's why I think clarifying it rather sooner than later is
necessary.


P.


> 
> Jason
> 

Re: [PATCH V8 04/14] rust: Add cpumask helpers

[Miguel Ojeda]

On Fri, Feb 14, 2025 at 8:11 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
>
> Philipp
> Greg

Since they were mentioned by name, we should Cc them out of courtesy.

Cheers,
Miguel

Re: [PATCH V8 04/14] rust: Add cpumask helpers

[Viresh Kumar]

On 11-02-25, 11:24, Yury Norov wrote:
> To begin with, this is the 8th version of the same patch, but you
> only now bothered to CC someone who is listed in MAINTAINERS. This is
> not how the community works.

Yes, this was my mistake. I assumed get_maintainers would Cc all
relevant people, but I overlooked the fact that these are new files,
so it didn’t include the maintainers. Unfortunately, the same issue
occurred with the clk bindings. Miguel pointed it out at V7, and I
corrected it immediately. There was no intention to bypass anyone.

These bindings had been in my tree since earlier versions, but I
hesitated to post them before V6 because I wasn’t confident in writing
bindings for a framework I didn’t fully understand. I eventually
shared them in V6 to unblock my series but inadvertently missed Cc’ing
few, as mentioned above.

> You also made it a patch bomb that touches multiple critical and very
> sensitive subsystems. You link them to an experimental and unstable
> project, and do it in a way that makes it really easy to slip through
> maintainers' attention.
> 
> Not speaking for others, but please for cpumasks create a separate
> series and start thorough discussion.

Agree, its better to post these separately.

-- 
viresh

Re: [PATCH V8 04/14] rust: Add cpumask helpers

[Andreas Hindborg]

"Viresh Kumar" <viresh.kumar@linaro.org> writes:

> On 11-02-25, 11:24, Yury Norov wrote:
>> To begin with, this is the 8th version of the same patch, but you
>> only now bothered to CC someone who is listed in MAINTAINERS. This is
>> not how the community works.
>
> Yes, this was my mistake. I assumed get_maintainers would Cc all
> relevant people, but I overlooked the fact that these are new files,
> so it didn’t include the maintainers. Unfortunately, the same issue
> occurred with the clk bindings. Miguel pointed it out at V7, and I
> corrected it immediately. There was no intention to bypass anyone.

I have to admit, I have done this as well, for the module_params series.
It feels horrible to commit this action, because it looks either
disrespectful or like an attempt to bypass people to sneak in stuff. But
as clarified this is _never_ the intention. It's simply people working
with a contribution model that they are still learning how to use. The
kernel tooling and process is not exactly intuitive to most people.

Just know that absolutely the largest fraction of contributors (I would
guess all) want to do things the right way. For all contributions, rust
or c, the patches benefit tremendously from reviews from domain experts.
The contributors appreciate this and hope for this to happen.


Best regards,
Andreas Hindborg

[PATCH V8 05/14] rust: Add bindings for cpumask

[Viresh Kumar]

This patch adds basic Rust bindings for struct cpumask.

These will be used by Rust based cpufreq / OPP core.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 MAINTAINERS            |   1 +
 rust/kernel/cpumask.rs | 138 +++++++++++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs     |   1 +
 3 files changed, 140 insertions(+)
 create mode 100644 rust/kernel/cpumask.rs

diff --git a/MAINTAINERS b/MAINTAINERS
index bfc1bf2ebd77..ff4511914e0a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4022,6 +4022,7 @@ F:	lib/find_bit.c
 F:	lib/find_bit_benchmark.c
 F:	lib/test_bitmap.c
 F:	rust/helpers/cpumask.c
+F:	rust/kernel/cpumask.rs
 F:	tools/include/linux/bitfield.h
 F:	tools/include/linux/bitmap.h
 F:	tools/include/linux/bits.h
diff --git a/rust/kernel/cpumask.rs b/rust/kernel/cpumask.rs
new file mode 100644
index 000000000000..b0be8c75a2c2
--- /dev/null
+++ b/rust/kernel/cpumask.rs
@@ -0,0 +1,138 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! CPU mask abstractions.
+//!
+//! C header: [`include/linux/cpumask.h`](srctree/include/linux/cpumask.h)
+
+use crate::{bindings, error::Result, prelude::ENOMEM};
+
+#[cfg(not(CONFIG_CPUMASK_OFFSTACK))]
+use crate::prelude::{KBox, GFP_KERNEL};
+
+#[cfg(CONFIG_CPUMASK_OFFSTACK)]
+use core::ptr;
+
+/// A simple implementation of `struct cpumask` from the C code.
+pub struct Cpumask {
+    ptr: *mut bindings::cpumask,
+    owned: bool,
+}
+
+impl Cpumask {
+    /// Creates empty cpumask.
+    #[cfg(CONFIG_CPUMASK_OFFSTACK)]
+    pub fn new() -> Result<Self> {
+        let mut ptr: *mut bindings::cpumask = ptr::null_mut();
+
+        // SAFETY: Depending on the value of `gfp_flags`, this call may sleep. Other than that, it
+        // is always safe to call this method.
+        if !unsafe { bindings::zalloc_cpumask_var(&mut ptr, bindings::GFP_KERNEL) } {
+            return Err(ENOMEM);
+        }
+
+        Ok(Self { ptr, owned: true })
+    }
+
+    /// Creates empty cpumask.
+    #[cfg(not(CONFIG_CPUMASK_OFFSTACK))]
+    pub fn new() -> Result<Self> {
+        let ptr = KBox::into_raw(KBox::new([bindings::cpumask::default(); 1], GFP_KERNEL)?);
+
+        // SAFETY: Depending on the value of `gfp_flags`, this call may sleep. Other than that, it
+        // is always safe to call this method.
+        if !unsafe { bindings::zalloc_cpumask_var(ptr, bindings::GFP_KERNEL) } {
+            return Err(ENOMEM);
+        }
+
+        Ok(Self {
+            ptr: ptr as *mut _,
+            owned: true,
+        })
+    }
+
+    /// Creates a new abstraction instance of an existing `struct cpumask` pointer.
+    ///
+    /// # Safety
+    ///
+    /// Callers must ensure that `ptr` is valid, and non-null.
+    #[cfg(CONFIG_CPUMASK_OFFSTACK)]
+    pub unsafe fn get_cpumask(ptr: &mut *mut bindings::cpumask) -> Self {
+        Self {
+            ptr: *ptr,
+            owned: false,
+        }
+    }
+
+    /// Creates a new abstraction instance of an existing `struct cpumask` pointer.
+    ///
+    /// # Safety
+    ///
+    /// Callers must ensure that `ptr` is valid, and non-null.
+    #[cfg(not(CONFIG_CPUMASK_OFFSTACK))]
+    pub unsafe fn get_cpumask(ptr: &mut bindings::cpumask_var_t) -> Self {
+        Self {
+            ptr: ptr as *mut _,
+            owned: false,
+        }
+    }
+
+    /// Obtain the raw `struct cpumask *`.
+    pub fn as_raw(&mut self) -> *mut bindings::cpumask {
+        self.ptr
+    }
+
+    /// Sets CPU in the cpumask.
+    ///
+    /// Update the cpumask with a single CPU.
+    pub fn set(&mut self, cpu: u32) {
+        // SAFETY: `ptr` is guaranteed to be valid for the lifetime of `self`. And it is safe to
+        // call `cpumask_set_cpus()` for any CPU.
+        unsafe { bindings::cpumask_set_cpu(cpu, self.ptr) };
+    }
+
+    /// Clears CPU in the cpumask.
+    ///
+    /// Update the cpumask with a single CPU.
+    pub fn clear(&mut self, cpu: i32) {
+        // SAFETY: `ptr` is guaranteed to be valid for the lifetime of `self`. And it is safe to
+        // call `cpumask_clear_cpu()` for any CPU.
+        unsafe { bindings::cpumask_clear_cpu(cpu, self.ptr) };
+    }
+
+    /// Sets all CPUs in the cpumask.
+    pub fn set_all(&mut self) {
+        // SAFETY: `ptr` is guaranteed to be valid for the lifetime of `self`. And it is safe to
+        // call `cpumask_setall()`.
+        unsafe { bindings::cpumask_setall(self.ptr) };
+    }
+
+    /// Gets weight of a cpumask.
+    pub fn weight(&self) -> u32 {
+        // SAFETY: `ptr` is guaranteed to be valid for the lifetime of `self`. And it is safe to
+        // call `cpumask_weight()`.
+        unsafe { bindings::cpumask_weight(self.ptr) }
+    }
+
+    /// Copies cpumask.
+    pub fn copy(&self, dstp: &mut Self) {
+        // SAFETY: `ptr` is guaranteed to be valid for the lifetime of `self`. And it is safe to
+        // call `cpumask_copy()`.
+        unsafe { bindings::cpumask_copy(dstp.as_raw(), self.ptr) };
+    }
+}
+
+impl Drop for Cpumask {
+    fn drop(&mut self) {
+        if self.owned {
+            // SAFETY: `ptr` is guaranteed to be valid for the lifetime of `self`. And it is safe
+            // to call `free_cpumask_var()`.
+            unsafe { bindings::free_cpumask_var(self.ptr) }
+
+            #[cfg(not(CONFIG_CPUMASK_OFFSTACK))]
+            // SAFETY: The pointer was earlier initialized from the result of `KBox::into_raw()`.
+            unsafe {
+                drop(KBox::from_raw(self.ptr))
+            };
+        }
+    }
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 415c500212dd..ccbf7fa087a0 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -41,6 +41,7 @@
 #[doc(hidden)]
 pub mod build_assert;
 pub mod cpu;
+pub mod cpumask;
 pub mod cred;
 pub mod device;
 pub mod device_id;
-- 
2.31.1.272.g89b43f80a514

[PATCH V8 06/14] rust: Add bare minimal bindings for clk framework

[Viresh Kumar]

This adds very basic bindings for the clk framework, implements only
clk_get() and clk_put(). These are the bare minimum bindings required
for many users and are simple enough to add in the first attempt.

These will be used by Rust based cpufreq / OPP core to begin with.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 MAINTAINERS                     |  1 +
 rust/bindings/bindings_helper.h |  1 +
 rust/kernel/clk.rs              | 48 +++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs              |  2 ++
 4 files changed, 52 insertions(+)
 create mode 100644 rust/kernel/clk.rs

diff --git a/MAINTAINERS b/MAINTAINERS
index ff4511914e0a..604717065476 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5780,6 +5780,7 @@ F:	include/dt-bindings/clock/
 F:	include/linux/clk-pr*
 F:	include/linux/clk/
 F:	include/linux/of_clk.h
+F:	rust/kernel/clk.rs
 X:	drivers/clk/clkdev.c
 
 COMMON INTERNET FILE SYSTEM CLIENT (CIFS and SMB3)
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 59b4bc49d039..4eadcf645df0 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -10,6 +10,7 @@
 #include <linux/blk-mq.h>
 #include <linux/blk_types.h>
 #include <linux/blkdev.h>
+#include <linux/clk.h>
 #include <linux/cpu.h>
 #include <linux/cpumask.h>
 #include <linux/cred.h>
diff --git a/rust/kernel/clk.rs b/rust/kernel/clk.rs
new file mode 100644
index 000000000000..123cdb43b115
--- /dev/null
+++ b/rust/kernel/clk.rs
@@ -0,0 +1,48 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Clock abstractions.
+//!
+//! C header: [`include/linux/clk.h`](srctree/include/linux/clk.h)
+
+use crate::{
+    bindings,
+    device::Device,
+    error::{from_err_ptr, Result},
+    prelude::*,
+};
+
+use core::ptr;
+
+/// A simple implementation of `struct clk` from the C code.
+#[repr(transparent)]
+pub struct Clk(*mut bindings::clk);
+
+impl Clk {
+    /// Creates `Clk` instance for a device and a connection id.
+    pub fn new(dev: &Device, name: Option<&CStr>) -> Result<Self> {
+        let con_id = if let Some(name) = name {
+            name.as_ptr() as *const _
+        } else {
+            ptr::null()
+        };
+
+        // SAFETY: It is safe to call `clk_get()`, on a device pointer earlier received from the C
+        // code.
+        Ok(Self(from_err_ptr(unsafe {
+            bindings::clk_get(dev.as_raw(), con_id)
+        })?))
+    }
+
+    /// Obtain the raw `struct clk *`.
+    pub fn as_raw(&self) -> *mut bindings::clk {
+        self.0
+    }
+}
+
+impl Drop for Clk {
+    fn drop(&mut self) {
+        // SAFETY: By the type invariants, we know that `self` owns a reference, so it is safe to
+        // relinquish it now.
+        unsafe { bindings::clk_put(self.0) };
+    }
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index ccbf7fa087a0..77d3b1f82154 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -40,6 +40,8 @@
 pub mod block;
 #[doc(hidden)]
 pub mod build_assert;
+#[cfg(CONFIG_COMMON_CLK)]
+pub mod clk;
 pub mod cpu;
 pub mod cpumask;
 pub mod cred;
-- 
2.31.1.272.g89b43f80a514

Re: [PATCH V8 06/14] rust: Add bare minimal bindings for clk framework

[Danilo Krummrich]

On Thu, Feb 06, 2025 at 02:58:27PM +0530, Viresh Kumar wrote:
> This adds very basic bindings for the clk framework, implements only
> clk_get() and clk_put(). These are the bare minimum bindings required
> for many users and are simple enough to add in the first attempt.
> 
> These will be used by Rust based cpufreq / OPP core to begin with.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  MAINTAINERS                     |  1 +
>  rust/bindings/bindings_helper.h |  1 +
>  rust/kernel/clk.rs              | 48 +++++++++++++++++++++++++++++++++
>  rust/kernel/lib.rs              |  2 ++
>  4 files changed, 52 insertions(+)
>  create mode 100644 rust/kernel/clk.rs
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index ff4511914e0a..604717065476 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -5780,6 +5780,7 @@ F:	include/dt-bindings/clock/
>  F:	include/linux/clk-pr*
>  F:	include/linux/clk/
>  F:	include/linux/of_clk.h
> +F:	rust/kernel/clk.rs
>  X:	drivers/clk/clkdev.c
>  
>  COMMON INTERNET FILE SYSTEM CLIENT (CIFS and SMB3)
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index 59b4bc49d039..4eadcf645df0 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -10,6 +10,7 @@
>  #include <linux/blk-mq.h>
>  #include <linux/blk_types.h>
>  #include <linux/blkdev.h>
> +#include <linux/clk.h>
>  #include <linux/cpu.h>
>  #include <linux/cpumask.h>
>  #include <linux/cred.h>
> diff --git a/rust/kernel/clk.rs b/rust/kernel/clk.rs
> new file mode 100644
> index 000000000000..123cdb43b115
> --- /dev/null
> +++ b/rust/kernel/clk.rs
> @@ -0,0 +1,48 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Clock abstractions.
> +//!
> +//! C header: [`include/linux/clk.h`](srctree/include/linux/clk.h)
> +
> +use crate::{
> +    bindings,
> +    device::Device,
> +    error::{from_err_ptr, Result},
> +    prelude::*,
> +};
> +
> +use core::ptr;
> +
> +/// A simple implementation of `struct clk` from the C code.
> +#[repr(transparent)]
> +pub struct Clk(*mut bindings::clk);

Guess this should be Opaque<bindings::clk>.

> +
> +impl Clk {
> +    /// Creates `Clk` instance for a device and a connection id.
> +    pub fn new(dev: &Device, name: Option<&CStr>) -> Result<Self> {
> +        let con_id = if let Some(name) = name {
> +            name.as_ptr() as *const _
> +        } else {
> +            ptr::null()
> +        };
> +
> +        // SAFETY: It is safe to call `clk_get()`, on a device pointer earlier received from the C
> +        // code.
> +        Ok(Self(from_err_ptr(unsafe {
> +            bindings::clk_get(dev.as_raw(), con_id)
> +        })?))
> +    }
> +
> +    /// Obtain the raw `struct clk *`.
> +    pub fn as_raw(&self) -> *mut bindings::clk {
> +        self.0
> +    }
> +}
> +
> +impl Drop for Clk {
> +    fn drop(&mut self) {
> +        // SAFETY: By the type invariants, we know that `self` owns a reference, so it is safe to
> +        // relinquish it now.
> +        unsafe { bindings::clk_put(self.0) };
> +    }
> +}
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index ccbf7fa087a0..77d3b1f82154 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -40,6 +40,8 @@
>  pub mod block;
>  #[doc(hidden)]
>  pub mod build_assert;
> +#[cfg(CONFIG_COMMON_CLK)]
> +pub mod clk;
>  pub mod cpu;
>  pub mod cpumask;
>  pub mod cred;
> -- 
> 2.31.1.272.g89b43f80a514
> 
> 

Re: [PATCH V8 06/14] rust: Add bare minimal bindings for clk framework

[Danilo Krummrich]

On Thu, Feb 06, 2025 at 12:49:14PM +0100, Danilo Krummrich wrote:
> On Thu, Feb 06, 2025 at 02:58:27PM +0530, Viresh Kumar wrote:
> > This adds very basic bindings for the clk framework, implements only
> > clk_get() and clk_put(). These are the bare minimum bindings required
> > for many users and are simple enough to add in the first attempt.
> > 
> > These will be used by Rust based cpufreq / OPP core to begin with.
> > 
> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > ---
> >  MAINTAINERS                     |  1 +
> >  rust/bindings/bindings_helper.h |  1 +
> >  rust/kernel/clk.rs              | 48 +++++++++++++++++++++++++++++++++
> >  rust/kernel/lib.rs              |  2 ++
> >  4 files changed, 52 insertions(+)
> >  create mode 100644 rust/kernel/clk.rs
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index ff4511914e0a..604717065476 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -5780,6 +5780,7 @@ F:	include/dt-bindings/clock/
> >  F:	include/linux/clk-pr*
> >  F:	include/linux/clk/
> >  F:	include/linux/of_clk.h
> > +F:	rust/kernel/clk.rs
> >  X:	drivers/clk/clkdev.c
> >  
> >  COMMON INTERNET FILE SYSTEM CLIENT (CIFS and SMB3)
> > diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> > index 59b4bc49d039..4eadcf645df0 100644
> > --- a/rust/bindings/bindings_helper.h
> > +++ b/rust/bindings/bindings_helper.h
> > @@ -10,6 +10,7 @@
> >  #include <linux/blk-mq.h>
> >  #include <linux/blk_types.h>
> >  #include <linux/blkdev.h>
> > +#include <linux/clk.h>
> >  #include <linux/cpu.h>
> >  #include <linux/cpumask.h>
> >  #include <linux/cred.h>
> > diff --git a/rust/kernel/clk.rs b/rust/kernel/clk.rs
> > new file mode 100644
> > index 000000000000..123cdb43b115
> > --- /dev/null
> > +++ b/rust/kernel/clk.rs
> > @@ -0,0 +1,48 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +//! Clock abstractions.
> > +//!
> > +//! C header: [`include/linux/clk.h`](srctree/include/linux/clk.h)
> > +
> > +use crate::{
> > +    bindings,
> > +    device::Device,
> > +    error::{from_err_ptr, Result},
> > +    prelude::*,
> > +};
> > +
> > +use core::ptr;
> > +
> > +/// A simple implementation of `struct clk` from the C code.
> > +#[repr(transparent)]
> > +pub struct Clk(*mut bindings::clk);
> 
> Guess this should be Opaque<bindings::clk>.

Sorry, I meant NonNull<bindings::clk>.

> 
> > +
> > +impl Clk {
> > +    /// Creates `Clk` instance for a device and a connection id.
> > +    pub fn new(dev: &Device, name: Option<&CStr>) -> Result<Self> {
> > +        let con_id = if let Some(name) = name {
> > +            name.as_ptr() as *const _
> > +        } else {
> > +            ptr::null()
> > +        };
> > +
> > +        // SAFETY: It is safe to call `clk_get()`, on a device pointer earlier received from the C
> > +        // code.
> > +        Ok(Self(from_err_ptr(unsafe {
> > +            bindings::clk_get(dev.as_raw(), con_id)
> > +        })?))
> > +    }
> > +
> > +    /// Obtain the raw `struct clk *`.
> > +    pub fn as_raw(&self) -> *mut bindings::clk {
> > +        self.0
> > +    }
> > +}
> > +
> > +impl Drop for Clk {
> > +    fn drop(&mut self) {
> > +        // SAFETY: By the type invariants, we know that `self` owns a reference, so it is safe to
> > +        // relinquish it now.
> > +        unsafe { bindings::clk_put(self.0) };
> > +    }
> > +}
> > diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> > index ccbf7fa087a0..77d3b1f82154 100644
> > --- a/rust/kernel/lib.rs
> > +++ b/rust/kernel/lib.rs
> > @@ -40,6 +40,8 @@
> >  pub mod block;
> >  #[doc(hidden)]
> >  pub mod build_assert;
> > +#[cfg(CONFIG_COMMON_CLK)]
> > +pub mod clk;
> >  pub mod cpu;
> >  pub mod cpumask;
> >  pub mod cred;
> > -- 
> > 2.31.1.272.g89b43f80a514
> > 
> > 

Re: [PATCH V8 06/14] rust: Add bare minimal bindings for clk framework

[Stephen Boyd]

Quoting Danilo Krummrich (2025-02-06 03:52:41)
> On Thu, Feb 06, 2025 at 12:49:14PM +0100, Danilo Krummrich wrote:
> > On Thu, Feb 06, 2025 at 02:58:27PM +0530, Viresh Kumar wrote:
> > > diff --git a/rust/kernel/clk.rs b/rust/kernel/clk.rs
> > > new file mode 100644
> > > index 000000000000..123cdb43b115
> > > --- /dev/null
> > > +++ b/rust/kernel/clk.rs
> > > @@ -0,0 +1,48 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +
> > > +//! Clock abstractions.
> > > +//!
> > > +//! C header: [`include/linux/clk.h`](srctree/include/linux/clk.h)
> > > +
> > > +use crate::{
> > > +    bindings,
> > > +    device::Device,
> > > +    error::{from_err_ptr, Result},
> > > +    prelude::*,
> > > +};
> > > +
> > > +use core::ptr;
> > > +
> > > +/// A simple implementation of `struct clk` from the C code.
> > > +#[repr(transparent)]
> > > +pub struct Clk(*mut bindings::clk);
> > 
> > Guess this should be Opaque<bindings::clk>.
> 
> Sorry, I meant NonNull<bindings::clk>.

NULL is a valid clk. It's like "don't care" in the common clk framework
as most clk consumer operations bail out early in that case.

Re: [PATCH V8 06/14] rust: Add bare minimal bindings for clk framework

[Danilo Krummrich]

On Thu, Feb 06, 2025 at 12:05:59PM -0800, Stephen Boyd wrote:
> Quoting Danilo Krummrich (2025-02-06 03:52:41)
> > On Thu, Feb 06, 2025 at 12:49:14PM +0100, Danilo Krummrich wrote:
> > > On Thu, Feb 06, 2025 at 02:58:27PM +0530, Viresh Kumar wrote:
> > > > diff --git a/rust/kernel/clk.rs b/rust/kernel/clk.rs
> > > > new file mode 100644
> > > > index 000000000000..123cdb43b115
> > > > --- /dev/null
> > > > +++ b/rust/kernel/clk.rs
> > > > @@ -0,0 +1,48 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +
> > > > +//! Clock abstractions.
> > > > +//!
> > > > +//! C header: [`include/linux/clk.h`](srctree/include/linux/clk.h)
> > > > +
> > > > +use crate::{
> > > > +    bindings,
> > > > +    device::Device,
> > > > +    error::{from_err_ptr, Result},
> > > > +    prelude::*,
> > > > +};
> > > > +
> > > > +use core::ptr;
> > > > +
> > > > +/// A simple implementation of `struct clk` from the C code.
> > > > +#[repr(transparent)]
> > > > +pub struct Clk(*mut bindings::clk);
> > > 
> > > Guess this should be Opaque<bindings::clk>.
> > 
> > Sorry, I meant NonNull<bindings::clk>.
> 
> NULL is a valid clk. It's like "don't care" in the common clk framework

Thanks for clarifying!

> as most clk consumer operations bail out early in that case.

Most? Does that mean NULL isn't *always* valid?

Re: [PATCH V8 06/14] rust: Add bare minimal bindings for clk framework

[Viresh Kumar]

On 07-02-25, 00:11, Danilo Krummrich wrote:
> On Thu, Feb 06, 2025 at 12:05:59PM -0800, Stephen Boyd wrote:
> > Quoting Danilo Krummrich (2025-02-06 03:52:41)
> > > On Thu, Feb 06, 2025 at 12:49:14PM +0100, Danilo Krummrich wrote:
> > > > On Thu, Feb 06, 2025 at 02:58:27PM +0530, Viresh Kumar wrote:

> > > > > +/// A simple implementation of `struct clk` from the C code.
> > > > > +#[repr(transparent)]
> > > > > +pub struct Clk(*mut bindings::clk);
> > > > 
> > > > Guess this should be Opaque<bindings::clk>.
> > > 
> > > Sorry, I meant NonNull<bindings::clk>.
> > 
> > NULL is a valid clk. It's like "don't care" in the common clk framework
> 
> Thanks for clarifying!

> Guess this should be Opaque<bindings::clk>.

So it should be this now ?

-- 
viresh

Re: [PATCH V8 06/14] rust: Add bare minimal bindings for clk framework

[Viresh Kumar]

On 07-02-25, 14:54, Viresh Kumar wrote:
> On 07-02-25, 00:11, Danilo Krummrich wrote:
> > Guess this should be Opaque<bindings::clk>.
> 
> So it should be this now ?

Also, I should be using ARef and AlwaysRefCounted along with that ? I
am not sure if I can use those with the clk API. Yes, it is refcounted
but there is no direct way of incrementing the refcount unlike other
frameworks. clk_get() accepts a dev pointer and a char pointer,
whereas clk_put() accepts the clk pointer itself.

-- 
viresh

Re: [PATCH V8 06/14] rust: Add bare minimal bindings for clk framework

[Danilo Krummrich]

On 2/7/25 10:24 AM, Viresh Kumar wrote:
> On 07-02-25, 00:11, Danilo Krummrich wrote:
>> On Thu, Feb 06, 2025 at 12:05:59PM -0800, Stephen Boyd wrote:
>>> Quoting Danilo Krummrich (2025-02-06 03:52:41)
>>>> On Thu, Feb 06, 2025 at 12:49:14PM +0100, Danilo Krummrich wrote:
>>>>> On Thu, Feb 06, 2025 at 02:58:27PM +0530, Viresh Kumar wrote:
> 
>>>>>> +/// A simple implementation of `struct clk` from the C code.
>>>>>> +#[repr(transparent)]
>>>>>> +pub struct Clk(*mut bindings::clk);
>>>>>
>>>>> Guess this should be Opaque<bindings::clk>.
>>>>
>>>> Sorry, I meant NonNull<bindings::clk>.
>>>
>>> NULL is a valid clk. It's like "don't care" in the common clk framework
>>
>> Thanks for clarifying!
> 
>> Guess this should be Opaque<bindings::clk>.
> 
> So it should be this now ?

I actually meant NonNull<bindings::clk>, which I corrected in a subsequent mail,
where Stephen pointed out that NULL is a valid value for a struct clk.

Re: [PATCH V8 06/14] rust: Add bare minimal bindings for clk framework

[Viresh Kumar]

On 07-02-25, 18:19, Danilo Krummrich wrote:
> On 2/7/25 10:24 AM, Viresh Kumar wrote:
> > On 07-02-25, 00:11, Danilo Krummrich wrote:
> > > On Thu, Feb 06, 2025 at 12:05:59PM -0800, Stephen Boyd wrote:
> > > > Quoting Danilo Krummrich (2025-02-06 03:52:41)
> > > > > On Thu, Feb 06, 2025 at 12:49:14PM +0100, Danilo Krummrich wrote:
> > > > > > On Thu, Feb 06, 2025 at 02:58:27PM +0530, Viresh Kumar wrote:
> > 
> > > > > > > +/// A simple implementation of `struct clk` from the C code.
> > > > > > > +#[repr(transparent)]
> > > > > > > +pub struct Clk(*mut bindings::clk);
> > > > > > 
> > > > > > Guess this should be Opaque<bindings::clk>.
> > > > > 
> > > > > Sorry, I meant NonNull<bindings::clk>.
> > > > 
> > > > NULL is a valid clk. It's like "don't care" in the common clk framework
> > > 
> > > Thanks for clarifying!
> > 
> > > Guess this should be Opaque<bindings::clk>.
> > 
> > So it should be this now ?
> 
> I actually meant NonNull<bindings::clk>, which I corrected in a subsequent mail,
> where Stephen pointed out that NULL is a valid value for a struct clk.

Ahh okay, so no changes required now. Thanks.

-- 
viresh

Re: [PATCH V8 06/14] rust: Add bare minimal bindings for clk framework

[Stephen Boyd]

Quoting Danilo Krummrich (2025-02-06 15:11:31)
> On Thu, Feb 06, 2025 at 12:05:59PM -0800, Stephen Boyd wrote:
> 
> > as most clk consumer operations bail out early in that case.
> 
> Most? Does that mean NULL isn't *always* valid?

I'm hedging because the common clk framework is just one implementation
of the clk.h API in the kernel. We still have a couple other
implementations (sadly) where I haven't checked to see what they do if a
NULL pointer is passed in. But NULL should always be a valid clk handle
per the clk.h API documentation, so an implementation that fails at that
is broken.

Re: [PATCH V8 06/14] rust: Add bare minimal bindings for clk framework

[Daniel Almeida]

Hi Viresh

> On 6 Feb 2025, at 06:28, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> 
> This adds very basic bindings for the clk framework, implements only
> clk_get() and clk_put(). These are the bare minimum bindings required
> for many users and are simple enough to add in the first attempt.

I am missing clk_prepare_enable/clk_disable_unprepare.

Otherwise I see no way of enabling and disabling clks. IMHO I would also
consider these as “bare minimum”.

— Daniel

Re: [PATCH V8 06/14] rust: Add bare minimal bindings for clk framework

[Viresh Kumar]

On 17-02-25, 09:19, Daniel Almeida wrote:
> Hi Viresh
> 
> > On 6 Feb 2025, at 06:28, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > 
> > This adds very basic bindings for the clk framework, implements only
> > clk_get() and clk_put(). These are the bare minimum bindings required
> > for many users and are simple enough to add in the first attempt.
> 
> I am missing clk_prepare_enable/clk_disable_unprepare.
> 
> Otherwise I see no way of enabling and disabling clks. IMHO I would also
> consider these as “bare minimum”.

I have posted the clk bindings separately now:

https://lore.kernel.org/all/cover.1740118863.git.viresh.kumar@linaro.org/

-- 
viresh

[PATCH V8 07/14] rust: Add initial bindings for OPP framework

[Viresh Kumar]

This commit adds initial Rust bindings for the Operating performance
points (OPP) core. This adds bindings for struct dev_pm_opp and
struct dev_pm_opp_data to begin with.

Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 MAINTAINERS                     |   1 +
 rust/bindings/bindings_helper.h |   1 +
 rust/kernel/lib.rs              |   2 +
 rust/kernel/opp.rs              | 189 ++++++++++++++++++++++++++++++++
 4 files changed, 193 insertions(+)
 create mode 100644 rust/kernel/opp.rs

diff --git a/MAINTAINERS b/MAINTAINERS
index 604717065476..cda0d2b74e29 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -17732,6 +17732,7 @@ F:	Documentation/devicetree/bindings/opp/
 F:	Documentation/power/opp.rst
 F:	drivers/opp/
 F:	include/linux/pm_opp.h
+F:	rust/kernel/opp.rs
 
 OPL4 DRIVER
 M:	Clemens Ladisch <clemens@ladisch.de>
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 4eadcf645df0..7f851d5907af 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -28,6 +28,7 @@
 #include <linux/phy.h>
 #include <linux/pid_namespace.h>
 #include <linux/platform_device.h>
+#include <linux/pm_opp.h>
 #include <linux/poll.h>
 #include <linux/property.h>
 #include <linux/refcount.h>
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 77d3b1f82154..8956f78a2805 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -64,6 +64,8 @@
 #[cfg(CONFIG_NET)]
 pub mod net;
 pub mod of;
+#[cfg(CONFIG_PM_OPP)]
+pub mod opp;
 pub mod page;
 #[cfg(CONFIG_PCI)]
 pub mod pci;
diff --git a/rust/kernel/opp.rs b/rust/kernel/opp.rs
new file mode 100644
index 000000000000..becb33880c92
--- /dev/null
+++ b/rust/kernel/opp.rs
@@ -0,0 +1,189 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Operating performance points.
+//!
+//! This module provides bindings for interacting with the OPP subsystem.
+//!
+//! C header: [`include/linux/pm_opp.h`](srctree/include/linux/pm_opp.h)
+
+use crate::{
+    bindings,
+    device::Device,
+    error::{code::*, to_result, Result},
+    types::{ARef, AlwaysRefCounted, Opaque},
+};
+
+use core::ptr;
+
+/// Dynamically created Operating performance point (OPP).
+pub struct Token {
+    dev: ARef<Device>,
+    freq: usize,
+}
+
+impl Token {
+    /// Adds an OPP dynamically.
+    pub fn new(dev: &ARef<Device>, mut data: Data) -> Result<Self> {
+        // SAFETY: The requirements are satisfied by the existence of `Device` and its safety
+        // requirements.
+        to_result(unsafe { bindings::dev_pm_opp_add_dynamic(dev.as_raw(), &mut data.0) })?;
+        Ok(Self {
+            dev: dev.clone(),
+            freq: data.freq(),
+        })
+    }
+}
+
+impl Drop for Token {
+    fn drop(&mut self) {
+        // SAFETY: The requirements are satisfied by the existence of `Device` and its safety
+        // requirements.
+        unsafe { bindings::dev_pm_opp_remove(self.dev.as_raw(), self.freq) };
+    }
+}
+
+/// Equivalent to `struct dev_pm_opp_data` in the C Code.
+#[repr(transparent)]
+pub struct Data(bindings::dev_pm_opp_data);
+
+impl Data {
+    /// Creates new instance of [`Data`].
+    pub fn new(freq: usize, u_volt: usize, level: u32, turbo: bool) -> Self {
+        Self(bindings::dev_pm_opp_data {
+            turbo,
+            freq,
+            u_volt,
+            level,
+        })
+    }
+
+    /// Adds an OPP dynamically. The OPP is freed once the [`Token`] gets freed.
+    pub fn add_opp(self, dev: &ARef<Device>) -> Result<Token> {
+        Token::new(dev, self)
+    }
+
+    fn freq(&self) -> usize {
+        self.0.freq
+    }
+}
+
+/// Operating performance point (OPP).
+///
+/// Wraps the kernel's `struct dev_pm_opp`.
+///
+/// The pointer to `struct dev_pm_opp` is non-null and valid for the lifetime of the `OPP`
+/// instance.
+///
+/// # Invariants
+///
+/// Instances of this type are reference-counted. The reference count is incremented by the
+/// `dev_pm_opp_get()` function and decremented by `dev_pm_opp_put`. The Rust type `ARef<OPP>`
+/// represents a pointer that owns a reference count on the OPP.
+///
+/// A reference to the `OPP`, `&OPP` isn't refcounted by the Rust code.
+
+#[repr(transparent)]
+pub struct OPP(Opaque<bindings::dev_pm_opp>);
+
+// SAFETY: It's OK to send the ownership of `OPP` across thread boundaries.
+unsafe impl Send for OPP {}
+
+// SAFETY: It's OK to access `OPP` through shared references from other threads because we're
+// either accessing properties that don't change or that are properly synchronised by C code.
+unsafe impl Sync for OPP {}
+
+// SAFETY: The type invariants guarantee that [`OPP`] is always refcounted.
+unsafe impl AlwaysRefCounted for OPP {
+    fn inc_ref(&self) {
+        // SAFETY: The existence of a shared reference means that the refcount is nonzero.
+        unsafe { bindings::dev_pm_opp_get(self.0.get()) };
+    }
+
+    unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
+        // SAFETY: The safety requirements guarantee that the refcount is nonzero.
+        unsafe { bindings::dev_pm_opp_put(obj.cast().as_ptr()) }
+    }
+}
+
+impl OPP {
+    /// Creates an owned reference to a [`OPP`] from a valid pointer.
+    ///
+    /// The refcount is incremented by the C code and will be decremented by `dec_ref()` when the
+    /// ARef object is dropped.
+    ///
+    /// # Safety
+    ///
+    /// The caller must ensure that `ptr` is valid and the OPP's refcount is incremented. The
+    /// caller must also ensure that it doesn't explicitly drop the refcount of the OPP, as the
+    /// returned ARef object takes over the refcount increment on the underlying object and the
+    /// same will be dropped along with it.
+    pub unsafe fn from_raw_opp_owned(ptr: *mut bindings::dev_pm_opp) -> Result<ARef<Self>> {
+        let ptr = ptr::NonNull::new(ptr).ok_or(ENODEV)?;
+
+        // SAFETY: The safety requirements guarantee the validity of the pointer.
+        Ok(unsafe { ARef::from_raw(ptr.cast()) })
+    }
+
+    /// Creates a reference to a [`OPP`] from a valid pointer.
+    ///
+    /// The refcount is not updated by the Rust API unless the returned reference is converted to
+    /// an ARef object.
+    ///
+    /// # Safety
+    ///
+    /// The caller must ensure that `ptr` is valid and remains valid for the duration of 'a.
+    pub unsafe fn from_raw_opp<'a>(ptr: *mut bindings::dev_pm_opp) -> Result<&'a Self> {
+        // SAFETY: The caller guarantees that the pointer is not dangling and stays valid for the
+        // duration of 'a. The cast is okay because `OPP` is `repr(transparent)`.
+        Ok(unsafe { &*ptr.cast() })
+    }
+
+    #[inline]
+    fn as_raw(&self) -> *mut bindings::dev_pm_opp {
+        self.0.get()
+    }
+
+    /// Returns the frequency of an OPP.
+    pub fn freq(&self, index: Option<u32>) -> usize {
+        let index = index.unwrap_or(0);
+
+        // SAFETY: By the type invariants, we know that `self` owns a reference, so it is safe to
+        // use it.
+        unsafe { bindings::dev_pm_opp_get_freq_indexed(self.as_raw(), index) }
+    }
+
+    /// Returns the voltage of an OPP.
+    pub fn voltage(&self) -> usize {
+        // SAFETY: By the type invariants, we know that `self` owns a reference, so it is safe to
+        // use it.
+        unsafe { bindings::dev_pm_opp_get_voltage(self.as_raw()) }
+    }
+
+    /// Returns the level of an OPP.
+    pub fn level(&self) -> u32 {
+        // SAFETY: By the type invariants, we know that `self` owns a reference, so it is safe to
+        // use it.
+        unsafe { bindings::dev_pm_opp_get_level(self.as_raw()) }
+    }
+
+    /// Returns the power of an OPP.
+    pub fn power(&self) -> usize {
+        // SAFETY: By the type invariants, we know that `self` owns a reference, so it is safe to
+        // use it.
+        unsafe { bindings::dev_pm_opp_get_power(self.as_raw()) }
+    }
+
+    /// Returns the required pstate of an OPP.
+    pub fn required_pstate(&self, index: u32) -> u32 {
+        // SAFETY: By the type invariants, we know that `self` owns a reference, so it is safe to
+        // use it.
+        unsafe { bindings::dev_pm_opp_get_required_pstate(self.as_raw(), index) }
+    }
+
+    /// Returns true if the OPP is turbo.
+    pub fn is_turbo(&self) -> bool {
+        // SAFETY: By the type invariants, we know that `self` owns a reference, so it is safe to
+        // use it.
+        unsafe { bindings::dev_pm_opp_is_turbo(self.as_raw()) }
+    }
+}
-- 
2.31.1.272.g89b43f80a514

[PATCH V8 08/14] rust: Extend OPP bindings for the OPP table

[Viresh Kumar]

This extends OPP bindings with the bindings for the struct opp_table.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 rust/kernel/opp.rs | 382 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 381 insertions(+), 1 deletion(-)

diff --git a/rust/kernel/opp.rs b/rust/kernel/opp.rs
index becb33880c92..c0daeadfb188 100644
--- a/rust/kernel/opp.rs
+++ b/rust/kernel/opp.rs
@@ -8,8 +8,9 @@
 
 use crate::{
     bindings,
+    cpumask::Cpumask,
     device::Device,
-    error::{code::*, to_result, Result},
+    error::{code::*, from_err_ptr, to_result, Error, Result},
     types::{ARef, AlwaysRefCounted, Opaque},
 };
 
@@ -67,6 +68,385 @@ fn freq(&self) -> usize {
     }
 }
 
+/// OPP search types.
+#[derive(Copy, Clone, Debug, Eq, PartialEq)]
+pub enum SearchType {
+    /// Search for exact value.
+    Exact,
+    /// Search for highest value less than equal to value.
+    Floor,
+    /// Search for lowest value greater than equal to value.
+    Ceil,
+}
+
+/// Operating performance point (OPP) table.
+///
+/// Wraps the kernel's `struct opp_table`.
+///
+/// The pointer stored in `Self` is non-null and valid for the lifetime of the `Table`.
+pub struct Table {
+    ptr: *mut bindings::opp_table,
+    dev: ARef<Device>,
+    em: bool,
+    of: bool,
+    cpumask: Option<Cpumask>,
+}
+
+// SAFETY: It is okay to send ownership of `Table` across thread boundaries.
+unsafe impl Send for Table {}
+
+// SAFETY: It's OK to access `Table` through shared references from other threads because we're
+// either accessing properties that don't change or that are properly synchronised by C code.
+unsafe impl Sync for Table {}
+
+impl Table {
+    /// Creates a new OPP table instance from raw pointer.
+    ///
+    /// # Safety
+    ///
+    /// Callers must ensure that `ptr` is valid and non-null.
+    unsafe fn from_raw_table(ptr: *mut bindings::opp_table, dev: &ARef<Device>) -> Self {
+        // SAFETY: By the safety requirements, ptr is valid and its refcount will be incremented.
+        unsafe { bindings::dev_pm_opp_get_opp_table_ref(ptr) };
+
+        Self {
+            ptr,
+            dev: dev.clone(),
+            em: false,
+            of: false,
+            cpumask: None,
+        }
+    }
+
+    /// Find OPP table from device.
+    pub fn from_dev(dev: &Device) -> Result<Self> {
+        // SAFETY: The requirements are satisfied by the existence of `Device` and its safety
+        // requirements. Refcount of the OPP table is incremented by the C code.
+        let ptr = from_err_ptr(unsafe { bindings::dev_pm_opp_get_opp_table(dev.as_raw()) })?;
+
+        Ok(Self {
+            ptr,
+            dev: dev.into(),
+            em: false,
+            of: false,
+            cpumask: None,
+        })
+    }
+
+    /// Add device tree based OPP table for the device.
+    #[cfg(CONFIG_OF)]
+    pub fn from_of(dev: &ARef<Device>, index: i32) -> Result<Self> {
+        // SAFETY: The requirements are satisfied by the existence of `Device` and its safety
+        // requirements. Refcount of the OPP table is incremented by the C code.
+        to_result(unsafe { bindings::dev_pm_opp_of_add_table_indexed(dev.as_raw(), index) })?;
+
+        // Fetch the newly created table.
+        let mut table = Self::from_dev(dev)?;
+        table.of = true;
+
+        Ok(table)
+    }
+
+    // Remove device tree based OPP table for the device.
+    #[cfg(CONFIG_OF)]
+    fn remove_of(&self) {
+        // SAFETY: The requirements are satisfied by the existence of `Device` and its safety
+        // requirements. We took the reference earlier from `from_of` earlier, it is safe to drop
+        // the same now.
+        unsafe { bindings::dev_pm_opp_of_remove_table(self.dev.as_raw()) };
+    }
+
+    /// Add device tree based OPP table for CPU devices.
+    #[cfg(CONFIG_OF)]
+    pub fn from_of_cpumask(dev: &Device, cpumask: &mut Cpumask) -> Result<Self> {
+        // SAFETY: The cpumask is valid and the returned ptr will be owned by the [`Table`]
+        // instance.
+        to_result(unsafe { bindings::dev_pm_opp_of_cpumask_add_table(cpumask.as_raw()) })?;
+
+        // Fetch the newly created table.
+        let mut table = Self::from_dev(dev)?;
+
+        let mut mask = Cpumask::new()?;
+        cpumask.copy(&mut mask);
+        table.cpumask = Some(mask);
+
+        Ok(table)
+    }
+
+    // Remove device tree based OPP table for CPU devices.
+    #[cfg(CONFIG_OF)]
+    fn remove_of_cpumask(&self, mut cpumask: Cpumask) {
+        // SAFETY: The cpumask is valid and we took the reference from `from_of_cpumask` earlier,
+        // it is safe to drop the same now.
+        unsafe { bindings::dev_pm_opp_of_cpumask_remove_table(cpumask.as_raw()) };
+    }
+
+    /// Returns the number of OPPs in the table.
+    pub fn opp_count(&self) -> Result<u32> {
+        // SAFETY: The requirements are satisfied by the existence of `Device` and its safety
+        // requirements.
+        let ret = unsafe { bindings::dev_pm_opp_get_opp_count(self.dev.as_raw()) };
+        if ret < 0 {
+            Err(Error::from_errno(ret))
+        } else {
+            Ok(ret as u32)
+        }
+    }
+
+    /// Returns max clock latency of the OPPs in the table.
+    pub fn max_clock_latency(&self) -> usize {
+        // SAFETY: The requirements are satisfied by the existence of `Device` and its safety
+        // requirements.
+        unsafe { bindings::dev_pm_opp_get_max_clock_latency(self.dev.as_raw()) }
+    }
+
+    /// Returns max volt latency of the OPPs in the table.
+    pub fn max_volt_latency(&self) -> usize {
+        // SAFETY: The requirements are satisfied by the existence of `Device` and its safety
+        // requirements.
+        unsafe { bindings::dev_pm_opp_get_max_volt_latency(self.dev.as_raw()) }
+    }
+
+    /// Returns max transition latency of the OPPs in the table.
+    pub fn max_transition_latency(&self) -> usize {
+        // SAFETY: The requirements are satisfied by the existence of `Device` and its safety
+        // requirements.
+        unsafe { bindings::dev_pm_opp_get_max_transition_latency(self.dev.as_raw()) }
+    }
+
+    /// Returns the suspend OPP.
+    pub fn suspend_freq(&self) -> usize {
+        // SAFETY: The requirements are satisfied by the existence of `Device` and its safety
+        // requirements.
+        unsafe { bindings::dev_pm_opp_get_suspend_opp_freq(self.dev.as_raw()) }
+    }
+
+    /// Synchronizes regulators used by the OPP table.
+    pub fn sync_regulators(&self) -> Result<()> {
+        // SAFETY: The requirements are satisfied by the existence of `Device` and its safety
+        // requirements.
+        to_result(unsafe { bindings::dev_pm_opp_sync_regulators(self.dev.as_raw()) })
+    }
+
+    /// Gets sharing CPUs.
+    pub fn sharing_cpus(dev: &Device, cpumask: &mut Cpumask) -> Result<()> {
+        // SAFETY: The requirements are satisfied by the existence of `Device` and its safety
+        // requirements.
+        to_result(unsafe { bindings::dev_pm_opp_get_sharing_cpus(dev.as_raw(), cpumask.as_raw()) })
+    }
+
+    /// Sets sharing CPUs.
+    pub fn set_sharing_cpus(&mut self, cpumask: &mut Cpumask) -> Result<()> {
+        // SAFETY: The requirements are satisfied by the existence of `Device` and its safety
+        // requirements.
+        to_result(unsafe {
+            bindings::dev_pm_opp_set_sharing_cpus(self.dev.as_raw(), cpumask.as_raw())
+        })?;
+
+        if let Some(mask) = self.cpumask.as_mut() {
+            // Update the cpumask as this will be used while removing the table.
+            cpumask.copy(mask);
+        }
+
+        Ok(())
+    }
+
+    /// Gets sharing CPUs from Device tree.
+    #[cfg(CONFIG_OF)]
+    pub fn of_sharing_cpus(dev: &Device, cpumask: &mut Cpumask) -> Result<()> {
+        // SAFETY: The requirements are satisfied by the existence of `Device` and its safety
+        // requirements.
+        to_result(unsafe {
+            bindings::dev_pm_opp_of_get_sharing_cpus(dev.as_raw(), cpumask.as_raw())
+        })
+    }
+
+    /// Updates the voltage value for an OPP.
+    pub fn adjust_voltage(
+        &self,
+        freq: usize,
+        u_volt: usize,
+        u_volt_min: usize,
+        u_volt_max: usize,
+    ) -> Result<()> {
+        // SAFETY: The requirements are satisfied by the existence of `Device` and its safety
+        // requirements.
+        to_result(unsafe {
+            bindings::dev_pm_opp_adjust_voltage(
+                self.dev.as_raw(),
+                freq,
+                u_volt,
+                u_volt_min,
+                u_volt_max,
+            )
+        })
+    }
+
+    /// Sets a matching OPP based on frequency.
+    pub fn set_rate(&self, freq: usize) -> Result<()> {
+        // SAFETY: The requirements are satisfied by the existence of `Device` and its safety
+        // requirements.
+        to_result(unsafe { bindings::dev_pm_opp_set_rate(self.dev.as_raw(), freq) })
+    }
+
+    /// Sets exact OPP.
+    pub fn set_opp(&self, opp: &OPP) -> Result<()> {
+        // SAFETY: The requirements are satisfied by the existence of `Device` and its safety
+        // requirements.
+        to_result(unsafe { bindings::dev_pm_opp_set_opp(self.dev.as_raw(), opp.as_raw()) })
+    }
+
+    /// Finds OPP based on frequency.
+    pub fn opp_from_freq(
+        &self,
+        mut freq: usize,
+        available: Option<bool>,
+        index: Option<u32>,
+        stype: SearchType,
+    ) -> Result<ARef<OPP>> {
+        let rdev = self.dev.as_raw();
+        let index = index.unwrap_or(0);
+
+        let ptr = from_err_ptr(match stype {
+            SearchType::Exact => {
+                if let Some(available) = available {
+                    // SAFETY: The requirements are satisfied by the existence of `Device` and
+                    // its safety requirements. The returned ptr will be owned by the new [`OPP`]
+                    // instance.
+                    unsafe {
+                        bindings::dev_pm_opp_find_freq_exact_indexed(rdev, freq, index, available)
+                    }
+                } else {
+                    return Err(EINVAL);
+                }
+            }
+
+            // SAFETY: The requirements are satisfied by the existence of `Device` and its
+            // safety requirements. The returned ptr will be owned by the new [`OPP`] instance.
+            SearchType::Ceil => unsafe {
+                bindings::dev_pm_opp_find_freq_ceil_indexed(rdev, &mut freq as *mut usize, index)
+            },
+
+            // SAFETY: The requirements are satisfied by the existence of `Device` and its
+            // safety requirements. The returned ptr will be owned by the new [`OPP`] instance.
+            SearchType::Floor => unsafe {
+                bindings::dev_pm_opp_find_freq_floor_indexed(rdev, &mut freq as *mut usize, index)
+            },
+        })?;
+
+        // SAFETY: The `ptr` is guaranteed by the C code to be valid.
+        unsafe { OPP::from_raw_opp_owned(ptr) }
+    }
+
+    /// Finds OPP based on level.
+    pub fn opp_from_level(&self, mut level: u32, stype: SearchType) -> Result<ARef<OPP>> {
+        let rdev = self.dev.as_raw();
+
+        let ptr = from_err_ptr(match stype {
+            // SAFETY: The requirements are satisfied by the existence of `Device` and its
+            // safety requirements. The returned ptr will be owned by the new [`OPP`] instance.
+            SearchType::Exact => unsafe { bindings::dev_pm_opp_find_level_exact(rdev, level) },
+
+            // SAFETY: The requirements are satisfied by the existence of `Device` and its
+            // safety requirements. The returned ptr will be owned by the new [`OPP`] instance.
+            SearchType::Ceil => unsafe {
+                bindings::dev_pm_opp_find_level_ceil(rdev, &mut level as *mut u32)
+            },
+
+            // SAFETY: The requirements are satisfied by the existence of `Device` and its
+            // safety requirements. The returned ptr will be owned by the new [`OPP`] instance.
+            SearchType::Floor => unsafe {
+                bindings::dev_pm_opp_find_level_floor(rdev, &mut level as *mut u32)
+            },
+        })?;
+
+        // SAFETY: The `ptr` is guaranteed by the C code to be valid.
+        unsafe { OPP::from_raw_opp_owned(ptr) }
+    }
+
+    /// Finds OPP based on bandwidth.
+    pub fn opp_from_bw(&self, mut bw: u32, index: i32, stype: SearchType) -> Result<ARef<OPP>> {
+        let rdev = self.dev.as_raw();
+
+        let ptr = from_err_ptr(match stype {
+            // The OPP core doesn't support this yet.
+            SearchType::Exact => return Err(EINVAL),
+
+            // SAFETY: The requirements are satisfied by the existence of `Device` and its
+            // safety requirements. The returned ptr will be owned by the new [`OPP`] instance.
+            SearchType::Ceil => unsafe {
+                bindings::dev_pm_opp_find_bw_ceil(rdev, &mut bw as *mut u32, index)
+            },
+
+            // SAFETY: The requirements are satisfied by the existence of `Device` and its
+            // safety requirements. The returned ptr will be owned by the new [`OPP`] instance.
+            SearchType::Floor => unsafe {
+                bindings::dev_pm_opp_find_bw_floor(rdev, &mut bw as *mut u32, index)
+            },
+        })?;
+
+        // SAFETY: The `ptr` is guaranteed by the C code to be valid.
+        unsafe { OPP::from_raw_opp_owned(ptr) }
+    }
+
+    /// Enable the OPP.
+    pub fn enable_opp(&self, freq: usize) -> Result<()> {
+        // SAFETY: The requirements are satisfied by the existence of `Device` and its safety
+        // requirements.
+        to_result(unsafe { bindings::dev_pm_opp_enable(self.dev.as_raw(), freq) })
+    }
+
+    /// Disable the OPP.
+    pub fn disable_opp(&self, freq: usize) -> Result<()> {
+        // SAFETY: The requirements are satisfied by the existence of `Device` and its safety
+        // requirements.
+        to_result(unsafe { bindings::dev_pm_opp_disable(self.dev.as_raw(), freq) })
+    }
+
+    /// Registers with Energy model.
+    #[cfg(CONFIG_OF)]
+    pub fn of_register_em(&mut self, cpumask: &mut Cpumask) -> Result<()> {
+        // SAFETY: The requirements are satisfied by the existence of `Device` and its safety
+        // requirements.
+        to_result(unsafe {
+            bindings::dev_pm_opp_of_register_em(self.dev.as_raw(), cpumask.as_raw())
+        })?;
+
+        self.em = true;
+        Ok(())
+    }
+
+    // Unregisters with Energy model.
+    #[cfg(all(CONFIG_OF, CONFIG_ENERGY_MODEL))]
+    fn of_unregister_em(&self) {
+        // SAFETY: The requirements are satisfied by the existence of `Device` and its safety
+        // requirements. We registered with the EM framework earlier, it is safe to unregister now.
+        unsafe { bindings::em_dev_unregister_perf_domain(self.dev.as_raw()) };
+    }
+}
+
+impl Drop for Table {
+    fn drop(&mut self) {
+        // SAFETY: By the type invariants, we know that `self` owns a reference, so it is safe
+        // to relinquish it now.
+        unsafe { bindings::dev_pm_opp_put_opp_table(self.ptr) };
+
+        #[cfg(CONFIG_OF)]
+        {
+            #[cfg(CONFIG_ENERGY_MODEL)]
+            if self.em {
+                self.of_unregister_em();
+            }
+
+            if self.of {
+                self.remove_of();
+            } else if let Some(cpumask) = self.cpumask.take() {
+                self.remove_of_cpumask(cpumask);
+            }
+        }
+    }
+}
+
 /// Operating performance point (OPP).
 ///
 /// Wraps the kernel's `struct dev_pm_opp`.
-- 
2.31.1.272.g89b43f80a514

[PATCH V8 09/14] rust: Extend OPP bindings for the configuration options

[Viresh Kumar]

This extends OPP bindings with the bindings for the OPP core
configuration options.

Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 rust/kernel/opp.rs | 262 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 260 insertions(+), 2 deletions(-)

diff --git a/rust/kernel/opp.rs b/rust/kernel/opp.rs
index c0daeadfb188..8028245e94aa 100644
--- a/rust/kernel/opp.rs
+++ b/rust/kernel/opp.rs
@@ -10,11 +10,28 @@
     bindings,
     cpumask::Cpumask,
     device::Device,
-    error::{code::*, from_err_ptr, to_result, Error, Result},
+    error::{code::*, from_err_ptr, from_result, to_result, Error, Result, VTABLE_DEFAULT_ERROR},
+    prelude::*,
+    str::CString,
     types::{ARef, AlwaysRefCounted, Opaque},
 };
 
-use core::ptr;
+use core::{marker::PhantomData, ptr};
+
+use macros::vtable;
+
+// Creates a null-terminated slice of pointers to Cstrings.
+fn to_c_str_array(names: &[CString]) -> Result<KVec<*const u8>> {
+    // Allocated a null-terminated vector of pointers.
+    let mut list = KVec::with_capacity(names.len() + 1, GFP_KERNEL)?;
+
+    for name in names.iter() {
+        list.push(name.as_ptr() as _, GFP_KERNEL)?;
+    }
+
+    list.push(ptr::null(), GFP_KERNEL)?;
+    Ok(list)
+}
 
 /// Dynamically created Operating performance point (OPP).
 pub struct Token {
@@ -79,6 +96,247 @@ pub enum SearchType {
     Ceil,
 }
 
+/// Implement this trait to provide OPP Configuration callbacks.
+#[vtable]
+pub trait ConfigOps {
+    /// Called by the OPP core to configure OPP clks.
+    fn config_clks(_dev: &Device, _table: &Table, _opp: &OPP, _scaling_down: bool) -> Result<()> {
+        build_error!(VTABLE_DEFAULT_ERROR)
+    }
+
+    /// Called by the OPP core to configure OPP regulators.
+    fn config_regulators(
+        _dev: &Device,
+        _opp_old: &OPP,
+        _opp_new: &OPP,
+        _data: *mut *mut bindings::regulator,
+        _count: u32,
+    ) -> Result<()> {
+        build_error!(VTABLE_DEFAULT_ERROR)
+    }
+}
+
+/// Config token returned by the C code.
+pub struct ConfigToken(i32);
+
+impl Drop for ConfigToken {
+    fn drop(&mut self) {
+        // SAFETY: Its safe to return the configuration token number previously received from the C
+        // code.
+        unsafe { bindings::dev_pm_opp_clear_config(self.0) };
+    }
+}
+
+/// Equivalent to `struct dev_pm_opp_config` in the C Code.
+#[derive(Default)]
+pub struct Config<T: ConfigOps> {
+    clk_names: Option<KVec<CString>>,
+    prop_name: Option<CString>,
+    regulator_names: Option<KVec<CString>>,
+    supported_hw: Option<KVec<u32>>,
+    required_dev: Option<ARef<Device>>,
+    required_dev_index: Option<u32>,
+    _data: PhantomData<T>,
+}
+
+impl<T: ConfigOps> Config<T> {
+    /// Creates a new instance of [`Config`].
+    pub fn new() -> Self {
+        Self {
+            clk_names: None,
+            prop_name: None,
+            regulator_names: None,
+            supported_hw: None,
+            required_dev: None,
+            required_dev_index: None,
+            _data: PhantomData,
+        }
+    }
+
+    /// Initializes clock names.
+    pub fn set_clk_names(mut self, names: KVec<CString>) -> Result<Self> {
+        // Already configured.
+        if self.clk_names.is_some() {
+            return Err(EBUSY);
+        }
+
+        if names.is_empty() {
+            return Err(EINVAL);
+        }
+
+        self.clk_names = Some(names);
+        Ok(self)
+    }
+
+    /// Initializes property name.
+    pub fn set_prop_name(mut self, name: CString) -> Result<Self> {
+        // Already configured.
+        if self.prop_name.is_some() {
+            return Err(EBUSY);
+        }
+
+        self.prop_name = Some(name);
+        Ok(self)
+    }
+
+    /// Initializes regulator names.
+    pub fn set_regulator_names(mut self, names: KVec<CString>) -> Result<Self> {
+        // Already configured.
+        if self.regulator_names.is_some() {
+            return Err(EBUSY);
+        }
+
+        if names.is_empty() {
+            return Err(EINVAL);
+        }
+
+        self.regulator_names = Some(names);
+
+        Ok(self)
+    }
+
+    /// Initializes required devices.
+    pub fn set_required_dev(mut self, dev: ARef<Device>, index: u32) -> Result<Self> {
+        // Already configured.
+        if self.required_dev.is_some() {
+            return Err(EBUSY);
+        }
+
+        self.required_dev = Some(dev);
+        self.required_dev_index = Some(index);
+        Ok(self)
+    }
+
+    /// Initializes supported hardware.
+    pub fn set_supported_hw(mut self, hw: KVec<u32>) -> Result<Self> {
+        // Already configured.
+        if self.supported_hw.is_some() {
+            return Err(EBUSY);
+        }
+
+        if hw.is_empty() {
+            return Err(EINVAL);
+        }
+
+        self.supported_hw = Some(hw);
+        Ok(self)
+    }
+
+    /// Sets the configuration with the OPP core.
+    pub fn set(self, dev: &Device) -> Result<ConfigToken> {
+        let (_clk_list, clk_names) = match &self.clk_names {
+            Some(x) => {
+                let list = to_c_str_array(x)?;
+                let ptr = list.as_ptr();
+                (Some(list), ptr)
+            }
+            None => (None, ptr::null()),
+        };
+
+        let (_regulator_list, regulator_names) = match &self.regulator_names {
+            Some(x) => {
+                let list = to_c_str_array(x)?;
+                let ptr = list.as_ptr();
+                (Some(list), ptr)
+            }
+            None => (None, ptr::null()),
+        };
+
+        let prop_name = match &self.prop_name {
+            Some(x) => x.as_char_ptr(),
+            None => ptr::null(),
+        };
+
+        let (supported_hw, supported_hw_count) = match &self.supported_hw {
+            Some(x) => (x.as_ptr(), x.len() as u32),
+            None => (ptr::null(), 0),
+        };
+
+        let (required_dev, required_dev_index) = match &self.required_dev {
+            Some(x) => (x.as_raw(), self.required_dev_index.unwrap()),
+            None => (ptr::null_mut(), 0),
+        };
+
+        let mut config = bindings::dev_pm_opp_config {
+            clk_names,
+            config_clks: if T::HAS_CONFIG_CLKS {
+                Some(Self::config_clks)
+            } else {
+                None
+            },
+            prop_name,
+            regulator_names,
+            config_regulators: if T::HAS_CONFIG_REGULATORS {
+                Some(Self::config_regulators)
+            } else {
+                None
+            },
+            supported_hw,
+            supported_hw_count,
+
+            required_dev,
+            required_dev_index,
+        };
+
+        // SAFETY: The requirements are satisfied by the existence of `Device` and its safety
+        // requirements. The OPP core guarantees to not use fields of `config`, after this call has
+        // returned and so we don't need to save a copy of them for future use
+        let ret = unsafe { bindings::dev_pm_opp_set_config(dev.as_raw(), &mut config) };
+        if ret < 0 {
+            Err(Error::from_errno(ret))
+        } else {
+            Ok(ConfigToken(ret))
+        }
+    }
+
+    // Config's config_clks callback.
+    extern "C" fn config_clks(
+        dev: *mut bindings::device,
+        opp_table: *mut bindings::opp_table,
+        opp: *mut bindings::dev_pm_opp,
+        _data: *mut core::ffi::c_void,
+        scaling_down: bool,
+    ) -> core::ffi::c_int {
+        from_result(|| {
+            // SAFETY: 'dev' is guaranteed by the C code to be valid.
+            let dev = unsafe { Device::get_device(dev) };
+            T::config_clks(
+                &dev,
+                // SAFETY: 'opp_table' is guaranteed by the C code to be valid.
+                &unsafe { Table::from_raw_table(opp_table, &dev) },
+                // SAFETY: 'opp' is guaranteed by the C code to be valid.
+                unsafe { OPP::from_raw_opp(opp)? },
+                scaling_down,
+            )
+            .map(|()| 0)
+        })
+    }
+
+    // Config's config_regulators callback.
+    extern "C" fn config_regulators(
+        dev: *mut bindings::device,
+        old_opp: *mut bindings::dev_pm_opp,
+        new_opp: *mut bindings::dev_pm_opp,
+        regulators: *mut *mut bindings::regulator,
+        count: core::ffi::c_uint,
+    ) -> core::ffi::c_int {
+        from_result(|| {
+            // SAFETY: 'dev' is guaranteed by the C code to be valid.
+            let dev = unsafe { Device::get_device(dev) };
+            T::config_regulators(
+                &dev,
+                // SAFETY: 'old_opp' is guaranteed by the C code to be valid.
+                unsafe { OPP::from_raw_opp(old_opp)? },
+                // SAFETY: 'new_opp' is guaranteed by the C code to be valid.
+                unsafe { OPP::from_raw_opp(new_opp)? },
+                regulators,
+                count,
+            )
+            .map(|()| 0)
+        })
+    }
+}
+
 /// Operating performance point (OPP) table.
 ///
 /// Wraps the kernel's `struct opp_table`.
-- 
2.31.1.272.g89b43f80a514

[PATCH V8 10/14] rust: Add initial bindings for cpufreq framework

[Viresh Kumar]

This commit adds initial Rust bindings for the cpufreq core. This adds
basic bindings for cpufreq flags, relations and cpufreq table.

Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 MAINTAINERS                     |   1 +
 rust/bindings/bindings_helper.h |   1 +
 rust/helpers/cpufreq.c          |  10 ++
 rust/helpers/helpers.c          |   1 +
 rust/kernel/cpufreq.rs          | 228 ++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs              |   2 +
 6 files changed, 243 insertions(+)
 create mode 100644 rust/helpers/cpufreq.c
 create mode 100644 rust/kernel/cpufreq.rs

diff --git a/MAINTAINERS b/MAINTAINERS
index cda0d2b74e29..f0270ceb82d9 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6024,6 +6024,7 @@ F:	drivers/cpufreq/
 F:	include/linux/cpufreq.h
 F:	include/linux/sched/cpufreq.h
 F:	kernel/sched/cpufreq*.c
+F:	rust/kernel/cpufreq.rs
 F:	tools/testing/selftests/cpufreq/
 
 CPU HOTPLUG
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 7f851d5907af..68bf1bc5bae8 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -12,6 +12,7 @@
 #include <linux/blkdev.h>
 #include <linux/clk.h>
 #include <linux/cpu.h>
+#include <linux/cpufreq.h>
 #include <linux/cpumask.h>
 #include <linux/cred.h>
 #include <linux/errname.h>
diff --git a/rust/helpers/cpufreq.c b/rust/helpers/cpufreq.c
new file mode 100644
index 000000000000..7c1343c4d65e
--- /dev/null
+++ b/rust/helpers/cpufreq.c
@@ -0,0 +1,10 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/cpufreq.h>
+
+#ifdef CONFIG_CPU_FREQ
+void rust_helper_cpufreq_register_em_with_opp(struct cpufreq_policy *policy)
+{
+	cpufreq_register_em_with_opp(policy);
+}
+#endif
diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
index de2341cfd917..32d0462219e5 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c
@@ -11,6 +11,7 @@
 #include "bug.c"
 #include "build_assert.c"
 #include "build_bug.c"
+#include "cpufreq.c"
 #include "cpumask.c"
 #include "cred.c"
 #include "device.c"
diff --git a/rust/kernel/cpufreq.rs b/rust/kernel/cpufreq.rs
new file mode 100644
index 000000000000..4546a70c7063
--- /dev/null
+++ b/rust/kernel/cpufreq.rs
@@ -0,0 +1,228 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! CPU frequency scaling.
+//!
+//! This module provides bindings for interacting with the cpufreq subsystem.
+//!
+//! C header: [`include/linux/cpufreq.h`](srctree/include/linux/cpufreq.h)
+
+use crate::{
+    bindings,
+    error::{code::*, to_result, Result},
+    prelude::*,
+};
+
+use core::{
+    pin::Pin,
+};
+
+/// Default transition latency value.
+pub const ETERNAL_LATENCY: u32 = bindings::CPUFREQ_ETERNAL as u32;
+
+/// Container for cpufreq driver flags.
+pub mod flags {
+    use crate::bindings;
+
+    /// Set by drivers that need to update internal upper and lower boundaries along with the
+    /// target frequency and so the core and governors should also invoke the driver if the target
+    /// frequency does not change, but the policy min or max may have changed.
+    pub const NEED_UPDATE_LIMITS: u16 = bindings::CPUFREQ_NEED_UPDATE_LIMITS as _;
+
+    /// Set by drivers for platforms where loops_per_jiffy or other kernel "constants" aren't
+    /// affected by frequency transitions.
+    pub const CONST_LOOPS: u16 = bindings::CPUFREQ_CONST_LOOPS as _;
+
+    /// Set by drivers that want the core to automatically register the cpufreq driver as a thermal
+    /// cooling device.
+    pub const IS_COOLING_DEV: u16 = bindings::CPUFREQ_IS_COOLING_DEV as _;
+
+    /// Set by drivers for platforms that have multiple clock-domains, i.e. supporting multiple
+    /// policies. With this sysfs directories of governor would be created in cpu/cpuN/cpufreq/
+    /// directory and so they can use the same governor with different tunables for different
+    /// clusters.
+    pub const HAVE_GOVERNOR_PER_POLICY: u16 = bindings::CPUFREQ_HAVE_GOVERNOR_PER_POLICY as _;
+
+    /// Set by drivers which do POSTCHANGE notifications from outside of their ->target() routine.
+    pub const ASYNC_NOTIFICATION: u16 = bindings::CPUFREQ_ASYNC_NOTIFICATION as _;
+
+    /// Set by drivers that want cpufreq core to check if CPU is running at a frequency present in
+    /// freq-table exposed by the driver. For these drivers if CPU is found running at an out of
+    /// table freq, the cpufreq core will try to change the frequency to a value from the table.
+    /// And if that fails, it will stop further boot process by issuing a BUG_ON().
+    pub const NEED_INITIAL_FREQ_CHECK: u16 = bindings::CPUFREQ_NEED_INITIAL_FREQ_CHECK as _;
+
+    /// Set by drivers to disallow use of governors with "dynamic_switching" flag set.
+    pub const NO_AUTO_DYNAMIC_SWITCHING: u16 = bindings::CPUFREQ_NO_AUTO_DYNAMIC_SWITCHING as _;
+}
+
+/// CPU frequency selection relations. Each value contains a `bool` argument which corresponds to
+/// the Relation being efficient.
+#[derive(Copy, Clone, Debug, Eq, PartialEq)]
+pub enum Relation {
+    /// Select the lowest frequency at or above target.
+    Low(bool),
+    /// Select the highest frequency below or at target.
+    High(bool),
+    /// Select the closest frequency to the target.
+    Close(bool),
+}
+
+impl Relation {
+    // Converts from a value compatible with the C code.
+    fn new(val: u32) -> Result<Self> {
+        let efficient = val & bindings::CPUFREQ_RELATION_E != 0;
+
+        Ok(match val & !bindings::CPUFREQ_RELATION_E {
+            bindings::CPUFREQ_RELATION_L => Self::Low(efficient),
+            bindings::CPUFREQ_RELATION_H => Self::High(efficient),
+            bindings::CPUFREQ_RELATION_C => Self::Close(efficient),
+            _ => return Err(EINVAL),
+        })
+    }
+
+    /// Converts to a value compatible with the C code.
+    pub fn val(&self) -> u32 {
+        let (mut val, e) = match self {
+            Self::Low(e) => (bindings::CPUFREQ_RELATION_L, e),
+            Self::High(e) => (bindings::CPUFREQ_RELATION_H, e),
+            Self::Close(e) => (bindings::CPUFREQ_RELATION_C, e),
+        };
+
+        if *e {
+            val |= bindings::CPUFREQ_RELATION_E;
+        }
+
+        val
+    }
+}
+
+/// Equivalent to `struct cpufreq_policy_data` in the C code.
+#[repr(transparent)]
+pub struct PolicyData(*mut bindings::cpufreq_policy_data);
+
+impl PolicyData {
+    /// Creates new instance of [`PolicyData`].
+    ///
+    /// # Safety
+    ///
+    /// Callers must ensure that `ptr` is valid and non-null.
+    pub unsafe fn from_raw_policy_data(ptr: *mut bindings::cpufreq_policy_data) -> Self {
+        Self(ptr)
+    }
+
+    /// Returns the raw pointer to the C structure.
+    #[inline]
+    pub fn as_raw(&self) -> *mut bindings::cpufreq_policy_data {
+        self.0
+    }
+
+    /// Provides a wrapper to the generic verify routine.
+    pub fn generic_verify(&self) -> Result<()> {
+        // SAFETY: By the type invariants, we know that `self` owns a reference, so it is safe to
+        // use it now.
+        to_result(unsafe { bindings::cpufreq_generic_frequency_table_verify(self.as_raw()) })
+    }
+}
+
+/// Builder for the `struct cpufreq_frequency_table` in the C code.
+#[repr(transparent)]
+#[derive(Default)]
+pub struct TableBuilder {
+    entries: KVec<bindings::cpufreq_frequency_table>,
+}
+
+impl TableBuilder {
+    /// Creates new instance of [`TableBuilder`].
+    pub fn new() -> Self {
+        Self {
+            entries: KVec::new(),
+        }
+    }
+
+    /// Adds a new entry to the table.
+    pub fn add(&mut self, frequency: u32, flags: u32, driver_data: u32) -> Result<()> {
+        // Adds new entry to the end of the vector.
+        Ok(self.entries.push(
+            bindings::cpufreq_frequency_table {
+                flags,
+                driver_data,
+                frequency,
+            },
+            GFP_KERNEL,
+        )?)
+    }
+
+    /// Creates [`Table`] from [`TableBuilder`].
+    pub fn into_table(mut self) -> Result<Table> {
+        // Add last entry to the table.
+        self.add(bindings::CPUFREQ_TABLE_END as u32, 0, 0)?;
+        Table::from_builder(self.entries)
+    }
+}
+
+/// A simple implementation of the cpufreq table, equivalent to the `struct
+/// cpufreq_frequency_table` in the C code.
+pub struct Table {
+    #[allow(dead_code)]
+    // Dynamically created table.
+    entries: Option<Pin<KVec<bindings::cpufreq_frequency_table>>>,
+
+    // Pointer to the statically or dynamically created table.
+    ptr: *mut bindings::cpufreq_frequency_table,
+}
+
+impl Table {
+    /// Creates new instance of [`Table`] from [`TableBuilder`].
+    fn from_builder(entries: KVec<bindings::cpufreq_frequency_table>) -> Result<Self> {
+        if entries.is_empty() {
+            return Err(EINVAL);
+        }
+
+        // Pin the entries to memory, since we are passing its pointer to the C code.
+        let mut entries = Pin::new(entries);
+
+        // The pointer is valid until the table gets dropped.
+        let ptr = entries.as_mut_ptr();
+
+        Ok(Self {
+            entries: Some(entries),
+            ptr,
+        })
+    }
+
+    /// Creates new instance of [`Table`] from raw pointer.
+    ///
+    /// # Safety
+    ///
+    /// Callers must ensure that `ptr` is valid and non-null for the lifetime of the [`Table`].
+    pub unsafe fn from_raw(ptr: *mut bindings::cpufreq_frequency_table) -> Self {
+        Self { entries: None, ptr }
+    }
+
+    /// Returns raw pointer to the `struct cpufreq_frequency_table` compatible with the C code.
+    #[inline]
+    pub fn as_raw(&self) -> *mut bindings::cpufreq_frequency_table {
+        self.ptr
+    }
+
+    /// Returns `frequency` at index in the [`Table`].
+    pub fn freq(&self, index: usize) -> Result<u32> {
+        // SAFETY: The pointer is guaranteed to be valid for the lifetime of `self` and `index` is
+        // guaranteed to be within limits of the frequency table by the C API.
+        Ok(unsafe { (*self.ptr.add(index)).frequency })
+    }
+
+    /// Returns `flags` at index in the [`Table`].
+    pub fn flags(&self, index: usize) -> Result<u32> {
+        // SAFETY: The pointer is guaranteed to be valid for the lifetime of `self` and `index` is
+        // guaranteed to be within limits of the frequency table by the C API.
+        Ok(unsafe { (*self.ptr.add(index)).flags })
+    }
+
+    /// Returns `data` at index in the [`Table`].
+    pub fn data(&self, index: usize) -> Result<u32> {
+        // SAFETY: The pointer is guaranteed to be valid for the lifetime of `self` and `index` is
+        // guaranteed to be within limits of the frequency table by the C API.
+        Ok(unsafe { (*self.ptr.add(index)).driver_data })
+    }
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 8956f78a2805..2a42e21d57f2 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -43,6 +43,8 @@
 #[cfg(CONFIG_COMMON_CLK)]
 pub mod clk;
 pub mod cpu;
+#[cfg(CONFIG_CPU_FREQ)]
+pub mod cpufreq;
 pub mod cpumask;
 pub mod cred;
 pub mod device;
-- 
2.31.1.272.g89b43f80a514

[PATCH V8 11/14] rust: Extend cpufreq bindings for policy and driver ops

[Viresh Kumar]

This extends the cpufreq bindings with bindings for cpufreq policy and
driver operations.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 rust/kernel/cpufreq.rs | 357 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 355 insertions(+), 2 deletions(-)

diff --git a/rust/kernel/cpufreq.rs b/rust/kernel/cpufreq.rs
index 4546a70c7063..63ea816017c0 100644
--- a/rust/kernel/cpufreq.rs
+++ b/rust/kernel/cpufreq.rs
@@ -7,15 +7,20 @@
 //! C header: [`include/linux/cpufreq.h`](srctree/include/linux/cpufreq.h)
 
 use crate::{
-    bindings,
-    error::{code::*, to_result, Result},
+    bindings, clk, cpumask,
+    device::Device,
+    error::{code::*, from_err_ptr, to_result, Result, VTABLE_DEFAULT_ERROR},
     prelude::*,
+    types::ForeignOwnable,
 };
 
 use core::{
     pin::Pin,
+    ptr::self,
 };
 
+use macros::vtable;
+
 /// Default transition latency value.
 pub const ETERNAL_LATENCY: u32 = bindings::CPUFREQ_ETERNAL as u32;
 
@@ -226,3 +231,351 @@ pub fn data(&self, index: usize) -> Result<u32> {
         Ok(unsafe { (*self.ptr.add(index)).driver_data })
     }
 }
+
+/// Equivalent to `struct cpufreq_policy` in the C code.
+pub struct Policy {
+    ptr: *mut bindings::cpufreq_policy,
+    put_cpu: bool,
+    cpumask: cpumask::Cpumask,
+}
+
+impl Policy {
+    /// Creates a new instance of [`Policy`].
+    ///
+    /// # Safety
+    ///
+    /// Callers must ensure that `ptr` is valid and non-null.
+    pub unsafe fn from_raw_policy(ptr: *mut bindings::cpufreq_policy) -> Self {
+        Self {
+            ptr,
+            put_cpu: false,
+            // SAFETY: The pointer is guaranteed to be valid for the lifetime of `Self`. The `cpus`
+            // pointer is guaranteed to be valid by the C code.
+            cpumask: unsafe { cpumask::Cpumask::get_cpumask(&mut ((*ptr).cpus)) },
+        }
+    }
+
+    fn from_cpu(cpu: u32) -> Result<Self> {
+        // SAFETY: It is safe to call `cpufreq_cpu_get()` for any CPU.
+        let ptr = from_err_ptr(unsafe { bindings::cpufreq_cpu_get(cpu) })?;
+
+        // SAFETY: The pointer is guaranteed to be valid by the C code.
+        let mut policy = unsafe { Policy::from_raw_policy(ptr) };
+        policy.put_cpu = true;
+        Ok(policy)
+    }
+
+    /// Raw pointer to the underlying cpufreq policy.
+    #[inline]
+    pub fn as_raw(&self) -> *mut bindings::cpufreq_policy {
+        self.ptr
+    }
+
+    fn as_ref(&self) -> &bindings::cpufreq_policy {
+        // SAFETY: By the type invariants, we know that `self` owns a reference to the pointer.
+        unsafe { &(*self.ptr) }
+    }
+    fn as_mut_ref(&mut self) -> &mut bindings::cpufreq_policy {
+        // SAFETY: By the type invariants, we know that `self` owns a reference to the pointer.
+        unsafe { &mut (*self.ptr) }
+    }
+
+    /// Returns the primary CPU for a cpufreq policy.
+    pub fn cpu(&self) -> u32 {
+        self.as_ref().cpu
+    }
+
+    /// Returns the minimum frequency for a cpufreq policy.
+    pub fn min(&self) -> u32 {
+        self.as_ref().min
+    }
+
+    /// Set the minimum frequency for a cpufreq policy.
+    pub fn set_min(&mut self, min: u32) -> &mut Self {
+        self.as_mut_ref().min = min;
+        self
+    }
+
+    /// Returns the maximum frequency for a cpufreq policy.
+    pub fn max(&self) -> u32 {
+        self.as_ref().max
+    }
+
+    /// Set the maximum frequency for a cpufreq policy.
+    pub fn set_max(&mut self, max: u32) -> &mut Self {
+        self.as_mut_ref().max = max;
+        self
+    }
+
+    /// Returns the current frequency for a cpufreq policy.
+    pub fn cur(&self) -> u32 {
+        self.as_ref().cur
+    }
+
+    /// Sets the suspend frequency for a cpufreq policy.
+    pub fn set_suspend_freq(&mut self, freq: u32) -> &mut Self {
+        self.as_mut_ref().suspend_freq = freq;
+        self
+    }
+
+    /// Returns the suspend frequency for a cpufreq policy.
+    pub fn suspend_freq(&self) -> u32 {
+        self.as_ref().suspend_freq
+    }
+
+    /// Provides a wrapper to the generic suspend routine.
+    pub fn generic_suspend(&self) -> Result<()> {
+        // SAFETY: By the type invariants, we know that `self` owns a reference, so it is safe to
+        // use it now.
+        to_result(unsafe { bindings::cpufreq_generic_suspend(self.as_raw()) })
+    }
+
+    /// Provides a wrapper to the generic get routine.
+    pub fn generic_get(&self) -> Result<u32> {
+        // SAFETY: By the type invariants, we know that `self` owns a reference, so it is safe to
+        // use it now.
+        Ok(unsafe { bindings::cpufreq_generic_get(self.cpu()) })
+    }
+
+    /// Provides a wrapper to the register em with OPP routine.
+    pub fn register_em_opp(&self) {
+        // SAFETY: By the type invariants, we know that `self` owns a reference, so it is safe to
+        // use it now.
+        unsafe { bindings::cpufreq_register_em_with_opp(self.as_raw()) };
+    }
+
+    /// Gets raw pointer to cpufreq policy's CPUs mask.
+    pub fn cpus(&mut self) -> &mut cpumask::Cpumask {
+        &mut self.cpumask
+    }
+
+    /// Sets clock for a cpufreq policy.
+    pub fn set_clk(&mut self, dev: &Device, name: Option<&CStr>) -> Result<clk::Clk> {
+        let clk = clk::Clk::new(dev, name)?;
+        self.as_mut_ref().clk = clk.as_raw();
+        Ok(clk)
+    }
+
+    /// Allows frequency switching code to run on any CPU.
+    pub fn set_dvfs_possible_from_any_cpu(&mut self) -> &mut Self {
+        self.as_mut_ref().dvfs_possible_from_any_cpu = true;
+        self
+    }
+
+    /// Get fast_switch_possible value.
+    pub fn fast_switch_possible(&self) -> bool {
+        self.as_ref().fast_switch_possible
+    }
+
+    /// Enable/disable fast frequency switching.
+    pub fn set_fast_switch_possible(&mut self, val: bool) -> &mut Self {
+        self.as_mut_ref().fast_switch_possible = val;
+        self
+    }
+
+    /// Sets transition latency for a cpufreq policy.
+    pub fn set_transition_latency(&mut self, latency: u32) -> &mut Self {
+        self.as_mut_ref().cpuinfo.transition_latency = latency;
+        self
+    }
+
+    /// Set cpuinfo.min_freq.
+    pub fn set_cpuinfo_min_freq(&mut self, min_freq: u32) -> &mut Self {
+        self.as_mut_ref().cpuinfo.min_freq = min_freq;
+        self
+    }
+
+    /// Set cpuinfo.max_freq.
+    pub fn set_cpuinfo_max_freq(&mut self, max_freq: u32) -> &mut Self {
+        self.as_mut_ref().cpuinfo.max_freq = max_freq;
+        self
+    }
+
+    /// Set transition_delay_us, i.e. time between successive freq. change requests.
+    pub fn set_transition_delay_us(&mut self, transition_delay_us: u32) -> &mut Self {
+        self.as_mut_ref().transition_delay_us = transition_delay_us;
+        self
+    }
+
+    /// Returns the cpufreq table for a cpufreq policy. The cpufreq table is recreated in a
+    /// light-weight manner from the raw pointer. The table in C code is not freed once this table
+    /// is dropped.
+    pub fn freq_table(&self) -> Result<Table> {
+        if self.as_ref().freq_table.is_null() {
+            return Err(EINVAL);
+        }
+
+        // SAFETY: The `freq_table` is guaranteed to be valid.
+        Ok(unsafe { Table::from_raw(self.as_ref().freq_table) })
+    }
+
+    /// Sets the cpufreq table for a cpufreq policy.
+    ///
+    /// The cpufreq driver must guarantee that the frequency table does not get freed while it is
+    /// still being used by the C code.
+    pub fn set_freq_table(&mut self, table: &Table) -> &mut Self {
+        self.as_mut_ref().freq_table = table.as_raw();
+        self
+    }
+
+    /// Returns the data for a cpufreq policy.
+    pub fn data<T: ForeignOwnable>(&mut self) -> Option<<T>::Borrowed<'_>> {
+        if self.as_ref().driver_data.is_null() {
+            None
+        } else {
+            // SAFETY: The data is earlier set by us from [`set_data()`].
+            Some(unsafe { T::borrow(self.as_ref().driver_data) })
+        }
+    }
+
+    // Sets the data for a cpufreq policy.
+    fn set_data<T: ForeignOwnable>(&mut self, data: T) -> Result<()> {
+        if self.as_ref().driver_data.is_null() {
+            // Pass the ownership of the data to the foreign interface.
+            self.as_mut_ref().driver_data = <T as ForeignOwnable>::into_foreign(data) as _;
+            Ok(())
+        } else {
+            Err(EBUSY)
+        }
+    }
+
+    // Returns the data for a cpufreq policy.
+    fn clear_data<T: ForeignOwnable>(&mut self) -> Option<T> {
+        if self.as_ref().driver_data.is_null() {
+            None
+        } else {
+            let data = Some(
+                // SAFETY: The data is earlier set by us from [`set_data()`]. It is safe to take
+                // back the ownership of the data from the foreign interface.
+                unsafe { <T as ForeignOwnable>::from_foreign(self.as_ref().driver_data) },
+            );
+            self.as_mut_ref().driver_data = ptr::null_mut();
+            data
+        }
+    }
+}
+
+impl Drop for Policy {
+    fn drop(&mut self) {
+        if self.put_cpu {
+            // SAFETY: By the type invariants, we know that `self` owns a reference, so it is safe
+            // to relinquish it now.
+            unsafe { bindings::cpufreq_cpu_put(self.as_raw()) };
+        }
+    }
+}
+
+/// Operations to be implemented by a cpufreq driver.
+#[vtable]
+pub trait Driver {
+    /// Driver specific data.
+    ///
+    /// Corresponds to the data retrieved via the kernel's
+    /// `cpufreq_get_driver_data()` function.
+    ///
+    /// Require that `Data` implements `ForeignOwnable`. We guarantee to
+    /// never move the underlying wrapped data structure.
+    type Data: ForeignOwnable;
+
+    /// Policy specific data.
+    ///
+    /// Require that `PData` implements `ForeignOwnable`. We guarantee to
+    /// never move the underlying wrapped data structure.
+    type PData: ForeignOwnable;
+
+    /// Policy's init callback.
+    fn init(policy: &mut Policy) -> Result<Self::PData>;
+
+    /// Policy's exit callback.
+    fn exit(_policy: &mut Policy, _data: Option<Self::PData>) -> Result<()> {
+        build_error!(VTABLE_DEFAULT_ERROR)
+    }
+
+    /// Policy's online callback.
+    fn online(_policy: &mut Policy) -> Result<()> {
+        build_error!(VTABLE_DEFAULT_ERROR)
+    }
+
+    /// Policy's offline callback.
+    fn offline(_policy: &mut Policy) -> Result<()> {
+        build_error!(VTABLE_DEFAULT_ERROR)
+    }
+
+    /// Policy's suspend callback.
+    fn suspend(_policy: &mut Policy) -> Result<()> {
+        build_error!(VTABLE_DEFAULT_ERROR)
+    }
+
+    /// Policy's resume callback.
+    fn resume(_policy: &mut Policy) -> Result<()> {
+        build_error!(VTABLE_DEFAULT_ERROR)
+    }
+
+    /// Policy's ready callback.
+    fn ready(_policy: &mut Policy) {
+        build_error!(VTABLE_DEFAULT_ERROR)
+    }
+
+    /// Policy's verify callback.
+    fn verify(data: &mut PolicyData) -> Result<()>;
+
+    /// Policy's setpolicy callback.
+    fn setpolicy(_policy: &mut Policy) -> Result<()> {
+        build_error!(VTABLE_DEFAULT_ERROR)
+    }
+
+    /// Policy's target callback.
+    fn target(_policy: &mut Policy, _target_freq: u32, _relation: Relation) -> Result<()> {
+        build_error!(VTABLE_DEFAULT_ERROR)
+    }
+
+    /// Policy's target_index callback.
+    fn target_index(_policy: &mut Policy, _index: u32) -> Result<()> {
+        build_error!(VTABLE_DEFAULT_ERROR)
+    }
+
+    /// Policy's fast_switch callback.
+    fn fast_switch(_policy: &mut Policy, _target_freq: u32) -> u32 {
+        build_error!(VTABLE_DEFAULT_ERROR)
+    }
+
+    /// Policy's adjust_perf callback.
+    fn adjust_perf(_policy: &mut Policy, _min_perf: usize, _target_perf: usize, _capacity: usize) {
+        build_error!(VTABLE_DEFAULT_ERROR)
+    }
+
+    /// Policy's get_intermediate callback.
+    fn get_intermediate(_policy: &mut Policy, _index: u32) -> u32 {
+        build_error!(VTABLE_DEFAULT_ERROR)
+    }
+
+    /// Policy's target_intermediate callback.
+    fn target_intermediate(_policy: &mut Policy, _index: u32) -> Result<()> {
+        build_error!(VTABLE_DEFAULT_ERROR)
+    }
+
+    /// Policy's get callback.
+    fn get(_policy: &mut Policy) -> Result<u32> {
+        build_error!(VTABLE_DEFAULT_ERROR)
+    }
+
+    /// Policy's update_limits callback.
+    fn update_limits(_policy: &mut Policy) {
+        build_error!(VTABLE_DEFAULT_ERROR)
+    }
+
+    /// Policy's bios_limit callback.
+    fn bios_limit(_policy: &mut Policy, _limit: &mut u32) -> Result<()> {
+        build_error!(VTABLE_DEFAULT_ERROR)
+    }
+
+    /// Policy's set_boost callback.
+    fn set_boost(_policy: &mut Policy, _state: i32) -> Result<()> {
+        build_error!(VTABLE_DEFAULT_ERROR)
+    }
+
+    /// Policy's register_em callback.
+    fn register_em(_policy: &mut Policy) {
+        build_error!(VTABLE_DEFAULT_ERROR)
+    }
+}
-- 
2.31.1.272.g89b43f80a514

[PATCH V8 12/14] rust: Extend cpufreq bindings for driver registration

[Viresh Kumar]

This extends the cpufreq bindings with bindings for registering a
driver.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 rust/kernel/cpufreq.rs | 475 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 473 insertions(+), 2 deletions(-)

diff --git a/rust/kernel/cpufreq.rs b/rust/kernel/cpufreq.rs
index 63ea816017c0..f92259d339d3 100644
--- a/rust/kernel/cpufreq.rs
+++ b/rust/kernel/cpufreq.rs
@@ -9,14 +9,17 @@
 use crate::{
     bindings, clk, cpumask,
     device::Device,
-    error::{code::*, from_err_ptr, to_result, Result, VTABLE_DEFAULT_ERROR},
+    devres::Devres,
+    error::{code::*, from_err_ptr, from_result, to_result, Result, VTABLE_DEFAULT_ERROR},
     prelude::*,
     types::ForeignOwnable,
 };
 
 use core::{
+    cell::UnsafeCell,
+    marker::PhantomData,
     pin::Pin,
-    ptr::self,
+    ptr::{self, addr_of_mut},
 };
 
 use macros::vtable;
@@ -579,3 +582,471 @@ fn register_em(_policy: &mut Policy) {
         build_error!(VTABLE_DEFAULT_ERROR)
     }
 }
+
+/// Registration of a cpufreq driver.
+pub struct Registration<T: Driver> {
+    drv: KBox<UnsafeCell<bindings::cpufreq_driver>>,
+    _p: PhantomData<T>,
+}
+
+// SAFETY: `Registration` doesn't offer any methods or access to fields when shared between threads
+// or CPUs, so it is safe to share it.
+unsafe impl<T: Driver> Sync for Registration<T> {}
+
+#[allow(clippy::non_send_fields_in_send_ty)]
+// SAFETY: Registration with and unregistration from the cpufreq subsystem can happen from any
+// thread.  Additionally, `T::Data` (which is dropped during unregistration) is `Send`, so it is
+// okay to move `Registration` to different threads.
+unsafe impl<T: Driver> Send for Registration<T> {}
+
+impl<T: Driver> Registration<T> {
+    /// Registers a cpufreq driver with the rest of the kernel.
+    pub fn new(name: &'static CStr, data: T::Data, flags: u16, boost: bool) -> Result<Self> {
+        let mut drv = KBox::new(
+            UnsafeCell::new(bindings::cpufreq_driver::default()),
+            GFP_KERNEL,
+        )?;
+        let drv_ref = drv.get_mut();
+
+        // Account for the trailing null character.
+        let len = name.len() + 1;
+        if len > drv_ref.name.len() {
+            return Err(EINVAL);
+        };
+
+        // SAFETY: `name` is a valid Cstr, and we are copying it to an array of equal or larger
+        // size.
+        let name = unsafe { &*(name.as_bytes_with_nul() as *const [u8]) };
+        drv_ref.name[..len].copy_from_slice(name);
+
+        drv_ref.boost_enabled = boost;
+        drv_ref.flags = flags;
+
+        // Allocate an array of 3 pointers to be passed to the C code.
+        let mut attr = KBox::new([ptr::null_mut(); 3], GFP_KERNEL)?;
+        let mut next = 0;
+
+        // SAFETY: The C code returns a valid pointer here, which is again passed to the C code in
+        // an array.
+        attr[next] =
+            unsafe { addr_of_mut!(bindings::cpufreq_freq_attr_scaling_available_freqs) as *mut _ };
+        next += 1;
+
+        if boost {
+            // SAFETY: The C code returns a valid pointer here, which is again passed to the C code
+            // in an array.
+            attr[next] =
+                unsafe { addr_of_mut!(bindings::cpufreq_freq_attr_scaling_boost_freqs) as *mut _ };
+            next += 1;
+        }
+        attr[next] = ptr::null_mut();
+
+        // Pass the ownership of the memory block to the C code. This will be freed when
+        // the [`Registration`] object goes out of scope.
+        drv_ref.attr = KBox::leak(attr) as *mut _;
+
+        // Initialize mandatory callbacks.
+        drv_ref.init = Some(Self::init_callback);
+        drv_ref.verify = Some(Self::verify_callback);
+
+        // Initialize optional callbacks.
+        drv_ref.setpolicy = if T::HAS_SETPOLICY {
+            Some(Self::setpolicy_callback)
+        } else {
+            None
+        };
+        drv_ref.target = if T::HAS_TARGET {
+            Some(Self::target_callback)
+        } else {
+            None
+        };
+        drv_ref.target_index = if T::HAS_TARGET_INDEX {
+            Some(Self::target_index_callback)
+        } else {
+            None
+        };
+        drv_ref.fast_switch = if T::HAS_FAST_SWITCH {
+            Some(Self::fast_switch_callback)
+        } else {
+            None
+        };
+        drv_ref.adjust_perf = if T::HAS_ADJUST_PERF {
+            Some(Self::adjust_perf_callback)
+        } else {
+            None
+        };
+        drv_ref.get_intermediate = if T::HAS_GET_INTERMEDIATE {
+            Some(Self::get_intermediate_callback)
+        } else {
+            None
+        };
+        drv_ref.target_intermediate = if T::HAS_TARGET_INTERMEDIATE {
+            Some(Self::target_intermediate_callback)
+        } else {
+            None
+        };
+        drv_ref.get = if T::HAS_GET {
+            Some(Self::get_callback)
+        } else {
+            None
+        };
+        drv_ref.update_limits = if T::HAS_UPDATE_LIMITS {
+            Some(Self::update_limits_callback)
+        } else {
+            None
+        };
+        drv_ref.bios_limit = if T::HAS_BIOS_LIMIT {
+            Some(Self::bios_limit_callback)
+        } else {
+            None
+        };
+        drv_ref.online = if T::HAS_ONLINE {
+            Some(Self::online_callback)
+        } else {
+            None
+        };
+        drv_ref.offline = if T::HAS_OFFLINE {
+            Some(Self::offline_callback)
+        } else {
+            None
+        };
+        drv_ref.exit = if T::HAS_EXIT {
+            Some(Self::exit_callback)
+        } else {
+            None
+        };
+        drv_ref.suspend = if T::HAS_SUSPEND {
+            Some(Self::suspend_callback)
+        } else {
+            None
+        };
+        drv_ref.resume = if T::HAS_RESUME {
+            Some(Self::resume_callback)
+        } else {
+            None
+        };
+        drv_ref.ready = if T::HAS_READY {
+            Some(Self::ready_callback)
+        } else {
+            None
+        };
+        drv_ref.set_boost = if T::HAS_SET_BOOST {
+            Some(Self::set_boost_callback)
+        } else {
+            None
+        };
+        drv_ref.register_em = if T::HAS_REGISTER_EM {
+            Some(Self::register_em_callback)
+        } else {
+            None
+        };
+
+        // Set driver data before registering the driver, as the cpufreq core may call few
+        // callbacks before `cpufreq_register_driver()` returns.
+        Self::set_data(drv_ref, data)?;
+
+        // SAFETY: It is safe to register the driver with the cpufreq core in the C code.
+        to_result(unsafe { bindings::cpufreq_register_driver(drv_ref) })?;
+
+        Ok(Self {
+            drv,
+            _p: PhantomData,
+        })
+    }
+
+    /// Same as [Registration::new`], but does not return a `Registration` instance.
+    /// Instead the `Registration` is owned by devres and will be revoked / dropped, once the
+    /// device is detached.
+    pub fn new_foreign_owned(
+        dev: &Device,
+        name: &'static CStr,
+        data: T::Data,
+        flags: u16,
+        boost: bool,
+    ) -> Result<()> {
+        let reg = Self::new(name, data, flags, boost)?;
+        Devres::new_foreign_owned(dev, reg, GFP_KERNEL)?;
+        Ok(())
+    }
+
+    // Sets the data for a cpufreq driver.
+    fn set_data(drv: &mut bindings::cpufreq_driver, data: T::Data) -> Result<()> {
+        if drv.driver_data.is_null() {
+            // Pass the ownership of the data to the foreign interface.
+            drv.driver_data = <T::Data as ForeignOwnable>::into_foreign(data) as _;
+            Ok(())
+        } else {
+            Err(EBUSY)
+        }
+    }
+
+    /// Returns the previous set data for a cpufreq driver.
+    pub fn data(&mut self) -> Option<<T::Data as ForeignOwnable>::Borrowed<'static>> {
+        let drv = self.drv.get_mut();
+
+        if drv.driver_data.is_null() {
+            None
+        } else {
+            // SAFETY: The data is earlier set by us from [`set_data()`].
+            Some(unsafe { <T::Data as ForeignOwnable>::borrow(drv.driver_data) })
+        }
+    }
+
+    // Clears and returns the data for a cpufreq driver.
+    fn clear_data(&mut self) -> Option<T::Data> {
+        let drv = self.drv.get_mut();
+
+        if drv.driver_data.is_null() {
+            None
+        } else {
+            // SAFETY: By the type invariants, we know that `self` owns a reference, so it is safe
+            // to relinquish it now.
+            let data = Some(unsafe { <T::Data as ForeignOwnable>::from_foreign(drv.driver_data) });
+            drv.driver_data = ptr::null_mut();
+            data
+        }
+    }
+}
+
+// cpufreq driver callbacks.
+impl<T: Driver> Registration<T> {
+    // Policy's init callback.
+    extern "C" fn init_callback(ptr: *mut bindings::cpufreq_policy) -> core::ffi::c_int {
+        from_result(|| {
+            // SAFETY: `ptr` is valid by the contract with the C code. `policy` is alive only for
+            // the duration of this call, so it is guaranteed to remain alive for the lifetime of
+            // `ptr`.
+            let mut policy = unsafe { Policy::from_raw_policy(ptr) };
+
+            let data = T::init(&mut policy)?;
+            policy.set_data(data)?;
+            Ok(0)
+        })
+    }
+
+    // Policy's exit callback.
+    extern "C" fn exit_callback(ptr: *mut bindings::cpufreq_policy) {
+        // SAFETY: `ptr` is valid by the contract with the C code. `policy` is alive only for the
+        // duration of this call, so it is guaranteed to remain alive for the lifetime of
+        // `ptr`.
+        let mut policy = unsafe { Policy::from_raw_policy(ptr) };
+
+        let data = policy.clear_data();
+        let _ = T::exit(&mut policy, data);
+    }
+
+    // Policy's online callback.
+    extern "C" fn online_callback(ptr: *mut bindings::cpufreq_policy) -> core::ffi::c_int {
+        from_result(|| {
+            // SAFETY: `ptr` is valid by the contract with the C code. `policy` is alive only for
+            // the duration of this call, so it is guaranteed to remain alive for the lifetime of
+            // `ptr`.
+            let mut policy = unsafe { Policy::from_raw_policy(ptr) };
+            T::online(&mut policy).map(|()| 0)
+        })
+    }
+
+    // Policy's offline callback.
+    extern "C" fn offline_callback(ptr: *mut bindings::cpufreq_policy) -> core::ffi::c_int {
+        from_result(|| {
+            // SAFETY: `ptr` is valid by the contract with the C code. `policy` is alive only for
+            // the duration of this call, so it is guaranteed to remain alive for the lifetime of
+            // `ptr`.
+            let mut policy = unsafe { Policy::from_raw_policy(ptr) };
+            T::offline(&mut policy).map(|()| 0)
+        })
+    }
+
+    // Policy's suspend callback.
+    extern "C" fn suspend_callback(ptr: *mut bindings::cpufreq_policy) -> core::ffi::c_int {
+        from_result(|| {
+            // SAFETY: `ptr` is valid by the contract with the C code. `policy` is alive only for
+            // the duration of this call, so it is guaranteed to remain alive for the lifetime of
+            // `ptr`.
+            let mut policy = unsafe { Policy::from_raw_policy(ptr) };
+            T::suspend(&mut policy).map(|()| 0)
+        })
+    }
+
+    // Policy's resume callback.
+    extern "C" fn resume_callback(ptr: *mut bindings::cpufreq_policy) -> core::ffi::c_int {
+        from_result(|| {
+            // SAFETY: `ptr` is valid by the contract with the C code. `policy` is alive only for
+            // the duration of this call, so it is guaranteed to remain alive for the lifetime of
+            // `ptr`.
+            let mut policy = unsafe { Policy::from_raw_policy(ptr) };
+            T::resume(&mut policy).map(|()| 0)
+        })
+    }
+
+    // Policy's ready callback.
+    extern "C" fn ready_callback(ptr: *mut bindings::cpufreq_policy) {
+        // SAFETY: `ptr` is valid by the contract with the C code. `policy` is alive only for the
+        // duration of this call, so it is guaranteed to remain alive for the lifetime of `ptr`.
+        let mut policy = unsafe { Policy::from_raw_policy(ptr) };
+        T::ready(&mut policy);
+    }
+
+    // Policy's verify callback.
+    extern "C" fn verify_callback(ptr: *mut bindings::cpufreq_policy_data) -> core::ffi::c_int {
+        from_result(|| {
+            // SAFETY: `ptr` is valid by the contract with the C code. `policy` is alive only for
+            // the duration of this call, so it is guaranteed to remain alive for the lifetime of
+            // `ptr`.
+            let mut data = unsafe { PolicyData::from_raw_policy_data(ptr) };
+            T::verify(&mut data).map(|()| 0)
+        })
+    }
+
+    // Policy's setpolicy callback.
+    extern "C" fn setpolicy_callback(ptr: *mut bindings::cpufreq_policy) -> core::ffi::c_int {
+        from_result(|| {
+            // SAFETY: `ptr` is valid by the contract with the C code. `policy` is alive only for
+            // the duration of this call, so it is guaranteed to remain alive for the lifetime of
+            // `ptr`.
+            let mut policy = unsafe { Policy::from_raw_policy(ptr) };
+            T::setpolicy(&mut policy).map(|()| 0)
+        })
+    }
+
+    // Policy's target callback.
+    extern "C" fn target_callback(
+        ptr: *mut bindings::cpufreq_policy,
+        target_freq: u32,
+        relation: u32,
+    ) -> core::ffi::c_int {
+        from_result(|| {
+            // SAFETY: `ptr` is valid by the contract with the C code. `policy` is alive only for
+            // the duration of this call, so it is guaranteed to remain alive for the lifetime of
+            // `ptr`.
+            let mut policy = unsafe { Policy::from_raw_policy(ptr) };
+            T::target(&mut policy, target_freq, Relation::new(relation)?).map(|()| 0)
+        })
+    }
+
+    // Policy's target_index callback.
+    extern "C" fn target_index_callback(
+        ptr: *mut bindings::cpufreq_policy,
+        index: u32,
+    ) -> core::ffi::c_int {
+        from_result(|| {
+            // SAFETY: `ptr` is valid by the contract with the C code. `policy` is alive only for
+            // the duration of this call, so it is guaranteed to remain alive for the lifetime of
+            // `ptr`.
+            let mut policy = unsafe { Policy::from_raw_policy(ptr) };
+            T::target_index(&mut policy, index).map(|()| 0)
+        })
+    }
+
+    // Policy's fast_switch callback.
+    extern "C" fn fast_switch_callback(
+        ptr: *mut bindings::cpufreq_policy,
+        target_freq: u32,
+    ) -> core::ffi::c_uint {
+        // SAFETY: `ptr` is valid by the contract with the C code. `policy` is alive only for the
+        // duration of this call, so it is guaranteed to remain alive for the lifetime of `ptr`.
+        let mut policy = unsafe { Policy::from_raw_policy(ptr) };
+        T::fast_switch(&mut policy, target_freq)
+    }
+
+    // Policy's adjust_perf callback.
+    extern "C" fn adjust_perf_callback(
+        cpu: u32,
+        min_perf: usize,
+        target_perf: usize,
+        capacity: usize,
+    ) {
+        if let Ok(mut policy) = Policy::from_cpu(cpu) {
+            T::adjust_perf(&mut policy, min_perf, target_perf, capacity);
+        }
+    }
+
+    // Policy's get_intermediate callback.
+    extern "C" fn get_intermediate_callback(
+        ptr: *mut bindings::cpufreq_policy,
+        index: u32,
+    ) -> core::ffi::c_uint {
+        // SAFETY: `ptr` is valid by the contract with the C code. `policy` is alive only for the
+        // duration of this call, so it is guaranteed to remain alive for the lifetime of `ptr`.
+        let mut policy = unsafe { Policy::from_raw_policy(ptr) };
+        T::get_intermediate(&mut policy, index)
+    }
+
+    // Policy's target_intermediate callback.
+    extern "C" fn target_intermediate_callback(
+        ptr: *mut bindings::cpufreq_policy,
+        index: u32,
+    ) -> core::ffi::c_int {
+        from_result(|| {
+            // SAFETY: `ptr` is valid by the contract with the C code. `policy` is alive only for
+            // the duration of this call, so it is guaranteed to remain alive for the lifetime of
+            // `ptr`.
+            let mut policy = unsafe { Policy::from_raw_policy(ptr) };
+            T::target_intermediate(&mut policy, index).map(|()| 0)
+        })
+    }
+
+    // Policy's get callback.
+    extern "C" fn get_callback(cpu: u32) -> core::ffi::c_uint {
+        Policy::from_cpu(cpu).map_or(0, |mut policy| T::get(&mut policy).map_or(0, |f| f))
+    }
+
+    // Policy's update_limit callback.
+    extern "C" fn update_limits_callback(cpu: u32) {
+        if let Ok(mut policy) = Policy::from_cpu(cpu) {
+            T::update_limits(&mut policy);
+        }
+    }
+
+    // Policy's bios_limit callback.
+    extern "C" fn bios_limit_callback(cpu: i32, limit: *mut u32) -> core::ffi::c_int {
+        from_result(|| {
+            let mut policy = Policy::from_cpu(cpu as u32)?;
+
+            // SAFETY: The pointer is guaranteed by the C code to be valid.
+            T::bios_limit(&mut policy, &mut (unsafe { *limit })).map(|()| 0)
+        })
+    }
+
+    // Policy's set_boost callback.
+    extern "C" fn set_boost_callback(
+        ptr: *mut bindings::cpufreq_policy,
+        state: i32,
+    ) -> core::ffi::c_int {
+        from_result(|| {
+            // SAFETY: `ptr` is valid by the contract with the C code. `policy` is alive only for
+            // the duration of this call, so it is guaranteed to remain alive for the lifetime of
+            // `ptr`.
+            let mut policy = unsafe { Policy::from_raw_policy(ptr) };
+            T::set_boost(&mut policy, state).map(|()| 0)
+        })
+    }
+
+    // Policy's register_em callback.
+    extern "C" fn register_em_callback(ptr: *mut bindings::cpufreq_policy) {
+        // SAFETY: `ptr` is valid by the contract with the C code. `policy` is alive only for the
+        // duration of this call, so it is guaranteed to remain alive for the lifetime of `ptr`.
+        let mut policy = unsafe { Policy::from_raw_policy(ptr) };
+        T::register_em(&mut policy);
+    }
+}
+
+impl<T: Driver> Drop for Registration<T> {
+    // Removes the registration from the kernel if it has completed successfully before.
+    fn drop(&mut self) {
+        pr_info!("Registration dropped\n");
+        let drv = self.drv.get_mut();
+
+        // SAFETY: The driver was earlier registered from `new()`.
+        unsafe { bindings::cpufreq_unregister_driver(drv) };
+
+        // Free the previously leaked memory to the C code.
+        if !drv.attr.is_null() {
+            // SAFETY: The pointer was earlier initialized from the result of `KBox::leak`.
+            unsafe { drop(KBox::from_raw(drv.attr)) };
+        }
+
+        // Free data
+        drop(self.clear_data());
+    }
+}
-- 
2.31.1.272.g89b43f80a514

Re: [PATCH V8 12/14] rust: Extend cpufreq bindings for driver registration

[Danilo Krummrich]

On Thu, Feb 06, 2025 at 02:58:33PM +0530, Viresh Kumar wrote:
> This extends the cpufreq bindings with bindings for registering a
> driver.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  rust/kernel/cpufreq.rs | 475 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 473 insertions(+), 2 deletions(-)
> 
> diff --git a/rust/kernel/cpufreq.rs b/rust/kernel/cpufreq.rs
> index 63ea816017c0..f92259d339d3 100644
> --- a/rust/kernel/cpufreq.rs
> +++ b/rust/kernel/cpufreq.rs
> @@ -9,14 +9,17 @@
>  use crate::{
>      bindings, clk, cpumask,
>      device::Device,
> -    error::{code::*, from_err_ptr, to_result, Result, VTABLE_DEFAULT_ERROR},
> +    devres::Devres,
> +    error::{code::*, from_err_ptr, from_result, to_result, Result, VTABLE_DEFAULT_ERROR},
>      prelude::*,
>      types::ForeignOwnable,
>  };
>  
>  use core::{
> +    cell::UnsafeCell,
> +    marker::PhantomData,
>      pin::Pin,
> -    ptr::self,
> +    ptr::{self, addr_of_mut},
>  };
>  
>  use macros::vtable;
> @@ -579,3 +582,471 @@ fn register_em(_policy: &mut Policy) {
>          build_error!(VTABLE_DEFAULT_ERROR)
>      }
>  }
> +
> +/// Registration of a cpufreq driver.
> +pub struct Registration<T: Driver> {
> +    drv: KBox<UnsafeCell<bindings::cpufreq_driver>>,
> +    _p: PhantomData<T>,
> +}
> +
> +// SAFETY: `Registration` doesn't offer any methods or access to fields when shared between threads
> +// or CPUs, so it is safe to share it.
> +unsafe impl<T: Driver> Sync for Registration<T> {}
> +
> +#[allow(clippy::non_send_fields_in_send_ty)]
> +// SAFETY: Registration with and unregistration from the cpufreq subsystem can happen from any
> +// thread.  Additionally, `T::Data` (which is dropped during unregistration) is `Send`, so it is
> +// okay to move `Registration` to different threads.
> +unsafe impl<T: Driver> Send for Registration<T> {}
> +
> +impl<T: Driver> Registration<T> {
> +    /// Registers a cpufreq driver with the rest of the kernel.
> +    pub fn new(name: &'static CStr, data: T::Data, flags: u16, boost: bool) -> Result<Self> {
> +        let mut drv = KBox::new(
> +            UnsafeCell::new(bindings::cpufreq_driver::default()),
> +            GFP_KERNEL,
> +        )?;
> +        let drv_ref = drv.get_mut();
> +
> +        // Account for the trailing null character.
> +        let len = name.len() + 1;
> +        if len > drv_ref.name.len() {
> +            return Err(EINVAL);
> +        };
> +
> +        // SAFETY: `name` is a valid Cstr, and we are copying it to an array of equal or larger
> +        // size.
> +        let name = unsafe { &*(name.as_bytes_with_nul() as *const [u8]) };
> +        drv_ref.name[..len].copy_from_slice(name);
> +
> +        drv_ref.boost_enabled = boost;
> +        drv_ref.flags = flags;
> +
> +        // Allocate an array of 3 pointers to be passed to the C code.
> +        let mut attr = KBox::new([ptr::null_mut(); 3], GFP_KERNEL)?;
> +        let mut next = 0;
> +
> +        // SAFETY: The C code returns a valid pointer here, which is again passed to the C code in
> +        // an array.
> +        attr[next] =
> +            unsafe { addr_of_mut!(bindings::cpufreq_freq_attr_scaling_available_freqs) as *mut _ };
> +        next += 1;
> +
> +        if boost {
> +            // SAFETY: The C code returns a valid pointer here, which is again passed to the C code
> +            // in an array.
> +            attr[next] =
> +                unsafe { addr_of_mut!(bindings::cpufreq_freq_attr_scaling_boost_freqs) as *mut _ };
> +            next += 1;
> +        }
> +        attr[next] = ptr::null_mut();
> +
> +        // Pass the ownership of the memory block to the C code. This will be freed when
> +        // the [`Registration`] object goes out of scope.
> +        drv_ref.attr = KBox::leak(attr) as *mut _;

I think this should be KBox::into_raw() instead.

> +
> +        // Initialize mandatory callbacks.
> +        drv_ref.init = Some(Self::init_callback);
> +        drv_ref.verify = Some(Self::verify_callback);
> +
> +        // Initialize optional callbacks.
> +        drv_ref.setpolicy = if T::HAS_SETPOLICY {
> +            Some(Self::setpolicy_callback)
> +        } else {
> +            None
> +        };
> +        drv_ref.target = if T::HAS_TARGET {
> +            Some(Self::target_callback)
> +        } else {
> +            None
> +        };
> +        drv_ref.target_index = if T::HAS_TARGET_INDEX {
> +            Some(Self::target_index_callback)
> +        } else {
> +            None
> +        };
> +        drv_ref.fast_switch = if T::HAS_FAST_SWITCH {
> +            Some(Self::fast_switch_callback)
> +        } else {
> +            None
> +        };
> +        drv_ref.adjust_perf = if T::HAS_ADJUST_PERF {
> +            Some(Self::adjust_perf_callback)
> +        } else {
> +            None
> +        };
> +        drv_ref.get_intermediate = if T::HAS_GET_INTERMEDIATE {
> +            Some(Self::get_intermediate_callback)
> +        } else {
> +            None
> +        };
> +        drv_ref.target_intermediate = if T::HAS_TARGET_INTERMEDIATE {
> +            Some(Self::target_intermediate_callback)
> +        } else {
> +            None
> +        };
> +        drv_ref.get = if T::HAS_GET {
> +            Some(Self::get_callback)
> +        } else {
> +            None
> +        };
> +        drv_ref.update_limits = if T::HAS_UPDATE_LIMITS {
> +            Some(Self::update_limits_callback)
> +        } else {
> +            None
> +        };
> +        drv_ref.bios_limit = if T::HAS_BIOS_LIMIT {
> +            Some(Self::bios_limit_callback)
> +        } else {
> +            None
> +        };
> +        drv_ref.online = if T::HAS_ONLINE {
> +            Some(Self::online_callback)
> +        } else {
> +            None
> +        };
> +        drv_ref.offline = if T::HAS_OFFLINE {
> +            Some(Self::offline_callback)
> +        } else {
> +            None
> +        };
> +        drv_ref.exit = if T::HAS_EXIT {
> +            Some(Self::exit_callback)
> +        } else {
> +            None
> +        };
> +        drv_ref.suspend = if T::HAS_SUSPEND {
> +            Some(Self::suspend_callback)
> +        } else {
> +            None
> +        };
> +        drv_ref.resume = if T::HAS_RESUME {
> +            Some(Self::resume_callback)
> +        } else {
> +            None
> +        };
> +        drv_ref.ready = if T::HAS_READY {
> +            Some(Self::ready_callback)
> +        } else {
> +            None
> +        };
> +        drv_ref.set_boost = if T::HAS_SET_BOOST {
> +            Some(Self::set_boost_callback)
> +        } else {
> +            None
> +        };
> +        drv_ref.register_em = if T::HAS_REGISTER_EM {
> +            Some(Self::register_em_callback)
> +        } else {
> +            None
> +        };
> +
> +        // Set driver data before registering the driver, as the cpufreq core may call few
> +        // callbacks before `cpufreq_register_driver()` returns.
> +        Self::set_data(drv_ref, data)?;
> +
> +        // SAFETY: It is safe to register the driver with the cpufreq core in the C code.
> +        to_result(unsafe { bindings::cpufreq_register_driver(drv_ref) })?;
> +
> +        Ok(Self {
> +            drv,
> +            _p: PhantomData,
> +        })
> +    }

...

> +// cpufreq driver callbacks.
> +impl<T: Driver> Registration<T> {
> +    // Policy's init callback.
> +    extern "C" fn init_callback(ptr: *mut bindings::cpufreq_policy) -> core::ffi::c_int {

I suggest to convert all the ffi types to kernel::ffi::*.

> +        from_result(|| {
> +            // SAFETY: `ptr` is valid by the contract with the C code. `policy` is alive only for
> +            // the duration of this call, so it is guaranteed to remain alive for the lifetime of
> +            // `ptr`.
> +            let mut policy = unsafe { Policy::from_raw_policy(ptr) };
> +
> +            let data = T::init(&mut policy)?;
> +            policy.set_data(data)?;
> +            Ok(0)
> +        })
> +    }

...

> +impl<T: Driver> Drop for Registration<T> {
> +    // Removes the registration from the kernel if it has completed successfully before.
> +    fn drop(&mut self) {
> +        pr_info!("Registration dropped\n");

This should be dropped.

> +        let drv = self.drv.get_mut();
> +
> +        // SAFETY: The driver was earlier registered from `new()`.
> +        unsafe { bindings::cpufreq_unregister_driver(drv) };
> +
> +        // Free the previously leaked memory to the C code.
> +        if !drv.attr.is_null() {
> +            // SAFETY: The pointer was earlier initialized from the result of `KBox::leak`.

Box::leak() returns a mutable reference, whereas Box::into_raw() returns a raw
pointer for exactly this purpose.

Now that I think of it, maybe Box::leak() should even be removed, since it
almost never makes any sense in the kernel.

> +            unsafe { drop(KBox::from_raw(drv.attr)) };

This could just be

let _ = unsafe { KBox::from_raw(drv.attr) };

At least drop() should not be within the unsafe block.

> +        }
> +
> +        // Free data
> +        drop(self.clear_data());

No need for drop(), but I also don't mind to be explicit.

> +    }
> +}
> -- 
> 2.31.1.272.g89b43f80a514
> 
> 

Re: [PATCH V8 12/14] rust: Extend cpufreq bindings for driver registration

[Alice Ryhl]

On Thu, Feb 6, 2025 at 1:04 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Thu, Feb 06, 2025 at 02:58:33PM +0530, Viresh Kumar wrote:
> > +            unsafe { drop(KBox::from_raw(drv.attr)) };
>
> This could just be
>
> let _ = unsafe { KBox::from_raw(drv.attr) };
>
> At least drop() should not be within the unsafe block.

I think we usually write this as

drop(unsafe { KBox::from_raw(drv.attr) });

Alice

Re: [PATCH V8 12/14] rust: Extend cpufreq bindings for driver registration

[Viresh Kumar]

On 06-02-25, 13:04, Danilo Krummrich wrote:
> > +            unsafe { drop(KBox::from_raw(drv.attr)) };
> 
> This could just be
> 
> let _ = unsafe { KBox::from_raw(drv.attr) };
> 
> At least drop() should not be within the unsafe block.
> 
> > +        }
> > +
> > +        // Free data
> > +        drop(self.clear_data());
> 
> No need for drop(), but I also don't mind to be explicit.

For both of these I kept the explicit drop() to avoid any potential
confusion. I do prefer them.

-- 
viresh

-------------------------8<-------------------------

diff --git a/rust/kernel/cpufreq.rs b/rust/kernel/cpufreq.rs
index ecf7c6e2cb89..d2e7913e170b 100644
--- a/rust/kernel/cpufreq.rs
+++ b/rust/kernel/cpufreq.rs
@@ -645,7 +645,7 @@ pub fn new(name: &'static CStr, data: T::Data, flags: u16, boost: bool) -> Resul
 
         // Pass the ownership of the memory block to the C code. This will be freed when
         // the [`Registration`] object goes out of scope.
-        drv_ref.attr = KBox::leak(attr) as *mut _;
+        drv_ref.attr = KBox::into_raw(attr) as *mut _;
 
         // Initialize mandatory callbacks.
         drv_ref.init = Some(Self::init_callback);
@@ -813,7 +813,7 @@ fn clear_data(&mut self) -> Option<T::Data> {
 // cpufreq driver callbacks.
 impl<T: Driver> Registration<T> {
     // Policy's init callback.
-    extern "C" fn init_callback(ptr: *mut bindings::cpufreq_policy) -> core::ffi::c_int {
+    extern "C" fn init_callback(ptr: *mut bindings::cpufreq_policy) -> kernel::ffi::c_int {
         from_result(|| {
             // SAFETY: `ptr` is valid by the contract with the C code. `policy` is alive only for
             // the duration of this call, so it is guaranteed to remain alive for the lifetime of
@@ -838,7 +838,7 @@ extern "C" fn exit_callback(ptr: *mut bindings::cpufreq_policy) {
     }
 
     // Policy's online callback.
-    extern "C" fn online_callback(ptr: *mut bindings::cpufreq_policy) -> core::ffi::c_int {
+    extern "C" fn online_callback(ptr: *mut bindings::cpufreq_policy) -> kernel::ffi::c_int {
         from_result(|| {
             // SAFETY: `ptr` is valid by the contract with the C code. `policy` is alive only for
             // the duration of this call, so it is guaranteed to remain alive for the lifetime of
@@ -849,7 +849,7 @@ extern "C" fn online_callback(ptr: *mut bindings::cpufreq_policy) -> core::ffi::
     }
 
     // Policy's offline callback.
-    extern "C" fn offline_callback(ptr: *mut bindings::cpufreq_policy) -> core::ffi::c_int {
+    extern "C" fn offline_callback(ptr: *mut bindings::cpufreq_policy) -> kernel::ffi::c_int {
         from_result(|| {
             // SAFETY: `ptr` is valid by the contract with the C code. `policy` is alive only for
             // the duration of this call, so it is guaranteed to remain alive for the lifetime of
@@ -860,7 +860,7 @@ extern "C" fn offline_callback(ptr: *mut bindings::cpufreq_policy) -> core::ffi:
     }
 
     // Policy's suspend callback.
-    extern "C" fn suspend_callback(ptr: *mut bindings::cpufreq_policy) -> core::ffi::c_int {
+    extern "C" fn suspend_callback(ptr: *mut bindings::cpufreq_policy) -> kernel::ffi::c_int {
         from_result(|| {
             // SAFETY: `ptr` is valid by the contract with the C code. `policy` is alive only for
             // the duration of this call, so it is guaranteed to remain alive for the lifetime of
@@ -871,7 +871,7 @@ extern "C" fn suspend_callback(ptr: *mut bindings::cpufreq_policy) -> core::ffi:
     }
 
     // Policy's resume callback.
-    extern "C" fn resume_callback(ptr: *mut bindings::cpufreq_policy) -> core::ffi::c_int {
+    extern "C" fn resume_callback(ptr: *mut bindings::cpufreq_policy) -> kernel::ffi::c_int {
         from_result(|| {
             // SAFETY: `ptr` is valid by the contract with the C code. `policy` is alive only for
             // the duration of this call, so it is guaranteed to remain alive for the lifetime of
@@ -890,7 +890,7 @@ extern "C" fn ready_callback(ptr: *mut bindings::cpufreq_policy) {
     }
 
     // Policy's verify callback.
-    extern "C" fn verify_callback(ptr: *mut bindings::cpufreq_policy_data) -> core::ffi::c_int {
+    extern "C" fn verify_callback(ptr: *mut bindings::cpufreq_policy_data) -> kernel::ffi::c_int {
         from_result(|| {
             // SAFETY: `ptr` is valid by the contract with the C code. `policy` is alive only for
             // the duration of this call, so it is guaranteed to remain alive for the lifetime of
@@ -901,7 +901,7 @@ extern "C" fn verify_callback(ptr: *mut bindings::cpufreq_policy_data) -> core::
     }
 
     // Policy's setpolicy callback.
-    extern "C" fn setpolicy_callback(ptr: *mut bindings::cpufreq_policy) -> core::ffi::c_int {
+    extern "C" fn setpolicy_callback(ptr: *mut bindings::cpufreq_policy) -> kernel::ffi::c_int {
         from_result(|| {
             // SAFETY: `ptr` is valid by the contract with the C code. `policy` is alive only for
             // the duration of this call, so it is guaranteed to remain alive for the lifetime of
@@ -916,7 +916,7 @@ extern "C" fn target_callback(
         ptr: *mut bindings::cpufreq_policy,
         target_freq: u32,
         relation: u32,
-    ) -> core::ffi::c_int {
+    ) -> kernel::ffi::c_int {
         from_result(|| {
             // SAFETY: `ptr` is valid by the contract with the C code. `policy` is alive only for
             // the duration of this call, so it is guaranteed to remain alive for the lifetime of
@@ -930,7 +930,7 @@ extern "C" fn target_callback(
     extern "C" fn target_index_callback(
         ptr: *mut bindings::cpufreq_policy,
         index: u32,
-    ) -> core::ffi::c_int {
+    ) -> kernel::ffi::c_int {
         from_result(|| {
             // SAFETY: `ptr` is valid by the contract with the C code. `policy` is alive only for
             // the duration of this call, so it is guaranteed to remain alive for the lifetime of
@@ -944,7 +944,7 @@ extern "C" fn target_index_callback(
     extern "C" fn fast_switch_callback(
         ptr: *mut bindings::cpufreq_policy,
         target_freq: u32,
-    ) -> core::ffi::c_uint {
+    ) -> kernel::ffi::c_uint {
         // SAFETY: `ptr` is valid by the contract with the C code. `policy` is alive only for the
         // duration of this call, so it is guaranteed to remain alive for the lifetime of `ptr`.
         let mut policy = unsafe { Policy::from_raw_policy(ptr) };
@@ -967,7 +967,7 @@ extern "C" fn adjust_perf_callback(
     extern "C" fn get_intermediate_callback(
         ptr: *mut bindings::cpufreq_policy,
         index: u32,
-    ) -> core::ffi::c_uint {
+    ) -> kernel::ffi::c_uint {
         // SAFETY: `ptr` is valid by the contract with the C code. `policy` is alive only for the
         // duration of this call, so it is guaranteed to remain alive for the lifetime of `ptr`.
         let mut policy = unsafe { Policy::from_raw_policy(ptr) };
@@ -978,7 +978,7 @@ extern "C" fn get_intermediate_callback(
     extern "C" fn target_intermediate_callback(
         ptr: *mut bindings::cpufreq_policy,
         index: u32,
-    ) -> core::ffi::c_int {
+    ) -> kernel::ffi::c_int {
         from_result(|| {
             // SAFETY: `ptr` is valid by the contract with the C code. `policy` is alive only for
             // the duration of this call, so it is guaranteed to remain alive for the lifetime of
@@ -989,7 +989,7 @@ extern "C" fn target_intermediate_callback(
     }
 
     // Policy's get callback.
-    extern "C" fn get_callback(cpu: u32) -> core::ffi::c_uint {
+    extern "C" fn get_callback(cpu: u32) -> kernel::ffi::c_uint {
         Policy::from_cpu(cpu).map_or(0, |mut policy| T::get(&mut policy).map_or(0, |f| f))
     }
 
@@ -1001,7 +1001,7 @@ extern "C" fn update_limits_callback(cpu: u32) {
     }
 
     // Policy's bios_limit callback.
-    extern "C" fn bios_limit_callback(cpu: i32, limit: *mut u32) -> core::ffi::c_int {
+    extern "C" fn bios_limit_callback(cpu: i32, limit: *mut u32) -> kernel::ffi::c_int {
         from_result(|| {
             let mut policy = Policy::from_cpu(cpu as u32)?;
 
@@ -1014,7 +1014,7 @@ extern "C" fn bios_limit_callback(cpu: i32, limit: *mut u32) -> core::ffi::c_int
     extern "C" fn set_boost_callback(
         ptr: *mut bindings::cpufreq_policy,
         state: i32,
-    ) -> core::ffi::c_int {
+    ) -> kernel::ffi::c_int {
         from_result(|| {
             // SAFETY: `ptr` is valid by the contract with the C code. `policy` is alive only for
             // the duration of this call, so it is guaranteed to remain alive for the lifetime of
@@ -1036,7 +1036,6 @@ extern "C" fn register_em_callback(ptr: *mut bindings::cpufreq_policy) {
 impl<T: Driver> Drop for Registration<T> {
     // Removes the registration from the kernel if it has completed successfully before.
     fn drop(&mut self) {
-        pr_info!("Registration dropped\n");
         let drv = self.drv.get_mut();
 
         // SAFETY: The driver was earlier registered from `new()`.
@@ -1044,8 +1043,8 @@ fn drop(&mut self) {
 
         // Free the previously leaked memory to the C code.
         if !drv.attr.is_null() {
-            // SAFETY: The pointer was earlier initialized from the result of `KBox::leak`.
-            unsafe { drop(KBox::from_raw(drv.attr)) };
+            // SAFETY: The pointer was earlier initialized from the result of `KBox::into_raw()`.
+            drop(unsafe { KBox::from_raw(drv.attr) });
         }
 
         // Free data
diff --git a/rust/kernel/opp.rs b/rust/kernel/opp.rs
index 4030953c2001..b83bd97a4f37 100644
--- a/rust/kernel/opp.rs
+++ b/rust/kernel/opp.rs
@@ -300,9 +300,9 @@ extern "C" fn config_clks(
         dev: *mut bindings::device,
         opp_table: *mut bindings::opp_table,
         opp: *mut bindings::dev_pm_opp,
-        _data: *mut core::ffi::c_void,
+        _data: *mut kernel::ffi::c_void,
         scaling_down: bool,
-    ) -> core::ffi::c_int {
+    ) -> kernel::ffi::c_int {
         from_result(|| {
             // SAFETY: 'dev' is guaranteed by the C code to be valid.
             let dev = unsafe { Device::get_device(dev) };
@@ -324,8 +324,8 @@ extern "C" fn config_regulators(
         old_opp: *mut bindings::dev_pm_opp,
         new_opp: *mut bindings::dev_pm_opp,
         regulators: *mut *mut bindings::regulator,
-        count: core::ffi::c_uint,
-    ) -> core::ffi::c_int {
+        count: kernel::ffi::c_uint,
+    ) -> kernel::ffi::c_int {
         from_result(|| {
             // SAFETY: 'dev' is guaranteed by the C code to be valid.
             let dev = unsafe { Device::get_device(dev) };

[PATCH V8 13/14] rust: Extend OPP bindings with CPU frequency table

[Viresh Kumar]

This commit adds bindings for CPUFreq core related API.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 rust/kernel/opp.rs | 62 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 62 insertions(+)

diff --git a/rust/kernel/opp.rs b/rust/kernel/opp.rs
index 8028245e94aa..4030953c2001 100644
--- a/rust/kernel/opp.rs
+++ b/rust/kernel/opp.rs
@@ -16,6 +16,12 @@
     types::{ARef, AlwaysRefCounted, Opaque},
 };
 
+#[cfg(CONFIG_CPU_FREQ)]
+use crate::cpufreq;
+
+#[cfg(CONFIG_CPU_FREQ)]
+use core::ops::Deref;
+
 use core::{marker::PhantomData, ptr};
 
 use macros::vtable;
@@ -337,6 +343,56 @@ extern "C" fn config_regulators(
     }
 }
 
+/// CPU Frequency table created from OPP entries.
+#[cfg(CONFIG_CPU_FREQ)]
+pub struct FreqTable {
+    dev: ARef<Device>,
+    table: cpufreq::Table,
+}
+
+#[cfg(CONFIG_CPU_FREQ)]
+impl FreqTable {
+    /// Creates new instance of [`FreqTable`] from raw pointer.
+    fn new(table: &Table) -> Result<Self> {
+        let mut ptr: *mut bindings::cpufreq_frequency_table = ptr::null_mut();
+
+        // SAFETY: The requirements are satisfied by the existence of `Device` and its safety
+        // requirements.
+        to_result(unsafe {
+            bindings::dev_pm_opp_init_cpufreq_table(table.dev.as_raw(), &mut ptr)
+        })?;
+        Ok(Self {
+            dev: table.dev.clone(),
+            // SAFETY: The `ptr` is guaranteed by the C code to be valid.
+            table: unsafe { cpufreq::Table::from_raw(ptr) },
+        })
+    }
+
+    /// Returns reference to the underlying [`cpufreq::Table`].
+    pub fn table(&self) -> &cpufreq::Table {
+        &self.table
+    }
+}
+
+#[cfg(CONFIG_CPU_FREQ)]
+impl Deref for FreqTable {
+    type Target = cpufreq::Table;
+
+    #[inline]
+    fn deref(&self) -> &Self::Target {
+        &self.table
+    }
+}
+
+#[cfg(CONFIG_CPU_FREQ)]
+impl Drop for FreqTable {
+    fn drop(&mut self) {
+        // SAFETY: The requirements are satisfied by the existence of `Device` and its safety
+        // requirements.
+        unsafe { bindings::dev_pm_opp_free_cpufreq_table(self.dev.as_raw(), &mut self.as_raw()) };
+    }
+}
+
 /// Operating performance point (OPP) table.
 ///
 /// Wraps the kernel's `struct opp_table`.
@@ -540,6 +596,12 @@ pub fn adjust_voltage(
         })
     }
 
+    /// Create cpufreq table from OPP table.
+    #[cfg(CONFIG_CPU_FREQ)]
+    pub fn to_cpufreq_table(&mut self) -> Result<FreqTable> {
+        FreqTable::new(self)
+    }
+
     /// Sets a matching OPP based on frequency.
     pub fn set_rate(&self, freq: usize) -> Result<()> {
         // SAFETY: The requirements are satisfied by the existence of `Device` and its safety
-- 
2.31.1.272.g89b43f80a514

[PATCH V8 14/14] cpufreq: Add Rust based cpufreq-dt driver

[Viresh Kumar]

This commit adds a Rust based cpufreq-dt driver, which covers most of
the functionality of the existing C based driver. Only a handful of
things are left, like fetching platform data from cpufreq-dt-platdev.c.

This is tested with the help of QEMU for now and switching of
frequencies work as expected.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/Kconfig        |  12 ++
 drivers/cpufreq/Makefile       |   1 +
 drivers/cpufreq/rcpufreq_dt.rs | 238 +++++++++++++++++++++++++++++++++
 rust/kernel/cpufreq.rs         |  10 +-
 4 files changed, 257 insertions(+), 4 deletions(-)
 create mode 100644 drivers/cpufreq/rcpufreq_dt.rs

diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig
index d64b07ec48e5..78702a08364f 100644
--- a/drivers/cpufreq/Kconfig
+++ b/drivers/cpufreq/Kconfig
@@ -217,6 +217,18 @@ config CPUFREQ_DT
 
 	  If in doubt, say N.
 
+config CPUFREQ_DT_RUST
+	tristate "Rust based Generic DT based cpufreq driver"
+	depends on HAVE_CLK && OF && RUST
+	select CPUFREQ_DT_PLATDEV
+	select PM_OPP
+	help
+	  This adds a Rust based generic DT based cpufreq driver for frequency
+	  management.  It supports both uniprocessor (UP) and symmetric
+	  multiprocessor (SMP) systems.
+
+	  If in doubt, say N.
+
 config CPUFREQ_VIRT
 	tristate "Virtual cpufreq driver"
 	depends on GENERIC_ARCH_TOPOLOGY
diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
index 890fff99f37d..7fa29bdec3c7 100644
--- a/drivers/cpufreq/Makefile
+++ b/drivers/cpufreq/Makefile
@@ -15,6 +15,7 @@ obj-$(CONFIG_CPU_FREQ_GOV_COMMON)		+= cpufreq_governor.o
 obj-$(CONFIG_CPU_FREQ_GOV_ATTR_SET)	+= cpufreq_governor_attr_set.o
 
 obj-$(CONFIG_CPUFREQ_DT)		+= cpufreq-dt.o
+obj-$(CONFIG_CPUFREQ_DT_RUST)		+= rcpufreq_dt.o
 obj-$(CONFIG_CPUFREQ_DT_PLATDEV)	+= cpufreq-dt-platdev.o
 obj-$(CONFIG_CPUFREQ_VIRT)		+= virtual-cpufreq.o
 
diff --git a/drivers/cpufreq/rcpufreq_dt.rs b/drivers/cpufreq/rcpufreq_dt.rs
new file mode 100644
index 000000000000..461e88006ed9
--- /dev/null
+++ b/drivers/cpufreq/rcpufreq_dt.rs
@@ -0,0 +1,238 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Rust based implementation of the cpufreq-dt driver.
+
+use core::format_args;
+
+use kernel::{
+    c_str, clk, cpu, cpufreq, cpumask::Cpumask, device::Device, error::code::*, fmt,
+    macros::vtable, module_platform_driver, of, opp, platform, prelude::*, str::CString, sync::Arc,
+};
+
+// Finds exact supply name from the OF node.
+fn find_supply_name_exact(dev: &Device, name: &str) -> Option<CString> {
+    let name_cstr = CString::try_from_fmt(fmt!("{}-supply", name)).ok()?;
+
+    if dev.property_present(&name_cstr) {
+        CString::try_from_fmt(fmt!("{}", name)).ok()
+    } else {
+        None
+    }
+}
+
+// Finds supply name for the CPU from DT.
+fn find_supply_names(dev: &Device, cpu: u32) -> Option<KVec<CString>> {
+    // Try "cpu0" for older DTs.
+    let name = match cpu {
+        0 => find_supply_name_exact(dev, "cpu0"),
+        _ => None,
+    }
+    .or(find_supply_name_exact(dev, "cpu"))?;
+
+    let mut list = KVec::with_capacity(1, GFP_KERNEL).ok()?;
+    list.push(name, GFP_KERNEL).ok()?;
+
+    Some(list)
+}
+
+// Represents the cpufreq dt device.
+struct CPUFreqDTDevice {
+    opp_table: opp::Table,
+    freq_table: opp::FreqTable,
+    #[allow(dead_code)]
+    mask: Cpumask,
+    #[allow(dead_code)]
+    token: Option<opp::ConfigToken>,
+    #[allow(dead_code)]
+    clk: clk::Clk,
+}
+
+struct CPUFreqDTDriver {
+    _pdev: platform::Device,
+}
+
+#[vtable]
+impl opp::ConfigOps for CPUFreqDTDriver {}
+
+#[vtable]
+impl cpufreq::Driver for CPUFreqDTDriver {
+    type Data = ();
+    type PData = Arc<CPUFreqDTDevice>;
+
+    fn init(policy: &mut cpufreq::Policy) -> Result<Self::PData> {
+        let cpu = policy.cpu();
+        // SAFETY: The CPU device is only used from the init() callback during which the CPU can't
+        // get hot-unplugged. Also the cpufreq core in C library, registers with CPU notifiers and
+        // the cpufreq core/driver won't use the CPU device, once the CPU is hot-unplugged.
+        let dev = unsafe { cpu::from_cpu(cpu)? };
+        let mut mask = Cpumask::new()?;
+
+        mask.set(cpu);
+
+        let token = match find_supply_names(dev, cpu) {
+            Some(names) => Some(
+                opp::Config::<Self>::new()
+                    .set_regulator_names(names)?
+                    .set(dev)?,
+            ),
+            _ => None,
+        };
+
+        // Get OPP-sharing information from "operating-points-v2" bindings.
+        let fallback = match opp::Table::of_sharing_cpus(dev, &mut mask) {
+            Ok(()) => false,
+            Err(e) => {
+                if e != ENOENT {
+                    return Err(e);
+                }
+
+                // "operating-points-v2" not supported. If the platform hasn't
+                // set sharing CPUs, fallback to all CPUs share the `Policy`
+                // for backward compatibility.
+                opp::Table::sharing_cpus(dev, &mut mask).is_err()
+            }
+        };
+
+        // Initialize OPP tables for all policy cpus.
+        //
+        // For platforms not using "operating-points-v2" bindings, we do this
+        // before updating policy cpus. Otherwise, we will end up creating
+        // duplicate OPPs for the CPUs.
+        //
+        // OPPs might be populated at runtime, don't fail for error here unless
+        // it is -EPROBE_DEFER.
+        let mut opp_table = match opp::Table::from_of_cpumask(dev, &mut mask) {
+            Ok(table) => table,
+            Err(e) => {
+                if e == EPROBE_DEFER {
+                    return Err(e);
+                }
+
+                // The table is added dynamically ?
+                opp::Table::from_dev(dev)?
+            }
+        };
+
+        // The OPP table must be initialized, statically or dynamically, by this point.
+        opp_table.opp_count()?;
+
+        // Set sharing cpus for fallback scenario.
+        if fallback {
+            mask.set_all();
+            opp_table.set_sharing_cpus(&mut mask)?;
+        }
+
+        let mut transition_latency = opp_table.max_transition_latency() as u32;
+        if transition_latency == 0 {
+            transition_latency = cpufreq::ETERNAL_LATENCY;
+        }
+
+        let freq_table = opp_table.to_cpufreq_table()?;
+        let clk = policy
+            .set_freq_table(freq_table.table())
+            .set_dvfs_possible_from_any_cpu()
+            .set_suspend_freq((opp_table.suspend_freq() / 1000) as u32)
+            .set_transition_latency(transition_latency)
+            .set_clk(dev, None)?;
+
+        mask.copy(policy.cpus());
+
+        Ok(Arc::new(
+            CPUFreqDTDevice {
+                opp_table,
+                freq_table,
+                mask,
+                token,
+                clk,
+            },
+            GFP_KERNEL,
+        )?)
+    }
+
+    fn exit(_policy: &mut cpufreq::Policy, _data: Option<Self::PData>) -> Result<()> {
+        Ok(())
+    }
+
+    fn online(_policy: &mut cpufreq::Policy) -> Result<()> {
+        // We did light-weight tear down earlier, nothing to do here.
+        Ok(())
+    }
+
+    fn offline(_policy: &mut cpufreq::Policy) -> Result<()> {
+        // Preserve policy->data and don't free resources on light-weight
+        // tear down.
+        Ok(())
+    }
+
+    fn suspend(policy: &mut cpufreq::Policy) -> Result<()> {
+        policy.generic_suspend()
+    }
+
+    fn verify(data: &mut cpufreq::PolicyData) -> Result<()> {
+        data.generic_verify()
+    }
+
+    fn target_index(policy: &mut cpufreq::Policy, index: u32) -> Result<()> {
+        let data = match policy.data::<Self::PData>() {
+            Some(data) => data,
+            None => return Err(ENOENT),
+        };
+
+        let freq = data.freq_table.freq(index.try_into().unwrap())? as usize;
+        data.opp_table.set_rate(freq * 1000)
+    }
+
+    fn get(policy: &mut cpufreq::Policy) -> Result<u32> {
+        policy.generic_get()
+    }
+
+    fn set_boost(_policy: &mut cpufreq::Policy, _state: i32) -> Result<()> {
+        Ok(())
+    }
+
+    fn register_em(policy: &mut cpufreq::Policy) {
+        policy.register_em_opp()
+    }
+}
+
+kernel::of_device_table!(
+    OF_TABLE,
+    MODULE_OF_TABLE,
+    <CPUFreqDTDriver as platform::Driver>::IdInfo,
+    [(of::DeviceId::new(c_str!("operating-points-v2")), ())]
+);
+
+impl platform::Driver for CPUFreqDTDriver {
+    type IdInfo = ();
+    const OF_ID_TABLE: Option<of::IdTable<Self::IdInfo>> = Some(&OF_TABLE);
+
+    fn probe(
+        pdev: &mut platform::Device,
+        _id_info: Option<&Self::IdInfo>,
+    ) -> Result<Pin<KBox<Self>>> {
+        cpufreq::Registration::<CPUFreqDTDriver>::new_foreign_owned(
+            pdev.as_ref(),
+            c_str!("cpufreq-dt"),
+            (),
+            cpufreq::flags::NEED_INITIAL_FREQ_CHECK | cpufreq::flags::IS_COOLING_DEV,
+            true,
+        )?;
+
+        let drvdata = KBox::new(
+            Self {
+                _pdev: pdev.clone(),
+            },
+            GFP_KERNEL,
+        )?;
+
+        Ok(drvdata.into())
+    }
+}
+
+module_platform_driver! {
+    type: CPUFreqDTDriver,
+    name: "cpufreq-dt",
+    author: "Viresh Kumar <viresh.kumar@linaro.org>",
+    description: "Generic CPUFreq DT driver",
+    license: "GPL v2",
+}
diff --git a/rust/kernel/cpufreq.rs b/rust/kernel/cpufreq.rs
index f92259d339d3..ecf7c6e2cb89 100644
--- a/rust/kernel/cpufreq.rs
+++ b/rust/kernel/cpufreq.rs
@@ -628,15 +628,17 @@ pub fn new(name: &'static CStr, data: T::Data, flags: u16, boost: bool) -> Resul
 
         // SAFETY: The C code returns a valid pointer here, which is again passed to the C code in
         // an array.
-        attr[next] =
-            unsafe { addr_of_mut!(bindings::cpufreq_freq_attr_scaling_available_freqs) as *mut _ };
+        attr[next] = unsafe {
+            addr_of_mut!(bindings::cpufreq_freq_attr_scaling_available_freqs) as *mut _
+        };
         next += 1;
 
         if boost {
             // SAFETY: The C code returns a valid pointer here, which is again passed to the C code
             // in an array.
-            attr[next] =
-                unsafe { addr_of_mut!(bindings::cpufreq_freq_attr_scaling_boost_freqs) as *mut _ };
+            attr[next] = unsafe {
+                addr_of_mut!(bindings::cpufreq_freq_attr_scaling_boost_freqs) as *mut _
+            };
             next += 1;
         }
         attr[next] = ptr::null_mut();
-- 
2.31.1.272.g89b43f80a514

Re: [PATCH V8 00/14] Rust bindings for cpufreq and OPP core + sample driver

[Danilo Krummrich]

Hi Viresh,

On Thu, Feb 06, 2025 at 02:58:21PM +0530, Viresh Kumar wrote:
> Hello,
> 
> I am seeking a few Acks for this patch series before merging it into the PM tree
> for the 6.15 merge window, unless there are any objections.
> 
> This series introduces initial Rust bindings for two subsystems: cpufreq and
> Operating Performance Points (OPP). The bindings cover most of the interfaces
> exposed by these subsystems. It also includes minimal bindings for the clk and
> cpumask frameworks, which are required by the cpufreq bindings.
> 
> Additionally, a sample cpufreq driver, rcpufreq-dt, is included. This is a
> duplicate of the existing cpufreq-dt driver, which is a platform-agnostic,
> device-tree-based driver commonly used on ARM platforms.
> 
> The implementation has been tested using QEMU, ensuring that frequency
> transitions, various configurations, and driver binding/unbinding work as
> expected. However, performance measurements have not been conducted yet.
> 
> For those interested in testing these patches, they can be found at:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/vireshk/linux.git rust/cpufreq-dt
> 
> This version is rebased on v6.14-rc1.

I gave it a quick shot and it seems there are a few Clippy warnings, plus
rustfmtcheck complains.

There are also two minor checkpatch complaints about line length.

Re: [PATCH V8 00/14] Rust bindings for cpufreq and OPP core + sample driver

[Viresh Kumar]

Hi Danilo,

On 06-02-25, 12:45, Danilo Krummrich wrote:
> I gave it a quick shot and it seems there are a few Clippy warnings,

I could find only one (related to core::format_args), are there more ?

(Earlier I had a debug commit on top of the series in my branch and
Clippy didn't give any warnings as format_flags was getting used from
there.)

> plus rustfmtcheck complains.

I am not sure how to solve them.

Diff in rust/kernel/cpufreq.rs at line 628:

         // SAFETY: The C code returns a valid pointer here, which is again passed to the C code in
         // an array.
-        attr[next] = unsafe {
-            addr_of_mut!(bindings::cpufreq_freq_attr_scaling_available_freqs) as *mut _
-        };
+        attr[next] =
+            unsafe { addr_of_mut!(bindings::cpufreq_freq_attr_scaling_available_freqs) as *mut _ };
         next += 1;

         if boost {

If I move the code as suggested here, then I get warning about not
adding a SAFETY comment for unsafe code (which looks to be a tool
specific bug).

I can move the entire thing into the unsafe block, but the assignment
to attr[next] isn't unsafe.

What do yo suggest here ?

> There are also two minor checkpatch complaints about line length.

Yeah, they came from the first commit (which wasn't written by me and
so I avoided touching it), fixed now.

-- 
viresh

Re: [PATCH V8 00/14] Rust bindings for cpufreq and OPP core + sample driver

[Miguel Ojeda]

On Fri, Feb 7, 2025 at 8:15 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> If I move the code as suggested here, then I get warning about not
> adding a SAFETY comment for unsafe code (which looks to be a tool
> specific bug).

The warning is there even if you don't run `rustfmt`, and it does not
look like a bug to me -- what Clippy is complaining about is that you
don't actually need the `unsafe` block to begin with:

    error: unnecessary `unsafe` block
    --> rust/kernel/cpufreq.rs:631:22
        |
    631 |         attr[next] = unsafe {
        |                      ^^^^^^ unnecessary `unsafe` block
        |
        = note: `-D unused-unsafe` implied by `-D warnings`
        = help: to override `-D warnings` add `#[allow(unused_unsafe)]`

since those operations are safe. Or am I missing something?

Then, when you remove it, Clippy will complain that there should not
be a SAFETY comment:

    error: statement has unnecessary safety comment
    --> rust/kernel/cpufreq.rs:625:9
        |
    625 |         attr[next] =
addr_of_mut!(bindings::cpufreq_freq_attr_scaling_available_freqs) as
*mut _;
        |
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
        |
    help: consider removing the safety comment
    --> rust/kernel/cpufreq.rs:623:9
        |
    623 |         // SAFETY: The C code returns a valid pointer here,
which is again passed to the C code in
        |
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
        = help: for further information visit
https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_safety_comment
        = note: `-D clippy::unnecessary-safety-comment` implied by `-D warnings`
        = help: to override `-D warnings` add
`#[allow(clippy::unnecessary_safety_comment)]`

And `rustfmt` will put things in a single line, since now they fit.

I would suggest reviewing all the SAFETY comments around this code,
i.e. something may be wrong, since these were not needed, and thus you
may have wanted to describe them elsewhere.

In any case, passing `rustfmtcheck` is a requirement. So in the worst
case, if you do find such a bug in e.g. Clippy, you may always
`expect` or `allow` the lint or disable `rustfmt` in that region of
code. But that should be really rare, and in such a case it should be
reported upstream.

I also found other build issues in the branch you mention in your
cover letter, so please double-check everything looks good before
adding it to linux-next. Please also make it Clippy-clean.

I hope that helps!

Cheers,
Miguel

Re: [PATCH V8 00/14] Rust bindings for cpufreq and OPP core + sample driver

[Viresh Kumar]

Hi Miguel,

On 07-02-25, 12:07, Miguel Ojeda wrote:
> The warning is there even if you don't run `rustfmt`, and it does not
> look like a bug to me -- what Clippy is complaining about is that you
> don't actually need the `unsafe` block to begin with:
> 
>     error: unnecessary `unsafe` block
>     --> rust/kernel/cpufreq.rs:631:22
>         |
>     631 |         attr[next] = unsafe {
>         |                      ^^^^^^ unnecessary `unsafe` block
>         |
>         = note: `-D unused-unsafe` implied by `-D warnings`
>         = help: to override `-D warnings` add `#[allow(unused_unsafe)]`
> 
> since those operations are safe. Or am I missing something?

One thing you are missing is the right branch to test. I mentioned the
wrong tree in cover-letter (though I don't remember getting above
errors earlier too, not sure why you are getting them) :(

git://git.kernel.org/pub/scm/linux/kernel/git/vireshk/pm.git rust/cpufreq-dt

This patchset was generated correctly though.

I don't get anything with CLIPPY with this branch, with rustfmtcheck I
get:

         // SAFETY: The C code returns a valid pointer here, which is again passed to the C code in
         // an array.
-        attr[next] = unsafe {
-            addr_of_mut!(bindings::cpufreq_freq_attr_scaling_available_freqs) as *mut _
-        };
+        attr[next] =
+            unsafe { addr_of_mut!(bindings::cpufreq_freq_attr_scaling_available_freqs) as *mut _ };
         next += 1;


If I make the above changes I get following with CLIPPY:

$ make CLIPPY=1 ARCH=arm64 O=../barm64t/ -j8 CROSS_COMPILE=aarch64-linux-gnu- CONFIG_DEBUG_SECTION_MISMATCH=y

warning: unsafe block missing a safety comment
   --> /mnt/ssd/all/work/repos/kernel/linux/rust/kernel/cpufreq.rs:632:13
    |
632 |             unsafe { addr_of_mut!(bindings::cpufreq_freq_attr_scaling_available_freqs) as *mut _ };
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = help: consider adding a safety comment on the preceding line
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#undocumented_unsafe_blocks
    = note: requested on the command line with `-W clippy::undocumented-unsafe-blocks`

warning: unsafe block missing a safety comment
   --> /mnt/ssd/all/work/repos/kernel/linux/rust/kernel/cpufreq.rs:639:17
    |
639 |                 unsafe { addr_of_mut!(bindings::cpufreq_freq_attr_scaling_boost_freqs) as *mut _ };
    |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = help: consider adding a safety comment on the preceding line
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#undocumented_unsafe_blocks

warning: 2 warnings emitted


This I thought was a bug (I may have seen this with other Rust
projects too, from what I can remember).

If I drop the unsafe here I get:

error[E0133]: use of mutable static is unsafe and requires unsafe block
   --> /mnt/ssd/all/work/repos/kernel/linux/rust/kernel/cpufreq.rs:632:26
    |
632 |             addr_of_mut!(bindings::cpufreq_freq_attr_scaling_available_freqs) as *mut _;
    |                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ use of mutable static
    |
    = note: mutable statics can be mutated by multiple threads: aliasing violations or data races will cause undefined behavior

I don't remember seeing a CLIPPY warning ever about removing the
unsafe here, as reported in your case.

> In any case, passing `rustfmtcheck` is a requirement. So in the worst
> case, if you do find such a bug in e.g. Clippy, you may always
> `expect` or `allow` the lint or disable `rustfmt` in that region of
> code. But that should be really rare, and in such a case it should be
> reported upstream.

It would require clippy::undocumented-unsafe-blocks to be disabled, in
this case.

> I also found other build issues in the branch you mention in your
> cover letter, so please double-check everything looks good before
> adding it to linux-next. Please also make it Clippy-clean.

Sorry about that, maybe there were other issues with the earlier
branch. Can you please try again from the tree I mentioned above ?

-- 
viresh

Re: [PATCH V8 00/14] Rust bindings for cpufreq and OPP core + sample driver

[Miguel Ojeda]

On Mon, Feb 10, 2025 at 9:06 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> error[E0133]: use of mutable static is unsafe and requires unsafe block
>    --> /mnt/ssd/all/work/repos/kernel/linux/rust/kernel/cpufreq.rs:632:26
>     |
> 632 |             addr_of_mut!(bindings::cpufreq_freq_attr_scaling_available_freqs) as *mut _;
>     |                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ use of mutable static

Ah, I see now -- yeah, this is due to:

    https://blog.rust-lang.org/2024/10/17/Rust-1.82.0.html#safely-addressing-unsafe-statics

You could do (probably with a comment):

        pub fn new(name: &'static CStr, data: T::Data, flags: u16,
boost: bool) -> Result<Self> {
    +        #![allow(unused_unsafe)]
    +
            let mut drv = KBox::new(

Yeah, a bit annoying... :(

> I don't remember seeing a CLIPPY warning ever about removing the
> unsafe here, as reported in your case.

Please use a newer version to see them, e.g. the latest stable 1.84.1.

In general, please test patches with the minimum version and the
latest stable. The latest will give you more lints in general, and the
minimum will make sure it builds for older versions.

> It would require clippy::undocumented-unsafe-blocks to be disabled, in
> this case.

Hmm... why? We need the `allow` above because we need to keep the
`unsafe` for older Rust. But you can provide a comment nevertheless,
even if "dummy", so you should not need to disable anything else.

With your branch + the `allow` above + running `rustfmt`, it is Clippy
clean for me. Please see the diff below as an example (I also cleaned
the other Clippy warning -- and sorry for the wrapping).

Cheers,
Miguel

diff --git a/rust/kernel/cpufreq.rs b/rust/kernel/cpufreq.rs
index d2e7913e170b..e7c62770fc3b 100644
--- a/rust/kernel/cpufreq.rs
+++ b/rust/kernel/cpufreq.rs
@@ -602,6 +602,8 @@ unsafe impl<T: Driver> Send for Registration<T> {}
 impl<T: Driver> Registration<T> {
     /// Registers a cpufreq driver with the rest of the kernel.
     pub fn new(name: &'static CStr, data: T::Data, flags: u16, boost:
bool) -> Result<Self> {
+        #![allow(unused_unsafe)]
+
         let mut drv = KBox::new(
             UnsafeCell::new(bindings::cpufreq_driver::default()),
             GFP_KERNEL,
@@ -626,19 +628,15 @@ pub fn new(name: &'static CStr, data: T::Data,
flags: u16, boost: bool) -> Resul
         let mut attr = KBox::new([ptr::null_mut(); 3], GFP_KERNEL)?;
         let mut next = 0;

-        // SAFETY: The C code returns a valid pointer here, which is
again passed to the C code in
-        // an array.
-        attr[next] = unsafe {
-            addr_of_mut!(bindings::cpufreq_freq_attr_scaling_available_freqs)
as *mut _
-        };
+        attr[next] =
+            // SAFETY: ...
+            unsafe {
addr_of_mut!(bindings::cpufreq_freq_attr_scaling_available_freqs) as
*mut _ };
         next += 1;

         if boost {
-            // SAFETY: The C code returns a valid pointer here, which
is again passed to the C code
-            // in an array.
-            attr[next] = unsafe {
-
addr_of_mut!(bindings::cpufreq_freq_attr_scaling_boost_freqs) as *mut
_
-            };
+            attr[next] =
+                // SAFETY: ...
+                unsafe {
addr_of_mut!(bindings::cpufreq_freq_attr_scaling_boost_freqs) as *mut
_ };
             next += 1;
         }
         attr[next] = ptr::null_mut();
diff --git a/rust/kernel/opp.rs b/rust/kernel/opp.rs
index b83bd97a4f37..aaf650f46ccb 100644
--- a/rust/kernel/opp.rs
+++ b/rust/kernel/opp.rs
@@ -781,7 +781,6 @@ fn drop(&mut self) {
 /// represents a pointer that owns a reference count on the OPP.
 ///
 /// A reference to the `OPP`, `&OPP` isn't refcounted by the Rust code.
-
 #[repr(transparent)]
 pub struct OPP(Opaque<bindings::dev_pm_opp>);

Re: [PATCH V8 00/14] Rust bindings for cpufreq and OPP core + sample driver

[Viresh Kumar]

On 17-02-25, 09:39, Miguel Ojeda wrote:
> Ah, I see now -- yeah, this is due to:
> 
>     https://blog.rust-lang.org/2024/10/17/Rust-1.82.0.html#safely-addressing-unsafe-statics
> 
> You could do (probably with a comment):
> 
>         pub fn new(name: &'static CStr, data: T::Data, flags: u16,
> boost: bool) -> Result<Self> {
>     +        #![allow(unused_unsafe)]
>     +
>             let mut drv = KBox::new(
> 
> Yeah, a bit annoying... :(

+        // Required due to Rust 1.82's stricter handling of `unsafe` in mutable statics. The
+        // `unsafe` blocks aren't required anymore with later versions.
+        #![allow(unused_unsafe)]

> diff --git a/rust/kernel/cpufreq.rs b/rust/kernel/cpufreq.rs
> index d2e7913e170b..e7c62770fc3b 100644
> --- a/rust/kernel/cpufreq.rs
> +++ b/rust/kernel/cpufreq.rs

> +        attr[next] =
> +            // SAFETY: ...

Ah, I wasn't sure if adding a SAFETY comment after `=` is fine.

Thanks.

-- 
viresh