[PATCH v2] rust: irq: add support for request_irq()

[PATCH v2] rust: irq: add support for request_irq()

[Daniel Almeida]

Add support for registering IRQ handlers in Rust.

IRQ handlers are extensively used in drivers when some peripheral wants to
obtain the CPU attention. Registering a handler will make the system invoke the
passed-in function whenever the chosen IRQ line is triggered.

Both regular and threaded IRQ handlers are supported through a Handler (or
ThreadedHandler) trait that is meant to be implemented by a type that:

a) provides a function to be run by the system when the IRQ fires and,

b) holds the shared data (i.e.: `T`) between process and IRQ contexts.

The requirement that T is Sync derives from the fact that handlers might run
concurrently with other processes executing the same driver, creating the
potential for data races.

Ideally, some interior mutability must be in place if T is to be mutated. This
should usually be done through the in-flight SpinLockIrq type.

Co-developed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
---

Changes from v1:

- Added Co-developed-by tag to account for the work that Alice did in order to
figure out how to do this without Opaque<T> (Thanks!)
- Removed Opaque<T> in favor of plain T
- Fixed the examples
- Made sure that the invariants sections are the last entry in the docs
- Switched to slot.cast() where applicable,
- Mentioned in the safety comments that we require that T: Sync,
- Removed ThreadedFnReturn in favor of IrqReturn,
- Improved the commit message

Link to v1: https://lore.kernel.org/rust-for-linux/20241024-topic-panthor-rs-request_irq-v1-1-7cbc51c182ca@collabora.com/


---
 rust/bindings/bindings_helper.h |   1 +
 rust/helpers/helpers.c          |   1 +
 rust/helpers/irq.c              |   9 +
 rust/kernel/irq.rs              |   6 +
 rust/kernel/irq/request.rs      | 517 ++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs              |   1 +
 6 files changed, 535 insertions(+)
 create mode 100644 rust/helpers/irq.c
 create mode 100644 rust/kernel/irq.rs
 create mode 100644 rust/kernel/irq/request.rs

diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 5c4dfe22f41a..0331c6273def 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -15,6 +15,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>
diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
index dcf827a61b52..bfc499d7f4c1 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c
@@ -13,6 +13,7 @@
 #include "build_bug.c"
 #include "cred.c"
 #include "err.c"
+#include "irq.c"
 #include "fs.c"
 #include "jump_label.c"
 #include "kunit.c"
diff --git a/rust/helpers/irq.c b/rust/helpers/irq.c
new file mode 100644
index 000000000000..1faca428e2c0
--- /dev/null
+++ b/rust/helpers/irq.c
@@ -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);
+}
diff --git a/rust/kernel/irq.rs b/rust/kernel/irq.rs
new file mode 100644
index 000000000000..3ab83c5bdb83
--- /dev/null
+++ b/rust/kernel/irq.rs
@@ -0,0 +1,6 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! IRQ abstractions
+
+/// IRQ allocation and handling
+pub mod request;
diff --git a/rust/kernel/irq/request.rs b/rust/kernel/irq/request.rs
new file mode 100644
index 000000000000..61e7d4a8f555
--- /dev/null
+++ b/rust/kernel/irq/request.rs
@@ -0,0 +1,517 @@
+// SPDX-License-Identifier: GPL-2.0
+// SPDX-FileCopyrightText: Copyright 2025 Collabora ltd.
+
+//! IRQ allocation and handling
+
+use core::marker::PhantomPinned;
+use core::ptr::addr_of_mut;
+
+use init::pin_init_from_closure;
+
+use crate::error::to_result;
+use crate::prelude::*;
+use crate::str::CStr;
+
+/// Flags to be used when registering IRQ handlers.
+///
+/// They can be combined with the operators `|`, `&`, and `!`.
+///
+/// Values can be used from the [`flags`] module.
+#[derive(Clone, Copy)]
+pub struct Flags(u64);
+
+impl core::ops::BitOr for Flags {
+    type Output = Self;
+    fn bitor(self, rhs: Self) -> Self::Output {
+        Self(self.0 | rhs.0)
+    }
+}
+
+impl core::ops::BitAnd for Flags {
+    type Output = Self;
+    fn bitand(self, rhs: Self) -> Self::Output {
+        Self(self.0 & rhs.0)
+    }
+}
+
+impl core::ops::Not for Flags {
+    type Output = Self;
+    fn not(self) -> Self::Output {
+        Self(!self.0)
+    }
+}
+
+/// The flags that can be used when registering an IRQ handler.
+pub mod flags {
+    use super::Flags;
+
+    use crate::bindings;
+
+    /// Use the interrupt line as already configured.
+    pub const TRIGGER_NONE: Flags = Flags(bindings::IRQF_TRIGGER_NONE as _);
+
+    /// The interrupt is triggered when the signal goes from low to high.
+    pub const TRIGGER_RISING: Flags = Flags(bindings::IRQF_TRIGGER_RISING as _);
+
+    /// The interrupt is triggered when the signal goes from high to low.
+    pub const TRIGGER_FALLING: Flags = Flags(bindings::IRQF_TRIGGER_FALLING as _);
+
+    /// The interrupt is triggered while the signal is held high.
+    pub const TRIGGER_HIGH: Flags = Flags(bindings::IRQF_TRIGGER_HIGH as _);
+
+    /// The interrupt is triggered while the signal is held low.
+    pub const TRIGGER_LOW: Flags = Flags(bindings::IRQF_TRIGGER_LOW as _);
+
+    /// Allow sharing the irq among several devices.
+    pub const SHARED: Flags = Flags(bindings::IRQF_SHARED as _);
+
+    /// Set by callers when they expect sharing mismatches to occur.
+    pub const PROBE_SHARED: Flags = Flags(bindings::IRQF_PROBE_SHARED as _);
+
+    /// Flag to mark this interrupt as timer interrupt.
+    pub const TIMER: Flags = Flags(bindings::IRQF_TIMER as _);
+
+    /// Interrupt is per cpu.
+    pub const PERCPU: Flags = Flags(bindings::IRQF_PERCPU as _);
+
+    /// Flag to exclude this interrupt from irq balancing.
+    pub const NOBALANCING: Flags = Flags(bindings::IRQF_NOBALANCING as _);
+
+    /// Interrupt is used for polling (only the interrupt that is registered
+    /// first in a shared interrupt is considered for performance reasons).
+    pub const IRQPOLL: Flags = Flags(bindings::IRQF_IRQPOLL as _);
+
+    /// Interrupt is not reenabled after the hardirq handler finished. Used by
+    /// threaded interrupts which need to keep the irq line disabled until the
+    /// threaded handler has been run.
+    pub const ONESHOT: Flags = Flags(bindings::IRQF_ONESHOT as _);
+
+    /// Do not disable this IRQ during suspend. Does not guarantee that this
+    /// interrupt will wake the system from a suspended state.
+    pub const NO_SUSPEND: Flags = Flags(bindings::IRQF_NO_SUSPEND as _);
+
+    /// Force enable it on resume even if [`NO_SUSPEND`] is set.
+    pub const FORCE_RESUME: Flags = Flags(bindings::IRQF_FORCE_RESUME as _);
+
+    /// Interrupt cannot be threaded.
+    pub const NO_THREAD: Flags = Flags(bindings::IRQF_NO_THREAD as _);
+
+    /// Resume IRQ early during syscore instead of at device resume time.
+    pub const EARLY_RESUME: Flags = Flags(bindings::IRQF_EARLY_RESUME as _);
+
+    /// If the IRQ is shared with a NO_SUSPEND user, execute this interrupt
+    /// handler after suspending interrupts. For system wakeup devices users
+    /// need to implement wakeup detection in their interrupt handlers.
+    pub const COND_SUSPEND: Flags = Flags(bindings::IRQF_COND_SUSPEND as _);
+
+    /// Don't enable IRQ or NMI automatically when users request it. Users will
+    /// enable it explicitly by `enable_irq` or `enable_nmi` later.
+    pub const NO_AUTOEN: Flags = Flags(bindings::IRQF_NO_AUTOEN as _);
+
+    /// Exclude from runnaway detection for IPI and similar handlers, depends on
+    /// `PERCPU`.
+    pub const NO_DEBUG: Flags = Flags(bindings::IRQF_NO_DEBUG as _);
+}
+
+/// 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 as _,
+
+    /// The interrupt was handled by this device.
+    Handled = bindings::irqreturn_IRQ_HANDLED as _,
+}
+
+/// Callbacks for an IRQ handler.
+pub trait Handler: Sync {
+    /// The actual handler function. As usual, sleeps are not allowed in IRQ
+    /// context.
+    fn handle_irq(&self) -> IrqReturn;
+}
+
+/// A registration of an IRQ handler for a given IRQ line.
+///
+/// # Examples
+///
+/// The following is an example of using `Registration`:
+///
+/// ```
+/// use kernel::prelude::*;
+/// use kernel::irq::request::flags;
+/// use kernel::irq::request::Registration;
+/// use kernel::irq::request::IrqReturn;
+/// use kernel::sync::Arc;
+/// use kernel::sync::SpinLock;
+/// 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(SpinLock<u32>);
+///
+/// // [`handle_irq`] takes &self. This example illustrates interior
+/// // mutability can be used when share the data between process context and IRQ
+/// // context.
+/// //
+/// // Ideally, this example would be using a version of SpinLock that is aware
+/// // of `spin_lock_irqsave` and `spin_lock_irqrestore`, but that is not yet
+/// // implemented.
+///
+/// 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_irq(&self) -> IrqReturn {
+///         // We now have exclusive access to the data by locking the SpinLock.
+///         let mut data = self.0.lock();
+///         *data += 1;
+///
+///         IrqReturn::Handled
+///     }
+/// }
+///
+/// // This is running in process context.
+/// fn register_irq(irq: u32, handler: Handler) -> Result<Arc<Registration<Handler>>> {
+///     let registration = Registration::register(irq, flags::SHARED, c_str!("my-device"), handler);
+///
+///     // You can have as many references to the registration as you want, so
+///     // multiple parts of the driver can access it.
+///     let registration = Arc::pin_init(registration, GFP_KERNEL)?;
+///
+///     // The handler may be called immediately after the function above
+///     // returns, possibly in a different CPU.
+///
+///     {
+///         // The data can be accessed from the process context too.
+///         let mut data = registration.handler().0.lock();
+///         *data = 42;
+///     }
+///
+///     Ok(registration)
+/// }
+///
+/// # Ok::<(), Error>(())
+///```
+///
+/// # Invariants
+///
+/// * We own an irq handler using `&self` as its private data.
+///
+#[pin_data(PinnedDrop)]
+pub struct Registration<T: Handler> {
+    irq: u32,
+    #[pin]
+    handler: T,
+    #[pin]
+    /// Pinned because we need address stability so that we can pass a pointer
+    /// to the callback.
+    _pin: PhantomPinned,
+}
+
+impl<T: Handler> Registration<T> {
+    /// Registers the IRQ handler with the system for the given IRQ number. The
+    /// handler must be able to be called as soon as this function returns.
+    pub fn register(
+        irq: u32,
+        flags: Flags,
+        name: &'static CStr,
+        handler: T,
+    ) -> impl PinInit<Self, Error> {
+        let closure = move |slot: *mut Self| {
+            // SAFETY: The slot passed to pin initializer is valid for writing.
+            unsafe {
+                slot.write(Self {
+                    irq,
+                    handler,
+                    _pin: PhantomPinned,
+                })
+            };
+
+            // 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.
+            let res = to_result(unsafe {
+                bindings::request_irq(
+                    irq,
+                    Some(handle_irq_callback::<T>),
+                    flags.0,
+                    name.as_char_ptr(),
+                    &*slot as *const _ as *mut core::ffi::c_void,
+                )
+            });
+
+            if res.is_err() {
+                // SAFETY: We are returning an error, so we can destroy the slot.
+                unsafe { core::ptr::drop_in_place(addr_of_mut!((*slot).handler)) };
+            }
+
+            res
+        };
+
+        // SAFETY:
+        // - if this returns Ok, then every field of `slot` is fully
+        // initialized.
+        // - if this returns an error, then the slot does not need to remain
+        // valid.
+        unsafe { pin_init_from_closure(closure) }
+    }
+
+    /// Returns a reference to the handler that was registered with the system.
+    pub fn handler(&self) -> &T {
+        // SAFETY: `handler` is initialized in `register`, and we require that
+        // T: Sync.
+        &self.handler
+    }
+}
+
+#[pinned_drop]
+impl<T: Handler> PinnedDrop for Registration<T> {
+    fn drop(self: Pin<&mut Self>) {
+        // SAFETY:
+        // - `self.irq` is the same as the one passed to `reques_irq`.
+        // -  `&self` was passed to `request_irq` as the cookie. It is
+        // guaranteed to be unique by the type system, since each call to
+        // `register` will return a different instance of `Registration`.
+        //
+        // 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 as *const Self as *mut core::ffi::c_void) };
+    }
+}
+
+/// The value that can be returned from `ThreadedHandler::handle_irq`.
+pub enum ThreadedIrqReturn {
+    /// The interrupt was not from this device or was not handled.
+    None = bindings::irqreturn_IRQ_NONE as _,
+
+    /// The interrupt was handled by this device.
+    Handled = bindings::irqreturn_IRQ_HANDLED as _,
+
+    /// The handler wants the handler thread to wake up.
+    WakeThread = bindings::irqreturn_IRQ_WAKE_THREAD as _,
+}
+
+/// Callbacks for a threaded IRQ handler.
+pub trait ThreadedHandler: Sync {
+    /// The actual handler function. As usual, sleeps are not allowed in IRQ
+    /// context.
+    fn handle_irq(&self) -> ThreadedIrqReturn;
+
+    /// The threaded handler function. This function is called from the irq
+    /// handler thread, which is automatically created by the system.
+    fn thread_fn(&self) -> IrqReturn;
+}
+
+/// A registration of a threaded IRQ handler for a given IRQ line.
+///
+/// Two callbacks are required: one to handle the IRQ, and one to handle any
+/// other work in a separate thread.
+///
+/// The thread handler is only called if the IRQ handler returns `WakeThread`.
+///
+/// # Examples
+///
+/// The following is an example of using `ThreadedRegistration`:
+///
+/// ```
+/// use kernel::prelude::*;
+/// use kernel::irq::request::flags;
+/// use kernel::irq::request::ThreadedIrqReturn;
+/// use kernel::irq::request::ThreadedRegistration;
+/// use kernel::irq::request::IrqReturn;
+/// use kernel::sync::Arc;
+/// use kernel::sync::SpinLock;
+/// use kernel::alloc::flags::GFP_KERNEL;
+/// use kernel::c_str;
+///
+/// // 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(SpinLock<u32>);
+///
+/// // [`handle_irq`] takes &self. This example illustrates interior
+/// // mutability can be used when share the data between process context and IRQ
+/// // context.
+/// //
+/// // Ideally, this example would be using a version of SpinLock that is aware
+/// // of `spin_lock_irqsave` and `spin_lock_irqrestore`, but that is not yet
+/// // implemented.
+///
+/// type Handler = Data;
+///
+/// impl kernel::irq::request::ThreadedHandler for Handler {
+///     // This is executing in IRQ context in some CPU. Other CPUs can still
+///     // try to access to data.
+///     fn handle_irq(&self) -> ThreadedIrqReturn {
+///         // We now have exclusive access to the data by locking the SpinLock.
+///         let mut data = self.0.lock();
+///         *data += 1;
+///
+///         // By returning `WakeThread`, we indicate to the system that the
+///         // thread function should be called. Otherwise, return
+///         // ThreadedIrqReturn::Handled.
+///         ThreadedIrqReturn::WakeThread
+///     }
+///
+///     // This will run (in a separate kthread) iff `handle_irq` returns
+///     // `WakeThread`.
+///     fn thread_fn(&self) -> IrqReturn {
+///         // We now have exclusive access to the data by locking the SpinLock.
+///         let mut data = self.0.lock();
+///         *data += 1;
+///
+///         IrqReturn::Handled
+///     }
+/// }
+///
+/// // This is running in process context.
+/// fn register_threaded_irq(irq: u32, handler: Handler) -> Result<Arc<ThreadedRegistration<Handler>>> {
+///     let registration = ThreadedRegistration::register(irq, flags::SHARED, c_str!("my-device"), handler);
+///
+///     // You can have as many references to the registration as you want, so
+///     // multiple parts of the driver can access it.
+///     let registration = Arc::pin_init(registration, GFP_KERNEL)?;
+///
+///     // The handler may be called immediately after the function above
+///     // returns, possibly in a different CPU.
+///
+///     {
+///         // The data can be accessed from the process context too.
+///         let mut data = registration.handler().0.lock();
+///         *data = 42;
+///     }
+///
+///     Ok(registration)
+/// }
+///
+///
+/// # Ok::<(), Error>(())
+///```
+///
+/// # Invariants
+///
+/// * We own an irq handler using `&self` as its private data.
+///
+#[pin_data(PinnedDrop)]
+pub struct ThreadedRegistration<T: ThreadedHandler> {
+    irq: u32,
+    #[pin]
+    handler: T,
+    #[pin]
+    /// Pinned because we need address stability so that we can pass a pointer
+    /// to the callback.
+    _pin: PhantomPinned,
+}
+
+impl<T: ThreadedHandler> ThreadedRegistration<T> {
+    /// Registers the IRQ handler with the system for the given IRQ number. The
+    /// handler must be able to be called as soon as this function returns.
+    pub fn register(
+        irq: u32,
+        flags: Flags,
+        name: &'static CStr,
+        handler: T,
+    ) -> impl PinInit<Self, Error> {
+        let closure = move |slot: *mut Self| {
+            // SAFETY: The slot passed to pin initializer is valid for writing.
+            unsafe {
+                slot.write(Self {
+                    irq,
+                    handler,
+                    _pin: PhantomPinned,
+                })
+            };
+
+            // SAFETY:
+            // - The callbacks are valid for use with request_threaded_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.
+            let res = to_result(unsafe {
+                bindings::request_threaded_irq(
+                    irq,
+                    Some(handle_threaded_irq_callback::<T>),
+                    Some(thread_fn_callback::<T>),
+                    flags.0,
+                    name.as_char_ptr(),
+                    slot.cast(),
+                )
+            });
+
+            if res.is_err() {
+                // SAFETY: We are returning an error, so we can destroy the slot.
+                unsafe { core::ptr::drop_in_place(addr_of_mut!((*slot).handler)) };
+            }
+
+            res
+        };
+
+        // SAFETY:
+        // - if this returns Ok(()), then every field of `slot` is fully
+        // initialized.
+        // - if this returns an error, then the slot does not need to remain
+        // valid.
+        unsafe { pin_init_from_closure(closure) }
+    }
+
+    /// Returns a reference to the handler that was registered with the system.
+    pub fn handler(&self) -> &T {
+        // SAFETY: `handler` is initialized in `register`, and we require that
+        // T: Sync.
+        &self.handler
+    }
+}
+
+#[pinned_drop]
+impl<T: ThreadedHandler> PinnedDrop for ThreadedRegistration<T> {
+    fn drop(self: Pin<&mut Self>) {
+        // SAFETY:
+        // - `self.irq` is the same as the one passed to `request_threaded_irq`.
+        // -  `&self` was passed to `request_threaded_irq` as the cookie. It is
+        // guaranteed to be unique by the type system, since each call to
+        // `register` will return a different instance of
+        // `ThreadedRegistration`.
+        //
+        // Notice that this will block until all handlers finish executing, so,
+        // at no point will &self be invalid while the handler is running.
+        unsafe { bindings::free_irq(self.irq, &*self as *const Self as *mut core::ffi::c_void) };
+    }
+}
+
+/// # Safety
+///
+/// This function should be only used as the callback in `request_irq`.
+unsafe extern "C" fn handle_irq_callback<T: Handler>(
+    _irq: i32,
+    ptr: *mut core::ffi::c_void,
+) -> core::ffi::c_uint {
+    // SAFETY: `ptr` is a pointer to T set in `Registration::new`
+    let data = unsafe { &*(ptr as *const T) };
+    T::handle_irq(data) as _
+}
+
+/// # Safety
+///
+/// This function should be only used as the callback in `request_threaded_irq`.
+unsafe extern "C" fn handle_threaded_irq_callback<T: ThreadedHandler>(
+    _irq: i32,
+    ptr: *mut core::ffi::c_void,
+) -> core::ffi::c_uint {
+    // SAFETY: `ptr` is a pointer to T set in `ThreadedRegistration::new`
+    let data = unsafe { &*(ptr as *const T) };
+    T::handle_irq(data) as _
+}
+
+/// # Safety
+///
+/// This function should be only used as the callback in `request_threaded_irq`.
+unsafe extern "C" fn thread_fn_callback<T: ThreadedHandler>(
+    _irq: i32,
+    ptr: *mut core::ffi::c_void,
+) -> core::ffi::c_uint {
+    // SAFETY: `ptr` is a pointer to T set in `ThreadedRegistration::new`
+    let data = unsafe { &*(ptr as *const T) };
+    T::thread_fn(data) as _
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index e1065a7551a3..bba782e138c5 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -41,6 +41,7 @@
 pub mod fs;
 pub mod init;
 pub mod ioctl;
+pub mod irq;
 pub mod jump_label;
 #[cfg(CONFIG_KUNIT)]
 pub mod kunit;
-- 
2.48.0

Re: [PATCH v2] rust: irq: add support for request_irq()

[Boqun Feng]

On Wed, Jan 22, 2025 at 01:39:30PM -0300, Daniel Almeida wrote:
> Add support for registering IRQ handlers in Rust.
> 
> IRQ handlers are extensively used in drivers when some peripheral wants to
> obtain the CPU attention. Registering a handler will make the system invoke the
> passed-in function whenever the chosen IRQ line is triggered.
> 
> Both regular and threaded IRQ handlers are supported through a Handler (or
> ThreadedHandler) trait that is meant to be implemented by a type that:
> 
> a) provides a function to be run by the system when the IRQ fires and,
> 
> b) holds the shared data (i.e.: `T`) between process and IRQ contexts.
> 
> The requirement that T is Sync derives from the fact that handlers might run
> concurrently with other processes executing the same driver, creating the
> potential for data races.
> 
> Ideally, some interior mutability must be in place if T is to be mutated. This
> should usually be done through the in-flight SpinLockIrq type.
> 
> Co-developed-by: Alice Ryhl <aliceryhl@google.com>
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
> ---
> 
> Changes from v1:
> 
> - Added Co-developed-by tag to account for the work that Alice did in order to
> figure out how to do this without Opaque<T> (Thanks!)
> - Removed Opaque<T> in favor of plain T

Hmmm...

[...]

> +#[pin_data(PinnedDrop)]
> +pub struct Registration<T: Handler> {
> +    irq: u32,
> +    #[pin]
> +    handler: T,

I think you still need to make `handler` as `!Unpin` because compilers
can assume a `&mut T` from a `Pin<&mut Registration>`, am I missing
something here?

Regards,
Boqun

> +    #[pin]
> +    /// Pinned because we need address stability so that we can pass a pointer
> +    /// to the callback.
> +    _pin: PhantomPinned,
> +}
> +

[...]

Re: [PATCH v2] rust: irq: add support for request_irq()

[Alice Ryhl]

On Thu, Jan 23, 2025 at 8:18 AM Boqun Feng <boqun.feng@gmail.com> wrote:
>
> On Wed, Jan 22, 2025 at 01:39:30PM -0300, Daniel Almeida wrote:
> > Add support for registering IRQ handlers in Rust.
> >
> > IRQ handlers are extensively used in drivers when some peripheral wants to
> > obtain the CPU attention. Registering a handler will make the system invoke the
> > passed-in function whenever the chosen IRQ line is triggered.
> >
> > Both regular and threaded IRQ handlers are supported through a Handler (or
> > ThreadedHandler) trait that is meant to be implemented by a type that:
> >
> > a) provides a function to be run by the system when the IRQ fires and,
> >
> > b) holds the shared data (i.e.: `T`) between process and IRQ contexts.
> >
> > The requirement that T is Sync derives from the fact that handlers might run
> > concurrently with other processes executing the same driver, creating the
> > potential for data races.
> >
> > Ideally, some interior mutability must be in place if T is to be mutated. This
> > should usually be done through the in-flight SpinLockIrq type.
> >
> > Co-developed-by: Alice Ryhl <aliceryhl@google.com>
> > Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> > Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
> > ---
> >
> > Changes from v1:
> >
> > - Added Co-developed-by tag to account for the work that Alice did in order to
> > figure out how to do this without Opaque<T> (Thanks!)
> > - Removed Opaque<T> in favor of plain T
>
> Hmmm...
>
> [...]
>
> > +#[pin_data(PinnedDrop)]
> > +pub struct Registration<T: Handler> {
> > +    irq: u32,
> > +    #[pin]
> > +    handler: T,
>
> I think you still need to make `handler` as `!Unpin` because compilers
> can assume a `&mut T` from a `Pin<&mut Registration>`, am I missing
> something here?

The current version operates under the assumption that PhantomPinned
is enough. But I'm happy to add Aliased here.

Alice

Re: [PATCH v2] rust: irq: add support for request_irq()

[Christian Schrefl]

On 22.01.25 5:39 PM, Daniel Almeida wrote:
> Add support for registering IRQ handlers in Rust.
> 
> IRQ handlers are extensively used in drivers when some peripheral wants to
> obtain the CPU attention. Registering a handler will make the system invoke the
> passed-in function whenever the chosen IRQ line is triggered.
> 
> Both regular and threaded IRQ handlers are supported through a Handler (or
> ThreadedHandler) trait that is meant to be implemented by a type that:
> 
> a) provides a function to be run by the system when the IRQ fires and,
> 
> b) holds the shared data (i.e.: `T`) between process and IRQ contexts.
> 
> The requirement that T is Sync derives from the fact that handlers might run
> concurrently with other processes executing the same driver, creating the
> potential for data races.
> 
> Ideally, some interior mutability must be in place if T is to be mutated. This
> should usually be done through the in-flight SpinLockIrq type.
> 
> Co-developed-by: Alice Ryhl <aliceryhl@google.com>
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>

It would be nice it you included a second patch that adds the ability to get
the irq number from a platform device, it should be pretty simple.
Of course this can also just be done separately.

Otherwise this looks pretty good, a few small comments below.

> ---
> 
> Changes from v1:
> 
> - Added Co-developed-by tag to account for the work that Alice did in order to
> figure out how to do this without Opaque<T> (Thanks!)
> - Removed Opaque<T> in favor of plain T
> - Fixed the examples
> - Made sure that the invariants sections are the last entry in the docs
> - Switched to slot.cast() where applicable,
> - Mentioned in the safety comments that we require that T: Sync,
> - Removed ThreadedFnReturn in favor of IrqReturn,
> - Improved the commit message
> 
> Link to v1: https://lore.kernel.org/rust-for-linux/20241024-topic-panthor-rs-request_irq-v1-1-7cbc51c182ca@collabora.com/
> 
> 
> ---
>  rust/bindings/bindings_helper.h |   1 +
>  rust/helpers/helpers.c          |   1 +
>  rust/helpers/irq.c              |   9 +
>  rust/kernel/irq.rs              |   6 +
>  rust/kernel/irq/request.rs      | 517 ++++++++++++++++++++++++++++++++
>  rust/kernel/lib.rs              |   1 +
>  6 files changed, 535 insertions(+)
>  create mode 100644 rust/helpers/irq.c
>  create mode 100644 rust/kernel/irq.rs
>  create mode 100644 rust/kernel/irq/request.rs
> 
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index 5c4dfe22f41a..0331c6273def 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -15,6 +15,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>
> diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
> index dcf827a61b52..bfc499d7f4c1 100644
> --- a/rust/helpers/helpers.c
> +++ b/rust/helpers/helpers.c
> @@ -13,6 +13,7 @@
>  #include "build_bug.c"
>  #include "cred.c"
>  #include "err.c"
> +#include "irq.c"
>  #include "fs.c"
>  #include "jump_label.c"
>  #include "kunit.c"
> diff --git a/rust/helpers/irq.c b/rust/helpers/irq.c
> new file mode 100644
> index 000000000000..1faca428e2c0
> --- /dev/null
> +++ b/rust/helpers/irq.c
> @@ -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);
> +}
> diff --git a/rust/kernel/irq.rs b/rust/kernel/irq.rs
> new file mode 100644
> index 000000000000..3ab83c5bdb83
> --- /dev/null
> +++ b/rust/kernel/irq.rs
> @@ -0,0 +1,6 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! IRQ abstractions
> +
> +/// IRQ allocation and handling
> +pub mod request;
> diff --git a/rust/kernel/irq/request.rs b/rust/kernel/irq/request.rs
> new file mode 100644
> index 000000000000..61e7d4a8f555
> --- /dev/null
> +++ b/rust/kernel/irq/request.rs
> @@ -0,0 +1,517 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// SPDX-FileCopyrightText: Copyright 2025 Collabora ltd.
> +
> +//! IRQ allocation and handling
> +
> +use core::marker::PhantomPinned;
> +use core::ptr::addr_of_mut;
> +
> +use init::pin_init_from_closure;
> +
> +use crate::error::to_result;
> +use crate::prelude::*;
> +use crate::str::CStr;
> +
> +/// Flags to be used when registering IRQ handlers.
> +///
> +/// They can be combined with the operators `|`, `&`, and `!`.
> +///
> +/// Values can be used from the [`flags`] module.
> +#[derive(Clone, Copy)]
> +pub struct Flags(u64);
> +
> +impl core::ops::BitOr for Flags {
> +    type Output = Self;
> +    fn bitor(self, rhs: Self) -> Self::Output {
> +        Self(self.0 | rhs.0)
> +    }
> +}
> +
> +impl core::ops::BitAnd for Flags {
> +    type Output = Self;
> +    fn bitand(self, rhs: Self) -> Self::Output {
> +        Self(self.0 & rhs.0)
> +    }
> +}
> +
> +impl core::ops::Not for Flags {
> +    type Output = Self;
> +    fn not(self) -> Self::Output {
> +        Self(!self.0)
> +    }
> +}
> +
> +/// The flags that can be used when registering an IRQ handler.
> +pub mod flags {

Maybe move flags into a separate file?
You even have a directory for irq.

> +    use super::Flags;
> +
> +    use crate::bindings;
> +
> +    /// Use the interrupt line as already configured.
> +    pub const TRIGGER_NONE: Flags = Flags(bindings::IRQF_TRIGGER_NONE as _);
> +
> +    /// The interrupt is triggered when the signal goes from low to high.
> +    pub const TRIGGER_RISING: Flags = Flags(bindings::IRQF_TRIGGER_RISING as _);
> +
> +    /// The interrupt is triggered when the signal goes from high to low.
> +    pub const TRIGGER_FALLING: Flags = Flags(bindings::IRQF_TRIGGER_FALLING as _);
> +
> +    /// The interrupt is triggered while the signal is held high.
> +    pub const TRIGGER_HIGH: Flags = Flags(bindings::IRQF_TRIGGER_HIGH as _);
> +
> +    /// The interrupt is triggered while the signal is held low.
> +    pub const TRIGGER_LOW: Flags = Flags(bindings::IRQF_TRIGGER_LOW as _);
> +
> +    /// Allow sharing the irq among several devices.
> +    pub const SHARED: Flags = Flags(bindings::IRQF_SHARED as _);
> +
> +    /// Set by callers when they expect sharing mismatches to occur.
> +    pub const PROBE_SHARED: Flags = Flags(bindings::IRQF_PROBE_SHARED as _);
> +
> +    /// Flag to mark this interrupt as timer interrupt.
> +    pub const TIMER: Flags = Flags(bindings::IRQF_TIMER as _);
> +
> +    /// Interrupt is per cpu.
> +    pub const PERCPU: Flags = Flags(bindings::IRQF_PERCPU as _);
> +
> +    /// Flag to exclude this interrupt from irq balancing.
> +    pub const NOBALANCING: Flags = Flags(bindings::IRQF_NOBALANCING as _);
> +
> +    /// Interrupt is used for polling (only the interrupt that is registered
> +    /// first in a shared interrupt is considered for performance reasons).
> +    pub const IRQPOLL: Flags = Flags(bindings::IRQF_IRQPOLL as _);
> +
> +    /// Interrupt is not reenabled after the hardirq handler finished. Used by
> +    /// threaded interrupts which need to keep the irq line disabled until the
> +    /// threaded handler has been run.
> +    pub const ONESHOT: Flags = Flags(bindings::IRQF_ONESHOT as _);
> +
> +    /// Do not disable this IRQ during suspend. Does not guarantee that this
> +    /// interrupt will wake the system from a suspended state.
> +    pub const NO_SUSPEND: Flags = Flags(bindings::IRQF_NO_SUSPEND as _);
> +
> +    /// Force enable it on resume even if [`NO_SUSPEND`] is set.
> +    pub const FORCE_RESUME: Flags = Flags(bindings::IRQF_FORCE_RESUME as _);
> +
> +    /// Interrupt cannot be threaded.
> +    pub const NO_THREAD: Flags = Flags(bindings::IRQF_NO_THREAD as _);
> +
> +    /// Resume IRQ early during syscore instead of at device resume time.
> +    pub const EARLY_RESUME: Flags = Flags(bindings::IRQF_EARLY_RESUME as _);
> +
> +    /// If the IRQ is shared with a NO_SUSPEND user, execute this interrupt
> +    /// handler after suspending interrupts. For system wakeup devices users
> +    /// need to implement wakeup detection in their interrupt handlers.
> +    pub const COND_SUSPEND: Flags = Flags(bindings::IRQF_COND_SUSPEND as _);
> +
> +    /// Don't enable IRQ or NMI automatically when users request it. Users will
> +    /// enable it explicitly by `enable_irq` or `enable_nmi` later.
> +    pub const NO_AUTOEN: Flags = Flags(bindings::IRQF_NO_AUTOEN as _);
> +
> +    /// Exclude from runnaway detection for IPI and similar handlers, depends on
> +    /// `PERCPU`.
> +    pub const NO_DEBUG: Flags = Flags(bindings::IRQF_NO_DEBUG as _);
> +}
> +
> +/// 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 as _,
> +
> +    /// The interrupt was handled by this device.
> +    Handled = bindings::irqreturn_IRQ_HANDLED as _,
> +}
> +
> +/// Callbacks for an IRQ handler.
> +pub trait Handler: Sync {
> +    /// The actual handler function. As usual, sleeps are not allowed in IRQ
> +    /// context.
> +    fn handle_irq(&self) -> IrqReturn;
> +}
> +
> +/// A registration of an IRQ handler for a given IRQ line.
> +///
> +/// # Examples
> +///
> +/// The following is an example of using `Registration`:
> +///
> +/// ```
> +/// use kernel::prelude::*;
> +/// use kernel::irq::request::flags;
> +/// use kernel::irq::request::Registration;
> +/// use kernel::irq::request::IrqReturn;
> +/// use kernel::sync::Arc;
> +/// use kernel::sync::SpinLock;
> +/// 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(SpinLock<u32>);
> +///
> +/// // [`handle_irq`] takes &self. This example illustrates interior
> +/// // mutability can be used when share the data between process context and IRQ
> +/// // context.
> +/// //
> +/// // Ideally, this example would be using a version of SpinLock that is aware
> +/// // of `spin_lock_irqsave` and `spin_lock_irqrestore`, but that is not yet
> +/// // implemented.
> +///
> +/// 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_irq(&self) -> IrqReturn {
> +///         // We now have exclusive access to the data by locking the SpinLock.
> +///         let mut data = self.0.lock();
> +///         *data += 1;
> +///
> +///         IrqReturn::Handled
> +///     }
> +/// }
> +///
> +/// // This is running in process context.
> +/// fn register_irq(irq: u32, handler: Handler) -> Result<Arc<Registration<Handler>>> {
> +///     let registration = Registration::register(irq, flags::SHARED, c_str!("my-device"), handler);
> +///
> +///     // You can have as many references to the registration as you want, so
> +///     // multiple parts of the driver can access it.
> +///     let registration = Arc::pin_init(registration, GFP_KERNEL)?;
> +///
> +///     // The handler may be called immediately after the function above
> +///     // returns, possibly in a different CPU.
> +///
> +///     {
> +///         // The data can be accessed from the process context too.
> +///         let mut data = registration.handler().0.lock();
> +///         *data = 42;
> +///     }
> +///
> +///     Ok(registration)
> +/// }
> +///
> +/// # Ok::<(), Error>(())
> +///```
> +///
> +/// # Invariants
> +///
> +/// * We own an irq handler using `&self` as its private data.
> +///
> +#[pin_data(PinnedDrop)]
> +pub struct Registration<T: Handler> {
> +    irq: u32,
> +    #[pin]
> +    handler: T,
> +    #[pin]
> +    /// Pinned because we need address stability so that we can pass a pointer
> +    /// to the callback.
> +    _pin: PhantomPinned,
> +}
> +
> +impl<T: Handler> Registration<T> {
> +    /// Registers the IRQ handler with the system for the given IRQ number. The
> +    /// handler must be able to be called as soon as this function returns.
> +    pub fn register(
> +        irq: u32,
> +        flags: Flags,
> +        name: &'static CStr,
> +        handler: T,
> +    ) -> impl PinInit<Self, Error> {
> +        let closure = move |slot: *mut Self| {
> +            // SAFETY: The slot passed to pin initializer is valid for writing.
> +            unsafe {
> +                slot.write(Self {
> +                    irq,
> +                    handler,
> +                    _pin: PhantomPinned,
> +                })
> +            };
> +
> +            // 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.
> +            let res = to_result(unsafe {
> +                bindings::request_irq(
> +                    irq,
> +                    Some(handle_irq_callback::<T>),
> +                    flags.0,
> +                    name.as_char_ptr(),
> +                    &*slot as *const _ as *mut core::ffi::c_void,
> +                )
> +            });
> +
> +            if res.is_err() {
> +                // SAFETY: We are returning an error, so we can destroy the slot.
> +                unsafe { core::ptr::drop_in_place(addr_of_mut!((*slot).handler)) };
> +            }
> +
> +            res
> +        };
> +
> +        // SAFETY:
> +        // - if this returns Ok, then every field of `slot` is fully
> +        // initialized.
> +        // - if this returns an error, then the slot does not need to remain
> +        // valid.
> +        unsafe { pin_init_from_closure(closure) }
> +    }
> +
> +    /// Returns a reference to the handler that was registered with the system.
> +    pub fn handler(&self) -> &T {
> +        // SAFETY: `handler` is initialized in `register`, and we require that
> +        // T: Sync.
> +        &self.handler
> +    }
> +}
> +
> +#[pinned_drop]
> +impl<T: Handler> PinnedDrop for Registration<T> {
> +    fn drop(self: Pin<&mut Self>) {
> +        // SAFETY:
> +        // - `self.irq` is the same as the one passed to `reques_irq`.
> +        // -  `&self` was passed to `request_irq` as the cookie. It is
> +        // guaranteed to be unique by the type system, since each call to
> +        // `register` will return a different instance of `Registration`.
> +        //
> +        // 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 as *const Self as *mut core::ffi::c_void) };
> +    }
> +}
> +
> +/// The value that can be returned from `ThreadedHandler::handle_irq`.
> +pub enum ThreadedIrqReturn {
> +    /// The interrupt was not from this device or was not handled.
> +    None = bindings::irqreturn_IRQ_NONE as _,
> +
> +    /// The interrupt was handled by this device.
> +    Handled = bindings::irqreturn_IRQ_HANDLED as _,
> +
> +    /// The handler wants the handler thread to wake up.
> +    WakeThread = bindings::irqreturn_IRQ_WAKE_THREAD as _,
> +}
> +
> +/// Callbacks for a threaded IRQ handler.
> +pub trait ThreadedHandler: Sync {
> +    /// The actual handler function. As usual, sleeps are not allowed in IRQ
> +    /// context.
> +    fn handle_irq(&self) -> ThreadedIrqReturn;
> +
> +    /// The threaded handler function. This function is called from the irq
> +    /// handler thread, which is automatically created by the system.
> +    fn thread_fn(&self) -> IrqReturn;
> +}
> +
> +/// A registration of a threaded IRQ handler for a given IRQ line.
> +///
> +/// Two callbacks are required: one to handle the IRQ, and one to handle any
> +/// other work in a separate thread.
> +///
> +/// The thread handler is only called if the IRQ handler returns `WakeThread`.
> +///
> +/// # Examples
> +///
> +/// The following is an example of using `ThreadedRegistration`:
> +///
> +/// ```
> +/// use kernel::prelude::*;
> +/// use kernel::irq::request::flags;
> +/// use kernel::irq::request::ThreadedIrqReturn;
> +/// use kernel::irq::request::ThreadedRegistration;
> +/// use kernel::irq::request::IrqReturn;
> +/// use kernel::sync::Arc;
> +/// use kernel::sync::SpinLock;
> +/// use kernel::alloc::flags::GFP_KERNEL;
> +/// use kernel::c_str;
> +///
> +/// // 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(SpinLock<u32>);
> +///
> +/// // [`handle_irq`] takes &self. This example illustrates interior
> +/// // mutability can be used when share the data between process context and IRQ
> +/// // context.
> +/// //
> +/// // Ideally, this example would be using a version of SpinLock that is aware
> +/// // of `spin_lock_irqsave` and `spin_lock_irqrestore`, but that is not yet
> +/// // implemented.
> +///
> +/// type Handler = Data;
> +///
> +/// impl kernel::irq::request::ThreadedHandler for Handler {
> +///     // This is executing in IRQ context in some CPU. Other CPUs can still
> +///     // try to access to data.
> +///     fn handle_irq(&self) -> ThreadedIrqReturn {
> +///         // We now have exclusive access to the data by locking the SpinLock.
> +///         let mut data = self.0.lock();
> +///         *data += 1;
> +///
> +///         // By returning `WakeThread`, we indicate to the system that the
> +///         // thread function should be called. Otherwise, return
> +///         // ThreadedIrqReturn::Handled.
> +///         ThreadedIrqReturn::WakeThread
> +///     }
> +///
> +///     // This will run (in a separate kthread) iff `handle_irq` returns
typo iff
> +///     // `WakeThread`.
> +///     fn thread_fn(&self) -> IrqReturn {
> +///         // We now have exclusive access to the data by locking the SpinLock.
> +///         let mut data = self.0.lock();
> +///         *data += 1;
> +///
> +///         IrqReturn::Handled
> +///     }
> +/// }
> +///
> +/// // This is running in process context.
> +/// fn register_threaded_irq(irq: u32, handler: Handler) -> Result<Arc<ThreadedRegistration<Handler>>> {
> +///     let registration = ThreadedRegistration::register(irq, flags::SHARED, c_str!("my-device"), handler);
> +///
> +///     // You can have as many references to the registration as you want, so
> +///     // multiple parts of the driver can access it.
> +///     let registration = Arc::pin_init(registration, GFP_KERNEL)?;
You can use Arc::new instead, since you don't need pin_init.
> +///
> +///     // The handler may be called immediately after the function above
> +///     // returns, possibly in a different CPU.
> +///
> +///     {
> +///         // The data can be accessed from the process context too.
> +///         let mut data = registration.handler().0.lock();
> +///         *data = 42;
> +///     }
> +///
> +///     Ok(registration)
> +/// }
> +///
> +///
> +/// # Ok::<(), Error>(())
> +///```
> +///
> +/// # Invariants
> +///
> +/// * We own an irq handler using `&self` as its private data.
> +///
> +#[pin_data(PinnedDrop)]
> +pub struct ThreadedRegistration<T: ThreadedHandler> {
> +    irq: u32,
> +    #[pin]
> +    handler: T,
> +    #[pin]
> +    /// Pinned because we need address stability so that we can pass a pointer
> +    /// to the callback.
> +    _pin: PhantomPinned,
> +}
> +
> +impl<T: ThreadedHandler> ThreadedRegistration<T> {
> +    /// Registers the IRQ handler with the system for the given IRQ number. The
> +    /// handler must be able to be called as soon as this function returns.
> +    pub fn register(
> +        irq: u32,
> +        flags: Flags,
> +        name: &'static CStr,
> +        handler: T,
> +    ) -> impl PinInit<Self, Error> {
> +        let closure = move |slot: *mut Self| {
> +            // SAFETY: The slot passed to pin initializer is valid for writing.
> +            unsafe {
> +                slot.write(Self {
> +                    irq,
> +                    handler,
> +                    _pin: PhantomPinned,
> +                })
> +            };
> +
> +            // SAFETY:
> +            // - The callbacks are valid for use with request_threaded_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.
> +            let res = to_result(unsafe {
> +                bindings::request_threaded_irq(
> +                    irq,
> +                    Some(handle_threaded_irq_callback::<T>),
> +                    Some(thread_fn_callback::<T>),
> +                    flags.0,
> +                    name.as_char_ptr(),
> +                    slot.cast(),
> +                )
> +            });
> +
> +            if res.is_err() {
> +                // SAFETY: We are returning an error, so we can destroy the slot.
> +                unsafe { core::ptr::drop_in_place(addr_of_mut!((*slot).handler)) };
> +            }
> +
> +            res
> +        };
> +
> +        // SAFETY:
> +        // - if this returns Ok(()), then every field of `slot` is fully
> +        // initialized.
> +        // - if this returns an error, then the slot does not need to remain
> +        // valid.
> +        unsafe { pin_init_from_closure(closure) }
> +    }
> +
> +    /// Returns a reference to the handler that was registered with the system.
> +    pub fn handler(&self) -> &T {
> +        // SAFETY: `handler` is initialized in `register`, and we require that
> +        // T: Sync.
> +        &self.handler
> +    }
> +}
> +
> +#[pinned_drop]
> +impl<T: ThreadedHandler> PinnedDrop for ThreadedRegistration<T> {
> +    fn drop(self: Pin<&mut Self>) {
> +        // SAFETY:
> +        // - `self.irq` is the same as the one passed to `request_threaded_irq`.
> +        // -  `&self` was passed to `request_threaded_irq` as the cookie. It is
> +        // guaranteed to be unique by the type system, since each call to
> +        // `register` will return a different instance of
> +        // `ThreadedRegistration`.
> +        //
> +        // Notice that this will block until all handlers finish executing, so,
> +        // at no point will &self be invalid while the handler is running.
> +        unsafe { bindings::free_irq(self.irq, &*self as *const Self as *mut core::ffi::c_void) };
> +    }
> +}
> +
> +/// # Safety
> +///
> +/// This function should be only used as the callback in `request_irq`.
> +unsafe extern "C" fn handle_irq_callback<T: Handler>(
> +    _irq: i32,
> +    ptr: *mut core::ffi::c_void,
> +) -> core::ffi::c_uint {
> +    // SAFETY: `ptr` is a poinhandle_threaded_irq_callbackter to T set in `Registration::new`
> +    let data = unsafe { &*(ptr as *const T) };
> +    T::handle_irq(data) as _
> +

Can `handle_irq_callback` and `handle_threaded_irq_callback` be shared somehow?

Otherwise maybe define them locally in the calling function, since nothing else
should ever use them, so we might as well restrict their scope.

> +
> +/// # Safety
> +///
> +/// This function should be only used as the callback in `request_threaded_irq`.
> +unsafe extern "C" fn handle_threaded_irq_callback<T: ThreadedHandler>(
> +    _irq: i32,
> +    ptr: *mut core::ffi::c_void,
> +) -> core::ffi::c_uint {
> +    // SAFETY: `ptr` is a pointer to T set in `ThreadedRegistration::new`
> +    let data = unsafe { &*(ptr as *const T) };
> +    T::handle_irq(data) as _
> +}
> +
> +/// # Safety
> +///
> +/// This function should be only used as the callback in `request_threaded_irq`.
> +unsafe extern "C" fn thread_fn_callback<T: ThreadedHandler>(
> +    _irq: i32,
> +    ptr: *mut core::ffi::c_void,
> +) -> core::ffi::c_uint {
> +    // SAFETY: `ptr` is a pointer to T set in `ThreadedRegistration::new`
> +    let data = unsafe { &*(ptr as *const T) };
> +    T::thread_fn(data) as _
> +}
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index e1065a7551a3..bba782e138c5 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -41,6 +41,7 @@
>  pub mod fs;
>  pub mod init;
>  pub mod ioctl;
> +pub mod irq;
>  pub mod jump_label;
>  #[cfg(CONFIG_KUNIT)]
>  pub mod kunit;

Re: [PATCH v2] rust: irq: add support for request_irq()

[Daniel Almeida]

Hi Christian,

> On 23 Jan 2025, at 13:00, Christian Schrefl <chrisi.schrefl@gmail.com> wrote:
> 
> On 22.01.25 5:39 PM, Daniel Almeida wrote:
>> Add support for registering IRQ handlers in Rust.
>> 
>> IRQ handlers are extensively used in drivers when some peripheral wants to
>> obtain the CPU attention. Registering a handler will make the system invoke the
>> passed-in function whenever the chosen IRQ line is triggered.
>> 
>> Both regular and threaded IRQ handlers are supported through a Handler (or
>> ThreadedHandler) trait that is meant to be implemented by a type that:
>> 
>> a) provides a function to be run by the system when the IRQ fires and,
>> 
>> b) holds the shared data (i.e.: `T`) between process and IRQ contexts.
>> 
>> The requirement that T is Sync derives from the fact that handlers might run
>> concurrently with other processes executing the same driver, creating the
>> potential for data races.
>> 
>> Ideally, some interior mutability must be in place if T is to be mutated. This
>> should usually be done through the in-flight SpinLockIrq type.
>> 
>> Co-developed-by: Alice Ryhl <aliceryhl@google.com>
>> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
>> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
> 
> It would be nice it you included a second patch that adds the ability to get
> the irq number from a platform device, it should be pretty simple.
> Of course this can also just be done separately.

Sure!

> 
> Otherwise this looks pretty good, a few small comments below.
> 
>> ---
>> 
>> Changes from v1:
>> 
>> - Added Co-developed-by tag to account for the work that Alice did in order to
>> figure out how to do this without Opaque<T> (Thanks!)
>> - Removed Opaque<T> in favor of plain T
>> - Fixed the examples
>> - Made sure that the invariants sections are the last entry in the docs
>> - Switched to slot.cast() where applicable,
>> - Mentioned in the safety comments that we require that T: Sync,
>> - Removed ThreadedFnReturn in favor of IrqReturn,
>> - Improved the commit message
>> 
>> Link to v1: https://lore.kernel.org/rust-for-linux/20241024-topic-panthor-rs-request_irq-v1-1-7cbc51c182ca@collabora.com/
>> 
>> 
>> ---
>> rust/bindings/bindings_helper.h |   1 +
>> rust/helpers/helpers.c          |   1 +
>> rust/helpers/irq.c              |   9 +
>> rust/kernel/irq.rs              |   6 +
>> rust/kernel/irq/request.rs      | 517 ++++++++++++++++++++++++++++++++
>> rust/kernel/lib.rs              |   1 +
>> 6 files changed, 535 insertions(+)
>> create mode 100644 rust/helpers/irq.c
>> create mode 100644 rust/kernel/irq.rs
>> create mode 100644 rust/kernel/irq/request.rs
>> 
>> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
>> index 5c4dfe22f41a..0331c6273def 100644
>> --- a/rust/bindings/bindings_helper.h
>> +++ b/rust/bindings/bindings_helper.h
>> @@ -15,6 +15,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>
>> diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
>> index dcf827a61b52..bfc499d7f4c1 100644
>> --- a/rust/helpers/helpers.c
>> +++ b/rust/helpers/helpers.c
>> @@ -13,6 +13,7 @@
>> #include "build_bug.c"
>> #include "cred.c"
>> #include "err.c"
>> +#include "irq.c"
>> #include "fs.c"
>> #include "jump_label.c"
>> #include "kunit.c"
>> diff --git a/rust/helpers/irq.c b/rust/helpers/irq.c
>> new file mode 100644
>> index 000000000000..1faca428e2c0
>> --- /dev/null
>> +++ b/rust/helpers/irq.c
>> @@ -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);
>> +}
>> diff --git a/rust/kernel/irq.rs b/rust/kernel/irq.rs
>> new file mode 100644
>> index 000000000000..3ab83c5bdb83
>> --- /dev/null
>> +++ b/rust/kernel/irq.rs
>> @@ -0,0 +1,6 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +//! IRQ abstractions
>> +
>> +/// IRQ allocation and handling
>> +pub mod request;
>> diff --git a/rust/kernel/irq/request.rs b/rust/kernel/irq/request.rs
>> new file mode 100644
>> index 000000000000..61e7d4a8f555
>> --- /dev/null
>> +++ b/rust/kernel/irq/request.rs
>> @@ -0,0 +1,517 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// SPDX-FileCopyrightText: Copyright 2025 Collabora ltd.
>> +
>> +//! IRQ allocation and handling
>> +
>> +use core::marker::PhantomPinned;
>> +use core::ptr::addr_of_mut;
>> +
>> +use init::pin_init_from_closure;
>> +
>> +use crate::error::to_result;
>> +use crate::prelude::*;
>> +use crate::str::CStr;
>> +
>> +/// Flags to be used when registering IRQ handlers.
>> +///
>> +/// They can be combined with the operators `|`, `&`, and `!`.
>> +///
>> +/// Values can be used from the [`flags`] module.
>> +#[derive(Clone, Copy)]
>> +pub struct Flags(u64);
>> +
>> +impl core::ops::BitOr for Flags {
>> +    type Output = Self;
>> +    fn bitor(self, rhs: Self) -> Self::Output {
>> +        Self(self.0 | rhs.0)
>> +    }
>> +}
>> +
>> +impl core::ops::BitAnd for Flags {
>> +    type Output = Self;
>> +    fn bitand(self, rhs: Self) -> Self::Output {
>> +        Self(self.0 & rhs.0)
>> +    }
>> +}
>> +
>> +impl core::ops::Not for Flags {
>> +    type Output = Self;
>> +    fn not(self) -> Self::Output {
>> +        Self(!self.0)
>> +    }
>> +}
>> +
>> +/// The flags that can be used when registering an IRQ handler.
>> +pub mod flags {
> 
> Maybe move flags into a separate file?
> You even have a directory for irq.

Why? Most flags are defined in the same file. See alloc::flags, for example.

I am not against this, but since this is merely aesthetic in nature, we should maybe
wait for input from more people. 

> 
>> +    use super::Flags;
>> +
>> +    use crate::bindings;
>> +
>> +    /// Use the interrupt line as already configured.
>> +    pub const TRIGGER_NONE: Flags = Flags(bindings::IRQF_TRIGGER_NONE as _);
>> +
>> +    /// The interrupt is triggered when the signal goes from low to high.
>> +    pub const TRIGGER_RISING: Flags = Flags(bindings::IRQF_TRIGGER_RISING as _);
>> +
>> +    /// The interrupt is triggered when the signal goes from high to low.
>> +    pub const TRIGGER_FALLING: Flags = Flags(bindings::IRQF_TRIGGER_FALLING as _);
>> +
>> +    /// The interrupt is triggered while the signal is held high.
>> +    pub const TRIGGER_HIGH: Flags = Flags(bindings::IRQF_TRIGGER_HIGH as _);
>> +
>> +    /// The interrupt is triggered while the signal is held low.
>> +    pub const TRIGGER_LOW: Flags = Flags(bindings::IRQF_TRIGGER_LOW as _);
>> +
>> +    /// Allow sharing the irq among several devices.
>> +    pub const SHARED: Flags = Flags(bindings::IRQF_SHARED as _);
>> +
>> +    /// Set by callers when they expect sharing mismatches to occur.
>> +    pub const PROBE_SHARED: Flags = Flags(bindings::IRQF_PROBE_SHARED as _);
>> +
>> +    /// Flag to mark this interrupt as timer interrupt.
>> +    pub const TIMER: Flags = Flags(bindings::IRQF_TIMER as _);
>> +
>> +    /// Interrupt is per cpu.
>> +    pub const PERCPU: Flags = Flags(bindings::IRQF_PERCPU as _);
>> +
>> +    /// Flag to exclude this interrupt from irq balancing.
>> +    pub const NOBALANCING: Flags = Flags(bindings::IRQF_NOBALANCING as _);
>> +
>> +    /// Interrupt is used for polling (only the interrupt that is registered
>> +    /// first in a shared interrupt is considered for performance reasons).
>> +    pub const IRQPOLL: Flags = Flags(bindings::IRQF_IRQPOLL as _);
>> +
>> +    /// Interrupt is not reenabled after the hardirq handler finished. Used by
>> +    /// threaded interrupts which need to keep the irq line disabled until the
>> +    /// threaded handler has been run.
>> +    pub const ONESHOT: Flags = Flags(bindings::IRQF_ONESHOT as _);
>> +
>> +    /// Do not disable this IRQ during suspend. Does not guarantee that this
>> +    /// interrupt will wake the system from a suspended state.
>> +    pub const NO_SUSPEND: Flags = Flags(bindings::IRQF_NO_SUSPEND as _);
>> +
>> +    /// Force enable it on resume even if [`NO_SUSPEND`] is set.
>> +    pub const FORCE_RESUME: Flags = Flags(bindings::IRQF_FORCE_RESUME as _);
>> +
>> +    /// Interrupt cannot be threaded.
>> +    pub const NO_THREAD: Flags = Flags(bindings::IRQF_NO_THREAD as _);
>> +
>> +    /// Resume IRQ early during syscore instead of at device resume time.
>> +    pub const EARLY_RESUME: Flags = Flags(bindings::IRQF_EARLY_RESUME as _);
>> +
>> +    /// If the IRQ is shared with a NO_SUSPEND user, execute this interrupt
>> +    /// handler after suspending interrupts. For system wakeup devices users
>> +    /// need to implement wakeup detection in their interrupt handlers.
>> +    pub const COND_SUSPEND: Flags = Flags(bindings::IRQF_COND_SUSPEND as _);
>> +
>> +    /// Don't enable IRQ or NMI automatically when users request it. Users will
>> +    /// enable it explicitly by `enable_irq` or `enable_nmi` later.
>> +    pub const NO_AUTOEN: Flags = Flags(bindings::IRQF_NO_AUTOEN as _);
>> +
>> +    /// Exclude from runnaway detection for IPI and similar handlers, depends on
>> +    /// `PERCPU`.
>> +    pub const NO_DEBUG: Flags = Flags(bindings::IRQF_NO_DEBUG as _);
>> +}
>> +
>> +/// 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 as _,
>> +
>> +    /// The interrupt was handled by this device.
>> +    Handled = bindings::irqreturn_IRQ_HANDLED as _,
>> +}
>> +
>> +/// Callbacks for an IRQ handler.
>> +pub trait Handler: Sync {
>> +    /// The actual handler function. As usual, sleeps are not allowed in IRQ
>> +    /// context.
>> +    fn handle_irq(&self) -> IrqReturn;
>> +}
>> +
>> +/// A registration of an IRQ handler for a given IRQ line.
>> +///
>> +/// # Examples
>> +///
>> +/// The following is an example of using `Registration`:
>> +///
>> +/// ```
>> +/// use kernel::prelude::*;
>> +/// use kernel::irq::request::flags;
>> +/// use kernel::irq::request::Registration;
>> +/// use kernel::irq::request::IrqReturn;
>> +/// use kernel::sync::Arc;
>> +/// use kernel::sync::SpinLock;
>> +/// 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(SpinLock<u32>);
>> +///
>> +/// // [`handle_irq`] takes &self. This example illustrates interior
>> +/// // mutability can be used when share the data between process context and IRQ
>> +/// // context.
>> +/// //
>> +/// // Ideally, this example would be using a version of SpinLock that is aware
>> +/// // of `spin_lock_irqsave` and `spin_lock_irqrestore`, but that is not yet
>> +/// // implemented.
>> +///
>> +/// 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_irq(&self) -> IrqReturn {
>> +///         // We now have exclusive access to the data by locking the SpinLock.
>> +///         let mut data = self.0.lock();
>> +///         *data += 1;
>> +///
>> +///         IrqReturn::Handled
>> +///     }
>> +/// }
>> +///
>> +/// // This is running in process context.
>> +/// fn register_irq(irq: u32, handler: Handler) -> Result<Arc<Registration<Handler>>> {
>> +///     let registration = Registration::register(irq, flags::SHARED, c_str!("my-device"), handler);
>> +///
>> +///     // You can have as many references to the registration as you want, so
>> +///     // multiple parts of the driver can access it.
>> +///     let registration = Arc::pin_init(registration, GFP_KERNEL)?;
>> +///
>> +///     // The handler may be called immediately after the function above
>> +///     // returns, possibly in a different CPU.
>> +///
>> +///     {
>> +///         // The data can be accessed from the process context too.
>> +///         let mut data = registration.handler().0.lock();
>> +///         *data = 42;
>> +///     }
>> +///
>> +///     Ok(registration)
>> +/// }
>> +///
>> +/// # Ok::<(), Error>(())
>> +///```
>> +///
>> +/// # Invariants
>> +///
>> +/// * We own an irq handler using `&self` as its private data.
>> +///
>> +#[pin_data(PinnedDrop)]
>> +pub struct Registration<T: Handler> {
>> +    irq: u32,
>> +    #[pin]
>> +    handler: T,
>> +    #[pin]
>> +    /// Pinned because we need address stability so that we can pass a pointer
>> +    /// to the callback.
>> +    _pin: PhantomPinned,
>> +}
>> +
>> +impl<T: Handler> Registration<T> {
>> +    /// Registers the IRQ handler with the system for the given IRQ number. The
>> +    /// handler must be able to be called as soon as this function returns.
>> +    pub fn register(
>> +        irq: u32,
>> +        flags: Flags,
>> +        name: &'static CStr,
>> +        handler: T,
>> +    ) -> impl PinInit<Self, Error> {
>> +        let closure = move |slot: *mut Self| {
>> +            // SAFETY: The slot passed to pin initializer is valid for writing.
>> +            unsafe {
>> +                slot.write(Self {
>> +                    irq,
>> +                    handler,
>> +                    _pin: PhantomPinned,
>> +                })
>> +            };
>> +
>> +            // 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.
>> +            let res = to_result(unsafe {
>> +                bindings::request_irq(
>> +                    irq,
>> +                    Some(handle_irq_callback::<T>),
>> +                    flags.0,
>> +                    name.as_char_ptr(),
>> +                    &*slot as *const _ as *mut core::ffi::c_void,
>> +                )
>> +            });
>> +
>> +            if res.is_err() {
>> +                // SAFETY: We are returning an error, so we can destroy the slot.
>> +                unsafe { core::ptr::drop_in_place(addr_of_mut!((*slot).handler)) };
>> +            }
>> +
>> +            res
>> +        };
>> +
>> +        // SAFETY:
>> +        // - if this returns Ok, then every field of `slot` is fully
>> +        // initialized.
>> +        // - if this returns an error, then the slot does not need to remain
>> +        // valid.
>> +        unsafe { pin_init_from_closure(closure) }
>> +    }
>> +
>> +    /// Returns a reference to the handler that was registered with the system.
>> +    pub fn handler(&self) -> &T {
>> +        // SAFETY: `handler` is initialized in `register`, and we require that
>> +        // T: Sync.
>> +        &self.handler
>> +    }
>> +}
>> +
>> +#[pinned_drop]
>> +impl<T: Handler> PinnedDrop for Registration<T> {
>> +    fn drop(self: Pin<&mut Self>) {
>> +        // SAFETY:
>> +        // - `self.irq` is the same as the one passed to `reques_irq`.
>> +        // -  `&self` was passed to `request_irq` as the cookie. It is
>> +        // guaranteed to be unique by the type system, since each call to
>> +        // `register` will return a different instance of `Registration`.
>> +        //
>> +        // 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 as *const Self as *mut core::ffi::c_void) };
>> +    }
>> +}
>> +
>> +/// The value that can be returned from `ThreadedHandler::handle_irq`.
>> +pub enum ThreadedIrqReturn {
>> +    /// The interrupt was not from this device or was not handled.
>> +    None = bindings::irqreturn_IRQ_NONE as _,
>> +
>> +    /// The interrupt was handled by this device.
>> +    Handled = bindings::irqreturn_IRQ_HANDLED as _,
>> +
>> +    /// The handler wants the handler thread to wake up.
>> +    WakeThread = bindings::irqreturn_IRQ_WAKE_THREAD as _,
>> +}
>> +
>> +/// Callbacks for a threaded IRQ handler.
>> +pub trait ThreadedHandler: Sync {
>> +    /// The actual handler function. As usual, sleeps are not allowed in IRQ
>> +    /// context.
>> +    fn handle_irq(&self) -> ThreadedIrqReturn;
>> +
>> +    /// The threaded handler function. This function is called from the irq
>> +    /// handler thread, which is automatically created by the system.
>> +    fn thread_fn(&self) -> IrqReturn;
>> +}
>> +
>> +/// A registration of a threaded IRQ handler for a given IRQ line.
>> +///
>> +/// Two callbacks are required: one to handle the IRQ, and one to handle any
>> +/// other work in a separate thread.
>> +///
>> +/// The thread handler is only called if the IRQ handler returns `WakeThread`.
>> +///
>> +/// # Examples
>> +///
>> +/// The following is an example of using `ThreadedRegistration`:
>> +///
>> +/// ```
>> +/// use kernel::prelude::*;
>> +/// use kernel::irq::request::flags;
>> +/// use kernel::irq::request::ThreadedIrqReturn;
>> +/// use kernel::irq::request::ThreadedRegistration;
>> +/// use kernel::irq::request::IrqReturn;
>> +/// use kernel::sync::Arc;
>> +/// use kernel::sync::SpinLock;
>> +/// use kernel::alloc::flags::GFP_KERNEL;
>> +/// use kernel::c_str;
>> +///
>> +/// // 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(SpinLock<u32>);
>> +///
>> +/// // [`handle_irq`] takes &self. This example illustrates interior
>> +/// // mutability can be used when share the data between process context and IRQ
>> +/// // context.
>> +/// //
>> +/// // Ideally, this example would be using a version of SpinLock that is aware
>> +/// // of `spin_lock_irqsave` and `spin_lock_irqrestore`, but that is not yet
>> +/// // implemented.
>> +///
>> +/// type Handler = Data;
>> +///
>> +/// impl kernel::irq::request::ThreadedHandler for Handler {
>> +///     // This is executing in IRQ context in some CPU. Other CPUs can still
>> +///     // try to access to data.
>> +///     fn handle_irq(&self) -> ThreadedIrqReturn {
>> +///         // We now have exclusive access to the data by locking the SpinLock.
>> +///         let mut data = self.0.lock();
>> +///         *data += 1;
>> +///
>> +///         // By returning `WakeThread`, we indicate to the system that the
>> +///         // thread function should be called. Otherwise, return
>> +///         // ThreadedIrqReturn::Handled.
>> +///         ThreadedIrqReturn::WakeThread
>> +///     }
>> +///
>> +///     // This will run (in a separate kthread) iff `handle_irq` returns
> typo iff

I mean the acronym for “if and only if”.

>> +///     // `WakeThread`.
>> +///     fn thread_fn(&self) -> IrqReturn {
>> +///         // We now have exclusive access to the data by locking the SpinLock.
>> +///         let mut data = self.0.lock();
>> +///         *data += 1;
>> +///
>> +///         IrqReturn::Handled
>> +///     }
>> +/// }
>> +///
>> +/// // This is running in process context.
>> +/// fn register_threaded_irq(irq: u32, handler: Handler) -> Result<Arc<ThreadedRegistration<Handler>>> {
>> +///     let registration = ThreadedRegistration::register(irq, flags::SHARED, c_str!("my-device"), handler);
>> +///
>> +///     // You can have as many references to the registration as you want, so
>> +///     // multiple parts of the driver can access it.
>> +///     let registration = Arc::pin_init(registration, GFP_KERNEL)?;
> You can use Arc::new instead, since you don't need pin_init.

Register returns `impl PinInit`, my understanding is that this has to be passed to the corresponding
`pin_init` function for types that implement the pin init trait?

>> +///
>> +///     // The handler may be called immediately after the function above
>> +///     // returns, possibly in a different CPU.
>> +///
>> +///     {
>> +///         // The data can be accessed from the process context too.
>> +///         let mut data = registration.handler().0.lock();
>> +///         *data = 42;
>> +///     }
>> +///
>> +///     Ok(registration)
>> +/// }
>> +///
>> +///
>> +/// # Ok::<(), Error>(())
>> +///```
>> +///
>> +/// # Invariants
>> +///
>> +/// * We own an irq handler using `&self` as its private data.
>> +///
>> +#[pin_data(PinnedDrop)]
>> +pub struct ThreadedRegistration<T: ThreadedHandler> {
>> +    irq: u32,
>> +    #[pin]
>> +    handler: T,
>> +    #[pin]
>> +    /// Pinned because we need address stability so that we can pass a pointer
>> +    /// to the callback.
>> +    _pin: PhantomPinned,
>> +}
>> +
>> +impl<T: ThreadedHandler> ThreadedRegistration<T> {
>> +    /// Registers the IRQ handler with the system for the given IRQ number. The
>> +    /// handler must be able to be called as soon as this function returns.
>> +    pub fn register(
>> +        irq: u32,
>> +        flags: Flags,
>> +        name: &'static CStr,
>> +        handler: T,
>> +    ) -> impl PinInit<Self, Error> {
>> +        let closure = move |slot: *mut Self| {
>> +            // SAFETY: The slot passed to pin initializer is valid for writing.
>> +            unsafe {
>> +                slot.write(Self {
>> +                    irq,
>> +                    handler,
>> +                    _pin: PhantomPinned,
>> +                })
>> +            };
>> +
>> +            // SAFETY:
>> +            // - The callbacks are valid for use with request_threaded_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.
>> +            let res = to_result(unsafe {
>> +                bindings::request_threaded_irq(
>> +                    irq,
>> +                    Some(handle_threaded_irq_callback::<T>),
>> +                    Some(thread_fn_callback::<T>),
>> +                    flags.0,
>> +                    name.as_char_ptr(),
>> +                    slot.cast(),
>> +                )
>> +            });
>> +
>> +            if res.is_err() {
>> +                // SAFETY: We are returning an error, so we can destroy the slot.
>> +                unsafe { core::ptr::drop_in_place(addr_of_mut!((*slot).handler)) };
>> +            }
>> +
>> +            res
>> +        };
>> +
>> +        // SAFETY:
>> +        // - if this returns Ok(()), then every field of `slot` is fully
>> +        // initialized.
>> +        // - if this returns an error, then the slot does not need to remain
>> +        // valid.
>> +        unsafe { pin_init_from_closure(closure) }
>> +    }
>> +
>> +    /// Returns a reference to the handler that was registered with the system.
>> +    pub fn handler(&self) -> &T {
>> +        // SAFETY: `handler` is initialized in `register`, and we require that
>> +        // T: Sync.
>> +        &self.handler
>> +    }
>> +}
>> +
>> +#[pinned_drop]
>> +impl<T: ThreadedHandler> PinnedDrop for ThreadedRegistration<T> {
>> +    fn drop(self: Pin<&mut Self>) {
>> +        // SAFETY:
>> +        // - `self.irq` is the same as the one passed to `request_threaded_irq`.
>> +        // -  `&self` was passed to `request_threaded_irq` as the cookie. It is
>> +        // guaranteed to be unique by the type system, since each call to
>> +        // `register` will return a different instance of
>> +        // `ThreadedRegistration`.
>> +        //
>> +        // Notice that this will block until all handlers finish executing, so,
>> +        // at no point will &self be invalid while the handler is running.
>> +        unsafe { bindings::free_irq(self.irq, &*self as *const Self as *mut core::ffi::c_void) };
>> +    }
>> +}
>> +
>> +/// # Safety
>> +///
>> +/// This function should be only used as the callback in `request_irq`.
>> +unsafe extern "C" fn handle_irq_callback<T: Handler>(
>> +    _irq: i32,
>> +    ptr: *mut core::ffi::c_void,
>> +) -> core::ffi::c_uint {
>> +    // SAFETY: `ptr` is a poinhandle_threaded_irq_callbackter to T set in `Registration::new`
>> +    let data = unsafe { &*(ptr as *const T) };
>> +    T::handle_irq(data) as _
>> +
> 
> Can `handle_irq_callback` and `handle_threaded_irq_callback` be shared somehow?

Why? In the quest to save a few lines we might introduce bugs.

> 
> Otherwise maybe define them locally in the calling function, since nothing else
> should ever use them, so we might as well restrict their scope.

I am not against this, but they’re private anyways, their scope is already restricted
as a result of their visibility. This looks cleaner too.

> 
>> +
>> +/// # Safety
>> +///
>> +/// This function should be only used as the callback in `request_threaded_irq`.
>> +unsafe extern "C" fn handle_threaded_irq_callback<T: ThreadedHandler>(
>> +    _irq: i32,
>> +    ptr: *mut core::ffi::c_void,
>> +) -> core::ffi::c_uint {
>> +    // SAFETY: `ptr` is a pointer to T set in `ThreadedRegistration::new`
>> +    let data = unsafe { &*(ptr as *const T) };
>> +    T::handle_irq(data) as _
>> +}
>> +
>> +/// # Safety
>> +///
>> +/// This function should be only used as the callback in `request_threaded_irq`.
>> +unsafe extern "C" fn thread_fn_callback<T: ThreadedHandler>(
>> +    _irq: i32,
>> +    ptr: *mut core::ffi::c_void,
>> +) -> core::ffi::c_uint {
>> +    // SAFETY: `ptr` is a pointer to T set in `ThreadedRegistration::new`
>> +    let data = unsafe { &*(ptr as *const T) };
>> +    T::thread_fn(data) as _
>> +}
>> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
>> index e1065a7551a3..bba782e138c5 100644
>> --- a/rust/kernel/lib.rs
>> +++ b/rust/kernel/lib.rs
>> @@ -41,6 +41,7 @@
>> pub mod fs;
>> pub mod init;
>> pub mod ioctl;
>> +pub mod irq;
>> pub mod jump_label;
>> #[cfg(CONFIG_KUNIT)]
>> pub mod kunit;

Re: [PATCH v2] rust: irq: add support for request_irq()

[Christian Schrefl]

On 23.01.25 5:27 PM, Daniel Almeida wrote:
> Hi Christian,
> 
>> On 23 Jan 2025, at 13:00, Christian Schrefl <chrisi.schrefl@gmail.com> wrote:
>>
>> On 22.01.25 5:39 PM, Daniel Almeida wrote:
>>> Add support for registering IRQ handlers in Rust.
>>>
>>> IRQ handlers are extensively used in drivers when some peripheral wants to
>>> obtain the CPU attention. Registering a handler will make the system invoke the
>>> passed-in function whenever the chosen IRQ line is triggered.
>>>
>>> Both regular and threaded IRQ handlers are supported through a Handler (or
>>> ThreadedHandler) trait that is meant to be implemented by a type that:
>>>
>>> a) provides a function to be run by the system when the IRQ fires and,
>>>
>>> b) holds the shared data (i.e.: `T`) between process and IRQ contexts.
>>>
>>> The requirement that T is Sync derives from the fact that handlers might run
>>> concurrently with other processes executing the same driver, creating the
>>> potential for data races.
>>>
>>> Ideally, some interior mutability must be in place if T is to be mutated. This
>>> should usually be done through the in-flight SpinLockIrq type.
>>>
>>> Co-developed-by: Alice Ryhl <aliceryhl@google.com>
>>> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
>>> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
>>
>> It would be nice it you included a second patch that adds the ability to get
>> the irq number from a platform device, it should be pretty simple.
>> Of course this can also just be done separately.
> 
> Sure!
> 
>>
>> Otherwise this looks pretty good, a few small comments below.
>>
>>> ---
>>>
>>> Changes from v1:
>>>
>>> - Added Co-developed-by tag to account for the work that Alice did in order to
>>> figure out how to do this without Opaque<T> (Thanks!)
>>> - Removed Opaque<T> in favor of plain T
>>> - Fixed the examples
>>> - Made sure that the invariants sections are the last entry in the docs
>>> - Switched to slot.cast() where applicable,
>>> - Mentioned in the safety comments that we require that T: Sync,
>>> - Removed ThreadedFnReturn in favor of IrqReturn,
>>> - Improved the commit message
>>>
>>> Link to v1: https://lore.kernel.org/rust-for-linux/20241024-topic-panthor-rs-request_irq-v1-1-7cbc51c182ca@collabora.com/
>>>
>>>
>>> ---
>>> rust/bindings/bindings_helper.h |   1 +
>>> rust/helpers/helpers.c          |   1 +
>>> rust/helpers/irq.c              |   9 +
>>> rust/kernel/irq.rs              |   6 +
>>> rust/kernel/irq/request.rs      | 517 ++++++++++++++++++++++++++++++++
>>> rust/kernel/lib.rs              |   1 +
>>> 6 files changed, 535 insertions(+)
>>> create mode 100644 rust/helpers/irq.c
>>> create mode 100644 rust/kernel/irq.rs
>>> create mode 100644 rust/kernel/irq/request.rs
>>>
>>> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
>>> index 5c4dfe22f41a..0331c6273def 100644
>>> --- a/rust/bindings/bindings_helper.h
>>> +++ b/rust/bindings/bindings_helper.h
>>> @@ -15,6 +15,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>
>>> diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
>>> index dcf827a61b52..bfc499d7f4c1 100644
>>> --- a/rust/helpers/helpers.c
>>> +++ b/rust/helpers/helpers.c
>>> @@ -13,6 +13,7 @@
>>> #include "build_bug.c"
>>> #include "cred.c"
>>> #include "err.c"
>>> +#include "irq.c"
>>> #include "fs.c"
>>> #include "jump_label.c"
>>> #include "kunit.c"
>>> diff --git a/rust/helpers/irq.c b/rust/helpers/irq.c
>>> new file mode 100644
>>> index 000000000000..1faca428e2c0
>>> --- /dev/null
>>> +++ b/rust/helpers/irq.c
>>> @@ -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);
>>> +}
>>> diff --git a/rust/kernel/irq.rs b/rust/kernel/irq.rs
>>> new file mode 100644
>>> index 000000000000..3ab83c5bdb83
>>> --- /dev/null
>>> +++ b/rust/kernel/irq.rs
>>> @@ -0,0 +1,6 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +
>>> +//! IRQ abstractions
>>> +
>>> +/// IRQ allocation and handling
>>> +pub mod request;
>>> diff --git a/rust/kernel/irq/request.rs b/rust/kernel/irq/request.rs
>>> new file mode 100644
>>> index 000000000000..61e7d4a8f555
>>> --- /dev/null
>>> +++ b/rust/kernel/irq/request.rs
>>> @@ -0,0 +1,517 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +// SPDX-FileCopyrightText: Copyright 2025 Collabora ltd.
>>> +
>>> +//! IRQ allocation and handling
>>> +
>>> +use core::marker::PhantomPinned;
>>> +use core::ptr::addr_of_mut;
>>> +
>>> +use init::pin_init_from_closure;
>>> +
>>> +use crate::error::to_result;
>>> +use crate::prelude::*;
>>> +use crate::str::CStr;
>>> +
>>> +/// Flags to be used when registering IRQ handlers.
>>> +///
>>> +/// They can be combined with the operators `|`, `&`, and `!`.
>>> +///
>>> +/// Values can be used from the [`flags`] module.
>>> +#[derive(Clone, Copy)]
>>> +pub struct Flags(u64);
>>> +
>>> +impl core::ops::BitOr for Flags {
>>> +    type Output = Self;
>>> +    fn bitor(self, rhs: Self) -> Self::Output {
>>> +        Self(self.0 | rhs.0)
>>> +    }
>>> +}
>>> +
>>> +impl core::ops::BitAnd for Flags {
>>> +    type Output = Self;
>>> +    fn bitand(self, rhs: Self) -> Self::Output {
>>> +        Self(self.0 & rhs.0)
>>> +    }
>>> +}
>>> +
>>> +impl core::ops::Not for Flags {
>>> +    type Output = Self;
>>> +    fn not(self) -> Self::Output {
>>> +        Self(!self.0)
>>> +    }
>>> +}
>>> +
>>> +/// The flags that can be used when registering an IRQ handler.
>>> +pub mod flags {
>>
>> Maybe move flags into a separate file?
>> You even have a directory for irq.
> 
> Why? Most flags are defined in the same file. See alloc::flags, for example.#

Yeah its in alloc.rs, but no other implementation is after that.
The implmentations of the containers are in other files.

> 
> I am not against this, but since this is merely aesthetic in nature, we should maybe
> wait for input from more people.

I just think its easier to read when the file doesn't have this many definitions before
some actual implementation. But I don't mind that much to keep it as-is.

> 
>>
>>> +    use super::Flags;
>>> +
>>> +    use crate::bindings;
>>> +

<snip>

>>> +///         ThreadedIrqReturn::WakeThread
>>> +///     }
>>> +///
>>> +///     // This will run (in a separate kthread) iff `handle_irq` returns
>> typo iff
> 
> I mean the acronym for “if and only if”.

Ok

>
>>> +///     // `WakeThread`.
>>> +///     fn thread_fn(&self) -> IrqReturn {
>>> +///         // We now have exclusive access to the data by locking the SpinLock.
>>> +///         let mut data = self.0.lock();
>>> +///         *data += 1;
>>> +///
>>> +///         IrqReturn::Handled
>>> +///     }
>>> +/// }
>>> +///
>>> +/// // This is running in process context.
>>> +/// fn register_threaded_irq(irq: u32, handler: Handler) -> Result<Arc<ThreadedRegistration<Handler>>> {
>>> +///     let registration = ThreadedRegistration::register(irq, flags::SHARED, c_str!("my-device"), handler);
>>> +///
>>> +///     // You can have as many references to the registration as you want, so
>>> +///     // multiple parts of the driver can access it.
>>> +///     let registration = Arc::pin_init(registration, GFP_KERNEL)?;
>> You can use Arc::new instead, since you don't need pin_init.
> 
> Register returns `impl PinInit`, my understanding is that this has to be passed to the corresponding
> `pin_init` function for types that implement the pin init trait?

Ah ok sorry I thought let registration already stored the initialized value, not the `impl PinInit`.
>
>>> +///
>>> +///     // The handler may be called immediately after the function above
>>> +///     // returns, possibly in a different CPU.
>>> +///
>>> +///     {
>>> +///         // The data can be accessed from the process context too.
>>> +///         let mut data = registration.handler().0.lock();
>>> +///         *data = 42;
>>> +///     }
>>> +///
>>> +///     Ok(registration)
>>> +/// }
>>> +///
>>> +///
>>> +/// # Ok::<(), Error>(())
>>> +///```
>>> +///
>>> +/// # Invariants
>>> +///
>>> +/// * We own an irq handler using `&self` as its private data.
>>> +///
>>> +#[pin_data(PinnedDrop)]
>>> +pub struct ThreadedRegistration<T: ThreadedHandler> {
>>> +    irq: u32,
>>> +    #[pin]
>>> +    handler: T,
>>> +    #[pin]
>>> +    /// Pinned because we need address stability so that we can pass a pointer
>>> +    /// to the callback.
>>> +    _pin: PhantomPinned,
>>> +}
>>> +
>>> +impl<T: ThreadedHandler> ThreadedRegistration<T> {
>>> +    /// Registers the IRQ handler with the system for the given IRQ number. The
>>> +    /// handler must be able to be called as soon as this function returns.
>>> +    pub fn register(
>>> +        irq: u32,
>>> +        flags: Flags,
>>> +        name: &'static CStr,
>>> +        handler: T,
>>> +    ) -> impl PinInit<Self, Error> {
>>> +        let closure = move |slot: *mut Self| {
>>> +            // SAFETY: The slot passed to pin initializer is valid for writing.
>>> +            unsafe {
>>> +                slot.write(Self {
>>> +                    irq,
>>> +                    handler,
>>> +                    _pin: PhantomPinned,
>>> +                })
>>> +            };
>>> +
>>> +            // SAFETY:
>>> +            // - The callbacks are valid for use with request_threaded_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.
>>> +            let res = to_result(unsafe {
>>> +                bindings::request_threaded_irq(
>>> +                    irq,
>>> +                    Some(handle_threaded_irq_callback::<T>),
>>> +                    Some(thread_fn_callback::<T>),
>>> +                    flags.0,
>>> +                    name.as_char_ptr(),
>>> +                    slot.cast(),
>>> +                )
>>> +            });
>>> +
>>> +            if res.is_err() {
>>> +                // SAFETY: We are returning an error, so we can destroy the slot.
>>> +                unsafe { core::ptr::drop_in_place(addr_of_mut!((*slot).handler)) };
>>> +            }
>>> +
>>> +            res
>>> +        };
>>> +
>>> +        // SAFETY:
>>> +        // - if this returns Ok(()), then every field of `slot` is fully
>>> +        // initialized.
>>> +        // - if this returns an error, then the slot does not need to remain
>>> +        // valid.
>>> +        unsafe { pin_init_from_closure(closure) }
>>> +    }
>>> +
>>> +    /// Returns a reference to the handler that was registered with the system.
>>> +    pub fn handler(&self) -> &T {
>>> +        // SAFETY: `handler` is initialized in `register`, and we require that
>>> +        // T: Sync.
>>> +        &self.handler
>>> +    }
>>> +}
>>> +
>>> +#[pinned_drop]
>>> +impl<T: ThreadedHandler> PinnedDrop for ThreadedRegistration<T> {
>>> +    fn drop(self: Pin<&mut Self>) {
>>> +        // SAFETY:
>>> +        // - `self.irq` is the same as the one passed to `request_threaded_irq`.
>>> +        // -  `&self` was passed to `request_threaded_irq` as the cookie. It is
>>> +        // guaranteed to be unique by the type system, since each call to
>>> +        // `register` will return a different instance of
>>> +        // `ThreadedRegistration`.
>>> +        //
>>> +        // Notice that this will block until all handlers finish executing, so,
>>> +        // at no point will &self be invalid while the handler is running.
>>> +        unsafe { bindings::free_irq(self.irq, &*self as *const Self as *mut core::ffi::c_void) };
>>> +    }
>>> +}
>>> +
>>> +/// # Safety
>>> +///
>>> +/// This function should be only used as the callback in `request_irq`.
>>> +unsafe extern "C" fn handle_irq_callback<T: Handler>(
>>> +    _irq: i32,
>>> +    ptr: *mut core::ffi::c_void,
>>> +) -> core::ffi::c_uint {
>>> +    // SAFETY: `ptr` is a poinhandle_threaded_irq_callbackter to T set in `Registration::new`
>>> +    let data = unsafe { &*(ptr as *const T) };
>>> +    T::handle_irq(data) as _
>>> +
>>
>> Can `handle_irq_callback` and `handle_threaded_irq_callback` be shared somehow?
> 
> Why? In the quest to save a few lines we might introduce bugs.

Mostly curious if its possible, if it would require to hacky code just ignore it.
> 
>>
>> Otherwise maybe define them locally in the calling function, since nothing else
>> should ever use them, so we might as well restrict their scope.
> 
> I am not against this, but they’re private anyways, their scope is already restricted
> as a result of their visibility. This looks cleaner too.

I just think its cleaner if something that is only supposed to be used in one scope is
defined in that scope so it is automatically restricted to that.

But I guess that's just a matter of taste and I'm fine with keeping it as is.

> 
>>
>>> +
>>> +/// # Safety
>>> +///
>>> +/// This function should be only used as the callback in `request_threaded_irq`.
>>> +unsafe extern "C" fn handle_threaded_irq_callback<T: ThreadedHandler>(
>>> +    _irq: i32,
>>> +    ptr: *mut core::ffi::c_void,
>>> +) -> core::ffi::c_uint {
>>> +    // SAFETY: `ptr` is a pointer to T set in `ThreadedRegistration::new`
>>> +    let data = unsafe { &*(ptr as *const T) };
>>> +    T::handle_irq(data) as _
>>> +}
>>> +
>>> +/// # Safety
>>> +///
>>> +/// This function should be only used as the callback in `request_threaded_irq`.
>>> +unsafe extern "C" fn thread_fn_callback<T: ThreadedHandler>(
>>> +    _irq: i32,
>>> +    ptr: *mut core::ffi::c_void,
>>> +) -> core::ffi::c_uint {
>>> +    // SAFETY: `ptr` is a pointer to T set in `ThreadedRegistration::new`
>>> +    let data = unsafe { &*(ptr as *const T) };
>>> +    T::thread_fn(data) as _
>>> +}
>>> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
>>> index e1065a7551a3..bba782e138c5 100644
>>> --- a/rust/kernel/lib.rs
>>> +++ b/rust/kernel/lib.rs
>>> @@ -41,6 +41,7 @@
>>> pub mod fs;
>>> pub mod init;
>>> pub mod ioctl;
>>> +pub mod irq;
>>> pub mod jump_label;
>>> #[cfg(CONFIG_KUNIT)]
>>> pub mod kunit;
> 
> 

Re: [PATCH v2] rust: irq: add support for request_irq()

[Daniel Almeida]

> On 22 Jan 2025, at 13:39, Daniel Almeida <daniel.almeida@collabora.com> wrote:
> 
> Add support for registering IRQ handlers in Rust.
> 
> IRQ handlers are extensively used in drivers when some peripheral wants to
> obtain the CPU attention. Registering a handler will make the system invoke the
> passed-in function whenever the chosen IRQ line is triggered.
> 
> Both regular and threaded IRQ handlers are supported through a Handler (or
> ThreadedHandler) trait that is meant to be implemented by a type that:
> 
> a) provides a function to be run by the system when the IRQ fires and,
> 
> b) holds the shared data (i.e.: `T`) between process and IRQ contexts.
> 
> The requirement that T is Sync derives from the fact that handlers might run
> concurrently with other processes executing the same driver, creating the
> potential for data races.
> 
> Ideally, some interior mutability must be in place if T is to be mutated. This
> should usually be done through the in-flight SpinLockIrq type.
> 
> Co-developed-by: Alice Ryhl <aliceryhl@google.com>
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
> ---
> 
> Changes from v1:
> 
> - Added Co-developed-by tag to account for the work that Alice did in order to
> figure out how to do this without Opaque<T> (Thanks!)
> - Removed Opaque<T> in favor of plain T
> - Fixed the examples
> - Made sure that the invariants sections are the last entry in the docs
> - Switched to slot.cast() where applicable,
> - Mentioned in the safety comments that we require that T: Sync,
> - Removed ThreadedFnReturn in favor of IrqReturn,
> - Improved the commit message
> 
> Link to v1: https://lore.kernel.org/rust-for-linux/20241024-topic-panthor-rs-request_irq-v1-1-7cbc51c182ca@collabora.com/
> 
> 
> ---
> rust/bindings/bindings_helper.h |   1 +
> rust/helpers/helpers.c          |   1 +
> rust/helpers/irq.c              |   9 +
> rust/kernel/irq.rs              |   6 +
> rust/kernel/irq/request.rs      | 517 ++++++++++++++++++++++++++++++++
> rust/kernel/lib.rs              |   1 +
> 6 files changed, 535 insertions(+)
> create mode 100644 rust/helpers/irq.c
> create mode 100644 rust/kernel/irq.rs
> create mode 100644 rust/kernel/irq/request.rs
> 
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index 5c4dfe22f41a..0331c6273def 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -15,6 +15,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>
> diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
> index dcf827a61b52..bfc499d7f4c1 100644
> --- a/rust/helpers/helpers.c
> +++ b/rust/helpers/helpers.c
> @@ -13,6 +13,7 @@
> #include "build_bug.c"
> #include "cred.c"
> #include "err.c"
> +#include "irq.c"
> #include "fs.c"
> #include "jump_label.c"
> #include "kunit.c"
> diff --git a/rust/helpers/irq.c b/rust/helpers/irq.c
> new file mode 100644
> index 000000000000..1faca428e2c0
> --- /dev/null
> +++ b/rust/helpers/irq.c
> @@ -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);
> +}
> diff --git a/rust/kernel/irq.rs b/rust/kernel/irq.rs
> new file mode 100644
> index 000000000000..3ab83c5bdb83
> --- /dev/null
> +++ b/rust/kernel/irq.rs
> @@ -0,0 +1,6 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! IRQ abstractions
> +
> +/// IRQ allocation and handling
> +pub mod request;
> diff --git a/rust/kernel/irq/request.rs b/rust/kernel/irq/request.rs
> new file mode 100644
> index 000000000000..61e7d4a8f555
> --- /dev/null
> +++ b/rust/kernel/irq/request.rs
> @@ -0,0 +1,517 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// SPDX-FileCopyrightText: Copyright 2025 Collabora ltd.
> +
> +//! IRQ allocation and handling
> +
> +use core::marker::PhantomPinned;
> +use core::ptr::addr_of_mut;
> +
> +use init::pin_init_from_closure;
> +
> +use crate::error::to_result;
> +use crate::prelude::*;
> +use crate::str::CStr;
> +
> +/// Flags to be used when registering IRQ handlers.
> +///
> +/// They can be combined with the operators `|`, `&`, and `!`.
> +///
> +/// Values can be used from the [`flags`] module.
> +#[derive(Clone, Copy)]
> +pub struct Flags(u64);
> +
> +impl core::ops::BitOr for Flags {
> +    type Output = Self;
> +    fn bitor(self, rhs: Self) -> Self::Output {
> +        Self(self.0 | rhs.0)
> +    }
> +}
> +
> +impl core::ops::BitAnd for Flags {
> +    type Output = Self;
> +    fn bitand(self, rhs: Self) -> Self::Output {
> +        Self(self.0 & rhs.0)
> +    }
> +}
> +
> +impl core::ops::Not for Flags {
> +    type Output = Self;
> +    fn not(self) -> Self::Output {
> +        Self(!self.0)
> +    }
> +}
> +
> +/// The flags that can be used when registering an IRQ handler.
> +pub mod flags {
> +    use super::Flags;
> +
> +    use crate::bindings;
> +
> +    /// Use the interrupt line as already configured.
> +    pub const TRIGGER_NONE: Flags = Flags(bindings::IRQF_TRIGGER_NONE as _);
> +
> +    /// The interrupt is triggered when the signal goes from low to high.
> +    pub const TRIGGER_RISING: Flags = Flags(bindings::IRQF_TRIGGER_RISING as _);
> +
> +    /// The interrupt is triggered when the signal goes from high to low.
> +    pub const TRIGGER_FALLING: Flags = Flags(bindings::IRQF_TRIGGER_FALLING as _);
> +
> +    /// The interrupt is triggered while the signal is held high.
> +    pub const TRIGGER_HIGH: Flags = Flags(bindings::IRQF_TRIGGER_HIGH as _);
> +
> +    /// The interrupt is triggered while the signal is held low.
> +    pub const TRIGGER_LOW: Flags = Flags(bindings::IRQF_TRIGGER_LOW as _);
> +
> +    /// Allow sharing the irq among several devices.
> +    pub const SHARED: Flags = Flags(bindings::IRQF_SHARED as _);
> +
> +    /// Set by callers when they expect sharing mismatches to occur.
> +    pub const PROBE_SHARED: Flags = Flags(bindings::IRQF_PROBE_SHARED as _);
> +
> +    /// Flag to mark this interrupt as timer interrupt.
> +    pub const TIMER: Flags = Flags(bindings::IRQF_TIMER as _);
> +
> +    /// Interrupt is per cpu.
> +    pub const PERCPU: Flags = Flags(bindings::IRQF_PERCPU as _);
> +
> +    /// Flag to exclude this interrupt from irq balancing.
> +    pub const NOBALANCING: Flags = Flags(bindings::IRQF_NOBALANCING as _);
> +
> +    /// Interrupt is used for polling (only the interrupt that is registered
> +    /// first in a shared interrupt is considered for performance reasons).
> +    pub const IRQPOLL: Flags = Flags(bindings::IRQF_IRQPOLL as _);
> +
> +    /// Interrupt is not reenabled after the hardirq handler finished. Used by
> +    /// threaded interrupts which need to keep the irq line disabled until the
> +    /// threaded handler has been run.
> +    pub const ONESHOT: Flags = Flags(bindings::IRQF_ONESHOT as _);
> +
> +    /// Do not disable this IRQ during suspend. Does not guarantee that this
> +    /// interrupt will wake the system from a suspended state.
> +    pub const NO_SUSPEND: Flags = Flags(bindings::IRQF_NO_SUSPEND as _);
> +
> +    /// Force enable it on resume even if [`NO_SUSPEND`] is set.
> +    pub const FORCE_RESUME: Flags = Flags(bindings::IRQF_FORCE_RESUME as _);
> +
> +    /// Interrupt cannot be threaded.
> +    pub const NO_THREAD: Flags = Flags(bindings::IRQF_NO_THREAD as _);
> +
> +    /// Resume IRQ early during syscore instead of at device resume time.
> +    pub const EARLY_RESUME: Flags = Flags(bindings::IRQF_EARLY_RESUME as _);
> +
> +    /// If the IRQ is shared with a NO_SUSPEND user, execute this interrupt
> +    /// handler after suspending interrupts. For system wakeup devices users
> +    /// need to implement wakeup detection in their interrupt handlers.
> +    pub const COND_SUSPEND: Flags = Flags(bindings::IRQF_COND_SUSPEND as _);
> +
> +    /// Don't enable IRQ or NMI automatically when users request it. Users will
> +    /// enable it explicitly by `enable_irq` or `enable_nmi` later.
> +    pub const NO_AUTOEN: Flags = Flags(bindings::IRQF_NO_AUTOEN as _);
> +
> +    /// Exclude from runnaway detection for IPI and similar handlers, depends on
> +    /// `PERCPU`.
> +    pub const NO_DEBUG: Flags = Flags(bindings::IRQF_NO_DEBUG as _);
> +}
> +
> +/// 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 as _,
> +
> +    /// The interrupt was handled by this device.
> +    Handled = bindings::irqreturn_IRQ_HANDLED as _,
> +}
> +
> +/// Callbacks for an IRQ handler.
> +pub trait Handler: Sync {
> +    /// The actual handler function. As usual, sleeps are not allowed in IRQ
> +    /// context.
> +    fn handle_irq(&self) -> IrqReturn;
> +}
> +
> +/// A registration of an IRQ handler for a given IRQ line.
> +///
> +/// # Examples
> +///
> +/// The following is an example of using `Registration`:
> +///
> +/// ```
> +/// use kernel::prelude::*;
> +/// use kernel::irq::request::flags;
> +/// use kernel::irq::request::Registration;

By the way, I wonder if a re-export would be beneficial? I find it a bit tedious to specify this path.

It also clashes with kernel::driver::Registration and kernel::driver::drm::Registration, so I find myself
continuously writing an alias for it, i.e.:

```
Use kernel::irq::request::Registration as IrqRegistration;
Use kernel::irq::request::Handler as IrqHandler;
```

Looking at mq.rs <http://mq.rs/>, I see Andreas did something similar:

```
pub use operations::Operations;
pub use request::Request;
pub use tag_set::TagSet;
```

Asking for opinions here since this is a bit cosmetic in nature. IMHO, at least the ‘request’ part of the path has to go.

— Daniel

Re: [PATCH v2] rust: irq: add support for request_irq()

[Guangbo Cui]

> By the way, I wonder if a re-export would be beneficial? I find it a bit tedious to specify this path.
> 
> It also clashes with kernel::driver::Registration and kernel::driver::drm::Registration, so I find myself
> continuously writing an alias for it, i.e.:
> 
> ```
> Use kernel::irq::request::Registration as IrqRegistration;
> Use kernel::irq::request::Handler as IrqHandler;
> ```
> 
> Looking at mq.rs <http://mq.rs/>, I see Andreas did something similar:
> 
> ```
> pub use operations::Operations;
> pub use request::Request;
> pub use tag_set::TagSet;
> ```
> 
> Asking for opinions here since this is a bit cosmetic in nature. IMHO, at least the ‘request’ part of the path has to go.

Maybe the Rust subsystem should have a unified convention for this？
Re-export is both beneficial and necessary. The naming retains `Registration`
and `Handler` to be consistent with other modules. And accessing it with the
module prefix, such as `irq::Registration`. Just like @dakr did in `rust_driver_pci.rs` 
and `rust_driver_platform.rs`: `pci::Driver` & `platform::Driver`; `pci::Device` & `platform::Device`.

Hope this help, if not, ignore it.

Best regards,
Guangbo Cui

Re: [PATCH v2] rust: irq: add support for request_irq()

[Andreas Hindborg]

"Daniel Almeida" <daniel.almeida@collabora.com> writes:

> Hi Christian,
>
>> On 23 Jan 2025, at 13:00, Christian Schrefl <chrisi.schrefl@gmail.com> wrote:
>>
>> On 22.01.25 5:39 PM, Daniel Almeida wrote:
>>> Add support for registering IRQ handlers in Rust.

[...]

>>> +/// The flags that can be used when registering an IRQ handler.
>>> +pub mod flags {
>>
>> Maybe move flags into a separate file?
>> You even have a directory for irq.
>
> Why? Most flags are defined in the same file. See alloc::flags, for example.
>
> I am not against this, but since this is merely aesthetic in nature, we should maybe
> wait for input from more people.

I would prefer them in another file as well. Not a hard requirement though.

[...]

>>> +///     // This will run (in a separate kthread) iff `handle_irq` returns
>> typo iff
>
> I mean the acronym for “if and only if”.

Since it is not immediately obvious to all readers, prefer "if and only
if". I think we discussed this at pin-init patches a long while ago.


Best regards,
Andreas Hindborg

Re: [PATCH v2] rust: irq: add support for request_irq()

[Andreas Hindborg]

"Daniel Almeida" <daniel.almeida@collabora.com> writes:

>> On 22 Jan 2025, at 13:39, Daniel Almeida <daniel.almeida@collabora.com> wrote:
>>
>> Add support for registering IRQ handlers in Rust.

[...]

>> +/// use kernel::prelude::*;
>> +/// use kernel::irq::request::flags;
>> +/// use kernel::irq::request::Registration;
>
> By the way, I wonder if a re-export would be beneficial? I find it a bit tedious to specify this path.
>
> It also clashes with kernel::driver::Registration and kernel::driver::drm::Registration, so I find myself
> continuously writing an alias for it, i.e.:
>
> ```
> Use kernel::irq::request::Registration as IrqRegistration;
> Use kernel::irq::request::Handler as IrqHandler;
> ```
>
> Looking at mq.rs <http://mq.rs/>, I see Andreas did something similar:
>
> ```
> pub use operations::Operations;
> pub use request::Request;
> pub use tag_set::TagSet;
> ```
>
> Asking for opinions here since this is a bit cosmetic in nature. IMHO, at least the ‘request’ part of the path has to go.

For block I usually import `mq` if there can be clashes, then I can use
`mq::Request`, which is not so bad. I think a reexport to make
`irq::request::Request` available as `irq::Request` would be nice.

In `mq`, most sub modules are not pub, so the only way to reach the
types is through the reexport.


Best regards,
Andreas Hindborg

Re: [PATCH v2] rust: irq: add support for request_irq()

[Andreas Hindborg]

"Daniel Almeida" <daniel.almeida@collabora.com> writes:

> Add support for registering IRQ handlers in Rust.
>
> IRQ handlers are extensively used in drivers when some peripheral wants to
> obtain the CPU attention. Registering a handler will make the system invoke the
> passed-in function whenever the chosen IRQ line is triggered.
>
> Both regular and threaded IRQ handlers are supported through a Handler (or
> ThreadedHandler) trait that is meant to be implemented by a type that:
>
> a) provides a function to be run by the system when the IRQ fires and,
>
> b) holds the shared data (i.e.: `T`) between process and IRQ contexts.
>
> The requirement that T is Sync derives from the fact that handlers might run
> concurrently with other processes executing the same driver, creating the
> potential for data races.
>
> Ideally, some interior mutability must be in place if T is to be mutated. This
> should usually be done through the in-flight SpinLockIrq type.
>
> Co-developed-by: Alice Ryhl <aliceryhl@google.com>
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>

What is the base commit to apply this patch to? I am getting some
compiler errors when trying it out:

    error[E0308]: mismatched types
      --> /home/aeh/src/linux-rust/linux/rust/kernel/irq/request.rs:240:21
        |
    237 |                 bindings::request_irq(
        |                 --------------------- arguments to this function are incorrect
    ...
    240 |                     flags.0,
        |                     ^^^^^^^ expected `usize`, found `u64`
        |

[...]

> diff --git a/rust/kernel/irq.rs b/rust/kernel/irq.rs
> new file mode 100644
> index 000000000000..3ab83c5bdb83
> --- /dev/null
> +++ b/rust/kernel/irq.rs
> @@ -0,0 +1,6 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! IRQ abstractions

We could do with a longer story here. Also, missing a period.

> +
> +/// IRQ allocation and handling
> +pub mod request;
> diff --git a/rust/kernel/irq/request.rs b/rust/kernel/irq/request.rs
> new file mode 100644
> index 000000000000..61e7d4a8f555
> --- /dev/null
> +++ b/rust/kernel/irq/request.rs
> @@ -0,0 +1,517 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// SPDX-FileCopyrightText: Copyright 2025 Collabora ltd.
> +
> +//! IRQ allocation and handling
> +
> +use core::marker::PhantomPinned;
> +use core::ptr::addr_of_mut;
> +
> +use init::pin_init_from_closure;
> +
> +use crate::error::to_result;
> +use crate::prelude::*;
> +use crate::str::CStr;
> +
> +/// Flags to be used when registering IRQ handlers.
> +///
> +/// They can be combined with the operators `|`, `&`, and `!`.
> +///
> +/// Values can be used from the [`flags`] module.
> +#[derive(Clone, Copy)]
> +pub struct Flags(u64);
> +
> +impl core::ops::BitOr for Flags {
> +    type Output = Self;
> +    fn bitor(self, rhs: Self) -> Self::Output {
> +        Self(self.0 | rhs.0)
> +    }
> +}
> +
> +impl core::ops::BitAnd for Flags {
> +    type Output = Self;
> +    fn bitand(self, rhs: Self) -> Self::Output {
> +        Self(self.0 & rhs.0)
> +    }
> +}
> +
> +impl core::ops::Not for Flags {
> +    type Output = Self;
> +    fn not(self) -> Self::Output {
> +        Self(!self.0)
> +    }
> +}
> +
> +/// The flags that can be used when registering an IRQ handler.
> +pub mod flags {
> +    use super::Flags;
> +
> +    use crate::bindings;
> +
> +    /// Use the interrupt line as already configured.
> +    pub const TRIGGER_NONE: Flags = Flags(bindings::IRQF_TRIGGER_NONE as _);

What is the reason for the `as _` in all these? Should the flag type be
something else?

> +
> +    /// The interrupt is triggered when the signal goes from low to high.
> +    pub const TRIGGER_RISING: Flags = Flags(bindings::IRQF_TRIGGER_RISING as _);
> +
> +    /// The interrupt is triggered when the signal goes from high to low.
> +    pub const TRIGGER_FALLING: Flags = Flags(bindings::IRQF_TRIGGER_FALLING as _);
> +
> +    /// The interrupt is triggered while the signal is held high.
> +    pub const TRIGGER_HIGH: Flags = Flags(bindings::IRQF_TRIGGER_HIGH as _);
> +
> +    /// The interrupt is triggered while the signal is held low.
> +    pub const TRIGGER_LOW: Flags = Flags(bindings::IRQF_TRIGGER_LOW as _);
> +
> +    /// Allow sharing the irq among several devices.
> +    pub const SHARED: Flags = Flags(bindings::IRQF_SHARED as _);
> +
> +    /// Set by callers when they expect sharing mismatches to occur.

What is a sharing mismatch?

> +    pub const PROBE_SHARED: Flags = Flags(bindings::IRQF_PROBE_SHARED as _);
> +
> +    /// Flag to mark this interrupt as timer interrupt.

The "Flag to ..." strikes me as odd when most of the other descriptions
have a different wording.

> +    pub const TIMER: Flags = Flags(bindings::IRQF_TIMER as _);
> +
> +    /// Interrupt is per cpu.
> +    pub const PERCPU: Flags = Flags(bindings::IRQF_PERCPU as _);
> +
> +    /// Flag to exclude this interrupt from irq balancing.
> +    pub const NOBALANCING: Flags = Flags(bindings::IRQF_NOBALANCING as _);
> +
> +    /// Interrupt is used for polling (only the interrupt that is registered
> +    /// first in a shared interrupt is considered for performance reasons).
> +    pub const IRQPOLL: Flags = Flags(bindings::IRQF_IRQPOLL as _);
> +
> +    /// Interrupt is not reenabled after the hardirq handler finished. Used by
> +    /// threaded interrupts which need to keep the irq line disabled until the
> +    /// threaded handler has been run.
> +    pub const ONESHOT: Flags = Flags(bindings::IRQF_ONESHOT as _);
> +
> +    /// Do not disable this IRQ during suspend. Does not guarantee that this
> +    /// interrupt will wake the system from a suspended state.
> +    pub const NO_SUSPEND: Flags = Flags(bindings::IRQF_NO_SUSPEND as _);
> +
> +    /// Force enable it on resume even if [`NO_SUSPEND`] is set.

Perhaps: Force enable the interrupt even if ...

> +    pub const FORCE_RESUME: Flags = Flags(bindings::IRQF_FORCE_RESUME as _);
> +
> +    /// Interrupt cannot be threaded.
> +    pub const NO_THREAD: Flags = Flags(bindings::IRQF_NO_THREAD as _);
> +
> +    /// Resume IRQ early during syscore instead of at device resume time.
> +    pub const EARLY_RESUME: Flags = Flags(bindings::IRQF_EARLY_RESUME as _);
> +
> +    /// If the IRQ is shared with a NO_SUSPEND user, execute this interrupt

Please link `NO_SUSPEND`.

> +    /// handler after suspending interrupts. For system wakeup devices users
> +    /// need to implement wakeup detection in their interrupt handlers.
> +    pub const COND_SUSPEND: Flags = Flags(bindings::IRQF_COND_SUSPEND as _);
> +
> +    /// Don't enable IRQ or NMI automatically when users request it. Users will
> +    /// enable it explicitly by `enable_irq` or `enable_nmi` later.
> +    pub const NO_AUTOEN: Flags = Flags(bindings::IRQF_NO_AUTOEN as _);
> +
> +    /// Exclude from runnaway detection for IPI and similar handlers, depends on
> +    /// `PERCPU`.
> +    pub const NO_DEBUG: Flags = Flags(bindings::IRQF_NO_DEBUG as _);
> +}
> +
> +/// The value that can be returned from an IrqHandler or a ThreadedIrqHandler.
> +pub enum IrqReturn {

I learned recently that if you choose the right representation here, you
don't need to cast here and when you call `Handler::handle_irq`. I think
`#[repr(u32)]` is the one to use here.

> +    /// The interrupt was not from this device or was not handled.
> +    None = bindings::irqreturn_IRQ_NONE as _,
> +
> +    /// The interrupt was handled by this device.
> +    Handled = bindings::irqreturn_IRQ_HANDLED as _,
> +}
> +
> +/// Callbacks for an IRQ handler.
> +pub trait Handler: Sync {
> +    /// The actual handler function. As usual, sleeps are not allowed in IRQ
> +    /// context.
> +    fn handle_irq(&self) -> IrqReturn;
> +}

What is the reason for moving away from the following:


    pub trait Handler {
        /// The context data associated with and made available to the handler.
        type Data: ForeignOwnable;

        /// Called from interrupt context when the irq happens.
        fn handle_irq(data: <Self::Data as ForeignOwnable>::Borrowed<'_>) -> Return;
    }


I think we will run into problems if we want to pass `Arc<Foo>` as the
handler. I don't think we can `impl Handler for Arc<Foo>` in a driver
crate, since both `Handler` and `Arc` are defined in external crates


[...]

> +#[pin_data(PinnedDrop)]
> +pub struct ThreadedRegistration<T: ThreadedHandler> {
> +    irq: u32,
> +    #[pin]
> +    handler: T,
> +    #[pin]
> +    /// Pinned because we need address stability so that we can pass a pointer
> +    /// to the callback.
> +    _pin: PhantomPinned,
> +}

As others have mentioned, I wonder if we can avoid the code duplication
that makes up most of the rest of this patch.


Best regards,
Andreas Hindborg

Re: [PATCH v2] rust: irq: add support for request_irq()

[Daniel Almeida]

Hi Andreas,

I will address your other comments later this week, as I’m off for a few days.

> On 4 Mar 2025, at 10:43, Andreas Hindborg <a.hindborg@kernel.org> wrote:
> 
> "Daniel Almeida" <daniel.almeida@collabora.com> writes:
> 
>> Add support for registering IRQ handlers in Rust.
>> 
>> IRQ handlers are extensively used in drivers when some peripheral wants to
>> obtain the CPU attention. Registering a handler will make the system invoke the
>> passed-in function whenever the chosen IRQ line is triggered.
>> 
>> Both regular and threaded IRQ handlers are supported through a Handler (or
>> ThreadedHandler) trait that is meant to be implemented by a type that:
>> 
>> a) provides a function to be run by the system when the IRQ fires and,
>> 
>> b) holds the shared data (i.e.: `T`) between process and IRQ contexts.
>> 
>> The requirement that T is Sync derives from the fact that handlers might run
>> concurrently with other processes executing the same driver, creating the
>> potential for data races.
>> 
>> Ideally, some interior mutability must be in place if T is to be mutated. This
>> should usually be done through the in-flight SpinLockIrq type.
>> 
>> Co-developed-by: Alice Ryhl <aliceryhl@google.com>
>> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
>> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
> 
> What is the base commit to apply this patch to? I am getting some
> compiler errors when trying it out:
> 
>   error[E0308]: mismatched types
>     --> /home/aeh/src/linux-rust/linux/rust/kernel/irq/request.rs:240:21
>       |
>   237 |                 bindings::request_irq(
>       |                 --------------------- arguments to this function are incorrect
>   ...
>   240 |                     flags.0,
>       |                     ^^^^^^^ expected `usize`, found `u64`
>       |
> 
> [...]

This patch needs a rebase. I’ve been waiting for Lyude & Boqun
new SpinLockIrq series, which just came out last week.

I’ll work on a v3 soon.

Re: [PATCH v2] rust: irq: add support for request_irq()

[Alice Ryhl]

On Tue, Mar 04, 2025 at 02:43:20PM +0100, Andreas Hindborg wrote:
> "Daniel Almeida" <daniel.almeida@collabora.com> writes:
> > +    /// handler after suspending interrupts. For system wakeup devices users
> > +    /// need to implement wakeup detection in their interrupt handlers.
> > +    pub const COND_SUSPEND: Flags = Flags(bindings::IRQF_COND_SUSPEND as _);
> > +
> > +    /// Don't enable IRQ or NMI automatically when users request it. Users will
> > +    /// enable it explicitly by `enable_irq` or `enable_nmi` later.
> > +    pub const NO_AUTOEN: Flags = Flags(bindings::IRQF_NO_AUTOEN as _);
> > +
> > +    /// Exclude from runnaway detection for IPI and similar handlers, depends on
> > +    /// `PERCPU`.
> > +    pub const NO_DEBUG: Flags = Flags(bindings::IRQF_NO_DEBUG as _);
> > +}
> > +
> > +/// The value that can be returned from an IrqHandler or a ThreadedIrqHandler.
> > +pub enum IrqReturn {
> 
> I learned recently that if you choose the right representation here, you
> don't need to cast here and when you call `Handler::handle_irq`. I think
> `#[repr(u32)]` is the one to use here.

I wonder if we can get it to use the repr of the same size as
irqreturn_t?

> > +    /// The interrupt was not from this device or was not handled.
> > +    None = bindings::irqreturn_IRQ_NONE as _,
> > +
> > +    /// The interrupt was handled by this device.
> > +    Handled = bindings::irqreturn_IRQ_HANDLED as _,
> > +}
> > +
> > +/// Callbacks for an IRQ handler.
> > +pub trait Handler: Sync {
> > +    /// The actual handler function. As usual, sleeps are not allowed in IRQ
> > +    /// context.
> > +    fn handle_irq(&self) -> IrqReturn;
> > +}
> 
> What is the reason for moving away from the following:
> 
> 
>     pub trait Handler {
>         /// The context data associated with and made available to the handler.
>         type Data: ForeignOwnable;
> 
>         /// Called from interrupt context when the irq happens.
>         fn handle_irq(data: <Self::Data as ForeignOwnable>::Borrowed<'_>) -> Return;
>     }
> 
> 
> I think we will run into problems if we want to pass `Arc<Foo>` as the
> handler. I don't think we can `impl Handler for Arc<Foo>` in a driver
> crate, since both `Handler` and `Arc` are defined in external crates

My understanding is that since the data is not stored behind a
private_data void pointer, we don't need ForeignOwnable. I think we
should avoid using ForeignOwnable when it's not necessary.

We can support the Arc / Box case by adding

impl<T: ?Sized + Handler> Handler for Arc<T> {
    fn handle_irq(&self) -> IrqReturn {
        T::handle_irq(self)
    }
}

This way, the user implements it for their struct and then it works with
Arc<MyStruct> too.

This kind of blanket impl for Arc/Box is very common in userspace Rust
too. For example, the Tokio traits AsyncRead/AsyncWrite have blanket
impls for Box.

> > +#[pin_data(PinnedDrop)]
> > +pub struct ThreadedRegistration<T: ThreadedHandler> {
> > +    irq: u32,
> > +    #[pin]
> > +    handler: T,
> > +    #[pin]
> > +    /// Pinned because we need address stability so that we can pass a pointer
> > +    /// to the callback.
> > +    _pin: PhantomPinned,
> > +}
> 
> As others have mentioned, I wonder if we can avoid the code duplication
> that makes up most of the rest of this patch.

I'm worried that getting rid of duplication makes the code too complex
in this case. I could be wrong, but it seems difficult to deduplicate in
a simple way.

Alice

Re: [PATCH v2] rust: irq: add support for request_irq()

[Daniel Almeida]

Hi Andreas,

I am going to work on a new iteration of this patch. Are you satisfied with the
answers Alice gave you w.r.t ForeignOwnable? My take is the same as hers: we
don't need it here. The code you linked to is more restrictive in the sense
that it requires an allocation, which is not the case currently.

Her solution will cater to the use case you've described, so this should be OK
on your end?

> On 4 Mar 2025, at 10:43, Andreas Hindborg <a.hindborg@kernel.org> wrote:
> 
> "Daniel Almeida" <daniel.almeida@collabora.com> writes:
> 
>> Add support for registering IRQ handlers in Rust.
>> 
>> IRQ handlers are extensively used in drivers when some peripheral wants to
>> obtain the CPU attention. Registering a handler will make the system invoke the
>> passed-in function whenever the chosen IRQ line is triggered.
>> 
>> Both regular and threaded IRQ handlers are supported through a Handler (or
>> ThreadedHandler) trait that is meant to be implemented by a type that:
>> 
>> a) provides a function to be run by the system when the IRQ fires and,
>> 
>> b) holds the shared data (i.e.: `T`) between process and IRQ contexts.
>> 
>> The requirement that T is Sync derives from the fact that handlers might run
>> concurrently with other processes executing the same driver, creating the
>> potential for data races.
>> 
>> Ideally, some interior mutability must be in place if T is to be mutated. This
>> should usually be done through the in-flight SpinLockIrq type.
>> 
>> Co-developed-by: Alice Ryhl <aliceryhl@google.com>
>> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
>> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
> 
> What is the base commit to apply this patch to? I am getting some
> compiler errors when trying it out:
> 
>    error[E0308]: mismatched types
>      --> /home/aeh/src/linux-rust/linux/rust/kernel/irq/request.rs:240:21
>        |
>    237 |                 bindings::request_irq(
>        |                 --------------------- arguments to this function are incorrect
>    ...
>    240 |                     flags.0,
>        |                     ^^^^^^^ expected `usize`, found `u64`
>        |
> 
> [...]

I will rebase on top of the newest rc for 6.15

> 
>> diff --git a/rust/kernel/irq.rs b/rust/kernel/irq.rs
>> new file mode 100644
>> index 000000000000..3ab83c5bdb83
>> --- /dev/null
>> +++ b/rust/kernel/irq.rs
>> @@ -0,0 +1,6 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +//! IRQ abstractions
> 
> We could do with a longer story here. Also, missing a period.
> 
>> +
>> +/// IRQ allocation and handling
>> +pub mod request;
>> diff --git a/rust/kernel/irq/request.rs b/rust/kernel/irq/request.rs
>> new file mode 100644
>> index 000000000000..61e7d4a8f555
>> --- /dev/null
>> +++ b/rust/kernel/irq/request.rs
>> @@ -0,0 +1,517 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// SPDX-FileCopyrightText: Copyright 2025 Collabora ltd.
>> +
>> +//! IRQ allocation and handling
>> +
>> +use core::marker::PhantomPinned;
>> +use core::ptr::addr_of_mut;
>> +
>> +use init::pin_init_from_closure;
>> +
>> +use crate::error::to_result;
>> +use crate::prelude::*;
>> +use crate::str::CStr;
>> +
>> +/// Flags to be used when registering IRQ handlers.
>> +///
>> +/// They can be combined with the operators `|`, `&`, and `!`.
>> +///
>> +/// Values can be used from the [`flags`] module.
>> +#[derive(Clone, Copy)]
>> +pub struct Flags(u64);
>> +
>> +impl core::ops::BitOr for Flags {
>> +    type Output = Self;
>> +    fn bitor(self, rhs: Self) -> Self::Output {
>> +        Self(self.0 | rhs.0)
>> +    }
>> +}
>> +
>> +impl core::ops::BitAnd for Flags {
>> +    type Output = Self;
>> +    fn bitand(self, rhs: Self) -> Self::Output {
>> +        Self(self.0 & rhs.0)
>> +    }
>> +}
>> +
>> +impl core::ops::Not for Flags {
>> +    type Output = Self;
>> +    fn not(self) -> Self::Output {
>> +        Self(!self.0)
>> +    }
>> +}
>> +
>> +/// The flags that can be used when registering an IRQ handler.
>> +pub mod flags {
>> +    use super::Flags;
>> +
>> +    use crate::bindings;
>> +
>> +    /// Use the interrupt line as already configured.
>> +    pub const TRIGGER_NONE: Flags = Flags(bindings::IRQF_TRIGGER_NONE as _);
> 
> What is the reason for the `as _` in all these? Should the flag type be
> something else?

I think this was before Gary’s patch to improve the mapping of FFI types in bindgen.

> 
>> +
>> +    /// The interrupt is triggered when the signal goes from low to high.
>> +    pub const TRIGGER_RISING: Flags = Flags(bindings::IRQF_TRIGGER_RISING as _);
>> +
>> +    /// The interrupt is triggered when the signal goes from high to low.
>> +    pub const TRIGGER_FALLING: Flags = Flags(bindings::IRQF_TRIGGER_FALLING as _);
>> +
>> +    /// The interrupt is triggered while the signal is held high.
>> +    pub const TRIGGER_HIGH: Flags = Flags(bindings::IRQF_TRIGGER_HIGH as _);
>> +
>> +    /// The interrupt is triggered while the signal is held low.
>> +    pub const TRIGGER_LOW: Flags = Flags(bindings::IRQF_TRIGGER_LOW as _);
>> +
>> +    /// Allow sharing the irq among several devices.
>> +    pub const SHARED: Flags = Flags(bindings::IRQF_SHARED as _);
>> +
>> +    /// Set by callers when they expect sharing mismatches to occur.
> 
> What is a sharing mismatch?
> 
>> +    pub const PROBE_SHARED: Flags = Flags(bindings::IRQF_PROBE_SHARED as _);
>> +
>> +    /// Flag to mark this interrupt as timer interrupt.
> 
> The "Flag to ..." strikes me as odd when most of the other descriptions
> have a different wording.
> 
>> +    pub const TIMER: Flags = Flags(bindings::IRQF_TIMER as _);
>> +
>> +    /// Interrupt is per cpu.
>> +    pub const PERCPU: Flags = Flags(bindings::IRQF_PERCPU as _);
>> +
>> +    /// Flag to exclude this interrupt from irq balancing.
>> +    pub const NOBALANCING: Flags = Flags(bindings::IRQF_NOBALANCING as _);
>> +
>> +    /// Interrupt is used for polling (only the interrupt that is registered
>> +    /// first in a shared interrupt is considered for performance reasons).
>> +    pub const IRQPOLL: Flags = Flags(bindings::IRQF_IRQPOLL as _);
>> +
>> +    /// Interrupt is not reenabled after the hardirq handler finished. Used by
>> +    /// threaded interrupts which need to keep the irq line disabled until the
>> +    /// threaded handler has been run.
>> +    pub const ONESHOT: Flags = Flags(bindings::IRQF_ONESHOT as _);
>> +
>> +    /// Do not disable this IRQ during suspend. Does not guarantee that this
>> +    /// interrupt will wake the system from a suspended state.
>> +    pub const NO_SUSPEND: Flags = Flags(bindings::IRQF_NO_SUSPEND as _);
>> +
>> +    /// Force enable it on resume even if [`NO_SUSPEND`] is set.
> 
> Perhaps: Force enable the interrupt even if ...


All your comments above are addressing the documentation that was deliberately
copied and pasted from C. IMHO, we should refrain from introducing our own
documentation in this case because it will just confuse users. Any changes here
should be proposed to the C side directly.

> 
>> +    pub const FORCE_RESUME: Flags = Flags(bindings::IRQF_FORCE_RESUME as _);
>> +
>> +    /// Interrupt cannot be threaded.
>> +    pub const NO_THREAD: Flags = Flags(bindings::IRQF_NO_THREAD as _);
>> +
>> +    /// Resume IRQ early during syscore instead of at device resume time.
>> +    pub const EARLY_RESUME: Flags = Flags(bindings::IRQF_EARLY_RESUME as _);
>> +
>> +    /// If the IRQ is shared with a NO_SUSPEND user, execute this interrupt
> 
> Please link `NO_SUSPEND`.

Sure

> 
>> +    /// handler after suspending interrupts. For system wakeup devices users
>> +    /// need to implement wakeup detection in their interrupt handlers.
>> +    pub const COND_SUSPEND: Flags = Flags(bindings::IRQF_COND_SUSPEND as _);
>> +
>> +    /// Don't enable IRQ or NMI automatically when users request it. Users will
>> +    /// enable it explicitly by `enable_irq` or `enable_nmi` later.
>> +    pub const NO_AUTOEN: Flags = Flags(bindings::IRQF_NO_AUTOEN as _);
>> +
>> +    /// Exclude from runnaway detection for IPI and similar handlers, depends on
>> +    /// `PERCPU`.
>> +    pub const NO_DEBUG: Flags = Flags(bindings::IRQF_NO_DEBUG as _);
>> +}
>> +
>> +/// The value that can be returned from an IrqHandler or a ThreadedIrqHandler.
>> +pub enum IrqReturn {
> 
> I learned recently that if you choose the right representation here, you
> don't need to cast here and when you call `Handler::handle_irq`. I think
> `#[repr(u32)]` is the one to use here.

Thanks!

> 
>> +    /// The interrupt was not from this device or was not handled.
>> +    None = bindings::irqreturn_IRQ_NONE as _,
>> +
>> +    /// The interrupt was handled by this device.
>> +    Handled = bindings::irqreturn_IRQ_HANDLED as _,
>> +}
>> +
>> +/// Callbacks for an IRQ handler.
>> +pub trait Handler: Sync {
>> +    /// The actual handler function. As usual, sleeps are not allowed in IRQ
>> +    /// context.
>> +    fn handle_irq(&self) -> IrqReturn;
>> +}
> 
> What is the reason for moving away from the following:
> 
> 
>    pub trait Handler {
>        /// The context data associated with and made available to the handler.
>        type Data: ForeignOwnable;
> 
>        /// Called from interrupt context when the irq happens.
>        fn handle_irq(data: <Self::Data as ForeignOwnable>::Borrowed<'_>) -> Return;
>    }
> 
> 
> I think we will run into problems if we want to pass `Arc<Foo>` as the
> handler. I don't think we can `impl Handler for Arc<Foo>` in a driver
> crate, since both `Handler` and `Arc` are defined in external crates
> 
> 
> [...]
> 
>> +#[pin_data(PinnedDrop)]
>> +pub struct ThreadedRegistration<T: ThreadedHandler> {
>> +    irq: u32,
>> +    #[pin]
>> +    handler: T,
>> +    #[pin]
>> +    /// Pinned because we need address stability so that we can pass a pointer
>> +    /// to the callback.
>> +    _pin: PhantomPinned,
>> +}
> 
> As others have mentioned, I wonder if we can avoid the code duplication
> that makes up most of the rest of this patch.
> 
> 
> Best regards,
> Andreas Hindborg

— Daniel

Re: [PATCH v2] rust: irq: add support for request_irq()

[Daniel Almeida]

Hi Alice,

> On 23 Jan 2025, at 06:07, Alice Ryhl <aliceryhl@google.com> wrote:
> 
> On Thu, Jan 23, 2025 at 8:18 AM Boqun Feng <boqun.feng@gmail.com> wrote:
>> 
>> On Wed, Jan 22, 2025 at 01:39:30PM -0300, Daniel Almeida wrote:
>>> Add support for registering IRQ handlers in Rust.
>>> 
>>> IRQ handlers are extensively used in drivers when some peripheral wants to
>>> obtain the CPU attention. Registering a handler will make the system invoke the
>>> passed-in function whenever the chosen IRQ line is triggered.
>>> 
>>> Both regular and threaded IRQ handlers are supported through a Handler (or
>>> ThreadedHandler) trait that is meant to be implemented by a type that:
>>> 
>>> a) provides a function to be run by the system when the IRQ fires and,
>>> 
>>> b) holds the shared data (i.e.: `T`) between process and IRQ contexts.
>>> 
>>> The requirement that T is Sync derives from the fact that handlers might run
>>> concurrently with other processes executing the same driver, creating the
>>> potential for data races.
>>> 
>>> Ideally, some interior mutability must be in place if T is to be mutated. This
>>> should usually be done through the in-flight SpinLockIrq type.
>>> 
>>> Co-developed-by: Alice Ryhl <aliceryhl@google.com>
>>> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
>>> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
>>> ---
>>> 
>>> Changes from v1:
>>> 
>>> - Added Co-developed-by tag to account for the work that Alice did in order to
>>> figure out how to do this without Opaque<T> (Thanks!)
>>> - Removed Opaque<T> in favor of plain T
>> 
>> Hmmm...
>> 
>> [...]
>> 
>>> +#[pin_data(PinnedDrop)]
>>> +pub struct Registration<T: Handler> {
>>> +    irq: u32,
>>> +    #[pin]
>>> +    handler: T,
>> 
>> I think you still need to make `handler` as `!Unpin` because compilers
>> can assume a `&mut T` from a `Pin<&mut Registration>`, am I missing
>> something here?
> 
> The current version operates under the assumption that PhantomPinned
> is enough. But I'm happy to add Aliased here.
> 
> Alice

Aliased? What is this? I can’t find that trait or type anywhere.

— Daniel

Re: [PATCH v2] rust: irq: add support for request_irq()

[Christian Schrefl]

Hi Daniel

On 14.05.25 4:44 PM, Daniel Almeida wrote:
> Hi Alice,
> 
>> On 23 Jan 2025, at 06:07, Alice Ryhl <aliceryhl@google.com> wrote:
>>
>> On Thu, Jan 23, 2025 at 8:18 AM Boqun Feng <boqun.feng@gmail.com> wrote:
>>>
>>> On Wed, Jan 22, 2025 at 01:39:30PM -0300, Daniel Almeida wrote:
>>>> Add support for registering IRQ handlers in Rust.
>>>>
>>>> IRQ handlers are extensively used in drivers when some peripheral wants to
>>>> obtain the CPU attention. Registering a handler will make the system invoke the
>>>> passed-in function whenever the chosen IRQ line is triggered.
>>>>
>>>> Both regular and threaded IRQ handlers are supported through a Handler (or
>>>> ThreadedHandler) trait that is meant to be implemented by a type that:
>>>>
>>>> a) provides a function to be run by the system when the IRQ fires and,
>>>>
>>>> b) holds the shared data (i.e.: `T`) between process and IRQ contexts.
>>>>
>>>> The requirement that T is Sync derives from the fact that handlers might run
>>>> concurrently with other processes executing the same driver, creating the
>>>> potential for data races.
>>>>
>>>> Ideally, some interior mutability must be in place if T is to be mutated. This
>>>> should usually be done through the in-flight SpinLockIrq type.
>>>>
>>>> Co-developed-by: Alice Ryhl <aliceryhl@google.com>
>>>> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
>>>> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
>>>> ---
>>>>
>>>> Changes from v1:
>>>>
>>>> - Added Co-developed-by tag to account for the work that Alice did in order to
>>>> figure out how to do this without Opaque<T> (Thanks!)
>>>> - Removed Opaque<T> in favor of plain T
>>>
>>> Hmmm...
>>>
>>> [...]
>>>
>>>> +#[pin_data(PinnedDrop)]
>>>> +pub struct Registration<T: Handler> {
>>>> +    irq: u32,
>>>> +    #[pin]
>>>> +    handler: T,
>>>
>>> I think you still need to make `handler` as `!Unpin` because compilers
>>> can assume a `&mut T` from a `Pin<&mut Registration>`, am I missing
>>> something here?
>>
>> The current version operates under the assumption that PhantomPinned
>> is enough. But I'm happy to add Aliased here.
>>
>> Alice
> 
> Aliased? What is this? I can’t find that trait or type anywhere.

Aliased was a name previously considered for `UnsafePinned` [0].
I believe that was what Alice referred to here.

Just marking the type as !Unpin should be fine on current
Rust versions, but on some future rust versions `UnsafePinned`
might be the only way to make this sound.

[0]: https://lore.kernel.org/rust-for-linux/D9VBVURZLSNT.4BTQQ8UCTGPJ@kernel.org/

Cheers
Christian