Re: [PATCH v2 2/5] rust: pci: resolve IRQ in vector() and embed IrqRequest in IrqVector

[Danilo Krummrich]

On Wed Aug 12, 2026 at 8:09 PM CEST, Gary Guo wrote:
> On Wed Aug 12, 2026 at 6:44 PM BST, Danilo Krummrich wrote:
>> On Wed Aug 12, 2026 at 6:38 PM CEST, Gary Guo wrote:
>>> On Wed Aug 12, 2026 at 12:39 AM BST, Danilo Krummrich wrote:
>>>>      pub fn vector(&self, index: usize) -> Result<IrqVector<'_>> {
>>>>          if index >= self.count.get() {
>>>>              return Err(EINVAL);
>>>>          }
>>>>  
>>>> -        // SAFETY: `index` is within bounds of this registration's allocation, and `self.dev` is
>>>> -        // the device it was allocated from.
>>>> -        Ok(unsafe { IrqVector::new(self.dev, self, index as u32) })
>>>> +        // SAFETY: `self.dev.as_raw()` is a valid pointer to a `struct pci_dev`.
>>>> +        let irq = unsafe { bindings::pci_irq_vector(self.dev.as_raw(), index as u32) };
>>>> +        if irq < 0 {
>>>> +            return Err(Error::from_errno(irq));
>>>> +        }
>>>
>>> Correct me if I'm wrong, but I believe that it's impossible for `pci_irq_vector`
>>> once we have allocated vector and the index is in bounds. (If that's not the
>>> case, we should ideally fix that instead.)
>>
>> You are correct, as of now it is unreachable with the index check above.
>>
>>> So I think we should just `.expect()` on the error in `Into`.
>>
>> I don't agree with the conclusion; I don't want this code to rely on an
>> implementation detail of pci_irq_vector(), which (even though unlikely) could
>> theoretically change.
>
> I think this is expected use pattern of `pci_irq_vector`. Many C code don't
> check the return code at all. If we want to mirror what C code do, we can also
> just drop this error code check and rely on `irq as u32` below doing the correct
> thing.
>
> For both this and the EINVAL case for patch 1, my reasoning is that if the error
> is never going to happen, then the code shouldn't be written as if it does, as
> it will only add confusion to people reading the code.
>
> I view these essentially as invariants, just not spelled out because it's
> written in another language. In this case, basically you can say that
> `pci_irq_vector(dev, index)` being successful is an invariant of
> `IrqVectorRegistration` type.

All this only checks out if we keep open-coding the range check and therefore
rely on implementation details of pci_irq_vector().

>>
>> If we want to remove the redundancy, then we could maybe drop the index check
>> above.
>
> I think the index check should stay.

Again, this seems backwards, why would we want to open-code a check that
pci_irq_vector() already does and subsequently rely on this implementation
detail?

>>
>> (I also prefer IrqVector to be a new type over IrqRequest, as it also guarantees
>> type wise that a valid IrqVector will always transform into a valid IrqRequest.)