Message-ID: <87wm71cahd.fsf@t14s.mail-host-address-is-not-set>
Date: Mon, 18 Aug 2025 10:14:38 +0200
From: Andreas Hindborg <a.hindborg@...nel.org>
To: Daniel Almeida <daniel.almeida@...labora.com>, Miguel Ojeda
 <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>, Boqun Feng
 <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>, Björn
 Roy Baron
 <bjorn3_gh@...tonmail.com>, Alice Ryhl <aliceryhl@...gle.com>, Trevor
 Gross <tmgross@...ch.edu>, Danilo Krummrich <dakr@...nel.org>, Greg
 Kroah-Hartman <gregkh@...uxfoundation.org>, "Rafael J. Wysocki"
 <rafael@...nel.org>, Thomas Gleixner <tglx@...utronix.de>, Bjorn Helgaas
 <bhelgaas@...gle.com>, Krzysztof Wilczyński
 <kwilczynski@...nel.org>,
 Benno Lossin <lossin@...nel.org>
Cc: linux-kernel@...r.kernel.org, rust-for-linux@...r.kernel.org,
 linux-pci@...r.kernel.org, Joel Fernandes <joelagnelf@...dia.com>, Dirk
 Behme <dirk.behme@...bosch.com>, Daniel Almeida
 <daniel.almeida@...labora.com>
Subject: Re: [PATCH v9 3/7] rust: irq: add support for non-threaded IRQs and
 handlers

"Daniel Almeida" <daniel.almeida@...labora.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@...dia.com>
> Tested-by: Dirk Behme <dirk.behme@...bosch.com>
> Reviewed-by: Alice Ryhl <aliceryhl@...gle.com>
> Signed-off-by: Daniel Almeida <daniel.almeida@...labora.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

Powered by blists - more mailing lists