[PATCH v2] rust: irq: add &Device<Bound> argument to irq callbacks

[PATCH v2] rust: irq: add &Device<Bound> argument to irq callbacks

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

[1]: https://lore.kernel.org/all/20250810-topics-tyr-request_irq2-v8-0-8163f4c4c3a6@collabora.com/
---
Changes in v2:
- Rebase on v8 of [1] (and hence v6.17-rc1).
- Link to v1: https://lore.kernel.org/r/20250721-irq-bound-device-v1-1-4fb2af418a63@google.com
---
 rust/kernel/irq/request.rs | 85 ++++++++++++++++++++++++++--------------------
 1 file changed, 49 insertions(+), 36 deletions(-)

diff --git a/rust/kernel/irq/request.rs b/rust/kernel/irq/request.rs
index 29597573e91106d0990ec096a8e7f93ef2f89f2b..ae5d967fb9d6a748b4274464b615e6dd965bca10 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};
@@ -153,7 +153,7 @@ pub fn irq(&self) -> u32 {
 ///
 /// impl irq::Handler for Handler {
 ///     // Executed in IRQ context.
-///     fn handle(&self) -> IrqReturn {
+///     fn handle(&self, _dev: &Device<Bound>) -> IrqReturn {
 ///         self.completion.complete_all();
 ///         IrqReturn::Handled
 ///     }
@@ -179,7 +179,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]
@@ -207,8 +207,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.
@@ -221,7 +221,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
@@ -258,9 +258,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`].
@@ -286,7 +290,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
     }
 
@@ -294,26 +299,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)
     }
 }
 
@@ -332,7 +337,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,
@@ -355,7 +360,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
@@ -388,7 +393,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]
@@ -416,8 +421,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.
@@ -431,7 +436,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
@@ -471,16 +476,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: 062b3e4a1f880f104a8d4b90b767788786aa7b78
change-id: 20250721-irq-bound-device-c9fdbfdd8cd9
prerequisite-change-id: 20250712-topics-tyr-request_irq2-ae7ee9b85854:v8
prerequisite-patch-id: 0ed57dd6c685034df8a741c0290e1647880ad9b9
prerequisite-patch-id: 939809f0395089541acb9421cdb798ea625bd31b
prerequisite-patch-id: 5106394e817b371de5035a8289f6820f8aea3895
prerequisite-patch-id: 402fb2b44076d5a5d2a7210a8496e665d05e7c2a
prerequisite-patch-id: c4d08dda330b2ae8b5c8bb7a9d6bdef379119a6d
prerequisite-patch-id: 0a639274db9c6e7002828a6652a9f5a7d1dfd67f

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

Re: [PATCH v2] rust: irq: add &Device<Bound> argument to irq callbacks

[Daniel Almeida]

> On 11 Aug 2025, at 09:33, 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.
> 
> [1]: https://lore.kernel.org/all/20250810-topics-tyr-request_irq2-v8-0-8163f4c4c3a6@collabora.com/
> ---
> Changes in v2:
> - Rebase on v8 of [1] (and hence v6.17-rc1).
> 

Thanks, I’ll apply on top of the series as a convenience to the maintainers.

— Daniel 

Re: [PATCH v2] rust: irq: add &Device<Bound> argument to irq callbacks

[Boqun Feng]

On Mon, Aug 11, 2025 at 12:33:51PM +0000, Alice Ryhl wrote:
[...]
> @@ -207,8 +207,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>(),

At this moment the `Regstration` is not fully initialized...

>                      irq: {
>                          // SAFETY:
>                          // - The callbacks are valid for use with request_irq.
> @@ -221,7 +221,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>(),
>                              )

... and interrupt can happen right after request_irq() ...

>                          })?;
>                          request.irq
> @@ -258,9 +258,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>) };

... hence it's not correct to construct a reference to `Registration`
here, but yes, both `handler` and the `device` part of `inner` has been
properly initialized. So

	let registration = ptr.cast::<Registration<T>>();

	// SAFETY: The `data` part of `Devres` is `Opaque` and here we
	// only access `.device()`, which has been properly initialized
	// before `request_irq()`.
	let device = unsafe { (*registration).inner.device() };
	
	// 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 { device.as_bound() };

	// SAFETY: `.handler` has been properly initialized before
	// `request_irq()`.
	T::handle(unsafe { &(*registration).handler }, device) as c_uint

Thoughts? Similar for the threaded one.

Regards,
Boqun

> +    // 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
>  }
>  
[...]

Re: [PATCH v2] rust: irq: add &Device<Bound> argument to irq callbacks

[Alice Ryhl]

On Mon, Aug 11, 2025 at 3:56 PM Boqun Feng <boqun.feng@gmail.com> wrote:
>
> On Mon, Aug 11, 2025 at 12:33:51PM +0000, Alice Ryhl wrote:
> [...]
> > @@ -207,8 +207,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>(),
>
> At this moment the `Regstration` is not fully initialized...
>
> >                      irq: {
> >                          // SAFETY:
> >                          // - The callbacks are valid for use with request_irq.
> > @@ -221,7 +221,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>(),
> >                              )
>
> ... and interrupt can happen right after request_irq() ...
>
> >                          })?;
> >                          request.irq
> > @@ -258,9 +258,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>) };
>
> ... hence it's not correct to construct a reference to `Registration`
> here, but yes, both `handler` and the `device` part of `inner` has been
> properly initialized. So
>
>         let registration = ptr.cast::<Registration<T>>();
>
>         // SAFETY: The `data` part of `Devres` is `Opaque` and here we
>         // only access `.device()`, which has been properly initialized
>         // before `request_irq()`.
>         let device = unsafe { (*registration).inner.device() };
>
>         // 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 { device.as_bound() };
>
>         // SAFETY: `.handler` has been properly initialized before
>         // `request_irq()`.
>         T::handle(unsafe { &(*registration).handler }, device) as c_uint
>
> Thoughts? Similar for the threaded one.

This code is no different. It creates a reference to `inner` before
the `irq` field is written. Of course, it's also no different in that
since data of a `Devres` is in `Opaque`, this is not actually UB.

What I can offer you is to use the closure form of pin-init to call
request_irq after initialization has fully completed.

Alice

Re: [PATCH v2] rust: irq: add &Device<Bound> argument to irq callbacks

[Boqun Feng]

On Mon, Aug 11, 2025 at 04:05:31PM +0200, Alice Ryhl wrote:
> On Mon, Aug 11, 2025 at 3:56 PM Boqun Feng <boqun.feng@gmail.com> wrote:
> >
> > On Mon, Aug 11, 2025 at 12:33:51PM +0000, Alice Ryhl wrote:
> > [...]
> > > @@ -207,8 +207,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>(),
> >
> > At this moment the `Regstration` is not fully initialized...
> >
> > >                      irq: {
> > >                          // SAFETY:
> > >                          // - The callbacks are valid for use with request_irq.
> > > @@ -221,7 +221,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>(),
> > >                              )
> >
> > ... and interrupt can happen right after request_irq() ...
> >
> > >                          })?;
> > >                          request.irq
> > > @@ -258,9 +258,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>) };
> >
> > ... hence it's not correct to construct a reference to `Registration`
> > here, but yes, both `handler` and the `device` part of `inner` has been
> > properly initialized. So
> >
> >         let registration = ptr.cast::<Registration<T>>();
> >
> >         // SAFETY: The `data` part of `Devres` is `Opaque` and here we
> >         // only access `.device()`, which has been properly initialized
> >         // before `request_irq()`.
> >         let device = unsafe { (*registration).inner.device() };
> >
> >         // 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 { device.as_bound() };
> >
> >         // SAFETY: `.handler` has been properly initialized before
> >         // `request_irq()`.
> >         T::handle(unsafe { &(*registration).handler }, device) as c_uint
> >
> > Thoughts? Similar for the threaded one.
> 
> This code is no different. It creates a reference to `inner` before
> the `irq` field is written. Of course, it's also no different in that
> since data of a `Devres` is in `Opaque`, this is not actually UB.
> 

Well, I think we need at least mentioning that it's safe because we
don't access .inner.inner here, but..

> What I can offer you is to use the closure form of pin-init to call
> request_irq after initialization has fully completed.
> 

.. now you mention this, I think we can just move the `request_irq()`
to the initializer of `_pin`:

------>8
diff --git a/rust/kernel/irq/request.rs b/rust/kernel/irq/request.rs
index ae5d967fb9d6..3343964fc1ab 100644
--- a/rust/kernel/irq/request.rs
+++ b/rust/kernel/irq/request.rs
@@ -209,26 +209,26 @@ pub fn new<'a>(
                 try_pin_init!(RegistrationInner {
                     // 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.
-                        to_result(unsafe {
-                            bindings::request_irq(
-                                request.irq,
-                                Some(handle_irq_callback::<T>),
-                                flags.into_inner(),
-                                name.as_char_ptr(),
-                                this.as_ptr().cast::<c_void>(),
-                            )
-                        })?;
-                        request.irq
-                    }
+                    irq: request.irq
                 })
             ),
-            _pin: PhantomPinned,
+            _pin: {
+                // 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(),
+                        name.as_char_ptr(),
+                        this.as_ptr().cast::<c_void>(),
+                    )
+                })?;
+                PhantomPinned
+            },
         })
     }


Thoughts?

Regards,
Boqun

Re: [PATCH v2] rust: irq: add &Device<Bound> argument to irq callbacks

[Alice Ryhl]

On Mon, Aug 11, 2025 at 4:24 PM Boqun Feng <boqun.feng@gmail.com> wrote:
>
> On Mon, Aug 11, 2025 at 04:05:31PM +0200, Alice Ryhl wrote:
> > On Mon, Aug 11, 2025 at 3:56 PM Boqun Feng <boqun.feng@gmail.com> wrote:
> > >
> > > On Mon, Aug 11, 2025 at 12:33:51PM +0000, Alice Ryhl wrote:
> > > [...]
> > > > @@ -207,8 +207,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>(),
> > >
> > > At this moment the `Regstration` is not fully initialized...
> > >
> > > >                      irq: {
> > > >                          // SAFETY:
> > > >                          // - The callbacks are valid for use with request_irq.
> > > > @@ -221,7 +221,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>(),
> > > >                              )
> > >
> > > ... and interrupt can happen right after request_irq() ...
> > >
> > > >                          })?;
> > > >                          request.irq
> > > > @@ -258,9 +258,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>) };
> > >
> > > ... hence it's not correct to construct a reference to `Registration`
> > > here, but yes, both `handler` and the `device` part of `inner` has been
> > > properly initialized. So
> > >
> > >         let registration = ptr.cast::<Registration<T>>();
> > >
> > >         // SAFETY: The `data` part of `Devres` is `Opaque` and here we
> > >         // only access `.device()`, which has been properly initialized
> > >         // before `request_irq()`.
> > >         let device = unsafe { (*registration).inner.device() };
> > >
> > >         // 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 { device.as_bound() };
> > >
> > >         // SAFETY: `.handler` has been properly initialized before
> > >         // `request_irq()`.
> > >         T::handle(unsafe { &(*registration).handler }, device) as c_uint
> > >
> > > Thoughts? Similar for the threaded one.
> >
> > This code is no different. It creates a reference to `inner` before
> > the `irq` field is written. Of course, it's also no different in that
> > since data of a `Devres` is in `Opaque`, this is not actually UB.
> >
>
> Well, I think we need at least mentioning that it's safe because we
> don't access .inner.inner here, but..
>
> > What I can offer you is to use the closure form of pin-init to call
> > request_irq after initialization has fully completed.
> >
>
> .. now you mention this, I think we can just move the `request_irq()`
> to the initializer of `_pin`:
>
> ------>8
> diff --git a/rust/kernel/irq/request.rs b/rust/kernel/irq/request.rs
> index ae5d967fb9d6..3343964fc1ab 100644
> --- a/rust/kernel/irq/request.rs
> +++ b/rust/kernel/irq/request.rs
> @@ -209,26 +209,26 @@ pub fn new<'a>(
>                  try_pin_init!(RegistrationInner {
>                      // 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.
> -                        to_result(unsafe {
> -                            bindings::request_irq(
> -                                request.irq,
> -                                Some(handle_irq_callback::<T>),
> -                                flags.into_inner(),
> -                                name.as_char_ptr(),
> -                                this.as_ptr().cast::<c_void>(),
> -                            )
> -                        })?;
> -                        request.irq
> -                    }
> +                    irq: request.irq
>                  })
>              ),
> -            _pin: PhantomPinned,
> +            _pin: {
> +                // 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(),
> +                        name.as_char_ptr(),
> +                        this.as_ptr().cast::<c_void>(),
> +                    )
> +                })?;
> +                PhantomPinned
> +            },
>          })
>      }
>
>
> Thoughts?

That calls free_irq if request_irq fails, which is illegal.

Alice

Re: [PATCH v2] rust: irq: add &Device<Bound> argument to irq callbacks

[Boqun Feng]

On Mon, Aug 11, 2025 at 04:25:50PM +0200, Alice Ryhl wrote:
> On Mon, Aug 11, 2025 at 4:24 PM Boqun Feng <boqun.feng@gmail.com> wrote:
> >
> > On Mon, Aug 11, 2025 at 04:05:31PM +0200, Alice Ryhl wrote:
> > > On Mon, Aug 11, 2025 at 3:56 PM Boqun Feng <boqun.feng@gmail.com> wrote:
> > > >
> > > > On Mon, Aug 11, 2025 at 12:33:51PM +0000, Alice Ryhl wrote:
> > > > [...]
> > > > > @@ -207,8 +207,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>(),
> > > >
> > > > At this moment the `Regstration` is not fully initialized...
> > > >
> > > > >                      irq: {
> > > > >                          // SAFETY:
> > > > >                          // - The callbacks are valid for use with request_irq.
> > > > > @@ -221,7 +221,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>(),
> > > > >                              )
> > > >
> > > > ... and interrupt can happen right after request_irq() ...
> > > >
> > > > >                          })?;
> > > > >                          request.irq
> > > > > @@ -258,9 +258,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>) };
> > > >
> > > > ... hence it's not correct to construct a reference to `Registration`
> > > > here, but yes, both `handler` and the `device` part of `inner` has been
> > > > properly initialized. So
> > > >
> > > >         let registration = ptr.cast::<Registration<T>>();
> > > >
> > > >         // SAFETY: The `data` part of `Devres` is `Opaque` and here we
> > > >         // only access `.device()`, which has been properly initialized
> > > >         // before `request_irq()`.
> > > >         let device = unsafe { (*registration).inner.device() };
> > > >
> > > >         // 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 { device.as_bound() };
> > > >
> > > >         // SAFETY: `.handler` has been properly initialized before
> > > >         // `request_irq()`.
> > > >         T::handle(unsafe { &(*registration).handler }, device) as c_uint
> > > >
> > > > Thoughts? Similar for the threaded one.
> > >
> > > This code is no different. It creates a reference to `inner` before
> > > the `irq` field is written. Of course, it's also no different in that
> > > since data of a `Devres` is in `Opaque`, this is not actually UB.
> > >
> >
> > Well, I think we need at least mentioning that it's safe because we
> > don't access .inner.inner here, but..
> >
> > > What I can offer you is to use the closure form of pin-init to call
> > > request_irq after initialization has fully completed.
> > >
> >
> > .. now you mention this, I think we can just move the `request_irq()`
> > to the initializer of `_pin`:
> >
> > ------>8
> > diff --git a/rust/kernel/irq/request.rs b/rust/kernel/irq/request.rs
> > index ae5d967fb9d6..3343964fc1ab 100644
> > --- a/rust/kernel/irq/request.rs
> > +++ b/rust/kernel/irq/request.rs
> > @@ -209,26 +209,26 @@ pub fn new<'a>(
> >                  try_pin_init!(RegistrationInner {
> >                      // 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.
> > -                        to_result(unsafe {
> > -                            bindings::request_irq(
> > -                                request.irq,
> > -                                Some(handle_irq_callback::<T>),
> > -                                flags.into_inner(),
> > -                                name.as_char_ptr(),
> > -                                this.as_ptr().cast::<c_void>(),
> > -                            )
> > -                        })?;
> > -                        request.irq
> > -                    }
> > +                    irq: request.irq
> >                  })
> >              ),
> > -            _pin: PhantomPinned,
> > +            _pin: {
> > +                // 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(),
> > +                        name.as_char_ptr(),
> > +                        this.as_ptr().cast::<c_void>(),
> > +                    )
> > +                })?;
> > +                PhantomPinned
> > +            },
> >          })
> >      }
> >
> >
> > Thoughts?
> 
> That calls free_irq if request_irq fails, which is illegal.
> 

Ah, right. I was missing that. Then back to the "we have to mention that
we are not accessing the data of Devres" suggestion, which I think is
more appropriate for this case.

Regards,
Boqun

> Alice

Re: [PATCH v2] rust: irq: add &Device<Bound> argument to irq callbacks

[Alice Ryhl]

On Mon, Aug 11, 2025 at 4:31 PM Boqun Feng <boqun.feng@gmail.com> wrote:
>
> On Mon, Aug 11, 2025 at 04:25:50PM +0200, Alice Ryhl wrote:
> > On Mon, Aug 11, 2025 at 4:24 PM Boqun Feng <boqun.feng@gmail.com> wrote:
> > >
> > > On Mon, Aug 11, 2025 at 04:05:31PM +0200, Alice Ryhl wrote:
> > > > On Mon, Aug 11, 2025 at 3:56 PM Boqun Feng <boqun.feng@gmail.com> wrote:
> > > > >
> > > > > On Mon, Aug 11, 2025 at 12:33:51PM +0000, Alice Ryhl wrote:
> > > > > [...]
> > > > > > @@ -207,8 +207,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>(),
> > > > >
> > > > > At this moment the `Regstration` is not fully initialized...
> > > > >
> > > > > >                      irq: {
> > > > > >                          // SAFETY:
> > > > > >                          // - The callbacks are valid for use with request_irq.
> > > > > > @@ -221,7 +221,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>(),
> > > > > >                              )
> > > > >
> > > > > ... and interrupt can happen right after request_irq() ...
> > > > >
> > > > > >                          })?;
> > > > > >                          request.irq
> > > > > > @@ -258,9 +258,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>) };
> > > > >
> > > > > ... hence it's not correct to construct a reference to `Registration`
> > > > > here, but yes, both `handler` and the `device` part of `inner` has been
> > > > > properly initialized. So
> > > > >
> > > > >         let registration = ptr.cast::<Registration<T>>();
> > > > >
> > > > >         // SAFETY: The `data` part of `Devres` is `Opaque` and here we
> > > > >         // only access `.device()`, which has been properly initialized
> > > > >         // before `request_irq()`.
> > > > >         let device = unsafe { (*registration).inner.device() };
> > > > >
> > > > >         // 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 { device.as_bound() };
> > > > >
> > > > >         // SAFETY: `.handler` has been properly initialized before
> > > > >         // `request_irq()`.
> > > > >         T::handle(unsafe { &(*registration).handler }, device) as c_uint
> > > > >
> > > > > Thoughts? Similar for the threaded one.
> > > >
> > > > This code is no different. It creates a reference to `inner` before
> > > > the `irq` field is written. Of course, it's also no different in that
> > > > since data of a `Devres` is in `Opaque`, this is not actually UB.
> > > >
> > >
> > > Well, I think we need at least mentioning that it's safe because we
> > > don't access .inner.inner here, but..
> > >
> > > > What I can offer you is to use the closure form of pin-init to call
> > > > request_irq after initialization has fully completed.
> > > >
> > >
> > > .. now you mention this, I think we can just move the `request_irq()`
> > > to the initializer of `_pin`:
> > >
> > > ------>8
> > > diff --git a/rust/kernel/irq/request.rs b/rust/kernel/irq/request.rs
> > > index ae5d967fb9d6..3343964fc1ab 100644
> > > --- a/rust/kernel/irq/request.rs
> > > +++ b/rust/kernel/irq/request.rs
> > > @@ -209,26 +209,26 @@ pub fn new<'a>(
> > >                  try_pin_init!(RegistrationInner {
> > >                      // 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.
> > > -                        to_result(unsafe {
> > > -                            bindings::request_irq(
> > > -                                request.irq,
> > > -                                Some(handle_irq_callback::<T>),
> > > -                                flags.into_inner(),
> > > -                                name.as_char_ptr(),
> > > -                                this.as_ptr().cast::<c_void>(),
> > > -                            )
> > > -                        })?;
> > > -                        request.irq
> > > -                    }
> > > +                    irq: request.irq
> > >                  })
> > >              ),
> > > -            _pin: PhantomPinned,
> > > +            _pin: {
> > > +                // 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(),
> > > +                        name.as_char_ptr(),
> > > +                        this.as_ptr().cast::<c_void>(),
> > > +                    )
> > > +                })?;
> > > +                PhantomPinned
> > > +            },
> > >          })
> > >      }
> > >
> > >
> > > Thoughts?
> >
> > That calls free_irq if request_irq fails, which is illegal.
> >
>
> Ah, right. I was missing that. Then back to the "we have to mention that
> we are not accessing the data of Devres" suggestion, which I think is
> more appropriate for this case.

I will add:

// - 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.

Will you offer your Reviewed-by ?

Alice

Re: [PATCH v2] rust: irq: add &Device<Bound> argument to irq callbacks

[Boqun Feng]

On Mon, Aug 11, 2025 at 04:37:44PM +0200, Alice Ryhl wrote:
> On Mon, Aug 11, 2025 at 4:31 PM Boqun Feng <boqun.feng@gmail.com> wrote:
> >
> > On Mon, Aug 11, 2025 at 04:25:50PM +0200, Alice Ryhl wrote:
> > > On Mon, Aug 11, 2025 at 4:24 PM Boqun Feng <boqun.feng@gmail.com> wrote:
> > > >
> > > > On Mon, Aug 11, 2025 at 04:05:31PM +0200, Alice Ryhl wrote:
> > > > > On Mon, Aug 11, 2025 at 3:56 PM Boqun Feng <boqun.feng@gmail.com> wrote:
> > > > > >
> > > > > > On Mon, Aug 11, 2025 at 12:33:51PM +0000, Alice Ryhl wrote:
> > > > > > [...]
> > > > > > > @@ -207,8 +207,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>(),
> > > > > >
> > > > > > At this moment the `Regstration` is not fully initialized...
> > > > > >
> > > > > > >                      irq: {
> > > > > > >                          // SAFETY:
> > > > > > >                          // - The callbacks are valid for use with request_irq.
> > > > > > > @@ -221,7 +221,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>(),
> > > > > > >                              )
> > > > > >
> > > > > > ... and interrupt can happen right after request_irq() ...
> > > > > >
> > > > > > >                          })?;
> > > > > > >                          request.irq
> > > > > > > @@ -258,9 +258,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>) };
> > > > > >
> > > > > > ... hence it's not correct to construct a reference to `Registration`
> > > > > > here, but yes, both `handler` and the `device` part of `inner` has been
> > > > > > properly initialized. So
> > > > > >
> > > > > >         let registration = ptr.cast::<Registration<T>>();
> > > > > >
> > > > > >         // SAFETY: The `data` part of `Devres` is `Opaque` and here we
> > > > > >         // only access `.device()`, which has been properly initialized
> > > > > >         // before `request_irq()`.
> > > > > >         let device = unsafe { (*registration).inner.device() };
> > > > > >
> > > > > >         // 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 { device.as_bound() };
> > > > > >
> > > > > >         // SAFETY: `.handler` has been properly initialized before
> > > > > >         // `request_irq()`.
> > > > > >         T::handle(unsafe { &(*registration).handler }, device) as c_uint
> > > > > >
> > > > > > Thoughts? Similar for the threaded one.
[...]
> >
> > Ah, right. I was missing that. Then back to the "we have to mention that
> > we are not accessing the data of Devres" suggestion, which I think is
> > more appropriate for this case.
> 
> I will add:
> 
> // - 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.
> 

This looks good to me.

> Will you offer your Reviewed-by ?
> 

I want to wait and see Daniel's new version with your patch included.
But overall LGTM. Thanks!

Regards,
Boqun

> Alice