Re: [PATCH v2 5/5] clocksource/timer-econet-en751221: Support irq number per timer

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [High] NULL pointer dereference due to requesting IRQ before initializing clock_event_device.
- [High] Interrupt storm and system lockup due to incorrect IRQ context handling.
- [High] Unconditional MMIO mapping based on NR_CPUS causes initialization failure on older platforms.
- [High] Use-After-Free of MMIO memory on initialization error path.
- [High] Out-of-bounds per-CPU memory access if DT defines more IRQs than nr_cpu_ids.
- [Medium] Returning `-EINVAL` from hotplug startup callback aborts CPU hotplug registration.
- [Medium] Missing CPU hotplug teardown callback leaves hardware timer running.
- [Medium] Heuristic hardware detection fails on uniprocessor configurations of multi-IRQ SoCs.
- [Low] Missing `CLOCK_EVT_FEAT_PERCPU` flag for clock event devices in multi-IRQ mode.
--

commit 7bc20cbfbe69fc307b80fd4b2cd349dcdd20410b
Author: Caleb James DeLisle <cjd@cjdns.fr>
Subject: clocksource/timer-econet-en751221: Support irq number per timer

This commit adds support for both percpu and individual IRQ per timer modes.
It handles standard mti,gic compatible interrupt controllers found on MIPS
1004Kc based systems.

Link: https://github.com/openwrt/openwrt/commit/fab098cb6121647ca9cc6e501d56ebe8a9ea550b#diff-a09ee5e4166e89df337d03c1455dce7b81eb89797b1d0f714476b188e6685334

> diff --git a/drivers/clocksource/timer-econet-en751221.c b/drivers/clocksource/timer-econet-en751221.c
> index 4b712eb4db6f6..642af9fcda60e 100644
> --- a/drivers/clocksource/timer-econet-en751221.c
> +++ b/drivers/clocksource/timer-econet-en751221.c
[ ... ]
> @@ -21,14 +22,26 @@
>  #define ECONET_MAX_DELTA		GENMASK(ECONET_BITS - 2, 0)
>  /* 34Kc hardware has 1 block and 1004Kc has 2. */
>  #define ECONET_NUM_BLOCKS		DIV_ROUND_UP(NR_CPUS, 2)
> +#define ECONET_MAX_IRQS			NR_CPUS
>  
>  static struct {
>  	void __iomem	*membase[ECONET_NUM_BLOCKS];
>  	u32		freq_hz;
> +	int		irqs[ECONET_MAX_IRQS];
> +	int		num_irqs;
>  } econet_timer __ro_after_init;
>  
>  static DEFINE_PER_CPU(struct clock_event_device, econet_timer_pcpu);
>  
> +/* This timer supports two interrupt controller models, either 1 IRQ which is in per-cpu
> + * mode which is used on 34Kc CPUs, and separate IRQ number per CPU which is used on
> + * 1004Kc CPUs with GIC intc.
> + */
> +static inline bool is_percpu_irq(void)
> +{
> +	return econet_timer.num_irqs == 1;
> +}

If an SoC using the standard GIC model is configured as a uniprocessor system,
will the Device Tree only describe 1 interrupt?

If so, does this heuristic mistakenly invoke request_percpu_irq() on a standard
SPI interrupt, preventing timer initialization?

> @@ -98,12 +111,21 @@ static int cevt_init_cpu(uint cpu)
>  	struct clock_event_device *cd = &per_cpu(econet_timer_pcpu, cpu);
>  	u32 reg;
>  
> +	if (!is_percpu_irq() && cpu >= econet_timer.num_irqs)
> +		return -EINVAL;

Since this function is registered as a CPU hotplug startup callback via
cpuhp_setup_state(), does returning an error here cause the hotplug core
to abort and rollback the state?

Should this return 0 instead when intentionally skipping a CPU?

> @@ -171,6 +193,44 @@ static int __init cevt_init(struct device_node *np)
>  	return ret;
>  }
>  
> +static int __init cevt_init_multi_irq(struct device_node *np)
> +{
> +	int i, ret;
> +
> +	for (i = 0; i < econet_timer.num_irqs; i++) {
> +		struct clock_event_device *cd = &per_cpu(econet_timer_pcpu, i);

If the system boots with restricted CPUs (e.g. nr_cpus=1) but the device tree
provides multiple IRQs, will this loop access uninitialized __per_cpu_offset
indices?

Should the iteration be bounded by nr_cpu_ids?

> +
> +		econet_timer.irqs[i] = irq_of_parse_and_map(np, i);
> +		if (econet_timer.irqs[i] <= 0) {
> +			pr_err("%pOFn: irq_of_parse_and_map failed", np);
> +			ret = -EINVAL;
> +			goto err_free_irqs;
> +		}
> +
> +		ret = request_irq(econet_timer.irqs[i], cevt_interrupt,
> +				  IRQF_TIMER | IRQF_NOBALANCING,
> +				  np->name, NULL);

Because standard IRQs can be routed to a default CPU before
irq_force_affinity() is applied, can an interrupt for CPU 1's timer execute
on CPU 0?

If so, will CPU 0 check its own timer, find it not pending, and
return IRQ_NONE without acknowledging CPU 1's timer, causing an IRQ storm?

Should a dev_id be passed here and retrieved in the interrupt handler
instead of relying on this_cpu_ptr()?

Also, since request_irq() immediately unmasks the interrupt, what happens if
a timer interrupt is already pending from the bootloader?

Will cevt_interrupt() execute immediately, retrieve the statically
zero-initialized clock_event_device, and dereference a NULL dev->cpumask
because cevt_setup_clockevent() has not yet been called?

> +		if (ret < 0) {
> +			pr_err("%pOFn: IRQ %d setup failed (%d)\n", np,
> +			       econet_timer.irqs[i], ret);
> +			irq_dispose_mapping(econet_timer.irqs[i]);
> +			goto err_free_irqs;
> +		}
> +
> +		cevt_setup_clockevent(cd, np, econet_timer.irqs[i], i);
> +		cevt_dev_init(i);

Should the CLOCK_EVT_FEAT_PERCPU flag be set on the clock_event_device
here to inform the clockevents core about the hardware's strict CPU
locality?

> @@ -184,6 +244,12 @@ static int __init timer_init(struct device_node *np)
>  
>  	econet_timer.freq_hz = clk_get_rate(clk);
>  
> +	econet_timer.num_irqs = of_irq_count(np);
> +	if (econet_timer.num_irqs <= 0 || econet_timer.num_irqs > ECONET_MAX_IRQS) {
> +		pr_err("%pOFn: invalid IRQ count %d\n", np, econet_timer.num_irqs);
> +		return -EINVAL;
> +	}
> +
>  	for (int i = 0; i < ARRAY_SIZE(econet_timer.membase); i++) {
>  		econet_timer.membase[i] = of_iomap(np, i);

Since the 34Kc platform only has 1 timer block and provides a single reg
region in its Device Tree, will of_iomap(np, 1) return NULL and cause
initialization to fail if NR_CPUS makes ARRAY_SIZE(econet_timer.membase)
evaluate to 2 or more?

> @@ -202,7 +268,11 @@ static int __init timer_init(struct device_node *np)
>  		goto err_unmap;
>  	}
>  
> -	ret = cevt_init(np);
> +	if (is_percpu_irq())
> +		ret = cevt_init(np);
> +	else
> +		ret = cevt_init_multi_irq(np);
> +
>  	if (ret < 0)
>  		goto err_unmap;

If cevt_init_multi_irq() fails and the code jumps to err_unmap, the
previously registered clocksource from clocksource_mmio_init() is not
unregistered.

Does this leave the timekeeping subsystem with a dangling pointer to the
unmapped MMIO region, leading to a crash on the next time read?

Additionally, when a CPU goes offline, is the hardware timer ever disabled
since there is no teardown callback provided to cpuhp_setup_state() in this
function and no set_state_shutdown function?

Will the online CPUs continuously receive timer interrupts from the offline
CPU's running timer?

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