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?
> >
> > 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;
+
+ 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
