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