Message-Id: <9FDC8FB8-B8DA-4647-A602-4732EFA4CCD6@collabora.com>
Date: Fri, 15 Aug 2025 10:23:55 -0300
From: Daniel Almeida <daniel.almeida@...labora.com>
To: Andreas Hindborg <a.hindborg@...nel.org>
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>,
 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>,
 Bjorn Helgaas <bhelgaas@...gle.com>,
 Krzysztof Wilczyński <kwilczynski@...nel.org>,
 Benno Lossin <lossin@...nel.org>,
 linux-kernel@...r.kernel.org,
 rust-for-linux@...r.kernel.org,
 linux-pci@...r.kernel.org,
 Joel Fernandes <joelagnelf@...dia.com>,
 Dirk Behme <dirk.behme@...bosch.com>
Subject: Re: [PATCH v9 2/7] rust: irq: add flags module



> On 15 Aug 2025, at 09:02, Andreas Hindborg <a.hindborg@...nel.org> wrote:
> 
> "Daniel Almeida" <daniel.almeida@...labora.com> writes:
> 
>> Manipulating IRQ flags (i.e.: IRQF_*) will soon be necessary, specially to
>> register IRQ handlers through bindings::request_irq().
>> 
>> Add a kernel::irq::Flags for that purpose.
>> 
>> Reviewed-by: Alice Ryhl <aliceryhl@...gle.com>
>> Tested-by: Joel Fernandes <joelagnelf@...dia.com>
>> Tested-by: Dirk Behme <dirk.behme@...bosch.com>
>> Signed-off-by: Daniel Almeida <daniel.almeida@...labora.com>
>> ---
>> rust/kernel/irq.rs       |   5 ++
>> rust/kernel/irq/flags.rs | 124 +++++++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 129 insertions(+)
>> 
>> diff --git a/rust/kernel/irq.rs b/rust/kernel/irq.rs
>> index fae7b15effc80c936d6bffbd5b4150000d6c2898..068df2fea31de51115c30344f7ebdb4da4ad86cc 100644
>> --- a/rust/kernel/irq.rs
>> +++ b/rust/kernel/irq.rs
>> @@ -9,3 +9,8 @@
>> //! drivers to register a handler for a given IRQ line.
>> //!
>> //! C header: [`include/linux/device.h`](srctree/include/linux/interrupt.h)
>> +
>> +/// Flags to be used when registering IRQ handlers.
>> +mod flags;
>> +
>> +pub use flags::Flags;
>> diff --git a/rust/kernel/irq/flags.rs b/rust/kernel/irq/flags.rs
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..e62820ea67755123b4f96e4331244bbb4fbcfd9d
>> --- /dev/null
>> +++ b/rust/kernel/irq/flags.rs
>> @@ -0,0 +1,124 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// SPDX-FileCopyrightText: Copyright 2025 Collabora ltd.
>> +
>> +use crate::bindings;
>> +use crate::prelude::*;
>> +
>> +/// Flags to be used when registering IRQ handlers.
>> +///
>> +/// Flags can be used to request specific behaviors when registering an IRQ
>> +/// handler, and can be combined using the `|`, `&`, and `!` operators to
>> +/// further control the system's behavior.
>> +///
>> +/// A common use case is to register a shared interrupt, as sharing the line
>> +/// between devices is increasingly common in modern systems and is even
>> +/// required for some buses. This requires setting [`Flags::SHARED`] when
>> +/// requesting the interrupt. Other use cases include setting the trigger type
>> +/// through `Flags::TRIGGER_*`, which determines when the interrupt fires, or
>> +/// controlling whether the interrupt is masked after the handler runs by using
>> +/// [`Flags::ONESHOT`].
>> +///
>> +/// If an invalid combination of flags is provided, the system will refuse to
>> +/// register the handler, and lower layers will enforce certain flags when
>> +/// necessary. This means, for example, that all the
>> +/// [`crate::irq::Registration`] for a shared interrupt have to agree on
> 
> `rustdoc` will complain about this being undefined.

I think Danilo fixed this before applying. As I said a few days ago, I just ran
rustdoc on the final result, not on the individual commits. My bad, I’ll
pay attention to this next time.

> 
>> +/// [`Flags::SHARED`] and on the same trigger type, if set.
>> +#[derive(Clone, Copy, PartialEq, Eq)]
>> +pub struct Flags(c_ulong);
>> +
>> +impl Flags {
>> +    /// Use the interrupt line as already configured.
>> +    pub const TRIGGER_NONE: Flags = Flags::new(bindings::IRQF_TRIGGER_NONE);
>> +
>> +    /// The interrupt is triggered when the signal goes from low to high.
>> +    pub const TRIGGER_RISING: Flags = Flags::new(bindings::IRQF_TRIGGER_RISING);
>> +
>> +    /// The interrupt is triggered when the signal goes from high to low.
>> +    pub const TRIGGER_FALLING: Flags = Flags::new(bindings::IRQF_TRIGGER_FALLING);
>> +
>> +    /// The interrupt is triggered while the signal is held high.
>> +    pub const TRIGGER_HIGH: Flags = Flags::new(bindings::IRQF_TRIGGER_HIGH);
>> +
>> +    /// The interrupt is triggered while the signal is held low.
>> +    pub const TRIGGER_LOW: Flags = Flags::new(bindings::IRQF_TRIGGER_LOW);
>> +
>> +    /// Allow sharing the IRQ among several devices.
>> +    pub const SHARED: Flags = Flags::new(bindings::IRQF_SHARED);
>> +
>> +    /// Set by callers when they expect sharing mismatches to occur.
>> +    pub const PROBE_SHARED: Flags = Flags::new(bindings::IRQF_PROBE_SHARED);
>> +
>> +    /// Flag to mark this interrupt as timer interrupt.
>> +    pub const TIMER: Flags = Flags::new(bindings::IRQF_TIMER);
>> +
>> +    /// Interrupt is per CPU.
>> +    pub const PERCPU: Flags = Flags::new(bindings::IRQF_PERCPU);
>> +
>> +    /// Flag to exclude this interrupt from irq balancing.
>> +    pub const NOBALANCING: Flags = Flags::new(bindings::IRQF_NOBALANCING);
>> +
>> +    /// 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::new(bindings::IRQF_IRQPOLL);
>> +
>> +    /// 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::new(bindings::IRQF_ONESHOT);
>> +
>> +    /// 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::new(bindings::IRQF_NO_SUSPEND);
>> +
>> +    /// Force enable it on resume even if [`Flags::NO_SUSPEND`] is set.
>> +    pub const FORCE_RESUME: Flags = Flags::new(bindings::IRQF_FORCE_RESUME);
>> +
>> +    /// Interrupt cannot be threaded.
>> +    pub const NO_THREAD: Flags = Flags::new(bindings::IRQF_NO_THREAD);
>> +
>> +    /// Resume IRQ early during syscore instead of at device resume time.
>> +    pub const EARLY_RESUME: Flags = Flags::new(bindings::IRQF_EARLY_RESUME);
>> +
>> +    /// If the IRQ is shared with a [`Flags::NO_SUSPEND`] user, execute this
>> +    /// interrupt handler after suspending interrupts. For system wakeup devices
>> +    /// users need to implement wakeup detection in their interrupt handlers.
>> +    pub const COND_SUSPEND: Flags = Flags::new(bindings::IRQF_COND_SUSPEND);
>> +
>> +    /// 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::new(bindings::IRQF_NO_AUTOEN);
>> +
>> +    /// Exclude from runnaway detection for IPI and similar handlers, depends on
>> +    /// `PERCPU`.
> 
> Should we link `PERCPU` here?
> 
>> +    pub const NO_DEBUG: Flags = Flags::new(bindings::IRQF_NO_DEBUG);
>> +
>> +    pub(crate) fn into_inner(self) -> c_ulong {
> 
> You need `#[expect(dead_code)]` here.
> 
>> +        self.0
>> +    }
>> +
>> +    const fn new(value: u32) -> Self {
>> +        build_assert!(value as u64 <= c_ulong::MAX as u64);
> 
> I am curious about this line. Can you add a short explanation?
> 
> I would think this can never assert. That would require c_ulong to be
> less than 32 bits, right? Are there any configurations where that is the case?

This is just a way to validate the cast at build time. IMHO, regardless of
whether this can possibly trigger, it's always nice to err on the side of
caution, specially because this type doesn't have a fixed bit width.

> 
> 
> Best regards,
> Andreas Hindborg

Powered by blists - more mailing lists