Message-Id: <45D09BD8-208C-4195-BACB-95B0922C5324@collabora.com>
Date: Mon, 4 Nov 2024 17:10:30 -0300
From: Daniel Almeida <daniel.almeida@...labora.com>
To: Alice Ryhl <aliceryhl@...gle.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()

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?


>> +        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.


> 
>> +    }
>> +}
>> +
>> +/// 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