Re: [PATCH v3 1/5] rust: io: factor common I/O helpers into Io trait

[Alice Ryhl <aliceryhl@google.com>]

On Thu, Oct 30, 2025 at 03:48:38PM +0000, Zhi Wang wrote:
> The previous Io<SIZE> type combined both the generic I/O access helpers
> and MMIO implementation details in a single struct.
> 
> To establish a cleaner layering between the I/O interface and its concrete
> backends, paving the way for supporting additional I/O mechanisms in the
> future, Io<SIZE> need to be factored.
> 
> Factor the common helpers into a new Io trait, and move the MMIO-specific
> logic into a dedicated Mmio<SIZE> type implementing that trait. Rename the
> IoRaw to MmioRaw and update the bus MMIO implementations to use MmioRaw.
> 
> No functional change intended.
> 
> Cc: Alexandre Courbot <acourbot@nvidia.com>
> Cc: Bjorn Helgaas <helgaas@kernel.org>
> Cc: Danilo Krummrich <dakr@kernel.org>
> Cc: John Hubbard <jhubbard@nvidia.com>
> Signed-off-by: Zhi Wang <zhiw@nvidia.com>

> +/// 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.
> +///
> +/// Types implementing this trait (e.g. MMIO BARs or PCI config
> +/// regions) can share the same accessors.
> +pub trait Io<const SIZE: usize> {

I would consider moving SIZE to an associated constant.

	pub trait Io {
	    const MIN_SIZE: usize;
	
	    ...
	}

If it's a generic parameter, then the same type can implement both Io<5>
and Io<7> at the same time, but I don't think it makes sense for a
single type to implement Io with different minimum sizes.

>      /// 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`.
> +    /// Performs runtime bounds checks using [`offset_valid`]
>      #[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);
>          }
>  
> @@ -217,50 +215,197 @@ 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, SIZE));
>  
>          self.addr() + offset
>      }
>  
> -    define_read!(read8, try_read8, readb -> u8);
> -    define_read!(read16, try_read16, readw -> u16);
> -    define_read!(read32, try_read32, readl -> u32);
> +    /// Infallible 8-bit read with compile-time bounds check.
> +    fn read8(&self, _offset: usize) -> u8 {
> +        !0
> +    }
> +
> +    /// Infallible 16-bit read with compile-time bounds check.
> +    fn read16(&self, _offset: usize) -> u16 {
> +        !0
> +    }
> +
> +    /// Infallible 32-bit read with compile-time bounds check.
> +    fn read32(&self, _offset: usize) -> u32 {
> +        !0
> +    }
> +
> +    /// Infallible 64-bit read with compile-time bounds check (64-bit only).
> +    #[cfg(CONFIG_64BIT)]
> +    fn read64(&self, _offset: usize) -> u64 {
> +        !0
> +    }
> +
> +    /// Fallible 8-bit read with runtime bounds check.
> +    fn try_read8(&self, _offset: usize) -> Result<u8> {
> +        Err(ENOTSUPP)
> +    }
> +
> +    /// Fallible 16-bit read with runtime bounds check.
> +    fn try_read16(&self, _offset: usize) -> Result<u16> {
> +        Err(ENOTSUPP)
> +    }
> +
> +    /// Fallible 32-bit read with runtime bounds check.
> +    fn try_read32(&self, _offset: usize) -> Result<u32> {
> +        Err(ENOTSUPP)
> +    }
> +
> +    /// Fallible 64-bit read with runtime bounds check (64-bit only).
> +    #[cfg(CONFIG_64BIT)]
> +    fn try_read64(&self, _offset: usize) -> Result<u64> {
> +        Err(ENOTSUPP)
> +    }
> +
> +    /// Infallible 8-bit write with compile-time bounds check.
> +    fn write8(&self, _value: u8, _offset: usize) {
> +        ()
> +    }
> +
> +    /// Infallible 16-bit write with compile-time bounds check.
> +    fn write16(&self, _value: u16, _offset: usize) {
> +        ()
> +    }
> +
> +    /// Infallible 32-bit write with compile-time bounds check.
> +    fn write32(&self, _value: u32, _offset: usize) {
> +        ()
> +    }
> +
> +    /// Infallible 64-bit write with compile-time bounds check (64-bit only).
> +    #[cfg(CONFIG_64BIT)]
> +    fn write64(&self, _value: u64, _offset: usize) {
> +        ()
> +    }
> +
> +    /// Fallible 8-bit write with runtime bounds check.
> +    fn try_write8(&self, value: u8, offset: usize) -> Result;
> +
> +    /// Fallible 16-bit write with runtime bounds check.
> +    fn try_write16(&self, value: u16, offset: usize) -> Result;
> +
> +    /// Fallible 32-bit write with runtime bounds check.
> +    fn try_write32(&self, value: u32, offset: usize) -> Result;
> +
> +    /// Fallible 64-bit write with runtime bounds check (64-bit only).
> +    #[cfg(CONFIG_64BIT)]
> +    fn try_write64(&self, _value: u64, _offset: usize) -> Result {
> +        Err(ENOTSUPP)
> +    }

Why are there default implementations for all of these trait methods? I
would suggest not providing any default implementations at all.

Alice