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

["Danilo Krummrich" <dakr@kernel.org>]

On Mon Jul 21, 2025 at 5:10 PM CEST, Daniel Almeida wrote:
> Hi Alice, thanks for looking into this again :)
>
>
> […]
>
>>> diff --git a/rust/kernel/irq/request.rs b/rust/kernel/irq/request.rs
>>> new file mode 100644
>>> index 0000000000000000000000000000000000000000..2f4637d8bc4c9fda23cbc8307687035957b0042a
>>> --- /dev/null
>>> +++ b/rust/kernel/irq/request.rs
>>> @@ -0,0 +1,267 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +// SPDX-FileCopyrightText: Copyright 2025 Collabora ltd.
>>> +
>>> +//! This module provides types like [`Registration`] which allow users to
>>> +//! register handlers for a given IRQ line.
>>> +
>>> +use core::marker::PhantomPinned;
>>> +
>>> +use crate::alloc::Allocator;
>>> +use crate::device::Bound;
>>> +use crate::device::Device;
>> 
>> The usual style is to write this as:
>> 
>> use crate::device::{Bound, Device};
>
> I dislike this syntax because I think it is a conflict magnet. Moreover, when
> you get conflicts, they are harder to solve than they are when each import
> is in its own line, at least IMHO.  

Intuitively, I would agree. However, I think practically it's not that bad.

While it's true that Rust has generally more conflict potential - especially in
the current phase - my feeling hasn't been that includes produce significantly
more conflicts then any other code so far.

> In any case, I don't think we have a guideline for imports at the moment?

No, but I think we should try to be as consistent as possible (at least within a
a certain logical unit, e.g. subsystem, module, etc.). Not sure where exactly
the IRQ stuff will end up yet. :)

>>> +/// 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.
>>> +///
>>> +/// ```
>>> +/// use core::sync::atomic::AtomicU32;
>>> +/// use core::sync::atomic::Ordering;
>>> +///
>>> +/// use kernel::prelude::*;
>>> +/// use kernel::device::Bound;
>>> +/// use kernel::irq::flags;
>>> +/// use kernel::irq::Registration;
>>> +/// use kernel::irq::IrqRequest;
>>> +/// use kernel::irq::IrqReturn;
>> 
>> /// use kernel::irq::{Flags, IrqRequest, IrqReturn, Registration};
>
> Same here. I’d rather not do this, if it’s ok with others.
>
> — Daniel