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.
