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

[Zhi Wang <zhiw@nvidia.com>]

On Fri, 31 Oct 2025 09:07:04 +0000
Alice Ryhl <aliceryhl@google.com> wrote:

> 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.
> 

I see your point. It also makes the code look cleaner.
From my understanding, this is essentially a choice between performing
static boundary checks through the type system using const generics, or
using build_assert!() with a trait or struct-associated constant.

Let me take a closer look and experiment a bit with it. :)

> >      /// Returns the base address of this mapping.
> > -    #[inline]
...ditto
> > +    /// 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.
> 

Yeah, I actually tried that in an earlier version.
I noticed that each backend is a bit different — for example, the PCI
config space routines don’t have read64()/write64() either. By
design, we don’t provide infallible versions for the PCI config space
backend (unlike the MMIO one). Other backends might have similar cases
as well.

So I ended up keeping the trait’s default implementation "minimal", only
including the methods every backend really has to implement. The default
impls are mainly there to catch situations where a driver calls something
it shouldn’t.

I should probably make the compiler complain when an infallible op isn’t
supported by a given backend. And if you have any ideas on making this
more elegant, I’m all ears. :)

Z.

> Alice