Re: [PATCH v7 3/6] rust: io: factor common I/O helpers into Io trait

["Alexandre Courbot" <acourbot@nvidia.com>]

On Tue Nov 25, 2025 at 11:22 PM JST, Alice Ryhl wrote:
> On Tue, Nov 25, 2025 at 3:15 PM Alexandre Courbot <acourbot@nvidia.com> wrote:
>>
>> On Tue Nov 25, 2025 at 11:09 PM JST, Alexandre Courbot wrote:
>> > On Wed Nov 19, 2025 at 8:21 PM JST, Zhi Wang wrote:
>> > <snip>
>> >> -impl<const SIZE: usize> Io<SIZE> {
>> >> -    /// Converts an `IoRaw` into an `Io` instance, providing the accessors to the MMIO mapping.
>> >> -    ///
>> >> -    /// # Safety
>> >> -    ///
>> >> -    /// Callers must ensure that `addr` is the start of a valid I/O mapped memory region of size
>> >> -    /// `maxsize`.
>> >> -    pub unsafe fn from_raw(raw: &IoRaw<SIZE>) -> &Self {
>> >> -        // SAFETY: `Io` is a transparent wrapper around `IoRaw`.
>> >> -        unsafe { &*core::ptr::from_ref(raw).cast() }
>> >> +/// Checks whether an access of type `U` at the given `offset`
>> >> +/// is valid within this region.
>> >> +#[inline]
>> >> +const fn offset_valid<U>(offset: usize, size: usize) -> bool {
>> >> +    let type_size = core::mem::size_of::<U>();
>> >> +    if let Some(end) = offset.checked_add(type_size) {
>> >> +        end <= size && offset % type_size == 0
>> >> +    } else {
>> >> +        false
>> >>      }
>> >> +}
>> >> +
>> >> +/// Represents a region of I/O space of a fixed size.
>> >> +///
>> >> +/// Provides common helpers for offset validation and address
>> >> +/// calculation on top of a base address and maximum size.
>> >> +///
>> >> +pub trait Io {
>> >> +    /// Minimum usable size of this region.
>> >> +    const MIN_SIZE: usize;
>> >
>> > This associated constant should probably be part of `IoInfallible` -
>> > otherwise what value should it take if some type only implement
>> > `IoFallible`?
>> >
>> >>
>> >>      /// Returns the base address of this mapping.
>> >> -    #[inline]
>> >> -    pub fn addr(&self) -> usize {
>> >> -        self.0.addr()
>> >> -    }
>> >> +    fn addr(&self) -> usize;
>> >>
>> >>      /// Returns the maximum size of this mapping.
>> >> -    #[inline]
>> >> -    pub fn maxsize(&self) -> usize {
>> >> -        self.0.maxsize()
>> >> -    }
>> >> -
>> >> -    #[inline]
>> >> -    const fn offset_valid<U>(offset: usize, size: usize) -> bool {
>> >> -        let type_size = core::mem::size_of::<U>();
>> >> -        if let Some(end) = offset.checked_add(type_size) {
>> >> -            end <= size && offset % type_size == 0
>> >> -        } else {
>> >> -            false
>> >> -        }
>> >> -    }
>> >> +    fn maxsize(&self) -> usize;
>> >>
>> >> +    /// Returns the absolute I/O address for a given `offset`,
>> >> +    /// performing runtime bound checks.
>> >>      #[inline]
>> >>      fn io_addr<U>(&self, offset: usize) -> Result<usize> {
>> >> -        if !Self::offset_valid::<U>(offset, self.maxsize()) {
>> >> +        if !offset_valid::<U>(offset, self.maxsize()) {
>> >>              return Err(EINVAL);
>> >>          }
>> >
>> > Similarly I cannot find any context where `maxsize` and `io_addr` are
>> > used outside of `IoFallible`, hinting that these should be part of this
>> > trait.
>> >
>> >>
>> >> @@ -239,50 +240,190 @@ fn io_addr<U>(&self, offset: usize) -> Result<usize> {
>> >>          self.addr().checked_add(offset).ok_or(EINVAL)
>> >>      }
>> >>
>> >> +    /// Returns the absolute I/O address for a given `offset`,
>> >> +    /// performing compile-time bound checks.
>> >>      #[inline]
>> >>      fn io_addr_assert<U>(&self, offset: usize) -> usize {
>> >> -        build_assert!(Self::offset_valid::<U>(offset, SIZE));
>> >> +        build_assert!(offset_valid::<U>(offset, Self::MIN_SIZE));
>> >>
>> >>          self.addr() + offset
>> >>      }
>> >
>> > ... and `io_addr_assert` is only used from `IoFallible`.
>> >
>> > So if my gut feeling is correct, we can disband `Io` entirely and only
>>
>> ... except that we can't due to `addr`, unless we find a better way to
>> provide this base. But even if we need to keep `Io`, the compile-time
>> and runtime members should be moved to their respective traits.
>
> If you have IoInfallible depend on IoFallible, then you can place
> `addr` on IoFallible.

Indeed. Maybe we could even make `IoInfallible` automatically
implemented, since it just needs to `unwrap_unchecked` the fallible
implementation if the range is valid.

> (And I still think you should rename IoFallible to Io and IoInfallible
> to IoKnownSize.)

Agreed, there are other reasons for I/O to fail than a bad index so this
should not be part of the name of these traits.