Re: [PATCH] arm64: arch_timer: Ensure timer is enabled before using istatus

[Marc Zyngier <maz@kernel.org>]

On Sat, 25 Jul 2020 09:49:55 +0100,
Shaokun Zhang <zhangshaokun@hisilicon.com> wrote:
> 
> Hi Marc,
> 
> 在 2020/7/24 18:22, Marc Zyngier 写道:
> > On 2020-07-24 10:47, Shaokun Zhang wrote:
> >> From: Nianyao Tang <tangnianyao@huawei.com>
> >>
> >> In Arm ARM spec, there is a description for timer control register, when
> >> the value of the ENABLE bit is 0, the ISTATUS field is UNKNOWN. We shall
> >> only read and use ISTATUS when ENABLE is 1, otherwise ISTATUS may be
> >> invalid.
> >>
> >> Cc: Mark Rutland <mark.rutland@arm.com>
> >> Cc: Marc Zyngier <maz@kernel.org>
> >> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> >> Signed-off-by: Nianyao Tang <tangnianyao@huawei.com>
> >> Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com>
> >> ---
> >>  drivers/clocksource/arm_arch_timer.c | 3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/clocksource/arm_arch_timer.c
> >> b/drivers/clocksource/arm_arch_timer.c
> >> index 6c3e84180146..0bbc2715de79 100644
> >> --- a/drivers/clocksource/arm_arch_timer.c
> >> +++ b/drivers/clocksource/arm_arch_timer.c
> >> @@ -641,7 +641,8 @@ static __always_inline irqreturn_t
> >> timer_handler(const int access,
> >>      unsigned long ctrl;
> >>
> >>      ctrl = arch_timer_reg_read(access, ARCH_TIMER_REG_CTRL, evt);
> >> -    if (ctrl & ARCH_TIMER_CTRL_IT_STAT) {
> >> +    if ((ctrl & ARCH_TIMER_CTRL_ENABLE) &&
> >> +        (ctrl & ARCH_TIMER_CTRL_IT_STAT)) {
> >>          ctrl |= ARCH_TIMER_CTRL_IT_MASK;
> >>          arch_timer_reg_write(access, ARCH_TIMER_REG_CTRL, ctrl, evt);
> >>          evt->event_handler(evt);
> > 
> > Interesting. A question for you though:
> > 
> > How do you think we made it in the interrupt handler if the timer
> > was disabled?
> 
> Let's assume this scenario as follow:
> a. Mask timer interrupt by PSTATE.I
> b. Timer interrupt is set and pending in GICC
> c. Disable timer by CNT{P,V}_CTL_EL0.ENABLE and the clear operation will consume
> much more time when GIC is very busy.
> d. Unmask timer interrupt by PSTATE.I, but timer interrupt is not clear in time
> and forward to cpu.
> e. We receive a timer interrupt with ENABLE=0

And that's a spurious interrupt. Big deal. Should we care? No, because
this can happen for any device, in any situation. If the GIC cannot
retire a level PPI quickly enough, that's a GIC quality of
implementation issue, and I don't plan to paper over it in all
existing drivers.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel