Message-ID: <CAJ-ks9=K06OT6cutUABj2QDHJHJ70719c-eJ=F3n-_bhkYbZ3w@mail.gmail.com>
Date: Wed, 12 Mar 2025 11:35:29 -0400
From: Tamir Duberstein <tamird@...il.com>
To: Benno Lossin <benno.lossin@...ton.me>
Cc: Masahiro Yamada <masahiroy@...nel.org>, Nathan Chancellor <nathan@...nel.org>, 
	Nicolas Schier <nicolas@...sle.eu>, Miguel Ojeda <ojeda@...nel.org>, 
	Alex Gaynor <alex.gaynor@...il.com>, Boqun Feng <boqun.feng@...il.com>, 
	Gary Guo <gary@...yguo.net>, Björn Roy Baron <bjorn3_gh@...tonmail.com>, 
	Andreas Hindborg <a.hindborg@...nel.org>, Alice Ryhl <aliceryhl@...gle.com>, 
	Trevor Gross <tmgross@...ch.edu>, Danilo Krummrich <dakr@...nel.org>, 
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>, "Rafael J. Wysocki" <rafael@...nel.org>, 
	Brendan Higgins <brendan.higgins@...ux.dev>, David Gow <davidgow@...gle.com>, 
	Rae Moar <rmoar@...gle.com>, Bjorn Helgaas <bhelgaas@...gle.com>, 
	Luis Chamberlain <mcgrof@...nel.org>, Russ Weight <russ.weight@...ux.dev>, Rob Herring <robh@...nel.org>, 
	Saravana Kannan <saravanak@...gle.com>, linux-kbuild@...r.kernel.org, 
	linux-kernel@...r.kernel.org, rust-for-linux@...r.kernel.org, 
	linux-kselftest@...r.kernel.org, kunit-dev@...glegroups.com, 
	linux-pci@...r.kernel.org, linux-block@...r.kernel.org, 
	devicetree@...r.kernel.org
Subject: Re: [PATCH v2 5/5] rust: enable `clippy::as_underscore` lint

On Wed, Mar 12, 2025 at 11:05 AM Benno Lossin <benno.lossin@...ton.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.

Powered by blists - more mailing lists