Message-Id: <D9WLQ3J2TRD1.AA50TVOPKMVQ@kernel.org>
Date: Thu, 15 May 2025 10:46:22 +0200
From: "Benno Lossin" <lossin@...nel.org>
To: "Daniel Almeida" <daniel.almeida@...labora.com>, "Gary Guo"
 <gary@...yguo.net>
Cc: "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()

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.

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

>>> 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?

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

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

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

---
Cheers,
Benno

Powered by blists - more mailing lists