[PATCH v6 6/6] rust: pci: add irq accessors

Daniel Almeida posted 6 patches 3 months ago
There is a newer version of this series
[PATCH v6 6/6] rust: pci: add irq accessors
Posted by Daniel Almeida 3 months ago
These accessors can be used to retrieve a irq::Registration or a
irq::ThreadedRegistration from a pci device. Alternatively, drivers can
retrieve an IrqRequest from a bound PCI device for later use.

These accessors ensure that only valid IRQ lines can ever be registered.

Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
---
 rust/helpers/pci.c |  8 ++++++++
 rust/kernel/pci.rs | 45 +++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 51 insertions(+), 2 deletions(-)

diff --git a/rust/helpers/pci.c b/rust/helpers/pci.c
index cd0e6bf2cc4d9b37db3a717e7a8422b054f348ec..f372a32e8fd19730563ab51500e8c8764854ae47 100644
--- a/rust/helpers/pci.c
+++ b/rust/helpers/pci.c
@@ -21,3 +21,11 @@ bool rust_helper_dev_is_pci(const struct device *dev)
 {
 	return dev_is_pci(dev);
 }
+
+#ifndef CONFIG_PCI_MSI
+int rust_helper_pci_irq_vector(struct pci_dev *pdev, unsigned int nvec)
+{
+	return pci_irq_vector(pdev, nvec);
+}
+
+#endif
diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
index db0eb7eaf9b10c5316366ef16fe722a03044a517..60d37d6459518c79136535ce03c73a5a3097eda8 100644
--- a/rust/kernel/pci.rs
+++ b/rust/kernel/pci.rs
@@ -10,8 +10,8 @@
     devres::Devres,
     driver,
     error::{to_result, Result},
-    io::Io,
-    io::IoRaw,
+    io::{Io, IoRaw},
+    irq::{self, request::IrqRequest},
     str::CStr,
     types::{ARef, ForeignOwnable, Opaque},
     ThisModule,
@@ -413,6 +413,47 @@ pub fn iomap_region<'a>(
     ) -> impl PinInit<Devres<Bar>, Error> + 'a {
         self.iomap_region_sized::<0>(bar, name)
     }
+
+    /// Returns an [`IrqRequest`] for the IRQ vector at the given index, if any.
+    pub fn request_irq_by_index(&self, index: u32) -> Result<IrqRequest<'_>> {
+        // SAFETY: `self.as_raw` returns a valid pointer to a `struct pci_dev`.
+        let irq = unsafe { crate::bindings::pci_irq_vector(self.as_raw(), 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 IRQ vector at the given
+    /// index.
+    pub fn irq_by_index<T: crate::irq::Handler + 'static>(
+        &self,
+        index: u32,
+        flags: irq::flags::Flags,
+        name: &'static CStr,
+        handler: T,
+    ) -> Result<impl PinInit<irq::Registration<T>, Error> + '_> {
+        let request = self.request_irq_by_index(index)?;
+
+        Ok(irq::Registration::<T>::new(request, flags, name, handler))
+    }
+
+    /// Returns a [`kernel::irq::ThreadedRegistration`] for the IRQ vector at
+    /// the given index.
+    pub fn threaded_irq_by_index<T: crate::irq::ThreadedHandler + 'static>(
+        &self,
+        index: u32,
+        flags: irq::flags::Flags,
+        name: &'static CStr,
+        handler: T,
+    ) -> Result<impl PinInit<irq::ThreadedRegistration<T>, Error> + '_> {
+        let request = self.request_irq_by_index(index)?;
+
+        Ok(irq::ThreadedRegistration::<T>::new(
+            request, flags, name, handler,
+        ))
+    }
 }
 
 impl Device<device::Core> {

-- 
2.50.0
Re: [PATCH v6 6/6] rust: pci: add irq accessors
Posted by Danilo Krummrich 3 months ago
On Thu, Jul 03, 2025 at 04:30:04PM -0300, Daniel Almeida wrote:
> +    /// Returns an [`IrqRequest`] for the IRQ vector at the given index, if any.
> +    pub fn request_irq_by_index(&self, index: u32) -> Result<IrqRequest<'_>> {

Same comment as for platform:

Please name the functions returning an IrqRequest without the 'request' prefix.
And instead put the 'request' prefix in front of the methods that return a
actual irq::Registration.

Besides that, I think we shouldn't name this method 'by_index'. Please align it
with the C function, i.e. Device::irq_vector().

> +    pub fn irq_by_index<T: crate::irq::Handler + 'static>(

I'd go with just request_irq() for this one and

> +    pub fn threaded_irq_by_index<T: crate::irq::ThreadedHandler + 'static>(

request_threaded_irq() for this one.
Re: [PATCH v6 6/6] rust: pci: add irq accessors
Posted by Alice Ryhl 3 months ago
On Thu, Jul 03, 2025 at 04:30:04PM -0300, Daniel Almeida wrote:
> These accessors can be used to retrieve a irq::Registration or a
> irq::ThreadedRegistration from a pci device. Alternatively, drivers can
> retrieve an IrqRequest from a bound PCI device for later use.
> 
> These accessors ensure that only valid IRQ lines can ever be registered.
> 
> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>

Same question as patch 5. With that answered:
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Re: [PATCH v6 6/6] rust: pci: add irq accessors
Posted by Danilo Krummrich 3 months ago
On Fri, Jul 04, 2025 at 07:56:56AM +0000, Alice Ryhl wrote:
> On Thu, Jul 03, 2025 at 04:30:04PM -0300, Daniel Almeida wrote:
> > These accessors can be used to retrieve a irq::Registration or a
> > irq::ThreadedRegistration from a pci device. Alternatively, drivers can
> > retrieve an IrqRequest from a bound PCI device for later use.
> > 
> > These accessors ensure that only valid IRQ lines can ever be registered.
> > 
> > Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
> 
> Same question as patch 5.

Here we don't do a lookup of the IRQ number by name, yet the API asks for the
name used for the IRQ registration, hence it needs a static lifetime, since the
pointer is directly stored in struct irqaction. It's accessed by trace events
for instance.

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