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

[Caleb James DeLisle <cjd@cjdns.fr>]

On 14/05/2026 18:18, sashiko-bot@kernel.org wrote:
> 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?
Such a DT wouldn't pass validation, however perhaps there is too much 
code inferring whether we're a EN751221 or an EN751627. Will think on it.
>
> 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?
A CPU without a timer is non-functional, so IMO this should fail as hard 
as possible.
>
>> @@ -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?
Okay yes.
>
>> +
>> +		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()?
Think I'll use IRQ_NOAUTOEN and enable later, so if for some reason the 
CPU never starts it will failsafe.
>
> Also, since request_irq() immediately unmasks the interrupt, what happens if
> a timer interrupt is already pending from the bootloader?
I think with IRQ_NOAUTOEN this will be fine.
>
> 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?
IRQ_NOAUTOEN fixes this.
>
>> +		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?
Whoops, that was a mistake.
>
>> @@ -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?
Will address per note on previous patch.
>
>> @@ -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?
Will re-work following notes on previous patch.
> 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?
Reference code does not set CONFIG_HOTPLUG_CPU, and similar chips 
(MT7621) do not set it in OpenWrt so I'm not sure it it's known how to 
get it working. In any case I think if we ever get there, the timer will 
be just one of many concerns.