Re: [PATCH 03/17] rust: pci: expose the allocated interrupt type

[Danilo Krummrich]

On Sat Aug 8, 2026 at 5:11 AM CEST, John Hubbard wrote:
> diff --git a/rust/helpers/pci.c b/rust/helpers/pci.c
> index 4ebf256dff23..87ccd0cec69f 100644
> --- a/rust/helpers/pci.c
> +++ b/rust/helpers/pci.c
> @@ -24,6 +24,17 @@ __rust_helper bool rust_helper_dev_is_pci(const struct device *dev)
>  	return dev_is_pci(dev);
>  }
>  
> +__rust_helper unsigned int rust_helper_pci_irq_type(struct pci_dev *pdev)
> +{
> +	if (pdev->msix_enabled)
> +		return PCI_IRQ_MSIX;
> +
> +	if (pdev->msi_enabled)
> +		return PCI_IRQ_MSI;
> +
> +	return PCI_IRQ_INTX;
> +}

Rust helpers should only be transparent wrappers of existing functions / macros.

In this case this can be easily lifeted to include/linux/pci.h, as it should be
a useful addition in general.

On the one hand there's already open-coded variants of this in drivers (such as
in [1]), and on the other hand I think it is not that great that drivers access
fields like msix_enabled directly.

Related to that, msix_enabled and msi_enabled are fields within a C bitfield of
struct pci_device, so accessing this under just the Bound device context is
formally UB (though in practice it shouldn't be an issue).

However, this makes me notice that pci_alloc_irq_vectors() and
pci_free_irq_vectors() both mutate those fields.

Consequently, IrqVectorRegistration::register() is technically unsound by
requiring a Device<Bound> and instead has to require a Device<Core>, such that
the C bitfield access is protected by the device lock.

Now, I think that there's already fields in the struct pci_dev C bitfield, which
are not protected with the device lock (such as block_cfg_access or
ats_enabled), so this is already racy regardless.

However, even if that wouldn't be the case, pci_alloc_irq_vectors() has valid
use-cases outside of bus callbacks, i.e. where the device lock is not held, e.g.
in [2] where it is called from a work item during device recovery.

IOW, just using the Core is the wrong solution (and insufficient anyway); Bound
is the correct context, but we need to fix the C bitfield issue.

I've also reported this in [3] for the is_busmaster field and it led to the
patch in [4]. However, I still think that there's quite some more fields in the
C bitfield that should be converted to bitops.

We recently had a similar rework [5] in driver-core that I suggested for similar
reasons. While not every field would have actually needed bitops, I think it is
simpler to just use bitops and be safe.

[1] https://elixir.bootlin.com/linux/v7.1.7/source/drivers/net/ethernet/aquantia/atlantic/aq_pci_func.c#L196
[2] https://elixir.bootlin.com/linux/v7.1.7/source/drivers/net/ethernet/mellanox/mlx5/core/pci_irq.c#L773
[3] https://lore.kernel.org/all/DJOEYVBS17MJ.1YD3TNGQBWHNK@xxxxxxxxxx/
[4] https://lore.kernel.org/all/20260714-pci-dev-flags-v2-1-a1d7dc441cf3@xxxxxxxxxxx/
[5] https://lore.kernel.org/all/20260406232444.3117516-1-dianders@xxxxxxxxxxxx/

>      /// Resolves the vector at `index` to the Linux IRQ number that delivers it.
>      ///
>      /// # Errors
> @@ -177,9 +187,21 @@ fn register<'a>(

Currently this function still uses devres::register(), but we should change it
to return Self being constrained to the lifetime of the &Device<Bound>.

This way the IrqAllocation type goes away and the IrqType and cound can be
directly on the IrqVectorRegistration type.

It also allows drivers to explicitly manage the lifetime of an
IrqVectorRegistration, which is something typically used by net and block
drivers.

Note that this also requires a borrow chain where irq::Registration keeps the
pci::IrqVectorRegistration alive.

This could be done with adding a generic on IrqRequest which defaults to () for
non-PCI stuff.

If you prefer, I can also send a patch for this that you could incorporate into
your patch series, so it doesn't conflict.

Thanks,
Danilo

>          // `pci_alloc_irq_vectors` returns the number of vectors it allocated.
>          let count = NonZero::new(ret as u32).ok_or(EINVAL)?;
>  
> -        // INVARIANT: `pci_alloc_irq_vectors` allocated `count` vectors for `dev`, numbered
> -        // from 0.
> -        let vectors = IrqAllocation { dev, count };
> +        // SAFETY: `dev.as_raw()` is a valid pointer to a `struct pci_dev`.
> +        let irq_type = match unsafe { bindings::pci_irq_type(dev.as_raw()) } {
> +            bindings::PCI_IRQ_MSIX => IrqType::MsiX,
> +            bindings::PCI_IRQ_MSI => IrqType::Msi,
> +            // The helper returns `PCI_IRQ_INTX` when neither MSI nor MSI-X is enabled.
> +            _ => IrqType::Intx,
> +        };
> +
> +        // INVARIANT: `pci_alloc_irq_vectors` allocated `count` vectors of `irq_type` for `dev`,
> +        // numbered from 0.
> +        let vectors = IrqAllocation {
> +            dev,
> +            count,
> +            irq_type,
> +        };