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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Missing validation for zero address blocks leads to a NULL pointer dereference on malformed device trees.
- [High] Missing CPU hotplug teardown callback causes unhandled IRQ storms and unbalanced IRQ enables.
- [High] Failure during CPU hotplug setup unmaps active timer memory, leading to a fatal page fault.
--

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

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

This commit adds support for the EN751627 hardware. It allows the driver to
operate with a different interrupt number for each CPU timer, rather than
relying on a single percpu interrupt as used by the EN751221.

> diff --git a/drivers/clocksource/timer-econet-en751221.c b/drivers/clocksource/timer-econet-en751221.c
> index ed750e39cc4fa..529f81118eef3 100644
> --- a/drivers/clocksource/timer-econet-en751221.c
> +++ b/drivers/clocksource/timer-econet-en751221.c
> @@ -99,6 +101,25 @@ static int cevt_init_cpu(uint cpu)
>  	struct clock_event_device *cd = &per_cpu(econet_timer_pcpu, cpu);
>  	u32 reg;
>  
> +	if (!reg_ctl(cpu)) {
> +		pr_err("%s: missing address resource for CPU %d\n", cd->name,
> +		       cpu);
> +		return -EINVAL;
> +	}
> +	if (cd->irq <= 0) {
> +		pr_err("%s: missing IRQ for CPU %d\n", cd->name, cpu);
> +		return -EINVAL;
> +	}
> +	if (!econet_timer.is_percpu) {
> +		int ret = irq_force_affinity(cd->irq, cpumask_of(cpu));
> +
> +		if (ret) {
> +			pr_err("%s: failed to set IRQ affinity to CPU %d: %pe\n",
> +			       cd->name, cpu, ERR_PTR(ret));
> +			return ret;
> +		}
> +	}
> +
>  	pr_debug("%s: Setting up clockevent for CPU %d\n", cd->name, cpu);
>  
>  	reg = ioread32(reg_ctl(cpu)) | ctl_bit_enabled(cpu);
> @@ -107,7 +128,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);

If a CPU is taken offline, does the lack of a teardown callback for the CPU
hotplug state mean disable_irq() is never called?

If the CPU comes back online, cevt_init_cpu() will call enable_irq() again.
Since the internal depth counter of the IRQ is already 0, could this trigger
an unbalanced enable warning?

Additionally, if the CPU goes offline and the still-active standard IRQ is
migrated to another CPU, could this cause an unhandled IRQ storm? The timer
is still running and the interrupt handler might fail to clear the pending
interrupt on the new CPU.

[ ... ]
> @@ -138,19 +162,48 @@ 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);

If a malformed device tree lacks a reg property, num_blocks will be 0. The
code below checks if num_blocks is greater than ARRAY_SIZE but doesn't check
if it is 0.

Could this lead to a NULL pointer dereference later when clocksource_mmio_init()
is called?

Since the of_iomap() loop will be skipped, econet_timer.membase[0] would
remain NULL, and reg_count(0) would compute an IO address based on it.

> +	int num_irqs = of_irq_count(np);
>  	struct clk *clk;
> -	int ret;
> +	int ret, i;
[ ... ]
> @@ -216,11 +279,19 @@ static int __init timer_init(struct device_node *np)
>  	}
>  
>  	cevt_init(np);
>  
>  	ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
>  				"clockevents/econet/timer:starting",
>  				cevt_init_cpu, NULL);
>  	if (ret < 0) {
>  		pr_err("%pOFn: cpuhp setup failed (%d)\n", np, ret);
>  		goto out_irq_free;
>  	}

If cevt_init_cpu() succeeds on the boot CPU but fails on a subsequent CPU
when irq_force_affinity() returns an error, cpuhp_setup_state() will return
an error and the driver will branch to out_irq_free and then out_membase.

Since the teardown callback is NULL, the clockevent device for the
successfully started CPU is never unregistered.

Could this lead to a fatal page fault when the clockevents core attempts
to access the timer registers after they have been unmapped by iounmap()?

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