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

[Danilo Krummrich]

On Mon, Jun 16, 2025 at 03:43:40PM +0200, Danilo Krummrich wrote:
> On Mon, Jun 16, 2025 at 03:33:06PM +0200, Alice Ryhl wrote:
> > On Mon, Jun 9, 2025 at 8:13 PM Danilo Krummrich <dakr@xxxxxxxxxx> wrote:
> > > 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.
> 
> As mentioned above, I agree with the latter. But I think leaving a note that
> this could be deduplicated rather easily with specialization probably doesn't
> hurt?
> 
> > > 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.
> 
> Driver don't need to use unsafe already. It's only the IRQ accessors in this
> patch series (in platform.rs and pci.rs) that are affected.
> 
> Let's also keep those accessors, from a driver perspective it's much nicer to
> have an API like this, i.e.

Just to clarify, I meant to additionally keep the accessors, since

> 
> 	// `irq` is an `irq::Registration`
> 	let irq = pdev.threaded_irq_by_name()?

this should be the most common case.

> 
> vs.
> 
> 	// `req` is an `IrqRequest`.
> 	let req = pdev.irq_by_name()?;
> 
> 	// `irq` is an `irq::Registration`
> 	let irq = irq::ThreadedRegistration::new(req)?;

But this can be useful as well, e.g. if a driver can handle devices from
multiple busses, the driver could obtain the IrqRequest and pass it down to bus
independent layers of the driver which then create the final irq::Registration.