[PATCH v9 0/7] rust: add support for request_irq

[PATCH v9 0/7] rust: add support for request_irq

[Daniel Almeida]

Changes in v9:
- Added more tags (thanks, Alice!)
- Added Alice's patch for &dev: Device<Bound> support
- Applied a diff to account for the latest review comment on the patch
  above
- Added #[pin_data] as applicable to the examples
- Got rid of the "Handler" type alias in the examples
- Removed the leading "#" from the imports in the examples so that they show
  up in the docs
- Made all inner modules private, removed #[doc(inline)] from the
  re-exports
- Link to v8: https://lore.kernel.org/r/20250810-topics-tyr-request_irq2-v8-0-8163f4c4c3a6@collabora.com

Changes in v8:
- Rebased on top of v6.17-rc1
- Used Completion instead of Atomics in the examples for non-threaded IRQs
  (Boqun)
- Take in "impl PinInit<T, Error>" instead of T in
  [Threaded]Registration::new() (Boqun)
- Propagated the changes above to the platform/pci accessors.
- Used a Mutex instead of Atomics in the examples for threaded IRQs.
- Added more links in the docs as appropriate (Alice)
- Re-exported irq::flags::Flags through a "pub use" (Alice).
- Note: left the above as optional  as it does not hurt to specify the full
  path anyway. As a result, no modules were made private.
- Added #[doc(inline)] as appropriate to the re-exports (Boqun).
- Formatted all the examples using nightly rustfmt +
  "format_code_in_doc_comments"
- Fixed a few issues pointed out by make rustdoc
- Merged imports (Alice)
- Defaulted ThreadedIrqHandler::handle() to WakeThread (Danilo)
- Added tags (thanks, Joel & Dirk!)
- Link to v7: https://lore.kernel.org/r/20250715-topics-tyr-request_irq2-v7-0-d469c0f37c07@collabora.com

Changes in v7:
- Rebased on top of driver-core-next
- Added Flags::new(), which is a const fn. This lets us use build_assert!()
  to verify the casts (hopefully this is what you meant, Alice?)
- Changed the Flags inner type to take c_ulong directly, to minimize casts
  (Thanks, Alice)
- Moved the flag constants into Impl Flags, instead of using a separate
  module (Alice)
- Reverted to using #[repr(u32)] in Threaded/IrqReturn (Thanks Alice,
  Benno)
- Fixed all instances where the full path was specified for types in the
  prelude (Alice)
- Removed 'static from the CStr used to perform the lookup in the platform
  accessor (Alice)
- Renamed the PCI accessors, as asked by Danilo
- Added more docs to Flags, going into more detail on what they do and how
  to use them (Miguel)
- Fixed the indentation in some of the docs (Alice)
- Added Alice's r-b as appropriate
- Link to v6: https://lore.kernel.org/rust-for-linux/20250703-topics-tyr-request_irq-v6-0-74103bdc7c52@collabora.com/

Changes in v6:
- Fixed some typos in the docs (thanks, Dirk!)
- Reordered the arguments for the accessors in platform.rs (Danilo)
- Renamed handle_on_thread() to handle_threaded() (Danilo)
- Changed the documentation for Handler and ThreadedHandler to what
  Danilo suggested
- Link to v5: https://lore.kernel.org/r/20250627-topics-tyr-request_irq-v5-0-0545ee4dadf6@collabora.com

Changes in v5:

Thanks, Danilo {
  - Removed extra scope in the examples.
  - Renamed Registration::register() to Registration::new(),
  - Switched to try_pin_init! in Registration::new() (thanks for the
    code and the help, Boqun and Benno)
  - Renamed the trait functions to handle() and handle_on_thread().
  - Introduced IrqRequest with an unsafe pub(crate) constructor
  - Made both register() and the accessors that return IrqRequest public
    the idea is to allow both of these to work:
	// `irq` is an `irq::Registration`
	let irq = pdev.threaded_irq_by_name()?
  and
	// `req` is an `IrqRequest`.
	let req = pdev.irq_by_name()?;
	// `irq` is an `irq::Registration`
	let irq = irq::ThreadedRegistration::new(req)?;

  - Added another name in the byname variants. There's now one for the
    request part and the other one to register()
  - Reworked the examples in request.rs
  - Implemented the irq accessors in place for pci.rs
  - Split the platform accessor macros into two
}

- Added a rust helper for pci_irq_vectors if !CONFIG_PCI_MSI (thanks,
Intel 0day bot)
- Link to v4: https://lore.kernel.org/r/20250608-topics-tyr-request_irq-v4-0-81cb81fb8073@collabora.com

Changes in v4:

Thanks, Benno {
  - Split series into more patches (see patches 1-4)
  - Use cast() where possible
  - Merge pub use statements.
  - Add {Threaded}IrqReturn::into_inner() instead of #[repr(u32)]
  - Used AtomicU32 instead of SpinLock to add interior mutability to the
    handler's data. SpinLockIrq did not land yet.
  - Mention that `&self` is !Unpin and was initialized using pin_init in
    drop()
  - Fix the docs slightly
}

- Add {try_}synchronize_irq().
- Use Devres for the irq registration (see RegistrationInner). This idea
  was suggested by Danilo and Alice.
- Added PCI accessors (as asked by Joel Fernandez)
- Fix a major oversight: we were passing in a pointer to Registration
  in register_{threaded}_irq() but casting it to Handler/ThreadedHandler in
  the callbacks.
- Make register() pub(crate) so drivers can only retrieve registrations
  through device-specific accessors. This forbids drivers from trying to
  register an invalid irq.
- I think this will still go through a few rounds, so I'll defer the
  patch to update MAINTAINERS for now.

- Link to v3: https://lore.kernel.org/r/20250514-topics-tyr-request_irq-v3-0-d6fcc2591a88@collabora.com

Changes in v3:
- Rebased on driver-core-next
- Added patch to get the irq numbers from a platform device (thanks,
  Christian!)
- Split flags into its own file.
- Change iff to "if and only if"
- Implement PartialEq and Eq for Flags
- Fix some broken docs/markdown
- Reexport most things so users can elide ::request from the path
- Add a blanket implementation of ThreadedHandler and Handler for
  Arc/Box<T: Handler> that just forwards the call to the T. This lets us
  have Arc<Foo> and Box<Foo> as handlers if Foo: Handler.
- Rework the examples a bit.
- Remove "as _" casts in favor of "as u64" for flags. This is needed to
  cast the individual flags into u64.
- Use #[repr(u32)] for ThreadedIrqReturn and IrqReturn.
- Wrapped commit messages to < 75 characters

- Link to v2: https://lore.kernel.org/r/20250122163932.46697-1-daniel.almeida@collabora.com

Changes in v2:
- Added Co-developed-by tag to account for the work that Alice did in order to
figure out how to do this without Opaque<T> (Thanks!)
- Removed Opaque<T> in favor of plain T
- Fixed the examples
- Made sure that the invariants sections are the last entry in the docs
- Switched to slot.cast() where applicable,
- Mentioned in the safety comments that we require that T: Sync,
- Removed ThreadedFnReturn in favor of IrqReturn,
- Improved the commit message

Link to v1: https://lore.kernel.org/rust-for-linux/20241024-topic-panthor-rs-request_irq-v1-1-7cbc51c182ca@collabora.com/

---
Alice Ryhl (1):
      rust: irq: add &Device<Bound> argument to irq callbacks

Daniel Almeida (6):
      rust: irq: add irq module
      rust: irq: add flags module
      rust: irq: add support for non-threaded IRQs and handlers
      rust: irq: add support for threaded IRQs and handlers
      rust: platform: add irq accessors
      rust: pci: add irq accessors

 rust/bindings/bindings_helper.h |   1 +
 rust/helpers/helpers.c          |   1 +
 rust/helpers/irq.c              |   9 +
 rust/helpers/pci.c              |   8 +
 rust/kernel/irq.rs              |  24 ++
 rust/kernel/irq/flags.rs        | 124 ++++++++++
 rust/kernel/irq/request.rs      | 507 ++++++++++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs              |   1 +
 rust/kernel/pci.rs              |  45 +++-
 rust/kernel/platform.rs         | 142 +++++++++++
 10 files changed, 860 insertions(+), 2 deletions(-)
---
base-commit: 8f5ae30d69d7543eee0d70083daf4de8fe15d585
change-id: 20250712-topics-tyr-request_irq2-ae7ee9b85854

Best regards,
-- 
Daniel Almeida <daniel.almeida@collabora.com>

[PATCH v9 1/7] rust: irq: add irq module

[Daniel Almeida]

Add the IRQ module. Future patches will then introduce support for IRQ
registrations and handlers.

Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Tested-by: Joel Fernandes <joelagnelf@nvidia.com>
Tested-by: Dirk Behme <dirk.behme@de.bosch.com>
Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
---
 rust/kernel/irq.rs | 11 +++++++++++
 rust/kernel/lib.rs |  1 +
 2 files changed, 12 insertions(+)

diff --git a/rust/kernel/irq.rs b/rust/kernel/irq.rs
new file mode 100644
index 0000000000000000000000000000000000000000..fae7b15effc80c936d6bffbd5b4150000d6c2898
--- /dev/null
+++ b/rust/kernel/irq.rs
@@ -0,0 +1,11 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! IRQ abstractions.
+//!
+//! An IRQ is an interrupt request from a device. It is used to get the CPU's
+//! attention so it can service a hardware event in a timely manner.
+//!
+//! The current abstractions handle IRQ requests and handlers, i.e.: it allows
+//! drivers to register a handler for a given IRQ line.
+//!
+//! C header: [`include/linux/device.h`](srctree/include/linux/interrupt.h)
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index ed53169e795c0badf548025a57f946fa18bc73e3..f8db761c5c95fc66e4c55f539b17fca613161ada 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -92,6 +92,7 @@
 pub mod init;
 pub mod io;
 pub mod ioctl;
+pub mod irq;
 pub mod jump_label;
 #[cfg(CONFIG_KUNIT)]
 pub mod kunit;

-- 
2.50.1

[PATCH v9 2/7] rust: irq: add flags module

[Daniel Almeida]

Manipulating IRQ flags (i.e.: IRQF_*) will soon be necessary, specially to
register IRQ handlers through bindings::request_irq().

Add a kernel::irq::Flags for that purpose.

Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Tested-by: Joel Fernandes <joelagnelf@nvidia.com>
Tested-by: Dirk Behme <dirk.behme@de.bosch.com>
Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
---
 rust/kernel/irq.rs       |   5 ++
 rust/kernel/irq/flags.rs | 124 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 129 insertions(+)

diff --git a/rust/kernel/irq.rs b/rust/kernel/irq.rs
index fae7b15effc80c936d6bffbd5b4150000d6c2898..068df2fea31de51115c30344f7ebdb4da4ad86cc 100644
--- a/rust/kernel/irq.rs
+++ b/rust/kernel/irq.rs
@@ -9,3 +9,8 @@
 //! drivers to register a handler for a given IRQ line.
 //!
 //! C header: [`include/linux/device.h`](srctree/include/linux/interrupt.h)
+
+/// Flags to be used when registering IRQ handlers.
+mod flags;
+
+pub use flags::Flags;
diff --git a/rust/kernel/irq/flags.rs b/rust/kernel/irq/flags.rs
new file mode 100644
index 0000000000000000000000000000000000000000..e62820ea67755123b4f96e4331244bbb4fbcfd9d
--- /dev/null
+++ b/rust/kernel/irq/flags.rs
@@ -0,0 +1,124 @@
+// SPDX-License-Identifier: GPL-2.0
+// SPDX-FileCopyrightText: Copyright 2025 Collabora ltd.
+
+use crate::bindings;
+use crate::prelude::*;
+
+/// Flags to be used when registering IRQ handlers.
+///
+/// Flags can be used to request specific behaviors when registering an IRQ
+/// handler, and can be combined using the `|`, `&`, and `!` operators to
+/// further control the system's behavior.
+///
+/// A common use case is to register a shared interrupt, as sharing the line
+/// between devices is increasingly common in modern systems and is even
+/// required for some buses. This requires setting [`Flags::SHARED`] when
+/// requesting the interrupt. Other use cases include setting the trigger type
+/// through `Flags::TRIGGER_*`, which determines when the interrupt fires, or
+/// controlling whether the interrupt is masked after the handler runs by using
+/// [`Flags::ONESHOT`].
+///
+/// If an invalid combination of flags is provided, the system will refuse to
+/// register the handler, and lower layers will enforce certain flags when
+/// necessary. This means, for example, that all the
+/// [`crate::irq::Registration`] for a shared interrupt have to agree on
+/// [`Flags::SHARED`] and on the same trigger type, if set.
+#[derive(Clone, Copy, PartialEq, Eq)]
+pub struct Flags(c_ulong);
+
+impl Flags {
+    /// Use the interrupt line as already configured.
+    pub const TRIGGER_NONE: Flags = Flags::new(bindings::IRQF_TRIGGER_NONE);
+
+    /// The interrupt is triggered when the signal goes from low to high.
+    pub const TRIGGER_RISING: Flags = Flags::new(bindings::IRQF_TRIGGER_RISING);
+
+    /// The interrupt is triggered when the signal goes from high to low.
+    pub const TRIGGER_FALLING: Flags = Flags::new(bindings::IRQF_TRIGGER_FALLING);
+
+    /// The interrupt is triggered while the signal is held high.
+    pub const TRIGGER_HIGH: Flags = Flags::new(bindings::IRQF_TRIGGER_HIGH);
+
+    /// The interrupt is triggered while the signal is held low.
+    pub const TRIGGER_LOW: Flags = Flags::new(bindings::IRQF_TRIGGER_LOW);
+
+    /// Allow sharing the IRQ among several devices.
+    pub const SHARED: Flags = Flags::new(bindings::IRQF_SHARED);
+
+    /// Set by callers when they expect sharing mismatches to occur.
+    pub const PROBE_SHARED: Flags = Flags::new(bindings::IRQF_PROBE_SHARED);
+
+    /// Flag to mark this interrupt as timer interrupt.
+    pub const TIMER: Flags = Flags::new(bindings::IRQF_TIMER);
+
+    /// Interrupt is per CPU.
+    pub const PERCPU: Flags = Flags::new(bindings::IRQF_PERCPU);
+
+    /// Flag to exclude this interrupt from irq balancing.
+    pub const NOBALANCING: Flags = Flags::new(bindings::IRQF_NOBALANCING);
+
+    /// Interrupt is used for polling (only the interrupt that is registered
+    /// first in a shared interrupt is considered for performance reasons).
+    pub const IRQPOLL: Flags = Flags::new(bindings::IRQF_IRQPOLL);
+
+    /// Interrupt is not reenabled after the hardirq handler finished. Used by
+    /// threaded interrupts which need to keep the irq line disabled until the
+    /// threaded handler has been run.
+    pub const ONESHOT: Flags = Flags::new(bindings::IRQF_ONESHOT);
+
+    /// Do not disable this IRQ during suspend. Does not guarantee that this
+    /// interrupt will wake the system from a suspended state.
+    pub const NO_SUSPEND: Flags = Flags::new(bindings::IRQF_NO_SUSPEND);
+
+    /// Force enable it on resume even if [`Flags::NO_SUSPEND`] is set.
+    pub const FORCE_RESUME: Flags = Flags::new(bindings::IRQF_FORCE_RESUME);
+
+    /// Interrupt cannot be threaded.
+    pub const NO_THREAD: Flags = Flags::new(bindings::IRQF_NO_THREAD);
+
+    /// Resume IRQ early during syscore instead of at device resume time.
+    pub const EARLY_RESUME: Flags = Flags::new(bindings::IRQF_EARLY_RESUME);
+
+    /// If the IRQ is shared with a [`Flags::NO_SUSPEND`] user, execute this
+    /// interrupt handler after suspending interrupts. For system wakeup devices
+    /// users need to implement wakeup detection in their interrupt handlers.
+    pub const COND_SUSPEND: Flags = Flags::new(bindings::IRQF_COND_SUSPEND);
+
+    /// Don't enable IRQ or NMI automatically when users request it. Users will
+    /// enable it explicitly by `enable_irq` or `enable_nmi` later.
+    pub const NO_AUTOEN: Flags = Flags::new(bindings::IRQF_NO_AUTOEN);
+
+    /// Exclude from runnaway detection for IPI and similar handlers, depends on
+    /// `PERCPU`.
+    pub const NO_DEBUG: Flags = Flags::new(bindings::IRQF_NO_DEBUG);
+
+    pub(crate) fn into_inner(self) -> c_ulong {
+        self.0
+    }
+
+    const fn new(value: u32) -> Self {
+        build_assert!(value as u64 <= c_ulong::MAX as u64);
+        Self(value as c_ulong)
+    }
+}
+
+impl core::ops::BitOr for Flags {
+    type Output = Self;
+    fn bitor(self, rhs: Self) -> Self::Output {
+        Self(self.0 | rhs.0)
+    }
+}
+
+impl core::ops::BitAnd for Flags {
+    type Output = Self;
+    fn bitand(self, rhs: Self) -> Self::Output {
+        Self(self.0 & rhs.0)
+    }
+}
+
+impl core::ops::Not for Flags {
+    type Output = Self;
+    fn not(self) -> Self::Output {
+        Self(!self.0)
+    }
+}

-- 
2.50.1

[PATCH v9 3/7] rust: irq: add support for non-threaded IRQs and handlers

[Daniel Almeida]

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

[PATCH v9 4/7] rust: irq: add support for threaded IRQs and handlers

[Daniel Almeida]

This patch adds support for threaded IRQs and handlers through
irq::ThreadedRegistration and the irq::ThreadedHandler trait.

Threaded interrupts are more permissive in the sense that further
processing is possible in a kthread. This means that said execution takes
place outside of interrupt context, which is rather restrictive in many
ways.

Registering a threaded irq is dependent upon having an 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/kernel/irq.rs         |   5 +-
 rust/kernel/irq/request.rs | 228 ++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 230 insertions(+), 3 deletions(-)

diff --git a/rust/kernel/irq.rs b/rust/kernel/irq.rs
index c1019bc36ad1e7ae7dd3af8a8b5c14780bf70712..20abd40566559652eccd03f77ec873a16b6f48b0 100644
--- a/rust/kernel/irq.rs
+++ b/rust/kernel/irq.rs
@@ -18,4 +18,7 @@
 
 pub use flags::Flags;
 
-pub use request::{Handler, IrqRequest, IrqReturn, Registration};
+pub use request::{
+    Handler, IrqRequest, IrqReturn, Registration, ThreadedHandler, ThreadedIrqReturn,
+    ThreadedRegistration,
+};
diff --git a/rust/kernel/irq/request.rs b/rust/kernel/irq/request.rs
index 57e00ebf694d8e6e870d9ed57af7ee2ecf86ec05..4033df7d0dce1dbc9cc9b0b0f32fb9f8aa285d6b 100644
--- a/rust/kernel/irq/request.rs
+++ b/rust/kernel/irq/request.rs
@@ -1,8 +1,9 @@
 // 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.
+//! This module provides types like [`Registration`] and
+//! [`ThreadedRegistration`], which allow users to register handlers for a given
+//! IRQ line.
 
 use core::marker::PhantomPinned;
 
@@ -262,3 +263,226 @@ pub fn synchronize(&self, dev: &Device<Bound>) -> Result {
     let handler = unsafe { &*(ptr as *const T) };
     T::handle(handler) as c_uint
 }
+
+/// The value that can be returned from [`ThreadedHandler::handle`].
+#[repr(u32)]
+pub enum ThreadedIrqReturn {
+    /// 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,
+
+    /// The handler wants the handler thread to wake up.
+    WakeThread = bindings::irqreturn_IRQ_WAKE_THREAD,
+}
+
+/// Callbacks for a threaded IRQ handler.
+pub trait ThreadedHandler: 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 the threaded
+    /// handler, i.e. [`ThreadedHandler::handle_threaded`].
+    ///
+    /// The default implementation returns [`ThreadedIrqReturn::WakeThread`].
+    fn handle(&self) -> ThreadedIrqReturn {
+        ThreadedIrqReturn::WakeThread
+    }
+
+    /// The threaded IRQ handler.
+    ///
+    /// This is executed in process context. The kernel creates a dedicated
+    /// `kthread` for this purpose.
+    fn handle_threaded(&self) -> IrqReturn;
+}
+
+impl<T: ?Sized + ThreadedHandler + Send> ThreadedHandler for Arc<T> {
+    fn handle(&self) -> ThreadedIrqReturn {
+        T::handle(self)
+    }
+
+    fn handle_threaded(&self) -> IrqReturn {
+        T::handle_threaded(self)
+    }
+}
+
+impl<T: ?Sized + ThreadedHandler, A: Allocator> ThreadedHandler for Box<T, A> {
+    fn handle(&self) -> ThreadedIrqReturn {
+        T::handle(self)
+    }
+
+    fn handle_threaded(&self) -> IrqReturn {
+        T::handle_threaded(self)
+    }
+}
+
+/// A registration of a threaded IRQ handler for a given IRQ line.
+///
+/// Two callbacks are required: one to handle the IRQ, and one to handle any
+/// other work in a separate thread.
+///
+/// The thread handler is only called if the IRQ handler returns
+/// [`ThreadedIrqReturn::WakeThread`].
+///
+/// # Examples
+///
+/// The following is an example of using [`ThreadedRegistration`]. It uses a
+/// [`Mutex`](kernel::sync::Mutex) to provide interior mutability.
+///
+/// ```
+/// use kernel::c_str;
+/// use kernel::device::Bound;
+/// use kernel::irq::{
+///   self, Flags, IrqRequest, IrqReturn, ThreadedHandler, ThreadedIrqReturn,
+///   ThreadedRegistration,
+/// };
+/// use kernel::prelude::*;
+/// use kernel::sync::{Arc, Mutex};
+///
+/// // Declare a struct that will be passed in when the interrupt fires. The u32
+/// // merely serves as an example of some internal data.
+/// //
+/// // [`irq::ThreadedHandler::handle`] takes `&self`. This example
+/// // illustrates how interior mutability can be used when sharing the data
+/// // between process context and IRQ context.
+/// #[pin_data]
+/// struct Data {
+///     #[pin]
+///     value: Mutex<u32>,
+/// }
+///
+/// impl ThreadedHandler for Data {
+///     // This will run (in a separate kthread) if and only if
+///     // [`ThreadedHandler::handle`] returns [`WakeThread`], which it does by
+///     // default.
+///     fn handle_threaded(&self) -> IrqReturn {
+///         let mut data = self.value.lock();
+///         *data += 1;
+///         IrqReturn::Handled
+///     }
+/// }
+///
+/// // Registers a threaded IRQ handler for the given [`IrqRequest`].
+/// //
+/// // This is executing in process context and assumes that `request` was
+/// // previously acquired from a device.
+/// fn register_threaded_irq(
+///     handler: impl PinInit<Data, Error>,
+///     request: IrqRequest<'_>,
+/// ) -> Result<Arc<ThreadedRegistration<Data>>> {
+///     let registration =
+///         ThreadedRegistration::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.
+///         let mut data = registration.handler().value.lock();
+///         *data += 1;
+///     }
+///
+///     Ok(registration)
+/// }
+/// # Ok::<(), Error>(())
+/// ```
+///
+/// # Invariants
+///
+/// * We own an irq handler using `&T` as its private data.
+#[pin_data]
+pub struct ThreadedRegistration<T: ThreadedHandler + '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: ThreadedHandler + 'static> ThreadedRegistration<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 `ThreadedRegistration` instance.
+                    cookie: unsafe { &raw mut (*this.as_ptr()).handler }.cast(),
+                    irq: {
+                        // SAFETY:
+                        // - The callbacks are valid for use with request_threaded_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_threaded_irq(
+                                request.irq,
+                                Some(handle_threaded_irq_callback::<T>),
+                                Some(thread_fn_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_threaded_irq`.
+unsafe extern "C" fn handle_threaded_irq_callback<T: ThreadedHandler>(
+    _irq: i32,
+    ptr: *mut c_void,
+) -> c_uint {
+    // SAFETY: `ptr` is a pointer to T set in `ThreadedRegistration::new`
+    let handler = unsafe { &*(ptr as *const T) };
+    T::handle(handler) as c_uint
+}
+
+/// # Safety
+///
+/// This function should be only used as the callback in `request_threaded_irq`.
+unsafe extern "C" fn thread_fn_callback<T: ThreadedHandler>(_irq: i32, ptr: *mut c_void) -> c_uint {
+    // SAFETY: `ptr` is a pointer to T set in `ThreadedRegistration::new`
+    let handler = unsafe { &*(ptr as *const T) };
+    T::handle_threaded(handler) as c_uint
+}

-- 
2.50.1

[PATCH v9 5/7] rust: platform: add irq accessors

[Daniel Almeida]

These accessors can be used to retrieve a irq::Registration and
irq::ThreadedRegistration from a platform device by
index or name. Alternatively, drivers can retrieve an IrqRequest from a
bound platform device for later use.

These accessors ensure that only valid IRQ lines can ever be registered.

Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Tested-by: Joel Fernandes <joelagnelf@nvidia.com>
Tested-by: Dirk Behme <dirk.behme@de.bosch.com>
Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
---
 rust/kernel/platform.rs | 142 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 142 insertions(+)

diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs
index 8f028c76f9fa6154f440b48921ba16573a9d3c54..6d640887f69f35fc44fb3c0db3b10f879b55d107 100644
--- a/rust/kernel/platform.rs
+++ b/rust/kernel/platform.rs
@@ -10,6 +10,7 @@
     driver,
     error::{from_result, to_result, Result},
     io::{mem::IoRequest, Resource},
+    irq::{self, IrqRequest},
     of,
     prelude::*,
     types::Opaque,
@@ -284,6 +285,147 @@ pub fn io_request_by_name(&self, name: &CStr) -> Option<IoRequest<'_>> {
     }
 }
 
+macro_rules! define_irq_accessor_by_index {
+    ($(#[$meta:meta])* $fn_name:ident, $request_fn:ident, $reg_type:ident, $handler_trait:ident) => {
+        $(#[$meta])*
+        pub fn $fn_name<'a, T: irq::$handler_trait + 'static>(
+            &'a self,
+            flags: irq::Flags,
+            index: u32,
+            name: &'static CStr,
+            handler: impl PinInit<T, Error> + 'a,
+        ) -> Result<impl PinInit<irq::$reg_type<T>, Error> + 'a> {
+            let request = self.$request_fn(index)?;
+
+            Ok(irq::$reg_type::<T>::new(
+                request,
+                flags,
+                name,
+                handler,
+            ))
+        }
+    };
+}
+
+macro_rules! define_irq_accessor_by_name {
+    ($(#[$meta:meta])* $fn_name:ident, $request_fn:ident, $reg_type:ident, $handler_trait:ident) => {
+        $(#[$meta])*
+        pub fn $fn_name<'a, T: irq::$handler_trait + 'static>(
+            &'a self,
+            flags: irq::Flags,
+            irq_name: &CStr,
+            name: &'static CStr,
+            handler: impl PinInit<T, Error> + 'a,
+        ) -> Result<impl PinInit<irq::$reg_type<T>, Error> + 'a> {
+            let request = self.$request_fn(irq_name)?;
+
+            Ok(irq::$reg_type::<T>::new(
+                request,
+                flags,
+                name,
+                handler,
+            ))
+        }
+    };
+}
+
+impl Device<Bound> {
+    /// Returns an [`IrqRequest`] for the IRQ at the given index, if any.
+    pub fn irq_by_index(&self, index: u32) -> Result<IrqRequest<'_>> {
+        // SAFETY: `self.as_raw` returns a valid pointer to a `struct platform_device`.
+        let irq = unsafe { bindings::platform_get_irq(self.as_raw(), index) };
+
+        if irq < 0 {
+            return Err(Error::from_errno(irq));
+        }
+
+        // SAFETY: `irq` is guaranteed to be a valid IRQ number for `&self`.
+        Ok(unsafe { IrqRequest::new(self.as_ref(), irq as u32) })
+    }
+
+    /// Returns an [`IrqRequest`] for the IRQ at the given index, but does not
+    /// print an error if the IRQ cannot be obtained.
+    pub fn optional_irq_by_index(&self, index: u32) -> Result<IrqRequest<'_>> {
+        // SAFETY: `self.as_raw` returns a valid pointer to a `struct platform_device`.
+        let irq = unsafe { bindings::platform_get_irq_optional(self.as_raw(), index) };
+
+        if irq < 0 {
+            return Err(Error::from_errno(irq));
+        }
+
+        // SAFETY: `irq` is guaranteed to be a valid IRQ number for `&self`.
+        Ok(unsafe { IrqRequest::new(self.as_ref(), irq as u32) })
+    }
+
+    /// Returns an [`IrqRequest`] for the IRQ with the given name, if any.
+    pub fn irq_by_name(&self, name: &CStr) -> Result<IrqRequest<'_>> {
+        // SAFETY: `self.as_raw` returns a valid pointer to a `struct platform_device`.
+        let irq = unsafe { bindings::platform_get_irq_byname(self.as_raw(), name.as_char_ptr()) };
+
+        if irq < 0 {
+            return Err(Error::from_errno(irq));
+        }
+
+        // SAFETY: `irq` is guaranteed to be a valid IRQ number for `&self`.
+        Ok(unsafe { IrqRequest::new(self.as_ref(), irq as u32) })
+    }
+
+    /// Returns an [`IrqRequest`] for the IRQ with the given name, but does not
+    /// print an error if the IRQ cannot be obtained.
+    pub fn optional_irq_by_name(&self, name: &CStr) -> Result<IrqRequest<'_>> {
+        // SAFETY: `self.as_raw` returns a valid pointer to a `struct platform_device`.
+        let irq = unsafe {
+            bindings::platform_get_irq_byname_optional(self.as_raw(), name.as_char_ptr())
+        };
+
+        if irq < 0 {
+            return Err(Error::from_errno(irq));
+        }
+
+        // SAFETY: `irq` is guaranteed to be a valid IRQ number for `&self`.
+        Ok(unsafe { IrqRequest::new(self.as_ref(), irq as u32) })
+    }
+
+    define_irq_accessor_by_index!(
+        /// Returns a [`irq::Registration`] for the IRQ at the given index.
+        request_irq_by_index, irq_by_index, Registration, Handler
+    );
+    define_irq_accessor_by_name!(
+        /// Returns a [`irq::Registration`] for the IRQ with the given name.
+        request_irq_by_name, irq_by_name, Registration, Handler
+    );
+    define_irq_accessor_by_index!(
+        /// Does the same as [`Self::request_irq_by_index`], except that it does
+        /// not print an error message if the IRQ cannot be obtained.
+        request_optional_irq_by_index, optional_irq_by_index, Registration, Handler
+    );
+    define_irq_accessor_by_name!(
+        /// Does the same as [`Self::request_irq_by_name`], except that it does
+        /// not print an error message if the IRQ cannot be obtained.
+        request_optional_irq_by_name, optional_irq_by_name, Registration, Handler
+    );
+
+    define_irq_accessor_by_index!(
+        /// Returns a [`irq::ThreadedRegistration`] for the IRQ at the given index.
+        request_threaded_irq_by_index, irq_by_index, ThreadedRegistration, ThreadedHandler
+    );
+    define_irq_accessor_by_name!(
+        /// Returns a [`irq::ThreadedRegistration`] for the IRQ with the given name.
+        request_threaded_irq_by_name, irq_by_name, ThreadedRegistration, ThreadedHandler
+    );
+    define_irq_accessor_by_index!(
+        /// Does the same as [`Self::request_threaded_irq_by_index`], except
+        /// that it does not print an error message if the IRQ cannot be
+        /// obtained.
+        request_optional_threaded_irq_by_index, optional_irq_by_index, ThreadedRegistration, ThreadedHandler
+    );
+    define_irq_accessor_by_name!(
+        /// Does the same as [`Self::request_threaded_irq_by_name`], except that
+        /// it does not print an error message if the IRQ cannot be obtained.
+        request_optional_threaded_irq_by_name, optional_irq_by_name, ThreadedRegistration, ThreadedHandler
+    );
+}
+
 // SAFETY: `Device` is a transparent wrapper of a type that doesn't depend on `Device`'s generic
 // argument.
 kernel::impl_device_context_deref!(unsafe { Device });

-- 
2.50.1

[PATCH v9 6/7] rust: pci: add irq accessors

[Daniel Almeida]

These accessors can be used to retrieve a irq::Registration or a
irq::ThreadedRegistration from a pci device. Alternatively, drivers can
retrieve an IrqRequest from a bound PCI device for later use.

These accessors ensure that only valid IRQ lines can ever be registered.

Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Tested-by: Joel Fernandes <joelagnelf@nvidia.com>
Tested-by: Dirk Behme <dirk.behme@de.bosch.com>
Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
---
 rust/helpers/pci.c |  8 ++++++++
 rust/kernel/pci.rs | 45 +++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 51 insertions(+), 2 deletions(-)

diff --git a/rust/helpers/pci.c b/rust/helpers/pci.c
index ef9cb38c81a6a5375f72c3676cd9730aad17757b..5bf56004478c6945dc3e1a394fcd787c656d8b2a 100644
--- a/rust/helpers/pci.c
+++ b/rust/helpers/pci.c
@@ -11,3 +11,11 @@ bool rust_helper_dev_is_pci(const struct device *dev)
 {
 	return dev_is_pci(dev);
 }
+
+#ifndef CONFIG_PCI_MSI
+int rust_helper_pci_irq_vector(struct pci_dev *pdev, unsigned int nvec)
+{
+	return pci_irq_vector(pdev, nvec);
+}
+
+#endif
diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
index 887ee611b55310e7edbd512f9017b708ff9d7bd8..3bf1737635a9e219c3eec3f4c5f8e4e07b553b86 100644
--- a/rust/kernel/pci.rs
+++ b/rust/kernel/pci.rs
@@ -10,8 +10,8 @@
     devres::Devres,
     driver,
     error::{from_result, to_result, Result},
-    io::Io,
-    io::IoRaw,
+    io::{Io, IoRaw},
+    irq::{self, IrqRequest},
     str::CStr,
     types::{ARef, Opaque},
     ThisModule,
@@ -431,6 +431,47 @@ pub fn iomap_region<'a>(
     ) -> impl PinInit<Devres<Bar>, Error> + 'a {
         self.iomap_region_sized::<0>(bar, name)
     }
+
+    /// Returns an [`IrqRequest`] for the IRQ vector at the given index, if any.
+    pub fn irq_vector(&self, index: u32) -> Result<IrqRequest<'_>> {
+        // SAFETY: `self.as_raw` returns a valid pointer to a `struct pci_dev`.
+        let irq = unsafe { crate::bindings::pci_irq_vector(self.as_raw(), index) };
+        if irq < 0 {
+            return Err(crate::error::Error::from_errno(irq));
+        }
+        // SAFETY: `irq` is guaranteed to be a valid IRQ number for `&self`.
+        Ok(unsafe { IrqRequest::new(self.as_ref(), irq as u32) })
+    }
+
+    /// Returns a [`kernel::irq::Registration`] for the IRQ vector at the given
+    /// index.
+    pub fn request_irq<'a, T: crate::irq::Handler + 'static>(
+        &'a self,
+        index: u32,
+        flags: irq::Flags,
+        name: &'static CStr,
+        handler: impl PinInit<T, Error> + 'a,
+    ) -> Result<impl PinInit<irq::Registration<T>, Error> + 'a> {
+        let request = self.irq_vector(index)?;
+
+        Ok(irq::Registration::<T>::new(request, flags, name, handler))
+    }
+
+    /// Returns a [`kernel::irq::ThreadedRegistration`] for the IRQ vector at
+    /// the given index.
+    pub fn request_threaded_irq<'a, T: crate::irq::ThreadedHandler + 'static>(
+        &'a self,
+        index: u32,
+        flags: irq::Flags,
+        name: &'static CStr,
+        handler: impl PinInit<T, Error> + 'a,
+    ) -> Result<impl PinInit<irq::ThreadedRegistration<T>, Error> + 'a> {
+        let request = self.irq_vector(index)?;
+
+        Ok(irq::ThreadedRegistration::<T>::new(
+            request, flags, name, handler,
+        ))
+    }
 }
 
 impl Device<device::Core> {

-- 
2.50.1

[PATCH v9 7/7] rust: irq: add &Device<Bound> argument to irq callbacks

[Daniel Almeida]

From: Alice Ryhl <aliceryhl@google.com>

When working with a bus device, many operations are only possible while
the device is still bound. The &Device<Bound> type represents a proof in
the type system that you are in a scope where the device is guaranteed
to still be bound. Since we deregister irq callbacks when unbinding a
device, if an irq callback is running, that implies that the device has
not yet been unbound.

To allow drivers to take advantage of that, add an additional argument
to irq callbacks.

Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
 rust/kernel/irq/request.rs | 95 +++++++++++++++++++++++++++-------------------
 1 file changed, 57 insertions(+), 38 deletions(-)

diff --git a/rust/kernel/irq/request.rs b/rust/kernel/irq/request.rs
index 4033df7d0dce1dbc9cc9b0b0f32fb9f8aa285d6b..b150563fdef809899c7ca39221c4928238e31110 100644
--- a/rust/kernel/irq/request.rs
+++ b/rust/kernel/irq/request.rs
@@ -36,18 +36,18 @@ pub trait Handler: Sync {
     /// 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;
+    fn handle(&self, device: &Device<Bound>) -> IrqReturn;
 }
 
 impl<T: ?Sized + Handler + Send> Handler for Arc<T> {
-    fn handle(&self) -> IrqReturn {
-        T::handle(self)
+    fn handle(&self, device: &Device<Bound>) -> IrqReturn {
+        T::handle(self, device)
     }
 }
 
 impl<T: ?Sized + Handler, A: Allocator> Handler for Box<T, A> {
-    fn handle(&self) -> IrqReturn {
-        T::handle(self)
+    fn handle(&self, device: &Device<Bound>) -> IrqReturn {
+        T::handle(self, device)
     }
 }
 
@@ -140,7 +140,7 @@ pub fn irq(&self) -> u32 {
 ///
 /// ```
 /// use kernel::c_str;
-/// use kernel::device::Bound;
+/// use kernel::device::{Bound, Device};
 /// use kernel::irq::{self, Flags, IrqRequest, IrqReturn, Registration};
 /// use kernel::prelude::*;
 /// use kernel::sync::{Arc, Completion};
@@ -154,7 +154,7 @@ pub fn irq(&self) -> u32 {
 ///
 /// impl irq::Handler for Data {
 ///     // Executed in IRQ context.
-///     fn handle(&self) -> IrqReturn {
+///     fn handle(&self, _dev: &Device<Bound>) -> IrqReturn {
 ///         self.completion.complete_all();
 ///         IrqReturn::Handled
 ///     }
@@ -180,7 +180,7 @@ pub fn irq(&self) -> u32 {
 ///
 /// # Invariants
 ///
-/// * We own an irq handler using `&self.handler` as its private data.
+/// * We own an irq handler whose cookie is a pointer to `Self`.
 #[pin_data]
 pub struct Registration<T: Handler + 'static> {
     #[pin]
@@ -208,21 +208,24 @@ pub fn new<'a>(
             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(),
+                    // INVARIANT: `this` is a valid pointer to the `Registration` instance
+                    cookie: this.as_ptr().cast::<c_void>(),
                     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.
+                        // - When request_irq is called, everything that handle_irq_callback will
+                        //   touch has already been initialized, so it's safe for the callback to
+                        //   be called immediately.
                         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(),
+                                this.as_ptr().cast::<c_void>(),
                             )
                         })?;
                         request.irq
@@ -259,9 +262,13 @@ pub fn synchronize(&self, dev: &Device<Bound>) -> Result {
 ///
 /// 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
+    // SAFETY: `ptr` is a pointer to `Registration<T>` set in `Registration::new`
+    let registration = unsafe { &*(ptr as *const Registration<T>) };
+    // SAFETY: The irq callback is removed before the device is unbound, so the fact that the irq
+    // callback is running implies that the device has not yet been unbound.
+    let device = unsafe { registration.inner.device().as_bound() };
+
+    T::handle(&registration.handler, device) as c_uint
 }
 
 /// The value that can be returned from [`ThreadedHandler::handle`].
@@ -287,7 +294,8 @@ pub trait ThreadedHandler: Sync {
     /// handler, i.e. [`ThreadedHandler::handle_threaded`].
     ///
     /// The default implementation returns [`ThreadedIrqReturn::WakeThread`].
-    fn handle(&self) -> ThreadedIrqReturn {
+    #[expect(unused_variables)]
+    fn handle(&self, device: &Device<Bound>) -> ThreadedIrqReturn {
         ThreadedIrqReturn::WakeThread
     }
 
@@ -295,26 +303,26 @@ fn handle(&self) -> ThreadedIrqReturn {
     ///
     /// This is executed in process context. The kernel creates a dedicated
     /// `kthread` for this purpose.
-    fn handle_threaded(&self) -> IrqReturn;
+    fn handle_threaded(&self, device: &Device<Bound>) -> IrqReturn;
 }
 
 impl<T: ?Sized + ThreadedHandler + Send> ThreadedHandler for Arc<T> {
-    fn handle(&self) -> ThreadedIrqReturn {
-        T::handle(self)
+    fn handle(&self, device: &Device<Bound>) -> ThreadedIrqReturn {
+        T::handle(self, device)
     }
 
-    fn handle_threaded(&self) -> IrqReturn {
-        T::handle_threaded(self)
+    fn handle_threaded(&self, device: &Device<Bound>) -> IrqReturn {
+        T::handle_threaded(self, device)
     }
 }
 
 impl<T: ?Sized + ThreadedHandler, A: Allocator> ThreadedHandler for Box<T, A> {
-    fn handle(&self) -> ThreadedIrqReturn {
-        T::handle(self)
+    fn handle(&self, device: &Device<Bound>) -> ThreadedIrqReturn {
+        T::handle(self, device)
     }
 
-    fn handle_threaded(&self) -> IrqReturn {
-        T::handle_threaded(self)
+    fn handle_threaded(&self, device: &Device<Bound>) -> IrqReturn {
+        T::handle_threaded(self, device)
     }
 }
 
@@ -333,7 +341,7 @@ fn handle_threaded(&self) -> IrqReturn {
 ///
 /// ```
 /// use kernel::c_str;
-/// use kernel::device::Bound;
+/// use kernel::device::{Bound, Device};
 /// use kernel::irq::{
 ///   self, Flags, IrqRequest, IrqReturn, ThreadedHandler, ThreadedIrqReturn,
 ///   ThreadedRegistration,
@@ -357,7 +365,7 @@ fn handle_threaded(&self) -> IrqReturn {
 ///     // This will run (in a separate kthread) if and only if
 ///     // [`ThreadedHandler::handle`] returns [`WakeThread`], which it does by
 ///     // default.
-///     fn handle_threaded(&self) -> IrqReturn {
+///     fn handle_threaded(&self, _dev: &Device<Bound>) -> IrqReturn {
 ///         let mut data = self.value.lock();
 ///         *data += 1;
 ///         IrqReturn::Handled
@@ -390,7 +398,7 @@ fn handle_threaded(&self) -> IrqReturn {
 ///
 /// # Invariants
 ///
-/// * We own an irq handler using `&T` as its private data.
+/// * We own an irq handler whose cookie is a pointer to `Self`.
 #[pin_data]
 pub struct ThreadedRegistration<T: ThreadedHandler + 'static> {
     #[pin]
@@ -418,14 +426,17 @@ pub fn new<'a>(
             inner <- Devres::new(
                 request.dev,
                 try_pin_init!(RegistrationInner {
-                    // SAFETY: `this` is a valid pointer to the `ThreadedRegistration` instance.
-                    cookie: unsafe { &raw mut (*this.as_ptr()).handler }.cast(),
+                    // INVARIANT: `this` is a valid pointer to the `ThreadedRegistration` instance.
+                    cookie: this.as_ptr().cast::<c_void>(),
                     irq: {
                         // SAFETY:
                         // - The callbacks are valid for use with request_threaded_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.
+                        //   destructor of Self runs, which will deregister the callbacks
+                        //   before the memory location becomes invalid.
+                        // - When request_threaded_irq is called, everything that the two callbacks
+                        //   will touch has already been initialized, so it's safe for the
+                        //   callbacks to be called immediately.
                         to_result(unsafe {
                             bindings::request_threaded_irq(
                                 request.irq,
@@ -433,7 +444,7 @@ pub fn new<'a>(
                                 Some(thread_fn_callback::<T>),
                                 flags.into_inner(),
                                 name.as_char_ptr(),
-                                (&raw mut (*this.as_ptr()).handler).cast(),
+                                this.as_ptr().cast::<c_void>(),
                             )
                         })?;
                         request.irq
@@ -473,16 +484,24 @@ pub fn synchronize(&self, dev: &Device<Bound>) -> Result {
     _irq: i32,
     ptr: *mut c_void,
 ) -> c_uint {
-    // SAFETY: `ptr` is a pointer to T set in `ThreadedRegistration::new`
-    let handler = unsafe { &*(ptr as *const T) };
-    T::handle(handler) as c_uint
+    // SAFETY: `ptr` is a pointer to `ThreadedRegistration<T>` set in `ThreadedRegistration::new`
+    let registration = unsafe { &*(ptr as *const ThreadedRegistration<T>) };
+    // SAFETY: The irq callback is removed before the device is unbound, so the fact that the irq
+    // callback is running implies that the device has not yet been unbound.
+    let device = unsafe { registration.inner.device().as_bound() };
+
+    T::handle(&registration.handler, device) as c_uint
 }
 
 /// # Safety
 ///
 /// This function should be only used as the callback in `request_threaded_irq`.
 unsafe extern "C" fn thread_fn_callback<T: ThreadedHandler>(_irq: i32, ptr: *mut c_void) -> c_uint {
-    // SAFETY: `ptr` is a pointer to T set in `ThreadedRegistration::new`
-    let handler = unsafe { &*(ptr as *const T) };
-    T::handle_threaded(handler) as c_uint
+    // SAFETY: `ptr` is a pointer to `ThreadedRegistration<T>` set in `ThreadedRegistration::new`
+    let registration = unsafe { &*(ptr as *const ThreadedRegistration<T>) };
+    // SAFETY: The irq callback is removed before the device is unbound, so the fact that the irq
+    // callback is running implies that the device has not yet been unbound.
+    let device = unsafe { registration.inner.device().as_bound() };
+
+    T::handle_threaded(&registration.handler, device) as c_uint
 }

-- 
2.50.1

Re: [PATCH v9 7/7] rust: irq: add &Device<Bound> argument to irq callbacks

[Daniel Almeida]

> On 11 Aug 2025, at 13:03, Daniel Almeida <daniel.almeida@collabora.com> wrote:
> 
> From: Alice Ryhl <aliceryhl@google.com>
> 
> When working with a bus device, many operations are only possible while
> the device is still bound. The &Device<Bound> type represents a proof in
> the type system that you are in a scope where the device is guaranteed
> to still be bound. Since we deregister irq callbacks when unbinding a
> device, if an irq callback is running, that implies that the device has
> not yet been unbound.
> 
> To allow drivers to take advantage of that, add an additional argument
> to irq callbacks.
> 
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>

Sorry. I forgot to add my SOB here.


Perhaps this can be added when the patch is being applied in order to cut down on the
number of versions, and therefore avoid the extra noise? Otherwise let me know.

Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>

Re: [PATCH v9 7/7] rust: irq: add &Device<Bound> argument to irq callbacks

[Boqun Feng]

On Mon, Aug 11, 2025 at 02:00:47PM -0300, Daniel Almeida wrote:
> 
> 
> > On 11 Aug 2025, at 13:03, Daniel Almeida <daniel.almeida@collabora.com> wrote:
> > 
> > From: Alice Ryhl <aliceryhl@google.com>
> > 
> > When working with a bus device, many operations are only possible while
> > the device is still bound. The &Device<Bound> type represents a proof in
> > the type system that you are in a scope where the device is guaranteed
> > to still be bound. Since we deregister irq callbacks when unbinding a
> > device, if an irq callback is running, that implies that the device has
> > not yet been unbound.
> > 
> > To allow drivers to take advantage of that, add an additional argument
> > to irq callbacks.
> > 
> > Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> 
> Sorry. I forgot to add my SOB here.
> 
> 
> Perhaps this can be added when the patch is being applied in order to cut down on the
> number of versions, and therefore avoid the extra noise? Otherwise let me know.
> 

I think it's fine to submit with only Alice's SoB, my understanding is
that you won't necessarily need to add your SoB if you are simply
re-submitting a patch (with minor changes). There are changes where your
SoB is needed: 1) you change the code significantly, in which case, you
may also need to add "Co-Developed-by" for Alice as well; 2) you're
submitting the patch as a maintainer, and you have queued that patch
already somewhere in your tree, in this case SoB shows how the patch
flows around. Of course, either case it's better to sync with Alice
first, which I believe you have already done that.

While I'm at it,

Reviewed-by: Boqun Feng <boqun.feng@gmail.com>

Regards,
Boqun

> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
> 
> 

Re: [PATCH v9 7/7] rust: irq: add &Device<Bound> argument to irq callbacks

[Danilo Krummrich]

On Mon Aug 11, 2025 at 7:00 PM CEST, Daniel Almeida wrote:
>
>
>> On 11 Aug 2025, at 13:03, Daniel Almeida <daniel.almeida@collabora.com> wrote:
>> 
>> From: Alice Ryhl <aliceryhl@google.com>
>> 
>> When working with a bus device, many operations are only possible while
>> the device is still bound. The &Device<Bound> type represents a proof in
>> the type system that you are in a scope where the device is guaranteed
>> to still be bound. Since we deregister irq callbacks when unbinding a
>> device, if an irq callback is running, that implies that the device has
>> not yet been unbound.
>> 
>> To allow drivers to take advantage of that, add an additional argument
>> to irq callbacks.
>> 
>> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
>
> Sorry. I forgot to add my SOB here.
>
>
> Perhaps this can be added when the patch is being applied in order to cut down on the
> number of versions, and therefore avoid the extra noise? Otherwise let me know.
>
> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>

Sure -- no need to resubmit AFAIC.

Re: [PATCH v9 7/7] rust: irq: add &Device<Bound> argument to irq callbacks

[Danilo Krummrich]

On Mon Aug 11, 2025 at 7:11 PM CEST, Boqun Feng wrote:
> I think it's fine to submit with only Alice's SoB, my understanding is
> that you won't necessarily need to add your SoB if you are simply
> re-submitting a patch (with minor changes).

I think it is required for everyone who is handling or transporting the patch in
any way, documenting the exact route a patch has taking while conforming with
the developer’s certificate of origin.

Re: [PATCH v9 0/7] rust: add support for request_irq

[Danilo Krummrich]

On Mon Aug 11, 2025 at 6:03 PM CEST, Daniel Almeida wrote:

Applied to driver-core-testing, thanks!

> Alice Ryhl (1):
>       rust: irq: add &Device<Bound> argument to irq callbacks
>
> Daniel Almeida (6):
>       rust: irq: add irq module
>       rust: irq: add flags module

    [ Use expect(dead_code) for into_inner(), fix broken intra-doc link and
      typo. - Danilo ]

>       rust: irq: add support for non-threaded IRQs and handlers

    [ Remove expect(dead_code) from Flags::into_inner(), add
      expect(dead_code) to IrqRequest::new(), fix intra-doc links. - Danilo ]

>       rust: irq: add support for threaded IRQs and handlers

    [ Add now available intra-doc links back in. - Danilo ]

>       rust: platform: add irq accessors

    [ Remove expect(dead_code) from IrqRequest::new(), re-format macros and
      macro invocations to not exceed 100 characters line length. - Danilo ]

>       rust: pci: add irq accessors

Re: [PATCH v9 0/7] rust: add support for request_irq

[Daniel Almeida]

Hi Danilo,

> On 12 Aug 2025, at 16:07, Danilo Krummrich <dakr@kernel.org> wrote:
> 
> On Mon Aug 11, 2025 at 6:03 PM CEST, Daniel Almeida wrote:
> 
> Applied to driver-core-testing, thanks!

Awesome, thanks everyone involved here :)

> 
>> Alice Ryhl (1):
>>      rust: irq: add &Device<Bound> argument to irq callbacks
>> 
>> Daniel Almeida (6):
>>      rust: irq: add irq module
>>      rust: irq: add flags module
> 
>    [ Use expect(dead_code) for into_inner(), fix broken intra-doc link and
>      typo. - Danilo ]

Right, I’ll consider inter-patch issues like this in the future, sorry.

[…]

> 
>    [ Remove expect(dead_code) from IrqRequest::new(), re-format macros and
>      macro invocations to not exceed 100 characters line length. - Danilo ]
> 
>>      rust: pci: add irq accessors
> 

How? rustfmt doesn’t format this, so I assume that you just manually
wrapped the lines in a suitable way?

— Daniel

Re: [PATCH v9 0/7] rust: add support for request_irq

[Danilo Krummrich]

On Tue Aug 12, 2025 at 9:17 PM CEST, Daniel Almeida wrote:
> How? rustfmt doesn’t format this, so I assume that you just manually
> wrapped the lines in a suitable way?

Yes, exactly.

Re: [PATCH v9 2/7] rust: irq: add flags module

[Andreas Hindborg]

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

> Manipulating IRQ flags (i.e.: IRQF_*) will soon be necessary, specially to
> register IRQ handlers through bindings::request_irq().
>
> Add a kernel::irq::Flags for that purpose.
>
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> Tested-by: Joel Fernandes <joelagnelf@nvidia.com>
> Tested-by: Dirk Behme <dirk.behme@de.bosch.com>
> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
> ---
>  rust/kernel/irq.rs       |   5 ++
>  rust/kernel/irq/flags.rs | 124 +++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 129 insertions(+)
>
> diff --git a/rust/kernel/irq.rs b/rust/kernel/irq.rs
> index fae7b15effc80c936d6bffbd5b4150000d6c2898..068df2fea31de51115c30344f7ebdb4da4ad86cc 100644
> --- a/rust/kernel/irq.rs
> +++ b/rust/kernel/irq.rs
> @@ -9,3 +9,8 @@
>  //! drivers to register a handler for a given IRQ line.
>  //!
>  //! C header: [`include/linux/device.h`](srctree/include/linux/interrupt.h)
> +
> +/// Flags to be used when registering IRQ handlers.
> +mod flags;
> +
> +pub use flags::Flags;
> diff --git a/rust/kernel/irq/flags.rs b/rust/kernel/irq/flags.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..e62820ea67755123b4f96e4331244bbb4fbcfd9d
> --- /dev/null
> +++ b/rust/kernel/irq/flags.rs
> @@ -0,0 +1,124 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// SPDX-FileCopyrightText: Copyright 2025 Collabora ltd.
> +
> +use crate::bindings;
> +use crate::prelude::*;
> +
> +/// Flags to be used when registering IRQ handlers.
> +///
> +/// Flags can be used to request specific behaviors when registering an IRQ
> +/// handler, and can be combined using the `|`, `&`, and `!` operators to
> +/// further control the system's behavior.
> +///
> +/// A common use case is to register a shared interrupt, as sharing the line
> +/// between devices is increasingly common in modern systems and is even
> +/// required for some buses. This requires setting [`Flags::SHARED`] when
> +/// requesting the interrupt. Other use cases include setting the trigger type
> +/// through `Flags::TRIGGER_*`, which determines when the interrupt fires, or
> +/// controlling whether the interrupt is masked after the handler runs by using
> +/// [`Flags::ONESHOT`].
> +///
> +/// If an invalid combination of flags is provided, the system will refuse to
> +/// register the handler, and lower layers will enforce certain flags when
> +/// necessary. This means, for example, that all the
> +/// [`crate::irq::Registration`] for a shared interrupt have to agree on

`rustdoc` will complain about this being undefined.

> +/// [`Flags::SHARED`] and on the same trigger type, if set.
> +#[derive(Clone, Copy, PartialEq, Eq)]
> +pub struct Flags(c_ulong);
> +
> +impl Flags {
> +    /// Use the interrupt line as already configured.
> +    pub const TRIGGER_NONE: Flags = Flags::new(bindings::IRQF_TRIGGER_NONE);
> +
> +    /// The interrupt is triggered when the signal goes from low to high.
> +    pub const TRIGGER_RISING: Flags = Flags::new(bindings::IRQF_TRIGGER_RISING);
> +
> +    /// The interrupt is triggered when the signal goes from high to low.
> +    pub const TRIGGER_FALLING: Flags = Flags::new(bindings::IRQF_TRIGGER_FALLING);
> +
> +    /// The interrupt is triggered while the signal is held high.
> +    pub const TRIGGER_HIGH: Flags = Flags::new(bindings::IRQF_TRIGGER_HIGH);
> +
> +    /// The interrupt is triggered while the signal is held low.
> +    pub const TRIGGER_LOW: Flags = Flags::new(bindings::IRQF_TRIGGER_LOW);
> +
> +    /// Allow sharing the IRQ among several devices.
> +    pub const SHARED: Flags = Flags::new(bindings::IRQF_SHARED);
> +
> +    /// Set by callers when they expect sharing mismatches to occur.
> +    pub const PROBE_SHARED: Flags = Flags::new(bindings::IRQF_PROBE_SHARED);
> +
> +    /// Flag to mark this interrupt as timer interrupt.
> +    pub const TIMER: Flags = Flags::new(bindings::IRQF_TIMER);
> +
> +    /// Interrupt is per CPU.
> +    pub const PERCPU: Flags = Flags::new(bindings::IRQF_PERCPU);
> +
> +    /// Flag to exclude this interrupt from irq balancing.
> +    pub const NOBALANCING: Flags = Flags::new(bindings::IRQF_NOBALANCING);
> +
> +    /// Interrupt is used for polling (only the interrupt that is registered
> +    /// first in a shared interrupt is considered for performance reasons).
> +    pub const IRQPOLL: Flags = Flags::new(bindings::IRQF_IRQPOLL);
> +
> +    /// Interrupt is not reenabled after the hardirq handler finished. Used by
> +    /// threaded interrupts which need to keep the irq line disabled until the
> +    /// threaded handler has been run.
> +    pub const ONESHOT: Flags = Flags::new(bindings::IRQF_ONESHOT);
> +
> +    /// Do not disable this IRQ during suspend. Does not guarantee that this
> +    /// interrupt will wake the system from a suspended state.
> +    pub const NO_SUSPEND: Flags = Flags::new(bindings::IRQF_NO_SUSPEND);
> +
> +    /// Force enable it on resume even if [`Flags::NO_SUSPEND`] is set.
> +    pub const FORCE_RESUME: Flags = Flags::new(bindings::IRQF_FORCE_RESUME);
> +
> +    /// Interrupt cannot be threaded.
> +    pub const NO_THREAD: Flags = Flags::new(bindings::IRQF_NO_THREAD);
> +
> +    /// Resume IRQ early during syscore instead of at device resume time.
> +    pub const EARLY_RESUME: Flags = Flags::new(bindings::IRQF_EARLY_RESUME);
> +
> +    /// If the IRQ is shared with a [`Flags::NO_SUSPEND`] user, execute this
> +    /// interrupt handler after suspending interrupts. For system wakeup devices
> +    /// users need to implement wakeup detection in their interrupt handlers.
> +    pub const COND_SUSPEND: Flags = Flags::new(bindings::IRQF_COND_SUSPEND);
> +
> +    /// Don't enable IRQ or NMI automatically when users request it. Users will
> +    /// enable it explicitly by `enable_irq` or `enable_nmi` later.
> +    pub const NO_AUTOEN: Flags = Flags::new(bindings::IRQF_NO_AUTOEN);
> +
> +    /// Exclude from runnaway detection for IPI and similar handlers, depends on
> +    /// `PERCPU`.

Should we link `PERCPU` here?

> +    pub const NO_DEBUG: Flags = Flags::new(bindings::IRQF_NO_DEBUG);
> +
> +    pub(crate) fn into_inner(self) -> c_ulong {

You need `#[expect(dead_code)]` here.

> +        self.0
> +    }
> +
> +    const fn new(value: u32) -> Self {
> +        build_assert!(value as u64 <= c_ulong::MAX as u64);

I am curious about this line. Can you add a short explanation?

I would think this can never assert. That would require c_ulong to be
less than 32 bits, right? Are there any configurations where that is the case?


Best regards,
Andreas Hindborg

Re: [PATCH v9 1/7] rust: irq: add irq module

[Andreas Hindborg]

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

> Add the IRQ module. Future patches will then introduce support for IRQ
> registrations and handlers.
>
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> Tested-by: Joel Fernandes <joelagnelf@nvidia.com>
> Tested-by: Dirk Behme <dirk.behme@de.bosch.com>
> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>

Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>


Best regards,
Andreas Hindborg

Re: [PATCH v9 2/7] rust: irq: add flags module

[Miguel Ojeda]

On Fri, Aug 15, 2025 at 2:07 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> I would think this can never assert. That would require c_ulong to be
> less than 32 bits, right? Are there any configurations where that is the case?

`long` is at least 4 bytes and could potentially be 16 in the future.

Cheers,
Miguel

Re: [PATCH v9 2/7] rust: irq: add flags module

[Daniel Almeida]

> On 15 Aug 2025, at 09:02, Andreas Hindborg <a.hindborg@kernel.org> wrote:
> 
> "Daniel Almeida" <daniel.almeida@collabora.com> writes:
> 
>> Manipulating IRQ flags (i.e.: IRQF_*) will soon be necessary, specially to
>> register IRQ handlers through bindings::request_irq().
>> 
>> Add a kernel::irq::Flags for that purpose.
>> 
>> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
>> Tested-by: Joel Fernandes <joelagnelf@nvidia.com>
>> Tested-by: Dirk Behme <dirk.behme@de.bosch.com>
>> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
>> ---
>> rust/kernel/irq.rs       |   5 ++
>> rust/kernel/irq/flags.rs | 124 +++++++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 129 insertions(+)
>> 
>> diff --git a/rust/kernel/irq.rs b/rust/kernel/irq.rs
>> index fae7b15effc80c936d6bffbd5b4150000d6c2898..068df2fea31de51115c30344f7ebdb4da4ad86cc 100644
>> --- a/rust/kernel/irq.rs
>> +++ b/rust/kernel/irq.rs
>> @@ -9,3 +9,8 @@
>> //! drivers to register a handler for a given IRQ line.
>> //!
>> //! C header: [`include/linux/device.h`](srctree/include/linux/interrupt.h)
>> +
>> +/// Flags to be used when registering IRQ handlers.
>> +mod flags;
>> +
>> +pub use flags::Flags;
>> diff --git a/rust/kernel/irq/flags.rs b/rust/kernel/irq/flags.rs
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..e62820ea67755123b4f96e4331244bbb4fbcfd9d
>> --- /dev/null
>> +++ b/rust/kernel/irq/flags.rs
>> @@ -0,0 +1,124 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// SPDX-FileCopyrightText: Copyright 2025 Collabora ltd.
>> +
>> +use crate::bindings;
>> +use crate::prelude::*;
>> +
>> +/// Flags to be used when registering IRQ handlers.
>> +///
>> +/// Flags can be used to request specific behaviors when registering an IRQ
>> +/// handler, and can be combined using the `|`, `&`, and `!` operators to
>> +/// further control the system's behavior.
>> +///
>> +/// A common use case is to register a shared interrupt, as sharing the line
>> +/// between devices is increasingly common in modern systems and is even
>> +/// required for some buses. This requires setting [`Flags::SHARED`] when
>> +/// requesting the interrupt. Other use cases include setting the trigger type
>> +/// through `Flags::TRIGGER_*`, which determines when the interrupt fires, or
>> +/// controlling whether the interrupt is masked after the handler runs by using
>> +/// [`Flags::ONESHOT`].
>> +///
>> +/// If an invalid combination of flags is provided, the system will refuse to
>> +/// register the handler, and lower layers will enforce certain flags when
>> +/// necessary. This means, for example, that all the
>> +/// [`crate::irq::Registration`] for a shared interrupt have to agree on
> 
> `rustdoc` will complain about this being undefined.

I think Danilo fixed this before applying. As I said a few days ago, I just ran
rustdoc on the final result, not on the individual commits. My bad, I’ll
pay attention to this next time.

> 
>> +/// [`Flags::SHARED`] and on the same trigger type, if set.
>> +#[derive(Clone, Copy, PartialEq, Eq)]
>> +pub struct Flags(c_ulong);
>> +
>> +impl Flags {
>> +    /// Use the interrupt line as already configured.
>> +    pub const TRIGGER_NONE: Flags = Flags::new(bindings::IRQF_TRIGGER_NONE);
>> +
>> +    /// The interrupt is triggered when the signal goes from low to high.
>> +    pub const TRIGGER_RISING: Flags = Flags::new(bindings::IRQF_TRIGGER_RISING);
>> +
>> +    /// The interrupt is triggered when the signal goes from high to low.
>> +    pub const TRIGGER_FALLING: Flags = Flags::new(bindings::IRQF_TRIGGER_FALLING);
>> +
>> +    /// The interrupt is triggered while the signal is held high.
>> +    pub const TRIGGER_HIGH: Flags = Flags::new(bindings::IRQF_TRIGGER_HIGH);
>> +
>> +    /// The interrupt is triggered while the signal is held low.
>> +    pub const TRIGGER_LOW: Flags = Flags::new(bindings::IRQF_TRIGGER_LOW);
>> +
>> +    /// Allow sharing the IRQ among several devices.
>> +    pub const SHARED: Flags = Flags::new(bindings::IRQF_SHARED);
>> +
>> +    /// Set by callers when they expect sharing mismatches to occur.
>> +    pub const PROBE_SHARED: Flags = Flags::new(bindings::IRQF_PROBE_SHARED);
>> +
>> +    /// Flag to mark this interrupt as timer interrupt.
>> +    pub const TIMER: Flags = Flags::new(bindings::IRQF_TIMER);
>> +
>> +    /// Interrupt is per CPU.
>> +    pub const PERCPU: Flags = Flags::new(bindings::IRQF_PERCPU);
>> +
>> +    /// Flag to exclude this interrupt from irq balancing.
>> +    pub const NOBALANCING: Flags = Flags::new(bindings::IRQF_NOBALANCING);
>> +
>> +    /// Interrupt is used for polling (only the interrupt that is registered
>> +    /// first in a shared interrupt is considered for performance reasons).
>> +    pub const IRQPOLL: Flags = Flags::new(bindings::IRQF_IRQPOLL);
>> +
>> +    /// Interrupt is not reenabled after the hardirq handler finished. Used by
>> +    /// threaded interrupts which need to keep the irq line disabled until the
>> +    /// threaded handler has been run.
>> +    pub const ONESHOT: Flags = Flags::new(bindings::IRQF_ONESHOT);
>> +
>> +    /// Do not disable this IRQ during suspend. Does not guarantee that this
>> +    /// interrupt will wake the system from a suspended state.
>> +    pub const NO_SUSPEND: Flags = Flags::new(bindings::IRQF_NO_SUSPEND);
>> +
>> +    /// Force enable it on resume even if [`Flags::NO_SUSPEND`] is set.
>> +    pub const FORCE_RESUME: Flags = Flags::new(bindings::IRQF_FORCE_RESUME);
>> +
>> +    /// Interrupt cannot be threaded.
>> +    pub const NO_THREAD: Flags = Flags::new(bindings::IRQF_NO_THREAD);
>> +
>> +    /// Resume IRQ early during syscore instead of at device resume time.
>> +    pub const EARLY_RESUME: Flags = Flags::new(bindings::IRQF_EARLY_RESUME);
>> +
>> +    /// If the IRQ is shared with a [`Flags::NO_SUSPEND`] user, execute this
>> +    /// interrupt handler after suspending interrupts. For system wakeup devices
>> +    /// users need to implement wakeup detection in their interrupt handlers.
>> +    pub const COND_SUSPEND: Flags = Flags::new(bindings::IRQF_COND_SUSPEND);
>> +
>> +    /// Don't enable IRQ or NMI automatically when users request it. Users will
>> +    /// enable it explicitly by `enable_irq` or `enable_nmi` later.
>> +    pub const NO_AUTOEN: Flags = Flags::new(bindings::IRQF_NO_AUTOEN);
>> +
>> +    /// Exclude from runnaway detection for IPI and similar handlers, depends on
>> +    /// `PERCPU`.
> 
> Should we link `PERCPU` here?
> 
>> +    pub const NO_DEBUG: Flags = Flags::new(bindings::IRQF_NO_DEBUG);
>> +
>> +    pub(crate) fn into_inner(self) -> c_ulong {
> 
> You need `#[expect(dead_code)]` here.
> 
>> +        self.0
>> +    }
>> +
>> +    const fn new(value: u32) -> Self {
>> +        build_assert!(value as u64 <= c_ulong::MAX as u64);
> 
> I am curious about this line. Can you add a short explanation?
> 
> I would think this can never assert. That would require c_ulong to be
> less than 32 bits, right? Are there any configurations where that is the case?

This is just a way to validate the cast at build time. IMHO, regardless of
whether this can possibly trigger, it's always nice to err on the side of
caution, specially because this type doesn't have a fixed bit width.

> 
> 
> Best regards,
> Andreas Hindborg

Re: [PATCH v9 2/7] rust: irq: add flags module

[Miguel Ojeda]

On Fri, Aug 15, 2025 at 3:24 PM Daniel Almeida
<daniel.almeida@collabora.com> wrote:
>
> This is just a way to validate the cast at build time. IMHO, regardless of
> whether this can possibly trigger, it's always nice to err on the side of
> caution, specially because this type doesn't have a fixed bit width.

But this could be an `static_assert!` -- no need to use the `value`, no?

In fact, we could have this cast provided somewhere, like `.into()`,
since it will always work as expected within the kernel.

Cheers,
Miguel

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

[Andreas Hindborg]

"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

Re: [PATCH v9 0/7] rust: add support for request_irq

[Andreas Hindborg]

"Danilo Krummrich" <dakr@kernel.org> writes:

> On Mon Aug 11, 2025 at 6:03 PM CEST, Daniel Almeida wrote:
>
> Applied to driver-core-testing, thanks!
>
>> Alice Ryhl (1):
>>       rust: irq: add &Device<Bound> argument to irq callbacks
>>
>> Daniel Almeida (6):
>>       rust: irq: add irq module
>>       rust: irq: add flags module
>
>     [ Use expect(dead_code) for into_inner(), fix broken intra-doc link and
>       typo. - Danilo ]
>
>>       rust: irq: add support for non-threaded IRQs and handlers
>
>     [ Remove expect(dead_code) from Flags::into_inner(), add
>       expect(dead_code) to IrqRequest::new(), fix intra-doc links. - Danilo ]
>
>>       rust: irq: add support for threaded IRQs and handlers
>
>     [ Add now available intra-doc links back in. - Danilo ]
>
>>       rust: platform: add irq accessors
>
>     [ Remove expect(dead_code) from IrqRequest::new(), re-format macros and
>       macro invocations to not exceed 100 characters line length. - Danilo ]
>
>>       rust: pci: add irq accessors

I somehow missed that you already applied this, so I just sent comments
for patch 3. I think there are some issues. If you want my comments for
the rest of the series, let me know. Otherwise I'll just skip that.


Best regards,
Andreas Hindborg

Re: [PATCH v9 0/7] rust: add support for request_irq

[Danilo Krummrich]

On 8/18/25 10:23 AM, Andreas Hindborg wrote:
> "Danilo Krummrich" <dakr@kernel.org> writes:
> 
>> On Mon Aug 11, 2025 at 6:03 PM CEST, Daniel Almeida wrote:
>>
>> Applied to driver-core-testing, thanks!
>>
>>> Alice Ryhl (1):
>>>        rust: irq: add &Device<Bound> argument to irq callbacks
>>>
>>> Daniel Almeida (6):
>>>        rust: irq: add irq module
>>>        rust: irq: add flags module
>>
>>      [ Use expect(dead_code) for into_inner(), fix broken intra-doc link and
>>        typo. - Danilo ]
>>
>>>        rust: irq: add support for non-threaded IRQs and handlers
>>
>>      [ Remove expect(dead_code) from Flags::into_inner(), add
>>        expect(dead_code) to IrqRequest::new(), fix intra-doc links. - Danilo ]
>>
>>>        rust: irq: add support for threaded IRQs and handlers
>>
>>      [ Add now available intra-doc links back in. - Danilo ]
>>
>>>        rust: platform: add irq accessors
>>
>>      [ Remove expect(dead_code) from IrqRequest::new(), re-format macros and
>>        macro invocations to not exceed 100 characters line length. - Danilo ]
>>
>>>        rust: pci: add irq accessors
> 
> I somehow missed that you already applied this, so I just sent comments
> for patch 3. I think there are some issues. If you want my comments for
> the rest of the series, let me know. Otherwise I'll just skip that.

Sure, please do -- I'm sure Daniel is willing to send patches for any
improvements that may arise.

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

[Daniel Almeida]

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

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

[Daniel Almeida]

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

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

[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

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

[Danilo Krummrich]

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. :)