linux-kernel - Re: [PATCH v7 3/6] rust: irq: add support for non-threaded IRQs and handlers

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aIEE4Tt7xtaX-9V9@tardis-2.local>
Date: Wed, 23 Jul 2025 08:50:57 -0700
From: Boqun Feng <boqun.feng@...il.com>
To: Daniel Almeida <daniel.almeida@...labora.com>
Cc: Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>,
	Gary Guo <gary@...yguo.net>,
	Björn Roy Baron <bjorn3_gh@...tonmail.com>,
	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>,
	Bjorn Helgaas <bhelgaas@...gle.com>,
	Krzysztof Wilczy´nski <kwilczynski@...nel.org>,
	Benno Lossin <lossin@...nel.org>, linux-kernel@...r.kernel.org,
	rust-for-linux@...r.kernel.org, linux-pci@...r.kernel.org
Subject: Re: [PATCH v7 3/6] rust: irq: add support for non-threaded IRQs and
 handlers

On Wed, Jul 23, 2025 at 11:54:09AM -0300, Daniel Almeida wrote:
> 
> 
> > On 23 Jul 2025, at 11:26, Boqun Feng <boqun.feng@...il.com> wrote:
> > 
> > On Wed, Jul 23, 2025 at 10:55:20AM -0300, Daniel Almeida wrote:
> >> Hi Boqun,
> >> 
> >> [...]
> >> 
> >>>> +        IrqRequest { dev, irq }
> >>>> +    }
> >>>> +
> >>>> +    /// Returns the IRQ number of an [`IrqRequest`].
> >>>> +    pub fn irq(&self) -> u32 {
> >>>> +        self.irq
> >>>> +    }
> >>>> +}
> >>>> +
> >>>> +/// A registration of an IRQ handler for a given IRQ line.
> >>>> +///
> >>>> +/// # Examples
> >>>> +///
> >>>> +/// The following is an example of using `Registration`. It uses a
> >>>> +/// [`AtomicU32`](core::sync::AtomicU32) to provide the interior mutability.
> >>> 
> >>> We are going to remove all usage of core::sync::Atomic* when the LKMM
> >>> atomics [1] land. You can probably use `Completion` here (handler does
> >>> complete_all(), and registration uses wait_for_completion()) because
> >>> `Completion` is irq-safe. And this brings my next comment..
> >> 
> >> How are completions equivalent to atomics? I am trying to highlight interior
> >> mutability in this example.
> >> 
> > 
> > Well, `Completion` also has interior mutability.
> > 
> >> Is the LKMM atomic series getting merged during the upcoming merge window? Because my
> >> understanding was that the IRQ series was ready to go in 6.17, pending a reply
> > 
> > Nope, it's likely to be in 6.18.
> > 
> >> from Thomas and some minor comments that have been mentioned in v7.
> >> 
> >> If the LKMM series is not ready yet, my proposal is to leave the
> >> Atomics->Completion change for a future patch (or really, to just use the new
> >> Atomic types introduced by your series, because again, I don't think Completion
> >> is the right thing to have there).
> >> 
> > 
> > Why? I can find a few examples that an irq handler does a
> > complete_all(), e.g. gpi_process_ch_ctrl_irq() in
> > drivers/dma/qcom/gpi.c. I think it's very normal for a driver thread to
> > use completions to wait for an irq to happen.
> > 
> > But sure, this and the handler pinned initializer thing is not a blocker
> > issue. However, I would like to see them resolved as soon as possible
> > once merged.
> > 
> > Regards,
> > Boqun
> > 
> >> 
> >> - Daniel
> 
> 
> Because it is not as explicit. The main thing we should be conveying to users
> here is how to get a &mut or otherwise mutate the data when running the
> handler. When people see AtomicU32, it's a quick jump to "I can make this work
> by using other locks, like SpinLockIrq". Completions hide this, IMHO.
> 

I understand your argument. However, I'm not sure the example of
`irq::Registration` is the right place to do this. On one hand, it's one
of the usage of interior mutability as you said, but on the other hand,
for people who are familiar with interior mutability, the difference
between `AtomicU32` and `Completion` is not that much. That's kinda my
argument why using `Completion` in the example here is fine.

Sounds reasonable?

> It's totally possible for someone to see this and say "ok, I can call
> complete() on this, but how can I mutate the data in some random T struct?",
> even though these are essentially the same thing from an interior mutability
> point of view.
> 

We probably better assume that interior mutability is commmon knowledge
or we could make an link to some documentation of interior mutability,
for example [1], in the documentation of `handler`. Not saying your
effort and consideration is not valid, but at the project level,
interior mutability should be widely acknowledged IMO.

[1]: https://doc.rust-lang.org/reference/interior-mutability.html

Regards,
Boqun

> -- Daniel
> 
>