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>
---
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 | 267 ++++++++++++++++++++++++++++++++++++++++
5 files changed, 283 insertions(+)
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 7e8f2285064797d5bbac5583990ff823b76c6bdc..fc73b89ff9d539e536a5da9388e4926a91a6130e 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/jiffies.h>
#include <linux/jump_label.h>
diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
index 0b09bd0e3561c7bf80bf79faf1aebd7eeb851984..653c3f7b85c5f7192b1584c748a9d7e4af3796e9 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 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};
diff --git a/rust/kernel/irq/request.rs b/rust/kernel/irq/request.rs
new file mode 100644
index 0000000000000000000000000000000000000000..2f4637d8bc4c9fda23cbc8307687035957b0042a
--- /dev/null
+++ b/rust/kernel/irq/request.rs
@@ -0,0 +1,267 @@
+// 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.
+#[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 {
+ 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.
+///
+/// ```
+/// 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,
+ ) -> 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,
+ })
+ }
+
+ /// 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.0
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@collabora.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,
> + })
> + }
[...]
Hi Boqun,
[…]
>> + 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..
How are completions equivalent to atomics? I am trying to highlight interior
mutability in this example.
Is the LKMM atomic series getting merged during the upcoming merge window? Because my
understanding was that the IRQ series was ready to go in 6.17, pending a reply
from Thomas and some minor comments that have been mentioned in v7.
If the LKMM series is not ready yet, my proposal is to leave the
Atomics->Completion change for a future patch (or really, to just use the new
Atomic types introduced by your series, because again, I don't think Completion
is the right thing to have there).
— Daniel
On Wed, Jul 23, 2025 at 10:55:20AM -0300, Daniel Almeida wrote:
> Hi Boqun,
>
> [...]
>
> >> + 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..
>
> How are completions equivalent to atomics? I am trying to highlight interior
> mutability in this example.
>
Well, `Completion` also has interior mutability.
> Is the LKMM atomic series getting merged during the upcoming merge window? Because my
> understanding was that the IRQ series was ready to go in 6.17, pending a reply
Nope, it's likely to be in 6.18.
> from Thomas and some minor comments that have been mentioned in v7.
>
> If the LKMM series is not ready yet, my proposal is to leave the
> Atomics->Completion change for a future patch (or really, to just use the new
> Atomic types introduced by your series, because again, I don't think Completion
> is the right thing to have there).
>
Why? I can find a few examples that an irq handler does a
complete_all(), e.g. gpi_process_ch_ctrl_irq() in
drivers/dma/qcom/gpi.c. I think it's very normal for a driver thread to
use completions to wait for an irq to happen.
But sure, this and the handler pinned initializer thing is not a blocker
issue. However, I would like to see them resolved as soon as possible
once merged.
Regards,
Boqun
>
> - Daniel
> On 23 Jul 2025, at 11:26, Boqun Feng <boqun.feng@gmail.com> wrote:
>
> On Wed, Jul 23, 2025 at 10:55:20AM -0300, Daniel Almeida wrote:
>> Hi Boqun,
>>
>> [...]
>>
>>>> + 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..
>>
>> How are completions equivalent to atomics? I am trying to highlight interior
>> mutability in this example.
>>
>
> Well, `Completion` also has interior mutability.
>
>> Is the LKMM atomic series getting merged during the upcoming merge window? Because my
>> understanding was that the IRQ series was ready to go in 6.17, pending a reply
>
> Nope, it's likely to be in 6.18.
>
>> from Thomas and some minor comments that have been mentioned in v7.
>>
>> If the LKMM series is not ready yet, my proposal is to leave the
>> Atomics->Completion change for a future patch (or really, to just use the new
>> Atomic types introduced by your series, because again, I don't think Completion
>> is the right thing to have there).
>>
>
> Why? I can find a few examples that an irq handler does a
> complete_all(), e.g. gpi_process_ch_ctrl_irq() in
> drivers/dma/qcom/gpi.c. I think it's very normal for a driver thread to
> use completions to wait for an irq to happen.
>
> But sure, this and the handler pinned initializer thing is not a blocker
> issue. However, I would like to see them resolved as soon as possible
> once merged.
>
> Regards,
> Boqun
>
>>
>> - Daniel
Because it is not as explicit. The main thing we should be conveying to users
here is how to get a &mut or otherwise mutate the data when running the
handler. When people see AtomicU32, it's a quick jump to "I can make this work
by using other locks, like SpinLockIrq". Completions hide this, IMHO.
It's totally possible for someone to see this and say "ok, I can call
complete() on this, but how can I mutate the data in some random T struct?",
even though these are essentially the same thing from an interior mutability
point of view.
-- Daniel
On Wed, Jul 23, 2025 at 11:54:09AM -0300, Daniel Almeida wrote:
>
>
> > On 23 Jul 2025, at 11:26, Boqun Feng <boqun.feng@gmail.com> wrote:
> >
> > On Wed, Jul 23, 2025 at 10:55:20AM -0300, Daniel Almeida wrote:
> >> Hi Boqun,
> >>
> >> [...]
> >>
> >>>> + 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..
> >>
> >> How are completions equivalent to atomics? I am trying to highlight interior
> >> mutability in this example.
> >>
> >
> > Well, `Completion` also has interior mutability.
> >
> >> Is the LKMM atomic series getting merged during the upcoming merge window? Because my
> >> understanding was that the IRQ series was ready to go in 6.17, pending a reply
> >
> > Nope, it's likely to be in 6.18.
> >
> >> from Thomas and some minor comments that have been mentioned in v7.
> >>
> >> If the LKMM series is not ready yet, my proposal is to leave the
> >> Atomics->Completion change for a future patch (or really, to just use the new
> >> Atomic types introduced by your series, because again, I don't think Completion
> >> is the right thing to have there).
> >>
> >
> > Why? I can find a few examples that an irq handler does a
> > complete_all(), e.g. gpi_process_ch_ctrl_irq() in
> > drivers/dma/qcom/gpi.c. I think it's very normal for a driver thread to
> > use completions to wait for an irq to happen.
> >
> > But sure, this and the handler pinned initializer thing is not a blocker
> > issue. However, I would like to see them resolved as soon as possible
> > once merged.
> >
> > Regards,
> > Boqun
> >
> >>
> >> - Daniel
>
>
> Because it is not as explicit. The main thing we should be conveying to users
> here is how to get a &mut or otherwise mutate the data when running the
> handler. When people see AtomicU32, it's a quick jump to "I can make this work
> by using other locks, like SpinLockIrq". Completions hide this, IMHO.
>
I understand your argument. However, I'm not sure the example of
`irq::Registration` is the right place to do this. On one hand, it's one
of the usage of interior mutability as you said, but on the other hand,
for people who are familiar with interior mutability, the difference
between `AtomicU32` and `Completion` is not that much. That's kinda my
argument why using `Completion` in the example here is fine.
Sounds reasonable?
> It's totally possible for someone to see this and say "ok, I can call
> complete() on this, but how can I mutate the data in some random T struct?",
> even though these are essentially the same thing from an interior mutability
> point of view.
>
We probably better assume that interior mutability is commmon knowledge
or we could make an link to some documentation of interior mutability,
for example [1], in the documentation of `handler`. Not saying your
effort and consideration is not valid, but at the project level,
interior mutability should be widely acknowledged IMO.
[1]: https://doc.rust-lang.org/reference/interior-mutability.html
Regards,
Boqun
> -- Daniel
>
>
[…] >> >> >> Because it is not as explicit. The main thing we should be conveying to users >> here is how to get a &mut or otherwise mutate the data when running the >> handler. When people see AtomicU32, it's a quick jump to "I can make this work >> by using other locks, like SpinLockIrq". Completions hide this, IMHO. >> > > I understand your argument. However, I'm not sure the example of > `irq::Registration` is the right place to do this. On one hand, it's one > of the usage of interior mutability as you said, but on the other hand, > for people who are familiar with interior mutability, the difference > between `AtomicU32` and `Completion` is not that much. That's kinda my > argument why using `Completion` in the example here is fine. > > Sounds reasonable? > >> It's totally possible for someone to see this and say "ok, I can call >> complete() on this, but how can I mutate the data in some random T struct?", >> even though these are essentially the same thing from an interior mutability >> point of view. >> > > We probably better assume that interior mutability is commmon knowledge > or we could make an link to some documentation of interior mutability, > for example [1], in the documentation of `handler`. Not saying your > effort and consideration is not valid, but at the project level, > interior mutability should be widely acknowledged IMO. > > [1]: https://doc.rust-lang.org/reference/interior-mutability.html > > Regards, > Boqun > >> -- Daniel I do expect mostly everbody (except brand-new newcomers) to be aware of interior mutability. What I don't expect is for people _immediately_ see that it's being used in Completion, and connect the dots from there. Keyword here being "immediately", users will naturally realize this in a couple of minutes at max, of course. Anyways, I guess we can use Completion then. TBH I wasn't aware of the UB thing, so I can see how you also have a point. On top of that, we can use the words "interior mutability" somewhere in the example as well to make it even clearer. I'll change it for v8. -- Daniel
On Wed Jul 23, 2025 at 6:07 PM CEST, Daniel Almeida wrote: > On top of that, we can use the > words "interior mutability" somewhere in the example as well to make it even > clearer. You *can* have this example and I encourage it, I think it is valuable. You can have spinlock or mutex for this purpose in threaded handler, no?
> On 23 Jul 2025, at 13:11, Danilo Krummrich <dakr@kernel.org> wrote: > > On Wed Jul 23, 2025 at 6:07 PM CEST, Daniel Almeida wrote: >> On top of that, we can use the >> words "interior mutability" somewhere in the example as well to make it even >> clearer. > > You *can* have this example and I encourage it, I think it is valuable. You can > have spinlock or mutex for this purpose in threaded handler, no? Right, but then what goes in the hard-irq part for ThreadedHandler? I guess we can leave that one blank then and only touch the data from the threaded part. If that’s the case, then I think it can work too. — Daniel
On Wed Jul 23, 2025 at 6:18 PM CEST, Daniel Almeida wrote: >> On 23 Jul 2025, at 13:11, Danilo Krummrich <dakr@kernel.org> wrote: >> On Wed Jul 23, 2025 at 6:07 PM CEST, Daniel Almeida wrote: >>> On top of that, we can use the >>> words "interior mutability" somewhere in the example as well to make it even >>> clearer. >> >> You *can* have this example and I encourage it, I think it is valuable. You can >> have spinlock or mutex for this purpose in threaded handler, no? > > Right, but then what goes in the hard-irq part for ThreadedHandler? I guess we > can leave that one blank then and only touch the data from the threaded part. > > If that’s the case, then I think it can work too. For instance, yes. It's a very common pattern to only have the threaded handler but not the hard irq handler implemented. IMHO, for ThreadedHandler the hard irq handler should even have a default blank implementation.
On 7/23/25 4:26 PM, Boqun Feng wrote: > On Wed, Jul 23, 2025 at 10:55:20AM -0300, Daniel Almeida wrote: > But sure, this and the handler pinned initializer thing is not a blocker > issue. However, I would like to see them resolved as soon as possible > once merged. I think it would be trivial to make the T an impl PinInit<T, E> and use a completion as example instead of an atomic. So, we should do it right away. - Danilo
> On 23 Jul 2025, at 11:35, Danilo Krummrich <dakr@kernel.org> wrote: > > On 7/23/25 4:26 PM, Boqun Feng wrote: >> On Wed, Jul 23, 2025 at 10:55:20AM -0300, Daniel Almeida wrote: >> But sure, this and the handler pinned initializer thing is not a blocker >> issue. However, I would like to see them resolved as soon as possible >> once merged. > > I think it would be trivial to make the T an impl PinInit<T, E> and use a > completion as example instead of an atomic. So, we should do it right away. > > - Danilo I agree that this is a trivial change to make. My point here is not to postpone the work; I am actually somewhat against switching to completions, as per the reasoning I provided in my latest reply to Boqun. My plan is to switch directly to whatever will substitute AtomicU32. The switch to impl PinInit is fine. — Daniel
On Wed Jul 23, 2025 at 4:56 PM CEST, Daniel Almeida wrote: >> On 23 Jul 2025, at 11:35, Danilo Krummrich <dakr@kernel.org> wrote: >> On 7/23/25 4:26 PM, Boqun Feng wrote: >>> On Wed, Jul 23, 2025 at 10:55:20AM -0300, Daniel Almeida wrote: >>> But sure, this and the handler pinned initializer thing is not a blocker >>> issue. However, I would like to see them resolved as soon as possible >>> once merged. >> >> I think it would be trivial to make the T an impl PinInit<T, E> and use a >> completion as example instead of an atomic. So, we should do it right away. >> >> - Danilo > > > I agree that this is a trivial change to make. My point here is not to postpone > the work; I am actually somewhat against switching to completions, as per the > reasoning I provided in my latest reply to Boqun. My plan is to switch directly > to whatever will substitute AtomicU32. I mean, Boqun has a point. AFAIK, the Rust atomics are UB in the kernel. So, this is a bit as if we would use spin_lock() instead of spin_lock_irq(), it's just not correct. Hence, we may not want to showcase it until it's actually resolved. The plain truth is, currently there's no synchronization primitive for getting interior mutability in interrupts. You can use a normal spinlock or mutex in the threaded handler though. And in the hard IRQ you can use a completion to indicate something has completed. Once we have proper atomics and spin_lock_irq() we can still change it. > The switch to impl PinInit is fine.
On Wed, Jul 23, 2025 at 05:03:12PM +0200, Danilo Krummrich wrote: > On Wed Jul 23, 2025 at 4:56 PM CEST, Daniel Almeida wrote: > >> On 23 Jul 2025, at 11:35, Danilo Krummrich <dakr@kernel.org> wrote: > >> On 7/23/25 4:26 PM, Boqun Feng wrote: > >>> On Wed, Jul 23, 2025 at 10:55:20AM -0300, Daniel Almeida wrote: > >>> But sure, this and the handler pinned initializer thing is not a blocker > >>> issue. However, I would like to see them resolved as soon as possible > >>> once merged. > >> > >> I think it would be trivial to make the T an impl PinInit<T, E> and use a > >> completion as example instead of an atomic. So, we should do it right away. > >> > >> - Danilo > > > > > > I agree that this is a trivial change to make. My point here is not to postpone > > the work; I am actually somewhat against switching to completions, as per the > > reasoning I provided in my latest reply to Boqun. My plan is to switch directly > > to whatever will substitute AtomicU32. > > I mean, Boqun has a point. AFAIK, the Rust atomics are UB in the kernel. > > So, this is a bit as if we would use spin_lock() instead of spin_lock_irq(), > it's just not correct. Hence, we may not want to showcase it until it's actually > resolved. > > The plain truth is, currently there's no synchronization primitive for getting > interior mutability in interrupts. Is the actual argument here "we are getting rid of Rust atomics in the next cycle, so please don't introduce any more users during the next cycle because if you do it will take one cycle longer to get rid of all Rust atomics"? I can accept that argument. But I don't accept the argument that we shouldn't use them here because of the UB technicality. That is an isolated demand for rigor and I think it is unreasonable. Using Rust atomics is an accepted workaround until the LKMM atomics land. Alice
On Wed Jul 23, 2025 at 5:44 PM CEST, Alice Ryhl wrote: > On Wed, Jul 23, 2025 at 05:03:12PM +0200, Danilo Krummrich wrote: >> On Wed Jul 23, 2025 at 4:56 PM CEST, Daniel Almeida wrote: >> >> On 23 Jul 2025, at 11:35, Danilo Krummrich <dakr@kernel.org> wrote: >> >> On 7/23/25 4:26 PM, Boqun Feng wrote: >> >>> On Wed, Jul 23, 2025 at 10:55:20AM -0300, Daniel Almeida wrote: >> >>> But sure, this and the handler pinned initializer thing is not a blocker >> >>> issue. However, I would like to see them resolved as soon as possible >> >>> once merged. >> >> >> >> I think it would be trivial to make the T an impl PinInit<T, E> and use a >> >> completion as example instead of an atomic. So, we should do it right away. >> >> >> >> - Danilo >> > >> > >> > I agree that this is a trivial change to make. My point here is not to postpone >> > the work; I am actually somewhat against switching to completions, as per the >> > reasoning I provided in my latest reply to Boqun. My plan is to switch directly >> > to whatever will substitute AtomicU32. >> >> I mean, Boqun has a point. AFAIK, the Rust atomics are UB in the kernel. >> >> So, this is a bit as if we would use spin_lock() instead of spin_lock_irq(), >> it's just not correct. Hence, we may not want to showcase it until it's actually >> resolved. >> >> The plain truth is, currently there's no synchronization primitive for getting >> interior mutability in interrupts. > > Is the actual argument here "we are getting rid of Rust atomics in the > next cycle, so please don't introduce any more users during the next > cycle because if you do it will take one cycle longer to get rid of > all Rust atomics"? That's an argument as well, I guess. > I can accept that argument. But I don't accept the argument that we > shouldn't use them here because of the UB technicality. That is an > isolated demand for rigor and I think it is unreasonable. Using Rust > atomics is an accepted workaround until the LKMM atomics land. I think there's a difference between actually needing them and using them to showcase something. Or in other words, limiting workarounds to only those places where we can't avoid them seems like the correct thing to do. Using a completion in the hard IRQ and showing a spinlock or mutex example in the threaded handler seems like a good mix to me.
On Wed, Jul 23, 2025 at 6:32 AM Boqun Feng <boqun.feng@gmail.com> wrote:
>
> 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@collabora.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.
You get the same effect by making `mod request` a private module.
Alice
On Wed, Jul 23, 2025 at 07:35:26AM +0200, Alice Ryhl wrote:
> On Wed, Jul 23, 2025 at 6:32 AM Boqun Feng <boqun.feng@gmail.com> wrote:
> >
> > 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@collabora.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.
>
> You get the same effect by making `mod request` a private module.
>
Oh yes, that also works! I think we probably should do that.
Regards,
Boqun
> Alice
On Tue, Jul 22, 2025 at 09:32:40PM -0700, Boqun Feng wrote: [...] > > +/// 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.. > Missed the link: [1]: https://lore.kernel.org/rust-for-linux/20250719030827.61357-1-boqun.feng@gmail.com/ Regards, Boqun [..]
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@collabora.com>
Overall LGTM. Some very minor nits below.
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index 7e8f2285064797d5bbac5583990ff823b76c6bdc..fc73b89ff9d539e536a5da9388e4926a91a6130e 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/jiffies.h>
> #include <linux/jump_label.h>
> diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
> index 0b09bd0e3561c7bf80bf79faf1aebd7eeb851984..653c3f7b85c5f7192b1584c748a9d7e4af3796e9 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 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};
> diff --git a/rust/kernel/irq/request.rs b/rust/kernel/irq/request.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..2f4637d8bc4c9fda23cbc8307687035957b0042a
> --- /dev/null
> +++ b/rust/kernel/irq/request.rs
> @@ -0,0 +1,267 @@
> +// 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;
The usual style is to write this as:
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 an IrqHandler or a ThreadedIrqHandler.
Missing links:
/// The value that can be returned from an [`IrqHandler`] or a [`ThreadedIrqHandler`].
> +#[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 {
> + 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.
> +///
> +/// ```
> +/// 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::irq::{Flags, IrqRequest, IrqReturn, Registration};
> +/// use kernel::sync::Arc;
> +/// use kernel::c_str;
> +/// use kernel::alloc::flags::GFP_KERNEL;
GFP_KERNEL is in the prelude.
> +/// // 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,
> + ) -> 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,
> + })
> + }
> +
> + /// 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.0
>
Hi Alice, thanks for looking into this again :)
[…]
>> diff --git a/rust/kernel/irq/request.rs b/rust/kernel/irq/request.rs
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..2f4637d8bc4c9fda23cbc8307687035957b0042a
>> --- /dev/null
>> +++ b/rust/kernel/irq/request.rs
>> @@ -0,0 +1,267 @@
>> +// 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;
>
> The usual style is to write this as:
>
> use crate::device::{Bound, Device};
I dislike this syntax because I think it is a conflict magnet. Moreover, when
you get conflicts, they are harder to solve than they are when each import
is in its own line, at least IMHO.
In any case, I don't think we have a guideline for imports at the moment?
[…]
>> +/// 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.
>> +///
>> +/// ```
>> +/// 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::irq::{Flags, IrqRequest, IrqReturn, Registration};
Same here. I’d rather not do this, if it’s ok with others.
— Daniel
On Mon Jul 21, 2025 at 5:10 PM CEST, Daniel Almeida wrote:
> Hi Alice, thanks for looking into this again :)
>
>
> […]
>
>>> diff --git a/rust/kernel/irq/request.rs b/rust/kernel/irq/request.rs
>>> new file mode 100644
>>> index 0000000000000000000000000000000000000000..2f4637d8bc4c9fda23cbc8307687035957b0042a
>>> --- /dev/null
>>> +++ b/rust/kernel/irq/request.rs
>>> @@ -0,0 +1,267 @@
>>> +// 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;
>>
>> The usual style is to write this as:
>>
>> use crate::device::{Bound, Device};
>
> I dislike this syntax because I think it is a conflict magnet. Moreover, when
> you get conflicts, they are harder to solve than they are when each import
> is in its own line, at least IMHO.
Intuitively, I would agree. However, I think practically it's not that bad.
While it's true that Rust has generally more conflict potential - especially in
the current phase - my feeling hasn't been that includes produce significantly
more conflicts then any other code so far.
> In any case, I don't think we have a guideline for imports at the moment?
No, but I think we should try to be as consistent as possible (at least within a
a certain logical unit, e.g. subsystem, module, etc.). Not sure where exactly
the IRQ stuff will end up yet. :)
>>> +/// 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.
>>> +///
>>> +/// ```
>>> +/// 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::irq::{Flags, IrqRequest, IrqReturn, Registration};
>
> Same here. I’d rather not do this, if it’s ok with others.
>
> — Daniel
> On 21 Jul 2025, at 12:28, Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Mon Jul 21, 2025 at 5:10 PM CEST, Daniel Almeida wrote:
>> Hi Alice, thanks for looking into this again :)
>>
>>
>> […]
>>
>>>> diff --git a/rust/kernel/irq/request.rs b/rust/kernel/irq/request.rs
>>>> new file mode 100644
>>>> index 0000000000000000000000000000000000000000..2f4637d8bc4c9fda23cbc8307687035957b0042a
>>>> --- /dev/null
>>>> +++ b/rust/kernel/irq/request.rs
>>>> @@ -0,0 +1,267 @@
>>>> +// 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;
>>>
>>> The usual style is to write this as:
>>>
>>> use crate::device::{Bound, Device};
>>
>> I dislike this syntax because I think it is a conflict magnet. Moreover, when
>> you get conflicts, they are harder to solve than they are when each import
>> is in its own line, at least IMHO.
>
> Intuitively, I would agree. However, I think practically it's not that bad.
>
> While it's true that Rust has generally more conflict potential - especially in
> the current phase - my feeling hasn't been that includes produce significantly
> more conflicts then any other code so far.
Hmm, I faced lots of conflicts for the platform I/O stuff, for example. They
were all on the imports and it was a bit hard to fix it by hand. i.e.: it’s
much simpler to discard the modifications and then ask rust-analyzer to figure
out what should be grouped where on the new code. This is a bit undesirable.
>
>> In any case, I don't think we have a guideline for imports at the moment?
>
> No, but I think we should try to be as consistent as possible (at least within a
> a certain logical unit, e.g. subsystem, module, etc.). Not sure where exactly
> the IRQ stuff will end up yet. :)
Sure, I just think we should discuss this at the kernel crate level at a future
point then, at least IMHO. I think it's something that Andreas had already
commented on, by the way.
— Daniel
Hi Daniel,
kernel test robot noticed the following build errors:
[auto build test ERROR on 3964d07dd821efe9680e90c51c86661a98e60a0f]
url: https://github.com/intel-lab-lkp/linux/commits/Daniel-Almeida/rust-irq-add-irq-module/20250715-232121
base: 3964d07dd821efe9680e90c51c86661a98e60a0f
patch link: https://lore.kernel.org/r/20250715-topics-tyr-request_irq2-v7-3-d469c0f37c07%40collabora.com
patch subject: [PATCH v7 3/6] rust: irq: add support for non-threaded IRQs and handlers
config: x86_64-rhel-9.4-rust (https://download.01.org/0day-ci/archive/20250717/202507170718.AVqYqRan-lkp@intel.com/config)
compiler: clang version 20.1.8 (https://github.com/llvm/llvm-project 87f0227cb60147a26a1eeb4fb06e3b505e9c7261)
rustc: rustc 1.88.0 (6b00bc388 2025-06-23)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250717/202507170718.AVqYqRan-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202507170718.AVqYqRan-lkp@intel.com/
All errors (new ones prefixed by >>):
>> error[E0425]: cannot find value `SHARED` in module `flags`
--> rust/doctests_kernel_generated.rs:4790:58
|
4790 | let registration = Registration::new(request, flags::SHARED, c_str!("my_device"), handler);
| ^^^^^^ not found in `flags`
|
help: consider importing this constant
|
3 + use kernel::mm::virt::flags::SHARED;
|
help: if you import `SHARED`, refer to it directly
|
4790 - let registration = Registration::new(request, flags::SHARED, c_str!("my_device"), handler);
4790 + let registration = Registration::new(request, SHARED, c_str!("my_device"), handler);
|
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
> On 16 Jul 2025, at 20:45, kernel test robot <lkp@intel.com> wrote:
>
> Hi Daniel,
>
> kernel test robot noticed the following build errors:
>
> [auto build test ERROR on 3964d07dd821efe9680e90c51c86661a98e60a0f]
>
> url: https://github.com/intel-lab-lkp/linux/commits/Daniel-Almeida/rust-irq-add-irq-module/20250715-232121
> base: 3964d07dd821efe9680e90c51c86661a98e60a0f
> patch link: https://lore.kernel.org/r/20250715-topics-tyr-request_irq2-v7-3-d469c0f37c07%40collabora.com
> patch subject: [PATCH v7 3/6] rust: irq: add support for non-threaded IRQs and handlers
> config: x86_64-rhel-9.4-rust (https://download.01.org/0day-ci/archive/20250717/202507170718.AVqYqRan-lkp@intel.com/config)
> compiler: clang version 20.1.8 (https://github.com/llvm/llvm-project 87f0227cb60147a26a1eeb4fb06e3b505e9c7261)
> rustc: rustc 1.88.0 (6b00bc388 2025-06-23)
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250717/202507170718.AVqYqRan-lkp@intel.com/reproduce)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202507170718.AVqYqRan-lkp@intel.com/
>
> All errors (new ones prefixed by >>):
>
>>> error[E0425]: cannot find value `SHARED` in module `flags`
> --> rust/doctests_kernel_generated.rs:4790:58
> |
> 4790 | let registration = Registration::new(request, flags::SHARED, c_str!("my_device"), handler);
> | ^^^^^^ not found in `flags`
> |
> help: consider importing this constant
> |
> 3 + use kernel::mm::virt::flags::SHARED;
> |
> help: if you import `SHARED`, refer to it directly
> |
> 4790 - let registration = Registration::new(request, flags::SHARED, c_str!("my_device"), handler);
> 4790 + let registration = Registration::new(request, SHARED, c_str!("my_device"), handler);
> |
>
> --
> 0-DAY CI Kernel Test Service
> https://github.com/intel/lkp-tests/wiki
>
This is a single character fix, so I am waiting for the discussion on the cover
letter [0] to advance before sending a new version.
[0] https://lore.kernel.org/all/DBCQKJIBVGGM.1R0QNKO3TE4N0@kernel.org/#t
On Thu, Jul 17, 2025 at 6:21 PM Daniel Almeida
<daniel.almeida@collabora.com> wrote:
>
>
>
> > On 16 Jul 2025, at 20:45, kernel test robot <lkp@intel.com> wrote:
> >
> > Hi Daniel,
> >
> > kernel test robot noticed the following build errors:
> >
> > [auto build test ERROR on 3964d07dd821efe9680e90c51c86661a98e60a0f]
> >
> > url: https://github.com/intel-lab-lkp/linux/commits/Daniel-Almeida/rust-irq-add-irq-module/20250715-232121
> > base: 3964d07dd821efe9680e90c51c86661a98e60a0f
> > patch link: https://lore.kernel.org/r/20250715-topics-tyr-request_irq2-v7-3-d469c0f37c07%40collabora.com
> > patch subject: [PATCH v7 3/6] rust: irq: add support for non-threaded IRQs and handlers
> > config: x86_64-rhel-9.4-rust (https://download.01.org/0day-ci/archive/20250717/202507170718.AVqYqRan-lkp@intel.com/config)
> > compiler: clang version 20.1.8 (https://github.com/llvm/llvm-project 87f0227cb60147a26a1eeb4fb06e3b505e9c7261)
> > rustc: rustc 1.88.0 (6b00bc388 2025-06-23)
> > reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250717/202507170718.AVqYqRan-lkp@intel.com/reproduce)
> >
> > If you fix the issue in a separate patch/commit (i.e. not just a new version of
> > the same patch/commit), kindly add following tags
> > | Reported-by: kernel test robot <lkp@intel.com>
> > | Closes: https://lore.kernel.org/oe-kbuild-all/202507170718.AVqYqRan-lkp@intel.com/
> >
> > All errors (new ones prefixed by >>):
> >
> >>> error[E0425]: cannot find value `SHARED` in module `flags`
> > --> rust/doctests_kernel_generated.rs:4790:58
> > |
> > 4790 | let registration = Registration::new(request, flags::SHARED, c_str!("my_device"), handler);
> > | ^^^^^^ not found in `flags`
> > |
> > help: consider importing this constant
> > |
> > 3 + use kernel::mm::virt::flags::SHARED;
> > |
> > help: if you import `SHARED`, refer to it directly
> > |
> > 4790 - let registration = Registration::new(request, flags::SHARED, c_str!("my_device"), handler);
> > 4790 + let registration = Registration::new(request, SHARED, c_str!("my_device"), handler);
> > |
> >
> > --
> > 0-DAY CI Kernel Test Service
> > https://github.com/intel/lkp-tests/wiki
> >
>
> This is a single character fix, so I am waiting for the discussion on the cover
> letter [0] to advance before sending a new version.
>
> [0] https://lore.kernel.org/all/DBCQKJIBVGGM.1R0QNKO3TE4N0@kernel.org/#t
My suggestion is to make the flags module private and re-export the
Flags type from the irq module. That way you don't have to write
use kernel::irq::flags::Flags;
Alice
© 2016 - 2025 Red Hat, Inc.