rust/kernel/firmware.rs | 31 ++++++----- rust/kernel/page.rs | 116 +++++++++++++++++++++++++++++++++++----- rust/kernel/types.rs | 62 +++++++++++++++++++++ 3 files changed, 184 insertions(+), 25 deletions(-)
Hi all, This series introduces the Owned type and Ownable trait which is the v2 of "rust: page: Add support for vmalloc_to_page" [0]. This series includes changes for firmware as well to make use of the new wrapper. Changes since v2: - Use Owned and Ownable types for constructing Page as suggested in [1] instad of using ptr::read(). [0] https://lore.kernel.org/rust-for-linux/20241007202752.3096472-1-abdiel.janulgue@gmail.com/ [1] https://lore.kernel.org/rust-for-linux/ZwUYmunVpzpexGV8@boqun-archlinux/ Abdiel Janulgue (5): rust: types: add `Owned` type and `Ownable` trait rust: page: Make ownership of the page pointer explicit. rust: page: Extend support to vmalloc_to_page rust: page: Add page_slice_to_page rust: firmware: implement `Ownable` for Firmware rust/kernel/firmware.rs | 31 ++++++----- rust/kernel/page.rs | 116 +++++++++++++++++++++++++++++++++++----- rust/kernel/types.rs | 62 +++++++++++++++++++++ 3 files changed, 184 insertions(+), 25 deletions(-) base-commit: 15541c9263ce34ff95a06bc68f45d9bc5c990bcd -- 2.43.0
On Wed, Oct 23, 2024 at 01:44:44AM +0300, Abdiel Janulgue wrote: > Hi all, > > This series introduces the Owned type and Ownable trait which is the v2 of > "rust: page: Add support for vmalloc_to_page" [0]. This series includes changes > for firmware as well to make use of the new wrapper. Please make sure to add all relevant maintainers. Since this includes a firmware patch, you should make sure to add all firmware maintainers. Remember to use scripts/get_maintainer.pl. Also there are a few minor checkpatch warnings. Please also make sure to run scripts/checkpatch.pl. Please also make sure to compile the code with `CLIPPY=1` (there are a bunch of warnings) and make sure to also run the `rustfmt` target (there are some formatting issues). I wonder if it would make sense to make `CLIPPY=1` the default and only disable it by explicitly passing `CLIPPY=0`. > > Changes since v2: > - Use Owned and Ownable types for constructing Page as suggested in [1] > instad of using ptr::read(). > > [0] https://lore.kernel.org/rust-for-linux/20241007202752.3096472-1-abdiel.janulgue@gmail.com/ > [1] https://lore.kernel.org/rust-for-linux/ZwUYmunVpzpexGV8@boqun-archlinux/ > > Abdiel Janulgue (5): > rust: types: add `Owned` type and `Ownable` trait > rust: page: Make ownership of the page pointer explicit. > rust: page: Extend support to vmalloc_to_page > rust: page: Add page_slice_to_page > rust: firmware: implement `Ownable` for Firmware > > rust/kernel/firmware.rs | 31 ++++++----- > rust/kernel/page.rs | 116 +++++++++++++++++++++++++++++++++++----- > rust/kernel/types.rs | 62 +++++++++++++++++++++ > 3 files changed, 184 insertions(+), 25 deletions(-) > > > base-commit: 15541c9263ce34ff95a06bc68f45d9bc5c990bcd > -- > 2.43.0 >
On Wed, Oct 23, 2024 at 10:03 AM Danilo Krummrich <dakr@kernel.org> wrote:
>
> I wonder if it would make sense to make `CLIPPY=1` the default and only disable
> it by explicitly passing `CLIPPY=0`.
That is what I wanted, but when I asked long ago to the Clippy
maintainers if using `clippy-driver` was guaranteed to not affect
codegen, the answer was no: for instance, optimization may be affected
(at least back then), and the maintainers said the intention is that
is not to be used for normal compiling. So I sent a PR to document
that. See:
https://github.com/rust-lang/rust-clippy/issues/8035
https://github.com/rust-lang/rust-clippy/pull/8037
Similarly, Christian proposed running `rustfmtcheck` unconditionally
on build and offering a way to turn it off instead. I think that would
be ideal too, but it could potentially lead to problems too, so I am
not sure either; see e.g.:
https://lore.kernel.org/rust-for-linux/CANiq72==AkkqCDaZMENQRg8cf4zdeHpTHwdWS3sZiFWm0vyJUA@mail.gmail.com/
So I wonder if we should instead go with a "dev mode" like `D=1` that
enables Clippy, `rustfmtcheck`, `-Dwarnings` (even if `WERROR=n` and
applying to everything, not just kernel objects,), potentially
`rustdoc`-related warnings too, and whatever other tooling/checks in
the future (e.g. klint), and not just for Rust but potentially for C
and other bits too (e.g. `W=1`, some important subset of Coccinelle
scripts...).
That way, "normal builds" (i.e. those done by users) stay as
fast/clean/warning-free/bug-free/optimized as possible even across
compiler versions, potential bugs in the toolchain, etc. And I imagine
it would be easier for newcomers, too.
Opinions welcome! I am happy to prepare an RFC, since it seems a few
people would like something like that.
Cheers,
Miguel
On Wed, Oct 23, 2024 at 11:51:47AM +0200, Miguel Ojeda wrote: > On Wed, Oct 23, 2024 at 10:03 AM Danilo Krummrich <dakr@kernel.org> wrote: > > > > I wonder if it would make sense to make `CLIPPY=1` the default and only disable > > it by explicitly passing `CLIPPY=0`. > > That is what I wanted, but when I asked long ago to the Clippy > maintainers if using `clippy-driver` was guaranteed to not affect > codegen, the answer was no: for instance, optimization may be affected > (at least back then), and the maintainers said the intention is that > is not to be used for normal compiling. So I sent a PR to document > that. See: > > https://github.com/rust-lang/rust-clippy/issues/8035 > https://github.com/rust-lang/rust-clippy/pull/8037 That's pretty unfortunate, I didn't know. I think for the long term it'd be good to find a way though. Once more and more subsystems / people start adding Rust code, I could imagine patches to start slipping through and fixing things up after it's been discovered in -next would be painful. > > Similarly, Christian proposed running `rustfmtcheck` unconditionally > on build and offering a way to turn it off instead. I think that would > be ideal too, but it could potentially lead to problems too, so I am > not sure either; see e.g.: > > https://lore.kernel.org/rust-for-linux/CANiq72==AkkqCDaZMENQRg8cf4zdeHpTHwdWS3sZiFWm0vyJUA@mail.gmail.com/ Yeah, that's a tricky one; if not enabled by default I'd be a bit worried about the same thing to happen as mentioned above. Additionally, for development trees where things slipped through it'd be annoying when `rustfmt` changes more stuff than expected. > > So I wonder if we should instead go with a "dev mode" like `D=1` that > enables Clippy, `rustfmtcheck`, `-Dwarnings` (even if `WERROR=n` and > applying to everything, not just kernel objects,), potentially > `rustdoc`-related warnings too, and whatever other tooling/checks in > the future (e.g. klint), and not just for Rust but potentially for C > and other bits too (e.g. `W=1`, some important subset of Coccinelle > scripts...). I think that'd be great for short / mid term, it'd make it much easier to ensure that all relevant checks were executed and hence make it less likely for things slip through. For the long term, I think it'd be great to keep looking for ways to always enable the clippy and format checks. Or at least the clippy ones if we're too concerned about `rustfmt` to break. > > That way, "normal builds" (i.e. those done by users) stay as > fast/clean/warning-free/bug-free/optimized as possible even across > compiler versions, potential bugs in the toolchain, etc. And I imagine > it would be easier for newcomers, too. > > Opinions welcome! I am happy to prepare an RFC, since it seems a few > people would like something like that. > > Cheers, > Miguel >
© 2016 - 2024 Red Hat, Inc.