Message-ID: <D8EDP4SMQG2M.3HUNZGX8X0IL7@proton.me>
Date: Wed, 12 Mar 2025 15:05:08 +0000
From: Benno Lossin <benno.lossin@...ton.me>
To: Tamir Duberstein <tamird@...il.com>, 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>
Cc: 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 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?

>  ///         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)

>  ///     }
>  /// }
>  ///
>  /// 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>()`?

>  ///     }
>  /// }
>  ///

> 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()`?

>      }
>  
>      /// 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)

>          }
>  
>          /// 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()`.

>      }
>  }
>  
> @@ -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

@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.

>              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.

>          }
>      }
>  
> @@ -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)`

---
Cheers,
Benno

>      }
>  
>      /// 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,
>                  )

Powered by blists - more mailing lists