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.

Tested-by: Joel Fernandes <joelagnelf@nvidia.com>
Tested-by: Dirk Behme <dirk.behme@de.bosch.com>
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
---
 rust/bindings/bindings_helper.h |   1 +
 rust/helpers/helpers.c          |   1 +
 rust/helpers/irq.c              |   9 ++
 rust/kernel/irq.rs              |   5 +
 rust/kernel/irq/request.rs      | 264 ++++++++++++++++++++++++++++++++++++++++
 5 files changed, 280 insertions(+)

diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 84d60635e8a9baef1f1a1b2752dc0fa044f8542f..69a975da829f0c35760f71a1b32b8fcb12c8a8dc 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -52,6 +52,7 @@
 #include <linux/ethtool.h>
 #include <linux/file.h>
 #include <linux/firmware.h>
+#include <linux/interrupt.h>
 #include <linux/fs.h>
 #include <linux/ioport.h>
 #include <linux/jiffies.h>
diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
index 7cf7fe95e41dd51717050648d6160bebebdf4b26..44b2005d50140d34a44ae37d01c2ddbae6aeaa32 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c
@@ -22,6 +22,7 @@
 #include "dma.c"
 #include "drm.c"
 #include "err.c"
+#include "irq.c"
 #include "fs.c"
 #include "io.c"
 #include "jump_label.c"
diff --git a/rust/helpers/irq.c b/rust/helpers/irq.c
new file mode 100644
index 0000000000000000000000000000000000000000..1faca428e2c047a656dec3171855c1508d67e60b
--- /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
index 068df2fea31de51115c30344f7ebdb4da4ad86cc..c1019bc36ad1e7ae7dd3af8a8b5c14780bf70712 100644
--- a/rust/kernel/irq.rs
+++ b/rust/kernel/irq.rs
@@ -13,4 +13,9 @@
 /// Flags to be used when registering IRQ handlers.
 mod flags;
 
+/// IRQ allocation and handling.
+mod request;
+
 pub use flags::Flags;
+
+pub use request::{Handler, IrqRequest, IrqReturn, Registration};
diff --git a/rust/kernel/irq/request.rs b/rust/kernel/irq/request.rs
new file mode 100644
index 0000000000000000000000000000000000000000..57e00ebf694d8e6e870d9ed57af7ee2ecf86ec05
--- /dev/null
+++ b/rust/kernel/irq/request.rs
@@ -0,0 +1,264 @@
+// 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, 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 a [`Handler`] or a [`ThreadedHandler`].
+#[repr(u32)]
+pub enum IrqReturn {
+    /// The interrupt was not from this device or was not handled.
+    None = bindings::irqreturn_IRQ_NONE,
+
+    /// The interrupt was handled by this device.
+    Handled = bindings::irqreturn_IRQ_HANDLED,
+}
+
+/// 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 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 {
+        // INVARIANT: `irq` is a valid IRQ number for `dev`.
+        IrqRequest { dev, irq }
+    }
+
+    /// Returns the IRQ number of an [`IrqRequest`].
+    pub fn irq(&self) -> u32 {
+        self.irq
+    }
+}
+
+/// A registration of an IRQ handler for a given IRQ line.
+///
+/// # Examples
+///
+/// The following is an example of using `Registration`. It uses a
+/// [`Completion`] to coordinate between the IRQ
+/// handler and process context. [`Completion`] uses interior mutability, so the
+/// handler can signal with [`Completion::complete_all()`] and the process
+/// context can wait with [`Completion::wait_for_completion()`] even though
+/// there is no way to get a mutable reference to the any of the fields in
+/// `Data`.
+///
+/// [`Completion`]: kernel::sync::Completion
+/// [`Completion::complete_all()`]: kernel::sync::Completion::complete_all
+/// [`Completion::wait_for_completion()`]: kernel::sync::Completion::wait_for_completion
+///
+/// ```
+/// use kernel::c_str;
+/// use kernel::device::Bound;
+/// use kernel::irq::{self, Flags, IrqRequest, IrqReturn, Registration};
+/// use kernel::prelude::*;
+/// use kernel::sync::{Arc, Completion};
+///
+/// // Data shared between process and IRQ context.
+/// #[pin_data]
+/// struct Data {
+///     #[pin]
+///     completion: Completion,
+/// }
+///
+/// impl irq::Handler for Data {
+///     // Executed in IRQ context.
+///     fn handle(&self) -> IrqReturn {
+///         self.completion.complete_all();
+///         IrqReturn::Handled
+///     }
+/// }
+///
+/// // Registers an IRQ handler for the given IrqRequest.
+/// //
+/// // This runs in process context and assumes `request` was previously acquired from a device.
+/// fn register_irq(
+///     handler: impl PinInit<Data, Error>,
+///     request: IrqRequest<'_>,
+/// ) -> Result<Arc<Registration<Data>>> {
+///     let registration = Registration::new(request, Flags::SHARED, c_str!("my_device"), handler);
+///
+///     let registration = Arc::pin_init(registration, GFP_KERNEL)?;
+///
+///     registration.handler().completion.wait_for_completion();
+///
+///     Ok(registration)
+/// }
+/// # Ok::<(), Error>(())
+/// ```
+///
+/// # Invariants
+///
+/// * We own an irq handler using `&self.handler` as its private data.
+#[pin_data]
+pub struct Registration<T: Handler + 'static> {
+    #[pin]
+    inner: Devres<RegistrationInner>,
+
+    #[pin]
+    handler: T,
+
+    /// Pinned because we need address stability so that we can pass a pointer
+    /// to the callback.
+    #[pin]
+    _pin: PhantomPinned,
+}
+
+impl<T: Handler + 'static> Registration<T> {
+    /// Registers the IRQ handler with the system for the given IRQ number.
+    pub fn new<'a>(
+        request: IrqRequest<'a>,
+        flags: Flags,
+        name: &'static CStr,
+        handler: impl PinInit<T, Error> + 'a,
+    ) -> impl PinInit<Self, Error> + 'a {
+        try_pin_init!(&this in Self {
+            handler <- handler,
+            inner <- Devres::new(
+                request.dev,
+                try_pin_init!(RegistrationInner {
+                    // SAFETY: `this` is a valid pointer to the `Registration` instance
+                    cookie: unsafe { &raw mut (*this.as_ptr()).handler }.cast(),
+                    irq: {
+                        // SAFETY:
+                        // - The callbacks are valid for use with request_irq.
+                        // - If this succeeds, the slot is guaranteed to be valid until the
+                        //   destructor of Self runs, which will deregister the callbacks
+                        //   before the memory location becomes invalid.
+                        to_result(unsafe {
+                            bindings::request_irq(
+                                request.irq,
+                                Some(handle_irq_callback::<T>),
+                                flags.into_inner(),
+                                name.as_char_ptr(),
+                                (&raw mut (*this.as_ptr()).handler).cast(),
+                            )
+                        })?;
+                        request.irq
+                    }
+                })
+            ),
+            _pin: PhantomPinned,
+        })
+    }
+
+    /// Returns a reference to the handler that was registered with the system.
+    pub fn handler(&self) -> &T {
+        &self.handler
+    }
+
+    /// Wait for pending IRQ handlers on other CPUs.
+    ///
+    /// This will attempt to access the inner [`Devres`] container.
+    pub fn try_synchronize(&self) -> Result {
+        let inner = self.inner.try_access().ok_or(ENODEV)?;
+        inner.synchronize();
+        Ok(())
+    }
+
+    /// Wait for pending IRQ handlers on other CPUs.
+    pub fn synchronize(&self, dev: &Device<Bound>) -> Result {
+        let inner = self.inner.access(dev)?;
+        inner.synchronize();
+        Ok(())
+    }
+}
+
+/// # 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 c_void) -> c_uint {
+    // SAFETY: `ptr` is a pointer to T set in `Registration::new`
+    let handler = unsafe { &*(ptr as *const T) };
+    T::handle(handler) as c_uint
+}

-- 
2.50.1

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

> 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.
>
> Tested-by: Joel Fernandes <joelagnelf@nvidia.com>
> Tested-by: Dirk Behme <dirk.behme@de.bosch.com>
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
> ---
>  rust/bindings/bindings_helper.h |   1 +
>  rust/helpers/helpers.c          |   1 +
>  rust/helpers/irq.c              |   9 ++
>  rust/kernel/irq.rs              |   5 +
>  rust/kernel/irq/request.rs      | 264 ++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 280 insertions(+)
>
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index 84d60635e8a9baef1f1a1b2752dc0fa044f8542f..69a975da829f0c35760f71a1b32b8fcb12c8a8dc 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -52,6 +52,7 @@
>  #include <linux/ethtool.h>
>  #include <linux/file.h>
>  #include <linux/firmware.h>
> +#include <linux/interrupt.h>
>  #include <linux/fs.h>
>  #include <linux/ioport.h>
>  #include <linux/jiffies.h>
> diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
> index 7cf7fe95e41dd51717050648d6160bebebdf4b26..44b2005d50140d34a44ae37d01c2ddbae6aeaa32 100644
> --- a/rust/helpers/helpers.c
> +++ b/rust/helpers/helpers.c
> @@ -22,6 +22,7 @@
>  #include "dma.c"
>  #include "drm.c"
>  #include "err.c"
> +#include "irq.c"
>  #include "fs.c"
>  #include "io.c"
>  #include "jump_label.c"
> diff --git a/rust/helpers/irq.c b/rust/helpers/irq.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..1faca428e2c047a656dec3171855c1508d67e60b
> --- /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
> index 068df2fea31de51115c30344f7ebdb4da4ad86cc..c1019bc36ad1e7ae7dd3af8a8b5c14780bf70712 100644
> --- a/rust/kernel/irq.rs
> +++ b/rust/kernel/irq.rs
> @@ -13,4 +13,9 @@
>  /// Flags to be used when registering IRQ handlers.
>  mod flags;
>
> +/// IRQ allocation and handling.
> +mod request;
> +
>  pub use flags::Flags;
> +
> +pub use request::{Handler, IrqRequest, IrqReturn, Registration};
> diff --git a/rust/kernel/irq/request.rs b/rust/kernel/irq/request.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..57e00ebf694d8e6e870d9ed57af7ee2ecf86ec05
> --- /dev/null
> +++ b/rust/kernel/irq/request.rs
> @@ -0,0 +1,264 @@
> +// 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, Device};

nit: I would suggest either normalizing all the imports, or using one
import per line consistently.

> +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 a [`Handler`] or a [`ThreadedHandler`].

error: unresolved link to `ThreadedHandler`
  --> /home/aeh/src/linux-rust/request-irq/rust/kernel/irq/request.rs:18:62
   |
18 | /// The value that can be returned from a [`Handler`] or a [`ThreadedHandler`].
   |                                                              ^^^^^^^^^^^^^^^ no item named `ThreadedHandler` in scope
   |

> +#[repr(u32)]
> +pub enum IrqReturn {
> +    /// The interrupt was not from this device or was not handled.
> +    None = bindings::irqreturn_IRQ_NONE,
> +
> +    /// The interrupt was handled by this device.
> +    Handled = bindings::irqreturn_IRQ_HANDLED,
> +}
> +
> +/// Callbacks for an IRQ handler.
> +pub trait Handler: Sync {
> +    /// The hard IRQ handler.

Could you do a vocabulary somewhere? What does it mean that the handler
is hard?

> +    ///
> +    /// 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`].

error: unresolved link to `ThreadedRegistration`
  --> /home/aeh/src/linux-rust/request-irq/rust/kernel/irq/request.rs:37:20
   |
37 |     /// See also [`ThreadedRegistration`].
   |                    ^^^^^^^^^^^^^^^^^^^^ no item named `ThreadedRegistration` in scope
   |

> +    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`.

This seems like a mix of invariant declaration and conformance. I don't
think the following belongs here:

  It is guaranteed to be unique
  by the type system, since each call to `new` will return a different instance of
  `Registration`.

You could replace it with a uniqueness requirement.

> +#[pin_data(PinnedDrop)]
> +struct RegistrationInner {
> +    irq: u32,
> +    cookie: *mut 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.

Why?

> +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 {

This needs `#[expect(dead_code)]`.

> +        // INVARIANT: `irq` is a valid IRQ number for `dev`.

I would suggest rephrasing:

  By function safety requirement, irq` is a valid IRQ number for `dev`.

> +        IrqRequest { dev, irq }
> +    }
> +
> +    /// Returns the IRQ number of an [`IrqRequest`].
> +    pub fn irq(&self) -> u32 {
> +        self.irq
> +    }
> +}
> +
> +/// A registration of an IRQ handler for a given IRQ line.
> +///
> +/// # Examples
> +///
> +/// The following is an example of using `Registration`. It uses a
> +/// [`Completion`] to coordinate between the IRQ
> +/// handler and process context. [`Completion`] uses interior mutability, so the
> +/// handler can signal with [`Completion::complete_all()`] and the process
> +/// context can wait with [`Completion::wait_for_completion()`] even though
> +/// there is no way to get a mutable reference to the any of the fields in
> +/// `Data`.
> +///
> +/// [`Completion`]: kernel::sync::Completion
> +/// [`Completion::complete_all()`]: kernel::sync::Completion::complete_all
> +/// [`Completion::wait_for_completion()`]: kernel::sync::Completion::wait_for_completion
> +///
> +/// ```
> +/// use kernel::c_str;
> +/// use kernel::device::Bound;
> +/// use kernel::irq::{self, Flags, IrqRequest, IrqReturn, Registration};
> +/// use kernel::prelude::*;
> +/// use kernel::sync::{Arc, Completion};
> +///
> +/// // Data shared between process and IRQ context.
> +/// #[pin_data]
> +/// struct Data {
> +///     #[pin]
> +///     completion: Completion,
> +/// }
> +///
> +/// impl irq::Handler for Data {
> +///     // Executed in IRQ context.
> +///     fn handle(&self) -> IrqReturn {
> +///         self.completion.complete_all();
> +///         IrqReturn::Handled
> +///     }
> +/// }
> +///
> +/// // Registers an IRQ handler for the given IrqRequest.
> +/// //
> +/// // This runs in process context and assumes `request` was previously acquired from a device.
> +/// fn register_irq(
> +///     handler: impl PinInit<Data, Error>,
> +///     request: IrqRequest<'_>,
> +/// ) -> Result<Arc<Registration<Data>>> {
> +///     let registration = Registration::new(request, Flags::SHARED, c_str!("my_device"), handler);
> +///
> +///     let registration = Arc::pin_init(registration, GFP_KERNEL)?;
> +///
> +///     registration.handler().completion.wait_for_completion();
> +///
> +///     Ok(registration)
> +/// }
> +/// # Ok::<(), Error>(())
> +/// ```
> +///
> +/// # Invariants
> +///
> +/// * We own an irq handler using `&self.handler` as its private data.
> +#[pin_data]
> +pub struct Registration<T: Handler + 'static> {
> +    #[pin]
> +    inner: Devres<RegistrationInner>,

Soundness of this API requires `inner` to be dropped before `handler`.
Maybe we should have a comment specifying that the order of these fields
is important?

> +
> +    #[pin]
> +    handler: T,
> +
> +    /// Pinned because we need address stability so that we can pass a pointer
> +    /// to the callback.
> +    #[pin]
> +    _pin: PhantomPinned,
> +}
> +
> +impl<T: Handler + 'static> Registration<T> {
> +    /// Registers the IRQ handler with the system for the given IRQ number.

Could this be "... for the IRQ number represented by `request`."? Because we
don't pass in an actual number here.

> +    pub fn new<'a>(
> +        request: IrqRequest<'a>,
> +        flags: Flags,
> +        name: &'static CStr,
> +        handler: impl PinInit<T, Error> + 'a,
> +    ) -> impl PinInit<Self, Error> + 'a {
> +        try_pin_init!(&this in Self {
> +            handler <- handler,
> +            inner <- Devres::new(
> +                request.dev,
> +                try_pin_init!(RegistrationInner {
> +                    // SAFETY: `this` is a valid pointer to the `Registration` instance
> +                    cookie: unsafe { &raw mut (*this.as_ptr()).handler }cast(),
> +                    irq: {
> +                        // SAFETY:
> +                        // - The callbacks are valid for use with request_irq.
> +                        // - If this succeeds, the slot is guaranteed to be valid until the
> +                        //   destructor of Self runs, which will deregister the callbacks
> +                        //   before the memory location becomes invalid.
> +                        to_result(unsafe {
> +                            bindings::request_irq(
> +                                request.irq,
> +                                Some(handle_irq_callback::<T>),
> +                                flags.into_inner(),
> +                                name.as_char_ptr(),
> +                                (&raw mut (*this.as_ptr()).handler).cast(),
> +                            )
> +                        })?;
> +                        request.irq
> +                    }
> +                })
> +            ),
> +            _pin: PhantomPinned,
> +        })
> +    }
> +
> +    /// Returns a reference to the handler that was registered with the system.
> +    pub fn handler(&self) -> &T {
> +        &self.handler
> +    }
> +
> +    /// Wait for pending IRQ handlers on other CPUs.
> +    ///
> +    /// This will attempt to access the inner [`Devres`] container.
> +    pub fn try_synchronize(&self) -> Result {
> +        let inner = self.inner.try_access().ok_or(ENODEV)?;
> +        inner.synchronize();
> +        Ok(())
> +    }
> +
> +    /// Wait for pending IRQ handlers on other CPUs.
> +    pub fn synchronize(&self, dev: &Device<Bound>) -> Result {
> +        let inner = self.inner.access(dev)?;
> +        inner.synchronize();
> +        Ok(())
> +    }
> +}
> +
> +/// # Safety
> +///
> +/// This function should be only used as the callback in `request_irq`.

I think this safety requirement is inadequate. We must require `ptr` to
be valid for use as a reference to `T`. When we install the pointer to
this function, we should certify why safety requirements are fulfilled
when C calls through the pointer.

> +unsafe extern "C" fn handle_irq_callback<T: Handler>(_irq: i32, ptr: *mut c_void) -> c_uint {
> +    // SAFETY: `ptr` is a pointer to T set in `Registration::new`
> +    let handler = unsafe { &*(ptr as *const T) };
> +    T::handle(handler) as c_uint
> +}


Best regards,
Andreas Hindborg

Hi Andreas,

> On 18 Aug 2025, at 05:14, Andreas Hindborg <a.hindborg@kernel.org> wrote:
> 
> "Daniel Almeida" <daniel.almeida@collabora.com> writes:
> 
>> 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.
>> 
>> Tested-by: Joel Fernandes <joelagnelf@nvidia.com>
>> Tested-by: Dirk Behme <dirk.behme@de.bosch.com>
>> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
>> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
>> ---
>> rust/bindings/bindings_helper.h |   1 +
>> rust/helpers/helpers.c          |   1 +
>> rust/helpers/irq.c              |   9 ++
>> rust/kernel/irq.rs              |   5 +
>> rust/kernel/irq/request.rs      | 264 ++++++++++++++++++++++++++++++++++++++++
>> 5 files changed, 280 insertions(+)
>> 
>> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
>> index 84d60635e8a9baef1f1a1b2752dc0fa044f8542f..69a975da829f0c35760f71a1b32b8fcb12c8a8dc 100644
>> --- a/rust/bindings/bindings_helper.h
>> +++ b/rust/bindings/bindings_helper.h
>> @@ -52,6 +52,7 @@
>> #include <linux/ethtool.h>
>> #include <linux/file.h>
>> #include <linux/firmware.h>
>> +#include <linux/interrupt.h>
>> #include <linux/fs.h>
>> #include <linux/ioport.h>
>> #include <linux/jiffies.h>
>> diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
>> index 7cf7fe95e41dd51717050648d6160bebebdf4b26..44b2005d50140d34a44ae37d01c2ddbae6aeaa32 100644
>> --- a/rust/helpers/helpers.c
>> +++ b/rust/helpers/helpers.c
>> @@ -22,6 +22,7 @@
>> #include "dma.c"
>> #include "drm.c"
>> #include "err.c"
>> +#include "irq.c"
>> #include "fs.c"
>> #include "io.c"
>> #include "jump_label.c"
>> diff --git a/rust/helpers/irq.c b/rust/helpers/irq.c
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..1faca428e2c047a656dec3171855c1508d67e60b
>> --- /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
>> index 068df2fea31de51115c30344f7ebdb4da4ad86cc..c1019bc36ad1e7ae7dd3af8a8b5c14780bf70712 100644
>> --- a/rust/kernel/irq.rs
>> +++ b/rust/kernel/irq.rs
>> @@ -13,4 +13,9 @@
>> /// Flags to be used when registering IRQ handlers.
>> mod flags;
>> 
>> +/// IRQ allocation and handling.
>> +mod request;
>> +
>> pub use flags::Flags;
>> +
>> +pub use request::{Handler, IrqRequest, IrqReturn, Registration};
>> diff --git a/rust/kernel/irq/request.rs b/rust/kernel/irq/request.rs
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..57e00ebf694d8e6e870d9ed57af7ee2ecf86ec05
>> --- /dev/null
>> +++ b/rust/kernel/irq/request.rs
>> @@ -0,0 +1,264 @@
>> +// 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, Device};
> 
> nit: I would suggest either normalizing all the imports, or using one
> import per line consistently.
> 
>> +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 a [`Handler`] or a [`ThreadedHandler`].
> 
> error: unresolved link to `ThreadedHandler`
>  --> /home/aeh/src/linux-rust/request-irq/rust/kernel/irq/request.rs:18:62
>   |
> 18 | /// The value that can be returned from a [`Handler`] or a [`ThreadedHandler`].
>   |                                                              ^^^^^^^^^^^^^^^ no item named `ThreadedHandler` in scope
>   |

This was introduced by the next commit. I wonder what is the right thing to do
here now?

> 
>> +#[repr(u32)]
>> +pub enum IrqReturn {
>> +    /// The interrupt was not from this device or was not handled.
>> +    None = bindings::irqreturn_IRQ_NONE,
>> +
>> +    /// The interrupt was handled by this device.
>> +    Handled = bindings::irqreturn_IRQ_HANDLED,
>> +}
>> +
>> +/// Callbacks for an IRQ handler.
>> +pub trait Handler: Sync {
>> +    /// The hard IRQ handler.
> 
> Could you do a vocabulary somewhere? What does it mean that the handler
> is hard?


This nomenclature is borrowed from the C part of the kernel. The hard part is
what runs immediately in interrupt context while the bottom half runs later. In
this case, the bottom half is a threaded handler, i.e.: code running in a
separate kthread.

I think this is immediately understandable most of the time because it's a term
that is often used in the kernel. Do you still feel that I should expand the
docs in this case?

> 
>> +    ///
>> +    /// 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`].
> 
> error: unresolved link to `ThreadedRegistration`
>  --> /home/aeh/src/linux-rust/request-irq/rust/kernel/irq/request.rs:37:20
>   |
> 37 |     /// See also [`ThreadedRegistration`].
>   |                    ^^^^^^^^^^^^^^^^^^^^ no item named `ThreadedRegistration` in scope
>   |
> 

Same as the previous doc issue you highlighted.

>> +    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`.
> 
> This seems like a mix of invariant declaration and conformance. I don't
> think the following belongs here:
> 
>  It is guaranteed to be unique
>  by the type system, since each call to `new` will return a different instance of
>  `Registration`.
> 
> You could replace it with a uniqueness requirement.

WDYM? This invariant is indeed provided by the type system and we do rely on it
to make the abstraction work.

> 
>> +#[pin_data(PinnedDrop)]
>> +struct RegistrationInner {
>> +    irq: u32,
>> +    cookie: *mut 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.
> 
> Why?

It's a u32 and an opaque pointer. The pointer itself (which is what demands a
manual send/sync impl) is only used on drop() and there are no restrictions
that prevent freeing an irq from another thread.

The cookie points to the handler, i.e.:

+                    cookie: unsafe { &raw mut (*this.as_ptr()).handler }cast(),

Perhaps we can add a bound on Send for Handler if that helps?

> 
>> +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 {
> 
> This needs `#[expect(dead_code)]`.

Danilo did that already before applying.

> 
>> +        // INVARIANT: `irq` is a valid IRQ number for `dev`.
> 
> I would suggest rephrasing:
> 
>  By function safety requirement, irq` is a valid IRQ number for `dev`.

Ack, I can send a patch.

> 
>> +        IrqRequest { dev, irq }
>> +    }
>> +
>> +    /// Returns the IRQ number of an [`IrqRequest`].
>> +    pub fn irq(&self) -> u32 {
>> +        self.irq
>> +    }
>> +}
>> +
>> +/// A registration of an IRQ handler for a given IRQ line.
>> +///
>> +/// # Examples
>> +///
>> +/// The following is an example of using `Registration`. It uses a
>> +/// [`Completion`] to coordinate between the IRQ
>> +/// handler and process context. [`Completion`] uses interior mutability, so the
>> +/// handler can signal with [`Completion::complete_all()`] and the process
>> +/// context can wait with [`Completion::wait_for_completion()`] even though
>> +/// there is no way to get a mutable reference to the any of the fields in
>> +/// `Data`.
>> +///
>> +/// [`Completion`]: kernel::sync::Completion
>> +/// [`Completion::complete_all()`]: kernel::sync::Completion::complete_all
>> +/// [`Completion::wait_for_completion()`]: kernel::sync::Completion::wait_for_completion
>> +///
>> +/// ```
>> +/// use kernel::c_str;
>> +/// use kernel::device::Bound;
>> +/// use kernel::irq::{self, Flags, IrqRequest, IrqReturn, Registration};
>> +/// use kernel::prelude::*;
>> +/// use kernel::sync::{Arc, Completion};
>> +///
>> +/// // Data shared between process and IRQ context.
>> +/// #[pin_data]
>> +/// struct Data {
>> +///     #[pin]
>> +///     completion: Completion,
>> +/// }
>> +///
>> +/// impl irq::Handler for Data {
>> +///     // Executed in IRQ context.
>> +///     fn handle(&self) -> IrqReturn {
>> +///         self.completion.complete_all();
>> +///         IrqReturn::Handled
>> +///     }
>> +/// }
>> +///
>> +/// // Registers an IRQ handler for the given IrqRequest.
>> +/// //
>> +/// // This runs in process context and assumes `request` was previously acquired from a device.
>> +/// fn register_irq(
>> +///     handler: impl PinInit<Data, Error>,
>> +///     request: IrqRequest<'_>,
>> +/// ) -> Result<Arc<Registration<Data>>> {
>> +///     let registration = Registration::new(request, Flags::SHARED, c_str!("my_device"), handler);
>> +///
>> +///     let registration = Arc::pin_init(registration, GFP_KERNEL)?;
>> +///
>> +///     registration.handler().completion.wait_for_completion();
>> +///
>> +///     Ok(registration)
>> +/// }
>> +/// # Ok::<(), Error>(())
>> +/// ```
>> +///
>> +/// # Invariants
>> +///
>> +/// * We own an irq handler using `&self.handler` as its private data.
>> +#[pin_data]
>> +pub struct Registration<T: Handler + 'static> {
>> +    #[pin]
>> +    inner: Devres<RegistrationInner>,
> 
> Soundness of this API requires `inner` to be dropped before `handler`.
> Maybe we should have a comment specifying that the order of these fields
> is important?

Ack.

> 
>> +
>> +    #[pin]
>> +    handler: T,
>> +
>> +    /// Pinned because we need address stability so that we can pass a pointer
>> +    /// to the callback.
>> +    #[pin]
>> +    _pin: PhantomPinned,
>> +}
>> +
>> +impl<T: Handler + 'static> Registration<T> {
>> +    /// Registers the IRQ handler with the system for the given IRQ number.
> 
> Could this be "... for the IRQ number represented by `request`."? Because we
> don't pass in an actual number here.

Ack.

> 
>> +    pub fn new<'a>(
>> +        request: IrqRequest<'a>,
>> +        flags: Flags,
>> +        name: &'static CStr,
>> +        handler: impl PinInit<T, Error> + 'a,
>> +    ) -> impl PinInit<Self, Error> + 'a {
>> +        try_pin_init!(&this in Self {
>> +            handler <- handler,
>> +            inner <- Devres::new(
>> +                request.dev,
>> +                try_pin_init!(RegistrationInner {
>> +                    // SAFETY: `this` is a valid pointer to the `Registration` instance
>> +                    cookie: unsafe { &raw mut (*this.as_ptr()).handler }cast(),
>> +                    irq: {
>> +                        // SAFETY:
>> +                        // - The callbacks are valid for use with request_irq.
>> +                        // - If this succeeds, the slot is guaranteed to be valid until the
>> +                        //   destructor of Self runs, which will deregister the callbacks
>> +                        //   before the memory location becomes invalid.
>> +                        to_result(unsafe {
>> +                            bindings::request_irq(
>> +                                request.irq,
>> +                                Some(handle_irq_callback::<T>),
>> +                                flags.into_inner(),
>> +                                name.as_char_ptr(),
>> +                                (&raw mut (*this.as_ptr()).handler).cast(),
>> +                            )
>> +                        })?;
>> +                        request.irq
>> +                    }
>> +                })
>> +            ),
>> +            _pin: PhantomPinned,
>> +        })
>> +    }
>> +
>> +    /// Returns a reference to the handler that was registered with the system.
>> +    pub fn handler(&self) -> &T {
>> +        &self.handler
>> +    }
>> +
>> +    /// Wait for pending IRQ handlers on other CPUs.
>> +    ///
>> +    /// This will attempt to access the inner [`Devres`] container.
>> +    pub fn try_synchronize(&self) -> Result {
>> +        let inner = self.inner.try_access().ok_or(ENODEV)?;
>> +        inner.synchronize();
>> +        Ok(())
>> +    }
>> +
>> +    /// Wait for pending IRQ handlers on other CPUs.
>> +    pub fn synchronize(&self, dev: &Device<Bound>) -> Result {
>> +        let inner = self.inner.access(dev)?;
>> +        inner.synchronize();
>> +        Ok(())
>> +    }
>> +}
>> +
>> +/// # Safety
>> +///
>> +/// This function should be only used as the callback in `request_irq`.
> 
> I think this safety requirement is inadequate. We must require `ptr` to
> be valid for use as a reference to `T`. When we install the pointer to
> this function, we should certify why safety requirements are fulfilled
> when C calls through the pointer.

Ack.

> 
>> +unsafe extern "C" fn handle_irq_callback<T: Handler>(_irq: i32, ptr: *mut c_void) -> c_uint {
>> +    // SAFETY: `ptr` is a pointer to T set in `Registration::new`
>> +    let handler = unsafe { &*(ptr as *const T) };
>> +    T::handle(handler) as c_uint
>> +}
> 
> 
> Best regards,
> Andreas Hindborg

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

> Hi Andreas,
>
>> On 18 Aug 2025, at 05:14, Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> "Daniel Almeida" <daniel.almeida@collabora.com> writes:
>>
>>> 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.
>>>
>>> Tested-by: Joel Fernandes <joelagnelf@nvidia.com>
>>> Tested-by: Dirk Behme <dirk.behme@de.bosch.com>
>>> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
>>> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
>>> ---
>>> rust/bindings/bindings_helper.h |   1 +
>>> rust/helpers/helpers.c          |   1 +
>>> rust/helpers/irq.c              |   9 ++
>>> rust/kernel/irq.rs              |   5 +
>>> rust/kernel/irq/request.rs      | 264 ++++++++++++++++++++++++++++++++++++++++
>>> 5 files changed, 280 insertions(+)
>>>
>>> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
>>> index 84d60635e8a9baef1f1a1b2752dc0fa044f8542f..69a975da829f0c35760f71a1b32b8fcb12c8a8dc 100644
>>> --- a/rust/bindings/bindings_helper.h
>>> +++ b/rust/bindings/bindings_helper.h
>>> @@ -52,6 +52,7 @@
>>> #include <linux/ethtool.h>
>>> #include <linux/file.h>
>>> #include <linux/firmware.h>
>>> +#include <linux/interrupt.h>
>>> #include <linux/fs.h>
>>> #include <linux/ioport.h>
>>> #include <linux/jiffies.h>
>>> diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
>>> index 7cf7fe95e41dd51717050648d6160bebebdf4b26..44b2005d50140d34a44ae37d01c2ddbae6aeaa32 100644
>>> --- a/rust/helpers/helpers.c
>>> +++ b/rust/helpers/helpers.c
>>> @@ -22,6 +22,7 @@
>>> #include "dma.c"
>>> #include "drm.c"
>>> #include "err.c"
>>> +#include "irq.c"
>>> #include "fs.c"
>>> #include "io.c"
>>> #include "jump_label.c"
>>> diff --git a/rust/helpers/irq.c b/rust/helpers/irq.c
>>> new file mode 100644
>>> index 0000000000000000000000000000000000000000..1faca428e2c047a656dec3171855c1508d67e60b
>>> --- /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
>>> index 068df2fea31de51115c30344f7ebdb4da4ad86cc..c1019bc36ad1e7ae7dd3af8a8b5c14780bf70712 100644
>>> --- a/rust/kernel/irq.rs
>>> +++ b/rust/kernel/irq.rs
>>> @@ -13,4 +13,9 @@
>>> /// Flags to be used when registering IRQ handlers.
>>> mod flags;
>>>
>>> +/// IRQ allocation and handling.
>>> +mod request;
>>> +
>>> pub use flags::Flags;
>>> +
>>> +pub use request::{Handler, IrqRequest, IrqReturn, Registration};
>>> diff --git a/rust/kernel/irq/request.rs b/rust/kernel/irq/request.rs
>>> new file mode 100644
>>> index 0000000000000000000000000000000000000000..57e00ebf694d8e6e870d9ed57af7ee2ecf86ec05
>>> --- /dev/null
>>> +++ b/rust/kernel/irq/request.rs
>>> @@ -0,0 +1,264 @@
>>> +// 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, Device};
>>
>> nit: I would suggest either normalizing all the imports, or using one
>> import per line consistently.
>>
>>> +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 a [`Handler`] or a [`ThreadedHandler`].
>>
>> error: unresolved link to `ThreadedHandler`
>>  --> /home/aeh/src/linux-rust/request-irq/rust/kernel/irq/request.rs:18:62
>>   |
>> 18 | /// The value that can be returned from a [`Handler`] or a [`ThreadedHandler`].
>>   |                                                              ^^^^^^^^^^^^^^^ no item named `ThreadedHandler` in scope
>>   |
>
> This was introduced by the next commit. I wonder what is the right thing to do
> here now?

I would suggest (next time, since this is applied) not linking the
symbol in this patch and then adding the link in the patch that adds the
link target.

Or, adding the paragraph with the patch that adds the link target.

>
>>
>>> +#[repr(u32)]
>>> +pub enum IrqReturn {
>>> +    /// The interrupt was not from this device or was not handled.
>>> +    None = bindings::irqreturn_IRQ_NONE,
>>> +
>>> +    /// The interrupt was handled by this device.
>>> +    Handled = bindings::irqreturn_IRQ_HANDLED,
>>> +}
>>> +
>>> +/// Callbacks for an IRQ handler.
>>> +pub trait Handler: Sync {
>>> +    /// The hard IRQ handler.
>>
>> Could you do a vocabulary somewhere? What does it mean that the handler
>> is hard?
>
>
> This nomenclature is borrowed from the C part of the kernel. The hard part is
> what runs immediately in interrupt context while the bottom half runs later. In
> this case, the bottom half is a threaded handler, i.e.: code running in a
> separate kthread.
>
> I think this is immediately understandable most of the time because it's a term
> that is often used in the kernel. Do you still feel that I should expand the
> docs in this case?

Yes, I do. We have to consider that some people reading this API might
not be aware of this tribal knowledge. This is our chance to properly
document public kernel APIs.

If you feel that you would be duplicating existing documentation, I
would suggest linking to that documentation.

We could just add a paragraph inline:

  The hard IRQ handler.

  A "hard" handler in the context of the Linux kernel is the part of an
  interrupt handler that runs in interrupt context. The hard handler
  usually defers time consuming processing to run in process context,
  for instance by queuing the work on a work queue for later execution.

(I am not sure if this description is correct, and I would suggest also
describing "bottom half" if you are going to use that term.)

>
>>
>>> +    ///
>>> +    /// 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`].
>>
>> error: unresolved link to `ThreadedRegistration`
>>  --> /home/aeh/src/linux-rust/request-irq/rust/kernel/irq/request.rs:37:20
>>   |
>> 37 |     /// See also [`ThreadedRegistration`].
>>   |                    ^^^^^^^^^^^^^^^^^^^^ no item named `ThreadedRegistration` in scope
>>   |
>>
>
> Same as the previous doc issue you highlighted.
>
>>> +    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`.
>>
>> This seems like a mix of invariant declaration and conformance. I don't
>> think the following belongs here:
>>
>>  It is guaranteed to be unique
>>  by the type system, since each call to `new` will return a different instance of
>>  `Registration`.
>>
>> You could replace it with a uniqueness requirement.
>
> WDYM? This invariant is indeed provided by the type system and we do rely on it
> to make the abstraction work.

The invariant section of the type documentation should be a list of
invariants, not reasoning about why the invariants hold. The reasoning
goes in the code where we construct the type, where we momentarily
break invariants, or where we change the value of the type.

In this case, I think the invariant is that `cookie` is unique. How we
conform to this invariant does not belong in the list. When you
construct the type, you should have an `// INVARIANT:` comment
explaining why the newly constructed type upholds the invariant.

At least that is how I understand intended use of the framework.

>
>>
>>> +#[pin_data(PinnedDrop)]
>>> +struct RegistrationInner {
>>> +    irq: u32,
>>> +    cookie: *mut 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.
>>
>> Why?
>
> It's a u32 and an opaque pointer. The pointer itself (which is what demands a
> manual send/sync impl) is only used on drop() and there are no restrictions
> that prevent freeing an irq from another thread.

Then use this as your safety comment. This text way more informative to
the reader than the original. Perhaps something like this:

  It is safe to transfer ownership of `RegistrationInner` from another
  thread, because it has no shared mutable state. The IRQ owned by
  `RegistrationInner` via `cookie` can be dropped from any thread.


Best regards,
Andreas Hindborg

On 8/27/25 9:50 AM, Andreas Hindborg wrote:
> I would suggest (next time, since this is applied) not linking the
> symbol in this patch and then adding the link in the patch that adds the
> link target.

That's what I did on apply. :)

Hi Andreas,

> On 18 Aug 2025, at 05:14, Andreas Hindborg <a.hindborg@kernel.org> wrote:
> 
> "Daniel Almeida" <daniel.almeida@collabora.com> writes:
> 
>> 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.
>> 
>> Tested-by: Joel Fernandes <joelagnelf@nvidia.com>
>> Tested-by: Dirk Behme <dirk.behme@de.bosch.com>
>> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
>> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
>> ---

This was merged as you noticed in the other thread, but I can of course send
new patches to address your comments.

I will get back to you on this soon.

— Daniel