Message-Id: <D9WQS8SVDO0V.2DS5K831HCP7X@kernel.org>
Date: Thu, 15 May 2025 14:44:16 +0200
From: "Benno Lossin" <lossin@...nel.org>
To: "Daniel Almeida" <daniel.almeida@...labora.com>
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()

On Thu May 15, 2025 at 2:06 PM CEST, Daniel Almeida wrote:
>>>>> 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.

Oh yeah, I didn't suggest an order, but rather a set of things :)

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

Yeah that looks good.

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

Ah we were talking about two different things. The functions take
`c_ulong`, but the definitions are pre-processor macros and thus don't
have a type annotation. That's why bindgen uses `u32`, because that's
the type that the constant fits in (AFAIK there are also some weird
rules in C about this? but don't quote me on that).

One way would be to turn the `#define`s into constants, but from
previous discussions I remember there were some reasons to not do that,
so I have no idea. The casts are unfortunate, but if we can't change the
C side to include the types in these constants, then it's fine.

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

But is it useful even without `SpinLockIrq`?

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

Do atomics work normally in IRQ context? (I know that Boqun's atomic
series also needs some work, so maybe not a good alternative)

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

I think that is better than including wrong examples with a caveat, as
some people might not read it or assume that the restriction somehow
doesn't apply to them.

---
Cheers,
Benno

Powered by blists - more mailing lists