On Tue, Nov 20, 2018 at 07:57:00PM +0100, Patrick Wildt wrote: > 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 <[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 > > > [snip] > Yes, you are right, it should be &&. [snip]
I've been running the updated patch and so far so good... If y'all don't mind, I want to keep pounding on it for the next day or two before giving a thumbs up. Thanks. +--+ Carlos
