> Date: Mon, 19 Nov 2018 20:35:44 +0100
> From: Patrick Wildt <[email protected]>
>
> 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 ||?
> +
> + 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
>
>
