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

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

[Caleb James DeLisle]

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/

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   | 170 +++++++++++++-----
 2 files changed, 146 insertions(+), 47 deletions(-)


base-commit: ff1c0c5d07028a84837950b619d30da623f8ddb2
-- 
2.39.5

[PATCH v4 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

[PATCH v4 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

[PATCH v4 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

[PATCH v4 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 | 121 ++++++++++++++++----
 1 file changed, 97 insertions(+), 24 deletions(-)

diff --git a/drivers/clocksource/timer-econet-en751221.c b/drivers/clocksource/timer-econet-en751221.c
index ed750e39cc4f..40f3b370485f 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,19 +162,48 @@ 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;
+	}
 
 	clk = of_clk_get(np, 0);
 	if (IS_ERR(clk)) {
@@ -160,7 +213,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 +222,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);
@@ -216,11 +279,20 @@ 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++) {
+	for (i = 0; i < num_blocks; i++) {
 		if (econet_timer.membase[i])
 			iounmap(econet_timer.membase[i]);
 	}
@@ -229,3 +301,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 v4 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:
- [Medium] Schema validation failure due to mismatched `compatible` property definition
--

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

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

This commit updates the EN751221 timer binding to account for the
EN751627 SoC, which uses four individual interrupts instead of a
single percpu interrupt. The example is updated to demonstrate this
by dropping the fallback compatible string.

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

Since the fallback string "econet,en751221-timer" was removed from the
example here, does the top-level compatible property definition also need
to be updated?

If the schema still strictly requires "econet,en751627-timer" to be followed
by the "econet,en751221-timer" fallback, will this standalone string cause
a regression during make dt_binding_check validation? Updating the property
definition to permit the standalone string would prevent this.

>          reg = <0x1fbf0400 0x100>, <0x1fbe0000 0x100>;
>          interrupt-parent = <&intc>;
[ ... ]

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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Teardown path unmaps memory while clockevents remain registered (Use-After-Free)
- [High] Missing CPU hotplug teardown callback leaves per-CPU IRQs enabled on offline CPUs
- [High] Pre-existing out-of-bounds mapping access with sparse CPU possible mask
- [Medium] Ignored error code from clocksource_mmio_init causes false success reporting
--

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

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

This commit reorganizes the driver initialization process into two
distinct phases, providing a rollback path for errors that occur prior to
the point of no return. It adds logic to unmap memory and release interrupts
if the setup steps fail.

> 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
> @@ -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;
>  		}
>  	}

This is a pre-existing issue that is resolved later in this patch series
by commit clocksource/timer-econet-en751221: Support EN751627 without
percpu IRQ, but I am mentioning it here for completeness.

Can this loop cause an out-of-bounds access with a sparse CPU mask?

The loop iterates up to num_blocks, which is calculated based on
num_possible_cpus(). If the system has a sparse CPU topology where
CPUs 0 and 4 are possible, num_possible_cpus() is 2, so the loop only
maps membase[0].

However, cevt_init() iterates over for_each_possible_cpu(i). For CPU 4,
cevt_dev_init(4) accesses membase[2], which remains null, potentially
causing a kernel panic when dereferenced.

[ ... ]

> +	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 here, can the teardown path cause a
use-after-free?

Because the teardown callback in cpuhp_setup_state() is null, the CPU
hotplug core's attempt to roll back the state on previously initialized
CPUs acts as a no-op. This leaves the clock event devices and hardware
timers fully active on those CPUs.

The error path then proceeds to out_irq_free and subsequently unmaps the
memory regions via iounmap(). When the clockevents core later tries to
access the active timer on those CPUs, it will read from and write to
unmapped memory.

Additionally, while this is a pre-existing issue, does providing a null
teardown callback leave per-CPU interrupts enabled on offline CPUs?

When a CPU is dynamically offlined, the teardown callback should stop the
hardware timer and call disable_percpu_irq(). Without it, the timer remains
free-running and the interrupt unmasked for the offline CPU.

> +
> +	/* 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);

Does removing the return on error here cause the driver to report success
when initialization fails?

The failure code is captured in ret, but execution falls through and the
function unconditionally returns 0 at the end. This allows the driver to
print success messages and report a successful initialization even if the
clocksource registration failed.

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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] The error path for `cpuhp_setup_state()` leaves the clockevent device registered and the per-CPU interrupt enabled, leading to a leaked action and a Use-After-Free/Data Abort.
- [High] The patch attempts to fix a NULL pointer dereference race condition but leaves the `dev->event_handler` uninitialized if the clockevent device is rejected by the core, leading to a kernel panic.
- [High] The driver registers its CPU hotplug state with a `NULL` teardown callback, leaving the per-CPU interrupt enabled when a CPU is offlined.
--

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

This commit reorders the clockevent registration sequence to call
enable_percpu_irq() after clockevents_config_and_register(). This aims
to eliminate a race condition where the timer interrupt could fire before
the event handler is initialized.

> 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);
> +
>  	return 0;
>  }

If tick_check_new_device() rejects the timer (for instance, because another
higher-rated clockevent device is available), does the device remain in the
DETACHED state without the clockevents core initializing dev->event_handler?

Since the hardware timer is already explicitly enabled before registration
via ioread32(reg_ctl(cpu)) | ctl_bit_enabled(cpu), it will eventually wrap
and trigger an interrupt.

With enable_percpu_irq() unmasking the interrupt line, could
cevt_interrupt() execute and dereference a NULL dev->event_handler?

Should cd->event_handler be explicitly initialized to
clockevents_handle_noop prior to registration to fully resolve this race?

[ ... ]

> @@ -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);
>  

Looking further down in timer_init(), the driver registers its CPU hotplug
state with a NULL teardown callback:

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

Since the driver registers its CPU hotplug state with a NULL teardown
callback, what happens when a CPU is hotplugged out?

Because the clockevent device lacks a set_state_shutdown callback, the
clockevents core cannot turn off the hardware timer. This appears to leave
the per-CPU interrupt unmasked at the interrupt controller and the timer
hardware actively running.

When the CPU is later brought back online and unmasks its interrupts, could
it instantly take a pending timer interrupt before initialization is
complete?

Additionally, if cpuhp_setup_state() fails on a subsequent CPU after
succeeding on CPU 0, it attempts to roll back CPU 0. Because of the NULL
teardown callback, disable_percpu_irq() and clockevents_unbind_device()
are never executed for CPU 0.

The error path then jumps to out_irq_free:

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]);
    }

If the interrupt is still enabled on CPU 0, will __free_percpu_irq() safely
warn and return without freeing the action?

Since the hardware timer is still running on CPU 0 and the driver proceeds
to unmap the membase via iounmap(), could the timer fire and cause the
still-registered cevt_interrupt handler to access unmapped MMIO memory,
resulting in a Data Abort?

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

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

[Caleb James DeLisle]

On 17/05/2026 00:33, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
> - [High] Teardown path unmaps memory while clockevents remain registered (Use-After-Free)
> - [High] Missing CPU hotplug teardown callback leaves per-CPU IRQs enabled on offline CPUs
> - [High] Pre-existing out-of-bounds mapping access with sparse CPU possible mask
> - [Medium] Ignored error code from clocksource_mmio_init causes false success reporting
> --
>
> commit d0c6a451d26e5e0afd704c60615c1cfa9457c425
> Author: Caleb James DeLisle <cjd@cjdns.fr>
>
> clocksource/timer-econet-en751221: Init teardown on error if possible
>
> This commit reorganizes the driver initialization process into two
> distinct phases, providing a rollback path for errors that occur prior to
> the point of no return. It adds logic to unmap memory and release interrupts
> if the setup steps fail.
>
>> 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
>> @@ -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;
>>   		}
>>   	}
> This is a pre-existing issue that is resolved later in this patch series
> by commit clocksource/timer-econet-en751221: Support EN751627 without
> percpu IRQ, but I am mentioning it here for completeness.
>
> Can this loop cause an out-of-bounds access with a sparse CPU mask?
>
> The loop iterates up to num_blocks, which is calculated based on
> num_possible_cpus(). If the system has a sparse CPU topology where
> CPUs 0 and 4 are possible, num_possible_cpus() is 2, so the loop only
> maps membase[0].
>
> However, cevt_init() iterates over for_each_possible_cpu(i). For CPU 4,
> cevt_dev_init(4) accesses membase[2], which remains null, potentially
> causing a kernel panic when dereferenced.
>
> [ ... ]
>
>> +	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 here, can the teardown path cause a
> use-after-free?
>
> Because the teardown callback in cpuhp_setup_state() is null, the CPU
> hotplug core's attempt to roll back the state on previously initialized
> CPUs acts as a no-op. This leaves the clock event devices and hardware
> timers fully active on those CPUs.
>
> The error path then proceeds to out_irq_free and subsequently unmaps the
> memory regions via iounmap(). When the clockevents core later tries to
> access the active timer on those CPUs, it will read from and write to
> unmapped memory.
>
> Additionally, while this is a pre-existing issue, does providing a null
> teardown callback leave per-CPU interrupts enabled on offline CPUs?
>
> When a CPU is dynamically offlined, the teardown callback should stop the
> hardware timer and call disable_percpu_irq(). Without it, the timer remains
> free-running and the interrupt unmasked for the offline CPU.
As said in previous version, I don't think this is an issue, but I need 
to fix that yaml so I'll move it below the point of no return to make 
the bot be quiet.
>
>> +
>> +	/* 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);
> Does removing the return on error here cause the driver to report success
> when initialization fails?
>
> The failure code is captured in ret, but execution falls through and the
> function unconditionally returns 0 at the end. This allows the driver to
> print success messages and report a successful initialization even if the
> clocksource registration failed.
It's below the point of no return so the idea is to complain but muddle 
through no matter what.

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

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 3052 bytes --]

On Sat, May 16, 2026 at 09:57:17PM +0000, Caleb James DeLisle wrote:
> 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";

This is not going to pass dt_binding_check.
Please remove my ack until this is fixed.

>          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
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Caleb James DeLisle]

On 17/05/2026 01:12, Conor Dooley wrote:
> On Sat, May 16, 2026 at 09:57:17PM +0000, Caleb James DeLisle wrote:
>> 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";
> This is not going to pass dt_binding_check.
> Please remove my ack until this is fixed.
Yeah I saw the bot message and I'll be re-sending in a minute.
>
>>           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 v4 1/4] dt-bindings: timer: econet: Update EN751627 for multi-IRQ

[Rob Herring (Arm)]

On Sat, 16 May 2026 21:57:17 +0000, Caleb James DeLisle wrote:
> 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(-)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/timer/econet,en751221-timer.example.dtb: timer@1fbf0400 (econet,en751627-timer): compatible: 'oneOf' conditional failed, one must be fixed:
	['econet,en751627-timer'] is too short
	'econet,en751221-timer' was expected
	from schema $id: http://devicetree.org/schemas/timer/econet,en751221-timer.yaml

doc reference errors (make refcheckdocs):

See https://patchwork.kernel.org/project/devicetree/patch/20260516215720.4160831-2-cjd@cjdns.fr

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] Initialization failure in cpuhp_setup_state() leaves dangling clockevent devices pointing to unmapped memory, causing a Use-After-Free/Unmap panic.
- [High] Missing CPU hotplug teardown callback leads to unhandled interrupt storms and unbalanced IRQ enable warnings.
- [High] The driver unconditionally rejects valid device trees on hardware with more cores than CONFIG_NR_CPUS, preventing system boot.
- [High] Missing validation for `num_blocks == 0` leads to a NULL pointer dereference and kernel panic.
--

commit 60ae37c5c796f8a8f7a5be1259491a4d42bb0aa2
Author: Caleb James DeLisle <cjd@cjdns.fr>
Date:   Sat May 16 21:57:20 2026 +0000

    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.
It updates the resource parsing to handle multiple IRQs and registers them
appropriately based on the device tree compatible string.

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

If cevt_init_cpu() succeeds on CPU 0 but fails here on a secondary CPU, does
this cause a use-after-free regression?

When cpuhp_setup_state() aborts and attempts to roll back, the rollback is a
no-op because the teardown callback was set to NULL during registration. This
leaves CPU 0's clockevent device fully registered in the timer core.

The driver then proceeds to its error handling path out_irq_free and executes
iounmap() on all timer MMIO regions. When the timer core inevitably tries to
program the next event, it invokes cevt_set_next_event(), which accesses the
now-unmapped memory regions.

> +
>  	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);

Does omitting a teardown callback for CPU hotplug cause an unhandled interrupt
storm here?

The driver registers a CPU hotplug startup callback via cpuhp_setup_state()
with NULL for the teardown callback, meaning the IRQ is never disabled when a
CPU is offlined.

If the CPU goes offline, the kernel's hotplug core might migrate the unmasked
timer IRQ to a surviving CPU. Since there is no set_state_shutdown callback,
the offlined CPU's timer hardware continues running.

When the timer fires, the interrupt is delivered to the newly affined online
CPU. The cevt_interrupt handler uses this_cpu_ptr(), checking the online CPU's
registers instead of the offlined CPU's timer. Finding no pending event, it
returns IRQ_NONE without clearing the interrupt, resulting in an interrupt
storm.

Additionally, if the CPU is brought back online, wouldn't cevt_init_cpu()
execute enable_irq() a second time while desc->depth is already 0, triggering
an unbalanced enable warning?

>  
>  	return 0;
>  }
[ ... ]
> @@ -138,19 +162,48 @@ static void __init cevt_init(struct device_node *np)
[ ... ]
>  static int __init timer_init(struct device_node *np)
>  {
> -	int num_blocks = DIV_ROUND_UP(num_possible_cpus(), 2);
> +	int num_blocks = of_address_count(np);
> +	int num_irqs = of_irq_count(np);

What happens if num_blocks is 0 due to a malformed device tree?

If num_blocks is 0, the mapping loop is skipped, leaving
econet_timer.membase[0] uninitialized. Later, clocksource_mmio_init() is
called with reg_count(0), which computes NULL + 0x08. The timer subsystem
dereferences this invalid pointer, resulting in a direct kernel page fault.

>  	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;
> +	}

Does this unconditionally reject valid device trees on hardware with more
cores than CONFIG_NR_CPUS?

The array sizes are constrained by CONFIG_NR_CPUS. If a kernel compiled with
CONFIG_NR_CPUS=2 is booted on a 4-core SoC, the DT will accurately describe
4 IRQs, but the driver will reject it, return -EINVAL, and abort timer
initialization.

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