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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] clocksource/drivers/nxp-stm: overshoot check runs before enabling comparator
--

commit 1dea5edcdc7544abe47a47e00f3d79d5b290fb1a
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 avoid counter overshoot under bus
contention on PREEMPT_RT kernels.

>     clocksource/drivers/nxp-stm: Increase min_delta to 20us for PREEMPT_RT
>     
>     Under bus contention on PREEMPT_RT kernels, the 2us min_delta is too
>     small and the counter can advance past the programmed comparator value
>     before the overshoot check runs. While the -ETIME fallback handles this
>     case, repeated retries add latency. Increase min_delta to 20us to reduce
>     collision probability.
>     
>     Cc: stable@xxxxxxxxxxxxxxx
>     Signed-off-by: Enric Balletbo i Serra <eballetb@xxxxxxxxxx>

> diff --git a/drivers/clocksource/timer-nxp-stm.c b/drivers/clocksource/timer-nxp-stm.c
> index ed7662a4e2bc4..a0761d7f8b013 100644
> --- a/drivers/clocksource/timer-nxp-stm.c
> +++ b/drivers/clocksource/timer-nxp-stm.c
> @@ -335,14 +335,14 @@ 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 bus
> +	 * contention on PREEMPT_RT, 2us is too small and causes missed
> +	 * matches resulting in 32s wraparounds. Increase to 20us.
>  	 *
> -	 * minimum ticks = (rate / MICRO) * 2
> +	 * 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]
The commit message states that the -ETIME fallback handles counter
overshoot and that increasing min_delta to 20us only reduces retry
latency, whereas the comment added in nxp_stm_clockevent_starting_cpu()
states that 2us causes missed matches resulting in 32s wraparounds.

Looking at nxp_stm_clockevent_set_next_event() in
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, so no
hardware comparator match can occur until nxp_stm_clockevent_enable()
writes STM_CCR_CEN back to STM_CCR0.

Because now is read and checked against val before calling
nxp_stm_clockevent_enable(), if now is sampled just before val, can the
counter reach and pass val while the STM_CCR0 enable write is still in
flight under bus contention, missing the hardware match while
nxp_stm_clockevent_set_next_event() returns 0 and causing the 32.2s
wraparound?

Should nxp_stm_clockevent_set_next_event() enable the comparator channel
before reading now for the overshoot check (and disable it again if
returning -ETIME) so that the -ETIME check covers the channel-enable
write latency as well?

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