Re: [PATCH v2 3/5] clocksource/timer-econet-en751221: Always map all membase blocks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Unconditional iteration up to a compile-time constant array size causes timer initialization to fail on hardware with fewer timer blocks, breaking boot.
- [High] The per-CPU timer interrupt is unmasked before the clockevent device is registered, creating a race window that can lead to a NULL pointer dereference.
--

commit 1a3827645212808ad5c364c42807919047db1da2
Author: Caleb James DeLisle <cjd@cjdns.fr>

clocksource/timer-econet-en751221: Always map all membase blocks

This commit simplifies the logic by unconditionally mapping all timer memory
blocks up to ARRAY_SIZE(econet_timer.membase), instead of computing the
number of blocks dynamically based on the number of possible CPUs.

> diff --git a/drivers/clocksource/timer-econet-en751221.c b/drivers/clocksource/timer-econet-en751221.c
> index 5def3e536b21c..e79069d9a826d 100644
> --- a/drivers/clocksource/timer-econet-en751221.c
> +++ b/drivers/clocksource/timer-econet-en751221.c
> @@ -173,7 +173,6 @@ static int __init cevt_init(struct device_node *np)
>  
>  static int __init timer_init(struct device_node *np)
>  {
> -	int num_blocks = DIV_ROUND_UP(num_possible_cpus(), 2);
>  	struct clk *clk;
>  	int ret;

[ ... ]

> @@ -185,7 +184,7 @@ static int __init timer_init(struct device_node *np)
>  
>  	econet_timer.freq_hz = clk_get_rate(clk);
>  
> -	for (int i = 0; i < num_blocks; i++) {
> +	for (int i = 0; i < ARRAY_SIZE(econet_timer.membase); i++) {
>  		econet_timer.membase[i] = of_iomap(np, i);
>  		if (!econet_timer.membase[i]) {
>  			pr_err("%pOFn: failed to map register [%d]\n", np, i);

Could this cause a regression on systems with fewer timer blocks, like the
EN751221 (34Kc) which only has 1 block?

Since ARRAY_SIZE(econet_timer.membase) is based on CONFIG_NR_CPUS, it
expands to a compile-time constant that can exceed the actual number of
hardware blocks.

If a kernel is built with CONFIG_NR_CPUS=4 or higher, this loop will attempt
to map 2 or more blocks. On a system with only 1 timer block and 1 reg entry
in its device tree, of_iomap(np, 1) will fail and return NULL.

Because the error path unconditionally returns -ENXIO and aborts timer
initialization, could this completely break booting on these platforms?

This isn't a regression introduced by this commit, but looking at
cevt_init_cpu():

drivers/clocksource/timer-econet-en751221.c:cevt_init_cpu() {
	...
	enable_percpu_irq(cd->irq, IRQ_TYPE_NONE);

	/* Do this last because it synchronously configures the timer */
	clockevents_config_and_register(cd, econet_timer.freq_hz,
					ECONET_MIN_DELTA, ECONET_MAX_DELTA);
	...
}

Is it possible for a timer interrupt to fire immediately after
enable_percpu_irq() is called?

If an interrupt fires before clockevents_config_and_register() completes,
could the interrupt handler dereference a NULL dev->event_handler(dev)?

Calling clockevents_config_and_register() before unmasking the interrupt
might close this potential race window.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260514000601.3430262-1-cjd@cjdns.fr?part=3