[PATCH v6 0/5] clocksource/timer-econet-en751221: Support irq number per timer

[PATCH v6 0/5] clocksource/timer-econet-en751221: Support irq number per timer

[Caleb James DeLisle]

This timer appears on both the EcoNet EN751221 (34Kc) and on the
EN751627 / EN7528 (1004Kc) SoCs. On the 34Kc the interrupt controller
uses a percpu interrupt while on the others it uses an individual
IRQ number for each CPU timer. Add support for EN751627 and EN7528.
Add support for IRQ number for each CPU timer under the
econet,en751627-timer compatible.

Changes since v5:
* Integrate patch fix refcount leak from:
Link: https://lore.kernel.org/linux-mips/20260526184105.18962-3-grandmaster@al2klimov.de/
* NULL econet_timer.membase after unmapping
* NULL check dev->event_handler in case clockevents_config_and_register
  failed to fully register.
* v5: https://lore.kernel.org/linux-mips/20260516233943.49502-1-cjd@cjdns.fr/

Changes from v4:
* Fixed incorrect compatible in DT schema
* Bracing in out_irq_free if statement
* I did not move cpuhp_setup_state past the point of no return
* v4: https://lore.kernel.org/linux-mips/20260516215720.4160831-1-cjd@cjdns.fr

Changes from v3:
* off-by-one in out_irq_free
* Gracefully fail if too many address resources are provided in the DT
* Allow fewer actual timers than num_possible_cpus, as long as they
never start
* v3: https://lore.kernel.org/linux-mips/20260516182648.3987792-1-cjd@cjdns.fr

v3 note:

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/

Alexander A. Klimov (1):
  clocksource/timer-econet-en751221: fix refcount leak

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 |  31 +--
 drivers/clocksource/timer-econet-en751221.c   | 181 ++++++++++++++----
 2 files changed, 159 insertions(+), 53 deletions(-)


base-commit: c8d32a0389fb97873285327ef4543a1431e54733
-- 
2.39.5

[PATCH v6 1/5] clocksource/timer-econet-en751221: fix refcount leak

[Caleb James DeLisle]

From: "Alexander A. Klimov" <grandmaster@al2klimov.de>

Every value returned from of_clk_get() is supposed to be cleaned up
via clk_put() once not needed anymore.

Fixes: 3b4c33ac87d0 ("clocksource/drivers: Add EcoNet Timer HPT driver")
Signed-off-by: Alexander A. Klimov <grandmaster@al2klimov.de>
Signed-off-by: Caleb James DeLisle <cjd@cjdns.fr>
---
 drivers/clocksource/timer-econet-en751221.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/clocksource/timer-econet-en751221.c b/drivers/clocksource/timer-econet-en751221.c
index 4008076b1a21..1859335345b5 100644
--- a/drivers/clocksource/timer-econet-en751221.c
+++ b/drivers/clocksource/timer-econet-en751221.c
@@ -181,6 +181,7 @@ static int __init timer_init(struct device_node *np)
 	}
 
 	econet_timer.freq_hz = clk_get_rate(clk);
+	clk_put(clk);
 
 	for (int i = 0; i < num_blocks; i++) {
 		econet_timer.membase[i] = of_iomap(np, i);
-- 
2.39.5

[PATCH v6 2/5] 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 | 31 ++++++++++++-------
 1 file changed, 20 insertions(+), 11 deletions(-)

diff --git a/Documentation/devicetree/bindings/timer/econet,en751221-timer.yaml b/Documentation/devicetree/bindings/timer/econet,en751221-timer.yaml
index c1e7c2b6afde..ac25785ecfdf 100644
--- a/Documentation/devicetree/bindings/timer/econet,en751221-timer.yaml
+++ b/Documentation/devicetree/bindings/timer/econet,en751221-timer.yaml
@@ -12,24 +12,23 @@ 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:
-    oneOf:
-      - const: econet,en751221-timer
-      - items:
-          - const: econet,en751627-timer
-          - const: econet,en751221-timer
+    enum:
+      - econet,en751221-timer
+      - econet,en751627-timer
 
   reg:
     minItems: 1
     maxItems: 2
 
   interrupts:
-    maxItems: 1
-    description: A percpu-devid timer interrupt shared across CPUs.
+    minItems: 1
+    maxItems: 4
 
   clocks:
     maxItems: 1
@@ -52,21 +51,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 v6 2/5] 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:
- [High] The commit modifies the `compatible` property to remove the fallback and strictly require 4 interrupts for the `econet,en751627-timer`, breaking backwards compatibility with existing device trees.
--

commit 4d76d221e102e3fd0badbb607f955a94dc92884a
Author: Caleb James DeLisle <cjd@cjdns.fr>

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

This commit updates the device tree binding for the EN751627 timer. It 
modifies the binding to require an individual IRQ number per timer instead 
of a single shared percpu IRQ.

> diff --git a/Documentation/devicetree/bindings/timer/econet,en751221-timer.yaml b/Documentation/devicetree/bindings/timer/econet,en751221-timer.yaml
> --- a/Documentation/devicetree/bindings/timer/econet,en751221-timer.yaml
> +++ b/Documentation/devicetree/bindings/timer/econet,en751221-timer.yaml
[ ... ]
>  properties:
>    compatible:
> -    oneOf:
> -      - const: econet,en751221-timer
> -      - items:
> -          - const: econet,en751627-timer
> -          - const: econet,en751221-timer
> +    enum:
> +      - econet,en751221-timer
> +      - econet,en751627-timer

[ ... ]

> @@ -52,21 +51,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

[Severity: High]
Does this change break backwards compatibility with existing device trees?

The previous schema allowed compatible = "econet,en751627-timer",
"econet,en751221-timer" with a single interrupt. 

Modifying the compatible property to an enum without the fallback and enforcing
minItems: 4 for interrupts on the en751627-timer means any existing device tree
using the old schema will now fail validation.

Can this cause boot regressions on previously functioning deployed systems if
an updated driver expects 4 interrupts and attempts to index into an
uninitialized IRQ array?

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

Re: [PATCH v6 2/5] dt-bindings: timer: econet: Update EN751627 for multi-IRQ

[Caleb James DeLisle]

On 28/05/2026 16:45, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] The commit modifies the `compatible` property to remove the fallback and strictly require 4 interrupts for the `econet,en751627-timer`, breaking backwards compatibility with existing device trees.
> --

When I first contributed this driver, I added an en751627 compatible 
because I knew it to be present on the hardware but en751627 support was 
not developed. Now we know that for this to work it requires 4 IRQs.

This was a mistake on my part the first time around.

Thanks,

Caleb

>
> commit 4d76d221e102e3fd0badbb607f955a94dc92884a
> Author: Caleb James DeLisle <cjd@cjdns.fr>
>
> dt-bindings: timer: econet: Update EN751627 for multi-IRQ
>
> This commit updates the device tree binding for the EN751627 timer. It
> modifies the binding to require an individual IRQ number per timer instead
> of a single shared percpu IRQ.
>
>> diff --git a/Documentation/devicetree/bindings/timer/econet,en751221-timer.yaml b/Documentation/devicetree/bindings/timer/econet,en751221-timer.yaml
>> --- a/Documentation/devicetree/bindings/timer/econet,en751221-timer.yaml
>> +++ b/Documentation/devicetree/bindings/timer/econet,en751221-timer.yaml
> [ ... ]
>>   properties:
>>     compatible:
>> -    oneOf:
>> -      - const: econet,en751221-timer
>> -      - items:
>> -          - const: econet,en751627-timer
>> -          - const: econet,en751221-timer
>> +    enum:
>> +      - econet,en751221-timer
>> +      - econet,en751627-timer
> [ ... ]
>
>> @@ -52,21 +51,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
> [Severity: High]
> Does this change break backwards compatibility with existing device trees?
>
> The previous schema allowed compatible = "econet,en751627-timer",
> "econet,en751221-timer" with a single interrupt.
>
> Modifying the compatible property to an enum without the fallback and enforcing
> minItems: 4 for interrupts on the en751627-timer means any existing device tree
> using the old schema will now fail validation.
>
> Can this cause boot regressions on previously functioning deployed systems if
> an updated driver expects 4 interrupts and attempts to index into an
> uninitialized IRQ array?
>

[PATCH v6 3/5] 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 | 84 ++++++++++++---------
 1 file changed, 50 insertions(+), 34 deletions(-)

diff --git a/drivers/clocksource/timer-econet-en751221.c b/drivers/clocksource/timer-econet-en751221.c
index 1859335345b5..714702b9ef12 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)
@@ -187,22 +166,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);
@@ -212,6 +214,20 @@ 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]);
+			econet_timer.membase[i] = NULL;
+		}
+	}
+
+	return ret;
 }
 
 TIMER_OF_DECLARE(econet_timer_hpt, "econet,en751221-timer", timer_init);
-- 
2.39.5

Re: [PATCH v6 3/5] clocksource/timer-econet-en751221: Init teardown on error if possible

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] The newly added error path for `cpuhp_setup_state()` failure creates a fatal use-after-unmap vulnerability by freeing IRQs and unmapping I/O memory without unregistering the active clockevent devices.
--

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

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

This commit reorganizes the initialization process of the econet timer by
separating it into two phases. It adds teardown logic to safely roll back
resource allocations like memory mappings and IRQ requests if an error occurs
before the point of no return.

> diff --git a/drivers/clocksource/timer-econet-en751221.c b/drivers/clocksource/timer-econet-en751221.c
> --- a/drivers/clocksource/timer-econet-en751221.c
> +++ b/drivers/clocksource/timer-econet-en751221.c
[ ... ]
> @@ -187,22 +166,45 @@ static int __init timer_init(struct device_node *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;
> +	}

[Severity: Critical]
When cpuhp_setup_state() executes the startup callback across online CPUs,
it registers the clockevent devices. If this fails on a subsequent CPU,
cpuhp_setup_state() aborts. Because a NULL teardown callback was provided,
the CPU hotplug core skips rollback and leaves the successfully registered
clockevent devices active on the earlier CPUs.

The error path here then frees the IRQs and unmaps the I/O memory. If the
timekeeping core attempts to handle a timer interrupt or program the next
event on the still-registered clockevent devices, will it dereference the
unmapped memory?

Should the active clockevent devices be unregistered, or a teardown callback
provided to cpuhp_setup_state(), before freeing these resources?

> +
> +	/* Point of no return, do not attempt to tear down after this. */
[ ... ]
> +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]);
> +			econet_timer.membase[i] = NULL;
> +		}
> +	}
> +
> +	return ret;
>  }

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

Re: [PATCH v6 3/5] clocksource/timer-econet-en751221: Init teardown on error if possible

[Caleb James DeLisle]

On 28/05/2026 17:23, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Critical] The newly added error path for `cpuhp_setup_state()` failure creates a fatal use-after-unmap vulnerability by freeing IRQs and unmapping I/O memory without unregistering the active clockevent devices.

I'm considering this not-a-bug because at the time the timer comes 
online, there is only one CPU, and if cevt_init_cpu errors out it 
requires no tear-down. I can silence this warning by not erroring on 
failed cpuhp_setup_state call, but I didn't consider "make Sashiko 
happy" a good enough reason to do it.

Thanks,

Caleb

> --
>
> commit e1e0b36b8164978eb079f38d9547d743cebd0a97
> Author: Caleb James DeLisle <cjd@cjdns.fr>
>
> clocksource/timer-econet-en751221: Init teardown on error if possible
>
> This commit reorganizes the initialization process of the econet timer by
> separating it into two phases. It adds teardown logic to safely roll back
> resource allocations like memory mappings and IRQ requests if an error occurs
> before the point of no return.
>
>> diff --git a/drivers/clocksource/timer-econet-en751221.c b/drivers/clocksource/timer-econet-en751221.c
>> --- a/drivers/clocksource/timer-econet-en751221.c
>> +++ b/drivers/clocksource/timer-econet-en751221.c
> [ ... ]
>> @@ -187,22 +166,45 @@ static int __init timer_init(struct device_node *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;
>> +	}
> [Severity: Critical]
> When cpuhp_setup_state() executes the startup callback across online CPUs,
> it registers the clockevent devices. If this fails on a subsequent CPU,
> cpuhp_setup_state() aborts. Because a NULL teardown callback was provided,
> the CPU hotplug core skips rollback and leaves the successfully registered
> clockevent devices active on the earlier CPUs.
>
> The error path here then frees the IRQs and unmaps the I/O memory. If the
> timekeeping core attempts to handle a timer interrupt or program the next
> event on the still-registered clockevent devices, will it dereference the
> unmapped memory?
>
> Should the active clockevent devices be unregistered, or a teardown callback
> provided to cpuhp_setup_state(), before freeing these resources?
>
>> +
>> +	/* Point of no return, do not attempt to tear down after this. */
> [ ... ]
>> +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]);
>> +			econet_timer.membase[i] = NULL;
>> +		}
>> +	}
>> +
>> +	return ret;
>>   }

[PATCH v6 4/5] 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. Additionally, NULL check dev->event_handler in
interrupt callback just in case clockevents_config_and_register failed.

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

diff --git a/drivers/clocksource/timer-econet-en751221.c b/drivers/clocksource/timer-econet-en751221.c
index 714702b9ef12..f2c4c1ee0a56 100644
--- a/drivers/clocksource/timer-econet-en751221.c
+++ b/drivers/clocksource/timer-econet-en751221.c
@@ -75,7 +75,10 @@ static irqreturn_t cevt_interrupt(int irq, void *dev_id)
 		return IRQ_NONE;
 
 	iowrite32(ioread32(reg_count(cpu)), reg_compare(cpu));
-	dev->event_handler(dev);
+
+	if (dev->event_handler)
+		dev->event_handler(dev);
+
 	return IRQ_HANDLED;
 }
 
@@ -104,12 +107,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;
 }
 
@@ -178,6 +180,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 v6 4/5] clocksource/timer-econet-en751221: Disable IRQ until cevt registered

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Incorrect assumption that clockevents_config_and_register() can fail.
- [Low] Unnecessary manual setting of IRQ_NOAUTOEN flag.
--

commit f75e71c2c11de4c792309a75e268796b0c4679bf
Author: Caleb James DeLisle <cjd@cjdns.fr>
clocksource/timer-econet-en751221: Disable IRQ until cevt registered

This commit addresses a race condition where the timer interrupt may
trigger before clockevents_config_and_register() sets up the event
handler. It resolves this by enabling the per-CPU IRQ only after the
clockevent device is fully registered and adds a NULL check for the
event handler.

> 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. Additionally, NULL check dev->event_handler in
> interrupt callback just in case clockevents_config_and_register failed.

[Severity: Medium]
Is the reasoning about clockevents_config_and_register() failing accurate
here? 

Looking at the implementation of clockevents_config_and_register(), it
returns void and does not have a failure path.

> diff --git a/drivers/clocksource/timer-econet-en751221.c b/drivers/clocksource/timer-econet-en751221.c
> --- a/drivers/clocksource/timer-econet-en751221.c
> +++ b/drivers/clocksource/timer-econet-en751221.c
> @@ -75,7 +75,10 @@ static irqreturn_t cevt_interrupt(int irq, void *dev_id)
>  		return IRQ_NONE;
>  
>  	iowrite32(ioread32(reg_count(cpu)), reg_compare(cpu));
> -	dev->event_handler(dev);
> +
> +	if (dev->event_handler)
> +		dev->event_handler(dev);
> +
>  	return IRQ_HANDLED;
>  }

[Severity: Medium]
Is this NULL check necessary? 

Because clockevents_register_device() unconditionally initializes the event
handler to a valid function (such as clockevents_handle_noop),
dev->event_handler should never be NULL once registration is complete.

[ ... ]

> @@ -178,6 +180,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);

[Severity: Low]
Is this manual status flag update needed? 

When an IRQ is configured as a per-CPU devid interrupt by the irqchip driver
via irq_set_percpu_devid(), the IRQ_NOAUTOEN flag is inherently applied
alongside IRQ_PER_CPU and IRQ_NOTHREAD. Setting it manually here appears
redundant.

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

[PATCH v6 5/5] 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 | 122 ++++++++++++++++----
 1 file changed, 99 insertions(+), 23 deletions(-)

diff --git a/drivers/clocksource/timer-econet-en751221.c b/drivers/clocksource/timer-econet-en751221.c
index f2c4c1ee0a56..040386ded132 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;
 
@@ -102,6 +104,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);
@@ -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);
 
 	return 0;
 }
@@ -141,19 +165,52 @@ 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;
 
-		cevt_dev_init(i);
+		/*
+		 * Tolerate CPUs that could exist but don't.
+		 * Fail in cevt_init_cpu when they try to start.
+		 */
+		if (reg_ctl(i))
+			cevt_dev_init(i);
 	}
 }
 
 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;
+	}
+	if (num_blocks == 0) {
+		pr_err("%pOFn: At least one reg block must be provided\n", np);
+		return -EINVAL;
+	}
 
 	clk = of_clk_get(np, 0);
 	if (IS_ERR(clk)) {
@@ -164,7 +221,7 @@ static int __init timer_init(struct device_node *np)
 	econet_timer.freq_hz = clk_get_rate(clk);
 	clk_put(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);
@@ -173,22 +230,32 @@ 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));
+			goto out_irq_free;
+		}
 	}
 
 	cevt_init(np);
@@ -220,9 +287,17 @@ static int __init timer_init(struct device_node *np)
 	return 0;
 
 out_irq_free:
-	free_percpu_irq(econet_timer.irq, &econet_timer_pcpu);
+	while (--i >= 0) {
+		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++) {
 		if (econet_timer.membase[i]) {
@@ -235,3 +310,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 v6 5/5] clocksource/timer-econet-en751221: Support EN751627 without percpu IRQ

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The interrupt handler incorrectly relies on `this_cpu_ptr` for standard IRQs, causing an interrupt storm if the IRQ executes on a different CPU.
- [Medium] Missing CPU hotplug teardown callback causes an unbalanced `enable_irq()` kernel warning when a CPU is offlined and then onlined again.
--

commit 0de42286a034bc944e765e97434f913831bc2960
Author: Caleb James DeLisle <cjd@cjdns.fr>
Subject: clocksource/timer-econet-en751221: Support EN751627 without percpu IRQ

This commit adds support for the EN751627 timer, which uses a different
interrupt number for each CPU timer instead of 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: Medium]
Will this cause an unbalanced enable_irq() warning if a CPU is offlined
and onlined again?

The driver registers a dynamic CPU hotplug state via cpuhp_setup_state()
with cevt_init_cpu() as the starting callback, but passes NULL for the
teardown callback.

Because there is no teardown callback to call disable_irq() when a CPU is
offlined, the IRQ descriptor's enable depth remains 0. When the CPU is
brought back online, cevt_init_cpu() will execute again and call
enable_irq() a second time.

While this missing teardown existed before this patch, it was hidden
because enable_percpu_irq() does not track descriptor depth. The warning
appears to be newly exposed by the introduction of enable_irq().

[ ... ]

> @@ -173,22 +230,32 @@ static int __init timer_init(struct device_node *np)

[ ... ]

> +	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]
Could passing NULL for dev_id lead to an interrupt storm if the IRQ
executes on a different CPU?

For standard (non-percpu) IRQs, cevt_interrupt() currently calculates the
timer device using this_cpu_ptr(). If the interrupt fires on a different
CPU (e.g., due to CPU hotplug migrating the IRQ, or a spurious interrupt),
this_cpu_ptr() will return the timer for the currently executing CPU, not
the CPU for which the interrupt was intended.

The handler would then check the wrong CPU's status, find it not pending,
and return IRQ_NONE. The actual pending interrupt would be left
unacknowledged.

Would it be better to pass the specific CPU's timer pointer as dev_id here
and update cevt_interrupt() to use it instead of this_cpu_ptr()?

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

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

[Caleb James DeLisle]

On 28/05/2026 18:41, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] The interrupt handler incorrectly relies on `this_cpu_ptr` for standard IRQs, causing an interrupt storm if the IRQ executes on a different CPU.
This should not be a real issue because cevt_init_cpu() calls 
irq_force_affinity() and on error the IRQ is never enabled. It could be 
nice to create a dev_id which is used for both percpu and 
IRQ-number-per-timer modes, but that's a more invasive changeset. Using 
the CPU number is how it worked previously.
> - [Medium] Missing CPU hotplug teardown callback causes an unbalanced `enable_irq()` kernel warning when a CPU is offlined and then onlined again.

These EcoNet devices don't support CPU hotplug and I don't see a 
realistic chance that they ever will. Vendor code does not support it so 
there's no clear path to how it would be done. So I think this is a 
theoretical issue and cluttering the code for it would be net-negative.


Thanks,

Caleb


> --
>
> commit 0de42286a034bc944e765e97434f913831bc2960
> Author: Caleb James DeLisle <cjd@cjdns.fr>
> Subject: clocksource/timer-econet-en751221: Support EN751627 without percpu IRQ
>
> This commit adds support for the EN751627 timer, which uses a different
> interrupt number for each CPU timer instead of 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: Medium]
> Will this cause an unbalanced enable_irq() warning if a CPU is offlined
> and onlined again?
>
> The driver registers a dynamic CPU hotplug state via cpuhp_setup_state()
> with cevt_init_cpu() as the starting callback, but passes NULL for the
> teardown callback.
>
> Because there is no teardown callback to call disable_irq() when a CPU is
> offlined, the IRQ descriptor's enable depth remains 0. When the CPU is
> brought back online, cevt_init_cpu() will execute again and call
> enable_irq() a second time.
>
> While this missing teardown existed before this patch, it was hidden
> because enable_percpu_irq() does not track descriptor depth. The warning
> appears to be newly exposed by the introduction of enable_irq().
>
> [ ... ]
>
>> @@ -173,22 +230,32 @@ static int __init timer_init(struct device_node *np)
> [ ... ]
>
>> +	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]
> Could passing NULL for dev_id lead to an interrupt storm if the IRQ
> executes on a different CPU?
>
> For standard (non-percpu) IRQs, cevt_interrupt() currently calculates the
> timer device using this_cpu_ptr(). If the interrupt fires on a different
> CPU (e.g., due to CPU hotplug migrating the IRQ, or a spurious interrupt),
> this_cpu_ptr() will return the timer for the currently executing CPU, not
> the CPU for which the interrupt was intended.
>
> The handler would then check the wrong CPU's status, find it not pending,
> and return IRQ_NONE. The actual pending interrupt would be left
> unacknowledged.
>
> Would it be better to pass the specific CPU's timer pointer as dev_id here
> and update cevt_interrupt() to use it instead of this_cpu_ptr()?
>