Re: [PATCH] rust: io: move MIN_SIZE and io_addr_assert to IoKnownSize

[Charalampos Mitrodimas <charmitro@posteo.net>]

Alexandre Courbot <acourbot@nvidia.com> writes:

> `MIN_SIZE` are `io_addr_assert` only ever used for IO types which

Hi,

Typo? should be "and" instead of "are"?

Cheers,
C. Mitrodimas

> implement `IoKnownSize` and do not make sense for types that don't.
>
> It looks like they should have been there since the beginning, so move
> them while the code is still fresh.
>
> Also update `IoKnownSize`'s documentation since it is not just a marker
> trait anymore.
>
> Fixes: 121d87b28e1d ("rust: io: separate generic I/O helpers from MMIO implementation")
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
>  rust/kernel/io.rs     | 36 ++++++++++++++++++------------------
>  rust/kernel/pci/io.rs |  7 +++----
>  2 files changed, 21 insertions(+), 22 deletions(-)
>
> diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
> index 056a3ec71647..c1cca7b438c3 100644
> --- a/rust/kernel/io.rs
> +++ b/rust/kernel/io.rs
> @@ -301,9 +301,6 @@ pub trait IoCapable<T> {}
>  /// For MMIO regions, all widths (u8, u16, u32, and u64 on 64-bit systems) are typically
>  /// supported. For PCI configuration space, u8, u16, and u32 are supported but u64 is not.
>  pub trait Io {
> -    /// Minimum usable size of this region.
> -    const MIN_SIZE: usize;
> -
>      /// Returns the base address of this mapping.
>      fn addr(&self) -> usize;
>  
> @@ -323,16 +320,6 @@ 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.
> -    // Always inline to optimize out error path of `build_assert`.
> -    #[inline(always)]
> -    fn io_addr_assert<U>(&self, offset: usize) -> usize {
> -        build_assert!(offset_valid::<U>(offset, Self::MIN_SIZE));
> -
> -        self.addr() + offset
> -    }
> -
>      /// Fallible 8-bit read with runtime bounds check.
>      #[inline(always)]
>      fn try_read8(&self, _offset: usize) -> Result<u8>
> @@ -478,14 +465,27 @@ fn write64(&self, _value: u64, _offset: usize)
>      }
>  }
>  
> -/// Marker trait for types with a known size at compile time.
> +/// Trait for types with a known size at compile time.
>  ///
>  /// This trait is implemented by I/O backends that have a compile-time known size,
>  /// enabling the use of infallible I/O accessors with compile-time bounds checking.
>  ///
>  /// Types implementing this trait can use the infallible methods in [`Io`] trait
>  /// (e.g., `read8`, `write32`), which require `Self: IoKnownSize` bound.
> -pub trait IoKnownSize: Io {}
> +pub trait IoKnownSize: Io {
> +    /// Minimum usable size of this region.
> +    const MIN_SIZE: usize;
> +
> +    /// Returns the absolute I/O address for a given `offset`,
> +    /// performing compile-time bound checks.
> +    // Always inline to optimize out error path of `build_assert`.
> +    #[inline(always)]
> +    fn io_addr_assert<U>(&self, offset: usize) -> usize {
> +        build_assert!(offset_valid::<U>(offset, Self::MIN_SIZE));
> +
> +        self.addr() + offset
> +    }
> +}
>  
>  // MMIO regions support 8, 16, and 32-bit accesses.
>  impl<const SIZE: usize> IoCapable<u8> for Mmio<SIZE> {}
> @@ -497,8 +497,6 @@ impl<const SIZE: usize> IoCapable<u32> for Mmio<SIZE> {}
>  impl<const SIZE: usize> IoCapable<u64> for Mmio<SIZE> {}
>  
>  impl<const SIZE: usize> Io for Mmio<SIZE> {
> -    const MIN_SIZE: usize = SIZE;
> -
>      /// Returns the base address of this mapping.
>      #[inline]
>      fn addr(&self) -> usize {
> @@ -552,7 +550,9 @@ fn maxsize(&self) -> usize {
>      );
>  }
>  
> -impl<const SIZE: usize> IoKnownSize for Mmio<SIZE> {}
> +impl<const SIZE: usize> IoKnownSize for Mmio<SIZE> {
> +    const MIN_SIZE: usize = SIZE;
> +}
>  
>  impl<const SIZE: usize> Mmio<SIZE> {
>      /// Converts an `MmioRaw` into an `Mmio` instance, providing the accessors to the MMIO mapping.
> diff --git a/rust/kernel/pci/io.rs b/rust/kernel/pci/io.rs
> index 026e7a3b69bd..6ca4cf75594c 100644
> --- a/rust/kernel/pci/io.rs
> +++ b/rust/kernel/pci/io.rs
> @@ -148,8 +148,6 @@ impl<'a, S: ConfigSpaceKind> IoCapable<u16> for ConfigSpace<'a, S> {}
>  impl<'a, S: ConfigSpaceKind> IoCapable<u32> for ConfigSpace<'a, S> {}
>  
>  impl<'a, S: ConfigSpaceKind> Io for ConfigSpace<'a, S> {
> -    const MIN_SIZE: usize = S::SIZE;
> -
>      /// Returns the base address of the I/O region. It is always 0 for configuration space.
>      #[inline]
>      fn addr(&self) -> usize {
> @@ -174,8 +172,9 @@ fn maxsize(&self) -> usize {
>      define_write!(infallible, write32, call_config_write(pci_write_config_dword) <- u32);
>  }
>  
> -/// Marker trait indicating ConfigSpace has a known size at compile time.
> -impl<'a, S: ConfigSpaceKind> IoKnownSize for ConfigSpace<'a, S> {}
> +impl<'a, S: ConfigSpaceKind> IoKnownSize for ConfigSpace<'a, S> {
> +    const MIN_SIZE: usize = S::SIZE;
> +}
>  
>  /// A PCI BAR to perform I/O-Operations on.
>  ///
>
> ---
> base-commit: c71257394bc9c59ea727803f6e55e83fe63db74e
> change-id: 20260130-io-min-size-f3c289c148ae
>
> Best regards,