[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-Id: <DDAEL8DQFWKX.1BSBDMMN9I5B0@kernel.org>
Date: Sun, 05 Oct 2025 14:56:08 +0200
From: "Danilo Krummrich" <dakr@...nel.org>
To: "Joel Fernandes" <joelagnelf@...dia.com>
Cc: <linux-kernel@...r.kernel.org>, <dri-devel@...ts.freedesktop.org>,
<nouveau@...ts.freedesktop.org>, <rust-for-linux@...r.kernel.org>,
<linux-pci@...r.kernel.org>, <acourbot@...dia.com>, "Alistair Popple"
<apopple@...dia.com>, "Miguel Ojeda" <ojeda@...nel.org>, "Alex Gaynor"
<alex.gaynor@...il.com>, "Boqun Feng" <boqun.feng@...il.com>, "Gary Guo"
<gary@...yguo.net>, Björn Roy Baron
<bjorn3_gh@...tonmail.com>, "Benno Lossin" <lossin@...nel.org>, "Andreas
Hindborg" <a.hindborg@...nel.org>, "Alice Ryhl" <aliceryhl@...gle.com>,
"Trevor Gross" <tmgross@...ch.edu>, "David Airlie" <airlied@...il.com>,
"Simona Vetter" <simona@...ll.ch>, "John Hubbard" <jhubbard@...dia.com>,
"Timur Tabi" <ttabi@...dia.com>, <joel@...lfernandes.org>, "Daniel Almeida"
<daniel.almeida@...labora.com>, "Bjorn Helgaas" <bhelgaas@...gle.com>,
Krzysztof Wilczyński <kwilczynski@...nel.org>
Subject: Re: [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@...dia.com>
> ---
> Previous patch was here:
> https://lore.kernel.org/all/20250910035415.381753-1-joelagnelf@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 flags
Please 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();
> + self
NIT: 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)
> + }
> }
Powered by blists - more mailing lists