Message-Id: <DCOZMX59W82I.1AH7XVW3RUX2D@kernel.org>
Date: Wed, 10 Sep 2025 10:47:05 +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] rust: pci: add PCI interrupt allocation and management
 support

On Wed Sep 10, 2025 at 5:54 AM CEST, Joel Fernandes wrote:
>  impl Device<device::Bound> {

The Bound context is not enough for some of the methods below, some of them
require the Core context, more below.

> +    /// Free all allocated IRQ vectors for this device.
> +    ///
> +    /// This should be called to release interrupt resources when they are no longer needed,
> +    /// during driver unbind or removal.
> +    pub fn free_irq_vectors(&self) {
> +        // SAFETY: `self.as_raw` is guaranteed to be a pointer to a valid `struct pci_dev`.
> +        // `pci_free_irq_vectors` is safe to call even if no vectors are currently allocated.
> +        unsafe { bindings::pci_free_irq_vectors(self.as_raw()) };
> +    }

This requires the Core context, but we should not provide this method at all to
begin with; it puts the burden on drivers to remember calling this.

Instead, alloc_irq_vectors() should register a devres object with
devres::register(), so this gets called automatically when the device is
unbound.

Note that a cleanup through devres is not in conflict with the Core context
requirement.

> +    /// Allocate IRQ vectors for this PCI device.
> +    ///
> +    /// 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.
> +    /// This is called during driver probe.
> +    ///
> +    /// # 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 the number of vectors successfully allocated, or an error if the allocation
> +    /// fails or cannot meet the minimum requirement.
> +    ///
> +    /// # Examples
> +    ///
> +    /// ```
> +    /// // Allocate using any available interrupt type in the order mentioned above.
> +    /// let nvecs = 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 nvecs = dev.alloc_irq_vectors(4, 16, msi_only)?;
> +    /// ```
> +    pub fn alloc_irq_vectors(
> +        &self,
> +        min_vecs: u32,
> +        max_vecs: u32,
> +        irq_types: IrqTypes,
> +    ) -> Result<u32> {
> +        // SAFETY: `self.as_raw` is guaranteed to be a pointer to a valid `struct pci_dev`.
> +        // `pci_alloc_irq_vectors` internally validates all parameters and returns error codes.
> +        let ret = unsafe {
> +            bindings::pci_alloc_irq_vectors(self.as_raw(), min_vecs, max_vecs, irq_types.raw())
> +        };
> +
> +        to_result(ret)?;
> +        Ok(ret as u32)
> +    }

This is only valid to be called from the Core context, as it modifies internal
fields of the inner struct device.

Also, it would be nice if it would return a new type that can serve as argument
for irq_vector(), such that we don't have to rely on random integers.

> +
> +    /// Get the Linux IRQ number for a specific vector.
> +    ///
> +    /// This is called during driver probe after successful IRQ allocation
> +    /// to obtain the IRQ numbers for registering interrupt handlers.
> +    ///
> +    /// # Arguments
> +    ///
> +    /// * `vector` - The vector index (0-based)
> +    ///
> +    /// # Returns
> +    ///
> +    /// Returns the Linux IRQ number for the specified vector, or an error if the vector
> +    /// index is invalid or no vectors are allocated.
> +    pub fn irq_vector(&self, vector: u32) -> Result<u32> {

This method is already staged for inclusion in v6.18 in driver-core-next. Please
make sure to base changes on top of the tree mentioned in the maintainers file,
driver-core in this case.

The signature of the existing method is:

	pub fn irq_vector(&self, index: u32) -> Result<IrqRequest<'_>>

We return an IrqRequest, which captures the IRQ number *and* the corresponding
device, such that you can't get the combination wrong.

Maybe it's worth looking at improving the index argument with a new type as
mentioned above.

> +        // SAFETY: `self.as_raw` is guaranteed to be a pointer to a valid `struct pci_dev`.
> +        let irq = unsafe { bindings::pci_irq_vector(self.as_raw(), vector) };
> +
> +        to_result(irq)?;
> +        Ok(irq as u32)
> +    }
>  }

Powered by blists - more mailing lists