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

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