Re: [PATCH v2 3/3] clocksource/drivers/nxp-stm: Increase min_delta to 20us for PREEMPT_RT

[Enric Balletbo i Serra]

Hi,

On Mon, Nov 2, 2026 at 10:25 PM <sashiko-bot@xxxxxxxxxx> wrote:
>
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] clocksource/drivers/nxp-stm: overshoot check precedes channel enable
> --
>
> commit 1283cc4078852e277605e923b00287b73e75d175
> Author: Enric Balletbo i Serra <eballetb@xxxxxxxxxx>
>
> clocksource/drivers/nxp-stm: Increase min_delta to 20us for PREEMPT_RT
>
> This commit increases the clockevent min_delta from 2us to 20us in
> nxp_stm_clockevent_starting_cpu() to prevent 32-second timer wraparound
> lockups under heavy bus contention on PREEMPT_RT kernels.
>
> > diff --git a/drivers/clocksource/timer-nxp-stm.c b/drivers/clocksource/timer-nxp-stm.c
> > index ed7662a4e2bc4..a1a0e23d33426 100644
> > --- a/drivers/clocksource/timer-nxp-stm.c
> > +++ b/drivers/clocksource/timer-nxp-stm.c
> > @@ -335,14 +335,18 @@ static int nxp_stm_clockevent_starting_cpu(unsigned int cpu)
> >       /*
> >        * The timings measurement show reading the counter register
> >        * and writing to the comparator register takes as a maximum
> > -      * value 1100 ns at 133MHz rate frequency. The timer must be
> > -      * set above this value and to be secure we set the minimum
> > -      * value equal to 2000ns, so 2us.
> > +      * value 1100 ns at 133MHz rate frequency. However, under heavy
> > +      * bus contention on PREEMPT_RT, 2us is too tight and can allow
> > +      * the counter to pass the match value before the channel-enable
> > +      * write takes effect, leading to 32s wraparounds.
> >        *
> > -      * minimum ticks = (rate / MICRO) * 2
> > +      * Increase to 20us to guarantee the comparator channel is fully
> > +      * enabled before the match value is reached.
> > +      *
> > +      * minimum ticks = (rate / MICRO) * 20
> >        */
> >       clockevents_config_and_register(&stm_timer->ced, stm_timer->rate,
> > -                                     (stm_timer->rate / MICRO) * 2, 0x7FFFFFFFU);
> > +                                     (stm_timer->rate / MICRO) * 20, 0x7FFFFFFFU);
>
> [Severity: Medium]
> As reported by Sashiko on v1 [1], does increasing min_delta to 20us in
> nxp_stm_clockevent_starting_cpu() actually guarantee that the comparator
> channel is enabled before the match value is reached?
>
> Looking at drivers/clocksource/timer-nxp-stm.c:
>
> static int nxp_stm_clockevent_set_next_event(unsigned long delta, struct clock_event_device *ced)
> {
>         struct stm_timer *stm_timer = ced_to_stm(ced);
>         u32 val, now;
>
>         nxp_stm_clockevent_disable(stm_timer);
>
>         stm_timer->delta = delta;
>
>         val = nxp_stm_clockevent_read_counter(stm_timer) + delta;
>
>         writel(val, STM_CMP0(stm_timer->base));
>         ...
>         now = nxp_stm_clockevent_read_counter(stm_timer);
>         if ((int)(val - now) <= 0)
>                 return -ETIME;
>
>         nxp_stm_clockevent_enable(stm_timer);
>
>         return 0;
> }
>
> nxp_stm_clockevent_disable() clears STM_CCR_CEN in STM_CCR0, and the
> hardware only generates a match event when STM_CNT equals STM_CMP0 while
> STM_CCR0[CEN] is enabled. Because nxp_stm_clockevent_enable() is called
> after reading now and checking (int)(val - now) <= 0, the latency of writing
> STM_CCR_CEN to STM_CCR0 is not covered by the -ETIME overshoot check.
>
> For any programmed delta (20us or larger), if bus contention, an NMI, or
> hypervisor/firmware delay between the two counter reads leaves val - now as
> a small positive number of ticks, the overshoot check passes while the
> channel is still disabled, and STM_CNT can pass val before the subsequent
> nxp_stm_clockevent_enable() write takes effect in hardware.
>
> Should nxp_stm_clockevent_set_next_event() enable the comparator channel
> before reading now for the overshoot check (and call
> nxp_stm_clockevent_disable() if (int)(val - now) <= 0 before returning
> -ETIME) so the counter read is ordered after the STM_CCR0 write on the bus?
>

I answered this in the first version of the PATCH 2/3, this describes
the same race you pointed in the PATCH 2/3 and I think the race is
possible but I believe it cannot occur in practice with the current
min_delta:

set_next_event is called with local interrupts disabled, so
nothing can preempt or extend that window. The gap between reading
now and the writel in nxp_stm_clockevent_enable() is a
comparison, a branch, and a single MMIO write. With min_delta = 2us, the
check is still close to the write.

Reordering to enable-before-check introduces a different problem: if
the comparator match fires between nxp_stm_clockevent_enable() and
reading now, the interrupt handler delivers the event via
ced->event_handler(ced) and then we also return -ETIME, causing, if
I'm not mistaken
the clockevents layer to program another event, causing a double delivery.

Thanks,
   Enric

> [1] https://lore.kernel.org/r/20261026-nxp-stm-fixes-v1-3-20a64a4ea454@xxxxxxxxxx
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20261102-nxp-stm-fixes-v2-0-8ca2706f30ef@xxxxxxxxxx?part=3
>