Re: [PATCH v2 1/1] ArmPkg/TimerDxe: Add ISB for timer compare value reload

[Guo Heyi <heyi.guo@linaro.org>]

On Tue, Mar 13, 2018 at 09:33:33AM +0000, Marc Zyngier wrote:
> On 13/03/18 00:31, Heyi Guo wrote:
> > If timer interrupt is level sensitive, reloading timer compare
> > register has a side effect of clearing GIC pending status, so a "ISB"
> > is needed to make sure this instruction is executed before enabling
> > CPU IRQ, or else we may get spurious timer interrupts.
> > 
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
> > Signed-off-by: Yi Li <phoenix.liyi@huawei.com>
> > Cc: Leif Lindholm <leif.lindholm@linaro.org>
> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Cc: Marc Zyngier <marc.zyngier@arm.com>
> > ---
> > 
> > Notes:
> >     v2:
> >     - Use ISB instead of DSB [Marc]
> >     - Update commit message accordingly.
> > 
> >  ArmPkg/Drivers/TimerDxe/TimerDxe.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/ArmPkg/Drivers/TimerDxe/TimerDxe.c b/ArmPkg/Drivers/TimerDxe/TimerDxe.c
> > index 33d7c922221f..32abee8726a0 100644
> > --- a/ArmPkg/Drivers/TimerDxe/TimerDxe.c
> > +++ b/ArmPkg/Drivers/TimerDxe/TimerDxe.c
> > @@ -337,6 +337,7 @@ TimerInterruptHandler (
> >  
> >      // Set next compare value
> >      ArmGenericTimerSetCompareVal (CompareValue);
> > +    ArmInstructionSynchronizationBarrier ();
> >      ArmGenericTimerEnableTimer ();
> >    }
> 
> Sorry for being pedantic here, but it would make more sense if ISB was
> placed after the enabling of the timer. Otherwise, you only force the
> update of the comparator. But on the other hand, the timer was never
> disabled the first place, in which case you'd wonder why you're trying
> to enable it again.
Yes, I also had such question and hesitated at this place :)
> 
> So either you leave the ISB here and remove the enable call, or move the
> ISB after the enable.

If we are going to remove the enable call, is it better to be changed in a
separate patch? It seems not related with adding ISB, though it is only a
one-line change.

Thanks and regards,
Heyi

> 
> Thanks,
> 
> 	M.
> -- 
> Jazz is not dead. It just smells funny...