rust: add support for request_irq

Add the IRQ module. Future patches will then introduce support for IRQ registrations and handlers.

Signed-off-by Daniel Almeida <[email protected]>

rust/kernel/irq/flags.rs new file +6 -0

+/// Use the interrupt line as already configured.
+pub const TRIGGER_NONE: Flags = Flags(bindings::IRQF_TRIGGER_NONE as u64);
+
+/// The interrupt is triggered when the signal goes from low to high.
+pub const TRIGGER_RISING: Flags = Flags(bindings::IRQF_TRIGGER_RISING as u64);
+

+pub const PERCPU: Flags = Flags(bindings::IRQF_PERCPU as u64);

Hi Daniel,

rust/kernel/irq/flags.rs line 10

Quoted 5 lines · Daniel Almeida

rust/kernel/irq/flags.rs new file +5 -0

+/// Flags to be used when registering IRQ handlers.
+///
+/// They can be combined with the operators `|`, `&`, and `!`.
+#[derive(Clone, Copy, PartialEq, Eq)]
+pub struct Flags(u64);

Why not Flags(u32)? You may get rid of all unnecessary casts later, plus save some extra bytes.

Quoted 11 lines · Daniel Sedlak

Hi Daniel,

rust/kernel/irq/flags.rs line 10

Quoted 5 lines · Daniel Almeida

rust/kernel/irq/flags.rs new file +5 -0

+/// Flags to be used when registering IRQ handlers.
+///
+/// They can be combined with the operators `|`, `&`, and `!`.
+#[derive(Clone, Copy, PartialEq, Eq)]
+pub struct Flags(u64);

Why not Flags(u32)? You may get rid of all unnecessary casts later, plus save some extra bytes.

It looks like the C methods take an unsigned long. In that case, I'd probably write the code to match that.

pub struct Flags(c_ulong);

and git rid of the cast when calling bindings::request_irq.

As for all the constants in this file, maybe it would be nice with a private constructor that uses the same type as bindings to avoid the casts?

impl Flags {
   const fn new(value: u32) -> Flags {
    ...
   }
}

Hi Alice,

Quoted 26 lines · Alice Ryhl

Quoted 11 lines · Daniel Sedlak

Hi Daniel,

rust/kernel/irq/flags.rs line 10

Quoted 5 lines · Daniel Almeida

rust/kernel/irq/flags.rs new file +5 -0

+/// Flags to be used when registering IRQ handlers.
+///
+/// They can be combined with the operators `|`, `&`, and `!`.
+#[derive(Clone, Copy, PartialEq, Eq)]
+pub struct Flags(u64);

Why not Flags(u32)? You may get rid of all unnecessary casts later, plus save some extra bytes.

It looks like the C methods take an unsigned long. In that case, I'd probably write the code to match that.

pub struct Flags(c_ulong);

and git rid of the cast when calling bindings::request_irq.

As for all the constants in this file, maybe it would be nice with a private constructor that uses the same type as bindings to avoid the casts?

Quoted 5 lines

impl Flags {
   const fn new(value: u32) -> Flags {
    ...
   }
}

Sure, but what goes here? This has to be "value as c_ulong” anyways so it doesn’t really reduce the number of casts.

We should probably switch to Flags(u32) as Daniel Sedlak suggested. Then it’s a matter of casting once for bindings::request_irq().

Quoted 39 lines · Daniel Almeida

Hi Alice,

Quoted 26 lines · Alice Ryhl

Quoted 11 lines · Daniel Sedlak

Hi Daniel,

rust/kernel/irq/flags.rs line 10

Quoted 5 lines · Daniel Almeida

rust/kernel/irq/flags.rs new file +5 -0

+/// Flags to be used when registering IRQ handlers.
+///
+/// They can be combined with the operators `|`, `&`, and `!`.
+#[derive(Clone, Copy, PartialEq, Eq)]
+pub struct Flags(u64);

Why not Flags(u32)? You may get rid of all unnecessary casts later, plus save some extra bytes.

It looks like the C methods take an unsigned long. In that case, I'd probably write the code to match that.

pub struct Flags(c_ulong);

and git rid of the cast when calling bindings::request_irq.

As for all the constants in this file, maybe it would be nice with a private constructor that uses the same type as bindings to avoid the casts?

Quoted 5 lines

impl Flags {

  const fn new(value: u32) -> Flags {
   ...

}
}

Sure, but what goes here? This has to be "value as c_ulong” anyways so it doesn’t really reduce the number of casts.

Quoted 2 lines

We should probably switch to Flags(u32) as Daniel Sedlak suggested. Then it’s a matter of casting once for bindings::request_irq().

IMO the advantage of doing it here is that we can fail compilation if the cast is out of bounds, whereas the other cast is at runtime so we can't do that.

Alice

Quoted 4 lines

Quoted 4 lines

impl Flags {
const fn new(value: u32) -> Flags {
Self(value as c_ulong)
}

Quoted 46 lines · Alice Ryhl

Quoted 39 lines · Daniel Almeida

Hi Alice,

Quoted 26 lines · Alice Ryhl

Quoted 11 lines · Daniel Sedlak

Hi Daniel,

rust/kernel/irq/flags.rs line 10

Quoted 5 lines · Daniel Almeida

rust/kernel/irq/flags.rs new file +5 -0

+/// Flags to be used when registering IRQ handlers.
+///
+/// They can be combined with the operators `|`, `&`, and `!`.
+#[derive(Clone, Copy, PartialEq, Eq)]
+pub struct Flags(u64);

Why not Flags(u32)? You may get rid of all unnecessary casts later, plus save some extra bytes.

It looks like the C methods take an unsigned long. In that case, I'd probably write the code to match that.

pub struct Flags(c_ulong);

and git rid of the cast when calling bindings::request_irq.

As for all the constants in this file, maybe it would be nice with a private constructor that uses the same type as bindings to avoid the casts?

Quoted 5 lines

impl Flags {

  const fn new(value: u32) -> Flags {
   ...

}
}

Sure, but what goes here? This has to be "value as c_ulong” anyways so it doesn’t really reduce the number of casts.

Quoted 8 lines

Quoted 2 lines

We should probably switch to Flags(u32) as Daniel Sedlak suggested. Then it’s a matter of casting once for bindings::request_irq().

IMO the advantage of doing it here is that we can fail compilation if the cast is out of bounds, whereas the other cast is at runtime so we can't do that.

Alice

I’m not sure I am following. How is this compile-time checked?

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

impl Flags {
const fn new(value: u32) -> Flags {
Self(value as c_ulong)
}

Or perhaps I misunderstood you?

Why not Flags(u32)? You may get rid of all unnecessary casts later, plus save some extra bytes.

This patch adds support for non-threaded IRQs and handlers through irq::Registration and the irq::Handler trait.

Registering an irq is dependent upon having a IrqRequest that was previously allocated by a given device. This will be introduced in subsequent patches.

Signed-off-by Daniel Almeida <[email protected]>

---
 rust/bindings/bindings_helper.h |   1 +
 rust/helpers/helpers.c          |   1 +
 rust/helpers/irq.c              |   9 ++
 rust/kernel/irq.rs              |   5 +
 rust/kernel/irq/request.rs      | 273 ++++++++++++++++++++++++++++++++++++++++
 5 files changed, 289 insertions(+)

rust/bindings/bindings_helper.h +1 -0

index 8cbb660e2ec218021d16e6e0144acf6f4d7cca13..da0bd23fad59a2373bd873d12ad69c55208aaa38 100644
@@ -51,6 +51,7 @@
 #include <linux/ethtool.h>
 #include <linux/file.h>
 #include <linux/firmware.h>
+#include <linux/interrupt.h>
 #include <linux/fs.h>
 #include <linux/jiffies.h>
 #include <linux/jump_label.h>

rust/helpers/helpers.c +1 -0

index 393ad201befb80a9ae39866a725744ab88620fbb..e3579fc7e1cfc30c913207a4a78b790259d7ae7a 100644
@@ -22,6 +22,7 @@
 #include "dma.c"
 #include "drm.c"
 #include "err.c"
+#include "irq.c"
 #include "fs.c"
 #include "io.c"
 #include "jump_label.c"

rust/helpers/irq.c new file +9 -0

new file mode 100644
index 0000000000000000000000000000000000000000..1faca428e2c047a656dec3171855c1508d67e60b
@@ -0,0 +1,9 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/interrupt.h>
+
+int rust_helper_request_irq(unsigned int irq, irq_handler_t handler,
+			    unsigned long flags, const char *name, void *dev)
+{
+	return request_irq(irq, handler, flags, name, dev);
+}

rust/kernel/irq.rs +5 -0

index 9abd9a6dc36f3e3ecc1f92ad7b0040176b56a079..01bd08884b72c2a3a9460897bce751c732a19794 100644
@@ -12,3 +12,8 @@
 
 /// Flags to be used when registering IRQ handlers.
 pub mod flags;
+
+/// IRQ allocation and handling.
+pub mod request;
+
+pub use request::{Handler, IrqRequest, IrqReturn, Registration};

rust/kernel/irq/request.rs new file +22 -0

new file mode 100644
index 0000000000000000000000000000000000000000..4f4beaa3c7887660440b9ddc52414020a0d165ac
@@ -0,0 +1,273 @@
+// SPDX-License-Identifier: GPL-2.0
+// SPDX-FileCopyrightText: Copyright 2025 Collabora ltd.
+
+//! This module provides types like [`Registration`] which allow users to
+//! register handlers for a given IRQ line.
+
+use core::marker::PhantomPinned;
+
+use crate::alloc::Allocator;
+use crate::device::Bound;
+use crate::device::Device;
+use crate::devres::Devres;
+use crate::error::to_result;
+use crate::irq::flags::Flags;
+use crate::prelude::*;
+use crate::str::CStr;
+use crate::sync::Arc;
+
+/// The value that can be returned from an IrqHandler or a ThreadedIrqHandler.
+pub enum IrqReturn {
+    /// The interrupt was not from this device or was not handled.
+    None,

rust/kernel/irq/request.rs new file +22 -0

+
+/// Callbacks for an IRQ handler.
+pub trait Handler: Sync {
+    /// The hard IRQ handler.
+    ///
+    /// This is executed in interrupt context, hence all corresponding
+    /// limitations do apply.
+    ///
+    /// All work that does not necessarily need to be executed from
+    /// interrupt context, should be deferred to a threaded handler.
+    /// See also [`ThreadedRegistration`].
+    fn handle(&self) -> IrqReturn;
+}
+
+impl<T: ?Sized + Handler + Send> Handler for Arc<T> {
+    fn handle(&self) -> IrqReturn {
+        T::handle(self)
+    }
+}
+
+impl<T: ?Sized + Handler, A: Allocator> Handler for Box<T, A> {
+    fn handle(&self) -> IrqReturn {

rust/kernel/irq/request.rs new file +188 -0

+}
+
+impl RegistrationInner {
+    fn synchronize(&self) {
+        // SAFETY: safe as per the invariants of `RegistrationInner`
+        unsafe { bindings::synchronize_irq(self.irq) };
+    }
+}
+
+#[pinned_drop]
+impl PinnedDrop for RegistrationInner {
+    fn drop(self: Pin<&mut Self>) {
+        // SAFETY:
+        //
+        // Safe as per the invariants of `RegistrationInner` and:
+        //
+        // - The containing struct is `!Unpin` and was initialized using
+        // pin-init, so it occupied the same memory location for the entirety of
+        // its lifetime.
+        //
+        // Notice that this will block until all handlers finish executing,
+        // i.e.: at no point will &self be invalid while the handler is running.
+        unsafe { bindings::free_irq(self.irq, self.cookie) };
+    }
+}
+
+// SAFETY: We only use `inner` on drop, which called at most once with no
+// concurrent access.
+unsafe impl Sync for RegistrationInner {}
+
+// SAFETY: It is safe to send `RegistrationInner` across threads.
+unsafe impl Send for RegistrationInner {}
+
+/// A request for an IRQ line for a given device.
+///
+/// # Invariants
+///
+/// - `ìrq` is the number of an interrupt source of `dev`.
+/// - `irq` has not been registered yet.
+pub struct IrqRequest<'a> {
+    dev: &'a Device<Bound>,
+    irq: u32,
+}
+
+impl<'a> IrqRequest<'a> {
+    /// Creates a new IRQ request for the given device and IRQ number.
+    ///
+    /// # Safety
+    ///
+    /// - `irq` should be a valid IRQ number for `dev`.
+    pub(crate) unsafe fn new(dev: &'a Device<Bound>, irq: u32) -> Self {
+        IrqRequest { dev, irq }
+    }
+}
+
+/// A registration of an IRQ handler for a given IRQ line.
+///
+/// # Examples
+///
+/// The following is an example of using `Registration`. It uses a
+/// [`AtomicU32`](core::sync::AtomicU32) to provide the interior mutability.
+///
+/// ```
+/// use core::sync::atomic::AtomicU32;
+/// use core::sync::atomic::Ordering;
+///
+/// use kernel::prelude::*;
+/// use kernel::device::Bound;
+/// use kernel::irq::flags;
+/// use kernel::irq::Registration;
+/// use kernel::irq::IrqRequest;
+/// use kernel::irq::IrqReturn;
+/// use kernel::sync::Arc;
+/// use kernel::c_str;
+/// use kernel::alloc::flags::GFP_KERNEL;
+///
+/// // Declare a struct that will be passed in when the interrupt fires. The u32
+/// // merely serves as an example of some internal data.
+/// struct Data(AtomicU32);
+///
+/// // [`kernel::irq::request::Handler::handle`] takes `&self`. This example
+/// // illustrates how interior mutability can be used when sharing the data
+/// // between process context and IRQ context.
+///
+/// type Handler = Data;
+///
+/// impl kernel::irq::request::Handler for Handler {
+///     // This is executing in IRQ context in some CPU. Other CPUs can still
+///     // try to access to data.
+///     fn handle(&self) -> IrqReturn {
+///         self.0.fetch_add(1, Ordering::Relaxed);
+///
+///         IrqReturn::Handled
+///     }
+/// }
+///
+/// // Registers an IRQ handler for the given IrqRequest.
+/// //
+/// // This is executing in process context and assumes that `request` was
+/// // previously acquired from a device.
+/// fn register_irq(handler: Handler, request: IrqRequest<'_>) -> Result<Arc<Registration<Handler>>> {
+///     let registration = Registration::new(request, flags::SHARED, c_str!("my_device"), handler);
+///
+///     let registration = Arc::pin_init(registration, GFP_KERNEL)?;
+///
+///     // The data can be accessed from process context too.
+///     registration.handler().0.fetch_add(1, Ordering::Relaxed);
+///
+///     Ok(registration)
+/// }
+/// # Ok::<(), Error>(())
+/// ```
+///
+/// # Invariants
+///
+/// * We own an irq handler using `&self.handler` as its private data.
+///
+#[pin_data]
+pub struct Registration<T: Handler + 'static> {
+    #[pin]
+    inner: Devres<RegistrationInner>,
+
+    #[pin]
+    handler: T,
+
+    /// Pinned because we need address stability so that we can pass a pointer
+    /// to the callback.
+    #[pin]
+    _pin: PhantomPinned,
+}
+
+impl<T: Handler + 'static> Registration<T> {
+    /// Registers the IRQ handler with the system for the given IRQ number.
+    pub fn new<'a>(
+        request: IrqRequest<'a>,
+        flags: Flags,
+        name: &'static CStr,
+        handler: T,
+    ) -> impl PinInit<Self, Error> + 'a {
+        try_pin_init!(&this in Self {
+            handler,
+            inner <- Devres::new(
+                request.dev,
+                try_pin_init!(RegistrationInner {
+                    // SAFETY: `this` is a valid pointer to the `Registration` instance
+                    cookie: unsafe { &raw mut (*this.as_ptr()).handler }.cast(),
+                    irq: {
+                        // SAFETY:
+                        // - The callbacks are valid for use with request_irq.
+                        // - If this succeeds, the slot is guaranteed to be valid until the
+                        // destructor of Self runs, which will deregister the callbacks
+                        // before the memory location becomes invalid.
+                        to_result(unsafe {
+                            bindings::request_irq(
+                                request.irq,
+                                Some(handle_irq_callback::<T>),
+                                flags.into_inner() as usize,
+                                name.as_char_ptr(),
+                                (&raw mut (*this.as_ptr()).handler).cast(),
+                            )
+                        })?;
+                        request.irq
+                    }
+                })
+            ),
+            _pin: PhantomPinned,
+        })
+    }
+
+    /// Returns a reference to the handler that was registered with the system.
+    pub fn handler(&self) -> &T {
+        &self.handler
+    }
+
+    /// Wait for pending IRQ handlers on other CPUs.
+    ///
+    /// This will attempt to access the inner [`Devres`] container.
+    pub fn try_synchronize(&self) -> Result {
+        let inner = self.inner.try_access().ok_or(ENODEV)?;
+        inner.synchronize();
+        Ok(())
+    }
+
+    /// Wait for pending IRQ handlers on other CPUs.
+    pub fn synchronize(&self, dev: &Device<Bound>) -> Result {
+        let inner = self.inner.access(dev)?;
+        inner.synchronize();
+        Ok(())

rust/kernel/irq/request.rs new file +4 -0

+    // SAFETY: `ptr` is a pointer to T set in `Registration::new`
+    let handler = unsafe { &*(ptr as *const T) };
+    T::handle(handler).into_inner()
+}

Signature · 2 lines

-- 
2.50.0

Quoted 4 lines

rust/kernel/irq/request.rs new file +4 -0

+/// The value that can be returned from an IrqHandler or a ThreadedIrqHandler.
+pub enum IrqReturn {
+    /// The interrupt was not from this device or was not handled.
+    None,

rust/kernel/irq/request.rs line 32

Quoted 10 lines

rust/kernel/irq/request.rs new file +10 -0

+
+    /// The interrupt was handled by this device.
+    Handled,
+}
+
+impl IrqReturn {
+    fn into_inner(self) -> u32 {
+        match self {
+            IrqReturn::None => bindings::irqreturn_IRQ_NONE,
+            IrqReturn::Handled => bindings::irqreturn_IRQ_HANDLED,

One option is to specify these in the enum:

/// The value that can be returned from an IrqHandler or a ThreadedIrqHandler. pub enum IrqReturn {
/// The interrupt was not from this device or was not handled. None = bindings::irqreturn_IRQ_NONE,

/// The interrupt was handled by this device.
Handled = bindings::irqreturn_IRQ_HANDLED,
}

Alice,

[…]

Quoted 22 lines

Quoted 4 lines

rust/kernel/irq/request.rs new file +4 -0

+/// The value that can be returned from an IrqHandler or a ThreadedIrqHandler.
+pub enum IrqReturn {
+    /// The interrupt was not from this device or was not handled.
+    None,

rust/kernel/irq/request.rs line 32

Quoted 10 lines

rust/kernel/irq/request.rs new file +10 -0

+
+    /// The interrupt was handled by this device.
+    Handled,
+}
+
+impl IrqReturn {
+    fn into_inner(self) -> u32 {
+        match self {
+            IrqReturn::None => bindings::irqreturn_IRQ_NONE,
+            IrqReturn::Handled => bindings::irqreturn_IRQ_HANDLED,

One option is to specify these in the enum:

/// The value that can be returned from an IrqHandler or a ThreadedIrqHandler. pub enum IrqReturn {
/// The interrupt was not from this device or was not handled. None = bindings::irqreturn_IRQ_NONE,

Quoted 3 lines

/// The interrupt was handled by this device.
Handled = bindings::irqreturn_IRQ_HANDLED,
}

This requires explicitly setting #[repr(u32)], which is something that was reverted at an earlier iteration of the series on Benno’s request.

Quoted 30 lines · Daniel Almeida

Alice,

[…]

Quoted 25 lines

Quoted 4 lines

rust/kernel/irq/request.rs new file +4 -0

+/// The value that can be returned from an IrqHandler or a ThreadedIrqHandler.
+pub enum IrqReturn {
+    /// The interrupt was not from this device or was not handled.
+    None,

rust/kernel/irq/request.rs line 32

Quoted 10 lines

rust/kernel/irq/request.rs new file +10 -0

+
+    /// The interrupt was handled by this device.
+    Handled,
+}
+
+impl IrqReturn {
+    fn into_inner(self) -> u32 {
+        match self {
+            IrqReturn::None => bindings::irqreturn_IRQ_NONE,
+            IrqReturn::Handled => bindings::irqreturn_IRQ_HANDLED,

One option is to specify these in the enum:

/// The value that can be returned from an IrqHandler or a ThreadedIrqHandler. pub enum IrqReturn {
/// The interrupt was not from this device or was not handled. None = bindings::irqreturn_IRQ_NONE,

/// The interrupt was handled by this device.
Handled = bindings::irqreturn_IRQ_HANDLED,
}

Quoted 2 lines

This requires explicitly setting #[repr(u32)], which is something that was reverted at an earlier iteration of the series on Benno’s request.

Yeah I requested this, because it increases the size of the enum to 4 bytes and I think we should try to make rust enums as small as possible.

@Alice what's the benefit of doing it directly in the enum?

Quoted 31 lines · Benno Lossin

Quoted 30 lines · Daniel Almeida

Alice,

[…]

Quoted 25 lines

Quoted 4 lines

rust/kernel/irq/request.rs new file +4 -0

+/// The value that can be returned from an IrqHandler or a ThreadedIrqHandler.
+pub enum IrqReturn {
+    /// The interrupt was not from this device or was not handled.
+    None,

rust/kernel/irq/request.rs line 32

Quoted 10 lines

rust/kernel/irq/request.rs new file +10 -0

+
+    /// The interrupt was handled by this device.
+    Handled,
+}
+
+impl IrqReturn {
+    fn into_inner(self) -> u32 {
+        match self {
+            IrqReturn::None => bindings::irqreturn_IRQ_NONE,
+            IrqReturn::Handled => bindings::irqreturn_IRQ_HANDLED,

One option is to specify these in the enum:

/// The value that can be returned from an IrqHandler or a ThreadedIrqHandler. pub enum IrqReturn {
/// The interrupt was not from this device or was not handled. None = bindings::irqreturn_IRQ_NONE,

/// The interrupt was handled by this device.
Handled = bindings::irqreturn_IRQ_HANDLED,
}

Quoted 7 lines

Quoted 2 lines

This requires explicitly setting #[repr(u32)], which is something that was reverted at an earlier iteration of the series on Benno’s request.

Yeah I requested this, because it increases the size of the enum to 4 bytes and I think we should try to make rust enums as small as possible.

@Alice what's the benefit of doing it directly in the enum?

Basically all uses of this enum are going to look like this:

	fn inner() -> IrqReturn {
	    if !should_handle() {
	        return IrqReturn::None;
	    }
	    // .. handle the irq
	    IrqReturn::Handled
	}
	
	fn outer() -> u32 {
	    match inner() {
	        IrqReturn::None => bindings::irqreturn_IRQ_NONE,
	        IrqReturn::Handled => bindings::irqreturn_IRQ_HANDLED,
	    }
	}

Setting the discriminant to the values ensures that the match in outer() is a no-op. The enum is never going to be stored long-term anywhere so its size doesn't matter.

On Thu, Jul 03, 2025 at 04:30:01PM -0300, Daniel Almeida wrote:
[...]

rust/kernel/irq/request.rs line 195

Quoted 7 lines

rust/kernel/irq/request.rs new file +7 -0

+#[pin_data]
+pub struct Registration<T: Handler + 'static> {
+    #[pin]
+    inner: Devres<RegistrationInner>,
+
+    #[pin]
+    handler: T,

IIRC, as a certain point, we want this to be a UnsafePinned<T>, is that requirement gone or we still need that but 1) UnsafePinned is not available and 2) we can rely on the whole struct being !Unpin for the address stability temporarily?

I think it was not a problem until we switched to try_pin_init!() instead of pin_init_from_closure() because we then had to pass the address of handler instead of the whole struct.

Since we certainly want to use try_pin_init!() and we certainly will have UnsafePinned, I think we should just keep this as it is for now,

and add a TODO so that we can clean it up later when we have UnsafePinned?

Thoughts?

Regards,
Boqun

Quoted 15 lines · Boqun Feng

On Thu, Jul 03, 2025 at 04:30:01PM -0300, Daniel Almeida wrote:
[...]

rust/kernel/irq/request.rs line 195

Quoted 7 lines

rust/kernel/irq/request.rs new file +7 -0

+#[pin_data]
+pub struct Registration<T: Handler + 'static> {
+    #[pin]
+    inner: Devres<RegistrationInner>,
+
+    #[pin]
+    handler: T,

IIRC, as a certain point, we want this to be a UnsafePinned<T>, is that requirement gone or we still need that but 1) UnsafePinned is not available and 2) we can rely on the whole struct being !Unpin for the address stability temporarily?

Quoted 6 lines

I think it was not a problem until we switched to try_pin_init!() instead of pin_init_from_closure() because we then had to pass the address of handler instead of the whole struct.

Since we certainly want to use try_pin_init!() and we certainly will have UnsafePinned, I think we should just keep this as it is for now,

Of course the assumption is we want to it in before UnsafePinned ;-) Alternatively we can do what Devres did:

	https://lore.kernel.org/rust-for-linux/[email protected]/

using an Opaque and manually drop for now.

This patch adds support for non-threaded IRQs and handlers through irq::Registration and the irq::Handler trait.

Registering an irq is dependent upon having a IrqRequest that was previously allocated by a given device. This will be introduced in subsequent patches.

Signed-off-by Daniel Almeida <[email protected]>

rust/kernel/irq/request.rs new file +115 -0

new file mode 100644
index 0000000000000000000000000000000000000000..4f4beaa3c7887660440b9ddc52414020a0d165ac
@@ -0,0 +1,273 @@
+// SPDX-License-Identifier: GPL-2.0
+// SPDX-FileCopyrightText: Copyright 2025 Collabora ltd.
+
+//! This module provides types like [`Registration`] which allow users to
+//! register handlers for a given IRQ line.
+
+use core::marker::PhantomPinned;
+
+use crate::alloc::Allocator;
+use crate::device::Bound;
+use crate::device::Device;
+use crate::devres::Devres;
+use crate::error::to_result;
+use crate::irq::flags::Flags;
+use crate::prelude::*;
+use crate::str::CStr;
+use crate::sync::Arc;
+
+/// The value that can be returned from an IrqHandler or a ThreadedIrqHandler.
+pub enum IrqReturn {
+    /// The interrupt was not from this device or was not handled.
+    None,
+
+    /// The interrupt was handled by this device.
+    Handled,
+}
+
+impl IrqReturn {
+    fn into_inner(self) -> u32 {
+        match self {
+            IrqReturn::None => bindings::irqreturn_IRQ_NONE,
+            IrqReturn::Handled => bindings::irqreturn_IRQ_HANDLED,
+        }
+    }
+}
+
+/// Callbacks for an IRQ handler.
+pub trait Handler: Sync {
+    /// The hard IRQ handler.
+    ///
+    /// This is executed in interrupt context, hence all corresponding
+    /// limitations do apply.
+    ///
+    /// All work that does not necessarily need to be executed from
+    /// interrupt context, should be deferred to a threaded handler.
+    /// See also [`ThreadedRegistration`].
+    fn handle(&self) -> IrqReturn;
+}
+
+impl<T: ?Sized + Handler + Send> Handler for Arc<T> {
+    fn handle(&self) -> IrqReturn {
+        T::handle(self)
+    }
+}
+
+impl<T: ?Sized + Handler, A: Allocator> Handler for Box<T, A> {
+    fn handle(&self) -> IrqReturn {
+        T::handle(self)
+    }
+}
+
+/// # Invariants
+///
+/// - `self.irq` is the same as the one passed to `request_{threaded}_irq`.
+/// - `cookie` was passed to `request_{threaded}_irq` as the cookie. It
+/// is guaranteed to be unique by the type system, since each call to
+/// `new` will return a different instance of `Registration`.
+#[pin_data(PinnedDrop)]
+struct RegistrationInner {
+    irq: u32,
+    cookie: *mut kernel::ffi::c_void,
+}
+
+impl RegistrationInner {
+    fn synchronize(&self) {
+        // SAFETY: safe as per the invariants of `RegistrationInner`
+        unsafe { bindings::synchronize_irq(self.irq) };
+    }
+}
+
+#[pinned_drop]
+impl PinnedDrop for RegistrationInner {
+    fn drop(self: Pin<&mut Self>) {
+        // SAFETY:
+        //
+        // Safe as per the invariants of `RegistrationInner` and:
+        //
+        // - The containing struct is `!Unpin` and was initialized using
+        // pin-init, so it occupied the same memory location for the entirety of
+        // its lifetime.
+        //
+        // Notice that this will block until all handlers finish executing,
+        // i.e.: at no point will &self be invalid while the handler is running.
+        unsafe { bindings::free_irq(self.irq, self.cookie) };
+    }
+}
+
+// SAFETY: We only use `inner` on drop, which called at most once with no
+// concurrent access.
+unsafe impl Sync for RegistrationInner {}
+
+// SAFETY: It is safe to send `RegistrationInner` across threads.
+unsafe impl Send for RegistrationInner {}
+
+/// A request for an IRQ line for a given device.
+///
+/// # Invariants
+///
+/// - `ìrq` is the number of an interrupt source of `dev`.
+/// - `irq` has not been registered yet.
+pub struct IrqRequest<'a> {
+    dev: &'a Device<Bound>,
+    irq: u32,
+}
+

For example for Resource the elements size, start, name and flags are accessible. Inspired by that, what do you think about exposing the irq number here, as well?

rust/kernel/irq/request.rs +5 -0

index bd489b8d2386..767d66e3e6c7 100644
@@ -123,6 +123,11 @@ impl<'a> IrqRequest<'a> {
      pub(crate) unsafe fn new(dev: &'a Device<Bound>, irq: u32) -> Self {
          IrqRequest { dev, irq }
      }
+
+    /// Returns the IRQ number of an [`IrqRequest`].
+    pub fn irq(&self) -> u32 {
+        self.irq
+    }
  }
 
 

I'm using that for some dev_info!().

rust/kernel/irq/request.rs line 48

Quoted 12 lines · Daniel Almeida

rust/kernel/irq/request.rs new file +12 -0

+/// Callbacks for an IRQ handler.
+pub trait Handler: Sync {
+    /// The hard IRQ handler.
+    ///
+    /// This is executed in interrupt context, hence all corresponding
+    /// limitations do apply.
+    ///
+    /// All work that does not necessarily need to be executed from
+    /// interrupt context, should be deferred to a threaded handler.
+    /// See also [`ThreadedRegistration`].
+    fn handle(&self) -> IrqReturn;
+}

One thing I forgot, the IRQ handlers should have a &Device<Bound> argument, i.e.:

fn handle(&self, dev: &Device<Bound>) -> IrqReturn

IRQ registrations naturally give us this guarantee, so we should take advantage of that.

- Danilo

Quoted 21 lines · Danilo Krummrich

rust/kernel/irq/request.rs line 48

Quoted 12 lines · Daniel Almeida

rust/kernel/irq/request.rs new file +12 -0

+/// Callbacks for an IRQ handler.
+pub trait Handler: Sync {
+    /// The hard IRQ handler.
+    ///
+    /// This is executed in interrupt context, hence all corresponding
+    /// limitations do apply.
+    ///
+    /// All work that does not necessarily need to be executed from
+    /// interrupt context, should be deferred to a threaded handler.
+    /// See also [`ThreadedRegistration`].
+    fn handle(&self) -> IrqReturn;
+}

One thing I forgot, the IRQ handlers should have a &Device<Bound> argument, i.e.:

Quoted 6 lines

fn handle(&self, dev: &Device<Bound>) -> IrqReturn

IRQ registrations naturally give us this guarantee, so we should take advantage of that.

- Danilo

Hi Danilo,

I do not immediately see a way to get a Device<Bound> from here:

unsafe extern "C" fn handle_irq_callback<T: Handler>(_irq: i32, ptr: *mut c_void) -> c_uint {

Refall that we've established ptr to be the address of the handler. This came after some back and forth and after the extensive discussion that Benno and Boqun had w.r.t to pinning in request_irq().

Quoted 30 lines · Daniel Almeida

Quoted 21 lines · Danilo Krummrich

rust/kernel/irq/request.rs line 48

Quoted 12 lines · Daniel Almeida

rust/kernel/irq/request.rs new file +12 -0

+/// Callbacks for an IRQ handler.
+pub trait Handler: Sync {
+    /// The hard IRQ handler.
+    ///
+    /// This is executed in interrupt context, hence all corresponding
+    /// limitations do apply.
+    ///
+    /// All work that does not necessarily need to be executed from
+    /// interrupt context, should be deferred to a threaded handler.
+    /// See also [`ThreadedRegistration`].
+    fn handle(&self) -> IrqReturn;
+}

One thing I forgot, the IRQ handlers should have a &Device<Bound> argument, i.e.:

Quoted 6 lines

fn handle(&self, dev: &Device<Bound>) -> IrqReturn

IRQ registrations naturally give us this guarantee, so we should take advantage of that.

- Danilo

Hi Danilo,

Quoted 7 lines

I do not immediately see a way to get a Device<Bound> from here:

unsafe extern "C" fn handle_irq_callback<T: Handler>(_irq: i32, ptr: *mut c_void) -> c_uint {

Refall that we've established ptr to be the address of the handler. This came after some back and forth and after the extensive discussion that Benno and Boqun had w.r.t to pinning in request_irq().

You can just wrap the Handler in a new type and store the pointer there:

	#[pin_data]
	struct Wrapper {
	   #[pin]
	   handler: T,
	   dev: NonNull<Device<Bound>>,
	}

And then pass a pointer to the Wrapper field to request_irq(); handle_irq_callback() can construct a &T and a &Device<Bound> from this.

Note that storing a device pointer, without its own reference count, is perfectly fine, since inner (Devres<RegistrationInner>) already holds a reference to the device and guarantees the bound scope for the handler callbacks.

It makes sense to document this as an invariant of Wrapper (or whatever we end up calling it).

Quoted 51 lines · Danilo Krummrich

Quoted 30 lines · Daniel Almeida

Quoted 21 lines · Danilo Krummrich

rust/kernel/irq/request.rs line 48

Quoted 12 lines · Daniel Almeida

rust/kernel/irq/request.rs new file +12 -0

+/// Callbacks for an IRQ handler.
+pub trait Handler: Sync {
+    /// The hard IRQ handler.
+    ///
+    /// This is executed in interrupt context, hence all corresponding
+    /// limitations do apply.
+    ///
+    /// All work that does not necessarily need to be executed from
+    /// interrupt context, should be deferred to a threaded handler.
+    /// See also [`ThreadedRegistration`].
+    fn handle(&self) -> IrqReturn;
+}

One thing I forgot, the IRQ handlers should have a &Device<Bound> argument, i.e.:

Quoted 6 lines

fn handle(&self, dev: &Device<Bound>) -> IrqReturn

IRQ registrations naturally give us this guarantee, so we should take advantage of that.

- Danilo

Hi Danilo,

Quoted 7 lines

I do not immediately see a way to get a Device<Bound> from here:

unsafe extern "C" fn handle_irq_callback<T: Handler>(_irq: i32, ptr: *mut c_void) -> c_uint {

Refall that we've established ptr to be the address of the handler. This came after some back and forth and after the extensive discussion that Benno and Boqun had w.r.t to pinning in request_irq().

You can just wrap the Handler in a new type and store the pointer there:

	#[pin_data]
	struct Wrapper {
	   #[pin]
	   handler: T,
	   dev: NonNull<Device<Bound>>,
	}

And then pass a pointer to the Wrapper field to request_irq(); handle_irq_callback() can construct a &T and a &Device<Bound> from this.

Quoted 4 lines

Note that storing a device pointer, without its own reference count, is perfectly fine, since inner (Devres<RegistrationInner>) already holds a reference to the device and guarantees the bound scope for the handler callbacks.

Can't we just add an accessor function to Devres?

Also Devres only stores Device<Normal>, not Device<Bound>...

Quoted 52 lines · Benno Lossin

Quoted 51 lines · Danilo Krummrich

Quoted 30 lines · Daniel Almeida

Quoted 21 lines · Danilo Krummrich

rust/kernel/irq/request.rs line 48

Quoted 12 lines · Daniel Almeida

rust/kernel/irq/request.rs new file +12 -0

+/// Callbacks for an IRQ handler.
+pub trait Handler: Sync {
+    /// The hard IRQ handler.
+    ///
+    /// This is executed in interrupt context, hence all corresponding
+    /// limitations do apply.
+    ///
+    /// All work that does not necessarily need to be executed from
+    /// interrupt context, should be deferred to a threaded handler.
+    /// See also [`ThreadedRegistration`].
+    fn handle(&self) -> IrqReturn;
+}

One thing I forgot, the IRQ handlers should have a &Device<Bound> argument, i.e.:

Quoted 6 lines

fn handle(&self, dev: &Device<Bound>) -> IrqReturn

IRQ registrations naturally give us this guarantee, so we should take advantage of that.

- Danilo

Hi Danilo,

Quoted 7 lines

I do not immediately see a way to get a Device<Bound> from here:

unsafe extern "C" fn handle_irq_callback<T: Handler>(_irq: i32, ptr: *mut c_void) -> c_uint {

Refall that we've established ptr to be the address of the handler. This came after some back and forth and after the extensive discussion that Benno and Boqun had w.r.t to pinning in request_irq().

You can just wrap the Handler in a new type and store the pointer there:

	#[pin_data]
	struct Wrapper {
	   #[pin]
	   handler: T,
	   dev: NonNull<Device<Bound>>,
	}

And then pass a pointer to the Wrapper field to request_irq(); handle_irq_callback() can construct a &T and a &Device<Bound> from this.

Quoted 6 lines

Quoted 4 lines

Note that storing a device pointer, without its own reference count, is perfectly fine, since inner (Devres<RegistrationInner>) already holds a reference to the device and guarantees the bound scope for the handler callbacks.

Can't we just add an accessor function to Devres?

	#[pin_data]
	pub struct Registration<T: Handler + 'static> {
	    #[pin]
	    inner: Devres<RegistrationInner>,
	
	    #[pin]
	    handler: T,
	
	    /// Pinned because we need address stability so that we can pass a pointer
	    /// to the callback.
	    #[pin]
	    _pin: PhantomPinned,
	}

Currently we pass the address of handler to request_irq(), so this doesn't help, hence my proposal to replace the above T with Wrapper (actually Wrapper<T>).

Quoted 1 line

Also Devres only stores Device<Normal>, not Device<Bound>...

The Devres instance itself may out-live device unbind, but it ensures that the encapsulated data does not, hence it holds a reference count, i.e. ARef<Device>.

Device<Bound> or ARef<Device<Bound>> *never* exists, only &'a Device<Bound> within a corresponding scope for which we can guarantee the device is bound.

In the proposed wrapper we can store a NonNull<Device<Bound>> though, because we can safely give out a &Device<Bound> in the IRQ's handle() callback. This is because:

(1) RegistrationInner is guarded by Devres and guarantees that free_irq() is completed *before* the device is unbound.

(2) It is guaranteed that the device pointer is valid because (1) guarantees it's even bound and because Devres<RegistrationInner> itself has a reference count.

Quoted 74 lines · Danilo Krummrich

Quoted 52 lines · Benno Lossin

Quoted 51 lines · Danilo Krummrich

Quoted 30 lines · Daniel Almeida

Quoted 21 lines · Danilo Krummrich

Review

Quoted 12 lines · Daniel Almeida

(quoted diff) +12 -0

+/// Callbacks for an IRQ handler.
+pub trait Handler: Sync {
+    /// The hard IRQ handler.
+    ///
+    /// This is executed in interrupt context, hence all corresponding
+    /// limitations do apply.
+    ///
+    /// All work that does not necessarily need to be executed from
+    /// interrupt context, should be deferred to a threaded handler.
+    /// See also [`ThreadedRegistration`].
+    fn handle(&self) -> IrqReturn;
+}

One thing I forgot, the IRQ handlers should have a &Device<Bound> argument, i.e.:

Quoted 6 lines

fn handle(&self, dev: &Device<Bound>) -> IrqReturn

IRQ registrations naturally give us this guarantee, so we should take advantage of that.

- Danilo

Hi Danilo,

Quoted 7 lines

I do not immediately see a way to get a Device<Bound> from here:

unsafe extern "C" fn handle_irq_callback<T: Handler>(_irq: i32, ptr: *mut c_void) -> c_uint {

Refall that we've established ptr to be the address of the handler. This came after some back and forth and after the extensive discussion that Benno and Boqun had w.r.t to pinning in request_irq().

You can just wrap the Handler in a new type and store the pointer there:

	#[pin_data]
	struct Wrapper {
	   #[pin]
	   handler: T,
	   dev: NonNull<Device<Bound>>,
	}

And then pass a pointer to the Wrapper field to request_irq(); handle_irq_callback() can construct a &T and a &Device<Bound> from this.

Quoted 6 lines

Quoted 4 lines

Note that storing a device pointer, without its own reference count, is perfectly fine, since inner (Devres<RegistrationInner>) already holds a reference to the device and guarantees the bound scope for the handler callbacks.

Can't we just add an accessor function to Devres?

	#[pin_data]
	pub struct Registration<T: Handler + 'static> {
	    #[pin]
	    inner: Devres<RegistrationInner>,
	
	    #[pin]
	    handler: T,
	
	    /// Pinned because we need address stability so that we can pass a pointer
	    /// to the callback.
	    #[pin]
	    _pin: PhantomPinned,
	}

Quoted 2 lines

Currently we pass the address of handler to request_irq(), so this doesn't help, hence my proposal to replace the above T with Wrapper (actually Wrapper<T>).

You can just use container_of!?

Quoted 8 lines

Quoted 1 line

Also Devres only stores Device<Normal>, not Device<Bound>...

The Devres instance itself may out-live device unbind, but it ensures that the encapsulated data does not, hence it holds a reference count, i.e. ARef<Device>.

Device<Bound> or ARef<Device<Bound>> *never* exists, only &'a Device<Bound> within a corresponding scope for which we can guarantee the device is bound.

Quoted 6 lines

In the proposed wrapper we can store a NonNull<Device<Bound>> though, because we can safely give out a &Device<Bound> in the IRQ's handle() callback. This is because:

(1) RegistrationInner is guarded by Devres and guarantees that free_irq() is completed *before* the device is unbound.

How does it ensure that?

Quoted 4 lines

(2) It is guaranteed that the device pointer is valid because (1) guarantees it's even bound and because Devres<RegistrationInner> itself has a reference count.

Yeah but I would find it much more natural (and also useful in other circumstances) if Devres<T> would give you access to Device (at least the Normal type state).

Depending on how (1) is ensured, we might just need an unsafe function that turns Device<Normal> into Device<Bound>.

On a second look, I wonder how useful this will be.

fn handle(&self, dev: &Device<Bound>) -> IrqReturn

Sorry for borrowing this terminology, but here we offer Device<Bound>, while I suspect that most drivers will be looking for the most derived Device type instead. So for drm drivers this will be drm::Device, for example, not the base dev::Device type. I assume that this pattern will hold for other subsystems as well.

Which brings me to my second point: drivers can store an ARef<drm::Device> on the handler itself, and I assume that the same will be possible in other subsystems.

Quoted 10 lines · Daniel Almeida

On a second look, I wonder how useful this will be.

fn handle(&self, dev: &Device<Bound>) -> IrqReturn

Sorry for borrowing this terminology, but here we offer Device<Bound>, while I suspect that most drivers will be looking for the most derived Device type instead. So for drm drivers this will be drm::Device, for example, not the base dev::Device type. I assume that this pattern will hold for other subsystems as well.

Quoted 3 lines

Which brings me to my second point: drivers can store an ARef<drm::Device> on the handler itself, and I assume that the same will be possible in other subsystems.

Well, the whole point is that you can use a &Device<Bound> to directly access device resources without any overhead, i.e.

	fn handle(&self, dev: &Device<Bound>) -> IrqReturn {
	   let io = self.iomem.access(dev);

	   io.write32(...);
	}

Quoted 18 lines · Danilo Krummrich

Quoted 10 lines · Daniel Almeida

On a second look, I wonder how useful this will be.

fn handle(&self, dev: &Device<Bound>) -> IrqReturn

Sorry for borrowing this terminology, but here we offer Device<Bound>, while I suspect that most drivers will be looking for the most derived Device type instead. So for drm drivers this will be drm::Device, for example, not the base dev::Device type. I assume that this pattern will hold for other subsystems as well.

Quoted 3 lines

Which brings me to my second point: drivers can store an ARef<drm::Device> on the handler itself, and I assume that the same will be possible in other subsystems.

Well, the whole point is that you can use a &Device<Bound> to directly access device resources without any overhead, i.e.

Quoted 5 lines

fn handle(&self, dev: &Device<Bound>) -> IrqReturn {
let io = self.iomem.access(dev);

io.write32(...);
}

So, yes, you can store anything in your handler, but the &Device<Bound> is a cookie for the scope.

Quoted 26 lines · Danilo Krummrich

Quoted 21 lines · Danilo Krummrich

Quoted 10 lines · Daniel Almeida

On a second look, I wonder how useful this will be.

fn handle(&self, dev: &Device<Bound>) -> IrqReturn

Sorry for borrowing this terminology, but here we offer Device<Bound>, while I suspect that most drivers will be looking for the most derived Device type instead. So for drm drivers this will be drm::Device, for example, not the base dev::Device type. I assume that this pattern will hold for other subsystems as well.

Quoted 3 lines

Which brings me to my second point: drivers can store an ARef<drm::Device> on the handler itself, and I assume that the same will be possible in other subsystems.

Well, the whole point is that you can use a &Device<Bound> to directly access device resources without any overhead, i.e.

fn handle(&self, dev: &Device<Bound>) -> IrqReturn {
let io = self.iomem.access(dev);

Quoted 5 lines

Quoted 2 lines

io.write32(...);
}

So, yes, you can store anything in your handler, but the &Device<Bound> is a cookie for the scope.

Fine, but can’t you get a &Device<Bound> from a ARef<drm::Device>, for example? Perhaps a nicer solution would be to offer this capability instead?

Quoted 4 lines

Quoted 9 lines

(2) Owning a reference count of a device (i.e. ARef<Device>) does *not* guarantee that the device is bound. You can own a reference count to the device object way beyond it being bound. Instead, the guarantee comes from the scope.

In this case, the scope is the IRQ callback, since the irq::Registration guarantees to call and complete free_irq() before the underlying bus device is unbound.

Oh, I see. I guess this is where I started to get a bit confused indeed.

— Daniel

Quoted 18 lines · Daniel Almeida

Quoted 13 lines

(2) Owning a reference count of a device (i.e. ARef<Device>) does *not* guarantee that the device is bound. You can own a reference count to the device object way beyond it being bound. Instead, the guarantee comes from the scope.

In this case, the scope is the IRQ callback, since the irq::Registration guarantees to call and complete free_irq() before the underlying bus device is unbound.

Quoted 3 lines

Oh, I see. I guess this is where I started to get a bit confused indeed.

— Daniel

Fine, I guess I can submit a newer version and test that on Tyr.

Dirk, can you also test the next iteration on your driver? It will possibly solve your use case as well.

Hi Daniel,

Quoted 18 lines · Daniel Almeida

Quoted 18 lines · Daniel Almeida

Quoted 13 lines

(2) Owning a reference count of a device (i.e. ARef<Device>) does *not* guarantee that the device is bound. You can own a reference count to the device object way beyond it being bound. Instead, the guarantee comes from the scope.

In this case, the scope is the IRQ callback, since the irq::Registration guarantees to call and complete free_irq() before the underlying bus device is unbound.

Quoted 8 lines

Quoted 3 lines

Oh, I see. I guess this is where I started to get a bit confused indeed.

— Daniel

Fine, I guess I can submit a newer version and test that on Tyr.

Dirk, can you also test the next iteration on your driver? It will possibly solve your use case as well.

Now, I'm slightly confused ;) I just saw your version 7 of this

[PATCH v7 3/6] rust: irq: add support for non-threaded IRQs and handlers
https://lore.kernel.org/rust-for-linux/[email protected]/

and somehow was expecting something like

fn handle(&self, dev: &Device<Bound>) -> IrqReturn

there. I.e. to get a bound device passed into the handler.

If I misunderstood the discussion and this is supposed to not be
added: Any hint how to get the &Device<Bound> from the probe function/irq registration into the irq handler, then? To be able to do something like

fn handle(&self) -> IrqReturn {
let dev = ??;
let io = self.iomem.access(dev);

fn handle(&self) -> IrqReturn {
let dev = ??;
let io = self.iomem.access(dev);

Thanks,

Dirk

Quoted 10 lines · Daniel Almeida

On a second look, I wonder how useful this will be.

fn handle(&self, dev: &Device<Bound>) -> IrqReturn

Sorry for borrowing this terminology, but here we offer Device<Bound>, while I suspect that most drivers will be looking for the most derived Device type instead. So for drm drivers this will be drm::Device, for example, not the base dev::Device type. I assume that this pattern will hold for other subsystems as well.

Quoted 3 lines

Which brings me to my second point: drivers can store an ARef<drm::Device> on the handler itself, and I assume that the same will be possible in other subsystems.

Well, the whole point is that you can use a &Device<Bound> to directly access device resources without any overhead, i.e.

	fn handle(&self, dev: &Device<Bound>) -> IrqReturn {
	   let io = self.iomem.access(dev);

	   io.write32(...);

Quoted 21 lines · Danilo Krummrich

Quoted 10 lines · Daniel Almeida

On a second look, I wonder how useful this will be.

fn handle(&self, dev: &Device<Bound>) -> IrqReturn

Sorry for borrowing this terminology, but here we offer Device<Bound>, while I suspect that most drivers will be looking for the most derived Device type instead. So for drm drivers this will be drm::Device, for example, not the base dev::Device type. I assume that this pattern will hold for other subsystems as well.

Quoted 3 lines

Which brings me to my second point: drivers can store an ARef<drm::Device> on the handler itself, and I assume that the same will be possible in other subsystems.

Well, the whole point is that you can use a &Device<Bound> to directly access device resources without any overhead, i.e.

	fn handle(&self, dev: &Device<Bound>) -> IrqReturn {
	   let io = self.iomem.access(dev);

Quoted 1 line

	   io.write32(...);

As this is exactly the example I was discussing privately with Daniel (many thanks!), independent on the device discussion here, just for my understanding:

Is it ok to do a 'self.iomem.access(dev)' at each interrupt?

Wouldn't it
be cheaper/faster to pass 'io' instead of 'iomem' to the interrupt handler?

Quoted 75 lines · Benno Lossin

Quoted 74 lines · Danilo Krummrich

Quoted 52 lines · Benno Lossin

Quoted 51 lines · Danilo Krummrich

Quoted 30 lines · Daniel Almeida

Quoted 21 lines · Danilo Krummrich

Review

Quoted 12 lines · Daniel Almeida

(quoted diff) +12 -0

+/// Callbacks for an IRQ handler.
+pub trait Handler: Sync {
+    /// The hard IRQ handler.
+    ///
+    /// This is executed in interrupt context, hence all corresponding
+    /// limitations do apply.
+    ///
+    /// All work that does not necessarily need to be executed from
+    /// interrupt context, should be deferred to a threaded handler.
+    /// See also [`ThreadedRegistration`].
+    fn handle(&self) -> IrqReturn;
+}

One thing I forgot, the IRQ handlers should have a &Device<Bound> argument, i.e.:

Quoted 6 lines

fn handle(&self, dev: &Device<Bound>) -> IrqReturn

IRQ registrations naturally give us this guarantee, so we should take advantage of that.

- Danilo

Hi Danilo,

Quoted 7 lines

I do not immediately see a way to get a Device<Bound> from here:

unsafe extern "C" fn handle_irq_callback<T: Handler>(_irq: i32, ptr: *mut c_void) -> c_uint {

Refall that we've established ptr to be the address of the handler. This came after some back and forth and after the extensive discussion that Benno and Boqun had w.r.t to pinning in request_irq().

You can just wrap the Handler in a new type and store the pointer there:

	#[pin_data]
	struct Wrapper {
	   #[pin]
	   handler: T,
	   dev: NonNull<Device<Bound>>,
	}

And then pass a pointer to the Wrapper field to request_irq(); handle_irq_callback() can construct a &T and a &Device<Bound> from this.

Quoted 6 lines

Quoted 4 lines

Note that storing a device pointer, without its own reference count, is perfectly fine, since inner (Devres<RegistrationInner>) already holds a reference to the device and guarantees the bound scope for the handler callbacks.

Can't we just add an accessor function to Devres?

	#[pin_data]
	pub struct Registration<T: Handler + 'static> {
	    #[pin]
	    inner: Devres<RegistrationInner>,
	
	    #[pin]
	    handler: T,
	
	    /// Pinned because we need address stability so that we can pass a pointer
	    /// to the callback.
	    #[pin]
	    _pin: PhantomPinned,
	}

Quoted 4 lines

Quoted 2 lines

Currently we pass the address of handler to request_irq(), so this doesn't help, hence my proposal to replace the above T with Wrapper (actually Wrapper<T>).

You can just use container_of!?

Sure, that's possible too.

Quoted 12 lines

Quoted 12 lines

Quoted 1 line

Also Devres only stores Device<Normal>, not Device<Bound>...

The Devres instance itself may out-live device unbind, but it ensures that the encapsulated data does not, hence it holds a reference count, i.e. ARef<Device>.

Device<Bound> or ARef<Device<Bound>> *never* exists, only &'a Device<Bound> within a corresponding scope for which we can guarantee the device is bound.

In the proposed wrapper we can store a NonNull<Device<Bound>> though, because we can safely give out a &Device<Bound> in the IRQ's handle() callback. This is because:

Quoted 4 lines

Quoted 2 lines

(1) RegistrationInner is guarded by Devres and guarantees that free_irq() is completed *before* the device is unbound.

How does it ensure that?

RegistrationInner calls free_irq() in it's drop() method; Devres revokes it on device unbind.

Quoted 8 lines

Quoted 4 lines

(2) It is guaranteed that the device pointer is valid because (1) guarantees it's even bound and because Devres<RegistrationInner> itself has a reference count.

Yeah but I would find it much more natural (and also useful in other circumstances) if Devres<T> would give you access to Device (at least the Normal type state).

If we use container_of!() instead or just pass the address of Self (i.e. Registration) to request_irq() instead,

	pub fn device(&self) -> &Device

is absolutely possible to add to Devres, of course.

Quoted 2 lines

Depending on how (1) is ensured, we might just need an unsafe function that turns Device<Normal> into Device<Bound>.

&Device<Normal> in &Device<Bound>, yes. I have such a method locally already (but haven't sent it yet), because that's going to be a use-case for other abstractions as well. One specific example is the PWM Chip abstraction [1].

[1] https://lore.kernel.org/lkml/20250710-rust-next-pwm-working-fan-for-sending-v11-3-93824a16f9ec@samsung.com/

I think this solution sounds much better than storing an additional NonNull<Device<Bound>>.

If we use container_of!() instead or just pass the address of Self (i.e. Registration) to request_irq() instead,

	pub fn device(&self) -> &Device

is absolutely possible to add to Devres, of course.

Quoted 2 lines

Depending on how (1) is ensured, we might just need an unsafe function that turns Device<Normal> into Device<Bound>.

&Device<Normal> in &Device<Bound>, yes. I have such a method locally already (but haven't sent it yet), because that's going to be a use-case for other abstractions as well. One specific example is the PWM Chip abstraction [1].

[1] https://lore.kernel.org/lkml/20250710-rust-next-pwm-working-fan-for-sending-v11-3-93824a16f9ec@samsung.com/

I think you need to reorder the initialization of inner to be after dev for this.

Quoted 2 lines · Boqun Feng

I think you need to reorder the initialization of inner to be after dev for this.

Indeed, good catch!

Quoted 6 lines

Quoted 5 lines

(2) It is guaranteed that the device pointer is valid because (1) guarantees it's even bound and because Devres<RegistrationInner> itself has a reference count.

Quoted 3 lines

Yeah but I would find it much more natural (and also useful in other circumstances) if Devres<T> would give you access to Device (at least the Normal type state).

If we use container_of!() instead or just pass the address of Self (i.e. Registration) to request_irq() instead,

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 6 lines

Well yes and no, with the Devres changes, the cookie can just be the address of the RegistrationInner & we can do it this way :)

---
Cheers,
Benno

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

No, we need this to be the address of the the whole thing (i.e. Registration<T>), otherwise you can’t access the handler in the irq callback.

You only need the access of handler in the irq callback, right? I.e. passing the address of handler would suffice (of course you need to change the irq callback as well).

Hi,

Quoted 10 lines

Quoted 9 lines

Quoted 5 lines

(2) It is guaranteed that the device pointer is valid because (1) guarantees it's even bound and because Devres<RegistrationInner> itself has a reference count.

Yeah but I would find it much more natural (and also useful in other circumstances) if Devres<T> would give you access to Device (at least the Normal type state).

Quoted 2 lines

If we use container_of!() instead or just pass the address of Self (i.e. Registration) to request_irq() instead,

Boqun, Benno, are you ok with passing the address of Registration<T> as the cookie?

Recall that this was a change requested in v4, so I am checking whether we are all on the same page before going back to that.

See [0], i.e.:

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 6 lines

Well yes and no, with the Devres changes, the cookie can just be the address of the RegistrationInner & we can do it this way :)

---
Cheers,
Benno

Quoted 7 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

No, we need this to be the address of the the whole thing (i.e. Registration<T>), otherwise you can’t access the handler in the irq callback.

You only need the access of handler in the irq callback, right? I.e. passing the address of handler would suffice (of course you need to change the irq callback as well).

— Daniel

[0] https://lore.kernel.org/all/[email protected]/

Hi,

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

(2) It is guaranteed that the device pointer is valid because (1) guarantees it's even bound and because Devres<RegistrationInner> itself has a reference count.

Quoted 6 lines

Quoted 3 lines

Yeah but I would find it much more natural (and also useful in other circumstances) if Devres<T> would give you access to Device (at least the Normal type state).

If we use container_of!() instead or just pass the address of Self (i.e. Registration) to request_irq() instead,

Boqun, Benno, are you ok with passing the address of Registration<T> as the cookie?

Recall that this was a change requested in v4, so I am checking whether we are all on the same page before going back to that.

See [0], i.e.:
[0] https://lore.kernel.org/all/[email protected]/

Quoted 22 lines · Daniel Almeida

Hi,

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

(2) It is guaranteed that the device pointer is valid because (1) guarantees it's even bound and because Devres<RegistrationInner> itself has a reference count.

Quoted 6 lines

Quoted 3 lines

Yeah but I would find it much more natural (and also useful in other circumstances) if Devres<T> would give you access to Device (at least the Normal type state).

If we use container_of!() instead or just pass the address of Self (i.e. Registration) to request_irq() instead,

Boqun, Benno, are you ok with passing the address of Registration<T> as the cookie?

Recall that this was a change requested in v4, so I am checking whether we are all on the same page before going back to that.

Quoted 2 lines

See [0], i.e.:
[0] https://lore.kernel.org/all/[email protected]/

After discussing this, Daniel and I agreed that I will implement the change adding a Device<Bound> argument to the callback. I will be sending a patch adding it separately as a follow-up to Daniel's work.

This patch adds support for threaded IRQs and handlers through irq::ThreadedRegistration and the irq::ThreadedHandler trait.

Threaded interrupts are more permissive in the sense that further processing is possible in a kthread. This means that said execution takes place outside of interrupt context, which is rather restrictive in many ways.

Registering a threaded irq is dependent upon having an IrqRequest that was previously allocated by a given device. This will be introduced in subsequent patches.

Signed-off-by Daniel Almeida <[email protected]>

These accessors can be used to retrieve a irq::Registration and irq::ThreadedRegistration from a platform device by
index or name. Alternatively, drivers can retrieve an IrqRequest from a bound platform device for later use.

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

Signed-off-by Daniel Almeida <[email protected]>

Quoted 8 lines · Daniel Almeida

These accessors can be used to retrieve a irq::Registration and irq::ThreadedRegistration from a platform device by
index or name. Alternatively, drivers can retrieve an IrqRequest from a bound platform device for later use.

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

Signed-off-by Daniel Almeida <[email protected]>

One question below. With that answered:

Reviewed-by Alice Ryhl <[email protected]>

rust/kernel/platform.rs fn as_raw(&self) -> *mut bindings::platform_device { · line 268

Quoted 2 lines

rust/kernel/platform.rs +2 -0

+    /// Returns an [`IrqRequest`] for the IRQ with the given name, if any.
+    pub fn request_irq_by_name(&self, name: &'static CStr) -> Result<IrqRequest<'_>> {

Does the name need to be static? That's surprising - isn't it just a lookup that needs to be valid during this call?

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 <[email protected]>

Quoted 7 lines · Daniel Almeida

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 <[email protected]>

Same question as patch 5.

With that answered:

Reviewed-by Alice Ryhl <[email protected]>

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

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

Daniel Almeida (6):
rust: irq: add irq module
rust: irq: add flags module
rust: irq: add support for non-threaded IRQs and handlers
rust: irq: add support for threaded IRQs and handlers
rust: platform: add irq accessors
rust: pci: add irq accessors