| Message ID | cover.1738832118.git.viresh.kumar@linaro.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | Rust bindings for cpufreq and OPP core + sample driver | expand |
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.
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.
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
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 ?
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
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.