rust-for-linux.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] rust: irq: add &Device<Bound> argument to irq callbacks
@ 2025-07-21 14:38 Alice Ryhl
  2025-07-21 19:13 ` Daniel Almeida
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Alice Ryhl @ 2025-07-21 14:38 UTC (permalink / raw)
  To: Daniel Almeida, Miguel Ojeda, Boqun Feng, Gary Guo,
	Björn Roy Baron, Andreas Hindborg, Trevor Gross,
	Danilo Krummrich, Greg Kroah-Hartman, Rafael J. Wysocki,
	Thomas Gleixner, Bjorn Helgaas, Krzysztof Wilczyński,
	Benno Lossin, Dirk Behme
  Cc: linux-kernel, rust-for-linux, linux-pci, Alice Ryhl

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>
---
This patch is a follow-up to Daniel's irq series [1] that adds a
&Device<Bound> argument to all irq callbacks. This allows you to use
operations that are only safe on a bound device inside an irq callback.

The patch is otherwise based on top of driver-core-next.

[1]: https://lore.kernel.org/r/20250715-topics-tyr-request_irq2-v7-0-d469c0f37c07@collabora.com
---
 rust/kernel/irq/request.rs | 88 ++++++++++++++++++++++++++--------------------
 1 file changed, 49 insertions(+), 39 deletions(-)

diff --git a/rust/kernel/irq/request.rs b/rust/kernel/irq/request.rs
index d070ddabd37e7806f76edefd5d2ad46524be620e..f99aff2dd479f5223c90f0d2694f57e6c864bdb5 100644
--- a/rust/kernel/irq/request.rs
+++ b/rust/kernel/irq/request.rs
@@ -37,18 +37,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)
     }
 }
 
@@ -134,7 +134,7 @@ pub fn irq(&self) -> u32 {
 /// use core::sync::atomic::Ordering;
 ///
 /// use kernel::prelude::*;
-/// use kernel::device::Bound;
+/// use kernel::device::{Bound, Device};
 /// use kernel::irq::flags::Flags;
 /// use kernel::irq::Registration;
 /// use kernel::irq::IrqRequest;
@@ -156,7 +156,7 @@ pub fn irq(&self) -> u32 {
 /// 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 {
+///     fn handle(&self, _dev: &Device<Bound>) -> IrqReturn {
 ///         self.0.fetch_add(1, Ordering::Relaxed);
 ///
 ///         IrqReturn::Handled
@@ -182,8 +182,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]
@@ -211,8 +210,8 @@ 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.
@@ -225,7 +224,7 @@ pub fn new<'a>(
                                 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
@@ -262,9 +261,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(&registration.handler, device) as c_uint
 }
 
 /// The value that can be returned from `ThreadedHandler::handle_irq`.
@@ -288,32 +291,32 @@ pub trait ThreadedHandler: Sync {
     /// limitations do apply. All work that does not necessarily need to be
     /// executed from interrupt context, should be deferred to the threaded
     /// handler, i.e. [`ThreadedHandler::handle_threaded`].
-    fn handle(&self) -> ThreadedIrqReturn;
+    fn handle(&self, device: &Device<Bound>) -> ThreadedIrqReturn;
 
     /// The threaded IRQ handler.
     ///
     /// This is executed in process context. The kernel creates a dedicated
     /// kthread for this purpose.
-    fn handle_threaded(&self) -> IrqReturn;
+    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)
     }
 }
 
@@ -334,7 +337,7 @@ fn handle_threaded(&self) -> IrqReturn {
 /// use core::sync::atomic::Ordering;
 ///
 /// use kernel::prelude::*;
-/// use kernel::device::Bound;
+/// use kernel::device::{Bound, Device};
 /// use kernel::irq::flags::Flags;
 /// use kernel::irq::ThreadedIrqReturn;
 /// use kernel::irq::ThreadedRegistration;
@@ -356,7 +359,7 @@ fn handle_threaded(&self) -> IrqReturn {
 /// impl kernel::irq::request::ThreadedHandler for Handler {
 ///     // This is executing in IRQ context in some CPU. Other CPUs can still
 ///     // try to access the data.
-///     fn handle(&self) -> ThreadedIrqReturn {
+///     fn handle(&self, _dev: &Device<Bound>) -> ThreadedIrqReturn {
 ///         self.0.fetch_add(1, Ordering::Relaxed);
 ///         // By returning `WakeThread`, we indicate to the system that the
 ///         // thread function should be called. Otherwise, return
@@ -366,7 +369,7 @@ fn handle_threaded(&self) -> IrqReturn {
 ///
 ///     // This will run (in a separate kthread) if and only if `handle`
 ///     // returns `WakeThread`.
-///     fn handle_threaded(&self) -> IrqReturn {
+///     fn handle_threaded(&self, _dev: &Device<Bound>) -> IrqReturn {
 ///         self.0.fetch_add(1, Ordering::Relaxed);
 ///         IrqReturn::Handled
 ///     }
@@ -391,8 +394,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]
@@ -420,8 +422,8 @@ 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.
@@ -435,7 +437,7 @@ pub fn new<'a>(
                                 Some(thread_fn_callback::<T>),
                                 flags.into_inner() as usize,
                                 name.as_char_ptr(),
-                                (&raw mut (*this.as_ptr()).handler).cast(),
+                                this.as_ptr().cast::<c_void>(),
                             )
                         })?;
                         request.irq
@@ -475,16 +477,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(&registration.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(&registration.handler, device) as c_uint
 }

---
base-commit: d860d29e91be18de62b0f441edee7d00f6cb4972
change-id: 20250721-irq-bound-device-c9fdbfdd8cd9

Best regards,
-- 
Alice Ryhl <aliceryhl@google.com>


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] rust: irq: add &Device<Bound> argument to irq callbacks
  2025-07-21 14:38 [PATCH] rust: irq: add &Device<Bound> argument to irq callbacks Alice Ryhl
@ 2025-07-21 19:13 ` Daniel Almeida
  2025-07-21 19:33   ` Alice Ryhl
  2025-07-22 11:47 ` Dirk Behme
  2025-08-11  0:39 ` Daniel Almeida
  2 siblings, 1 reply; 7+ messages in thread
From: Daniel Almeida @ 2025-07-21 19:13 UTC (permalink / raw)
  To: Alice Ryhl
  Cc: Miguel Ojeda, Boqun Feng, Gary Guo, Björn Roy Baron,
	Andreas Hindborg, Trevor Gross, Danilo Krummrich,
	Greg Kroah-Hartman, Rafael J. Wysocki, Thomas Gleixner,
	Bjorn Helgaas, Krzysztof Wilczyński, Benno Lossin,
	Dirk Behme, linux-kernel, rust-for-linux, linux-pci

Alice,

> On 21 Jul 2025, at 11:38, Alice Ryhl <aliceryhl@google.com> wrote:
> 
> 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>
> ---
> This patch is a follow-up to Daniel's irq series [1] that adds a
> &Device<Bound> argument to all irq callbacks. This allows you to use
> operations that are only safe on a bound device inside an irq callback.
> 
> The patch is otherwise based on top of driver-core-next.
> 
> [1]: https://lore.kernel.org/r/20250715-topics-tyr-request_irq2-v7-0-d469c0f37c07@collabora.com

I am having a hard time applying this locally.

> ///
> /// 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() };

Where was this function introduced? i.e. I am missing the change that brought
in RegistrationInner::device(), or maybe some Deref impl that would make this
possible?

Also, I wonder if we can't make the scope of this unsafe block smaller?

— Daniel



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] rust: irq: add &Device<Bound> argument to irq callbacks
  2025-07-21 19:13 ` Daniel Almeida
@ 2025-07-21 19:33   ` Alice Ryhl
  2025-08-11  0:49     ` Daniel Almeida
  0 siblings, 1 reply; 7+ messages in thread
From: Alice Ryhl @ 2025-07-21 19:33 UTC (permalink / raw)
  To: Daniel Almeida
  Cc: Miguel Ojeda, Boqun Feng, Gary Guo, Björn Roy Baron,
	Andreas Hindborg, Trevor Gross, Danilo Krummrich,
	Greg Kroah-Hartman, Rafael J. Wysocki, Thomas Gleixner,
	Bjorn Helgaas, Krzysztof Wilczyński, Benno Lossin,
	Dirk Behme, linux-kernel, rust-for-linux, linux-pci

On Mon, Jul 21, 2025 at 9:14 PM Daniel Almeida
<daniel.almeida@collabora.com> wrote:
>
> Alice,
>
> > On 21 Jul 2025, at 11:38, Alice Ryhl <aliceryhl@google.com> wrote:
> >
> > 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>
> > ---
> > This patch is a follow-up to Daniel's irq series [1] that adds a
> > &Device<Bound> argument to all irq callbacks. This allows you to use
> > operations that are only safe on a bound device inside an irq callback.
> >
> > The patch is otherwise based on top of driver-core-next.
> >
> > [1]: https://lore.kernel.org/r/20250715-topics-tyr-request_irq2-v7-0-d469c0f37c07@collabora.com
>
> I am having a hard time applying this locally.

Your irq series currently doesn't apply cleanly on top of
driver-core-next and requires resolving a minor conflict. You can find
the commits here:
https://github.com/Darksonn/linux/commits/sent/20250721-irq-bound-device-c9fdbfdd8cd9-v1/

> > ///
> > /// 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() };
>
> Where was this function introduced? i.e. I am missing the change that brought
> in RegistrationInner::device(), or maybe some Deref impl that would make this
> possible?

In this series:
https://lore.kernel.org/all/20250713182737.64448-2-dakr@kernel.org/

> Also, I wonder if we can't make the scope of this unsafe block smaller?

I guess we could with an extra `let` statement.

Alice

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] rust: irq: add &Device<Bound> argument to irq callbacks
  2025-07-21 14:38 [PATCH] rust: irq: add &Device<Bound> argument to irq callbacks Alice Ryhl
  2025-07-21 19:13 ` Daniel Almeida
@ 2025-07-22 11:47 ` Dirk Behme
  2025-08-11  0:39 ` Daniel Almeida
  2 siblings, 0 replies; 7+ messages in thread
From: Dirk Behme @ 2025-07-22 11:47 UTC (permalink / raw)
  To: Alice Ryhl, Daniel Almeida, Miguel Ojeda, Boqun Feng, Gary Guo,
	Björn Roy Baron, Andreas Hindborg, Trevor Gross,
	Danilo Krummrich, Greg Kroah-Hartman, Rafael J. Wysocki,
	Thomas Gleixner, Bjorn Helgaas, Krzysztof Wilczyński,
	Benno Lossin, Dirk Behme
  Cc: linux-kernel, rust-for-linux, linux-pci

On 21/07/2025 16:38, Alice Ryhl wrote:
> 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>


With

https://lore.kernel.org/rust-for-linux/dd34e5f4-5027-4096-9f32-129c8a067d0a@de.bosch.com/

let me add

Tested-by: Dirk Behme <dirk.behme@de.bosch.com>

here as well.

Thanks!

Dirk


> ---
> This patch is a follow-up to Daniel's irq series [1] that adds a
> &Device<Bound> argument to all irq callbacks. This allows you to use
> operations that are only safe on a bound device inside an irq callback.
> 
> The patch is otherwise based on top of driver-core-next.
> 
> [1]: https://lore.kernel.org/r/20250715-topics-tyr-request_irq2-v7-0-d469c0f37c07@collabora.com
> ---
>  rust/kernel/irq/request.rs | 88 ++++++++++++++++++++++++++--------------------
>  1 file changed, 49 insertions(+), 39 deletions(-)
> 
> diff --git a/rust/kernel/irq/request.rs b/rust/kernel/irq/request.rs
> index d070ddabd37e7806f76edefd5d2ad46524be620e..f99aff2dd479f5223c90f0d2694f57e6c864bdb5 100644
> --- a/rust/kernel/irq/request.rs
> +++ b/rust/kernel/irq/request.rs
> @@ -37,18 +37,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)
>      }
>  }
>  
> @@ -134,7 +134,7 @@ pub fn irq(&self) -> u32 {
>  /// use core::sync::atomic::Ordering;
>  ///
>  /// use kernel::prelude::*;
> -/// use kernel::device::Bound;
> +/// use kernel::device::{Bound, Device};
>  /// use kernel::irq::flags::Flags;
>  /// use kernel::irq::Registration;
>  /// use kernel::irq::IrqRequest;
> @@ -156,7 +156,7 @@ pub fn irq(&self) -> u32 {
>  /// 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 {
> +///     fn handle(&self, _dev: &Device<Bound>) -> IrqReturn {
>  ///         self.0.fetch_add(1, Ordering::Relaxed);
>  ///
>  ///         IrqReturn::Handled
> @@ -182,8 +182,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]
> @@ -211,8 +210,8 @@ 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.
> @@ -225,7 +224,7 @@ pub fn new<'a>(
>                                  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
> @@ -262,9 +261,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(&registration.handler, device) as c_uint
>  }
>  
>  /// The value that can be returned from `ThreadedHandler::handle_irq`.
> @@ -288,32 +291,32 @@ pub trait ThreadedHandler: Sync {
>      /// limitations do apply. All work that does not necessarily need to be
>      /// executed from interrupt context, should be deferred to the threaded
>      /// handler, i.e. [`ThreadedHandler::handle_threaded`].
> -    fn handle(&self) -> ThreadedIrqReturn;
> +    fn handle(&self, device: &Device<Bound>) -> ThreadedIrqReturn;
>  
>      /// The threaded IRQ handler.
>      ///
>      /// This is executed in process context. The kernel creates a dedicated
>      /// kthread for this purpose.
> -    fn handle_threaded(&self) -> IrqReturn;
> +    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)
>      }
>  }
>  
> @@ -334,7 +337,7 @@ fn handle_threaded(&self) -> IrqReturn {
>  /// use core::sync::atomic::Ordering;
>  ///
>  /// use kernel::prelude::*;
> -/// use kernel::device::Bound;
> +/// use kernel::device::{Bound, Device};
>  /// use kernel::irq::flags::Flags;
>  /// use kernel::irq::ThreadedIrqReturn;
>  /// use kernel::irq::ThreadedRegistration;
> @@ -356,7 +359,7 @@ fn handle_threaded(&self) -> IrqReturn {
>  /// impl kernel::irq::request::ThreadedHandler for Handler {
>  ///     // This is executing in IRQ context in some CPU. Other CPUs can still
>  ///     // try to access the data.
> -///     fn handle(&self) -> ThreadedIrqReturn {
> +///     fn handle(&self, _dev: &Device<Bound>) -> ThreadedIrqReturn {
>  ///         self.0.fetch_add(1, Ordering::Relaxed);
>  ///         // By returning `WakeThread`, we indicate to the system that the
>  ///         // thread function should be called. Otherwise, return
> @@ -366,7 +369,7 @@ fn handle_threaded(&self) -> IrqReturn {
>  ///
>  ///     // This will run (in a separate kthread) if and only if `handle`
>  ///     // returns `WakeThread`.
> -///     fn handle_threaded(&self) -> IrqReturn {
> +///     fn handle_threaded(&self, _dev: &Device<Bound>) -> IrqReturn {
>  ///         self.0.fetch_add(1, Ordering::Relaxed);
>  ///         IrqReturn::Handled
>  ///     }
> @@ -391,8 +394,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]
> @@ -420,8 +422,8 @@ 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.
> @@ -435,7 +437,7 @@ pub fn new<'a>(
>                                  Some(thread_fn_callback::<T>),
>                                  flags.into_inner() as usize,
>                                  name.as_char_ptr(),
> -                                (&raw mut (*this.as_ptr()).handler).cast(),
> +                                this.as_ptr().cast::<c_void>(),
>                              )
>                          })?;
>                          request.irq
> @@ -475,16 +477,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(&registration.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(&registration.handler, device) as c_uint
>  }
> 
> ---
> base-commit: d860d29e91be18de62b0f441edee7d00f6cb4972
> change-id: 20250721-irq-bound-device-c9fdbfdd8cd9
> 
> Best regards,


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] rust: irq: add &Device<Bound> argument to irq callbacks
  2025-07-21 14:38 [PATCH] rust: irq: add &Device<Bound> argument to irq callbacks Alice Ryhl
  2025-07-21 19:13 ` Daniel Almeida
  2025-07-22 11:47 ` Dirk Behme
@ 2025-08-11  0:39 ` Daniel Almeida
  2 siblings, 0 replies; 7+ messages in thread
From: Daniel Almeida @ 2025-08-11  0:39 UTC (permalink / raw)
  To: Alice Ryhl
  Cc: Miguel Ojeda, Boqun Feng, Gary Guo, Björn Roy Baron,
	Andreas Hindborg, Trevor Gross, Danilo Krummrich,
	Greg Kroah-Hartman, Rafael J. Wysocki, Thomas Gleixner,
	Bjorn Helgaas, Krzysztof Wilczyński, Benno Lossin,
	Dirk Behme, linux-kernel, rust-for-linux, linux-pci

Hi Alice,

> On 21 Jul 2025, at 11:38, Alice Ryhl <aliceryhl@google.com> wrote:
> 
> 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>
> ---
> This patch is a follow-up to Daniel's irq series [1] that adds a
> &Device<Bound> argument to all irq callbacks. This allows you to use
> operations that are only safe on a bound device inside an irq callback.
> 
> The patch is otherwise based on top of driver-core-next.
> 
> [1]: https://lore.kernel.org/r/20250715-topics-tyr-request_irq2-v7-0-d469c0f37c07@collabora.com
> ---
> 

I tried to rebase this so we could send it together with v8 of the request_irq
series. However, is it me or this doesn't apply on v7?

> ---
> base-commit: d860d29e91be18de62b0f441edee7d00f6cb4972

Yeah, I couldn’t find this, sorry.

> change-id: 20250721-irq-bound-device-c9fdbfdd8cd9
> 
> Best regards,
> -- 
> Alice Ryhl <aliceryhl@google.com>
> 
> 


My apologies.

— Daniel


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] rust: irq: add &Device<Bound> argument to irq callbacks
  2025-07-21 19:33   ` Alice Ryhl
@ 2025-08-11  0:49     ` Daniel Almeida
  2025-08-11  6:54       ` Alice Ryhl
  0 siblings, 1 reply; 7+ messages in thread
From: Daniel Almeida @ 2025-08-11  0:49 UTC (permalink / raw)
  To: Alice Ryhl
  Cc: Miguel Ojeda, Boqun Feng, Gary Guo, Björn Roy Baron,
	Andreas Hindborg, Trevor Gross, Danilo Krummrich,
	Greg Kroah-Hartman, Rafael J. Wysocki, Thomas Gleixner,
	Bjorn Helgaas, Krzysztof Wilczyński, Benno Lossin,
	Dirk Behme, linux-kernel, rust-for-linux, linux-pci



> On 21 Jul 2025, at 16:33, Alice Ryhl <aliceryhl@google.com> wrote:
> 
> On Mon, Jul 21, 2025 at 9:14 PM Daniel Almeida
> <daniel.almeida@collabora.com> wrote:
>> 
>> Alice,
>> 
>>> On 21 Jul 2025, at 11:38, Alice Ryhl <aliceryhl@google.com> wrote:
>>> 
>>> 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>
>>> ---
>>> This patch is a follow-up to Daniel's irq series [1] that adds a
>>> &Device<Bound> argument to all irq callbacks. This allows you to use
>>> operations that are only safe on a bound device inside an irq callback.
>>> 
>>> The patch is otherwise based on top of driver-core-next.
>>> 
>>> [1]: https://lore.kernel.org/r/20250715-topics-tyr-request_irq2-v7-0-d469c0f37c07@collabora.com
>> 
>> I am having a hard time applying this locally.
> 
> Your irq series currently doesn't apply cleanly on top of
> driver-core-next and requires resolving a minor conflict. You can find
> the commits here:
> https://github.com/Darksonn/linux/commits/sent/20250721-irq-bound-device-c9fdbfdd8cd9-v1/

Ah, we’ve already discussed this, it seems.

> 
>>> ///
>>> /// 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() };
>> 
>> Where was this function introduced? i.e. I am missing the change that brought
>> in RegistrationInner::device(), or maybe some Deref impl that would make this
>> possible?
> 
> In this series:
> https://lore.kernel.org/all/20250713182737.64448-2-dakr@kernel.org/
> 
>> Also, I wonder if we can't make the scope of this unsafe block smaller?
> 
> I guess we could with an extra `let` statement.
> 
> Alice



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] rust: irq: add &Device<Bound> argument to irq callbacks
  2025-08-11  0:49     ` Daniel Almeida
@ 2025-08-11  6:54       ` Alice Ryhl
  0 siblings, 0 replies; 7+ messages in thread
From: Alice Ryhl @ 2025-08-11  6:54 UTC (permalink / raw)
  To: Daniel Almeida
  Cc: Miguel Ojeda, Boqun Feng, Gary Guo, Björn Roy Baron,
	Andreas Hindborg, Trevor Gross, Danilo Krummrich,
	Greg Kroah-Hartman, Rafael J. Wysocki, Thomas Gleixner,
	Bjorn Helgaas, Krzysztof Wilczyński, Benno Lossin,
	Dirk Behme, linux-kernel, rust-for-linux, linux-pci

On Mon, Aug 11, 2025 at 2:49 AM Daniel Almeida
<daniel.almeida@collabora.com> wrote:
>
>
>
> > On 21 Jul 2025, at 16:33, Alice Ryhl <aliceryhl@google.com> wrote:
> >
> > On Mon, Jul 21, 2025 at 9:14 PM Daniel Almeida
> > <daniel.almeida@collabora.com> wrote:
> >>
> >> Alice,
> >>
> >>> On 21 Jul 2025, at 11:38, Alice Ryhl <aliceryhl@google.com> wrote:
> >>>
> >>> 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>
> >>> ---
> >>> This patch is a follow-up to Daniel's irq series [1] that adds a
> >>> &Device<Bound> argument to all irq callbacks. This allows you to use
> >>> operations that are only safe on a bound device inside an irq callback.
> >>>
> >>> The patch is otherwise based on top of driver-core-next.
> >>>
> >>> [1]: https://lore.kernel.org/r/20250715-topics-tyr-request_irq2-v7-0-d469c0f37c07@collabora.com
> >>
> >> I am having a hard time applying this locally.
> >
> > Your irq series currently doesn't apply cleanly on top of
> > driver-core-next and requires resolving a minor conflict. You can find
> > the commits here:
> > https://github.com/Darksonn/linux/commits/sent/20250721-irq-bound-device-c9fdbfdd8cd9-v1/
>
> Ah, we’ve already discussed this, it seems.

My suggestion is that you pull the tag I shared and cherry-pick it from there.

Alice

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2025-08-11  6:54 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-07-21 14:38 [PATCH] rust: irq: add &Device<Bound> argument to irq callbacks Alice Ryhl
2025-07-21 19:13 ` Daniel Almeida
2025-07-21 19:33   ` Alice Ryhl
2025-08-11  0:49     ` Daniel Almeida
2025-08-11  6:54       ` Alice Ryhl
2025-07-22 11:47 ` Dirk Behme
2025-08-11  0:39 ` Daniel Almeida

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).