On Wed, Mar 12, 2025 at 1:05 PM Benno Lossin benno.lossin@proton.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@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<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 usizeThis 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/131415Yep! 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?