Message-ID: <CAH5fLgiWO6u3_frPDrXP2vD0_5NRDVVV=z6VEz3sTjQ_7h1arA@mail.gmail.com>
Date: Wed, 6 Nov 2024 12:17:49 +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 Mon, Nov 4, 2024 at 9:10 PM Daniel Almeida
<daniel.almeida@...labora.com> wrote:
>
> Hi Alice, thanks for the review!
>
>
> > On 28 Oct 2024, at 12:29, Alice Ryhl <aliceryhl@...gle.com> wrote:
> >
> > 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>
> >
>
> Yeah, as I was saying, my latest patches were sent with some provisional
> commit messages. Sometimes these things slip through.
>
> In fact, as this was my first time switching to b4, it took me a while to
> realize I had sent the patches to myself only, so you can see I started off
> with the “wrong foot” here.
>
> > 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>>>.
>
> Sorry, I was under the impression that `rustdoc` would compile the examples too.
>
> >
> >> +///     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?
>
> Actually, the lifetime relationship that we want to express here is that `name` should
> live for at least as long as &self.
>
> Most of the time in C, this is solved by having `name` point to a statically allocated string,
> usually a string literal, so this version of the patch implemented that.
>
> What about:
>
> ```
> Registration<‘a> {
>         name: PhantomData<&‘a CStr>
> }
> ```
>
> Where calling register() with some c_str!(“foo”) would create a Registration<’static>
> anyways?

Ah ... what you propose would be correct but it doesn't sound useful.
Let's just keep '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
> > ```
>
> My thought process here is that Pin<Ptr<T>> dereferences to Ptr::Target, i.e. Self,
> which is then borrowed, i.e. &Self.
>
> I do not see how this can create a pointer to a pointer, but you’re right, it’s
> always good to be more explicit by spelling out the full type. I will fix that.

Ah, yes, I suppose the Pin wrapper does make it more clear. Still, I
think spelling out the type would improve the clarity in this case.

> >> +    }
> >> +}
> >> +
> >> +/// 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?
>
> Thanks for noticing this. It indeed ended up as the same type after all.
>
> >
> >> +/// 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
>
> — Daniel
>

Powered by blists - more mailing lists