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

[Marc Zyngier <marc.zyngier@arm.com>]

On Wed, 14 Mar 2018 00:25:09 +0000,
Guo Heyi wrote:
> 
> 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.

I guess a separate patch doesn't hurt, but that's for Ard and Leif to
decide.

Thanks,

	M.

-- 
Jazz is not dead, it just smell funny.