[PATCH v6 RESEND 0/7] rust: pci: add config space read/write support

[PATCH v6 RESEND 0/7] rust: pci: add config space read/write support

[Zhi Wang]

In the NVIDIA vGPU RFC [1], the PCI configuration space access is
required in nova-core for preparing gspVFInfo when vGPU support is
enabled. This series is the following up of the discussion with Danilo
for how to introduce support of PCI configuration space access in Rust
PCI abstractions.

This patch series is implemented based on kernel vertical fixes patches
[2][3].

v6:

- Implement config_space() and config_space_extended() in device::Bound
  lifecycle. (Danilo)
- Fix the "use" in the comment for generating proper rust docs, verify
  the output of rustdoc. (Miguel)
- Improve the comments of PCI configuration space when checking the
  output of rustdoc.

v5:

- Remove fallible accessors of PCI configuration space. (Danilo)
- Add #[repr(usize)] for enum ConfigSpace. (Danilo)
- Refine the handling of return value in read accessors. (Danilo)
- Add debug_assert!() in pdev::cfg_size(). (Danilo)
- Add ConfigSpace.as_raw() for extracting the raw value. (Danilo)
- Rebase the patches on top of driver-core-testing branch.
- Convert imports touched by this series to vertical style.

v4:

- Refactor the SIZE constant to be an associated constant. (Alice)
- Remove the default method implementations in the Io trait. (Alice)
- Make cfg_size() private. (Danilo/Bjorn)
- Implement the infallible accessors of ConfigSpace. (Danilo)
- Create a new Io64 trait specifically for 64-bit accessors. (Danilo)
- Provide two separate methods for driver: config_space() and
  config_space_extended(). (Danilo)
- Update the sample driver to test the infallible accessors. (Danilo)

v3:

- Turn offset_valid() into a private function of kernel::io:Io. (Alex)
- Separate try and non-try variants. (Danilo)
- Move all the {try_}{read,write}{8,16,32,64} accessors to the I/O trait.
  (Danilo)
- Replace the hardcoded MMIO type constraint with a generic trait bound
  so that register! macro can be used in other places. (Danilo)
- Fix doctest. (John)
- Add an enum for PCI configuration space size. (Danilo)
- Refine the patch comments. (Bjorn)

v2:

- Factor out common trait as 'Io' and keep the rest routines in original
  'Io' as 'Mmio'. (Danilo)
- Rename 'IoRaw' to 'MmioRaw'. Update the bus MMIO implementation to use
  'MmioRaw'.
- Introduce pci::Device<Bound>::config_space(). (Danilo)
- Implement both infallible and fallible read/write routines, the device
  driver decicdes which version should be used.

This ideas of this series are:

- Factor out common traits for other accessors to share the same
  compiling/runtime check like before.

- Factor the MMIO read/write macros from the define_read! and
  define_write! macros. Thus, define_{read, write}! can be used in other
  backends.

  In detail:

  * Introduce `call_mmio_read!` and `call_mmio_write!` helper macros
    to encapsulate the unsafe FFI calls.
  * Update `define_read!` and `define_write!` macros to delegate to
    the call macros.
  * Export `define_read` and `define_write` so they can be reused
    for other I/O backends (e.g. PCI config space).

- Implement the PCI configuration space access backend in PCI
  abstractions.

- Add tests for config space routines in rust PCI sample driver.

[1] https://lore.kernel.org/all/20250903221111.3866249-1-zhiw@nvidia.com/
[2] https://lore.kernel.org/all/20251104133301.59402-1-dakr@kernel.org/
[3] https://lore.kernel.org/all/20251105120352.77603-1-dakr@kernel.org/

Zhi Wang (7):
  samples: rust: rust_driver_pci: use "kernel vertical" style for
    imports
  rust: devres: style for imports
  rust: io: style for imports
  rust: io: factor common I/O helpers into Io trait
  rust: io: factor out MMIO read/write macros
  rust: pci: add config space read/write support
  sample: rust: pci: add tests for config space routines

 drivers/gpu/nova-core/regs/macros.rs |  90 +++++---
 drivers/gpu/nova-core/vbios.rs       |   1 +
 rust/kernel/devres.rs                |  64 ++++--
 rust/kernel/io.rs                    | 323 ++++++++++++++++++++-------
 rust/kernel/io/mem.rs                |  16 +-
 rust/kernel/io/poll.rs               |   8 +-
 rust/kernel/pci.rs                   |  41 +++-
 rust/kernel/pci/io.rs                |  85 ++++++-
 samples/rust/rust_driver_pci.rs      |  38 +++-
 9 files changed, 519 insertions(+), 147 deletions(-)

-- 
2.51.0

[PATCH v6 RESEND 1/7] samples: rust: rust_driver_pci: use "kernel vertical" style for imports

[Zhi Wang]

Convert all imports in the rust_driver_pci to use "kernel vertical" style.

Signed-off-by: Zhi Wang <zhiw@nvidia.com>
---
 samples/rust/rust_driver_pci.rs | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/samples/rust/rust_driver_pci.rs b/samples/rust/rust_driver_pci.rs
index 5823787bea8e..ee6248b8cda5 100644
--- a/samples/rust/rust_driver_pci.rs
+++ b/samples/rust/rust_driver_pci.rs
@@ -4,7 +4,14 @@
 //!
 //! To make this driver probe, QEMU must be run with `-device pci-testdev`.
 
-use kernel::{c_str, device::Core, devres::Devres, pci, prelude::*, sync::aref::ARef};
+use kernel::{
+    c_str,
+    device::Core,
+    devres::Devres,
+    pci,
+    prelude::*,
+    sync::aref::ARef, //
+};
 
 struct Regs;
 
-- 
2.51.0

[PATCH v6 RESEND 2/7] rust: devres: style for imports

[Zhi Wang]

Convert all imports in the devres to use "kernel vertical" style. Drop
unnecessary imports covered by prelude::*.

Cc: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>
Signed-off-by: Zhi Wang <zhiw@nvidia.com>
---
 rust/kernel/devres.rs | 54 +++++++++++++++++++++++++++++++++++--------
 1 file changed, 45 insertions(+), 9 deletions(-)

diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
index 10a6a1789854..3376c7090ccd 100644
--- a/rust/kernel/devres.rs
+++ b/rust/kernel/devres.rs
@@ -8,13 +8,28 @@
 use crate::{
     alloc::Flags,
     bindings,
-    device::{Bound, Device},
-    error::{to_result, Error, Result},
-    ffi::c_void,
+    device::{
+        Bound,
+        Device, //
+    },
+    error::{
+        to_result, //
+    },
     prelude::*,
-    revocable::{Revocable, RevocableGuard},
-    sync::{aref::ARef, rcu, Completion},
-    types::{ForeignOwnable, Opaque, ScopeGuard},
+    revocable::{
+        Revocable,
+        RevocableGuard, //
+    },
+    sync::{
+        aref::ARef,
+        rcu,
+        Completion, //
+    },
+    types::{
+        ForeignOwnable,
+        Opaque,
+        ScopeGuard, //
+    },
 };
 
 use pin_init::Wrapper;
@@ -52,7 +67,18 @@ struct Inner<T: Send> {
 /// # Examples
 ///
 /// ```no_run
-/// # use kernel::{bindings, device::{Bound, Device}, devres::Devres, io::{Io, IoRaw}};
+/// # use kernel::{
+/// #   bindings,
+/// #   device::{
+/// #       Bound,
+/// #       Device, //
+/// #   },
+/// #   devres::Devres,
+/// #   io::{
+/// #       Io,
+/// #       IoRaw, //
+/// #   }, //
+/// # };
 /// # use core::ops::Deref;
 ///
 /// // See also [`pci::Bar`] for a real example.
@@ -230,7 +256,11 @@ pub fn device(&self) -> &Device {
     ///
     /// ```no_run
     /// # #![cfg(CONFIG_PCI)]
-    /// # use kernel::{device::Core, devres::Devres, pci};
+    /// # use kernel::{
+    /// #   device::Core,
+    /// #   devres::Devres,
+    /// #   pci, //
+    /// # };
     ///
     /// fn from_core(dev: &pci::Device<Core>, devres: Devres<pci::Bar<0x4>>) -> Result {
     ///     let bar = devres.access(dev.as_ref())?;
@@ -333,7 +363,13 @@ fn register_foreign<P>(dev: &Device<Bound>, data: P) -> Result
 /// # Examples
 ///
 /// ```no_run
-/// use kernel::{device::{Bound, Device}, devres};
+/// use kernel::{
+///     device::{
+///         Bound,
+///         Device, //
+///     },
+///     devres, //
+/// };
 ///
 /// /// Registration of e.g. a class device, IRQ, etc.
 /// struct Registration;
-- 
2.51.0

[PATCH v6 RESEND 3/7] rust: io: style for imports

[Zhi Wang]

Convert the imports in the commit in io to use "kernel vertical" style.

Signed-off-by: Zhi Wang <zhiw@nvidia.com>
---
 rust/kernel/io.rs | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
index 1aa9495f7774..5af04c3b807b 100644
--- a/rust/kernel/io.rs
+++ b/rust/kernel/io.rs
@@ -64,7 +64,14 @@ pub fn maxsize(&self) -> usize {
 /// # Examples
 ///
 /// ```no_run
-/// # use kernel::{bindings, ffi::c_void, io::{Io, IoRaw}};
+/// # use kernel::{
+/// #   bindings,
+/// #   ffi::c_void,
+/// #   io::{
+/// #       Io,
+/// #       IoRaw, //
+/// #   }, //
+/// # };
 /// # use core::ops::Deref;
 ///
 /// // See also [`pci::Bar`] for a real example.
-- 
2.51.0

[PATCH v6 RESEND 4/7] rust: io: factor common I/O helpers into Io trait

[Zhi Wang]

The previous Io<SIZE> type combined both the generic I/O access helpers
and MMIO implementation details in a single struct.

To establish a cleaner layering between the I/O interface and its concrete
backends, paving the way for supporting additional I/O mechanisms in the
future, Io<SIZE> need to be factored.

Factor the common helpers into a new Io trait, and move the MMIO-specific
logic into a dedicated Mmio<SIZE> type implementing that trait. Rename the
IoRaw to MmioRaw and update the bus MMIO implementations to use MmioRaw.

No functional change intended.

Cc: Alexandre Courbot <acourbot@nvidia.com>
Cc: Bjorn Helgaas <helgaas@kernel.org>
Cc: Danilo Krummrich <dakr@kernel.org>
Cc: John Hubbard <jhubbard@nvidia.com>
Signed-off-by: Zhi Wang <zhiw@nvidia.com>
---
 drivers/gpu/nova-core/regs/macros.rs |  90 +++++----
 drivers/gpu/nova-core/vbios.rs       |   1 +
 rust/kernel/devres.rs                |  14 +-
 rust/kernel/io.rs                    | 264 ++++++++++++++++++++-------
 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, 277 insertions(+), 130 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 3376c7090ccd..1dc7cb30d9f2 100644
--- a/rust/kernel/devres.rs
+++ b/rust/kernel/devres.rs
@@ -75,14 +75,15 @@ struct Inner<T: Send> {
 /// #   },
 /// #   devres::Devres,
 /// #   io::{
-/// #       Io,
-/// #       IoRaw, //
+/// #       IoInfallible,
+/// #       Mmio,
+/// #       MmioRaw, //
 /// #   }, //
 /// # };
 /// # 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
@@ -97,7 +98,7 @@ struct Inner<T: Send> {
 ///             return Err(ENOMEM);
 ///         }
 ///
-///         Ok(IoMem(IoRaw::new(addr as usize, SIZE)?))
+///         Ok(IoMem(MmioRaw::new(addr as usize, SIZE)?))
 ///     }
 /// }
 ///
@@ -109,11 +110,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> {
@@ -259,6 +260,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 5af04c3b807b..4d98d431b523 100644
--- a/rust/kernel/io.rs
+++ b/rust/kernel/io.rs
@@ -20,16 +20,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);
@@ -68,14 +68,16 @@ pub fn maxsize(&self) -> usize {
 /// #   bindings,
 /// #   ffi::c_void,
 /// #   io::{
-/// #       Io,
-/// #       IoRaw, //
+/// #       IoFallible,
+/// #       IoInfallible,
+/// #       Mmio,
+/// #       MmioRaw, //
 /// #   }, //
 /// # };
 /// # 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
@@ -90,7 +92,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)?))
 ///     }
 /// }
 ///
@@ -102,11 +104,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) }
 ///    }
 /// }
 ///
@@ -120,29 +122,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.
@@ -152,26 +156,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.
@@ -181,43 +187,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);
         }
 
@@ -226,50 +227,173 @@ 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;
+}
+
+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);
+}
+
+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!(read8, try_read8, readb -> u8);
-    define_read!(read16, try_read16, readw -> u16);
-    define_read!(read32, try_read32, readl -> u32);
     define_read!(
+        infallible,
         #[cfg(CONFIG_64BIT)]
-        read64,
-        try_read64,
+        pub read64,
         readq -> 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_write!(
+        infallible,
+        #[cfg(CONFIG_64BIT)]
+        pub write64,
+        writeq <- u64
+    );
+
     define_read!(
+        fallible,
         #[cfg(CONFIG_64BIT)]
-        read64_relaxed,
-        try_read64_relaxed,
-        readq_relaxed -> u64
+        pub try_read64,
+        readq -> u64
     );
 
-    define_write!(write8, try_write8, writeb <- u8);
-    define_write!(write16, try_write16, writew <- u16);
-    define_write!(write32, try_write32, writel <- u32);
     define_write!(
+        fallible,
         #[cfg(CONFIG_64BIT)]
-        write64,
-        try_write64,
+        pub try_write64,
         writeq <- 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_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)]
+        pub read64_relaxed,
+        readq_relaxed -> u64
+    );
+
+    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)]
+        pub try_read64_relaxed,
+        readq_relaxed -> u64
+    );
+
+    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)]
+        pub write64_relaxed,
+        writeq_relaxed <- u64
+    );
+
+    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.0

[PATCH v6 RESEND 5/7] rust: io: factor out MMIO read/write macros

[Zhi Wang]

Refactor the existing MMIO accessors to use common call macros
instead of inlining the bindings calls in each `define_{read,write}!`
expansion.

This factoring separates the common offset/bounds checks from the
low-level call pattern, making it easier to add additional I/O accessor
families.

No functional change intended.

Signed-off-by: Zhi Wang <zhiw@nvidia.com>
---
 rust/kernel/io.rs | 110 ++++++++++++++++++++++++++++++----------------
 1 file changed, 73 insertions(+), 37 deletions(-)

diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
index 4d98d431b523..090d1b11a896 100644
--- a/rust/kernel/io.rs
+++ b/rust/kernel/io.rs
@@ -124,8 +124,34 @@ pub fn maxsize(&self) -> usize {
 #[repr(transparent)]
 pub struct Mmio<const SIZE: usize = 0>(MmioRaw<SIZE>);
 
+macro_rules! call_mmio_read {
+    (infallible, $c_fn:ident, $self:ident, $type:ty, $addr:expr) => {
+        // SAFETY: By the type invariant `addr` is a valid address for MMIO operations.
+        unsafe { bindings::$c_fn($addr as *const c_void) as $type }
+    };
+
+    (fallible, $c_fn:ident, $self:ident, $type:ty, $addr:expr) => {{
+        // SAFETY: By the type invariant `addr` is a valid address for MMIO operations.
+        Ok(unsafe { bindings::$c_fn($addr as *const c_void) as $type })
+    }};
+}
+
+macro_rules! call_mmio_write {
+    (infallible, $c_fn:ident, $self:ident, $ty:ty, $addr:expr, $value:expr) => {
+        // SAFETY: By the type invariant `addr` is a valid address for MMIO operations.
+        unsafe { bindings::$c_fn($value, $addr as *mut c_void) }
+    };
+
+    (fallible, $c_fn:ident, $self:ident, $ty:ty, $addr:expr, $value:expr) => {{
+        // SAFETY: By the type invariant `addr` is a valid address for MMIO operations.
+        unsafe { bindings::$c_fn($value, $addr as *mut c_void) };
+        Ok(())
+    }};
+}
+
 macro_rules! define_read {
-    (infallible, $(#[$attr:meta])* $vis:vis $name:ident, $c_fn:ident -> $type_name:ty) => {
+    (infallible, $(#[$attr:meta])* $vis:vis $name:ident, $call_macro: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
@@ -135,12 +161,13 @@ macro_rules! define_read {
         $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) }
+            // SAFETY: By the type invariant `addr` is a valid address for IO operations.
+            $call_macro!(infallible, $c_fn, self, $type_name, addr)
         }
     };
 
-    (fallible, $(#[$attr:meta])* $vis:vis $try_name:ident, $c_fn:ident -> $type_name:ty) => {
+    (fallible, $(#[$attr:meta])* $vis:vis $try_name:ident, $call_macro: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
@@ -149,14 +176,16 @@ macro_rules! define_read {
         $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.
-            Ok(unsafe { bindings::$c_fn(addr as *const c_void) })
+            // SAFETY: By the type invariant `addr` is a valid address for IO operations.
+            $call_macro!(fallible, $c_fn, self, $type_name, addr)
         }
     };
 }
+pub(crate) use define_read;
 
 macro_rules! define_write {
-    (infallible, $(#[$attr:meta])* $vis:vis $name:ident, $c_fn:ident <- $type_name:ty) => {
+    (infallible, $(#[$attr:meta])* $vis:vis $name:ident, $call_macro: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
@@ -166,12 +195,12 @@ macro_rules! define_write {
         $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) }
+            $call_macro!(infallible, $c_fn, self, $type_name, addr, value);
         }
     };
 
-    (fallible, $(#[$attr:meta])* $vis:vis $try_name:ident, $c_fn:ident <- $type_name:ty) => {
+    (fallible, $(#[$attr:meta])* $vis:vis $try_name:ident, $call_macro: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
@@ -180,12 +209,11 @@ macro_rules! define_write {
         $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.
-            unsafe { bindings::$c_fn(value, addr as *mut c_void) }
-            Ok(())
+            $call_macro!(fallible, $c_fn, self, $type_name, addr, value)
         }
     };
 }
+pub(crate) use define_write;
 
 /// Checks whether an access of type `U` at the given `offset`
 /// is valid within this region.
@@ -298,23 +326,23 @@ fn maxsize(&self) -> usize {
 }
 
 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_read!(infallible, read8, call_mmio_read, readb -> u8);
+    define_read!(infallible, read16, call_mmio_read, readw -> u16);
+    define_read!(infallible, read32, call_mmio_read, readl -> u32);
 
-    define_write!(infallible, write8, writeb <- u8);
-    define_write!(infallible, write16, writew <- u16);
-    define_write!(infallible, write32, writel <- u32);
+    define_write!(infallible, write8, call_mmio_write, writeb <- u8);
+    define_write!(infallible, write16, call_mmio_write, writew <- u16);
+    define_write!(infallible, write32, call_mmio_write, 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_read!(fallible, try_read8, call_mmio_read, readb -> u8);
+    define_read!(fallible, try_read16, call_mmio_read, readw -> u16);
+    define_read!(fallible, try_read32, call_mmio_read, readl -> u32);
 
-    define_write!(fallible, try_write8, writeb <- u8);
-    define_write!(fallible, try_write16, writew <- u16);
-    define_write!(fallible, try_write32, writel <- u32);
+    define_write!(fallible, try_write8, call_mmio_write, writeb <- u8);
+    define_write!(fallible, try_write16, call_mmio_write, writew <- u16);
+    define_write!(fallible, try_write32, call_mmio_write, writel <- u32);
 }
 
 impl<const SIZE: usize> Mmio<SIZE> {
@@ -333,6 +361,7 @@ pub unsafe fn from_raw(raw: &MmioRaw<SIZE>) -> &Self {
         infallible,
         #[cfg(CONFIG_64BIT)]
         pub read64,
+        call_mmio_read,
         readq -> u64
     );
 
@@ -340,6 +369,7 @@ pub unsafe fn from_raw(raw: &MmioRaw<SIZE>) -> &Self {
         infallible,
         #[cfg(CONFIG_64BIT)]
         pub write64,
+        call_mmio_write,
         writeq <- u64
     );
 
@@ -347,6 +377,7 @@ pub unsafe fn from_raw(raw: &MmioRaw<SIZE>) -> &Self {
         fallible,
         #[cfg(CONFIG_64BIT)]
         pub try_read64,
+        call_mmio_read,
         readq -> u64
     );
 
@@ -354,46 +385,51 @@ pub unsafe fn from_raw(raw: &MmioRaw<SIZE>) -> &Self {
         fallible,
         #[cfg(CONFIG_64BIT)]
         pub try_write64,
+        call_mmio_write,
         writeq <- u64
     );
 
-    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, pub read8_relaxed, call_mmio_read, readb_relaxed -> u8);
+    define_read!(infallible, pub read16_relaxed, call_mmio_read, readw_relaxed -> u16);
+    define_read!(infallible, pub read32_relaxed, call_mmio_read, readl_relaxed -> u32);
     define_read!(
         infallible,
         #[cfg(CONFIG_64BIT)]
         pub read64_relaxed,
+        call_mmio_read,
         readq_relaxed -> u64
     );
 
-    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, pub try_read8_relaxed, call_mmio_read, readb_relaxed -> u8);
+    define_read!(fallible, pub try_read16_relaxed, call_mmio_read, readw_relaxed -> u16);
+    define_read!(fallible, pub try_read32_relaxed, call_mmio_read, readl_relaxed -> u32);
     define_read!(
         fallible,
         #[cfg(CONFIG_64BIT)]
         pub try_read64_relaxed,
+        call_mmio_read,
         readq_relaxed -> u64
     );
 
-    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, pub write8_relaxed, call_mmio_write, writeb_relaxed <- u8);
+    define_write!(infallible, pub write16_relaxed, call_mmio_write, writew_relaxed <- u16);
+    define_write!(infallible, pub write32_relaxed, call_mmio_write, writel_relaxed <- u32);
     define_write!(
         infallible,
         #[cfg(CONFIG_64BIT)]
         pub write64_relaxed,
+        call_mmio_write,
         writeq_relaxed <- u64
     );
 
-    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, pub try_write8_relaxed, call_mmio_write, writeb_relaxed <- u8);
+    define_write!(fallible, pub try_write16_relaxed, call_mmio_write, writew_relaxed <- u16);
+    define_write!(fallible, pub try_write32_relaxed, call_mmio_write, writel_relaxed <- u32);
     define_write!(
         fallible,
         #[cfg(CONFIG_64BIT)]
         pub try_write64_relaxed,
+        call_mmio_write,
         writeq_relaxed <- u64
     );
 }
-- 
2.51.0

[PATCH v6 RESEND 6/7] rust: pci: add config space read/write support

[Zhi Wang]

Drivers might need to access PCI config space for querying capability
structures and access the registers inside the structures.

For Rust drivers need to access PCI config space, the Rust PCI abstraction
needs to support it in a way that upholds Rust's safety principles.

Introduce a `ConfigSpace` wrapper in Rust PCI abstraction to provide safe
accessors for PCI config space. The new type implements the `Io` trait to
share offset validation and bound-checking logic with others.

Cc: Danilo Krummrich <dakr@kernel.org>
Signed-off-by: Zhi Wang <zhiw@nvidia.com>
---
 rust/kernel/pci.rs    | 41 ++++++++++++++++++++++-
 rust/kernel/pci/io.rs | 75 ++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 114 insertions(+), 2 deletions(-)

diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
index 410b79d46632..d8048c7d0f32 100644
--- a/rust/kernel/pci.rs
+++ b/rust/kernel/pci.rs
@@ -39,7 +39,10 @@
     ClassMask,
     Vendor, //
 };
-pub use self::io::Bar;
+pub use self::io::{
+    Bar,
+    ConfigSpace, //
+};
 pub use self::irq::{
     IrqType,
     IrqTypes,
@@ -330,6 +333,28 @@ fn as_raw(&self) -> *mut bindings::pci_dev {
     }
 }
 
+/// Represents the size of a PCI configuration space.
+///
+/// PCI devices can have either a *normal* (legacy) configuration space of 256 bytes,
+/// or an *extended* configuration space of 4096 bytes as defined in the PCI Express
+/// specification.
+#[repr(usize)]
+pub enum ConfigSpaceSize {
+    /// 256-byte legacy PCI configuration space.
+    Normal = 256,
+
+    /// 4096-byte PCIe extended configuration space.
+    Extended = 4096,
+}
+
+impl ConfigSpaceSize {
+    /// Get the raw value of this enum.
+    #[inline(always)]
+    pub const fn as_raw(self) -> usize {
+        self as usize
+    }
+}
+
 impl Device {
     /// Returns the PCI vendor ID as [`Vendor`].
     ///
@@ -426,6 +451,20 @@ pub fn pci_class(&self) -> Class {
         // SAFETY: `self.as_raw` is a valid pointer to a `struct pci_dev`.
         Class::from_raw(unsafe { (*self.as_raw()).class })
     }
+
+    /// Returns the size of configuration space.
+    fn cfg_size(&self) -> Result<ConfigSpaceSize> {
+        // SAFETY: `self.as_raw` is a valid pointer to a `struct pci_dev`.
+        let size = unsafe { (*self.as_raw()).cfg_size };
+        match size {
+            256 => Ok(ConfigSpaceSize::Normal),
+            4096 => Ok(ConfigSpaceSize::Extended),
+            _ => {
+                debug_assert!(false);
+                Err(EINVAL)
+            }
+        }
+    }
 }
 
 impl Device<device::Core> {
diff --git a/rust/kernel/pci/io.rs b/rust/kernel/pci/io.rs
index 2bbb3261198d..bb78a83fe92c 100644
--- a/rust/kernel/pci/io.rs
+++ b/rust/kernel/pci/io.rs
@@ -2,12 +2,19 @@
 
 //! PCI memory-mapped I/O infrastructure.
 
-use super::Device;
+use super::{
+    ConfigSpaceSize,
+    Device, //
+};
 use crate::{
     bindings,
     device,
     devres::Devres,
     io::{
+        define_read,
+        define_write,
+        Io,
+        IoInfallible,
         Mmio,
         MmioRaw, //
     },
@@ -16,6 +23,58 @@
 };
 use core::ops::Deref;
 
+/// The PCI configuration space of a device.
+///
+/// Provides typed read and write accessors for configuration registers
+/// using the standard `pci_read_config_*` and `pci_write_config_*` helpers.
+///
+/// The generic const parameter `SIZE` can be used to indicate the
+/// maximum size of the configuration space (e.g. 256 bytes for legacy,
+/// 4096 bytes for extended config space).
+pub struct ConfigSpace<'a, const SIZE: usize = { ConfigSpaceSize::Extended as usize }> {
+    pub(crate) pdev: &'a Device<device::Bound>,
+}
+
+macro_rules! call_config_read {
+    (infallible, $c_fn:ident, $self:ident, $ty:ty, $addr:expr) => {{
+        let mut val: $ty = 0;
+        let _ret = unsafe { bindings::$c_fn($self.pdev.as_raw(), $addr as i32, &mut val) };
+        val
+    }};
+}
+
+macro_rules! call_config_write {
+    (infallible, $c_fn:ident, $self:ident, $ty:ty, $addr:expr, $value:expr) => {
+        let _ret = unsafe { bindings::$c_fn($self.pdev.as_raw(), $addr as i32, $value) };
+    };
+}
+
+impl<'a, const SIZE: usize> Io for ConfigSpace<'a, SIZE> {
+    const MIN_SIZE: usize = SIZE;
+
+    /// Returns the base address of the I/O region. It is always 0 for configuration space.
+    #[inline]
+    fn addr(&self) -> usize {
+        0
+    }
+
+    /// Returns the maximum size of the configuration space.
+    #[inline]
+    fn maxsize(&self) -> usize {
+        self.pdev.cfg_size().map_or(0, |v| v as usize)
+    }
+}
+
+impl<'a, const SIZE: usize> IoInfallible for ConfigSpace<'a, SIZE> {
+    define_read!(infallible, read8, call_config_read, pci_read_config_byte -> u8);
+    define_read!(infallible, read16, call_config_read, pci_read_config_word -> u16);
+    define_read!(infallible, read32, call_config_read, pci_read_config_dword -> u32);
+
+    define_write!(infallible, write8, call_config_write, pci_write_config_byte <- u8);
+    define_write!(infallible, write16, call_config_write, pci_write_config_word <- u16);
+    define_write!(infallible, write32, call_config_write, pci_write_config_dword <- u32);
+}
+
 /// A PCI BAR to perform I/O-Operations on.
 ///
 /// # Invariants
@@ -141,4 +200,18 @@ pub fn iomap_region<'a>(
     ) -> impl PinInit<Devres<Bar>, Error> + 'a {
         self.iomap_region_sized::<0>(bar, name)
     }
+
+    /// Return an initialized config space object.
+    pub fn config_space<'a>(
+        &'a self,
+    ) -> Result<ConfigSpace<'a, { ConfigSpaceSize::Normal.as_raw() }>> {
+        Ok(ConfigSpace { pdev: self })
+    }
+
+    /// Return an initialized config space object.
+    pub fn config_space_exteneded<'a>(
+        &'a self,
+    ) -> Result<ConfigSpace<'a, { ConfigSpaceSize::Extended.as_raw() }>> {
+        Ok(ConfigSpace { pdev: self })
+    }
 }
-- 
2.51.0

[PATCH v6 RESNED 7/7] sample: rust: pci: add tests for config space routines

[Zhi Wang]

Add tests exercising the PCI configuration space helpers.

Suggested-by: Danilo Krummrich <dakr@kernel.org>
Signed-off-by: Zhi Wang <zhiw@nvidia.com>
---
 samples/rust/rust_driver_pci.rs | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/samples/rust/rust_driver_pci.rs b/samples/rust/rust_driver_pci.rs
index 74b93ca7c338..8f549b2100bc 100644
--- a/samples/rust/rust_driver_pci.rs
+++ b/samples/rust/rust_driver_pci.rs
@@ -67,6 +67,32 @@ fn testdev(index: &TestIndex, bar: &Bar0) -> Result<u32> {
 
         Ok(bar.read32(Regs::COUNT))
     }
+
+    fn config_space(pdev: &pci::Device<Core>) -> Result {
+        let config = pdev.config_space()?;
+
+        // TODO: use the register!() macro for defining PCI configuration space registers once it
+        // has been move out of nova-core.
+        dev_info!(
+            pdev.as_ref(),
+            "pci-testdev config space read8 rev ID: {:x}\n",
+            config.read8(0x8)
+        );
+
+        dev_info!(
+            pdev.as_ref(),
+            "pci-testdev config space read16 vendor ID: {:x}\n",
+            config.read16(0)
+        );
+
+        dev_info!(
+            pdev.as_ref(),
+            "pci-testdev config space read32 BAR 0: {:x}\n",
+            config.read32(0x10)
+        );
+
+        Ok(())
+    }
 }
 
 impl pci::Driver for SampleDriver {
@@ -98,6 +124,7 @@ fn probe(pdev: &pci::Device<Core>, info: &Self::IdInfo) -> impl PinInit<Self, Er
                         "pci-testdev data-match count: {}\n",
                         Self::testdev(info, bar)?
                     );
+                    Self::config_space(pdev)?;
                 },
                 pdev: pdev.into(),
             }))
-- 
2.51.0

Re: [PATCH v6 RESEND 0/7] rust: pci: add config space read/write support

[Joel Fernandes]

On 11/10/2025 3:41 PM, Zhi Wang wrote:
> In the NVIDIA vGPU RFC [1], the PCI configuration space access is
> required in nova-core for preparing gspVFInfo when vGPU support is
> enabled. This series is the following up of the discussion with Danilo
> for how to introduce support of PCI configuration space access in Rust
> PCI abstractions.

Hi Zhi, is there a tree with all the patches and dependencies for this series?

Typically it is a good idea to provide it with all dependencies, so folks can
checkout the tree.

git format-patch also has an --auto option that adds base commit information, so
folks know

Thanks.

Re: [PATCH v6 RESEND 0/7] rust: pci: add config space read/write support

[Zhi Wang]

On Mon, 10 Nov 2025 19:01:39 -0500
Joel Fernandes <joelagnelf@nvidia.com> wrote:

> On 11/10/2025 3:41 PM, Zhi Wang wrote:
> > In the NVIDIA vGPU RFC [1], the PCI configuration space access is
> > required in nova-core for preparing gspVFInfo when vGPU support is
> > enabled. This series is the following up of the discussion with
> > Danilo for how to introduce support of PCI configuration space
> > access in Rust PCI abstractions.
> 
> Hi Zhi, is there a tree with all the patches and dependencies for
> this series?
> 

Hi Joel, I uploaded the tree here:

https://github.com/zhiwang-nvidia/nova-core/tree/rust-for-linux/pci-configuration-space-v6

> Typically it is a good idea to provide it with all dependencies, so
> folks can checkout the tree.
> 
> git format-patch also has an --auto option that adds base commit
> information, so folks know
> 
> Thanks.

Re: [PATCH v6 RESEND 4/7] rust: io: factor common I/O helpers into Io trait

[Alexandre Courbot]

Hi Zhi,

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

This is looking quite good to me! I tried really hard to find stuff to
comment about, but couldn't. :)

Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>

Re: [PATCH v6 RESEND 5/7] rust: io: factor out MMIO read/write macros

[Alexandre Courbot]

On Tue Nov 11, 2025 at 5:41 AM JST, Zhi Wang wrote:
> Refactor the existing MMIO accessors to use common call macros
> instead of inlining the bindings calls in each `define_{read,write}!`
> expansion.
>
> This factoring separates the common offset/bounds checks from the
> low-level call pattern, making it easier to add additional I/O accessor
> families.
>
> No functional change intended.
>
> Signed-off-by: Zhi Wang <zhiw@nvidia.com>
> ---
>  rust/kernel/io.rs | 110 ++++++++++++++++++++++++++++++----------------
>  1 file changed, 73 insertions(+), 37 deletions(-)
>
> diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
> index 4d98d431b523..090d1b11a896 100644
> --- a/rust/kernel/io.rs
> +++ b/rust/kernel/io.rs
> @@ -124,8 +124,34 @@ pub fn maxsize(&self) -> usize {
>  #[repr(transparent)]
>  pub struct Mmio<const SIZE: usize = 0>(MmioRaw<SIZE>);
>  
> +macro_rules! call_mmio_read {
> +    (infallible, $c_fn:ident, $self:ident, $type:ty, $addr:expr) => {
> +        // SAFETY: By the type invariant `addr` is a valid address for MMIO operations.
> +        unsafe { bindings::$c_fn($addr as *const c_void) as $type }
> +    };
> +
> +    (fallible, $c_fn:ident, $self:ident, $type:ty, $addr:expr) => {{
> +        // SAFETY: By the type invariant `addr` is a valid address for MMIO operations.
> +        Ok(unsafe { bindings::$c_fn($addr as *const c_void) as $type })
> +    }};
> +}
> +
> +macro_rules! call_mmio_write {
> +    (infallible, $c_fn:ident, $self:ident, $ty:ty, $addr:expr, $value:expr) => {
> +        // SAFETY: By the type invariant `addr` is a valid address for MMIO operations.
> +        unsafe { bindings::$c_fn($value, $addr as *mut c_void) }
> +    };
> +
> +    (fallible, $c_fn:ident, $self:ident, $ty:ty, $addr:expr, $value:expr) => {{
> +        // SAFETY: By the type invariant `addr` is a valid address for MMIO operations.
> +        unsafe { bindings::$c_fn($value, $addr as *mut c_void) };
> +        Ok(())
> +    }};
> +}

I understand the intent from the commit message, but this is starting to
look like an intricate maze of macro expansion and it might not be as
easy for first-time readers - could you add an explanatory doccomment
for these?

> +
>  macro_rules! define_read {
> -    (infallible, $(#[$attr:meta])* $vis:vis $name:ident, $c_fn:ident -> $type_name:ty) => {
> +    (infallible, $(#[$attr:meta])* $vis:vis $name:ident, $call_macro: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
> @@ -135,12 +161,13 @@ macro_rules! define_read {
>          $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) }
> +            // SAFETY: By the type invariant `addr` is a valid address for IO operations.
> +            $call_macro!(infallible, $c_fn, self, $type_name, addr)
>          }
>      };

To convey the fact that `$c_fn` is passed to `$call_macro`, how about
changing the syntax to something like 

  `define_read(infallible, $vis $name $call_macro($c_fn) -> $type_name`

?

Re: [PATCH v6 RESEND 6/7] rust: pci: add config space read/write support

[Alexandre Courbot]

On Tue Nov 11, 2025 at 5:41 AM JST, Zhi Wang wrote:
> Drivers might need to access PCI config space for querying capability
> structures and access the registers inside the structures.
>
> For Rust drivers need to access PCI config space, the Rust PCI abstraction
> needs to support it in a way that upholds Rust's safety principles.
>
> Introduce a `ConfigSpace` wrapper in Rust PCI abstraction to provide safe
> accessors for PCI config space. The new type implements the `Io` trait to
> share offset validation and bound-checking logic with others.
>
> Cc: Danilo Krummrich <dakr@kernel.org>
> Signed-off-by: Zhi Wang <zhiw@nvidia.com>
> ---
>  rust/kernel/pci.rs    | 41 ++++++++++++++++++++++-
>  rust/kernel/pci/io.rs | 75 ++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 114 insertions(+), 2 deletions(-)
>
> diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
> index 410b79d46632..d8048c7d0f32 100644
> --- a/rust/kernel/pci.rs
> +++ b/rust/kernel/pci.rs
> @@ -39,7 +39,10 @@
>      ClassMask,
>      Vendor, //
>  };
> -pub use self::io::Bar;
> +pub use self::io::{
> +    Bar,
> +    ConfigSpace, //
> +};
>  pub use self::irq::{
>      IrqType,
>      IrqTypes,
> @@ -330,6 +333,28 @@ fn as_raw(&self) -> *mut bindings::pci_dev {
>      }
>  }
>  
> +/// Represents the size of a PCI configuration space.
> +///
> +/// PCI devices can have either a *normal* (legacy) configuration space of 256 bytes,
> +/// or an *extended* configuration space of 4096 bytes as defined in the PCI Express
> +/// specification.

The comment says this is either, but below we have:

> @@ -141,4 +200,18 @@ pub fn iomap_region<'a>(
>      ) -> impl PinInit<Devres<Bar>, Error> + 'a {
>          self.iomap_region_sized::<0>(bar, name)
>      }
> +
> +    /// Return an initialized config space object.
> +    pub fn config_space<'a>(
> +        &'a self,
> +    ) -> Result<ConfigSpace<'a, { ConfigSpaceSize::Normal.as_raw() }>> {
> +        Ok(ConfigSpace { pdev: self })
> +    }
> +
> +    /// Return an initialized config space object.
> +    pub fn config_space_exteneded<'a>(
> +        &'a self,
> +    ) -> Result<ConfigSpace<'a, { ConfigSpaceSize::Extended.as_raw() }>> {
> +        Ok(ConfigSpace { pdev: self })
> +    }
>  }

(typo on "exteneded" btw)

Which means that a caller can infallibly (both methods return a `Result`
but can never fail, for some reason) create a `ConfigSpace` that does
not match its actual size.

That's particularly a problem is `cfg_size` returns `256` but we call
`config_space_extended`, as the returned `ConfigSpace` will have a
`maxsize` that is smaller than its `MIN_SIZE`...

I guess we should either validate the size using `cfg_size` before
creating and returning the `ConfigSpace`, or have a single method that
returns a Result containing an enum which variants are the supported
sizes?

I am also wondering whether we want `ConfigSpace` to be generic over a
constant - it makes it possible to have instances or arbitrary size, and
makes them a bit cumbersome to define as we need to do something like
`ConfigSpace<'a, { ConfigSpaceSize::Extended.as_raw() }>`.

Maybe we can use types instead, e.g.

pub trait ConfigSpaceSize {
    const MIN_SIZE: usize;
}

pub enum Normal {}
impl ConfigSpaceSize for Normal {
    const MIN_SIZE: usize = 256;
}

pub enum Extended {}
impl ConfigSpaceSize for Extended {
    const MIN_SIZE: usize = 4096;
}

pub struct ConfigSpace<'a, S: ConfigSpaceSize> {
    ...

impl<'a, const SIZE: usize> Io for ConfigSpace<'a, S: ConfigSpaceSize> {
    const MIN_SIZE: usize = S::MIN_SIZE;
    ...

And then `cfg_size` could be replaced by just `config_space` which
returns an enum of the possible `ConfigSpace`s supported, that the
caller needs to match against.

Just an idea for your consideration, I don't know whether that would
actually work better. :)

Re: [PATCH v6 RESEND 6/7] rust: pci: add config space read/write support

[Joel Fernandes]

Hi Zhi,

On Mon, Nov 10, 2025 at 10:41:18PM +0200, Zhi Wang wrote:
[..]  
>  impl Device<device::Core> {
> diff --git a/rust/kernel/pci/io.rs b/rust/kernel/pci/io.rs
> index 2bbb3261198d..bb78a83fe92c 100644
> --- a/rust/kernel/pci/io.rs
> +++ b/rust/kernel/pci/io.rs
> @@ -2,12 +2,19 @@
>  
>  //! PCI memory-mapped I/O infrastructure.
>  
> -use super::Device;
> +use super::{
> +    ConfigSpaceSize,
> +    Device, //
> +};
>  use crate::{
>      bindings,
>      device,
>      devres::Devres,
>      io::{
> +        define_read,
> +        define_write,
> +        Io,
> +        IoInfallible,
>          Mmio,
>          MmioRaw, //
>      },
> @@ -16,6 +23,58 @@
>  };
>  use core::ops::Deref;
>  
> +/// The PCI configuration space of a device.
> +///
> +/// Provides typed read and write accessors for configuration registers
> +/// using the standard `pci_read_config_*` and `pci_write_config_*` helpers.
> +///
> +/// The generic const parameter `SIZE` can be used to indicate the
> +/// maximum size of the configuration space (e.g. 256 bytes for legacy,
> +/// 4096 bytes for extended config space).
> +pub struct ConfigSpace<'a, const SIZE: usize = { ConfigSpaceSize::Extended as usize }> {
> +    pub(crate) pdev: &'a Device<device::Bound>,
> +}
> +
> +macro_rules! call_config_read {
> +    (infallible, $c_fn:ident, $self:ident, $ty:ty, $addr:expr) => {{
> +        let mut val: $ty = 0;
> +        let _ret = unsafe { bindings::$c_fn($self.pdev.as_raw(), $addr as i32, &mut val) };
> +        val
> +    }};
> +}
> +
> +macro_rules! call_config_write {
> +    (infallible, $c_fn:ident, $self:ident, $ty:ty, $addr:expr, $value:expr) => {
> +        let _ret = unsafe { bindings::$c_fn($self.pdev.as_raw(), $addr as i32, $value) };

unsafe block needs safety comments, also I understand 'as' to convert is
generally forbidden without a CAST: comment or using ::from() for conversion
because it can by a lossy conversion.

Also we should have a comment on why its safe for _ret to be ignored.
Basically what guarantees that the call is really infallible? Anything we can
do to ensure errors are not silently ignored? Let me know if I missed
something.

[..]

> +
> +    /// Return an initialized config space object.
> +    pub fn config_space_exteneded<'a>(

typo in func name.

thanks,

 - Joel

> +        &'a self,
> +    ) -> Result<ConfigSpace<'a, { ConfigSpaceSize::Extended.as_raw() }>> {
> +        Ok(ConfigSpace { pdev: self })
> +    }
>  }
> -- 
> 2.51.0
> 

Re: [PATCH v6 RESEND 4/7] rust: io: factor common I/O helpers into Io trait

[Alice Ryhl]

On Mon, Nov 10, 2025 at 10:41:16PM +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 a new Io trait, and move the MMIO-specific
> logic into a dedicated Mmio<SIZE> type implementing that trait. Rename the
> IoRaw to MmioRaw and update the bus MMIO implementations to use MmioRaw.
> 
> No functional change intended.
> 
> Cc: Alexandre Courbot <acourbot@nvidia.com>
> Cc: Bjorn Helgaas <helgaas@kernel.org>
> Cc: Danilo Krummrich <dakr@kernel.org>
> Cc: John Hubbard <jhubbard@nvidia.com>
> Signed-off-by: Zhi Wang <zhiw@nvidia.com>

This defines three traits:

* Io
* IoInfallible: Io
* IoFallible: Io

This particular split says that there are going to be cases where we
implement IoInfallible only, cases where we implement IoFallible only,
and maybe cases where we implement both.

And the distiction between them is whether the bounds check is runtime
or compile-time.

But this doesn't make much sense to me. Surely any Io resource that can
provide compile-time checked io can also provide runtime-checked io, so
maybe IoFallible should extend IoInfallible?

And why are these separate traits at all? Why not support both
compile-time and runtime-checked IO always?

I noticed also that the trait does not have methods for 64-bit writes,
and that these are left as inherent methods on Mmio.

The traits that would make sense to me are these:

* Io
* Io64: Io

where Io provides everything the three traits you have now provides, and
Io64 provides the 64-bit operations. That way, everything needs to
support operations of various sizes with both compile-time and
runtime-checked bounds, but types may opt-in to providing 64-bit ops.

Thoughts?

Alice

Re: [PATCH v6 RESEND 5/7] rust: io: factor out MMIO read/write macros

[Zhi Wang]

On Thu, 13 Nov 2025 16:36:47 +0900
"Alexandre Courbot" <acourbot@nvidia.com> wrote:


> I understand the intent from the commit message, but this is starting
> to look like an intricate maze of macro expansion and it might not be
> as easy for first-time readers - could you add an explanatory
> doccomment for these?
> 

Sure.

> > +
> >  macro_rules! define_read {
> > -    (infallible, $(#[$attr:meta])* $vis:vis $name:ident,
> > $c_fn:ident -> $type_name:ty) => {
> > +    (infallible, $(#[$attr:meta])* $vis:vis $name:ident,
> > $call_macro: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 @@ -135,12 +161,13 @@
> > macro_rules! define_read { $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) }
> > +            // SAFETY: By the type invariant `addr` is a valid
> > address for IO operations.
> > +            $call_macro!(infallible, $c_fn, self, $type_name, addr)
> >          }
> >      };
> 
> To convey the fact that `$c_fn` is passed to `$call_macro`, how about
> changing the syntax to something like 
> 
>   `define_read(infallible, $vis $name $call_macro($c_fn) ->
> $type_name`
> 
> ?

Then the macros will look like:

define_read!(infallible, pub read32 call_mmio_read!(ioread32) -> u32);
define_write!(infallible, pub write32 call_mmio_write!(iowrite32) ->
u32);

The readability is indeed better - I’ll take that, thanks. :)

Z.

Re: [PATCH v6 RESEND 6/7] rust: pci: add config space read/write support

[Zhi Wang]

On Thu, 13 Nov 2025 16:56:28 +0900
"Alexandre Courbot" <acourbot@nvidia.com> wrote:

> On Tue Nov 11, 2025 at 5:41 AM JST, Zhi Wang wrote:
> > Drivers might need to access PCI config space for querying
> > capability structures and access the registers inside the
> > structures.
> >
> > For Rust drivers need to access PCI config space, the Rust PCI
> > abstraction needs to support it in a way that upholds Rust's safety
> > principles.
> >
> > Introduce a `ConfigSpace` wrapper in Rust PCI abstraction to
> > provide safe accessors for PCI config space. The new type
> > implements the `Io` trait to share offset validation and
> > bound-checking logic with others.
> >
> > Cc: Danilo Krummrich <dakr@kernel.org>
> > Signed-off-by: Zhi Wang <zhiw@nvidia.com>
> > ---
> >  rust/kernel/pci.rs    | 41 ++++++++++++++++++++++-
> >  rust/kernel/pci/io.rs | 75
> > ++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 114
> > insertions(+), 2 deletions(-)
> >
> > diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
> > index 410b79d46632..d8048c7d0f32 100644
> > --- a/rust/kernel/pci.rs
> > +++ b/rust/kernel/pci.rs
> > @@ -39,7 +39,10 @@
> >      ClassMask,
> >      Vendor, //
> >  };
> > -pub use self::io::Bar;
> > +pub use self::io::{
> > +    Bar,
> > +    ConfigSpace, //
> > +};
> >  pub use self::irq::{
> >      IrqType,
> >      IrqTypes,
> > @@ -330,6 +333,28 @@ fn as_raw(&self) -> *mut bindings::pci_dev {
> >      }
> >  }
> >  
> > +/// Represents the size of a PCI configuration space.
> > +///
> > +/// PCI devices can have either a *normal* (legacy) configuration
> > space of 256 bytes, +/// or an *extended* configuration space of
> > 4096 bytes as defined in the PCI Express +/// specification.
> 
> The comment says this is either, but below we have:
> 
> > @@ -141,4 +200,18 @@ pub fn iomap_region<'a>(
> >      ) -> impl PinInit<Devres<Bar>, Error> + 'a {
> >          self.iomap_region_sized::<0>(bar, name)
> >      }
> > +
> > +    /// Return an initialized config space object.
> > +    pub fn config_space<'a>(
> > +        &'a self,
> > +    ) -> Result<ConfigSpace<'a, { ConfigSpaceSize::Normal.as_raw()
> > }>> {
> > +        Ok(ConfigSpace { pdev: self })
> > +    }
> > +
> > +    /// Return an initialized config space object.
> > +    pub fn config_space_exteneded<'a>(
> > +        &'a self,
> > +    ) -> Result<ConfigSpace<'a, {
> > ConfigSpaceSize::Extended.as_raw() }>> {
> > +        Ok(ConfigSpace { pdev: self })
> > +    }
> >  }
> 
> (typo on "exteneded" btw)
> 
> Which means that a caller can infallibly (both methods return a
> `Result` but can never fail, for some reason) create a `ConfigSpace`
> that does not match its actual size.
> 
> That's particularly a problem is `cfg_size` returns `256` but we call
> `config_space_extended`, as the returned `ConfigSpace` will have a
> `maxsize` that is smaller than its `MIN_SIZE`...
> 
> I guess we should either validate the size using `cfg_size` before
> creating and returning the `ConfigSpace`, or have a single method that
> returns a Result containing an enum which variants are the supported
> sizes?
> 

AFAIU, this was intentional according to usage model of the Io trait.
It has two checking paths, one is at build time and one is at run time.
Pretty much similar with MMIO traits:

- The driver assumes a minimum/expected working region size at build
  time. In PCI configuration space case, the driver knows if its device
  have a extended area or not, and choose
  config_space()/config_space_extended() accordingly.

- The actual available region size is decided at runtime, which will be
  from maxsize() method in the trait. So accessing the region will be
  checked 

The fallible accessors will do runtime check, while infallible
accessors will do build time check.

To following that model,

- cfg_size is only known at runtime. So I didn't get it invovled
  in the config_space()/config_space_extended() path, which is for
  runtime path.

- If a driver chooses the wrong config_space()/config_space_extended()
  which doesn't match its device nature at build time and compiling
  passes:

  a. device has extended area, but driver chooses config_space():
	- the infallible accessors only allows to acccess the legacy
	  256-byte area at build time. If the driver uses the fallible
	  accessors, it still can access the extended area at runtime.

  b. device doesn't have extended area, but driver chooses
  config_space_extended():

	- In this case, the driver can use the infallible accessors to
	  reach the unexpected area and slipped away from the build
	  time check (I think it is the similar situation in MMIO path
	  since it is device specific.). The driver will get !0 at
	  runtime if it reads extended area.

	- Infallible path is not affected. 

> Just an idea for your consideration, I don't know whether that would
> actually work better. :)

It is always good to know new and nice tricks. :)

Z.

Re: [PATCH v6 RESEND 4/7] rust: io: factor common I/O helpers into Io trait

[Zhi Wang]

On Fri, 14 Nov 2025 12:58:31 +0000
Alice Ryhl <aliceryhl@google.com> wrote:

> On Mon, Nov 10, 2025 at 10:41:16PM +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 a new Io trait, and move the
> > MMIO-specific logic into a dedicated Mmio<SIZE> type implementing
> > that trait. Rename the IoRaw to MmioRaw and update the bus MMIO
> > implementations to use MmioRaw.
> > 
> > No functional change intended.
> > 
> > Cc: Alexandre Courbot <acourbot@nvidia.com>
> > Cc: Bjorn Helgaas <helgaas@kernel.org>
> > Cc: Danilo Krummrich <dakr@kernel.org>
> > Cc: John Hubbard <jhubbard@nvidia.com>
> > Signed-off-by: Zhi Wang <zhiw@nvidia.com>
> 
> This defines three traits:
> 
> * Io
> * IoInfallible: Io
> * IoFallible: Io
> 
> This particular split says that there are going to be cases where we
> implement IoInfallible only, cases where we implement IoFallible only,
> and maybe cases where we implement both.
> 
> And the distiction between them is whether the bounds check is runtime
> or compile-time.
> 
> But this doesn't make much sense to me. Surely any Io resource that
> can provide compile-time checked io can also provide runtime-checked
> io, so maybe IoFallible should extend IoInfallible?
> 
> And why are these separate traits at all? Why not support both
> compile-time and runtime-checked IO always?
>

Hi Alice:

Thanks for comments. I did have a version that PCI configuration space
only have fallible accessors because I thought the device can be
unplugged or a VF might fail its FLR and get unresponsive, so the driver
may need to check the return all the time. And Danilo's comments were
let's have the infallible accessors for PCI configuration space and add
them later if some driver needs it. [1]

I am open to either options. like have both or having infallibles first
and fallibles later.

> I noticed also that the trait does not have methods for 64-bit writes,
> and that these are left as inherent methods on Mmio.
> 
> The traits that would make sense to me are these:
> 
> * Io
> * Io64: Io
> 

Hehe. I had the same idea here [2]:

> Io trait - Main trait + 32-bit access
>  | 
>  | -- Common address/bound checks
>  |
>  |	(accessor traits)
>  | -- Io Fallible trait - (MMIO backend implements)
>  | -- Io Infallible trait - (MMIO/ConfigSpace backend implements this)
>  |
>  | -- Io64 trait - For backend supports 64 bit access
> 	   |      (accessor traits)
>          | -- Io64 Faillable trait (MMIO backend implements this)
> 	   | -- Io64 Infallible trait (MMIO backend implements this)

I am struggling with how many IO backends actually need 64bit read/write
other than MMIO backend. E.g. SPI, I2C. The conclusion we had so far was
we can add it at any time when someone need it. If we think this is
necessary, I can add it in the next spin.

[1] https://lore.kernel.org/all/DE00WIVOSYC2.2CAGWUYWE6FZ@kernel.org/
[2]
https://lore.kernel.org/all/e7a75899-be93-4f0f-9c9f-0d63d03f4806@kernel.org/

> where Io provides everything the three traits you have now provides,
> and Io64 provides the 64-bit operations. That way, everything needs to
> support operations of various sizes with both compile-time and
> runtime-checked bounds, but types may opt-in to providing 64-bit ops.
> 
> Thoughts?
> 
> Alice

Re: [PATCH v6 RESEND 4/7] rust: io: factor common I/O helpers into Io trait

[Tamir Duberstein]

On Fri, Nov 14, 2025 at 12:37 PM Zhi Wang <zhiw@nvidia.com> wrote:
>
> On Fri, 14 Nov 2025 12:58:31 +0000
> Alice Ryhl <aliceryhl@google.com> wrote:
>
> > On Mon, Nov 10, 2025 at 10:41:16PM +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 a new Io trait, and move the
> > > MMIO-specific logic into a dedicated Mmio<SIZE> type implementing
> > > that trait. Rename the IoRaw to MmioRaw and update the bus MMIO
> > > implementations to use MmioRaw.
> > >
> > > No functional change intended.
> > >
> > > Cc: Alexandre Courbot <acourbot@nvidia.com>
> > > Cc: Bjorn Helgaas <helgaas@kernel.org>
> > > Cc: Danilo Krummrich <dakr@kernel.org>
> > > Cc: John Hubbard <jhubbard@nvidia.com>
> > > Signed-off-by: Zhi Wang <zhiw@nvidia.com>
> >
> > This defines three traits:
> >
> > * Io
> > * IoInfallible: Io
> > * IoFallible: Io
> >
> > This particular split says that there are going to be cases where we
> > implement IoInfallible only, cases where we implement IoFallible only,
> > and maybe cases where we implement both.
> >
> > And the distiction between them is whether the bounds check is runtime
> > or compile-time.
> >
> > But this doesn't make much sense to me. Surely any Io resource that
> > can provide compile-time checked io can also provide runtime-checked
> > io, so maybe IoFallible should extend IoInfallible?
> >
> > And why are these separate traits at all? Why not support both
> > compile-time and runtime-checked IO always?
> >
>
> Hi Alice:
>
> Thanks for comments. I did have a version that PCI configuration space
> only have fallible accessors because I thought the device can be
> unplugged or a VF might fail its FLR and get unresponsive, so the driver
> may need to check the return all the time. And Danilo's comments were
> let's have the infallible accessors for PCI configuration space and add
> them later if some driver needs it. [1]
>
> I am open to either options. like have both or having infallibles first
> and fallibles later.

What about using an associated Err type? In the infallible case, it
would be `core::convert::Infallible`. It would be slightly more
ergonomic if associated type defaults were stable[0], though.

[0] https://github.com/rust-lang/rust/issues/29661

Re: [PATCH v6 RESEND 4/7] rust: io: factor common I/O helpers into Io trait

[Danilo Krummrich]

On Sat Nov 15, 2025 at 4:27 AM AEDT, Zhi Wang wrote:
> On Fri, 14 Nov 2025 12:58:31 +0000
> Alice Ryhl <aliceryhl@google.com> wrote:
>> This defines three traits:
>> 
>> * Io
>> * IoInfallible: Io
>> * IoFallible: Io
>> 
>> This particular split says that there are going to be cases where we
>> implement IoInfallible only, cases where we implement IoFallible only,
>> and maybe cases where we implement both.
>> 
>> And the distiction between them is whether the bounds check is runtime
>> or compile-time.
>> 
>> But this doesn't make much sense to me. Surely any Io resource that
>> can provide compile-time checked io can also provide runtime-checked
>> io, so maybe IoFallible should extend IoInfallible?

Yeah, though I did like that with this split we can enforce one or the other.

E.g. in the case of the PCI configuration space we can always assert the
expected size at compile time and drivers should not have to deal with runtime
offsets either.

Hence, with this split we can avoid that drivers implement unnecessary runtime
checks.

Either is fine with me though.

>> And why are these separate traits at all? Why not support both
>> compile-time and runtime-checked IO always?
>>
>
> Hi Alice:
>
> Thanks for comments. I did have a version that PCI configuration space
> only have fallible accessors because I thought the device can be
> unplugged or a VF might fail its FLR and get unresponsive, so the driver
> may need to check the return all the time. And Danilo's comments were
> let's have the infallible accessors for PCI configuration space and add
> them later if some driver needs it. [1]

Yeah, that's the same with MMIO accesses as well, yet we don't check all the
time. Actually, when a device falls from the bus, the error state is not
available immediately either and hence drivers have to be robust enough to deal
with it anyway.

Adding in that a driver is also unbound as soon as the device falling from the
bus is detected, there is not much value in those checks.

Re: [PATCH v6 RESEND 4/7] rust: io: factor common I/O helpers into Io trait

[Zhi Wang]

On Fri, 14 Nov 2025 13:53:01 -0500
Tamir Duberstein <tamird@gmail.com> wrote:

> On Fri, Nov 14, 2025 at 12:37 PM Zhi Wang <zhiw@nvidia.com> wrote:
> >

snip

> 
> What about using an associated Err type? In the infallible case, it
> would be `core::convert::Infallible`. It would be slightly more
> ergonomic if associated type defaults were stable[0], though.
> 
> [0] https://github.com/rust-lang/rust/issues/29661

Thanks, Tamir. From Alice's and Danilo’s discussion, it seems we’ll keep
the fallible and infallible traits separate for now.

Still, I really like the idea of an associated Err type. Thanks for the
trick. :)

Z.

Re: [PATCH v6 RESEND 6/7] rust: pci: add config space read/write support

[Zhi Wang]

On Thu, 13 Nov 2025 19:20:05 -0500
Joel Fernandes <joelagnelf@nvidia.com> wrote:

> Hi Zhi,
> 
> On Mon, Nov 10, 2025 at 10:41:18PM +0200, Zhi Wang wrote:
> [..]  
> >  impl Device<device::Core> {
> > diff --git a/rust/kernel/pci/io.rs b/rust/kernel/pci/io.rs
> > index 2bbb3261198d..bb78a83fe92c 100644
> > --- a/rust/kernel/pci/io.rs
> > +++ b/rust/kernel/pci/io.rs
> > @@ -2,12 +2,19 @@
> >  
> >  //! PCI memory-mapped I/O infrastructure.
> >  
> > -use super::Device;
> > +use super::{
> > +    ConfigSpaceSize,
> > +    Device, //
> > +};
> >  use crate::{
> >      bindings,
> >      device,
> >      devres::Devres,
> >      io::{
> > +        define_read,
> > +        define_write,
> > +        Io,
> > +        IoInfallible,
> >          Mmio,
> >          MmioRaw, //
> >      },
> > @@ -16,6 +23,58 @@
> >  };
> >  use core::ops::Deref;
> >  
> > +/// The PCI configuration space of a device.
> > +///
> > +/// Provides typed read and write accessors for configuration
> > registers +/// using the standard `pci_read_config_*` and
> > `pci_write_config_*` helpers. +///
> > +/// The generic const parameter `SIZE` can be used to indicate the
> > +/// maximum size of the configuration space (e.g. 256 bytes for
> > legacy, +/// 4096 bytes for extended config space).
> > +pub struct ConfigSpace<'a, const SIZE: usize = {
> > ConfigSpaceSize::Extended as usize }> {
> > +    pub(crate) pdev: &'a Device<device::Bound>,
> > +}
> > +
> > +macro_rules! call_config_read {
> > +    (infallible, $c_fn:ident, $self:ident, $ty:ty, $addr:expr) =>
> > {{
> > +        let mut val: $ty = 0;
> > +        let _ret = unsafe { bindings::$c_fn($self.pdev.as_raw(),
> > $addr as i32, &mut val) };
> > +        val
> > +    }};
> > +}
> > +
> > +macro_rules! call_config_write {
> > +    (infallible, $c_fn:ident, $self:ident, $ty:ty, $addr:expr,
> > $value:expr) => {
> > +        let _ret = unsafe { bindings::$c_fn($self.pdev.as_raw(),
> > $addr as i32, $value) };
> 
> unsafe block needs safety comments, also I understand 'as' to convert

snip

> is generally forbidden without a CAST: comment or using ::from() for
> conversion because it can by a lossy conversion.
> 

Let me take a look, basically this was similar with the define_{read,
mmio} macros, which has originally been from the mainline. Might have to
fix that part as well.

> Also we should have a comment on why its safe for _ret to be ignored.
> Basically what guarantees that the call is really infallible?
> Anything we can do to ensure errors are not silently ignored? Let me
> know if I missed something.
> 

This was discussed with Danilo on Zulip. The driver will observe a
!0 value on read when an error occurs in the infallible accessors. For
writes, I think the driver should read the value back. (similar with
MMIO cases)

Besides those, I think the driver needs to use fallible ones if it
really cares about the return.

Z.

> [..]
> 
> > +
> > +    /// Return an initialized config space object.
> > +    pub fn config_space_exteneded<'a>(
> 
> typo in func name.
> 
> thanks,
> 
>  - Joel
> 
> > +        &'a self,
> > +    ) -> Result<ConfigSpace<'a, {
> > ConfigSpaceSize::Extended.as_raw() }>> {
> > +        Ok(ConfigSpace { pdev: self })
> > +    }
> >  }
> > -- 
> > 2.51.0
> > 

Re: [PATCH v6 RESEND 6/7] rust: pci: add config space read/write support

[Danilo Krummrich]

On Fri Nov 14, 2025 at 1:20 PM NZDT, Joel Fernandes wrote:
> Also we should have a comment on why its safe for _ret to be ignored.
> Basically what guarantees that the call is really infallible? Anything we can
> do to ensure errors are not silently ignored? Let me know if I missed
> something.

Please also see [1].

[1] https://lore.kernel.org/all/DE8PBRC48D14.IX6FUPOLLVHR@kernel.org/

Re: [PATCH v6 RESEND 4/7] rust: io: factor common I/O helpers into Io trait

[John Hubbard]

On 11/14/25 4:58 AM, Alice Ryhl wrote:
> On Mon, Nov 10, 2025 at 10:41:16PM +0200, Zhi Wang wrote:
...
> This defines three traits:
> 
> * Io
> * IoInfallible: Io
> * IoFallible: Io
> 
> This particular split says that there are going to be cases where we
> implement IoInfallible only, cases where we implement IoFallible only,
> and maybe cases where we implement both.
> 
> And the distiction between them is whether the bounds check is runtime
> or compile-time.
> 
> But this doesn't make much sense to me. Surely any Io resource that can
> provide compile-time checked io can also provide runtime-checked io, so
> maybe IoFallible should extend IoInfallible?

IO is generally something that can fail, so this whole idea of infallible
IO is making me uneasy.

I understand that we're trying to wrap it up into a bound device, but
bound devices are all about whether or not the driver lifetime is OK,
not so much about IO.

For PCIe, it is still possible for the device to fall off of the bus, and 
in that case you'll usually see 0xFFFF_FFFF returned from PCIe reads. The
Open RM driver has sprinkled around checks for this value (not fun, I
know), and Danilo hinted elsewhere that bound-ness requires not getting
these, so maybe that suffices. But it means that Rust will be "interesting"
here, because falling off the bus means that there will be a time window in
which the IO is, in fact, fallible.

Other IO subsystems can also get IO errors, too.

I wonder if we should just provide IoFallible? (It could check for the
0xFFFF_FFFF case, for example, which is helpful to simplify the caller.)

Again, it feels *really* odd to claim infallibility on something that,
almost (but not quite) by it's very nature is going to generate errors
at times.

> 
> And why are these separate traits at all? Why not support both
> compile-time and runtime-checked IO always?
> 
> I noticed also that the trait does not have methods for 64-bit writes,
> and that these are left as inherent methods on Mmio.
> 
> The traits that would make sense to me are these:
> 
> * Io
> * Io64: Io
> 
> where Io provides everything the three traits you have now provides, and
> Io64 provides the 64-bit operations. That way, everything needs to
> support operations of various sizes with both compile-time and
> runtime-checked bounds, but types may opt-in to providing 64-bit ops.
> 
> Thoughts?
> 
> Alice

thanks,
-- 
John Hubbard

Re: [PATCH v6 RESEND 4/7] rust: io: factor common I/O helpers into Io trait

[Danilo Krummrich]

On Tue Nov 18, 2025 at 11:44 AM NZDT, John Hubbard wrote:
> IO is generally something that can fail, so this whole idea of infallible
> IO is making me uneasy.
>
> I understand that we're trying to wrap it up into a bound device, but
> bound devices are all about whether or not the driver lifetime is OK,
> not so much about IO.

That is correct, device context states are about driver lifetime. However, it is
at least related, see below.

> For PCIe, it is still possible for the device to fall off of the bus, and 
> in that case you'll usually see 0xFFFF_FFFF returned from PCIe reads. The
> Open RM driver has sprinkled around checks for this value (not fun, I
> know), and Danilo hinted elsewhere that bound-ness requires not getting
> these, so maybe that suffices. But it means that Rust will be "interesting"
> here, because falling off the bus means that there will be a time window in
> which the IO is, in fact, fallible.

The PCI configuration space accessors indeed check a flag that is set when the
device falls off the bus. However, it is not sufficient, since you still have a
period of time when the device fell off the bus where the flag isn't set yet and
the I/O accessor may still be used concurrently.

(If you look at C drivers you will note that almost none of the drivers actually
check the return value of the configuration space accessors; needless to say
MMIO ones don't even have the flag.)

Because of that, there is not a point in making all the I/O accessors fallible,
because you'd have to deal with false negatives anyways, i.e. check the read
value for plausibility, because the device could already be gone, while the flag
is not set yet.

Additionally, when the device fell off the bus the driver core will unbind the
driver, so the period where fallability would serve at least some purpose would
be very short anyways.

Instead, drivers have to be designed to be robust enough to deal with broken
data read from the bus.

> Other IO subsystems can also get IO errors, too.
>
> I wonder if we should just provide IoFallible? (It could check for the
> 0xFFFF_FFFF case, for example, which is helpful to simplify the caller.)

For some registers this could be an expected value, plus a device can fall off
the bus during a read was well, leaving you with broken data.

I don't think trying to make all I/O operations fallible is the way to go, it's
just unreliable to detect in the generic layer. Instead, drivers should perform
a plausibility check on the read values (which they have to do anyways).

Re: [PATCH v6 RESEND 4/7] rust: io: factor common I/O helpers into Io trait

[John Hubbard]

On 11/18/25 1:18 PM, Danilo Krummrich wrote:
> On Tue Nov 18, 2025 at 11:44 AM NZDT, John Hubbard wrote:
...
>> Other IO subsystems can also get IO errors, too.
>>
>> I wonder if we should just provide IoFallible? (It could check for the
>> 0xFFFF_FFFF case, for example, which is helpful to simplify the caller.)
> 
> For some registers this could be an expected value, plus a device can fall off
> the bus during a read was well, leaving you with broken data.
> 
> I don't think trying to make all I/O operations fallible is the way to go, it's
> just unreliable to detect in the generic layer. Instead, drivers should perform
> a plausibility check on the read values (which they have to do anyways).

OK, I feel more comfortable with the approach now, thanks.

In return, I'll resist making jokes about calling it IoMostlyFallible.
You're welcome. haha :)


thanks,
-- 
John Hubbard