Re: [PATCH v7 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] Missing CPU hotplug teardown callback and missing IRQ_PER_CPU flag causes an interrupt storm and unbalanced enable_irq warnings on CPU offline/online.
- [High] Timer driver completely fails to probe if the kernel is compiled with an `NR_CPUS` smaller than the hardware's core count.
--

--- Patch [5]: [PATCH v7 5/5] clocksource/timer-econet-en751221: Support EN751627 without percpu IRQ ---
commit 4d23ebf2434d347d3628c6259b6442637444c87f
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
> 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: High]
Since cpuhp_setup_state() is registered in timer_init() with a NULL
teardown callback, disable_irq() is never called when a CPU goes offline.

Does this mean that onlining the CPU again will call enable_irq() on an IRQ
that was never disabled, triggering an unbalanced enable kernel warning?

[ ... ]

> @@ -141,19 +165,52 @@ static void __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);
> +	int num_blocks = of_address_count(np);
> +	int num_irqs = of_irq_count(np);
>  	struct clk *clk;
> -	int ret;
> +	int ret, i;
> +
> +	econet_timer.is_percpu = of_device_is_compatible(np, "econet,en751221-timer");
> +
> +	if (econet_timer.is_percpu && num_irqs != 1) {
> +		pr_err("%pOFn: EN751221 clock must have 1 IRQ not %d\n", np,
> +		       num_irqs);
> +		return -EINVAL;
> +	}
> +	if (num_irqs > ARRAY_SIZE(econet_timer.irqs)) {
> +		pr_err("%pOFn: Too many IRQs max %d got %d\n", np,
> +		       ARRAY_SIZE(econet_timer.irqs), num_irqs);
> +		return -EINVAL;
> +	}
> +	if (num_blocks > ARRAY_SIZE(econet_timer.membase)) {
> +		pr_err("%pOFn: Too many regs: max %d got %d\n", np,
> +		       ARRAY_SIZE(econet_timer.membase), num_blocks);
> +		return -EINVAL;
> +	}

[Severity: High]
Does this code prevent the system from booting if the kernel is compiled
with an NR_CPUS limit smaller than the actual hardware core count?

If the Device Tree exposes more IRQs or register blocks than the
compile-time NR_CPUS bound allows, returning -EINVAL here will cause the
timer probe to fail entirely.

Would it be safer to gracefully bound the resource counts with min() and
initialize up to NR_CPUS timers instead of erroring out?

[ ... ]

> -	irq_set_status_flags(econet_timer.irq, IRQ_NOAUTOEN);
> -
> -	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]
Does this non-percpu path need to set the IRQ_PER_CPU flag on the interrupt?

When a CPU is offlined, irq_migrate_all_off_this_cpu() will forcibly migrate
this interrupt to a surviving CPU because it isn't marked as a per-CPU
interrupt.

When the hardware timer interrupts the new CPU, cevt_interrupt() uses
this_cpu_ptr() to check the new CPU's timer pending bit. That evaluates
to false, causing it to return IRQ_NONE without acknowledging the original
CPU's timer interrupt.

Could this lead to an unhandled interrupt storm that hangs the system?

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

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