On 8 August 2016 at 15:42, Ard Biesheuvel <ard.biesheu...@linaro.org> wrote: > On 8 August 2016 at 15:22, Cohen, Eugene <eug...@hp.com> wrote: >> Guys, sorry to join so late, something about timezones... Let me try to >> provide some context and history. >> >>> > it does change the contract we have with registered interrupt handlers >>> >>> Looks like it does not: >>> From edk2\EmbeddedPkg\Include\Protocol\HardwareInterrupt.h: >>> >>> " Abstraction for hardware based interrupt routine >>> >>> ...The driver implementing >>> this protocol is responsible for clearing the pending interrupt in the >>> interrupt routing hardware. The HARDWARE_INTERRUPT_HANDLER is >>> responsible >>> for clearing interrupt sources from individual devices." >> >> I think you are reading more deeply into this verbiage than was intended. >> From a separation-of-concerns perspective one driver is concerned with >> writing to the hardware that generates the interrupt (handler) and another >> is concerned with writing to the hardware for the interrupt controller to >> signal the end of interrupt. So all this is saying is that "the code that >> touches the interrupt controller is implemented in the driver that publishes >> this protocol". It does not say how this code is activated, only who is >> responsible for poking the register. >> >> The historical expectation is that the handler driver calls the EOI >> interface in the protocol. (If it was the opposite then this interface >> wouldn't even exist since the interrupt controller driver could just do it >> implicitly.) You're next question will probably be why it was designed this >> way - for that we'll have to ask Andrew Fish (added). >> >> I did a little digging and see that the PC-AT chipset implemented an 8259 >> interrupt protocol (IntelFrameworkPkg\Include\Protocol\Legacy8259.h) that is >> quite similar to HwInterrupt. Notice the explicit EndOfInterrupt interface >> here and how it's used by the timer driver at >> PcAtChipsetPkg\8254TimerDxe\Timer.c(88). >> >> Given this I asked that you keep the EndOfInterrupt in the handler driver(s) >> and remove the auto-EOI in the interrupt controller driver, at least for >> cases where a driver handled the interrupt. >> >> Feel free to clarify the text in the protocol header to align with this - >> the current wording is not very clear. >> > > Thanks for the context. I did some archaeology of my own, and it turns > out that this code was introduced by Andrew in git commit > 1bfda055dfbc52678 (svn #11293) more than 5 years ago. > > In any case, it appears we are in agreement that it is in fact > incorrect (and deviates from the other implementations) to signal EOI > in the GIC driver, and so I suppose Alexei's patch is good (and we > only need to clarify the comment that he quoted in this thread). > > My primary concern was that we change the contract with existing > handlers, but if there was a contract to begin with, we were already > violating it, and so any out of tree breakage is not really our > problem :-) >
Pushed as 7989300df7 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
