Message-ID: <CAH5fLgjRJtdpcOZySpRN-keLSMJjJdfXZGOhy_cEbiM3uNU7Tw@mail.gmail.com>
Date: Mon, 28 Oct 2024 16:29:36 +0100
From: Alice Ryhl <aliceryhl@...gle.com>
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>, 
	Benno Lossin <benno.lossin@...ton.me>, Andreas Hindborg <a.hindborg@...nel.org>, 
	Trevor Gross <tmgross@...ch.edu>, linux-kernel@...r.kernel.org, 
	rust-for-linux@...r.kernel.org
Subject: Re: [PATCH] rust: irq: add support for request_irq()

On Thu, Oct 24, 2024 at 4:20 PM Daniel Almeida
<daniel.almeida@...labora.com> wrote:
>
> Both regular and threaded versions are supported.
>
> Signed-off-by: Daniel Almeida <daniel.almeida@...labora.com>

I left some comments below:

> diff --git a/rust/kernel/irq/request.rs b/rust/kernel/irq/request.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..4b5c5b80c3f43d482132423c2c52cfa5696b7661
> --- /dev/null
> +++ b/rust/kernel/irq/request.rs
> @@ -0,0 +1,450 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// SPDX-FileCopyrightText: Copyright 2019 Collabora ltd.

should this be 2024?

> +/// The value that can be returned from an IrqHandler;
> +pub enum IrqReturn {
> +    /// The interrupt was not from this device or was not handled.
> +    None = bindings::irqreturn_IRQ_NONE as _,
> +
> +    /// The interrupt was handled by this device.
> +    Handled = bindings::irqreturn_IRQ_HANDLED as _,
> +}
> +
> +/// Callbacks for an IRQ handler.
> +pub trait Handler: Sync {
> +    /// The actual handler function. As usual, sleeps are not allowed in IRQ
> +    /// context.
> +    fn handle_irq(&self) -> IrqReturn;
> +}
> +
> +/// A registration of an IRQ handler for a given IRQ line.
> +///
> +/// # Invariants
> +///
> +/// * We own an irq handler using `&self` as its private data.

The invariants section is usually last.

> +/// # Examples
> +///
> +/// The following is an example of using `Registration`:
> +///
> +/// ```
> +/// use kernel::prelude::*;
> +/// use kernel::irq;
> +/// use kernel::irq::Registration;
> +/// use kernel::sync::Arc;
> +/// use kernel::sync::lock::SpinLock;
> +///
> +/// // Declare a struct that will be passed in when the interrupt fires. The u32
> +/// // merely serves as an example of some internal data.
> +/// struct Data(u32);
> +///
> +/// // [`handle_irq`] returns &self. This example illustrates interior
> +/// // mutability can be used when share the data between process context and IRQ
> +/// // context.
> +/// //
> +/// // Ideally, this example would be using a version of SpinLock that is aware
> +/// // of `spin_lock_irqsave` and `spin_lock_irqrestore`, but that is not yet
> +/// // implemented.
> +///
> +/// type Handler = SpinLock<Data>;

I doubt this will compile outside of the kernel crate. It fails the
orphan rule because your driver neither owns the SpinLock type or the
Handler trait. You should move `SpinLock` inside `Data` instead.

> +/// impl kernel::irq::Handler for Handler {
> +///     // This is executing in IRQ context in some CPU. Other CPUs can still
> +///     // try to access to data.
> +///     fn handle_irq(&self) -> irq::IrqReturn {
> +///         // We now have exclusive access to the data by locking the SpinLock.
> +///         let mut handler = self.lock();
> +///         handler.0 += 1;
> +///
> +///         IrqReturn::Handled
> +///     }
> +/// }
> +///
> +/// // This is running in process context.
> +/// fn register_irq(irq: u32, handler: Handler) -> Result<irq::Registration<Handler>> {

Please try compiling the example. The return type should be
Result<Arc<irq::Registration<Handler>>>.

> +///     let registration = Registration::register(irq, irq::flags::SHARED, "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)?;
> +///
> +///     // 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().lock().0 = 42;
> +///
> +///     Ok(registration)
> +/// }
> +///
> +/// # Ok::<(), Error>(())
> +///```
> +#[pin_data(PinnedDrop)]
> +pub struct Registration<T: Handler> {
> +    irq: u32,
> +    #[pin]
> +    handler: Opaque<T>,
> +}
> +
> +impl<T: Handler> Registration<T> {
> +    /// Registers the IRQ handler with the system for the given IRQ number. The
> +    /// handler must be able to be called as soon as this function returns.
> +    pub fn register(
> +        irq: u32,
> +        flags: Flags,
> +        name: &'static CStr,

Does the name need to be 'static?

> +        handler: T,
> +    ) -> impl PinInit<Self, Error> {
> +        try_pin_init!(Self {
> +            irq,
> +            handler: Opaque::new(handler)
> +        })
> +        .pin_chain(move |slot| {
> +            // SAFETY:
> +            // - `handler` points to a valid function defined below.
> +            // - only valid flags can be constructed using the `flags` module.
> +            // - `devname` is a nul-terminated string with a 'static lifetime.
> +            // - `ptr` is a cookie used to identify the handler. The same cookie is
> +            // passed back when the system calls the handler.
> +            to_result(unsafe {
> +                bindings::request_irq(
> +                    irq,
> +                    Some(handle_irq_callback::<T>),
> +                    flags.0,
> +                    name.as_char_ptr(),
> +                    &*slot as *const _ as *mut core::ffi::c_void,

Can simplify to `slot as *mut c_void` or `slot.cast()`.

> +                )
> +            })?;
> +
> +            Ok(())
> +        })
> +    }
> +
> +    /// Returns a reference to the handler that was registered with the system.
> +    pub fn handler(&self) -> &T {
> +        // SAFETY: `handler` is initialized in `register`.
> +        unsafe { &*self.handler.get() }

This relies on T being Sync as it could also get accessed by the irq
handler in parallel. You probably want the SAFETY comment to mention
that.

> +    }
> +}
> +
> +#[pinned_drop]
> +impl<T: Handler> PinnedDrop for Registration<T> {
> +    fn drop(self: Pin<&mut Self>) {
> +        // SAFETY:
> +        // - `self.irq` is the same as the one passed to `reques_irq`.
> +        // -  `&self` was passed to `request_irq` as the cookie. It is
> +        // guaranteed to be unique by the type system, since each call to
> +        // `register` will return a different instance of `Registration`.
> +        //
> +        // Notice that this will block until all handlers finish executing, so,
> +        // at no point will &self be invalid while the handler is running.
> +        unsafe { bindings::free_irq(self.irq, &*self as *const _ as *mut core::ffi::c_void) };

I can't tell if this creates a pointer to the Registration or a
pointer to a pointer to the Registration. Please spell out the type:
```
&*self as *const Self as *mut core::ffi::c_void
```

> +    }
> +}
> +
> +/// The value that can be returned from `ThreadedHandler::handle_irq`.
> +pub enum ThreadedIrqReturn {
> +    /// The interrupt was not from this device or was not handled.
> +    None = bindings::irqreturn_IRQ_NONE as _,
> +
> +    /// The interrupt was handled by this device.
> +    Handled = bindings::irqreturn_IRQ_HANDLED as _,
> +
> +    /// The handler wants the handler thread to wake up.
> +    WakeThread = bindings::irqreturn_IRQ_WAKE_THREAD as _,
> +}
> +
> +/// The value that can be returned from `ThreadedFnHandler::thread_fn`.
> +pub enum ThreadedFnReturn {
> +    /// The thread function did not make any progress.
> +    None = bindings::irqreturn_IRQ_NONE as _,
> +
> +    /// The thread function ran successfully.
> +    Handled = bindings::irqreturn_IRQ_HANDLED as _,
> +}

This is the same as IrqReturn?

> +/// Callbacks for a threaded IRQ handler.
> +pub trait ThreadedHandler: Sync {
> +    /// The actual handler function. As usual, sleeps are not allowed in IRQ
> +    /// context.
> +    fn handle_irq(&self) -> ThreadedIrqReturn;
> +
> +    /// The threaded handler function. This function is called from the irq
> +    /// handler thread, which is automatically created by the system.
> +    fn thread_fn(&self) -> ThreadedFnReturn;
> +}

Most of my comments above also reply to ThreadedHandler.

Alice

Powered by blists - more mailing lists