Re: [PATCH v2 5/5] rust: enable `clippy::as_underscore` lint

Benno Lossin
2025-03-12T17:05:50Z

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@proton.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(&self, offset: usize) -> usize {
> build_assert!(Self::offset_valid::(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