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 new {Io, Io64} traits, 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: Alice Ryhl <aliceryhl@google.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>
---
drivers/gpu/nova-core/regs/macros.rs | 90 +++++----
drivers/gpu/nova-core/vbios.rs | 1 +
rust/kernel/devres.rs | 14 +-
rust/kernel/io.rs | 283 ++++++++++++++++++++-------
rust/kernel/io/mem.rs | 16 +-
rust/kernel/io/poll.rs | 8 +-
rust/kernel/pci/io.rs | 12 +-
samples/rust/rust_driver_pci.rs | 2 +
8 files changed, 295 insertions(+), 131 deletions(-)
diff --git a/drivers/gpu/nova-core/regs/macros.rs b/drivers/gpu/nova-core/regs/macros.rs
index 8058e1696df9..39b1069a3429 100644
--- a/drivers/gpu/nova-core/regs/macros.rs
+++ b/drivers/gpu/nova-core/regs/macros.rs
@@ -608,16 +608,18 @@ impl $name {
/// Read the register from its address in `io`.
#[inline(always)]
- pub(crate) fn read<const SIZE: usize, T>(io: &T) -> Self where
- T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
+ pub(crate) fn read<T, I>(io: &T) -> Self where
+ T: ::core::ops::Deref<Target = I>,
+ I: ::kernel::io::IoInfallible,
{
Self(io.read32($offset))
}
/// Write the value contained in `self` to the register address in `io`.
#[inline(always)]
- pub(crate) fn write<const SIZE: usize, T>(self, io: &T) where
- T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
+ pub(crate) fn write<T, I>(self, io: &T) where
+ T: ::core::ops::Deref<Target = I>,
+ I: ::kernel::io::IoInfallible,
{
io.write32(self.0, $offset)
}
@@ -625,11 +627,12 @@ pub(crate) fn write<const SIZE: usize, T>(self, io: &T) where
/// Read the register from its address in `io` and run `f` on its value to obtain a new
/// value to write back.
#[inline(always)]
- pub(crate) fn alter<const SIZE: usize, T, F>(
+ pub(crate) fn alter<T, I, F>(
io: &T,
f: F,
) where
- T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
+ T: ::core::ops::Deref<Target = I>,
+ I: ::kernel::io::IoInfallible,
F: ::core::ops::FnOnce(Self) -> Self,
{
let reg = f(Self::read(io));
@@ -647,12 +650,13 @@ impl $name {
/// Read the register from `io`, using the base address provided by `base` and adding
/// the register's offset to it.
#[inline(always)]
- pub(crate) fn read<const SIZE: usize, T, B>(
+ pub(crate) fn read<T, I, B>(
io: &T,
#[allow(unused_variables)]
base: &B,
) -> Self where
- T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
+ T: ::core::ops::Deref<Target = I>,
+ I: ::kernel::io::IoInfallible,
B: crate::regs::macros::RegisterBase<$base>,
{
const OFFSET: usize = $name::OFFSET;
@@ -667,13 +671,14 @@ pub(crate) fn read<const SIZE: usize, T, B>(
/// Write the value contained in `self` to `io`, using the base address provided by
/// `base` and adding the register's offset to it.
#[inline(always)]
- pub(crate) fn write<const SIZE: usize, T, B>(
+ pub(crate) fn write<T, I, B>(
self,
io: &T,
#[allow(unused_variables)]
base: &B,
) where
- T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
+ T: ::core::ops::Deref<Target = I>,
+ I: ::kernel::io::IoInfallible,
B: crate::regs::macros::RegisterBase<$base>,
{
const OFFSET: usize = $name::OFFSET;
@@ -688,12 +693,13 @@ pub(crate) fn write<const SIZE: usize, T, B>(
/// the register's offset to it, then run `f` on its value to obtain a new value to
/// write back.
#[inline(always)]
- pub(crate) fn alter<const SIZE: usize, T, B, F>(
+ pub(crate) fn alter<T, I, B, F>(
io: &T,
base: &B,
f: F,
) where
- T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
+ T: ::core::ops::Deref<Target = I>,
+ I: ::kernel::io::IoInfallible,
B: crate::regs::macros::RegisterBase<$base>,
F: ::core::ops::FnOnce(Self) -> Self,
{
@@ -713,11 +719,12 @@ impl $name {
/// Read the array register at index `idx` from its address in `io`.
#[inline(always)]
- pub(crate) fn read<const SIZE: usize, T>(
+ pub(crate) fn read<T, I>(
io: &T,
idx: usize,
) -> Self where
- T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
+ T: ::core::ops::Deref<Target = I>,
+ I: ::kernel::io::IoInfallible,
{
build_assert!(idx < Self::SIZE);
@@ -729,12 +736,13 @@ pub(crate) fn read<const SIZE: usize, T>(
/// Write the value contained in `self` to the array register with index `idx` in `io`.
#[inline(always)]
- pub(crate) fn write<const SIZE: usize, T>(
+ pub(crate) fn write<T, I>(
self,
io: &T,
idx: usize
) where
- T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
+ T: ::core::ops::Deref<Target = I>,
+ I: ::kernel::io::IoInfallible,
{
build_assert!(idx < Self::SIZE);
@@ -746,12 +754,13 @@ pub(crate) fn write<const SIZE: usize, T>(
/// Read the array register at index `idx` in `io` and run `f` on its value to obtain a
/// new value to write back.
#[inline(always)]
- pub(crate) fn alter<const SIZE: usize, T, F>(
+ pub(crate) fn alter<T, I, F>(
io: &T,
idx: usize,
f: F,
) where
- T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
+ T: ::core::ops::Deref<Target = I>,
+ I: ::kernel::io::IoInfallible,
F: ::core::ops::FnOnce(Self) -> Self,
{
let reg = f(Self::read(io, idx));
@@ -763,11 +772,12 @@ pub(crate) fn alter<const SIZE: usize, T, F>(
/// The validity of `idx` is checked at run-time, and `EINVAL` is returned is the
/// access was out-of-bounds.
#[inline(always)]
- pub(crate) fn try_read<const SIZE: usize, T>(
+ pub(crate) fn try_read<T, I>(
io: &T,
idx: usize,
) -> ::kernel::error::Result<Self> where
- T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
+ T: ::core::ops::Deref<Target = I>,
+ I: ::kernel::io::IoInfallible,
{
if idx < Self::SIZE {
Ok(Self::read(io, idx))
@@ -781,12 +791,13 @@ pub(crate) fn try_read<const SIZE: usize, T>(
/// The validity of `idx` is checked at run-time, and `EINVAL` is returned is the
/// access was out-of-bounds.
#[inline(always)]
- pub(crate) fn try_write<const SIZE: usize, T>(
+ pub(crate) fn try_write<T, I>(
self,
io: &T,
idx: usize,
) -> ::kernel::error::Result where
- T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
+ T: ::core::ops::Deref<Target = I>,
+ I: ::kernel::io::IoInfallible,
{
if idx < Self::SIZE {
Ok(self.write(io, idx))
@@ -801,12 +812,13 @@ pub(crate) fn try_write<const SIZE: usize, T>(
/// The validity of `idx` is checked at run-time, and `EINVAL` is returned is the
/// access was out-of-bounds.
#[inline(always)]
- pub(crate) fn try_alter<const SIZE: usize, T, F>(
+ pub(crate) fn try_alter<T, I, F>(
io: &T,
idx: usize,
f: F,
) -> ::kernel::error::Result where
- T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
+ T: ::core::ops::Deref<Target = I>,
+ I: ::kernel::io::IoInfallible,
F: ::core::ops::FnOnce(Self) -> Self,
{
if idx < Self::SIZE {
@@ -832,13 +844,14 @@ impl $name {
/// Read the array register at index `idx` from `io`, using the base address provided
/// by `base` and adding the register's offset to it.
#[inline(always)]
- pub(crate) fn read<const SIZE: usize, T, B>(
+ pub(crate) fn read<T, I, B>(
io: &T,
#[allow(unused_variables)]
base: &B,
idx: usize,
) -> Self where
- T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
+ T: ::core::ops::Deref<Target = I>,
+ I: ::kernel::io::IoInfallible,
B: crate::regs::macros::RegisterBase<$base>,
{
build_assert!(idx < Self::SIZE);
@@ -853,14 +866,15 @@ pub(crate) fn read<const SIZE: usize, T, B>(
/// Write the value contained in `self` to `io`, using the base address provided by
/// `base` and adding the offset of array register `idx` to it.
#[inline(always)]
- pub(crate) fn write<const SIZE: usize, T, B>(
+ pub(crate) fn write<T, I, B>(
self,
io: &T,
#[allow(unused_variables)]
base: &B,
idx: usize
) where
- T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
+ T: ::core::ops::Deref<Target = I>,
+ I: ::kernel::io::IoInfallible,
B: crate::regs::macros::RegisterBase<$base>,
{
build_assert!(idx < Self::SIZE);
@@ -875,13 +889,14 @@ pub(crate) fn write<const SIZE: usize, T, B>(
/// by `base` and adding the register's offset to it, then run `f` on its value to
/// obtain a new value to write back.
#[inline(always)]
- pub(crate) fn alter<const SIZE: usize, T, B, F>(
+ pub(crate) fn alter<T, I, B, F>(
io: &T,
base: &B,
idx: usize,
f: F,
) where
- T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
+ T: ::core::ops::Deref<Target = I>,
+ I: ::kernel::io::IoInfallible,
B: crate::regs::macros::RegisterBase<$base>,
F: ::core::ops::FnOnce(Self) -> Self,
{
@@ -895,12 +910,13 @@ pub(crate) fn alter<const SIZE: usize, T, B, F>(
/// The validity of `idx` is checked at run-time, and `EINVAL` is returned is the
/// access was out-of-bounds.
#[inline(always)]
- pub(crate) fn try_read<const SIZE: usize, T, B>(
+ pub(crate) fn try_read<T, I, B>(
io: &T,
base: &B,
idx: usize,
) -> ::kernel::error::Result<Self> where
- T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
+ T: ::core::ops::Deref<Target = I>,
+ I: ::kernel::io::IoInfallible,
B: crate::regs::macros::RegisterBase<$base>,
{
if idx < Self::SIZE {
@@ -916,13 +932,14 @@ pub(crate) fn try_read<const SIZE: usize, T, B>(
/// The validity of `idx` is checked at run-time, and `EINVAL` is returned is the
/// access was out-of-bounds.
#[inline(always)]
- pub(crate) fn try_write<const SIZE: usize, T, B>(
+ pub(crate) fn try_write<T, I, B>(
self,
io: &T,
base: &B,
idx: usize,
) -> ::kernel::error::Result where
- T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
+ T: ::core::ops::Deref<Target = I>,
+ I: ::kernel::io::IoInfallible,
B: crate::regs::macros::RegisterBase<$base>,
{
if idx < Self::SIZE {
@@ -939,13 +956,14 @@ pub(crate) fn try_write<const SIZE: usize, T, B>(
/// The validity of `idx` is checked at run-time, and `EINVAL` is returned is the
/// access was out-of-bounds.
#[inline(always)]
- pub(crate) fn try_alter<const SIZE: usize, T, B, F>(
+ pub(crate) fn try_alter<T, I, B, F>(
io: &T,
base: &B,
idx: usize,
f: F,
) -> ::kernel::error::Result where
- T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
+ T: ::core::ops::Deref<Target = I>,
+ I: ::kernel::io::IoInfallible,
B: crate::regs::macros::RegisterBase<$base>,
F: ::core::ops::FnOnce(Self) -> Self,
{
diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
index 71fbe71b84db..7a0121ab9b09 100644
--- a/drivers/gpu/nova-core/vbios.rs
+++ b/drivers/gpu/nova-core/vbios.rs
@@ -8,6 +8,7 @@
use core::convert::TryFrom;
use kernel::device;
use kernel::error::Result;
+use kernel::io::IoFallible;
use kernel::prelude::*;
use kernel::ptr::{Alignable, Alignment};
use kernel::types::ARef;
diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
index 659f513a72a6..ad72943adbbe 100644
--- a/rust/kernel/devres.rs
+++ b/rust/kernel/devres.rs
@@ -75,15 +75,16 @@ struct Inner<T: Send> {
/// },
/// devres::Devres,
/// io::{
-/// Io,
-/// IoRaw,
+/// IoInfallible,
+/// Mmio,
+/// MmioRaw,
/// PhysAddr,
/// },
/// };
/// use core::ops::Deref;
///
/// // See also [`pci::Bar`] for a real example.
-/// struct IoMem<const SIZE: usize>(IoRaw<SIZE>);
+/// struct IoMem<const SIZE: usize>(MmioRaw<SIZE>);
///
/// impl<const SIZE: usize> IoMem<SIZE> {
/// /// # Safety
@@ -98,7 +99,7 @@ struct Inner<T: Send> {
/// return Err(ENOMEM);
/// }
///
-/// Ok(IoMem(IoRaw::new(addr as usize, SIZE)?))
+/// Ok(IoMem(MmioRaw::new(addr as usize, SIZE)?))
/// }
/// }
///
@@ -110,11 +111,11 @@ struct Inner<T: Send> {
/// }
///
/// impl<const SIZE: usize> Deref for IoMem<SIZE> {
-/// type Target = Io<SIZE>;
+/// type Target = Mmio<SIZE>;
///
/// fn deref(&self) -> &Self::Target {
/// // SAFETY: The memory range stored in `self` has been properly mapped in `Self::new`.
-/// unsafe { Io::from_raw(&self.0) }
+/// unsafe { Mmio::from_raw(&self.0) }
/// }
/// }
/// # fn no_run(dev: &Device<Bound>) -> Result<(), Error> {
@@ -260,6 +261,7 @@ pub fn device(&self) -> &Device {
/// use kernel::{
/// device::Core,
/// devres::Devres,
+ /// io::IoInfallible,
/// pci, //
/// };
///
diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
index 98e8b84e68d1..3e6ea29835ae 100644
--- a/rust/kernel/io.rs
+++ b/rust/kernel/io.rs
@@ -32,16 +32,16 @@
/// By itself, the existence of an instance of this structure does not provide any guarantees that
/// the represented MMIO region does exist or is properly mapped.
///
-/// Instead, the bus specific MMIO implementation must convert this raw representation into an `Io`
-/// instance providing the actual memory accessors. Only by the conversion into an `Io` structure
-/// any guarantees are given.
-pub struct IoRaw<const SIZE: usize = 0> {
+/// Instead, the bus specific MMIO implementation must convert this raw representation into an
+/// `Mmio` instance providing the actual memory accessors. Only by the conversion into an `Mmio`
+/// structure any guarantees are given.
+pub struct MmioRaw<const SIZE: usize = 0> {
addr: usize,
maxsize: usize,
}
-impl<const SIZE: usize> IoRaw<SIZE> {
- /// Returns a new `IoRaw` instance on success, an error otherwise.
+impl<const SIZE: usize> MmioRaw<SIZE> {
+ /// Returns a new `MmioRaw` instance on success, an error otherwise.
pub fn new(addr: usize, maxsize: usize) -> Result<Self> {
if maxsize < SIZE {
return Err(EINVAL);
@@ -80,15 +80,17 @@ pub fn maxsize(&self) -> usize {
/// bindings,
/// ffi::c_void,
/// io::{
-/// Io,
-/// IoRaw,
+/// IoFallible,
+/// IoInfallible,
+/// Mmio,
+/// MmioRaw,
/// PhysAddr,
/// },
/// };
/// use core::ops::Deref;
///
/// // See also [`pci::Bar`] for a real example.
-/// struct IoMem<const SIZE: usize>(IoRaw<SIZE>);
+/// struct IoMem<const SIZE: usize>(MmioRaw<SIZE>);
///
/// impl<const SIZE: usize> IoMem<SIZE> {
/// /// # Safety
@@ -103,7 +105,7 @@ pub fn maxsize(&self) -> usize {
/// return Err(ENOMEM);
/// }
///
-/// Ok(IoMem(IoRaw::new(addr as usize, SIZE)?))
+/// Ok(IoMem(MmioRaw::new(addr as usize, SIZE)?))
/// }
/// }
///
@@ -115,11 +117,11 @@ pub fn maxsize(&self) -> usize {
/// }
///
/// impl<const SIZE: usize> Deref for IoMem<SIZE> {
-/// type Target = Io<SIZE>;
+/// type Target = Mmio<SIZE>;
///
/// fn deref(&self) -> &Self::Target {
/// // SAFETY: The memory range stored in `self` has been properly mapped in `Self::new`.
-/// unsafe { Io::from_raw(&self.0) }
+/// unsafe { Mmio::from_raw(&self.0) }
/// }
/// }
///
@@ -133,29 +135,31 @@ pub fn maxsize(&self) -> usize {
/// # }
/// ```
#[repr(transparent)]
-pub struct Io<const SIZE: usize = 0>(IoRaw<SIZE>);
+pub struct Mmio<const SIZE: usize = 0>(MmioRaw<SIZE>);
macro_rules! define_read {
- ($(#[$attr:meta])* $name:ident, $try_name:ident, $c_fn:ident -> $type_name:ty) => {
+ (infallible, $(#[$attr:meta])* $vis:vis $name:ident, $c_fn:ident -> $type_name:ty) => {
/// Read IO data from a given offset known at compile time.
///
/// Bound checks are performed on compile time, hence if the offset is not known at compile
/// time, the build will fail.
$(#[$attr])*
#[inline]
- pub fn $name(&self, offset: usize) -> $type_name {
+ $vis fn $name(&self, offset: usize) -> $type_name {
let addr = self.io_addr_assert::<$type_name>(offset);
// SAFETY: By the type invariant `addr` is a valid address for MMIO operations.
unsafe { bindings::$c_fn(addr as *const c_void) }
}
+ };
+ (fallible, $(#[$attr:meta])* $vis:vis $try_name:ident, $c_fn:ident -> $type_name:ty) => {
/// Read IO data from a given offset.
///
/// Bound checks are performed on runtime, it fails if the offset (plus the type size) is
/// out of bounds.
$(#[$attr])*
- pub fn $try_name(&self, offset: usize) -> Result<$type_name> {
+ $vis fn $try_name(&self, offset: usize) -> Result<$type_name> {
let addr = self.io_addr::<$type_name>(offset)?;
// SAFETY: By the type invariant `addr` is a valid address for MMIO operations.
@@ -165,26 +169,28 @@ pub fn $try_name(&self, offset: usize) -> Result<$type_name> {
}
macro_rules! define_write {
- ($(#[$attr:meta])* $name:ident, $try_name:ident, $c_fn:ident <- $type_name:ty) => {
+ (infallible, $(#[$attr:meta])* $vis:vis $name:ident, $c_fn:ident <- $type_name:ty) => {
/// Write IO data from a given offset known at compile time.
///
/// Bound checks are performed on compile time, hence if the offset is not known at compile
/// time, the build will fail.
$(#[$attr])*
#[inline]
- pub fn $name(&self, value: $type_name, offset: usize) {
+ $vis fn $name(&self, value: $type_name, offset: usize) {
let addr = self.io_addr_assert::<$type_name>(offset);
// SAFETY: By the type invariant `addr` is a valid address for MMIO operations.
unsafe { bindings::$c_fn(value, addr as *mut c_void) }
}
+ };
+ (fallible, $(#[$attr:meta])* $vis:vis $try_name:ident, $c_fn:ident <- $type_name:ty) => {
/// Write IO data from a given offset.
///
/// Bound checks are performed on runtime, it fails if the offset (plus the type size) is
/// out of bounds.
$(#[$attr])*
- pub fn $try_name(&self, value: $type_name, offset: usize) -> Result {
+ $vis fn $try_name(&self, value: $type_name, offset: usize) -> Result {
let addr = self.io_addr::<$type_name>(offset)?;
// SAFETY: By the type invariant `addr` is a valid address for MMIO operations.
@@ -194,43 +200,38 @@ pub fn $try_name(&self, value: $type_name, offset: usize) -> Result {
};
}
-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;
/// 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);
}
@@ -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
}
+}
+
+/// Types implementing this trait (e.g. MMIO BARs or PCI config
+/// regions) can share the same Infallible accessors.
+pub trait IoInfallible: Io {
+ /// Infallible 8-bit read with compile-time bounds check.
+ fn read8(&self, offset: usize) -> u8;
+
+ /// Infallible 16-bit read with compile-time bounds check.
+ fn read16(&self, offset: usize) -> u16;
+
+ /// Infallible 32-bit read with compile-time bounds check.
+ fn read32(&self, offset: usize) -> u32;
+
+ /// 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);
+}
+
+/// Types implementing this trait (e.g. MMIO BARs or PCI config
+/// regions) can share the same Fallible accessors.
+pub trait IoFallible: Io {
+ /// Fallible 8-bit read with runtime bounds check.
+ fn try_read8(&self, offset: usize) -> Result<u8>;
+
+ /// Fallible 16-bit read with runtime bounds check.
+ fn try_read16(&self, offset: usize) -> Result<u16>;
+
+ /// Fallible 32-bit read with runtime bounds check.
+ fn try_read32(&self, offset: usize) -> Result<u32>;
+
+ /// 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;
+}
+
+/// Represents a region of I/O space of a fixed size with 64-bit accessors.
+///
+/// Provides common helpers for offset validation and address
+/// calculation on top of a base address and maximum size.
+///
+#[cfg(CONFIG_64BIT)]
+pub trait Io64: Io {}
- define_read!(read8, try_read8, readb -> u8);
- define_read!(read16, try_read16, readw -> u16);
- define_read!(read32, try_read32, readl -> u32);
+/// Types implementing this trait can share the same Infallible accessors.
+#[cfg(CONFIG_64BIT)]
+pub trait IoInfallible64: Io64 {
+ /// Infallible 64-bit read with compile-time bounds check.
+ fn read64(&self, offset: usize) -> u64;
+
+ /// Infallible 64-bit write with compile-time bounds check.
+ fn write64(&self, value: u64, offset: usize);
+}
+
+/// Types implementing this trait can share the same Infallible accessors.
+#[cfg(CONFIG_64BIT)]
+pub trait IoFallible64: Io64 {
+ /// Fallible 64-bit read with runtime bounds check.
+ fn try_read64(&self, offset: usize) -> Result<u64>;
+
+ /// Fallible 64-bit write with runtime bounds check.
+ fn try_write64(&self, value: u64, offset: usize) -> Result;
+}
+
+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()
+ }
+}
+
+impl<const SIZE: usize> IoInfallible for Mmio<SIZE> {
+ define_read!(infallible, read8, readb -> u8);
+ define_read!(infallible, read16, readw -> u16);
+ define_read!(infallible, read32, readl -> u32);
+
+ define_write!(infallible, write8, writeb <- u8);
+ define_write!(infallible, write16, writew <- u16);
+ define_write!(infallible, write32, writel <- u32);
+}
+
+impl<const SIZE: usize> IoFallible for Mmio<SIZE> {
+ define_read!(fallible, try_read8, readb -> u8);
+ define_read!(fallible, try_read16, readw -> u16);
+ define_read!(fallible, try_read32, readl -> u32);
+
+ define_write!(fallible, try_write8, writeb <- u8);
+ define_write!(fallible, try_write16, writew <- u16);
+ define_write!(fallible, try_write32, writel <- u32);
+}
+
+#[cfg(CONFIG_64BIT)]
+impl<const SIZE: usize> Io64 for Mmio<SIZE> {}
+
+#[cfg(CONFIG_64BIT)]
+impl<const SIZE: usize> IoInfallible64 for Mmio<SIZE> {
+ define_read!(infallible, read64, readq -> u64);
+
+ define_write!(infallible, write64, writeq <- u64);
+}
+
+#[cfg(CONFIG_64BIT)]
+impl<const SIZE: usize> IoFallible64 for Mmio<SIZE> {
+ define_read!(fallible, try_read64, readq -> u64);
+
+ define_write!(fallible, try_write64, writeq <- u64);
+}
+
+impl<const SIZE: usize> Mmio<SIZE> {
+ /// Converts an `MmioRaw` into an `Mmio` 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: &MmioRaw<SIZE>) -> &Self {
+ // SAFETY: `Mmio` is a transparent wrapper around `MmioRaw`.
+ unsafe { &*core::ptr::from_ref(raw).cast() }
+ }
+
+ define_read!(infallible, pub read8_relaxed, readb_relaxed -> u8);
+ define_read!(infallible, pub read16_relaxed, readw_relaxed -> u16);
+ define_read!(infallible, pub read32_relaxed, readl_relaxed -> u32);
define_read!(
+ infallible,
#[cfg(CONFIG_64BIT)]
- read64,
- try_read64,
- readq -> u64
+ pub read64_relaxed,
+ readq_relaxed -> u64
);
- define_read!(read8_relaxed, try_read8_relaxed, readb_relaxed -> u8);
- define_read!(read16_relaxed, try_read16_relaxed, readw_relaxed -> u16);
- define_read!(read32_relaxed, try_read32_relaxed, readl_relaxed -> u32);
+ define_read!(fallible, pub try_read8_relaxed, readb_relaxed -> u8);
+ define_read!(fallible, pub try_read16_relaxed, readw_relaxed -> u16);
+ define_read!(fallible, pub try_read32_relaxed, readl_relaxed -> u32);
define_read!(
+ fallible,
#[cfg(CONFIG_64BIT)]
- read64_relaxed,
- try_read64_relaxed,
+ pub try_read64_relaxed,
readq_relaxed -> u64
);
- define_write!(write8, try_write8, writeb <- u8);
- define_write!(write16, try_write16, writew <- u16);
- define_write!(write32, try_write32, writel <- u32);
+ define_write!(infallible, pub write8_relaxed, writeb_relaxed <- u8);
+ define_write!(infallible, pub write16_relaxed, writew_relaxed <- u16);
+ define_write!(infallible, pub write32_relaxed, writel_relaxed <- u32);
define_write!(
+ infallible,
#[cfg(CONFIG_64BIT)]
- write64,
- try_write64,
- writeq <- u64
+ pub write64_relaxed,
+ writeq_relaxed <- u64
);
- define_write!(write8_relaxed, try_write8_relaxed, writeb_relaxed <- u8);
- define_write!(write16_relaxed, try_write16_relaxed, writew_relaxed <- u16);
- define_write!(write32_relaxed, try_write32_relaxed, writel_relaxed <- u32);
+ define_write!(fallible, pub try_write8_relaxed, writeb_relaxed <- u8);
+ define_write!(fallible, pub try_write16_relaxed, writew_relaxed <- u16);
+ define_write!(fallible, pub try_write32_relaxed, writel_relaxed <- u32);
define_write!(
+ fallible,
#[cfg(CONFIG_64BIT)]
- write64_relaxed,
- try_write64_relaxed,
+ pub try_write64_relaxed,
writeq_relaxed <- u64
);
}
diff --git a/rust/kernel/io/mem.rs b/rust/kernel/io/mem.rs
index b03b82cd531b..5dcd7c901427 100644
--- a/rust/kernel/io/mem.rs
+++ b/rust/kernel/io/mem.rs
@@ -17,8 +17,8 @@
Region,
Resource, //
},
- Io,
- IoRaw, //
+ Mmio,
+ MmioRaw, //
},
prelude::*,
};
@@ -203,7 +203,7 @@ pub fn new<'a>(io_request: IoRequest<'a>) -> impl PinInit<Devres<Self>, Error> +
}
impl<const SIZE: usize> Deref for ExclusiveIoMem<SIZE> {
- type Target = Io<SIZE>;
+ type Target = Mmio<SIZE>;
fn deref(&self) -> &Self::Target {
&self.iomem
@@ -217,10 +217,10 @@ fn deref(&self) -> &Self::Target {
///
/// # Invariants
///
-/// [`IoMem`] always holds an [`IoRaw`] instance that holds a valid pointer to the
+/// [`IoMem`] always holds an [`MmioRaw`] instance that holds a valid pointer to the
/// start of the I/O memory mapped region.
pub struct IoMem<const SIZE: usize = 0> {
- io: IoRaw<SIZE>,
+ io: MmioRaw<SIZE>,
}
impl<const SIZE: usize> IoMem<SIZE> {
@@ -255,7 +255,7 @@ fn ioremap(resource: &Resource) -> Result<Self> {
return Err(ENOMEM);
}
- let io = IoRaw::new(addr as usize, size)?;
+ let io = MmioRaw::new(addr as usize, size)?;
let io = IoMem { io };
Ok(io)
@@ -278,10 +278,10 @@ fn drop(&mut self) {
}
impl<const SIZE: usize> Deref for IoMem<SIZE> {
- type Target = Io<SIZE>;
+ type Target = Mmio<SIZE>;
fn deref(&self) -> &Self::Target {
// SAFETY: Safe as by the invariant of `IoMem`.
- unsafe { Io::from_raw(&self.io) }
+ unsafe { Mmio::from_raw(&self.io) }
}
}
diff --git a/rust/kernel/io/poll.rs b/rust/kernel/io/poll.rs
index b1a2570364f4..543a4b7cea0d 100644
--- a/rust/kernel/io/poll.rs
+++ b/rust/kernel/io/poll.rs
@@ -45,12 +45,12 @@
/// # Examples
///
/// ```no_run
-/// use kernel::io::{Io, poll::read_poll_timeout};
+/// use kernel::io::{IoFallible, Mmio, poll::read_poll_timeout};
/// use kernel::time::Delta;
///
/// const HW_READY: u16 = 0x01;
///
-/// fn wait_for_hardware<const SIZE: usize>(io: &Io<SIZE>) -> Result {
+/// fn wait_for_hardware<const SIZE: usize>(io: &Mmio<SIZE>) -> Result {
/// read_poll_timeout(
/// // The `op` closure reads the value of a specific status register.
/// || io.try_read16(0x1000),
@@ -128,12 +128,12 @@ pub fn read_poll_timeout<Op, Cond, T>(
/// # Examples
///
/// ```no_run
-/// use kernel::io::{poll::read_poll_timeout_atomic, Io};
+/// use kernel::io::{poll::read_poll_timeout_atomic, IoFallible, Mmio};
/// use kernel::time::Delta;
///
/// const HW_READY: u16 = 0x01;
///
-/// fn wait_for_hardware<const SIZE: usize>(io: &Io<SIZE>) -> Result {
+/// fn wait_for_hardware<const SIZE: usize>(io: &Mmio<SIZE>) -> Result {
/// read_poll_timeout_atomic(
/// // The `op` closure reads the value of a specific status register.
/// || io.try_read16(0x1000),
diff --git a/rust/kernel/pci/io.rs b/rust/kernel/pci/io.rs
index 0d55c3139b6f..2bbb3261198d 100644
--- a/rust/kernel/pci/io.rs
+++ b/rust/kernel/pci/io.rs
@@ -8,8 +8,8 @@
device,
devres::Devres,
io::{
- Io,
- IoRaw, //
+ Mmio,
+ MmioRaw, //
},
prelude::*,
sync::aref::ARef, //
@@ -24,7 +24,7 @@
/// memory mapped PCI BAR and its size.
pub struct Bar<const SIZE: usize = 0> {
pdev: ARef<Device>,
- io: IoRaw<SIZE>,
+ io: MmioRaw<SIZE>,
num: i32,
}
@@ -60,7 +60,7 @@ pub(super) fn new(pdev: &Device, num: u32, name: &CStr) -> Result<Self> {
return Err(ENOMEM);
}
- let io = match IoRaw::new(ioptr, len as usize) {
+ let io = match MmioRaw::new(ioptr, len as usize) {
Ok(io) => io,
Err(err) => {
// SAFETY:
@@ -114,11 +114,11 @@ fn drop(&mut self) {
}
impl<const SIZE: usize> Deref for Bar<SIZE> {
- type Target = Io<SIZE>;
+ type Target = Mmio<SIZE>;
fn deref(&self) -> &Self::Target {
// SAFETY: By the type invariant of `Self`, the MMIO range in `self.io` is properly mapped.
- unsafe { Io::from_raw(&self.io) }
+ unsafe { Mmio::from_raw(&self.io) }
}
}
diff --git a/samples/rust/rust_driver_pci.rs b/samples/rust/rust_driver_pci.rs
index ee6248b8cda5..74b93ca7c338 100644
--- a/samples/rust/rust_driver_pci.rs
+++ b/samples/rust/rust_driver_pci.rs
@@ -8,6 +8,8 @@
c_str,
device::Core,
devres::Devres,
+ io::IoFallible,
+ io::IoInfallible,
pci,
prelude::*,
sync::aref::ARef, //
--
2.51.0On 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.
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.
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.
(And I still think you should rename IoFallible to Io and IoInfallible
to IoKnownSize.)
Alice
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.
On 11/25/25 6:46 AM, Alexandre Courbot wrote: > 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: ... >> 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. Great! That neatly fixes the naming problem that was bothering me about Io*, too. thanks, -- John Hubbard
On Wed, Nov 19, 2025 at 01:21:13PM +0200, 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 new {Io, Io64} traits, 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: Alice Ryhl <aliceryhl@google.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>
I said this on a previous version, but I still don't buy the split
into IoFallible and IoInfallible.
For one, we're never going to have a method that can accept any Io - we
will always want to accept either IoInfallible or IoFallible, so the
base Io trait serves no purpose.
For another, the docs explain that the distinction between them is
whether the bounds check is done at compile-time or runtime. That is not
the kind of capability one normally uses different traits to distinguish
between. It makes sense to have additional traits to distinguish
between e.g.:
* Whether IO ops can fail for reasons *other* than bounds checks.
* Whether 64-bit IO ops are possible.
Well ... I guess one could distinguish between whether it's possible to
check bounds at compile-time at all. But if you can check them at
compile-time, it should always be possible to check at runtime too, so
one should be a sub-trait of the other if you want to distinguish
them. (And then a trait name of KnownSizeIo would be more idiomatic.)
And I'm not really convinced that the current compile-time checked
traits are a good idea at all. See:
https://lore.kernel.org/all/DEEEZRYSYSS0.28PPK371D100F@nvidia.com/
If we want to have a compile-time checked trait, then the idiomatic way
to do that in Rust would be to have a new integer type that's guaranteed
to only contain integers <= the size. For example, the Bounded integer
being added elsewhere.
Alice
On Fri Nov 21, 2025 at 11:20 PM JST, Alice Ryhl wrote:
> On Wed, Nov 19, 2025 at 01:21:13PM +0200, 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 new {Io, Io64} traits, 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: Alice Ryhl <aliceryhl@google.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>
>
> I said this on a previous version, but I still don't buy the split
> into IoFallible and IoInfallible.
>
> For one, we're never going to have a method that can accept any Io - we
> will always want to accept either IoInfallible or IoFallible, so the
> base Io trait serves no purpose.
>
> For another, the docs explain that the distinction between them is
> whether the bounds check is done at compile-time or runtime. That is not
> the kind of capability one normally uses different traits to distinguish
> between. It makes sense to have additional traits to distinguish
> between e.g.:
>
> * Whether IO ops can fail for reasons *other* than bounds checks.
> * Whether 64-bit IO ops are possible.
>
> Well ... I guess one could distinguish between whether it's possible to
> check bounds at compile-time at all. But if you can check them at
> compile-time, it should always be possible to check at runtime too, so
> one should be a sub-trait of the other if you want to distinguish
> them. (And then a trait name of KnownSizeIo would be more idiomatic.)
>
> And I'm not really convinced that the current compile-time checked
> traits are a good idea at all. See:
> https://lore.kernel.org/all/DEEEZRYSYSS0.28PPK371D100F@nvidia.com/
>
> If we want to have a compile-time checked trait, then the idiomatic way
> to do that in Rust would be to have a new integer type that's guaranteed
> to only contain integers <= the size. For example, the Bounded integer
> being added elsewhere.
Would that be so different from using an associated const value though?
IIUC the bounded integer type would play the same role, only slightly
differently - by that I mean that if the offset is expressed by an
expression that is not const (such as an indexed access), then the
bounded integer still needs to rely on `build_assert` to be built.
On Tue, Nov 25, 2025 at 10:44:29PM +0900, Alexandre Courbot wrote:
> On Fri Nov 21, 2025 at 11:20 PM JST, Alice Ryhl wrote:
> > On Wed, Nov 19, 2025 at 01:21:13PM +0200, 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 new {Io, Io64} traits, 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: Alice Ryhl <aliceryhl@google.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>
> >
> > I said this on a previous version, but I still don't buy the split
> > into IoFallible and IoInfallible.
> >
> > For one, we're never going to have a method that can accept any Io - we
> > will always want to accept either IoInfallible or IoFallible, so the
> > base Io trait serves no purpose.
> >
> > For another, the docs explain that the distinction between them is
> > whether the bounds check is done at compile-time or runtime. That is not
> > the kind of capability one normally uses different traits to distinguish
> > between. It makes sense to have additional traits to distinguish
> > between e.g.:
> >
> > * Whether IO ops can fail for reasons *other* than bounds checks.
> > * Whether 64-bit IO ops are possible.
> >
> > Well ... I guess one could distinguish between whether it's possible to
> > check bounds at compile-time at all. But if you can check them at
> > compile-time, it should always be possible to check at runtime too, so
> > one should be a sub-trait of the other if you want to distinguish
> > them. (And then a trait name of KnownSizeIo would be more idiomatic.)
> >
> > And I'm not really convinced that the current compile-time checked
> > traits are a good idea at all. See:
> > https://lore.kernel.org/all/DEEEZRYSYSS0.28PPK371D100F@nvidia.com/
> >
> > If we want to have a compile-time checked trait, then the idiomatic way
> > to do that in Rust would be to have a new integer type that's guaranteed
> > to only contain integers <= the size. For example, the Bounded integer
> > being added elsewhere.
>
> Would that be so different from using an associated const value though?
> IIUC the bounded integer type would play the same role, only slightly
> differently - by that I mean that if the offset is expressed by an
> expression that is not const (such as an indexed access), then the
> bounded integer still needs to rely on `build_assert` to be built.
I mean something like this:
trait Io {
const SIZE: usize;
fn write(&mut self, i: Bounded<Self::SIZE>);
}
You know that Bounded<SIZE> contains a number less than SIZE, so you
know it's in-bounds without any build_assert required.
Yes, if there's a constructor for Bounded that utilizes build_assert,
then you end up with a build_assert to create it. But I think in many
cases it's avoidable depending on where the index comes from.
For example if you iterate all indices 0..SIZE, there could be a way to
directly create Bounded<SIZE> values from an iterator. Or if the index
comes from an ioctl, then you probably runtime check the integer at the
ioctl entrypoint, in which case you want the runtime-checked
constructor.
Alice
On Tue Nov 25, 2025 at 11:58 PM JST, Alice Ryhl wrote:
> On Tue, Nov 25, 2025 at 10:44:29PM +0900, Alexandre Courbot wrote:
>> On Fri Nov 21, 2025 at 11:20 PM JST, Alice Ryhl wrote:
>> > On Wed, Nov 19, 2025 at 01:21:13PM +0200, 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 new {Io, Io64} traits, 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: Alice Ryhl <aliceryhl@google.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>
>> >
>> > I said this on a previous version, but I still don't buy the split
>> > into IoFallible and IoInfallible.
>> >
>> > For one, we're never going to have a method that can accept any Io - we
>> > will always want to accept either IoInfallible or IoFallible, so the
>> > base Io trait serves no purpose.
>> >
>> > For another, the docs explain that the distinction between them is
>> > whether the bounds check is done at compile-time or runtime. That is not
>> > the kind of capability one normally uses different traits to distinguish
>> > between. It makes sense to have additional traits to distinguish
>> > between e.g.:
>> >
>> > * Whether IO ops can fail for reasons *other* than bounds checks.
>> > * Whether 64-bit IO ops are possible.
>> >
>> > Well ... I guess one could distinguish between whether it's possible to
>> > check bounds at compile-time at all. But if you can check them at
>> > compile-time, it should always be possible to check at runtime too, so
>> > one should be a sub-trait of the other if you want to distinguish
>> > them. (And then a trait name of KnownSizeIo would be more idiomatic.)
>> >
>> > And I'm not really convinced that the current compile-time checked
>> > traits are a good idea at all. See:
>> > https://lore.kernel.org/all/DEEEZRYSYSS0.28PPK371D100F@nvidia.com/
>> >
>> > If we want to have a compile-time checked trait, then the idiomatic way
>> > to do that in Rust would be to have a new integer type that's guaranteed
>> > to only contain integers <= the size. For example, the Bounded integer
>> > being added elsewhere.
>>
>> Would that be so different from using an associated const value though?
>> IIUC the bounded integer type would play the same role, only slightly
>> differently - by that I mean that if the offset is expressed by an
>> expression that is not const (such as an indexed access), then the
>> bounded integer still needs to rely on `build_assert` to be built.
>
> I mean something like this:
>
> trait Io {
> const SIZE: usize;
> fn write(&mut self, i: Bounded<Self::SIZE>);
> }
I have experimented a bit with this idea, and unfortunately expressing
`Bounded<Self::SIZE>` requires the generic_const_exprs feature and is
not doable as of today.
Bounding an integer with an upper/lower bound also proves to be more
demanding than the current `Bounded` design. For the `MIN` and `MAX`
constants must be of the same type as the wrapped `T` type, which again
makes rustc unhappy ("the type of const parameters must not depend on
other generic parameters"). A workaround would be to use a macro to
define individual types for each integer type we want to support - or to
just limit this to `usize`.
But the requirement for generic_const_exprs makes this a non-starter I'm
afraid. :/
On Wed, Nov 26, 2025 at 04:52:05PM +0900, Alexandre Courbot wrote:
> On Tue Nov 25, 2025 at 11:58 PM JST, Alice Ryhl wrote:
> > On Tue, Nov 25, 2025 at 10:44:29PM +0900, Alexandre Courbot wrote:
> >> On Fri Nov 21, 2025 at 11:20 PM JST, Alice Ryhl wrote:
> >> > On Wed, Nov 19, 2025 at 01:21:13PM +0200, 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 new {Io, Io64} traits, 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: Alice Ryhl <aliceryhl@google.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>
> >> >
> >> > I said this on a previous version, but I still don't buy the split
> >> > into IoFallible and IoInfallible.
> >> >
> >> > For one, we're never going to have a method that can accept any Io - we
> >> > will always want to accept either IoInfallible or IoFallible, so the
> >> > base Io trait serves no purpose.
> >> >
> >> > For another, the docs explain that the distinction between them is
> >> > whether the bounds check is done at compile-time or runtime. That is not
> >> > the kind of capability one normally uses different traits to distinguish
> >> > between. It makes sense to have additional traits to distinguish
> >> > between e.g.:
> >> >
> >> > * Whether IO ops can fail for reasons *other* than bounds checks.
> >> > * Whether 64-bit IO ops are possible.
> >> >
> >> > Well ... I guess one could distinguish between whether it's possible to
> >> > check bounds at compile-time at all. But if you can check them at
> >> > compile-time, it should always be possible to check at runtime too, so
> >> > one should be a sub-trait of the other if you want to distinguish
> >> > them. (And then a trait name of KnownSizeIo would be more idiomatic.)
> >> >
> >> > And I'm not really convinced that the current compile-time checked
> >> > traits are a good idea at all. See:
> >> > https://lore.kernel.org/all/DEEEZRYSYSS0.28PPK371D100F@nvidia.com/
> >> >
> >> > If we want to have a compile-time checked trait, then the idiomatic way
> >> > to do that in Rust would be to have a new integer type that's guaranteed
> >> > to only contain integers <= the size. For example, the Bounded integer
> >> > being added elsewhere.
> >>
> >> Would that be so different from using an associated const value though?
> >> IIUC the bounded integer type would play the same role, only slightly
> >> differently - by that I mean that if the offset is expressed by an
> >> expression that is not const (such as an indexed access), then the
> >> bounded integer still needs to rely on `build_assert` to be built.
> >
> > I mean something like this:
> >
> > trait Io {
> > const SIZE: usize;
> > fn write(&mut self, i: Bounded<Self::SIZE>);
> > }
>
> I have experimented a bit with this idea, and unfortunately expressing
> `Bounded<Self::SIZE>` requires the generic_const_exprs feature and is
> not doable as of today.
>
> Bounding an integer with an upper/lower bound also proves to be more
> demanding than the current `Bounded` design. For the `MIN` and `MAX`
> constants must be of the same type as the wrapped `T` type, which again
> makes rustc unhappy ("the type of const parameters must not depend on
> other generic parameters"). A workaround would be to use a macro to
> define individual types for each integer type we want to support - or to
> just limit this to `usize`.
>
> But the requirement for generic_const_exprs makes this a non-starter I'm
> afraid. :/
Can you try this?
trait Io {
type IdxInt: Int;
fn write(&mut self, i: Self::IdxInt);
}
then implementers would write:
impl Io for MyIo {
type IdxInt = Bounded<17>;
}
instead of:
impl Io for MyIo {
const SIZE = 17;
}
Alice
On Wed Nov 26, 2025 at 6:50 PM JST, Alice Ryhl wrote:
> On Wed, Nov 26, 2025 at 04:52:05PM +0900, Alexandre Courbot wrote:
>> On Tue Nov 25, 2025 at 11:58 PM JST, Alice Ryhl wrote:
>> > On Tue, Nov 25, 2025 at 10:44:29PM +0900, Alexandre Courbot wrote:
>> >> On Fri Nov 21, 2025 at 11:20 PM JST, Alice Ryhl wrote:
>> >> > On Wed, Nov 19, 2025 at 01:21:13PM +0200, 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 new {Io, Io64} traits, 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: Alice Ryhl <aliceryhl@google.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>
>> >> >
>> >> > I said this on a previous version, but I still don't buy the split
>> >> > into IoFallible and IoInfallible.
>> >> >
>> >> > For one, we're never going to have a method that can accept any Io - we
>> >> > will always want to accept either IoInfallible or IoFallible, so the
>> >> > base Io trait serves no purpose.
>> >> >
>> >> > For another, the docs explain that the distinction between them is
>> >> > whether the bounds check is done at compile-time or runtime. That is not
>> >> > the kind of capability one normally uses different traits to distinguish
>> >> > between. It makes sense to have additional traits to distinguish
>> >> > between e.g.:
>> >> >
>> >> > * Whether IO ops can fail for reasons *other* than bounds checks.
>> >> > * Whether 64-bit IO ops are possible.
>> >> >
>> >> > Well ... I guess one could distinguish between whether it's possible to
>> >> > check bounds at compile-time at all. But if you can check them at
>> >> > compile-time, it should always be possible to check at runtime too, so
>> >> > one should be a sub-trait of the other if you want to distinguish
>> >> > them. (And then a trait name of KnownSizeIo would be more idiomatic.)
>> >> >
>> >> > And I'm not really convinced that the current compile-time checked
>> >> > traits are a good idea at all. See:
>> >> > https://lore.kernel.org/all/DEEEZRYSYSS0.28PPK371D100F@nvidia.com/
>> >> >
>> >> > If we want to have a compile-time checked trait, then the idiomatic way
>> >> > to do that in Rust would be to have a new integer type that's guaranteed
>> >> > to only contain integers <= the size. For example, the Bounded integer
>> >> > being added elsewhere.
>> >>
>> >> Would that be so different from using an associated const value though?
>> >> IIUC the bounded integer type would play the same role, only slightly
>> >> differently - by that I mean that if the offset is expressed by an
>> >> expression that is not const (such as an indexed access), then the
>> >> bounded integer still needs to rely on `build_assert` to be built.
>> >
>> > I mean something like this:
>> >
>> > trait Io {
>> > const SIZE: usize;
>> > fn write(&mut self, i: Bounded<Self::SIZE>);
>> > }
>>
>> I have experimented a bit with this idea, and unfortunately expressing
>> `Bounded<Self::SIZE>` requires the generic_const_exprs feature and is
>> not doable as of today.
>>
>> Bounding an integer with an upper/lower bound also proves to be more
>> demanding than the current `Bounded` design. For the `MIN` and `MAX`
>> constants must be of the same type as the wrapped `T` type, which again
>> makes rustc unhappy ("the type of const parameters must not depend on
>> other generic parameters"). A workaround would be to use a macro to
>> define individual types for each integer type we want to support - or to
>> just limit this to `usize`.
>>
>> But the requirement for generic_const_exprs makes this a non-starter I'm
>> afraid. :/
>
> Can you try this?
>
> trait Io {
> type IdxInt: Int;
> fn write(&mut self, i: Self::IdxInt);
> }
>
> then implementers would write:
>
> impl Io for MyIo {
> type IdxInt = Bounded<17>;
> }
>
> instead of:
> impl Io for MyIo {
> const SIZE = 17;
> }
The following builds (using the existing `Bounded` type for
demonstration purposes):
trait Io {
// Type containing an index guaranteed to be valid for this IO.
type IdxInt: Into<usize>;
fn write(&mut self, i: Self::IdxInt);
}
struct FooIo;
impl Io for FooIo {
type IdxInt = Bounded<usize, 8>;
fn write(&mut self, i: Self::IdxInt) {
let idx: usize = i.into();
// Now do the IO knowing that `idx` is a valid index.
}
}
That looks promising, and I like how we can effectively use a wider set
of index types - even, say, a `u16` if a particular I/O happens to have
a guaranteed size of 65536!
I suspect it also changes how we would design the Io interfaces, but I
am not sure how yet. Maybe `IoKnownSize` being built on top of `Io`, and
either unwrapping the result of its fallible methods or using some
`unchecked` accessors?
On Wed Nov 26, 2025 at 10:37 PM JST, Alexandre Courbot wrote:
> On Wed Nov 26, 2025 at 6:50 PM JST, Alice Ryhl wrote:
>> On Wed, Nov 26, 2025 at 04:52:05PM +0900, Alexandre Courbot wrote:
>>> On Tue Nov 25, 2025 at 11:58 PM JST, Alice Ryhl wrote:
>>> > On Tue, Nov 25, 2025 at 10:44:29PM +0900, Alexandre Courbot wrote:
>>> >> On Fri Nov 21, 2025 at 11:20 PM JST, Alice Ryhl wrote:
>>> >> > On Wed, Nov 19, 2025 at 01:21:13PM +0200, 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 new {Io, Io64} traits, 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: Alice Ryhl <aliceryhl@google.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>
>>> >> >
>>> >> > I said this on a previous version, but I still don't buy the split
>>> >> > into IoFallible and IoInfallible.
>>> >> >
>>> >> > For one, we're never going to have a method that can accept any Io - we
>>> >> > will always want to accept either IoInfallible or IoFallible, so the
>>> >> > base Io trait serves no purpose.
>>> >> >
>>> >> > For another, the docs explain that the distinction between them is
>>> >> > whether the bounds check is done at compile-time or runtime. That is not
>>> >> > the kind of capability one normally uses different traits to distinguish
>>> >> > between. It makes sense to have additional traits to distinguish
>>> >> > between e.g.:
>>> >> >
>>> >> > * Whether IO ops can fail for reasons *other* than bounds checks.
>>> >> > * Whether 64-bit IO ops are possible.
>>> >> >
>>> >> > Well ... I guess one could distinguish between whether it's possible to
>>> >> > check bounds at compile-time at all. But if you can check them at
>>> >> > compile-time, it should always be possible to check at runtime too, so
>>> >> > one should be a sub-trait of the other if you want to distinguish
>>> >> > them. (And then a trait name of KnownSizeIo would be more idiomatic.)
>>> >> >
>>> >> > And I'm not really convinced that the current compile-time checked
>>> >> > traits are a good idea at all. See:
>>> >> > https://lore.kernel.org/all/DEEEZRYSYSS0.28PPK371D100F@nvidia.com/
>>> >> >
>>> >> > If we want to have a compile-time checked trait, then the idiomatic way
>>> >> > to do that in Rust would be to have a new integer type that's guaranteed
>>> >> > to only contain integers <= the size. For example, the Bounded integer
>>> >> > being added elsewhere.
>>> >>
>>> >> Would that be so different from using an associated const value though?
>>> >> IIUC the bounded integer type would play the same role, only slightly
>>> >> differently - by that I mean that if the offset is expressed by an
>>> >> expression that is not const (such as an indexed access), then the
>>> >> bounded integer still needs to rely on `build_assert` to be built.
>>> >
>>> > I mean something like this:
>>> >
>>> > trait Io {
>>> > const SIZE: usize;
>>> > fn write(&mut self, i: Bounded<Self::SIZE>);
>>> > }
>>>
>>> I have experimented a bit with this idea, and unfortunately expressing
>>> `Bounded<Self::SIZE>` requires the generic_const_exprs feature and is
>>> not doable as of today.
>>>
>>> Bounding an integer with an upper/lower bound also proves to be more
>>> demanding than the current `Bounded` design. For the `MIN` and `MAX`
>>> constants must be of the same type as the wrapped `T` type, which again
>>> makes rustc unhappy ("the type of const parameters must not depend on
>>> other generic parameters"). A workaround would be to use a macro to
>>> define individual types for each integer type we want to support - or to
>>> just limit this to `usize`.
>>>
>>> But the requirement for generic_const_exprs makes this a non-starter I'm
>>> afraid. :/
>>
>> Can you try this?
>>
>> trait Io {
>> type IdxInt: Int;
>> fn write(&mut self, i: Self::IdxInt);
>> }
>>
>> then implementers would write:
>>
>> impl Io for MyIo {
>> type IdxInt = Bounded<17>;
>> }
>>
>> instead of:
>> impl Io for MyIo {
>> const SIZE = 17;
>> }
>
> The following builds (using the existing `Bounded` type for
> demonstration purposes):
>
> trait Io {
> // Type containing an index guaranteed to be valid for this IO.
> type IdxInt: Into<usize>;
>
> fn write(&mut self, i: Self::IdxInt);
> }
>
> struct FooIo;
>
> impl Io for FooIo {
> type IdxInt = Bounded<usize, 8>;
>
> fn write(&mut self, i: Self::IdxInt) {
> let idx: usize = i.into();
>
> // Now do the IO knowing that `idx` is a valid index.
> }
> }
>
> That looks promising, and I like how we can effectively use a wider set
> of index types - even, say, a `u16` if a particular I/O happens to have
> a guaranteed size of 65536!
>
> I suspect it also changes how we would design the Io interfaces, but I
> am not sure how yet. Maybe `IoKnownSize` being built on top of `Io`, and
> either unwrapping the result of its fallible methods or using some
> `unchecked` accessors?
Mmm, one important point I have neglected is that the index type will
have to validate not only the range, but also the alignment of the
index! And the valid alignment is dependent on the access width. So
getting this right is going to take some time and some experimenting I'm
afraid.
Meanwhile, it would be great if we could agree (and proceed) with the
split of the I/O interface into a trait, as other work depends on it.
Changing the index type of compile-time checked bounds is I think an
improvement that is orthogonal to this task.
I have been thinking a bit (too much? ^_^;) about the general design for
this interface, how it would work with the `register!` macro, and how we
could maybe limit the boilerplate. Sorry in advance for this is going to
be a long post.
IIUC there are several aspects we need to tackle with the I/O interface:
- Raw IO access. Given an address, perform the IO operation without any
check. Depending on the bus, this might return the data directly (e.g.
`Mmio`), or a `Result` (e.g. the PCI `ConfigSpace`). The current
implementation ignores the bus error, which we probably shouldn't.
Also the raw access is reimplemented twice, in both the build-time and
runtime accessors, a fact that is mostly hidden by the use of macros.
- Access with dynamic bounds check. This can return `EINVAL` if the
provided index is invalid (out-of-bounds or not aligned), on top of
the bus errors, if any.
- Access with build-time index check. Same as above, but the error
occurs at build time if the index is invalid. Otherwise the return
type of the raw IO accessor is returned.
At the moment we have two traits for build-time and runtime index
checks. What bothers me a bit about them is that they basically
re-implement the same raw I/O accessors. This strongly hints that we
should implement the raw accessors as a base trait, which the
user-facing API would call into:
pub trait Io {
/// Error type returned by IO accessors. Can be `Infallible` for e.g. `Mmio`.
type Error: Into<Error>;
/// Returns the base address of this mapping.
fn addr(&self) -> usize;
/// Returns the maximum size of this mapping.
fn maxsize(&self) -> usize;
unsafe fn try_read8_unchecked(&self, addr: usize) -> Result<u8, Self::Error>;
unsafe fn try_write8_unchecked(&self, value: u8, addr: usize) -> Result<(), Self::Error>;
// etc. for 16/32 bits accessors.
}
Then we could build the current `IoFallible` trait on top of it:
pub trait IoFallible: Io {
fn io_addr<U>(&self, offset: usize) -> Result<usize> {
if !offset_valid::<U>(offset, self.maxsize()) {
return Err(EINVAL);
}
self.addr().checked_add(offset).ok_or(EINVAL)
}
/// 8-bit read with runtime bounds check.
fn try_read8_checked(&self, offset: usize) -> Result<u8> {
let addr = self.io_addr::<u8>(offset)?;
unsafe { self.try_read8_unchecked(addr) }.map_err(Into::into)
}
/// 8-bit write with runtime bounds check.
fn try_write8_checked(&self, value: u8, offset: usize) -> Result {
let addr = self.io_addr::<u8>(offset)?;
unsafe { self.try_write8_unchecked(value, addr) }.map_err(Into::into)
}
}
Note how this trait is now auto-implemented. Making it available for all
implementers of `Io` is as simple as:
impl<IO: Io> IoFallible for IO {}
(... which hints that maybe it should simply be folded into `Io`, as
Alice previously suggested)
`IoKnownSize` also calls into the base `Io` trait:
/// Trait for IO with a build-time known valid range.
pub unsafe trait IoKnownSize: Io {
/// Minimum usable size of this region.
const MIN_SIZE: usize;
#[inline(always)]
fn io_addr_assert<U>(&self, offset: usize) -> usize {
build_assert!(offset_valid::<U>(offset, Self::MIN_SIZE));
self.addr() + offset
}
/// 8-bit read with compile-time bounds check.
#[inline(always)]
fn try_read8(&self, offset: usize) -> Result<u8, Self::Error> {
let addr = self.io_addr_assert::<u8>(offset);
unsafe { self.try_read8_unchecked(addr) }
}
/// 8-bit write with compile-time bounds check.
#[inline(always)]
fn try_write8(&self, value: u8, offset: usize) -> Result<(), Self::Error> {
let addr = self.io_addr_assert::<u8>(offset);
unsafe { self.try_write8_unchecked(value, addr) }
}
}
Its accessors now return the error type of the bus, which is good for
safety, but not for ergonomics when dealing with e.g. code that works
with `Mmio`, which we know is infallible. But we can provide an extra
set of methods in this trait for this case:
/// Infallible 8-bit read with compile-time bounds check.
#[inline(always)]
fn read8(&self, offset: usize) -> u8
where
Self: Io<Error = Infallible>,
{
self.read8(offset).unwrap_or_else(|e| match e {})
}
/// Infallible 8-bit write with compile-time bounds check.
#[inline(always)]
fn write8(&self, value: u8, offset: usize)
where
Self: Io<Error = Infallible>,
{
self.write8(value, offset).unwrap_or_else(|e| match e {})
}
`Mmio`'s impl blocks are now reduced to the following:
impl<const SIZE: usize> Io for Mmio<SIZE> {
type Error = core::convert::Infallible;
#[inline]
fn addr(&self) -> usize {
self.0.addr()
}
#[inline]
fn maxsize(&self) -> usize {
self.0.maxsize()
}
unsafe fn try_read8_unchecked(&self, addr: usize) -> Result<u8, Self::Error> {
Ok(unsafe { bindings::readb(addr as *const c_void) })
}
unsafe fn try_write8_unchecked(&self, value: u8, addr: usize) -> Result<(), Self::Error> {
Ok(unsafe { bindings::writeb(value, addr as *mut c_void) })
}
}
unsafe impl<const SIZE: usize> IoKnownSize for Mmio<SIZE> {
const MIN_SIZE: usize = SIZE;
}
... and that's enough to provide everything we had so far - all of the
accessors called by users are already implemented in the base traits.
Note also the lack of macros.
Another point that I noticed was the relaxed MMIO accessors. They are
currently implemented as a set of dedicated methods (e.g.
`read8_relaxed`) that are not part of a trait. This results in a lot of
additional methods, and limits their usefulness as the same generic
function could not be used with both regular and relaxed accesses.
So I'd propose to implement them using a relaxed wrapper type:
/// Wrapper for [`Mmio`] that performs relaxed I/O accesses.
pub struct RelaxedMmio<'a, const SIZE: usize>(&'a Mmio<SIZE>);
impl<'a, const SIZE: usize> RelaxedMmio<'a, SIZE> {
pub fn new(mmio: &'a Mmio<SIZE>) -> Self {
Self(mmio)
}
}
impl<'a, const SIZE: usize> Io for RelaxedMmio<'a, SIZE> {
fn addr(&self) -> usize {
self.0.addr()
}
fn maxsize(&self) -> usize {
self.0.maxsize()
}
type Error = <Mmio as Io>::Error;
unsafe fn try_read8_unchecked(&self, addr: usize) -> Result<u8, Self::Error> {
Ok(unsafe { bindings::readb_relaxed(addr as *const c_void) })
}
unsafe fn try_write8_unchecked(&self, value: u8, addr: usize) -> Result<(), Self::Error> {
Ok(unsafe { bindings::writeb_relaxed(value, addr as *mut c_void) })
}
// SAFETY: `MIN_SIZE` is the same as the wrapped type, which also implements `IoKnownSize`.
unsafe impl<'a, const SIZE: usize> IoKnownSize for RelaxedMmio<'a, SIZE> {
const MIN_SIZE: usize = SIZE;
}
}
This way, when you need to do I/O using a register, you can either pass
the `Mmio` instance or derive a `RelaxedMmio` from it, if that access
pattern is more adequate.
How does this sound? I can share a branch with a basic implementation
of this idea if that helps.
On Mon, Dec 01, 2025 at 08:57:09PM +0900, Alexandre Courbot wrote:
> On Wed Nov 26, 2025 at 10:37 PM JST, Alexandre Courbot wrote:
> > On Wed Nov 26, 2025 at 6:50 PM JST, Alice Ryhl wrote:
> >> On Wed, Nov 26, 2025 at 04:52:05PM +0900, Alexandre Courbot wrote:
> >>> On Tue Nov 25, 2025 at 11:58 PM JST, Alice Ryhl wrote:
> >>> > On Tue, Nov 25, 2025 at 10:44:29PM +0900, Alexandre Courbot wrote:
> >>> >> On Fri Nov 21, 2025 at 11:20 PM JST, Alice Ryhl wrote:
> >>> >> > On Wed, Nov 19, 2025 at 01:21:13PM +0200, 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 new {Io, Io64} traits, 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: Alice Ryhl <aliceryhl@google.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>
> >>> >> >
> >>> >> > I said this on a previous version, but I still don't buy the split
> >>> >> > into IoFallible and IoInfallible.
> >>> >> >
> >>> >> > For one, we're never going to have a method that can accept any Io - we
> >>> >> > will always want to accept either IoInfallible or IoFallible, so the
> >>> >> > base Io trait serves no purpose.
> >>> >> >
> >>> >> > For another, the docs explain that the distinction between them is
> >>> >> > whether the bounds check is done at compile-time or runtime. That is not
> >>> >> > the kind of capability one normally uses different traits to distinguish
> >>> >> > between. It makes sense to have additional traits to distinguish
> >>> >> > between e.g.:
> >>> >> >
> >>> >> > * Whether IO ops can fail for reasons *other* than bounds checks.
> >>> >> > * Whether 64-bit IO ops are possible.
> >>> >> >
> >>> >> > Well ... I guess one could distinguish between whether it's possible to
> >>> >> > check bounds at compile-time at all. But if you can check them at
> >>> >> > compile-time, it should always be possible to check at runtime too, so
> >>> >> > one should be a sub-trait of the other if you want to distinguish
> >>> >> > them. (And then a trait name of KnownSizeIo would be more idiomatic.)
> >>> >> >
> >>> >> > And I'm not really convinced that the current compile-time checked
> >>> >> > traits are a good idea at all. See:
> >>> >> > https://lore.kernel.org/all/DEEEZRYSYSS0.28PPK371D100F@nvidia.com/
> >>> >> >
> >>> >> > If we want to have a compile-time checked trait, then the idiomatic way
> >>> >> > to do that in Rust would be to have a new integer type that's guaranteed
> >>> >> > to only contain integers <= the size. For example, the Bounded integer
> >>> >> > being added elsewhere.
> >>> >>
> >>> >> Would that be so different from using an associated const value though?
> >>> >> IIUC the bounded integer type would play the same role, only slightly
> >>> >> differently - by that I mean that if the offset is expressed by an
> >>> >> expression that is not const (such as an indexed access), then the
> >>> >> bounded integer still needs to rely on `build_assert` to be built.
> >>> >
> >>> > I mean something like this:
> >>> >
> >>> > trait Io {
> >>> > const SIZE: usize;
> >>> > fn write(&mut self, i: Bounded<Self::SIZE>);
> >>> > }
> >>>
> >>> I have experimented a bit with this idea, and unfortunately expressing
> >>> `Bounded<Self::SIZE>` requires the generic_const_exprs feature and is
> >>> not doable as of today.
> >>>
> >>> Bounding an integer with an upper/lower bound also proves to be more
> >>> demanding than the current `Bounded` design. For the `MIN` and `MAX`
> >>> constants must be of the same type as the wrapped `T` type, which again
> >>> makes rustc unhappy ("the type of const parameters must not depend on
> >>> other generic parameters"). A workaround would be to use a macro to
> >>> define individual types for each integer type we want to support - or to
> >>> just limit this to `usize`.
> >>>
> >>> But the requirement for generic_const_exprs makes this a non-starter I'm
> >>> afraid. :/
> >>
> >> Can you try this?
> >>
> >> trait Io {
> >> type IdxInt: Int;
> >> fn write(&mut self, i: Self::IdxInt);
> >> }
> >>
> >> then implementers would write:
> >>
> >> impl Io for MyIo {
> >> type IdxInt = Bounded<17>;
> >> }
> >>
> >> instead of:
> >> impl Io for MyIo {
> >> const SIZE = 17;
> >> }
> >
> > The following builds (using the existing `Bounded` type for
> > demonstration purposes):
> >
> > trait Io {
> > // Type containing an index guaranteed to be valid for this IO.
> > type IdxInt: Into<usize>;
> >
> > fn write(&mut self, i: Self::IdxInt);
> > }
> >
> > struct FooIo;
> >
> > impl Io for FooIo {
> > type IdxInt = Bounded<usize, 8>;
> >
> > fn write(&mut self, i: Self::IdxInt) {
> > let idx: usize = i.into();
> >
> > // Now do the IO knowing that `idx` is a valid index.
> > }
> > }
> >
> > That looks promising, and I like how we can effectively use a wider set
> > of index types - even, say, a `u16` if a particular I/O happens to have
> > a guaranteed size of 65536!
> >
> > I suspect it also changes how we would design the Io interfaces, but I
> > am not sure how yet. Maybe `IoKnownSize` being built on top of `Io`, and
> > either unwrapping the result of its fallible methods or using some
> > `unchecked` accessors?
>
> Mmm, one important point I have neglected is that the index type will
> have to validate not only the range, but also the alignment of the
> index! And the valid alignment is dependent on the access width. So
> getting this right is going to take some time and some experimenting I'm
> afraid.
>
> Meanwhile, it would be great if we could agree (and proceed) with the
> split of the I/O interface into a trait, as other work depends on it.
> Changing the index type of compile-time checked bounds is I think an
> improvement that is orthogonal to this task.
> I have been thinking a bit (too much? ^_^;) about the general design for
> this interface, how it would work with the `register!` macro, and how we
> could maybe limit the boilerplate. Sorry in advance for this is going to
> be a long post.
>
> IIUC there are several aspects we need to tackle with the I/O interface:
>
> - Raw IO access. Given an address, perform the IO operation without any
> check. Depending on the bus, this might return the data directly (e.g.
> `Mmio`), or a `Result` (e.g. the PCI `ConfigSpace`). The current
> implementation ignores the bus error, which we probably shouldn't.
> Also the raw access is reimplemented twice, in both the build-time and
> runtime accessors, a fact that is mostly hidden by the use of macros.
> - Access with dynamic bounds check. This can return `EINVAL` if the
> provided index is invalid (out-of-bounds or not aligned), on top of
> the bus errors, if any.
> - Access with build-time index check. Same as above, but the error
> occurs at build time if the index is invalid. Otherwise the return
> type of the raw IO accessor is returned.
>
> At the moment we have two traits for build-time and runtime index
> checks. What bothers me a bit about them is that they basically
> re-implement the same raw I/O accessors. This strongly hints that we
> should implement the raw accessors as a base trait, which the
> user-facing API would call into:
>
> pub trait Io {
> /// Error type returned by IO accessors. Can be `Infallible` for e.g. `Mmio`.
> type Error: Into<Error>;
>
> /// Returns the base address of this mapping.
> fn addr(&self) -> usize;
>
> /// Returns the maximum size of this mapping.
> fn maxsize(&self) -> usize;
>
> unsafe fn try_read8_unchecked(&self, addr: usize) -> Result<u8, Self::Error>;
> unsafe fn try_write8_unchecked(&self, value: u8, addr: usize) -> Result<(), Self::Error>;
> // etc. for 16/32 bits accessors.
> }
>
> Then we could build the current `IoFallible` trait on top of it:
>
> pub trait IoFallible: Io {
> fn io_addr<U>(&self, offset: usize) -> Result<usize> {
> if !offset_valid::<U>(offset, self.maxsize()) {
> return Err(EINVAL);
> }
>
> self.addr().checked_add(offset).ok_or(EINVAL)
> }
>
> /// 8-bit read with runtime bounds check.
> fn try_read8_checked(&self, offset: usize) -> Result<u8> {
> let addr = self.io_addr::<u8>(offset)?;
>
> unsafe { self.try_read8_unchecked(addr) }.map_err(Into::into)
> }
>
> /// 8-bit write with runtime bounds check.
> fn try_write8_checked(&self, value: u8, offset: usize) -> Result {
> let addr = self.io_addr::<u8>(offset)?;
>
> unsafe { self.try_write8_unchecked(value, addr) }.map_err(Into::into)
> }
> }
>
> Note how this trait is now auto-implemented. Making it available for all
> implementers of `Io` is as simple as:
>
> impl<IO: Io> IoFallible for IO {}
>
> (... which hints that maybe it should simply be folded into `Io`, as
> Alice previously suggested)
Yes, it probably should. At the very least, it should be an extension
trait, which means that it should never be used in trait bounds, since
T: IoFallible is equivalent to T: Io. But in this case, probably just
fold it into Io.
> `IoKnownSize` also calls into the base `Io` trait:
>
> /// Trait for IO with a build-time known valid range.
> pub unsafe trait IoKnownSize: Io {
> /// Minimum usable size of this region.
> const MIN_SIZE: usize;
>
> #[inline(always)]
> fn io_addr_assert<U>(&self, offset: usize) -> usize {
> build_assert!(offset_valid::<U>(offset, Self::MIN_SIZE));
>
> self.addr() + offset
> }
>
> /// 8-bit read with compile-time bounds check.
> #[inline(always)]
> fn try_read8(&self, offset: usize) -> Result<u8, Self::Error> {
> let addr = self.io_addr_assert::<u8>(offset);
>
> unsafe { self.try_read8_unchecked(addr) }
> }
>
> /// 8-bit write with compile-time bounds check.
> #[inline(always)]
> fn try_write8(&self, value: u8, offset: usize) -> Result<(), Self::Error> {
> let addr = self.io_addr_assert::<u8>(offset);
>
> unsafe { self.try_write8_unchecked(value, addr) }
> }
> }
>
> Its accessors now return the error type of the bus, which is good for
> safety, but not for ergonomics when dealing with e.g. code that works
> with `Mmio`, which we know is infallible. But we can provide an extra
> set of methods in this trait for this case:
>
> /// Infallible 8-bit read with compile-time bounds check.
> #[inline(always)]
> fn read8(&self, offset: usize) -> u8
> where
> Self: Io<Error = Infallible>,
> {
> self.read8(offset).unwrap_or_else(|e| match e {})
> }
>
> /// Infallible 8-bit write with compile-time bounds check.
> #[inline(always)]
> fn write8(&self, value: u8, offset: usize)
> where
> Self: Io<Error = Infallible>,
> {
> self.write8(value, offset).unwrap_or_else(|e| match e {})
> }
>
> `Mmio`'s impl blocks are now reduced to the following:
>
> impl<const SIZE: usize> Io for Mmio<SIZE> {
> type Error = core::convert::Infallible;
>
> #[inline]
> fn addr(&self) -> usize {
> self.0.addr()
> }
>
> #[inline]
> fn maxsize(&self) -> usize {
> self.0.maxsize()
> }
>
> unsafe fn try_read8_unchecked(&self, addr: usize) -> Result<u8, Self::Error> {
> Ok(unsafe { bindings::readb(addr as *const c_void) })
> }
>
> unsafe fn try_write8_unchecked(&self, value: u8, addr: usize) -> Result<(), Self::Error> {
> Ok(unsafe { bindings::writeb(value, addr as *mut c_void) })
> }
> }
>
> unsafe impl<const SIZE: usize> IoKnownSize for Mmio<SIZE> {
> const MIN_SIZE: usize = SIZE;
> }
>
> ... and that's enough to provide everything we had so far - all of the
> accessors called by users are already implemented in the base traits.
> Note also the lack of macros.
>
> Another point that I noticed was the relaxed MMIO accessors. They are
> currently implemented as a set of dedicated methods (e.g.
> `read8_relaxed`) that are not part of a trait. This results in a lot of
> additional methods, and limits their usefulness as the same generic
> function could not be used with both regular and relaxed accesses.
>
> So I'd propose to implement them using a relaxed wrapper type:
>
> /// Wrapper for [`Mmio`] that performs relaxed I/O accesses.
> pub struct RelaxedMmio<'a, const SIZE: usize>(&'a Mmio<SIZE>);
>
> impl<'a, const SIZE: usize> RelaxedMmio<'a, SIZE> {
> pub fn new(mmio: &'a Mmio<SIZE>) -> Self {
> Self(mmio)
> }
> }
>
> impl<'a, const SIZE: usize> Io for RelaxedMmio<'a, SIZE> {
> fn addr(&self) -> usize {
> self.0.addr()
> }
>
> fn maxsize(&self) -> usize {
> self.0.maxsize()
> }
>
> type Error = <Mmio as Io>::Error;
>
> unsafe fn try_read8_unchecked(&self, addr: usize) -> Result<u8, Self::Error> {
> Ok(unsafe { bindings::readb_relaxed(addr as *const c_void) })
> }
>
> unsafe fn try_write8_unchecked(&self, value: u8, addr: usize) -> Result<(), Self::Error> {
> Ok(unsafe { bindings::writeb_relaxed(value, addr as *mut c_void) })
> }
>
> // SAFETY: `MIN_SIZE` is the same as the wrapped type, which also implements `IoKnownSize`.
> unsafe impl<'a, const SIZE: usize> IoKnownSize for RelaxedMmio<'a, SIZE> {
> const MIN_SIZE: usize = SIZE;
> }
> }
>
> This way, when you need to do I/O using a register, you can either pass
> the `Mmio` instance or derive a `RelaxedMmio` from it, if that access
> pattern is more adequate.
This sounds like a reasonable way to handle relaxed mmio.
> How does this sound? I can share a branch with a basic implementation
> of this idea if that helps.
My main thoughts are:
First, we need to think some more about the naming. Currently you have
several methods with the same name. For example, you have a read8 method
implemented in terms of a different read8 method. It'd be nice with a
summary of the meaning of:
* try_ prefix
* _unchecked suffix
* _checked suffix (not the same as try_ prefix?)
Second, I think we need to think a bit more about the error types.
Perhaps the trait could define:
/// Error type used by `*_unchecked` methods.
type Error;
/// Error type that can be either `Self::Error` or a failed bounds
/// check.
type TryError: From<Self::Error> + From<BoundsError>;
where BoundsError is a new zero-sized error type we can define
somewhere. This way, Mmio can use these errors:
type Error = Infallible;
type TryError = BoundsError;
wheres cases that can fail with an IO error can use kernel::error::Error
for both cases.
Third, if we're going to postpone the custom integer type for
IoKnownSize, then I think we should get rid of build-checked IO ops
entirely for now.
Fourth, I didn't know about the alignment requirement. I would like to
know how that fits in with the rest of this. Is it treated like a bounds
check? That could make sense, and we could also have a custom integer
type that both has a max value and alignment invariant. But what about
the *_unchecked and runtime bounds-checked methods?
Alice
On Wed Nov 26, 2025 at 10:37 PM JST, Alexandre Courbot wrote:
> On Wed Nov 26, 2025 at 6:50 PM JST, Alice Ryhl wrote:
>> On Wed, Nov 26, 2025 at 04:52:05PM +0900, Alexandre Courbot wrote:
>>> On Tue Nov 25, 2025 at 11:58 PM JST, Alice Ryhl wrote:
>>> > On Tue, Nov 25, 2025 at 10:44:29PM +0900, Alexandre Courbot wrote:
>>> >> On Fri Nov 21, 2025 at 11:20 PM JST, Alice Ryhl wrote:
>>> >> > On Wed, Nov 19, 2025 at 01:21:13PM +0200, 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 new {Io, Io64} traits, 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: Alice Ryhl <aliceryhl@google.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>
>>> >> >
>>> >> > I said this on a previous version, but I still don't buy the split
>>> >> > into IoFallible and IoInfallible.
>>> >> >
>>> >> > For one, we're never going to have a method that can accept any Io - we
>>> >> > will always want to accept either IoInfallible or IoFallible, so the
>>> >> > base Io trait serves no purpose.
>>> >> >
>>> >> > For another, the docs explain that the distinction between them is
>>> >> > whether the bounds check is done at compile-time or runtime. That is not
>>> >> > the kind of capability one normally uses different traits to distinguish
>>> >> > between. It makes sense to have additional traits to distinguish
>>> >> > between e.g.:
>>> >> >
>>> >> > * Whether IO ops can fail for reasons *other* than bounds checks.
>>> >> > * Whether 64-bit IO ops are possible.
>>> >> >
>>> >> > Well ... I guess one could distinguish between whether it's possible to
>>> >> > check bounds at compile-time at all. But if you can check them at
>>> >> > compile-time, it should always be possible to check at runtime too, so
>>> >> > one should be a sub-trait of the other if you want to distinguish
>>> >> > them. (And then a trait name of KnownSizeIo would be more idiomatic.)
>>> >> >
>>> >> > And I'm not really convinced that the current compile-time checked
>>> >> > traits are a good idea at all. See:
>>> >> > https://lore.kernel.org/all/DEEEZRYSYSS0.28PPK371D100F@nvidia.com/
>>> >> >
>>> >> > If we want to have a compile-time checked trait, then the idiomatic way
>>> >> > to do that in Rust would be to have a new integer type that's guaranteed
>>> >> > to only contain integers <= the size. For example, the Bounded integer
>>> >> > being added elsewhere.
>>> >>
>>> >> Would that be so different from using an associated const value though?
>>> >> IIUC the bounded integer type would play the same role, only slightly
>>> >> differently - by that I mean that if the offset is expressed by an
>>> >> expression that is not const (such as an indexed access), then the
>>> >> bounded integer still needs to rely on `build_assert` to be built.
>>> >
>>> > I mean something like this:
>>> >
>>> > trait Io {
>>> > const SIZE: usize;
>>> > fn write(&mut self, i: Bounded<Self::SIZE>);
>>> > }
>>>
>>> I have experimented a bit with this idea, and unfortunately expressing
>>> `Bounded<Self::SIZE>` requires the generic_const_exprs feature and is
>>> not doable as of today.
>>>
>>> Bounding an integer with an upper/lower bound also proves to be more
>>> demanding than the current `Bounded` design. For the `MIN` and `MAX`
>>> constants must be of the same type as the wrapped `T` type, which again
>>> makes rustc unhappy ("the type of const parameters must not depend on
>>> other generic parameters"). A workaround would be to use a macro to
>>> define individual types for each integer type we want to support - or to
>>> just limit this to `usize`.
>>>
>>> But the requirement for generic_const_exprs makes this a non-starter I'm
>>> afraid. :/
>>
>> Can you try this?
>>
>> trait Io {
>> type IdxInt: Int;
>> fn write(&mut self, i: Self::IdxInt);
>> }
>>
>> then implementers would write:
>>
>> impl Io for MyIo {
>> type IdxInt = Bounded<17>;
>> }
>>
>> instead of:
>> impl Io for MyIo {
>> const SIZE = 17;
>> }
>
> The following builds (using the existing `Bounded` type for
> demonstration purposes):
>
> trait Io {
> // Type containing an index guaranteed to be valid for this IO.
> type IdxInt: Into<usize>;
>
> fn write(&mut self, i: Self::IdxInt);
> }
>
> struct FooIo;
>
> impl Io for FooIo {
> type IdxInt = Bounded<usize, 8>;
>
> fn write(&mut self, i: Self::IdxInt) {
> let idx: usize = i.into();
>
> // Now do the IO knowing that `idx` is a valid index.
> }
> }
>
> That looks promising, and I like how we can effectively use a wider set
> of index types - even, say, a `u16` if a particular I/O happens to have
> a guaranteed size of 65536!
>
> I suspect it also changes how we would design the Io interfaces, but I
> am not sure how yet. Maybe `IoKnownSize` being built on top of `Io`, and
> either unwrapping the result of its fallible methods or using some
> `unchecked` accessors?
That last sentence was ambiguous - for it to make sense, please rename
`Io` to `IoKnownSize` in the code sample above.
On Tue Nov 25, 2025 at 11:58 PM JST, Alice Ryhl wrote:
> On Tue, Nov 25, 2025 at 10:44:29PM +0900, Alexandre Courbot wrote:
>> On Fri Nov 21, 2025 at 11:20 PM JST, Alice Ryhl wrote:
>> > On Wed, Nov 19, 2025 at 01:21:13PM +0200, 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 new {Io, Io64} traits, 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: Alice Ryhl <aliceryhl@google.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>
>> >
>> > I said this on a previous version, but I still don't buy the split
>> > into IoFallible and IoInfallible.
>> >
>> > For one, we're never going to have a method that can accept any Io - we
>> > will always want to accept either IoInfallible or IoFallible, so the
>> > base Io trait serves no purpose.
>> >
>> > For another, the docs explain that the distinction between them is
>> > whether the bounds check is done at compile-time or runtime. That is not
>> > the kind of capability one normally uses different traits to distinguish
>> > between. It makes sense to have additional traits to distinguish
>> > between e.g.:
>> >
>> > * Whether IO ops can fail for reasons *other* than bounds checks.
>> > * Whether 64-bit IO ops are possible.
>> >
>> > Well ... I guess one could distinguish between whether it's possible to
>> > check bounds at compile-time at all. But if you can check them at
>> > compile-time, it should always be possible to check at runtime too, so
>> > one should be a sub-trait of the other if you want to distinguish
>> > them. (And then a trait name of KnownSizeIo would be more idiomatic.)
>> >
>> > And I'm not really convinced that the current compile-time checked
>> > traits are a good idea at all. See:
>> > https://lore.kernel.org/all/DEEEZRYSYSS0.28PPK371D100F@nvidia.com/
>> >
>> > If we want to have a compile-time checked trait, then the idiomatic way
>> > to do that in Rust would be to have a new integer type that's guaranteed
>> > to only contain integers <= the size. For example, the Bounded integer
>> > being added elsewhere.
>>
>> Would that be so different from using an associated const value though?
>> IIUC the bounded integer type would play the same role, only slightly
>> differently - by that I mean that if the offset is expressed by an
>> expression that is not const (such as an indexed access), then the
>> bounded integer still needs to rely on `build_assert` to be built.
>
> I mean something like this:
>
> trait Io {
> const SIZE: usize;
> fn write(&mut self, i: Bounded<Self::SIZE>);
> }
>
> You know that Bounded<SIZE> contains a number less than SIZE, so you
> know it's in-bounds without any build_assert required.
>
> Yes, if there's a constructor for Bounded that utilizes build_assert,
> then you end up with a build_assert to create it. But I think in many
> cases it's avoidable depending on where the index comes from.
>
> For example if you iterate all indices 0..SIZE, there could be a way to
> directly create Bounded<SIZE> values from an iterator. Or if the index
> comes from an ioctl, then you probably runtime check the integer at the
> ioctl entrypoint, in which case you want the runtime-checked
> constructor.
Thanks for elaborating. I really like this idea.
You are right that is would drastically reduce the number of times we
need to rely on `build_assert`, as well as concentrating its use to a
single point (bounded int constructor) instead of having to sprinkle
extra invocations in the Io module.
Now I would also like to also keep the ability to define "integers which
only X LSBs represent the value", so I guess we could have a distinct
type for "integers within a lower and higher bound", since the latter
requires two generic constants vs one for the former.
Maybe we could derive the current `Bounded` and that new type from a
more generic "constrained integer" type, which constraint rule itself is
given as a generic argument? From this we could then easily build all
sort of funny things. That could turn into quite an undertaking though.
On Fri, 21 Nov 2025 14:20:13 +0000
Alice Ryhl <aliceryhl@google.com> wrote:
> On Wed, Nov 19, 2025 at 01:21:13PM +0200, 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 new {Io, Io64} traits, 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: Alice Ryhl <aliceryhl@google.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>
>
> I said this on a previous version, but I still don't buy the split
> into IoFallible and IoInfallible.
>
> For one, we're never going to have a method that can accept any Io -
> we will always want to accept either IoInfallible or IoFallible, so
> the base Io trait serves no purpose.
>
> For another, the docs explain that the distinction between them is
> whether the bounds check is done at compile-time or runtime. That is
> not the kind of capability one normally uses different traits to
> distinguish between. It makes sense to have additional traits to
> distinguish between e.g.:
>
> * Whether IO ops can fail for reasons *other* than bounds checks.
> * Whether 64-bit IO ops are possible.
>
> Well ... I guess one could distinguish between whether it's possible
> to check bounds at compile-time at all. But if you can check them at
> compile-time, it should always be possible to check at runtime too, so
> one should be a sub-trait of the other if you want to distinguish
> them. (And then a trait name of KnownSizeIo would be more idiomatic.)
>
Thanks a lot for the detailed feedback. Agree with the points. Let's
keep the IoFallible and IoInfallible traits but not just tie them to the
bound checks in the docs.
> And I'm not really convinced that the current compile-time checked
> traits are a good idea at all. See:
> https://lore.kernel.org/all/DEEEZRYSYSS0.28PPK371D100F@nvidia.com/
>
> If we want to have a compile-time checked trait, then the idiomatic
> way to do that in Rust would be to have a new integer type that's
> guaranteed to only contain integers <= the size. For example, the
> Bounded integer being added elsewhere.
>
Oops, this is a interesting bug. :) I think we can apply the bound
integer to IoFallible and IoInfallible to avoid possible problems
mentioned above. E.g. constructing a Bounded interger when constructing
Mmio and ConfigSpace objects and use them in the boundary checks in the
trait methods.
I saw Alex had already had an implementation of Bounded integer [1] in
rust-next. While my patchset is against driver-core-testing
branch. Would it be OK that we move on without it and switch to Bounded
integer when it is landed to driver-core-testing? I am open to
suggestions. :)
Z.
[1]
https://lore.kernel.org/all/CANiq72nV1zwoCCcHuizdfqWF=e8hvd6RO1CBXTEt73eqe4ayaA@mail.gmail.com/
> Alice
On Mon, Nov 24, 2025 at 12:08:46PM +0200, Zhi Wang wrote:
> On Fri, 21 Nov 2025 14:20:13 +0000
> Alice Ryhl <aliceryhl@google.com> wrote:
>
> > On Wed, Nov 19, 2025 at 01:21:13PM +0200, 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 new {Io, Io64} traits, 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: Alice Ryhl <aliceryhl@google.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>
> >
> > I said this on a previous version, but I still don't buy the split
> > into IoFallible and IoInfallible.
> >
> > For one, we're never going to have a method that can accept any Io -
> > we will always want to accept either IoInfallible or IoFallible, so
> > the base Io trait serves no purpose.
> >
> > For another, the docs explain that the distinction between them is
> > whether the bounds check is done at compile-time or runtime. That is
> > not the kind of capability one normally uses different traits to
> > distinguish between. It makes sense to have additional traits to
> > distinguish between e.g.:
> >
> > * Whether IO ops can fail for reasons *other* than bounds checks.
> > * Whether 64-bit IO ops are possible.
> >
> > Well ... I guess one could distinguish between whether it's possible
> > to check bounds at compile-time at all. But if you can check them at
> > compile-time, it should always be possible to check at runtime too, so
> > one should be a sub-trait of the other if you want to distinguish
> > them. (And then a trait name of KnownSizeIo would be more idiomatic.)
> >
>
> Thanks a lot for the detailed feedback. Agree with the points. Let's
> keep the IoFallible and IoInfallible traits but not just tie them to the
> bound checks in the docs.
What do you plan to write in the docs instead?
> > And I'm not really convinced that the current compile-time checked
> > traits are a good idea at all. See:
> > https://lore.kernel.org/all/DEEEZRYSYSS0.28PPK371D100F@nvidia.com/
> >
> > If we want to have a compile-time checked trait, then the idiomatic
> > way to do that in Rust would be to have a new integer type that's
> > guaranteed to only contain integers <= the size. For example, the
> > Bounded integer being added elsewhere.
> >
>
> Oops, this is a interesting bug. :) I think we can apply the bound
> integer to IoFallible and IoInfallible to avoid possible problems
> mentioned above. E.g. constructing a Bounded interger when constructing
> Mmio and ConfigSpace objects and use them in the boundary checks in the
> trait methods.
>
> I saw Alex had already had an implementation of Bounded integer [1] in
> rust-next. While my patchset is against driver-core-testing
> branch. Would it be OK that we move on without it and switch to Bounded
> integer when it is landed to driver-core-testing? I am open to
> suggestions. :)
The last -rc of this cycle is already out, so I don't think you need to
worry about branch issues - you won't land it in time for that.
But there is another problem: Bounded only supports the case where the
bound is a power of two, so I don't think it's usable here. You can have
Io regions whose size is not a power of two.
Alice
On Mon, 24 Nov 2025 10:20:41 +0000
Alice Ryhl <aliceryhl@google.com> wrote:
> On Mon, Nov 24, 2025 at 12:08:46PM +0200, Zhi Wang wrote:
> > On Fri, 21 Nov 2025 14:20:13 +0000
> > Alice Ryhl <aliceryhl@google.com> wrote:
> >
> > > On Wed, Nov 19, 2025 at 01:21:13PM +0200, 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 new {Io, Io64} traits, 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: Alice Ryhl <aliceryhl@google.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>
> > >
> > > I said this on a previous version, but I still don't buy the split
> > > into IoFallible and IoInfallible.
> > >
> > > For one, we're never going to have a method that can accept any
> > > Io - we will always want to accept either IoInfallible or
> > > IoFallible, so the base Io trait serves no purpose.
> > >
> > > For another, the docs explain that the distinction between them is
> > > whether the bounds check is done at compile-time or runtime. That
> > > is not the kind of capability one normally uses different traits
> > > to distinguish between. It makes sense to have additional traits
> > > to distinguish between e.g.:
> > >
> > > * Whether IO ops can fail for reasons *other* than bounds checks.
> > > * Whether 64-bit IO ops are possible.
> > >
> > > Well ... I guess one could distinguish between whether it's
> > > possible to check bounds at compile-time at all. But if you can
> > > check them at compile-time, it should always be possible to check
> > > at runtime too, so one should be a sub-trait of the other if you
> > > want to distinguish them. (And then a trait name of KnownSizeIo
> > > would be more idiomatic.)
> > >
> >
> > Thanks a lot for the detailed feedback. Agree with the points. Let's
> > keep the IoFallible and IoInfallible traits but not just tie them
> > to the bound checks in the docs.
>
> What do you plan to write in the docs instead?
>
What I understad according to the discussion:
1. Infallible vs Fallible:
- Infallible indicates the I/O operation can will not return error from
the API level, and doesn't guarentee the hardware status from device
level.
- Fallible indicates the I/O operation can return error from the
API level.
2. compiling-time check vs run-time check:
- driver specifies a known-valid-size I/O region, we go compiling-time
check (saves the cost of run-time check).
- driver is not able to specifiy a known-valid-size I/O region, we
should go run-time check.
For IoInfallible, I would write the doc as:
A trait for I/O accessors that are guaranteed to succeed at the API
level.
Implementations of this trait provide I/O operations that do
not return errors to the caller. From the perspective of the I/O
API, the I/O operation is always considered successful.
Note that this does *not* mean that the underlying device is guaranteed
to be in a healthy state. Hardware-specific exceptional states must be
detected and handled by the driver or subsystem managing the device.
For Iofallible,
A trait for I/O accessors that may return an error.
This trait represents I/O operations where the API can intentionally
return an error. The error typically reflects issues detected by the
subsystem.
> > > And I'm not really convinced that the current compile-time checked
> > > traits are a good idea at all. See:
> > > https://lore.kernel.org/all/DEEEZRYSYSS0.28PPK371D100F@nvidia.com/
snip
> The last -rc of this cycle is already out, so I don't think you need
> to worry about branch issues - you won't land it in time for that.
>
I am mostly refering to the dependances if I have to implement this on
top of bounded integer on driver-core-testing.
>
> But there is another problem: Bounded only supports the case where the
> bound is a power of two, so I don't think it's usable here. You can
> have Io regions whose size is not a power of two.
Any suggestion on this? :) Should I implement something like
BoundedOffset? Also would like to hear some inputs from Danilo as well.
Z.
>
> Alice
On Mon Nov 24, 2025 at 10:32 PM JST, Zhi Wang wrote: <snip> >> But there is another problem: Bounded only supports the case where the >> bound is a power of two, so I don't think it's usable here. You can >> have Io regions whose size is not a power of two. > > Any suggestion on this? :) Should I implement something like > BoundedOffset? Also would like to hear some inputs from Danilo as well. Bounded integers were written with bitfields in mind and that is reflected in the current implementation, but provided one can devise a way to check for upper and lower bounds that is simple and works for both signed and unsigned ints, there should be little standing in the way of making it more granular and accepting any valid range as the bounds constraint. If this proves useful here and we can keep things simple for the bitfield use-case, then making them more flexible is definitely an option! :)
© 2016 - 2025 Red Hat, Inc.