Message-Id: <DEHU2XNZ50HW.281CCT1CZ79CF@nvidia.com>
Date: Tue, 25 Nov 2025 23:09:10 +0900
From: "Alexandre Courbot" <acourbot@...dia.com>
To: "Zhi Wang" <zhiw@...dia.com>, <rust-for-linux@...r.kernel.org>,
 <linux-pci@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Cc: <dakr@...nel.org>, <aliceryhl@...gle.com>, <bhelgaas@...gle.com>,
 <kwilczynski@...nel.org>, <ojeda@...nel.org>, <alex.gaynor@...il.com>,
 <boqun.feng@...il.com>, <gary@...yguo.net>, <bjorn3_gh@...tonmail.com>,
 <lossin@...nel.org>, <a.hindborg@...nel.org>, <tmgross@...ch.edu>,
 <markus.probst@...teo.de>, <helgaas@...nel.org>, <cjia@...dia.com>,
 <smitra@...dia.com>, <ankita@...dia.com>, <aniketa@...dia.com>,
 <kwankhede@...dia.com>, <targupta@...dia.com>, <acourbot@...dia.com>,
 <joelagnelf@...dia.com>, <jhubbard@...dia.com>, <zhiwang@...nel.org>
Subject: Re: [PATCH v7 3/6] rust: io: factor common I/O helpers into Io
 trait

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
rely on `IoFallible` and `IoInfallible`, which is exactly what Alice
said in her review. It makes sense to me as well because as she
mentioned, users need to specify one or the other already if they want
to call I/O methods - so why keep part of their non-shared functionality
in another trait.

This design would also reflect the fact that they perform the same
checks, except one does them at compile-time and the other at runtime.

Another point that Alice mentioned is that if you can do the check at
compile-time, you should be able to do it at runtime as well, and some
(most) non-bus specific APIs will probably only expect an `IoFallible`.
For these cases, rather than imposing a hierarchy on the traits, I'd
suggest a e.g. `IoFallibleAdapter` that wraps a reference to a
`IoInfallible` and exposes the fallible API.

<snip>
> +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 {
> +        self.0.addr()
> +    }
> +
> +    /// Returns the maximum size of this mapping.
> +    #[inline]
> +    fn maxsize(&self) -> usize {
> +        self.0.maxsize()
> +    }

Nit: when implementing a trait, you don't need to repeat the doccomment
of its methods - LSP will pick them from the source.

Powered by blists - more mailing lists