Re: [PATCH v8 5/5] clocksource/timer-econet-en751221: Support EN751627 without percpu IRQ

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] timer-econet: interrupt storm on CPU hotplug due to missing teardown and this_cpu_ptr()
--

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

clocksource/timer-econet-en751221: Support EN751627 without percpu IRQ

EN751627 is based on the 1004Kc which uses a different interrupt number
for each CPU timer. Support both this and the EN751221 which uses a
single percpu interrupt.

> 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
[ ... ]
> @@ -110,7 +131,10 @@ static int cevt_init_cpu(uint cpu)
>  	clockevents_config_and_register(cd, econet_timer.freq_hz,
>  					ECONET_MIN_DELTA, ECONET_MAX_DELTA);
>  
> -	enable_percpu_irq(cd->irq, IRQ_TYPE_NONE);
> +	if (econet_timer.is_percpu)
> +		enable_percpu_irq(cd->irq, IRQ_TYPE_NONE);
> +	else
> +		enable_irq(cd->irq);

[Severity: High]
Will this trigger an unbalanced enable warning when a CPU is brought online?

Since the timer is not stopped and the IRQ is never disabled when a CPU is
offlined, calling enable_irq() here operates on an already-enabled IRQ.

[ ... ]
> @@ -173,22 +230,32 @@ static int __init timer_init(struct device_node *np)
[ ... ]
> -	ret = request_percpu_irq(econet_timer.irq, cevt_interrupt, np->name,
> -				 &econet_timer_pcpu);
> -
> -	if (ret < 0) {
> -		pr_err("%pOFn: IRQ %d setup failed (%d)\n", np,
> -		       econet_timer.irq, ret);
> -		goto out_irq_mapping;
> +	for (i = 0; i < num_irqs; i++) {
> +		irq_set_status_flags(econet_timer.irqs[i], IRQ_NOAUTOEN);
> +
> +		if (econet_timer.is_percpu)
> +			ret = request_percpu_irq(econet_timer.irqs[i],
> +						 cevt_interrupt, np->name,
> +						 &econet_timer_pcpu);
> +		else
> +			ret = request_irq(econet_timer.irqs[i], cevt_interrupt,
> +					  IRQF_TIMER | IRQF_NOBALANCING,
> +					  np->name, NULL);

[Severity: High]
Is it safe to pass NULL for dev_id here when setting up migratable standard
IRQs?

When a non-per-cpu IRQ is migrated to another CPU, the handler will incorrectly
fetch the timer context of the executing CPU rather than the one that fired:

drivers/clocksource/timer-econet-en751221.c:cevt_interrupt() {
    struct clock_event_device *dev = this_cpu_ptr(&econet_timer_pcpu);
    ...
}

Furthermore, is a teardown callback missing when registering the CPU hotplug
state?

drivers/clocksource/timer-econet-en751221.c:timer_init() {
    ...
    ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
                            "clockevents/econet/timer:starting",
                            cevt_init_cpu, NULL);
    ...
}

Because there is no hotplug teardown callback or set_state_shutdown, the timer
is never stopped when a CPU is offlined.

The standard timer IRQ migrates to an online CPU, where the interrupt handler
checks the wrong CPU's timer pending bit and constantly returns IRQ_NONE. Since
IRQF_TIMER bypasses the spurious IRQ detector, this sequence appears to create
an unbreakable interrupt storm that hangs the kernel.

> +
> +		if (ret < 0) {
> +			pr_err("%pOFn: IRQ %d setup failed: %pe\n", np,
> +			       i, ERR_PTR(ret));
> +			goto out_irq_free;
>  		}
>  	}
>  
>  	cevt_init(np);

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