Re: [PATCH v2 1/5] rust: pci: convert IrqVectorRegistration to a lifetime-managed owning type

["Gary Guo" <gary@garyguo.net>]

On Wed Aug 12, 2026 at 6:37 PM BST, Danilo Krummrich wrote:
> On Wed Aug 12, 2026 at 6:26 PM CEST, Gary Guo wrote:
>>>  /// IRQ type flags for PCI interrupt allocation.
>>>  #[derive(Debug, Clone, Copy)]
>>> @@ -78,6 +75,7 @@ const fn as_raw(self) -> u32 {
>>>  #[derive(Clone, Copy)]
>>>  pub struct IrqVector<'a> {
>>>      dev: &'a Device<Bound>,
>>> +    reg: &'a IrqVectorRegistration<'a>,
>>
>> The registration has a refence to the device so we don't need to keep both reg
>> and dev?
>
> In this patch dev is still needed for the TryInto impl, but a subsquent patch
> does remove it in favor of IrqRequest.
>
>>> +pub struct IrqVectorRegistration<'a> {
>>> +    dev: &'a Device<Bound>,
>>> +    count: NonZero<usize>,
>>
>> I wonder if it should be called "len" as I view this as a collection of IRQ
>> vectors.
>
> Either sounds good to me.
>
>>> +    #[inline]
>>> +    pub fn vector(&self, index: usize) -> Result<IrqVector<'_>> {
>>> +        if index >= self.count.get() {
>>> +            return Err(EINVAL);
>>> +        }
>>
>> Given that the error is for out-of-bound access only, perhaps return `Option`
>> like `get()` function of various containers?
>
> It's not really a case a driver would handle other than just follow it up with
> .ok_or(EINVAL)? anyways, so I'd like to keep that.

Well, I'd expect some drivers want to do `.vector(v).expect()` rather than just
propagating the error if `v` is a constant that is less than `min_vecs`..

Best,
Gary

>
> (Further consideration on a subsequent patch.)
>
>>> @@ -256,7 +249,21 @@ pub fn alloc_irq_vectors(
>>>          min_vecs: u32,
>>>          max_vecs: u32,
>>>          irq_types: IrqTypes,
>>> -    ) -> Result<RangeInclusive<IrqVector<'_>>> {
>>> -        IrqVectorRegistration::register(self, min_vecs, max_vecs, irq_types)
>>> +    ) -> Result<IrqVectorRegistration<'_>> {
>>> +        // SAFETY:
>>> +        // - `self.as_raw()` is guaranteed to be a valid pointer to a `struct pci_dev`
>>> +        //   by the type invariant of `Device`.
>>> +        // - `pci_alloc_irq_vectors` internally validates all other parameters
>>> +        //   and returns error codes.
>>> +        let ret = unsafe {
>>> +            bindings::pci_alloc_irq_vectors(self.as_raw(), min_vecs, max_vecs, irq_types.as_raw())
>>> +        };
>>> +
>>> +        to_result(ret)?;
>>> +
>>> +        let count = NonZero::new(ret as usize).ok_or(EINVAL)?;
>>
>> I don't think `ret` can ever be zero. `expect` or `new_unchecked()` perhaps?
>
> Correct, but I don't see a reason to BUG_ON() for this. A WARN_ON() makes sense,
> but I see this to be the job of the C API making the promise.
>
> We could use new_unchecked(), but since this method is fallible already and not
> a hot path, I don't think it's worth.