| Message ID | cover.1749554685.git.viresh.kumar@linaro.org |
|---|---|
| Headers | show |
| Series | rust: Introduce CpuId and fix cpumask doctest | expand |
On 10-06-25, 19:10, Miguel Ojeda wrote: > On Tue, Jun 10, 2025 at 3:22 PM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > > Here is another attempt at fixing the cpumask doctest. This series creates a new > > abstraction `CpuId`, which is used to write a cleaner cpumask example which > > doesn't fail in those corner cases. > > > > Rebased over v6.16-rc1 + [1]. > > Given this is growing, should we apply something trivial right away as > a fix meanwhile? Or are you planning to send this as a fix during the > -rcs? Yeah, I am planning to send this for rc2 or rc3.
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@kernel.org> > Closes: https://lore.kernel.org/rust-for-linux/CANiq72k3ozKkLMinTLQwvkyg9K=BeRxs1oYZSKhJHY-veEyZdg@mail.gmail.com/ > Reported-by: Andreas Hindborg <a.hindborg@kernel.org> > Closes: https://lore.kernel.org/all/87qzzy3ric.fsf@kernel.org/ > Suggested-by: Boqun Feng <boqun.feng@gmail.com> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> Reviewed-by: Boqun Feng <boqun.feng@gmail.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); > } [...]
On 11-06-25, 09:12, Boqun Feng wrote: > 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. Updated as: diff --git a/rust/kernel/cpu.rs b/rust/kernel/cpu.rs index 7549594fad7f..abc780d7a8ec 100644 --- a/rust/kernel/cpu.rs +++ b/rust/kernel/cpu.rs @@ -129,7 +129,7 @@ fn from(id: CpuId) -> Self { /// any references to the CPU device within the notifier's callback. 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.into()) }; + let ptr = unsafe { bindings::get_cpu_device(u32::from(cpu)) }; if ptr.is_null() { return Err(ENODEV); } diff --git a/rust/kernel/cpufreq.rs b/rust/kernel/cpufreq.rs index ea6106db5c29..11b03e9d7e89 100644 --- a/rust/kernel/cpufreq.rs +++ b/rust/kernel/cpufreq.rs @@ -527,7 +527,7 @@ pub fn generic_suspend(&mut self) -> Result { #[inline] pub fn generic_get(&self) -> Result<u32> { // SAFETY: By the type invariant, the pointer stored in `self` is valid. - Ok(unsafe { bindings::cpufreq_generic_get(self.cpu().into()) }) + Ok(unsafe { bindings::cpufreq_generic_get(u32::from(self.cpu())) }) } /// Provides a wrapper to the register with energy model using the OPP core. @@ -682,7 +682,7 @@ fn clear_data<T: ForeignOwnable>(&mut self) -> Option<T> { impl<'a> PolicyCpu<'a> { fn from_cpu(cpu: CpuId) -> Result<Self> { // SAFETY: It is safe to call `cpufreq_cpu_get` for any valid CPU. - let ptr = from_err_ptr(unsafe { bindings::cpufreq_cpu_get(cpu.into()) })?; + let ptr = from_err_ptr(unsafe { bindings::cpufreq_cpu_get(u32::from(cpu)) })?; Ok(Self( // SAFETY: The `ptr` is guaranteed to be valid and remains valid for the lifetime of diff --git a/rust/kernel/cpumask.rs b/rust/kernel/cpumask.rs index 11ddd43edcb5..19c607709b5f 100644 --- a/rust/kernel/cpumask.rs +++ b/rust/kernel/cpumask.rs @@ -94,7 +94,7 @@ pub fn as_raw(&self) -> *mut bindings::cpumask { #[inline] pub fn set(&mut self, cpu: CpuId) { // SAFETY: By the type invariant, `self.as_raw` is a valid argument to `__cpumask_set_cpu`. - unsafe { bindings::__cpumask_set_cpu(cpu.into(), self.as_raw()) }; + unsafe { bindings::__cpumask_set_cpu(u32::from(cpu), self.as_raw()) }; } /// Clear `cpu` in the cpumask. @@ -106,7 +106,7 @@ pub fn set(&mut self, cpu: CpuId) { pub fn clear(&mut self, cpu: CpuId) { // SAFETY: By the type invariant, `self.as_raw` is a valid argument to // `__cpumask_clear_cpu`. - unsafe { bindings::__cpumask_clear_cpu(cpu.into(), self.as_raw()) }; + unsafe { bindings::__cpumask_clear_cpu(i32::from(cpu), self.as_raw()) }; } /// Test `cpu` in the cpumask. @@ -115,7 +115,7 @@ pub fn clear(&mut self, cpu: CpuId) { #[inline] pub fn test(&self, cpu: CpuId) -> bool { // SAFETY: By the type invariant, `self.as_raw` is a valid argument to `cpumask_test_cpu`. - unsafe { bindings::cpumask_test_cpu(cpu.into(), self.as_raw()) } + unsafe { bindings::cpumask_test_cpu(i32::from(cpu), self.as_raw()) } } /// Set all CPUs in the cpumask.
On Thu, Jun 12, 2025 at 10:31:17AM +0530, Viresh Kumar wrote: > On 11-06-25, 09:12, Boqun Feng wrote: > > 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. > > Updated as: > > diff --git a/rust/kernel/cpu.rs b/rust/kernel/cpu.rs > index 7549594fad7f..abc780d7a8ec 100644 > --- a/rust/kernel/cpu.rs > +++ b/rust/kernel/cpu.rs > @@ -129,7 +129,7 @@ fn from(id: CpuId) -> Self { > /// any references to the CPU device within the notifier's callback. > 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.into()) }; > + let ptr = unsafe { bindings::get_cpu_device(u32::from(cpu)) }; > if ptr.is_null() { > return Err(ENODEV); > } > diff --git a/rust/kernel/cpufreq.rs b/rust/kernel/cpufreq.rs > index ea6106db5c29..11b03e9d7e89 100644 > --- a/rust/kernel/cpufreq.rs > +++ b/rust/kernel/cpufreq.rs > @@ -527,7 +527,7 @@ pub fn generic_suspend(&mut self) -> Result { > #[inline] > pub fn generic_get(&self) -> Result<u32> { > // SAFETY: By the type invariant, the pointer stored in `self` is valid. > - Ok(unsafe { bindings::cpufreq_generic_get(self.cpu().into()) }) > + Ok(unsafe { bindings::cpufreq_generic_get(u32::from(self.cpu())) }) > } > > /// Provides a wrapper to the register with energy model using the OPP core. > @@ -682,7 +682,7 @@ fn clear_data<T: ForeignOwnable>(&mut self) -> Option<T> { > impl<'a> PolicyCpu<'a> { > fn from_cpu(cpu: CpuId) -> Result<Self> { > // SAFETY: It is safe to call `cpufreq_cpu_get` for any valid CPU. > - let ptr = from_err_ptr(unsafe { bindings::cpufreq_cpu_get(cpu.into()) })?; > + let ptr = from_err_ptr(unsafe { bindings::cpufreq_cpu_get(u32::from(cpu)) })?; > > Ok(Self( > // SAFETY: The `ptr` is guaranteed to be valid and remains valid for the lifetime of > diff --git a/rust/kernel/cpumask.rs b/rust/kernel/cpumask.rs > index 11ddd43edcb5..19c607709b5f 100644 > --- a/rust/kernel/cpumask.rs > +++ b/rust/kernel/cpumask.rs > @@ -94,7 +94,7 @@ pub fn as_raw(&self) -> *mut bindings::cpumask { > #[inline] > pub fn set(&mut self, cpu: CpuId) { > // SAFETY: By the type invariant, `self.as_raw` is a valid argument to `__cpumask_set_cpu`. > - unsafe { bindings::__cpumask_set_cpu(cpu.into(), self.as_raw()) }; > + unsafe { bindings::__cpumask_set_cpu(u32::from(cpu), self.as_raw()) }; > } > > /// Clear `cpu` in the cpumask. > @@ -106,7 +106,7 @@ pub fn set(&mut self, cpu: CpuId) { > pub fn clear(&mut self, cpu: CpuId) { > // SAFETY: By the type invariant, `self.as_raw` is a valid argument to > // `__cpumask_clear_cpu`. > - unsafe { bindings::__cpumask_clear_cpu(cpu.into(), self.as_raw()) }; > + unsafe { bindings::__cpumask_clear_cpu(i32::from(cpu), self.as_raw()) }; > } > > /// Test `cpu` in the cpumask. > @@ -115,7 +115,7 @@ pub fn clear(&mut self, cpu: CpuId) { > #[inline] > pub fn test(&self, cpu: CpuId) -> bool { > // SAFETY: By the type invariant, `self.as_raw` is a valid argument to `cpumask_test_cpu`. > - unsafe { bindings::cpumask_test_cpu(cpu.into(), self.as_raw()) } > + unsafe { bindings::cpumask_test_cpu(i32::from(cpu), self.as_raw()) } > } > LGTM,thanks! Regards, Boqun > /// Set all CPUs in the cpumask. > > -- > viresh