Re: [PATCH-1] A new irq device interface

[Samuel Thibault]

Junling Ma, le dim. 02 août 2020 16:19:39 -0700, a ecrit:
> I was distracted by using a counter at all in the original code,
> because an irq cannot be fired more than once before it is acked.

Right, normally that doesn't happen.

> What we really need is a boolean_t type per notification,

And that could be an option.

> > > IN fact, I was pondering on the idea that each irq* device should be
> > > opened exclusively,
> >
> > Then things won't work. Shared IRQ is a thing you can't circumvent with
> > the hardware that people have atm.
> 
> You may have missed the part where I say let user side [/dev/irq*] translator
> do the multiplexing.

I didn't but that looks like extra complexity, and makes the whole thing
less robust: if the multiplexer gets to die somehow, everything dies.

> And when MSI/MSI-X is introduced, irq sharing will not be needed.

That would still ignore all the hardware that people still have.

> > > The original way of counting means that the interrupt will be
> > > unfortunately blocked forever if a message somehow failed to deliver.
> >
> > There is no such thing. Either the message gets delivered, or it does
> > not. It can take some time to get delivered. Then other translators
> > using the same IRQ will suffer from the situation. But that's what we
> > *have* to do. Until the userland translator either acknowledges the irq
> > or dies, we *have* to keep the irq disabled (and know that we have done
> > so for the death case).  We can't go around it anyway, so there is no
> > point in trying to fix it.
> 
> The delivery could have returned an error, for example, if ikm_alloc return
> NULL, as checked in deliver_intr.

In such a case, I don't think we can do anything better than closing
the user port, so the user can get notified of the missed IRQ, and
possibly try to restart the driver (which is fine for network cards for
instance). So we're back to the "port death" case.

> > > This patch increases n_unacked only if a notification is delivered,
> >
> > That would mean that the responsibility of ack-ing interrupts belongs to
> > userland?
> 
> We already require that user land ack the interrupt by calling 
> device_intr_ack,
> right?

That it acks, yes. That it tracks how many times __enable_irq() should
be called on port death, no.

Again, just checking for port death is a simple way and gets everything
working as expected. What I can grasp from what you proposed, it is much
less clear.

> As I understand, if a handler has not acked, then the n_unacked remain
> positive, and also __enable_irq will not be called, and thus the irq
> remains disabled, for all other handlers attached to the same irq.

Yes. Until the user notices the translator is stuck, and thus kills it,
and thus the kernel notices the port death, and can unlock the IRQ.

> That is what I felt from the IRC discussion yesterday, too. And that is fine 
> if
> there is no interest in this patch.

The thing is: I do not see what improvement there is here. It took us
some time to get to this working state of ack semantic. If you want to
improve it, then fine, but you have not explained where there is actual
improvement (and just rewriting the whole thing for the sake of it will
most probably only bring bugs).

> But still, new comes may ask the same question as I did, why would
> any device driver need to implement device_intr_register and
> device_intr_ack,

That question is complementely independent of everything we have been
discussing above. Again a reason why to separate changes into separate
patches, so we can discuss separately and avoid squashing together
things that don't actually need to be, thus blocking improvements on
some side just because some other side poses question.

My only comment on patch-2 was that it was introducing other unrelated
changes, making it unreviewable. As discussed on IRC the other day, your
device IPC change makes sense. But I don't see any relation with the IRQ
*management* that you have introduced. Make that patch independent from
the whole rest, and we'll be able to discuss it.


Really, I'm wondering how it cannot be obvious for newcomers that yes,
you need to separate out changes so we can discuss things separately,
otherwise it's just a big mess and we can't work things out.

Samuel