On Mon, Jun 9, 2025 at 8:13 PM Danilo Krummrich <dakr@xxxxxxxxxx> wrote:
>
> 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?
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
