[PATCH v3 0/4] clocksource/timer-econet-en751221: Support irq number per timer

[PATCH v3 0/4] clocksource/timer-econet-en751221: Support irq number per timer

[Caleb James DeLisle]

I decided to take this patchset in a slightly different direction from
v2. There is a certain amount of initialization that once it has run,
it cannot be reverted in case of error. The original driver init was a
mixture of revertible and non-revertible code which made it too easy
for code which is designed with best practices in mind to in fact
introduce a bug. Therefore I start by making the init process more
legible but without any business logic changes, then fix a race
condition by keeping IRQ disabled until after cevt is setup, then
finally add non-percpu IRQ support.

* v2: https://lore.kernel.org/linux-mips/20260514000601.3430262-1-cjd@cjdns.fr

Changes from v1:
* Split changes over 3 refactoring patches + main patch
* Remove driver discussion from dt commit message
* v1: https://lore.kernel.org/linux-mips/20260416175101.958073-1-cjd@cjdns.fr/


Caleb James DeLisle (4):
  dt-bindings: timer: econet: Update EN751627 for multi-IRQ
  clocksource/timer-econet-en751221: Init teardown on error if possible
  clocksource/timer-econet-en751221: Disable IRQ until cevt registered
  clocksource/timer-econet-en751221: Support EN751627 without percpu IRQ

 .../bindings/timer/econet,en751221-timer.yaml |  23 ++-
 drivers/clocksource/timer-econet-en751221.c   | 159 +++++++++++++-----
 2 files changed, 136 insertions(+), 46 deletions(-)


base-commit: ff1c0c5d07028a84837950b619d30da623f8ddb2
-- 
2.39.5

[PATCH v3 1/4] dt-bindings: timer: econet: Update EN751627 for multi-IRQ

[Caleb James DeLisle]

This hardware is found in the EN751221 SoC family as well as the
EN751627. The former uses a percpu IRQ for all timers while the
latter uses an individual IRQ number per timer.

Signed-off-by: Caleb James DeLisle <cjd@cjdns.fr>
Acked-by: Conor Dooley <conor.dooley@microchip.com>
---
 .../bindings/timer/econet,en751221-timer.yaml | 23 ++++++++++++++-----
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/timer/econet,en751221-timer.yaml b/Documentation/devicetree/bindings/timer/econet,en751221-timer.yaml
index c1e7c2b6afde..16f32741473f 100644
--- a/Documentation/devicetree/bindings/timer/econet,en751221-timer.yaml
+++ b/Documentation/devicetree/bindings/timer/econet,en751221-timer.yaml
@@ -12,8 +12,9 @@ maintainers:
 description:
   The EcoNet High Precision Timer (HPT) is a timer peripheral found in various
   EcoNet SoCs, including the EN751221 and EN751627 families. It provides per-VPE
-  count/compare registers and a per-CPU control register, with a single interrupt
-  line using a percpu-devid interrupt mechanism.
+  count/compare registers and a per-CPU control register. On EN751221 it uses a
+  single interrupt line using a percpu-devid interrupt mechanism, and on
+  EN751627 it uses an interrupt per VPE.
 
 properties:
   compatible:
@@ -28,8 +29,8 @@ properties:
     maxItems: 2
 
   interrupts:
-    maxItems: 1
-    description: A percpu-devid timer interrupt shared across CPUs.
+    minItems: 1
+    maxItems: 4
 
   clocks:
     maxItems: 1
@@ -52,21 +53,31 @@ allOf:
           items:
             - description: VPE timers 0 and 1
             - description: VPE timers 2 and 3
+        interrupts:
+          description: An interrupt for each timer (one per VPE)
+          minItems: 4
     else:
       properties:
         reg:
           items:
             - description: VPE timers 0 and 1
+        interrupts:
+          description: A percpu-devid timer interrupt shared across timers
+          maxItems: 1
 
 additionalProperties: false
 
 examples:
   - |
+    #include <dt-bindings/interrupt-controller/mips-gic.h>
     timer@1fbf0400 {
-        compatible = "econet,en751627-timer", "econet,en751221-timer";
+        compatible = "econet,en751627-timer";
         reg = <0x1fbf0400 0x100>, <0x1fbe0000 0x100>;
         interrupt-parent = <&intc>;
-        interrupts = <30>;
+        interrupts = <GIC_SHARED 30 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SHARED 29 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SHARED 37 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SHARED 36 IRQ_TYPE_LEVEL_HIGH>;
         clocks = <&hpt_clock>;
     };
   - |
-- 
2.39.5

Re: [PATCH v3 1/4] dt-bindings: timer: econet: Update EN751627 for multi-IRQ

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] DT Binding Example Validation Failure
--

commit 2aadbe8d15e40c585e5998e7532f23b02294a2a3
Author: Caleb James DeLisle <cjd@cjdns.fr>

dt-bindings: timer: econet: Update EN751627 for multi-IRQ

This commit updates the EN751627 timer binding to describe its use of an
individual IRQ per timer (4 total) compared to the single percpu IRQ used
by the EN751221 family. The constraints and example are updated accordingly.

> diff --git a/Documentation/devicetree/bindings/timer/econet,en751221-timer.yaml b/Documentation/devicetree/bindings/timer/econet,en751221-timer.yaml
> index c1e7c2b6afde1..16f32741473fe 100644
> --- a/Documentation/devicetree/bindings/timer/econet,en751221-timer.yaml
> +++ b/Documentation/devicetree/bindings/timer/econet,en751221-timer.yaml

[ ... ]

> @@ -52,21 +53,31 @@ allOf:

[ ... ]

>  examples:
>    - |
> +    #include <dt-bindings/interrupt-controller/mips-gic.h>
>      timer@1fbf0400 {
> -        compatible = "econet,en751627-timer", "econet,en751221-timer";
> +        compatible = "econet,en751627-timer";

Does this updated example fail dt_binding_check validation?

The example changes the compatible property to a single string, removing the
fallback to "econet,en751221-timer". This correctly reflects that the two
hardware variants have incompatible interrupt topologies.

However, it appears the properties definition for compatible in this schema
was not updated. If the schema still requires "econet,en751627-timer" to be
part of an array with the "econet,en751221-timer" fallback, this single
string example will not match the expected array schema.

Should the compatible schema definition be updated to allow
"econet,en751627-timer" as a standalone string?

>          reg = <0x1fbf0400 0x100>, <0x1fbe0000 0x100>;
>          interrupt-parent = <&intc>;
> -        interrupts = <30>;
> +        interrupts = <GIC_SHARED 30 IRQ_TYPE_LEVEL_HIGH>,
> +                     <GIC_SHARED 29 IRQ_TYPE_LEVEL_HIGH>,
> +                     <GIC_SHARED 37 IRQ_TYPE_LEVEL_HIGH>,
> +                     <GIC_SHARED 36 IRQ_TYPE_LEVEL_HIGH>;
>          clocks = <&hpt_clock>;
>      };

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

[PATCH v3 2/4] clocksource/timer-econet-en751221: Init teardown on error if possible

[Caleb James DeLisle]

As a clocksource, much of the initialization process is irreversible
and the impact of a failure to initialize is a failure to boot.
That said, good practice is to attempt a clean exit if probing fails,
and supporting this pattern will reduce the likelihood that future
contributions introduce a bug by trying to teardown after it is no
longer possible to do so.

Convert the init process into two clearly delineated phases, one
which is reverted in case of error, and the other which can't be.
Move all IRQ and address resource mapping before that point, and add
teardown logic in case of error before the point of no return.

Signed-off-by: Caleb James DeLisle <cjd@cjdns.fr>
---
 drivers/clocksource/timer-econet-en751221.c | 82 ++++++++++++---------
 1 file changed, 48 insertions(+), 34 deletions(-)

diff --git a/drivers/clocksource/timer-econet-en751221.c b/drivers/clocksource/timer-econet-en751221.c
index 4008076b1a21..155471f68e6f 100644
--- a/drivers/clocksource/timer-econet-en751221.c
+++ b/drivers/clocksource/timer-econet-en751221.c
@@ -24,6 +24,7 @@
 
 static struct {
 	void __iomem	*membase[ECONET_NUM_BLOCKS];
+	int		irq;
 	u32		freq_hz;
 } econet_timer __ro_after_init;
 
@@ -126,22 +127,9 @@ static void __init cevt_dev_init(uint cpu)
 	iowrite32(U32_MAX, reg_compare(cpu));
 }
 
-static int __init cevt_init(struct device_node *np)
+static void __init cevt_init(struct device_node *np)
 {
-	int i, irq, ret;
-
-	irq = irq_of_parse_and_map(np, 0);
-	if (irq <= 0) {
-		pr_err("%pOFn: irq_of_parse_and_map failed", np);
-		return -EINVAL;
-	}
-
-	ret = request_percpu_irq(irq, cevt_interrupt, np->name, &econet_timer_pcpu);
-
-	if (ret < 0) {
-		pr_err("%pOFn: IRQ %d setup failed (%d)\n", np, irq, ret);
-		goto err_unmap_irq;
-	}
+	int i;
 
 	for_each_possible_cpu(i) {
 		struct clock_event_device *cd = &per_cpu(econet_timer_pcpu, i);
@@ -151,21 +139,12 @@ static int __init cevt_init(struct device_node *np)
 					  CLOCK_EVT_FEAT_C3STOP |
 					  CLOCK_EVT_FEAT_PERCPU;
 		cd->set_next_event	= cevt_set_next_event;
-		cd->irq			= irq;
+		cd->irq			= econet_timer.irq;
 		cd->cpumask		= cpumask_of(i);
 		cd->name		= np->name;
 
 		cevt_dev_init(i);
 	}
-
-	cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
-			  "clockevents/econet/timer:starting",
-			  cevt_init_cpu, NULL);
-	return 0;
-
-err_unmap_irq:
-	irq_dispose_mapping(irq);
-	return ret;
 }
 
 static int __init timer_init(struct device_node *np)
@@ -186,22 +165,45 @@ static int __init timer_init(struct device_node *np)
 		econet_timer.membase[i] = of_iomap(np, i);
 		if (!econet_timer.membase[i]) {
 			pr_err("%pOFn: failed to map register [%d]\n", np, i);
-			return -ENXIO;
+			ret = -ENXIO;
+			goto out_membase;
 		}
 	}
 
+	econet_timer.irq = irq_of_parse_and_map(np, 0);
+	if (econet_timer.irq <= 0) {
+		pr_err("%pOFn: irq_of_parse_and_map failed\n", np);
+		ret = -EINVAL;
+		goto out_membase;
+	}
+
+	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;
+	}
+
+	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;
+	}
+
+	/* Point of no return, do not attempt to tear down after this. */
+
 	/* For clocksource purposes always read clock zero, whatever the CPU */
 	ret = clocksource_mmio_init(reg_count(0), np->name,
 				    econet_timer.freq_hz, 301, ECONET_BITS,
 				    clocksource_mmio_readl_up);
-	if (ret) {
-		pr_err("%pOFn: clocksource_mmio_init failed: %d", np, ret);
-		return ret;
-	}
-
-	ret = cevt_init(np);
-	if (ret < 0)
-		return ret;
+	if (ret)
+		pr_err("%pOFn: clocksource_mmio_init failed: %d\n", np, ret);
 
 	sched_clock_register(sched_clock_read, ECONET_BITS,
 			     econet_timer.freq_hz);
@@ -211,6 +213,18 @@ static int __init timer_init(struct device_node *np)
 		(econet_timer.freq_hz / 1000) % 1000);
 
 	return 0;
+
+out_irq_free:
+	free_percpu_irq(econet_timer.irq, &econet_timer_pcpu);
+out_irq_mapping:
+	irq_dispose_mapping(econet_timer.irq);
+out_membase:
+	for (int i = 0; i < ARRAY_SIZE(econet_timer.membase); i++) {
+		if (econet_timer.membase[i])
+			iounmap(econet_timer.membase[i]);
+	}
+
+	return ret;
 }
 
 TIMER_OF_DECLARE(econet_timer_hpt, "econet,en751221-timer", timer_init);
-- 
2.39.5

Re: [PATCH v3 2/4] clocksource/timer-econet-en751221: Init teardown on error if possible

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The error teardown path for `cpuhp_setup_state()` failure unmaps MMIO memory without unregistering active clock event devices, creating a latent use-after-unmap bug.
- [High] `cevt_dev_init()` accesses memory blocks using absolute CPU IDs, causing a NULL pointer dereference if the CPU mask is sparse or the device tree is incomplete.
--

commit 28fb215150f468c2962087cefa4f634e20465bd8
Author: Caleb James DeLisle <cjd@cjdns.fr>

clocksource/timer-econet-en751221: Init teardown on error if possible

This commit converts the initialization process into two clearly delineated
phases. It moves all IRQ and address resource mapping before the point of no
return and adds teardown logic in case of error.

> diff --git a/drivers/clocksource/timer-econet-en751221.c b/drivers/clocksource/timer-econet-en751221.c
> index 4008076b1a210..155471f68e6f8 100644
> --- a/drivers/clocksource/timer-econet-en751221.c
> +++ b/drivers/clocksource/timer-econet-en751221.c

[ ... ]

> @@ -126,22 +127,9 @@ static void __init cevt_dev_init(uint cpu)
>  	iowrite32(U32_MAX, reg_compare(cpu));
>  }
>  
> -static int __init cevt_init(struct device_node *np)
> +static void __init cevt_init(struct device_node *np)
>  {
> -	int i, irq, ret;
> -
> -	irq = irq_of_parse_and_map(np, 0);
> -	if (irq <= 0) {
> -		pr_err("%pOFn: irq_of_parse_and_map failed", np);
> -		return -EINVAL;
> -	}
> -
> -	ret = request_percpu_irq(irq, cevt_interrupt, np->name, &econet_timer_pcpu);
> -
> -	if (ret < 0) {
> -		pr_err("%pOFn: IRQ %d setup failed (%d)\n", np, irq, ret);
> -		goto err_unmap_irq;
> -	}
> +	int i;
>  
>  	for_each_possible_cpu(i) {
>  		struct clock_event_device *cd = &per_cpu(econet_timer_pcpu, i);
> @@ -151,21 +139,12 @@ static int __init cevt_init(struct device_node *np)
>  					  CLOCK_EVT_FEAT_C3STOP |
>  					  CLOCK_EVT_FEAT_PERCPU;
>  		cd->set_next_event	= cevt_set_next_event;
> -		cd->irq			= irq;
> +		cd->irq			= econet_timer.irq;
>  		cd->cpumask		= cpumask_of(i);
>  		cd->name		= np->name;
>  
>  		cevt_dev_init(i);

Could this iteration cause a NULL pointer dereference if the CPU mask is
sparse?

timer_init() allocates memory blocks based on num_possible_cpus(). If
cpu_possible_mask is sparse (for example, if only CPUs 0 and 3 are present),
num_possible_cpus() is 2, and only 1 block is mapped.

When this loop reaches CPU 3, cevt_dev_init(3) might try to access an
unmapped membase pointer.

A later patch in this series changes the allocation to use of_address_count()
and adds a check in cevt_init_cpu(), but since cevt_dev_init() is called
unconditionally here, the issue appears to remain reachable during early
initialization.

>  	}

[ ... ]

> @@ -186,22 +165,45 @@ static int __init timer_init(struct device_node *np)
>  		econet_timer.membase[i] = of_iomap(np, i);
>  		if (!econet_timer.membase[i]) {
>  			pr_err("%pOFn: failed to map register [%d]\n", np, i);
> -			return -ENXIO;
> +			ret = -ENXIO;
> +			goto out_membase;
>  		}
>  	}

[ ... ]

> +	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 cpuhp_setup_state() fails, does this error path leave active clock event
devices registered with unmapped MMIO memory?

cpuhp_setup_state() invokes its startup callback (cevt_init_cpu) on all
online CPUs, which activates the clock events. If it fails on a subsequent
CPU, the NULL teardown callback means no rollback occurs.

When the error path jumps to out_irq_free and unmaps the MMIO memory, any
subsequent timer interrupt could dereference the unmapped memory.

A later patch in this series modifies cevt_init_cpu() to return errors,
which makes this teardown path reachable.

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

Re: [PATCH v3 2/4] clocksource/timer-econet-en751221: Init teardown on error if possible

[Caleb James DeLisle]

On 16/05/2026 21:05, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] The error teardown path for `cpuhp_setup_state()` failure unmaps MMIO memory without unregistering active clock event devices, creating a latent use-after-unmap bug.
> - [High] `cevt_dev_init()` accesses memory blocks using absolute CPU IDs, causing a NULL pointer dereference if the CPU mask is sparse or the device tree is incomplete.
> --
>
> commit 28fb215150f468c2962087cefa4f634e20465bd8
> Author: Caleb James DeLisle <cjd@cjdns.fr>
>
> clocksource/timer-econet-en751221: Init teardown on error if possible
>
> This commit converts the initialization process into two clearly delineated
> phases. It moves all IRQ and address resource mapping before the point of no
> return and adds teardown logic in case of error.
>
>> diff --git a/drivers/clocksource/timer-econet-en751221.c b/drivers/clocksource/timer-econet-en751221.c
>> index 4008076b1a210..155471f68e6f8 100644
>> --- a/drivers/clocksource/timer-econet-en751221.c
>> +++ b/drivers/clocksource/timer-econet-en751221.c
> [ ... ]
>
>> @@ -126,22 +127,9 @@ static void __init cevt_dev_init(uint cpu)
>>   	iowrite32(U32_MAX, reg_compare(cpu));
>>   }
>>   
>> -static int __init cevt_init(struct device_node *np)
>> +static void __init cevt_init(struct device_node *np)
>>   {
>> -	int i, irq, ret;
>> -
>> -	irq = irq_of_parse_and_map(np, 0);
>> -	if (irq <= 0) {
>> -		pr_err("%pOFn: irq_of_parse_and_map failed", np);
>> -		return -EINVAL;
>> -	}
>> -
>> -	ret = request_percpu_irq(irq, cevt_interrupt, np->name, &econet_timer_pcpu);
>> -
>> -	if (ret < 0) {
>> -		pr_err("%pOFn: IRQ %d setup failed (%d)\n", np, irq, ret);
>> -		goto err_unmap_irq;
>> -	}
>> +	int i;
>>   
>>   	for_each_possible_cpu(i) {
>>   		struct clock_event_device *cd = &per_cpu(econet_timer_pcpu, i);
>> @@ -151,21 +139,12 @@ static int __init cevt_init(struct device_node *np)
>>   					  CLOCK_EVT_FEAT_C3STOP |
>>   					  CLOCK_EVT_FEAT_PERCPU;
>>   		cd->set_next_event	= cevt_set_next_event;
>> -		cd->irq			= irq;
>> +		cd->irq			= econet_timer.irq;
>>   		cd->cpumask		= cpumask_of(i);
>>   		cd->name		= np->name;
>>   
>>   		cevt_dev_init(i);
> Could this iteration cause a NULL pointer dereference if the CPU mask is
> sparse?
>
> timer_init() allocates memory blocks based on num_possible_cpus(). If
> cpu_possible_mask is sparse (for example, if only CPUs 0 and 3 are present),
> num_possible_cpus() is 2, and only 1 block is mapped.
>
> When this loop reaches CPU 3, cevt_dev_init(3) might try to access an
> unmapped membase pointer.
>
> A later patch in this series changes the allocation to use of_address_count()
> and adds a check in cevt_init_cpu(), but since cevt_dev_init() is called
> unconditionally here, the issue appears to remain reachable during early
> initialization.
This is a potential issue but it predates this patch, and this patch 
only tries to fix one thing.
>
>>   	}
> [ ... ]
>
>> @@ -186,22 +165,45 @@ static int __init timer_init(struct device_node *np)
>>   		econet_timer.membase[i] = of_iomap(np, i);
>>   		if (!econet_timer.membase[i]) {
>>   			pr_err("%pOFn: failed to map register [%d]\n", np, i);
>> -			return -ENXIO;
>> +			ret = -ENXIO;
>> +			goto out_membase;
>>   		}
>>   	}
> [ ... ]
>
>> +	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 cpuhp_setup_state() fails, does this error path leave active clock event
> devices registered with unmapped MMIO memory?
>
> cpuhp_setup_state() invokes its startup callback (cevt_init_cpu) on all
> online CPUs, which activates the clock events. If it fails on a subsequent
> CPU, the NULL teardown callback means no rollback occurs.
>
> When the error path jumps to out_irq_free and unmaps the MMIO memory, any
> subsequent timer interrupt could dereference the unmapped memory.
>
> A later patch in this series modifies cevt_init_cpu() to return errors,
> which makes this teardown path reachable.

cevt_init_cpu either returns an error, or it registers a timer, not both. Per my understanding, if there's an error running on CPU0, cpuhp_setup_state will not register it to run on other CPUs.

So I don't think there's anything to be done for this.

[PATCH v3 3/4] clocksource/timer-econet-en751221: Disable IRQ until cevt registered

[Caleb James DeLisle]

Eliminate a race condition where cevt_interrupt may trigger before
clockevents_config_and_register has been called, and dev->event_handler
is at that point NULL.

Signed-off-by: Caleb James DeLisle <cjd@cjdns.fr>
---
 drivers/clocksource/timer-econet-en751221.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/clocksource/timer-econet-en751221.c b/drivers/clocksource/timer-econet-en751221.c
index 155471f68e6f..ed750e39cc4f 100644
--- a/drivers/clocksource/timer-econet-en751221.c
+++ b/drivers/clocksource/timer-econet-en751221.c
@@ -104,12 +104,11 @@ static int cevt_init_cpu(uint cpu)
 	reg = ioread32(reg_ctl(cpu)) | ctl_bit_enabled(cpu);
 	iowrite32(reg, reg_ctl(cpu));
 
-	enable_percpu_irq(cd->irq, IRQ_TYPE_NONE);
-
-	/* Do this last because it synchronously configures the timer */
 	clockevents_config_and_register(cd, econet_timer.freq_hz,
 					ECONET_MIN_DELTA, ECONET_MAX_DELTA);
 
+	enable_percpu_irq(cd->irq, IRQ_TYPE_NONE);
+
 	return 0;
 }
 
@@ -177,6 +176,8 @@ static int __init timer_init(struct device_node *np)
 		goto out_membase;
 	}
 
+	irq_set_status_flags(econet_timer.irq, IRQ_NOAUTOEN);
+
 	ret = request_percpu_irq(econet_timer.irq, cevt_interrupt, np->name,
 				 &econet_timer_pcpu);
 
-- 
2.39.5

Re: [PATCH v3 3/4] clocksource/timer-econet-en751221: Disable IRQ until cevt registered

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Incomplete Race Condition Fix / NULL Pointer Dereference
- [High] Missing Teardown in CPU Hotplug / Error Path Rollback Failure
- [High] Missing Hardware Clock Enable and Reference Leak
- [Low] Redundant API Usage for per-CPU Interrupt
--

commit 5ebbb6312519c50062084d8964d9f2a0fe342d0f
Author: Caleb James DeLisle <cjd@cjdns.fr>

clocksource/timer-econet-en751221: Disable IRQ until cevt registered

This commit attempts to eliminate a race condition where the interrupt
could trigger before the clockevent device is fully registered and its
event handler is set. It achieves this by setting the no-auto-enable
flag and delaying the interrupt enablement until after registration.

> diff --git a/drivers/clocksource/timer-econet-en751221.c b/drivers/clocksource/timer-econet-en751221.c
> index 155471f68e6f8..ed750e39cc4fa 100644
> --- a/drivers/clocksource/timer-econet-en751221.c
> +++ b/drivers/clocksource/timer-econet-en751221.c
> @@ -104,12 +104,11 @@ static int cevt_init_cpu(uint cpu)
>  	reg = ioread32(reg_ctl(cpu)) | ctl_bit_enabled(cpu);
>  	iowrite32(reg, reg_ctl(cpu));
>  
> -	enable_percpu_irq(cd->irq, IRQ_TYPE_NONE);
> -
> -	/* Do this last because it synchronously configures the timer */
>  	clockevents_config_and_register(cd, econet_timer.freq_hz,
>  					ECONET_MIN_DELTA, ECONET_MAX_DELTA);
>  
> +	enable_percpu_irq(cd->irq, IRQ_TYPE_NONE);

Does this completely resolve the race condition?

If the clockevents core rejects the new device during registration,
dev->event_handler remains null. Because the hardware timer is unconditionally
started above and there is no set_state_shutdown callback, the timer cannot
be stopped on rejection.

When the timer eventually wraps and fires, will cevt_interrupt() execute
a null event_handler and crash?

>  	return 0;
>  }
>  
> @@ -177,6 +176,8 @@ static int __init timer_init(struct device_node *np)
>  		goto out_membase;
>  	}
>  
> +	irq_set_status_flags(econet_timer.irq, IRQ_NOAUTOEN);

This isn't a bug, but is this explicit flag setting redundant?

request_percpu_irq() requires the target interrupt to be configured as a
per-cpu devid interrupt. The core function irq_set_percpu_devid_flags()
already applies IRQ_NOAUTOEN alongside IRQ_PER_CPU_DEVID, meaning the flag
is guaranteed to be set before request_percpu_irq() is executed.

Are we missing a call to clk_prepare_enable() for the clock fetched earlier
in timer_init()?

If the clock is never formally enabled with a positive reference count, the
common clock framework late initcall clk_disable_unused() might turn off
the clock. Would shutting off the clock underneath an active system timer
instantly stop the hardware counter and lock up timekeeping?

Also, does this code leak the clock reference by never calling clk_put()
on the returned clock pointer on both success and error paths?

Is a teardown callback missing for cpu hotplug?

timer_init() calls cpuhp_setup_state() with null as the teardown callback:

    ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
                            "clockevents/econet/timer:starting",
                            cevt_init_cpu, NULL);

When a cpu is offlined, disable_percpu_irq() is never called, leaving the
per-cpu interrupt unmasked on the dying cpu. Without a set_state_shutdown
callback, does the hardware timer remain running, potentially causing
spurious wakeups or preventing low-power states?

Furthermore, if cpuhp_setup_state() fails on a cpu, the core cannot roll back
the successfully started processors because of the missing teardown callback.
The error path then frees the interrupt action while hardware timers are
still running. Will this lead to an unhandled interrupt storm?

>  	ret = request_percpu_irq(econet_timer.irq, cevt_interrupt, np->name,
>  				 &econet_timer_pcpu);
>

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

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

[Caleb James DeLisle]

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.

Signed-off-by: Caleb James DeLisle <cjd@cjdns.fr>
---
 drivers/clocksource/timer-econet-en751221.c | 110 ++++++++++++++++----
 1 file changed, 87 insertions(+), 23 deletions(-)

diff --git a/drivers/clocksource/timer-econet-en751221.c b/drivers/clocksource/timer-econet-en751221.c
index ed750e39cc4f..dea6dbafa16e 100644
--- a/drivers/clocksource/timer-econet-en751221.c
+++ b/drivers/clocksource/timer-econet-en751221.c
@@ -21,10 +21,12 @@
 #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_NUM_IRQS			NR_CPUS
 
 static struct {
 	void __iomem	*membase[ECONET_NUM_BLOCKS];
-	int		irq;
+	int		irqs[ECONET_NUM_IRQS];
+	bool		is_percpu;
 	u32		freq_hz;
 } econet_timer __ro_after_init;
 
@@ -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);
 
 	return 0;
 }
@@ -138,7 +162,12 @@ static void __init cevt_init(struct device_node *np)
 					  CLOCK_EVT_FEAT_C3STOP |
 					  CLOCK_EVT_FEAT_PERCPU;
 		cd->set_next_event	= cevt_set_next_event;
-		cd->irq			= econet_timer.irq;
+
+		if (econet_timer.is_percpu)
+			cd->irq = econet_timer.irqs[0];
+		else
+			cd->irq = econet_timer.irqs[i];
+
 		cd->cpumask		= cpumask_of(i);
 		cd->name		= np->name;
 
@@ -148,9 +177,23 @@ 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;
+	}
 
 	clk = of_clk_get(np, 0);
 	if (IS_ERR(clk)) {
@@ -160,7 +203,7 @@ static int __init timer_init(struct device_node *np)
 
 	econet_timer.freq_hz = clk_get_rate(clk);
 
-	for (int i = 0; i < num_blocks; i++) {
+	for (i = 0; i < num_blocks; i++) {
 		econet_timer.membase[i] = of_iomap(np, i);
 		if (!econet_timer.membase[i]) {
 			pr_err("%pOFn: failed to map register [%d]\n", np, i);
@@ -169,22 +212,33 @@ static int __init timer_init(struct device_node *np)
 		}
 	}
 
-	econet_timer.irq = irq_of_parse_and_map(np, 0);
-	if (econet_timer.irq <= 0) {
-		pr_err("%pOFn: irq_of_parse_and_map failed\n", np);
-		ret = -EINVAL;
-		goto out_membase;
+	for (i = 0; i < num_irqs; i++) {
+		econet_timer.irqs[i] = irq_of_parse_and_map(np, i);
+		if (econet_timer.irqs[i] <= 0) {
+			pr_err("%pOFn: failed mapping irq %d\n", np, i);
+			ret = -EINVAL;
+			goto out_irq_mapping;
+		}
 	}
 
-	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);
+
+		if (ret < 0) {
+			pr_err("%pOFn: IRQ %d setup failed: %pe\n", np,
+			       i, ERR_PTR(ret));
+			i--;
+			goto out_irq_free;
+		}
 	}
 
 	cevt_init(np);
@@ -216,11 +270,20 @@ static int __init timer_init(struct device_node *np)
 	return 0;
 
 out_irq_free:
-	free_percpu_irq(econet_timer.irq, &econet_timer_pcpu);
+	for (; i >= 0; i--) {
+		if (econet_timer.is_percpu) {
+			free_percpu_irq(econet_timer.irqs[i], &econet_timer_pcpu);
+		} else {
+			free_irq(econet_timer.irqs[i], NULL);
+		}
+	}
 out_irq_mapping:
-	irq_dispose_mapping(econet_timer.irq);
+	for (i = 0; i < num_irqs; i++) {
+		if (econet_timer.irqs[i] > 0)
+			irq_dispose_mapping(econet_timer.irqs[i]);
+	}
 out_membase:
-	for (int i = 0; i < ARRAY_SIZE(econet_timer.membase); i++) {
+	for (i = 0; i < num_blocks; i++) {
 		if (econet_timer.membase[i])
 			iounmap(econet_timer.membase[i]);
 	}
@@ -229,3 +292,4 @@ static int __init timer_init(struct device_node *np)
 }
 
 TIMER_OF_DECLARE(econet_timer_hpt, "econet,en751221-timer", timer_init);
+TIMER_OF_DECLARE(econet_timer_en751627, "econet,en751627-timer", timer_init);
-- 
2.39.5

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Missing bounds check on `num_blocks` leads to buffer overflow and memory corruption.
- [High] Off-by-one error in `cpuhp_setup_state` failure path causes out-of-bounds array access and invalid IRQ free.
- [High] Missing CPU hotplug teardown callback causes unbalanced `enable_irq` warnings and use-after-unmap crashes on initialization failure.
--

commit c9306a7aa40ae1fa4dc34819e52ba88137234f75
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 ed750e39cc4fa..dea6dbafa16ea 100644
> --- a/drivers/clocksource/timer-econet-en751221.c
> +++ b/drivers/clocksource/timer-econet-en751221.c
> @@ -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);

Will this cause unbalanced enable_irq() warnings when a CPU is brought back
online?

Since cpuhp_setup_state() provides NULL for the teardown callback, taking a
CPU offline leaves the IRQ depth at 0.

When brought back online, cevt_init_cpu() calls enable_irq() again, which
could trigger the IRQ subsystem's unbalanced enable warning.

Also, if cpuhp_setup_state() fails midway, the hotplug core attempts to roll
back CPUs but does nothing due to the NULL teardown callback. The driver then
proceeds to unmap MMIO memory and free IRQs.

Could this leave active clockevent devices registered that will crash the
system when their set_next_event callback accesses the unmapped memory?

[ ... ]

> @@ -148,9 +177,23 @@ 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);

[ ... ]

> @@ -160,7 +203,7 @@ static int __init timer_init(struct device_node *np)
>  
>  	econet_timer.freq_hz = clk_get_rate(clk);
>  
> -	for (int i = 0; i < num_blocks; i++) {
> +	for (i = 0; i < num_blocks; i++) {
>  		econet_timer.membase[i] = of_iomap(np, i);

Could this overflow the econet_timer.membase array?

The econet_timer.membase array is statically sized to ECONET_NUM_BLOCKS.

If a device tree specifies more address resources than ECONET_NUM_BLOCKS, this
loop will write out of bounds and corrupt adjacent memory like the irqs array.

Also, if num_blocks is less than required for the number of CPUs, will
cevt_dev_init() dereference unmapped NULL memory?

[ ... ]

> @@ -216,11 +270,20 @@ static int __init timer_init(struct device_node *np)
>  	return 0;
>  
>  out_irq_free:
> -	free_percpu_irq(econet_timer.irq, &econet_timer_pcpu);
> +	for (; i >= 0; i--) {
> +		if (econet_timer.is_percpu) {
> +			free_percpu_irq(econet_timer.irqs[i], &econet_timer_pcpu);
> +		} else {
> +			free_irq(econet_timer.irqs[i], NULL);
> +		}
> +	}

Could this perform an out-of-bounds access or free an uninitialized IRQ?

After successfully requesting num_irqs interrupts in the loop above, the loop
variable i is equal to num_irqs.

If the subsequent cpuhp_setup_state() call fails, the code executes goto
out_irq_free without decrementing i.

If num_irqs equals ARRAY_SIZE(econet_timer.irqs), this will access
econet_timer.irqs[num_irqs], which is past the end of the array. Even if
within bounds, it calls free_irq() on an unrequested index.

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

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

[Caleb James DeLisle]

On 16/05/2026 22:06, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [High] Missing bounds check on `num_blocks` leads to buffer overflow and memory corruption.
> - [High] Off-by-one error in `cpuhp_setup_state` failure path causes out-of-bounds array access and invalid IRQ free.
> - [High] Missing CPU hotplug teardown callback causes unbalanced `enable_irq` warnings and use-after-unmap crashes on initialization failure.
> --
>
> commit c9306a7aa40ae1fa4dc34819e52ba88137234f75
> 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 ed750e39cc4fa..dea6dbafa16ea 100644
>> --- a/drivers/clocksource/timer-econet-en751221.c
>> +++ b/drivers/clocksource/timer-econet-en751221.c
>> @@ -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);
> Will this cause unbalanced enable_irq() warnings when a CPU is brought back
> online?
Doesn't happen on this SoC.
>
> Since cpuhp_setup_state() provides NULL for the teardown callback, taking a
> CPU offline leaves the IRQ depth at 0.
>
> When brought back online, cevt_init_cpu() calls enable_irq() again, which
> could trigger the IRQ subsystem's unbalanced enable warning.
>
> Also, if cpuhp_setup_state() fails midway, the hotplug core attempts to roll
> back CPUs but does nothing due to the NULL teardown callback. The driver then
> proceeds to unmap MMIO memory and free IRQs.
Per last patch note, cevt_init_cpu only either errors OR sets up a 
timer, not both.
>
> Could this leave active clockevent devices registered that will crash the
> system when their set_next_event callback accesses the unmapped memory?
>
> [ ... ]
>
>> @@ -148,9 +177,23 @@ 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);
> [ ... ]
>
>> @@ -160,7 +203,7 @@ static int __init timer_init(struct device_node *np)
>>   
>>   	econet_timer.freq_hz = clk_get_rate(clk);
>>   
>> -	for (int i = 0; i < num_blocks; i++) {
>> +	for (i = 0; i < num_blocks; i++) {
>>   		econet_timer.membase[i] = of_iomap(np, i);
> Could this overflow the econet_timer.membase array?
>
> The econet_timer.membase array is statically sized to ECONET_NUM_BLOCKS.
>
> If a device tree specifies more address resources than ECONET_NUM_BLOCKS, this
> loop will write out of bounds and corrupt adjacent memory like the irqs array.
>
> Also, if num_blocks is less than required for the number of CPUs, will
> cevt_dev_init() dereference unmapped NULL memory?
Indeed, invalid DT will cause a crash. I can fix this easy enough.
>
> [ ... ]
>
>> @@ -216,11 +270,20 @@ static int __init timer_init(struct device_node *np)
>>   	return 0;
>>   
>>   out_irq_free:
>> -	free_percpu_irq(econet_timer.irq, &econet_timer_pcpu);
>> +	for (; i >= 0; i--) {
>> +		if (econet_timer.is_percpu) {
>> +			free_percpu_irq(econet_timer.irqs[i], &econet_timer_pcpu);
>> +		} else {
>> +			free_irq(econet_timer.irqs[i], NULL);
>> +		}
>> +	}
> Could this perform an out-of-bounds access or free an uninitialized IRQ?
>
> After successfully requesting num_irqs interrupts in the loop above, the loop
> variable i is equal to num_irqs.
>
> If the subsequent cpuhp_setup_state() call fails, the code executes goto
> out_irq_free without decrementing i.
>
> If num_irqs equals ARRAY_SIZE(econet_timer.irqs), this will access
> econet_timer.irqs[num_irqs], which is past the end of the array. Even if
> within bounds, it calls free_irq() on an unrequested index.
Oops yes that needs fixing. Will send v4 for this and sweep in a few of 
the others as well.
>