On Thu, Apr 17, 2025 at 1:55 PM Boqun Feng <boqun.f...@gmail.com> wrote:
>
> 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.los...@proton.me>
> > Signed-off-by: Tamir Duberstein <tam...@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.
Sure. I think there's only this instance at the moment.
>
> > 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) };
This doesn't compile because `paddr` is usize, and
`bindings::phys_addr_t` is u64 (on my machine, which is aarch64).
> > /// 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.
This also doesn't compile: "the trait `core::convert::From<u32>` is
not implemented for `usize`". Upstream has ambitions of running on
16-bit, I guess :)
>
> > }
> >
> > /// 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()`.
As above, doesn't compile.
> > /// 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
Thanks!
