Re: [PATCH v6 3/6] rust: irq: add support for non-threaded IRQs and handlers

[Boqun Feng <boqun.feng@gmail.com>]

On Sun, Jul 13, 2025 at 02:42:41PM +0200, Danilo Krummrich wrote:
[...]
> If we use container_of!() instead or just pass the address of Self (i.e.
> Registration) to request_irq() instead,
> 
> 	pub fn device(&self) -> &Device
> 
> is absolutely possible to add to Devres, of course.
> 

One thing to notice is that in `Devres::new()`, `inner` is initialized
before `dev`:

        try_pin_init!(&this in Self {
            // INVARIANT: `inner` is properly initialized.
            inner <- Opaque::pin_init(try_pin_init!(Inner {
                data <- Revocable::new(data),
                devm <- Completion::new(),
                revoke <- Completion::new(),
            })),
	    
For `irq::Registration`, request_irq() is called at `inner`
initialization. So now interrupts can happen at any moment, but `dev` is
still uninitialized.

            callback,
            dev: {
                // SAFETY: `this` is a valid pointer to uninitialized memory.
                let inner = unsafe { &raw mut (*this.as_ptr()).inner };

                // SAFETY:
                // - `dev.as_raw()` is a pointer to a valid bound device.
                // - `inner` is guaranteed to be a valid for the duration of the lifetime of `Self`.
                // - `devm_add_action()` is guaranteed not to call `callback` until `this` has been
                //    properly initialized, because we require `dev` (i.e. the *bound* device) to
                //    live at least as long as the returned `impl PinInit<Self, Error>`.
                to_result(unsafe {
                    bindings::devm_add_action(dev.as_raw(), Some(callback), inner.cast())
                })?;

                dev.into()
            },
        })

I think you need to reorder the initialization of `inner` to be after
`dev` for this. And it should be fine, because the whole device is in
bound state while the PinInit being called, so `inner.cast()` being a
pointer to uninitialized memory should be fine (because the `callback`
won't be called).

Regards,
Boqun

> > Depending on how (1) is ensured, we might just need an unsafe function
> > that turns `Device<Normal>` into `Device<Bound>`.
> 
> `&Device<Normal>` in `&Device<Bound>`, yes. I have such a method locally
> already (but haven't sent it yet), because that's going to be a use-case for
> other abstractions as well. One specific example is the PWM Chip abstraction
> [1].
> 
> [1] https://lore.kernel.org/lkml/20250710-rust-next-pwm-working-fan-for-sending-v11-3-93824a16f9ec@samsung.com/