On Tue, Nov 20, 2018 at 04:44:56PM +0100, Mark Kettenis wrote:
> > Date: Mon, 19 Nov 2018 20:35:44 +0100
> > From: Patrick Wildt <patr...@blueri.se>
> >
> > On Mon, Nov 19, 2018 at 08:16:31PM +0100, Patrick Wildt wrote:
> > > On Mon, Nov 19, 2018 at 11:27:54AM -0700, Theo de Raadt wrote:
> > > > > That means bnxt(4) and dwpcie(4) share the same interrupt line, but
> > > > > one
> > > > > has IPL_AUDIO and the other one IPL_NET. Since the highest IPL on a
> > > > > pin counts, this line is now IPL_AUDIO (which is >IPL_VM). A bnxt(4)
> > > > > interrupt is now allowed to interrupt IPL_VM.
> > > > >
> > > > > I don't think there's a regression, I think it behaves as implemented.
> > > > > The question that remains is: How should it behave if not like that?
>
> Admittedly the current INTx implementation in dwpcie(4) is a bit of a
> hack. The hardware has a register that signals which of the four
> legacy PCI interrupts pin was triggered so the driver really should
> expose itself as a secondary interrupt controller. But proper support
> for secondary interrupt controllers requires a bit of thought. For
> one thing we'd need a way to unblock interrupts on decondary interrupt
> controllers in response to an splx(9) call.
>
> > > > When shared, it should operate at the lowest level not the highest.
> > > > That
> > > > may cause other difficulties.
> > >
> > > Actually it seems like we do set the lowest level, not the highest,
> > > sorry. But I think there might be a bug. I will have a look.
> > >
> >
> > I hope I'm not wrong, but I think there's a bug in the calculation
> > code. The ampinctc_calc_mask() function sets the priority of an IRQ
> > every time a handler is established or disestablished on the given IRQ.
> > In this case, the IPL_AUDIO is probably established before the IPL_NET,
> > thus when the IPL_NET establish happens, iq_irq is already set to "max"
> > as in IPL_AUDIO. That means the code that sets the priority to "min" is
> > skipped, as the continue statement will take effect.
> >
> > I guess each intrq object should have the lowest and highest IPL, and
> > the loop should only continue of both are the same. The lowest IPL is
> > then set for the IPL priority, and the highest IPL is used to splraise()
> > once the IRQ actually hits and the handler is suppoed to run.
> >
> > Untested, I hope that someone has a look as well and to spot if I have
> > a mistake in my understanding.
> >
> > We have this issue in at least 4 files: agintc/ampintc for arm64/armv7.
> >
> > Patrick
> >
> > diff --git a/sys/arch/arm64/dev/ampintc.c b/sys/arch/arm64/dev/ampintc.c
> > index 2c4ce63a150..a1b28a20ea7 100644
> > --- a/sys/arch/arm64/dev/ampintc.c
> > +++ b/sys/arch/arm64/dev/ampintc.c
> > @@ -160,8 +160,8 @@ struct intrhand {
> >
> > struct intrq {
> > TAILQ_HEAD(, intrhand) iq_list; /* handler list */
> > - int iq_irq; /* IRQ to mask while handling */
> > - int iq_levels; /* IPL_*'s this IRQ has */
> > + int iq_irq_max; /* IRQ to mask while handling */
> > + int iq_irq_min; /* lowest IRQ when shared */
> > int iq_ist; /* share type */
> > };
> >
> > @@ -469,14 +469,16 @@ ampintc_calc_mask(void)
> > min = ih->ih_ipl;
> > }
> >
> > - if (sc->sc_handler[irq].iq_irq == max) {
> > - continue;
> > - }
> > - sc->sc_handler[irq].iq_irq = max;
> > -
> > if (max == IPL_NONE)
> > min = IPL_NONE;
> >
> > + if (sc->sc_handler[irq].iq_irq_max == max ||
> > + sc->sc_handler[irq].iq_irq_min == min)
> > + continue;
>
> Shouldn't that be && instead of ||?
Yes, you are right, it should be &&.
> > +
> > + sc->sc_handler[irq].iq_irq_max = max;
> > + sc->sc_handler[irq].iq_irq_min = min;
> > +
> > /* Enable interrupts at lower levels, clear -> enable */
> > /* Set interrupt priority/enable */
> > if (min != IPL_NONE) {
> > @@ -604,7 +606,7 @@ ampintc_route_irq(void *v, int enable, struct cpu_info
> > *ci)
> > bus_space_write_4(sc->sc_iot, sc->sc_d_ioh, ICD_ICRn(ih->ih_irq), 0);
> > if (enable) {
> > ampintc_set_priority(ih->ih_irq,
> > - sc->sc_handler[ih->ih_irq].iq_irq);
> > + sc->sc_handler[ih->ih_irq].iq_irq_min);
> > ampintc_intr_enable(ih->ih_irq);
> > }
> >
> > @@ -646,7 +648,7 @@ ampintc_irq_handler(void *frame)
> > if (irq >= sc->sc_nintr)
> > return;
> >
> > - pri = sc->sc_handler[irq].iq_irq;
> > + pri = sc->sc_handler[irq].iq_irq_max;
> > s = ampintc_splraise(pri);
> > TAILQ_FOREACH(ih, &sc->sc_handler[irq].iq_list, ih_list) {
> > #ifdef MULTIPROCESSOR
> >
> >
>
