Message-ID: <aEbJt0YSc3-60OBY@pollux>
Date: Mon, 9 Jun 2025 13:47:03 +0200
From: Danilo Krummrich <dakr@...nel.org>
To: Daniel Almeida <daniel.almeida@...labora.com>
Cc: Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>,
	Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
	Björn Roy Baron <bjorn3_gh@...tonmail.com>,
	Andreas Hindborg <a.hindborg@...nel.org>,
	Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	"Rafael J. Wysocki" <rafael@...nel.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Benno Lossin <lossin@...nel.org>,
	Bjorn Helgaas <bhelgaas@...gle.com>,
	Krzysztof Wilczyński <kwilczynski@...nel.org>,
	linux-kernel@...r.kernel.org, rust-for-linux@...r.kernel.org,
	linux-pci@...r.kernel.org
Subject: Re: [PATCH v4 3/6] rust: irq: add support for non-threaded IRQs and
 handlers

On Sun, Jun 08, 2025 at 07:51:08PM -0300, Daniel Almeida wrote:
> +/// // This is running in process context.
> +/// fn register_irq(handler: Handler, dev: &platform::Device<Bound>) -> Result<Arc<Registration<Handler>>> {
> +///     let registration = dev.irq_by_index(0, flags::SHARED, c_str!("my-device"), handler)?;
> +///
> +///     // You can have as many references to the registration as you want, so
> +///     // multiple parts of the driver can access it.
> +///     let registration = Arc::pin_init(registration, GFP_KERNEL)?;
> +///
> +///     // The handler may be called immediately after the function above
> +///     // returns, possibly in a different CPU.
> +///
> +///     {
> +///         // The data can be accessed from the process context too.
> +///         registration.handler().0.fetch_add(1, Ordering::Relaxed);
> +///     }

Why the extra scope?

> +///
> +///     Ok(registration)
> +/// }
> +///
> +/// # Ok::<(), Error>(())
> +///```
> +///
> +/// # Invariants
> +///
> +/// * We own an irq handler using `&self` as its private data.
> +///
> +#[pin_data]
> +pub struct Registration<T: Handler + 'static> {
> +    inner: Devres<RegistrationInner>,
> +
> +    #[pin]
> +    handler: T,
> +
> +    /// Pinned because we need address stability so that we can pass a pointer
> +    /// to the callback.
> +    #[pin]
> +    _pin: PhantomPinned,
> +}
> +
> +impl<T: Handler + 'static> Registration<T> {
> +    /// Registers the IRQ handler with the system for the given IRQ number.
> +    pub(crate) fn register<'a>(

I think we should call this Registration::new() instead. Except for
MiscDeviceRegistration, which is representing not *only* a registration, all
other Registration types just use new() and it'd be nice to be consistent.

> +        dev: &'a Device<Bound>,
> +        irq: u32,
> +        flags: Flags,
> +        name: &'static CStr,
> +        handler: T,
> +    ) -> impl PinInit<Self, Error> + 'a {
> +        let closure = move |slot: *mut Self| {
> +            // SAFETY: The slot passed to pin initializer is valid for writing.
> +            unsafe {
> +                slot.write(Self {
> +                    inner: Devres::new(
> +                        dev,
> +                        RegistrationInner {
> +                            irq,
> +                            cookie: slot.cast(),
> +                        },
> +                        GFP_KERNEL,
> +                    )?,
> +                    handler,
> +                    _pin: PhantomPinned,
> +                })
> +            };
> +
> +            // 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.
> +            let res = to_result(unsafe {
> +                bindings::request_irq(
> +                    irq,
> +                    Some(handle_irq_callback::<T>),
> +                    flags.into_inner() as usize,
> +                    name.as_char_ptr(),
> +                    slot.cast(),
> +                )
> +            });
> +
> +            if res.is_err() {
> +                // SAFETY: We are returning an error, so we can destroy the slot.
> +                unsafe { core::ptr::drop_in_place(&raw mut (*slot).handler) };
> +            }
> +
> +            res
> +        };
> +
> +        // SAFETY:
> +        // - if this returns Ok, then every field of `slot` is fully
> +        // initialized.
> +        // - if this returns an error, then the slot does not need to remain
> +        // valid.
> +        unsafe { pin_init_from_closure(closure) }

Can't we use try_pin_init!() instead, move request_irq() into the initializer of
RegistrationInner and initialize inner last?

> +    }
> +
> +    /// Returns a reference to the handler that was registered with the system.
> +    pub fn handler(&self) -> &T {
> +        &self.handler
> +    }
> +
> +    /// Wait for pending IRQ handlers on other CPUs.
> +    ///
> +    /// This will attempt to access the inner [`Devres`] container.
> +    pub fn try_synchronize(&self) -> Result {
> +        let inner = self.inner.try_access().ok_or(ENODEV)?;
> +        inner.synchronize();
> +        Ok(())
> +    }
> +
> +    /// Wait for pending IRQ handlers on other CPUs.
> +    pub fn synchronize(&self, dev: &Device<Bound>) -> Result {
> +        let inner = self.inner.access(dev)?;
> +        inner.synchronize();
> +        Ok(())
> +    }
> +}
> +
> +/// # Safety
> +///
> +/// 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 core::ffi::c_void,
> +) -> core::ffi::c_uint {
> +    // SAFETY: `ptr` is a pointer to Registration<T> set in `Registration::new`
> +    let data = unsafe { &*(ptr as *const Registration<T>) };
> +    T::handle_irq(&data.handler).into_inner()
> +}
> 
> -- 
> 2.49.0
> 

Powered by blists - more mailing lists