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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The interrupt handler incorrectly relies on `this_cpu_ptr` for standard IRQs, causing an interrupt storm if the IRQ executes on a different CPU.
- [Medium] Missing CPU hotplug teardown callback causes an unbalanced `enable_irq()` kernel warning when a CPU is offlined and then onlined again.
--

commit 0de42286a034bc944e765e97434f913831bc2960
Author: Caleb James DeLisle <cjd@cjdns.fr>
Subject: clocksource/timer-econet-en751221: Support EN751627 without percpu IRQ

This commit adds support for the EN751627 timer, which uses a different
interrupt number for each CPU timer instead of a single percpu interrupt.

> diff --git a/drivers/clocksource/timer-econet-en751221.c b/drivers/clocksource/timer-econet-en751221.c
> index f2c4c1ee0a56b..040386ded132a 100644
> --- 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: Medium]
Will this cause an unbalanced enable_irq() warning if a CPU is offlined
and onlined again?

The driver registers a dynamic CPU hotplug state via cpuhp_setup_state()
with cevt_init_cpu() as the starting callback, but passes NULL for the
teardown callback.

Because there is no teardown callback to call disable_irq() when a CPU is
offlined, the IRQ descriptor's enable depth remains 0. When the CPU is
brought back online, cevt_init_cpu() will execute again and call
enable_irq() a second time.

While this missing teardown existed before this patch, it was hidden
because enable_percpu_irq() does not track descriptor depth. The warning
appears to be newly exposed by the introduction of enable_irq().

[ ... ]

> @@ -173,22 +230,32 @@ static int __init timer_init(struct device_node *np)

[ ... ]

> +	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]
Could passing NULL for dev_id lead to an interrupt storm if the IRQ
executes on a different CPU?

For standard (non-percpu) IRQs, cevt_interrupt() currently calculates the
timer device using this_cpu_ptr(). If the interrupt fires on a different
CPU (e.g., due to CPU hotplug migrating the IRQ, or a spurious interrupt),
this_cpu_ptr() will return the timer for the currently executing CPU, not
the CPU for which the interrupt was intended.

The handler would then check the wrong CPU's status, find it not pending,
and return IRQ_NONE. The actual pending interrupt would be left
unacknowledged.

Would it be better to pass the specific CPU's timer pointer as dev_id here
and update cevt_interrupt() to use it instead of this_cpu_ptr()?

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