[PATCH v3 0/2] rust: add support for request_irq

[PATCH v3 0/2] rust: add support for request_irq

[Daniel Almeida]

---
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 (2):
      rust: irq: add support for request_irq()
      rust: platform: add irq accessors

 rust/bindings/bindings_helper.h |   1 +
 rust/helpers/helpers.c          |   1 +
 rust/helpers/irq.c              |   9 +
 rust/kernel/irq.rs              |  24 +++
 rust/kernel/irq/flags.rs        | 102 +++++++++
 rust/kernel/irq/request.rs      | 455 ++++++++++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs              |   1 +
 rust/kernel/platform.rs         |  52 +++++
 8 files changed, 645 insertions(+)
---
base-commit: 0af2f6be1b4281385b618cb86ad946eded089ac8
change-id: 20250514-topics-tyr-request_irq-4f6a30837ea8

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

[PATCH v3 1/2] rust: irq: add support for request_irq()

[Daniel Almeida]

Add support for registering IRQ handlers in Rust.

IRQ handlers are extensively used in drivers when some peripheral wants
to obtain the CPU attention. Registering a handler will make the system
invoke the passed-in function whenever the chosen IRQ line is triggered.

Both regular and threaded IRQ handlers are supported through a Handler
(or ThreadedHandler) trait that is meant to be implemented by a type
that:

a) provides a function to be run by the system when the IRQ fires and,

b) holds the shared data (i.e.: `T`) between process and IRQ contexts.

The requirement that T is Sync derives from the fact that handlers might
run concurrently with other processes executing the same driver,
creating the potential for data races.

Ideally, some interior mutability must be in place if T is to be
mutated. This should usually be done through the in-flight SpinLockIrq
type.

Co-developed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
---
 rust/bindings/bindings_helper.h |   1 +
 rust/helpers/helpers.c          |   1 +
 rust/helpers/irq.c              |   9 +
 rust/kernel/irq.rs              |  24 +++
 rust/kernel/irq/flags.rs        | 102 +++++++++
 rust/kernel/irq/request.rs      | 455 ++++++++++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs              |   1 +
 7 files changed, 593 insertions(+)

diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index ab37e1d35c70d52e69b754bf855bc19911d156d8..37fff7476c81ed39fcfd280bfc432d043725df30 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -18,6 +18,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 e1c21eba9b15b672c5e4ab10c4e4c01ed407fae6..8c6d00219fe021b2e2b3ca27ad1d5ccb58b365fd 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c
@@ -15,6 +15,7 @@
 #include "cred.c"
 #include "device.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
new file mode 100644
index 0000000000000000000000000000000000000000..639690b7166f5201f5aaf52267ffe6e1e712d805
--- /dev/null
+++ b/rust/kernel/irq.rs
@@ -0,0 +1,24 @@
+// 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)
+
+/// IRQ allocation and handling.
+pub mod request;
+
+/// Flags to be used when registering IRQ handlers.
+pub mod flags;
+
+pub use request::Handler;
+pub use request::IrqReturn;
+pub use request::Registration;
+pub use request::ThreadedHandler;
+pub use request::ThreadedIrqReturn;
+pub use request::ThreadedRegistration;
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);
diff --git a/rust/kernel/irq/request.rs b/rust/kernel/irq/request.rs
new file mode 100644
index 0000000000000000000000000000000000000000..55f0ea8f9a93dc9ada67ce91af686a9634c8e8ed
--- /dev/null
+++ b/rust/kernel/irq/request.rs
@@ -0,0 +1,455 @@
+// SPDX-License-Identifier: GPL-2.0
+// SPDX-FileCopyrightText: Copyright 2025 Collabora ltd.
+
+//! IRQ allocation and handling
+
+use core::marker::PhantomPinned;
+use core::ptr::addr_of_mut;
+
+use pin_init::pin_init_from_closure;
+
+use crate::alloc::Allocator;
+use crate::error::to_result;
+use crate::irq::flags::Flags;
+use crate::prelude::*;
+use crate::str::CStr;
+use crate::sync::Arc;
+
+/// The value that can be returned from an IrqHandler or a ThreadedIrqHandler.
+#[repr(u32)]
+pub enum IrqReturn {
+    /// The interrupt was not from this device or was not handled.
+    None = bindings::irqreturn_IRQ_NONE,
+
+    /// The interrupt was handled by this device.
+    Handled = bindings::irqreturn_IRQ_HANDLED,
+}
+
+/// Callbacks for an IRQ handler.
+pub trait Handler: Sync {
+    /// The 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)
+    }
+}
+
+/// A registration of an IRQ handler for a given IRQ line.
+///
+/// # Examples
+///
+/// The following is an example of using `Registration`. It uses a
+/// [`SpinLock`](crate::sync::SpinLockIrq) to provide the interior mutability.
+/// Note that Spinlocks are not safe to use in IRQ context as of now, but may be
+/// in the future.
+///
+/// ```
+/// use kernel::prelude::*;
+/// use kernel::irq::flags;
+/// use kernel::irq::Registration;
+/// use kernel::irq::IrqReturn;
+/// use kernel::sync::Arc;
+/// use kernel::sync::SpinLock;
+/// 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(SpinLock<u32>);
+///
+/// // [`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 {
+///         // We now have exclusive access to the data by locking the
+///         // SpinLock.
+///         let mut data = self.0.lock();
+///         *data += 1;
+///
+///         IrqReturn::Handled
+///     }
+/// }
+///
+/// // This is running in process context.
+/// fn register_irq(irq: u32, handler: Handler) -> Result<Arc<Registration<Handler>>> {
+///     let registration = Registration::register(irq, 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.
+///         let mut data = registration.handler().0.lock();
+///         *data = 42;
+///     }
+///
+///     Ok(registration)
+/// }
+///
+/// # Ok::<(), Error>(())
+///```
+///
+/// # Invariants
+///
+/// * We own an irq handler using `&self` as its private data.
+///
+#[pin_data(PinnedDrop)]
+pub struct Registration<T: Handler> {
+    irq: u32,
+    #[pin]
+    handler: T,
+    #[pin]
+    /// Pinned because we need address stability so that we can pass a pointer
+    /// to the callback.
+    _pin: PhantomPinned,
+}
+
+impl<T: Handler> Registration<T> {
+    /// Registers the IRQ handler with the system for the given IRQ number. The
+    /// handler must be able to be called as soon as this function returns.
+    pub fn register(
+        irq: u32,
+        flags: Flags,
+        name: &'static CStr,
+        handler: T,
+    ) -> impl PinInit<Self, Error> {
+        let closure = move |slot: *mut Self| {
+            // SAFETY: The slot passed to pin initializer is valid for writing.
+            unsafe {
+                slot.write(Self {
+                    irq,
+                    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 as *const _ as *mut core::ffi::c_void,
+                )
+            });
+
+            if res.is_err() {
+                // SAFETY: We are returning an error, so we can destroy the slot.
+                unsafe { core::ptr::drop_in_place(addr_of_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
+    }
+}
+
+#[pinned_drop]
+impl<T: Handler> PinnedDrop for Registration<T> {
+    fn drop(self: Pin<&mut Self>) {
+        // SAFETY:
+        // - `self.irq` is the same as the one passed to `reques_irq`.
+        // -  `&self` was passed to `request_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`.
+        //
+        // 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 as *const Self as *mut core::ffi::c_void) };
+    }
+}
+
+/// The value that can be returned from `ThreadedHandler::handle_irq`.
+#[repr(u32)]
+pub enum ThreadedIrqReturn {
+    /// The interrupt was not from this device or was not handled.
+    None = bindings::irqreturn_IRQ_NONE,
+
+    /// The interrupt was handled by this device.
+    Handled = bindings::irqreturn_IRQ_HANDLED,
+
+    /// The handler wants the handler thread to wake up.
+    WakeThread = bindings::irqreturn_IRQ_WAKE_THREAD,
+}
+
+/// Callbacks for a threaded IRQ handler.
+pub trait ThreadedHandler: Sync {
+    /// The 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
+/// [`SpinLock`](crate::sync::SpinLockIrq) to provide the interior mutability.
+/// Note that Spinlocks are not safe to use in IRQ context as of now, but may be
+/// in the future.
+///
+/// ```
+/// use kernel::prelude::*;
+/// use kernel::irq::flags;
+/// use kernel::irq::ThreadedIrqReturn;
+/// use kernel::irq::ThreadedRegistration;
+/// use kernel::irq::IrqReturn;
+/// 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(SpinLock<u32>);
+///
+/// // [`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 {
+///         // We now have exclusive access to the data by locking the
+///         // SpinLockIrq.
+///         let mut data = self.0.lock();
+///         *data += 1;
+///
+///         // 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 {
+///         // We now have exclusive access to the data by locking the SpinLock.
+///         //
+///         // Ideally, we should disable interrupts while we are doing this to
+///         // avoid deadlocks, but this is not currently possible.
+///         let mut data = self.0.lock();
+///         *data += 1;
+///
+///         IrqReturn::Handled
+///     }
+/// }
+///
+/// // This is running in process context.
+/// fn register_threaded_irq(irq: u32, handler: Handler) -> Result<Arc<ThreadedRegistration<Handler>>> {
+///     let registration = ThreadedRegistration::register(irq, 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.
+///         //
+///         // Ideally, we should disable interrupts while we are doing this to
+///         // avoid deadlocks, but this is not currently possible.
+///         let mut data = registration.handler().0.lock();
+///         *data = 42;
+///     }
+///
+///     Ok(registration)
+/// }
+///
+///
+/// # Ok::<(), Error>(())
+///```
+///
+/// # Invariants
+///
+/// * We own an irq handler using `&self` as its private data.
+///
+#[pin_data(PinnedDrop)]
+pub struct ThreadedRegistration<T: ThreadedHandler> {
+    irq: u32,
+    #[pin]
+    handler: T,
+    #[pin]
+    /// Pinned because we need address stability so that we can pass a pointer
+    /// to the callback.
+    _pin: PhantomPinned,
+}
+
+impl<T: ThreadedHandler> ThreadedRegistration<T> {
+    /// Registers the IRQ handler with the system for the given IRQ number. The
+    /// handler must be able to be called as soon as this function returns.
+    pub fn register(
+        irq: u32,
+        flags: Flags,
+        name: &'static CStr,
+        handler: T,
+    ) -> impl PinInit<Self, Error> {
+        let closure = move |slot: *mut Self| {
+            // SAFETY: The slot passed to pin initializer is valid for writing.
+            unsafe {
+                slot.write(Self {
+                    irq,
+                    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(addr_of_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
+    }
+}
+
+#[pinned_drop]
+impl<T: ThreadedHandler> PinnedDrop for ThreadedRegistration<T> {
+    fn drop(self: Pin<&mut Self>) {
+        // SAFETY:
+        // - `self.irq` is the same as the one passed to `request_threaded_irq`.
+        // -  `&self` 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
+        // `ThreadedRegistration`.
+        //
+        // Notice that this will block until all handlers finish executing, so,
+        // at no point will &self be invalid while the handler is running.
+        unsafe { bindings::free_irq(self.irq, &*self as *const Self as *mut core::ffi::c_void) };
+    }
+}
+
+/// # Safety
+///
+/// This function should be only used as the callback in `request_irq`.
+unsafe extern "C" fn handle_irq_callback<T: Handler>(
+    _irq: i32,
+    ptr: *mut core::ffi::c_void,
+) -> core::ffi::c_uint {
+    // SAFETY: `ptr` is a pointer to T set in `Registration::new`
+    let data = unsafe { &*(ptr as *const T) };
+    T::handle_irq(data) as _
+}
+
+/// # Safety
+///
+/// This function should be only used as the callback in `request_threaded_irq`.
+unsafe extern "C" fn handle_threaded_irq_callback<T: ThreadedHandler>(
+    _irq: i32,
+    ptr: *mut core::ffi::c_void,
+) -> core::ffi::c_uint {
+    // SAFETY: `ptr` is a pointer to T set in `ThreadedRegistration::new`
+    let data = unsafe { &*(ptr as *const T) };
+    T::handle_irq(data) as _
+}
+
+/// # Safety
+///
+/// This function should be only used as the callback in `request_threaded_irq`.
+unsafe extern "C" fn thread_fn_callback<T: ThreadedHandler>(
+    _irq: i32,
+    ptr: *mut core::ffi::c_void,
+) -> core::ffi::c_uint {
+    // SAFETY: `ptr` is a pointer to T set in `ThreadedRegistration::new`
+    let data = unsafe { &*(ptr as *const T) };
+    T::thread_fn(data) as _
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index de07aadd1ff5fe46fd89517e234b97a6590c8e93..4caa1a8f96c6c9a2ac6979bd7fcb60d2b6f55132 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -56,6 +56,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 v3 2/2] rust: platform: add irq accessors

[Daniel Almeida]

These accessors can be used to retrieve an IRQ from a platform device by
index or name. The IRQ can then be used to request the line using
irq::request::Registration::register() and
irq::request::ThreadedRegistration::register().

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

diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs
index 4917cb34e2fe8027d3d861e90de51de85f006735..1acaebf38d99d06f93fa13b0b356671ea77ed97a 100644
--- a/rust/kernel/platform.rs
+++ b/rust/kernel/platform.rs
@@ -188,6 +188,58 @@ impl Device {
     fn as_raw(&self) -> *mut bindings::platform_device {
         self.0.get()
     }
+
+    /// Returns an IRQ for the device by index.
+    pub fn irq_by_index(&self, index: u32) -> Result<u32> {
+        // SAFETY: `self.as_raw` returns a valid pointer to a `struct platform_device`.
+        let res = unsafe { bindings::platform_get_irq(self.as_raw(), index) };
+
+        if res < 0 {
+            return Err(Error::from_errno(res));
+        }
+
+        Ok(res as u32)
+    }
+
+    /// Same as [`Self::irq_by_index`] but does not print an error message if an IRQ
+    /// cannot be obtained.
+    pub fn optional_irq_by_index(&self, index: u32) -> Result<u32> {
+        // SAFETY: `self.as_raw` returns a valid pointer to a `struct platform_device`.
+        let res = unsafe { bindings::platform_get_irq_optional(self.as_raw(), index) };
+
+        if res < 0 {
+            return Err(Error::from_errno(res));
+        }
+
+        Ok(res as u32)
+    }
+
+    /// Returns an IRQ for the device by name.
+    pub fn irq_by_name(&self, name: &CStr) -> Result<u32> {
+        // SAFETY: `self.as_raw` returns a valid pointer to a `struct platform_device`.
+        let res = unsafe { bindings::platform_get_irq_byname(self.as_raw(), name.as_char_ptr()) };
+
+        if res < 0 {
+            return Err(Error::from_errno(res));
+        }
+
+        Ok(res as u32)
+    }
+
+    /// Same as [`Self::irq_by_name`] but does not print an error message if an IRQ
+    /// cannot be obtained.
+    pub fn optional_irq_by_name(&self, name: &CStr) -> Result<u32> {
+        // SAFETY: `self.as_raw` returns a valid pointer to a `struct platform_device`.
+        let res = unsafe {
+            bindings::platform_get_irq_byname_optional(self.as_raw(), name.as_char_ptr())
+        };
+
+        if res < 0 {
+            return Err(Error::from_errno(res));
+        }
+
+        Ok(res as u32)
+    }
 }
 
 impl Deref for Device<device::Core> {

-- 
2.49.0

Re: [PATCH v3 1/2] rust: irq: add support for request_irq()

[Benno Lossin]

On Wed May 14, 2025 at 9:20 PM CEST, Daniel Almeida wrote:
> Add support for registering IRQ handlers in Rust.
>
> IRQ handlers are extensively used in drivers when some peripheral wants
> to obtain the CPU attention. Registering a handler will make the system
> invoke the passed-in function whenever the chosen IRQ line is triggered.
>
> Both regular and threaded IRQ handlers are supported through a Handler
> (or ThreadedHandler) trait that is meant to be implemented by a type
> that:
>
> a) provides a function to be run by the system when the IRQ fires and,
>
> b) holds the shared data (i.e.: `T`) between process and IRQ contexts.
>
> The requirement that T is Sync derives from the fact that handlers might
> run concurrently with other processes executing the same driver,
> creating the potential for data races.
>
> Ideally, some interior mutability must be in place if T is to be
> mutated. This should usually be done through the in-flight SpinLockIrq
> type.
>
> Co-developed-by: Alice Ryhl <aliceryhl@google.com>
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
> ---
>  rust/bindings/bindings_helper.h |   1 +
>  rust/helpers/helpers.c          |   1 +
>  rust/helpers/irq.c              |   9 +
>  rust/kernel/irq.rs              |  24 +++
>  rust/kernel/irq/flags.rs        | 102 +++++++++
>  rust/kernel/irq/request.rs      | 455 ++++++++++++++++++++++++++++++++++++++++
>  rust/kernel/lib.rs              |   1 +
>  7 files changed, 593 insertions(+)

Could you split this patch into smaller chunks?

> +pub use request::Handler;
> +pub use request::IrqReturn;
> +pub use request::Registration;
> +pub use request::ThreadedHandler;
> +pub use request::ThreadedIrqReturn;
> +pub use request::ThreadedRegistration;

Why not?:

    pub use request::{Handler, ..., ThreadedRegistration};

> 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);

The constants below seem to all be 32 bit, why did you choose u64?

> +
> +impl Flags {
> +    pub(crate) fn into_inner(self) -> u64 {
> +        self.0
> +    }
> +}
> +pub const NO_DEBUG: Flags = Flags(bindings::IRQF_NO_DEBUG as u64);

> diff --git a/rust/kernel/irq/request.rs b/rust/kernel/irq/request.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..55f0ea8f9a93dc9ada67ce91af686a9634c8e8ed
> --- /dev/null
> +++ b/rust/kernel/irq/request.rs
> @@ -0,0 +1,455 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// SPDX-FileCopyrightText: Copyright 2025 Collabora ltd.
> +
> +//! IRQ allocation and handling

Missing `.`.

> +
> +use core::marker::PhantomPinned;
> +use core::ptr::addr_of_mut;
> +
> +use pin_init::pin_init_from_closure;
> +
> +use crate::alloc::Allocator;
> +use crate::error::to_result;
> +use crate::irq::flags::Flags;
> +use crate::prelude::*;
> +use crate::str::CStr;
> +use crate::sync::Arc;
> +
> +/// The value that can be returned from an IrqHandler or a ThreadedIrqHandler.
> +#[repr(u32)]

I think we should let the compiler decide the layout & discriminants, it
might do something smarter when returning this value together with
others. Then we just need this function:

    fn into_inner(self) -> u32 {
        match self {
            Self::None => bindings::irqreturn_IRQ_NONE,
            Self::Handled => bindings::irqreturn_IRQ_HANDLED,
        }
    }

> +pub enum IrqReturn {
> +    /// The interrupt was not from this device or was not handled.
> +    None = bindings::irqreturn_IRQ_NONE,
> +
> +    /// The interrupt was handled by this device.
> +    Handled = bindings::irqreturn_IRQ_HANDLED,
> +}
> +
> +/// Callbacks for an IRQ handler.
> +pub trait Handler: Sync {
> +    /// The 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)
> +    }
> +}
> +
> +/// A registration of an IRQ handler for a given IRQ line.
> +///
> +/// # Examples
> +///
> +/// The following is an example of using `Registration`. It uses a
> +/// [`SpinLock`](crate::sync::SpinLockIrq) to provide the interior mutability.
> +/// Note that Spinlocks are not safe to use in IRQ context as of now, but may be
> +/// in the future.

Didn't your commit message mention SpinLockIrq?

> +///
> +/// ```
> +/// use kernel::prelude::*;
> +/// use kernel::irq::flags;
> +/// use kernel::irq::Registration;
> +/// use kernel::irq::IrqReturn;
> +/// use kernel::sync::Arc;
> +/// use kernel::sync::SpinLock;
> +/// 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(SpinLock<u32>);
> +///
> +/// // [`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 {
> +///         // We now have exclusive access to the data by locking the
> +///         // SpinLock.
> +///         let mut data = self.0.lock();
> +///         *data += 1;
> +///
> +///         IrqReturn::Handled
> +///     }
> +/// }
> +///
> +/// // This is running in process context.
> +/// fn register_irq(irq: u32, handler: Handler) -> Result<Arc<Registration<Handler>>> {
> +///     let registration = Registration::register(irq, 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.
> +///         let mut data = registration.handler().0.lock();
> +///         *data = 42;
> +///     }
> +///
> +///     Ok(registration)
> +/// }
> +///
> +/// # Ok::<(), Error>(())
> +///```
> +///
> +/// # Invariants
> +///
> +/// * We own an irq handler using `&self` as its private data.
> +///
> +#[pin_data(PinnedDrop)]
> +pub struct Registration<T: Handler> {
> +    irq: u32,
> +    #[pin]
> +    handler: T,
> +    #[pin]
> +    /// Pinned because we need address stability so that we can pass a pointer
> +    /// to the callback.
> +    _pin: PhantomPinned,
> +}
> +
> +impl<T: Handler> Registration<T> {
> +    /// Registers the IRQ handler with the system for the given IRQ number. The
> +    /// handler must be able to be called as soon as this function returns.

The first line of documentation should be a single sentence description
of what the item does. It will get rendered next to it on the summary &
search pages.

What is meant by the second sentence? What about this phrasing?: "The
handler might be called immediately after this function returns.".

> +    pub fn register(
> +        irq: u32,
> +        flags: Flags,
> +        name: &'static CStr,
> +        handler: T,
> +    ) -> impl PinInit<Self, Error> {
> +        let closure = move |slot: *mut Self| {
> +            // SAFETY: The slot passed to pin initializer is valid for writing.
> +            unsafe {
> +                slot.write(Self {
> +                    irq,
> +                    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 as *const _ as *mut core::ffi::c_void,

Please don't use `as` casts when possible, instead use `.cast()` on
pointers.

> +                )
> +            });
> +
> +            if res.is_err() {
> +                // SAFETY: We are returning an error, so we can destroy the slot.
> +                unsafe { core::ptr::drop_in_place(addr_of_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) }

Please don't use `pin_init_from_closure`, instead do this:

    pin_init!(Self {
        irq,
        handler,
        _pin: PhantomPinned
    })
    .pin_chain(|this| {
        // SAFETY: TODO: correct FFI safety requirements
        to_result(unsafe {
            bindings::request_irq(...)
        })
    })

The `pin_chain` function is exactly for this use-case, doing some
operation that might fail after initializing & it will drop the value
when the closure fails.

> +    }
> +
> +    /// Returns a reference to the handler that was registered with the system.
> +    pub fn handler(&self) -> &T {
> +        &self.handler
> +    }
> +}
> +
> +#[pinned_drop]
> +impl<T: Handler> PinnedDrop for Registration<T> {
> +    fn drop(self: Pin<&mut Self>) {
> +        // SAFETY:
> +        // - `self.irq` is the same as the one passed to `reques_irq`.
> +        // -  `&self` was passed to `request_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`.

This is missing important information: `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.

This is good to know!

> +        unsafe { bindings::free_irq(self.irq, &*self as *const Self as *mut core::ffi::c_void) };
> +    }
> +}
> +
> +/// The value that can be returned from `ThreadedHandler::handle_irq`.
> +#[repr(u32)]
> +pub enum ThreadedIrqReturn {
> +    /// The interrupt was not from this device or was not handled.
> +    None = bindings::irqreturn_IRQ_NONE,
> +
> +    /// The interrupt was handled by this device.
> +    Handled = bindings::irqreturn_IRQ_HANDLED,
> +
> +    /// The handler wants the handler thread to wake up.

How about "The handler wants the handler thread to take care of the
interrupt." or is that incorrect?

> +    WakeThread = bindings::irqreturn_IRQ_WAKE_THREAD,
> +}
> +
> +/// Callbacks for a threaded IRQ handler.

What is the difference to a normal one?

> +pub trait ThreadedHandler: Sync {
> +    /// The actual handler function. As usual, sleeps are not allowed in IRQ
> +    /// context.
> +    fn handle_irq(&self) -> ThreadedIrqReturn;

Why does this `handle_irq` function return a `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;

... and this `thread_fn` an `IrqReturn`? I would have expected it to be
the other way around.

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

Ah this is some information that should be on `ThreadedHandler`. (it
also explains the difference in return types above)

> +///
> +/// # Examples
> +///
> +/// The following is an example of using `ThreadedRegistration`. It uses a
> +/// [`SpinLock`](crate::sync::SpinLockIrq) to provide the interior mutability.
> +/// Note that Spinlocks are not safe to use in IRQ context as of now, but may be
> +/// in the future.
> +///
> +/// ```
> +/// use kernel::prelude::*;
> +/// use kernel::irq::flags;
> +/// use kernel::irq::ThreadedIrqReturn;
> +/// use kernel::irq::ThreadedRegistration;
> +/// use kernel::irq::IrqReturn;
> +/// 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(SpinLock<u32>);
> +///
> +/// // [`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 {
> +///         // We now have exclusive access to the data by locking the
> +///         // SpinLockIrq.
> +///         let mut data = self.0.lock();
> +///         *data += 1;
> +///
> +///         // 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 {
> +///         // We now have exclusive access to the data by locking the SpinLock.
> +///         //
> +///         // Ideally, we should disable interrupts while we are doing this to
> +///         // avoid deadlocks, but this is not currently possible.

Would this be solved by SpinLockIrq?

---
Cheers,
Benno

> +///         let mut data = self.0.lock();
> +///         *data += 1;
> +///
> +///         IrqReturn::Handled
> +///     }
> +/// }

Re: [PATCH v3 2/2] rust: platform: add irq accessors

[Benno Lossin]

On Wed May 14, 2025 at 9:20 PM CEST, Daniel Almeida wrote:
> +    /// Same as [`Self::irq_by_name`] but does not print an error message if an IRQ
> +    /// cannot be obtained.
> +    pub fn optional_irq_by_name(&self, name: &CStr) -> Result<u32> {
> +        // SAFETY: `self.as_raw` returns a valid pointer to a `struct platform_device`.
> +        let res = unsafe {
> +            bindings::platform_get_irq_byname_optional(self.as_raw(), name.as_char_ptr())
> +        };
> +
> +        if res < 0 {
> +            return Err(Error::from_errno(res));
> +        }
> +
> +        Ok(res as u32)

This patch could make much use of the function for `Error` that does the
last 4 lines.

---
Cheers,
Benno

> +    }
>  }
>  
>  impl Deref for Device<device::Core> {

Re: [PATCH v3 1/2] rust: irq: add support for request_irq()

[Daniel Almeida]

Hi Benno, I really appreciate you taking the time to review my patches.


> On 14 May 2025, at 17:04, Benno Lossin <lossin@kernel.org> wrote:
> 
> On Wed May 14, 2025 at 9:20 PM CEST, Daniel Almeida wrote:
>> Add support for registering IRQ handlers in Rust.
>> 
>> IRQ handlers are extensively used in drivers when some peripheral wants
>> to obtain the CPU attention. Registering a handler will make the system
>> invoke the passed-in function whenever the chosen IRQ line is triggered.
>> 
>> Both regular and threaded IRQ handlers are supported through a Handler
>> (or ThreadedHandler) trait that is meant to be implemented by a type
>> that:
>> 
>> a) provides a function to be run by the system when the IRQ fires and,
>> 
>> b) holds the shared data (i.e.: `T`) between process and IRQ contexts.
>> 
>> The requirement that T is Sync derives from the fact that handlers might
>> run concurrently with other processes executing the same driver,
>> creating the potential for data races.
>> 
>> Ideally, some interior mutability must be in place if T is to be
>> mutated. This should usually be done through the in-flight SpinLockIrq
>> type.
>> 
>> Co-developed-by: Alice Ryhl <aliceryhl@google.com>
>> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
>> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
>> ---
>> rust/bindings/bindings_helper.h |   1 +
>> rust/helpers/helpers.c          |   1 +
>> rust/helpers/irq.c              |   9 +
>> rust/kernel/irq.rs              |  24 +++
>> rust/kernel/irq/flags.rs        | 102 +++++++++
>> rust/kernel/irq/request.rs      | 455 ++++++++++++++++++++++++++++++++++++++++
>> rust/kernel/lib.rs              |   1 +
>> 7 files changed, 593 insertions(+)
> 
> Could you split this patch into smaller chunks?

How? This patch does one thing: it adds request_irq functionality.

> 
>> +pub use request::Handler;
>> +pub use request::IrqReturn;
>> +pub use request::Registration;
>> +pub use request::ThreadedHandler;
>> +pub use request::ThreadedIrqReturn;
>> +pub use request::ThreadedRegistration;
> 
> Why not?:
> 
>    pub use request::{Handler, ..., ThreadedRegistration};

I dislike this notation. It is just a magnet for conflicts.

> 
>> 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);
> 
> The constants below seem to all be 32 bit, why did you choose u64?

The C code takes in ffi::c_ulong. Shouldn’t this map to u64? Or maybe usize.

> 
>> +
>> +impl Flags {
>> +    pub(crate) fn into_inner(self) -> u64 {
>> +        self.0
>> +    }
>> +}
>> +pub const NO_DEBUG: Flags = Flags(bindings::IRQF_NO_DEBUG as u64);
> 
>> diff --git a/rust/kernel/irq/request.rs b/rust/kernel/irq/request.rs
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..55f0ea8f9a93dc9ada67ce91af686a9634c8e8ed
>> --- /dev/null
>> +++ b/rust/kernel/irq/request.rs
>> @@ -0,0 +1,455 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// SPDX-FileCopyrightText: Copyright 2025 Collabora ltd.
>> +
>> +//! IRQ allocation and handling
> 
> Missing `.`.
> 
>> +
>> +use core::marker::PhantomPinned;
>> +use core::ptr::addr_of_mut;
>> +
>> +use pin_init::pin_init_from_closure;
>> +
>> +use crate::alloc::Allocator;
>> +use crate::error::to_result;
>> +use crate::irq::flags::Flags;
>> +use crate::prelude::*;
>> +use crate::str::CStr;
>> +use crate::sync::Arc;
>> +
>> +/// The value that can be returned from an IrqHandler or a ThreadedIrqHandler.
>> +#[repr(u32)]
> 
> I think we should let the compiler decide the layout & discriminants, it
> might do something smarter when returning this value together with
> others. Then we just need this function:
> 
>    fn into_inner(self) -> u32 {
>        match self {
>            Self::None => bindings::irqreturn_IRQ_NONE,
>            Self::Handled => bindings::irqreturn_IRQ_HANDLED,
>        }
>    }

Right

> 
>> +pub enum IrqReturn {
>> +    /// The interrupt was not from this device or was not handled.
>> +    None = bindings::irqreturn_IRQ_NONE,
>> +
>> +    /// The interrupt was handled by this device.
>> +    Handled = bindings::irqreturn_IRQ_HANDLED,
>> +}
>> +
>> +/// Callbacks for an IRQ handler.
>> +pub trait Handler: Sync {
>> +    /// The 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)
>> +    }
>> +}
>> +
>> +/// A registration of an IRQ handler for a given IRQ line.
>> +///
>> +/// # Examples
>> +///
>> +/// The following is an example of using `Registration`. It uses a
>> +/// [`SpinLock`](crate::sync::SpinLockIrq) to provide the interior mutability.
>> +/// Note that Spinlocks are not safe to use in IRQ context as of now, but may be
>> +/// in the future.
> 
> Didn't your commit message mention SpinLockIrq?

This is not upstream yet. I removed all mentions of SpinLockIrq on
purpose, but I apparently missed some as you say.

We definitely need interrupt-aware spinlocks to access the data in interrupt
context. It is just a matter of deciding whether we will be referring to a type
whose API is not 100% formalized as we speak, or to SpinLock itself - which is
already upstream - with a caveat. I chose the latter.


> 
>> +///
>> +/// ```
>> +/// use kernel::prelude::*;
>> +/// use kernel::irq::flags;
>> +/// use kernel::irq::Registration;
>> +/// use kernel::irq::IrqReturn;
>> +/// use kernel::sync::Arc;
>> +/// use kernel::sync::SpinLock;
>> +/// 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(SpinLock<u32>);
>> +///
>> +/// // [`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 {
>> +///         // We now have exclusive access to the data by locking the
>> +///         // SpinLock.
>> +///         let mut data = self.0.lock();
>> +///         *data += 1;
>> +///
>> +///         IrqReturn::Handled
>> +///     }
>> +/// }
>> +///
>> +/// // This is running in process context.
>> +/// fn register_irq(irq: u32, handler: Handler) -> Result<Arc<Registration<Handler>>> {
>> +///     let registration = Registration::register(irq, 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.
>> +///         let mut data = registration.handler().0.lock();
>> +///         *data = 42;
>> +///     }
>> +///
>> +///     Ok(registration)
>> +/// }
>> +///
>> +/// # Ok::<(), Error>(())
>> +///```
>> +///
>> +/// # Invariants
>> +///
>> +/// * We own an irq handler using `&self` as its private data.
>> +///
>> +#[pin_data(PinnedDrop)]
>> +pub struct Registration<T: Handler> {
>> +    irq: u32,
>> +    #[pin]
>> +    handler: T,
>> +    #[pin]
>> +    /// Pinned because we need address stability so that we can pass a pointer
>> +    /// to the callback.
>> +    _pin: PhantomPinned,
>> +}
>> +
>> +impl<T: Handler> Registration<T> {
>> +    /// Registers the IRQ handler with the system for the given IRQ number. The
>> +    /// handler must be able to be called as soon as this function returns.
> 
> The first line of documentation should be a single sentence description
> of what the item does. It will get rendered next to it on the summary &
> search pages.

Right, I actually rendered the docs before submitting, but apparently I missed this.

> 
> What is meant by the second sentence? What about this phrasing?: "The
> handler might be called immediately after this function returns.".

It's a reminder, inherited from the C side, that literally as soon as
request_irq() returns, your handler may be called. Anything that you need to
setup before your handler can run must already have been done by this point.

Is this the “second sentence” you are referring to?

> 
>> +    pub fn register(
>> +        irq: u32,
>> +        flags: Flags,
>> +        name: &'static CStr,
>> +        handler: T,
>> +    ) -> impl PinInit<Self, Error> {
>> +        let closure = move |slot: *mut Self| {
>> +            // SAFETY: The slot passed to pin initializer is valid for writing.
>> +            unsafe {
>> +                slot.write(Self {
>> +                    irq,
>> +                    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 as *const _ as *mut core::ffi::c_void,
> 
> Please don't use `as` casts when possible, instead use `.cast()` on
> pointers.

Ack

> 
>> +                )
>> +            });
>> +
>> +            if res.is_err() {
>> +                // SAFETY: We are returning an error, so we can destroy the slot.
>> +                unsafe { core::ptr::drop_in_place(addr_of_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) }
> 
> Please don't use `pin_init_from_closure`, instead do this:
> 
>    pin_init!(Self {
>        irq,
>        handler,
>        _pin: PhantomPinned
>    })
>    .pin_chain(|this| {
>        // SAFETY: TODO: correct FFI safety requirements
>        to_result(unsafe {
>            bindings::request_irq(...)
>        })
>    })
> 
> The `pin_chain` function is exactly for this use-case, doing some
> operation that might fail after initializing & it will drop the value
> when the closure fails.

Ack

> 
>> +    }
>> +
>> +    /// Returns a reference to the handler that was registered with the system.
>> +    pub fn handler(&self) -> &T {
>> +        &self.handler
>> +    }
>> +}
>> +
>> +#[pinned_drop]
>> +impl<T: Handler> PinnedDrop for Registration<T> {
>> +    fn drop(self: Pin<&mut Self>) {
>> +        // SAFETY:
>> +        // - `self.irq` is the same as the one passed to `reques_irq`.
>> +        // -  `&self` was passed to `request_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`.
> 
> This is missing important information: `self` is `!Unpin` **and** was
> initializing using pin-init, so it occupied the same memory location for
> the entirety of its lifetime.
> 

Ack

>> +        //
>> +        // Notice that this will block until all handlers finish executing,
>> +        // i.e.: at no point will &self be invalid while the handler is running.
> 
> This is good to know!
> 
>> +        unsafe { bindings::free_irq(self.irq, &*self as *const Self as *mut core::ffi::c_void) };
>> +    }
>> +}
>> +
>> +/// The value that can be returned from `ThreadedHandler::handle_irq`.
>> +#[repr(u32)]
>> +pub enum ThreadedIrqReturn {
>> +    /// The interrupt was not from this device or was not handled.
>> +    None = bindings::irqreturn_IRQ_NONE,
>> +
>> +    /// The interrupt was handled by this device.
>> +    Handled = bindings::irqreturn_IRQ_HANDLED,
>> +
>> +    /// The handler wants the handler thread to wake up.
> 
> How about "The handler wants the handler thread to take care of the
> interrupt." or is that incorrect?

This is taken straight from the C docs. As I told Andreas, I’d prefer if we
didn’t add our own docs for C things that have their own docs already.

> 
>> +    WakeThread = bindings::irqreturn_IRQ_WAKE_THREAD,
>> +}
>> +
>> +/// Callbacks for a threaded IRQ handler.
> 
> What is the difference to a normal one?
> 
>> +pub trait ThreadedHandler: Sync {
>> +    /// The actual handler function. As usual, sleeps are not allowed in IRQ
>> +    /// context.
>> +    fn handle_irq(&self) -> ThreadedIrqReturn;
> 
> Why does this `handle_irq` function return a `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;
> 
> ... and this `thread_fn` an `IrqReturn`? I would have expected it to be
> the other way around.
> 
>> +}
>> +
>> +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`.
> 
> Ah this is some information that should be on `ThreadedHandler`. (it
> also explains the difference in return types above)

Ack

> 
>> +///
>> +/// # Examples
>> +///
>> +/// The following is an example of using `ThreadedRegistration`. It uses a
>> +/// [`SpinLock`](crate::sync::SpinLockIrq) to provide the interior mutability.
>> +/// Note that Spinlocks are not safe to use in IRQ context as of now, but may be
>> +/// in the future.
>> +///
>> +/// ```
>> +/// use kernel::prelude::*;
>> +/// use kernel::irq::flags;
>> +/// use kernel::irq::ThreadedIrqReturn;
>> +/// use kernel::irq::ThreadedRegistration;
>> +/// use kernel::irq::IrqReturn;
>> +/// 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(SpinLock<u32>);
>> +///
>> +/// // [`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 {
>> +///         // We now have exclusive access to the data by locking the
>> +///         // SpinLockIrq.
>> +///         let mut data = self.0.lock();
>> +///         *data += 1;
>> +///
>> +///         // 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 {
>> +///         // We now have exclusive access to the data by locking the SpinLock.
>> +///         //
>> +///         // Ideally, we should disable interrupts while we are doing this to
>> +///         // avoid deadlocks, but this is not currently possible.
> 
> Would this be solved by SpinLockIrq?


Yes, that is the point of SpinLockIrq. The exact syntax is still up for debate,
although by this time I think Lyude & Boqun have already settled on something.


> 
> ---
> Cheers,
> Benno
> 
>> +///         let mut data = self.0.lock();
>> +///         *data += 1;
>> +///
>> +///         IrqReturn::Handled
>> +///     }
>> +/// }

— Daniel

Re: [PATCH v3 1/2] rust: irq: add support for request_irq()

[Daniel Almeida]

>> Didn't your commit message mention SpinLockIrq?
> 
> This is not upstream yet. I removed all mentions of SpinLockIrq on
> purpose, but I apparently missed some as you say.
> 
> We definitely need interrupt-aware spinlocks to access the data in interrupt
> context. It is just a matter of deciding whether we will be referring to a type
> whose API is not 100% formalized as we speak, or to SpinLock itself - which is
> already upstream - with a caveat. I chose the latter.
> 

Minor correction => to access the data in process context.

— Daniel

Re: [PATCH v3 1/2] rust: irq: add support for request_irq()

[Danilo Krummrich]

On Wed, May 14, 2025 at 04:20:51PM -0300, Daniel Almeida wrote:
> +/// // This is running in process context.
> +/// fn register_irq(irq: u32, handler: Handler) -> Result<Arc<Registration<Handler>>> {
> +///     let registration = Registration::register(irq, 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)?;

This makes it possible to arbitrarily extend the lifetime of an IRQ
registration. However, we must guarantee that the IRQ is unregistered when the
corresponding device is unbound. We can't allow drivers to hold on to device
resources after the corresponding device has been unbound.

Why does the data need to be part of the IRQ registration itself? Why can't we
pass in an Arc<T> instance already when we register the IRQ?

This way we'd never have a reason to ever access the Registration instance
itself ever again and we can easily wrap it as Devres<irq::Registration> -
analogously to devm_request_irq() on the C side - without any penalties.

> +///     // 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.
> +///         let mut data = registration.handler().0.lock();
> +///         *data = 42;
> +///     }
> +///
> +///     Ok(registration)
> +/// }

Re: [PATCH v3 1/2] rust: irq: add support for request_irq()

[Benno Lossin]

On Wed May 14, 2025 at 10:58 PM CEST, Daniel Almeida wrote:
> Hi Benno, I really appreciate you taking the time to review my patches.

Glad I can help :)

>> On 14 May 2025, at 17:04, Benno Lossin <lossin@kernel.org> wrote:
>> On Wed May 14, 2025 at 9:20 PM CEST, Daniel Almeida wrote:
>>> Add support for registering IRQ handlers in Rust.
>>> 
>>> IRQ handlers are extensively used in drivers when some peripheral wants
>>> to obtain the CPU attention. Registering a handler will make the system
>>> invoke the passed-in function whenever the chosen IRQ line is triggered.
>>> 
>>> Both regular and threaded IRQ handlers are supported through a Handler
>>> (or ThreadedHandler) trait that is meant to be implemented by a type
>>> that:
>>> 
>>> a) provides a function to be run by the system when the IRQ fires and,
>>> 
>>> b) holds the shared data (i.e.: `T`) between process and IRQ contexts.
>>> 
>>> The requirement that T is Sync derives from the fact that handlers might
>>> run concurrently with other processes executing the same driver,
>>> creating the potential for data races.
>>> 
>>> Ideally, some interior mutability must be in place if T is to be
>>> mutated. This should usually be done through the in-flight SpinLockIrq
>>> type.
>>> 
>>> Co-developed-by: Alice Ryhl <aliceryhl@google.com>
>>> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
>>> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
>>> ---
>>> rust/bindings/bindings_helper.h |   1 +
>>> rust/helpers/helpers.c          |   1 +
>>> rust/helpers/irq.c              |   9 +
>>> rust/kernel/irq.rs              |  24 +++
>>> rust/kernel/irq/flags.rs        | 102 +++++++++
>>> rust/kernel/irq/request.rs      | 455 ++++++++++++++++++++++++++++++++++++++++
>>> rust/kernel/lib.rs              |   1 +
>>> 7 files changed, 593 insertions(+)
>> 
>> Could you split this patch into smaller chunks?
>
> How? This patch does one thing: it adds request_irq functionality.

You could split off the threaded irq stuff and the flags module.

Smaller patches are much much easier to review IMO.

>>> +pub use request::Handler;
>>> +pub use request::IrqReturn;
>>> +pub use request::Registration;
>>> +pub use request::ThreadedHandler;
>>> +pub use request::ThreadedIrqReturn;
>>> +pub use request::ThreadedRegistration;
>> 
>> Why not?:
>> 
>>    pub use request::{Handler, ..., ThreadedRegistration};
>
> I dislike this notation. It is just a magnet for conflicts.

Rust-analyzer has the "normalize imports" code action that can help with
that. In one of our meetings, we once discussed having a custom tool
that could merge imports, but nobody took the time to build it.

I don't see the argument here, as this re-export shouldn't really
change.

>>> 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);
>> 
>> The constants below seem to all be 32 bit, why did you choose u64?
>
> The C code takes in ffi::c_ulong. Shouldn’t this map to u64? Or maybe usize.

Maybe bindgen is doing some funky stuff, Gary changed the mappings a
couple months ago (from rust/ffi.rs):

    c_long = isize;
    c_ulong = usize;

So this indeed should be usize, what is going on here?

>>> +/// A registration of an IRQ handler for a given IRQ line.
>>> +///
>>> +/// # Examples
>>> +///
>>> +/// The following is an example of using `Registration`. It uses a
>>> +/// [`SpinLock`](crate::sync::SpinLockIrq) to provide the interior mutability.
>>> +/// Note that Spinlocks are not safe to use in IRQ context as of now, but may be
>>> +/// in the future.
>> 
>> Didn't your commit message mention SpinLockIrq?
>
> This is not upstream yet. I removed all mentions of SpinLockIrq on
> purpose, but I apparently missed some as you say.
>
> We definitely need interrupt-aware spinlocks to access the data in interrupt
> context. It is just a matter of deciding whether we will be referring to a type
> whose API is not 100% formalized as we speak, or to SpinLock itself - which is
> already upstream - with a caveat. I chose the latter.

I don't think we should knowingly do something that is "not safe". If
this requires `SpinLockIrq`, then that should land first. I think
referring to a not-yet-existing type is fine.

>>> +/// # Invariants
>>> +///
>>> +/// * We own an irq handler using `&self` as its private data.
>>> +///
>>> +#[pin_data(PinnedDrop)]
>>> +pub struct Registration<T: Handler> {
>>> +    irq: u32,
>>> +    #[pin]
>>> +    handler: T,
>>> +    #[pin]
>>> +    /// Pinned because we need address stability so that we can pass a pointer
>>> +    /// to the callback.
>>> +    _pin: PhantomPinned,
>>> +}
>>> +
>>> +impl<T: Handler> Registration<T> {
>>> +    /// Registers the IRQ handler with the system for the given IRQ number. The
>>> +    /// handler must be able to be called as soon as this function returns.
>> 
>> The first line of documentation should be a single sentence description
>> of what the item does. It will get rendered next to it on the summary &
>> search pages.
>
> Right, I actually rendered the docs before submitting, but apparently I missed this.
>
>> 
>> What is meant by the second sentence? What about this phrasing?: "The
>> handler might be called immediately after this function returns.".
>
> It's a reminder, inherited from the C side, that literally as soon as
> request_irq() returns, your handler may be called. Anything that you need to
> setup before your handler can run must already have been done by this point.
>
> Is this the “second sentence” you are referring to?

Yes. The sentence was worded in a weird way, since in Rust, I would
normally expect that anything can happen to a value, if I give up
ownership.

Also it is not true, since the code in the function body is only run
once you use the initializer, so running this code doesn't do anything:

    let handler = ...;
    let flags = ...;
    let irg = ...;
    

    let reg = Registration::register(irq, flags, cstr!("foo"), handler);

    // the registration is not yet created, only an initializer for a registration.

    let reg = Arc::pin_init(reg);

    // now the registration has been created and the handler can be called.

>>> +    pub fn register(
>>> +        irq: u32,
>>> +        flags: Flags,
>>> +        name: &'static CStr,
>>> +        handler: T,
>>> +    ) -> impl PinInit<Self, Error> {

>>> +/// The value that can be returned from `ThreadedHandler::handle_irq`.
>>> +#[repr(u32)]
>>> +pub enum ThreadedIrqReturn {
>>> +    /// The interrupt was not from this device or was not handled.
>>> +    None = bindings::irqreturn_IRQ_NONE,
>>> +
>>> +    /// The interrupt was handled by this device.
>>> +    Handled = bindings::irqreturn_IRQ_HANDLED,
>>> +
>>> +    /// The handler wants the handler thread to wake up.
>> 
>> How about "The handler wants the handler thread to take care of the
>> interrupt." or is that incorrect?
>
> This is taken straight from the C docs. As I told Andreas, I’d prefer if we
> didn’t add our own docs for C things that have their own docs already.

Fine with me, but we could also change the C docs :)

>>> +///     // This will run (in a separate kthread) if and only if `handle_irq`
>>> +///     // returns `WakeThread`.
>>> +///     fn thread_fn(&self) -> IrqReturn {
>>> +///         // We now have exclusive access to the data by locking the SpinLock.
>>> +///         //
>>> +///         // Ideally, we should disable interrupts while we are doing this to
>>> +///         // avoid deadlocks, but this is not currently possible.
>> 
>> Would this be solved by SpinLockIrq?
>
>
> Yes, that is the point of SpinLockIrq. The exact syntax is still up for debate,
> although by this time I think Lyude & Boqun have already settled on something.

I also was part of the discussion at some point, but dropped out & was
busy with other things. I'll move it up in my todo-list.

---
Cheers,
Benno

Re: [PATCH v3 1/2] rust: irq: add support for request_irq()

[Daniel Almeida]

Hi Danilo,

> On 14 May 2025, at 18:53, Danilo Krummrich <dakr@kernel.org> wrote:
> 
> On Wed, May 14, 2025 at 04:20:51PM -0300, Daniel Almeida wrote:
>> +/// // This is running in process context.
>> +/// fn register_irq(irq: u32, handler: Handler) -> Result<Arc<Registration<Handler>>> {
>> +///     let registration = Registration::register(irq, 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)?;
> 
> This makes it possible to arbitrarily extend the lifetime of an IRQ
> registration. However, we must guarantee that the IRQ is unregistered when the
> corresponding device is unbound. We can't allow drivers to hold on to device
> resources after the corresponding device has been unbound.
> 
> Why does the data need to be part of the IRQ registration itself? Why can't we
> pass in an Arc<T> instance already when we register the IRQ?
> 
> This way we'd never have a reason to ever access the Registration instance
> itself ever again and we can easily wrap it as Devres<irq::Registration> -
> analogously to devm_request_irq() on the C side - without any penalties.
> 
>> +///     // 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.
>> +///         let mut data = registration.handler().0.lock();
>> +///         *data = 42;
>> +///     }
>> +///
>> +///     Ok(registration)
>> +/// }
> 

Up until this point, there was no need for the data to not be inline with the
registration. This new design would force an Arc, which, apart from the
heap-allocation, is restrictive for users.

Can’t we use Devres with the current implementation?

IIUC from a very cursory glance, all that would mean is that you'd have to call
try_access() on your handler, which should be fine?

— Daniel

Re: [PATCH v3 1/2] rust: irq: add support for request_irq()

[Danilo Krummrich]

On Thu, May 15, 2025 at 08:54:35AM -0300, Daniel Almeida wrote:
> Hi Danilo,
> 
> > On 14 May 2025, at 18:53, Danilo Krummrich <dakr@kernel.org> wrote:
> > 
> > On Wed, May 14, 2025 at 04:20:51PM -0300, Daniel Almeida wrote:
> >> +/// // This is running in process context.
> >> +/// fn register_irq(irq: u32, handler: Handler) -> Result<Arc<Registration<Handler>>> {
> >> +///     let registration = Registration::register(irq, 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)?;
> > 
> > This makes it possible to arbitrarily extend the lifetime of an IRQ
> > registration. However, we must guarantee that the IRQ is unregistered when the
> > corresponding device is unbound. We can't allow drivers to hold on to device
> > resources after the corresponding device has been unbound.
> > 
> > Why does the data need to be part of the IRQ registration itself? Why can't we
> > pass in an Arc<T> instance already when we register the IRQ?
> > 
> > This way we'd never have a reason to ever access the Registration instance
> > itself ever again and we can easily wrap it as Devres<irq::Registration> -
> > analogously to devm_request_irq() on the C side - without any penalties.
> > 
> >> +///     // 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.
> >> +///         let mut data = registration.handler().0.lock();
> >> +///         *data = 42;
> >> +///     }
> >> +///
> >> +///     Ok(registration)
> >> +/// }
> > 
> 
> Up until this point, there was no need for the data to not be inline with the
> registration. This new design would force an Arc, which, apart from the
> heap-allocation, is restrictive for users.

Does the current design not also imply a heap allocation heap allocation? With
my proposal irq::Registration::new() can just return an irq::Registration
instance, not an impl PinInit that you need to stuff into a Box or Arc instead.
Hence, there shouldn't be a difference.

> Can’t we use Devres with the current implementation?
> 
> IIUC from a very cursory glance, all that would mean is that you'd have to call
> try_access() on your handler, which should be fine?

Well, that would work indeed.

But people will - with good reason - be upset that every access to the handler's
data needs to be guarded with the RCU read side critical section implied by
Revocable and hence Devres.

We can easily avoid that in this case, hence we should do it.

Re: [PATCH v3 1/2] rust: irq: add support for request_irq()

[Daniel Almeida]

Hi Benno,

> On 15 May 2025, at 05:46, Benno Lossin <lossin@kernel.org> wrote:
> 
> On Wed May 14, 2025 at 10:58 PM CEST, Daniel Almeida wrote:
>> Hi Benno, I really appreciate you taking the time to review my patches.
> 
> Glad I can help :)
> 
>>> On 14 May 2025, at 17:04, Benno Lossin <lossin@kernel.org> wrote:
>>> On Wed May 14, 2025 at 9:20 PM CEST, Daniel Almeida wrote:
>>>> Add support for registering IRQ handlers in Rust.
>>>> 
>>>> IRQ handlers are extensively used in drivers when some peripheral wants
>>>> to obtain the CPU attention. Registering a handler will make the system
>>>> invoke the passed-in function whenever the chosen IRQ line is triggered.
>>>> 
>>>> Both regular and threaded IRQ handlers are supported through a Handler
>>>> (or ThreadedHandler) trait that is meant to be implemented by a type
>>>> that:
>>>> 
>>>> a) provides a function to be run by the system when the IRQ fires and,
>>>> 
>>>> b) holds the shared data (i.e.: `T`) between process and IRQ contexts.
>>>> 
>>>> The requirement that T is Sync derives from the fact that handlers might
>>>> run concurrently with other processes executing the same driver,
>>>> creating the potential for data races.
>>>> 
>>>> Ideally, some interior mutability must be in place if T is to be
>>>> mutated. This should usually be done through the in-flight SpinLockIrq
>>>> type.
>>>> 
>>>> Co-developed-by: Alice Ryhl <aliceryhl@google.com>
>>>> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
>>>> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
>>>> ---
>>>> rust/bindings/bindings_helper.h |   1 +
>>>> rust/helpers/helpers.c          |   1 +
>>>> rust/helpers/irq.c              |   9 +
>>>> rust/kernel/irq.rs              |  24 +++
>>>> rust/kernel/irq/flags.rs        | 102 +++++++++
>>>> rust/kernel/irq/request.rs      | 455 ++++++++++++++++++++++++++++++++++++++++
>>>> rust/kernel/lib.rs              |   1 +
>>>> 7 files changed, 593 insertions(+)
>>> 
>>> Could you split this patch into smaller chunks?
>> 
>> How? This patch does one thing: it adds request_irq functionality.
> 
> You could split off the threaded irq stuff and the flags module.
> 
> Smaller patches are much much easier to review IMO.

The flags are needed for non-threaded IRQs too.

I think this can probably be split into:

"Add IRQ module"
"Add IRQ flags" <--- in preparation for next patch
"Add non-threaded IRQs"
"Add threaded IRQs”

WDYT?

> 
>>>> +pub use request::Handler;
>>>> +pub use request::IrqReturn;
>>>> +pub use request::Registration;
>>>> +pub use request::ThreadedHandler;
>>>> +pub use request::ThreadedIrqReturn;
>>>> +pub use request::ThreadedRegistration;
>>> 
>>> Why not?:
>>> 
>>>   pub use request::{Handler, ..., ThreadedRegistration};
>> 
>> I dislike this notation. It is just a magnet for conflicts.
> 
> Rust-analyzer has the "normalize imports" code action that can help with
> that. In one of our meetings, we once discussed having a custom tool
> that could merge imports, but nobody took the time to build it.
> 
> I don't see the argument here, as this re-export shouldn't really
> change.

Yeah, in this instance that is true. 


> 
>>>> 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);
>>> 
>>> The constants below seem to all be 32 bit, why did you choose u64?
>> 
>> The C code takes in ffi::c_ulong. Shouldn’t this map to u64? Or maybe usize.
> 
> Maybe bindgen is doing some funky stuff, Gary changed the mappings a
> couple months ago (from rust/ffi.rs):
> 
>    c_long = isize;
>    c_ulong = usize;
> 
> So this indeed should be usize, what is going on here?

I think this is working as intended. The C bindgen function does take usize, so
the u64 can go away. I guess this confusion started because the individual
flags are u32 though, so at least a conversion from u32 to usize will be
needed.

> 
>>>> +/// A registration of an IRQ handler for a given IRQ line.
>>>> +///
>>>> +/// # Examples
>>>> +///
>>>> +/// The following is an example of using `Registration`. It uses a
>>>> +/// [`SpinLock`](crate::sync::SpinLockIrq) to provide the interior mutability.
>>>> +/// Note that Spinlocks are not safe to use in IRQ context as of now, but may be
>>>> +/// in the future.
>>> 
>>> Didn't your commit message mention SpinLockIrq?
>> 
>> This is not upstream yet. I removed all mentions of SpinLockIrq on
>> purpose, but I apparently missed some as you say.
>> 
>> We definitely need interrupt-aware spinlocks to access the data in interrupt
>> context. It is just a matter of deciding whether we will be referring to a type
>> whose API is not 100% formalized as we speak, or to SpinLock itself - which is
>> already upstream - with a caveat. I chose the latter.
> 
> I don't think we should knowingly do something that is "not safe". If
> this requires `SpinLockIrq`, then that should land first. I think
> referring to a not-yet-existing type is fine.

Well, SpinLockIrq has been brewing for quite a while. Waiting for it to land
can introduce an unbounded delay here. Note that IRQ handling is extremely
important for drivers.

What we can do is to simply remove the locks from all the examples. The code
will still work fine, you just won't be able to mutate the data without the
interior mutability, of course.

A subsequent patch can (re)introduce the examples where the data is mutated
when SpinLockIrq lands. WDYT?

> 
>>>> +/// # Invariants
>>>> +///
>>>> +/// * We own an irq handler using `&self` as its private data.
>>>> +///
>>>> +#[pin_data(PinnedDrop)]
>>>> +pub struct Registration<T: Handler> {
>>>> +    irq: u32,
>>>> +    #[pin]
>>>> +    handler: T,
>>>> +    #[pin]
>>>> +    /// Pinned because we need address stability so that we can pass a pointer
>>>> +    /// to the callback.
>>>> +    _pin: PhantomPinned,
>>>> +}
>>>> +
>>>> +impl<T: Handler> Registration<T> {
>>>> +    /// Registers the IRQ handler with the system for the given IRQ number. The
>>>> +    /// handler must be able to be called as soon as this function returns.
>>> 
>>> The first line of documentation should be a single sentence description
>>> of what the item does. It will get rendered next to it on the summary &
>>> search pages.
>> 
>> Right, I actually rendered the docs before submitting, but apparently I missed this.
>> 
>>> 
>>> What is meant by the second sentence? What about this phrasing?: "The
>>> handler might be called immediately after this function returns.".
>> 
>> It's a reminder, inherited from the C side, that literally as soon as
>> request_irq() returns, your handler may be called. Anything that you need to
>> setup before your handler can run must already have been done by this point.
>> 
>> Is this the “second sentence” you are referring to?
> 
> Yes. The sentence was worded in a weird way, since in Rust, I would
> normally expect that anything can happen to a value, if I give up
> ownership.
> 
> Also it is not true, since the code in the function body is only run
> once you use the initializer, so running this code doesn't do anything:
> 
>    let handler = ...;
>    let flags = ...;
>    let irg = ...;
> 
> 
>    let reg = Registration::register(irq, flags, cstr!("foo"), handler);
> 
>    // the registration is not yet created, only an initializer for a registration.
> 
>    let reg = Arc::pin_init(reg);
> 
>    // now the registration has been created and the handler can be called.

Right, this is true.

> 
>>>> +    pub fn register(
>>>> +        irq: u32,
>>>> +        flags: Flags,
>>>> +        name: &'static CStr,
>>>> +        handler: T,
>>>> +    ) -> impl PinInit<Self, Error> {
> 
>>>> +/// The value that can be returned from `ThreadedHandler::handle_irq`.
>>>> +#[repr(u32)]
>>>> +pub enum ThreadedIrqReturn {
>>>> +    /// The interrupt was not from this device or was not handled.
>>>> +    None = bindings::irqreturn_IRQ_NONE,
>>>> +
>>>> +    /// The interrupt was handled by this device.
>>>> +    Handled = bindings::irqreturn_IRQ_HANDLED,
>>>> +
>>>> +    /// The handler wants the handler thread to wake up.
>>> 
>>> How about "The handler wants the handler thread to take care of the
>>> interrupt." or is that incorrect?
>> 
>> This is taken straight from the C docs. As I told Andreas, I’d prefer if we
>> didn’t add our own docs for C things that have their own docs already.
> 
> Fine with me, but we could also change the C docs :)
> 
>>>> +///     // This will run (in a separate kthread) if and only if `handle_irq`
>>>> +///     // returns `WakeThread`.
>>>> +///     fn thread_fn(&self) -> IrqReturn {
>>>> +///         // We now have exclusive access to the data by locking the SpinLock.
>>>> +///         //
>>>> +///         // Ideally, we should disable interrupts while we are doing this to
>>>> +///         // avoid deadlocks, but this is not currently possible.
>>> 
>>> Would this be solved by SpinLockIrq?
>> 
>> 
>> Yes, that is the point of SpinLockIrq. The exact syntax is still up for debate,
>> although by this time I think Lyude & Boqun have already settled on something.
> 
> I also was part of the discussion at some point, but dropped out & was
> busy with other things. I'll move it up in my todo-list.

Ah yes, that would be very helpful. SpinLockIrq is another very important
missing piece in the ecosystem.

> 
> ---
> Cheers,
> Benno

Re: [PATCH v3 1/2] rust: irq: add support for request_irq()

[Daniel Almeida]

> On 15 May 2025, at 09:04, Danilo Krummrich <dakr@kernel.org> wrote:
> 
> On Thu, May 15, 2025 at 08:54:35AM -0300, Daniel Almeida wrote:
>> Hi Danilo,
>> 
>>> On 14 May 2025, at 18:53, Danilo Krummrich <dakr@kernel.org> wrote:
>>> 
>>> On Wed, May 14, 2025 at 04:20:51PM -0300, Daniel Almeida wrote:
>>>> +/// // This is running in process context.
>>>> +/// fn register_irq(irq: u32, handler: Handler) -> Result<Arc<Registration<Handler>>> {
>>>> +///     let registration = Registration::register(irq, 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)?;
>>> 
>>> This makes it possible to arbitrarily extend the lifetime of an IRQ
>>> registration. However, we must guarantee that the IRQ is unregistered when the
>>> corresponding device is unbound. We can't allow drivers to hold on to device
>>> resources after the corresponding device has been unbound.
>>> 
>>> Why does the data need to be part of the IRQ registration itself? Why can't we
>>> pass in an Arc<T> instance already when we register the IRQ?
>>> 
>>> This way we'd never have a reason to ever access the Registration instance
>>> itself ever again and we can easily wrap it as Devres<irq::Registration> -
>>> analogously to devm_request_irq() on the C side - without any penalties.
>>> 
>>>> +///     // 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.
>>>> +///         let mut data = registration.handler().0.lock();
>>>> +///         *data = 42;
>>>> +///     }
>>>> +///
>>>> +///     Ok(registration)
>>>> +/// }
>>> 
>> 
>> Up until this point, there was no need for the data to not be inline with the
>> registration. This new design would force an Arc, which, apart from the
>> heap-allocation, is restrictive for users.
> 
> Does the current design not also imply a heap allocation heap allocation? With
> my proposal irq::Registration::new() can just return an irq::Registration
> instance, not an impl PinInit that you need to stuff into a Box or Arc instead.
> Hence, there shouldn't be a difference.

Well, not really, because this impl PinInit can be assigned to something larger
that is already pinned, like drm::Device::Data for example, which is (or was)
already behind an Arc, or any other private data in other subsystems.

IIUC what you proposed has yet another indirection. If we reuse the example
from above, that would be an Arc for the drm Data, and another Arc for the
handler itself?

I definitely see your point here, I am just trying to brainstorm another way of
doing this.

> 
>> Can’t we use Devres with the current implementation?
>> 
>> IIUC from a very cursory glance, all that would mean is that you'd have to call
>> try_access() on your handler, which should be fine?
> 
> Well, that would work indeed.
> 
> But people will - with good reason - be upset that every access to the handler's
> data needs to be guarded with the RCU read side critical section implied by
> Revocable and hence Devres.

True, I totally missed that.

> 
> We can easily avoid that in this case, hence we should do it.

— Daniel

Re: [PATCH v3 1/2] rust: irq: add support for request_irq()

[Benno Lossin]

On Thu May 15, 2025 at 2:06 PM CEST, Daniel Almeida wrote:
>>>>> rust/bindings/bindings_helper.h |   1 +
>>>>> rust/helpers/helpers.c          |   1 +
>>>>> rust/helpers/irq.c              |   9 +
>>>>> rust/kernel/irq.rs              |  24 +++
>>>>> rust/kernel/irq/flags.rs        | 102 +++++++++
>>>>> rust/kernel/irq/request.rs      | 455 ++++++++++++++++++++++++++++++++++++++++
>>>>> rust/kernel/lib.rs              |   1 +
>>>>> 7 files changed, 593 insertions(+)
>>>> 
>>>> Could you split this patch into smaller chunks?
>>> 
>>> How? This patch does one thing: it adds request_irq functionality.
>> 
>> You could split off the threaded irq stuff and the flags module.
>> 
>> Smaller patches are much much easier to review IMO.
>
> The flags are needed for non-threaded IRQs too.

Oh yeah, I didn't suggest an order, but rather a set of things :)

> I think this can probably be split into:
>
> "Add IRQ module"
> "Add IRQ flags" <--- in preparation for next patch
> "Add non-threaded IRQs"
> "Add threaded IRQs”
>
> WDYT?

Yeah that looks good.

>>>>> 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);
>>>> 
>>>> The constants below seem to all be 32 bit, why did you choose u64?
>>> 
>>> The C code takes in ffi::c_ulong. Shouldn’t this map to u64? Or maybe usize.
>> 
>> Maybe bindgen is doing some funky stuff, Gary changed the mappings a
>> couple months ago (from rust/ffi.rs):
>> 
>>    c_long = isize;
>>    c_ulong = usize;
>> 
>> So this indeed should be usize, what is going on here?
>
> I think this is working as intended. The C bindgen function does take usize, so
> the u64 can go away. I guess this confusion started because the individual
> flags are u32 though, so at least a conversion from u32 to usize will be
> needed.

Ah we were talking about two different things. The functions take
`c_ulong`, but the definitions are pre-processor macros and thus don't
have a type annotation. That's why bindgen uses `u32`, because that's
the type that the constant fits in (AFAIK there are also some weird
rules in C about this? but don't quote me on that).

One way would be to turn the `#define`s into constants, but from
previous discussions I remember there were some reasons to not do that,
so I have no idea. The casts are unfortunate, but if we can't change the
C side to include the types in these constants, then it's fine.

>>>>> +/// A registration of an IRQ handler for a given IRQ line.
>>>>> +///
>>>>> +/// # Examples
>>>>> +///
>>>>> +/// The following is an example of using `Registration`. It uses a
>>>>> +/// [`SpinLock`](crate::sync::SpinLockIrq) to provide the interior mutability.
>>>>> +/// Note that Spinlocks are not safe to use in IRQ context as of now, but may be
>>>>> +/// in the future.
>>>> 
>>>> Didn't your commit message mention SpinLockIrq?
>>> 
>>> This is not upstream yet. I removed all mentions of SpinLockIrq on
>>> purpose, but I apparently missed some as you say.
>>> 
>>> We definitely need interrupt-aware spinlocks to access the data in interrupt
>>> context. It is just a matter of deciding whether we will be referring to a type
>>> whose API is not 100% formalized as we speak, or to SpinLock itself - which is
>>> already upstream - with a caveat. I chose the latter.
>> 
>> I don't think we should knowingly do something that is "not safe". If
>> this requires `SpinLockIrq`, then that should land first. I think
>> referring to a not-yet-existing type is fine.
>
> Well, SpinLockIrq has been brewing for quite a while. Waiting for it to land
> can introduce an unbounded delay here. Note that IRQ handling is extremely
> important for drivers.

But is it useful even without `SpinLockIrq`?

> What we can do is to simply remove the locks from all the examples. The code
> will still work fine, you just won't be able to mutate the data without the
> interior mutability, of course.

Do atomics work normally in IRQ context? (I know that Boqun's atomic
series also needs some work, so maybe not a good alternative)

> A subsequent patch can (re)introduce the examples where the data is mutated
> when SpinLockIrq lands. WDYT?

I think that is better than including wrong examples with a caveat, as
some people might not read it or assume that the restriction somehow
doesn't apply to them.

---
Cheers,
Benno

Re: [PATCH v3 1/2] rust: irq: add support for request_irq()

[Danilo Krummrich]

On Thu, May 15, 2025 at 09:27:51AM -0300, Daniel Almeida wrote:
> 
> 
> > On 15 May 2025, at 09:04, Danilo Krummrich <dakr@kernel.org> wrote:
> > 
> > On Thu, May 15, 2025 at 08:54:35AM -0300, Daniel Almeida wrote:
> >> Hi Danilo,
> >> 
> >>> On 14 May 2025, at 18:53, Danilo Krummrich <dakr@kernel.org> wrote:
> >>> 
> >>> On Wed, May 14, 2025 at 04:20:51PM -0300, Daniel Almeida wrote:
> >>>> +/// // This is running in process context.
> >>>> +/// fn register_irq(irq: u32, handler: Handler) -> Result<Arc<Registration<Handler>>> {
> >>>> +///     let registration = Registration::register(irq, 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)?;
> >>> 
> >>> This makes it possible to arbitrarily extend the lifetime of an IRQ
> >>> registration. However, we must guarantee that the IRQ is unregistered when the
> >>> corresponding device is unbound. We can't allow drivers to hold on to device
> >>> resources after the corresponding device has been unbound.
> >>> 
> >>> Why does the data need to be part of the IRQ registration itself? Why can't we
> >>> pass in an Arc<T> instance already when we register the IRQ?
> >>> 
> >>> This way we'd never have a reason to ever access the Registration instance
> >>> itself ever again and we can easily wrap it as Devres<irq::Registration> -
> >>> analogously to devm_request_irq() on the C side - without any penalties.
> >>> 
> >>>> +///     // 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.
> >>>> +///         let mut data = registration.handler().0.lock();
> >>>> +///         *data = 42;
> >>>> +///     }
> >>>> +///
> >>>> +///     Ok(registration)
> >>>> +/// }
> >>> 
> >> 
> >> Up until this point, there was no need for the data to not be inline with the
> >> registration. This new design would force an Arc, which, apart from the
> >> heap-allocation, is restrictive for users.
> > 
> > Does the current design not also imply a heap allocation heap allocation? With
> > my proposal irq::Registration::new() can just return an irq::Registration
> > instance, not an impl PinInit that you need to stuff into a Box or Arc instead.
> > Hence, there shouldn't be a difference.
> 
> Well, not really, because this impl PinInit can be assigned to something larger
> that is already pinned, like drm::Device::Data for example, which is (or was)
> already behind an Arc, or any other private data in other subsystems.
> 
> IIUC what you proposed has yet another indirection. If we reuse the example
> from above, that would be an Arc for the drm Data, and another Arc for the
> handler itself?

Can't you implement Handler for drm::Device::Data and e.g. make Registration
take an Arc<T: Handler>?

The irq::Registration itself doesn't need to be allocated dynamically, so it'd
still be a single allocation, no?

> I definitely see your point here, I am just trying to brainstorm another way of
> doing this.
> > 
> >> Can’t we use Devres with the current implementation?
> >> 
> >> IIUC from a very cursory glance, all that would mean is that you'd have to call
> >> try_access() on your handler, which should be fine?
> > 
> > Well, that would work indeed.
> > 
> > But people will - with good reason - be upset that every access to the handler's
> > data needs to be guarded with the RCU read side critical section implied by
> > Revocable and hence Devres.
> 
> True, I totally missed that.
> 
> > 
> > We can easily avoid that in this case, hence we should do it.
> 
> — Daniel
> 

Re: [PATCH v3 1/2] rust: irq: add support for request_irq()

[Daniel Almeida]

>> 
>> Well, not really, because this impl PinInit can be assigned to something larger
>> that is already pinned, like drm::Device::Data for example, which is (or was)
>> already behind an Arc, or any other private data in other subsystems.
>> 
>> IIUC what you proposed has yet another indirection. If we reuse the example
>> from above, that would be an Arc for the drm Data, and another Arc for the
>> handler itself?
> 
> Can't you implement Handler for drm::Device::Data and e.g. make Registration
> take an Arc<T: Handler>?

No, because drivers may need more than one handler. Panthor needs 3, for
example, for 3 different lines.

> 
> The irq::Registration itself doesn't need to be allocated dynamically, so it'd
> still be a single allocation, no?
> 

Right, the registrations don't, but the handlers do.

— Daniel

Re: [PATCH v3 1/2] rust: irq: add support for request_irq()

[Benno Lossin]

On Thu May 15, 2025 at 2:45 PM CEST, Danilo Krummrich wrote:
> On Thu, May 15, 2025 at 09:27:51AM -0300, Daniel Almeida wrote:
>> Well, not really, because this impl PinInit can be assigned to something larger
>> that is already pinned, like drm::Device::Data for example, which is (or was)
>> already behind an Arc, or any other private data in other subsystems.
>> 
>> IIUC what you proposed has yet another indirection. If we reuse the example
>> from above, that would be an Arc for the drm Data, and another Arc for the
>> handler itself?
>
> Can't you implement Handler for drm::Device::Data and e.g. make Registration
> take an Arc<T: Handler>?

Couldn't you store the registration inside of the `drm::Device::Data`?
Or is that too early in the device lifecycle?

---
Cheers,
Benno

Re: [PATCH v3 1/2] rust: irq: add support for request_irq()

[Danilo Krummrich]

On Thu, May 15, 2025 at 10:16:33AM -0300, Daniel Almeida wrote:
> 
> >> 
> >> Well, not really, because this impl PinInit can be assigned to something larger
> >> that is already pinned, like drm::Device::Data for example, which is (or was)
> >> already behind an Arc, or any other private data in other subsystems.
> >> 
> >> IIUC what you proposed has yet another indirection. If we reuse the example
> >> from above, that would be an Arc for the drm Data, and another Arc for the
> >> handler itself?
> > 
> > Can't you implement Handler for drm::Device::Data and e.g. make Registration
> > take an Arc<T: Handler>?
> 
> No, because drivers may need more than one handler. Panthor needs 3, for
> example, for 3 different lines.
> 
> > 
> > The irq::Registration itself doesn't need to be allocated dynamically, so it'd
> > still be a single allocation, no?
> > 
> 
> Right, the registrations don't, but the handlers do.

Why does the handler need to be allocated dynamically?

What about something like the following?

	pub struct Registration<T, H: Handler<T>> { ... };

	pub trait Handler<T> {
	   fn handle_irq(&T) -> IrqReturn;
	}

	// Could be `drm::Device::Data`.
	struct MyData { ... };

	// Implements `Handler<MyData>`.
	struct IRQHandler1;
	struct IRQHandler2;

	// `data` is `Arc<MyData>`
	irq::Registration::<IRQHandler1>::new(data, ...);
	irq::Registration::<IRQHandler2>::new(data, ...);

With that you can have as many IRQs as you want without any additional
allocation.

Re: [PATCH v3 1/2] rust: irq: add support for request_irq()

[Danilo Krummrich]

On Thu, May 15, 2025 at 03:45:28PM +0200, Danilo Krummrich wrote:
> On Thu, May 15, 2025 at 10:16:33AM -0300, Daniel Almeida wrote:
> > 
> > >> 
> > >> Well, not really, because this impl PinInit can be assigned to something larger
> > >> that is already pinned, like drm::Device::Data for example, which is (or was)
> > >> already behind an Arc, or any other private data in other subsystems.
> > >> 
> > >> IIUC what you proposed has yet another indirection. If we reuse the example
> > >> from above, that would be an Arc for the drm Data, and another Arc for the
> > >> handler itself?
> > > 
> > > Can't you implement Handler for drm::Device::Data and e.g. make Registration
> > > take an Arc<T: Handler>?
> > 
> > No, because drivers may need more than one handler. Panthor needs 3, for
> > example, for 3 different lines.
> > 
> > > 
> > > The irq::Registration itself doesn't need to be allocated dynamically, so it'd
> > > still be a single allocation, no?
> > > 
> > 
> > Right, the registrations don't, but the handlers do.
> 
> Why does the handler need to be allocated dynamically?
> 
> What about something like the following?
> 
> 	pub struct Registration<T, H: Handler<T>> { ... };
> 
> 	pub trait Handler<T> {
> 	   fn handle_irq(&T) -> IrqReturn;
> 	}
> 
> 	// Could be `drm::Device::Data`.
> 	struct MyData { ... };
> 
> 	// Implements `Handler<MyData>`.
> 	struct IRQHandler1;
> 	struct IRQHandler2;
> 
> 	// `data` is `Arc<MyData>`
> 	irq::Registration::<IRQHandler1>::new(data, ...);
> 	irq::Registration::<IRQHandler2>::new(data, ...);
> 
> With that you can have as many IRQs as you want without any additional
> allocation.

Alternatively we could also do the following, which is probably simpler.

	pub struct Registration<H: Handler> { ... };

	pub trait Handler {
	   fn handle_irq(&self) -> IrqReturn;
	}

	// Could be `drm::Device::Data`.
	struct MyData { ... };

	// Implements `Handler`.
	struct IRQHandler1(Arc<MyData>);
	struct IRQHandler2(Arc<MyData>);

	// `data` is `Arc<MyData>`
	let handler1 = IRQHandler1::new(data);
	let handler2 = IRQHandler2::new(data);

	irq::Registration::new(handler1, ...);
	irq::Registration::new(handler2, ...);

Re: [PATCH v3 1/2] rust: irq: add support for request_irq()

[Alexandre Courbot]

Hi Daniel,

On Thu May 15, 2025 at 4:20 AM JST, Daniel Almeida wrote:
<snip>
> +/// 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)
> +    }
> +}

I see that every smart pointer would have to implement Handler in order
to be used with this module, but...

> +#[pin_data(PinnedDrop)]
> +pub struct Registration<T: Handler> {
> +    irq: u32,
> +    #[pin]
> +    handler: T,

... what if you store another type `U: Borrow<T>` here, and take it as
the argument of `register`? This way you should be able to use anything
that implements Borrow<T>, which includes T itself, Arc<T>, Box<T>, and
more, and can remove the two impl blocks above.

Re: [PATCH v3 1/2] rust: irq: add support for request_irq()

[Benno Lossin]

On Sun May 18, 2025 at 3:24 PM CEST, Alexandre Courbot wrote:
> Hi Daniel,
>
> On Thu May 15, 2025 at 4:20 AM JST, Daniel Almeida wrote:
> <snip>
>> +/// 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)
>> +    }
>> +}
>
> I see that every smart pointer would have to implement Handler in order
> to be used with this module, but...
>
>> +#[pin_data(PinnedDrop)]
>> +pub struct Registration<T: Handler> {
>> +    irq: u32,
>> +    #[pin]
>> +    handler: T,
>
> ... what if you store another type `U: Borrow<T>` here, and take it as
> the argument of `register`? This way you should be able to use anything
> that implements Borrow<T>, which includes T itself, Arc<T>, Box<T>, and
> more, and can remove the two impl blocks above.

I don't think that this is easily possible, since then the
`Registration` struct will have two generics, which I think is worse.

The impls above are pretty common for these kinds of traits, so I don't
think it's too bad.

---
Cheers,
Benno

Re: [PATCH v3 2/2] rust: platform: add irq accessors

[Danilo Krummrich]

On Wed, May 14, 2025 at 04:20:52PM -0300, Daniel Almeida wrote:
> These accessors can be used to retrieve an IRQ from a platform device by
> index or name. The IRQ can then be used to request the line using
> irq::request::Registration::register() and
> irq::request::ThreadedRegistration::register().
> 
> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
> ---
>  rust/kernel/platform.rs | 52 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 52 insertions(+)
> 
> diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs
> index 4917cb34e2fe8027d3d861e90de51de85f006735..1acaebf38d99d06f93fa13b0b356671ea77ed97a 100644
> --- a/rust/kernel/platform.rs
> +++ b/rust/kernel/platform.rs
> @@ -188,6 +188,58 @@ impl Device {
>      fn as_raw(&self) -> *mut bindings::platform_device {
>          self.0.get()
>      }
> +
> +    /// Returns an IRQ for the device by index.
> +    pub fn irq_by_index(&self, index: u32) -> Result<u32> {
> +        // SAFETY: `self.as_raw` returns a valid pointer to a `struct platform_device`.
> +        let res = unsafe { bindings::platform_get_irq(self.as_raw(), index) };
> +
> +        if res < 0 {
> +            return Err(Error::from_errno(res));
> +        }
> +
> +        Ok(res as u32)
> +    }
> +
> +    /// Same as [`Self::irq_by_index`] but does not print an error message if an IRQ
> +    /// cannot be obtained.
> +    pub fn optional_irq_by_index(&self, index: u32) -> Result<u32> {
> +        // SAFETY: `self.as_raw` returns a valid pointer to a `struct platform_device`.
> +        let res = unsafe { bindings::platform_get_irq_optional(self.as_raw(), index) };
> +
> +        if res < 0 {
> +            return Err(Error::from_errno(res));
> +        }
> +
> +        Ok(res as u32)
> +    }
> +
> +    /// Returns an IRQ for the device by name.
> +    pub fn irq_by_name(&self, name: &CStr) -> Result<u32> {
> +        // SAFETY: `self.as_raw` returns a valid pointer to a `struct platform_device`.
> +        let res = unsafe { bindings::platform_get_irq_byname(self.as_raw(), name.as_char_ptr()) };
> +
> +        if res < 0 {
> +            return Err(Error::from_errno(res));
> +        }
> +
> +        Ok(res as u32)
> +    }
> +
> +    /// Same as [`Self::irq_by_name`] but does not print an error message if an IRQ
> +    /// cannot be obtained.
> +    pub fn optional_irq_by_name(&self, name: &CStr) -> Result<u32> {
> +        // SAFETY: `self.as_raw` returns a valid pointer to a `struct platform_device`.
> +        let res = unsafe {
> +            bindings::platform_get_irq_byname_optional(self.as_raw(), name.as_char_ptr())
> +        };
> +
> +        if res < 0 {
> +            return Err(Error::from_errno(res));
> +        }
> +
> +        Ok(res as u32)
> +    }

I don't like the indirection of claiming a u32 representing the IRQ number from
a bus device and stuffing it into an irq::Registration.

It would be better we we'd make it impossible (or at least harder) for a driver
to pass the wrong number to irq::Registration.

I see two options:

  1) Make the platform::Device accessors themselves return an
     irq::Registration.

  2) Make the platform::Device accessors return some kind of transparent cookie,
     that drivers can't create themselves that can be fed into the
     irq::Registration.

My preference would be 1) if there's no major ergonomic issue with that.

Re: [PATCH v3 1/2] rust: irq: add support for request_irq()

[Daniel Almeida]

Danilo,

> On 15 May 2025, at 10:52, Danilo Krummrich <dakr@kernel.org> wrote:
> 
> On Thu, May 15, 2025 at 03:45:28PM +0200, Danilo Krummrich wrote:
>> On Thu, May 15, 2025 at 10:16:33AM -0300, Daniel Almeida wrote:
>>> 
>>>>> 
>>>>> Well, not really, because this impl PinInit can be assigned to something larger
>>>>> that is already pinned, like drm::Device::Data for example, which is (or was)
>>>>> already behind an Arc, or any other private data in other subsystems.
>>>>> 
>>>>> IIUC what you proposed has yet another indirection. If we reuse the example
>>>>> from above, that would be an Arc for the drm Data, and another Arc for the
>>>>> handler itself?
>>>> 
>>>> Can't you implement Handler for drm::Device::Data and e.g. make Registration
>>>> take an Arc<T: Handler>?
>>> 
>>> No, because drivers may need more than one handler. Panthor needs 3, for
>>> example, for 3 different lines.
>>> 
>>>> 
>>>> The irq::Registration itself doesn't need to be allocated dynamically, so it'd
>>>> still be a single allocation, no?
>>>> 
>>> 
>>> Right, the registrations don't, but the handlers do.
>> 
>> Why does the handler need to be allocated dynamically?
>> 
>> What about something like the following?
>> 
>> pub struct Registration<T, H: Handler<T>> { ... };
>> 
>> pub trait Handler<T> {
>>   fn handle_irq(&T) -> IrqReturn;
>> }
>> 
>> // Could be `drm::Device::Data`.
>> struct MyData { ... };
>> 
>> // Implements `Handler<MyData>`.
>> struct IRQHandler1;
>> struct IRQHandler2;
>> 
>> // `data` is `Arc<MyData>`
>> irq::Registration::<IRQHandler1>::new(data, ...);
>> irq::Registration::<IRQHandler2>::new(data, ...);
>> 
>> With that you can have as many IRQs as you want without any additional
>> allocation.
> 
> Alternatively we could also do the following, which is probably simpler.
> 
> pub struct Registration<H: Handler> { ... };
> 
> pub trait Handler {
>   fn handle_irq(&self) -> IrqReturn;
> }
> 
> // Could be `drm::Device::Data`.
> struct MyData { ... };
> 
> // Implements `Handler`.
> struct IRQHandler1(Arc<MyData>);
> struct IRQHandler2(Arc<MyData>);
> 
> // `data` is `Arc<MyData>`
> let handler1 = IRQHandler1::new(data);
> let handler2 = IRQHandler2::new(data);
> 
> irq::Registration::new(handler1, ...);
> irq::Registration::new(handler2, ...);
> 

I am trying to implement this, but isn't this what we have already?

I guess the only difference would be removing the handler() accessor, as the
caller is now expected to figure this part out on his own, i.e.:

In your example (IIUC) that would mean accessing the Arc in IRQHandler1
and IRQHandler2 through some other clone and from the actual T:Handler in
the callback.

— Daniel

Re: [PATCH v3 2/2] rust: platform: add irq accessors

[Daniel Almeida]

Hi Danilo,

[…]

>> +
>> +    /// Same as [`Self::irq_by_name`] but does not print an error message if an IRQ
>> +    /// cannot be obtained.
>> +    pub fn optional_irq_by_name(&self, name: &CStr) -> Result<u32> {
>> +        // SAFETY: `self.as_raw` returns a valid pointer to a `struct platform_device`.
>> +        let res = unsafe {
>> +            bindings::platform_get_irq_byname_optional(self.as_raw(), name.as_char_ptr())
>> +        };
>> +
>> +        if res < 0 {
>> +            return Err(Error::from_errno(res));
>> +        }
>> +
>> +        Ok(res as u32)
>> +    }
> 
> I don't like the indirection of claiming a u32 representing the IRQ number from
> a bus device and stuffing it into an irq::Registration.
> 
> It would be better we we'd make it impossible (or at least harder) for a driver
> to pass the wrong number to irq::Registration.
> 
> I see two options:
> 
>  1) Make the platform::Device accessors themselves return an
>     irq::Registration.
> 
>  2) Make the platform::Device accessors return some kind of transparent cookie,
>     that drivers can't create themselves that can be fed into the
>     irq::Registration.
> 
> My preference would be 1) if there's no major ergonomic issue with that.

Isn’t 1 way more cluttered?

That's because the accessors would have to forward all of the arguments (i.e.:
currently 4) to register().

Going with approach 2 lets us keep the two APIs distinct, we'd only have to
take in the cookie in place of the u32, of course.

— Daniel

Re: [PATCH v3 1/2] rust: irq: add support for request_irq()

[Alice Ryhl]

On Wed, May 14, 2025 at 10:04:43PM +0200, Benno Lossin wrote:
> On Wed May 14, 2025 at 9:20 PM CEST, Daniel Almeida wrote:
> > +                )
> > +            });
> > +
> > +            if res.is_err() {
> > +                // SAFETY: We are returning an error, so we can destroy the slot.
> > +                unsafe { core::ptr::drop_in_place(addr_of_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) }
> 
> Please don't use `pin_init_from_closure`, instead do this:
> 
>     pin_init!(Self {
>         irq,
>         handler,
>         _pin: PhantomPinned
>     })
>     .pin_chain(|this| {
>         // SAFETY: TODO: correct FFI safety requirements
>         to_result(unsafe {
>             bindings::request_irq(...)
>         })
>     })
> 
> The `pin_chain` function is exactly for this use-case, doing some
> operation that might fail after initializing & it will drop the value
> when the closure fails.

No, that doesn't work. Using pin_chain will call free_irq if the call to
request_irq fails, which is incorrect.

Alice

Re: [PATCH v3 1/2] rust: irq: add support for request_irq()

[Alice Ryhl]

On Thu, May 15, 2025 at 03:52:29PM +0200, Danilo Krummrich wrote:
> On Thu, May 15, 2025 at 03:45:28PM +0200, Danilo Krummrich wrote:
> > On Thu, May 15, 2025 at 10:16:33AM -0300, Daniel Almeida wrote:
> > > 
> > > >> 
> > > >> Well, not really, because this impl PinInit can be assigned to something larger
> > > >> that is already pinned, like drm::Device::Data for example, which is (or was)
> > > >> already behind an Arc, or any other private data in other subsystems.
> > > >> 
> > > >> IIUC what you proposed has yet another indirection. If we reuse the example
> > > >> from above, that would be an Arc for the drm Data, and another Arc for the
> > > >> handler itself?
> > > > 
> > > > Can't you implement Handler for drm::Device::Data and e.g. make Registration
> > > > take an Arc<T: Handler>?
> > > 
> > > No, because drivers may need more than one handler. Panthor needs 3, for
> > > example, for 3 different lines.
> > > 
> > > > 
> > > > The irq::Registration itself doesn't need to be allocated dynamically, so it'd
> > > > still be a single allocation, no?
> > > > 
> > > 
> > > Right, the registrations don't, but the handlers do.
> > 
> > Why does the handler need to be allocated dynamically?
> > 
> > What about something like the following?
> > 
> > 	pub struct Registration<T, H: Handler<T>> { ... };
> > 
> > 	pub trait Handler<T> {
> > 	   fn handle_irq(&T) -> IrqReturn;
> > 	}
> > 
> > 	// Could be `drm::Device::Data`.
> > 	struct MyData { ... };
> > 
> > 	// Implements `Handler<MyData>`.
> > 	struct IRQHandler1;
> > 	struct IRQHandler2;
> > 
> > 	// `data` is `Arc<MyData>`
> > 	irq::Registration::<IRQHandler1>::new(data, ...);
> > 	irq::Registration::<IRQHandler2>::new(data, ...);
> > 
> > With that you can have as many IRQs as you want without any additional
> > allocation.
> 
> Alternatively we could also do the following, which is probably simpler.
> 
> 	pub struct Registration<H: Handler> { ... };
> 
> 	pub trait Handler {
> 	   fn handle_irq(&self) -> IrqReturn;
> 	}
> 
> 	// Could be `drm::Device::Data`.
> 	struct MyData { ... };
> 
> 	// Implements `Handler`.
> 	struct IRQHandler1(Arc<MyData>);
> 	struct IRQHandler2(Arc<MyData>);
> 
> 	// `data` is `Arc<MyData>`
> 	let handler1 = IRQHandler1::new(data);
> 	let handler2 = IRQHandler2::new(data);
> 
> 	irq::Registration::new(handler1, ...);
> 	irq::Registration::new(handler2, ...);

Yeah there's no reason to *force* the user to store an Arc. The user
could store data that is only accessed by the irq callback without an
Arc. I agree that for shared content you probably want to access it
through an Arc.

Alice

Re: [PATCH v3 1/2] rust: irq: add support for request_irq()

[Alice Ryhl]

On Wed, May 14, 2025 at 11:53:21PM +0200, Danilo Krummrich wrote:
> On Wed, May 14, 2025 at 04:20:51PM -0300, Daniel Almeida wrote:
> > +/// // This is running in process context.
> > +/// fn register_irq(irq: u32, handler: Handler) -> Result<Arc<Registration<Handler>>> {
> > +///     let registration = Registration::register(irq, 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)?;
> 
> This makes it possible to arbitrarily extend the lifetime of an IRQ
> registration. However, we must guarantee that the IRQ is unregistered when the
> corresponding device is unbound. We can't allow drivers to hold on to device
> resources after the corresponding device has been unbound.
> 
> Why does the data need to be part of the IRQ registration itself? Why can't we
> pass in an Arc<T> instance already when we register the IRQ?
> 
> This way we'd never have a reason to ever access the Registration instance
> itself ever again and we can easily wrap it as Devres<irq::Registration> -
> analogously to devm_request_irq() on the C side - without any penalties.

If we step away from the various Rust abstractions for a moment, then it
sounds like the request_irq API must follow these rules:

1. Ensure that free_irq is called before the device is unbound.
2. Ensure that associated data remains valid until after free_irq is
   called.

We don't necessarily need to ensure that the Registration object itself
is dropped before the device is unbound - as long as free_irq is called
in time, it's okay.

Now, if we use Devres, the way this is enforced is that during cleanup
of a device, we call free_irq *and* we destroy the associated data right
afterwards. By also destroying the associated data at that moment, it
becomes necessary to use rcu_read_lock() to access the associated data.
But if we just don't destroy the associated data during device cleanup,
then that requirement goes away.

Based on this, we could imagine something along these lines:

    struct RegistrationInner(i32);
    impl Drop for RegistrationInner {
        fn drop(&mut self) {
            free_irq(...);
        }
    }
    
    struct Registration<T> {
        reg: Devres<RegistrationInner>,
        data: T,
    }

Here you can access the `data` on the registration at any time without
synchronization.

Note that with this, I don't think the rcu-based devres is really the
right choice. It would make more sense to have a mutex along these
lines:

    // drop Registration
    if devm_remove_callback() {
        free_irq();
    } else {
        mutex_lock();
        free_irq();
        mutex_unlock();
    }
    
    // devm callback
    mutex_lock();
    free_irq();
    mutex_unlock();

This avoids that really expensive call to synchronize_rcu() in the devm
callback.

Of course, for cases where the callback is only removed in the devm
callback, you could also do away with the registration, take a
ForeignOwnable that you can turn into the void pointer, and have the
devm callback call free_irq followed by dropping the ForeignOwnable
pointer.

Alice

Re: [PATCH v3 1/2] rust: irq: add support for request_irq()

[Danilo Krummrich]

On Mon, Jun 02, 2025 at 04:19:21PM +0000, Alice Ryhl wrote:
> On Wed, May 14, 2025 at 11:53:21PM +0200, Danilo Krummrich wrote:
> > On Wed, May 14, 2025 at 04:20:51PM -0300, Daniel Almeida wrote:
> > > +/// // This is running in process context.
> > > +/// fn register_irq(irq: u32, handler: Handler) -> Result<Arc<Registration<Handler>>> {
> > > +///     let registration = Registration::register(irq, 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)?;
> > 
> > This makes it possible to arbitrarily extend the lifetime of an IRQ
> > registration. However, we must guarantee that the IRQ is unregistered when the
> > corresponding device is unbound. We can't allow drivers to hold on to device
> > resources after the corresponding device has been unbound.
> > 
> > Why does the data need to be part of the IRQ registration itself? Why can't we
> > pass in an Arc<T> instance already when we register the IRQ?
> > 
> > This way we'd never have a reason to ever access the Registration instance
> > itself ever again and we can easily wrap it as Devres<irq::Registration> -
> > analogously to devm_request_irq() on the C side - without any penalties.
> 
> If we step away from the various Rust abstractions for a moment, then it
> sounds like the request_irq API must follow these rules:
> 
> 1. Ensure that free_irq is called before the device is unbound.
> 2. Ensure that associated data remains valid until after free_irq is
>    called.
> 
> We don't necessarily need to ensure that the Registration object itself
> is dropped before the device is unbound - as long as free_irq is called
> in time, it's okay.
> 
> Now, if we use Devres, the way this is enforced is that during cleanup
> of a device, we call free_irq *and* we destroy the associated data right
> afterwards. By also destroying the associated data at that moment, it
> becomes necessary to use rcu_read_lock() to access the associated data.
> But if we just don't destroy the associated data during device cleanup,
> then that requirement goes away.
> 
> Based on this, we could imagine something along these lines:
> 
>     struct RegistrationInner(i32);
>     impl Drop for RegistrationInner {
>         fn drop(&mut self) {
>             free_irq(...);
>         }
>     }
>     
>     struct Registration<T> {
>         reg: Devres<RegistrationInner>,
>         data: T,
>     }
> 
> Here you can access the `data` on the registration at any time without
> synchronization.

I was just about to reply to Daniel proposing exactly this alternative, it's
equivalent with what I went with in the MiscDeviceRegistration patches for
supporting the device driver use-case [1].

So, I'm perfectly fine with this approach.

[1] https://lore.kernel.org/lkml/20250530142447.166524-6-dakr@kernel.org/

> 
> Note that with this, I don't think the rcu-based devres is really the
> right choice. It would make more sense to have a mutex along these
> lines:
> 
>     // drop Registration
>     if devm_remove_callback() {
>         free_irq();
>     } else {
>         mutex_lock();
>         free_irq();
>         mutex_unlock();
>     }
>     
>     // devm callback
>     mutex_lock();
>     free_irq();
>     mutex_unlock();
> 
> This avoids that really expensive call to synchronize_rcu() in the devm
> callback.

This would indeed be an optimization for the special case that we never care
about actually accessing the Revocable, i.e. where we have no need to make the
"read" have minimal overhead.

However, I think we can do even better and, at the same time, avoid this special
case optimization and have everything be the common case with what I already
plan on implementing:

I want that regardless of how many devres callbacks a driver registers by having
Devres wrapped objects around, there's only a *single* synchronize_rcu() call for
all of them.

We can achieve this by having the devres callbacks on driver unbind in two
stages, the first callback flips the Revocable's atomic for for all Devres
objects, then there is a single synchronize_rcu() and then we drop_in_place()
all the Revocable's data for all Devres objects.

As mentioned above I plan on implementing this, but given that it's "just" a
performance optimization for the driver unbind path and given that we're not
very close to a production driver upstream, it haven't had the highest priority
for me to implement so far.

Re: [PATCH v3 1/2] rust: irq: add support for request_irq()

[Danilo Krummrich]

On Mon, Jun 02, 2025 at 11:40:43AM -0300, Daniel Almeida wrote:
> I guess the only difference would be removing the handler() accessor, as the
> caller is now expected to figure this part out on his own, i.e.:
> 
> In your example (IIUC) that would mean accessing the Arc in IRQHandler1
> and IRQHandler2 through some other clone and from the actual T:Handler in
> the callback.

Basically yes, but there's also the other alternative in [1] -- either is fine
for me.

[1] https://lore.kernel.org/lkml/aD3f1GSZJ6K-RP5r@pollux/

Re: [PATCH v3 2/2] rust: platform: add irq accessors

[Danilo Krummrich]

On Mon, Jun 02, 2025 at 11:56:28AM -0300, Daniel Almeida wrote:
> Hi Danilo,
> 
> […]
> 
> >> +
> >> +    /// Same as [`Self::irq_by_name`] but does not print an error message if an IRQ
> >> +    /// cannot be obtained.
> >> +    pub fn optional_irq_by_name(&self, name: &CStr) -> Result<u32> {
> >> +        // SAFETY: `self.as_raw` returns a valid pointer to a `struct platform_device`.
> >> +        let res = unsafe {
> >> +            bindings::platform_get_irq_byname_optional(self.as_raw(), name.as_char_ptr())
> >> +        };
> >> +
> >> +        if res < 0 {
> >> +            return Err(Error::from_errno(res));
> >> +        }
> >> +
> >> +        Ok(res as u32)
> >> +    }
> > 
> > I don't like the indirection of claiming a u32 representing the IRQ number from
> > a bus device and stuffing it into an irq::Registration.
> > 
> > It would be better we we'd make it impossible (or at least harder) for a driver
> > to pass the wrong number to irq::Registration.
> > 
> > I see two options:
> > 
> >  1) Make the platform::Device accessors themselves return an
> >     irq::Registration.
> > 
> >  2) Make the platform::Device accessors return some kind of transparent cookie,
> >     that drivers can't create themselves that can be fed into the
> >     irq::Registration.
> > 
> > My preference would be 1) if there's no major ergonomic issue with that.
> 
> Isn’t 1 way more cluttered?

I don't think so, your irq::Registration::register() function needs a reference
to the registering device anyways.

And given that, now that I think about it, it even has to be this way.

Otherwise, I could provide irq::Registration::register() an IRQ number that does
not belong to the bus device that is passed into irq::Registration::register().

So, irq::Registration::register() even needs to be unsafe, with the safety
requirement that the IRQ number must belong to the corresponding device and
should be wrapped by bus device abstractions providing the safe driver API.

> That's because the accessors would have to forward all of the arguments (i.e.:
> currently 4) to register().

It's only the additional arguments below, which you can also wrap in an
irq::Args structure to keep things cleaner. :)

+        flags: Flags,
+        name: &'static CStr,
+        handler: T,

Re: [PATCH v3 1/2] rust: irq: add support for request_irq()

[Alice Ryhl]

On Mon, Jun 02, 2025 at 07:31:00PM +0200, Danilo Krummrich wrote:
> On Mon, Jun 02, 2025 at 04:19:21PM +0000, Alice Ryhl wrote:
> > On Wed, May 14, 2025 at 11:53:21PM +0200, Danilo Krummrich wrote:
> > > On Wed, May 14, 2025 at 04:20:51PM -0300, Daniel Almeida wrote:
> > > > +/// // This is running in process context.
> > > > +/// fn register_irq(irq: u32, handler: Handler) -> Result<Arc<Registration<Handler>>> {
> > > > +///     let registration = Registration::register(irq, 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)?;
> > > 
> > > This makes it possible to arbitrarily extend the lifetime of an IRQ
> > > registration. However, we must guarantee that the IRQ is unregistered when the
> > > corresponding device is unbound. We can't allow drivers to hold on to device
> > > resources after the corresponding device has been unbound.
> > > 
> > > Why does the data need to be part of the IRQ registration itself? Why can't we
> > > pass in an Arc<T> instance already when we register the IRQ?
> > > 
> > > This way we'd never have a reason to ever access the Registration instance
> > > itself ever again and we can easily wrap it as Devres<irq::Registration> -
> > > analogously to devm_request_irq() on the C side - without any penalties.
> > 
> > If we step away from the various Rust abstractions for a moment, then it
> > sounds like the request_irq API must follow these rules:
> > 
> > 1. Ensure that free_irq is called before the device is unbound.
> > 2. Ensure that associated data remains valid until after free_irq is
> >    called.
> > 
> > We don't necessarily need to ensure that the Registration object itself
> > is dropped before the device is unbound - as long as free_irq is called
> > in time, it's okay.
> > 
> > Now, if we use Devres, the way this is enforced is that during cleanup
> > of a device, we call free_irq *and* we destroy the associated data right
> > afterwards. By also destroying the associated data at that moment, it
> > becomes necessary to use rcu_read_lock() to access the associated data.
> > But if we just don't destroy the associated data during device cleanup,
> > then that requirement goes away.
> > 
> > Based on this, we could imagine something along these lines:
> > 
> >     struct RegistrationInner(i32);
> >     impl Drop for RegistrationInner {
> >         fn drop(&mut self) {
> >             free_irq(...);
> >         }
> >     }
> >     
> >     struct Registration<T> {
> >         reg: Devres<RegistrationInner>,
> >         data: T,
> >     }
> > 
> > Here you can access the `data` on the registration at any time without
> > synchronization.
> 
> I was just about to reply to Daniel proposing exactly this alternative, it's
> equivalent with what I went with in the MiscDeviceRegistration patches for
> supporting the device driver use-case [1].
> 
> So, I'm perfectly fine with this approach.
> 
> [1] https://lore.kernel.org/lkml/20250530142447.166524-6-dakr@kernel.org/

Ah, ok.

> > 
> > Note that with this, I don't think the rcu-based devres is really the
> > right choice. It would make more sense to have a mutex along these
> > lines:
> > 
> >     // drop Registration
> >     if devm_remove_callback() {
> >         free_irq();
> >     } else {
> >         mutex_lock();
> >         free_irq();
> >         mutex_unlock();
> >     }
> >     
> >     // devm callback
> >     mutex_lock();
> >     free_irq();
> >     mutex_unlock();
> > 
> > This avoids that really expensive call to synchronize_rcu() in the devm
> > callback.
> 
> This would indeed be an optimization for the special case that we never care
> about actually accessing the Revocable, i.e. where we have no need to make the
> "read" have minimal overhead.
> 
> However, I think we can do even better and, at the same time, avoid this special
> case optimization and have everything be the common case with what I already
> plan on implementing:
> 
> I want that regardless of how many devres callbacks a driver registers by having
> Devres wrapped objects around, there's only a *single* synchronize_rcu() call for
> all of them.
> 
> We can achieve this by having the devres callbacks on driver unbind in two
> stages, the first callback flips the Revocable's atomic for for all Devres
> objects, then there is a single synchronize_rcu() and then we drop_in_place()
> all the Revocable's data for all Devres objects.
> 
> As mentioned above I plan on implementing this, but given that it's "just" a
> performance optimization for the driver unbind path and given that we're not
> very close to a production driver upstream, it haven't had the highest priority
> for me to implement so far.

That optimization sounds like something we definitely want, but I have
one question: is free_irq() safe to use in atomic context / inside
rcu_read_lock()? What about the threaded-irq variant? After all, don't
they sleep until existing callbacks finish running?

Alice

Re: [PATCH v3 1/2] rust: irq: add support for request_irq()

[Danilo Krummrich]

On Tue, Jun 03, 2025 at 08:28:42AM +0000, Alice Ryhl wrote:
> That optimization sounds like something we definitely want, but I have
> one question: is free_irq() safe to use in atomic context / inside
> rcu_read_lock()? What about the threaded-irq variant?

No, free_irq() must not be called from atomic context. Hence, it's not valid to
call it from within an RCU read-side critical section.

I assume you're confusing something, free_irq() is called from the destructor of
the irq::Registration object, hence it is either called when the object itself
is dropped or from the devres callback, which is called after the
synchronize_rcu(), but not from an RCU read-side critical section.

Re: [PATCH v3 1/2] rust: irq: add support for request_irq()

[Alice Ryhl]

On Tue, Jun 03, 2025 at 10:46:28AM +0200, Danilo Krummrich wrote:
> On Tue, Jun 03, 2025 at 08:28:42AM +0000, Alice Ryhl wrote:
> > That optimization sounds like something we definitely want, but I have
> > one question: is free_irq() safe to use in atomic context / inside
> > rcu_read_lock()? What about the threaded-irq variant?
> 
> No, free_irq() must not be called from atomic context. Hence, it's not valid to
> call it from within an RCU read-side critical section.
> 
> I assume you're confusing something, free_irq() is called from the destructor of
> the irq::Registration object, hence it is either called when the object itself
> is dropped or from the devres callback, which is called after the
> synchronize_rcu(), but not from an RCU read-side critical section.

Ok hold on ... I guess the issue I thought was there manifests itself in
another way. What about this situation?

Thread 1                 Thread 2
device removal starts
                         Drop for Devres starts running
                         devm_remove_action() = 0
device is fully unbound
                         free_irq()

Now the call to free_irq() happens too late, because there's nothing in
the devm callback stack to wait for it.

Alice

Re: [PATCH v3 1/2] rust: irq: add support for request_irq()

[Danilo Krummrich]

On Tue, Jun 03, 2025 at 08:54:56AM +0000, Alice Ryhl wrote:
> On Tue, Jun 03, 2025 at 10:46:28AM +0200, Danilo Krummrich wrote:
> > On Tue, Jun 03, 2025 at 08:28:42AM +0000, Alice Ryhl wrote:
> > > That optimization sounds like something we definitely want, but I have
> > > one question: is free_irq() safe to use in atomic context / inside
> > > rcu_read_lock()? What about the threaded-irq variant?
> > 
> > No, free_irq() must not be called from atomic context. Hence, it's not valid to
> > call it from within an RCU read-side critical section.
> > 
> > I assume you're confusing something, free_irq() is called from the destructor of
> > the irq::Registration object, hence it is either called when the object itself
> > is dropped or from the devres callback, which is called after the
> > synchronize_rcu(), but not from an RCU read-side critical section.
> 
> Ok hold on ... I guess the issue I thought was there manifests itself in
> another way. What about this situation?
> 
> Thread 1                 Thread 2
> device removal starts
>                          Drop for Devres starts running
>                          devm_remove_action() = 0
> device is fully unbound
>                          free_irq()
> 
> Now the call to free_irq() happens too late, because there's nothing in
> the devm callback stack to wait for it.

This is indeed a flaw in the Devres implementation.

In my initial implementation I even thought of this, but then obviously forgot
about it and introduced this bug in commit 8ff656643d30 ("rust: devres: remove
action in `Devres::drop`").

In order to fix this, we should just revert this commit -- thanks for catching
this!

Re: [PATCH v3 1/2] rust: irq: add support for request_irq()

[Alice Ryhl]

On Tue, Jun 3, 2025 at 11:10 AM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Tue, Jun 03, 2025 at 08:54:56AM +0000, Alice Ryhl wrote:
> > On Tue, Jun 03, 2025 at 10:46:28AM +0200, Danilo Krummrich wrote:
> > > On Tue, Jun 03, 2025 at 08:28:42AM +0000, Alice Ryhl wrote:
> > > > That optimization sounds like something we definitely want, but I have
> > > > one question: is free_irq() safe to use in atomic context / inside
> > > > rcu_read_lock()? What about the threaded-irq variant?
> > >
> > > No, free_irq() must not be called from atomic context. Hence, it's not valid to
> > > call it from within an RCU read-side critical section.
> > >
> > > I assume you're confusing something, free_irq() is called from the destructor of
> > > the irq::Registration object, hence it is either called when the object itself
> > > is dropped or from the devres callback, which is called after the
> > > synchronize_rcu(), but not from an RCU read-side critical section.
> >
> > Ok hold on ... I guess the issue I thought was there manifests itself in
> > another way. What about this situation?
> >
> > Thread 1                 Thread 2
> > device removal starts
> >                          Drop for Devres starts running
> >                          devm_remove_action() = 0
> > device is fully unbound
> >                          free_irq()
> >
> > Now the call to free_irq() happens too late, because there's nothing in
> > the devm callback stack to wait for it.
>
> This is indeed a flaw in the Devres implementation.
>
> In my initial implementation I even thought of this, but then obviously forgot
> about it and introduced this bug in commit 8ff656643d30 ("rust: devres: remove
> action in `Devres::drop`").
>
> In order to fix this, we should just revert this commit -- thanks for catching
> this!

I don't think that helps. If Devres::drop gets to swap is_available
before the devm callback performs the swap, then the devm callback is
just a no-op and the device still doesn't wait for free_irq() to
finish running.

Alice

Re: [PATCH v3 1/2] rust: irq: add support for request_irq()

[Danilo Krummrich]

On Tue, Jun 03, 2025 at 11:18:40AM +0200, Alice Ryhl wrote:
> I don't think that helps. If Devres::drop gets to swap is_available
> before the devm callback performs the swap, then the devm callback is
> just a no-op and the device still doesn't wait for free_irq() to
> finish running.

True, this will indeed always be racy. The rule from the C API has always been
that devm_{remove,release}_action() must not be called if a concurrent unbind
can't be ruled out. Consequently, the same is true for Revocable::revoke() in
this case.

I think Devres::drop() shouldn't do anything then and instead we should provide
Devres::release() and Devres::remove(), which require the &Device<Bound>
reference the Devres object was created with, in order to prove that there
can't be a concurrent unbind, just like Devres::access().

Re: [PATCH v3 1/2] rust: irq: add support for request_irq()

[Alice Ryhl]

On Tue, Jun 3, 2025 at 11:43 AM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Tue, Jun 03, 2025 at 11:18:40AM +0200, Alice Ryhl wrote:
> > I don't think that helps. If Devres::drop gets to swap is_available
> > before the devm callback performs the swap, then the devm callback is
> > just a no-op and the device still doesn't wait for free_irq() to
> > finish running.
>
> True, this will indeed always be racy. The rule from the C API has always been
> that devm_{remove,release}_action() must not be called if a concurrent unbind
> can't be ruled out. Consequently, the same is true for Revocable::revoke() in
> this case.
>
> I think Devres::drop() shouldn't do anything then and instead we should provide
> Devres::release() and Devres::remove(), which require the &Device<Bound>
> reference the Devres object was created with, in order to prove that there
> can't be a concurrent unbind, just like Devres::access().

What I suggested with the mutex would work if you remove the devm
callback *after* calling free_irq.

    // drop Registration
    mutex_lock();
    free_irq();
    mutex_unlock();
    devm_remove_callback();

    // devm callback
    mutex_lock();
    free_irq();
    mutex_unlock();

Another simpler option is to just not support unregistering the irq
callback except through devm. Then you don't have a registration at
all. Creating the callback can take an irq number and a ForeignOwnable
to put in the void pointer. The devm callback calls free_irq and drops
the ForeignOwnable.

Alice

Re: [PATCH v3 1/2] rust: irq: add support for request_irq()

[Danilo Krummrich]

On Tue, Jun 03, 2025 at 11:57:22AM +0200, Alice Ryhl wrote:
> On Tue, Jun 3, 2025 at 11:43 AM Danilo Krummrich <dakr@kernel.org> wrote:
> >
> > On Tue, Jun 03, 2025 at 11:18:40AM +0200, Alice Ryhl wrote:
> > > I don't think that helps. If Devres::drop gets to swap is_available
> > > before the devm callback performs the swap, then the devm callback is
> > > just a no-op and the device still doesn't wait for free_irq() to
> > > finish running.
> >
> > True, this will indeed always be racy. The rule from the C API has always been
> > that devm_{remove,release}_action() must not be called if a concurrent unbind
> > can't be ruled out. Consequently, the same is true for Revocable::revoke() in
> > this case.
> >
> > I think Devres::drop() shouldn't do anything then and instead we should provide
> > Devres::release() and Devres::remove(), which require the &Device<Bound>
> > reference the Devres object was created with, in order to prove that there
> > can't be a concurrent unbind, just like Devres::access().
> 
> What I suggested with the mutex would work if you remove the devm
> callback *after* calling free_irq.
> 
>     // drop Registration
>     mutex_lock();
>     free_irq();
>     mutex_unlock();
>     devm_remove_callback();

I think it would need to be

	if (!devm_remove_callback()) {
		mutex_lock();
		free_irq();
		mutex_unlock();
	}

>     // devm callback
>     mutex_lock();
>     free_irq();
>     mutex_unlock();

Yes, we could solve this with a lock as well, but it would be an additional
lock, just to maintain the current drop() semantics, which I don't see much
value in.

The common case is that the object wrapped in a Devres is meant to live for the
entire duration the device is bound to the driver.

> Another simpler option is to just not support unregistering the irq
> callback except through devm. Then you don't have a registration at
> all. Creating the callback can take an irq number and a ForeignOwnable
> to put in the void pointer. The devm callback calls free_irq and drops
> the ForeignOwnable.

That's basically what Devres::new_foreign_owned() already does.

Re: [PATCH v3 1/2] rust: irq: add support for request_irq()

[Danilo Krummrich]

On Tue, Jun 03, 2025 at 12:09:05PM +0200, Danilo Krummrich wrote:
> Yes, we could solve this with a lock as well, but it would be an additional
> lock, just to maintain the current drop() semantics, which I don't see much
> value in.

If we want to keep the current drop() semantics we could use a completion
instead.

	// Devres::drop()
	revoke_nosync()
	complete()

	// devres_callback
	if !try_revoke() {
		// we end up here if try_revoke() indicates that the object was
		// revoked already
		wait_for_completion()
	}

Re: [PATCH v3 1/2] rust: irq: add support for request_irq()

[Benno Lossin]

On Mon Jun 2, 2025 at 5:20 PM CEST, Alice Ryhl wrote:
> On Wed, May 14, 2025 at 10:04:43PM +0200, Benno Lossin wrote:
>> On Wed May 14, 2025 at 9:20 PM CEST, Daniel Almeida wrote:
>> > +                )
>> > +            });
>> > +
>> > +            if res.is_err() {
>> > +                // SAFETY: We are returning an error, so we can destroy the slot.
>> > +                unsafe { core::ptr::drop_in_place(addr_of_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) }
>> 
>> Please don't use `pin_init_from_closure`, instead do this:
>> 
>>     pin_init!(Self {
>>         irq,
>>         handler,
>>         _pin: PhantomPinned
>>     })
>>     .pin_chain(|this| {
>>         // SAFETY: TODO: correct FFI safety requirements
>>         to_result(unsafe {
>>             bindings::request_irq(...)
>>         })
>>     })
>> 
>> The `pin_chain` function is exactly for this use-case, doing some
>> operation that might fail after initializing & it will drop the value
>> when the closure fails.
>
> No, that doesn't work. Using pin_chain will call free_irq if the call to
> request_irq fails, which is incorrect.

Good catch. That's a bit annoying then... I wonder if there is a
primitive missing in pin-init that could help with this... Any ideas?

---
Cheers,
Benno

Re: [PATCH v3 1/2] rust: irq: add support for request_irq()

[Alice Ryhl]

On Wed, Jun 4, 2025 at 9:37 AM Benno Lossin <lossin@kernel.org> wrote:
>
> On Mon Jun 2, 2025 at 5:20 PM CEST, Alice Ryhl wrote:
> > On Wed, May 14, 2025 at 10:04:43PM +0200, Benno Lossin wrote:
> >> On Wed May 14, 2025 at 9:20 PM CEST, Daniel Almeida wrote:
> >> > +                )
> >> > +            });
> >> > +
> >> > +            if res.is_err() {
> >> > +                // SAFETY: We are returning an error, so we can destroy the slot.
> >> > +                unsafe { core::ptr::drop_in_place(addr_of_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) }
> >>
> >> Please don't use `pin_init_from_closure`, instead do this:
> >>
> >>     pin_init!(Self {
> >>         irq,
> >>         handler,
> >>         _pin: PhantomPinned
> >>     })
> >>     .pin_chain(|this| {
> >>         // SAFETY: TODO: correct FFI safety requirements
> >>         to_result(unsafe {
> >>             bindings::request_irq(...)
> >>         })
> >>     })
> >>
> >> The `pin_chain` function is exactly for this use-case, doing some
> >> operation that might fail after initializing & it will drop the value
> >> when the closure fails.
> >
> > No, that doesn't work. Using pin_chain will call free_irq if the call to
> > request_irq fails, which is incorrect.
>
> Good catch. That's a bit annoying then... I wonder if there is a
> primitive missing in pin-init that could help with this... Any ideas?

I believe initializers for underscore fields would do it. We could
potentially abuse the _pin field, but frankly I think that's too
confusing to the reader.

Alice

Re: [PATCH v3 1/2] rust: irq: add support for request_irq()

[Benno Lossin]

On Wed Jun 4, 2025 at 9:48 AM CEST, Alice Ryhl wrote:
> On Wed, Jun 4, 2025 at 9:37 AM Benno Lossin <lossin@kernel.org> wrote:
>> On Mon Jun 2, 2025 at 5:20 PM CEST, Alice Ryhl wrote:
>> > On Wed, May 14, 2025 at 10:04:43PM +0200, Benno Lossin wrote:
>> >> On Wed May 14, 2025 at 9:20 PM CEST, Daniel Almeida wrote:
>> >> > +                )
>> >> > +            });
>> >> > +
>> >> > +            if res.is_err() {
>> >> > +                // SAFETY: We are returning an error, so we can destroy the slot.
>> >> > +                unsafe { core::ptr::drop_in_place(addr_of_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) }
>> >>
>> >> Please don't use `pin_init_from_closure`, instead do this:
>> >>
>> >>     pin_init!(Self {
>> >>         irq,
>> >>         handler,
>> >>         _pin: PhantomPinned
>> >>     })
>> >>     .pin_chain(|this| {
>> >>         // SAFETY: TODO: correct FFI safety requirements
>> >>         to_result(unsafe {
>> >>             bindings::request_irq(...)
>> >>         })
>> >>     })
>> >>
>> >> The `pin_chain` function is exactly for this use-case, doing some
>> >> operation that might fail after initializing & it will drop the value
>> >> when the closure fails.
>> >
>> > No, that doesn't work. Using pin_chain will call free_irq if the call to
>> > request_irq fails, which is incorrect.
>>
>> Good catch. That's a bit annoying then... I wonder if there is a
>> primitive missing in pin-init that could help with this... Any ideas?
>
> I believe initializers for underscore fields would do it. We could
> potentially abuse the _pin field, but frankly I think that's too
> confusing to the reader.

Oh yeah that's the feature we want here, will add it to my list for the
next cycle :)

---
Cheers,
Benno

Re: [PATCH v3 1/2] rust: irq: add support for request_irq()

[Daniel Almeida]

So, what is the result of this discussion?

> On 3 Jun 2025, at 07:16, Danilo Krummrich <dakr@kernel.org> wrote:
> 
> On Tue, Jun 03, 2025 at 12:09:05PM +0200, Danilo Krummrich wrote:
>> Yes, we could solve this with a lock as well, but it would be an additional
>> lock, just to maintain the current drop() semantics, which I don't see much
>> value in.
> 
> If we want to keep the current drop() semantics we could use a completion
> instead.
> 
> // Devres::drop()
> revoke_nosync()
> complete()
> 
> // devres_callback
> if !try_revoke() {
> // we end up here if try_revoke() indicates that the object was
> // revoked already
> wait_for_completion()
> }

This looks like what is going on here [0], so should I implement what Alice suggested? i.e.:

> > Based on this, we could imagine something along these lines:
> > 
> > struct RegistrationInner(i32);
> > impl Drop for RegistrationInner {
> > fn drop(&mut self) {
> > free_irq(...);
> > }
> > }
> > 
> > struct Registration<T> {
> > reg: Devres<RegistrationInner>,
> > data: T,
> > }
> > 
> > Here you can access the `data` on the registration at any time without
> > synchronization.

— Daniel

[0] https://lore.kernel.org/rust-for-linux/CANiq72nfOcOGJuktzSpNAUWwekaZ98JL7c1FhMKCjPSG80JMPA@mail.gmail.com/T/#t

Re: [PATCH v3 1/2] rust: irq: add support for request_irq()

[Danilo Krummrich]

On Wed, Jun 04, 2025 at 03:32:16PM -0300, Daniel Almeida wrote:
> So, what is the result of this discussion?
> 
> > On 3 Jun 2025, at 07:16, Danilo Krummrich <dakr@kernel.org> wrote:
> > 
> > On Tue, Jun 03, 2025 at 12:09:05PM +0200, Danilo Krummrich wrote:
> >> Yes, we could solve this with a lock as well, but it would be an additional
> >> lock, just to maintain the current drop() semantics, which I don't see much
> >> value in.
> > 
> > If we want to keep the current drop() semantics we could use a completion
> > instead.
> > 
> > // Devres::drop()
> > revoke_nosync()
> > complete()
> > 
> > // devres_callback
> > if !try_revoke() {
> > // we end up here if try_revoke() indicates that the object was
> > // revoked already
> > wait_for_completion()
> > }
> 
> This looks like what is going on here [0], so should I implement what Alice suggested? i.e.:
> 
> > > Based on this, we could imagine something along these lines:
> > > 
> > > struct RegistrationInner(i32);
> > > impl Drop for RegistrationInner {
> > > fn drop(&mut self) {
> > > free_irq(...);
> > > }
> > > }
> > > 
> > > struct Registration<T> {
> > > reg: Devres<RegistrationInner>,
> > > data: T,
> > > }
> > > 
> > > Here you can access the `data` on the registration at any time without
> > > synchronization.

Yes, I think that's the best approach. You can also have a look at [1], which
implements this approach for misc device already, even though it's slightly more
complicated.

[1] https://lore.kernel.org/lkml/20250530142447.166524-6-dakr@kernel.org/

> 
> — Daniel
> 
> [0] https://lore.kernel.org/rust-for-linux/CANiq72nfOcOGJuktzSpNAUWwekaZ98JL7c1FhMKCjPSG80JMPA@mail.gmail.com/T/#t