Re: [PATCH 2/3] clocksource/drivers/nxp-stm: Fix missed comparator match in set_next_event

[Enric Balletbo i Serra]

Hi,

On Mon, Oct 26, 2026 at 10:50 AM <sashiko-bot@xxxxxxxxxx> wrote:
>
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] clocksource: nxp-stm: overshoot race in nxp_stm_clockevent_set_next_event()
> --
>
> commit 45f273b782a28cec6ef30880d738a9cb690edce9
> Author: Enric Balletbo i Serra <eballetb@xxxxxxxxxx>
>
> clocksource/drivers/nxp-stm: Fix missed comparator match in set_next_event
>
> This patch fixes the overshoot check in nxp_stm_clockevent_set_next_event()
> by using a signed comparison between the target comparator value and the
> current counter value to avoid missed matches and 32-bit wraparound stalls.
>
> > diff --git a/drivers/clocksource/timer-nxp-stm.c b/drivers/clocksource/timer-nxp-stm.c
> > index ee249fed1b918..ed7662a4e2bc4 100644
> > --- a/drivers/clocksource/timer-nxp-stm.c
> > +++ b/drivers/clocksource/timer-nxp-stm.c
> > @@ -247,7 +247,7 @@ static int nxp_stm_clockevent_shutdown(struct clock_event_device *ced)
> >  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;
> > +     u32 val, now;
> >
> >       nxp_stm_clockevent_disable(stm_timer);
> >
> > @@ -258,16 +258,12 @@ static int nxp_stm_clockevent_set_next_event(unsigned long delta, struct clock_e
> >       writel(val, STM_CMP0(stm_timer->base));
> >
> >       /*
> > -      * The counter is shared across the channels and can not be
> > -      * stopped while we are setting the next event. If the delta
> > -      * is very small it is possible the counter increases above
> > -      * the computed 'val'. The min_delta value specified when
> > -      * registering the clockevent will prevent that. The second
> > -      * case is if the counter wraps while we compute the 'val' and
> > -      * before writing the comparator register. We read the counter,
> > -      * check if we are back in time and abort the timer with -ETIME.
> > +      * If the counter reached or passed 'val' during or before the
> > +      * write to the comparator, the hardware match will be missed,
> > +      * resulting in a full 32-bit (32.2s) wraparound lockup.
> >        */
> > -     if (val > nxp_stm_clockevent_read_counter(stm_timer) + delta)
> > +     now = nxp_stm_clockevent_read_counter(stm_timer);
> > +     if ((int)(val - now) <= 0)
> >               return -ETIME;
> >
> >       nxp_stm_clockevent_enable(stm_timer);
>
> [Severity: High]
> Can the counter still advance past val before nxp_stm_clockevent_enable()
> arms the channel comparator in drivers/clocksource/timer-nxp-stm.c?
>
> At the beginning of nxp_stm_clockevent_set_next_event(), channel 0 is
> disabled before programming STM_CMP0:
>
>         nxp_stm_clockevent_disable(stm_timer);
>         ...
>         val = nxp_stm_clockevent_read_counter(stm_timer) + delta;
>         writel(val, STM_CMP0(stm_timer->base));
>
> Unlike timer-imx-gpt where the compare interrupt remains enabled while
> writing TCMP, writing STM_CMP0 in the NXP STM does not arm the comparator
> until nxp_stm_clockevent_enable() writes STM_CCR_CEN to STM_CCR0.
>
> Because now is read from STM_CNT before nxp_stm_clockevent_enable() is
> called, if now is sampled just below val (so (int)(val - now) <= 0 is false)
> and STM_CNT reaches or passes val before the subsequent write to STM_CCR0
> takes effect in hardware, the match occurs while channel 0 is still disabled
> and is missed.
>
> Should nxp_stm_clockevent_enable(stm_timer) be called before reading now for
> the overshoot check, with nxp_stm_clockevent_disable(stm_timer) called on
> the -ETIME path?
>

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
the clockevents layer to program another event — a double delivery.

This check-then-enable ordering is also pre-existing in the original
driver; this series only fixes the comparison logic, not the
sequencing.

> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20261026-nxp-stm-fixes-v1-0-20a64a4ea454@xxxxxxxxxx?part=2
>