Message-ID: <CAH5fLgj+za85ajgNwJepoa7PSFkMm+3J2wJJVJ24m6YZoFVmVw@mail.gmail.com>
Date: Mon, 16 Jun 2025 15:33:06 +0200
From: Alice Ryhl <aliceryhl@...gle.com>
To: Danilo Krummrich <dakr@...nel.org>
Cc: Daniel Almeida <daniel.almeida@...labora.com>, 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>, 
	Andreas Hindborg <a.hindborg@...nel.org>, Trevor Gross <tmgross@...ch.edu>, 
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>, "Rafael J. Wysocki" <rafael@...nel.org>, 
	Thomas Gleixner <tglx@...utronix.de>, Benno Lossin <lossin@...nel.org>, 
	Bjorn Helgaas <bhelgaas@...gle.com>, Krzysztof Wilczyński <kwilczynski@...nel.org>, 
	linux-kernel@...r.kernel.org, rust-for-linux@...r.kernel.org, 
	linux-pci@...r.kernel.org
Subject: Re: [PATCH v4 4/6] rust: irq: add support for threaded IRQs and handlers

On Mon, Jun 9, 2025 at 8:13 PM Danilo Krummrich <dakr@...nel.org> wrote:
>
> On Mon, Jun 09, 2025 at 01:24:40PM -0300, Daniel Almeida wrote:
> > > On 9 Jun 2025, at 09:27, Danilo Krummrich <dakr@...nel.org> wrote:
> > >> +#[pin_data]
> > >> +pub struct ThreadedRegistration<T: ThreadedHandler + 'static> {
> > >> +    inner: Devres<RegistrationInner>,
> > >> +
> > >> +    #[pin]
> > >> +    handler: T,
> > >> +
> > >> +    /// Pinned because we need address stability so that we can pass a pointer
> > >> +    /// to the callback.
> > >> +    #[pin]
> > >> +    _pin: PhantomPinned,
> > >> +}
> > >
> > > Most of the code in this file is a duplicate of the non-threaded registration.
> > >
> > > I think this would greatly generalize with specialization and an HandlerInternal
> > > trait.
> > >
> > > Without specialization I think we could use enums to generalize.
> > >
> > > The most trivial solution would be to define the Handler trait as
> > >
> > > trait Handler {
> > >   fn handle(&self);
> > >   fn handle_threaded(&self) {};
> > > }
> > >
> > > but that's pretty dodgy.
> >
> > A lot of the comments up until now have touched on somehow having threaded and
> > non-threaded versions implemented together. I personally see no problem in
> > having things duplicated here, because I think it's easier to reason about what
> > is going on this way. Alice has expressed a similar view in a previous iteration.
> >
> > Can you expand a bit more on your suggestion? Perhaps there's a clean way to do
> > it (without macros and etc), but so far I don't see it.
>
> I think with specialization it'd be trivial to generalize, but this isn't
> stable yet. The enum approach is probably unnecessarily complicated, so I agree
> to leave it as it is.
>
> Maybe a comment that this can be generalized once we get specialization would be
> good?

Specialization is really far out. I don't think we should try to take
it into account when designing things today. I think that the
duplication in this case is perfectly acceptable and trying to
deduplicate makes things too hard to read.

> > >> +impl<T: ThreadedHandler + 'static> ThreadedRegistration<T> {
> > >> +    /// Registers the IRQ handler with the system for the given IRQ number.
> > >> +    pub(crate) fn register<'a>(
> > >> +        dev: &'a Device<Bound>,
> > >> +        irq: u32,
> > >> +        flags: Flags,
> > >> +        name: &'static CStr,
> > >> +        handler: T,
> > >> +    ) -> impl PinInit<Self, Error> + 'a {
> > >
> > > What happens if `dev`  does not match `irq`? The caller is responsible to only
> > > provide an IRQ number that was obtained from this device.
> > >
> > > This should be a safety requirement and a type invariant.
> >
> > This iteration converted register() from pub to pub(crate). The idea was to
> > force drivers to use the accessors. I assumed this was enough to make the API
> > safe, as the few users in the kernel crate (i.e.: so far platform and pci)
> > could be manually checked for correctness.
> >
> > To summarize my point, there is still the possibility of misusing this from the
> > kernel crate itself, but that is no longer possible from a driver's
> > perspective.
>
> Correct, you made Registration::new() crate private, such that drivers can't
> access it anymore. But that doesn't make the function safe by itself. It's still
> unsafe to be used from platform::Device and pci::Device.
>
> While that's fine, we can't ignore it and still have to add the corresponding
> safety requirements to Registration::new().
>
> I think there is a way to make this interface safe as well -- this is also
> something that Benno would be great to have a look at.
>
> I'm thinking of something like
>
>         /// # Invariant
>         ///
>         /// `ěrq` is the number of an interrupt source of `dev`.
>         struct IrqRequest<'a> {
>            dev: &'a Device<Bound>,
>            irq: u32,
>         }
>
> and from the caller you could create an instance like this:
>
>         // INVARIANT: [...]
>         let req = IrqRequest { dev, irq };
>
> I'm not sure whether this needs an unsafe constructor though.

The API you shared would definitely work. It pairs the irq number with
the device it matches. Yes, I would probably give it an unsafe
constructor, but I imagine that most methods that return an irq number
could be changed to just return this type so that drivers do not need
to use said unsafe.

Alice

Powered by blists - more mailing lists