Message-Id: <BA37BE0B-33A6-4B23-9405-ED796B451427@collabora.com>
Date: Thu, 15 May 2025 09:06:09 -0300
From: Daniel Almeida <daniel.almeida@...labora.com>
To: Benno Lossin <lossin@...nel.org>
Cc: Gary Guo <gary@...yguo.net>,
 Miguel Ojeda <ojeda@...nel.org>,
 Alex Gaynor <alex.gaynor@...il.com>,
 Boqun Feng <boqun.feng@...il.com>,
 Björn Roy Baron <bjorn3_gh@...tonmail.com>,
 Benno Lossin <benno.lossin@...ton.me>,
 Andreas Hindborg <a.hindborg@...nel.org>,
 Alice Ryhl <aliceryhl@...gle.com>,
 Trevor Gross <tmgross@...ch.edu>,
 Danilo Krummrich <dakr@...nel.org>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 "Rafael J. Wysocki" <rafael@...nel.org>,
 Thomas Gleixner <tglx@...utronix.de>,
 linux-kernel@...r.kernel.org,
 rust-for-linux@...r.kernel.org
Subject: Re: [PATCH v3 1/2] rust: irq: add support for request_irq()

Hi Benno,

> On 15 May 2025, at 05:46, Benno Lossin <lossin@...nel.org> wrote:
> 
> On Wed May 14, 2025 at 10:58 PM CEST, Daniel Almeida wrote:
>> Hi Benno, I really appreciate you taking the time to review my patches.
> 
> Glad I can help :)
> 
>>> On 14 May 2025, at 17:04, Benno Lossin <lossin@...nel.org> wrote:
>>> On Wed May 14, 2025 at 9:20 PM CEST, Daniel Almeida wrote:
>>>> Add support for registering IRQ handlers in Rust.
>>>> 
>>>> IRQ handlers are extensively used in drivers when some peripheral wants
>>>> to obtain the CPU attention. Registering a handler will make the system
>>>> invoke the passed-in function whenever the chosen IRQ line is triggered.
>>>> 
>>>> Both regular and threaded IRQ handlers are supported through a Handler
>>>> (or ThreadedHandler) trait that is meant to be implemented by a type
>>>> that:
>>>> 
>>>> a) provides a function to be run by the system when the IRQ fires and,
>>>> 
>>>> b) holds the shared data (i.e.: `T`) between process and IRQ contexts.
>>>> 
>>>> The requirement that T is Sync derives from the fact that handlers might
>>>> run concurrently with other processes executing the same driver,
>>>> creating the potential for data races.
>>>> 
>>>> Ideally, some interior mutability must be in place if T is to be
>>>> mutated. This should usually be done through the in-flight SpinLockIrq
>>>> type.
>>>> 
>>>> Co-developed-by: Alice Ryhl <aliceryhl@...gle.com>
>>>> Signed-off-by: Alice Ryhl <aliceryhl@...gle.com>
>>>> Signed-off-by: Daniel Almeida <daniel.almeida@...labora.com>
>>>> ---
>>>> rust/bindings/bindings_helper.h |   1 +
>>>> rust/helpers/helpers.c          |   1 +
>>>> rust/helpers/irq.c              |   9 +
>>>> rust/kernel/irq.rs              |  24 +++
>>>> rust/kernel/irq/flags.rs        | 102 +++++++++
>>>> rust/kernel/irq/request.rs      | 455 ++++++++++++++++++++++++++++++++++++++++
>>>> rust/kernel/lib.rs              |   1 +
>>>> 7 files changed, 593 insertions(+)
>>> 
>>> Could you split this patch into smaller chunks?
>> 
>> How? This patch does one thing: it adds request_irq functionality.
> 
> You could split off the threaded irq stuff and the flags module.
> 
> Smaller patches are much much easier to review IMO.

The flags are needed for non-threaded IRQs too.

I think this can probably be split into:

"Add IRQ module"
"Add IRQ flags" <--- in preparation for next patch
"Add non-threaded IRQs"
"Add threaded IRQs”

WDYT?

> 
>>>> +pub use request::Handler;
>>>> +pub use request::IrqReturn;
>>>> +pub use request::Registration;
>>>> +pub use request::ThreadedHandler;
>>>> +pub use request::ThreadedIrqReturn;
>>>> +pub use request::ThreadedRegistration;
>>> 
>>> Why not?:
>>> 
>>>   pub use request::{Handler, ..., ThreadedRegistration};
>> 
>> I dislike this notation. It is just a magnet for conflicts.
> 
> Rust-analyzer has the "normalize imports" code action that can help with
> that. In one of our meetings, we once discussed having a custom tool
> that could merge imports, but nobody took the time to build it.
> 
> I don't see the argument here, as this re-export shouldn't really
> change.

Yeah, in this instance that is true. 


> 
>>>> diff --git a/rust/kernel/irq/flags.rs b/rust/kernel/irq/flags.rs
>>>> new file mode 100644
>>>> index 0000000000000000000000000000000000000000..3cfaef65ae14f6c02f55ebcf4d52450c0052df30
>>>> --- /dev/null
>>>> +++ b/rust/kernel/irq/flags.rs
>>>> @@ -0,0 +1,102 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +// SPDX-FileCopyrightText: Copyright 2025 Collabora ltd.
>>>> +
>>>> +use crate::bindings;
>>>> +
>>>> +/// Flags to be used when registering IRQ handlers.
>>>> +///
>>>> +/// They can be combined with the operators `|`, `&`, and `!`.
>>>> +#[derive(Clone, Copy, PartialEq, Eq)]
>>>> +pub struct Flags(u64);
>>> 
>>> The constants below seem to all be 32 bit, why did you choose u64?
>> 
>> The C code takes in ffi::c_ulong. Shouldn’t this map to u64? Or maybe usize.
> 
> Maybe bindgen is doing some funky stuff, Gary changed the mappings a
> couple months ago (from rust/ffi.rs):
> 
>    c_long = isize;
>    c_ulong = usize;
> 
> So this indeed should be usize, what is going on here?

I think this is working as intended. The C bindgen function does take usize, so
the u64 can go away. I guess this confusion started because the individual
flags are u32 though, so at least a conversion from u32 to usize will be
needed.

> 
>>>> +/// A registration of an IRQ handler for a given IRQ line.
>>>> +///
>>>> +/// # Examples
>>>> +///
>>>> +/// The following is an example of using `Registration`. It uses a
>>>> +/// [`SpinLock`](crate::sync::SpinLockIrq) to provide the interior mutability.
>>>> +/// Note that Spinlocks are not safe to use in IRQ context as of now, but may be
>>>> +/// in the future.
>>> 
>>> Didn't your commit message mention SpinLockIrq?
>> 
>> This is not upstream yet. I removed all mentions of SpinLockIrq on
>> purpose, but I apparently missed some as you say.
>> 
>> We definitely need interrupt-aware spinlocks to access the data in interrupt
>> context. It is just a matter of deciding whether we will be referring to a type
>> whose API is not 100% formalized as we speak, or to SpinLock itself - which is
>> already upstream - with a caveat. I chose the latter.
> 
> I don't think we should knowingly do something that is "not safe". If
> this requires `SpinLockIrq`, then that should land first. I think
> referring to a not-yet-existing type is fine.

Well, SpinLockIrq has been brewing for quite a while. Waiting for it to land
can introduce an unbounded delay here. Note that IRQ handling is extremely
important for drivers.

What we can do is to simply remove the locks from all the examples. The code
will still work fine, you just won't be able to mutate the data without the
interior mutability, of course.

A subsequent patch can (re)introduce the examples where the data is mutated
when SpinLockIrq lands. WDYT?

> 
>>>> +/// # Invariants
>>>> +///
>>>> +/// * We own an irq handler using `&self` as its private data.
>>>> +///
>>>> +#[pin_data(PinnedDrop)]
>>>> +pub struct Registration<T: Handler> {
>>>> +    irq: u32,
>>>> +    #[pin]
>>>> +    handler: T,
>>>> +    #[pin]
>>>> +    /// Pinned because we need address stability so that we can pass a pointer
>>>> +    /// to the callback.
>>>> +    _pin: PhantomPinned,
>>>> +}
>>>> +
>>>> +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.
>>> 
>>> The first line of documentation should be a single sentence description
>>> of what the item does. It will get rendered next to it on the summary &
>>> search pages.
>> 
>> Right, I actually rendered the docs before submitting, but apparently I missed this.
>> 
>>> 
>>> What is meant by the second sentence? What about this phrasing?: "The
>>> handler might be called immediately after this function returns.".
>> 
>> It's a reminder, inherited from the C side, that literally as soon as
>> request_irq() returns, your handler may be called. Anything that you need to
>> setup before your handler can run must already have been done by this point.
>> 
>> Is this the “second sentence” you are referring to?
> 
> Yes. The sentence was worded in a weird way, since in Rust, I would
> normally expect that anything can happen to a value, if I give up
> ownership.
> 
> Also it is not true, since the code in the function body is only run
> once you use the initializer, so running this code doesn't do anything:
> 
>    let handler = ...;
>    let flags = ...;
>    let irg = ...;
> 
> 
>    let reg = Registration::register(irq, flags, cstr!("foo"), handler);
> 
>    // the registration is not yet created, only an initializer for a registration.
> 
>    let reg = Arc::pin_init(reg);
> 
>    // now the registration has been created and the handler can be called.

Right, this is true.

> 
>>>> +    pub fn register(
>>>> +        irq: u32,
>>>> +        flags: Flags,
>>>> +        name: &'static CStr,
>>>> +        handler: T,
>>>> +    ) -> impl PinInit<Self, Error> {
> 
>>>> +/// The value that can be returned from `ThreadedHandler::handle_irq`.
>>>> +#[repr(u32)]
>>>> +pub enum ThreadedIrqReturn {
>>>> +    /// The interrupt was not from this device or was not handled.
>>>> +    None = bindings::irqreturn_IRQ_NONE,
>>>> +
>>>> +    /// The interrupt was handled by this device.
>>>> +    Handled = bindings::irqreturn_IRQ_HANDLED,
>>>> +
>>>> +    /// The handler wants the handler thread to wake up.
>>> 
>>> How about "The handler wants the handler thread to take care of the
>>> interrupt." or is that incorrect?
>> 
>> This is taken straight from the C docs. As I told Andreas, I’d prefer if we
>> didn’t add our own docs for C things that have their own docs already.
> 
> Fine with me, but we could also change the C docs :)
> 
>>>> +///     // This will run (in a separate kthread) if and only if `handle_irq`
>>>> +///     // returns `WakeThread`.
>>>> +///     fn thread_fn(&self) -> IrqReturn {
>>>> +///         // We now have exclusive access to the data by locking the SpinLock.
>>>> +///         //
>>>> +///         // Ideally, we should disable interrupts while we are doing this to
>>>> +///         // avoid deadlocks, but this is not currently possible.
>>> 
>>> Would this be solved by SpinLockIrq?
>> 
>> 
>> Yes, that is the point of SpinLockIrq. The exact syntax is still up for debate,
>> although by this time I think Lyude & Boqun have already settled on something.
> 
> I also was part of the discussion at some point, but dropped out & was
> busy with other things. I'll move it up in my todo-list.

Ah yes, that would be very helpful. SpinLockIrq is another very important
missing piece in the ecosystem.

> 
> ---
> Cheers,
> Benno

Powered by blists - more mailing lists