Message-ID: <aIBl6JPh4MQq-0gu@tardis-2.local>
Date: Tue, 22 Jul 2025 21:32:40 -0700
From: Boqun Feng <boqun.feng@...il.com>
To: Daniel Almeida <daniel.almeida@...labora.com>
Cc: Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>,
	Gary Guo <gary@...yguo.net>,
	Björn Roy Baron <bjorn3_gh@...tonmail.com>,
	Andreas Hindborg <a.hindborg@...nel.org>,
	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´nski <kwilczynski@...nel.org>,
	Benno Lossin <lossin@...nel.org>, linux-kernel@...r.kernel.org,
	rust-for-linux@...r.kernel.org, linux-pci@...r.kernel.org
Subject: Re: [PATCH v7 3/6] rust: irq: add support for non-threaded IRQs and
 handlers

On Tue, Jul 15, 2025 at 12:16:40PM -0300, 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@...labora.com>
> ---
[...]
> diff --git a/rust/kernel/irq.rs b/rust/kernel/irq.rs
> index 9abd9a6dc36f3e3ecc1f92ad7b0040176b56a079..01bd08884b72c2a3a9460897bce751c732a19794 100644
> --- a/rust/kernel/irq.rs
> +++ b/rust/kernel/irq.rs
> @@ -12,3 +12,8 @@
>  
>  /// Flags to be used when registering IRQ handlers.
>  pub mod flags;
> +
> +/// IRQ allocation and handling.
> +pub mod request;
> +
> +pub use request::{Handler, IrqRequest, IrqReturn, Registration};

I woulde use #[doc(inline)] here for these re-export. It'll give a list
of struct/trait users can use in the `irq` module.

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

Missing "// INVARIANT" comment here.

> +        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
> +/// [`AtomicU32`](core::sync::AtomicU32) to provide the interior mutability.

We are going to remove all usage of core::sync::Atomic* when the LKMM
atomics [1] land. You can probably use `Completion` here (handler does
complete_all(), and registration uses wait_for_completion()) because
`Completion` is irq-safe. And this brings my next comment..

> +///
> +/// ```
> +/// use core::sync::atomic::AtomicU32;
> +/// use core::sync::atomic::Ordering;
> +///
> +/// use kernel::prelude::*;
> +/// use kernel::device::Bound;
> +/// use kernel::irq::flags;
> +/// use kernel::irq::Registration;
> +/// use kernel::irq::IrqRequest;
> +/// use kernel::irq::IrqReturn;
> +/// use kernel::sync::Arc;
> +/// 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(AtomicU32);
> +///
> +/// // [`kernel::irq::request::Handler::handle`] takes `&self`. This example
> +/// // illustrates how interior mutability can be used when sharing the data
> +/// // between process context and IRQ context.
> +///
> +/// 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(&self) -> IrqReturn {
> +///         self.0.fetch_add(1, Ordering::Relaxed);
> +///
> +///         IrqReturn::Handled
> +///     }
> +/// }
> +///
> +/// // Registers an IRQ handler for the given IrqRequest.
> +/// //
> +/// // This is executing in process context and assumes that `request` was
> +/// // previously acquired from a device.
> +/// fn register_irq(handler: Handler, request: IrqRequest<'_>) -> Result<Arc<Registration<Handler>>> {
> +///     let registration = Registration::new(request, flags::SHARED, c_str!("my_device"), handler);
> +///
> +///     let registration = Arc::pin_init(registration, GFP_KERNEL)?;
> +///
> +///     // The data can be accessed from process context too.
> +///     registration.handler().0.fetch_add(1, Ordering::Relaxed);
> +///
> +///     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: T,

... to use `Completion` which requires pin-init, it seems that the
current API is a bit limited, we can make this parameter be a:

	handler: impl PinInit<T, Error>

? It'll still support the current usage because we have blanket impl.

Regards,
Boqun

> +    ) -> impl PinInit<Self, Error> + 'a {
> +        try_pin_init!(&this in Self {
> +            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,
> +        })
> +    }
[...]

Powered by blists - more mailing lists