Message-Id: <DFV4IJDQC2J6.1Q91JOAL6CJSG@kernel.org>
Date: Thu, 22 Jan 2026 13:40:22 +0100
From: "Danilo Krummrich" <dakr@...nel.org>
To: "Gary Guo" <gary@...yguo.net>
Cc: "Zhi Wang" <zhiw@...dia.com>, <rust-for-linux@...r.kernel.org>,
 <linux-pci@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
 <aliceryhl@...gle.com>, <bhelgaas@...gle.com>, <kwilczynski@...nel.org>,
 <ojeda@...nel.org>, <alex.gaynor@...il.com>, <boqun.feng@...il.com>,
 <bjorn3_gh@...tonmail.com>, <lossin@...nel.org>, <a.hindborg@...nel.org>,
 <tmgross@...ch.edu>, <markus.probst@...teo.de>, <helgaas@...nel.org>,
 <cjia@...dia.com>, <smitra@...dia.com>, <ankita@...dia.com>,
 <aniketa@...dia.com>, <kwankhede@...dia.com>, <targupta@...dia.com>,
 <acourbot@...dia.com>, <joelagnelf@...dia.com>, <jhubbard@...dia.com>,
 <zhiwang@...nel.org>, <daniel.almeida@...labora.com>
Subject: Re: [PATCH v12 4/5] rust: pci: add config space read/write support

On Thu Jan 22, 2026 at 12:59 PM CET, Gary Guo wrote:
> On Wed Jan 21, 2026 at 8:22 PM GMT, Zhi Wang wrote:
>> +    /// Returns the size of configuration space in bytes.
>> +    fn cfg_size(&self) -> Result<usize> {
>> +        // SAFETY: `self.as_raw` is a valid pointer to a `struct pci_dev`.
>> +        let size = unsafe { (*self.as_raw()).cfg_size };
>> +        match size {
>> +            256 | 4096 => Ok(size as usize),
>> +            _ => {
>> +                debug_assert!(false);
>> +                Err(EINVAL)
>> +            }
>> +        }
>> +    }
>
> This method is only invoked from maxsize, which turns error into `0`. Do apart
> from the debug assertion, the error code is pointless. I think this function
> should just return `usize` as it's specified in the device (we should trust the
> C side that the value is sensible).

That seems reasonable, but I also think we should keep the enum ConfigSpaceSize
we had before and call the new trait ConfigSpaceKind instead, such that this
method becomes:

	fn cfg_size(&self) -> ConfigSpaceSize;

> The check, as Alex mentioned, need to be done when ConfigSpace is created in
> the first place and is too late when you already hand out `Ok(ConfigSpace)`.

We need the check for config_space_extended(), but not for config_space(), as it
represents the minimum size, i.e. it's always valid.

Here's a diff of what I think this should look like on top of this series.

(@Zhi: If we all agree on the diff and nothing else comes up you don't need to
resend. :)

diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
index 9020959ce0c7..1d1a253e5d5d 100644
--- a/rust/kernel/pci.rs
+++ b/rust/kernel/pci.rs
@@ -42,6 +42,7 @@
 };
 pub use self::io::{
     Bar,
+    ConfigSpaceKind,
     ConfigSpaceSize,
     Extended,
     Normal, //
diff --git a/rust/kernel/pci/io.rs b/rust/kernel/pci/io.rs
index 39df41d0eaab..5dbdfe516418 100644
--- a/rust/kernel/pci/io.rs
+++ b/rust/kernel/pci/io.rs
@@ -24,6 +24,31 @@
     ops::Deref, //
 };

+/// 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)]
+#[derive(PartialEq)]
+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 into_raw(self) -> usize {
+        // CAST: PCI configuration space size is at most 4096 bytes, so the value always fits
+        // within `usize` without truncation or sign change.
+        self as usize
+    }
+}
+
 /// Marker type for normal (256-byte) PCI configuration space.
 pub struct Normal;

@@ -34,16 +59,16 @@
 ///
 /// This trait is implemented by [`Normal`] and [`Extended`] to provide
 /// compile-time knowledge of the configuration space size.
-pub trait ConfigSpaceSize {
+pub trait ConfigSpaceKind {
     /// The size of this configuration space in bytes.
     const SIZE: usize;
 }

-impl ConfigSpaceSize for Normal {
+impl ConfigSpaceKind for Normal {
     const SIZE: usize = 256;
 }

-impl ConfigSpaceSize for Extended {
+impl ConfigSpaceKind for Extended {
     const SIZE: usize = 4096;
 }

@@ -55,7 +80,7 @@ impl ConfigSpaceSize for Extended {
 /// The generic parameter `S` indicates the maximum size of the configuration space.
 /// Use [`Normal`] for 256-byte legacy configuration space or [`Extended`] for
 /// 4096-byte PCIe extended configuration space (default).
-pub struct ConfigSpace<'a, S: ConfigSpaceSize = Extended> {
+pub struct ConfigSpace<'a, S: ConfigSpaceKind = Extended> {
     pub(crate) pdev: &'a Device<device::Bound>,
     _marker: PhantomData<S>,
 }
@@ -118,11 +143,11 @@ macro_rules! call_config_write {
 }

 // PCI configuration space supports 8, 16, and 32-bit accesses.
-impl<'a, S: ConfigSpaceSize> IoCapable<u8> for ConfigSpace<'a, S> {}
-impl<'a, S: ConfigSpaceSize> IoCapable<u16> for ConfigSpace<'a, S> {}
-impl<'a, S: ConfigSpaceSize> IoCapable<u32> for ConfigSpace<'a, S> {}
+impl<'a, S: ConfigSpaceKind> IoCapable<u8> for ConfigSpace<'a, S> {}
+impl<'a, S: ConfigSpaceKind> IoCapable<u16> for ConfigSpace<'a, S> {}
+impl<'a, S: ConfigSpaceKind> IoCapable<u32> for ConfigSpace<'a, S> {}

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

     /// Returns the base address of the I/O region. It is always 0 for configuration space.
@@ -134,7 +159,7 @@ fn addr(&self) -> usize {
     /// Returns the maximum size of the configuration space.
     #[inline]
     fn maxsize(&self) -> usize {
-        self.pdev.cfg_size().map_or(0, |v| v)
+        self.pdev.cfg_size().into_raw()
     }

     // PCI configuration space does not support fallible operations.
@@ -150,7 +175,7 @@ fn maxsize(&self) -> usize {
 }

 /// Marker trait indicating ConfigSpace has a known size at compile time.
-impl<'a, S: ConfigSpaceSize> IoKnownSize for ConfigSpace<'a, S> {}
+impl<'a, S: ConfigSpaceKind> IoKnownSize for ConfigSpace<'a, S> {}

 /// A PCI BAR to perform I/O-Operations on.
 ///
@@ -281,29 +306,35 @@ pub fn iomap_region<'a>(
         self.iomap_region_sized::<0>(bar, name)
     }

-    /// Returns the size of configuration space in bytes.
-    fn cfg_size(&self) -> Result<usize> {
+    /// Returns the size of configuration space.
+    fn cfg_size(&self) -> 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 | 4096 => Ok(size as usize),
+            256 => ConfigSpaceSize::Normal,
+            4096 => ConfigSpaceSize::Extended,
             _ => {
-                debug_assert!(false);
-                Err(EINVAL)
+                // PANIC: The PCI subsystem only ever reports the configuration space size as either
+                // `ConfigSpaceSize::Normal` or `ConfigSpaceSize::Extended`.
+                unreachable!();
             }
         }
     }

     /// Return an initialized normal (256-byte) config space object.
-    pub fn config_space<'a>(&'a self) -> Result<ConfigSpace<'a, Normal>> {
-        Ok(ConfigSpace {
+    pub fn config_space<'a>(&'a self) -> ConfigSpace<'a, Normal> {
+        ConfigSpace {
             pdev: self,
             _marker: PhantomData,
-        })
+        }
     }

     /// Return an initialized extended (4096-byte) config space object.
     pub fn config_space_extended<'a>(&'a self) -> Result<ConfigSpace<'a, Extended>> {
+        if self.cfg_size() != ConfigSpaceSize::Extended {
+            return Err(EINVAL);
+        }
+
         Ok(ConfigSpace {
             pdev: self,
             _marker: PhantomData,
diff --git a/samples/rust/rust_driver_pci.rs b/samples/rust/rust_driver_pci.rs
index 1bc5bd1a8df5..8eea79e858a2 100644
--- a/samples/rust/rust_driver_pci.rs
+++ b/samples/rust/rust_driver_pci.rs
@@ -67,8 +67,8 @@ fn testdev(index: &TestIndex, bar: &Bar0) -> Result<u32> {
         Ok(bar.read32(Regs::COUNT))
     }

-    fn config_space(pdev: &pci::Device<Bound>) -> Result {
-        let config = pdev.config_space()?;
+    fn config_space(pdev: &pci::Device<Bound>) {
+        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.
@@ -89,8 +89,6 @@ fn config_space(pdev: &pci::Device<Bound>) -> Result {
             "pci-testdev config space read32 BAR 0: {:x}\n",
             config.read32(0x10)
         );
-
-        Ok(())
     }
 }

@@ -123,7 +121,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)?;
+                    Self::config_space(pdev);
                 },
                 pdev: pdev.into(),
             }))

Powered by blists - more mailing lists