diff mbox series

[v2,5/5] rust: enable `clippy::as_underscore` lint

Message ID 20250309-ptr-as-ptr-v2-5-25d60ad922b7@gmail.com
State Superseded
Headers show
Series rust: reduce pointer casts, enable related lints | expand

Commit Message

Tamir Duberstein March 9, 2025, 4 p.m. UTC
In Rust 1.63.0, Clippy introduced the `as_underscore` lint [1]:

> The conversion might include lossy conversion or a dangerous cast that
> might go undetected due to the type being inferred.
>
> The lint is allowed by default as using `_` is less wordy than always
> specifying the type.

Always specifying the type is especially helpful in function call
contexts where the inferred type may change at a distance. Specifying
the type also allows Clippy to spot more cases of `useless_conversion`.

The primary downside is the need to specify the type in trivial getters.
There are 4 such functions: 3 have become slightly less ergonomic, 1 was
revealed to be a `useless_conversion`.

While this doesn't eliminate unchecked `as` conversions, it makes such
conversions easier to scrutinize.  It also has the slight benefit of
removing a degree of freedom on which to bikeshed. Thus apply the
changes and enable the lint -- no functional change intended.

Link: https://rust-lang.github.io/rust-clippy/master/index.html#as_underscore [1]
Signed-off-by: Tamir Duberstein <tamird@gmail.com>
---
 Makefile                           |  1 +
 rust/kernel/block/mq/operations.rs |  2 +-
 rust/kernel/block/mq/request.rs    |  2 +-
 rust/kernel/device_id.rs           |  2 +-
 rust/kernel/devres.rs              | 15 ++++++++-------
 rust/kernel/error.rs               |  2 +-
 rust/kernel/firmware.rs            |  2 +-
 rust/kernel/io.rs                  | 18 +++++++++---------
 rust/kernel/miscdevice.rs          |  2 +-
 rust/kernel/of.rs                  |  6 +++---
 rust/kernel/pci.rs                 |  9 ++++++---
 rust/kernel/str.rs                 |  8 ++++----
 rust/kernel/workqueue.rs           |  2 +-
 13 files changed, 38 insertions(+), 33 deletions(-)

Comments

Tamir Duberstein March 12, 2025, 3:35 p.m. UTC | #1
On Wed, Mar 12, 2025 at 11:05 AM Benno Lossin <benno.lossin@proton.me> wrote:
>
> On Sun Mar 9, 2025 at 5:00 PM CET, Tamir Duberstein wrote:
> > diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
> > index 598001157293..20159b7c9293 100644
> > --- a/rust/kernel/devres.rs
> > +++ b/rust/kernel/devres.rs
> > @@ -45,7 +45,7 @@ struct DevresInner<T> {
> >  /// # Example
> >  ///
> >  /// ```no_run
> > -/// # use kernel::{bindings, c_str, device::Device, devres::Devres, io::{Io, IoRaw}};
> > +/// # use kernel::{bindings, c_str, device::Device, devres::Devres, ffi::c_void, io::{Io, IoRaw}};
> >  /// # use core::ops::Deref;
> >  ///
> >  /// // See also [`pci::Bar`] for a real example.
> > @@ -59,19 +59,19 @@ struct DevresInner<T> {
> >  ///     unsafe fn new(paddr: usize) -> Result<Self>{
> >  ///         // SAFETY: By the safety requirements of this function [`paddr`, `paddr` + `SIZE`) is
> >  ///         // valid for `ioremap`.
> > -///         let addr = unsafe { bindings::ioremap(paddr as _, SIZE as _) };
> > +///         let addr = unsafe { bindings::ioremap(paddr as u64, SIZE) };
>
> The argument of `ioremap` is defined as `resource_size_t` which
> ultimately maps to `u64` on 64 bit systems and `u32` on 32 bit ones. I
> don't think that we should have code like this... Is there another
> option?
>
> Maybe Gary knows something here, do we have a type that represents that
> better?

Ah yeah the problem is that this type is an alias rather than a
newtype. How do you feel about `as bindings::phys_addr_t`?

> >  ///         if addr.is_null() {
> >  ///             return Err(ENOMEM);
> >  ///         }
> >  ///
> > -///         Ok(IoMem(IoRaw::new(addr as _, SIZE)?))
> > +///         Ok(IoMem(IoRaw::new(addr as usize, SIZE)?))
>
> This should be `addr.addr()` (requires `strict_provenance` on Rust 1.83
> & before).
>
> (I am assuming that we're never casting the usize back to a pointer,
> since otherwise this change would introduce UB)

Yeah, we don't have strict provenance APIs (and we can't introduce
them without compiler tooling or bumping MSRV). I'm not sure if we are
casting back to a pointer, but either way this change doesn't change
the semantics - it is only spelling out the type.

> >  ///     }
> >  /// }
> >  ///
> >  /// impl<const SIZE: usize> Drop for IoMem<SIZE> {
> >  ///     fn drop(&mut self) {
> >  ///         // SAFETY: `self.0.addr()` is guaranteed to be properly mapped by `Self::new`.
> > -///         unsafe { bindings::iounmap(self.0.addr() as _); };
> > +///         unsafe { bindings::iounmap(self.0.addr() as *mut c_void); };
>
> Can't this be a `.cast::<c_void>()`?

This is an integer-to-pointer cast. `addr` returns `usize`:

impl<const SIZE: usize> IoRaw<SIZE> {
    [...]

    /// Returns the base address of the MMIO region.
    #[inline]
    pub fn addr(&self) -> usize {
        self.addr
    }

    [...]
}

>
> >  ///     }
> >  /// }
> >  ///
>
> > diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
> > index 8654d52b0bb9..eb8fa52f08ba 100644
> > --- a/rust/kernel/error.rs
> > +++ b/rust/kernel/error.rs
> > @@ -152,7 +152,7 @@ pub(crate) fn to_blk_status(self) -> bindings::blk_status_t {
> >      /// Returns the error encoded as a pointer.
> >      pub fn to_ptr<T>(self) -> *mut T {
> >          // SAFETY: `self.0` is a valid error due to its invariant.
> > -        unsafe { bindings::ERR_PTR(self.0.get() as _).cast() }
> > +        unsafe { bindings::ERR_PTR(self.0.get() as isize).cast() }
>
> Can't this be a `.into()`?

error[E0277]: the trait bound `isize: core::convert::From<i32>` is not satisfied
   --> ../rust/kernel/error.rs:155:49
    |
155 |         unsafe { bindings::ERR_PTR(self.0.get().into()).cast() }
    |                                                 ^^^^ the trait
`core::convert::From<i32>` is not implemented for `isize`

>
> >      }
> >
> >      /// Returns a string representing the error, if one exists.
>
> > @@ -119,7 +119,7 @@ pub fn $name(&self, offset: usize) -> $type_name {
> >              let addr = self.io_addr_assert::<$type_name>(offset);
> >
> >              // SAFETY: By the type invariant `addr` is a valid address for MMIO operations.
> > -            unsafe { bindings::$name(addr as _) }
> > +            unsafe { bindings::$name(addr as *const c_void) }
>
> Also here, is `.cast::<c_void>()` enough? (and below)

It's an integer-to-pointer cast. In the same `impl<const SIZE: usize>
IoRaw<SIZE>` as above:

    fn io_addr_assert<U>(&self, offset: usize) -> usize {
        build_assert!(Self::offset_valid::<U>(offset, SIZE));

        self.addr() + offset
    }

>
> >          }
> >
> >          /// Read IO data from a given offset.
>
> > diff --git a/rust/kernel/of.rs b/rust/kernel/of.rs
> > index 04f2d8ef29cb..40d1bd13682c 100644
> > --- a/rust/kernel/of.rs
> > +++ b/rust/kernel/of.rs
> > @@ -22,7 +22,7 @@ unsafe impl RawDeviceId for DeviceId {
> >      const DRIVER_DATA_OFFSET: usize = core::mem::offset_of!(bindings::of_device_id, data);
> >
> >      fn index(&self) -> usize {
> > -        self.0.data as _
> > +        self.0.data as usize
>
> This should also be `self.0.data.addr()`.

Can't do it without strict_provenance.

>
> >      }
> >  }
> >
> > @@ -34,10 +34,10 @@ pub const fn new(compatible: &'static CStr) -> Self {
> >          // SAFETY: FFI type is valid to be zero-initialized.
> >          let mut of: bindings::of_device_id = unsafe { core::mem::zeroed() };
> >
> > -        // TODO: Use `clone_from_slice` once the corresponding types do match.
> > +        // TODO: Use `copy_from_slice` once stabilized for `const`.
>
> This feature has just been stabilized (5 days ago!):
>
>     https://github.com/rust-lang/rust/issues/131415

Yep! I know :)

> @Miguel: Do we already have a target Rust version for dropping the
> `RUSTC_BOOTSTRAP=1`? If not, then I think we should use this feature
> now, since it will be stable by the time we bump the minimum version.
> (not in this patch [series] though)
>
> >          let mut i = 0;
> >          while i < src.len() {
> > -            of.compatible[i] = src[i] as _;
> > +            of.compatible[i] = src[i];
> >              i += 1;
> >          }
>
> > @@ -317,7 +320,7 @@ unsafe fn do_release(pdev: &Device, ioptr: usize, num: i32) {
> >          // `ioptr` is valid by the safety requirements.
> >          // `num` is valid by the safety requirements.
> >          unsafe {
> > -            bindings::pci_iounmap(pdev.as_raw(), ioptr as _);
> > +            bindings::pci_iounmap(pdev.as_raw(), ioptr as *mut kernel::ffi::c_void);
>
> Again, probably castable.

How? `ioptr` is a `usize` (you can see the prototype).

>
> >              bindings::pci_release_region(pdev.as_raw(), num);
> >          }
> >      }
> > diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
> > index 6a1a982b946d..0b80a119d5f0 100644
> > --- a/rust/kernel/str.rs
> > +++ b/rust/kernel/str.rs
> > @@ -692,9 +692,9 @@ fn new() -> Self {
> >      pub(crate) unsafe fn from_ptrs(pos: *mut u8, end: *mut u8) -> Self {
> >          // INVARIANT: The safety requirements guarantee the type invariants.
> >          Self {
> > -            beg: pos as _,
> > -            pos: pos as _,
> > -            end: end as _,
> > +            beg: pos as usize,
> > +            pos: pos as usize,
> > +            end: end as usize,
>
> I would prefer if we use `pos.expose_provenance()` (also for `end`)
> here.

Me too! But we can't until we actually start using strict provenance.

>
> >          }
> >      }
> >
> > @@ -719,7 +719,7 @@ pub(crate) unsafe fn from_buffer(buf: *mut u8, len: usize) -> Self {
> >      ///
> >      /// N.B. It may point to invalid memory.
> >      pub(crate) fn pos(&self) -> *mut u8 {
> > -        self.pos as _
> > +        self.pos as *mut u8
>
> This should then also be `with_exposed_provenance(self.pos)`

Same as other instances of strict provenance.
Miguel Ojeda March 12, 2025, 3:49 p.m. UTC | #2
On Wed, Mar 12, 2025 at 4:36 PM Tamir Duberstein <tamird@gmail.com> wrote:
>
> Yeah, we don't have strict provenance APIs (and we can't introduce
> them without compiler tooling or bumping MSRV). I'm not sure if we are

The strict provenance APIs were added a long time ago (1.61) -- in
fact we briefly discussed doing so back then (we started before that,
with 1.58).

So unless we need some detail that changed recently (i.e. since 1.78),
we should be able to use them, and it should be fairly safe since they
are stable now (1.84).

Cheers,
Miguel
Tamir Duberstein March 12, 2025, 6:01 p.m. UTC | #3
On Wed, Mar 12, 2025 at 1:05 PM Benno Lossin <benno.lossin@proton.me> wrote:
>
> On Wed Mar 12, 2025 at 4:35 PM CET, Tamir Duberstein wrote:
> > On Wed, Mar 12, 2025 at 11:05 AM Benno Lossin <benno.lossin@proton.me> wrote:
> >>
> >> On Sun Mar 9, 2025 at 5:00 PM CET, Tamir Duberstein wrote:
> >> > diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
> >> > index 598001157293..20159b7c9293 100644
> >> > --- a/rust/kernel/devres.rs
> >> > +++ b/rust/kernel/devres.rs
> >> > @@ -45,7 +45,7 @@ struct DevresInner<T> {
> >> >  /// # Example
> >> >  ///
> >> >  /// ```no_run
> >> > -/// # use kernel::{bindings, c_str, device::Device, devres::Devres, io::{Io, IoRaw}};
> >> > +/// # use kernel::{bindings, c_str, device::Device, devres::Devres, ffi::c_void, io::{Io, IoRaw}};
> >> >  /// # use core::ops::Deref;
> >> >  ///
> >> >  /// // See also [`pci::Bar`] for a real example.
> >> > @@ -59,19 +59,19 @@ struct DevresInner<T> {
> >> >  ///     unsafe fn new(paddr: usize) -> Result<Self>{
> >> >  ///         // SAFETY: By the safety requirements of this function [`paddr`, `paddr` + `SIZE`) is
> >> >  ///         // valid for `ioremap`.
> >> > -///         let addr = unsafe { bindings::ioremap(paddr as _, SIZE as _) };
> >> > +///         let addr = unsafe { bindings::ioremap(paddr as u64, SIZE) };
> >>
> >> The argument of `ioremap` is defined as `resource_size_t` which
> >> ultimately maps to `u64` on 64 bit systems and `u32` on 32 bit ones. I
> >> don't think that we should have code like this... Is there another
> >> option?
> >>
> >> Maybe Gary knows something here, do we have a type that represents that
> >> better?
> >
> > Ah yeah the problem is that this type is an alias rather than a
> > newtype. How do you feel about `as bindings::phys_addr_t`?
>
> Yeah that's better.
>
> >> >  ///         if addr.is_null() {
> >> >  ///             return Err(ENOMEM);
> >> >  ///         }
> >> >  ///
> >> > -///         Ok(IoMem(IoRaw::new(addr as _, SIZE)?))
> >> > +///         Ok(IoMem(IoRaw::new(addr as usize, SIZE)?))
> >>
> >> This should be `addr.addr()` (requires `strict_provenance` on Rust 1.83
> >> & before).
> >>
> >> (I am assuming that we're never casting the usize back to a pointer,
> >> since otherwise this change would introduce UB)
> >
> > Yeah, we don't have strict provenance APIs (and we can't introduce
> > them without compiler tooling or bumping MSRV). I'm not sure if we are
> > casting back to a pointer, but either way this change doesn't change
> > the semantics - it is only spelling out the type.
>
> It's fine to enable the feature, since it's stable in a newer version of
> the compiler.
>
> >> >  ///     }
> >> >  /// }
> >> >  ///
> >> >  /// impl<const SIZE: usize> Drop for IoMem<SIZE> {
> >> >  ///     fn drop(&mut self) {
> >> >  ///         // SAFETY: `self.0.addr()` is guaranteed to be properly mapped by `Self::new`.
> >> > -///         unsafe { bindings::iounmap(self.0.addr() as _); };
> >> > +///         unsafe { bindings::iounmap(self.0.addr() as *mut c_void); };
> >>
> >> Can't this be a `.cast::<c_void>()`?
> >
> > This is an integer-to-pointer cast. `addr` returns `usize`:
>
> Oh I missed the `*mut`... In that case, we can't use the `addr`
> suggestion that I made above, instead we should use `expose_provenance`
> above and `with_exposed_provenance` here.
>
> > impl<const SIZE: usize> IoRaw<SIZE> {
> >     [...]
> >
> >     /// Returns the base address of the MMIO region.
> >     #[inline]
> >     pub fn addr(&self) -> usize {
> >         self.addr
> >     }
> >
> >     [...]
> > }
> >
> >>
> >> >  ///     }
> >> >  /// }
> >> >  ///
> >>
> >> > diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
> >> > index 8654d52b0bb9..eb8fa52f08ba 100644
> >> > --- a/rust/kernel/error.rs
> >> > +++ b/rust/kernel/error.rs
> >> > @@ -152,7 +152,7 @@ pub(crate) fn to_blk_status(self) -> bindings::blk_status_t {
> >> >      /// Returns the error encoded as a pointer.
> >> >      pub fn to_ptr<T>(self) -> *mut T {
> >> >          // SAFETY: `self.0` is a valid error due to its invariant.
> >> > -        unsafe { bindings::ERR_PTR(self.0.get() as _).cast() }
> >> > +        unsafe { bindings::ERR_PTR(self.0.get() as isize).cast() }
> >>
> >> Can't this be a `.into()`?
> >
> > error[E0277]: the trait bound `isize: core::convert::From<i32>` is not satisfied
> >    --> ../rust/kernel/error.rs:155:49
> >     |
> > 155 |         unsafe { bindings::ERR_PTR(self.0.get().into()).cast() }
> >     |                                                 ^^^^ the trait
> > `core::convert::From<i32>` is not implemented for `isize`
>
> That's a bummer... I wonder why that doesn't exist.
>
> >> >      }
> >> >
> >> >      /// Returns a string representing the error, if one exists.
> >>
> >> > @@ -119,7 +119,7 @@ pub fn $name(&self, offset: usize) -> $type_name {
> >> >              let addr = self.io_addr_assert::<$type_name>(offset);
> >> >
> >> >              // SAFETY: By the type invariant `addr` is a valid address for MMIO operations.
> >> > -            unsafe { bindings::$name(addr as _) }
> >> > +            unsafe { bindings::$name(addr as *const c_void) }
> >>
> >> Also here, is `.cast::<c_void>()` enough? (and below)
> >
> > It's an integer-to-pointer cast. In the same `impl<const SIZE: usize>
> > IoRaw<SIZE>` as above:
> >
> >     fn io_addr_assert<U>(&self, offset: usize) -> usize {
> >         build_assert!(Self::offset_valid::<U>(offset, SIZE));
> >
> >         self.addr() + offset
> >     }
>
> I would prefer we use the strict_provenance API.
>
> >> >          }
> >> >
> >> >          /// Read IO data from a given offset.
> >>
> >> > diff --git a/rust/kernel/of.rs b/rust/kernel/of.rs
> >> > index 04f2d8ef29cb..40d1bd13682c 100644
> >> > --- a/rust/kernel/of.rs
> >> > +++ b/rust/kernel/of.rs
> >> > @@ -22,7 +22,7 @@ unsafe impl RawDeviceId for DeviceId {
> >> >      const DRIVER_DATA_OFFSET: usize = core::mem::offset_of!(bindings::of_device_id, data);
> >> >
> >> >      fn index(&self) -> usize {
> >> > -        self.0.data as _
> >> > +        self.0.data as usize
> >>
> >> This should also be `self.0.data.addr()`.
> >
> > Can't do it without strict_provenance.
> >
> >>
> >> >      }
> >> >  }
> >> >
> >> > @@ -34,10 +34,10 @@ pub const fn new(compatible: &'static CStr) -> Self {
> >> >          // SAFETY: FFI type is valid to be zero-initialized.
> >> >          let mut of: bindings::of_device_id = unsafe { core::mem::zeroed() };
> >> >
> >> > -        // TODO: Use `clone_from_slice` once the corresponding types do match.
> >> > +        // TODO: Use `copy_from_slice` once stabilized for `const`.
> >>
> >> This feature has just been stabilized (5 days ago!):
> >>
> >>     https://github.com/rust-lang/rust/issues/131415
> >
> > Yep! I know :)
> >
> >> @Miguel: Do we already have a target Rust version for dropping the
> >> `RUSTC_BOOTSTRAP=1`? If not, then I think we should use this feature
> >> now, since it will be stable by the time we bump the minimum version.
> >> (not in this patch [series] though)
> >>
> >> >          let mut i = 0;
> >> >          while i < src.len() {
> >> > -            of.compatible[i] = src[i] as _;
> >> > +            of.compatible[i] = src[i];
> >> >              i += 1;
> >> >          }
> >>
> >> > @@ -317,7 +320,7 @@ unsafe fn do_release(pdev: &Device, ioptr: usize, num: i32) {
> >> >          // `ioptr` is valid by the safety requirements.
> >> >          // `num` is valid by the safety requirements.
> >> >          unsafe {
> >> > -            bindings::pci_iounmap(pdev.as_raw(), ioptr as _);
> >> > +            bindings::pci_iounmap(pdev.as_raw(), ioptr as *mut kernel::ffi::c_void);
> >>
> >> Again, probably castable.
> >
> > How? `ioptr` is a `usize` (you can see the prototype).
>
> Sorry, I missed all the `*mut`/`*const` prefixes here.
>
> ---
> Cheers,
> Benno
>

I think all the remaining comments are about strict provenance. I buy
that this is a useful thing to do, but in the absence of automated
tools to help do it, I'm not sure it's worth it to do it for just
these things I happen to be touching rather than doing it throughout.

I couldn't find a clippy lint. Do you know of one? If not, should we
file an issue?
Tamir Duberstein March 12, 2025, 7:19 p.m. UTC | #4
On Wed, Mar 12, 2025 at 3:11 PM Benno Lossin <benno.lossin@proton.me> wrote:
>
> On Wed Mar 12, 2025 at 7:01 PM CET, Tamir Duberstein wrote:
> > I think all the remaining comments are about strict provenance. I buy
> > that this is a useful thing to do, but in the absence of automated
> > tools to help do it, I'm not sure it's worth it to do it for just
> > these things I happen to be touching rather than doing it throughout.
> >
> > I couldn't find a clippy lint. Do you know of one? If not, should we
> > file an issue?
>
> A quick search gave me:
>
>     https://doc.rust-lang.org/nightly/unstable-book/language-features/strict-provenance-lints.html
>
> The linked tracking issue is closed which seems like a mistake, since
> it's not yet stabilized. I found a different issue tracking it though:
>
>     https://github.com/rust-lang/rust/issues/130351
>
> We probably should use both in that case:
>
>     #![feature(strict_provenance_lints)]
>     #![warn(fuzzy_provenance_casts, lossy_provenance_casts)]

Nice! I didn't think to check rustc lints.

> I don't want to push more work onto this series, so it's fine to leave
> them in. Thus:
>
> Reviewed-by: Benno Lossin <benno.lossin@proton.me>

Thanks!

>
> We can either make this a good-first-issue, or if you also want to
> tackle this, then go ahead :)

I tried using the strict provenance lints locally and I think we can't
until we properly bump MSRV due to `clippy::incompatible_msrv`:

warning: current MSRV (Minimum Supported Rust Version) is `1.78.0` but
this item is stable since `1.84.0`
   --> ../rust/kernel/str.rs:696:22
    |
696 |             pos: pos.expose_provenance(),
    |                      ^^^^^^^^^^^^^^^^^^^
    |
    = help: for further information visit
https://rust-lang.github.io/rust-clippy/master/index.html#incompatible_msrv

This is with `#![feature(strict_provenance)]`. I can file the issue
but I think it's blocked on MSRV >= 1.84.0. But maybe you know of a
path forward :)

I'll send v3 with the tags and the phys_addr_t fix shortly.
Benno Lossin March 12, 2025, 7:43 p.m. UTC | #5
On Wed Mar 12, 2025 at 8:19 PM CET, Tamir Duberstein wrote:
> I tried using the strict provenance lints locally and I think we can't
> until we properly bump MSRV due to `clippy::incompatible_msrv`:
>
> warning: current MSRV (Minimum Supported Rust Version) is `1.78.0` but
> this item is stable since `1.84.0`
>    --> ../rust/kernel/str.rs:696:22
>     |
> 696 |             pos: pos.expose_provenance(),
>     |                      ^^^^^^^^^^^^^^^^^^^
>     |
>     = help: for further information visit
> https://rust-lang.github.io/rust-clippy/master/index.html#incompatible_msrv

Oh this is annoying...

> This is with `#![feature(strict_provenance)]`. I can file the issue
> but I think it's blocked on MSRV >= 1.84.0. But maybe you know of a
> path forward :)

I think we should be able to just `allow(clippy::incompatible_msrv)`,
since Miguel & other maintainers will test with 1.78 (or at least are
supposed to :).

---
Cheers,
Benno
Benno Lossin March 12, 2025, 8:42 p.m. UTC | #6
On Wed Mar 12, 2025 at 9:07 PM CET, Tamir Duberstein wrote:
> On Wed, Mar 12, 2025 at 3:43 PM Benno Lossin <benno.lossin@proton.me> wrote:
>>
>> On Wed Mar 12, 2025 at 8:19 PM CET, Tamir Duberstein wrote:
>> > I tried using the strict provenance lints locally and I think we can't
>> > until we properly bump MSRV due to `clippy::incompatible_msrv`:
>> >
>> > warning: current MSRV (Minimum Supported Rust Version) is `1.78.0` but
>> > this item is stable since `1.84.0`
>> >    --> ../rust/kernel/str.rs:696:22
>> >     |
>> > 696 |             pos: pos.expose_provenance(),
>> >     |                      ^^^^^^^^^^^^^^^^^^^
>> >     |
>> >     = help: for further information visit
>> > https://rust-lang.github.io/rust-clippy/master/index.html#incompatible_msrv
>>
>> Oh this is annoying...
>>
>> > This is with `#![feature(strict_provenance)]`. I can file the issue
>> > but I think it's blocked on MSRV >= 1.84.0. But maybe you know of a
>> > path forward :)
>>
>> I think we should be able to just `allow(clippy::incompatible_msrv)`,
>> since Miguel & other maintainers will test with 1.78 (or at least are
>> supposed to :).
>
> Alright, you've sniped me.

Sorry about that :)

> This is coming in v3.

Thanks a lot!

---
Cheers,
Benno
diff mbox series

Patch

diff --git a/Makefile b/Makefile
index bb15b86182a3..2af40bfed9ce 100644
--- a/Makefile
+++ b/Makefile
@@ -478,6 +478,7 @@  export rust_common_flags := --edition=2021 \
 			    -Wunreachable_pub \
 			    -Wclippy::all \
 			    -Wclippy::as_ptr_cast_mut \
+			    -Wclippy::as_underscore \
 			    -Wclippy::ignored_unit_patterns \
 			    -Wclippy::mut_mut \
 			    -Wclippy::needless_bitwise_bool \
diff --git a/rust/kernel/block/mq/operations.rs b/rust/kernel/block/mq/operations.rs
index 864ff379dc91..d18ef55490da 100644
--- a/rust/kernel/block/mq/operations.rs
+++ b/rust/kernel/block/mq/operations.rs
@@ -101,7 +101,7 @@  impl<T: Operations> OperationsVTable<T> {
         if let Err(e) = ret {
             e.to_blk_status()
         } else {
-            bindings::BLK_STS_OK as _
+            bindings::BLK_STS_OK as u8
         }
     }
 
diff --git a/rust/kernel/block/mq/request.rs b/rust/kernel/block/mq/request.rs
index 10c6d69be7f3..bcf2b73d9189 100644
--- a/rust/kernel/block/mq/request.rs
+++ b/rust/kernel/block/mq/request.rs
@@ -125,7 +125,7 @@  pub fn end_ok(this: ARef<Self>) -> Result<(), ARef<Self>> {
         // success of the call to `try_set_end` guarantees that there are no
         // `ARef`s pointing to this request. Therefore it is safe to hand it
         // back to the block layer.
-        unsafe { bindings::blk_mq_end_request(request_ptr, bindings::BLK_STS_OK as _) };
+        unsafe { bindings::blk_mq_end_request(request_ptr, bindings::BLK_STS_OK as u8) };
 
         Ok(())
     }
diff --git a/rust/kernel/device_id.rs b/rust/kernel/device_id.rs
index e5859217a579..4063f09d76d9 100644
--- a/rust/kernel/device_id.rs
+++ b/rust/kernel/device_id.rs
@@ -82,7 +82,7 @@  impl<T: RawDeviceId, U, const N: usize> IdArray<T, U, N> {
             unsafe {
                 raw_ids[i]
                     .as_mut_ptr()
-                    .byte_offset(T::DRIVER_DATA_OFFSET as _)
+                    .byte_add(T::DRIVER_DATA_OFFSET)
                     .cast::<usize>()
                     .write(i);
             }
diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
index 598001157293..20159b7c9293 100644
--- a/rust/kernel/devres.rs
+++ b/rust/kernel/devres.rs
@@ -45,7 +45,7 @@  struct DevresInner<T> {
 /// # Example
 ///
 /// ```no_run
-/// # use kernel::{bindings, c_str, device::Device, devres::Devres, io::{Io, IoRaw}};
+/// # use kernel::{bindings, c_str, device::Device, devres::Devres, ffi::c_void, io::{Io, IoRaw}};
 /// # use core::ops::Deref;
 ///
 /// // See also [`pci::Bar`] for a real example.
@@ -59,19 +59,19 @@  struct DevresInner<T> {
 ///     unsafe fn new(paddr: usize) -> Result<Self>{
 ///         // SAFETY: By the safety requirements of this function [`paddr`, `paddr` + `SIZE`) is
 ///         // valid for `ioremap`.
-///         let addr = unsafe { bindings::ioremap(paddr as _, SIZE as _) };
+///         let addr = unsafe { bindings::ioremap(paddr as u64, SIZE) };
 ///         if addr.is_null() {
 ///             return Err(ENOMEM);
 ///         }
 ///
-///         Ok(IoMem(IoRaw::new(addr as _, SIZE)?))
+///         Ok(IoMem(IoRaw::new(addr as usize, SIZE)?))
 ///     }
 /// }
 ///
 /// impl<const SIZE: usize> Drop for IoMem<SIZE> {
 ///     fn drop(&mut self) {
 ///         // SAFETY: `self.0.addr()` is guaranteed to be properly mapped by `Self::new`.
-///         unsafe { bindings::iounmap(self.0.addr() as _); };
+///         unsafe { bindings::iounmap(self.0.addr() as *mut c_void); };
 ///     }
 /// }
 ///
@@ -115,8 +115,9 @@  fn new(dev: &Device, data: T, flags: Flags) -> Result<Arc<DevresInner<T>>> {
 
         // SAFETY: `devm_add_action` guarantees to call `Self::devres_callback` once `dev` is
         // detached.
-        let ret =
-            unsafe { bindings::devm_add_action(dev.as_raw(), Some(inner.callback), data as _) };
+        let ret = unsafe {
+            bindings::devm_add_action(dev.as_raw(), Some(inner.callback), data.cast_mut().cast())
+        };
 
         if ret != 0 {
             // SAFETY: We just created another reference to `inner` in order to pass it to
@@ -130,7 +131,7 @@  fn new(dev: &Device, data: T, flags: Flags) -> Result<Arc<DevresInner<T>>> {
     }
 
     fn as_ptr(&self) -> *const Self {
-        self as _
+        self
     }
 
     fn remove_action(this: &Arc<Self>) {
diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
index 8654d52b0bb9..eb8fa52f08ba 100644
--- a/rust/kernel/error.rs
+++ b/rust/kernel/error.rs
@@ -152,7 +152,7 @@  pub(crate) fn to_blk_status(self) -> bindings::blk_status_t {
     /// Returns the error encoded as a pointer.
     pub fn to_ptr<T>(self) -> *mut T {
         // SAFETY: `self.0` is a valid error due to its invariant.
-        unsafe { bindings::ERR_PTR(self.0.get() as _).cast() }
+        unsafe { bindings::ERR_PTR(self.0.get() as isize).cast() }
     }
 
     /// Returns a string representing the error, if one exists.
diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs
index c5162fdc95ff..9a279330261c 100644
--- a/rust/kernel/firmware.rs
+++ b/rust/kernel/firmware.rs
@@ -61,7 +61,7 @@  fn request_internal(name: &CStr, dev: &Device, func: FwFunc) -> Result<Self> {
 
         // SAFETY: `pfw` is a valid pointer to a NULL initialized `bindings::firmware` pointer.
         // `name` and `dev` are valid as by their type invariants.
-        let ret = unsafe { func.0(pfw as _, name.as_char_ptr(), dev.as_raw()) };
+        let ret = unsafe { func.0(pfw, name.as_char_ptr(), dev.as_raw()) };
         if ret != 0 {
             return Err(Error::from_errno(ret));
         }
diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
index d4a73e52e3ee..d375eed73dc8 100644
--- a/rust/kernel/io.rs
+++ b/rust/kernel/io.rs
@@ -5,7 +5,7 @@ 
 //! C header: [`include/asm-generic/io.h`](srctree/include/asm-generic/io.h)
 
 use crate::error::{code::EINVAL, Result};
-use crate::{bindings, build_assert};
+use crate::{bindings, build_assert, ffi::c_void};
 
 /// Raw representation of an MMIO region.
 ///
@@ -56,7 +56,7 @@  pub fn maxsize(&self) -> usize {
 /// # Examples
 ///
 /// ```no_run
-/// # use kernel::{bindings, io::{Io, IoRaw}};
+/// # use kernel::{bindings, ffi::c_void, io::{Io, IoRaw}};
 /// # use core::ops::Deref;
 ///
 /// // See also [`pci::Bar`] for a real example.
@@ -70,19 +70,19 @@  pub fn maxsize(&self) -> usize {
 ///     unsafe fn new(paddr: usize) -> Result<Self>{
 ///         // SAFETY: By the safety requirements of this function [`paddr`, `paddr` + `SIZE`) is
 ///         // valid for `ioremap`.
-///         let addr = unsafe { bindings::ioremap(paddr as _, SIZE as _) };
+///         let addr = unsafe { bindings::ioremap(paddr as u64, SIZE) };
 ///         if addr.is_null() {
 ///             return Err(ENOMEM);
 ///         }
 ///
-///         Ok(IoMem(IoRaw::new(addr as _, SIZE)?))
+///         Ok(IoMem(IoRaw::new(addr as usize, SIZE)?))
 ///     }
 /// }
 ///
 /// impl<const SIZE: usize> Drop for IoMem<SIZE> {
 ///     fn drop(&mut self) {
 ///         // SAFETY: `self.0.addr()` is guaranteed to be properly mapped by `Self::new`.
-///         unsafe { bindings::iounmap(self.0.addr() as _); };
+///         unsafe { bindings::iounmap(self.0.addr() as *mut c_void); };
 ///     }
 /// }
 ///
@@ -119,7 +119,7 @@  pub fn $name(&self, offset: usize) -> $type_name {
             let addr = self.io_addr_assert::<$type_name>(offset);
 
             // SAFETY: By the type invariant `addr` is a valid address for MMIO operations.
-            unsafe { bindings::$name(addr as _) }
+            unsafe { bindings::$name(addr as *const c_void) }
         }
 
         /// Read IO data from a given offset.
@@ -131,7 +131,7 @@  pub fn $try_name(&self, offset: usize) -> Result<$type_name> {
             let addr = self.io_addr::<$type_name>(offset)?;
 
             // SAFETY: By the type invariant `addr` is a valid address for MMIO operations.
-            Ok(unsafe { bindings::$name(addr as _) })
+            Ok(unsafe { bindings::$name(addr as *const c_void) })
         }
     };
 }
@@ -148,7 +148,7 @@  pub fn $name(&self, value: $type_name, offset: usize) {
             let addr = self.io_addr_assert::<$type_name>(offset);
 
             // SAFETY: By the type invariant `addr` is a valid address for MMIO operations.
-            unsafe { bindings::$name(value, addr as _, ) }
+            unsafe { bindings::$name(value, addr as *mut c_void) }
         }
 
         /// Write IO data from a given offset.
@@ -160,7 +160,7 @@  pub fn $try_name(&self, value: $type_name, offset: usize) -> Result {
             let addr = self.io_addr::<$type_name>(offset)?;
 
             // SAFETY: By the type invariant `addr` is a valid address for MMIO operations.
-            unsafe { bindings::$name(value, addr as _) }
+            unsafe { bindings::$name(value, addr as *mut c_void) }
             Ok(())
         }
     };
diff --git a/rust/kernel/miscdevice.rs b/rust/kernel/miscdevice.rs
index e14433b2ab9d..2c66e926bffb 100644
--- a/rust/kernel/miscdevice.rs
+++ b/rust/kernel/miscdevice.rs
@@ -33,7 +33,7 @@  impl MiscDeviceOptions {
     pub const fn into_raw<T: MiscDevice>(self) -> bindings::miscdevice {
         // SAFETY: All zeros is valid for this C type.
         let mut result: bindings::miscdevice = unsafe { MaybeUninit::zeroed().assume_init() };
-        result.minor = bindings::MISC_DYNAMIC_MINOR as _;
+        result.minor = bindings::MISC_DYNAMIC_MINOR as i32;
         result.name = self.name.as_char_ptr();
         result.fops = create_vtable::<T>();
         result
diff --git a/rust/kernel/of.rs b/rust/kernel/of.rs
index 04f2d8ef29cb..40d1bd13682c 100644
--- a/rust/kernel/of.rs
+++ b/rust/kernel/of.rs
@@ -22,7 +22,7 @@  unsafe impl RawDeviceId for DeviceId {
     const DRIVER_DATA_OFFSET: usize = core::mem::offset_of!(bindings::of_device_id, data);
 
     fn index(&self) -> usize {
-        self.0.data as _
+        self.0.data as usize
     }
 }
 
@@ -34,10 +34,10 @@  pub const fn new(compatible: &'static CStr) -> Self {
         // SAFETY: FFI type is valid to be zero-initialized.
         let mut of: bindings::of_device_id = unsafe { core::mem::zeroed() };
 
-        // TODO: Use `clone_from_slice` once the corresponding types do match.
+        // TODO: Use `copy_from_slice` once stabilized for `const`.
         let mut i = 0;
         while i < src.len() {
-            of.compatible[i] = src[i] as _;
+            of.compatible[i] = src[i];
             i += 1;
         }
 
diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
index 63218abb7a25..a925732f6c7a 100644
--- a/rust/kernel/pci.rs
+++ b/rust/kernel/pci.rs
@@ -166,7 +166,7 @@  unsafe impl RawDeviceId for DeviceId {
     const DRIVER_DATA_OFFSET: usize = core::mem::offset_of!(bindings::pci_device_id, driver_data);
 
     fn index(&self) -> usize {
-        self.0.driver_data as _
+        self.0.driver_data
     }
 }
 
@@ -201,7 +201,10 @@  macro_rules! pci_device_table {
 ///     MODULE_PCI_TABLE,
 ///     <MyDriver as pci::Driver>::IdInfo,
 ///     [
-///         (pci::DeviceId::from_id(bindings::PCI_VENDOR_ID_REDHAT, bindings::PCI_ANY_ID as _), ())
+///         (
+///             pci::DeviceId::from_id(bindings::PCI_VENDOR_ID_REDHAT, bindings::PCI_ANY_ID as u32),
+///             (),
+///         )
 ///     ]
 /// );
 ///
@@ -317,7 +320,7 @@  unsafe fn do_release(pdev: &Device, ioptr: usize, num: i32) {
         // `ioptr` is valid by the safety requirements.
         // `num` is valid by the safety requirements.
         unsafe {
-            bindings::pci_iounmap(pdev.as_raw(), ioptr as _);
+            bindings::pci_iounmap(pdev.as_raw(), ioptr as *mut kernel::ffi::c_void);
             bindings::pci_release_region(pdev.as_raw(), num);
         }
     }
diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
index 6a1a982b946d..0b80a119d5f0 100644
--- a/rust/kernel/str.rs
+++ b/rust/kernel/str.rs
@@ -692,9 +692,9 @@  fn new() -> Self {
     pub(crate) unsafe fn from_ptrs(pos: *mut u8, end: *mut u8) -> Self {
         // INVARIANT: The safety requirements guarantee the type invariants.
         Self {
-            beg: pos as _,
-            pos: pos as _,
-            end: end as _,
+            beg: pos as usize,
+            pos: pos as usize,
+            end: end as usize,
         }
     }
 
@@ -719,7 +719,7 @@  pub(crate) unsafe fn from_buffer(buf: *mut u8, len: usize) -> Self {
     ///
     /// N.B. It may point to invalid memory.
     pub(crate) fn pos(&self) -> *mut u8 {
-        self.pos as _
+        self.pos as *mut u8
     }
 
     /// Returns the number of bytes written to the formatter.
diff --git a/rust/kernel/workqueue.rs b/rust/kernel/workqueue.rs
index 8ff54105be3f..d03f3440cb5a 100644
--- a/rust/kernel/workqueue.rs
+++ b/rust/kernel/workqueue.rs
@@ -198,7 +198,7 @@  pub fn enqueue<W, const ID: u64>(&self, w: W) -> W::EnqueueOutput
         unsafe {
             w.__enqueue(move |work_ptr| {
                 bindings::queue_work_on(
-                    bindings::wq_misc_consts_WORK_CPU_UNBOUND as _,
+                    bindings::wq_misc_consts_WORK_CPU_UNBOUND as i32,
                     queue_ptr,
                     work_ptr,
                 )