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

[Daniel Almeida]

> On 22 Jan 2025, at 13:39, Daniel Almeida <daniel.almeida@xxxxxxxxxxxxx> 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@xxxxxxxxxx>
> Signed-off-by: Alice Ryhl <aliceryhl@xxxxxxxxxx>
> Signed-off-by: Daniel Almeida <daniel.almeida@xxxxxxxxxxxxx>
> ---
> 
> 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@xxxxxxxxxxxxx/
> 
> 
> ---
> 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