Danilo Krummrich
2025-Oct-05 12:56 UTC
[PATCH v2] rust: pci: Allocate and manage PCI interrupt vectors
On Thu Oct 2, 2025 at 8:39 PM CEST, Joel Fernandes wrote:> Add support to PCI rust module to allocate, free and manage IRQ vectors. > Integrate with devres for managing the allocated resources. > > Signed-off-by: Joel Fernandes <joelagnelf at nvidia.com> > --- > Previous patch was here: > https://lore.kernel.org/all/20250910035415.381753-1-joelagnelf at nvidia.com/ > > rust/kernel/pci.rs | 199 ++++++++++++++++++++++++++++++++++++++++++--- > 1 file changed, 186 insertions(+), 13 deletions(-) > > diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs > index 78271bf88cea..f97a6a36cf5e 100644 > --- a/rust/kernel/pci.rs > +++ b/rust/kernel/pci.rs > @@ -6,8 +6,9 @@ > > use crate::{ > bindings, container_of, device, > + device::Bound, > device_id::{RawDeviceId, RawDeviceIdIndex}, > - devres::Devres, > + devres::{self, Devres}, > driver, > error::{from_result, to_result, Result}, > io::{Io, IoRaw}, > @@ -19,7 +20,7 @@ > }; > use core::{ > marker::PhantomData, > - ops::Deref, > + ops::{Deref, RangeInclusive}, > ptr::{addr_of_mut, NonNull}, > }; > use kernel::prelude::*; > @@ -28,6 +29,59 @@ > > pub use self::id::{Class, ClassMask, Vendor}; > > +/// IRQ type flags for PCI interrupt allocation. > +#[derive(Debug, Clone, Copy)] > +pub enum IrqType { > + /// Legacy INTx interrupts > + Legacy,Like Bjorn said, I'd go with INTx too, also given that the C define is PCI_IRQ_INTX.> + /// Message Signaled Interrupts (MSI) > + Msi, > + /// Extended Message Signaled Interrupts (MSI-X) > + MsiX, > +} > + > +impl IrqType { > + /// Convert to the corresponding kernel flagsPlease end with a period, here and in multiple other places.> + const fn as_raw(self) -> u32 { > + match self { > + IrqType::Legacy => 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(mut self, irq_type: IrqType) -> Self { > + self.0 |= irq_type.as_raw(); > + selfNIT: I'd probably write this as: Self(self.0 | irq_type.as_raw())> + } > + > + /// Get the raw flags value > + const fn as_raw(self) -> u32 { > + self.0 > + } > +} > + > /// An adapter for the registration of PCI drivers. > pub struct Adapter<T: Driver>(T); > > @@ -516,6 +570,76 @@ 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`. > + unsafe fn new(dev: &'a Device<Bound>, index: u32) -> Self { > + Self { dev, index } > + } > + > + /// Returns the raw vector index. > + fn index(&self) -> u32 { > + self.index > + } > +} > + > +/// 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. > +struct IrqVectorRegistration { > + dev: ARef<Device>, > +} > + > +impl IrqVectorRegistration { > + /// Allocate IRQ vectors for the given PCI device. > + /// > + /// Returns the registration object and a range of valid IRQ vectors. > + fn new<'a>( > + dev: &'a Device<Bound>, > + min_vecs: u32, > + max_vecs: u32, > + irq_types: IrqTypes, > + ) -> Result<(Self, 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 parameters and returns error codes."all other parameters"? Please also format multiple statements in a safety comment as list.> + 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: Vectors are 0-based, so valid indices are [0, count-1]. > + // pci_alloc_irq_vectors guarantees count >= min_vecs > 0, so count - 1 is valid.This is a justification why the range makes sense (which makes sense to keep as a separate comment), but it doesn't justify the safety requirement of IrqVector::new().> + let range = unsafe { IrqVector::new(dev, 0)..=IrqVector::new(dev, count - 1) }; > + > + Ok((Self { dev: dev.into() }, range)) > + } > +} > + > +impl Drop for IrqVectorRegistration { > + fn drop(&mut self) { > + // SAFETY: `self.dev` is a valid ARef to a `struct pci_dev` that has successfully > + // allocated IRQ vectors.The "successfully allocated IRQ vectors" part should be a type invariant. NIT: s/ARef/`ARef`/> + unsafe { bindings::pci_free_irq_vectors(self.dev.as_raw()) }; > + } > +}<snip>> + /// 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 > + /// > + /// ```ignore > + /// // Allocate using any available interrupt type in the order mentioned above. > + /// let vectors = dev.alloc_irq_vectors(1, 32, IrqTypes::all())?; > + /// > + /// // Allocate MSI or MSI-X only (no legacy interrupts) > + /// let msi_only = IrqTypes::default() > + /// .with(IrqType::Msi) > + /// .with(IrqType::MsiX); > + /// let vectors = dev.alloc_irq_vectors(4, 16, msi_only)?; > + /// ``` > + pub fn alloc_irq_vectors( > + &self, > + min_vecs: u32, > + max_vecs: u32, > + irq_types: IrqTypes, > + ) -> Result<RangeInclusive<IrqVector<'_>>> { > + let (irq_vecs, range) = IrqVectorRegistration::new(self, min_vecs, max_vecs, irq_types)?; > + > + devres::register(self.as_ref(), irq_vecs, GFP_KERNEL)?;If we move the call to devres::register() into IrqVectorRegistration::new() (which I'd call IrqVectorRegistration::register() then) we can enforce the guarantee that an IrqVectorRegistration must not out-live the device / driver binding internally.> + Ok(range) > + } > }