linux-kernel - Re: [PATCH V3 2/3] rust: Use CpuId in place of raw CPU numbers

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aEmq8fs1fHSB3z4i@tardis.local>
Date: Wed, 11 Jun 2025 09:12:33 -0700
From: Boqun Feng <boqun.feng@...il.com>
To: Viresh Kumar <viresh.kumar@...aro.org>
Cc: "Rafael J. Wysocki" <rafael@...nel.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Peter Zijlstra <peterz@...radead.org>,
	Miguel Ojeda <ojeda@...nel.org>,
	Alex Gaynor <alex.gaynor@...il.com>, Gary Guo <gary@...yguo.net>,
	Björn Roy Baron <bjorn3_gh@...tonmail.com>,
	Benno Lossin <lossin@...nel.org>,
	Andreas Hindborg <a.hindborg@...nel.org>,
	Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>,
	Danilo Krummrich <dakr@...nel.org>,
	Yury Norov <yury.norov@...il.com>,
	Vincent Guittot <vincent.guittot@...aro.org>,
	rust-for-linux@...r.kernel.org, linux-pm@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH V3 2/3] rust: Use CpuId in place of raw CPU numbers

On Tue, Jun 10, 2025 at 06:51:57PM +0530, Viresh Kumar wrote:
> Use the newly defined `CpuId` abstraction instead of raw CPU numbers.
> 
> This also fixes a doctest failure for configurations where `nr_cpu_ids <
> 4`.
> 
> The C `cpumask_{set|clear}_cpu()` APIs emit a warning when given an
> invalid CPU number - but only if `CONFIG_DEBUG_PER_CPU_MAPS=y` is set.
> 
> Meanwhile, `cpumask_weight()` only considers CPUs up to `nr_cpu_ids`,
> which can cause inconsistencies: a CPU number greater than `nr_cpu_ids`
> may be set in the mask, yet the weight calculation won't reflect it.
> 
> This leads to doctest failures when `nr_cpu_ids < 4`, as the test tries
> to set CPUs 2 and 3:
> 
>   rust_doctest_kernel_cpumask_rs_0.location: rust/kernel/cpumask.rs:180
>   rust_doctest_kernel_cpumask_rs_0: ASSERTION FAILED at rust/kernel/cpumask.rs:190
> 
> Fixes: 8961b8cb3099 ("rust: cpumask: Add initial abstractions")
> Reported-by: Miguel Ojeda <ojeda@...nel.org>
> Closes: https://lore.kernel.org/rust-for-linux/CANiq72k3ozKkLMinTLQwvkyg9K=BeRxs1oYZSKhJHY-veEyZdg@mail.gmail.com/
> Reported-by: Andreas Hindborg <a.hindborg@...nel.org>
> Closes: https://lore.kernel.org/all/87qzzy3ric.fsf@kernel.org/
> Suggested-by: Boqun Feng <boqun.feng@...il.com>
> Signed-off-by: Viresh Kumar <viresh.kumar@...aro.org>

Reviewed-by: Boqun Feng <boqun.feng@...il.com>

One nit below..

> ---
>  drivers/cpufreq/rcpufreq_dt.rs |  4 +--
>  rust/kernel/cpu.rs             |  4 +--
>  rust/kernel/cpufreq.rs         | 27 ++++++++++++------
>  rust/kernel/cpumask.rs         | 51 ++++++++++++++++++++++++----------
>  4 files changed, 59 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/cpufreq/rcpufreq_dt.rs b/drivers/cpufreq/rcpufreq_dt.rs
> index 94ed81644fe1..43c87d0259b6 100644
> --- a/drivers/cpufreq/rcpufreq_dt.rs
> +++ b/drivers/cpufreq/rcpufreq_dt.rs
> @@ -26,9 +26,9 @@ fn find_supply_name_exact(dev: &Device, name: &str) -> Option<CString> {
>  }
>  
>  /// Finds supply name for the CPU from DT.
> -fn find_supply_names(dev: &Device, cpu: u32) -> Option<KVec<CString>> {
> +fn find_supply_names(dev: &Device, cpu: cpu::CpuId) -> Option<KVec<CString>> {
>      // Try "cpu0" for older DTs, fallback to "cpu".
> -    let name = (cpu == 0)
> +    let name = (cpu.as_u32() == 0)
>          .then(|| find_supply_name_exact(dev, "cpu0"))
>          .flatten()
>          .or_else(|| find_supply_name_exact(dev, "cpu"))?;
> diff --git a/rust/kernel/cpu.rs b/rust/kernel/cpu.rs
> index 6a3aecb12468..7549594fad7f 100644
> --- a/rust/kernel/cpu.rs
> +++ b/rust/kernel/cpu.rs
> @@ -127,9 +127,9 @@ fn from(id: CpuId) -> Self {
>  /// 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> {
> +pub unsafe fn from_cpu(cpu: CpuId) -> Result<&'static Device> {
>      // SAFETY: It is safe to call `get_cpu_device()` for any CPU.
> -    let ptr = unsafe { bindings::get_cpu_device(cpu) };
> +    let ptr = unsafe { bindings::get_cpu_device(cpu.into()) };


I generally found that `u32::from(cpu)` is more clear than `cpu.into()`,
but it's up to you. Same for the rest of `cpu.into()` cases.

Regards,
Boqun

>      if ptr.is_null() {
>          return Err(ENODEV);
>      }
[...]