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) })
+    }
+}
+
 /// Represents an IRQ vector allocation for a PCI device.
 ///
 /// This type ensures that IRQ vectors are properly allocated and freed by
@@ -675,31 +689,15 @@ pub fn iomap_region<'a>(
         self.iomap_region_sized::<0>(bar, name)
     }
 
-    /// Returns an [`IrqRequest`] for the given IRQ vector.
-    pub fn irq_vector(&self, vector: IrqVector<'_>) -> Result<IrqRequest<'_>> {
-        // Verify that the vector belongs to this device.
-        if !core::ptr::eq(vector.dev.as_raw(), self.as_raw()) {
-            return Err(EINVAL);
-        }
-
-        // SAFETY: `self.as_raw` returns a valid pointer to a `struct pci_dev`.
-        let irq = unsafe { crate::bindings::pci_irq_vector(self.as_raw(), vector.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.as_ref(), irq as u32) })
-    }
-
     /// Returns a [`kernel::irq::Registration`] for the given IRQ vector.
     pub fn request_irq<'a, T: crate::irq::Handler + 'static>(
         &'a self,
-        vector: IrqVector<'_>,
+        vector: IrqVector<'a>,
         flags: irq::Flags,
         name: &'static CStr,
         handler: impl PinInit<T, Error> + 'a,
     ) -> Result<impl PinInit<irq::Registration<T>, Error> + 'a> {
-        let request = self.irq_vector(vector)?;
+        let request = vector.try_into()?;
 
         Ok(irq::Registration::<T>::new(request, flags, name, handler))
     }
@@ -707,12 +705,12 @@ pub fn request_irq<'a, T: crate::irq::Handler + 'static>(
     /// Returns a [`kernel::irq::ThreadedRegistration`] for the given IRQ vector.
     pub fn request_threaded_irq<'a, T: crate::irq::ThreadedHandler + 'static>(
         &'a self,
-        vector: IrqVector<'_>,
+        vector: IrqVector<'a>,
         flags: irq::Flags,
         name: &'static CStr,
         handler: impl PinInit<T, Error> + 'a,
     ) -> Result<impl PinInit<irq::ThreadedRegistration<T>, Error> + 'a> {
-        let request = self.irq_vector(vector)?;
+        let request = vector.try_into()?;
 
         Ok(irq::ThreadedRegistration::<T>::new(
             request, flags, name, handler,
-- 
2.51.0

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. Or we can do both in a
vector.make_request() function (which is basically this patch but not using
TryInto).

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>

thanks,

 - Joel


> +
>  /// Represents an IRQ vector allocation for a PCI device.
>  ///
>  /// This type ensures that IRQ vectors are properly allocated and freed by
> @@ -675,31 +689,15 @@ pub fn iomap_region<'a>(
>          self.iomap_region_sized::<0>(bar, name)
>      }
>  
> -    /// Returns an [`IrqRequest`] for the given IRQ vector.
> -    pub fn irq_vector(&self, vector: IrqVector<'_>) -> Result<IrqRequest<'_>> {
> -        // Verify that the vector belongs to this device.
> -        if !core::ptr::eq(vector.dev.as_raw(), self.as_raw()) {
> -            return Err(EINVAL);
> -        }
> -
> -        // SAFETY: `self.as_raw` returns a valid pointer to a `struct pci_dev`.
> -        let irq = unsafe { crate::bindings::pci_irq_vector(self.as_raw(), vector.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.as_ref(), irq as u32) })
> -    }
> -
>      /// Returns a [`kernel::irq::Registration`] for the given IRQ vector.
>      pub fn request_irq<'a, T: crate::irq::Handler + 'static>(
>          &'a self,
> -        vector: IrqVector<'_>,
> +        vector: IrqVector<'a>,
>          flags: irq::Flags,
>          name: &'static CStr,
>          handler: impl PinInit<T, Error> + 'a,
>      ) -> Result<impl PinInit<irq::Registration<T>, Error> + 'a> {
> -        let request = self.irq_vector(vector)?;
> +        let request = vector.try_into()?;
>  
>          Ok(irq::Registration::<T>::new(request, flags, name, handler))
>      }
> @@ -707,12 +705,12 @@ pub fn request_irq<'a, T: crate::irq::Handler + 'static>(
>      /// Returns a [`kernel::irq::ThreadedRegistration`] for the given IRQ vector.
>      pub fn request_threaded_irq<'a, T: crate::irq::ThreadedHandler + 'static>(
>          &'a self,
> -        vector: IrqVector<'_>,
> +        vector: IrqVector<'a>,
>          flags: irq::Flags,
>          name: &'static CStr,
>          handler: impl PinInit<T, Error> + 'a,
>      ) -> Result<impl PinInit<irq::ThreadedRegistration<T>, Error> + 'a> {
> -        let request = self.irq_vector(vector)?;
> +        let request = vector.try_into()?;
>  
>          Ok(irq::ThreadedRegistration::<T>::new(
>              request, flags, name, handler,
> -- 
> 2.51.0
>

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>

On 10/16/2025 6:57 PM, Danilo Krummrich wrote:
> 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.
Ah true, I think the naming "IrqRequest" throw me off. Sorry about that, so this
patch is good then :) Thanks,

 - Joel

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) })
> +    }
> +}
> +
>  /// Represents an IRQ vector allocation for a PCI device.
>  ///
>  /// This type ensures that IRQ vectors are properly allocated and freed by
> @@ -675,31 +689,15 @@ pub fn iomap_region<'a>(
>          self.iomap_region_sized::<0>(bar, name)
>      }
>  
> -    /// Returns an [`IrqRequest`] for the given IRQ vector.
> -    pub fn irq_vector(&self, vector: IrqVector<'_>) -> Result<IrqRequest<'_>> {
> -        // Verify that the vector belongs to this device.
> -        if !core::ptr::eq(vector.dev.as_raw(), self.as_raw()) {
> -            return Err(EINVAL);
> -        }
> -
> -        // SAFETY: `self.as_raw` returns a valid pointer to a `struct pci_dev`.
> -        let irq = unsafe { crate::bindings::pci_irq_vector(self.as_raw(), vector.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.as_ref(), irq as u32) })
> -    }
> -
>      /// Returns a [`kernel::irq::Registration`] for the given IRQ vector.
>      pub fn request_irq<'a, T: crate::irq::Handler + 'static>(
>          &'a self,
> -        vector: IrqVector<'_>,
> +        vector: IrqVector<'a>,
>          flags: irq::Flags,
>          name: &'static CStr,
>          handler: impl PinInit<T, Error> + 'a,
>      ) -> Result<impl PinInit<irq::Registration<T>, Error> + 'a> {
> -        let request = self.irq_vector(vector)?;
> +        let request = vector.try_into()?;
>  
>          Ok(irq::Registration::<T>::new(request, flags, name, handler))
>      }
> @@ -707,12 +705,12 @@ pub fn request_irq<'a, T: crate::irq::Handler + 'static>(
>      /// Returns a [`kernel::irq::ThreadedRegistration`] for the given IRQ vector.
>      pub fn request_threaded_irq<'a, T: crate::irq::ThreadedHandler + 'static>(
>          &'a self,
> -        vector: IrqVector<'_>,
> +        vector: IrqVector<'a>,
>          flags: irq::Flags,
>          name: &'static CStr,
>          handler: impl PinInit<T, Error> + 'a,
>      ) -> Result<impl PinInit<irq::ThreadedRegistration<T>, Error> + 'a> {
> -        let request = self.irq_vector(vector)?;
> +        let request = vector.try_into()?;

The resulting change to the lifetime semantics is curious, but seems
right.

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

On 10/16/25 5:01 PM, Alice Ryhl wrote:
> On Wed, Oct 15, 2025 at 08:14:29PM +0200, Danilo Krummrich wrote:
>> @@ -707,12 +705,12 @@ pub fn request_irq<'a, T: crate::irq::Handler + 'static>(
>>      /// Returns a [`kernel::irq::ThreadedRegistration`] for the given IRQ vector.
>>      pub fn request_threaded_irq<'a, T: crate::irq::ThreadedHandler + 'static>(
>>          &'a self,
>> -        vector: IrqVector<'_>,
>> +        vector: IrqVector<'a>,
>>          flags: irq::Flags,
>>          name: &'static CStr,
>>          handler: impl PinInit<T, Error> + 'a,
>>      ) -> Result<impl PinInit<irq::ThreadedRegistration<T>, Error> + 'a> {
>> -        let request = self.irq_vector(vector)?;
>> +        let request = vector.try_into()?;
> 
> The resulting change to the lifetime semantics is curious, but seems
> right.

Yes, IrqVector has to have the same lifetime as &self, but I was surprised that
with this change the compiler does require an explicit lifetime (even though I
think the explicit one is better anyways).
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>