From: Alice Ryhl <aliceryhl@google.com>
When working with a bus device, many operations are only possible while
the device is still bound. The &Device<Bound> type represents a proof in
the type system that you are in a scope where the device is guaranteed
to still be bound. Since we deregister irq callbacks when unbinding a
device, if an irq callback is running, that implies that the device has
not yet been unbound.
To allow drivers to take advantage of that, add an additional argument
to irq callbacks.
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
rust/kernel/irq/request.rs | 95 +++++++++++++++++++++++++++-------------------
1 file changed, 57 insertions(+), 38 deletions(-)
diff --git a/rust/kernel/irq/request.rs b/rust/kernel/irq/request.rs
index 4033df7d0dce1dbc9cc9b0b0f32fb9f8aa285d6b..b150563fdef809899c7ca39221c4928238e31110 100644
--- a/rust/kernel/irq/request.rs
+++ b/rust/kernel/irq/request.rs
@@ -36,18 +36,18 @@ pub trait Handler: Sync {
/// All work that does not necessarily need to be executed from
/// interrupt context, should be deferred to a threaded handler.
/// See also [`ThreadedRegistration`].
- fn handle(&self) -> IrqReturn;
+ fn handle(&self, device: &Device<Bound>) -> IrqReturn;
}
impl<T: ?Sized + Handler + Send> Handler for Arc<T> {
- fn handle(&self) -> IrqReturn {
- T::handle(self)
+ fn handle(&self, device: &Device<Bound>) -> IrqReturn {
+ T::handle(self, device)
}
}
impl<T: ?Sized + Handler, A: Allocator> Handler for Box<T, A> {
- fn handle(&self) -> IrqReturn {
- T::handle(self)
+ fn handle(&self, device: &Device<Bound>) -> IrqReturn {
+ T::handle(self, device)
}
}
@@ -140,7 +140,7 @@ pub fn irq(&self) -> u32 {
///
/// ```
/// use kernel::c_str;
-/// use kernel::device::Bound;
+/// use kernel::device::{Bound, Device};
/// use kernel::irq::{self, Flags, IrqRequest, IrqReturn, Registration};
/// use kernel::prelude::*;
/// use kernel::sync::{Arc, Completion};
@@ -154,7 +154,7 @@ pub fn irq(&self) -> u32 {
///
/// impl irq::Handler for Data {
/// // Executed in IRQ context.
-/// fn handle(&self) -> IrqReturn {
+/// fn handle(&self, _dev: &Device<Bound>) -> IrqReturn {
/// self.completion.complete_all();
/// IrqReturn::Handled
/// }
@@ -180,7 +180,7 @@ pub fn irq(&self) -> u32 {
///
/// # Invariants
///
-/// * We own an irq handler using `&self.handler` as its private data.
+/// * We own an irq handler whose cookie is a pointer to `Self`.
#[pin_data]
pub struct Registration<T: Handler + 'static> {
#[pin]
@@ -208,21 +208,24 @@ pub fn new<'a>(
inner <- Devres::new(
request.dev,
try_pin_init!(RegistrationInner {
- // SAFETY: `this` is a valid pointer to the `Registration` instance
- cookie: unsafe { &raw mut (*this.as_ptr()).handler }.cast(),
+ // INVARIANT: `this` is a valid pointer to the `Registration` instance
+ cookie: this.as_ptr().cast::<c_void>(),
irq: {
// SAFETY:
// - The callbacks are valid for use with request_irq.
// - If this succeeds, the slot is guaranteed to be valid until the
// destructor of Self runs, which will deregister the callbacks
// before the memory location becomes invalid.
+ // - When request_irq is called, everything that handle_irq_callback will
+ // touch has already been initialized, so it's safe for the callback to
+ // be called immediately.
to_result(unsafe {
bindings::request_irq(
request.irq,
Some(handle_irq_callback::<T>),
flags.into_inner(),
name.as_char_ptr(),
- (&raw mut (*this.as_ptr()).handler).cast(),
+ this.as_ptr().cast::<c_void>(),
)
})?;
request.irq
@@ -259,9 +262,13 @@ pub fn synchronize(&self, dev: &Device<Bound>) -> Result {
///
/// This function should be only used as the callback in `request_irq`.
unsafe extern "C" fn handle_irq_callback<T: Handler>(_irq: i32, ptr: *mut c_void) -> c_uint {
- // SAFETY: `ptr` is a pointer to T set in `Registration::new`
- let handler = unsafe { &*(ptr as *const T) };
- T::handle(handler) as c_uint
+ // SAFETY: `ptr` is a pointer to `Registration<T>` set in `Registration::new`
+ let registration = unsafe { &*(ptr as *const Registration<T>) };
+ // SAFETY: The irq callback is removed before the device is unbound, so the fact that the irq
+ // callback is running implies that the device has not yet been unbound.
+ let device = unsafe { registration.inner.device().as_bound() };
+
+ T::handle(®istration.handler, device) as c_uint
}
/// The value that can be returned from [`ThreadedHandler::handle`].
@@ -287,7 +294,8 @@ pub trait ThreadedHandler: Sync {
/// handler, i.e. [`ThreadedHandler::handle_threaded`].
///
/// The default implementation returns [`ThreadedIrqReturn::WakeThread`].
- fn handle(&self) -> ThreadedIrqReturn {
+ #[expect(unused_variables)]
+ fn handle(&self, device: &Device<Bound>) -> ThreadedIrqReturn {
ThreadedIrqReturn::WakeThread
}
@@ -295,26 +303,26 @@ fn handle(&self) -> ThreadedIrqReturn {
///
/// This is executed in process context. The kernel creates a dedicated
/// `kthread` for this purpose.
- fn handle_threaded(&self) -> IrqReturn;
+ fn handle_threaded(&self, device: &Device<Bound>) -> IrqReturn;
}
impl<T: ?Sized + ThreadedHandler + Send> ThreadedHandler for Arc<T> {
- fn handle(&self) -> ThreadedIrqReturn {
- T::handle(self)
+ fn handle(&self, device: &Device<Bound>) -> ThreadedIrqReturn {
+ T::handle(self, device)
}
- fn handle_threaded(&self) -> IrqReturn {
- T::handle_threaded(self)
+ fn handle_threaded(&self, device: &Device<Bound>) -> IrqReturn {
+ T::handle_threaded(self, device)
}
}
impl<T: ?Sized + ThreadedHandler, A: Allocator> ThreadedHandler for Box<T, A> {
- fn handle(&self) -> ThreadedIrqReturn {
- T::handle(self)
+ fn handle(&self, device: &Device<Bound>) -> ThreadedIrqReturn {
+ T::handle(self, device)
}
- fn handle_threaded(&self) -> IrqReturn {
- T::handle_threaded(self)
+ fn handle_threaded(&self, device: &Device<Bound>) -> IrqReturn {
+ T::handle_threaded(self, device)
}
}
@@ -333,7 +341,7 @@ fn handle_threaded(&self) -> IrqReturn {
///
/// ```
/// use kernel::c_str;
-/// use kernel::device::Bound;
+/// use kernel::device::{Bound, Device};
/// use kernel::irq::{
/// self, Flags, IrqRequest, IrqReturn, ThreadedHandler, ThreadedIrqReturn,
/// ThreadedRegistration,
@@ -357,7 +365,7 @@ fn handle_threaded(&self) -> IrqReturn {
/// // This will run (in a separate kthread) if and only if
/// // [`ThreadedHandler::handle`] returns [`WakeThread`], which it does by
/// // default.
-/// fn handle_threaded(&self) -> IrqReturn {
+/// fn handle_threaded(&self, _dev: &Device<Bound>) -> IrqReturn {
/// let mut data = self.value.lock();
/// *data += 1;
/// IrqReturn::Handled
@@ -390,7 +398,7 @@ fn handle_threaded(&self) -> IrqReturn {
///
/// # Invariants
///
-/// * We own an irq handler using `&T` as its private data.
+/// * We own an irq handler whose cookie is a pointer to `Self`.
#[pin_data]
pub struct ThreadedRegistration<T: ThreadedHandler + 'static> {
#[pin]
@@ -418,14 +426,17 @@ pub fn new<'a>(
inner <- Devres::new(
request.dev,
try_pin_init!(RegistrationInner {
- // SAFETY: `this` is a valid pointer to the `ThreadedRegistration` instance.
- cookie: unsafe { &raw mut (*this.as_ptr()).handler }.cast(),
+ // INVARIANT: `this` is a valid pointer to the `ThreadedRegistration` instance.
+ cookie: this.as_ptr().cast::<c_void>(),
irq: {
// SAFETY:
// - The callbacks are valid for use with request_threaded_irq.
// - If this succeeds, the slot is guaranteed to be valid until the
- // destructor of Self runs, which will deregister the callbacks
- // before the memory location becomes invalid.
+ // destructor of Self runs, which will deregister the callbacks
+ // before the memory location becomes invalid.
+ // - When request_threaded_irq is called, everything that the two callbacks
+ // will touch has already been initialized, so it's safe for the
+ // callbacks to be called immediately.
to_result(unsafe {
bindings::request_threaded_irq(
request.irq,
@@ -433,7 +444,7 @@ pub fn new<'a>(
Some(thread_fn_callback::<T>),
flags.into_inner(),
name.as_char_ptr(),
- (&raw mut (*this.as_ptr()).handler).cast(),
+ this.as_ptr().cast::<c_void>(),
)
})?;
request.irq
@@ -473,16 +484,24 @@ pub fn synchronize(&self, dev: &Device<Bound>) -> Result {
_irq: i32,
ptr: *mut c_void,
) -> c_uint {
- // SAFETY: `ptr` is a pointer to T set in `ThreadedRegistration::new`
- let handler = unsafe { &*(ptr as *const T) };
- T::handle(handler) as c_uint
+ // SAFETY: `ptr` is a pointer to `ThreadedRegistration<T>` set in `ThreadedRegistration::new`
+ let registration = unsafe { &*(ptr as *const ThreadedRegistration<T>) };
+ // SAFETY: The irq callback is removed before the device is unbound, so the fact that the irq
+ // callback is running implies that the device has not yet been unbound.
+ let device = unsafe { registration.inner.device().as_bound() };
+
+ T::handle(®istration.handler, device) as c_uint
}
/// # Safety
///
/// This function should be only used as the callback in `request_threaded_irq`.
unsafe extern "C" fn thread_fn_callback<T: ThreadedHandler>(_irq: i32, ptr: *mut c_void) -> c_uint {
- // SAFETY: `ptr` is a pointer to T set in `ThreadedRegistration::new`
- let handler = unsafe { &*(ptr as *const T) };
- T::handle_threaded(handler) as c_uint
+ // SAFETY: `ptr` is a pointer to `ThreadedRegistration<T>` set in `ThreadedRegistration::new`
+ let registration = unsafe { &*(ptr as *const ThreadedRegistration<T>) };
+ // SAFETY: The irq callback is removed before the device is unbound, so the fact that the irq
+ // callback is running implies that the device has not yet been unbound.
+ let device = unsafe { registration.inner.device().as_bound() };
+
+ T::handle_threaded(®istration.handler, device) as c_uint
}
--
2.50.1> On 11 Aug 2025, at 13:03, Daniel Almeida <daniel.almeida@collabora.com> wrote: > > From: Alice Ryhl <aliceryhl@google.com> > > When working with a bus device, many operations are only possible while > the device is still bound. The &Device<Bound> type represents a proof in > the type system that you are in a scope where the device is guaranteed > to still be bound. Since we deregister irq callbacks when unbinding a > device, if an irq callback is running, that implies that the device has > not yet been unbound. > > To allow drivers to take advantage of that, add an additional argument > to irq callbacks. > > Signed-off-by: Alice Ryhl <aliceryhl@google.com> Sorry. I forgot to add my SOB here. Perhaps this can be added when the patch is being applied in order to cut down on the number of versions, and therefore avoid the extra noise? Otherwise let me know. Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
On Mon Aug 11, 2025 at 7:00 PM CEST, Daniel Almeida wrote: > > >> On 11 Aug 2025, at 13:03, Daniel Almeida <daniel.almeida@collabora.com> wrote: >> >> From: Alice Ryhl <aliceryhl@google.com> >> >> When working with a bus device, many operations are only possible while >> the device is still bound. The &Device<Bound> type represents a proof in >> the type system that you are in a scope where the device is guaranteed >> to still be bound. Since we deregister irq callbacks when unbinding a >> device, if an irq callback is running, that implies that the device has >> not yet been unbound. >> >> To allow drivers to take advantage of that, add an additional argument >> to irq callbacks. >> >> Signed-off-by: Alice Ryhl <aliceryhl@google.com> > > Sorry. I forgot to add my SOB here. > > > Perhaps this can be added when the patch is being applied in order to cut down on the > number of versions, and therefore avoid the extra noise? Otherwise let me know. > > Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com> Sure -- no need to resubmit AFAIC.
On Mon, Aug 11, 2025 at 02:00:47PM -0300, Daniel Almeida wrote: > > > > On 11 Aug 2025, at 13:03, Daniel Almeida <daniel.almeida@collabora.com> wrote: > > > > From: Alice Ryhl <aliceryhl@google.com> > > > > When working with a bus device, many operations are only possible while > > the device is still bound. The &Device<Bound> type represents a proof in > > the type system that you are in a scope where the device is guaranteed > > to still be bound. Since we deregister irq callbacks when unbinding a > > device, if an irq callback is running, that implies that the device has > > not yet been unbound. > > > > To allow drivers to take advantage of that, add an additional argument > > to irq callbacks. > > > > Signed-off-by: Alice Ryhl <aliceryhl@google.com> > > Sorry. I forgot to add my SOB here. > > > Perhaps this can be added when the patch is being applied in order to cut down on the > number of versions, and therefore avoid the extra noise? Otherwise let me know. > I think it's fine to submit with only Alice's SoB, my understanding is that you won't necessarily need to add your SoB if you are simply re-submitting a patch (with minor changes). There are changes where your SoB is needed: 1) you change the code significantly, in which case, you may also need to add "Co-Developed-by" for Alice as well; 2) you're submitting the patch as a maintainer, and you have queued that patch already somewhere in your tree, in this case SoB shows how the patch flows around. Of course, either case it's better to sync with Alice first, which I believe you have already done that. While I'm at it, Reviewed-by: Boqun Feng <boqun.feng@gmail.com> Regards, Boqun > Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com> > >
On Mon Aug 11, 2025 at 7:11 PM CEST, Boqun Feng wrote: > I think it's fine to submit with only Alice's SoB, my understanding is > that you won't necessarily need to add your SoB if you are simply > re-submitting a patch (with minor changes). I think it is required for everyone who is handling or transporting the patch in any way, documenting the exact route a patch has taking while conforming with the developer’s certificate of origin.
© 2016 - 2025 Red Hat, Inc.