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

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

[Daniel Almeida]

---
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/kernel/irq.rs              |  21 ++
 rust/kernel/irq/flags.rs        | 102 ++++++++
 rust/kernel/irq/request.rs      | 515 ++++++++++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs              |   1 +
 rust/kernel/pci.rs              |  35 +++
 rust/kernel/platform.rs         | 127 +++++++++-
 9 files changed, 810 insertions(+), 2 deletions(-)
---
base-commit: e271ed52b344ac02d4581286961d0c40acc54c03
change-id: 20250514-topics-tyr-request_irq-4f6a30837ea8

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

[PATCH v4 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.49.0

[PATCH v4 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.49.0

[PATCH v4 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 defined as pub(crate) so that future patches can
define proper (safe) accessors for the IRQs exposed by a device. Said
accessors are bus-specific.

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

diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index bc494745f67b82e7a3a6f53055ece0fc3acf6e0d..32b95df509f1aec2d05035a1c49ec262d1ed7624 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -50,6 +50,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 0f1b5d11598591bc62bb6439747211af164b76d6..25c927264835271f84d8c95d79f4ad6a381a7071 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c
@@ -20,6 +20,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..650c9409a86ba25dfc2453cd10350f299de2450d 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, IrqReturn, Registration};
diff --git a/rust/kernel/irq/request.rs b/rust/kernel/irq/request.rs
new file mode 100644
index 0000000000000000000000000000000000000000..e0bf8ca192e7b27c6dbfe611c3a6cc8df8153c35
--- /dev/null
+++ b/rust/kernel/irq/request.rs
@@ -0,0 +1,259 @@
+// SPDX-License-Identifier: GPL-2.0
+// SPDX-FileCopyrightText: Copyright 2025 Collabora ltd.
+
+//! This module provides types like [`Registration`] and
+//! [`ThreadedRegistration`], which allow users to register handlers for a given
+//! IRQ line.
+
+use core::marker::PhantomPinned;
+
+use pin_init::pin_init_from_closure;
+
+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 actual handler function. As usual, sleeps are not allowed in IRQ
+    /// context.
+    fn handle_irq(&self) -> IrqReturn;
+}
+
+impl<T: ?Sized + Handler + Send> Handler for Arc<T> {
+    fn handle_irq(&self) -> IrqReturn {
+        T::handle_irq(self)
+    }
+}
+
+impl<T: ?Sized + Handler, A: Allocator> Handler for Box<T, A> {
+    fn handle_irq(&self) -> IrqReturn {
+        T::handle_irq(self)
+    }
+}
+
+struct RegistrationInner {
+    irq: u32,
+    cookie: *mut kernel::ffi::c_void,
+}
+
+impl RegistrationInner {
+    fn synchronize(&self) {
+        // SAFETY:
+        // - `self.irq` is the same as the one passed to `request_{threaded}_irq`.
+        unsafe { bindings::synchronize_irq(self.irq) };
+    }
+}
+
+impl Drop for RegistrationInner {
+    fn drop(&mut self) {
+        // SAFETY:
+        // - `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
+        // `register` will return a different instance of `Registration`.
+        //
+        // - `&self` is `!Unpin` and was initializing 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) };
+    }
+}
+
+/// 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::IrqReturn;
+/// use kernel::platform;
+/// 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);
+///
+/// // [`handle_irq`] takes &self. This example illustrates interior
+/// // mutability can be used when share 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_irq(&self) -> IrqReturn {
+///         self.0.fetch_add(1, Ordering::Relaxed);
+///
+///         IrqReturn::Handled
+///     }
+/// }
+///
+/// // This is running in process context.
+/// fn register_irq(handler: Handler, dev: &platform::Device<Bound>) -> Result<Arc<Registration<Handler>>> {
+///     let registration = dev.irq_by_index(0, flags::SHARED, c_str!("my-device"), handler)?;
+///
+///     // You can have as many references to the registration as you want, so
+///     // multiple parts of the driver can access it.
+///     let registration = Arc::pin_init(registration, GFP_KERNEL)?;
+///
+///     // The handler may be called immediately after the function above
+///     // returns, possibly in a different CPU.
+///
+///     {
+///         // The data can be accessed from the process context too.
+///         registration.handler().0.fetch_add(1, Ordering::Relaxed);
+///     }
+///
+///     Ok(registration)
+/// }
+///
+/// # Ok::<(), Error>(())
+///```
+///
+/// # Invariants
+///
+/// * We own an irq handler using `&self` as its private data.
+///
+#[pin_data]
+pub struct Registration<T: Handler + 'static> {
+    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(crate) fn register<'a>(
+        dev: &'a Device<Bound>,
+        irq: u32,
+        flags: Flags,
+        name: &'static CStr,
+        handler: T,
+    ) -> impl PinInit<Self, Error> + 'a {
+        let closure = move |slot: *mut Self| {
+            // SAFETY: The slot passed to pin initializer is valid for writing.
+            unsafe {
+                slot.write(Self {
+                    inner: Devres::new(
+                        dev,
+                        RegistrationInner {
+                            irq,
+                            cookie: slot.cast(),
+                        },
+                        GFP_KERNEL,
+                    )?,
+                    handler,
+                    _pin: PhantomPinned,
+                })
+            };
+
+            // 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.
+            let res = to_result(unsafe {
+                bindings::request_irq(
+                    irq,
+                    Some(handle_irq_callback::<T>),
+                    flags.into_inner() as usize,
+                    name.as_char_ptr(),
+                    slot.cast(),
+                )
+            });
+
+            if res.is_err() {
+                // SAFETY: We are returning an error, so we can destroy the slot.
+                unsafe { core::ptr::drop_in_place(&raw mut (*slot).handler) };
+            }
+
+            res
+        };
+
+        // SAFETY:
+        // - if this returns Ok, then every field of `slot` is fully
+        // initialized.
+        // - if this returns an error, then the slot does not need to remain
+        // valid.
+        unsafe { pin_init_from_closure(closure) }
+    }
+
+    /// 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 Registration<T> set in `Registration::new`
+    let data = unsafe { &*(ptr as *const Registration<T>) };
+    T::handle_irq(&data.handler).into_inner()
+}

-- 
2.49.0

[PATCH v4 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 an IRQ is defined as pub(crate) so that future patches can
define proper (safe) accessors for the IRQs exposed by a device. Said
accessors are bus-specific.

Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
---
 rust/kernel/irq.rs         |   4 +-
 rust/kernel/irq/request.rs | 256 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 259 insertions(+), 1 deletion(-)

diff --git a/rust/kernel/irq.rs b/rust/kernel/irq.rs
index 650c9409a86ba25dfc2453cd10350f299de2450d..3a762069df210e0c7a833529b29864b0c90e2483 100644
--- a/rust/kernel/irq.rs
+++ b/rust/kernel/irq.rs
@@ -16,4 +16,6 @@
 /// IRQ allocation and handling.
 pub mod request;
 
-pub use request::{Handler, IrqReturn, Registration};
+pub use request::{
+    Handler, IrqReturn, Registration, ThreadedHandler, ThreadedIrqReturn, ThreadedRegistration,
+};
diff --git a/rust/kernel/irq/request.rs b/rust/kernel/irq/request.rs
index e0bf8ca192e7b27c6dbfe611c3a6cc8df8153c35..37bbffe6c982ce0a9424f9dfcbd5e9b98766160b 100644
--- a/rust/kernel/irq/request.rs
+++ b/rust/kernel/irq/request.rs
@@ -257,3 +257,259 @@ pub fn synchronize(&self, dev: &Device<Bound>) -> Result {
     let data = unsafe { &*(ptr as *const Registration<T>) };
     T::handle_irq(&data.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 actual handler function. As usual, sleeps are not allowed in IRQ
+    /// context.
+    fn handle_irq(&self) -> ThreadedIrqReturn;
+
+    /// The threaded handler function. This function is called from the irq
+    /// handler thread, which is automatically created by the system.
+    fn thread_fn(&self) -> IrqReturn;
+}
+
+impl<T: ?Sized + ThreadedHandler + Send> ThreadedHandler for Arc<T> {
+    fn handle_irq(&self) -> ThreadedIrqReturn {
+        T::handle_irq(self)
+    }
+
+    fn thread_fn(&self) -> IrqReturn {
+        T::thread_fn(self)
+    }
+}
+
+impl<T: ?Sized + ThreadedHandler, A: Allocator> ThreadedHandler for Box<T, A> {
+    fn handle_irq(&self) -> ThreadedIrqReturn {
+        T::handle_irq(self)
+    }
+
+    fn thread_fn(&self) -> IrqReturn {
+        T::thread_fn(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::IrqReturn;
+/// use kernel::platform;
+/// use kernel::sync::Arc;
+/// use kernel::sync::SpinLock;
+/// use kernel::alloc::flags::GFP_KERNEL;
+/// use kernel::c_str;
+///
+/// // 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);
+///
+/// // [`handle_irq`] takes &self. This example illustrates interior
+/// // mutability can be used when share 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 to data.
+///     fn handle_irq(&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_irq`
+///     // returns `WakeThread`.
+///     fn thread_fn(&self) -> IrqReturn {
+///         self.0.fetch_add(1, Ordering::Relaxed);
+///
+///         IrqReturn::Handled
+///     }
+/// }
+///
+/// // This is running in process context.
+/// fn register_threaded_irq(handler: Handler, dev: &platform::Device<Bound>) -> Result<Arc<ThreadedRegistration<Handler>>> {
+///     let registration = dev.threaded_irq_by_index(0, flags::SHARED, c_str!("my-device"), handler)?;
+///
+///     // You can have as many references to the registration as you want, so
+///     // multiple parts of the driver can access it.
+///     let registration = Arc::pin_init(registration, GFP_KERNEL)?;
+///
+///     // The handler may be called immediately after the function above
+///     // returns, possibly in a different CPU.
+///
+///     {
+///         // The data can be accessed from the process context too.
+///         registration.handler().0.fetch_add(1, Ordering::Relaxed);
+///     }
+///
+///     Ok(registration)
+/// }
+///
+///
+/// # Ok::<(), Error>(())
+///```
+///
+/// # Invariants
+///
+/// * We own an irq handler using `&self` as its private data.
+///
+#[pin_data]
+pub struct ThreadedRegistration<T: ThreadedHandler + 'static> {
+    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(crate) fn register<'a>(
+        dev: &'a Device<Bound>,
+        irq: u32,
+        flags: Flags,
+        name: &'static CStr,
+        handler: T,
+    ) -> impl PinInit<Self, Error> + 'a {
+        let closure = move |slot: *mut Self| {
+            // SAFETY: The slot passed to pin initializer is valid for writing.
+            unsafe {
+                slot.write(Self {
+                    inner: Devres::new(
+                        dev,
+                        RegistrationInner {
+                            irq,
+                            cookie: slot.cast(),
+                        },
+                        GFP_KERNEL,
+                    )?,
+                    handler,
+                    _pin: PhantomPinned,
+                })
+            };
+
+            // 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.
+            let res = to_result(unsafe {
+                bindings::request_threaded_irq(
+                    irq,
+                    Some(handle_threaded_irq_callback::<T>),
+                    Some(thread_fn_callback::<T>),
+                    flags.into_inner() as usize,
+                    name.as_char_ptr(),
+                    slot.cast(),
+                )
+            });
+
+            if res.is_err() {
+                // SAFETY: We are returning an error, so we can destroy the slot.
+                unsafe { core::ptr::drop_in_place(&raw mut (*slot).handler) };
+            }
+
+            res
+        };
+
+        // SAFETY:
+        // - if this returns Ok(()), then every field of `slot` is fully
+        // initialized.
+        // - if this returns an error, then the slot does not need to remain
+        // valid.
+        unsafe { pin_init_from_closure(closure) }
+    }
+
+    /// 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 ThreadedRegistration<T> set in `ThreadedRegistration::new`
+    let data = unsafe { &*(ptr as *const ThreadedRegistration<T>) };
+    T::handle_irq(&data.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 ThreadedRegistration<T> set in `ThreadedRegistration::new`
+    let data = unsafe { &*(ptr as *const ThreadedRegistration<T>) };
+    T::thread_fn(&data.handler).into_inner()
+}

-- 
2.49.0

[PATCH v4 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.

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 | 127 +++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 125 insertions(+), 2 deletions(-)

diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs
index 5b21fa517e55348582622ec10471918919502959..3a97e40b2da75f1a48fade3e61760a5814f34653 100644
--- a/rust/kernel/platform.rs
+++ b/rust/kernel/platform.rs
@@ -5,9 +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},
-    of,
+    irq, of,
     prelude::*,
     str::CStr,
     types::{ForeignOwnable, Opaque},
@@ -190,6 +192,127 @@ fn as_raw(&self) -> *mut bindings::platform_device {
     }
 }
 
+macro_rules! gen_irq_accessor {
+    ($(#[$meta:meta])* $fn_name:ident, $reg_type:ident, $handler_trait:ident, index, $irq_fn:ident) => {
+        $(#[$meta])*
+        pub fn $fn_name<T: irq::$handler_trait + 'static>(
+            &self,
+            index: u32,
+            flags: irq::flags::Flags,
+            name: &'static CStr,
+            handler: T,
+        ) -> Result<impl PinInit<irq::$reg_type<T>, Error> + '_> {
+            // SAFETY: `self.as_raw` returns a valid pointer to a `struct platform_device`.
+            let irq = unsafe { bindings::$irq_fn(self.as_raw(), index) };
+
+            if irq < 0 {
+                return Err(Error::from_errno(irq));
+            }
+
+            Ok(irq::$reg_type::<T>::register(
+                self.as_ref(),
+                irq as u32,
+                flags,
+                name,
+                handler,
+            ))
+        }
+    };
+
+    ($(#[$meta:meta])* $fn_name:ident, $reg_type:ident, $handler_trait:ident, name, $irq_fn:ident) => {
+        $(#[$meta])*
+        pub fn $fn_name<T: irq::$handler_trait + 'static>(
+            &self,
+            name: &'static CStr,
+            flags: irq::flags::Flags,
+            handler: T,
+        ) -> Result<impl PinInit<irq::$reg_type<T>, Error> + '_> {
+            // SAFETY: `self.as_raw` returns a valid pointer to a `struct platform_device`.
+            let irq = unsafe { bindings::$irq_fn(self.as_raw(), name.as_char_ptr()) };
+
+            if irq < 0 {
+                return Err(Error::from_errno(irq));
+            }
+
+            Ok(irq::$reg_type::<T>::register(
+                self.as_ref(),
+                irq as u32,
+                flags,
+                name,
+                handler,
+            ))
+        }
+    };
+}
+
+impl Device<Bound> {
+    gen_irq_accessor!(
+        /// Returns a [`irq::Registration`] for the IRQ at the given index.
+        irq_by_index, Registration, Handler, index, platform_get_irq
+    );
+    gen_irq_accessor!(
+        /// Returns a [`irq::Registration`] for the IRQ with the given name.
+        irq_by_name,
+        Registration,
+        Handler,
+        name,
+        platform_get_irq_byname
+    );
+    gen_irq_accessor!(
+        /// 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,
+        Registration,
+        Handler,
+        index,
+        platform_get_irq_optional
+    );
+    gen_irq_accessor!(
+        /// 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,
+        Registration,
+        Handler,
+        name,
+        platform_get_irq_byname_optional
+    );
+
+    gen_irq_accessor!(
+        /// Returns a [`irq::ThreadedRegistration`] for the IRQ at the given index.
+        threaded_irq_by_index,
+        ThreadedRegistration,
+        ThreadedHandler,
+        index,
+        platform_get_irq
+    );
+    gen_irq_accessor!(
+        /// Returns a [`irq::ThreadedRegistration`] for the IRQ with the given name.
+        threaded_irq_by_name,
+        ThreadedRegistration,
+        ThreadedHandler,
+        name,
+        platform_get_irq_byname
+    );
+    gen_irq_accessor!(
+        /// 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,
+        ThreadedRegistration,
+        ThreadedHandler,
+        index,
+        platform_get_irq_optional
+    );
+    gen_irq_accessor!(
+        /// 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,
+        ThreadedRegistration,
+        ThreadedHandler,
+        name,
+        platform_get_irq_byname_optional
+    );
+}
+
 // 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.49.0

[PATCH v4 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.

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

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

diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
index 8435f8132e38129ccc3495e7c4d3237fcaa97ad9..c690fa1c739c937324e902e61e68df238dbd733b 100644
--- a/rust/kernel/pci.rs
+++ b/rust/kernel/pci.rs
@@ -395,6 +395,32 @@ pub fn resource_len(&self, bar: u32) -> Result<bindings::resource_size_t> {
     }
 }
 
+macro_rules! gen_irq_accessor {
+    ($(#[$meta:meta])* $fn_name:ident, $reg_type:ident, $handler_trait:ident) => {
+        $(#[$meta])*
+        pub fn $fn_name<T: crate::irq::$handler_trait + 'static>(
+            &self,
+            index: u32,
+            flags: crate::irq::flags::Flags,
+            name: &'static crate::str::CStr,
+            handler: T,
+        ) -> Result<impl PinInit<crate::irq::$reg_type<T>, crate::error::Error> + '_> {
+            // 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));
+            }
+            Ok(crate::irq::$reg_type::<T>::register(
+                self.as_ref(),
+                irq as u32,
+                flags,
+                name,
+                handler,
+            ))
+        }
+    };
+}
+
 impl Device<device::Bound> {
     /// Mapps an entire PCI-BAR after performing a region-request on it. I/O operation bound checks
     /// can be performed on compile time for offsets (plus the requested type size) < SIZE.
@@ -413,6 +439,15 @@ pub fn iomap_region_sized<const SIZE: usize>(
     pub fn iomap_region(&self, bar: u32, name: &CStr) -> Result<Devres<Bar>> {
         self.iomap_region_sized::<0>(bar, name)
     }
+
+    gen_irq_accessor!(
+        /// Returns a [`kernel::irq::Registration`] for the IRQ vector at the given index.
+        irq_by_index, Registration, Handler
+    );
+    gen_irq_accessor!(
+        /// Returns a [`kernel::irq::ThreadedRegistration`] for the IRQ vector at the given index.
+        threaded_irq_by_index, ThreadedRegistration, ThreadedHandler
+    );
 }
 
 impl Device<device::Core> {

-- 
2.49.0

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

[Danilo Krummrich]

On Sun, Jun 08, 2025 at 07:51:08PM -0300, Daniel Almeida wrote:
> +/// // This is running in process context.
> +/// fn register_irq(handler: Handler, dev: &platform::Device<Bound>) -> Result<Arc<Registration<Handler>>> {
> +///     let registration = dev.irq_by_index(0, flags::SHARED, c_str!("my-device"), handler)?;
> +///
> +///     // You can have as many references to the registration as you want, so
> +///     // multiple parts of the driver can access it.
> +///     let registration = Arc::pin_init(registration, GFP_KERNEL)?;
> +///
> +///     // The handler may be called immediately after the function above
> +///     // returns, possibly in a different CPU.
> +///
> +///     {
> +///         // The data can be accessed from the process context too.
> +///         registration.handler().0.fetch_add(1, Ordering::Relaxed);
> +///     }

Why the extra scope?

> +///
> +///     Ok(registration)
> +/// }
> +///
> +/// # Ok::<(), Error>(())
> +///```
> +///
> +/// # Invariants
> +///
> +/// * We own an irq handler using `&self` as its private data.
> +///
> +#[pin_data]
> +pub struct Registration<T: Handler + 'static> {
> +    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(crate) fn register<'a>(

I think we should call this Registration::new() instead. Except for
MiscDeviceRegistration, which is representing not *only* a registration, all
other Registration types just use new() and it'd be nice to be consistent.

> +        dev: &'a Device<Bound>,
> +        irq: u32,
> +        flags: Flags,
> +        name: &'static CStr,
> +        handler: T,
> +    ) -> impl PinInit<Self, Error> + 'a {
> +        let closure = move |slot: *mut Self| {
> +            // SAFETY: The slot passed to pin initializer is valid for writing.
> +            unsafe {
> +                slot.write(Self {
> +                    inner: Devres::new(
> +                        dev,
> +                        RegistrationInner {
> +                            irq,
> +                            cookie: slot.cast(),
> +                        },
> +                        GFP_KERNEL,
> +                    )?,
> +                    handler,
> +                    _pin: PhantomPinned,
> +                })
> +            };
> +
> +            // 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.
> +            let res = to_result(unsafe {
> +                bindings::request_irq(
> +                    irq,
> +                    Some(handle_irq_callback::<T>),
> +                    flags.into_inner() as usize,
> +                    name.as_char_ptr(),
> +                    slot.cast(),
> +                )
> +            });
> +
> +            if res.is_err() {
> +                // SAFETY: We are returning an error, so we can destroy the slot.
> +                unsafe { core::ptr::drop_in_place(&raw mut (*slot).handler) };
> +            }
> +
> +            res
> +        };
> +
> +        // SAFETY:
> +        // - if this returns Ok, then every field of `slot` is fully
> +        // initialized.
> +        // - if this returns an error, then the slot does not need to remain
> +        // valid.
> +        unsafe { pin_init_from_closure(closure) }

Can't we use try_pin_init!() instead, move request_irq() into the initializer of
RegistrationInner and initialize inner last?

> +    }
> +
> +    /// 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 Registration<T> set in `Registration::new`
> +    let data = unsafe { &*(ptr as *const Registration<T>) };
> +    T::handle_irq(&data.handler).into_inner()
> +}
> 
> -- 
> 2.49.0
> 

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

[Danilo Krummrich]

On Sun, Jun 08, 2025 at 07:51:09PM -0300, Daniel Almeida wrote:
> +/// Callbacks for a threaded IRQ handler.
> +pub trait ThreadedHandler: Sync {
> +    /// The actual handler function. As usual, sleeps are not allowed in IRQ
> +    /// context.
> +    fn handle_irq(&self) -> ThreadedIrqReturn;
> +
> +    /// The threaded handler function. This function is called from the irq
> +    /// handler thread, which is automatically created by the system.
> +    fn thread_fn(&self) -> IrqReturn;
> +}
> +
> +impl<T: ?Sized + ThreadedHandler + Send> ThreadedHandler for Arc<T> {
> +    fn handle_irq(&self) -> ThreadedIrqReturn {
> +        T::handle_irq(self)
> +    }
> +
> +    fn thread_fn(&self) -> IrqReturn {
> +        T::thread_fn(self)
> +    }
> +}

In case you intend to be consistent with the function pointer names in
request_threaded_irq(), it'd need to be handler() and thread_fn(). But I don't
think there's a need for that, both aren't really nice for names of trait
methods.

What about irq::Handler::handle() and irq::Handler::handle_threaded() for
instance?

Alternatively, why not just

	trait Handler {
	   fn handle(&self);
	}
	
	trait ThreadedHandler {
	   fn handle(&self);
	}

and then we ask for `T: Handler + ThreadedHandler`.

> +
> +impl<T: ?Sized + ThreadedHandler, A: Allocator> ThreadedHandler for Box<T, A> {
> +    fn handle_irq(&self) -> ThreadedIrqReturn {
> +        T::handle_irq(self)
> +    }
> +
> +    fn thread_fn(&self) -> IrqReturn {
> +        T::thread_fn(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::IrqReturn;
> +/// use kernel::platform;
> +/// use kernel::sync::Arc;
> +/// use kernel::sync::SpinLock;
> +/// use kernel::alloc::flags::GFP_KERNEL;
> +/// use kernel::c_str;
> +///
> +/// // 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);
> +///
> +/// // [`handle_irq`] takes &self. This example illustrates interior
> +/// // mutability can be used when share 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 to data.
> +///     fn handle_irq(&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_irq`
> +///     // returns `WakeThread`.
> +///     fn thread_fn(&self) -> IrqReturn {
> +///         self.0.fetch_add(1, Ordering::Relaxed);
> +///
> +///         IrqReturn::Handled
> +///     }
> +/// }
> +///
> +/// // This is running in process context.
> +/// fn register_threaded_irq(handler: Handler, dev: &platform::Device<Bound>) -> Result<Arc<ThreadedRegistration<Handler>>> {
> +///     let registration = dev.threaded_irq_by_index(0, flags::SHARED, c_str!("my-device"), handler)?;

This doesn't compile (yet). I think this should be a "raw" example, i.e. the
function should take an IRQ number.

The example you sketch up here is for platform::Device::threaded_irq_by_index().

> +///
> +///     // You can have as many references to the registration as you want, so
> +///     // multiple parts of the driver can access it.
> +///     let registration = Arc::pin_init(registration, GFP_KERNEL)?;
> +///
> +///     // The handler may be called immediately after the function above
> +///     // returns, possibly in a different CPU.
> +///
> +///     {
> +///         // The data can be accessed from the process context too.
> +///         registration.handler().0.fetch_add(1, Ordering::Relaxed);
> +///     }

Why the extra scope?

> +///
> +///     Ok(registration)
> +/// }
> +///
> +///
> +/// # Ok::<(), Error>(())
> +///```
> +///
> +/// # Invariants
> +///
> +/// * We own an irq handler using `&self` as its private data.
> +///
> +#[pin_data]
> +pub struct ThreadedRegistration<T: ThreadedHandler + 'static> {
> +    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,
> +}

Most of the code in this file is a duplicate of the non-threaded registration.

I think this would greatly generalize with specialization and an HandlerInternal
trait.

Without specialization I think we could use enums to generalize.

The most trivial solution would be to define the Handler trait as

	trait Handler {
	   fn handle(&self);
	   fn handle_threaded(&self) {};
	}

but that's pretty dodgy.

> +impl<T: ThreadedHandler + 'static> ThreadedRegistration<T> {
> +    /// Registers the IRQ handler with the system for the given IRQ number.
> +    pub(crate) fn register<'a>(
> +        dev: &'a Device<Bound>,
> +        irq: u32,
> +        flags: Flags,
> +        name: &'static CStr,
> +        handler: T,
> +    ) -> impl PinInit<Self, Error> + 'a {

What happens if `dev`  does not match `irq`? The caller is responsible to only
provide an IRQ number that was obtained from this device.

This should be a safety requirement and a type invariant.

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

[Danilo Krummrich]

On Sun, Jun 08, 2025 at 07:51:10PM -0300, Daniel Almeida wrote:
> +macro_rules! gen_irq_accessor {
> +    ($(#[$meta:meta])* $fn_name:ident, $reg_type:ident, $handler_trait:ident, index, $irq_fn:ident) => {
> +        $(#[$meta])*
> +        pub fn $fn_name<T: irq::$handler_trait + 'static>(
> +            &self,
> +            index: u32,
> +            flags: irq::flags::Flags,
> +            name: &'static CStr,
> +            handler: T,
> +        ) -> Result<impl PinInit<irq::$reg_type<T>, Error> + '_> {
> +            // SAFETY: `self.as_raw` returns a valid pointer to a `struct platform_device`.
> +            let irq = unsafe { bindings::$irq_fn(self.as_raw(), index) };
> +
> +            if irq < 0 {
> +                return Err(Error::from_errno(irq));
> +            }
> +
> +            Ok(irq::$reg_type::<T>::register(
> +                self.as_ref(),
> +                irq as u32,
> +                flags,
> +                name,
> +                handler,
> +            ))
> +        }
> +    };
> +
> +    ($(#[$meta:meta])* $fn_name:ident, $reg_type:ident, $handler_trait:ident, name, $irq_fn:ident) => {
> +        $(#[$meta])*
> +        pub fn $fn_name<T: irq::$handler_trait + 'static>(
> +            &self,
> +            name: &'static CStr,
> +            flags: irq::flags::Flags,
> +            handler: T,
> +        ) -> Result<impl PinInit<irq::$reg_type<T>, Error> + '_> {
> +            // SAFETY: `self.as_raw` returns a valid pointer to a `struct platform_device`.
> +            let irq = unsafe { bindings::$irq_fn(self.as_raw(), name.as_char_ptr()) };

Do we always want to force that this name is the same name as...

> +
> +            if irq < 0 {
> +                return Err(Error::from_errno(irq));
> +            }
> +
> +            Ok(irq::$reg_type::<T>::register(
> +                self.as_ref(),
> +                irq as u32,
> +                flags,
> +                name,

...this name?

> +                handler,
> +            ))
> +        }
> +    };
> +}

Please split this in two macros, define_request_irq_by_index!() and
define_request_irq_by_name!() and keep the order of arguments the same between
the two.

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

[Danilo Krummrich]

On Sun, Jun 08, 2025 at 07:51:11PM -0300, Daniel Almeida wrote:
> These accessors can be used to retrieve a irq::Registration or a
> irq::ThreadedRegistration from a pci device.
> 
> These accessors ensure that only valid IRQ lines can ever be registered.
> 
> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
> ---
>  rust/kernel/pci.rs | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
> index 8435f8132e38129ccc3495e7c4d3237fcaa97ad9..c690fa1c739c937324e902e61e68df238dbd733b 100644
> --- a/rust/kernel/pci.rs
> +++ b/rust/kernel/pci.rs
> @@ -395,6 +395,32 @@ pub fn resource_len(&self, bar: u32) -> Result<bindings::resource_size_t> {
>      }
>  }
>  
> +macro_rules! gen_irq_accessor {
> +    ($(#[$meta:meta])* $fn_name:ident, $reg_type:ident, $handler_trait:ident) => {
> +        $(#[$meta])*
> +        pub fn $fn_name<T: crate::irq::$handler_trait + 'static>(
> +            &self,
> +            index: u32,
> +            flags: crate::irq::flags::Flags,
> +            name: &'static crate::str::CStr,
> +            handler: T,
> +        ) -> Result<impl PinInit<crate::irq::$reg_type<T>, crate::error::Error> + '_> {
> +            // 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));
> +            }
> +            Ok(crate::irq::$reg_type::<T>::register(
> +                self.as_ref(),
> +                irq as u32,
> +                flags,
> +                name,
> +                handler,
> +            ))
> +        }
> +    };
> +}

Given that we only have two invocations below, please implement them in-place. I
don't think it's worth having the macro indirection.

>  impl Device<device::Bound> {
>      /// Mapps an entire PCI-BAR after performing a region-request on it. I/O operation bound checks
>      /// can be performed on compile time for offsets (plus the requested type size) < SIZE.
> @@ -413,6 +439,15 @@ pub fn iomap_region_sized<const SIZE: usize>(
>      pub fn iomap_region(&self, bar: u32, name: &CStr) -> Result<Devres<Bar>> {
>          self.iomap_region_sized::<0>(bar, name)
>      }
> +
> +    gen_irq_accessor!(
> +        /// Returns a [`kernel::irq::Registration`] for the IRQ vector at the given index.
> +        irq_by_index, Registration, Handler
> +    );
> +    gen_irq_accessor!(
> +        /// Returns a [`kernel::irq::ThreadedRegistration`] for the IRQ vector at the given index.
> +        threaded_irq_by_index, ThreadedRegistration, ThreadedHandler
> +    );
>  }
>  
>  impl Device<device::Core> {
> 
> -- 
> 2.49.0
> 

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

[Daniel Almeida]

Hi Danilo,

> On 9 Jun 2025, at 09:27, Danilo Krummrich <dakr@kernel.org> wrote:
> 
> On Sun, Jun 08, 2025 at 07:51:09PM -0300, Daniel Almeida wrote:
>> +/// Callbacks for a threaded IRQ handler.
>> +pub trait ThreadedHandler: Sync {
>> +    /// The actual handler function. As usual, sleeps are not allowed in IRQ
>> +    /// context.
>> +    fn handle_irq(&self) -> ThreadedIrqReturn;
>> +
>> +    /// The threaded handler function. This function is called from the irq
>> +    /// handler thread, which is automatically created by the system.
>> +    fn thread_fn(&self) -> IrqReturn;
>> +}
>> +
>> +impl<T: ?Sized + ThreadedHandler + Send> ThreadedHandler for Arc<T> {
>> +    fn handle_irq(&self) -> ThreadedIrqReturn {
>> +        T::handle_irq(self)
>> +    }
>> +
>> +    fn thread_fn(&self) -> IrqReturn {
>> +        T::thread_fn(self)
>> +    }
>> +}
> 
> In case you intend to be consistent with the function pointer names in
> request_threaded_irq(), it'd need to be handler() and thread_fn(). But I don't
> think there's a need for that, both aren't really nice for names of trait
> methods.
> 
> What about irq::Handler::handle() and irq::Handler::handle_threaded() for
> instance?
> 
> Alternatively, why not just
> 
> trait Handler {
>   fn handle(&self);
> }
> 
> trait ThreadedHandler {
>   fn handle(&self);
> }
> 
> and then we ask for `T: Handler + ThreadedHandler`.

Sure, I am totally OK with renaming things, but IIRC I've tried  Handler +
ThreadedHandler in the past and found it to be problematic. I don't recall why,
though, so maybe it's worth another attempt.

> 
>> +
>> +impl<T: ?Sized + ThreadedHandler, A: Allocator> ThreadedHandler for Box<T, A> {
>> +    fn handle_irq(&self) -> ThreadedIrqReturn {
>> +        T::handle_irq(self)
>> +    }
>> +
>> +    fn thread_fn(&self) -> IrqReturn {
>> +        T::thread_fn(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::IrqReturn;
>> +/// use kernel::platform;
>> +/// use kernel::sync::Arc;
>> +/// use kernel::sync::SpinLock;
>> +/// use kernel::alloc::flags::GFP_KERNEL;
>> +/// use kernel::c_str;
>> +///
>> +/// // 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);
>> +///
>> +/// // [`handle_irq`] takes &self. This example illustrates interior
>> +/// // mutability can be used when share 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 to data.
>> +///     fn handle_irq(&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_irq`
>> +///     // returns `WakeThread`.
>> +///     fn thread_fn(&self) -> IrqReturn {
>> +///         self.0.fetch_add(1, Ordering::Relaxed);
>> +///
>> +///         IrqReturn::Handled
>> +///     }
>> +/// }
>> +///
>> +/// // This is running in process context.
>> +/// fn register_threaded_irq(handler: Handler, dev: &platform::Device<Bound>) -> Result<Arc<ThreadedRegistration<Handler>>> {
>> +///     let registration = dev.threaded_irq_by_index(0, flags::SHARED, c_str!("my-device"), handler)?;
> 
> This doesn't compile (yet). I think this should be a "raw" example, i.e. the
> function should take an IRQ number.
> 
> The example you sketch up here is for platform::Device::threaded_irq_by_index().

Yes, I originally had an example along the lines of what you mentioned. Except
that with the changes in register() from pub to pub(crate) they stopped
compiling.

I am not sure how the doctest to kunit machinery works, but I was expecting
tests to have access to everything within the module they're defined in, but
this is apparently not the case.

> 
>> +///
>> +///     // You can have as many references to the registration as you want, so
>> +///     // multiple parts of the driver can access it.
>> +///     let registration = Arc::pin_init(registration, GFP_KERNEL)?;
>> +///
>> +///     // The handler may be called immediately after the function above
>> +///     // returns, possibly in a different CPU.
>> +///
>> +///     {
>> +///         // The data can be accessed from the process context too.
>> +///         registration.handler().0.fetch_add(1, Ordering::Relaxed);
>> +///     }
> 
> Why the extra scope?

There used to be a lock here instead of AtomicU32. This extra scope was there to drop it.

It’s gone now so there’s no reason to have the extra scope indeed.

> 
>> +///
>> +///     Ok(registration)
>> +/// }
>> +///
>> +///
>> +/// # Ok::<(), Error>(())
>> +///```
>> +///
>> +/// # Invariants
>> +///
>> +/// * We own an irq handler using `&self` as its private data.
>> +///
>> +#[pin_data]
>> +pub struct ThreadedRegistration<T: ThreadedHandler + 'static> {
>> +    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,
>> +}
> 
> Most of the code in this file is a duplicate of the non-threaded registration.
> 
> I think this would greatly generalize with specialization and an HandlerInternal
> trait.
> 
> Without specialization I think we could use enums to generalize.
> 
> The most trivial solution would be to define the Handler trait as
> 
> trait Handler {
>   fn handle(&self);
>   fn handle_threaded(&self) {};
> }
> 
> but that's pretty dodgy.

A lot of the comments up until now have touched on somehow having threaded and
non-threaded versions implemented together. I personally see no problem in
having things duplicated here, because I think it's easier to reason about what
is going on this way. Alice has expressed a similar view in a previous iteration.

Can you expand a bit more on your suggestion? Perhaps there's a clean way to do
it (without macros and etc), but so far I don't see it.

> 
>> +impl<T: ThreadedHandler + 'static> ThreadedRegistration<T> {
>> +    /// Registers the IRQ handler with the system for the given IRQ number.
>> +    pub(crate) fn register<'a>(
>> +        dev: &'a Device<Bound>,
>> +        irq: u32,
>> +        flags: Flags,
>> +        name: &'static CStr,
>> +        handler: T,
>> +    ) -> impl PinInit<Self, Error> + 'a {
> 
> What happens if `dev`  does not match `irq`? The caller is responsible to only
> provide an IRQ number that was obtained from this device.
> 
> This should be a safety requirement and a type invariant.

This iteration converted register() from pub to pub(crate). The idea was to
force drivers to use the accessors. I assumed this was enough to make the API
safe, as the few users in the kernel crate (i.e.: so far platform and pci)
could be manually checked for correctness.

To summarize my point, there is still the possibility of misusing this from the
kernel crate itself, but that is no longer possible from a driver's
perspective.

— Daniel

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

[Danilo Krummrich]

On Mon, Jun 09, 2025 at 01:24:40PM -0300, Daniel Almeida wrote:
> > On 9 Jun 2025, at 09:27, Danilo Krummrich <dakr@kernel.org> wrote:
> >> +#[pin_data]
> >> +pub struct ThreadedRegistration<T: ThreadedHandler + 'static> {
> >> +    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,
> >> +}
> > 
> > Most of the code in this file is a duplicate of the non-threaded registration.
> > 
> > I think this would greatly generalize with specialization and an HandlerInternal
> > trait.
> > 
> > Without specialization I think we could use enums to generalize.
> > 
> > The most trivial solution would be to define the Handler trait as
> > 
> > trait Handler {
> >   fn handle(&self);
> >   fn handle_threaded(&self) {};
> > }
> > 
> > but that's pretty dodgy.
> 
> A lot of the comments up until now have touched on somehow having threaded and
> non-threaded versions implemented together. I personally see no problem in
> having things duplicated here, because I think it's easier to reason about what
> is going on this way. Alice has expressed a similar view in a previous iteration.
> 
> Can you expand a bit more on your suggestion? Perhaps there's a clean way to do
> it (without macros and etc), but so far I don't see it.

I think with specialization it'd be trivial to generalize, but this isn't
stable yet. The enum approach is probably unnecessarily complicated, so I agree
to leave it as it is.

Maybe a comment that this can be generalized once we get specialization would be
good?

> >> +impl<T: ThreadedHandler + 'static> ThreadedRegistration<T> {
> >> +    /// Registers the IRQ handler with the system for the given IRQ number.
> >> +    pub(crate) fn register<'a>(
> >> +        dev: &'a Device<Bound>,
> >> +        irq: u32,
> >> +        flags: Flags,
> >> +        name: &'static CStr,
> >> +        handler: T,
> >> +    ) -> impl PinInit<Self, Error> + 'a {
> > 
> > What happens if `dev`  does not match `irq`? The caller is responsible to only
> > provide an IRQ number that was obtained from this device.
> > 
> > This should be a safety requirement and a type invariant.
> 
> This iteration converted register() from pub to pub(crate). The idea was to
> force drivers to use the accessors. I assumed this was enough to make the API
> safe, as the few users in the kernel crate (i.e.: so far platform and pci)
> could be manually checked for correctness.
> 
> To summarize my point, there is still the possibility of misusing this from the
> kernel crate itself, but that is no longer possible from a driver's
> perspective.

Correct, you made Registration::new() crate private, such that drivers can't
access it anymore. But that doesn't make the function safe by itself. It's still
unsafe to be used from platform::Device and pci::Device.

While that's fine, we can't ignore it and still have to add the corresponding
safety requirements to Registration::new().

I think there is a way to make this interface safe as well -- this is also
something that Benno would be great to have a look at.

I'm thinking of something like

	/// # Invariant
	///
	/// `ìrq` is the number of an interrupt source of `dev`.
	struct IrqRequest<'a> {
	   dev: &'a Device<Bound>,
	   irq: u32,
	}

and from the caller you could create an instance like this:

	// INVARIANT: [...]
	let req = IrqRequest { dev, irq };

I'm not sure whether this needs an unsafe constructor though.

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

[Miguel Ojeda]

On Mon, Jun 9, 2025 at 8:13 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Mon, Jun 09, 2025 at 01:24:40PM -0300, Daniel Almeida wrote:
> >
> > This iteration converted register() from pub to pub(crate). The idea was to
> > force drivers to use the accessors. I assumed this was enough to make the API
> > safe, as the few users in the kernel crate (i.e.: so far platform and pci)
> > could be manually checked for correctness.
> >
> > To summarize my point, there is still the possibility of misusing this from the
> > kernel crate itself, but that is no longer possible from a driver's
> > perspective.
>
> Correct, you made Registration::new() crate private, such that drivers can't
> access it anymore. But that doesn't make the function safe by itself. It's still
> unsafe to be used from platform::Device and pci::Device.

Yeah.

Even if a function is fully private (i.e. not even `pub(crate)`), then
it should still be marked as unsafe if it is so.

Cheers,
Miguel

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

[kernel test robot]

Hi Daniel,

kernel test robot noticed the following build errors:

[auto build test ERROR on e271ed52b344ac02d4581286961d0c40acc54c03]

url:    https://github.com/intel-lab-lkp/linux/commits/Daniel-Almeida/rust-irq-add-irq-module/20250609-065947
base:   e271ed52b344ac02d4581286961d0c40acc54c03
patch link:    https://lore.kernel.org/r/20250608-topics-tyr-request_irq-v4-6-81cb81fb8073%40collabora.com
patch subject: [PATCH v4 6/6] rust: pci: add irq accessors
config: x86_64-randconfig-005-20250610 (https://download.01.org/0day-ci/archive/20250610/202506100619.ZG8fk4Yz-lkp@intel.com/config)
compiler: clang version 20.1.2 (https://github.com/llvm/llvm-project 58df0ef89dd64126512e4ee27b4ac3fd8ddf6247)
rustc: rustc 1.78.0 (9b00956e5 2024-04-29)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250610/202506100619.ZG8fk4Yz-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202506100619.ZG8fk4Yz-lkp@intel.com/

All errors (new ones prefixed by >>):

   ***
   *** Rust bindings generator 'bindgen' < 0.69.5 together with libclang >= 19.1
   *** may not work due to a bug (https://github.com/rust-lang/rust-bindgen/pull/2824),
   *** unless patched (like Debian's).
   ***   Your bindgen version:  0.65.1
   ***   Your libclang version: 20.1.2
   ***
   ***
   *** Please see Documentation/rust/quick-start.rst for details
   *** on how to set up the Rust support.
   ***
>> error[E0425]: cannot find function `pci_irq_vector` in crate `crate::bindings`
   --> rust/kernel/pci.rs:409:49
   |
   409 |               let irq = unsafe { crate::bindings::pci_irq_vector(self.as_raw(), index) };
   |                                                   ^^^^^^^^^^^^^^ not found in `crate::bindings`
   ...
   443 | /     gen_irq_accessor!(
   444 | |         /// Returns a [`kernel::irq::Registration`] for the IRQ vector at the given index.
   445 | |         irq_by_index, Registration, Handler
   446 | |     );
   | |_____- in this macro invocation
   |
   = note: this error originates in the macro `gen_irq_accessor` (in Nightly builds, run with -Z macro-backtrace for more info)
--
>> error[E0425]: cannot find function `pci_irq_vector` in crate `crate::bindings`
   --> rust/kernel/pci.rs:409:49
   |
   409 |               let irq = unsafe { crate::bindings::pci_irq_vector(self.as_raw(), index) };
   |                                                   ^^^^^^^^^^^^^^ not found in `crate::bindings`
   ...
   447 | /     gen_irq_accessor!(
   448 | |         /// Returns a [`kernel::irq::ThreadedRegistration`] for the IRQ vector at the given index.
   449 | |         threaded_irq_by_index, ThreadedRegistration, ThreadedHandler
   450 | |     );
   | |_____- in this macro invocation
   |
   = note: this error originates in the macro `gen_irq_accessor` (in Nightly builds, run with -Z macro-backtrace for more info)

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[Alice Ryhl]

On Mon, Jun 9, 2025 at 8:13 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Mon, Jun 09, 2025 at 01:24:40PM -0300, Daniel Almeida wrote:
> > > On 9 Jun 2025, at 09:27, Danilo Krummrich <dakr@kernel.org> wrote:
> > >> +#[pin_data]
> > >> +pub struct ThreadedRegistration<T: ThreadedHandler + 'static> {
> > >> +    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,
> > >> +}
> > >
> > > Most of the code in this file is a duplicate of the non-threaded registration.
> > >
> > > I think this would greatly generalize with specialization and an HandlerInternal
> > > trait.
> > >
> > > Without specialization I think we could use enums to generalize.
> > >
> > > The most trivial solution would be to define the Handler trait as
> > >
> > > trait Handler {
> > >   fn handle(&self);
> > >   fn handle_threaded(&self) {};
> > > }
> > >
> > > but that's pretty dodgy.
> >
> > A lot of the comments up until now have touched on somehow having threaded and
> > non-threaded versions implemented together. I personally see no problem in
> > having things duplicated here, because I think it's easier to reason about what
> > is going on this way. Alice has expressed a similar view in a previous iteration.
> >
> > Can you expand a bit more on your suggestion? Perhaps there's a clean way to do
> > it (without macros and etc), but so far I don't see it.
>
> I think with specialization it'd be trivial to generalize, but this isn't
> stable yet. The enum approach is probably unnecessarily complicated, so I agree
> to leave it as it is.
>
> Maybe a comment that this can be generalized once we get specialization would be
> good?

Specialization is really far out. I don't think we should try to take
it into account when designing things today. I think that the
duplication in this case is perfectly acceptable and trying to
deduplicate makes things too hard to read.

> > >> +impl<T: ThreadedHandler + 'static> ThreadedRegistration<T> {
> > >> +    /// Registers the IRQ handler with the system for the given IRQ number.
> > >> +    pub(crate) fn register<'a>(
> > >> +        dev: &'a Device<Bound>,
> > >> +        irq: u32,
> > >> +        flags: Flags,
> > >> +        name: &'static CStr,
> > >> +        handler: T,
> > >> +    ) -> impl PinInit<Self, Error> + 'a {
> > >
> > > What happens if `dev`  does not match `irq`? The caller is responsible to only
> > > provide an IRQ number that was obtained from this device.
> > >
> > > This should be a safety requirement and a type invariant.
> >
> > This iteration converted register() from pub to pub(crate). The idea was to
> > force drivers to use the accessors. I assumed this was enough to make the API
> > safe, as the few users in the kernel crate (i.e.: so far platform and pci)
> > could be manually checked for correctness.
> >
> > To summarize my point, there is still the possibility of misusing this from the
> > kernel crate itself, but that is no longer possible from a driver's
> > perspective.
>
> Correct, you made Registration::new() crate private, such that drivers can't
> access it anymore. But that doesn't make the function safe by itself. It's still
> unsafe to be used from platform::Device and pci::Device.
>
> While that's fine, we can't ignore it and still have to add the corresponding
> safety requirements to Registration::new().
>
> I think there is a way to make this interface safe as well -- this is also
> something that Benno would be great to have a look at.
>
> I'm thinking of something like
>
>         /// # Invariant
>         ///
>         /// `ěrq` is the number of an interrupt source of `dev`.
>         struct IrqRequest<'a> {
>            dev: &'a Device<Bound>,
>            irq: u32,
>         }
>
> and from the caller you could create an instance like this:
>
>         // INVARIANT: [...]
>         let req = IrqRequest { dev, irq };
>
> I'm not sure whether this needs an unsafe constructor though.

The API you shared would definitely work. It pairs the irq number with
the device it matches. Yes, I would probably give it an unsafe
constructor, but I imagine that most methods that return an irq number
could be changed to just return this type so that drivers do not need
to use said unsafe.

Alice

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

[Danilo Krummrich]

On Mon, Jun 16, 2025 at 03:33:06PM +0200, Alice Ryhl wrote:
> On Mon, Jun 9, 2025 at 8:13 PM Danilo Krummrich <dakr@kernel.org> wrote:
> > I think with specialization it'd be trivial to generalize, but this isn't
> > stable yet. The enum approach is probably unnecessarily complicated, so I agree
> > to leave it as it is.
> >
> > Maybe a comment that this can be generalized once we get specialization would be
> > good?
> 
> Specialization is really far out. I don't think we should try to take
> it into account when designing things today. I think that the
> duplication in this case is perfectly acceptable and trying to
> deduplicate makes things too hard to read.

As mentioned above, I agree with the latter. But I think leaving a note that
this could be deduplicated rather easily with specialization probably doesn't
hurt?

> > I'm thinking of something like
> >
> >         /// # Invariant
> >         ///
> >         /// `ěrq` is the number of an interrupt source of `dev`.
> >         struct IrqRequest<'a> {
> >            dev: &'a Device<Bound>,
> >            irq: u32,
> >         }
> >
> > and from the caller you could create an instance like this:
> >
> >         // INVARIANT: [...]
> >         let req = IrqRequest { dev, irq };
> >
> > I'm not sure whether this needs an unsafe constructor though.
> 
> The API you shared would definitely work. It pairs the irq number with
> the device it matches. Yes, I would probably give it an unsafe
> constructor, but I imagine that most methods that return an irq number
> could be changed to just return this type so that drivers do not need
> to use said unsafe.

Driver don't need to use unsafe already. It's only the IRQ accessors in this
patch series (in platform.rs and pci.rs) that are affected.

Let's also keep those accessors, from a driver perspective it's much nicer to
have an API like this, i.e.

	// `irq` is an `irq::Registration`
	let irq = pdev.threaded_irq_by_name()?

vs.

	// `req` is an `IrqRequest`.
	let req = pdev.irq_by_name()?;

	// `irq` is an `irq::Registration`
	let irq = irq::ThreadedRegistration::new(req)?;

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

[Daniel Almeida]

Hi Danilo,

> On 9 Jun 2025, at 13:24, Daniel Almeida <daniel.almeida@collabora.com> wrote:
> 
> Hi Danilo,
> 
>> On 9 Jun 2025, at 09:27, Danilo Krummrich <dakr@kernel.org> wrote:
>> 
>> On Sun, Jun 08, 2025 at 07:51:09PM -0300, Daniel Almeida wrote:
>>> +/// Callbacks for a threaded IRQ handler.
>>> +pub trait ThreadedHandler: Sync {
>>> +    /// The actual handler function. As usual, sleeps are not allowed in IRQ
>>> +    /// context.
>>> +    fn handle_irq(&self) -> ThreadedIrqReturn;
>>> +
>>> +    /// The threaded handler function. This function is called from the irq
>>> +    /// handler thread, which is automatically created by the system.
>>> +    fn thread_fn(&self) -> IrqReturn;
>>> +}
>>> +
>>> +impl<T: ?Sized + ThreadedHandler + Send> ThreadedHandler for Arc<T> {
>>> +    fn handle_irq(&self) -> ThreadedIrqReturn {
>>> +        T::handle_irq(self)
>>> +    }
>>> +
>>> +    fn thread_fn(&self) -> IrqReturn {
>>> +        T::thread_fn(self)
>>> +    }
>>> +}
>> 
>> In case you intend to be consistent with the function pointer names in
>> request_threaded_irq(), it'd need to be handler() and thread_fn(). But I don't
>> think there's a need for that, both aren't really nice for names of trait
>> methods.
>> 
>> What about irq::Handler::handle() and irq::Handler::handle_threaded() for
>> instance?
>> 
>> Alternatively, why not just
>> 
>> trait Handler {
>>  fn handle(&self);
>> }
>> 
>> trait ThreadedHandler {
>>  fn handle(&self);
>> }
>> 
>> and then we ask for `T: Handler + ThreadedHandler`.
> 
> Sure, I am totally OK with renaming things, but IIRC I've tried  Handler +
> ThreadedHandler in the past and found it to be problematic. I don't recall why,
> though, so maybe it's worth another attempt.

Handler::handle() returns IrqReturn and ThreadedHandler::handle() returns
ThreadedIrqReturn, which includes WakeThread, so these had to be separate
traits.

I'd say lets keep it this way. This really looks like the discussion on
de-duplicating code, and as I said (IMHO) it just complicates the
implementation for no gain.

— Daniel

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

[Daniel Almeida]

Hi,

>> 
>>> +
>>> +impl<T: ?Sized + ThreadedHandler, A: Allocator> ThreadedHandler for Box<T, A> {
>>> +    fn handle_irq(&self) -> ThreadedIrqReturn {
>>> +        T::handle_irq(self)
>>> +    }
>>> +
>>> +    fn thread_fn(&self) -> IrqReturn {
>>> +        T::thread_fn(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::IrqReturn;
>>> +/// use kernel::platform;
>>> +/// use kernel::sync::Arc;
>>> +/// use kernel::sync::SpinLock;
>>> +/// use kernel::alloc::flags::GFP_KERNEL;
>>> +/// use kernel::c_str;
>>> +///
>>> +/// // 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);
>>> +///
>>> +/// // [`handle_irq`] takes &self. This example illustrates interior
>>> +/// // mutability can be used when share 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 to data.
>>> +///     fn handle_irq(&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_irq`
>>> +///     // returns `WakeThread`.
>>> +///     fn thread_fn(&self) -> IrqReturn {
>>> +///         self.0.fetch_add(1, Ordering::Relaxed);
>>> +///
>>> +///         IrqReturn::Handled
>>> +///     }
>>> +/// }
>>> +///
>>> +/// // This is running in process context.
>>> +/// fn register_threaded_irq(handler: Handler, dev: &platform::Device<Bound>) -> Result<Arc<ThreadedRegistration<Handler>>> {
>>> +///     let registration = dev.threaded_irq_by_index(0, flags::SHARED, c_str!("my-device"), handler)?;
>> 
>> This doesn't compile (yet). I think this should be a "raw" example, i.e. the
>> function should take an IRQ number.
>> 
>> The example you sketch up here is for platform::Device::threaded_irq_by_index().
> 
> Yes, I originally had an example along the lines of what you mentioned. Except
> that with the changes in register() from pub to pub(crate) they stopped
> compiling.
> 
> I am not sure how the doctest to kunit machinery works, but I was expecting
> tests to have access to everything within the module they're defined in, but
> this is apparently not the case.

Does anybody have any input on this? Again, I tried it already before sending
the current version but it does not compile due to pub(crate).

— Daniel

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

[Danilo Krummrich]

On Mon, Jun 16, 2025 at 10:48:37AM -0300, Daniel Almeida wrote:
> Hi Danilo,
> 
> > On 9 Jun 2025, at 13:24, Daniel Almeida <daniel.almeida@collabora.com> wrote:
> > 
> > Hi Danilo,
> > 
> >> On 9 Jun 2025, at 09:27, Danilo Krummrich <dakr@kernel.org> wrote:
> >> 
> >> On Sun, Jun 08, 2025 at 07:51:09PM -0300, Daniel Almeida wrote:
> >>> +/// Callbacks for a threaded IRQ handler.
> >>> +pub trait ThreadedHandler: Sync {
> >>> +    /// The actual handler function. As usual, sleeps are not allowed in IRQ
> >>> +    /// context.
> >>> +    fn handle_irq(&self) -> ThreadedIrqReturn;
> >>> +
> >>> +    /// The threaded handler function. This function is called from the irq
> >>> +    /// handler thread, which is automatically created by the system.
> >>> +    fn thread_fn(&self) -> IrqReturn;
> >>> +}
> >>> +
> >>> +impl<T: ?Sized + ThreadedHandler + Send> ThreadedHandler for Arc<T> {
> >>> +    fn handle_irq(&self) -> ThreadedIrqReturn {
> >>> +        T::handle_irq(self)
> >>> +    }
> >>> +
> >>> +    fn thread_fn(&self) -> IrqReturn {
> >>> +        T::thread_fn(self)
> >>> +    }
> >>> +}
> >> 
> >> In case you intend to be consistent with the function pointer names in
> >> request_threaded_irq(), it'd need to be handler() and thread_fn(). But I don't
> >> think there's a need for that, both aren't really nice for names of trait
> >> methods.
> >> 
> >> What about irq::Handler::handle() and irq::Handler::handle_threaded() for
> >> instance?
> >> 
> >> Alternatively, why not just
> >> 
> >> trait Handler {
> >>  fn handle(&self);
> >> }
> >> 
> >> trait ThreadedHandler {
> >>  fn handle(&self);
> >> }
> >> 
> >> and then we ask for `T: Handler + ThreadedHandler`.
> > 
> > Sure, I am totally OK with renaming things, but IIRC I've tried  Handler +
> > ThreadedHandler in the past and found it to be problematic. I don't recall why,
> > though, so maybe it's worth another attempt.
> 
> Handler::handle() returns IrqReturn and ThreadedHandler::handle() returns
> ThreadedIrqReturn, which includes WakeThread, so these had to be separate
> traits.

Ok, that fine then. But I'd still prefer the better naming as mentioned above.

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

[Danilo Krummrich]

On Mon, Jun 16, 2025 at 03:43:40PM +0200, Danilo Krummrich wrote:
> On Mon, Jun 16, 2025 at 03:33:06PM +0200, Alice Ryhl wrote:
> > On Mon, Jun 9, 2025 at 8:13 PM Danilo Krummrich <dakr@kernel.org> wrote:
> > > I think with specialization it'd be trivial to generalize, but this isn't
> > > stable yet. The enum approach is probably unnecessarily complicated, so I agree
> > > to leave it as it is.
> > >
> > > Maybe a comment that this can be generalized once we get specialization would be
> > > good?
> > 
> > Specialization is really far out. I don't think we should try to take
> > it into account when designing things today. I think that the
> > duplication in this case is perfectly acceptable and trying to
> > deduplicate makes things too hard to read.
> 
> As mentioned above, I agree with the latter. But I think leaving a note that
> this could be deduplicated rather easily with specialization probably doesn't
> hurt?
> 
> > > I'm thinking of something like
> > >
> > >         /// # Invariant
> > >         ///
> > >         /// `ěrq` is the number of an interrupt source of `dev`.
> > >         struct IrqRequest<'a> {
> > >            dev: &'a Device<Bound>,
> > >            irq: u32,
> > >         }
> > >
> > > and from the caller you could create an instance like this:
> > >
> > >         // INVARIANT: [...]
> > >         let req = IrqRequest { dev, irq };
> > >
> > > I'm not sure whether this needs an unsafe constructor though.
> > 
> > The API you shared would definitely work. It pairs the irq number with
> > the device it matches. Yes, I would probably give it an unsafe
> > constructor, but I imagine that most methods that return an irq number
> > could be changed to just return this type so that drivers do not need
> > to use said unsafe.
> 
> Driver don't need to use unsafe already. It's only the IRQ accessors in this
> patch series (in platform.rs and pci.rs) that are affected.
> 
> Let's also keep those accessors, from a driver perspective it's much nicer to
> have an API like this, i.e.

Just to clarify, I meant to additionally keep the accessors, since

> 
> 	// `irq` is an `irq::Registration`
> 	let irq = pdev.threaded_irq_by_name()?

this should be the most common case.

> 
> vs.
> 
> 	// `req` is an `IrqRequest`.
> 	let req = pdev.irq_by_name()?;
> 
> 	// `irq` is an `irq::Registration`
> 	let irq = irq::ThreadedRegistration::new(req)?;

But this can be useful as well, e.g. if a driver can handle devices from
multiple busses, the driver could obtain the IrqRequest and pass it down to bus
independent layers of the driver which then create the final irq::Registration.

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

[Benno Lossin]

On Mon Jun 9, 2025 at 8:13 PM CEST, Danilo Krummrich wrote:
> On Mon, Jun 09, 2025 at 01:24:40PM -0300, Daniel Almeida wrote:
>> > On 9 Jun 2025, at 09:27, Danilo Krummrich <dakr@kernel.org> wrote:
>> >> +#[pin_data]
>> >> +pub struct ThreadedRegistration<T: ThreadedHandler + 'static> {
>> >> +    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,
>> >> +}
>> > 
>> > Most of the code in this file is a duplicate of the non-threaded registration.
>> > 
>> > I think this would greatly generalize with specialization and an HandlerInternal
>> > trait.
>> > 
>> > Without specialization I think we could use enums to generalize.
>> > 
>> > The most trivial solution would be to define the Handler trait as
>> > 
>> > trait Handler {
>> >   fn handle(&self);
>> >   fn handle_threaded(&self) {};
>> > }
>> > 
>> > but that's pretty dodgy.
>> 
>> A lot of the comments up until now have touched on somehow having threaded and
>> non-threaded versions implemented together. I personally see no problem in
>> having things duplicated here, because I think it's easier to reason about what
>> is going on this way. Alice has expressed a similar view in a previous iteration.
>> 
>> Can you expand a bit more on your suggestion? Perhaps there's a clean way to do
>> it (without macros and etc), but so far I don't see it.
>
> I think with specialization it'd be trivial to generalize, but this isn't
> stable yet. The enum approach is probably unnecessarily complicated, so I agree
> to leave it as it is.
>
> Maybe a comment that this can be generalized once we get specialization would be
> good?
>
>> >> +impl<T: ThreadedHandler + 'static> ThreadedRegistration<T> {
>> >> +    /// Registers the IRQ handler with the system for the given IRQ number.
>> >> +    pub(crate) fn register<'a>(
>> >> +        dev: &'a Device<Bound>,
>> >> +        irq: u32,
>> >> +        flags: Flags,
>> >> +        name: &'static CStr,
>> >> +        handler: T,
>> >> +    ) -> impl PinInit<Self, Error> + 'a {
>> > 
>> > What happens if `dev`  does not match `irq`? The caller is responsible to only
>> > provide an IRQ number that was obtained from this device.
>> > 
>> > This should be a safety requirement and a type invariant.
>> 
>> This iteration converted register() from pub to pub(crate). The idea was to
>> force drivers to use the accessors. I assumed this was enough to make the API
>> safe, as the few users in the kernel crate (i.e.: so far platform and pci)
>> could be manually checked for correctness.
>> 
>> To summarize my point, there is still the possibility of misusing this from the
>> kernel crate itself, but that is no longer possible from a driver's
>> perspective.
>
> Correct, you made Registration::new() crate private, such that drivers can't
> access it anymore. But that doesn't make the function safe by itself. It's still
> unsafe to be used from platform::Device and pci::Device.
>
> While that's fine, we can't ignore it and still have to add the corresponding
> safety requirements to Registration::new().
>
> I think there is a way to make this interface safe as well -- this is also
> something that Benno would be great to have a look at.

Finally had some time to look through this thread, thought I needed a
whole lot of context, but turns out the question is simple :)

Your idea looks sound :)

---
Cheers,
Benno

> I'm thinking of something like
>
> 	/// # Invariant
> 	///
> 	/// `ìrq` is the number of an interrupt source of `dev`.
> 	struct IrqRequest<'a> {
> 	   dev: &'a Device<Bound>,
> 	   irq: u32,
> 	}
>
> and from the caller you could create an instance like this:
>
> 	// INVARIANT: [...]
> 	let req = IrqRequest { dev, irq };
>
> I'm not sure whether this needs an unsafe constructor though.

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

[Alice Ryhl]

On Mon, Jun 9, 2025 at 12:47 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Sun, Jun 08, 2025 at 07:51:08PM -0300, Daniel Almeida wrote:
> > +        dev: &'a Device<Bound>,
> > +        irq: u32,
> > +        flags: Flags,
> > +        name: &'static CStr,
> > +        handler: T,
> > +    ) -> impl PinInit<Self, Error> + 'a {
> > +        let closure = move |slot: *mut Self| {
> > +            // SAFETY: The slot passed to pin initializer is valid for writing.
> > +            unsafe {
> > +                slot.write(Self {
> > +                    inner: Devres::new(
> > +                        dev,
> > +                        RegistrationInner {
> > +                            irq,
> > +                            cookie: slot.cast(),
> > +                        },
> > +                        GFP_KERNEL,
> > +                    )?,
> > +                    handler,
> > +                    _pin: PhantomPinned,
> > +                })
> > +            };
> > +
> > +            // 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.
> > +            let res = to_result(unsafe {
> > +                bindings::request_irq(
> > +                    irq,
> > +                    Some(handle_irq_callback::<T>),
> > +                    flags.into_inner() as usize,
> > +                    name.as_char_ptr(),
> > +                    slot.cast(),
> > +                )
> > +            });
> > +
> > +            if res.is_err() {
> > +                // SAFETY: We are returning an error, so we can destroy the slot.
> > +                unsafe { core::ptr::drop_in_place(&raw mut (*slot).handler) };
> > +            }
> > +
> > +            res
> > +        };
> > +
> > +        // SAFETY:
> > +        // - if this returns Ok, then every field of `slot` is fully
> > +        // initialized.
> > +        // - if this returns an error, then the slot does not need to remain
> > +        // valid.
> > +        unsafe { pin_init_from_closure(closure) }
>
> Can't we use try_pin_init!() instead, move request_irq() into the initializer of
> RegistrationInner and initialize inner last?

We need a pointer to the entire struct when calling
bindings::request_irq. I'm not sure this allows you to easily get one?
I don't think using container_of! here is worth it.

Alice

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

[Danilo Krummrich]

On Mon, Jun 23, 2025 at 04:10:50PM +0100, Alice Ryhl wrote:
> On Mon, Jun 9, 2025 at 12:47 PM Danilo Krummrich <dakr@kernel.org> wrote:
> >
> > On Sun, Jun 08, 2025 at 07:51:08PM -0300, Daniel Almeida wrote:
> > > +        dev: &'a Device<Bound>,
> > > +        irq: u32,
> > > +        flags: Flags,
> > > +        name: &'static CStr,
> > > +        handler: T,
> > > +    ) -> impl PinInit<Self, Error> + 'a {
> > > +        let closure = move |slot: *mut Self| {
> > > +            // SAFETY: The slot passed to pin initializer is valid for writing.
> > > +            unsafe {
> > > +                slot.write(Self {
> > > +                    inner: Devres::new(
> > > +                        dev,
> > > +                        RegistrationInner {
> > > +                            irq,
> > > +                            cookie: slot.cast(),
> > > +                        },
> > > +                        GFP_KERNEL,
> > > +                    )?,
> > > +                    handler,
> > > +                    _pin: PhantomPinned,
> > > +                })
> > > +            };
> > > +
> > > +            // 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.
> > > +            let res = to_result(unsafe {
> > > +                bindings::request_irq(
> > > +                    irq,
> > > +                    Some(handle_irq_callback::<T>),
> > > +                    flags.into_inner() as usize,
> > > +                    name.as_char_ptr(),
> > > +                    slot.cast(),
> > > +                )
> > > +            });
> > > +
> > > +            if res.is_err() {
> > > +                // SAFETY: We are returning an error, so we can destroy the slot.
> > > +                unsafe { core::ptr::drop_in_place(&raw mut (*slot).handler) };
> > > +            }
> > > +
> > > +            res
> > > +        };
> > > +
> > > +        // SAFETY:
> > > +        // - if this returns Ok, then every field of `slot` is fully
> > > +        // initialized.
> > > +        // - if this returns an error, then the slot does not need to remain
> > > +        // valid.
> > > +        unsafe { pin_init_from_closure(closure) }
> >
> > Can't we use try_pin_init!() instead, move request_irq() into the initializer of
> > RegistrationInner and initialize inner last?
> 
> We need a pointer to the entire struct when calling
> bindings::request_irq. I'm not sure this allows you to easily get one?
> I don't think using container_of! here is worth it.

Would `try_pin_init!(&this in Self { ...` work?

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

[Alice Ryhl]

On Mon, Jun 23, 2025 at 4:23 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Mon, Jun 23, 2025 at 04:10:50PM +0100, Alice Ryhl wrote:
> > On Mon, Jun 9, 2025 at 12:47 PM Danilo Krummrich <dakr@kernel.org> wrote:
> > >
> > > On Sun, Jun 08, 2025 at 07:51:08PM -0300, Daniel Almeida wrote:
> > > > +        dev: &'a Device<Bound>,
> > > > +        irq: u32,
> > > > +        flags: Flags,
> > > > +        name: &'static CStr,
> > > > +        handler: T,
> > > > +    ) -> impl PinInit<Self, Error> + 'a {
> > > > +        let closure = move |slot: *mut Self| {
> > > > +            // SAFETY: The slot passed to pin initializer is valid for writing.
> > > > +            unsafe {
> > > > +                slot.write(Self {
> > > > +                    inner: Devres::new(
> > > > +                        dev,
> > > > +                        RegistrationInner {
> > > > +                            irq,
> > > > +                            cookie: slot.cast(),
> > > > +                        },
> > > > +                        GFP_KERNEL,
> > > > +                    )?,
> > > > +                    handler,
> > > > +                    _pin: PhantomPinned,
> > > > +                })
> > > > +            };
> > > > +
> > > > +            // 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.
> > > > +            let res = to_result(unsafe {
> > > > +                bindings::request_irq(
> > > > +                    irq,
> > > > +                    Some(handle_irq_callback::<T>),
> > > > +                    flags.into_inner() as usize,
> > > > +                    name.as_char_ptr(),
> > > > +                    slot.cast(),
> > > > +                )
> > > > +            });
> > > > +
> > > > +            if res.is_err() {
> > > > +                // SAFETY: We are returning an error, so we can destroy the slot.
> > > > +                unsafe { core::ptr::drop_in_place(&raw mut (*slot).handler) };
> > > > +            }
> > > > +
> > > > +            res
> > > > +        };
> > > > +
> > > > +        // SAFETY:
> > > > +        // - if this returns Ok, then every field of `slot` is fully
> > > > +        // initialized.
> > > > +        // - if this returns an error, then the slot does not need to remain
> > > > +        // valid.
> > > > +        unsafe { pin_init_from_closure(closure) }
> > >
> > > Can't we use try_pin_init!() instead, move request_irq() into the initializer of
> > > RegistrationInner and initialize inner last?
> >
> > We need a pointer to the entire struct when calling
> > bindings::request_irq. I'm not sure this allows you to easily get one?
> > I don't think using container_of! here is worth it.
>
> Would `try_pin_init!(&this in Self { ...` work?

Ah, could be. If that works, then that's fine with me.

Alice

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

[Benno Lossin]

On Mon Jun 23, 2025 at 5:10 PM CEST, Alice Ryhl wrote:
> On Mon, Jun 9, 2025 at 12:47 PM Danilo Krummrich <dakr@kernel.org> wrote:
>> On Sun, Jun 08, 2025 at 07:51:08PM -0300, Daniel Almeida wrote:
>> > +        dev: &'a Device<Bound>,
>> > +        irq: u32,
>> > +        flags: Flags,
>> > +        name: &'static CStr,
>> > +        handler: T,
>> > +    ) -> impl PinInit<Self, Error> + 'a {
>> > +        let closure = move |slot: *mut Self| {
>> > +            // SAFETY: The slot passed to pin initializer is valid for writing.
>> > +            unsafe {
>> > +                slot.write(Self {
>> > +                    inner: Devres::new(
>> > +                        dev,
>> > +                        RegistrationInner {
>> > +                            irq,
>> > +                            cookie: slot.cast(),
>> > +                        },
>> > +                        GFP_KERNEL,
>> > +                    )?,
>> > +                    handler,
>> > +                    _pin: PhantomPinned,
>> > +                })
>> > +            };
>> > +
>> > +            // 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.
>> > +            let res = to_result(unsafe {
>> > +                bindings::request_irq(
>> > +                    irq,
>> > +                    Some(handle_irq_callback::<T>),
>> > +                    flags.into_inner() as usize,
>> > +                    name.as_char_ptr(),
>> > +                    slot.cast(),
>> > +                )
>> > +            });
>> > +
>> > +            if res.is_err() {
>> > +                // SAFETY: We are returning an error, so we can destroy the slot.
>> > +                unsafe { core::ptr::drop_in_place(&raw mut (*slot).handler) };
>> > +            }
>> > +
>> > +            res
>> > +        };
>> > +
>> > +        // SAFETY:
>> > +        // - if this returns Ok, then every field of `slot` is fully
>> > +        // initialized.
>> > +        // - if this returns an error, then the slot does not need to remain
>> > +        // valid.
>> > +        unsafe { pin_init_from_closure(closure) }
>>
>> Can't we use try_pin_init!() instead, move request_irq() into the initializer of
>> RegistrationInner and initialize inner last?
>
> We need a pointer to the entire struct when calling
> bindings::request_irq. I'm not sure this allows you to easily get one?
> I don't think using container_of! here is worth it.

There is the `&this in` syntax (`this` is of type `NonNull<Self>`):

    try_pin_init!(&this in Self {
        inner: Devres::new(
            dev,
            RegistrationInner {
                irq,
                cookie: this.as_ptr().cast(),
            },
            GFP_KERNEL,
        )?,
        handler,
        _pin: {
            to_result(unsafe {
                bindings::request_irq(
                    irq,
                    Some(handle_irq_callback::<T>),
                    flags.into_inner() as usize,
                    name.as_char_ptr(),
                    slot.as_ptr().cast(),
                )
            })?;
            PhantomPinned
        },
    })

Last time around, I also asked this question and you replied with that
we need to abort the initializer when `request_irq` returns false and
avoid running `Self::drop` (thus we can't do it using `pin_chain`).

I asked what we could do instead and you mentioned the `_: {}`
initializers and those would indeed solve it, but we can abuse the
`_pin` field for that :)

---
Cheers,
Benno

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

[Boqun Feng]

On Mon, Jun 23, 2025 at 05:26:14PM +0200, Benno Lossin wrote:
> On Mon Jun 23, 2025 at 5:10 PM CEST, Alice Ryhl wrote:
> > On Mon, Jun 9, 2025 at 12:47 PM Danilo Krummrich <dakr@kernel.org> wrote:
> >> On Sun, Jun 08, 2025 at 07:51:08PM -0300, Daniel Almeida wrote:
> >> > +        dev: &'a Device<Bound>,
> >> > +        irq: u32,
> >> > +        flags: Flags,
> >> > +        name: &'static CStr,
> >> > +        handler: T,
> >> > +    ) -> impl PinInit<Self, Error> + 'a {
> >> > +        let closure = move |slot: *mut Self| {
> >> > +            // SAFETY: The slot passed to pin initializer is valid for writing.
> >> > +            unsafe {
> >> > +                slot.write(Self {
> >> > +                    inner: Devres::new(
> >> > +                        dev,
> >> > +                        RegistrationInner {
> >> > +                            irq,
> >> > +                            cookie: slot.cast(),
> >> > +                        },
> >> > +                        GFP_KERNEL,
> >> > +                    )?,
> >> > +                    handler,
> >> > +                    _pin: PhantomPinned,
> >> > +                })
> >> > +            };
> >> > +
> >> > +            // 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.
> >> > +            let res = to_result(unsafe {
> >> > +                bindings::request_irq(
> >> > +                    irq,
> >> > +                    Some(handle_irq_callback::<T>),
> >> > +                    flags.into_inner() as usize,
> >> > +                    name.as_char_ptr(),
> >> > +                    slot.cast(),
> >> > +                )
> >> > +            });
> >> > +
> >> > +            if res.is_err() {
> >> > +                // SAFETY: We are returning an error, so we can destroy the slot.
> >> > +                unsafe { core::ptr::drop_in_place(&raw mut (*slot).handler) };
> >> > +            }
> >> > +
> >> > +            res
> >> > +        };
> >> > +
> >> > +        // SAFETY:
> >> > +        // - if this returns Ok, then every field of `slot` is fully
> >> > +        // initialized.
> >> > +        // - if this returns an error, then the slot does not need to remain
> >> > +        // valid.
> >> > +        unsafe { pin_init_from_closure(closure) }
> >>
> >> Can't we use try_pin_init!() instead, move request_irq() into the initializer of
> >> RegistrationInner and initialize inner last?
> >
> > We need a pointer to the entire struct when calling
> > bindings::request_irq. I'm not sure this allows you to easily get one?
> > I don't think using container_of! here is worth it.
> 
> There is the `&this in` syntax (`this` is of type `NonNull<Self>`):
> 
>     try_pin_init!(&this in Self {
>         inner: Devres::new(
>             dev,
>             RegistrationInner {
>                 irq,
>                 cookie: this.as_ptr().cast(),
>             },
>             GFP_KERNEL,
>         )?,
>         handler,
>         _pin: {
>             to_result(unsafe {
>                 bindings::request_irq(
>                     irq,
>                     Some(handle_irq_callback::<T>),
>                     flags.into_inner() as usize,
>                     name.as_char_ptr(),
>                     slot.as_ptr().cast(),

this is "this" instead of "slot", right?

>                 )
>             })?;
>             PhantomPinned
>         },
>     })
> 
> Last time around, I also asked this question and you replied with that
> we need to abort the initializer when `request_irq` returns false and
> avoid running `Self::drop` (thus we can't do it using `pin_chain`).
> 
> I asked what we could do instead and you mentioned the `_: {}`
> initializers and those would indeed solve it, but we can abuse the
> `_pin` field for that :)
> 

Hmm.. but if request_irq() fails, aren't we going to call `drop` on
`inner`, which drops the `Devres` which will eventually call
`RegistrationInner::drop()`? And that's a `free_irq()` without
`request_irq()` succeeded.

Regards,
Boqun

> ---
> Cheers,
> Benno

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

[Boqun Feng]

On Mon, Jun 23, 2025 at 10:31:16AM -0700, Boqun Feng wrote:
> On Mon, Jun 23, 2025 at 05:26:14PM +0200, Benno Lossin wrote:
> > On Mon Jun 23, 2025 at 5:10 PM CEST, Alice Ryhl wrote:
> > > On Mon, Jun 9, 2025 at 12:47 PM Danilo Krummrich <dakr@kernel.org> wrote:
> > >> On Sun, Jun 08, 2025 at 07:51:08PM -0300, Daniel Almeida wrote:
> > >> > +        dev: &'a Device<Bound>,
> > >> > +        irq: u32,
> > >> > +        flags: Flags,
> > >> > +        name: &'static CStr,
> > >> > +        handler: T,
> > >> > +    ) -> impl PinInit<Self, Error> + 'a {
> > >> > +        let closure = move |slot: *mut Self| {
> > >> > +            // SAFETY: The slot passed to pin initializer is valid for writing.
> > >> > +            unsafe {
> > >> > +                slot.write(Self {
> > >> > +                    inner: Devres::new(
> > >> > +                        dev,
> > >> > +                        RegistrationInner {
> > >> > +                            irq,
> > >> > +                            cookie: slot.cast(),
> > >> > +                        },
> > >> > +                        GFP_KERNEL,
> > >> > +                    )?,
> > >> > +                    handler,
> > >> > +                    _pin: PhantomPinned,
> > >> > +                })
> > >> > +            };
> > >> > +
> > >> > +            // 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.
> > >> > +            let res = to_result(unsafe {
> > >> > +                bindings::request_irq(
> > >> > +                    irq,
> > >> > +                    Some(handle_irq_callback::<T>),
> > >> > +                    flags.into_inner() as usize,
> > >> > +                    name.as_char_ptr(),
> > >> > +                    slot.cast(),
> > >> > +                )
> > >> > +            });
> > >> > +
> > >> > +            if res.is_err() {
> > >> > +                // SAFETY: We are returning an error, so we can destroy the slot.
> > >> > +                unsafe { core::ptr::drop_in_place(&raw mut (*slot).handler) };
> > >> > +            }
> > >> > +
> > >> > +            res
> > >> > +        };
> > >> > +
> > >> > +        // SAFETY:
> > >> > +        // - if this returns Ok, then every field of `slot` is fully
> > >> > +        // initialized.
> > >> > +        // - if this returns an error, then the slot does not need to remain
> > >> > +        // valid.
> > >> > +        unsafe { pin_init_from_closure(closure) }
> > >>
> > >> Can't we use try_pin_init!() instead, move request_irq() into the initializer of
> > >> RegistrationInner and initialize inner last?
> > >
> > > We need a pointer to the entire struct when calling
> > > bindings::request_irq. I'm not sure this allows you to easily get one?
> > > I don't think using container_of! here is worth it.
> > 
> > There is the `&this in` syntax (`this` is of type `NonNull<Self>`):
> > 
> >     try_pin_init!(&this in Self {
> >         inner: Devres::new(
> >             dev,
> >             RegistrationInner {
> >                 irq,
> >                 cookie: this.as_ptr().cast(),
> >             },
> >             GFP_KERNEL,
> >         )?,
> >         handler,
> >         _pin: {
> >             to_result(unsafe {
> >                 bindings::request_irq(
> >                     irq,
> >                     Some(handle_irq_callback::<T>),
> >                     flags.into_inner() as usize,
> >                     name.as_char_ptr(),
> >                     slot.as_ptr().cast(),
> 
> this is "this" instead of "slot", right?
> 
> >                 )
> >             })?;
> >             PhantomPinned
> >         },
> >     })
> > 
> > Last time around, I also asked this question and you replied with that
> > we need to abort the initializer when `request_irq` returns false and
> > avoid running `Self::drop` (thus we can't do it using `pin_chain`).
> > 
> > I asked what we could do instead and you mentioned the `_: {}`
> > initializers and those would indeed solve it, but we can abuse the
> > `_pin` field for that :)
> > 
> 
> Hmm.. but if request_irq() fails, aren't we going to call `drop` on
> `inner`, which drops the `Devres` which will eventually call
> `RegistrationInner::drop()`? And that's a `free_irq()` without
> `request_irq()` succeeded.
> 

This may however work ;-) Because at `request_irq()` time, all it needs
is ready, and if it fails, `RegistrationInner` won't construct.

    try_pin_init!(&this in Self {
        handler,
        inner: Devres::new(
            dev,
            RegistrationInner {
                // Needs to use `handler` address as cookie, same for
                // request_irq().
                cookie: &raw (*(this.as_ptr().cast()).handler),
                irq: {
                     to_result(unsafe { bindings::request_irq(...) })?;
					 irq
				}
             },
             GFP_KERNEL,
        )?,
        _pin: PhantomPinned
    })

Regards,
Boqun

> Regards,
> Boqun
> 
> > ---
> > Cheers,
> > Benno

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

[Benno Lossin]

On Mon Jun 23, 2025 at 7:31 PM CEST, Boqun Feng wrote:
> On Mon, Jun 23, 2025 at 05:26:14PM +0200, Benno Lossin wrote:
>> On Mon Jun 23, 2025 at 5:10 PM CEST, Alice Ryhl wrote:
>> > On Mon, Jun 9, 2025 at 12:47 PM Danilo Krummrich <dakr@kernel.org> wrote:
>> >> On Sun, Jun 08, 2025 at 07:51:08PM -0300, Daniel Almeida wrote:
>> >> > +        dev: &'a Device<Bound>,
>> >> > +        irq: u32,
>> >> > +        flags: Flags,
>> >> > +        name: &'static CStr,
>> >> > +        handler: T,
>> >> > +    ) -> impl PinInit<Self, Error> + 'a {
>> >> > +        let closure = move |slot: *mut Self| {
>> >> > +            // SAFETY: The slot passed to pin initializer is valid for writing.
>> >> > +            unsafe {
>> >> > +                slot.write(Self {
>> >> > +                    inner: Devres::new(
>> >> > +                        dev,
>> >> > +                        RegistrationInner {
>> >> > +                            irq,
>> >> > +                            cookie: slot.cast(),
>> >> > +                        },
>> >> > +                        GFP_KERNEL,
>> >> > +                    )?,
>> >> > +                    handler,
>> >> > +                    _pin: PhantomPinned,
>> >> > +                })
>> >> > +            };
>> >> > +
>> >> > +            // 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.
>> >> > +            let res = to_result(unsafe {
>> >> > +                bindings::request_irq(
>> >> > +                    irq,
>> >> > +                    Some(handle_irq_callback::<T>),
>> >> > +                    flags.into_inner() as usize,
>> >> > +                    name.as_char_ptr(),
>> >> > +                    slot.cast(),
>> >> > +                )
>> >> > +            });
>> >> > +
>> >> > +            if res.is_err() {
>> >> > +                // SAFETY: We are returning an error, so we can destroy the slot.
>> >> > +                unsafe { core::ptr::drop_in_place(&raw mut (*slot).handler) };
>> >> > +            }
>> >> > +
>> >> > +            res
>> >> > +        };
>> >> > +
>> >> > +        // SAFETY:
>> >> > +        // - if this returns Ok, then every field of `slot` is fully
>> >> > +        // initialized.
>> >> > +        // - if this returns an error, then the slot does not need to remain
>> >> > +        // valid.
>> >> > +        unsafe { pin_init_from_closure(closure) }
>> >>
>> >> Can't we use try_pin_init!() instead, move request_irq() into the initializer of
>> >> RegistrationInner and initialize inner last?
>> >
>> > We need a pointer to the entire struct when calling
>> > bindings::request_irq. I'm not sure this allows you to easily get one?
>> > I don't think using container_of! here is worth it.
>> 
>> There is the `&this in` syntax (`this` is of type `NonNull<Self>`):
>> 
>>     try_pin_init!(&this in Self {
>>         inner: Devres::new(
>>             dev,
>>             RegistrationInner {
>>                 irq,
>>                 cookie: this.as_ptr().cast(),
>>             },
>>             GFP_KERNEL,
>>         )?,
>>         handler,
>>         _pin: {
>>             to_result(unsafe {
>>                 bindings::request_irq(
>>                     irq,
>>                     Some(handle_irq_callback::<T>),
>>                     flags.into_inner() as usize,
>>                     name.as_char_ptr(),
>>                     slot.as_ptr().cast(),
>
> this is "this" instead of "slot", right?
>
>>                 )
>>             })?;
>>             PhantomPinned
>>         },
>>     })
>> 
>> Last time around, I also asked this question and you replied with that
>> we need to abort the initializer when `request_irq` returns false and
>> avoid running `Self::drop` (thus we can't do it using `pin_chain`).
>> 
>> I asked what we could do instead and you mentioned the `_: {}`
>> initializers and those would indeed solve it, but we can abuse the
>> `_pin` field for that :)
>> 
>
> Hmm.. but if request_irq() fails, aren't we going to call `drop` on
> `inner`, which drops the `Devres` which will eventually call
> `RegistrationInner::drop()`? And that's a `free_irq()` without
> `request_irq()` succeeded.

That is indeed correct :(

But hold on, we aren't allowed to forget the `Devres`, it's a pinned
type and thus the pin guarantee is that it must be dropped before the
underlying memory is freed. So the current version is unsound.

---
Cheers,
Benno

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

[Benno Lossin]

On Mon Jun 23, 2025 at 9:25 PM CEST, Benno Lossin wrote:
> On Mon Jun 23, 2025 at 7:31 PM CEST, Boqun Feng wrote:
>> On Mon, Jun 23, 2025 at 05:26:14PM +0200, Benno Lossin wrote:
>>> On Mon Jun 23, 2025 at 5:10 PM CEST, Alice Ryhl wrote:
>>> > On Mon, Jun 9, 2025 at 12:47 PM Danilo Krummrich <dakr@kernel.org> wrote:
>>> >> On Sun, Jun 08, 2025 at 07:51:08PM -0300, Daniel Almeida wrote:
>>> >> > +        dev: &'a Device<Bound>,
>>> >> > +        irq: u32,
>>> >> > +        flags: Flags,
>>> >> > +        name: &'static CStr,
>>> >> > +        handler: T,
>>> >> > +    ) -> impl PinInit<Self, Error> + 'a {
>>> >> > +        let closure = move |slot: *mut Self| {
>>> >> > +            // SAFETY: The slot passed to pin initializer is valid for writing.
>>> >> > +            unsafe {
>>> >> > +                slot.write(Self {
>>> >> > +                    inner: Devres::new(
>>> >> > +                        dev,
>>> >> > +                        RegistrationInner {
>>> >> > +                            irq,
>>> >> > +                            cookie: slot.cast(),
>>> >> > +                        },
>>> >> > +                        GFP_KERNEL,
>>> >> > +                    )?,
>>> >> > +                    handler,
>>> >> > +                    _pin: PhantomPinned,
>>> >> > +                })
>>> >> > +            };
>>> >> > +
>>> >> > +            // 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.
>>> >> > +            let res = to_result(unsafe {
>>> >> > +                bindings::request_irq(
>>> >> > +                    irq,
>>> >> > +                    Some(handle_irq_callback::<T>),
>>> >> > +                    flags.into_inner() as usize,
>>> >> > +                    name.as_char_ptr(),
>>> >> > +                    slot.cast(),
>>> >> > +                )
>>> >> > +            });
>>> >> > +
>>> >> > +            if res.is_err() {
>>> >> > +                // SAFETY: We are returning an error, so we can destroy the slot.
>>> >> > +                unsafe { core::ptr::drop_in_place(&raw mut (*slot).handler) };
>>> >> > +            }
>>> >> > +
>>> >> > +            res
>>> >> > +        };
>>> >> > +
>>> >> > +        // SAFETY:
>>> >> > +        // - if this returns Ok, then every field of `slot` is fully
>>> >> > +        // initialized.
>>> >> > +        // - if this returns an error, then the slot does not need to remain
>>> >> > +        // valid.
>>> >> > +        unsafe { pin_init_from_closure(closure) }
>>> >>
>>> >> Can't we use try_pin_init!() instead, move request_irq() into the initializer of
>>> >> RegistrationInner and initialize inner last?
>>> >
>>> > We need a pointer to the entire struct when calling
>>> > bindings::request_irq. I'm not sure this allows you to easily get one?
>>> > I don't think using container_of! here is worth it.
>>> 
>>> There is the `&this in` syntax (`this` is of type `NonNull<Self>`):
>>> 
>>>     try_pin_init!(&this in Self {
>>>         inner: Devres::new(
>>>             dev,
>>>             RegistrationInner {
>>>                 irq,
>>>                 cookie: this.as_ptr().cast(),
>>>             },
>>>             GFP_KERNEL,
>>>         )?,
>>>         handler,
>>>         _pin: {
>>>             to_result(unsafe {
>>>                 bindings::request_irq(
>>>                     irq,
>>>                     Some(handle_irq_callback::<T>),
>>>                     flags.into_inner() as usize,
>>>                     name.as_char_ptr(),
>>>                     slot.as_ptr().cast(),
>>
>> this is "this" instead of "slot", right?
>>
>>>                 )
>>>             })?;
>>>             PhantomPinned
>>>         },
>>>     })
>>> 
>>> Last time around, I also asked this question and you replied with that
>>> we need to abort the initializer when `request_irq` returns false and
>>> avoid running `Self::drop` (thus we can't do it using `pin_chain`).
>>> 
>>> I asked what we could do instead and you mentioned the `_: {}`
>>> initializers and those would indeed solve it, but we can abuse the
>>> `_pin` field for that :)
>>> 
>>
>> Hmm.. but if request_irq() fails, aren't we going to call `drop` on
>> `inner`, which drops the `Devres` which will eventually call
>> `RegistrationInner::drop()`? And that's a `free_irq()` without
>> `request_irq()` succeeded.
>
> That is indeed correct :(
>
> But hold on, we aren't allowed to forget the `Devres`, it's a pinned
> type and thus the pin guarantee is that it must be dropped before the
> underlying memory is freed. So the current version is unsound.

Ah oops, already had the devres improvements in mind, this version uses
the non-pinned devres, which when not dropped will leak an `Arc` in the
`DevresInner`... Which also isn't desired.

---
Cheers,
Benno

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

[Benno Lossin]

On Mon Jun 23, 2025 at 9:18 PM CEST, Boqun Feng wrote:
> On Mon, Jun 23, 2025 at 10:31:16AM -0700, Boqun Feng wrote:
>> On Mon, Jun 23, 2025 at 05:26:14PM +0200, Benno Lossin wrote:
>> > On Mon Jun 23, 2025 at 5:10 PM CEST, Alice Ryhl wrote:
>> > > On Mon, Jun 9, 2025 at 12:47 PM Danilo Krummrich <dakr@kernel.org> wrote:
>> > >> On Sun, Jun 08, 2025 at 07:51:08PM -0300, Daniel Almeida wrote:
>> > >> > +        dev: &'a Device<Bound>,
>> > >> > +        irq: u32,
>> > >> > +        flags: Flags,
>> > >> > +        name: &'static CStr,
>> > >> > +        handler: T,
>> > >> > +    ) -> impl PinInit<Self, Error> + 'a {
>> > >> > +        let closure = move |slot: *mut Self| {
>> > >> > +            // SAFETY: The slot passed to pin initializer is valid for writing.
>> > >> > +            unsafe {
>> > >> > +                slot.write(Self {
>> > >> > +                    inner: Devres::new(
>> > >> > +                        dev,
>> > >> > +                        RegistrationInner {
>> > >> > +                            irq,
>> > >> > +                            cookie: slot.cast(),
>> > >> > +                        },
>> > >> > +                        GFP_KERNEL,
>> > >> > +                    )?,
>> > >> > +                    handler,
>> > >> > +                    _pin: PhantomPinned,
>> > >> > +                })
>> > >> > +            };
>> > >> > +
>> > >> > +            // 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.
>> > >> > +            let res = to_result(unsafe {
>> > >> > +                bindings::request_irq(
>> > >> > +                    irq,
>> > >> > +                    Some(handle_irq_callback::<T>),
>> > >> > +                    flags.into_inner() as usize,
>> > >> > +                    name.as_char_ptr(),
>> > >> > +                    slot.cast(),
>> > >> > +                )
>> > >> > +            });
>> > >> > +
>> > >> > +            if res.is_err() {
>> > >> > +                // SAFETY: We are returning an error, so we can destroy the slot.
>> > >> > +                unsafe { core::ptr::drop_in_place(&raw mut (*slot).handler) };
>> > >> > +            }
>> > >> > +
>> > >> > +            res
>> > >> > +        };
>> > >> > +
>> > >> > +        // SAFETY:
>> > >> > +        // - if this returns Ok, then every field of `slot` is fully
>> > >> > +        // initialized.
>> > >> > +        // - if this returns an error, then the slot does not need to remain
>> > >> > +        // valid.
>> > >> > +        unsafe { pin_init_from_closure(closure) }
>> > >>
>> > >> Can't we use try_pin_init!() instead, move request_irq() into the initializer of
>> > >> RegistrationInner and initialize inner last?
>> > >
>> > > We need a pointer to the entire struct when calling
>> > > bindings::request_irq. I'm not sure this allows you to easily get one?
>> > > I don't think using container_of! here is worth it.
>> > 
>> > There is the `&this in` syntax (`this` is of type `NonNull<Self>`):
>> > 
>> >     try_pin_init!(&this in Self {
>> >         inner: Devres::new(
>> >             dev,
>> >             RegistrationInner {
>> >                 irq,
>> >                 cookie: this.as_ptr().cast(),
>> >             },
>> >             GFP_KERNEL,
>> >         )?,
>> >         handler,
>> >         _pin: {
>> >             to_result(unsafe {
>> >                 bindings::request_irq(
>> >                     irq,
>> >                     Some(handle_irq_callback::<T>),
>> >                     flags.into_inner() as usize,
>> >                     name.as_char_ptr(),
>> >                     slot.as_ptr().cast(),
>> 
>> this is "this" instead of "slot", right?
>> 
>> >                 )
>> >             })?;
>> >             PhantomPinned
>> >         },
>> >     })
>> > 
>> > Last time around, I also asked this question and you replied with that
>> > we need to abort the initializer when `request_irq` returns false and
>> > avoid running `Self::drop` (thus we can't do it using `pin_chain`).
>> > 
>> > I asked what we could do instead and you mentioned the `_: {}`
>> > initializers and those would indeed solve it, but we can abuse the
>> > `_pin` field for that :)
>> > 
>> 
>> Hmm.. but if request_irq() fails, aren't we going to call `drop` on
>> `inner`, which drops the `Devres` which will eventually call
>> `RegistrationInner::drop()`? And that's a `free_irq()` without
>> `request_irq()` succeeded.
>> 
>
> This may however work ;-) Because at `request_irq()` time, all it needs
> is ready, and if it fails, `RegistrationInner` won't construct.
>
>     try_pin_init!(&this in Self {
>         handler,
>         inner: Devres::new(
>             dev,
>             RegistrationInner {
>                 // Needs to use `handler` address as cookie, same for
>                 // request_irq().
>                 cookie: &raw (*(this.as_ptr().cast()).handler),
>                 irq: {
>                      to_result(unsafe { bindings::request_irq(...) })?;
> 					 irq
> 				}
>              },
>              GFP_KERNEL,
>         )?,
>         _pin: PhantomPinned
>     })

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

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

[Daniel Almeida]

> On 23 Jun 2025, at 16:28, Benno Lossin <lossin@kernel.org> wrote:
> 
> On Mon Jun 23, 2025 at 9:18 PM CEST, Boqun Feng wrote:
>> On Mon, Jun 23, 2025 at 10:31:16AM -0700, Boqun Feng wrote:
>>> On Mon, Jun 23, 2025 at 05:26:14PM +0200, Benno Lossin wrote:
>>>> On Mon Jun 23, 2025 at 5:10 PM CEST, Alice Ryhl wrote:
>>>>> On Mon, Jun 9, 2025 at 12:47 PM Danilo Krummrich <dakr@kernel.org> wrote:
>>>>>> On Sun, Jun 08, 2025 at 07:51:08PM -0300, Daniel Almeida wrote:
>>>>>>> +        dev: &'a Device<Bound>,
>>>>>>> +        irq: u32,
>>>>>>> +        flags: Flags,
>>>>>>> +        name: &'static CStr,
>>>>>>> +        handler: T,
>>>>>>> +    ) -> impl PinInit<Self, Error> + 'a {
>>>>>>> +        let closure = move |slot: *mut Self| {
>>>>>>> +            // SAFETY: The slot passed to pin initializer is valid for writing.
>>>>>>> +            unsafe {
>>>>>>> +                slot.write(Self {
>>>>>>> +                    inner: Devres::new(
>>>>>>> +                        dev,
>>>>>>> +                        RegistrationInner {
>>>>>>> +                            irq,
>>>>>>> +                            cookie: slot.cast(),
>>>>>>> +                        },
>>>>>>> +                        GFP_KERNEL,
>>>>>>> +                    )?,
>>>>>>> +                    handler,
>>>>>>> +                    _pin: PhantomPinned,
>>>>>>> +                })
>>>>>>> +            };
>>>>>>> +
>>>>>>> +            // 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.
>>>>>>> +            let res = to_result(unsafe {
>>>>>>> +                bindings::request_irq(
>>>>>>> +                    irq,
>>>>>>> +                    Some(handle_irq_callback::<T>),
>>>>>>> +                    flags.into_inner() as usize,
>>>>>>> +                    name.as_char_ptr(),
>>>>>>> +                    slot.cast(),
>>>>>>> +                )
>>>>>>> +            });
>>>>>>> +
>>>>>>> +            if res.is_err() {
>>>>>>> +                // SAFETY: We are returning an error, so we can destroy the slot.
>>>>>>> +                unsafe { core::ptr::drop_in_place(&raw mut (*slot).handler) };
>>>>>>> +            }
>>>>>>> +
>>>>>>> +            res
>>>>>>> +        };
>>>>>>> +
>>>>>>> +        // SAFETY:
>>>>>>> +        // - if this returns Ok, then every field of `slot` is fully
>>>>>>> +        // initialized.
>>>>>>> +        // - if this returns an error, then the slot does not need to remain
>>>>>>> +        // valid.
>>>>>>> +        unsafe { pin_init_from_closure(closure) }
>>>>>> 
>>>>>> Can't we use try_pin_init!() instead, move request_irq() into the initializer of
>>>>>> RegistrationInner and initialize inner last?
>>>>> 
>>>>> We need a pointer to the entire struct when calling
>>>>> bindings::request_irq. I'm not sure this allows you to easily get one?
>>>>> I don't think using container_of! here is worth it.
>>>> 
>>>> There is the `&this in` syntax (`this` is of type `NonNull<Self>`):
>>>> 
>>>>    try_pin_init!(&this in Self {
>>>>        inner: Devres::new(
>>>>            dev,
>>>>            RegistrationInner {
>>>>                irq,
>>>>                cookie: this.as_ptr().cast(),
>>>>            },
>>>>            GFP_KERNEL,
>>>>        )?,
>>>>        handler,
>>>>        _pin: {
>>>>            to_result(unsafe {
>>>>                bindings::request_irq(
>>>>                    irq,
>>>>                    Some(handle_irq_callback::<T>),
>>>>                    flags.into_inner() as usize,
>>>>                    name.as_char_ptr(),
>>>>                    slot.as_ptr().cast(),
>>> 
>>> this is "this" instead of "slot", right?
>>> 
>>>>                )
>>>>            })?;
>>>>            PhantomPinned
>>>>        },
>>>>    })
>>>> 
>>>> Last time around, I also asked this question and you replied with that
>>>> we need to abort the initializer when `request_irq` returns false and
>>>> avoid running `Self::drop` (thus we can't do it using `pin_chain`).
>>>> 
>>>> I asked what we could do instead and you mentioned the `_: {}`
>>>> initializers and those would indeed solve it, but we can abuse the
>>>> `_pin` field for that :)
>>>> 
>>> 
>>> Hmm.. but if request_irq() fails, aren't we going to call `drop` on
>>> `inner`, which drops the `Devres` which will eventually call
>>> `RegistrationInner::drop()`? And that's a `free_irq()` without
>>> `request_irq()` succeeded.
>>> 
>> 
>> This may however work ;-) Because at `request_irq()` time, all it needs
>> is ready, and if it fails, `RegistrationInner` won't construct.
>> 
>>    try_pin_init!(&this in Self {
>>        handler,
>>        inner: Devres::new(
>>            dev,
>>            RegistrationInner {
>>                // Needs to use `handler` address as cookie, same for
>>                // request_irq().
>>                cookie: &raw (*(this.as_ptr().cast()).handler),
>>                irq: {
>>                     to_result(unsafe { bindings::request_irq(...) })?;
>>  irq
>> }
>>             },
>>             GFP_KERNEL,
>>        )?,
>>        _pin: PhantomPinned
>>    })
> 
> 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.



— Daniel

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

[Benno Lossin]

On Tue Jun 24, 2025 at 2:31 PM CEST, Daniel Almeida wrote:
> On 23 Jun 2025, at 16:28, Benno Lossin <lossin@kernel.org> wrote:
>> On Mon Jun 23, 2025 at 9:18 PM CEST, Boqun Feng wrote:
>>>    try_pin_init!(&this in Self {
>>>        handler,
>>>        inner: Devres::new(
>>>            dev,
>>>            RegistrationInner {
>>>                // Needs to use `handler` address as cookie, same for
>>>                // request_irq().
>>>                cookie: &raw (*(this.as_ptr().cast()).handler),
>>>                irq: {
>>>                     to_result(unsafe { bindings::request_irq(...) })?;
>>>  irq
>>> }
>>>             },
>>>             GFP_KERNEL,
>>>        )?,
>>>        _pin: PhantomPinned
>>>    })
>> 
>> 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.

Gotcha, so you keep the cookie field, but you should still be able to
use `try_pin_init` & the devres improvements to avoid the use of
`pin_init_from_closure`.

---
Cheers,
Benno

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

[Alice Ryhl]

On Tue, Jun 24, 2025 at 1:46 PM Benno Lossin <lossin@kernel.org> wrote:
>
> On Tue Jun 24, 2025 at 2:31 PM CEST, Daniel Almeida wrote:
> > On 23 Jun 2025, at 16:28, Benno Lossin <lossin@kernel.org> wrote:
> >> On Mon Jun 23, 2025 at 9:18 PM CEST, Boqun Feng wrote:
> >>>    try_pin_init!(&this in Self {
> >>>        handler,
> >>>        inner: Devres::new(
> >>>            dev,
> >>>            RegistrationInner {
> >>>                // Needs to use `handler` address as cookie, same for
> >>>                // request_irq().
> >>>                cookie: &raw (*(this.as_ptr().cast()).handler),
> >>>                irq: {
> >>>                     to_result(unsafe { bindings::request_irq(...) })?;
> >>>  irq
> >>> }
> >>>             },
> >>>             GFP_KERNEL,
> >>>        )?,
> >>>        _pin: PhantomPinned
> >>>    })
> >>
> >> 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.
>
> Gotcha, so you keep the cookie field, but you should still be able to
> use `try_pin_init` & the devres improvements to avoid the use of
> `pin_init_from_closure`.

It sounds like this is getting too complicated and that
`pin_init_from_closure` is the simpler way to go.

Alice

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

[Boqun Feng]

On Tue, Jun 24, 2025 at 02:50:23PM +0100, Alice Ryhl wrote:
> On Tue, Jun 24, 2025 at 1:46 PM Benno Lossin <lossin@kernel.org> wrote:
> >
> > On Tue Jun 24, 2025 at 2:31 PM CEST, Daniel Almeida wrote:
> > > On 23 Jun 2025, at 16:28, Benno Lossin <lossin@kernel.org> wrote:
> > >> On Mon Jun 23, 2025 at 9:18 PM CEST, Boqun Feng wrote:
> > >>>    try_pin_init!(&this in Self {
> > >>>        handler,
> > >>>        inner: Devres::new(
> > >>>            dev,
> > >>>            RegistrationInner {
> > >>>                // Needs to use `handler` address as cookie, same for
> > >>>                // request_irq().
> > >>>                cookie: &raw (*(this.as_ptr().cast()).handler),
> > >>>                irq: {
> > >>>                     to_result(unsafe { bindings::request_irq(...) })?;
> > >>>  irq
> > >>> }
> > >>>             },
> > >>>             GFP_KERNEL,
> > >>>        )?,
> > >>>        _pin: PhantomPinned
> > >>>    })
> > >>
> > >> 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).

> >
> > Gotcha, so you keep the cookie field, but you should still be able to
> > use `try_pin_init` & the devres improvements to avoid the use of
> > `pin_init_from_closure`.
> 
> It sounds like this is getting too complicated and that
> `pin_init_from_closure` is the simpler way to go.

Even if we use `pin_init_from_closure`, we still need the other
`try_pin_init` anyway for `Devres::new()` (or alternatively we can
implement a `RegistrationInner::new()`).

Below is what would look like with the Devres changes in mind:


    try_pin_init!(&this in Self {
        handler,
        inner: <- Devres::new(
            dev,
            try_pin_init!( RegistrationInner {
                // Needs to use `handler` address as cookie, same for
                // request_irq().
                cookie: &raw (*(this.as_ptr().cast()).handler),
		// @Benno, would this "this" work here?
                irq: {
                     to_result(unsafe { bindings::request_irq(...) })?;
                     irq
		}
             }),
        )?,
        _pin: PhantomPinned
    })


Besides, working on this made me realize that we have to request_irq()
before `Devres::new()`, otherwise we may leak the irq resource,
considering the follow code from the current `pin_init_from_closure`
approach:

        let closure = move |slot: *mut Self| {
            // SAFETY: The slot passed to pin initializer is valid for writing.
            unsafe {
                slot.write(Self {
                    inner: Devres::new(
                        dev,
                        RegistrationInner {
                            irq,
                            cookie: slot.cast(),
                        },
                        GFP_KERNEL,
                    )?,
                    handler,
                    _pin: PhantomPinned,
                })
            };

`dev` can be unbound at here, right? If so, the devm callback will
revoke the `RegistrationInner`, `RegistrationInner::drop()` will then
call `free_irq()` before `request_irq()`, the best case is that we would
request_irq() with no one going to free it.

            // 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.
            let res = to_result(unsafe {
                bindings::request_irq(
                    irq,
                    Some(handle_irq_callback::<T>),
                    flags.into_inner() as usize,
                    name.as_char_ptr(),
                    slot.cast(),
                )
            });
            ...
        }

So seems to me the order of initialization has to be:

1. Initialize the `handler`.
2. `request_irq()`, i.e initialize the `RegistrationInner`.
3. `Devres::new()`, i.e initialize the `Devres`.

Regards,
Boqun

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

[Boqun Feng]

On Tue, Jun 24, 2025 at 07:33:35AM -0700, Boqun Feng wrote:
> On Tue, Jun 24, 2025 at 02:50:23PM +0100, Alice Ryhl wrote:
> > On Tue, Jun 24, 2025 at 1:46 PM Benno Lossin <lossin@kernel.org> wrote:
> > >
> > > On Tue Jun 24, 2025 at 2:31 PM CEST, Daniel Almeida wrote:
> > > > On 23 Jun 2025, at 16:28, Benno Lossin <lossin@kernel.org> wrote:
> > > >> On Mon Jun 23, 2025 at 9:18 PM CEST, Boqun Feng wrote:
> > > >>>    try_pin_init!(&this in Self {
> > > >>>        handler,
> > > >>>        inner: Devres::new(
> > > >>>            dev,
> > > >>>            RegistrationInner {
> > > >>>                // Needs to use `handler` address as cookie, same for
> > > >>>                // request_irq().
> > > >>>                cookie: &raw (*(this.as_ptr().cast()).handler),
> > > >>>                irq: {
> > > >>>                     to_result(unsafe { bindings::request_irq(...) })?;
> > > >>>  irq
> > > >>> }
> > > >>>             },
> > > >>>             GFP_KERNEL,
> > > >>>        )?,
> > > >>>        _pin: PhantomPinned
> > > >>>    })
> > > >>
> > > >> 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).
> 
> > >
> > > Gotcha, so you keep the cookie field, but you should still be able to
> > > use `try_pin_init` & the devres improvements to avoid the use of
> > > `pin_init_from_closure`.
> > 
> > It sounds like this is getting too complicated and that
> > `pin_init_from_closure` is the simpler way to go.
> 
> Even if we use `pin_init_from_closure`, we still need the other
> `try_pin_init` anyway for `Devres::new()` (or alternatively we can
> implement a `RegistrationInner::new()`).
> 
> Below is what would look like with the Devres changes in mind:
> 
> 
>     try_pin_init!(&this in Self {
>         handler,
>         inner: <- Devres::new(
>             dev,
>             try_pin_init!( RegistrationInner {
>                 // Needs to use `handler` address as cookie, same for
>                 // request_irq().
>                 cookie: &raw (*(this.as_ptr().cast()).handler),
> 		// @Benno, would this "this" work here?
>                 irq: {
>                      to_result(unsafe { bindings::request_irq(...) })?;
>                      irq
> 		}
>              }),
>         )?,
>         _pin: PhantomPinned
>     })
> 
> 

Never mind, `dev` is a `Device<Bound>` so it cannot be unbounded during
the call ;-)

Regards,
Boqun

> Besides, working on this made me realize that we have to request_irq()
> before `Devres::new()`, otherwise we may leak the irq resource,
> considering the follow code from the current `pin_init_from_closure`
> approach:
> 
>         let closure = move |slot: *mut Self| {
>             // SAFETY: The slot passed to pin initializer is valid for writing.
>             unsafe {
>                 slot.write(Self {
>                     inner: Devres::new(
>                         dev,
>                         RegistrationInner {
>                             irq,
>                             cookie: slot.cast(),
>                         },
>                         GFP_KERNEL,
>                     )?,
>                     handler,
>                     _pin: PhantomPinned,
>                 })
>             };
> 
> `dev` can be unbound at here, right? If so, the devm callback will
> revoke the `RegistrationInner`, `RegistrationInner::drop()` will then
> call `free_irq()` before `request_irq()`, the best case is that we would
> request_irq() with no one going to free it.
> 
>             // 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.
>             let res = to_result(unsafe {
>                 bindings::request_irq(
>                     irq,
>                     Some(handle_irq_callback::<T>),
>                     flags.into_inner() as usize,
>                     name.as_char_ptr(),
>                     slot.cast(),
>                 )
>             });
>             ...
>         }
> 
> So seems to me the order of initialization has to be:
> 
> 1. Initialize the `handler`.
> 2. `request_irq()`, i.e initialize the `RegistrationInner`.
> 3. `Devres::new()`, i.e initialize the `Devres`.
> 
> Regards,
> Boqun

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

[Danilo Krummrich]

On Tue, Jun 24, 2025 at 07:42:05AM -0700, Boqun Feng wrote:
> On Tue, Jun 24, 2025 at 07:33:35AM -0700, Boqun Feng wrote:
> > On Tue, Jun 24, 2025 at 02:50:23PM +0100, Alice Ryhl wrote:
> > > On Tue, Jun 24, 2025 at 1:46 PM Benno Lossin <lossin@kernel.org> wrote:
> > > >
> > > > On Tue Jun 24, 2025 at 2:31 PM CEST, Daniel Almeida wrote:
> > > > > On 23 Jun 2025, at 16:28, Benno Lossin <lossin@kernel.org> wrote:
> > > > >> On Mon Jun 23, 2025 at 9:18 PM CEST, Boqun Feng wrote:
> > > > >>>    try_pin_init!(&this in Self {
> > > > >>>        handler,
> > > > >>>        inner: Devres::new(
> > > > >>>            dev,
> > > > >>>            RegistrationInner {
> > > > >>>                // Needs to use `handler` address as cookie, same for
> > > > >>>                // request_irq().
> > > > >>>                cookie: &raw (*(this.as_ptr().cast()).handler),
> > > > >>>                irq: {
> > > > >>>                     to_result(unsafe { bindings::request_irq(...) })?;
> > > > >>>  irq
> > > > >>> }
> > > > >>>             },
> > > > >>>             GFP_KERNEL,
> > > > >>>        )?,
> > > > >>>        _pin: PhantomPinned
> > > > >>>    })
> > > > >>
> > > > >> 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).
> > 
> > > >
> > > > Gotcha, so you keep the cookie field, but you should still be able to
> > > > use `try_pin_init` & the devres improvements to avoid the use of
> > > > `pin_init_from_closure`.
> > > 
> > > It sounds like this is getting too complicated and that
> > > `pin_init_from_closure` is the simpler way to go.
> > 
> > Even if we use `pin_init_from_closure`, we still need the other
> > `try_pin_init` anyway for `Devres::new()` (or alternatively we can
> > implement a `RegistrationInner::new()`).
> > 
> > Below is what would look like with the Devres changes in mind:
> > 
> > 
> >     try_pin_init!(&this in Self {
> >         handler,
> >         inner: <- Devres::new(
> >             dev,
> >             try_pin_init!( RegistrationInner {
> >                 // Needs to use `handler` address as cookie, same for
> >                 // request_irq().
> >                 cookie: &raw (*(this.as_ptr().cast()).handler),
> > 		// @Benno, would this "this" work here?
> >                 irq: {
> >                      to_result(unsafe { bindings::request_irq(...) })?;
> >                      irq
> > 		}
> >              }),
> >         )?,
> >         _pin: PhantomPinned
> >     })
> > 
> > 
> 
> Never mind, `dev` is a `Device<Bound>` so it cannot be unbounded during
> the call ;-)

We even know that `dev` won't be unbound as long as the returned
`impl PinInit<Self, Error> + 'a` lives. :)

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

[Benno Lossin]

On Tue Jun 24, 2025 at 4:33 PM CEST, Boqun Feng wrote:
> On Tue, Jun 24, 2025 at 02:50:23PM +0100, Alice Ryhl wrote:
>> On Tue, Jun 24, 2025 at 1:46 PM Benno Lossin <lossin@kernel.org> wrote:
>> > On Tue Jun 24, 2025 at 2:31 PM CEST, Daniel Almeida wrote:
>> > > On 23 Jun 2025, at 16:28, Benno Lossin <lossin@kernel.org> wrote:
>> > >> On Mon Jun 23, 2025 at 9:18 PM CEST, Boqun Feng wrote:
>> > >>>    try_pin_init!(&this in Self {
>> > >>>        handler,
>> > >>>        inner: Devres::new(
>> > >>>            dev,
>> > >>>            RegistrationInner {
>> > >>>                // Needs to use `handler` address as cookie, same for
>> > >>>                // request_irq().
>> > >>>                cookie: &raw (*(this.as_ptr().cast()).handler),
>> > >>>                irq: {
>> > >>>                     to_result(unsafe { bindings::request_irq(...) })?;
>> > >>>  irq
>> > >>> }
>> > >>>             },
>> > >>>             GFP_KERNEL,
>> > >>>        )?,
>> > >>>        _pin: PhantomPinned
>> > >>>    })
>> > >>
>> > >> 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).
>
>> >
>> > Gotcha, so you keep the cookie field, but you should still be able to
>> > use `try_pin_init` & the devres improvements to avoid the use of
>> > `pin_init_from_closure`.
>> 
>> It sounds like this is getting too complicated and that
>> `pin_init_from_closure` is the simpler way to go.

The current code is not correct and the version that Boqun has below
looks pretty correct (and much simpler).

> Even if we use `pin_init_from_closure`, we still need the other
> `try_pin_init` anyway for `Devres::new()` (or alternatively we can
> implement a `RegistrationInner::new()`).
>
> Below is what would look like with the Devres changes in mind:
>
>
>     try_pin_init!(&this in Self {
>         handler,
>         inner: <- Devres::new(
>             dev,
>             try_pin_init!( RegistrationInner {
>                 // Needs to use `handler` address as cookie, same for
>                 // request_irq().
>                 cookie: &raw (*(this.as_ptr().cast()).handler),
> 		// @Benno, would this "this" work here?

Yes.

>                 irq: {
>                      to_result(unsafe { bindings::request_irq(...) })?;
>                      irq
> 		}
>              }),
>         )?,
>         _pin: PhantomPinned
>     })

---
Cheers,
Benno