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

[John Hubbard <jhubbard@nvidia.com>]

On 8/9/26 6:24 AM, Danilo Krummrich wrote:
> 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.

Will do.

> 
> 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@kernel.org/
> [4] https://lore.kernel.org/all/20260714-pci-dev-flags-v2-1-a1d7dc441cf3@mailbox.org/
> [5] https://lore.kernel.org/all/20260406232444.3117516-1-dianders@chromium.org/

An interesting read, thanks for the write-up and the references!

OK, so I'll leave things using Device<Bound>.

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

Yes, please. Then my patches 2 and 3 collapse into a single patch that
adds count(), irq_type() and an index-to-IrqVector accessor. Or, they go
away entirely if you end up putting those on the type yourself.


thanks,
-- 
John Hubbard