Re: [PATCH v4 4/6] rust: irq: add support for threaded IRQs and handlers

[Danilo Krummrich]

On Mon, Jun 09, 2025 at 01:24:40PM -0300, Daniel Almeida wrote:
> > On 9 Jun 2025, at 09:27, Danilo Krummrich <dakr@xxxxxxxxxx> 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?

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