Re: [PATCH v9 4/6] rust: enable `clippy::as_underscore` lint

[Boqun Feng <boqun.feng@gmail.com>]

On Wed, Apr 16, 2025 at 01:36:08PM -0400, Tamir Duberstein wrote:
> 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]
> Reviewed-by: Benno Lossin <benno.lossin@proton.me>
> 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/dma.rs                 |  2 +-
>  rust/kernel/error.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(-)
> 
> diff --git a/Makefile b/Makefile
> index 04a5246171f9..57080a64913f 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -475,6 +475,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 af5c9ac94f36..22697104bf8c 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) };
>  

We could consider defining a const block::mq::BLK_STATUS_OK as:

	const BLK_STATUS_OK: u8 = bindings::BLK_STS_OK as u8;

, because repeating the as pattern is a bit err-prone. But maybe in a
later patch.

>          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 f7e8f5f53622..70d12014e476 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 bindings::phys_addr_t, SIZE) };


///         let addr = unsafe { bindings::ioremap(bindings::phys_addr_t::from(paddr), SIZE) };

better? Or even with .into()

///         let addr = unsafe { bindings::ioremap(paddr.into(), 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); };
>  ///     }
>  /// }
>  ///
[...]
> diff --git a/rust/kernel/dma.rs b/rust/kernel/dma.rs
> index 43ecf3c2e860..851a6339aa90 100644
> --- a/rust/kernel/dma.rs
> +++ b/rust/kernel/dma.rs
> @@ -38,7 +38,7 @@
>  impl Attrs {
>      /// Get the raw representation of this attribute.
>      pub(crate) fn as_raw(self) -> crate::ffi::c_ulong {
> -        self.0 as _
> +        self.0 as crate::ffi::c_ulong

        crate::ffi::c_ulong::from(self.0)

maybe, a C unsigned long should always be able to hold the whole `Attr`
and a lossly casting is what this function does.

>      }
>  
>      /// Check whether `flags` is contained in `self`.
[...]
> @@ -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 bindings::phys_addr_t, SIZE) };

Similarly:

///         let addr = unsafe { bindings::ioremap(paddr.into(), SIZE) };

or `from()`.

>  ///         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); };
>  ///     }
>  /// }
>  ///
[...]

The rest looks good to me. Thanks!

Regards,
Boqun