Re: [PATCH v2] rust: pci: Allocate and manage PCI interrupt vectors

[Joel Fernandes <joelagnelf@nvidia.com>]

Hi Danilo,

On 10/5/2025 8:56 AM, Danilo Krummrich wrote:
> 
>> +    /// 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.

Great idea, so paraphrasing for myself, your point is with the above code,
someone could theoretically do:

  1. Call new() directly on IrqVectorRegistration (bypassing alloc_irq_vectors()).
  2. Forget to call devres::register().
  3. Store the IrqVectorRegistration somewhere.
  4. Device gets unbound.
  5. Later when IrqVectorRegistration::drop() runs, it tries to free vectors on
a device that's gone.

Is that right?

So a better approach as you mentioned, is to do the devres registration during
the construction of the IrqVectorRegistration, so there's no way to do one
without the other. Did I get that right? Anyway great point and I have made this
change, thanks!

 - Joel