[PATCH v6 0/6] rust: add support for request_irq

[PATCH v6 0/6] rust: add support for request_irq

[Daniel Almeida]

---
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/

---
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              |  22 ++
 rust/kernel/irq/flags.rs        | 102 ++++++++
 rust/kernel/irq/request.rs      | 508 ++++++++++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs              |   1 +
 rust/kernel/pci.rs              |  45 +++-
 rust/kernel/platform.rs         | 143 ++++++++++-
 10 files changed, 837 insertions(+), 3 deletions(-)
---
base-commit: 40b37285e5ecf9bbf7ab29ed5a6e9640e7684e5d
change-id: 20250514-topics-tyr-request_irq-4f6a30837ea8

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

[PATCH v6 1/6] rust: irq: add irq module

[Daniel Almeida]

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

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 6b4774b2b1c37f4da1866e993be6230bc6715841..28dd0ef077fa47211fc0eae899ae4ac82fb6be24 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -81,6 +81,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.0

[PATCH v6 2/6] 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.

Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
---
 rust/kernel/irq.rs       |   3 ++
 rust/kernel/irq/flags.rs | 102 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 105 insertions(+)

diff --git a/rust/kernel/irq.rs b/rust/kernel/irq.rs
index fae7b15effc80c936d6bffbd5b4150000d6c2898..9abd9a6dc36f3e3ecc1f92ad7b0040176b56a079 100644
--- a/rust/kernel/irq.rs
+++ b/rust/kernel/irq.rs
@@ -9,3 +9,6 @@
 //! 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.
+pub mod flags;
diff --git a/rust/kernel/irq/flags.rs b/rust/kernel/irq/flags.rs
new file mode 100644
index 0000000000000000000000000000000000000000..3cfaef65ae14f6c02f55ebcf4d52450c0052df30
--- /dev/null
+++ b/rust/kernel/irq/flags.rs
@@ -0,0 +1,102 @@
+// SPDX-License-Identifier: GPL-2.0
+// SPDX-FileCopyrightText: Copyright 2025 Collabora ltd.
+
+use crate::bindings;
+
+/// Flags to be used when registering IRQ handlers.
+///
+/// They can be combined with the operators `|`, `&`, and `!`.
+#[derive(Clone, Copy, PartialEq, Eq)]
+pub struct Flags(u64);
+
+impl Flags {
+    pub(crate) fn into_inner(self) -> u64 {
+        self.0
+    }
+}
+
+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)
+    }
+}
+
+/// Use the interrupt line as already configured.
+pub const TRIGGER_NONE: Flags = Flags(bindings::IRQF_TRIGGER_NONE as u64);
+
+/// The interrupt is triggered when the signal goes from low to high.
+pub const TRIGGER_RISING: Flags = Flags(bindings::IRQF_TRIGGER_RISING as u64);
+
+/// The interrupt is triggered when the signal goes from high to low.
+pub const TRIGGER_FALLING: Flags = Flags(bindings::IRQF_TRIGGER_FALLING as u64);
+
+/// The interrupt is triggered while the signal is held high.
+pub const TRIGGER_HIGH: Flags = Flags(bindings::IRQF_TRIGGER_HIGH as u64);
+
+/// The interrupt is triggered while the signal is held low.
+pub const TRIGGER_LOW: Flags = Flags(bindings::IRQF_TRIGGER_LOW as u64);
+
+/// Allow sharing the irq among several devices.
+pub const SHARED: Flags = Flags(bindings::IRQF_SHARED as u64);
+
+/// Set by callers when they expect sharing mismatches to occur.
+pub const PROBE_SHARED: Flags = Flags(bindings::IRQF_PROBE_SHARED as u64);
+
+/// Flag to mark this interrupt as timer interrupt.
+pub const TIMER: Flags = Flags(bindings::IRQF_TIMER as u64);
+
+/// Interrupt is per cpu.
+pub const PERCPU: Flags = Flags(bindings::IRQF_PERCPU as u64);
+
+/// Flag to exclude this interrupt from irq balancing.
+pub const NOBALANCING: Flags = Flags(bindings::IRQF_NOBALANCING as u64);
+
+/// 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(bindings::IRQF_IRQPOLL as u64);
+
+/// 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(bindings::IRQF_ONESHOT as u64);
+
+/// 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(bindings::IRQF_NO_SUSPEND as u64);
+
+/// Force enable it on resume even if [`NO_SUSPEND`] is set.
+pub const FORCE_RESUME: Flags = Flags(bindings::IRQF_FORCE_RESUME as u64);
+
+/// Interrupt cannot be threaded.
+pub const NO_THREAD: Flags = Flags(bindings::IRQF_NO_THREAD as u64);
+
+/// Resume IRQ early during syscore instead of at device resume time.
+pub const EARLY_RESUME: Flags = Flags(bindings::IRQF_EARLY_RESUME as u64);
+
+/// If the IRQ is shared with a [`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(bindings::IRQF_COND_SUSPEND as u64);
+
+/// 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(bindings::IRQF_NO_AUTOEN as u64);
+
+/// Exclude from runnaway detection for IPI and similar handlers, depends on
+/// `PERCPU`.
+pub const NO_DEBUG: Flags = Flags(bindings::IRQF_NO_DEBUG as u64);

-- 
2.50.0

[PATCH v6 3/6] 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.

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      | 273 ++++++++++++++++++++++++++++++++++++++++
 5 files changed, 289 insertions(+)

diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 8cbb660e2ec218021d16e6e0144acf6f4d7cca13..da0bd23fad59a2373bd873d12ad69c55208aaa38 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -51,6 +51,7 @@
 #include <linux/ethtool.h>
 #include <linux/file.h>
 #include <linux/firmware.h>
+#include <linux/interrupt.h>
 #include <linux/fs.h>
 #include <linux/jiffies.h>
 #include <linux/jump_label.h>
diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
index 393ad201befb80a9ae39866a725744ab88620fbb..e3579fc7e1cfc30c913207a4a78b790259d7ae7a 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c
@@ -22,6 +22,7 @@
 #include "dma.c"
 #include "drm.c"
 #include "err.c"
+#include "irq.c"
 #include "fs.c"
 #include "io.c"
 #include "jump_label.c"
diff --git a/rust/helpers/irq.c b/rust/helpers/irq.c
new file mode 100644
index 0000000000000000000000000000000000000000..1faca428e2c047a656dec3171855c1508d67e60b
--- /dev/null
+++ b/rust/helpers/irq.c
@@ -0,0 +1,9 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/interrupt.h>
+
+int rust_helper_request_irq(unsigned int irq, irq_handler_t handler,
+			    unsigned long flags, const char *name, void *dev)
+{
+	return request_irq(irq, handler, flags, name, dev);
+}
diff --git a/rust/kernel/irq.rs b/rust/kernel/irq.rs
index 9abd9a6dc36f3e3ecc1f92ad7b0040176b56a079..01bd08884b72c2a3a9460897bce751c732a19794 100644
--- a/rust/kernel/irq.rs
+++ b/rust/kernel/irq.rs
@@ -12,3 +12,8 @@
 
 /// Flags to be used when registering IRQ handlers.
 pub mod flags;
+
+/// IRQ allocation and handling.
+pub mod request;
+
+pub use request::{Handler, IrqRequest, IrqReturn, Registration};
diff --git a/rust/kernel/irq/request.rs b/rust/kernel/irq/request.rs
new file mode 100644
index 0000000000000000000000000000000000000000..4f4beaa3c7887660440b9ddc52414020a0d165ac
--- /dev/null
+++ b/rust/kernel/irq/request.rs
@@ -0,0 +1,273 @@
+// SPDX-License-Identifier: GPL-2.0
+// SPDX-FileCopyrightText: Copyright 2025 Collabora ltd.
+
+//! This module provides types like [`Registration`] which allow users to
+//! register handlers for a given IRQ line.
+
+use core::marker::PhantomPinned;
+
+use crate::alloc::Allocator;
+use crate::device::Bound;
+use crate::device::Device;
+use crate::devres::Devres;
+use crate::error::to_result;
+use crate::irq::flags::Flags;
+use crate::prelude::*;
+use crate::str::CStr;
+use crate::sync::Arc;
+
+/// The value that can be returned from an IrqHandler or a ThreadedIrqHandler.
+pub enum IrqReturn {
+    /// The interrupt was not from this device or was not handled.
+    None,
+
+    /// The interrupt was handled by this device.
+    Handled,
+}
+
+impl IrqReturn {
+    fn into_inner(self) -> u32 {
+        match self {
+            IrqReturn::None => bindings::irqreturn_IRQ_NONE,
+            IrqReturn::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 kernel::ffi::c_void,
+}
+
+impl RegistrationInner {
+    fn synchronize(&self) {
+        // SAFETY: safe as per the invariants of `RegistrationInner`
+        unsafe { bindings::synchronize_irq(self.irq) };
+    }
+}
+
+#[pinned_drop]
+impl PinnedDrop for RegistrationInner {
+    fn drop(self: Pin<&mut Self>) {
+        // SAFETY:
+        //
+        // Safe as per the invariants of `RegistrationInner` and:
+        //
+        // - The containing struct is `!Unpin` and was initialized using
+        // pin-init, so it occupied the same memory location for the entirety of
+        // its lifetime.
+        //
+        // Notice that this will block until all handlers finish executing,
+        // i.e.: at no point will &self be invalid while the handler is running.
+        unsafe { bindings::free_irq(self.irq, self.cookie) };
+    }
+}
+
+// SAFETY: We only use `inner` on drop, which called at most once with no
+// concurrent access.
+unsafe impl Sync for RegistrationInner {}
+
+// SAFETY: It is safe to send `RegistrationInner` across threads.
+unsafe impl Send for RegistrationInner {}
+
+/// A request for an IRQ line for a given device.
+///
+/// # Invariants
+///
+/// - `ìrq` is the number of an interrupt source of `dev`.
+/// - `irq` has not been registered yet.
+pub struct IrqRequest<'a> {
+    dev: &'a Device<Bound>,
+    irq: u32,
+}
+
+impl<'a> IrqRequest<'a> {
+    /// Creates a new IRQ request for the given device and IRQ number.
+    ///
+    /// # Safety
+    ///
+    /// - `irq` should be a valid IRQ number for `dev`.
+    pub(crate) unsafe fn new(dev: &'a Device<Bound>, irq: u32) -> Self {
+        IrqRequest { dev, irq }
+    }
+}
+
+/// A registration of an IRQ handler for a given IRQ line.
+///
+/// # Examples
+///
+/// The following is an example of using `Registration`. It uses a
+/// [`AtomicU32`](core::sync::AtomicU32) to provide the interior mutability.
+///
+/// ```
+/// use core::sync::atomic::AtomicU32;
+/// use core::sync::atomic::Ordering;
+///
+/// use kernel::prelude::*;
+/// use kernel::device::Bound;
+/// use kernel::irq::flags;
+/// use kernel::irq::Registration;
+/// use kernel::irq::IrqRequest;
+/// use kernel::irq::IrqReturn;
+/// use kernel::sync::Arc;
+/// use kernel::c_str;
+/// use kernel::alloc::flags::GFP_KERNEL;
+///
+/// // Declare a struct that will be passed in when the interrupt fires. The u32
+/// // merely serves as an example of some internal data.
+/// struct Data(AtomicU32);
+///
+/// // [`kernel::irq::request::Handler::handle`] takes `&self`. This example
+/// // illustrates how interior mutability can be used when sharing the data
+/// // between process context and IRQ context.
+///
+/// type Handler = Data;
+///
+/// impl kernel::irq::request::Handler for Handler {
+///     // This is executing in IRQ context in some CPU. Other CPUs can still
+///     // try to access to data.
+///     fn handle(&self) -> IrqReturn {
+///         self.0.fetch_add(1, Ordering::Relaxed);
+///
+///         IrqReturn::Handled
+///     }
+/// }
+///
+/// // Registers an IRQ handler for the given IrqRequest.
+/// //
+/// // This is executing in process context and assumes that `request` was
+/// // previously acquired from a device.
+/// fn register_irq(handler: Handler, request: IrqRequest<'_>) -> Result<Arc<Registration<Handler>>> {
+///     let registration = Registration::new(request, flags::SHARED, c_str!("my_device"), handler);
+///
+///     let registration = Arc::pin_init(registration, GFP_KERNEL)?;
+///
+///     // The data can be accessed from process context too.
+///     registration.handler().0.fetch_add(1, Ordering::Relaxed);
+///
+///     Ok(registration)
+/// }
+/// # Ok::<(), Error>(())
+/// ```
+///
+/// # Invariants
+///
+/// * We own an irq handler using `&self.handler` as its private data.
+///
+#[pin_data]
+pub struct Registration<T: Handler + 'static> {
+    #[pin]
+    inner: Devres<RegistrationInner>,
+
+    #[pin]
+    handler: T,
+
+    /// Pinned because we need address stability so that we can pass a pointer
+    /// to the callback.
+    #[pin]
+    _pin: PhantomPinned,
+}
+
+impl<T: Handler + 'static> Registration<T> {
+    /// Registers the IRQ handler with the system for the given IRQ number.
+    pub fn new<'a>(
+        request: IrqRequest<'a>,
+        flags: Flags,
+        name: &'static CStr,
+        handler: T,
+    ) -> impl PinInit<Self, Error> + 'a {
+        try_pin_init!(&this in Self {
+            handler,
+            inner <- Devres::new(
+                request.dev,
+                try_pin_init!(RegistrationInner {
+                    // SAFETY: `this` is a valid pointer to the `Registration` instance
+                    cookie: unsafe { &raw mut (*this.as_ptr()).handler }.cast(),
+                    irq: {
+                        // SAFETY:
+                        // - The callbacks are valid for use with request_irq.
+                        // - If this succeeds, the slot is guaranteed to be valid until the
+                        // destructor of Self runs, which will deregister the callbacks
+                        // before the memory location becomes invalid.
+                        to_result(unsafe {
+                            bindings::request_irq(
+                                request.irq,
+                                Some(handle_irq_callback::<T>),
+                                flags.into_inner() as usize,
+                                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 core::ffi::c_void,
+) -> core::ffi::c_uint {
+    // SAFETY: `ptr` is a pointer to T set in `Registration::new`
+    let handler = unsafe { &*(ptr as *const T) };
+    T::handle(handler).into_inner()
+}

-- 
2.50.0

[PATCH v6 4/6] 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.

Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
---
 rust/kernel/irq.rs         |   5 +-
 rust/kernel/irq/request.rs | 239 ++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 241 insertions(+), 3 deletions(-)

diff --git a/rust/kernel/irq.rs b/rust/kernel/irq.rs
index 01bd08884b72c2a3a9460897bce751c732a19794..aaa40001bafca617c588c799bb41144921595cae 100644
--- a/rust/kernel/irq.rs
+++ b/rust/kernel/irq.rs
@@ -16,4 +16,7 @@
 /// IRQ allocation and handling.
 pub mod request;
 
-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 4f4beaa3c7887660440b9ddc52414020a0d165ac..bd489b8d23868a3b315a4f212f94a31870c9f02e 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;
 
@@ -271,3 +272,237 @@ pub fn synchronize(&self, dev: &Device<Bound>) -> Result {
     let handler = unsafe { &*(ptr as *const T) };
     T::handle(handler).into_inner()
 }
+
+/// The value that can be returned from `ThreadedHandler::handle_irq`.
+pub enum ThreadedIrqReturn {
+    /// The interrupt was not from this device or was not handled.
+    None,
+
+    /// The interrupt was handled by this device.
+    Handled,
+
+    /// The handler wants the handler thread to wake up.
+    WakeThread,
+}
+
+impl ThreadedIrqReturn {
+    fn into_inner(self) -> u32 {
+        match self {
+            ThreadedIrqReturn::None => bindings::irqreturn_IRQ_NONE,
+            ThreadedIrqReturn::Handled => bindings::irqreturn_IRQ_HANDLED,
+            ThreadedIrqReturn::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`].
+    fn handle(&self) -> ThreadedIrqReturn;
+
+    /// 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 `WakeThread`.
+///
+/// # Examples
+///
+/// The following is an example of using `ThreadedRegistration`. It uses a
+/// [`AtomicU32`](core::sync::AtomicU32) to provide the interior mutability.
+///
+/// ```
+/// use core::sync::atomic::AtomicU32;
+/// use core::sync::atomic::Ordering;
+///
+/// use kernel::prelude::*;
+/// use kernel::device::Bound;
+/// use kernel::irq::flags;
+/// use kernel::irq::ThreadedIrqReturn;
+/// use kernel::irq::ThreadedRegistration;
+/// use kernel::irq::IrqRequest;
+/// use kernel::irq::IrqReturn;
+/// use kernel::sync::Arc;
+/// use kernel::c_str;
+/// use kernel::alloc::flags::GFP_KERNEL;
+///
+/// // Declare a struct that will be passed in when the interrupt fires. The u32
+/// // merely serves as an example of some internal data.
+/// struct Data(AtomicU32);
+///
+/// // [`kernel::irq::request::ThreadedHandler::handle`] takes `&self`. This example
+/// // illustrates how interior mutability can be used when sharing the data
+/// // between process context and IRQ context.
+/// type Handler = Data;
+///
+/// impl kernel::irq::request::ThreadedHandler for Handler {
+///     // This is executing in IRQ context in some CPU. Other CPUs can still
+///     // try to access the data.
+///     fn handle(&self) -> ThreadedIrqReturn {
+///         self.0.fetch_add(1, Ordering::Relaxed);
+///         // By returning `WakeThread`, we indicate to the system that the
+///         // thread function should be called. Otherwise, return
+///         // ThreadedIrqReturn::Handled.
+///         ThreadedIrqReturn::WakeThread
+///     }
+///
+///     // This will run (in a separate kthread) if and only if `handle`
+///     // returns `WakeThread`.
+///     fn handle_threaded(&self) -> IrqReturn {
+///         self.0.fetch_add(1, Ordering::Relaxed);
+///         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: Handler, request: IrqRequest<'_>) -> Result<Arc<ThreadedRegistration<Handler>>> {
+///     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.
+///     registration.handler().0.fetch_add(1, Ordering::Relaxed);
+///
+///     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: T,
+    ) -> impl PinInit<Self, Error> + 'a {
+        try_pin_init!(&this in Self {
+            handler,
+            inner <- Devres::new(
+                request.dev,
+                try_pin_init!(RegistrationInner {
+                    // SAFETY: `this` is a valid pointer to the `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() as usize,
+                                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 core::ffi::c_void,
+) -> core::ffi::c_uint {
+    // SAFETY: `ptr` is a pointer to T set in `ThreadedRegistration::new`
+    let handler = unsafe { &*(ptr as *const T) };
+    T::handle(handler).into_inner()
+}
+
+/// # 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 core::ffi::c_void,
+) -> core::ffi::c_uint {
+    // SAFETY: `ptr` is a pointer to T set in `ThreadedRegistration::new`
+    let handler = unsafe { &*(ptr as *const T) };
+    T::handle_threaded(handler).into_inner()
+}

-- 
2.50.0

[PATCH v6 5/6] 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.

Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
---
 rust/kernel/platform.rs | 143 +++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 142 insertions(+), 1 deletion(-)

diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs
index 5b21fa517e55348582622ec10471918919502959..bfcb4962dda7ec420dc86dfa04311be0462ad9f2 100644
--- a/rust/kernel/platform.rs
+++ b/rust/kernel/platform.rs
@@ -5,8 +5,11 @@
 //! C header: [`include/linux/platform_device.h`](srctree/include/linux/platform_device.h)
 
 use crate::{
-    bindings, container_of, device, driver,
+    bindings, container_of,
+    device::{self, Bound},
+    driver,
     error::{to_result, Result},
+    irq::{self, request::IrqRequest},
     of,
     prelude::*,
     str::CStr,
@@ -190,6 +193,144 @@ fn as_raw(&self) -> *mut bindings::platform_device {
     }
 }
 
+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<T: irq::$handler_trait + 'static>(
+            &self,
+            flags: irq::flags::Flags,
+            index: u32,
+            name: &'static CStr,
+            handler: T,
+        ) -> Result<impl PinInit<irq::$reg_type<T>, Error> + '_> {
+            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<T: irq::$handler_trait + 'static>(
+            &self,
+            flags: irq::flags::Flags,
+            irq_name: &'static CStr,
+            name: &'static CStr,
+            handler: T,
+        ) -> Result<impl PinInit<irq::$reg_type<T>, Error> + '_> {
+            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 request_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 request_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 request_irq_by_name(&self, name: &'static 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 request_optional_irq_by_name(&self, name: &'static 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.
+        irq_by_index, request_irq_by_index, Registration, Handler
+    );
+    define_irq_accessor_by_name!(
+        /// Returns a [`irq::Registration`] for the IRQ with the given name.
+        irq_by_name, request_irq_by_name, Registration, Handler
+    );
+    define_irq_accessor_by_index!(
+        /// Does the same as [`Self::irq_by_index`], except that it does not
+        /// print an error message if the IRQ cannot be obtained.
+        optional_irq_by_index, request_optional_irq_by_index, Registration, Handler
+    );
+    define_irq_accessor_by_name!(
+        /// Does the same as [`Self::irq_by_name`], except that it does not
+        /// print an error message if the IRQ cannot be obtained.
+        optional_irq_by_name, request_optional_irq_by_name, Registration, Handler
+    );
+
+    define_irq_accessor_by_index!(
+        /// Returns a [`irq::ThreadedRegistration`] for the IRQ at the given index.
+        threaded_irq_by_index, request_irq_by_index, ThreadedRegistration, ThreadedHandler
+    );
+    define_irq_accessor_by_name!(
+        /// Returns a [`irq::ThreadedRegistration`] for the IRQ with the given name.
+        threaded_irq_by_name, request_irq_by_name, ThreadedRegistration, ThreadedHandler
+    );
+    define_irq_accessor_by_index!(
+        /// Does the same as [`Self::threaded_irq_by_index`], except that it
+        /// does not print an error message if the IRQ cannot be obtained.
+        optional_threaded_irq_by_index, request_optional_irq_by_index, ThreadedRegistration, ThreadedHandler
+    );
+    define_irq_accessor_by_name!(
+        /// Does the same as [`Self::threaded_irq_by_name`], except that it
+        /// does not print an error message if the IRQ cannot be obtained.
+        optional_threaded_irq_by_name, request_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.0

[PATCH v6 6/6] 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.

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 cd0e6bf2cc4d9b37db3a717e7a8422b054f348ec..f372a32e8fd19730563ab51500e8c8764854ae47 100644
--- a/rust/helpers/pci.c
+++ b/rust/helpers/pci.c
@@ -21,3 +21,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 db0eb7eaf9b10c5316366ef16fe722a03044a517..60d37d6459518c79136535ce03c73a5a3097eda8 100644
--- a/rust/kernel/pci.rs
+++ b/rust/kernel/pci.rs
@@ -10,8 +10,8 @@
     devres::Devres,
     driver,
     error::{to_result, Result},
-    io::Io,
-    io::IoRaw,
+    io::{Io, IoRaw},
+    irq::{self, request::IrqRequest},
     str::CStr,
     types::{ARef, ForeignOwnable, Opaque},
     ThisModule,
@@ -413,6 +413,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 request_irq_by_index(&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 irq_by_index<T: crate::irq::Handler + 'static>(
+        &self,
+        index: u32,
+        flags: irq::flags::Flags,
+        name: &'static CStr,
+        handler: T,
+    ) -> Result<impl PinInit<irq::Registration<T>, Error> + '_> {
+        let request = self.request_irq_by_index(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 threaded_irq_by_index<T: crate::irq::ThreadedHandler + 'static>(
+        &self,
+        index: u32,
+        flags: irq::flags::Flags,
+        name: &'static CStr,
+        handler: T,
+    ) -> Result<impl PinInit<irq::ThreadedRegistration<T>, Error> + '_> {
+        let request = self.request_irq_by_index(index)?;
+
+        Ok(irq::ThreadedRegistration::<T>::new(
+            request, flags, name, handler,
+        ))
+    }
 }
 
 impl Device<device::Core> {

-- 
2.50.0

Re: [PATCH v6 0/6] rust: add support for request_irq

[Danilo Krummrich]

On Thu, Jul 03, 2025 at 04:29:58PM -0300, Daniel Almeida wrote:
> 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

What's the intended merge path for this series? Should I take it through
driver-core? I'd assume so, given that, besides the series also containing
platform and PCI patches, it depends on patches that are in driver-core-next.

@Thomas: Is there any agreement on how the IRQ Rust code should be maintained?
What's your preference?

- Danilo

Re: [PATCH v6 2/6] rust: irq: add flags module

[Daniel Sedlak]

Hi Daniel,

On 7/3/25 9:30 PM, Daniel Almeida wrote:
> +/// Flags to be used when registering IRQ handlers.
> +///
> +/// They can be combined with the operators `|`, `&`, and `!`.
> +#[derive(Clone, Copy, PartialEq, Eq)]
> +pub struct Flags(u64);

Why not Flags(u32)? You may get rid of all unnecessary casts later, plus 
save some extra bytes.
> +/// Use the interrupt line as already configured.
> +pub const TRIGGER_NONE: Flags = Flags(bindings::IRQF_TRIGGER_NONE as u64);
> +
> +/// The interrupt is triggered when the signal goes from low to high.
> +pub const TRIGGER_RISING: Flags = Flags(bindings::IRQF_TRIGGER_RISING as u64);
> +
> +/// The interrupt is triggered when the signal goes from high to low.
> +pub const TRIGGER_FALLING: Flags = Flags(bindings::IRQF_TRIGGER_FALLING as u64);
> +
> +/// The interrupt is triggered while the signal is held high.
> +pub const TRIGGER_HIGH: Flags = Flags(bindings::IRQF_TRIGGER_HIGH as u64);
> +
> +/// The interrupt is triggered while the signal is held low.
> +pub const TRIGGER_LOW: Flags = Flags(bindings::IRQF_TRIGGER_LOW as u64);
> +
> +/// Allow sharing the irq among several devices.

nit: irq -> IRQ?

> +pub const SHARED: Flags = Flags(bindings::IRQF_SHARED as u64);
> +
> +/// Set by callers when they expect sharing mismatches to occur.
> +pub const PROBE_SHARED: Flags = Flags(bindings::IRQF_PROBE_SHARED as u64);
> +
> +/// Flag to mark this interrupt as timer interrupt.
> +pub const TIMER: Flags = Flags(bindings::IRQF_TIMER as u64);
> +
> +/// Interrupt is per cpu.

nit: cpu -> CPU?

> +pub const PERCPU: Flags = Flags(bindings::IRQF_PERCPU as u64);
Thanks!
Daniel

Re: [PATCH v6 2/6] rust: irq: add flags module

[Alice Ryhl]

On Fri, Jul 04, 2025 at 08:14:11AM +0200, Daniel Sedlak wrote:
> Hi Daniel,
> 
> On 7/3/25 9:30 PM, Daniel Almeida wrote:
> > +/// Flags to be used when registering IRQ handlers.
> > +///
> > +/// They can be combined with the operators `|`, `&`, and `!`.
> > +#[derive(Clone, Copy, PartialEq, Eq)]
> > +pub struct Flags(u64);
> 
> Why not Flags(u32)? You may get rid of all unnecessary casts later, plus
> save some extra bytes.

It looks like the C methods take an `unsigned long`. In that case, I'd
probably write the code to match that.

	pub struct Flags(c_ulong);

and git rid of the cast when calling bindings::request_irq.

As for all the constants in this file, maybe it would be nice with a
private constructor that uses the same type as bindings to avoid the
casts?

	impl Flags {
	    const fn new(value: u32) -> Flags {
	    	...
	    }
	}
	
	/// Use the interrupt line as already configured.
	pub const TRIGGER_NONE: Flags = Flags::new(bindings::IRQF_TRIGGER_NONE);

Not a big deal, but avoiding the cast when calling bindings::request_irq
is a good idea I think.

With that:
Reviewed-by: Alice Ryhl <aliceryhl@google.com>

Alice

Re: [PATCH v6 1/6] rust: irq: add irq module

[Alice Ryhl]

On Thu, Jul 03, 2025 at 04:29:59PM -0300, Daniel Almeida wrote:
> Add the IRQ module. Future patches will then introduce support for IRQ
> registrations and handlers.
> 
> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

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

[Alice Ryhl]

On Thu, Jul 03, 2025 at 04:30:01PM -0300, Daniel Almeida wrote:
> This patch adds support for non-threaded IRQs and handlers through
> irq::Registration and the irq::Handler trait.
> 
> Registering an irq is dependent upon having a IrqRequest that was
> previously allocated by a given device. This will be introduced in
> subsequent patches.
> 
> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
> ---
>  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      | 273 ++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 289 insertions(+)
> 
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index 8cbb660e2ec218021d16e6e0144acf6f4d7cca13..da0bd23fad59a2373bd873d12ad69c55208aaa38 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -51,6 +51,7 @@
>  #include <linux/ethtool.h>
>  #include <linux/file.h>
>  #include <linux/firmware.h>
> +#include <linux/interrupt.h>
>  #include <linux/fs.h>
>  #include <linux/jiffies.h>
>  #include <linux/jump_label.h>
> diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
> index 393ad201befb80a9ae39866a725744ab88620fbb..e3579fc7e1cfc30c913207a4a78b790259d7ae7a 100644
> --- a/rust/helpers/helpers.c
> +++ b/rust/helpers/helpers.c
> @@ -22,6 +22,7 @@
>  #include "dma.c"
>  #include "drm.c"
>  #include "err.c"
> +#include "irq.c"
>  #include "fs.c"
>  #include "io.c"
>  #include "jump_label.c"
> diff --git a/rust/helpers/irq.c b/rust/helpers/irq.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..1faca428e2c047a656dec3171855c1508d67e60b
> --- /dev/null
> +++ b/rust/helpers/irq.c
> @@ -0,0 +1,9 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/interrupt.h>
> +
> +int rust_helper_request_irq(unsigned int irq, irq_handler_t handler,
> +			    unsigned long flags, const char *name, void *dev)
> +{
> +	return request_irq(irq, handler, flags, name, dev);
> +}
> diff --git a/rust/kernel/irq.rs b/rust/kernel/irq.rs
> index 9abd9a6dc36f3e3ecc1f92ad7b0040176b56a079..01bd08884b72c2a3a9460897bce751c732a19794 100644
> --- a/rust/kernel/irq.rs
> +++ b/rust/kernel/irq.rs
> @@ -12,3 +12,8 @@
>  
>  /// Flags to be used when registering IRQ handlers.
>  pub mod flags;
> +
> +/// IRQ allocation and handling.
> +pub mod request;
> +
> +pub use request::{Handler, IrqRequest, IrqReturn, Registration};
> diff --git a/rust/kernel/irq/request.rs b/rust/kernel/irq/request.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..4f4beaa3c7887660440b9ddc52414020a0d165ac
> --- /dev/null
> +++ b/rust/kernel/irq/request.rs
> @@ -0,0 +1,273 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// SPDX-FileCopyrightText: Copyright 2025 Collabora ltd.
> +
> +//! This module provides types like [`Registration`] which allow users to
> +//! register handlers for a given IRQ line.
> +
> +use core::marker::PhantomPinned;
> +
> +use crate::alloc::Allocator;
> +use crate::device::Bound;
> +use crate::device::Device;
> +use crate::devres::Devres;
> +use crate::error::to_result;
> +use crate::irq::flags::Flags;
> +use crate::prelude::*;
> +use crate::str::CStr;
> +use crate::sync::Arc;
> +
> +/// The value that can be returned from an IrqHandler or a ThreadedIrqHandler.
> +pub enum IrqReturn {
> +    /// The interrupt was not from this device or was not handled.
> +    None,
> +
> +    /// The interrupt was handled by this device.
> +    Handled,
> +}
> +
> +impl IrqReturn {
> +    fn into_inner(self) -> u32 {
> +        match self {
> +            IrqReturn::None => bindings::irqreturn_IRQ_NONE,
> +            IrqReturn::Handled => bindings::irqreturn_IRQ_HANDLED,

One option is to specify these in the enum:

/// The value that can be returned from an IrqHandler or a ThreadedIrqHandler.
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,
}

impl IrqReturn {
    fn into_inner(self) -> c_uint {
        self as c_uint
    }
}

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

I recall there being a clippy lint about the indentation here. Did it
not trigger?

/// - `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 kernel::ffi::c_void,

The c_void type is in the prelude.

> +}
> +
> +impl RegistrationInner {
> +    fn synchronize(&self) {
> +        // SAFETY: safe as per the invariants of `RegistrationInner`
> +        unsafe { bindings::synchronize_irq(self.irq) };
> +    }
> +}
> +
> +#[pinned_drop]
> +impl PinnedDrop for RegistrationInner {
> +    fn drop(self: Pin<&mut Self>) {
> +        // SAFETY:
> +        //
> +        // Safe as per the invariants of `RegistrationInner` and:
> +        //
> +        // - The containing struct is `!Unpin` and was initialized using
> +        // pin-init, so it occupied the same memory location for the entirety of
> +        // its lifetime.
> +        //
> +        // Notice that this will block until all handlers finish executing,
> +        // i.e.: at no point will &self be invalid while the handler is running.
> +        unsafe { bindings::free_irq(self.irq, self.cookie) };
> +    }
> +}
> +
> +// SAFETY: We only use `inner` on drop, which called at most once with no
> +// concurrent access.
> +unsafe impl Sync for RegistrationInner {}
> +
> +// SAFETY: It is safe to send `RegistrationInner` across threads.
> +unsafe impl Send for RegistrationInner {}
> +
> +/// A request for an IRQ line for a given device.
> +///
> +/// # Invariants
> +///
> +/// - `ìrq` is the number of an interrupt source of `dev`.
> +/// - `irq` has not been registered yet.
> +pub struct IrqRequest<'a> {
> +    dev: &'a Device<Bound>,
> +    irq: u32,
> +}
> +
> +impl<'a> IrqRequest<'a> {
> +    /// Creates a new IRQ request for the given device and IRQ number.
> +    ///
> +    /// # Safety
> +    ///
> +    /// - `irq` should be a valid IRQ number for `dev`.
> +    pub(crate) unsafe fn new(dev: &'a Device<Bound>, irq: u32) -> Self {
> +        IrqRequest { dev, irq }
> +    }
> +}
> +
> +/// A registration of an IRQ handler for a given IRQ line.
> +///
> +/// # Examples
> +///
> +/// The following is an example of using `Registration`. It uses a
> +/// [`AtomicU32`](core::sync::AtomicU32) to provide the interior mutability.
> +///
> +/// ```
> +/// use core::sync::atomic::AtomicU32;
> +/// use core::sync::atomic::Ordering;
> +///
> +/// use kernel::prelude::*;
> +/// use kernel::device::Bound;
> +/// use kernel::irq::flags;
> +/// use kernel::irq::Registration;
> +/// use kernel::irq::IrqRequest;
> +/// use kernel::irq::IrqReturn;
> +/// use kernel::sync::Arc;
> +/// use kernel::c_str;
> +/// use kernel::alloc::flags::GFP_KERNEL;
> +///
> +/// // Declare a struct that will be passed in when the interrupt fires. The u32
> +/// // merely serves as an example of some internal data.
> +/// struct Data(AtomicU32);
> +///
> +/// // [`kernel::irq::request::Handler::handle`] takes `&self`. This example
> +/// // illustrates how interior mutability can be used when sharing the data
> +/// // between process context and IRQ context.
> +///
> +/// type Handler = Data;
> +///
> +/// impl kernel::irq::request::Handler for Handler {
> +///     // This is executing in IRQ context in some CPU. Other CPUs can still
> +///     // try to access to data.
> +///     fn handle(&self) -> IrqReturn {
> +///         self.0.fetch_add(1, Ordering::Relaxed);
> +///
> +///         IrqReturn::Handled
> +///     }
> +/// }
> +///
> +/// // Registers an IRQ handler for the given IrqRequest.
> +/// //
> +/// // This is executing in process context and assumes that `request` was
> +/// // previously acquired from a device.
> +/// fn register_irq(handler: Handler, request: IrqRequest<'_>) -> Result<Arc<Registration<Handler>>> {
> +///     let registration = Registration::new(request, flags::SHARED, c_str!("my_device"), handler);
> +///
> +///     let registration = Arc::pin_init(registration, GFP_KERNEL)?;
> +///
> +///     // The data can be accessed from process context too.
> +///     registration.handler().0.fetch_add(1, Ordering::Relaxed);
> +///
> +///     Ok(registration)
> +/// }
> +/// # Ok::<(), Error>(())
> +/// ```
> +///
> +/// # Invariants
> +///
> +/// * We own an irq handler using `&self.handler` as its private data.
> +///
> +#[pin_data]
> +pub struct Registration<T: Handler + 'static> {
> +    #[pin]
> +    inner: Devres<RegistrationInner>,
> +
> +    #[pin]
> +    handler: T,
> +
> +    /// Pinned because we need address stability so that we can pass a pointer
> +    /// to the callback.
> +    #[pin]
> +    _pin: PhantomPinned,
> +}
> +
> +impl<T: Handler + 'static> Registration<T> {
> +    /// Registers the IRQ handler with the system for the given IRQ number.
> +    pub fn new<'a>(
> +        request: IrqRequest<'a>,
> +        flags: Flags,
> +        name: &'static CStr,
> +        handler: T,
> +    ) -> impl PinInit<Self, Error> + 'a {
> +        try_pin_init!(&this in Self {
> +            handler,
> +            inner <- Devres::new(
> +                request.dev,
> +                try_pin_init!(RegistrationInner {
> +                    // SAFETY: `this` is a valid pointer to the `Registration` instance
> +                    cookie: unsafe { &raw mut (*this.as_ptr()).handler }.cast(),
> +                    irq: {
> +                        // SAFETY:
> +                        // - The callbacks are valid for use with request_irq.
> +                        // - If this succeeds, the slot is guaranteed to be valid until the
> +                        // destructor of Self runs, which will deregister the callbacks
> +                        // before the memory location becomes invalid.
> +                        to_result(unsafe {
> +                            bindings::request_irq(
> +                                request.irq,
> +                                Some(handle_irq_callback::<T>),
> +                                flags.into_inner() as usize,
> +                                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 core::ffi::c_void,
> +) -> core::ffi::c_uint {

You should just use `c_uint` without the prefix. This way you get it
from `kernel::prelude::*` which has the correct typedefs rather than
`core::ffi`.

> +    // SAFETY: `ptr` is a pointer to T set in `Registration::new`
> +    let handler = unsafe { &*(ptr as *const T) };
> +    T::handle(handler).into_inner()
> +}
> 
> -- 
> 2.50.0
> 

Re: [PATCH v6 4/6] rust: irq: add support for threaded IRQs and handlers

[Alice Ryhl]

On Thu, Jul 03, 2025 at 04:30:02PM -0300, Daniel Almeida wrote:
> 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.
> 
> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>

Same comments as patch 3, otherwise LGTM.

Alice

Re: [PATCH v6 5/6] rust: platform: add irq accessors

[Alice Ryhl]

On Thu, Jul 03, 2025 at 04:30:03PM -0300, Daniel Almeida wrote:
> 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.
> 
> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>

One question below. With that answered:
Reviewed-by: Alice Ryhl <aliceryhl@google.com>

> +    /// Returns an [`IrqRequest`] for the IRQ with the given name, if any.
> +    pub fn request_irq_by_name(&self, name: &'static CStr) -> Result<IrqRequest<'_>> {

Does the name need to be static? That's surprising - isn't it just a
lookup that needs to be valid during this call?

Alice

Re: [PATCH v6 6/6] rust: pci: add irq accessors

[Alice Ryhl]

On Thu, Jul 03, 2025 at 04:30:04PM -0300, Daniel Almeida wrote:
> 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.
> 
> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>

Same question as patch 5. With that answered:
Reviewed-by: Alice Ryhl <aliceryhl@google.com>

Re: [PATCH v6 2/6] rust: irq: add flags module

[Miguel Ojeda]

On Fri, Jul 4, 2025 at 8:14 AM Daniel Sedlak <daniel@sedlak.dev> wrote:
>
> Why not Flags(u32)? You may get rid of all unnecessary casts later, plus
> save some extra bytes.

Yeah, avoiding the casts would be nice -- however it is done.

On the patch: it wouldn't hurt to add an example for each of the
operators too. Even better if it is a common use case of each (say,
for `|`, a common combination of these flags).

(By the way, the use of the `into_inner()` name here is another one we
will want to discuss for the new naming guidelines table.)

Thanks!

Cheers,
Miguel

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

[Boqun Feng]

On Thu, Jul 03, 2025 at 04:30:01PM -0300, Daniel Almeida wrote:
[...]
> +#[pin_data]
> +pub struct Registration<T: Handler + 'static> {
> +    #[pin]
> +    inner: Devres<RegistrationInner>,
> +
> +    #[pin]
> +    handler: T,

IIRC, as a certain point, we want this to be a `UnsafePinned<T>`, is
that requirement gone or we still need that but 1) `UnsafePinned` is not
available and 2) we can rely on the whole struct being !Unpin for the
address stability temporarily?

I think it was not a problem until we switched to `try_pin_init!()`
instead of `pin_init_from_closure()` because we then had to pass the
address of `handler` instead of the whole struct.

Since we certainly want to use `try_pin_init!()` and we certainly will
have `UnsafePinned`, I think we should just keep this as it is for now,
and add a TODO so that we can clean it up later when we have
`UnsafePinned`?

Thoughts?

Regards,
Boqun

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

[Boqun Feng]

On Fri, Jul 04, 2025 at 09:39:01AM -0700, Boqun Feng wrote:
> On Thu, Jul 03, 2025 at 04:30:01PM -0300, Daniel Almeida wrote:
> [...]
> > +#[pin_data]
> > +pub struct Registration<T: Handler + 'static> {
> > +    #[pin]
> > +    inner: Devres<RegistrationInner>,
> > +
> > +    #[pin]
> > +    handler: T,
> 
> IIRC, as a certain point, we want this to be a `UnsafePinned<T>`, is
> that requirement gone or we still need that but 1) `UnsafePinned` is not
> available and 2) we can rely on the whole struct being !Unpin for the
> address stability temporarily?
> 
> I think it was not a problem until we switched to `try_pin_init!()`
> instead of `pin_init_from_closure()` because we then had to pass the
> address of `handler` instead of the whole struct.
> 
> Since we certainly want to use `try_pin_init!()` and we certainly will
> have `UnsafePinned`, I think we should just keep this as it is for now,

Of course the assumption is we want to it in before `UnsafePinned` ;-)
Alternatively we can do what `Devres` did:

	https://lore.kernel.org/rust-for-linux/20250626200054.243480-4-dakr@kernel.org/

using an `Opaque` and manually drop for now.

Regards,
Boqun

> and add a TODO so that we can clean it up later when we have
> `UnsafePinned`?
> 
> Thoughts?
> 
> Regards,
> Boqun

Re: [PATCH v6 5/6] rust: platform: add irq accessors

[Danilo Krummrich]

On Fri, Jul 04, 2025 at 07:56:11AM +0000, Alice Ryhl wrote:
> On Thu, Jul 03, 2025 at 04:30:03PM -0300, Daniel Almeida wrote:
> > 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.
> > 
> > Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
> 
> One question below. With that answered:
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> 
> > +    /// Returns an [`IrqRequest`] for the IRQ with the given name, if any.
> > +    pub fn request_irq_by_name(&self, name: &'static CStr) -> Result<IrqRequest<'_>> {
> 
> Does the name need to be static? That's surprising - isn't it just a
> lookup that needs to be valid during this call?

The string used to lookup the irq number is only used for comparison, and hence
doesn't need to have a static lifetime.

Re: [PATCH v6 6/6] rust: pci: add irq accessors

[Danilo Krummrich]

On Fri, Jul 04, 2025 at 07:56:56AM +0000, Alice Ryhl wrote:
> On Thu, Jul 03, 2025 at 04:30:04PM -0300, Daniel Almeida wrote:
> > 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.
> > 
> > Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
> 
> Same question as patch 5.

Here we don't do a lookup of the IRQ number by name, yet the API asks for the
name used for the IRQ registration, hence it needs a static lifetime, since the
pointer is directly stored in struct irqaction. It's accessed by trace events
for instance.

> With that answered:
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>

Re: [PATCH v6 5/6] rust: platform: add irq accessors

[Danilo Krummrich]

On Thu, Jul 03, 2025 at 04:30:03PM -0300, Daniel Almeida wrote:
> +impl Device<Bound> {
> +    /// Returns an [`IrqRequest`] for the IRQ at the given index, if any.
> +    pub fn request_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) })
> +    }

Sorry that I didn't notice that before: Please just name the functions returning
an IrqRequest e.g. irq_by_index(), without the 'request' prefix. And instead put
the 'request' prefix in front of the methods that return a actual
irq::Registration.

This is more in line with the C functions being named request_irq() and
request_threaded_irq().

Re: [PATCH v6 6/6] rust: pci: add irq accessors

[Danilo Krummrich]

On Thu, Jul 03, 2025 at 04:30:04PM -0300, Daniel Almeida wrote:
> +    /// Returns an [`IrqRequest`] for the IRQ vector at the given index, if any.
> +    pub fn request_irq_by_index(&self, index: u32) -> Result<IrqRequest<'_>> {

Same comment as for platform:

Please name the functions returning an IrqRequest without the 'request' prefix.
And instead put the 'request' prefix in front of the methods that return a
actual irq::Registration.

Besides that, I think we shouldn't name this method 'by_index'. Please align it
with the C function, i.e. Device::irq_vector().

> +    pub fn irq_by_index<T: crate::irq::Handler + 'static>(

I'd go with just request_irq() for this one and

> +    pub fn threaded_irq_by_index<T: crate::irq::ThreadedHandler + 'static>(

request_threaded_irq() for this one.

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

[Alice Ryhl]

On Fri, Jul 04, 2025 at 09:41:49AM -0700, Boqun Feng wrote:
> On Fri, Jul 04, 2025 at 09:39:01AM -0700, Boqun Feng wrote:
> > On Thu, Jul 03, 2025 at 04:30:01PM -0300, Daniel Almeida wrote:
> > [...]
> > > +#[pin_data]
> > > +pub struct Registration<T: Handler + 'static> {
> > > +    #[pin]
> > > +    inner: Devres<RegistrationInner>,
> > > +
> > > +    #[pin]
> > > +    handler: T,
> > 
> > IIRC, as a certain point, we want this to be a `UnsafePinned<T>`, is
> > that requirement gone or we still need that but 1) `UnsafePinned` is not
> > available and 2) we can rely on the whole struct being !Unpin for the
> > address stability temporarily?
> > 
> > I think it was not a problem until we switched to `try_pin_init!()`
> > instead of `pin_init_from_closure()` because we then had to pass the
> > address of `handler` instead of the whole struct.
> > 
> > Since we certainly want to use `try_pin_init!()` and we certainly will
> > have `UnsafePinned`, I think we should just keep this as it is for now,
> 
> Of course the assumption is we want to it in before `UnsafePinned` ;-)
> Alternatively we can do what `Devres` did:
> 
> 	https://lore.kernel.org/rust-for-linux/20250626200054.243480-4-dakr@kernel.org/
> 
> using an `Opaque` and manually drop for now.

The struct uses PhantomPinned, so the code is correct as-is. Using a
common abstraction for UnsafePinned is of course nice, but I suggest
that we keep it like this if both patches land in the same cycle. We can
always have it use UnsafePinned in a follow-up.

Alice

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

[Daniel Almeida]

Alice,

[…]

>> +/// The value that can be returned from an IrqHandler or a ThreadedIrqHandler.
>> +pub enum IrqReturn {
>> +    /// The interrupt was not from this device or was not handled.
>> +    None,
>> +
>> +    /// The interrupt was handled by this device.
>> +    Handled,
>> +}
>> +
>> +impl IrqReturn {
>> +    fn into_inner(self) -> u32 {
>> +        match self {
>> +            IrqReturn::None => bindings::irqreturn_IRQ_NONE,
>> +            IrqReturn::Handled => bindings::irqreturn_IRQ_HANDLED,
> 
> One option is to specify these in the enum:
> 
> /// The value that can be returned from an IrqHandler or a ThreadedIrqHandler.
> 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,
> }

This requires explicitly setting #[repr(u32)], which is something that was
reverted at an earlier iteration of the series on Benno’s request.

— Daniel

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

[Benno Lossin]

On Mon Jul 7, 2025 at 6:18 PM CEST, Daniel Almeida wrote:
> Alice,
>
> […]
>
>>> +/// The value that can be returned from an IrqHandler or a ThreadedIrqHandler.
>>> +pub enum IrqReturn {
>>> +    /// The interrupt was not from this device or was not handled.
>>> +    None,
>>> +
>>> +    /// The interrupt was handled by this device.
>>> +    Handled,
>>> +}
>>> +
>>> +impl IrqReturn {
>>> +    fn into_inner(self) -> u32 {
>>> +        match self {
>>> +            IrqReturn::None => bindings::irqreturn_IRQ_NONE,
>>> +            IrqReturn::Handled => bindings::irqreturn_IRQ_HANDLED,
>> 
>> One option is to specify these in the enum:
>> 
>> /// The value that can be returned from an IrqHandler or a ThreadedIrqHandler.
>> 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,
>> }
>
> This requires explicitly setting #[repr(u32)], which is something that was
> reverted at an earlier iteration of the series on Benno’s request.

Yeah I requested this, because it increases the size of the enum to 4
bytes and I think we should try to make rust enums as small as possible.

@Alice what's the benefit of doing it directly in the enum?

---
Cheers,
Benno

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

[Alice Ryhl]

On Mon, Jul 07, 2025 at 10:30:27PM +0200, Benno Lossin wrote:
> On Mon Jul 7, 2025 at 6:18 PM CEST, Daniel Almeida wrote:
> > Alice,
> >
> > […]
> >
> >>> +/// The value that can be returned from an IrqHandler or a ThreadedIrqHandler.
> >>> +pub enum IrqReturn {
> >>> +    /// The interrupt was not from this device or was not handled.
> >>> +    None,
> >>> +
> >>> +    /// The interrupt was handled by this device.
> >>> +    Handled,
> >>> +}
> >>> +
> >>> +impl IrqReturn {
> >>> +    fn into_inner(self) -> u32 {
> >>> +        match self {
> >>> +            IrqReturn::None => bindings::irqreturn_IRQ_NONE,
> >>> +            IrqReturn::Handled => bindings::irqreturn_IRQ_HANDLED,
> >> 
> >> One option is to specify these in the enum:
> >> 
> >> /// The value that can be returned from an IrqHandler or a ThreadedIrqHandler.
> >> 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,
> >> }
> >
> > This requires explicitly setting #[repr(u32)], which is something that was
> > reverted at an earlier iteration of the series on Benno’s request.
> 
> Yeah I requested this, because it increases the size of the enum to 4
> bytes and I think we should try to make rust enums as small as possible.
> 
> @Alice what's the benefit of doing it directly in the enum?

Basically all uses of this enum are going to look like this:

	fn inner() -> IrqReturn {
	    if !should_handle() {
	        return IrqReturn::None;
	    }
	    // .. handle the irq
	    IrqReturn::Handled
	}
	
	fn outer() -> u32 {
	    match inner() {
	        IrqReturn::None => bindings::irqreturn_IRQ_NONE,
	        IrqReturn::Handled => bindings::irqreturn_IRQ_HANDLED,
	    }
	}

Setting the discriminant to the values ensures that the match in outer()
is a no-op. The enum is never going to be stored long-term anywhere so
its size doesn't matter.

Alice

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

[Dirk Behme]

Hi Daniel,

On 03/07/2025 21:30, Daniel Almeida wrote:
> This patch adds support for non-threaded IRQs and handlers through
> irq::Registration and the irq::Handler trait.
> 
> Registering an irq is dependent upon having a IrqRequest that was
> previously allocated by a given device. This will be introduced in
> subsequent patches.
> 
> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
...
> diff --git a/rust/kernel/irq/request.rs b/rust/kernel/irq/request.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..4f4beaa3c7887660440b9ddc52414020a0d165ac
> --- /dev/null
> +++ b/rust/kernel/irq/request.rs
> @@ -0,0 +1,273 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// SPDX-FileCopyrightText: Copyright 2025 Collabora ltd.
> +
> +//! This module provides types like [`Registration`] which allow users to
> +//! register handlers for a given IRQ line.
> +
> +use core::marker::PhantomPinned;
> +
> +use crate::alloc::Allocator;
> +use crate::device::Bound;
> +use crate::device::Device;
> +use crate::devres::Devres;
> +use crate::error::to_result;
> +use crate::irq::flags::Flags;
> +use crate::prelude::*;
> +use crate::str::CStr;
> +use crate::sync::Arc;
> +
> +/// The value that can be returned from an IrqHandler or a ThreadedIrqHandler.
> +pub enum IrqReturn {
> +    /// The interrupt was not from this device or was not handled.
> +    None,
> +
> +    /// The interrupt was handled by this device.
> +    Handled,
> +}
> +
> +impl IrqReturn {
> +    fn into_inner(self) -> u32 {
> +        match self {
> +            IrqReturn::None => bindings::irqreturn_IRQ_NONE,
> +            IrqReturn::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 kernel::ffi::c_void,
> +}
> +
> +impl RegistrationInner {
> +    fn synchronize(&self) {
> +        // SAFETY: safe as per the invariants of `RegistrationInner`
> +        unsafe { bindings::synchronize_irq(self.irq) };
> +    }
> +}
> +
> +#[pinned_drop]
> +impl PinnedDrop for RegistrationInner {
> +    fn drop(self: Pin<&mut Self>) {
> +        // SAFETY:
> +        //
> +        // Safe as per the invariants of `RegistrationInner` and:
> +        //
> +        // - The containing struct is `!Unpin` and was initialized using
> +        // pin-init, so it occupied the same memory location for the entirety of
> +        // its lifetime.
> +        //
> +        // Notice that this will block until all handlers finish executing,
> +        // i.e.: at no point will &self be invalid while the handler is running.
> +        unsafe { bindings::free_irq(self.irq, self.cookie) };
> +    }
> +}
> +
> +// SAFETY: We only use `inner` on drop, which called at most once with no
> +// concurrent access.
> +unsafe impl Sync for RegistrationInner {}
> +
> +// SAFETY: It is safe to send `RegistrationInner` across threads.
> +unsafe impl Send for RegistrationInner {}
> +
> +/// A request for an IRQ line for a given device.
> +///
> +/// # Invariants
> +///
> +/// - `ìrq` is the number of an interrupt source of `dev`.
> +/// - `irq` has not been registered yet.
> +pub struct IrqRequest<'a> {
> +    dev: &'a Device<Bound>,
> +    irq: u32,
> +}
> +
> +impl<'a> IrqRequest<'a> {
> +    /// Creates a new IRQ request for the given device and IRQ number.
> +    ///
> +    /// # Safety
> +    ///
> +    /// - `irq` should be a valid IRQ number for `dev`.
> +    pub(crate) unsafe fn new(dev: &'a Device<Bound>, irq: u32) -> Self {
> +        IrqRequest { dev, irq }
> +    }
> +}


For example for Resource the elements size, start, name and flags are
accessible. Inspired by that, what do you think about exposing the irq
number here, as well?

diff --git a/rust/kernel/irq/request.rs b/rust/kernel/irq/request.rs
index bd489b8d2386..767d66e3e6c7 100644
--- a/rust/kernel/irq/request.rs
+++ b/rust/kernel/irq/request.rs
@@ -123,6 +123,11 @@ impl<'a> IrqRequest<'a> {
     pub(crate) unsafe fn new(dev: &'a Device<Bound>, irq: u32) -> Self {
         IrqRequest { dev, irq }
     }
+
+    /// Returns the IRQ number of an [`IrqRequest`].
+    pub fn irq(&self) -> u32 {
+        self.irq
+    }
 }


I'm using that for some dev_info!().

Best regards

Dirk

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

[Danilo Krummrich]

On 7/8/25 2:15 PM, Dirk Behme wrote:
> For example for Resource the elements size, start, name and flags are
> accessible. Inspired by that, what do you think about exposing the irq
> number here, as well?
> 
> diff --git a/rust/kernel/irq/request.rs b/rust/kernel/irq/request.rs
> index bd489b8d2386..767d66e3e6c7 100644
> --- a/rust/kernel/irq/request.rs
> +++ b/rust/kernel/irq/request.rs
> @@ -123,6 +123,11 @@ impl<'a> IrqRequest<'a> {
>       pub(crate) unsafe fn new(dev: &'a Device<Bound>, irq: u32) -> Self {
>           IrqRequest { dev, irq }
>       }
> +
> +    /// Returns the IRQ number of an [`IrqRequest`].
> +    pub fn irq(&self) -> u32 {
> +        self.irq
> +    }
>   }
> 
> 
> I'm using that for some dev_info!().

Not sure that's a reasonable candidate for dev_info!() prints, but maybe it can
be useful for some debug prints.

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

[Benno Lossin]

On Tue Jul 8, 2025 at 1:49 PM CEST, Alice Ryhl wrote:
> On Mon, Jul 07, 2025 at 10:30:27PM +0200, Benno Lossin wrote:
>> On Mon Jul 7, 2025 at 6:18 PM CEST, Daniel Almeida wrote:
>> > Alice,
>> >
>> > […]
>> >
>> >>> +/// The value that can be returned from an IrqHandler or a ThreadedIrqHandler.
>> >>> +pub enum IrqReturn {
>> >>> +    /// The interrupt was not from this device or was not handled.
>> >>> +    None,
>> >>> +
>> >>> +    /// The interrupt was handled by this device.
>> >>> +    Handled,
>> >>> +}
>> >>> +
>> >>> +impl IrqReturn {
>> >>> +    fn into_inner(self) -> u32 {
>> >>> +        match self {
>> >>> +            IrqReturn::None => bindings::irqreturn_IRQ_NONE,
>> >>> +            IrqReturn::Handled => bindings::irqreturn_IRQ_HANDLED,
>> >> 
>> >> One option is to specify these in the enum:
>> >> 
>> >> /// The value that can be returned from an IrqHandler or a ThreadedIrqHandler.
>> >> 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,
>> >> }
>> >
>> > This requires explicitly setting #[repr(u32)], which is something that was
>> > reverted at an earlier iteration of the series on Benno’s request.
>> 
>> Yeah I requested this, because it increases the size of the enum to 4
>> bytes and I think we should try to make rust enums as small as possible.
>> 
>> @Alice what's the benefit of doing it directly in the enum?
>
> Basically all uses of this enum are going to look like this:
>
> 	fn inner() -> IrqReturn {
> 	    if !should_handle() {
> 	        return IrqReturn::None;
> 	    }
> 	    // .. handle the irq
> 	    IrqReturn::Handled
> 	}
> 	
> 	fn outer() -> u32 {
> 	    match inner() {
> 	        IrqReturn::None => bindings::irqreturn_IRQ_NONE,
> 	        IrqReturn::Handled => bindings::irqreturn_IRQ_HANDLED,
> 	    }
> 	}
>
> Setting the discriminant to the values ensures that the match in outer()
> is a no-op. The enum is never going to be stored long-term anywhere so
> its size doesn't matter.

Hmm in this particular case, I think the optimizer will be able to
remove the additional branch too. But I haven't checked.

I usually give the advice to not explicitly set the discriminants and
let the compiler do it. I don't have a strong opinion on this, but we
should figure out which one is better in which cases.

---
Cheers,
Benno

Re: [PATCH v6 2/6] rust: irq: add flags module

[Daniel Almeida]

Hi Alice,

> On 4 Jul 2025, at 04:42, Alice Ryhl <aliceryhl@google.com> wrote:
> 
> On Fri, Jul 04, 2025 at 08:14:11AM +0200, Daniel Sedlak wrote:
>> Hi Daniel,
>> 
>> On 7/3/25 9:30 PM, Daniel Almeida wrote:
>>> +/// Flags to be used when registering IRQ handlers.
>>> +///
>>> +/// They can be combined with the operators `|`, `&`, and `!`.
>>> +#[derive(Clone, Copy, PartialEq, Eq)]
>>> +pub struct Flags(u64);
>> 
>> Why not Flags(u32)? You may get rid of all unnecessary casts later, plus
>> save some extra bytes.
> 
> It looks like the C methods take an `unsigned long`. In that case, I'd
> probably write the code to match that.
> 
> pub struct Flags(c_ulong);
> 
> and git rid of the cast when calling bindings::request_irq.
> 
> As for all the constants in this file, maybe it would be nice with a
> private constructor that uses the same type as bindings to avoid the
> casts?
> 
> impl Flags {
>    const fn new(value: u32) -> Flags {
>     ...
>    }
> }


Sure, but what goes here? This has to be "value as c_ulong” anyways so it
doesn’t really reduce the number of casts.

We should probably switch to Flags(u32) as Daniel Sedlak suggested. Then
it’s a matter of casting once for bindings::request_irq().

— Daniel

Re: [PATCH v6 2/6] rust: irq: add flags module

[Alice Ryhl]

On Sat, Jul 12, 2025 at 6:27 PM Daniel Almeida
<daniel.almeida@collabora.com> wrote:
>
> Hi Alice,
>
> > On 4 Jul 2025, at 04:42, Alice Ryhl <aliceryhl@google.com> wrote:
> >
> > On Fri, Jul 04, 2025 at 08:14:11AM +0200, Daniel Sedlak wrote:
> >> Hi Daniel,
> >>
> >> On 7/3/25 9:30 PM, Daniel Almeida wrote:
> >>> +/// Flags to be used when registering IRQ handlers.
> >>> +///
> >>> +/// They can be combined with the operators `|`, `&`, and `!`.
> >>> +#[derive(Clone, Copy, PartialEq, Eq)]
> >>> +pub struct Flags(u64);
> >>
> >> Why not Flags(u32)? You may get rid of all unnecessary casts later, plus
> >> save some extra bytes.
> >
> > It looks like the C methods take an `unsigned long`. In that case, I'd
> > probably write the code to match that.
> >
> > pub struct Flags(c_ulong);
> >
> > and git rid of the cast when calling bindings::request_irq.
> >
> > As for all the constants in this file, maybe it would be nice with a
> > private constructor that uses the same type as bindings to avoid the
> > casts?
> >
> > impl Flags {
> >    const fn new(value: u32) -> Flags {
> >     ...
> >    }
> > }
>
>
> Sure, but what goes here? This has to be "value as c_ulong” anyways so it
> doesn’t really reduce the number of casts.
>
> We should probably switch to Flags(u32) as Daniel Sedlak suggested. Then
> it’s a matter of casting once for bindings::request_irq().

IMO the advantage of doing it here is that we can fail compilation if
the cast is out of bounds, whereas the other cast is at runtime so we
can't do that.

Alice

Re: [PATCH v6 2/6] rust: irq: add flags module

[Daniel Almeida]

> On 12 Jul 2025, at 17:03, Alice Ryhl <aliceryhl@google.com> wrote:
> 
> On Sat, Jul 12, 2025 at 6:27 PM Daniel Almeida
> <daniel.almeida@collabora.com> wrote:
>> 
>> Hi Alice,
>> 
>>> On 4 Jul 2025, at 04:42, Alice Ryhl <aliceryhl@google.com> wrote:
>>> 
>>> On Fri, Jul 04, 2025 at 08:14:11AM +0200, Daniel Sedlak wrote:
>>>> Hi Daniel,
>>>> 
>>>> On 7/3/25 9:30 PM, Daniel Almeida wrote:
>>>>> +/// Flags to be used when registering IRQ handlers.
>>>>> +///
>>>>> +/// They can be combined with the operators `|`, `&`, and `!`.
>>>>> +#[derive(Clone, Copy, PartialEq, Eq)]
>>>>> +pub struct Flags(u64);
>>>> 
>>>> Why not Flags(u32)? You may get rid of all unnecessary casts later, plus
>>>> save some extra bytes.
>>> 
>>> It looks like the C methods take an `unsigned long`. In that case, I'd
>>> probably write the code to match that.
>>> 
>>> pub struct Flags(c_ulong);
>>> 
>>> and git rid of the cast when calling bindings::request_irq.
>>> 
>>> As for all the constants in this file, maybe it would be nice with a
>>> private constructor that uses the same type as bindings to avoid the
>>> casts?
>>> 
>>> impl Flags {
>>>   const fn new(value: u32) -> Flags {
>>>    ...
>>>   }
>>> }
>> 
>> 
>> Sure, but what goes here? This has to be "value as c_ulong” anyways so it
>> doesn’t really reduce the number of casts.
>> 
>> We should probably switch to Flags(u32) as Daniel Sedlak suggested. Then
>> it’s a matter of casting once for bindings::request_irq().
> 
> IMO the advantage of doing it here is that we can fail compilation if
> the cast is out of bounds, whereas the other cast is at runtime so we
> can't do that.
> 
> Alice

I’m not sure I am following. How is this compile-time checked?

>>> impl Flags {
>>>   const fn new(value: u32) -> Flags {
>>>    Self(value as c_ulong)
>>>   }

Or perhaps I misunderstood you?

— Daniel

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

[Danilo Krummrich]

On Thu Jul 3, 2025 at 9:30 PM CEST, Daniel Almeida wrote:
> +/// 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;
> +}

One thing I forgot, the IRQ handlers should have a &Device<Bound> argument,
i.e.:

	fn handle(&self, dev: &Device<Bound>) -> IrqReturn

IRQ registrations naturally give us this guarantee, so we should take advantage
of that.

- Danilo

Re: [PATCH v6 2/6] rust: irq: add flags module

[Alice Ryhl]

On Sat, Jul 12, 2025 at 10:49 PM Daniel Almeida
<daniel.almeida@collabora.com> wrote:
>
>
>
> > On 12 Jul 2025, at 17:03, Alice Ryhl <aliceryhl@google.com> wrote:
> >
> > On Sat, Jul 12, 2025 at 6:27 PM Daniel Almeida
> > <daniel.almeida@collabora.com> wrote:
> >>
> >> Hi Alice,
> >>
> >>> On 4 Jul 2025, at 04:42, Alice Ryhl <aliceryhl@google.com> wrote:
> >>>
> >>> On Fri, Jul 04, 2025 at 08:14:11AM +0200, Daniel Sedlak wrote:
> >>>> Hi Daniel,
> >>>>
> >>>> On 7/3/25 9:30 PM, Daniel Almeida wrote:
> >>>>> +/// Flags to be used when registering IRQ handlers.
> >>>>> +///
> >>>>> +/// They can be combined with the operators `|`, `&`, and `!`.
> >>>>> +#[derive(Clone, Copy, PartialEq, Eq)]
> >>>>> +pub struct Flags(u64);
> >>>>
> >>>> Why not Flags(u32)? You may get rid of all unnecessary casts later, plus
> >>>> save some extra bytes.
> >>>
> >>> It looks like the C methods take an `unsigned long`. In that case, I'd
> >>> probably write the code to match that.
> >>>
> >>> pub struct Flags(c_ulong);
> >>>
> >>> and git rid of the cast when calling bindings::request_irq.
> >>>
> >>> As for all the constants in this file, maybe it would be nice with a
> >>> private constructor that uses the same type as bindings to avoid the
> >>> casts?
> >>>
> >>> impl Flags {
> >>>   const fn new(value: u32) -> Flags {
> >>>    ...
> >>>   }
> >>> }
> >>
> >>
> >> Sure, but what goes here? This has to be "value as c_ulong” anyways so it
> >> doesn’t really reduce the number of casts.
> >>
> >> We should probably switch to Flags(u32) as Daniel Sedlak suggested. Then
> >> it’s a matter of casting once for bindings::request_irq().
> >
> > IMO the advantage of doing it here is that we can fail compilation if
> > the cast is out of bounds, whereas the other cast is at runtime so we
> > can't do that.
> >
> > Alice
>
> I’m not sure I am following. How is this compile-time checked?
>
> >>> impl Flags {
> >>>   const fn new(value: u32) -> Flags {
> >>>    Self(value as c_ulong)
> >>>   }
>
> Or perhaps I misunderstood you?

Well, that particular implementation would not be. But you could
implement it to compile-time check.

Alice

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

[Daniel Almeida]

> On 12 Jul 2025, at 18:24, Danilo Krummrich <dakr@kernel.org> wrote:
> 
> On Thu Jul 3, 2025 at 9:30 PM CEST, Daniel Almeida wrote:
>> +/// 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;
>> +}
> 
> One thing I forgot, the IRQ handlers should have a &Device<Bound> argument,
> i.e.:
> 
> fn handle(&self, dev: &Device<Bound>) -> IrqReturn
> 
> IRQ registrations naturally give us this guarantee, so we should take advantage
> of that.
> 
> - Danilo

Hi Danilo,

I do not immediately see a way to get a Device<Bound> from here:

unsafe extern "C" fn handle_irq_callback<T: Handler>(_irq: i32, ptr: *mut c_void) -> c_uint {

Refall that we've established `ptr` to be the address of the handler. This
came after some back and forth and after the extensive discussion that Benno
and Boqun had w.r.t to pinning in request_irq().

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

[Danilo Krummrich]

On Sun Jul 13, 2025 at 1:32 AM CEST, Daniel Almeida wrote:
>
>
>> On 12 Jul 2025, at 18:24, Danilo Krummrich <dakr@kernel.org> wrote:
>> 
>> On Thu Jul 3, 2025 at 9:30 PM CEST, Daniel Almeida wrote:
>>> +/// 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;
>>> +}
>> 
>> One thing I forgot, the IRQ handlers should have a &Device<Bound> argument,
>> i.e.:
>> 
>> fn handle(&self, dev: &Device<Bound>) -> IrqReturn
>> 
>> IRQ registrations naturally give us this guarantee, so we should take advantage
>> of that.
>> 
>> - Danilo
>
> Hi Danilo,
>
> I do not immediately see a way to get a Device<Bound> from here:
>
> unsafe extern "C" fn handle_irq_callback<T: Handler>(_irq: i32, ptr: *mut c_void) -> c_uint {
>
> Refall that we've established `ptr` to be the address of the handler. This
> came after some back and forth and after the extensive discussion that Benno
> and Boqun had w.r.t to pinning in request_irq().

You can just wrap the Handler in a new type and store the pointer there:

	#[pin_data]
	struct Wrapper {
	   #[pin]
	   handler: T,
	   dev: NonNull<Device<Bound>>,
	}

And then pass a pointer to the Wrapper field to request_irq();
handle_irq_callback() can construct a &T and a &Device<Bound> from this.

Note that storing a device pointer, without its own reference count, is
perfectly fine, since inner (Devres<RegistrationInner>) already holds a
reference to the device and guarantees the bound scope for the handler
callbacks.

It makes sense to document this as an invariant of Wrapper (or whatever we end
up calling it).

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

[Benno Lossin]

On Sun Jul 13, 2025 at 12:24 PM CEST, Danilo Krummrich wrote:
> On Sun Jul 13, 2025 at 1:32 AM CEST, Daniel Almeida wrote:
>>
>>
>>> On 12 Jul 2025, at 18:24, Danilo Krummrich <dakr@kernel.org> wrote:
>>> 
>>> On Thu Jul 3, 2025 at 9:30 PM CEST, Daniel Almeida wrote:
>>>> +/// 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;
>>>> +}
>>> 
>>> One thing I forgot, the IRQ handlers should have a &Device<Bound> argument,
>>> i.e.:
>>> 
>>> fn handle(&self, dev: &Device<Bound>) -> IrqReturn
>>> 
>>> IRQ registrations naturally give us this guarantee, so we should take advantage
>>> of that.
>>> 
>>> - Danilo
>>
>> Hi Danilo,
>>
>> I do not immediately see a way to get a Device<Bound> from here:
>>
>> unsafe extern "C" fn handle_irq_callback<T: Handler>(_irq: i32, ptr: *mut c_void) -> c_uint {
>>
>> Refall that we've established `ptr` to be the address of the handler. This
>> came after some back and forth and after the extensive discussion that Benno
>> and Boqun had w.r.t to pinning in request_irq().
>
> You can just wrap the Handler in a new type and store the pointer there:
>
> 	#[pin_data]
> 	struct Wrapper {
> 	   #[pin]
> 	   handler: T,
> 	   dev: NonNull<Device<Bound>>,
> 	}
>
> And then pass a pointer to the Wrapper field to request_irq();
> handle_irq_callback() can construct a &T and a &Device<Bound> from this.
>
> Note that storing a device pointer, without its own reference count, is
> perfectly fine, since inner (Devres<RegistrationInner>) already holds a
> reference to the device and guarantees the bound scope for the handler
> callbacks.

Can't we just add an accessor function to `Devres`?

Also `Devres` only stores `Device<Normal>`, not `Device<Bound>`...

---
Cheers,
Benno

> It makes sense to document this as an invariant of Wrapper (or whatever we end
> up calling it).

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

[Danilo Krummrich]

On Sun Jul 13, 2025 at 1:19 PM CEST, Benno Lossin wrote:
> On Sun Jul 13, 2025 at 12:24 PM CEST, Danilo Krummrich wrote:
>> On Sun Jul 13, 2025 at 1:32 AM CEST, Daniel Almeida wrote:
>>>
>>>
>>>> On 12 Jul 2025, at 18:24, Danilo Krummrich <dakr@kernel.org> wrote:
>>>> 
>>>> On Thu Jul 3, 2025 at 9:30 PM CEST, Daniel Almeida wrote:
>>>>> +/// 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;
>>>>> +}
>>>> 
>>>> One thing I forgot, the IRQ handlers should have a &Device<Bound> argument,
>>>> i.e.:
>>>> 
>>>> fn handle(&self, dev: &Device<Bound>) -> IrqReturn
>>>> 
>>>> IRQ registrations naturally give us this guarantee, so we should take advantage
>>>> of that.
>>>> 
>>>> - Danilo
>>>
>>> Hi Danilo,
>>>
>>> I do not immediately see a way to get a Device<Bound> from here:
>>>
>>> unsafe extern "C" fn handle_irq_callback<T: Handler>(_irq: i32, ptr: *mut c_void) -> c_uint {
>>>
>>> Refall that we've established `ptr` to be the address of the handler. This
>>> came after some back and forth and after the extensive discussion that Benno
>>> and Boqun had w.r.t to pinning in request_irq().
>>
>> You can just wrap the Handler in a new type and store the pointer there:
>>
>> 	#[pin_data]
>> 	struct Wrapper {
>> 	   #[pin]
>> 	   handler: T,
>> 	   dev: NonNull<Device<Bound>>,
>> 	}
>>
>> And then pass a pointer to the Wrapper field to request_irq();
>> handle_irq_callback() can construct a &T and a &Device<Bound> from this.
>>
>> Note that storing a device pointer, without its own reference count, is
>> perfectly fine, since inner (Devres<RegistrationInner>) already holds a
>> reference to the device and guarantees the bound scope for the handler
>> callbacks.
>
> Can't we just add an accessor function to `Devres`?

	#[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,
	}

Currently we pass the address of handler to request_irq(), so this doesn't help,
hence my proposal to replace the above T with Wrapper (actually Wrapper<T>).

> Also `Devres` only stores `Device<Normal>`, not `Device<Bound>`...

The Devres instance itself may out-live device unbind, but it ensures that the
encapsulated data does not, hence it holds a reference count, i.e. ARef<Device>.

Device<Bound> or ARef<Device<Bound>> *never* exists, only &'a Device<Bound>
within a corresponding scope for which we can guarantee the device is bound.

In the proposed wrapper we can store a NonNull<Device<Bound>> though, because we
can safely give out a &Device<Bound> in the IRQ's handle() callback. This is
because:

  (1) RegistrationInner is guarded by Devres and guarantees that free_irq() is
      completed *before* the device is unbound.

  (2) It is guaranteed that the device pointer is valid because (1) guarantees
      it's even bound and because Devres<RegistrationInner> itself has a
      reference count.

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

[Benno Lossin]

On Sun Jul 13, 2025 at 1:57 PM CEST, Danilo Krummrich wrote:
> On Sun Jul 13, 2025 at 1:19 PM CEST, Benno Lossin wrote:
>> On Sun Jul 13, 2025 at 12:24 PM CEST, Danilo Krummrich wrote:
>>> On Sun Jul 13, 2025 at 1:32 AM CEST, Daniel Almeida wrote:
>>>>
>>>>
>>>>> On 12 Jul 2025, at 18:24, Danilo Krummrich <dakr@kernel.org> wrote:
>>>>> 
>>>>> On Thu Jul 3, 2025 at 9:30 PM CEST, Daniel Almeida wrote:
>>>>>> +/// 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;
>>>>>> +}
>>>>> 
>>>>> One thing I forgot, the IRQ handlers should have a &Device<Bound> argument,
>>>>> i.e.:
>>>>> 
>>>>> fn handle(&self, dev: &Device<Bound>) -> IrqReturn
>>>>> 
>>>>> IRQ registrations naturally give us this guarantee, so we should take advantage
>>>>> of that.
>>>>> 
>>>>> - Danilo
>>>>
>>>> Hi Danilo,
>>>>
>>>> I do not immediately see a way to get a Device<Bound> from here:
>>>>
>>>> unsafe extern "C" fn handle_irq_callback<T: Handler>(_irq: i32, ptr: *mut c_void) -> c_uint {
>>>>
>>>> Refall that we've established `ptr` to be the address of the handler. This
>>>> came after some back and forth and after the extensive discussion that Benno
>>>> and Boqun had w.r.t to pinning in request_irq().
>>>
>>> You can just wrap the Handler in a new type and store the pointer there:
>>>
>>> 	#[pin_data]
>>> 	struct Wrapper {
>>> 	   #[pin]
>>> 	   handler: T,
>>> 	   dev: NonNull<Device<Bound>>,
>>> 	}
>>>
>>> And then pass a pointer to the Wrapper field to request_irq();
>>> handle_irq_callback() can construct a &T and a &Device<Bound> from this.
>>>
>>> Note that storing a device pointer, without its own reference count, is
>>> perfectly fine, since inner (Devres<RegistrationInner>) already holds a
>>> reference to the device and guarantees the bound scope for the handler
>>> callbacks.
>>
>> Can't we just add an accessor function to `Devres`?
>
> 	#[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,
> 	}
>
> Currently we pass the address of handler to request_irq(), so this doesn't help,
> hence my proposal to replace the above T with Wrapper (actually Wrapper<T>).

You can just use `container_of!`?

>> Also `Devres` only stores `Device<Normal>`, not `Device<Bound>`...
>
> The Devres instance itself may out-live device unbind, but it ensures that the
> encapsulated data does not, hence it holds a reference count, i.e. ARef<Device>.
>
> Device<Bound> or ARef<Device<Bound>> *never* exists, only &'a Device<Bound>
> within a corresponding scope for which we can guarantee the device is bound.
>
> In the proposed wrapper we can store a NonNull<Device<Bound>> though, because we
> can safely give out a &Device<Bound> in the IRQ's handle() callback. This is
> because:
>
>   (1) RegistrationInner is guarded by Devres and guarantees that free_irq() is
>       completed *before* the device is unbound.

How does it ensure that?

>
>   (2) It is guaranteed that the device pointer is valid because (1) guarantees
>       it's even bound and because Devres<RegistrationInner> itself has a
>       reference count.

Yeah but I would find it much more natural (and also useful in other
circumstances) if `Devres<T>` would give you access to `Device` (at
least the `Normal` type state).

Depending on how (1) is ensured, we might just need an unsafe function
that turns `Device<Normal>` into `Device<Bound>`.

---
Cheers,
Benno

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

[Danilo Krummrich]

On Sun Jul 13, 2025 at 2:16 PM CEST, Benno Lossin wrote:
> On Sun Jul 13, 2025 at 1:57 PM CEST, Danilo Krummrich wrote:
>> On Sun Jul 13, 2025 at 1:19 PM CEST, Benno Lossin wrote:
>>> On Sun Jul 13, 2025 at 12:24 PM CEST, Danilo Krummrich wrote:
>>>> On Sun Jul 13, 2025 at 1:32 AM CEST, Daniel Almeida wrote:
>>>>>
>>>>>
>>>>>> On 12 Jul 2025, at 18:24, Danilo Krummrich <dakr@kernel.org> wrote:
>>>>>> 
>>>>>> On Thu Jul 3, 2025 at 9:30 PM CEST, Daniel Almeida wrote:
>>>>>>> +/// 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;
>>>>>>> +}
>>>>>> 
>>>>>> One thing I forgot, the IRQ handlers should have a &Device<Bound> argument,
>>>>>> i.e.:
>>>>>> 
>>>>>> fn handle(&self, dev: &Device<Bound>) -> IrqReturn
>>>>>> 
>>>>>> IRQ registrations naturally give us this guarantee, so we should take advantage
>>>>>> of that.
>>>>>> 
>>>>>> - Danilo
>>>>>
>>>>> Hi Danilo,
>>>>>
>>>>> I do not immediately see a way to get a Device<Bound> from here:
>>>>>
>>>>> unsafe extern "C" fn handle_irq_callback<T: Handler>(_irq: i32, ptr: *mut c_void) -> c_uint {
>>>>>
>>>>> Refall that we've established `ptr` to be the address of the handler. This
>>>>> came after some back and forth and after the extensive discussion that Benno
>>>>> and Boqun had w.r.t to pinning in request_irq().
>>>>
>>>> You can just wrap the Handler in a new type and store the pointer there:
>>>>
>>>> 	#[pin_data]
>>>> 	struct Wrapper {
>>>> 	   #[pin]
>>>> 	   handler: T,
>>>> 	   dev: NonNull<Device<Bound>>,
>>>> 	}
>>>>
>>>> And then pass a pointer to the Wrapper field to request_irq();
>>>> handle_irq_callback() can construct a &T and a &Device<Bound> from this.
>>>>
>>>> Note that storing a device pointer, without its own reference count, is
>>>> perfectly fine, since inner (Devres<RegistrationInner>) already holds a
>>>> reference to the device and guarantees the bound scope for the handler
>>>> callbacks.
>>>
>>> Can't we just add an accessor function to `Devres`?
>>
>> 	#[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,
>> 	}
>>
>> Currently we pass the address of handler to request_irq(), so this doesn't help,
>> hence my proposal to replace the above T with Wrapper (actually Wrapper<T>).
>
> You can just use `container_of!`?

Sure, that's possible too.

>>> Also `Devres` only stores `Device<Normal>`, not `Device<Bound>`...
>>
>> The Devres instance itself may out-live device unbind, but it ensures that the
>> encapsulated data does not, hence it holds a reference count, i.e. ARef<Device>.
>>
>> Device<Bound> or ARef<Device<Bound>> *never* exists, only &'a Device<Bound>
>> within a corresponding scope for which we can guarantee the device is bound.
>>
>> In the proposed wrapper we can store a NonNull<Device<Bound>> though, because we
>> can safely give out a &Device<Bound> in the IRQ's handle() callback. This is
>> because:
>>
>>   (1) RegistrationInner is guarded by Devres and guarantees that free_irq() is
>>       completed *before* the device is unbound.
>
> How does it ensure that?

RegistrationInner calls free_irq() in it's drop() method; Devres revokes it on
device unbind.

>>
>>   (2) It is guaranteed that the device pointer is valid because (1) guarantees
>>       it's even bound and because Devres<RegistrationInner> itself has a
>>       reference count.
>
> Yeah but I would find it much more natural (and also useful in other
> circumstances) if `Devres<T>` would give you access to `Device` (at
> least the `Normal` type state).

If we use container_of!() instead or just pass the address of Self (i.e.
Registration) to request_irq() instead,

	pub fn device(&self) -> &Device

is absolutely possible to add to Devres, of course.

> Depending on how (1) is ensured, we might just need an unsafe function
> that turns `Device<Normal>` into `Device<Bound>`.

`&Device<Normal>` in `&Device<Bound>`, yes. I have such a method locally
already (but haven't sent it yet), because that's going to be a use-case for
other abstractions as well. One specific example is the PWM Chip abstraction
[1].

[1] https://lore.kernel.org/lkml/20250710-rust-next-pwm-working-fan-for-sending-v11-3-93824a16f9ec@samsung.com/

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

[Daniel Almeida]

On a second look, I wonder how useful this will be.

 fn handle(&self, dev: &Device<Bound>) -> IrqReturn

Sorry for borrowing this terminology, but here we offer Device<Bound>, while I
suspect that most drivers will be looking for the most derived Device type
instead. So for drm drivers this will be drm::Device, for example, not the base
dev::Device type. I assume that this pattern will hold for other subsystems as
well.

Which brings me to my second point: drivers can store an ARef<drm::Device> on
the handler itself, and I assume that the same will be possible in other
subsystems.

-- Daniel

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

[Danilo Krummrich]

On Sun Jul 13, 2025 at 4:09 PM CEST, Daniel Almeida wrote:
> On a second look, I wonder how useful this will be.
>
>  fn handle(&self, dev: &Device<Bound>) -> IrqReturn
>
> Sorry for borrowing this terminology, but here we offer Device<Bound>, while I
> suspect that most drivers will be looking for the most derived Device type
> instead. So for drm drivers this will be drm::Device, for example, not the base
> dev::Device type. I assume that this pattern will hold for other subsystems as
> well.
>
> Which brings me to my second point: drivers can store an ARef<drm::Device> on
> the handler itself, and I assume that the same will be possible in other
> subsystems.

Well, the whole point is that you can use a &Device<Bound> to directly access
device resources without any overhead, i.e.

	fn handle(&self, dev: &Device<Bound>) -> IrqReturn {
	   let io = self.iomem.access(dev);

	   io.write32(...);
	}

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

[Danilo Krummrich]

On Sun Jul 13, 2025 at 4:19 PM CEST, Danilo Krummrich wrote:
> On Sun Jul 13, 2025 at 4:09 PM CEST, Daniel Almeida wrote:
>> On a second look, I wonder how useful this will be.
>>
>>  fn handle(&self, dev: &Device<Bound>) -> IrqReturn
>>
>> Sorry for borrowing this terminology, but here we offer Device<Bound>, while I
>> suspect that most drivers will be looking for the most derived Device type
>> instead. So for drm drivers this will be drm::Device, for example, not the base
>> dev::Device type. I assume that this pattern will hold for other subsystems as
>> well.
>>
>> Which brings me to my second point: drivers can store an ARef<drm::Device> on
>> the handler itself, and I assume that the same will be possible in other
>> subsystems.
>
> Well, the whole point is that you can use a &Device<Bound> to directly access
> device resources without any overhead, i.e.
>
> 	fn handle(&self, dev: &Device<Bound>) -> IrqReturn {
> 	   let io = self.iomem.access(dev);
>
> 	   io.write32(...);
> 	}

So, yes, you can store anything in your handler, but the &Device<Bound> is a
cookie for the scope.

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

[Daniel Almeida]

> On 13 Jul 2025, at 11:27, Danilo Krummrich <dakr@kernel.org> wrote:
> 
> On Sun Jul 13, 2025 at 4:19 PM CEST, Danilo Krummrich wrote:
>> On Sun Jul 13, 2025 at 4:09 PM CEST, Daniel Almeida wrote:
>>> On a second look, I wonder how useful this will be.
>>> 
>>> fn handle(&self, dev: &Device<Bound>) -> IrqReturn
>>> 
>>> Sorry for borrowing this terminology, but here we offer Device<Bound>, while I
>>> suspect that most drivers will be looking for the most derived Device type
>>> instead. So for drm drivers this will be drm::Device, for example, not the base
>>> dev::Device type. I assume that this pattern will hold for other subsystems as
>>> well.
>>> 
>>> Which brings me to my second point: drivers can store an ARef<drm::Device> on
>>> the handler itself, and I assume that the same will be possible in other
>>> subsystems.
>> 
>> Well, the whole point is that you can use a &Device<Bound> to directly access
>> device resources without any overhead, i.e.
>> 
>> fn handle(&self, dev: &Device<Bound>) -> IrqReturn {
>>   let io = self.iomem.access(dev);
>> 
>>   io.write32(...);
>> }
> 
> So, yes, you can store anything in your handler, but the &Device<Bound> is a
> cookie for the scope.

Fine, but can’t you get a &Device<Bound> from a ARef<drm::Device>, for example?
Perhaps a nicer solution would be to offer this capability instead?

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

[Danilo Krummrich]

On Sun Jul 13, 2025 at 4:48 PM CEST, Daniel Almeida wrote:
>
>
>> On 13 Jul 2025, at 11:27, Danilo Krummrich <dakr@kernel.org> wrote:
>> 
>> On Sun Jul 13, 2025 at 4:19 PM CEST, Danilo Krummrich wrote:
>>> On Sun Jul 13, 2025 at 4:09 PM CEST, Daniel Almeida wrote:
>>>> On a second look, I wonder how useful this will be.
>>>> 
>>>> fn handle(&self, dev: &Device<Bound>) -> IrqReturn
>>>> 
>>>> Sorry for borrowing this terminology, but here we offer Device<Bound>, while I
>>>> suspect that most drivers will be looking for the most derived Device type
>>>> instead. So for drm drivers this will be drm::Device, for example, not the base
>>>> dev::Device type. I assume that this pattern will hold for other subsystems as
>>>> well.
>>>> 
>>>> Which brings me to my second point: drivers can store an ARef<drm::Device> on
>>>> the handler itself, and I assume that the same will be possible in other
>>>> subsystems.
>>> 
>>> Well, the whole point is that you can use a &Device<Bound> to directly access
>>> device resources without any overhead, i.e.
>>> 
>>> fn handle(&self, dev: &Device<Bound>) -> IrqReturn {
>>>   let io = self.iomem.access(dev);
>>> 
>>>   io.write32(...);
>>> }
>> 
>> So, yes, you can store anything in your handler, but the &Device<Bound> is a
>> cookie for the scope.
>
> Fine, but can’t you get a &Device<Bound> from a ARef<drm::Device>, for example?
> Perhaps a nicer solution would be to offer this capability instead?

I think you're confusing quite some things here.

  (1) I'm talking about the bus device the IRQ is registered for (e.g. PCI,
      platform, etc.). drm::Device represents a class device, which do not
      have DeviceContext states, such as Bound.

  (2) Owning a reference count of a device (i.e. ARef<Device>) does *not*
      guarantee that the device is bound. You can own a reference count to the
      device object way beyond it being bound. Instead, the guarantee comes from
      the scope.

      In this case, the scope is the IRQ callback, since the irq::Registration
      guarantees to call and complete free_irq() before the underlying bus
      device is unbound.

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

[Benno Lossin]

On Sun Jul 13, 2025 at 2:42 PM CEST, Danilo Krummrich wrote:
> On Sun Jul 13, 2025 at 2:16 PM CEST, Benno Lossin wrote:
>> On Sun Jul 13, 2025 at 1:57 PM CEST, Danilo Krummrich wrote:
>>> On Sun Jul 13, 2025 at 1:19 PM CEST, Benno Lossin wrote:
>>>> On Sun Jul 13, 2025 at 12:24 PM CEST, Danilo Krummrich wrote:
>>>>> On Sun Jul 13, 2025 at 1:32 AM CEST, Daniel Almeida wrote:
>>>>>>
>>>>>>
>>>>>>> On 12 Jul 2025, at 18:24, Danilo Krummrich <dakr@kernel.org> wrote:
>>>>>>> 
>>>>>>> On Thu Jul 3, 2025 at 9:30 PM CEST, Daniel Almeida wrote:
>>>>>>>> +/// 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;
>>>>>>>> +}
>>>>>>> 
>>>>>>> One thing I forgot, the IRQ handlers should have a &Device<Bound> argument,
>>>>>>> i.e.:
>>>>>>> 
>>>>>>> fn handle(&self, dev: &Device<Bound>) -> IrqReturn
>>>>>>> 
>>>>>>> IRQ registrations naturally give us this guarantee, so we should take advantage
>>>>>>> of that.
>>>>>>> 
>>>>>>> - Danilo
>>>>>>
>>>>>> Hi Danilo,
>>>>>>
>>>>>> I do not immediately see a way to get a Device<Bound> from here:
>>>>>>
>>>>>> unsafe extern "C" fn handle_irq_callback<T: Handler>(_irq: i32, ptr: *mut c_void) -> c_uint {
>>>>>>
>>>>>> Refall that we've established `ptr` to be the address of the handler. This
>>>>>> came after some back and forth and after the extensive discussion that Benno
>>>>>> and Boqun had w.r.t to pinning in request_irq().
>>>>>
>>>>> You can just wrap the Handler in a new type and store the pointer there:
>>>>>
>>>>> 	#[pin_data]
>>>>> 	struct Wrapper {
>>>>> 	   #[pin]
>>>>> 	   handler: T,
>>>>> 	   dev: NonNull<Device<Bound>>,
>>>>> 	}
>>>>>
>>>>> And then pass a pointer to the Wrapper field to request_irq();
>>>>> handle_irq_callback() can construct a &T and a &Device<Bound> from this.
>>>>>
>>>>> Note that storing a device pointer, without its own reference count, is
>>>>> perfectly fine, since inner (Devres<RegistrationInner>) already holds a
>>>>> reference to the device and guarantees the bound scope for the handler
>>>>> callbacks.
>>>>
>>>> Can't we just add an accessor function to `Devres`?
>>>
>>> 	#[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,
>>> 	}
>>>
>>> Currently we pass the address of handler to request_irq(), so this doesn't help,
>>> hence my proposal to replace the above T with Wrapper (actually Wrapper<T>).
>>
>> You can just use `container_of!`?
>
> Sure, that's possible too.
>
>>>> Also `Devres` only stores `Device<Normal>`, not `Device<Bound>`...
>>>
>>> The Devres instance itself may out-live device unbind, but it ensures that the
>>> encapsulated data does not, hence it holds a reference count, i.e. ARef<Device>.
>>>
>>> Device<Bound> or ARef<Device<Bound>> *never* exists, only &'a Device<Bound>
>>> within a corresponding scope for which we can guarantee the device is bound.
>>>
>>> In the proposed wrapper we can store a NonNull<Device<Bound>> though, because we
>>> can safely give out a &Device<Bound> in the IRQ's handle() callback. This is
>>> because:
>>>
>>>   (1) RegistrationInner is guarded by Devres and guarantees that free_irq() is
>>>       completed *before* the device is unbound.
>>
>> How does it ensure that?
>
> RegistrationInner calls free_irq() in it's drop() method; Devres revokes it on
> device unbind.

Makes sense, so we probably do need the unsafe typestate change
function in this case.

>>>
>>>   (2) It is guaranteed that the device pointer is valid because (1) guarantees
>>>       it's even bound and because Devres<RegistrationInner> itself has a
>>>       reference count.
>>
>> Yeah but I would find it much more natural (and also useful in other
>> circumstances) if `Devres<T>` would give you access to `Device` (at
>> least the `Normal` type state).
>
> If we use container_of!() instead or just pass the address of Self (i.e.
> Registration) to request_irq() instead,
>
> 	pub fn device(&self) -> &Device
>
> is absolutely possible to add to Devres, of course.
>
>> Depending on how (1) is ensured, we might just need an unsafe function
>> that turns `Device<Normal>` into `Device<Bound>`.
>
> `&Device<Normal>` in `&Device<Bound>`, yes. I have such a method locally
> already (but haven't sent it yet), because that's going to be a use-case for
> other abstractions as well. One specific example is the PWM Chip abstraction
> [1].
>
> [1] https://lore.kernel.org/lkml/20250710-rust-next-pwm-working-fan-for-sending-v11-3-93824a16f9ec@samsung.com/

I think this solution sounds much better than storing an additional
`NonNull<Device<Bound>>`.

---
Cheers,
Benno

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

[Daniel Almeida]

> On 13 Jul 2025, at 12:28, Daniel Almeida <daniel.almeida@collabora.com> wrote:
> 
> 
> 
>> 
>> (2) Owning a reference count of a device (i.e. ARef<Device>) does *not*
>>     guarantee that the device is bound. You can own a reference count to the
>>     device object way beyond it being bound. Instead, the guarantee comes from
>>     the scope.
>> 
>>     In this case, the scope is the IRQ callback, since the irq::Registration
>>     guarantees to call and complete free_irq() before the underlying bus
>>     device is unbound.
>> 
> 
> 
> Oh, I see. I guess this is where I started to get a bit confused indeed.
> 
> — Daniel

Fine, I guess I can submit a newer version and test that on Tyr.

Dirk, can you also test the next iteration on your driver? It will possibly
solve your use case as well.

— Daniel

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

[Danilo Krummrich]

On Sun Jul 13, 2025 at 5:29 PM CEST, Benno Lossin wrote:
> I think this solution sounds much better than storing an additional
> `NonNull<Device<Bound>>`.

I can send two patches for that. The IRQ abstractions have to either land
through driver-core or in the next cycle anyways.

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

[Boqun Feng]

On Sun, Jul 13, 2025 at 02:42:41PM +0200, Danilo Krummrich wrote:
[...]
> If we use container_of!() instead or just pass the address of Self (i.e.
> Registration) to request_irq() instead,
> 
> 	pub fn device(&self) -> &Device
> 
> is absolutely possible to add to Devres, of course.
> 

One thing to notice is that in `Devres::new()`, `inner` is initialized
before `dev`:

        try_pin_init!(&this in Self {
            // INVARIANT: `inner` is properly initialized.
            inner <- Opaque::pin_init(try_pin_init!(Inner {
                data <- Revocable::new(data),
                devm <- Completion::new(),
                revoke <- Completion::new(),
            })),
	    
For `irq::Registration`, request_irq() is called at `inner`
initialization. So now interrupts can happen at any moment, but `dev` is
still uninitialized.

            callback,
            dev: {
                // SAFETY: `this` is a valid pointer to uninitialized memory.
                let inner = unsafe { &raw mut (*this.as_ptr()).inner };

                // SAFETY:
                // - `dev.as_raw()` is a pointer to a valid bound device.
                // - `inner` is guaranteed to be a valid for the duration of the lifetime of `Self`.
                // - `devm_add_action()` is guaranteed not to call `callback` until `this` has been
                //    properly initialized, because we require `dev` (i.e. the *bound* device) to
                //    live at least as long as the returned `impl PinInit<Self, Error>`.
                to_result(unsafe {
                    bindings::devm_add_action(dev.as_raw(), Some(callback), inner.cast())
                })?;

                dev.into()
            },
        })

I think you need to reorder the initialization of `inner` to be after
`dev` for this. And it should be fine, because the whole device is in
bound state while the PinInit being called, so `inner.cast()` being a
pointer to uninitialized memory should be fine (because the `callback`
won't be called).

Regards,
Boqun

> > Depending on how (1) is ensured, we might just need an unsafe function
> > that turns `Device<Normal>` into `Device<Bound>`.
> 
> `&Device<Normal>` in `&Device<Bound>`, yes. I have such a method locally
> already (but haven't sent it yet), because that's going to be a use-case for
> other abstractions as well. One specific example is the PWM Chip abstraction
> [1].
> 
> [1] https://lore.kernel.org/lkml/20250710-rust-next-pwm-working-fan-for-sending-v11-3-93824a16f9ec@samsung.com/

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

[Dirk Behme]

On 13/07/2025 16:19, Danilo Krummrich wrote:
> On Sun Jul 13, 2025 at 4:09 PM CEST, Daniel Almeida wrote:
>> On a second look, I wonder how useful this will be.
>>
>>  fn handle(&self, dev: &Device<Bound>) -> IrqReturn
>>
>> Sorry for borrowing this terminology, but here we offer Device<Bound>, while I
>> suspect that most drivers will be looking for the most derived Device type
>> instead. So for drm drivers this will be drm::Device, for example, not the base
>> dev::Device type. I assume that this pattern will hold for other subsystems as
>> well.
>>
>> Which brings me to my second point: drivers can store an ARef<drm::Device> on
>> the handler itself, and I assume that the same will be possible in other
>> subsystems.
> 
> Well, the whole point is that you can use a &Device<Bound> to directly access
> device resources without any overhead, i.e.
> 
> 	fn handle(&self, dev: &Device<Bound>) -> IrqReturn {
> 	   let io = self.iomem.access(dev);
> 
> 	   io.write32(...);

As this is exactly the example I was discussing privately with Daniel
(many thanks!), independent on the device discussion here, just for my
understanding:

Is it ok to do a 'self.iomem.access(dev)' at each interrupt? Wouldn't it
be cheaper/faster to pass 'io' instead of 'iomem' to the interrupt handler?

fn handle(...) -> IrqReturn {

    self.io.write32(...);

?

Thanks

Dirk

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

[Danilo Krummrich]

On Mon Jul 14, 2025 at 8:42 AM CEST, Boqun Feng wrote:
> I think you need to reorder the initialization of `inner` to be after
> `dev` for this.

Indeed, good catch!

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

[Danilo Krummrich]

On Mon Jul 14, 2025 at 9:57 AM CEST, Dirk Behme wrote:
> On 13/07/2025 16:19, Danilo Krummrich wrote:
>> On Sun Jul 13, 2025 at 4:09 PM CEST, Daniel Almeida wrote:
>>> On a second look, I wonder how useful this will be.
>>>
>>>  fn handle(&self, dev: &Device<Bound>) -> IrqReturn
>>>
>>> Sorry for borrowing this terminology, but here we offer Device<Bound>, while I
>>> suspect that most drivers will be looking for the most derived Device type
>>> instead. So for drm drivers this will be drm::Device, for example, not the base
>>> dev::Device type. I assume that this pattern will hold for other subsystems as
>>> well.
>>>
>>> Which brings me to my second point: drivers can store an ARef<drm::Device> on
>>> the handler itself, and I assume that the same will be possible in other
>>> subsystems.
>> 
>> Well, the whole point is that you can use a &Device<Bound> to directly access
>> device resources without any overhead, i.e.
>> 
>> 	fn handle(&self, dev: &Device<Bound>) -> IrqReturn {
>> 	   let io = self.iomem.access(dev);
>> 
>> 	   io.write32(...);
>
> As this is exactly the example I was discussing privately with Daniel
> (many thanks!), independent on the device discussion here, just for my
> understanding:
>
> Is it ok to do a 'self.iomem.access(dev)' at each interrupt?

Absolutely, Devres::access() is a very cheap accessor, see also [1]. Compiled
down, the only thing that Revocable::access() does is deriving a pointer from
another pointer by adding an offset.

That's exactly why we want the &Device<Bound> cookie, to avoid more expensive
operations.

[1] https://rust.docs.kernel.org/src/kernel/revocable.rs.html#151

> Wouldn't it
> be cheaper/faster to pass 'io' instead of 'iomem' to the interrupt handler?

Well, consider the types of the example:

	iomem: Devres<IoMem<SIZE>>
	io: &IoMem<Size>

You can't store a reference with a non-static lifetime in something with an
open-ended lifetime, such as the Handler object.

How would you ensure that the reference is still valid? The Devres<IoMem<SIZE>>
object might have been dropped already, either by the user or by Devres revoking
the inner object due to device unbind.

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

[Danilo Krummrich]

On Mon Jul 14, 2025 at 11:24 AM CEST, Danilo Krummrich wrote:
> On Mon Jul 14, 2025 at 8:42 AM CEST, Boqun Feng wrote:
>> I think you need to reorder the initialization of `inner` to be after
>> `dev` for this.
>
> Indeed, good catch!

Just to add, it's more than the order of inner and dev. For such use-cases we
have to ensure that Devres as a whole is properly initialized once
Devres::inner::data is initialized.

Going to send a patch for that.

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

[Daniel Almeida]

Hi,

> 
>>> 
>>>  (2) It is guaranteed that the device pointer is valid because (1) guarantees
>>>      it's even bound and because Devres<RegistrationInner> itself has a
>>>      reference count.
>> 
>> Yeah but I would find it much more natural (and also useful in other
>> circumstances) if `Devres<T>` would give you access to `Device` (at
>> least the `Normal` type state).
> 
> If we use container_of!() instead or just pass the address of Self (i.e.
> Registration) to request_irq() instead,


Boqun, Benno, are you ok with passing the address of Registration<T> as the cookie?

Recall that this was a change requested in v4, so I am checking whether we are
all on the same page before going back to that.

See [0], i.e.:

> > > >> Well yes and no, with the Devres changes, the `cookie` can just be the
> > > >> address of the `RegistrationInner` & we can do it this way :)
> > > >>
> > > >> ---
> > > >> Cheers,
> > > >> Benno
> > > >
> > > >
> > > > No, we need this to be the address of the the whole thing (i.e.
> > > > Registration<T>), otherwise you can’t access the handler in the irq
> > > > callback.
> 
> You only need the access of `handler` in the irq callback, right? I.e.
> passing the address of `handler` would suffice (of course you need
> to change the irq callback as well).


— Daniel

[0] https://lore.kernel.org/all/aFq3P_4XgP0dUrAS@Mac.home/

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

[Benno Lossin]

On Mon Jul 14, 2025 at 5:12 PM CEST, Daniel Almeida wrote:
> Hi,
>
>> 
>>>> 
>>>>  (2) It is guaranteed that the device pointer is valid because (1) guarantees
>>>>      it's even bound and because Devres<RegistrationInner> itself has a
>>>>      reference count.
>>> 
>>> Yeah but I would find it much more natural (and also useful in other
>>> circumstances) if `Devres<T>` would give you access to `Device` (at
>>> least the `Normal` type state).
>> 
>> If we use container_of!() instead or just pass the address of Self (i.e.
>> Registration) to request_irq() instead,
>
>
> Boqun, Benno, are you ok with passing the address of Registration<T> as the cookie?
>
> Recall that this was a change requested in v4, so I am checking whether we are
> all on the same page before going back to that.

I looked at the conversation again and the important part is that you
aren't allowed to initialize the `RegistrationInner` before the
`request_irq` call was successful (because the drop will run
`irq_free`). What pointer you use as the cookie doesn't matter for this.

Feel free to double check again with the concrete code.

---
Cheers,
Benno

> See [0], i.e.:
>
>> > > >> Well yes and no, with the Devres changes, the `cookie` can just be the
>> > > >> address of the `RegistrationInner` & we can do it this way :)
>> > > >>
>> > > >> ---
>> > > >> Cheers,
>> > > >> Benno
>> > > >
>> > > >
>> > > > No, we need this to be the address of the the whole thing (i.e.
>> > > > Registration<T>), otherwise you can’t access the handler in the irq
>> > > > callback.
>> 
>> You only need the access of `handler` in the irq callback, right? I.e.
>> passing the address of `handler` would suffice (of course you need
>> to change the irq callback as well).
>
>
> — Daniel
>
> [0] https://lore.kernel.org/all/aFq3P_4XgP0dUrAS@Mac.home/

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

[Alice Ryhl]

On Mon, Jul 14, 2025 at 5:13 PM Daniel Almeida
<daniel.almeida@collabora.com> wrote:
>
> Hi,
>
> >
> >>>
> >>>  (2) It is guaranteed that the device pointer is valid because (1) guarantees
> >>>      it's even bound and because Devres<RegistrationInner> itself has a
> >>>      reference count.
> >>
> >> Yeah but I would find it much more natural (and also useful in other
> >> circumstances) if `Devres<T>` would give you access to `Device` (at
> >> least the `Normal` type state).
> >
> > If we use container_of!() instead or just pass the address of Self (i.e.
> > Registration) to request_irq() instead,
>
>
> Boqun, Benno, are you ok with passing the address of Registration<T> as the cookie?
>
> Recall that this was a change requested in v4, so I am checking whether we are
> all on the same page before going back to that.
>
> See [0], i.e.:
> [0] https://lore.kernel.org/all/aFq3P_4XgP0dUrAS@Mac.home/

After discussing this, Daniel and I agreed that I will implement the
change adding a Device<Bound> argument to the callback. I will be
sending a patch adding it separately as a follow-up to Daniel's work.

Alice

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

[Danilo Krummrich]

On Tue Jul 15, 2025 at 2:33 PM CEST, Alice Ryhl wrote:
> On Mon, Jul 14, 2025 at 5:13 PM Daniel Almeida
> <daniel.almeida@collabora.com> wrote:
>>
>> Hi,
>>
>> >
>> >>>
>> >>>  (2) It is guaranteed that the device pointer is valid because (1) guarantees
>> >>>      it's even bound and because Devres<RegistrationInner> itself has a
>> >>>      reference count.
>> >>
>> >> Yeah but I would find it much more natural (and also useful in other
>> >> circumstances) if `Devres<T>` would give you access to `Device` (at
>> >> least the `Normal` type state).
>> >
>> > If we use container_of!() instead or just pass the address of Self (i.e.
>> > Registration) to request_irq() instead,
>>
>>
>> Boqun, Benno, are you ok with passing the address of Registration<T> as the cookie?
>>
>> Recall that this was a change requested in v4, so I am checking whether we are
>> all on the same page before going back to that.
>>
>> See [0], i.e.:
>> [0] https://lore.kernel.org/all/aFq3P_4XgP0dUrAS@Mac.home/
>
> After discussing this, Daniel and I agreed that I will implement the
> change adding a Device<Bound> argument to the callback. I will be
> sending a patch adding it separately as a follow-up to Daniel's work.

That works for me, thanks!

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

[Dirk Behme]

Hi Daniel,

On 13.07.25 17:32, Daniel Almeida wrote:
> 
> 
>> On 13 Jul 2025, at 12:28, Daniel Almeida <daniel.almeida@collabora.com> wrote:
>>
>>
>>
>>>
>>> (2) Owning a reference count of a device (i.e. ARef<Device>) does *not*
>>>     guarantee that the device is bound. You can own a reference count to the
>>>     device object way beyond it being bound. Instead, the guarantee comes from
>>>     the scope.
>>>
>>>     In this case, the scope is the IRQ callback, since the irq::Registration
>>>     guarantees to call and complete free_irq() before the underlying bus
>>>     device is unbound.
>>>
>>
>>
>> Oh, I see. I guess this is where I started to get a bit confused indeed.
>>
>> — Daniel
> 
> Fine, I guess I can submit a newer version and test that on Tyr.
> 
> Dirk, can you also test the next iteration on your driver? It will possibly
> solve your use case as well.


Now, I'm slightly confused ;) I just saw your version 7 of this

[PATCH v7 3/6] rust: irq: add support for non-threaded IRQs and handlers
https://lore.kernel.org/rust-for-linux/20250715-topics-tyr-request_irq2-v7-3-d469c0f37c07@collabora.com/

and somehow was expecting something like

fn handle(&self, dev: &Device<Bound>) -> IrqReturn

there. I.e. to get a bound device passed into the handler.

If I misunderstood the discussion and this is supposed to not be
added: Any hint how to get the &Device<Bound> from the probe
function/irq registration into the irq handler, then? To be able to do
something like

fn handle(&self) -> IrqReturn {
  let dev = ??;
  let io = self.iomem.access(dev);

Thanks,

Dirk

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

[Alice Ryhl]

On Sat, Jul 19, 2025 at 7:47 AM Dirk Behme <dirk.behme@gmail.com> wrote:
>
> Hi Daniel,
>
> On 13.07.25 17:32, Daniel Almeida wrote:
> >
> >
> >> On 13 Jul 2025, at 12:28, Daniel Almeida <daniel.almeida@collabora.com> wrote:
> >>
> >>
> >>
> >>>
> >>> (2) Owning a reference count of a device (i.e. ARef<Device>) does *not*
> >>>     guarantee that the device is bound. You can own a reference count to the
> >>>     device object way beyond it being bound. Instead, the guarantee comes from
> >>>     the scope.
> >>>
> >>>     In this case, the scope is the IRQ callback, since the irq::Registration
> >>>     guarantees to call and complete free_irq() before the underlying bus
> >>>     device is unbound.
> >>>
> >>
> >>
> >> Oh, I see. I guess this is where I started to get a bit confused indeed.
> >>
> >> — Daniel
> >
> > Fine, I guess I can submit a newer version and test that on Tyr.
> >
> > Dirk, can you also test the next iteration on your driver? It will possibly
> > solve your use case as well.
>
>
> Now, I'm slightly confused ;) I just saw your version 7 of this
>
> [PATCH v7 3/6] rust: irq: add support for non-threaded IRQs and handlers
> https://lore.kernel.org/rust-for-linux/20250715-topics-tyr-request_irq2-v7-3-d469c0f37c07@collabora.com/
>
> and somehow was expecting something like
>
> fn handle(&self, dev: &Device<Bound>) -> IrqReturn
>
> there. I.e. to get a bound device passed into the handler.
>
> If I misunderstood the discussion and this is supposed to not be
> added: Any hint how to get the &Device<Bound> from the probe
> function/irq registration into the irq handler, then? To be able to do
> something like
>
> fn handle(&self) -> IrqReturn {
>   let dev = ??;
>   let io = self.iomem.access(dev);

Please see:
https://lore.kernel.org/all/CAH5fLgibCtmgFpKNrC+jcSEqSUctyVMuYwEC0QSo+vzyDXK0zg@mail.gmail.com/

Alice

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

[Daniel Almeida]

Hi Dirk,

> 
> fn handle(&self) -> IrqReturn {
>  let dev = ??;
>  let io = self.iomem.access(dev);
> 
> Thanks,
> 
> Dirk

FYI, until there is a patch to provide Device<Bound> as an argument, you can
resort to the slower try_access().

— Daniel