Message-ID: <CAJ-ks9=+3MQb-tp8TAwYvVj=GOFFFVKJxRMprc8YXZHKhqnDrg@mail.gmail.com>
Date: Wed, 12 Mar 2025 14:01:10 -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 1:05 PM Benno Lossin <benno.lossin@...ton.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@...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`?
>
> 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?

Powered by blists - more mailing lists