Message-Id: <608A7955-1A23-4559-AE93-FBBD34A9B01E@collabora.com>
Date: Fri, 9 May 2025 10:58:46 -0300
From: Daniel Almeida <daniel.almeida@...labora.com>
To: Andreas Hindborg <a.hindborg@...nel.org>
Cc: ojeda@...nel.org,
 alex.gaynor@...il.com,
 boqun.feng@...il.com,
 gary@...yguo.net,
 bjorn3_gh@...tonmail.com,
 benno.lossin@...ton.me,
 tmgross@...ch.edu,
 rust-for-linux@...r.kernel.org,
 linux-kernel@...r.kernel.org,
 Alice Ryhl <aliceryhl@...gle.com>
Subject: Re: [PATCH v2] rust: irq: add support for request_irq()

Hi Andreas,

I am going to work on a new iteration of this patch. Are you satisfied with the
answers Alice gave you w.r.t ForeignOwnable? My take is the same as hers: we
don't need it here. The code you linked to is more restrictive in the sense
that it requires an allocation, which is not the case currently.

Her solution will cater to the use case you've described, so this should be OK
on your end?

> On 4 Mar 2025, at 10:43, Andreas Hindborg <a.hindborg@...nel.org> wrote:
> 
> "Daniel Almeida" <daniel.almeida@...labora.com> writes:
> 
>> 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>
> 
> What is the base commit to apply this patch to? I am getting some
> compiler errors when trying it out:
> 
>    error[E0308]: mismatched types
>      --> /home/aeh/src/linux-rust/linux/rust/kernel/irq/request.rs:240:21
>        |
>    237 |                 bindings::request_irq(
>        |                 --------------------- arguments to this function are incorrect
>    ...
>    240 |                     flags.0,
>        |                     ^^^^^^^ expected `usize`, found `u64`
>        |
> 
> [...]

I will rebase on top of the newest rc for 6.15

> 
>> diff --git a/rust/kernel/irq.rs b/rust/kernel/irq.rs
>> new file mode 100644
>> index 000000000000..3ab83c5bdb83
>> --- /dev/null
>> +++ b/rust/kernel/irq.rs
>> @@ -0,0 +1,6 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +//! IRQ abstractions
> 
> We could do with a longer story here. Also, missing a period.
> 
>> +
>> +/// IRQ allocation and handling
>> +pub mod request;
>> diff --git a/rust/kernel/irq/request.rs b/rust/kernel/irq/request.rs
>> new file mode 100644
>> index 000000000000..61e7d4a8f555
>> --- /dev/null
>> +++ b/rust/kernel/irq/request.rs
>> @@ -0,0 +1,517 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// SPDX-FileCopyrightText: Copyright 2025 Collabora ltd.
>> +
>> +//! IRQ allocation and handling
>> +
>> +use core::marker::PhantomPinned;
>> +use core::ptr::addr_of_mut;
>> +
>> +use init::pin_init_from_closure;
>> +
>> +use crate::error::to_result;
>> +use crate::prelude::*;
>> +use crate::str::CStr;
>> +
>> +/// Flags to be used when registering IRQ handlers.
>> +///
>> +/// They can be combined with the operators `|`, `&`, and `!`.
>> +///
>> +/// Values can be used from the [`flags`] module.
>> +#[derive(Clone, Copy)]
>> +pub struct Flags(u64);
>> +
>> +impl core::ops::BitOr for Flags {
>> +    type Output = Self;
>> +    fn bitor(self, rhs: Self) -> Self::Output {
>> +        Self(self.0 | rhs.0)
>> +    }
>> +}
>> +
>> +impl core::ops::BitAnd for Flags {
>> +    type Output = Self;
>> +    fn bitand(self, rhs: Self) -> Self::Output {
>> +        Self(self.0 & rhs.0)
>> +    }
>> +}
>> +
>> +impl core::ops::Not for Flags {
>> +    type Output = Self;
>> +    fn not(self) -> Self::Output {
>> +        Self(!self.0)
>> +    }
>> +}
>> +
>> +/// The flags that can be used when registering an IRQ handler.
>> +pub mod flags {
>> +    use super::Flags;
>> +
>> +    use crate::bindings;
>> +
>> +    /// Use the interrupt line as already configured.
>> +    pub const TRIGGER_NONE: Flags = Flags(bindings::IRQF_TRIGGER_NONE as _);
> 
> What is the reason for the `as _` in all these? Should the flag type be
> something else?

I think this was before Gary’s patch to improve the mapping of FFI types in bindgen.

> 
>> +
>> +    /// The interrupt is triggered when the signal goes from low to high.
>> +    pub const TRIGGER_RISING: Flags = Flags(bindings::IRQF_TRIGGER_RISING as _);
>> +
>> +    /// The interrupt is triggered when the signal goes from high to low.
>> +    pub const TRIGGER_FALLING: Flags = Flags(bindings::IRQF_TRIGGER_FALLING as _);
>> +
>> +    /// The interrupt is triggered while the signal is held high.
>> +    pub const TRIGGER_HIGH: Flags = Flags(bindings::IRQF_TRIGGER_HIGH as _);
>> +
>> +    /// The interrupt is triggered while the signal is held low.
>> +    pub const TRIGGER_LOW: Flags = Flags(bindings::IRQF_TRIGGER_LOW as _);
>> +
>> +    /// Allow sharing the irq among several devices.
>> +    pub const SHARED: Flags = Flags(bindings::IRQF_SHARED as _);
>> +
>> +    /// Set by callers when they expect sharing mismatches to occur.
> 
> What is a sharing mismatch?
> 
>> +    pub const PROBE_SHARED: Flags = Flags(bindings::IRQF_PROBE_SHARED as _);
>> +
>> +    /// Flag to mark this interrupt as timer interrupt.
> 
> The "Flag to ..." strikes me as odd when most of the other descriptions
> have a different wording.
> 
>> +    pub const TIMER: Flags = Flags(bindings::IRQF_TIMER as _);
>> +
>> +    /// Interrupt is per cpu.
>> +    pub const PERCPU: Flags = Flags(bindings::IRQF_PERCPU as _);
>> +
>> +    /// Flag to exclude this interrupt from irq balancing.
>> +    pub const NOBALANCING: Flags = Flags(bindings::IRQF_NOBALANCING as _);
>> +
>> +    /// Interrupt is used for polling (only the interrupt that is registered
>> +    /// first in a shared interrupt is considered for performance reasons).
>> +    pub const IRQPOLL: Flags = Flags(bindings::IRQF_IRQPOLL as _);
>> +
>> +    /// Interrupt is not reenabled after the hardirq handler finished. Used by
>> +    /// threaded interrupts which need to keep the irq line disabled until the
>> +    /// threaded handler has been run.
>> +    pub const ONESHOT: Flags = Flags(bindings::IRQF_ONESHOT as _);
>> +
>> +    /// Do not disable this IRQ during suspend. Does not guarantee that this
>> +    /// interrupt will wake the system from a suspended state.
>> +    pub const NO_SUSPEND: Flags = Flags(bindings::IRQF_NO_SUSPEND as _);
>> +
>> +    /// Force enable it on resume even if [`NO_SUSPEND`] is set.
> 
> Perhaps: Force enable the interrupt even if ...


All your comments above are addressing the documentation that was deliberately
copied and pasted from C. IMHO, we should refrain from introducing our own
documentation in this case because it will just confuse users. Any changes here
should be proposed to the C side directly.

> 
>> +    pub const FORCE_RESUME: Flags = Flags(bindings::IRQF_FORCE_RESUME as _);
>> +
>> +    /// Interrupt cannot be threaded.
>> +    pub const NO_THREAD: Flags = Flags(bindings::IRQF_NO_THREAD as _);
>> +
>> +    /// Resume IRQ early during syscore instead of at device resume time.
>> +    pub const EARLY_RESUME: Flags = Flags(bindings::IRQF_EARLY_RESUME as _);
>> +
>> +    /// If the IRQ is shared with a NO_SUSPEND user, execute this interrupt
> 
> Please link `NO_SUSPEND`.

Sure

> 
>> +    /// handler after suspending interrupts. For system wakeup devices users
>> +    /// need to implement wakeup detection in their interrupt handlers.
>> +    pub const COND_SUSPEND: Flags = Flags(bindings::IRQF_COND_SUSPEND as _);
>> +
>> +    /// Don't enable IRQ or NMI automatically when users request it. Users will
>> +    /// enable it explicitly by `enable_irq` or `enable_nmi` later.
>> +    pub const NO_AUTOEN: Flags = Flags(bindings::IRQF_NO_AUTOEN as _);
>> +
>> +    /// Exclude from runnaway detection for IPI and similar handlers, depends on
>> +    /// `PERCPU`.
>> +    pub const NO_DEBUG: Flags = Flags(bindings::IRQF_NO_DEBUG as _);
>> +}
>> +
>> +/// The value that can be returned from an IrqHandler or a ThreadedIrqHandler.
>> +pub enum IrqReturn {
> 
> I learned recently that if you choose the right representation here, you
> don't need to cast here and when you call `Handler::handle_irq`. I think
> `#[repr(u32)]` is the one to use here.

Thanks!

> 
>> +    /// 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;
>> +}
> 
> What is the reason for moving away from the following:
> 
> 
>    pub trait Handler {
>        /// The context data associated with and made available to the handler.
>        type Data: ForeignOwnable;
> 
>        /// Called from interrupt context when the irq happens.
>        fn handle_irq(data: <Self::Data as ForeignOwnable>::Borrowed<'_>) -> Return;
>    }
> 
> 
> I think we will run into problems if we want to pass `Arc<Foo>` as the
> handler. I don't think we can `impl Handler for Arc<Foo>` in a driver
> crate, since both `Handler` and `Arc` are defined in external crates
> 
> 
> [...]
> 
>> +#[pin_data(PinnedDrop)]
>> +pub struct ThreadedRegistration<T: ThreadedHandler> {
>> +    irq: u32,
>> +    #[pin]
>> +    handler: T,
>> +    #[pin]
>> +    /// Pinned because we need address stability so that we can pass a pointer
>> +    /// to the callback.
>> +    _pin: PhantomPinned,
>> +}
> 
> As others have mentioned, I wonder if we can avoid the code duplication
> that makes up most of the rest of this patch.
> 
> 
> Best regards,
> Andreas Hindborg

— Daniel

Powered by blists - more mailing lists