This patch adds support for non-threaded IRQs and handlers through
irq::Registration and the irq::Handler trait.
Registering an irq is dependent upon having a IrqRequest that was
previously allocated by a given device. This will be introduced in
subsequent patches.
Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
---
rust/bindings/bindings_helper.h | 1 +
rust/helpers/helpers.c | 1 +
rust/helpers/irq.c | 9 ++
rust/kernel/irq.rs | 5 +
rust/kernel/irq/request.rs | 273 ++++++++++++++++++++++++++++++++++++++++
5 files changed, 289 insertions(+)
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 8cbb660e2ec218021d16e6e0144acf6f4d7cca13..da0bd23fad59a2373bd873d12ad69c55208aaa38 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -51,6 +51,7 @@
#include <linux/ethtool.h>
#include <linux/file.h>
#include <linux/firmware.h>
+#include <linux/interrupt.h>
#include <linux/fs.h>
#include <linux/jiffies.h>
#include <linux/jump_label.h>
diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
index 393ad201befb80a9ae39866a725744ab88620fbb..e3579fc7e1cfc30c913207a4a78b790259d7ae7a 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c
@@ -22,6 +22,7 @@
#include "dma.c"
#include "drm.c"
#include "err.c"
+#include "irq.c"
#include "fs.c"
#include "io.c"
#include "jump_label.c"
diff --git a/rust/helpers/irq.c b/rust/helpers/irq.c
new file mode 100644
index 0000000000000000000000000000000000000000..1faca428e2c047a656dec3171855c1508d67e60b
--- /dev/null
+++ b/rust/helpers/irq.c
@@ -0,0 +1,9 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/interrupt.h>
+
+int rust_helper_request_irq(unsigned int irq, irq_handler_t handler,
+ unsigned long flags, const char *name, void *dev)
+{
+ return request_irq(irq, handler, flags, name, dev);
+}
diff --git a/rust/kernel/irq.rs b/rust/kernel/irq.rs
index 9abd9a6dc36f3e3ecc1f92ad7b0040176b56a079..01bd08884b72c2a3a9460897bce751c732a19794 100644
--- a/rust/kernel/irq.rs
+++ b/rust/kernel/irq.rs
@@ -12,3 +12,8 @@
/// Flags to be used when registering IRQ handlers.
pub mod flags;
+
+/// IRQ allocation and handling.
+pub mod request;
+
+pub use request::{Handler, IrqRequest, IrqReturn, Registration};
diff --git a/rust/kernel/irq/request.rs b/rust/kernel/irq/request.rs
new file mode 100644
index 0000000000000000000000000000000000000000..4f4beaa3c7887660440b9ddc52414020a0d165ac
--- /dev/null
+++ b/rust/kernel/irq/request.rs
@@ -0,0 +1,273 @@
+// SPDX-License-Identifier: GPL-2.0
+// SPDX-FileCopyrightText: Copyright 2025 Collabora ltd.
+
+//! This module provides types like [`Registration`] which allow users to
+//! register handlers for a given IRQ line.
+
+use core::marker::PhantomPinned;
+
+use crate::alloc::Allocator;
+use crate::device::Bound;
+use crate::device::Device;
+use crate::devres::Devres;
+use crate::error::to_result;
+use crate::irq::flags::Flags;
+use crate::prelude::*;
+use crate::str::CStr;
+use crate::sync::Arc;
+
+/// The value that can be returned from an IrqHandler or a ThreadedIrqHandler.
+pub enum IrqReturn {
+ /// The interrupt was not from this device or was not handled.
+ None,
+
+ /// The interrupt was handled by this device.
+ Handled,
+}
+
+impl IrqReturn {
+ fn into_inner(self) -> u32 {
+ match self {
+ IrqReturn::None => bindings::irqreturn_IRQ_NONE,
+ IrqReturn::Handled => bindings::irqreturn_IRQ_HANDLED,
+ }
+ }
+}
+
+/// Callbacks for an IRQ handler.
+pub trait Handler: Sync {
+ /// The hard IRQ handler.
+ ///
+ /// This is executed in interrupt context, hence all corresponding
+ /// limitations do apply.
+ ///
+ /// All work that does not necessarily need to be executed from
+ /// interrupt context, should be deferred to a threaded handler.
+ /// See also [`ThreadedRegistration`].
+ fn handle(&self) -> IrqReturn;
+}
+
+impl<T: ?Sized + Handler + Send> Handler for Arc<T> {
+ fn handle(&self) -> IrqReturn {
+ T::handle(self)
+ }
+}
+
+impl<T: ?Sized + Handler, A: Allocator> Handler for Box<T, A> {
+ fn handle(&self) -> IrqReturn {
+ T::handle(self)
+ }
+}
+
+/// # Invariants
+///
+/// - `self.irq` is the same as the one passed to `request_{threaded}_irq`.
+/// - `cookie` was passed to `request_{threaded}_irq` as the cookie. It
+/// is guaranteed to be unique by the type system, since each call to
+/// `new` will return a different instance of `Registration`.
+#[pin_data(PinnedDrop)]
+struct RegistrationInner {
+ irq: u32,
+ cookie: *mut kernel::ffi::c_void,
+}
+
+impl RegistrationInner {
+ fn synchronize(&self) {
+ // SAFETY: safe as per the invariants of `RegistrationInner`
+ unsafe { bindings::synchronize_irq(self.irq) };
+ }
+}
+
+#[pinned_drop]
+impl PinnedDrop for RegistrationInner {
+ fn drop(self: Pin<&mut Self>) {
+ // SAFETY:
+ //
+ // Safe as per the invariants of `RegistrationInner` and:
+ //
+ // - The containing struct is `!Unpin` and was initialized using
+ // pin-init, so it occupied the same memory location for the entirety of
+ // its lifetime.
+ //
+ // Notice that this will block until all handlers finish executing,
+ // i.e.: at no point will &self be invalid while the handler is running.
+ unsafe { bindings::free_irq(self.irq, self.cookie) };
+ }
+}
+
+// SAFETY: We only use `inner` on drop, which called at most once with no
+// concurrent access.
+unsafe impl Sync for RegistrationInner {}
+
+// SAFETY: It is safe to send `RegistrationInner` across threads.
+unsafe impl Send for RegistrationInner {}
+
+/// A request for an IRQ line for a given device.
+///
+/// # Invariants
+///
+/// - `ìrq` is the number of an interrupt source of `dev`.
+/// - `irq` has not been registered yet.
+pub struct IrqRequest<'a> {
+ dev: &'a Device<Bound>,
+ irq: u32,
+}
+
+impl<'a> IrqRequest<'a> {
+ /// Creates a new IRQ request for the given device and IRQ number.
+ ///
+ /// # Safety
+ ///
+ /// - `irq` should be a valid IRQ number for `dev`.
+ pub(crate) unsafe fn new(dev: &'a Device<Bound>, irq: u32) -> Self {
+ IrqRequest { dev, irq }
+ }
+}
+
+/// A registration of an IRQ handler for a given IRQ line.
+///
+/// # Examples
+///
+/// The following is an example of using `Registration`. It uses a
+/// [`AtomicU32`](core::sync::AtomicU32) to provide the interior mutability.
+///
+/// ```
+/// use core::sync::atomic::AtomicU32;
+/// use core::sync::atomic::Ordering;
+///
+/// use kernel::prelude::*;
+/// use kernel::device::Bound;
+/// use kernel::irq::flags;
+/// use kernel::irq::Registration;
+/// use kernel::irq::IrqRequest;
+/// use kernel::irq::IrqReturn;
+/// use kernel::sync::Arc;
+/// use kernel::c_str;
+/// use kernel::alloc::flags::GFP_KERNEL;
+///
+/// // Declare a struct that will be passed in when the interrupt fires. The u32
+/// // merely serves as an example of some internal data.
+/// struct Data(AtomicU32);
+///
+/// // [`kernel::irq::request::Handler::handle`] takes `&self`. This example
+/// // illustrates how interior mutability can be used when sharing the data
+/// // between process context and IRQ context.
+///
+/// type Handler = Data;
+///
+/// impl kernel::irq::request::Handler for Handler {
+/// // This is executing in IRQ context in some CPU. Other CPUs can still
+/// // try to access to data.
+/// fn handle(&self) -> IrqReturn {
+/// self.0.fetch_add(1, Ordering::Relaxed);
+///
+/// IrqReturn::Handled
+/// }
+/// }
+///
+/// // Registers an IRQ handler for the given IrqRequest.
+/// //
+/// // This is executing in process context and assumes that `request` was
+/// // previously acquired from a device.
+/// fn register_irq(handler: Handler, request: IrqRequest<'_>) -> Result<Arc<Registration<Handler>>> {
+/// let registration = Registration::new(request, flags::SHARED, c_str!("my_device"), handler);
+///
+/// let registration = Arc::pin_init(registration, GFP_KERNEL)?;
+///
+/// // The data can be accessed from process context too.
+/// registration.handler().0.fetch_add(1, Ordering::Relaxed);
+///
+/// Ok(registration)
+/// }
+/// # Ok::<(), Error>(())
+/// ```
+///
+/// # Invariants
+///
+/// * We own an irq handler using `&self.handler` as its private data.
+///
+#[pin_data]
+pub struct Registration<T: Handler + 'static> {
+ #[pin]
+ inner: Devres<RegistrationInner>,
+
+ #[pin]
+ handler: T,
+
+ /// Pinned because we need address stability so that we can pass a pointer
+ /// to the callback.
+ #[pin]
+ _pin: PhantomPinned,
+}
+
+impl<T: Handler + 'static> Registration<T> {
+ /// Registers the IRQ handler with the system for the given IRQ number.
+ pub fn new<'a>(
+ request: IrqRequest<'a>,
+ flags: Flags,
+ name: &'static CStr,
+ handler: T,
+ ) -> impl PinInit<Self, Error> + 'a {
+ try_pin_init!(&this in Self {
+ handler,
+ inner <- Devres::new(
+ request.dev,
+ try_pin_init!(RegistrationInner {
+ // SAFETY: `this` is a valid pointer to the `Registration` instance
+ cookie: unsafe { &raw mut (*this.as_ptr()).handler }.cast(),
+ irq: {
+ // SAFETY:
+ // - The callbacks are valid for use with request_irq.
+ // - If this succeeds, the slot is guaranteed to be valid until the
+ // destructor of Self runs, which will deregister the callbacks
+ // before the memory location becomes invalid.
+ to_result(unsafe {
+ bindings::request_irq(
+ request.irq,
+ Some(handle_irq_callback::<T>),
+ flags.into_inner() as usize,
+ name.as_char_ptr(),
+ (&raw mut (*this.as_ptr()).handler).cast(),
+ )
+ })?;
+ request.irq
+ }
+ })
+ ),
+ _pin: PhantomPinned,
+ })
+ }
+
+ /// Returns a reference to the handler that was registered with the system.
+ pub fn handler(&self) -> &T {
+ &self.handler
+ }
+
+ /// Wait for pending IRQ handlers on other CPUs.
+ ///
+ /// This will attempt to access the inner [`Devres`] container.
+ pub fn try_synchronize(&self) -> Result {
+ let inner = self.inner.try_access().ok_or(ENODEV)?;
+ inner.synchronize();
+ Ok(())
+ }
+
+ /// Wait for pending IRQ handlers on other CPUs.
+ pub fn synchronize(&self, dev: &Device<Bound>) -> Result {
+ let inner = self.inner.access(dev)?;
+ inner.synchronize();
+ Ok(())
+ }
+}
+
+/// # Safety
+///
+/// This function should be only used as the callback in `request_irq`.
+unsafe extern "C" fn handle_irq_callback<T: Handler>(
+ _irq: i32,
+ ptr: *mut core::ffi::c_void,
+) -> core::ffi::c_uint {
+ // SAFETY: `ptr` is a pointer to T set in `Registration::new`
+ let handler = unsafe { &*(ptr as *const T) };
+ T::handle(handler).into_inner()
+}
--
2.50.0
On Thu Jul 3, 2025 at 9:30 PM CEST, Daniel Almeida wrote:
> +/// Callbacks for an IRQ handler.
> +pub trait Handler: Sync {
> + /// The hard IRQ handler.
> + ///
> + /// This is executed in interrupt context, hence all corresponding
> + /// limitations do apply.
> + ///
> + /// All work that does not necessarily need to be executed from
> + /// interrupt context, should be deferred to a threaded handler.
> + /// See also [`ThreadedRegistration`].
> + fn handle(&self) -> IrqReturn;
> +}
One thing I forgot, the IRQ handlers should have a &Device<Bound> argument,
i.e.:
fn handle(&self, dev: &Device<Bound>) -> IrqReturn
IRQ registrations naturally give us this guarantee, so we should take advantage
of that.
- Danilo
> On 12 Jul 2025, at 18:24, Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Thu Jul 3, 2025 at 9:30 PM CEST, Daniel Almeida wrote:
>> +/// Callbacks for an IRQ handler.
>> +pub trait Handler: Sync {
>> + /// The hard IRQ handler.
>> + ///
>> + /// This is executed in interrupt context, hence all corresponding
>> + /// limitations do apply.
>> + ///
>> + /// All work that does not necessarily need to be executed from
>> + /// interrupt context, should be deferred to a threaded handler.
>> + /// See also [`ThreadedRegistration`].
>> + fn handle(&self) -> IrqReturn;
>> +}
>
> One thing I forgot, the IRQ handlers should have a &Device<Bound> argument,
> i.e.:
>
> fn handle(&self, dev: &Device<Bound>) -> IrqReturn
>
> IRQ registrations naturally give us this guarantee, so we should take advantage
> of that.
>
> - Danilo
Hi Danilo,
I do not immediately see a way to get a Device<Bound> from here:
unsafe extern "C" fn handle_irq_callback<T: Handler>(_irq: i32, ptr: *mut c_void) -> c_uint {
Refall that we've established `ptr` to be the address of the handler. This
came after some back and forth and after the extensive discussion that Benno
and Boqun had w.r.t to pinning in request_irq().
On Sun Jul 13, 2025 at 1:32 AM CEST, Daniel Almeida wrote:
>
>
>> On 12 Jul 2025, at 18:24, Danilo Krummrich <dakr@kernel.org> wrote:
>>
>> On Thu Jul 3, 2025 at 9:30 PM CEST, Daniel Almeida wrote:
>>> +/// Callbacks for an IRQ handler.
>>> +pub trait Handler: Sync {
>>> + /// The hard IRQ handler.
>>> + ///
>>> + /// This is executed in interrupt context, hence all corresponding
>>> + /// limitations do apply.
>>> + ///
>>> + /// All work that does not necessarily need to be executed from
>>> + /// interrupt context, should be deferred to a threaded handler.
>>> + /// See also [`ThreadedRegistration`].
>>> + fn handle(&self) -> IrqReturn;
>>> +}
>>
>> One thing I forgot, the IRQ handlers should have a &Device<Bound> argument,
>> i.e.:
>>
>> fn handle(&self, dev: &Device<Bound>) -> IrqReturn
>>
>> IRQ registrations naturally give us this guarantee, so we should take advantage
>> of that.
>>
>> - Danilo
>
> Hi Danilo,
>
> I do not immediately see a way to get a Device<Bound> from here:
>
> unsafe extern "C" fn handle_irq_callback<T: Handler>(_irq: i32, ptr: *mut c_void) -> c_uint {
>
> Refall that we've established `ptr` to be the address of the handler. This
> came after some back and forth and after the extensive discussion that Benno
> and Boqun had w.r.t to pinning in request_irq().
You can just wrap the Handler in a new type and store the pointer there:
#[pin_data]
struct Wrapper {
#[pin]
handler: T,
dev: NonNull<Device<Bound>>,
}
And then pass a pointer to the Wrapper field to request_irq();
handle_irq_callback() can construct a &T and a &Device<Bound> from this.
Note that storing a device pointer, without its own reference count, is
perfectly fine, since inner (Devres<RegistrationInner>) already holds a
reference to the device and guarantees the bound scope for the handler
callbacks.
It makes sense to document this as an invariant of Wrapper (or whatever we end
up calling it).
On Sun Jul 13, 2025 at 12:24 PM CEST, Danilo Krummrich wrote:
> On Sun Jul 13, 2025 at 1:32 AM CEST, Daniel Almeida wrote:
>>
>>
>>> On 12 Jul 2025, at 18:24, Danilo Krummrich <dakr@kernel.org> wrote:
>>>
>>> On Thu Jul 3, 2025 at 9:30 PM CEST, Daniel Almeida wrote:
>>>> +/// Callbacks for an IRQ handler.
>>>> +pub trait Handler: Sync {
>>>> + /// The hard IRQ handler.
>>>> + ///
>>>> + /// This is executed in interrupt context, hence all corresponding
>>>> + /// limitations do apply.
>>>> + ///
>>>> + /// All work that does not necessarily need to be executed from
>>>> + /// interrupt context, should be deferred to a threaded handler.
>>>> + /// See also [`ThreadedRegistration`].
>>>> + fn handle(&self) -> IrqReturn;
>>>> +}
>>>
>>> One thing I forgot, the IRQ handlers should have a &Device<Bound> argument,
>>> i.e.:
>>>
>>> fn handle(&self, dev: &Device<Bound>) -> IrqReturn
>>>
>>> IRQ registrations naturally give us this guarantee, so we should take advantage
>>> of that.
>>>
>>> - Danilo
>>
>> Hi Danilo,
>>
>> I do not immediately see a way to get a Device<Bound> from here:
>>
>> unsafe extern "C" fn handle_irq_callback<T: Handler>(_irq: i32, ptr: *mut c_void) -> c_uint {
>>
>> Refall that we've established `ptr` to be the address of the handler. This
>> came after some back and forth and after the extensive discussion that Benno
>> and Boqun had w.r.t to pinning in request_irq().
>
> You can just wrap the Handler in a new type and store the pointer there:
>
> #[pin_data]
> struct Wrapper {
> #[pin]
> handler: T,
> dev: NonNull<Device<Bound>>,
> }
>
> And then pass a pointer to the Wrapper field to request_irq();
> handle_irq_callback() can construct a &T and a &Device<Bound> from this.
>
> Note that storing a device pointer, without its own reference count, is
> perfectly fine, since inner (Devres<RegistrationInner>) already holds a
> reference to the device and guarantees the bound scope for the handler
> callbacks.
Can't we just add an accessor function to `Devres`?
Also `Devres` only stores `Device<Normal>`, not `Device<Bound>`...
---
Cheers,
Benno
> It makes sense to document this as an invariant of Wrapper (or whatever we end
> up calling it).
On Sun Jul 13, 2025 at 1:19 PM CEST, Benno Lossin wrote:
> On Sun Jul 13, 2025 at 12:24 PM CEST, Danilo Krummrich wrote:
>> On Sun Jul 13, 2025 at 1:32 AM CEST, Daniel Almeida wrote:
>>>
>>>
>>>> On 12 Jul 2025, at 18:24, Danilo Krummrich <dakr@kernel.org> wrote:
>>>>
>>>> On Thu Jul 3, 2025 at 9:30 PM CEST, Daniel Almeida wrote:
>>>>> +/// Callbacks for an IRQ handler.
>>>>> +pub trait Handler: Sync {
>>>>> + /// The hard IRQ handler.
>>>>> + ///
>>>>> + /// This is executed in interrupt context, hence all corresponding
>>>>> + /// limitations do apply.
>>>>> + ///
>>>>> + /// All work that does not necessarily need to be executed from
>>>>> + /// interrupt context, should be deferred to a threaded handler.
>>>>> + /// See also [`ThreadedRegistration`].
>>>>> + fn handle(&self) -> IrqReturn;
>>>>> +}
>>>>
>>>> One thing I forgot, the IRQ handlers should have a &Device<Bound> argument,
>>>> i.e.:
>>>>
>>>> fn handle(&self, dev: &Device<Bound>) -> IrqReturn
>>>>
>>>> IRQ registrations naturally give us this guarantee, so we should take advantage
>>>> of that.
>>>>
>>>> - Danilo
>>>
>>> Hi Danilo,
>>>
>>> I do not immediately see a way to get a Device<Bound> from here:
>>>
>>> unsafe extern "C" fn handle_irq_callback<T: Handler>(_irq: i32, ptr: *mut c_void) -> c_uint {
>>>
>>> Refall that we've established `ptr` to be the address of the handler. This
>>> came after some back and forth and after the extensive discussion that Benno
>>> and Boqun had w.r.t to pinning in request_irq().
>>
>> You can just wrap the Handler in a new type and store the pointer there:
>>
>> #[pin_data]
>> struct Wrapper {
>> #[pin]
>> handler: T,
>> dev: NonNull<Device<Bound>>,
>> }
>>
>> And then pass a pointer to the Wrapper field to request_irq();
>> handle_irq_callback() can construct a &T and a &Device<Bound> from this.
>>
>> Note that storing a device pointer, without its own reference count, is
>> perfectly fine, since inner (Devres<RegistrationInner>) already holds a
>> reference to the device and guarantees the bound scope for the handler
>> callbacks.
>
> Can't we just add an accessor function to `Devres`?
#[pin_data]
pub struct Registration<T: Handler + 'static> {
#[pin]
inner: Devres<RegistrationInner>,
#[pin]
handler: T,
/// Pinned because we need address stability so that we can pass a pointer
/// to the callback.
#[pin]
_pin: PhantomPinned,
}
Currently we pass the address of handler to request_irq(), so this doesn't help,
hence my proposal to replace the above T with Wrapper (actually Wrapper<T>).
> Also `Devres` only stores `Device<Normal>`, not `Device<Bound>`...
The Devres instance itself may out-live device unbind, but it ensures that the
encapsulated data does not, hence it holds a reference count, i.e. ARef<Device>.
Device<Bound> or ARef<Device<Bound>> *never* exists, only &'a Device<Bound>
within a corresponding scope for which we can guarantee the device is bound.
In the proposed wrapper we can store a NonNull<Device<Bound>> though, because we
can safely give out a &Device<Bound> in the IRQ's handle() callback. This is
because:
(1) RegistrationInner is guarded by Devres and guarantees that free_irq() is
completed *before* the device is unbound.
(2) It is guaranteed that the device pointer is valid because (1) guarantees
it's even bound and because Devres<RegistrationInner> itself has a
reference count.
On Sun Jul 13, 2025 at 1:57 PM CEST, Danilo Krummrich wrote:
> On Sun Jul 13, 2025 at 1:19 PM CEST, Benno Lossin wrote:
>> On Sun Jul 13, 2025 at 12:24 PM CEST, Danilo Krummrich wrote:
>>> On Sun Jul 13, 2025 at 1:32 AM CEST, Daniel Almeida wrote:
>>>>
>>>>
>>>>> On 12 Jul 2025, at 18:24, Danilo Krummrich <dakr@kernel.org> wrote:
>>>>>
>>>>> On Thu Jul 3, 2025 at 9:30 PM CEST, Daniel Almeida wrote:
>>>>>> +/// Callbacks for an IRQ handler.
>>>>>> +pub trait Handler: Sync {
>>>>>> + /// The hard IRQ handler.
>>>>>> + ///
>>>>>> + /// This is executed in interrupt context, hence all corresponding
>>>>>> + /// limitations do apply.
>>>>>> + ///
>>>>>> + /// All work that does not necessarily need to be executed from
>>>>>> + /// interrupt context, should be deferred to a threaded handler.
>>>>>> + /// See also [`ThreadedRegistration`].
>>>>>> + fn handle(&self) -> IrqReturn;
>>>>>> +}
>>>>>
>>>>> One thing I forgot, the IRQ handlers should have a &Device<Bound> argument,
>>>>> i.e.:
>>>>>
>>>>> fn handle(&self, dev: &Device<Bound>) -> IrqReturn
>>>>>
>>>>> IRQ registrations naturally give us this guarantee, so we should take advantage
>>>>> of that.
>>>>>
>>>>> - Danilo
>>>>
>>>> Hi Danilo,
>>>>
>>>> I do not immediately see a way to get a Device<Bound> from here:
>>>>
>>>> unsafe extern "C" fn handle_irq_callback<T: Handler>(_irq: i32, ptr: *mut c_void) -> c_uint {
>>>>
>>>> Refall that we've established `ptr` to be the address of the handler. This
>>>> came after some back and forth and after the extensive discussion that Benno
>>>> and Boqun had w.r.t to pinning in request_irq().
>>>
>>> You can just wrap the Handler in a new type and store the pointer there:
>>>
>>> #[pin_data]
>>> struct Wrapper {
>>> #[pin]
>>> handler: T,
>>> dev: NonNull<Device<Bound>>,
>>> }
>>>
>>> And then pass a pointer to the Wrapper field to request_irq();
>>> handle_irq_callback() can construct a &T and a &Device<Bound> from this.
>>>
>>> Note that storing a device pointer, without its own reference count, is
>>> perfectly fine, since inner (Devres<RegistrationInner>) already holds a
>>> reference to the device and guarantees the bound scope for the handler
>>> callbacks.
>>
>> Can't we just add an accessor function to `Devres`?
>
> #[pin_data]
> pub struct Registration<T: Handler + 'static> {
> #[pin]
> inner: Devres<RegistrationInner>,
>
> #[pin]
> handler: T,
>
> /// Pinned because we need address stability so that we can pass a pointer
> /// to the callback.
> #[pin]
> _pin: PhantomPinned,
> }
>
> Currently we pass the address of handler to request_irq(), so this doesn't help,
> hence my proposal to replace the above T with Wrapper (actually Wrapper<T>).
You can just use `container_of!`?
>> Also `Devres` only stores `Device<Normal>`, not `Device<Bound>`...
>
> The Devres instance itself may out-live device unbind, but it ensures that the
> encapsulated data does not, hence it holds a reference count, i.e. ARef<Device>.
>
> Device<Bound> or ARef<Device<Bound>> *never* exists, only &'a Device<Bound>
> within a corresponding scope for which we can guarantee the device is bound.
>
> In the proposed wrapper we can store a NonNull<Device<Bound>> though, because we
> can safely give out a &Device<Bound> in the IRQ's handle() callback. This is
> because:
>
> (1) RegistrationInner is guarded by Devres and guarantees that free_irq() is
> completed *before* the device is unbound.
How does it ensure that?
>
> (2) It is guaranteed that the device pointer is valid because (1) guarantees
> it's even bound and because Devres<RegistrationInner> itself has a
> reference count.
Yeah but I would find it much more natural (and also useful in other
circumstances) if `Devres<T>` would give you access to `Device` (at
least the `Normal` type state).
Depending on how (1) is ensured, we might just need an unsafe function
that turns `Device<Normal>` into `Device<Bound>`.
---
Cheers,
Benno
On Sun Jul 13, 2025 at 2:16 PM CEST, Benno Lossin wrote:
> On Sun Jul 13, 2025 at 1:57 PM CEST, Danilo Krummrich wrote:
>> On Sun Jul 13, 2025 at 1:19 PM CEST, Benno Lossin wrote:
>>> On Sun Jul 13, 2025 at 12:24 PM CEST, Danilo Krummrich wrote:
>>>> On Sun Jul 13, 2025 at 1:32 AM CEST, Daniel Almeida wrote:
>>>>>
>>>>>
>>>>>> On 12 Jul 2025, at 18:24, Danilo Krummrich <dakr@kernel.org> wrote:
>>>>>>
>>>>>> On Thu Jul 3, 2025 at 9:30 PM CEST, Daniel Almeida wrote:
>>>>>>> +/// Callbacks for an IRQ handler.
>>>>>>> +pub trait Handler: Sync {
>>>>>>> + /// The hard IRQ handler.
>>>>>>> + ///
>>>>>>> + /// This is executed in interrupt context, hence all corresponding
>>>>>>> + /// limitations do apply.
>>>>>>> + ///
>>>>>>> + /// All work that does not necessarily need to be executed from
>>>>>>> + /// interrupt context, should be deferred to a threaded handler.
>>>>>>> + /// See also [`ThreadedRegistration`].
>>>>>>> + fn handle(&self) -> IrqReturn;
>>>>>>> +}
>>>>>>
>>>>>> One thing I forgot, the IRQ handlers should have a &Device<Bound> argument,
>>>>>> i.e.:
>>>>>>
>>>>>> fn handle(&self, dev: &Device<Bound>) -> IrqReturn
>>>>>>
>>>>>> IRQ registrations naturally give us this guarantee, so we should take advantage
>>>>>> of that.
>>>>>>
>>>>>> - Danilo
>>>>>
>>>>> Hi Danilo,
>>>>>
>>>>> I do not immediately see a way to get a Device<Bound> from here:
>>>>>
>>>>> unsafe extern "C" fn handle_irq_callback<T: Handler>(_irq: i32, ptr: *mut c_void) -> c_uint {
>>>>>
>>>>> Refall that we've established `ptr` to be the address of the handler. This
>>>>> came after some back and forth and after the extensive discussion that Benno
>>>>> and Boqun had w.r.t to pinning in request_irq().
>>>>
>>>> You can just wrap the Handler in a new type and store the pointer there:
>>>>
>>>> #[pin_data]
>>>> struct Wrapper {
>>>> #[pin]
>>>> handler: T,
>>>> dev: NonNull<Device<Bound>>,
>>>> }
>>>>
>>>> And then pass a pointer to the Wrapper field to request_irq();
>>>> handle_irq_callback() can construct a &T and a &Device<Bound> from this.
>>>>
>>>> Note that storing a device pointer, without its own reference count, is
>>>> perfectly fine, since inner (Devres<RegistrationInner>) already holds a
>>>> reference to the device and guarantees the bound scope for the handler
>>>> callbacks.
>>>
>>> Can't we just add an accessor function to `Devres`?
>>
>> #[pin_data]
>> pub struct Registration<T: Handler + 'static> {
>> #[pin]
>> inner: Devres<RegistrationInner>,
>>
>> #[pin]
>> handler: T,
>>
>> /// Pinned because we need address stability so that we can pass a pointer
>> /// to the callback.
>> #[pin]
>> _pin: PhantomPinned,
>> }
>>
>> Currently we pass the address of handler to request_irq(), so this doesn't help,
>> hence my proposal to replace the above T with Wrapper (actually Wrapper<T>).
>
> You can just use `container_of!`?
Sure, that's possible too.
>>> Also `Devres` only stores `Device<Normal>`, not `Device<Bound>`...
>>
>> The Devres instance itself may out-live device unbind, but it ensures that the
>> encapsulated data does not, hence it holds a reference count, i.e. ARef<Device>.
>>
>> Device<Bound> or ARef<Device<Bound>> *never* exists, only &'a Device<Bound>
>> within a corresponding scope for which we can guarantee the device is bound.
>>
>> In the proposed wrapper we can store a NonNull<Device<Bound>> though, because we
>> can safely give out a &Device<Bound> in the IRQ's handle() callback. This is
>> because:
>>
>> (1) RegistrationInner is guarded by Devres and guarantees that free_irq() is
>> completed *before* the device is unbound.
>
> How does it ensure that?
RegistrationInner calls free_irq() in it's drop() method; Devres revokes it on
device unbind.
>>
>> (2) It is guaranteed that the device pointer is valid because (1) guarantees
>> it's even bound and because Devres<RegistrationInner> itself has a
>> reference count.
>
> Yeah but I would find it much more natural (and also useful in other
> circumstances) if `Devres<T>` would give you access to `Device` (at
> least the `Normal` type state).
If we use container_of!() instead or just pass the address of Self (i.e.
Registration) to request_irq() instead,
pub fn device(&self) -> &Device
is absolutely possible to add to Devres, of course.
> Depending on how (1) is ensured, we might just need an unsafe function
> that turns `Device<Normal>` into `Device<Bound>`.
`&Device<Normal>` in `&Device<Bound>`, yes. I have such a method locally
already (but haven't sent it yet), because that's going to be a use-case for
other abstractions as well. One specific example is the PWM Chip abstraction
[1].
[1] https://lore.kernel.org/lkml/20250710-rust-next-pwm-working-fan-for-sending-v11-3-93824a16f9ec@samsung.com/
Hi, > >>> >>> (2) It is guaranteed that the device pointer is valid because (1) guarantees >>> it's even bound and because Devres<RegistrationInner> itself has a >>> reference count. >> >> Yeah but I would find it much more natural (and also useful in other >> circumstances) if `Devres<T>` would give you access to `Device` (at >> least the `Normal` type state). > > If we use container_of!() instead or just pass the address of Self (i.e. > Registration) to request_irq() instead, Boqun, Benno, are you ok with passing the address of Registration<T> as the cookie? Recall that this was a change requested in v4, so I am checking whether we are all on the same page before going back to that. See [0], i.e.: > > > >> Well yes and no, with the Devres changes, the `cookie` can just be the > > > >> address of the `RegistrationInner` & we can do it this way :) > > > >> > > > >> --- > > > >> Cheers, > > > >> Benno > > > > > > > > > > > > No, we need this to be the address of the the whole thing (i.e. > > > > Registration<T>), otherwise you can’t access the handler in the irq > > > > callback. > > You only need the access of `handler` in the irq callback, right? I.e. > passing the address of `handler` would suffice (of course you need > to change the irq callback as well). — Daniel [0] https://lore.kernel.org/all/aFq3P_4XgP0dUrAS@Mac.home/
On Mon, Jul 14, 2025 at 5:13 PM Daniel Almeida <daniel.almeida@collabora.com> wrote: > > Hi, > > > > >>> > >>> (2) It is guaranteed that the device pointer is valid because (1) guarantees > >>> it's even bound and because Devres<RegistrationInner> itself has a > >>> reference count. > >> > >> Yeah but I would find it much more natural (and also useful in other > >> circumstances) if `Devres<T>` would give you access to `Device` (at > >> least the `Normal` type state). > > > > If we use container_of!() instead or just pass the address of Self (i.e. > > Registration) to request_irq() instead, > > > Boqun, Benno, are you ok with passing the address of Registration<T> as the cookie? > > Recall that this was a change requested in v4, so I am checking whether we are > all on the same page before going back to that. > > See [0], i.e.: > [0] https://lore.kernel.org/all/aFq3P_4XgP0dUrAS@Mac.home/ After discussing this, Daniel and I agreed that I will implement the change adding a Device<Bound> argument to the callback. I will be sending a patch adding it separately as a follow-up to Daniel's work. Alice
On Tue Jul 15, 2025 at 2:33 PM CEST, Alice Ryhl wrote: > On Mon, Jul 14, 2025 at 5:13 PM Daniel Almeida > <daniel.almeida@collabora.com> wrote: >> >> Hi, >> >> > >> >>> >> >>> (2) It is guaranteed that the device pointer is valid because (1) guarantees >> >>> it's even bound and because Devres<RegistrationInner> itself has a >> >>> reference count. >> >> >> >> Yeah but I would find it much more natural (and also useful in other >> >> circumstances) if `Devres<T>` would give you access to `Device` (at >> >> least the `Normal` type state). >> > >> > If we use container_of!() instead or just pass the address of Self (i.e. >> > Registration) to request_irq() instead, >> >> >> Boqun, Benno, are you ok with passing the address of Registration<T> as the cookie? >> >> Recall that this was a change requested in v4, so I am checking whether we are >> all on the same page before going back to that. >> >> See [0], i.e.: >> [0] https://lore.kernel.org/all/aFq3P_4XgP0dUrAS@Mac.home/ > > After discussing this, Daniel and I agreed that I will implement the > change adding a Device<Bound> argument to the callback. I will be > sending a patch adding it separately as a follow-up to Daniel's work. That works for me, thanks!
On Mon Jul 14, 2025 at 5:12 PM CEST, Daniel Almeida wrote: > Hi, > >> >>>> >>>> (2) It is guaranteed that the device pointer is valid because (1) guarantees >>>> it's even bound and because Devres<RegistrationInner> itself has a >>>> reference count. >>> >>> Yeah but I would find it much more natural (and also useful in other >>> circumstances) if `Devres<T>` would give you access to `Device` (at >>> least the `Normal` type state). >> >> If we use container_of!() instead or just pass the address of Self (i.e. >> Registration) to request_irq() instead, > > > Boqun, Benno, are you ok with passing the address of Registration<T> as the cookie? > > Recall that this was a change requested in v4, so I am checking whether we are > all on the same page before going back to that. I looked at the conversation again and the important part is that you aren't allowed to initialize the `RegistrationInner` before the `request_irq` call was successful (because the drop will run `irq_free`). What pointer you use as the cookie doesn't matter for this. Feel free to double check again with the concrete code. --- Cheers, Benno > See [0], i.e.: > >> > > >> Well yes and no, with the Devres changes, the `cookie` can just be the >> > > >> address of the `RegistrationInner` & we can do it this way :) >> > > >> >> > > >> --- >> > > >> Cheers, >> > > >> Benno >> > > > >> > > > >> > > > No, we need this to be the address of the the whole thing (i.e. >> > > > Registration<T>), otherwise you can’t access the handler in the irq >> > > > callback. >> >> You only need the access of `handler` in the irq callback, right? I.e. >> passing the address of `handler` would suffice (of course you need >> to change the irq callback as well). > > > — Daniel > > [0] https://lore.kernel.org/all/aFq3P_4XgP0dUrAS@Mac.home/
On Sun, Jul 13, 2025 at 02:42:41PM +0200, Danilo Krummrich wrote:
[...]
> If we use container_of!() instead or just pass the address of Self (i.e.
> Registration) to request_irq() instead,
>
> pub fn device(&self) -> &Device
>
> is absolutely possible to add to Devres, of course.
>
One thing to notice is that in `Devres::new()`, `inner` is initialized
before `dev`:
try_pin_init!(&this in Self {
// INVARIANT: `inner` is properly initialized.
inner <- Opaque::pin_init(try_pin_init!(Inner {
data <- Revocable::new(data),
devm <- Completion::new(),
revoke <- Completion::new(),
})),
For `irq::Registration`, request_irq() is called at `inner`
initialization. So now interrupts can happen at any moment, but `dev` is
still uninitialized.
callback,
dev: {
// SAFETY: `this` is a valid pointer to uninitialized memory.
let inner = unsafe { &raw mut (*this.as_ptr()).inner };
// SAFETY:
// - `dev.as_raw()` is a pointer to a valid bound device.
// - `inner` is guaranteed to be a valid for the duration of the lifetime of `Self`.
// - `devm_add_action()` is guaranteed not to call `callback` until `this` has been
// properly initialized, because we require `dev` (i.e. the *bound* device) to
// live at least as long as the returned `impl PinInit<Self, Error>`.
to_result(unsafe {
bindings::devm_add_action(dev.as_raw(), Some(callback), inner.cast())
})?;
dev.into()
},
})
I think you need to reorder the initialization of `inner` to be after
`dev` for this. And it should be fine, because the whole device is in
bound state while the PinInit being called, so `inner.cast()` being a
pointer to uninitialized memory should be fine (because the `callback`
won't be called).
Regards,
Boqun
> > Depending on how (1) is ensured, we might just need an unsafe function
> > that turns `Device<Normal>` into `Device<Bound>`.
>
> `&Device<Normal>` in `&Device<Bound>`, yes. I have such a method locally
> already (but haven't sent it yet), because that's going to be a use-case for
> other abstractions as well. One specific example is the PWM Chip abstraction
> [1].
>
> [1] https://lore.kernel.org/lkml/20250710-rust-next-pwm-working-fan-for-sending-v11-3-93824a16f9ec@samsung.com/
On Mon Jul 14, 2025 at 8:42 AM CEST, Boqun Feng wrote: > I think you need to reorder the initialization of `inner` to be after > `dev` for this. Indeed, good catch!
On Mon Jul 14, 2025 at 11:24 AM CEST, Danilo Krummrich wrote: > On Mon Jul 14, 2025 at 8:42 AM CEST, Boqun Feng wrote: >> I think you need to reorder the initialization of `inner` to be after >> `dev` for this. > > Indeed, good catch! Just to add, it's more than the order of inner and dev. For such use-cases we have to ensure that Devres as a whole is properly initialized once Devres::inner::data is initialized. Going to send a patch for that.
On Sun Jul 13, 2025 at 2:42 PM CEST, Danilo Krummrich wrote:
> On Sun Jul 13, 2025 at 2:16 PM CEST, Benno Lossin wrote:
>> On Sun Jul 13, 2025 at 1:57 PM CEST, Danilo Krummrich wrote:
>>> On Sun Jul 13, 2025 at 1:19 PM CEST, Benno Lossin wrote:
>>>> On Sun Jul 13, 2025 at 12:24 PM CEST, Danilo Krummrich wrote:
>>>>> On Sun Jul 13, 2025 at 1:32 AM CEST, Daniel Almeida wrote:
>>>>>>
>>>>>>
>>>>>>> On 12 Jul 2025, at 18:24, Danilo Krummrich <dakr@kernel.org> wrote:
>>>>>>>
>>>>>>> On Thu Jul 3, 2025 at 9:30 PM CEST, Daniel Almeida wrote:
>>>>>>>> +/// Callbacks for an IRQ handler.
>>>>>>>> +pub trait Handler: Sync {
>>>>>>>> + /// The hard IRQ handler.
>>>>>>>> + ///
>>>>>>>> + /// This is executed in interrupt context, hence all corresponding
>>>>>>>> + /// limitations do apply.
>>>>>>>> + ///
>>>>>>>> + /// All work that does not necessarily need to be executed from
>>>>>>>> + /// interrupt context, should be deferred to a threaded handler.
>>>>>>>> + /// See also [`ThreadedRegistration`].
>>>>>>>> + fn handle(&self) -> IrqReturn;
>>>>>>>> +}
>>>>>>>
>>>>>>> One thing I forgot, the IRQ handlers should have a &Device<Bound> argument,
>>>>>>> i.e.:
>>>>>>>
>>>>>>> fn handle(&self, dev: &Device<Bound>) -> IrqReturn
>>>>>>>
>>>>>>> IRQ registrations naturally give us this guarantee, so we should take advantage
>>>>>>> of that.
>>>>>>>
>>>>>>> - Danilo
>>>>>>
>>>>>> Hi Danilo,
>>>>>>
>>>>>> I do not immediately see a way to get a Device<Bound> from here:
>>>>>>
>>>>>> unsafe extern "C" fn handle_irq_callback<T: Handler>(_irq: i32, ptr: *mut c_void) -> c_uint {
>>>>>>
>>>>>> Refall that we've established `ptr` to be the address of the handler. This
>>>>>> came after some back and forth and after the extensive discussion that Benno
>>>>>> and Boqun had w.r.t to pinning in request_irq().
>>>>>
>>>>> You can just wrap the Handler in a new type and store the pointer there:
>>>>>
>>>>> #[pin_data]
>>>>> struct Wrapper {
>>>>> #[pin]
>>>>> handler: T,
>>>>> dev: NonNull<Device<Bound>>,
>>>>> }
>>>>>
>>>>> And then pass a pointer to the Wrapper field to request_irq();
>>>>> handle_irq_callback() can construct a &T and a &Device<Bound> from this.
>>>>>
>>>>> Note that storing a device pointer, without its own reference count, is
>>>>> perfectly fine, since inner (Devres<RegistrationInner>) already holds a
>>>>> reference to the device and guarantees the bound scope for the handler
>>>>> callbacks.
>>>>
>>>> Can't we just add an accessor function to `Devres`?
>>>
>>> #[pin_data]
>>> pub struct Registration<T: Handler + 'static> {
>>> #[pin]
>>> inner: Devres<RegistrationInner>,
>>>
>>> #[pin]
>>> handler: T,
>>>
>>> /// Pinned because we need address stability so that we can pass a pointer
>>> /// to the callback.
>>> #[pin]
>>> _pin: PhantomPinned,
>>> }
>>>
>>> Currently we pass the address of handler to request_irq(), so this doesn't help,
>>> hence my proposal to replace the above T with Wrapper (actually Wrapper<T>).
>>
>> You can just use `container_of!`?
>
> Sure, that's possible too.
>
>>>> Also `Devres` only stores `Device<Normal>`, not `Device<Bound>`...
>>>
>>> The Devres instance itself may out-live device unbind, but it ensures that the
>>> encapsulated data does not, hence it holds a reference count, i.e. ARef<Device>.
>>>
>>> Device<Bound> or ARef<Device<Bound>> *never* exists, only &'a Device<Bound>
>>> within a corresponding scope for which we can guarantee the device is bound.
>>>
>>> In the proposed wrapper we can store a NonNull<Device<Bound>> though, because we
>>> can safely give out a &Device<Bound> in the IRQ's handle() callback. This is
>>> because:
>>>
>>> (1) RegistrationInner is guarded by Devres and guarantees that free_irq() is
>>> completed *before* the device is unbound.
>>
>> How does it ensure that?
>
> RegistrationInner calls free_irq() in it's drop() method; Devres revokes it on
> device unbind.
Makes sense, so we probably do need the unsafe typestate change
function in this case.
>>>
>>> (2) It is guaranteed that the device pointer is valid because (1) guarantees
>>> it's even bound and because Devres<RegistrationInner> itself has a
>>> reference count.
>>
>> Yeah but I would find it much more natural (and also useful in other
>> circumstances) if `Devres<T>` would give you access to `Device` (at
>> least the `Normal` type state).
>
> If we use container_of!() instead or just pass the address of Self (i.e.
> Registration) to request_irq() instead,
>
> pub fn device(&self) -> &Device
>
> is absolutely possible to add to Devres, of course.
>
>> Depending on how (1) is ensured, we might just need an unsafe function
>> that turns `Device<Normal>` into `Device<Bound>`.
>
> `&Device<Normal>` in `&Device<Bound>`, yes. I have such a method locally
> already (but haven't sent it yet), because that's going to be a use-case for
> other abstractions as well. One specific example is the PWM Chip abstraction
> [1].
>
> [1] https://lore.kernel.org/lkml/20250710-rust-next-pwm-working-fan-for-sending-v11-3-93824a16f9ec@samsung.com/
I think this solution sounds much better than storing an additional
`NonNull<Device<Bound>>`.
---
Cheers,
Benno
On Sun Jul 13, 2025 at 5:29 PM CEST, Benno Lossin wrote: > I think this solution sounds much better than storing an additional > `NonNull<Device<Bound>>`. I can send two patches for that. The IRQ abstractions have to either land through driver-core or in the next cycle anyways.
On a second look, I wonder how useful this will be. fn handle(&self, dev: &Device<Bound>) -> IrqReturn Sorry for borrowing this terminology, but here we offer Device<Bound>, while I suspect that most drivers will be looking for the most derived Device type instead. So for drm drivers this will be drm::Device, for example, not the base dev::Device type. I assume that this pattern will hold for other subsystems as well. Which brings me to my second point: drivers can store an ARef<drm::Device> on the handler itself, and I assume that the same will be possible in other subsystems. -- Daniel
On Sun Jul 13, 2025 at 4:09 PM CEST, Daniel Almeida wrote:
> On a second look, I wonder how useful this will be.
>
> fn handle(&self, dev: &Device<Bound>) -> IrqReturn
>
> Sorry for borrowing this terminology, but here we offer Device<Bound>, while I
> suspect that most drivers will be looking for the most derived Device type
> instead. So for drm drivers this will be drm::Device, for example, not the base
> dev::Device type. I assume that this pattern will hold for other subsystems as
> well.
>
> Which brings me to my second point: drivers can store an ARef<drm::Device> on
> the handler itself, and I assume that the same will be possible in other
> subsystems.
Well, the whole point is that you can use a &Device<Bound> to directly access
device resources without any overhead, i.e.
fn handle(&self, dev: &Device<Bound>) -> IrqReturn {
let io = self.iomem.access(dev);
io.write32(...);
}
On 13/07/2025 16:19, Danilo Krummrich wrote:
> On Sun Jul 13, 2025 at 4:09 PM CEST, Daniel Almeida wrote:
>> On a second look, I wonder how useful this will be.
>>
>> fn handle(&self, dev: &Device<Bound>) -> IrqReturn
>>
>> Sorry for borrowing this terminology, but here we offer Device<Bound>, while I
>> suspect that most drivers will be looking for the most derived Device type
>> instead. So for drm drivers this will be drm::Device, for example, not the base
>> dev::Device type. I assume that this pattern will hold for other subsystems as
>> well.
>>
>> Which brings me to my second point: drivers can store an ARef<drm::Device> on
>> the handler itself, and I assume that the same will be possible in other
>> subsystems.
>
> Well, the whole point is that you can use a &Device<Bound> to directly access
> device resources without any overhead, i.e.
>
> fn handle(&self, dev: &Device<Bound>) -> IrqReturn {
> let io = self.iomem.access(dev);
>
> io.write32(...);
As this is exactly the example I was discussing privately with Daniel
(many thanks!), independent on the device discussion here, just for my
understanding:
Is it ok to do a 'self.iomem.access(dev)' at each interrupt? Wouldn't it
be cheaper/faster to pass 'io' instead of 'iomem' to the interrupt handler?
fn handle(...) -> IrqReturn {
self.io.write32(...);
?
Thanks
Dirk
On Mon Jul 14, 2025 at 9:57 AM CEST, Dirk Behme wrote:
> On 13/07/2025 16:19, Danilo Krummrich wrote:
>> On Sun Jul 13, 2025 at 4:09 PM CEST, Daniel Almeida wrote:
>>> On a second look, I wonder how useful this will be.
>>>
>>> fn handle(&self, dev: &Device<Bound>) -> IrqReturn
>>>
>>> Sorry for borrowing this terminology, but here we offer Device<Bound>, while I
>>> suspect that most drivers will be looking for the most derived Device type
>>> instead. So for drm drivers this will be drm::Device, for example, not the base
>>> dev::Device type. I assume that this pattern will hold for other subsystems as
>>> well.
>>>
>>> Which brings me to my second point: drivers can store an ARef<drm::Device> on
>>> the handler itself, and I assume that the same will be possible in other
>>> subsystems.
>>
>> Well, the whole point is that you can use a &Device<Bound> to directly access
>> device resources without any overhead, i.e.
>>
>> fn handle(&self, dev: &Device<Bound>) -> IrqReturn {
>> let io = self.iomem.access(dev);
>>
>> io.write32(...);
>
> As this is exactly the example I was discussing privately with Daniel
> (many thanks!), independent on the device discussion here, just for my
> understanding:
>
> Is it ok to do a 'self.iomem.access(dev)' at each interrupt?
Absolutely, Devres::access() is a very cheap accessor, see also [1]. Compiled
down, the only thing that Revocable::access() does is deriving a pointer from
another pointer by adding an offset.
That's exactly why we want the &Device<Bound> cookie, to avoid more expensive
operations.
[1] https://rust.docs.kernel.org/src/kernel/revocable.rs.html#151
> Wouldn't it
> be cheaper/faster to pass 'io' instead of 'iomem' to the interrupt handler?
Well, consider the types of the example:
iomem: Devres<IoMem<SIZE>>
io: &IoMem<Size>
You can't store a reference with a non-static lifetime in something with an
open-ended lifetime, such as the Handler object.
How would you ensure that the reference is still valid? The Devres<IoMem<SIZE>>
object might have been dropped already, either by the user or by Devres revoking
the inner object due to device unbind.
On Sun Jul 13, 2025 at 4:19 PM CEST, Danilo Krummrich wrote:
> On Sun Jul 13, 2025 at 4:09 PM CEST, Daniel Almeida wrote:
>> On a second look, I wonder how useful this will be.
>>
>> fn handle(&self, dev: &Device<Bound>) -> IrqReturn
>>
>> Sorry for borrowing this terminology, but here we offer Device<Bound>, while I
>> suspect that most drivers will be looking for the most derived Device type
>> instead. So for drm drivers this will be drm::Device, for example, not the base
>> dev::Device type. I assume that this pattern will hold for other subsystems as
>> well.
>>
>> Which brings me to my second point: drivers can store an ARef<drm::Device> on
>> the handler itself, and I assume that the same will be possible in other
>> subsystems.
>
> Well, the whole point is that you can use a &Device<Bound> to directly access
> device resources without any overhead, i.e.
>
> fn handle(&self, dev: &Device<Bound>) -> IrqReturn {
> let io = self.iomem.access(dev);
>
> io.write32(...);
> }
So, yes, you can store anything in your handler, but the &Device<Bound> is a
cookie for the scope.
> On 13 Jul 2025, at 11:27, Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Sun Jul 13, 2025 at 4:19 PM CEST, Danilo Krummrich wrote:
>> On Sun Jul 13, 2025 at 4:09 PM CEST, Daniel Almeida wrote:
>>> On a second look, I wonder how useful this will be.
>>>
>>> fn handle(&self, dev: &Device<Bound>) -> IrqReturn
>>>
>>> Sorry for borrowing this terminology, but here we offer Device<Bound>, while I
>>> suspect that most drivers will be looking for the most derived Device type
>>> instead. So for drm drivers this will be drm::Device, for example, not the base
>>> dev::Device type. I assume that this pattern will hold for other subsystems as
>>> well.
>>>
>>> Which brings me to my second point: drivers can store an ARef<drm::Device> on
>>> the handler itself, and I assume that the same will be possible in other
>>> subsystems.
>>
>> Well, the whole point is that you can use a &Device<Bound> to directly access
>> device resources without any overhead, i.e.
>>
>> fn handle(&self, dev: &Device<Bound>) -> IrqReturn {
>> let io = self.iomem.access(dev);
>>
>> io.write32(...);
>> }
>
> So, yes, you can store anything in your handler, but the &Device<Bound> is a
> cookie for the scope.
Fine, but can’t you get a &Device<Bound> from a ARef<drm::Device>, for example?
Perhaps a nicer solution would be to offer this capability instead?
On Sun Jul 13, 2025 at 4:48 PM CEST, Daniel Almeida wrote:
>
>
>> On 13 Jul 2025, at 11:27, Danilo Krummrich <dakr@kernel.org> wrote:
>>
>> On Sun Jul 13, 2025 at 4:19 PM CEST, Danilo Krummrich wrote:
>>> On Sun Jul 13, 2025 at 4:09 PM CEST, Daniel Almeida wrote:
>>>> On a second look, I wonder how useful this will be.
>>>>
>>>> fn handle(&self, dev: &Device<Bound>) -> IrqReturn
>>>>
>>>> Sorry for borrowing this terminology, but here we offer Device<Bound>, while I
>>>> suspect that most drivers will be looking for the most derived Device type
>>>> instead. So for drm drivers this will be drm::Device, for example, not the base
>>>> dev::Device type. I assume that this pattern will hold for other subsystems as
>>>> well.
>>>>
>>>> Which brings me to my second point: drivers can store an ARef<drm::Device> on
>>>> the handler itself, and I assume that the same will be possible in other
>>>> subsystems.
>>>
>>> Well, the whole point is that you can use a &Device<Bound> to directly access
>>> device resources without any overhead, i.e.
>>>
>>> fn handle(&self, dev: &Device<Bound>) -> IrqReturn {
>>> let io = self.iomem.access(dev);
>>>
>>> io.write32(...);
>>> }
>>
>> So, yes, you can store anything in your handler, but the &Device<Bound> is a
>> cookie for the scope.
>
> Fine, but can’t you get a &Device<Bound> from a ARef<drm::Device>, for example?
> Perhaps a nicer solution would be to offer this capability instead?
I think you're confusing quite some things here.
(1) I'm talking about the bus device the IRQ is registered for (e.g. PCI,
platform, etc.). drm::Device represents a class device, which do not
have DeviceContext states, such as Bound.
(2) Owning a reference count of a device (i.e. ARef<Device>) does *not*
guarantee that the device is bound. You can own a reference count to the
device object way beyond it being bound. Instead, the guarantee comes from
the scope.
In this case, the scope is the IRQ callback, since the irq::Registration
guarantees to call and complete free_irq() before the underlying bus
device is unbound.
Hi Daniel,
On 03/07/2025 21:30, Daniel Almeida wrote:
> This patch adds support for non-threaded IRQs and handlers through
> irq::Registration and the irq::Handler trait.
>
> Registering an irq is dependent upon having a IrqRequest that was
> previously allocated by a given device. This will be introduced in
> subsequent patches.
>
> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
...
> diff --git a/rust/kernel/irq/request.rs b/rust/kernel/irq/request.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..4f4beaa3c7887660440b9ddc52414020a0d165ac
> --- /dev/null
> +++ b/rust/kernel/irq/request.rs
> @@ -0,0 +1,273 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// SPDX-FileCopyrightText: Copyright 2025 Collabora ltd.
> +
> +//! This module provides types like [`Registration`] which allow users to
> +//! register handlers for a given IRQ line.
> +
> +use core::marker::PhantomPinned;
> +
> +use crate::alloc::Allocator;
> +use crate::device::Bound;
> +use crate::device::Device;
> +use crate::devres::Devres;
> +use crate::error::to_result;
> +use crate::irq::flags::Flags;
> +use crate::prelude::*;
> +use crate::str::CStr;
> +use crate::sync::Arc;
> +
> +/// The value that can be returned from an IrqHandler or a ThreadedIrqHandler.
> +pub enum IrqReturn {
> + /// The interrupt was not from this device or was not handled.
> + None,
> +
> + /// The interrupt was handled by this device.
> + Handled,
> +}
> +
> +impl IrqReturn {
> + fn into_inner(self) -> u32 {
> + match self {
> + IrqReturn::None => bindings::irqreturn_IRQ_NONE,
> + IrqReturn::Handled => bindings::irqreturn_IRQ_HANDLED,
> + }
> + }
> +}
> +
> +/// Callbacks for an IRQ handler.
> +pub trait Handler: Sync {
> + /// The hard IRQ handler.
> + ///
> + /// This is executed in interrupt context, hence all corresponding
> + /// limitations do apply.
> + ///
> + /// All work that does not necessarily need to be executed from
> + /// interrupt context, should be deferred to a threaded handler.
> + /// See also [`ThreadedRegistration`].
> + fn handle(&self) -> IrqReturn;
> +}
> +
> +impl<T: ?Sized + Handler + Send> Handler for Arc<T> {
> + fn handle(&self) -> IrqReturn {
> + T::handle(self)
> + }
> +}
> +
> +impl<T: ?Sized + Handler, A: Allocator> Handler for Box<T, A> {
> + fn handle(&self) -> IrqReturn {
> + T::handle(self)
> + }
> +}
> +
> +/// # Invariants
> +///
> +/// - `self.irq` is the same as the one passed to `request_{threaded}_irq`.
> +/// - `cookie` was passed to `request_{threaded}_irq` as the cookie. It
> +/// is guaranteed to be unique by the type system, since each call to
> +/// `new` will return a different instance of `Registration`.
> +#[pin_data(PinnedDrop)]
> +struct RegistrationInner {
> + irq: u32,
> + cookie: *mut kernel::ffi::c_void,
> +}
> +
> +impl RegistrationInner {
> + fn synchronize(&self) {
> + // SAFETY: safe as per the invariants of `RegistrationInner`
> + unsafe { bindings::synchronize_irq(self.irq) };
> + }
> +}
> +
> +#[pinned_drop]
> +impl PinnedDrop for RegistrationInner {
> + fn drop(self: Pin<&mut Self>) {
> + // SAFETY:
> + //
> + // Safe as per the invariants of `RegistrationInner` and:
> + //
> + // - The containing struct is `!Unpin` and was initialized using
> + // pin-init, so it occupied the same memory location for the entirety of
> + // its lifetime.
> + //
> + // Notice that this will block until all handlers finish executing,
> + // i.e.: at no point will &self be invalid while the handler is running.
> + unsafe { bindings::free_irq(self.irq, self.cookie) };
> + }
> +}
> +
> +// SAFETY: We only use `inner` on drop, which called at most once with no
> +// concurrent access.
> +unsafe impl Sync for RegistrationInner {}
> +
> +// SAFETY: It is safe to send `RegistrationInner` across threads.
> +unsafe impl Send for RegistrationInner {}
> +
> +/// A request for an IRQ line for a given device.
> +///
> +/// # Invariants
> +///
> +/// - `ìrq` is the number of an interrupt source of `dev`.
> +/// - `irq` has not been registered yet.
> +pub struct IrqRequest<'a> {
> + dev: &'a Device<Bound>,
> + irq: u32,
> +}
> +
> +impl<'a> IrqRequest<'a> {
> + /// Creates a new IRQ request for the given device and IRQ number.
> + ///
> + /// # Safety
> + ///
> + /// - `irq` should be a valid IRQ number for `dev`.
> + pub(crate) unsafe fn new(dev: &'a Device<Bound>, irq: u32) -> Self {
> + IrqRequest { dev, irq }
> + }
> +}
For example for Resource the elements size, start, name and flags are
accessible. Inspired by that, what do you think about exposing the irq
number here, as well?
diff --git a/rust/kernel/irq/request.rs b/rust/kernel/irq/request.rs
index bd489b8d2386..767d66e3e6c7 100644
--- a/rust/kernel/irq/request.rs
+++ b/rust/kernel/irq/request.rs
@@ -123,6 +123,11 @@ impl<'a> IrqRequest<'a> {
pub(crate) unsafe fn new(dev: &'a Device<Bound>, irq: u32) -> Self {
IrqRequest { dev, irq }
}
+
+ /// Returns the IRQ number of an [`IrqRequest`].
+ pub fn irq(&self) -> u32 {
+ self.irq
+ }
}
I'm using that for some dev_info!().
Best regards
Dirk
On 7/8/25 2:15 PM, Dirk Behme wrote:
> For example for Resource the elements size, start, name and flags are
> accessible. Inspired by that, what do you think about exposing the irq
> number here, as well?
>
> diff --git a/rust/kernel/irq/request.rs b/rust/kernel/irq/request.rs
> index bd489b8d2386..767d66e3e6c7 100644
> --- a/rust/kernel/irq/request.rs
> +++ b/rust/kernel/irq/request.rs
> @@ -123,6 +123,11 @@ impl<'a> IrqRequest<'a> {
> pub(crate) unsafe fn new(dev: &'a Device<Bound>, irq: u32) -> Self {
> IrqRequest { dev, irq }
> }
> +
> + /// Returns the IRQ number of an [`IrqRequest`].
> + pub fn irq(&self) -> u32 {
> + self.irq
> + }
> }
>
>
> I'm using that for some dev_info!().
Not sure that's a reasonable candidate for dev_info!() prints, but maybe it can
be useful for some debug prints.
On Thu, Jul 03, 2025 at 04:30:01PM -0300, Daniel Almeida wrote:
[...]
> +#[pin_data]
> +pub struct Registration<T: Handler + 'static> {
> + #[pin]
> + inner: Devres<RegistrationInner>,
> +
> + #[pin]
> + handler: T,
IIRC, as a certain point, we want this to be a `UnsafePinned<T>`, is
that requirement gone or we still need that but 1) `UnsafePinned` is not
available and 2) we can rely on the whole struct being !Unpin for the
address stability temporarily?
I think it was not a problem until we switched to `try_pin_init!()`
instead of `pin_init_from_closure()` because we then had to pass the
address of `handler` instead of the whole struct.
Since we certainly want to use `try_pin_init!()` and we certainly will
have `UnsafePinned`, I think we should just keep this as it is for now,
and add a TODO so that we can clean it up later when we have
`UnsafePinned`?
Thoughts?
Regards,
Boqun
On Fri, Jul 04, 2025 at 09:39:01AM -0700, Boqun Feng wrote:
> On Thu, Jul 03, 2025 at 04:30:01PM -0300, Daniel Almeida wrote:
> [...]
> > +#[pin_data]
> > +pub struct Registration<T: Handler + 'static> {
> > + #[pin]
> > + inner: Devres<RegistrationInner>,
> > +
> > + #[pin]
> > + handler: T,
>
> IIRC, as a certain point, we want this to be a `UnsafePinned<T>`, is
> that requirement gone or we still need that but 1) `UnsafePinned` is not
> available and 2) we can rely on the whole struct being !Unpin for the
> address stability temporarily?
>
> I think it was not a problem until we switched to `try_pin_init!()`
> instead of `pin_init_from_closure()` because we then had to pass the
> address of `handler` instead of the whole struct.
>
> Since we certainly want to use `try_pin_init!()` and we certainly will
> have `UnsafePinned`, I think we should just keep this as it is for now,
Of course the assumption is we want to it in before `UnsafePinned` ;-)
Alternatively we can do what `Devres` did:
https://lore.kernel.org/rust-for-linux/20250626200054.243480-4-dakr@kernel.org/
using an `Opaque` and manually drop for now.
Regards,
Boqun
> and add a TODO so that we can clean it up later when we have
> `UnsafePinned`?
>
> Thoughts?
>
> Regards,
> Boqun
On Fri, Jul 04, 2025 at 09:41:49AM -0700, Boqun Feng wrote:
> On Fri, Jul 04, 2025 at 09:39:01AM -0700, Boqun Feng wrote:
> > On Thu, Jul 03, 2025 at 04:30:01PM -0300, Daniel Almeida wrote:
> > [...]
> > > +#[pin_data]
> > > +pub struct Registration<T: Handler + 'static> {
> > > + #[pin]
> > > + inner: Devres<RegistrationInner>,
> > > +
> > > + #[pin]
> > > + handler: T,
> >
> > IIRC, as a certain point, we want this to be a `UnsafePinned<T>`, is
> > that requirement gone or we still need that but 1) `UnsafePinned` is not
> > available and 2) we can rely on the whole struct being !Unpin for the
> > address stability temporarily?
> >
> > I think it was not a problem until we switched to `try_pin_init!()`
> > instead of `pin_init_from_closure()` because we then had to pass the
> > address of `handler` instead of the whole struct.
> >
> > Since we certainly want to use `try_pin_init!()` and we certainly will
> > have `UnsafePinned`, I think we should just keep this as it is for now,
>
> Of course the assumption is we want to it in before `UnsafePinned` ;-)
> Alternatively we can do what `Devres` did:
>
> https://lore.kernel.org/rust-for-linux/20250626200054.243480-4-dakr@kernel.org/
>
> using an `Opaque` and manually drop for now.
The struct uses PhantomPinned, so the code is correct as-is. Using a
common abstraction for UnsafePinned is of course nice, but I suggest
that we keep it like this if both patches land in the same cycle. We can
always have it use UnsafePinned in a follow-up.
Alice
On Thu, Jul 03, 2025 at 04:30:01PM -0300, Daniel Almeida wrote:
> This patch adds support for non-threaded IRQs and handlers through
> irq::Registration and the irq::Handler trait.
>
> Registering an irq is dependent upon having a IrqRequest that was
> previously allocated by a given device. This will be introduced in
> subsequent patches.
>
> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
> ---
> rust/bindings/bindings_helper.h | 1 +
> rust/helpers/helpers.c | 1 +
> rust/helpers/irq.c | 9 ++
> rust/kernel/irq.rs | 5 +
> rust/kernel/irq/request.rs | 273 ++++++++++++++++++++++++++++++++++++++++
> 5 files changed, 289 insertions(+)
>
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index 8cbb660e2ec218021d16e6e0144acf6f4d7cca13..da0bd23fad59a2373bd873d12ad69c55208aaa38 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -51,6 +51,7 @@
> #include <linux/ethtool.h>
> #include <linux/file.h>
> #include <linux/firmware.h>
> +#include <linux/interrupt.h>
> #include <linux/fs.h>
> #include <linux/jiffies.h>
> #include <linux/jump_label.h>
> diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
> index 393ad201befb80a9ae39866a725744ab88620fbb..e3579fc7e1cfc30c913207a4a78b790259d7ae7a 100644
> --- a/rust/helpers/helpers.c
> +++ b/rust/helpers/helpers.c
> @@ -22,6 +22,7 @@
> #include "dma.c"
> #include "drm.c"
> #include "err.c"
> +#include "irq.c"
> #include "fs.c"
> #include "io.c"
> #include "jump_label.c"
> diff --git a/rust/helpers/irq.c b/rust/helpers/irq.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..1faca428e2c047a656dec3171855c1508d67e60b
> --- /dev/null
> +++ b/rust/helpers/irq.c
> @@ -0,0 +1,9 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/interrupt.h>
> +
> +int rust_helper_request_irq(unsigned int irq, irq_handler_t handler,
> + unsigned long flags, const char *name, void *dev)
> +{
> + return request_irq(irq, handler, flags, name, dev);
> +}
> diff --git a/rust/kernel/irq.rs b/rust/kernel/irq.rs
> index 9abd9a6dc36f3e3ecc1f92ad7b0040176b56a079..01bd08884b72c2a3a9460897bce751c732a19794 100644
> --- a/rust/kernel/irq.rs
> +++ b/rust/kernel/irq.rs
> @@ -12,3 +12,8 @@
>
> /// Flags to be used when registering IRQ handlers.
> pub mod flags;
> +
> +/// IRQ allocation and handling.
> +pub mod request;
> +
> +pub use request::{Handler, IrqRequest, IrqReturn, Registration};
> diff --git a/rust/kernel/irq/request.rs b/rust/kernel/irq/request.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..4f4beaa3c7887660440b9ddc52414020a0d165ac
> --- /dev/null
> +++ b/rust/kernel/irq/request.rs
> @@ -0,0 +1,273 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// SPDX-FileCopyrightText: Copyright 2025 Collabora ltd.
> +
> +//! This module provides types like [`Registration`] which allow users to
> +//! register handlers for a given IRQ line.
> +
> +use core::marker::PhantomPinned;
> +
> +use crate::alloc::Allocator;
> +use crate::device::Bound;
> +use crate::device::Device;
> +use crate::devres::Devres;
> +use crate::error::to_result;
> +use crate::irq::flags::Flags;
> +use crate::prelude::*;
> +use crate::str::CStr;
> +use crate::sync::Arc;
> +
> +/// The value that can be returned from an IrqHandler or a ThreadedIrqHandler.
> +pub enum IrqReturn {
> + /// The interrupt was not from this device or was not handled.
> + None,
> +
> + /// The interrupt was handled by this device.
> + Handled,
> +}
> +
> +impl IrqReturn {
> + fn into_inner(self) -> u32 {
> + match self {
> + IrqReturn::None => bindings::irqreturn_IRQ_NONE,
> + IrqReturn::Handled => bindings::irqreturn_IRQ_HANDLED,
One option is to specify these in the enum:
/// The value that can be returned from an IrqHandler or a ThreadedIrqHandler.
pub enum IrqReturn {
/// The interrupt was not from this device or was not handled.
None = bindings::irqreturn_IRQ_NONE,
/// The interrupt was handled by this device.
Handled = bindings::irqreturn_IRQ_HANDLED,
}
impl IrqReturn {
fn into_inner(self) -> c_uint {
self as c_uint
}
}
> +
> +/// Callbacks for an IRQ handler.
> +pub trait Handler: Sync {
> + /// The hard IRQ handler.
> + ///
> + /// This is executed in interrupt context, hence all corresponding
> + /// limitations do apply.
> + ///
> + /// All work that does not necessarily need to be executed from
> + /// interrupt context, should be deferred to a threaded handler.
> + /// See also [`ThreadedRegistration`].
> + fn handle(&self) -> IrqReturn;
> +}
> +
> +impl<T: ?Sized + Handler + Send> Handler for Arc<T> {
> + fn handle(&self) -> IrqReturn {
> + T::handle(self)
> + }
> +}
> +
> +impl<T: ?Sized + Handler, A: Allocator> Handler for Box<T, A> {
> + fn handle(&self) -> IrqReturn {
> + T::handle(self)
> + }
> +}
> +
> +/// # Invariants
> +///
> +/// - `self.irq` is the same as the one passed to `request_{threaded}_irq`.
> +/// - `cookie` was passed to `request_{threaded}_irq` as the cookie. It
> +/// is guaranteed to be unique by the type system, since each call to
> +/// `new` will return a different instance of `Registration`.
I recall there being a clippy lint about the indentation here. Did it
not trigger?
/// - `cookie` was passed to `request_{threaded}_irq` as the cookie. It
/// is guaranteed to be unique by the type system, since each call to
/// `new` will return a different instance of `Registration`.
> +#[pin_data(PinnedDrop)]
> +struct RegistrationInner {
> + irq: u32,
> + cookie: *mut kernel::ffi::c_void,
The c_void type is in the prelude.
> +}
> +
> +impl RegistrationInner {
> + fn synchronize(&self) {
> + // SAFETY: safe as per the invariants of `RegistrationInner`
> + unsafe { bindings::synchronize_irq(self.irq) };
> + }
> +}
> +
> +#[pinned_drop]
> +impl PinnedDrop for RegistrationInner {
> + fn drop(self: Pin<&mut Self>) {
> + // SAFETY:
> + //
> + // Safe as per the invariants of `RegistrationInner` and:
> + //
> + // - The containing struct is `!Unpin` and was initialized using
> + // pin-init, so it occupied the same memory location for the entirety of
> + // its lifetime.
> + //
> + // Notice that this will block until all handlers finish executing,
> + // i.e.: at no point will &self be invalid while the handler is running.
> + unsafe { bindings::free_irq(self.irq, self.cookie) };
> + }
> +}
> +
> +// SAFETY: We only use `inner` on drop, which called at most once with no
> +// concurrent access.
> +unsafe impl Sync for RegistrationInner {}
> +
> +// SAFETY: It is safe to send `RegistrationInner` across threads.
> +unsafe impl Send for RegistrationInner {}
> +
> +/// A request for an IRQ line for a given device.
> +///
> +/// # Invariants
> +///
> +/// - `ìrq` is the number of an interrupt source of `dev`.
> +/// - `irq` has not been registered yet.
> +pub struct IrqRequest<'a> {
> + dev: &'a Device<Bound>,
> + irq: u32,
> +}
> +
> +impl<'a> IrqRequest<'a> {
> + /// Creates a new IRQ request for the given device and IRQ number.
> + ///
> + /// # Safety
> + ///
> + /// - `irq` should be a valid IRQ number for `dev`.
> + pub(crate) unsafe fn new(dev: &'a Device<Bound>, irq: u32) -> Self {
> + IrqRequest { dev, irq }
> + }
> +}
> +
> +/// A registration of an IRQ handler for a given IRQ line.
> +///
> +/// # Examples
> +///
> +/// The following is an example of using `Registration`. It uses a
> +/// [`AtomicU32`](core::sync::AtomicU32) to provide the interior mutability.
> +///
> +/// ```
> +/// use core::sync::atomic::AtomicU32;
> +/// use core::sync::atomic::Ordering;
> +///
> +/// use kernel::prelude::*;
> +/// use kernel::device::Bound;
> +/// use kernel::irq::flags;
> +/// use kernel::irq::Registration;
> +/// use kernel::irq::IrqRequest;
> +/// use kernel::irq::IrqReturn;
> +/// use kernel::sync::Arc;
> +/// use kernel::c_str;
> +/// use kernel::alloc::flags::GFP_KERNEL;
> +///
> +/// // Declare a struct that will be passed in when the interrupt fires. The u32
> +/// // merely serves as an example of some internal data.
> +/// struct Data(AtomicU32);
> +///
> +/// // [`kernel::irq::request::Handler::handle`] takes `&self`. This example
> +/// // illustrates how interior mutability can be used when sharing the data
> +/// // between process context and IRQ context.
> +///
> +/// type Handler = Data;
> +///
> +/// impl kernel::irq::request::Handler for Handler {
> +/// // This is executing in IRQ context in some CPU. Other CPUs can still
> +/// // try to access to data.
> +/// fn handle(&self) -> IrqReturn {
> +/// self.0.fetch_add(1, Ordering::Relaxed);
> +///
> +/// IrqReturn::Handled
> +/// }
> +/// }
> +///
> +/// // Registers an IRQ handler for the given IrqRequest.
> +/// //
> +/// // This is executing in process context and assumes that `request` was
> +/// // previously acquired from a device.
> +/// fn register_irq(handler: Handler, request: IrqRequest<'_>) -> Result<Arc<Registration<Handler>>> {
> +/// let registration = Registration::new(request, flags::SHARED, c_str!("my_device"), handler);
> +///
> +/// let registration = Arc::pin_init(registration, GFP_KERNEL)?;
> +///
> +/// // The data can be accessed from process context too.
> +/// registration.handler().0.fetch_add(1, Ordering::Relaxed);
> +///
> +/// Ok(registration)
> +/// }
> +/// # Ok::<(), Error>(())
> +/// ```
> +///
> +/// # Invariants
> +///
> +/// * We own an irq handler using `&self.handler` as its private data.
> +///
> +#[pin_data]
> +pub struct Registration<T: Handler + 'static> {
> + #[pin]
> + inner: Devres<RegistrationInner>,
> +
> + #[pin]
> + handler: T,
> +
> + /// Pinned because we need address stability so that we can pass a pointer
> + /// to the callback.
> + #[pin]
> + _pin: PhantomPinned,
> +}
> +
> +impl<T: Handler + 'static> Registration<T> {
> + /// Registers the IRQ handler with the system for the given IRQ number.
> + pub fn new<'a>(
> + request: IrqRequest<'a>,
> + flags: Flags,
> + name: &'static CStr,
> + handler: T,
> + ) -> impl PinInit<Self, Error> + 'a {
> + try_pin_init!(&this in Self {
> + handler,
> + inner <- Devres::new(
> + request.dev,
> + try_pin_init!(RegistrationInner {
> + // SAFETY: `this` is a valid pointer to the `Registration` instance
> + cookie: unsafe { &raw mut (*this.as_ptr()).handler }.cast(),
> + irq: {
> + // SAFETY:
> + // - The callbacks are valid for use with request_irq.
> + // - If this succeeds, the slot is guaranteed to be valid until the
> + // destructor of Self runs, which will deregister the callbacks
> + // before the memory location becomes invalid.
> + to_result(unsafe {
> + bindings::request_irq(
> + request.irq,
> + Some(handle_irq_callback::<T>),
> + flags.into_inner() as usize,
> + name.as_char_ptr(),
> + (&raw mut (*this.as_ptr()).handler).cast(),
> + )
> + })?;
> + request.irq
> + }
> + })
> + ),
> + _pin: PhantomPinned,
> + })
> + }
> +
> + /// Returns a reference to the handler that was registered with the system.
> + pub fn handler(&self) -> &T {
> + &self.handler
> + }
> +
> + /// Wait for pending IRQ handlers on other CPUs.
> + ///
> + /// This will attempt to access the inner [`Devres`] container.
> + pub fn try_synchronize(&self) -> Result {
> + let inner = self.inner.try_access().ok_or(ENODEV)?;
> + inner.synchronize();
> + Ok(())
> + }
> +
> + /// Wait for pending IRQ handlers on other CPUs.
> + pub fn synchronize(&self, dev: &Device<Bound>) -> Result {
> + let inner = self.inner.access(dev)?;
> + inner.synchronize();
> + Ok(())
> + }
> +}
> +
> +/// # Safety
> +///
> +/// This function should be only used as the callback in `request_irq`.
> +unsafe extern "C" fn handle_irq_callback<T: Handler>(
> + _irq: i32,
> + ptr: *mut core::ffi::c_void,
> +) -> core::ffi::c_uint {
You should just use `c_uint` without the prefix. This way you get it
from `kernel::prelude::*` which has the correct typedefs rather than
`core::ffi`.
> + // SAFETY: `ptr` is a pointer to T set in `Registration::new`
> + let handler = unsafe { &*(ptr as *const T) };
> + T::handle(handler).into_inner()
> +}
>
> --
> 2.50.0
>
Alice,
[…]
>> +/// The value that can be returned from an IrqHandler or a ThreadedIrqHandler.
>> +pub enum IrqReturn {
>> + /// The interrupt was not from this device or was not handled.
>> + None,
>> +
>> + /// The interrupt was handled by this device.
>> + Handled,
>> +}
>> +
>> +impl IrqReturn {
>> + fn into_inner(self) -> u32 {
>> + match self {
>> + IrqReturn::None => bindings::irqreturn_IRQ_NONE,
>> + IrqReturn::Handled => bindings::irqreturn_IRQ_HANDLED,
>
> One option is to specify these in the enum:
>
> /// The value that can be returned from an IrqHandler or a ThreadedIrqHandler.
> pub enum IrqReturn {
> /// The interrupt was not from this device or was not handled.
> None = bindings::irqreturn_IRQ_NONE,
>
> /// The interrupt was handled by this device.
> Handled = bindings::irqreturn_IRQ_HANDLED,
> }
This requires explicitly setting #[repr(u32)], which is something that was
reverted at an earlier iteration of the series on Benno’s request.
— Daniel
On Mon Jul 7, 2025 at 6:18 PM CEST, Daniel Almeida wrote:
> Alice,
>
> […]
>
>>> +/// The value that can be returned from an IrqHandler or a ThreadedIrqHandler.
>>> +pub enum IrqReturn {
>>> + /// The interrupt was not from this device or was not handled.
>>> + None,
>>> +
>>> + /// The interrupt was handled by this device.
>>> + Handled,
>>> +}
>>> +
>>> +impl IrqReturn {
>>> + fn into_inner(self) -> u32 {
>>> + match self {
>>> + IrqReturn::None => bindings::irqreturn_IRQ_NONE,
>>> + IrqReturn::Handled => bindings::irqreturn_IRQ_HANDLED,
>>
>> One option is to specify these in the enum:
>>
>> /// The value that can be returned from an IrqHandler or a ThreadedIrqHandler.
>> pub enum IrqReturn {
>> /// The interrupt was not from this device or was not handled.
>> None = bindings::irqreturn_IRQ_NONE,
>>
>> /// The interrupt was handled by this device.
>> Handled = bindings::irqreturn_IRQ_HANDLED,
>> }
>
> This requires explicitly setting #[repr(u32)], which is something that was
> reverted at an earlier iteration of the series on Benno’s request.
Yeah I requested this, because it increases the size of the enum to 4
bytes and I think we should try to make rust enums as small as possible.
@Alice what's the benefit of doing it directly in the enum?
---
Cheers,
Benno
On Mon, Jul 07, 2025 at 10:30:27PM +0200, Benno Lossin wrote:
> On Mon Jul 7, 2025 at 6:18 PM CEST, Daniel Almeida wrote:
> > Alice,
> >
> > […]
> >
> >>> +/// The value that can be returned from an IrqHandler or a ThreadedIrqHandler.
> >>> +pub enum IrqReturn {
> >>> + /// The interrupt was not from this device or was not handled.
> >>> + None,
> >>> +
> >>> + /// The interrupt was handled by this device.
> >>> + Handled,
> >>> +}
> >>> +
> >>> +impl IrqReturn {
> >>> + fn into_inner(self) -> u32 {
> >>> + match self {
> >>> + IrqReturn::None => bindings::irqreturn_IRQ_NONE,
> >>> + IrqReturn::Handled => bindings::irqreturn_IRQ_HANDLED,
> >>
> >> One option is to specify these in the enum:
> >>
> >> /// The value that can be returned from an IrqHandler or a ThreadedIrqHandler.
> >> pub enum IrqReturn {
> >> /// The interrupt was not from this device or was not handled.
> >> None = bindings::irqreturn_IRQ_NONE,
> >>
> >> /// The interrupt was handled by this device.
> >> Handled = bindings::irqreturn_IRQ_HANDLED,
> >> }
> >
> > This requires explicitly setting #[repr(u32)], which is something that was
> > reverted at an earlier iteration of the series on Benno’s request.
>
> Yeah I requested this, because it increases the size of the enum to 4
> bytes and I think we should try to make rust enums as small as possible.
>
> @Alice what's the benefit of doing it directly in the enum?
Basically all uses of this enum are going to look like this:
fn inner() -> IrqReturn {
if !should_handle() {
return IrqReturn::None;
}
// .. handle the irq
IrqReturn::Handled
}
fn outer() -> u32 {
match inner() {
IrqReturn::None => bindings::irqreturn_IRQ_NONE,
IrqReturn::Handled => bindings::irqreturn_IRQ_HANDLED,
}
}
Setting the discriminant to the values ensures that the match in outer()
is a no-op. The enum is never going to be stored long-term anywhere so
its size doesn't matter.
Alice
On Tue Jul 8, 2025 at 1:49 PM CEST, Alice Ryhl wrote:
> On Mon, Jul 07, 2025 at 10:30:27PM +0200, Benno Lossin wrote:
>> On Mon Jul 7, 2025 at 6:18 PM CEST, Daniel Almeida wrote:
>> > Alice,
>> >
>> > […]
>> >
>> >>> +/// The value that can be returned from an IrqHandler or a ThreadedIrqHandler.
>> >>> +pub enum IrqReturn {
>> >>> + /// The interrupt was not from this device or was not handled.
>> >>> + None,
>> >>> +
>> >>> + /// The interrupt was handled by this device.
>> >>> + Handled,
>> >>> +}
>> >>> +
>> >>> +impl IrqReturn {
>> >>> + fn into_inner(self) -> u32 {
>> >>> + match self {
>> >>> + IrqReturn::None => bindings::irqreturn_IRQ_NONE,
>> >>> + IrqReturn::Handled => bindings::irqreturn_IRQ_HANDLED,
>> >>
>> >> One option is to specify these in the enum:
>> >>
>> >> /// The value that can be returned from an IrqHandler or a ThreadedIrqHandler.
>> >> pub enum IrqReturn {
>> >> /// The interrupt was not from this device or was not handled.
>> >> None = bindings::irqreturn_IRQ_NONE,
>> >>
>> >> /// The interrupt was handled by this device.
>> >> Handled = bindings::irqreturn_IRQ_HANDLED,
>> >> }
>> >
>> > This requires explicitly setting #[repr(u32)], which is something that was
>> > reverted at an earlier iteration of the series on Benno’s request.
>>
>> Yeah I requested this, because it increases the size of the enum to 4
>> bytes and I think we should try to make rust enums as small as possible.
>>
>> @Alice what's the benefit of doing it directly in the enum?
>
> Basically all uses of this enum are going to look like this:
>
> fn inner() -> IrqReturn {
> if !should_handle() {
> return IrqReturn::None;
> }
> // .. handle the irq
> IrqReturn::Handled
> }
>
> fn outer() -> u32 {
> match inner() {
> IrqReturn::None => bindings::irqreturn_IRQ_NONE,
> IrqReturn::Handled => bindings::irqreturn_IRQ_HANDLED,
> }
> }
>
> Setting the discriminant to the values ensures that the match in outer()
> is a no-op. The enum is never going to be stored long-term anywhere so
> its size doesn't matter.
Hmm in this particular case, I think the optimizer will be able to
remove the additional branch too. But I haven't checked.
I usually give the advice to not explicitly set the discriminants and
let the compiler do it. I don't have a strong opinion on this, but we
should figure out which one is better in which cases.
---
Cheers,
Benno
© 2016 - 2025 Red Hat, Inc.