[PATCH] rust: irq: request: touch up the documentation

[PATCH] rust: irq: request: touch up the documentation

[Daniel Almeida]

Parts of the documentation are either unclear or misplaced and can
otherwise be improved. This commit fixes them.

This is specially important in the context of the safety requirements of
functions or type invariants, as users have to uphold the former and may
rely on the latter, so we should avoid anything that may create
confusion.

Suggested-by: Andreas Hindborg <a.hindborg@kernel.org>
Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
---
 rust/kernel/irq/request.rs | 44 ++++++++++++++++++++++++++++++++++----------
 1 file changed, 34 insertions(+), 10 deletions(-)

diff --git a/rust/kernel/irq/request.rs b/rust/kernel/irq/request.rs
index b150563fdef809899c7ca39221c4928238e31110..a1d0e934972fa555968dba5731077cb11606a0d3 100644
--- a/rust/kernel/irq/request.rs
+++ b/rust/kernel/irq/request.rs
@@ -30,6 +30,11 @@ pub enum IrqReturn {
 pub trait Handler: Sync {
     /// The hard IRQ handler.
     ///
+    /// A "hard" handler in the context of the Linux kernel is the part of an
+    /// interrupt handler that runs in interrupt context. The hard handler
+    /// usually defers time consuming processing to run in process context, for
+    /// instance by queuing the work on a work queue for later execution.
+    ///
     /// This is executed in interrupt context, hence all corresponding
     /// limitations do apply.
     ///
@@ -54,9 +59,7 @@ fn handle(&self, device: &Device<Bound>) -> IrqReturn {
 /// # Invariants
 ///
 /// - `self.irq` is the same as the one passed to `request_{threaded}_irq`.
-/// - `cookie` was passed to `request_{threaded}_irq` as the cookie. It is guaranteed to be unique
-///   by the type system, since each call to `new` will return a different instance of
-///   `Registration`.
+/// - `cookie` is unique.
 #[pin_data(PinnedDrop)]
 struct RegistrationInner {
     irq: u32,
@@ -91,7 +94,9 @@ fn drop(self: Pin<&mut Self>) {
 // concurrent access.
 unsafe impl Sync for RegistrationInner {}
 
-// SAFETY: It is safe to send `RegistrationInner` across threads.
+// SAFETY: It is safe to transfer ownership of `RegistrationInner` from another
+// thread, because it has no shared mutable state. The IRQ owned by
+// `RegistrationInner` via `cookie` can be dropped from any thread.
 unsafe impl Send for RegistrationInner {}
 
 /// A request for an IRQ line for a given device.
@@ -112,7 +117,7 @@ impl<'a> IrqRequest<'a> {
     ///
     /// - `irq` should be a valid IRQ number for `dev`.
     pub(crate) unsafe fn new(dev: &'a Device<Bound>, irq: u32) -> Self {
-        // INVARIANT: `irq` is a valid IRQ number for `dev`.
+        // By function safety requirement, irq` is a valid IRQ number for `dev`.
         IrqRequest { dev, irq }
     }
 
@@ -183,6 +188,8 @@ pub fn irq(&self) -> u32 {
 /// * We own an irq handler whose cookie is a pointer to `Self`.
 #[pin_data]
 pub struct Registration<T: Handler + 'static> {
+    /// We need to drop inner before handler, as we must ensure that the handler
+    /// is valid until `free_irq` is called.
     #[pin]
     inner: Devres<RegistrationInner>,
 
@@ -196,7 +203,8 @@ pub struct Registration<T: Handler + 'static> {
 }
 
 impl<T: Handler + 'static> Registration<T> {
-    /// Registers the IRQ handler with the system for the given IRQ number.
+    /// Registers the IRQ handler with the system for the IRQ number represented
+    /// by `request`.
     pub fn new<'a>(
         request: IrqRequest<'a>,
         flags: Flags,
@@ -208,7 +216,11 @@ pub fn new<'a>(
             inner <- Devres::new(
                 request.dev,
                 try_pin_init!(RegistrationInner {
-                    // INVARIANT: `this` is a valid pointer to the `Registration` instance
+                    // INVARIANT: `this` is a valid pointer to the `Registration` instance.
+                    // INVARIANT: `cookie` is being passed to `request_irq` as
+                    // the cookie. It is guaranteed to be unique by the type
+                    // system, since each call to `new` will return a different
+                    // instance of `Registration`.
                     cookie: this.as_ptr().cast::<c_void>(),
                     irq: {
                         // SAFETY:
@@ -260,7 +272,8 @@ pub fn synchronize(&self, dev: &Device<Bound>) -> Result {
 
 /// # Safety
 ///
-/// This function should be only used as the callback in `request_irq`.
+/// - This function should be only used as the callback in `request_irq`.
+/// - `ptr` must be valid for use as a reference to `T`
 unsafe extern "C" fn handle_irq_callback<T: Handler>(_irq: i32, ptr: *mut c_void) -> c_uint {
     // SAFETY: `ptr` is a pointer to `Registration<T>` set in `Registration::new`
     let registration = unsafe { &*(ptr as *const Registration<T>) };
@@ -288,6 +301,11 @@ pub enum ThreadedIrqReturn {
 pub trait ThreadedHandler: Sync {
     /// The hard IRQ handler.
     ///
+    /// A "hard" handler in the context of the Linux kernel is the part of an
+    /// interrupt handler that runs in interrupt context. The hard handler
+    /// usually defers time consuming processing to run in process context, for
+    /// instance by queuing the work on a work queue for later execution.
+    ///
     /// This is executed in interrupt context, hence all corresponding
     /// limitations do apply. All work that does not necessarily need to be
     /// executed from interrupt context, should be deferred to the threaded
@@ -427,6 +445,10 @@ pub fn new<'a>(
                 request.dev,
                 try_pin_init!(RegistrationInner {
                     // INVARIANT: `this` is a valid pointer to the `ThreadedRegistration` instance.
+                    // INVARIANT: `cookie` is being passed to
+                    // `request_threaded_irq` as the cookie. It is guaranteed to
+                    // be unique by the type system, since each call to `new`
+                    // will return a different instance of `Registration`.
                     cookie: this.as_ptr().cast::<c_void>(),
                     irq: {
                         // SAFETY:
@@ -479,7 +501,8 @@ pub fn synchronize(&self, dev: &Device<Bound>) -> Result {
 
 /// # Safety
 ///
-/// This function should be only used as the callback in `request_threaded_irq`.
+/// - This function should be only used as the callback in `request_threaded_irq`.
+/// - `ptr` must be valid for use as a reference to `T`
 unsafe extern "C" fn handle_threaded_irq_callback<T: ThreadedHandler>(
     _irq: i32,
     ptr: *mut c_void,
@@ -495,7 +518,8 @@ pub fn synchronize(&self, dev: &Device<Bound>) -> Result {
 
 /// # Safety
 ///
-/// This function should be only used as the callback in `request_threaded_irq`.
+/// - This function should be only used as the callback in `request_threaded_irq`.
+/// - `ptr` must be valid for use as a reference to `T`
 unsafe extern "C" fn thread_fn_callback<T: ThreadedHandler>(_irq: i32, ptr: *mut c_void) -> c_uint {
     // SAFETY: `ptr` is a pointer to `ThreadedRegistration<T>` set in `ThreadedRegistration::new`
     let registration = unsafe { &*(ptr as *const ThreadedRegistration<T>) };

---
base-commit: 4c48aed6dfcd32ea23e52adc1072405a62facf46
change-id: 20250909-irq-andreas-fixes-4773537cdb81

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

Re: [PATCH] rust: irq: request: touch up the documentation

[Alice Ryhl]

On Tue, Sep 09, 2025 at 05:46:55PM -0300, Daniel Almeida wrote:
> Parts of the documentation are either unclear or misplaced and can
> otherwise be improved. This commit fixes them.
> 
> This is specially important in the context of the safety requirements of
> functions or type invariants, as users have to uphold the former and may
> rely on the latter, so we should avoid anything that may create
> confusion.
> 
> Suggested-by: Andreas Hindborg <a.hindborg@kernel.org>
> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>

>  /// A request for an IRQ line for a given device.
> @@ -112,7 +117,7 @@ impl<'a> IrqRequest<'a> {
>      ///
>      /// - `irq` should be a valid IRQ number for `dev`.
>      pub(crate) unsafe fn new(dev: &'a Device<Bound>, irq: u32) -> Self {
> -        // INVARIANT: `irq` is a valid IRQ number for `dev`.
> +        // By function safety requirement, irq` is a valid IRQ number for `dev`.
>          IrqRequest { dev, irq }

When creating a value with an Invariants section, we usually have an
INVARIANT comment. Why was this one removed?

>      }
>  
> @@ -183,6 +188,8 @@ pub fn irq(&self) -> u32 {
>  /// * We own an irq handler whose cookie is a pointer to `Self`.
>  #[pin_data]
>  pub struct Registration<T: Handler + 'static> {
> +    /// We need to drop inner before handler, as we must ensure that the handler
> +    /// is valid until `free_irq` is called.
>      #[pin]
>      inner: Devres<RegistrationInner>,
>  
> @@ -196,7 +203,8 @@ pub struct Registration<T: Handler + 'static> {
>  }
>  
>  impl<T: Handler + 'static> Registration<T> {
> -    /// Registers the IRQ handler with the system for the given IRQ number.
> +    /// Registers the IRQ handler with the system for the IRQ number represented
> +    /// by `request`.
>      pub fn new<'a>(
>          request: IrqRequest<'a>,
>          flags: Flags,
> @@ -208,7 +216,11 @@ pub fn new<'a>(
>              inner <- Devres::new(
>                  request.dev,
>                  try_pin_init!(RegistrationInner {
> -                    // INVARIANT: `this` is a valid pointer to the `Registration` instance
> +                    // INVARIANT: `this` is a valid pointer to the `Registration` instance.
> +                    // INVARIANT: `cookie` is being passed to `request_irq` as
> +                    // the cookie. It is guaranteed to be unique by the type
> +                    // system, since each call to `new` will return a different
> +                    // instance of `Registration`.
>                      cookie: this.as_ptr().cast::<c_void>(),

I believe these comments address the invariants of RegistrationInner. In
that case, we usually place them on the struct:

	// INVARIANT: `this` is a valid pointer to the `Registration` instance.
	// INVARIANT: `cookie` is being passed to `request_irq` as
	// the cookie. It is guaranteed to be unique by the type
	// system, since each call to `new` will return a different
	// instance of `Registration`.
	try_pin_init!(RegistrationInner {

Also, I don't really understand these comments. The first invariant is:

	`self.irq` is the same as the one passed to `request_{threaded}_irq`.

and the justification for that is that `this` is a valid pointer to the
`Registration` instance. That doesn't make sense to me because this
comment talks about `this`/`cookie` when it should talk about `irq`.

>                      irq: {
>                          // SAFETY:

Alice

Re: [PATCH] rust: irq: request: touch up the documentation

[Daniel Almeida]

Hi Alice,

> On 10 Sep 2025, at 03:38, Alice Ryhl <aliceryhl@google.com> wrote:
> 
> On Tue, Sep 09, 2025 at 05:46:55PM -0300, Daniel Almeida wrote:
>> Parts of the documentation are either unclear or misplaced and can
>> otherwise be improved. This commit fixes them.
>> 
>> This is specially important in the context of the safety requirements of
>> functions or type invariants, as users have to uphold the former and may
>> rely on the latter, so we should avoid anything that may create
>> confusion.
>> 
>> Suggested-by: Andreas Hindborg <a.hindborg@kernel.org>
>> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
> 
>> /// A request for an IRQ line for a given device.
>> @@ -112,7 +117,7 @@ impl<'a> IrqRequest<'a> {
>>     ///
>>     /// - `irq` should be a valid IRQ number for `dev`.
>>     pub(crate) unsafe fn new(dev: &'a Device<Bound>, irq: u32) -> Self {
>> -        // INVARIANT: `irq` is a valid IRQ number for `dev`.
>> +        // By function safety requirement, irq` is a valid IRQ number for `dev`.
>>         IrqRequest { dev, irq }
> 
> When creating a value with an Invariants section, we usually have an
> INVARIANT comment. Why was this one removed?

This was requested by Andreas [0].

> 
>>     }
>> 
>> @@ -183,6 +188,8 @@ pub fn irq(&self) -> u32 {
>> /// * We own an irq handler whose cookie is a pointer to `Self`.
>> #[pin_data]
>> pub struct Registration<T: Handler + 'static> {
>> +    /// We need to drop inner before handler, as we must ensure that the handler
>> +    /// is valid until `free_irq` is called.
>>     #[pin]
>>     inner: Devres<RegistrationInner>,
>> 
>> @@ -196,7 +203,8 @@ pub struct Registration<T: Handler + 'static> {
>> }
>> 
>> impl<T: Handler + 'static> Registration<T> {
>> -    /// Registers the IRQ handler with the system for the given IRQ number.
>> +    /// Registers the IRQ handler with the system for the IRQ number represented
>> +    /// by `request`.
>>     pub fn new<'a>(
>>         request: IrqRequest<'a>,
>>         flags: Flags,
>> @@ -208,7 +216,11 @@ pub fn new<'a>(
>>             inner <- Devres::new(
>>                 request.dev,
>>                 try_pin_init!(RegistrationInner {
>> -                    // INVARIANT: `this` is a valid pointer to the `Registration` instance
>> +                    // INVARIANT: `this` is a valid pointer to the `Registration` instance.
>> +                    // INVARIANT: `cookie` is being passed to `request_irq` as
>> +                    // the cookie. It is guaranteed to be unique by the type
>> +                    // system, since each call to `new` will return a different
>> +                    // instance of `Registration`.
>>                     cookie: this.as_ptr().cast::<c_void>(),
> 
> I believe these comments address the invariants of RegistrationInner. In
> that case, we usually place them on the struct:
> 
> // INVARIANT: `this` is a valid pointer to the `Registration` instance.
> // INVARIANT: `cookie` is being passed to `request_irq` as
> // the cookie. It is guaranteed to be unique by the type
> // system, since each call to `new` will return a different
> // instance of `Registration`.
> try_pin_init!(RegistrationInner {
> 

Also requested by Andreas, i.e.:

> >>> +/// # Invariants
> >>> +///
> >>> +/// - `self.irq` is the same as the one passed to `request_{threaded}_irq`.
> >>> +/// - `cookie` was passed to `request_{threaded}_irq` as the cookie. It is guaranteed to be unique
> >>> +/// by the type system, since each call to `new` will return a different instance of
> >>> +/// `Registration`.
> >>
> >> This seems like a mix of invariant declaration and conformance. I don't
> >> think the following belongs here:
> >>
> >> It is guaranteed to be unique
> >> by the type system, since each call to `new` will return a different instance of
> >> `Registration`.
> >>
> >> You could replace it with a uniqueness requirement.
> >
> > WDYM? This invariant is indeed provided by the type system and we do rely on it
> > to make the abstraction work.
> 
> The invariant section of the type documentation should be a list of
> invariants, not reasoning about why the invariants hold. The reasoning
> goes in the code where we construct the type, where we momentarily
> break invariants, or where we change the value of the type.
> 
> In this case, I think the invariant is that `cookie` is unique. How we
> conform to this invariant does not belong in the list. When you
> construct the type, you should have an `// INVARIANT:` comment
> explaining why the newly constructed type upholds the invariant.
> 
> At least that is how I understand intended use of the framework.



> Also, I don't really understand these comments. The first invariant is:
> 
> `self.irq` is the same as the one passed to `request_{threaded}_irq`.
> 
> and the justification for that is that `this` is a valid pointer to the
> `Registration` instance. That doesn't make sense to me because this
> comment talks about `this`/`cookie` when it should talk about `irq`.
> 
>>                     irq: {
>>                         // SAFETY:
> 
> Alice

Is the order supposed to match? I wasn’t relating these two at this particular “// INVARIANT: "

[0]: https://lore.kernel.org/rust-for-linux/87zfblurtj.fsf@t14s.mail-host-address-is-not-set/