Re: [PATCH] ArmPkg: Fix double GIC EIOR write per interrupt

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 8 August 2016 at 13:08, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 8 August 2016 at 13:06, Alexei Fedorov <Alexei.Fedorov@arm.com> wrote:
>> Timer's pending interrupt is cleared  HARDWARE_INTERRUPT_HANDLER TimerInterruptHandler()  in when timer is re-programmed for the next shot.
>
> Yes, that is the timer side.
>
>> Does it mean that TimerDxe driver is part EFI_HARDWARE_INTERRUPT_PROTOCOL?
>>
>
> No. The peripheral and the GIC each have their own mask and enable
> registers for the interrupt. That is exactly why the comment describes
> in detail which part is the responsibility of the GIC, and which is
> the responsibility of the peripheral.
>

... and actually, looking at TimerInterruptHandler (), I don't see the
point of re-enabling the interrupt early, given that

308: OriginalTPL = gBS->RaiseTPL (TPL_HIGH_LEVEL);

disables interrupts globally, and only re-enables them on line 346, at
which point the mTimerNotifyFunction() has already returned.

So I propose we simply do

diff --git a/ArmPkg/Drivers/TimerDxe/TimerDxe.c
b/ArmPkg/Drivers/TimerDxe/TimerDxe.c
index 1169d426b255..f0fcb05757ac 100644
--- a/ArmPkg/Drivers/TimerDxe/TimerDxe.c
+++ b/ArmPkg/Drivers/TimerDxe/TimerDxe.c
@@ -308,10 +308,7 @@ TimerInterruptHandler (
   OriginalTPL = gBS->RaiseTPL (TPL_HIGH_LEVEL);

   // Check if the timer interrupt is active
-  if ((ArmGenericTimerGetTimerCtrlReg () ) & ARM_ARCH_TIMER_ISTATUS) {
-
-    // Signal end of interrupt early to help avoid losing subsequent
ticks from long duration handlers
-    gInterrupt->EndOfInterrupt (gInterrupt, Source);
+  while ((ArmGenericTimerGetTimerCtrlReg () ) & ARM_ARCH_TIMER_ISTATUS) {

     if (mTimerNotifyFunction) {
       mTimerNotifyFunction (mTimerPeriod * mElapsedPeriod);

so that if the condition exists that we know will trigger the
interrupt immediately as soon as we unmask it, we simply enter the
loop again just like we would when taking the [nested] interrupt.

@Heyi: any thoughts?

-- 
Ard.