Message-ID: <20250910180955.GA598866@joelbox2>
Date: Wed, 10 Sep 2025 14:09:55 -0400
From: Joel Fernandes <joelagnelf@...dia.com>
To: Danilo Krummrich <dakr@...nel.org>
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 10:47:05AM +0200, Danilo Krummrich wrote:
> 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.

Actually my patch already does that, the diff format creates confusion. Some
of the below methods (like alloc) are in fact added to the device::Core
context.

> > +    /// 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.

Great idea, thanks I will try this out.
> 
> Note that a cleanup through devres is not in conflict with the Core context
> requirement.

Got it.

> > +    /// 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.

It is called from core context, the diff format confuses.
> 
> 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.

Makes sense, I will do that.

> > +
> > +    /// 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.

Ah Ok, thanks for pointing this out. I will rebase and reuse this.

thanks,

 - Joel

> 
> > +        // 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