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

[Alice Ryhl <aliceryhl@google.com>]

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