Re: [PATCH v6 3/6] rust: irq: add support for non-threaded IRQs and handlers

Dirk Behme
2025-07-08T12:15:45Z

Hi Daniel,

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

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

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

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

Best regards

Dirk