Re: [PATCH 1/3] rust: pci: implement TryInto<IrqRequest<'a>> for IrqVector<'a>

["Danilo Krummrich" <dakr@kernel.org>]

On Fri Oct 17, 2025 at 12:24 AM CEST, Joel Fernandes wrote:
> On Wed, Oct 15, 2025 at 08:14:29PM +0200, Danilo Krummrich wrote:
>> Implement TryInto<IrqRequest<'a>> for IrqVector<'a> to directly convert
>> a pci::IrqVector into a generic IrqRequest, instead of taking the
>> indirection via an unrelated pci::Device method.
>> 
>> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
>> ---
>>  rust/kernel/pci.rs | 38 ++++++++++++++++++--------------------
>>  1 file changed, 18 insertions(+), 20 deletions(-)
>> 
>> diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
>> index d91ec9f008ae..c6b750047b2e 100644
>> --- a/rust/kernel/pci.rs
>> +++ b/rust/kernel/pci.rs
>> @@ -596,6 +596,20 @@ fn index(&self) -> u32 {
>>      }
>>  }
>>  
>> +impl<'a> TryInto<IrqRequest<'a>> for IrqVector<'a> {
>> +    type Error = Error;
>> +
>> +    fn try_into(self) -> Result<IrqRequest<'a>> {
>> +        // SAFETY: `self.as_raw` returns a valid pointer to a `struct pci_dev`.
>> +        let irq = unsafe { bindings::pci_irq_vector(self.dev.as_raw(), self.index()) };
>> +        if irq < 0 {
>> +            return Err(crate::error::Error::from_errno(irq));
>> +        }
>> +        // SAFETY: `irq` is guaranteed to be a valid IRQ number for `&self`.
>> +        Ok(unsafe { IrqRequest::new(self.dev.as_ref(), irq as u32) })
>> +    }
>> +}A
>
>
> Nice change, looks good to me but I do feel it is odd to 'convert' an
> IrqVector directly into a IrqRequest using TryInto (one is a device-relative
> vector index and the other holds the notion of an IRQ request).
>
> Instead, we should convert IrqVector into something like LinuxIrqNumber
> (using TryInto) because we're converting one number to another, and then pass
> that to a separate function to create the IrqRequest.

Well, IrqRequest is exactly that, a representation of an IRQ number. So, this is
already doing exactly that, converting one number to another:

	pub struct IrqRequest<'a> {
	    dev: &'a Device<Bound>,
	    irq: u32,
	}

(The reason this is called IrqRequest instead of IrqNumber is that the number is
an irrelevant implementation detail of how an IRQ is requested.)

	pub struct IrqVector<'a> {
	    dev: &'a Device<Bound>,
	    index: u32,
	}

So, what happens here is that we convert the vector index from IrqVector into
the irq number in IrqRequest.

> Or we can do both in a
> vector.make_request() function (which is basically this patch but not using
> TryInto).

See above, there is no "both", it's the same thing. :)

Regarding make_request() vs. TryInto, I think TryInto is the idiomatic thing to
do here: Both structures have the same layout, as in they both carry the
&Device<Bound> reference the corresponding number belongs to, plus the number
itself; the device reference is taken over, the number is converted.

> Actually even my original code had this oddity:
> The function irq_vector should have been called irq_request or something but
> instead was:
> pub fn irq_vector(&self, vector: IrqVector<'_>) -> Result<IrqRequest<'_>>
>
> I think we can incrementally improve this though, so LGTM.
>
> Reviewed-by: Joel Fernandes <joelagnelf@nvidia.com>