Re: [PATCH v3 3/4] clocksource/timer-econet-en751221: Disable IRQ until cevt registered

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Incomplete Race Condition Fix / NULL Pointer Dereference
- [High] Missing Teardown in CPU Hotplug / Error Path Rollback Failure
- [High] Missing Hardware Clock Enable and Reference Leak
- [Low] Redundant API Usage for per-CPU Interrupt
--

commit 5ebbb6312519c50062084d8964d9f2a0fe342d0f
Author: Caleb James DeLisle <cjd@cjdns.fr>

clocksource/timer-econet-en751221: Disable IRQ until cevt registered

This commit attempts to eliminate a race condition where the interrupt
could trigger before the clockevent device is fully registered and its
event handler is set. It achieves this by setting the no-auto-enable
flag and delaying the interrupt enablement until after registration.

> diff --git a/drivers/clocksource/timer-econet-en751221.c b/drivers/clocksource/timer-econet-en751221.c
> index 155471f68e6f8..ed750e39cc4fa 100644
> --- a/drivers/clocksource/timer-econet-en751221.c
> +++ b/drivers/clocksource/timer-econet-en751221.c
> @@ -104,12 +104,11 @@ static int cevt_init_cpu(uint cpu)
>  	reg = ioread32(reg_ctl(cpu)) | ctl_bit_enabled(cpu);
>  	iowrite32(reg, reg_ctl(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);
>  
> +	enable_percpu_irq(cd->irq, IRQ_TYPE_NONE);

Does this completely resolve the race condition?

If the clockevents core rejects the new device during registration,
dev->event_handler remains null. Because the hardware timer is unconditionally
started above and there is no set_state_shutdown callback, the timer cannot
be stopped on rejection.

When the timer eventually wraps and fires, will cevt_interrupt() execute
a null event_handler and crash?

>  	return 0;
>  }
>  
> @@ -177,6 +176,8 @@ static int __init timer_init(struct device_node *np)
>  		goto out_membase;
>  	}
>  
> +	irq_set_status_flags(econet_timer.irq, IRQ_NOAUTOEN);

This isn't a bug, but is this explicit flag setting redundant?

request_percpu_irq() requires the target interrupt to be configured as a
per-cpu devid interrupt. The core function irq_set_percpu_devid_flags()
already applies IRQ_NOAUTOEN alongside IRQ_PER_CPU_DEVID, meaning the flag
is guaranteed to be set before request_percpu_irq() is executed.

Are we missing a call to clk_prepare_enable() for the clock fetched earlier
in timer_init()?

If the clock is never formally enabled with a positive reference count, the
common clock framework late initcall clk_disable_unused() might turn off
the clock. Would shutting off the clock underneath an active system timer
instantly stop the hardware counter and lock up timekeeping?

Also, does this code leak the clock reference by never calling clk_put()
on the returned clock pointer on both success and error paths?

Is a teardown callback missing for cpu hotplug?

timer_init() calls cpuhp_setup_state() with null as the teardown callback:

    ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
                            "clockevents/econet/timer:starting",
                            cevt_init_cpu, NULL);

When a cpu is offlined, disable_percpu_irq() is never called, leaving the
per-cpu interrupt unmasked on the dying cpu. Without a set_state_shutdown
callback, does the hardware timer remain running, potentially causing
spurious wakeups or preventing low-power states?

Furthermore, if cpuhp_setup_state() fails on a cpu, the core cannot roll back
the successfully started processors because of the missing teardown callback.
The error path then frees the interrupt action while hardware timers are
still running. Will this lead to an unhandled interrupt storm?

>  	ret = request_percpu_irq(econet_timer.irq, cevt_interrupt, np->name,
>  				 &econet_timer_pcpu);
>

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