[PATCH 0/3] Rust PCI housekeeping

[PATCH 0/3] Rust PCI housekeeping

[Danilo Krummrich]

Some minor housekeeping:

- Implement TryInto<IrqRequest<'a>> for IrqVector<'a> to directly convert a
  pci::IrqVector into a generic IrqRequest, instead of taking the indirection
  via an unrelated pci::Device method.

- Besides that, move I/O and IRQ specific code into separate sub-modules to keep
  things organized.

Danilo Krummrich (3):
  rust: pci: implement TryInto<IrqRequest<'a>> for IrqVector<'a>
  rust: pci: move I/O infrastructure to separate file
  rust: pci: move IRQ infrastructure to separate file

 rust/kernel/pci.rs     | 365 +----------------------------------------
 rust/kernel/pci/io.rs  | 141 ++++++++++++++++
 rust/kernel/pci/irq.rs | 244 +++++++++++++++++++++++++++
 3 files changed, 389 insertions(+), 361 deletions(-)
 create mode 100644 rust/kernel/pci/io.rs
 create mode 100644 rust/kernel/pci/irq.rs


base-commit: 340ccc973544a6e7e331729bc4944603085cafab
-- 
2.51.0

[PATCH 1/3] rust: pci: implement TryInto<IrqRequest<'a>> for IrqVector<'a>

[Danilo Krummrich]

Implement TryInto<IrqRequest<'a>> for IrqVector<'a> to directly convert
a pci::IrqVector into a generic IrqRequest, instead of taking the
indirection via an unrelated pci::Device method.

Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 rust/kernel/pci.rs | 38 ++++++++++++++++++--------------------
 1 file changed, 18 insertions(+), 20 deletions(-)

diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
index d91ec9f008ae..c6b750047b2e 100644
--- a/rust/kernel/pci.rs
+++ b/rust/kernel/pci.rs
@@ -596,6 +596,20 @@ fn index(&self) -> u32 {
     }
 }
 
+impl<'a> TryInto<IrqRequest<'a>> for IrqVector<'a> {
+    type Error = Error;
+
+    fn try_into(self) -> Result<IrqRequest<'a>> {
+        // SAFETY: `self.as_raw` returns a valid pointer to a `struct pci_dev`.
+        let irq = unsafe { bindings::pci_irq_vector(self.dev.as_raw(), self.index()) };
+        if irq < 0 {
+            return Err(crate::error::Error::from_errno(irq));
+        }
+        // SAFETY: `irq` is guaranteed to be a valid IRQ number for `&self`.
+        Ok(unsafe { IrqRequest::new(self.dev.as_ref(), irq as u32) })
+    }
+}
+
 /// Represents an IRQ vector allocation for a PCI device.
 ///
 /// This type ensures that IRQ vectors are properly allocated and freed by
@@ -675,31 +689,15 @@ pub fn iomap_region<'a>(
         self.iomap_region_sized::<0>(bar, name)
     }
 
-    /// Returns an [`IrqRequest`] for the given IRQ vector.
-    pub fn irq_vector(&self, vector: IrqVector<'_>) -> Result<IrqRequest<'_>> {
-        // Verify that the vector belongs to this device.
-        if !core::ptr::eq(vector.dev.as_raw(), self.as_raw()) {
-            return Err(EINVAL);
-        }
-
-        // SAFETY: `self.as_raw` returns a valid pointer to a `struct pci_dev`.
-        let irq = unsafe { crate::bindings::pci_irq_vector(self.as_raw(), vector.index()) };
-        if irq < 0 {
-            return Err(crate::error::Error::from_errno(irq));
-        }
-        // SAFETY: `irq` is guaranteed to be a valid IRQ number for `&self`.
-        Ok(unsafe { IrqRequest::new(self.as_ref(), irq as u32) })
-    }
-
     /// Returns a [`kernel::irq::Registration`] for the given IRQ vector.
     pub fn request_irq<'a, T: crate::irq::Handler + 'static>(
         &'a self,
-        vector: IrqVector<'_>,
+        vector: IrqVector<'a>,
         flags: irq::Flags,
         name: &'static CStr,
         handler: impl PinInit<T, Error> + 'a,
     ) -> Result<impl PinInit<irq::Registration<T>, Error> + 'a> {
-        let request = self.irq_vector(vector)?;
+        let request = vector.try_into()?;
 
         Ok(irq::Registration::<T>::new(request, flags, name, handler))
     }
@@ -707,12 +705,12 @@ pub fn request_irq<'a, T: crate::irq::Handler + 'static>(
     /// Returns a [`kernel::irq::ThreadedRegistration`] for the given IRQ vector.
     pub fn request_threaded_irq<'a, T: crate::irq::ThreadedHandler + 'static>(
         &'a self,
-        vector: IrqVector<'_>,
+        vector: IrqVector<'a>,
         flags: irq::Flags,
         name: &'static CStr,
         handler: impl PinInit<T, Error> + 'a,
     ) -> Result<impl PinInit<irq::ThreadedRegistration<T>, Error> + 'a> {
-        let request = self.irq_vector(vector)?;
+        let request = vector.try_into()?;
 
         Ok(irq::ThreadedRegistration::<T>::new(
             request, flags, name, handler,
-- 
2.51.0

[PATCH 2/3] rust: pci: move I/O infrastructure to separate file

[Danilo Krummrich]

Move the PCI I/O infrastructure to a separate sub-module in order to
keep things organized.

Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 rust/kernel/pci.rs    | 133 ++-------------------------------------
 rust/kernel/pci/io.rs | 141 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 145 insertions(+), 129 deletions(-)
 create mode 100644 rust/kernel/pci/io.rs

diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
index c6b750047b2e..ed9d8619f944 100644
--- a/rust/kernel/pci.rs
+++ b/rust/kernel/pci.rs
@@ -8,10 +8,8 @@
     bindings, container_of, device,
     device::Bound,
     device_id::{RawDeviceId, RawDeviceIdIndex},
-    devres::{self, Devres},
-    driver,
+    devres, driver,
     error::{from_result, to_result, Result},
-    io::{Io, IoRaw},
     irq::{self, IrqRequest},
     str::CStr,
     sync::aref::ARef,
@@ -20,14 +18,16 @@
 };
 use core::{
     marker::PhantomData,
-    ops::{Deref, RangeInclusive},
+    ops::RangeInclusive,
     ptr::{addr_of_mut, NonNull},
 };
 use kernel::prelude::*;
 
 mod id;
+mod io;
 
 pub use self::id::{Class, ClassMask, Vendor};
+pub use self::io::Bar;
 
 /// IRQ type flags for PCI interrupt allocation.
 #[derive(Debug, Clone, Copy)]
@@ -358,112 +358,6 @@ pub struct Device<Ctx: device::DeviceContext = device::Normal>(
     PhantomData<Ctx>,
 );
 
-/// A PCI BAR to perform I/O-Operations on.
-///
-/// # Invariants
-///
-/// `Bar` always holds an `IoRaw` inststance that holds a valid pointer to the start of the I/O
-/// memory mapped PCI bar and its size.
-pub struct Bar<const SIZE: usize = 0> {
-    pdev: ARef<Device>,
-    io: IoRaw<SIZE>,
-    num: i32,
-}
-
-impl<const SIZE: usize> Bar<SIZE> {
-    fn new(pdev: &Device, num: u32, name: &CStr) -> Result<Self> {
-        let len = pdev.resource_len(num)?;
-        if len == 0 {
-            return Err(ENOMEM);
-        }
-
-        // Convert to `i32`, since that's what all the C bindings use.
-        let num = i32::try_from(num)?;
-
-        // SAFETY:
-        // `pdev` is valid by the invariants of `Device`.
-        // `num` is checked for validity by a previous call to `Device::resource_len`.
-        // `name` is always valid.
-        let ret = unsafe { bindings::pci_request_region(pdev.as_raw(), num, name.as_char_ptr()) };
-        if ret != 0 {
-            return Err(EBUSY);
-        }
-
-        // SAFETY:
-        // `pdev` is valid by the invariants of `Device`.
-        // `num` is checked for validity by a previous call to `Device::resource_len`.
-        // `name` is always valid.
-        let ioptr: usize = unsafe { bindings::pci_iomap(pdev.as_raw(), num, 0) } as usize;
-        if ioptr == 0 {
-            // SAFETY:
-            // `pdev` valid by the invariants of `Device`.
-            // `num` is checked for validity by a previous call to `Device::resource_len`.
-            unsafe { bindings::pci_release_region(pdev.as_raw(), num) };
-            return Err(ENOMEM);
-        }
-
-        let io = match IoRaw::new(ioptr, len as usize) {
-            Ok(io) => io,
-            Err(err) => {
-                // SAFETY:
-                // `pdev` is valid by the invariants of `Device`.
-                // `ioptr` is guaranteed to be the start of a valid I/O mapped memory region.
-                // `num` is checked for validity by a previous call to `Device::resource_len`.
-                unsafe { Self::do_release(pdev, ioptr, num) };
-                return Err(err);
-            }
-        };
-
-        Ok(Bar {
-            pdev: pdev.into(),
-            io,
-            num,
-        })
-    }
-
-    /// # Safety
-    ///
-    /// `ioptr` must be a valid pointer to the memory mapped PCI bar number `num`.
-    unsafe fn do_release(pdev: &Device, ioptr: usize, num: i32) {
-        // SAFETY:
-        // `pdev` is valid by the invariants of `Device`.
-        // `ioptr` is valid by the safety requirements.
-        // `num` is valid by the safety requirements.
-        unsafe {
-            bindings::pci_iounmap(pdev.as_raw(), ioptr as *mut c_void);
-            bindings::pci_release_region(pdev.as_raw(), num);
-        }
-    }
-
-    fn release(&self) {
-        // SAFETY: The safety requirements are guaranteed by the type invariant of `self.pdev`.
-        unsafe { Self::do_release(&self.pdev, self.io.addr(), self.num) };
-    }
-}
-
-impl Bar {
-    #[inline]
-    fn index_is_valid(index: u32) -> bool {
-        // A `struct pci_dev` owns an array of resources with at most `PCI_NUM_RESOURCES` entries.
-        index < bindings::PCI_NUM_RESOURCES
-    }
-}
-
-impl<const SIZE: usize> Drop for Bar<SIZE> {
-    fn drop(&mut self) {
-        self.release();
-    }
-}
-
-impl<const SIZE: usize> Deref for Bar<SIZE> {
-    type Target = Io<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) }
-    }
-}
-
 impl<Ctx: device::DeviceContext> Device<Ctx> {
     #[inline]
     fn as_raw(&self) -> *mut bindings::pci_dev {
@@ -670,25 +564,6 @@ fn drop(&mut self) {
 }
 
 impl Device<device::Bound> {
-    /// Mapps an entire PCI-BAR after performing a region-request on it. I/O operation bound checks
-    /// can be performed on compile time for offsets (plus the requested type size) < SIZE.
-    pub fn iomap_region_sized<'a, const SIZE: usize>(
-        &'a self,
-        bar: u32,
-        name: &'a CStr,
-    ) -> impl PinInit<Devres<Bar<SIZE>>, Error> + 'a {
-        Devres::new(self.as_ref(), Bar::<SIZE>::new(self, bar, name))
-    }
-
-    /// Mapps an entire PCI-BAR after performing a region-request on it.
-    pub fn iomap_region<'a>(
-        &'a self,
-        bar: u32,
-        name: &'a CStr,
-    ) -> impl PinInit<Devres<Bar>, Error> + 'a {
-        self.iomap_region_sized::<0>(bar, name)
-    }
-
     /// Returns a [`kernel::irq::Registration`] for the given IRQ vector.
     pub fn request_irq<'a, T: crate::irq::Handler + 'static>(
         &'a self,
diff --git a/rust/kernel/pci/io.rs b/rust/kernel/pci/io.rs
new file mode 100644
index 000000000000..65151a0a1a41
--- /dev/null
+++ b/rust/kernel/pci/io.rs
@@ -0,0 +1,141 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! PCI memory-mapped I/O infrastructure.
+
+use super::Device;
+use crate::{
+    bindings, device,
+    devres::Devres,
+    io::{Io, IoRaw},
+    str::CStr,
+    sync::aref::ARef,
+};
+use core::ops::Deref;
+use kernel::prelude::*;
+
+/// A PCI BAR to perform I/O-Operations on.
+///
+/// # Invariants
+///
+/// `Bar` always holds an `IoRaw` inststance that holds a valid pointer to the start of the I/O
+/// memory mapped PCI bar and its size.
+pub struct Bar<const SIZE: usize = 0> {
+    pdev: ARef<Device>,
+    io: IoRaw<SIZE>,
+    num: i32,
+}
+
+impl<const SIZE: usize> Bar<SIZE> {
+    pub(super) fn new(pdev: &Device, num: u32, name: &CStr) -> Result<Self> {
+        let len = pdev.resource_len(num)?;
+        if len == 0 {
+            return Err(ENOMEM);
+        }
+
+        // Convert to `i32`, since that's what all the C bindings use.
+        let num = i32::try_from(num)?;
+
+        // SAFETY:
+        // `pdev` is valid by the invariants of `Device`.
+        // `num` is checked for validity by a previous call to `Device::resource_len`.
+        // `name` is always valid.
+        let ret = unsafe { bindings::pci_request_region(pdev.as_raw(), num, name.as_char_ptr()) };
+        if ret != 0 {
+            return Err(EBUSY);
+        }
+
+        // SAFETY:
+        // `pdev` is valid by the invariants of `Device`.
+        // `num` is checked for validity by a previous call to `Device::resource_len`.
+        // `name` is always valid.
+        let ioptr: usize = unsafe { bindings::pci_iomap(pdev.as_raw(), num, 0) } as usize;
+        if ioptr == 0 {
+            // SAFETY:
+            // `pdev` valid by the invariants of `Device`.
+            // `num` is checked for validity by a previous call to `Device::resource_len`.
+            unsafe { bindings::pci_release_region(pdev.as_raw(), num) };
+            return Err(ENOMEM);
+        }
+
+        let io = match IoRaw::new(ioptr, len as usize) {
+            Ok(io) => io,
+            Err(err) => {
+                // SAFETY:
+                // `pdev` is valid by the invariants of `Device`.
+                // `ioptr` is guaranteed to be the start of a valid I/O mapped memory region.
+                // `num` is checked for validity by a previous call to `Device::resource_len`.
+                unsafe { Self::do_release(pdev, ioptr, num) };
+                return Err(err);
+            }
+        };
+
+        Ok(Bar {
+            pdev: pdev.into(),
+            io,
+            num,
+        })
+    }
+
+    /// # Safety
+    ///
+    /// `ioptr` must be a valid pointer to the memory mapped PCI bar number `num`.
+    unsafe fn do_release(pdev: &Device, ioptr: usize, num: i32) {
+        // SAFETY:
+        // `pdev` is valid by the invariants of `Device`.
+        // `ioptr` is valid by the safety requirements.
+        // `num` is valid by the safety requirements.
+        unsafe {
+            bindings::pci_iounmap(pdev.as_raw(), ioptr as *mut c_void);
+            bindings::pci_release_region(pdev.as_raw(), num);
+        }
+    }
+
+    fn release(&self) {
+        // SAFETY: The safety requirements are guaranteed by the type invariant of `self.pdev`.
+        unsafe { Self::do_release(&self.pdev, self.io.addr(), self.num) };
+    }
+}
+
+impl Bar {
+    #[inline]
+    pub(super) fn index_is_valid(index: u32) -> bool {
+        // A `struct pci_dev` owns an array of resources with at most `PCI_NUM_RESOURCES` entries.
+        index < bindings::PCI_NUM_RESOURCES
+    }
+}
+
+impl<const SIZE: usize> Drop for Bar<SIZE> {
+    fn drop(&mut self) {
+        self.release();
+    }
+}
+
+impl<const SIZE: usize> Deref for Bar<SIZE> {
+    type Target = Io<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) }
+    }
+}
+
+impl Device<device::Bound> {
+    /// Mapps an entire PCI-BAR after performing a region-request on it. I/O operation bound checks
+    /// can be performed on compile time for offsets (plus the requested type size) < SIZE.
+    pub fn iomap_region_sized<'a, const SIZE: usize>(
+        &'a self,
+        bar: u32,
+        name: &'a CStr,
+    ) -> impl PinInit<Devres<Bar<SIZE>>, Error> + 'a {
+        Devres::new(self.as_ref(), Bar::<SIZE>::new(self, bar, name))
+    }
+
+    /// Mapps an entire PCI-BAR after performing a region-request on it.
+    pub fn iomap_region<'a>(
+        &'a self,
+        bar: u32,
+        name: &'a CStr,
+    ) -> impl PinInit<Devres<Bar>, Error> + 'a {
+        self.iomap_region_sized::<0>(bar, name)
+    }
+}
-- 
2.51.0

[PATCH 3/3] rust: pci: move IRQ infrastructure to separate file

[Danilo Krummrich]

Move the PCI interrupt infrastructure to a separate sub-module in order
to keep things organized.

Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 rust/kernel/pci.rs     | 236 +--------------------------------------
 rust/kernel/pci/irq.rs | 244 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 247 insertions(+), 233 deletions(-)
 create mode 100644 rust/kernel/pci/irq.rs

diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
index ed9d8619f944..ce612c9b7b56 100644
--- a/rust/kernel/pci.rs
+++ b/rust/kernel/pci.rs
@@ -6,80 +6,26 @@
 
 use crate::{
     bindings, container_of, device,
-    device::Bound,
     device_id::{RawDeviceId, RawDeviceIdIndex},
-    devres, driver,
+    driver,
     error::{from_result, to_result, Result},
-    irq::{self, IrqRequest},
     str::CStr,
-    sync::aref::ARef,
     types::Opaque,
     ThisModule,
 };
 use core::{
     marker::PhantomData,
-    ops::RangeInclusive,
     ptr::{addr_of_mut, NonNull},
 };
 use kernel::prelude::*;
 
 mod id;
 mod io;
+mod irq;
 
 pub use self::id::{Class, ClassMask, Vendor};
 pub use self::io::Bar;
-
-/// IRQ type flags for PCI interrupt allocation.
-#[derive(Debug, Clone, Copy)]
-pub enum IrqType {
-    /// INTx interrupts.
-    Intx,
-    /// Message Signaled Interrupts (MSI).
-    Msi,
-    /// Extended Message Signaled Interrupts (MSI-X).
-    MsiX,
-}
-
-impl IrqType {
-    /// Convert to the corresponding kernel flags.
-    const fn as_raw(self) -> u32 {
-        match self {
-            IrqType::Intx => bindings::PCI_IRQ_INTX,
-            IrqType::Msi => bindings::PCI_IRQ_MSI,
-            IrqType::MsiX => bindings::PCI_IRQ_MSIX,
-        }
-    }
-}
-
-/// Set of IRQ types that can be used for PCI interrupt allocation.
-#[derive(Debug, Clone, Copy, Default)]
-pub struct IrqTypes(u32);
-
-impl IrqTypes {
-    /// Create a set containing all IRQ types (MSI-X, MSI, and Legacy).
-    pub const fn all() -> Self {
-        Self(bindings::PCI_IRQ_ALL_TYPES)
-    }
-
-    /// Build a set of IRQ types.
-    ///
-    /// # Examples
-    ///
-    /// ```ignore
-    /// // Create a set with only MSI and MSI-X (no legacy interrupts).
-    /// let msi_only = IrqTypes::default()
-    ///     .with(IrqType::Msi)
-    ///     .with(IrqType::MsiX);
-    /// ```
-    pub const fn with(self, irq_type: IrqType) -> Self {
-        Self(self.0 | irq_type.as_raw())
-    }
-
-    /// Get the raw flags value.
-    const fn as_raw(self) -> u32 {
-        self.0
-    }
-}
+pub use self::irq::{IrqType, IrqTypes, IrqVector};
 
 /// An adapter for the registration of PCI drivers.
 pub struct Adapter<T: Driver>(T);
@@ -463,182 +409,6 @@ pub fn pci_class(&self) -> Class {
     }
 }
 
-/// Represents an allocated IRQ vector for a specific PCI device.
-///
-/// This type ties an IRQ vector to the device it was allocated for,
-/// ensuring the vector is only used with the correct device.
-#[derive(Clone, Copy)]
-pub struct IrqVector<'a> {
-    dev: &'a Device<Bound>,
-    index: u32,
-}
-
-impl<'a> IrqVector<'a> {
-    /// Creates a new [`IrqVector`] for the given device and index.
-    ///
-    /// # Safety
-    ///
-    /// - `index` must be a valid IRQ vector index for `dev`.
-    /// - `dev` must point to a [`Device`] that has successfully allocated IRQ vectors.
-    unsafe fn new(dev: &'a Device<Bound>, index: u32) -> Self {
-        Self { dev, index }
-    }
-
-    /// Returns the raw vector index.
-    fn index(&self) -> u32 {
-        self.index
-    }
-}
-
-impl<'a> TryInto<IrqRequest<'a>> for IrqVector<'a> {
-    type Error = Error;
-
-    fn try_into(self) -> Result<IrqRequest<'a>> {
-        // SAFETY: `self.as_raw` returns a valid pointer to a `struct pci_dev`.
-        let irq = unsafe { bindings::pci_irq_vector(self.dev.as_raw(), self.index()) };
-        if irq < 0 {
-            return Err(crate::error::Error::from_errno(irq));
-        }
-        // SAFETY: `irq` is guaranteed to be a valid IRQ number for `&self`.
-        Ok(unsafe { IrqRequest::new(self.dev.as_ref(), irq as u32) })
-    }
-}
-
-/// Represents an IRQ vector allocation for a PCI device.
-///
-/// This type ensures that IRQ vectors are properly allocated and freed by
-/// tying the allocation to the lifetime of this registration object.
-///
-/// # Invariants
-///
-/// The [`Device`] has successfully allocated IRQ vectors.
-struct IrqVectorRegistration {
-    dev: ARef<Device>,
-}
-
-impl IrqVectorRegistration {
-    /// Allocate and register IRQ vectors for the given PCI device.
-    ///
-    /// Allocates IRQ vectors and registers them with devres for automatic cleanup.
-    /// Returns a range of valid IRQ vectors.
-    fn register<'a>(
-        dev: &'a Device<Bound>,
-        min_vecs: u32,
-        max_vecs: u32,
-        irq_types: IrqTypes,
-    ) -> Result<RangeInclusive<IrqVector<'a>>> {
-        // SAFETY:
-        // - `dev.as_raw()` is guaranteed to be a valid pointer to a `struct pci_dev`
-        //   by the type invariant of `Device`.
-        // - `pci_alloc_irq_vectors` internally validates all other parameters
-        //   and returns error codes.
-        let ret = unsafe {
-            bindings::pci_alloc_irq_vectors(dev.as_raw(), min_vecs, max_vecs, irq_types.as_raw())
-        };
-
-        to_result(ret)?;
-        let count = ret as u32;
-
-        // SAFETY:
-        // - `pci_alloc_irq_vectors` returns the number of allocated vectors on success.
-        // - Vectors are 0-based, so valid indices are [0, count-1].
-        // - `pci_alloc_irq_vectors` guarantees `count >= min_vecs > 0`, so both `0` and
-        //   `count - 1` are valid IRQ vector indices for `dev`.
-        let range = unsafe { IrqVector::new(dev, 0)..=IrqVector::new(dev, count - 1) };
-
-        // INVARIANT: The IRQ vector allocation for `dev` above was successful.
-        let irq_vecs = Self { dev: dev.into() };
-        devres::register(dev.as_ref(), irq_vecs, GFP_KERNEL)?;
-
-        Ok(range)
-    }
-}
-
-impl Drop for IrqVectorRegistration {
-    fn drop(&mut self) {
-        // SAFETY:
-        // - By the type invariant, `self.dev.as_raw()` is a valid pointer to a `struct pci_dev`.
-        // - `self.dev` has successfully allocated IRQ vectors.
-        unsafe { bindings::pci_free_irq_vectors(self.dev.as_raw()) };
-    }
-}
-
-impl Device<device::Bound> {
-    /// Returns a [`kernel::irq::Registration`] for the given IRQ vector.
-    pub fn request_irq<'a, T: crate::irq::Handler + 'static>(
-        &'a self,
-        vector: IrqVector<'a>,
-        flags: irq::Flags,
-        name: &'static CStr,
-        handler: impl PinInit<T, Error> + 'a,
-    ) -> Result<impl PinInit<irq::Registration<T>, Error> + 'a> {
-        let request = vector.try_into()?;
-
-        Ok(irq::Registration::<T>::new(request, flags, name, handler))
-    }
-
-    /// Returns a [`kernel::irq::ThreadedRegistration`] for the given IRQ vector.
-    pub fn request_threaded_irq<'a, T: crate::irq::ThreadedHandler + 'static>(
-        &'a self,
-        vector: IrqVector<'a>,
-        flags: irq::Flags,
-        name: &'static CStr,
-        handler: impl PinInit<T, Error> + 'a,
-    ) -> Result<impl PinInit<irq::ThreadedRegistration<T>, Error> + 'a> {
-        let request = vector.try_into()?;
-
-        Ok(irq::ThreadedRegistration::<T>::new(
-            request, flags, name, handler,
-        ))
-    }
-
-    /// Allocate IRQ vectors for this PCI device with automatic cleanup.
-    ///
-    /// Allocates between `min_vecs` and `max_vecs` interrupt vectors for the device.
-    /// The allocation will use MSI-X, MSI, or legacy interrupts based on the `irq_types`
-    /// parameter and hardware capabilities. When multiple types are specified, the kernel
-    /// will try them in order of preference: MSI-X first, then MSI, then legacy interrupts.
-    ///
-    /// The allocated vectors are automatically freed when the device is unbound, using the
-    /// devres (device resource management) system.
-    ///
-    /// # Arguments
-    ///
-    /// * `min_vecs` - Minimum number of vectors required.
-    /// * `max_vecs` - Maximum number of vectors to allocate.
-    /// * `irq_types` - Types of interrupts that can be used.
-    ///
-    /// # Returns
-    ///
-    /// Returns a range of IRQ vectors that were successfully allocated, or an error if the
-    /// allocation fails or cannot meet the minimum requirement.
-    ///
-    /// # Examples
-    ///
-    /// ```
-    /// # use kernel::{ device::Bound, pci};
-    /// # fn no_run(dev: &pci::Device<Bound>) -> Result {
-    /// // Allocate using any available interrupt type in the order mentioned above.
-    /// let vectors = dev.alloc_irq_vectors(1, 32, pci::IrqTypes::all())?;
-    ///
-    /// // Allocate MSI or MSI-X only (no legacy interrupts).
-    /// let msi_only = pci::IrqTypes::default()
-    ///     .with(pci::IrqType::Msi)
-    ///     .with(pci::IrqType::MsiX);
-    /// let vectors = dev.alloc_irq_vectors(4, 16, msi_only)?;
-    /// # Ok(())
-    /// # }
-    /// ```
-    pub fn alloc_irq_vectors(
-        &self,
-        min_vecs: u32,
-        max_vecs: u32,
-        irq_types: IrqTypes,
-    ) -> Result<RangeInclusive<IrqVector<'_>>> {
-        IrqVectorRegistration::register(self, min_vecs, max_vecs, irq_types)
-    }
-}
-
 impl Device<device::Core> {
     /// Enable memory resources for this device.
     pub fn enable_device_mem(&self) -> Result {
diff --git a/rust/kernel/pci/irq.rs b/rust/kernel/pci/irq.rs
new file mode 100644
index 000000000000..77235c271876
--- /dev/null
+++ b/rust/kernel/pci/irq.rs
@@ -0,0 +1,244 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! PCI interrupt infrastructure.
+
+use super::Device;
+use crate::{
+    bindings, device,
+    device::Bound,
+    devres,
+    error::{to_result, Result},
+    irq::{self, IrqRequest},
+    str::CStr,
+    sync::aref::ARef,
+};
+use core::ops::RangeInclusive;
+use kernel::prelude::*;
+
+/// IRQ type flags for PCI interrupt allocation.
+#[derive(Debug, Clone, Copy)]
+pub enum IrqType {
+    /// INTx interrupts.
+    Intx,
+    /// Message Signaled Interrupts (MSI).
+    Msi,
+    /// Extended Message Signaled Interrupts (MSI-X).
+    MsiX,
+}
+
+impl IrqType {
+    /// Convert to the corresponding kernel flags.
+    const fn as_raw(self) -> u32 {
+        match self {
+            IrqType::Intx => bindings::PCI_IRQ_INTX,
+            IrqType::Msi => bindings::PCI_IRQ_MSI,
+            IrqType::MsiX => bindings::PCI_IRQ_MSIX,
+        }
+    }
+}
+
+/// Set of IRQ types that can be used for PCI interrupt allocation.
+#[derive(Debug, Clone, Copy, Default)]
+pub struct IrqTypes(u32);
+
+impl IrqTypes {
+    /// Create a set containing all IRQ types (MSI-X, MSI, and Legacy).
+    pub const fn all() -> Self {
+        Self(bindings::PCI_IRQ_ALL_TYPES)
+    }
+
+    /// Build a set of IRQ types.
+    ///
+    /// # Examples
+    ///
+    /// ```ignore
+    /// // Create a set with only MSI and MSI-X (no legacy interrupts).
+    /// let msi_only = IrqTypes::default()
+    ///     .with(IrqType::Msi)
+    ///     .with(IrqType::MsiX);
+    /// ```
+    pub const fn with(self, irq_type: IrqType) -> Self {
+        Self(self.0 | irq_type.as_raw())
+    }
+
+    /// Get the raw flags value.
+    const fn as_raw(self) -> u32 {
+        self.0
+    }
+}
+
+/// Represents an allocated IRQ vector for a specific PCI device.
+///
+/// This type ties an IRQ vector to the device it was allocated for,
+/// ensuring the vector is only used with the correct device.
+#[derive(Clone, Copy)]
+pub struct IrqVector<'a> {
+    dev: &'a Device<Bound>,
+    index: u32,
+}
+
+impl<'a> IrqVector<'a> {
+    /// Creates a new [`IrqVector`] for the given device and index.
+    ///
+    /// # Safety
+    ///
+    /// - `index` must be a valid IRQ vector index for `dev`.
+    /// - `dev` must point to a [`Device`] that has successfully allocated IRQ vectors.
+    unsafe fn new(dev: &'a Device<Bound>, index: u32) -> Self {
+        Self { dev, index }
+    }
+
+    /// Returns the raw vector index.
+    fn index(&self) -> u32 {
+        self.index
+    }
+}
+
+impl<'a> TryInto<IrqRequest<'a>> for IrqVector<'a> {
+    type Error = Error;
+
+    fn try_into(self) -> Result<IrqRequest<'a>> {
+        // SAFETY: `self.as_raw` returns a valid pointer to a `struct pci_dev`.
+        let irq = unsafe { bindings::pci_irq_vector(self.dev.as_raw(), self.index()) };
+        if irq < 0 {
+            return Err(crate::error::Error::from_errno(irq));
+        }
+        // SAFETY: `irq` is guaranteed to be a valid IRQ number for `&self`.
+        Ok(unsafe { IrqRequest::new(self.dev.as_ref(), irq as u32) })
+    }
+}
+
+/// Represents an IRQ vector allocation for a PCI device.
+///
+/// This type ensures that IRQ vectors are properly allocated and freed by
+/// tying the allocation to the lifetime of this registration object.
+///
+/// # Invariants
+///
+/// The [`Device`] has successfully allocated IRQ vectors.
+struct IrqVectorRegistration {
+    dev: ARef<Device>,
+}
+
+impl IrqVectorRegistration {
+    /// Allocate and register IRQ vectors for the given PCI device.
+    ///
+    /// Allocates IRQ vectors and registers them with devres for automatic cleanup.
+    /// Returns a range of valid IRQ vectors.
+    fn register<'a>(
+        dev: &'a Device<Bound>,
+        min_vecs: u32,
+        max_vecs: u32,
+        irq_types: IrqTypes,
+    ) -> Result<RangeInclusive<IrqVector<'a>>> {
+        // SAFETY:
+        // - `dev.as_raw()` is guaranteed to be a valid pointer to a `struct pci_dev`
+        //   by the type invariant of `Device`.
+        // - `pci_alloc_irq_vectors` internally validates all other parameters
+        //   and returns error codes.
+        let ret = unsafe {
+            bindings::pci_alloc_irq_vectors(dev.as_raw(), min_vecs, max_vecs, irq_types.as_raw())
+        };
+
+        to_result(ret)?;
+        let count = ret as u32;
+
+        // SAFETY:
+        // - `pci_alloc_irq_vectors` returns the number of allocated vectors on success.
+        // - Vectors are 0-based, so valid indices are [0, count-1].
+        // - `pci_alloc_irq_vectors` guarantees `count >= min_vecs > 0`, so both `0` and
+        //   `count - 1` are valid IRQ vector indices for `dev`.
+        let range = unsafe { IrqVector::new(dev, 0)..=IrqVector::new(dev, count - 1) };
+
+        // INVARIANT: The IRQ vector allocation for `dev` above was successful.
+        let irq_vecs = Self { dev: dev.into() };
+        devres::register(dev.as_ref(), irq_vecs, GFP_KERNEL)?;
+
+        Ok(range)
+    }
+}
+
+impl Drop for IrqVectorRegistration {
+    fn drop(&mut self) {
+        // SAFETY:
+        // - By the type invariant, `self.dev.as_raw()` is a valid pointer to a `struct pci_dev`.
+        // - `self.dev` has successfully allocated IRQ vectors.
+        unsafe { bindings::pci_free_irq_vectors(self.dev.as_raw()) };
+    }
+}
+
+impl Device<device::Bound> {
+    /// Returns a [`kernel::irq::Registration`] for the given IRQ vector.
+    pub fn request_irq<'a, T: crate::irq::Handler + 'static>(
+        &'a self,
+        vector: IrqVector<'a>,
+        flags: irq::Flags,
+        name: &'static CStr,
+        handler: impl PinInit<T, Error> + 'a,
+    ) -> Result<impl PinInit<irq::Registration<T>, Error> + 'a> {
+        let request = vector.try_into()?;
+
+        Ok(irq::Registration::<T>::new(request, flags, name, handler))
+    }
+
+    /// Returns a [`kernel::irq::ThreadedRegistration`] for the given IRQ vector.
+    pub fn request_threaded_irq<'a, T: crate::irq::ThreadedHandler + 'static>(
+        &'a self,
+        vector: IrqVector<'a>,
+        flags: irq::Flags,
+        name: &'static CStr,
+        handler: impl PinInit<T, Error> + 'a,
+    ) -> Result<impl PinInit<irq::ThreadedRegistration<T>, Error> + 'a> {
+        let request = vector.try_into()?;
+
+        Ok(irq::ThreadedRegistration::<T>::new(
+            request, flags, name, handler,
+        ))
+    }
+
+    /// Allocate IRQ vectors for this PCI device with automatic cleanup.
+    ///
+    /// Allocates between `min_vecs` and `max_vecs` interrupt vectors for the device.
+    /// The allocation will use MSI-X, MSI, or legacy interrupts based on the `irq_types`
+    /// parameter and hardware capabilities. When multiple types are specified, the kernel
+    /// will try them in order of preference: MSI-X first, then MSI, then legacy interrupts.
+    ///
+    /// The allocated vectors are automatically freed when the device is unbound, using the
+    /// devres (device resource management) system.
+    ///
+    /// # Arguments
+    ///
+    /// * `min_vecs` - Minimum number of vectors required.
+    /// * `max_vecs` - Maximum number of vectors to allocate.
+    /// * `irq_types` - Types of interrupts that can be used.
+    ///
+    /// # Returns
+    ///
+    /// Returns a range of IRQ vectors that were successfully allocated, or an error if the
+    /// allocation fails or cannot meet the minimum requirement.
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// # use kernel::{ device::Bound, pci};
+    /// # fn no_run(dev: &pci::Device<Bound>) -> Result {
+    /// // Allocate using any available interrupt type in the order mentioned above.
+    /// let vectors = dev.alloc_irq_vectors(1, 32, pci::IrqTypes::all())?;
+    ///
+    /// // Allocate MSI or MSI-X only (no legacy interrupts).
+    /// let msi_only = pci::IrqTypes::default()
+    ///     .with(pci::IrqType::Msi)
+    ///     .with(pci::IrqType::MsiX);
+    /// let vectors = dev.alloc_irq_vectors(4, 16, msi_only)?;
+    /// # Ok(())
+    /// # }
+    /// ```
+    pub fn alloc_irq_vectors(
+        &self,
+        min_vecs: u32,
+        max_vecs: u32,
+        irq_types: IrqTypes,
+    ) -> Result<RangeInclusive<IrqVector<'_>>> {
+        IrqVectorRegistration::register(self, min_vecs, max_vecs, irq_types)
+    }
+}
-- 
2.51.0

Re: [PATCH 2/3] rust: pci: move I/O infrastructure to separate file

[Bjorn Helgaas]

On Wed, Oct 15, 2025 at 08:14:30PM +0200, Danilo Krummrich wrote:
> Move the PCI I/O infrastructure to a separate sub-module in order to
> keep things organized.

> +++ b/rust/kernel/pci/io.rs

> +/// A PCI BAR to perform I/O-Operations on.
> ...
> +/// memory mapped PCI bar and its size.
> ...
> +    /// `ioptr` must be a valid pointer to the memory mapped PCI bar number `num`.

I know this is just a move, but "BAR" vs "bar" usage is inconsistent.
I think "BAR" is clearer in comments.

> +    /// Mapps an entire PCI-BAR after performing a region-request on it. I/O operation bound checks
> ...
> +    /// Mapps an entire PCI-BAR after performing a region-request on it.

Similarly, s/Mapps/Maps/ and s/PCI-BAR/PCI BAR/

Re: [PATCH 3/3] rust: pci: move IRQ infrastructure to separate file

[Bjorn Helgaas]

On Wed, Oct 15, 2025 at 08:14:31PM +0200, Danilo Krummrich wrote:
> Move the PCI interrupt infrastructure to a separate sub-module in order
> to keep things organized.

> +++ b/rust/kernel/pci/irq.rs

> +pub enum IrqType {
> +    /// INTx interrupts.
> +    Intx,
> +    /// Message Signaled Interrupts (MSI).
> +    Msi,
> +    /// Extended Message Signaled Interrupts (MSI-X).
> +    MsiX,
> +}

> +impl IrqTypes {
> +    /// Create a set containing all IRQ types (MSI-X, MSI, and Legacy).
> ...
> +    /// // Create a set with only MSI and MSI-X (no legacy interrupts).
> ...
> +    /// The allocation will use MSI-X, MSI, or legacy interrupts based on the `irq_types`
> +    /// parameter and hardware capabilities. When multiple types are specified, the kernel
> +    /// will try them in order of preference: MSI-X first, then MSI, then legacy interrupts.
> ...
> +    /// // Allocate MSI or MSI-X only (no legacy interrupts).

Again, just a move, but could s/Legacy/INTx/ to make them all match.

Re: [PATCH 2/3] rust: pci: move I/O infrastructure to separate file

[Danilo Krummrich]

On Thu Oct 16, 2025 at 12:58 AM CEST, Bjorn Helgaas wrote:
> On Wed, Oct 15, 2025 at 08:14:30PM +0200, Danilo Krummrich wrote:
>> Move the PCI I/O infrastructure to a separate sub-module in order to
>> keep things organized.
>
>> +++ b/rust/kernel/pci/io.rs
>
>> +/// A PCI BAR to perform I/O-Operations on.
>> ...
>> +/// memory mapped PCI bar and its size.
>> ...
>> +    /// `ioptr` must be a valid pointer to the memory mapped PCI bar number `num`.
>
> I know this is just a move, but "BAR" vs "bar" usage is inconsistent.
> I think "BAR" is clearer in comments.

Yes, I agree.

>> +    /// Mapps an entire PCI-BAR after performing a region-request on it. I/O operation bound checks
>> ...
>> +    /// Mapps an entire PCI-BAR after performing a region-request on it.
>
> Similarly, s/Mapps/Maps/ and s/PCI-BAR/PCI BAR/

Thanks for catching those!

I think we should fix those in a follow-up patch. Even for trivial things, I
prefer not to fix them with a code move.

Those could also be a candidate for the list of "good first issues" of the Rust
for Linux project [1].

(Same for your the "legacy vs. INTx" inconsistency you spotted in the other
patch.)

[1] https://github.com/Rust-for-Linux/linux/contribute

Re: [PATCH 1/3] rust: pci: implement TryInto<IrqRequest<'a>> for IrqVector<'a>

[Alice Ryhl]

On Wed, Oct 15, 2025 at 08:14:29PM +0200, Danilo Krummrich wrote:
> Implement TryInto<IrqRequest<'a>> for IrqVector<'a> to directly convert
> a pci::IrqVector into a generic IrqRequest, instead of taking the
> indirection via an unrelated pci::Device method.
> 
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> ---
>  rust/kernel/pci.rs | 38 ++++++++++++++++++--------------------
>  1 file changed, 18 insertions(+), 20 deletions(-)
> 
> diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
> index d91ec9f008ae..c6b750047b2e 100644
> --- a/rust/kernel/pci.rs
> +++ b/rust/kernel/pci.rs
> @@ -596,6 +596,20 @@ fn index(&self) -> u32 {
>      }
>  }
>  
> +impl<'a> TryInto<IrqRequest<'a>> for IrqVector<'a> {
> +    type Error = Error;
> +
> +    fn try_into(self) -> Result<IrqRequest<'a>> {
> +        // SAFETY: `self.as_raw` returns a valid pointer to a `struct pci_dev`.
> +        let irq = unsafe { bindings::pci_irq_vector(self.dev.as_raw(), self.index()) };
> +        if irq < 0 {
> +            return Err(crate::error::Error::from_errno(irq));
> +        }
> +        // SAFETY: `irq` is guaranteed to be a valid IRQ number for `&self`.
> +        Ok(unsafe { IrqRequest::new(self.dev.as_ref(), irq as u32) })
> +    }
> +}
> +
>  /// Represents an IRQ vector allocation for a PCI device.
>  ///
>  /// This type ensures that IRQ vectors are properly allocated and freed by
> @@ -675,31 +689,15 @@ pub fn iomap_region<'a>(
>          self.iomap_region_sized::<0>(bar, name)
>      }
>  
> -    /// Returns an [`IrqRequest`] for the given IRQ vector.
> -    pub fn irq_vector(&self, vector: IrqVector<'_>) -> Result<IrqRequest<'_>> {
> -        // Verify that the vector belongs to this device.
> -        if !core::ptr::eq(vector.dev.as_raw(), self.as_raw()) {
> -            return Err(EINVAL);
> -        }
> -
> -        // SAFETY: `self.as_raw` returns a valid pointer to a `struct pci_dev`.
> -        let irq = unsafe { crate::bindings::pci_irq_vector(self.as_raw(), vector.index()) };
> -        if irq < 0 {
> -            return Err(crate::error::Error::from_errno(irq));
> -        }
> -        // SAFETY: `irq` is guaranteed to be a valid IRQ number for `&self`.
> -        Ok(unsafe { IrqRequest::new(self.as_ref(), irq as u32) })
> -    }
> -
>      /// Returns a [`kernel::irq::Registration`] for the given IRQ vector.
>      pub fn request_irq<'a, T: crate::irq::Handler + 'static>(
>          &'a self,
> -        vector: IrqVector<'_>,
> +        vector: IrqVector<'a>,
>          flags: irq::Flags,
>          name: &'static CStr,
>          handler: impl PinInit<T, Error> + 'a,
>      ) -> Result<impl PinInit<irq::Registration<T>, Error> + 'a> {
> -        let request = self.irq_vector(vector)?;
> +        let request = vector.try_into()?;
>  
>          Ok(irq::Registration::<T>::new(request, flags, name, handler))
>      }
> @@ -707,12 +705,12 @@ pub fn request_irq<'a, T: crate::irq::Handler + 'static>(
>      /// Returns a [`kernel::irq::ThreadedRegistration`] for the given IRQ vector.
>      pub fn request_threaded_irq<'a, T: crate::irq::ThreadedHandler + 'static>(
>          &'a self,
> -        vector: IrqVector<'_>,
> +        vector: IrqVector<'a>,
>          flags: irq::Flags,
>          name: &'static CStr,
>          handler: impl PinInit<T, Error> + 'a,
>      ) -> Result<impl PinInit<irq::ThreadedRegistration<T>, Error> + 'a> {
> -        let request = self.irq_vector(vector)?;
> +        let request = vector.try_into()?;

The resulting change to the lifetime semantics is curious, but seems
right.

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

Re: [PATCH 2/3] rust: pci: move I/O infrastructure to separate file

[Bjorn Helgaas]

On Thu, Oct 16, 2025 at 02:34:57PM +0200, Danilo Krummrich wrote:
> On Thu Oct 16, 2025 at 12:58 AM CEST, Bjorn Helgaas wrote:
> > On Wed, Oct 15, 2025 at 08:14:30PM +0200, Danilo Krummrich wrote:
> >> Move the PCI I/O infrastructure to a separate sub-module in order to
> >> keep things organized.
> >
> >> +++ b/rust/kernel/pci/io.rs
> >
> >> +/// A PCI BAR to perform I/O-Operations on.
> >> ...
> >> +/// memory mapped PCI bar and its size.
> >> ...
> >> +    /// `ioptr` must be a valid pointer to the memory mapped PCI bar number `num`.
> >
> > I know this is just a move, but "BAR" vs "bar" usage is inconsistent.
> > I think "BAR" is clearer in comments.
> 
> Yes, I agree.
> 
> >> +    /// Mapps an entire PCI-BAR after performing a region-request on it. I/O operation bound checks
> >> ...
> >> +    /// Mapps an entire PCI-BAR after performing a region-request on it.
> >
> > Similarly, s/Mapps/Maps/ and s/PCI-BAR/PCI BAR/
> 
> Thanks for catching those!
> 
> I think we should fix those in a follow-up patch. Even for trivial things, I
> prefer not to fix them with a code move.

Makes sense, thanks!

Re: [PATCH 1/3] rust: pci: implement TryInto<IrqRequest<'a>> for IrqVector<'a>

[Danilo Krummrich]

On 10/16/25 5:01 PM, Alice Ryhl wrote:
> On Wed, Oct 15, 2025 at 08:14:29PM +0200, Danilo Krummrich wrote:
>> @@ -707,12 +705,12 @@ pub fn request_irq<'a, T: crate::irq::Handler + 'static>(
>>      /// Returns a [`kernel::irq::ThreadedRegistration`] for the given IRQ vector.
>>      pub fn request_threaded_irq<'a, T: crate::irq::ThreadedHandler + 'static>(
>>          &'a self,
>> -        vector: IrqVector<'_>,
>> +        vector: IrqVector<'a>,
>>          flags: irq::Flags,
>>          name: &'static CStr,
>>          handler: impl PinInit<T, Error> + 'a,
>>      ) -> Result<impl PinInit<irq::ThreadedRegistration<T>, Error> + 'a> {
>> -        let request = self.irq_vector(vector)?;
>> +        let request = vector.try_into()?;
> 
> The resulting change to the lifetime semantics is curious, but seems
> right.

Yes, IrqVector has to have the same lifetime as &self, but I was surprised that
with this change the compiler does require an explicit lifetime (even though I
think the explicit one is better anyways).
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>

Re: [PATCH 2/3] rust: pci: move I/O infrastructure to separate file

[Miguel Ojeda]

On Thu, Oct 16, 2025 at 2:35 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> Those could also be a candidate for the list of "good first issues" of the Rust
> for Linux project [1].

Indeed -- done as an example:

    https://github.com/Rust-for-Linux/linux/issues/1196

Bjorn and others: of course please feel free to create more.

Cheers,
Miguel

Re: [PATCH 1/3] rust: pci: implement TryInto<IrqRequest<'a>> for IrqVector<'a>

[Joel Fernandes]

On Wed, Oct 15, 2025 at 08:14:29PM +0200, Danilo Krummrich wrote:
> Implement TryInto<IrqRequest<'a>> for IrqVector<'a> to directly convert
> a pci::IrqVector into a generic IrqRequest, instead of taking the
> indirection via an unrelated pci::Device method.
> 
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> ---
>  rust/kernel/pci.rs | 38 ++++++++++++++++++--------------------
>  1 file changed, 18 insertions(+), 20 deletions(-)
> 
> diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
> index d91ec9f008ae..c6b750047b2e 100644
> --- a/rust/kernel/pci.rs
> +++ b/rust/kernel/pci.rs
> @@ -596,6 +596,20 @@ fn index(&self) -> u32 {
>      }
>  }
>  
> +impl<'a> TryInto<IrqRequest<'a>> for IrqVector<'a> {
> +    type Error = Error;
> +
> +    fn try_into(self) -> Result<IrqRequest<'a>> {
> +        // SAFETY: `self.as_raw` returns a valid pointer to a `struct pci_dev`.
> +        let irq = unsafe { bindings::pci_irq_vector(self.dev.as_raw(), self.index()) };
> +        if irq < 0 {
> +            return Err(crate::error::Error::from_errno(irq));
> +        }
> +        // SAFETY: `irq` is guaranteed to be a valid IRQ number for `&self`.
> +        Ok(unsafe { IrqRequest::new(self.dev.as_ref(), irq as u32) })
> +    }
> +}A


Nice change, looks good to me but I do feel it is odd to 'convert' an
IrqVector directly into a IrqRequest using TryInto (one is a device-relative
vector index and the other holds the notion of an IRQ request).

Instead, we should convert IrqVector into something like LinuxIrqNumber
(using TryInto) because we're converting one number to another, and then pass
that to a separate function to create the IrqRequest. Or we can do both in a
vector.make_request() function (which is basically this patch but not using
TryInto).

Actually even my original code had this oddity:
The function irq_vector should have been called irq_request or something but
instead was:
pub fn irq_vector(&self, vector: IrqVector<'_>) -> Result<IrqRequest<'_>>

I think we can incrementally improve this though, so LGTM.

Reviewed-by: Joel Fernandes <joelagnelf@nvidia.com>

thanks,

 - Joel


> +
>  /// Represents an IRQ vector allocation for a PCI device.
>  ///
>  /// This type ensures that IRQ vectors are properly allocated and freed by
> @@ -675,31 +689,15 @@ pub fn iomap_region<'a>(
>          self.iomap_region_sized::<0>(bar, name)
>      }
>  
> -    /// Returns an [`IrqRequest`] for the given IRQ vector.
> -    pub fn irq_vector(&self, vector: IrqVector<'_>) -> Result<IrqRequest<'_>> {
> -        // Verify that the vector belongs to this device.
> -        if !core::ptr::eq(vector.dev.as_raw(), self.as_raw()) {
> -            return Err(EINVAL);
> -        }
> -
> -        // SAFETY: `self.as_raw` returns a valid pointer to a `struct pci_dev`.
> -        let irq = unsafe { crate::bindings::pci_irq_vector(self.as_raw(), vector.index()) };
> -        if irq < 0 {
> -            return Err(crate::error::Error::from_errno(irq));
> -        }
> -        // SAFETY: `irq` is guaranteed to be a valid IRQ number for `&self`.
> -        Ok(unsafe { IrqRequest::new(self.as_ref(), irq as u32) })
> -    }
> -
>      /// Returns a [`kernel::irq::Registration`] for the given IRQ vector.
>      pub fn request_irq<'a, T: crate::irq::Handler + 'static>(
>          &'a self,
> -        vector: IrqVector<'_>,
> +        vector: IrqVector<'a>,
>          flags: irq::Flags,
>          name: &'static CStr,
>          handler: impl PinInit<T, Error> + 'a,
>      ) -> Result<impl PinInit<irq::Registration<T>, Error> + 'a> {
> -        let request = self.irq_vector(vector)?;
> +        let request = vector.try_into()?;
>  
>          Ok(irq::Registration::<T>::new(request, flags, name, handler))
>      }
> @@ -707,12 +705,12 @@ pub fn request_irq<'a, T: crate::irq::Handler + 'static>(
>      /// Returns a [`kernel::irq::ThreadedRegistration`] for the given IRQ vector.
>      pub fn request_threaded_irq<'a, T: crate::irq::ThreadedHandler + 'static>(
>          &'a self,
> -        vector: IrqVector<'_>,
> +        vector: IrqVector<'a>,
>          flags: irq::Flags,
>          name: &'static CStr,
>          handler: impl PinInit<T, Error> + 'a,
>      ) -> Result<impl PinInit<irq::ThreadedRegistration<T>, Error> + 'a> {
> -        let request = self.irq_vector(vector)?;
> +        let request = vector.try_into()?;
>  
>          Ok(irq::ThreadedRegistration::<T>::new(
>              request, flags, name, handler,
> -- 
> 2.51.0
> 

Re: [PATCH 1/3] rust: pci: implement TryInto<IrqRequest<'a>> for IrqVector<'a>

[Danilo Krummrich]

On Fri Oct 17, 2025 at 12:24 AM CEST, Joel Fernandes wrote:
> On Wed, Oct 15, 2025 at 08:14:29PM +0200, Danilo Krummrich wrote:
>> Implement TryInto<IrqRequest<'a>> for IrqVector<'a> to directly convert
>> a pci::IrqVector into a generic IrqRequest, instead of taking the
>> indirection via an unrelated pci::Device method.
>> 
>> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
>> ---
>>  rust/kernel/pci.rs | 38 ++++++++++++++++++--------------------
>>  1 file changed, 18 insertions(+), 20 deletions(-)
>> 
>> diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
>> index d91ec9f008ae..c6b750047b2e 100644
>> --- a/rust/kernel/pci.rs
>> +++ b/rust/kernel/pci.rs
>> @@ -596,6 +596,20 @@ fn index(&self) -> u32 {
>>      }
>>  }
>>  
>> +impl<'a> TryInto<IrqRequest<'a>> for IrqVector<'a> {
>> +    type Error = Error;
>> +
>> +    fn try_into(self) -> Result<IrqRequest<'a>> {
>> +        // SAFETY: `self.as_raw` returns a valid pointer to a `struct pci_dev`.
>> +        let irq = unsafe { bindings::pci_irq_vector(self.dev.as_raw(), self.index()) };
>> +        if irq < 0 {
>> +            return Err(crate::error::Error::from_errno(irq));
>> +        }
>> +        // SAFETY: `irq` is guaranteed to be a valid IRQ number for `&self`.
>> +        Ok(unsafe { IrqRequest::new(self.dev.as_ref(), irq as u32) })
>> +    }
>> +}A
>
>
> Nice change, looks good to me but I do feel it is odd to 'convert' an
> IrqVector directly into a IrqRequest using TryInto (one is a device-relative
> vector index and the other holds the notion of an IRQ request).
>
> Instead, we should convert IrqVector into something like LinuxIrqNumber
> (using TryInto) because we're converting one number to another, and then pass
> that to a separate function to create the IrqRequest.

Well, IrqRequest is exactly that, a representation of an IRQ number. So, this is
already doing exactly that, converting one number to another:

	pub struct IrqRequest<'a> {
	    dev: &'a Device<Bound>,
	    irq: u32,
	}

(The reason this is called IrqRequest instead of IrqNumber is that the number is
an irrelevant implementation detail of how an IRQ is requested.)

	pub struct IrqVector<'a> {
	    dev: &'a Device<Bound>,
	    index: u32,
	}

So, what happens here is that we convert the vector index from IrqVector into
the irq number in IrqRequest.

> Or we can do both in a
> vector.make_request() function (which is basically this patch but not using
> TryInto).

See above, there is no "both", it's the same thing. :)

Regarding make_request() vs. TryInto, I think TryInto is the idiomatic thing to
do here: Both structures have the same layout, as in they both carry the
&Device<Bound> reference the corresponding number belongs to, plus the number
itself; the device reference is taken over, the number is converted.

> Actually even my original code had this oddity:
> The function irq_vector should have been called irq_request or something but
> instead was:
> pub fn irq_vector(&self, vector: IrqVector<'_>) -> Result<IrqRequest<'_>>
>
> I think we can incrementally improve this though, so LGTM.
>
> Reviewed-by: Joel Fernandes <joelagnelf@nvidia.com>

Re: [PATCH 1/3] rust: pci: implement TryInto<IrqRequest<'a>> for IrqVector<'a>

[Joel Fernandes]

On 10/16/2025 6:57 PM, Danilo Krummrich wrote:
> On Fri Oct 17, 2025 at 12:24 AM CEST, Joel Fernandes wrote:
>> On Wed, Oct 15, 2025 at 08:14:29PM +0200, Danilo Krummrich wrote:
>>> Implement TryInto<IrqRequest<'a>> for IrqVector<'a> to directly convert
>>> a pci::IrqVector into a generic IrqRequest, instead of taking the
>>> indirection via an unrelated pci::Device method.
>>>
>>> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
>>> ---
>>>  rust/kernel/pci.rs | 38 ++++++++++++++++++--------------------
>>>  1 file changed, 18 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
>>> index d91ec9f008ae..c6b750047b2e 100644
>>> --- a/rust/kernel/pci.rs
>>> +++ b/rust/kernel/pci.rs
>>> @@ -596,6 +596,20 @@ fn index(&self) -> u32 {
>>>      }
>>>  }
>>>  
>>> +impl<'a> TryInto<IrqRequest<'a>> for IrqVector<'a> {
>>> +    type Error = Error;
>>> +
>>> +    fn try_into(self) -> Result<IrqRequest<'a>> {
>>> +        // SAFETY: `self.as_raw` returns a valid pointer to a `struct pci_dev`.
>>> +        let irq = unsafe { bindings::pci_irq_vector(self.dev.as_raw(), self.index()) };
>>> +        if irq < 0 {
>>> +            return Err(crate::error::Error::from_errno(irq));
>>> +        }
>>> +        // SAFETY: `irq` is guaranteed to be a valid IRQ number for `&self`.
>>> +        Ok(unsafe { IrqRequest::new(self.dev.as_ref(), irq as u32) })
>>> +    }
>>> +}A
>>
>>
>> Nice change, looks good to me but I do feel it is odd to 'convert' an
>> IrqVector directly into a IrqRequest using TryInto (one is a device-relative
>> vector index and the other holds the notion of an IRQ request).
>>
>> Instead, we should convert IrqVector into something like LinuxIrqNumber
>> (using TryInto) because we're converting one number to another, and then pass
>> that to a separate function to create the IrqRequest.
> 
> Well, IrqRequest is exactly that, a representation of an IRQ number. So, this is
> already doing exactly that, converting one number to another:
> 
> 	pub struct IrqRequest<'a> {
> 	    dev: &'a Device<Bound>,
> 	    irq: u32,
> 	}
> 
> (The reason this is called IrqRequest instead of IrqNumber is that the number is
> an irrelevant implementation detail of how an IRQ is requested.)
> 
> 	pub struct IrqVector<'a> {
> 	    dev: &'a Device<Bound>,
> 	    index: u32,
> 	}
> 
> So, what happens here is that we convert the vector index from IrqVector into
> the irq number in IrqRequest.
Ah true, I think the naming "IrqRequest" throw me off. Sorry about that, so this
patch is good then :) Thanks,

 - Joel

Re: [PATCH 0/3] Rust PCI housekeeping

[Danilo Krummrich]

On Wed Oct 15, 2025 at 8:14 PM CEST, Danilo Krummrich wrote:
> Danilo Krummrich (3):
>   rust: pci: implement TryInto<IrqRequest<'a>> for IrqVector<'a>
>   rust: pci: move I/O infrastructure to separate file
>   rust: pci: move IRQ infrastructure to separate file

Applied to driver-core-testing, thanks!