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

[sashiko-bot]

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?

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