Re: [PATCH v2 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 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?

[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