Re: [PATCH v2 2/5] clocksource/timer-econet-en751221: Move generic logic out of cevt_init

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The return value of `cpuhp_setup_state()` is ignored, masking critical initialization failures.
- [High] The hardware clock is retrieved but never enabled before accessing memory-mapped registers.
- [High] Missing teardown callback for CPU hotplug leaves per-CPU IRQs enabled and the timer running when CPUs go offline.
--

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

clocksource/timer-econet-en751221: Move generic logic out of cevt_init

This commit simplifies cevt_init by moving common generic logic out
into a new cevt_setup_clockevent function. This change prepares the driver
to support either a percpu IRQ or multiple IRQ numbers.

> diff --git a/drivers/clocksource/timer-econet-en751221.c b/drivers/clocksource/timer-econet-en751221.c
> --- a/drivers/clocksource/timer-econet-en751221.c
> +++ b/drivers/clocksource/timer-econet-en751221.c

[ ... ]

> @@ -203,6 +206,10 @@ static int __init timer_init(struct device_node *np)
>  	if (ret < 0)
>  		return ret;
>  

This isn't a regression introduced by this patch, but does the hardware clock
retrieved via of_clk_get() earlier in timer_init() need to be enabled with
clk_prepare_enable() before memory-mapped registers are accessed?

> +	cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
> +			  "clockevents/econet/timer:starting",
> +			  cevt_init_cpu, NULL);
> +

Since cpuhp_setup_state() can fail and return a negative error code, should
the return value be checked here?

Also, because there is no teardown callback provided, does this leave per-CPU
IRQs enabled and the timer running when a CPU goes offline?

>  	sched_clock_register(sched_clock_read, ECONET_BITS,
>  			     econet_timer.freq_hz);
>

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