[PATCH v3 0/2] irqchip/sifive-plic: Fix wrong nr_irqs handling

[PATCH v3 0/2] irqchip/sifive-plic: Fix wrong nr_irqs handling

[Yangyu Chen]

This patch series fixes long standing bugs in sifive-plic driver regarding
the handling of nr_irqs. Some code assumes the first irq source is 0 while
some assumes it is 1. Since the first irq source is actually 1, this causes
various issues including memory corruption when the number of irqs is
multiple of 32. Also, some code assumes nr_irqs is the maximum irq source
ID while some assumes it is the total number of irq sources including the
reserved source 0. This patch series standardizes the handling of nr_irqs
to be the maximum irq source ID, and the first irq source is 1.

This bug can be reproduced by modifying the PLIC node in DT to have ndev as
exactly multiple of 32, e.g., 32, 64, etc., then triggering some interrupts
and checking dmesg for memory corruption:

plic: plic@3c000000 {
	compatible = "riscv,plic0";
	reg = <0x0 0x3c000000 0x0 0x4000000>;
	#interrupt-cells = <1>;
	interrupt-controller;
	interrupts-extended = <&cpu0_intc 11>, <&cpu0_intc 9>;
	riscv,max-priority = <7>;
	riscv,ndev = <64>;
};

Here is an example dmesg log when ndev is 64:
[    0.077196] Unable to handle kernel paging request at virtual address ffffaf8000000000
[    0.077205] Current swapper/0 pgtable: 4K pagesize, 48-bit VAs, pgdp=0x0000000081c2d000
[    0.077215] [ffffaf8000000000] pgd=000000009ffffc01, p4d=000000009ffffc01, pud=000000009ffff801, pmd=000000009ffff401, pte=0000000000000000
[    0.077240] Oops [#1]
[    0.077246] Modules linked in:
[    0.077254] CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.19.0-rc6 #36 NONE 
[    0.077266] Hardware name: XiangShan (DT)
[    0.077273] epc : __kmalloc_node_track_caller_noprof+0x1a0/0x524
[    0.077284]  ra : kstrdup+0x32/0x60
[    0.077293] epc : ffffffff80253c70 ra : ffffffff801fa70e sp : ffff8f800000b700
[    0.077304]  gp : ffffffff81a1b580 tp : ffffaf8080158000 t0 : 0000000000000264
[    0.077313]  t1 : 0000000000000003 t2 : 0000000000000000 s0 : ffff8f800000b750
[    0.077323]  s1 : 0000000000000002 a0 : ffffaf8000000000 a1 : 0000000000000cc0
[    0.077332]  a2 : ffff8d800200bfc0 a3 : ffffffff81a5c5e0 a4 : ffffaf8000000000
[    0.077342]  a5 : 0000000000000003 a6 : ffffffffffffffff a7 : ffffaf8080001400
[    0.077352]  s2 : ffffaf80802ff178 s3 : ffffffff810107f0 s4 : 0000000000000000
[    0.077362]  s5 : 0000000000000000 s6 : ffff8f800000b9c0 s7 : ffffaf8080823200
[    0.077372]  s8 : ffffffff81a20580 s9 : ffffaf808012c990 s10: ffffffffffffffff
[    0.077382]  s11: 0000000000000000 t3 : 0000000000000cc0 t4 : ffffffff801fa764
[    0.077391]  t5 : 0000000000000000 t6 : 0000000000000263
[    0.077399] status: 0000000200000120 badaddr: ffffaf8000000000 cause: 000000000000000d
[    0.077409] [<ffffffff80253c70>] __kmalloc_node_track_caller_noprof+0x1a0/0x524
[    0.077422] [<ffffffff801fa70e>] kstrdup+0x32/0x60
[    0.077433] [<ffffffff801fa764>] kstrdup_const+0x28/0x34
[    0.077444] [<ffffffff80318438>] __kernfs_new_node+0x3c/0x274
[    0.077457] [<ffffffff80318a90>] kernfs_new_node+0x44/0x6c
[    0.077470] [<ffffffff80318f40>] kernfs_create_dir_ns+0x20/0x7c
[    0.077483] [<ffffffff8031b8f8>] sysfs_create_dir_ns+0x60/0xcc
[    0.077497] [<ffffffff80b41bea>] kobject_add_internal+0xae/0x2d8
[    0.077509] [<ffffffff80b422d6>] kobject_add+0x52/0xb8
[    0.077520] [<ffffffff80b6401c>] __irq_alloc_descs+0x190/0x328
[    0.077534] [<ffffffff800976de>] irq_domain_alloc_descs.part.0+0x46/0x78
[    0.077549] [<ffffffff8009827a>] irq_create_mapping_affinity+0x72/0xcc
[    0.077561] [<ffffffff805d27d2>] plic_probe+0x2e2/0x6c8
[    0.077573] [<ffffffff805d2bc8>] plic_platform_probe+0x10/0x18

Changes since v2:
- Clarify the riscv,ndev meaning in the devicetree binding
  documentation for PLIC.
- Fix the entire driver code to have all nr_irqs handling consistent
  with the standard definition.

v2: https://lore.kernel.org/lkml/tencent_A697393AE256C4288768342AF245099A690A@qq.com/

Changes since v1:
- Add more Fixes tags for earlier commits that are also affected by this
  bug.
- Add more explanation about the bug's history.

v1: https://lore.kernel.org/lkml/tencent_6E9A1A3DF88005E3B4A11C4D7039637E4309@qq.com/

Yangyu Chen (2):
  irqchip/sifive-plic: Fix wrong nr_irqs handling
  dt-binding: riscv: Clarify the riscv,ndev meaning in PLIC

 .../interrupt-controller/sifive,plic-1.0.0.yaml  |  2 ++
 drivers/irqchip/irq-sifive-plic.c                | 16 ++++++++--------
 2 files changed, 10 insertions(+), 8 deletions(-)

-- 
2.51.0

[PATCH v3 1/2] irqchip/sifive-plic: Fix wrong nr_irqs handling

[Yangyu Chen]

Since the first irq source is 1 instead of 0, when the number of
irqs is multiple of 32, the last irq group will be ignored during
allocation, saving, and restoring. This lead to memory corruption
when accessing enable_save beyond allocated memory after commit
14ff9e54dd14 ("irqchip/sifive-plic: Cache the interrupt enable state")
which will access enable_save for all sources during plic_probe.
Thus, we should allocate irq_groups based on (nr_irqs + 1) instead of
nr_irqs to avoid this issue. This commit also fixes related loops
to have all consumer of nr_irqs consistent.

This is an long standing bug since Linux v5.6 but since the last irq
source is rarely used, it may not be triggered in practice until commit
14ff9e54dd14 ("irqchip/sifive-plic: Cache the interrupt enable state").

Fixes: 466008f98435 ("irqchip/sifive-plic: Support irq domain hierarchy")
Fixes: e80f0b6a2cf3 ("irqchip/irq-sifive-plic: Add syscore callbacks for hibernation")
Fixes: 4d936f10ff80 ("irqchip/sifive-plic: Probe plic driver early for Allwinner D1 platform")
Fixes: f75e07bf5226 ("irqchip/sifive-plic: Avoid interrupt ID 0 handling during suspend/resume")
Fixes: 14ff9e54dd14 ("irqchip/sifive-plic: Cache the interrupt enable state")
Fixes: 539d147ef69c ("irqchip/sifive-plic: Add support for UltraRISC DP1000 PLIC")
Fixes: a045359e7245 ("irqchip/sifive-plic: Fix call to __plic_toggle() in M-Mode code path")
Signed-off-by: Yangyu Chen <cyy@cyyself.name>
---
 drivers/irqchip/irq-sifive-plic.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
index 210a57959637..4658cad0d502 100644
--- a/drivers/irqchip/irq-sifive-plic.c
+++ b/drivers/irqchip/irq-sifive-plic.c
@@ -262,7 +262,7 @@ static int plic_irq_suspend(void *data)
 	priv = per_cpu_ptr(&plic_handlers, smp_processor_id())->priv;
 
 	/* irq ID 0 is reserved */
-	for (unsigned int i = 1; i < priv->nr_irqs; i++) {
+	for (unsigned int i = 1; i <= priv->nr_irqs; i++) {
 		__assign_bit(i, priv->prio_save,
 			     readl(priv->regs + PRIORITY_BASE + i * PRIORITY_PER_ID));
 	}
@@ -280,7 +280,7 @@ static void plic_irq_resume(void *data)
 	priv = per_cpu_ptr(&plic_handlers, smp_processor_id())->priv;
 
 	/* irq ID 0 is reserved */
-	for (i = 1; i < priv->nr_irqs; i++) {
+	for (i = 1; i <= priv->nr_irqs; i++) {
 		index = BIT_WORD(i);
 		writel((priv->prio_save[index] & BIT_MASK(i)) ? 1 : 0,
 		       priv->regs + PRIORITY_BASE + i * PRIORITY_PER_ID);
@@ -293,7 +293,7 @@ static void plic_irq_resume(void *data)
 			continue;
 
 		raw_spin_lock_irqsave(&handler->enable_lock, flags);
-		for (i = 0; i < DIV_ROUND_UP(priv->nr_irqs, 32); i++) {
+		for (i = 0; i < DIV_ROUND_UP(priv->nr_irqs + 1, 32); i++) {
 			reg = handler->enable_base + i * sizeof(u32);
 			writel(handler->enable_save[i], reg);
 		}
@@ -351,7 +351,7 @@ static int plic_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
 	if (ret)
 		return ret;
 
-	for (i = 0; i < nr_irqs; i++) {
+	for (i = 1; i <= nr_irqs; i++) {
 		ret = plic_irqdomain_map(domain, virq + i, hwirq + i);
 		if (ret)
 			return ret;
@@ -431,7 +431,7 @@ static u32 cp100_isolate_pending_irq(int nr_irq_groups, struct plic_handler *han
 
 static irq_hw_number_t cp100_get_hwirq(struct plic_handler *handler, void __iomem *claim)
 {
-	int nr_irq_groups = DIV_ROUND_UP(handler->priv->nr_irqs, 32);
+	int nr_irq_groups = DIV_ROUND_UP(handler->priv->nr_irqs + 1, 32);
 	u32 __iomem *enable = handler->enable_base;
 	irq_hw_number_t hwirq = 0;
 	u32 iso_mask;
@@ -652,7 +652,7 @@ static int plic_probe(struct fwnode_handle *fwnode)
 	priv->gsi_base = gsi_base;
 	priv->acpi_plic_id = id;
 
-	priv->prio_save = bitmap_zalloc(nr_irqs, GFP_KERNEL);
+	priv->prio_save = bitmap_zalloc(nr_irqs + 1, GFP_KERNEL);
 	if (!priv->prio_save) {
 		error = -ENOMEM;
 		goto fail_free_priv;
@@ -686,7 +686,7 @@ static int plic_probe(struct fwnode_handle *fwnode)
 				u32 __iomem *enable_base = priv->regs +	CONTEXT_ENABLE_BASE +
 							   i * CONTEXT_ENABLE_SIZE;
 
-				for (int j = 0; j <= nr_irqs / 32; j++)
+				for (int j = 0; j <= (nr_irqs + 1) / 32; j++)
 					writel(0, enable_base + j);
 			}
 			continue;
@@ -718,7 +718,7 @@ static int plic_probe(struct fwnode_handle *fwnode)
 			context_id * CONTEXT_ENABLE_SIZE;
 		handler->priv = priv;
 
-		handler->enable_save = kcalloc(DIV_ROUND_UP(nr_irqs, 32),
+		handler->enable_save = kcalloc(DIV_ROUND_UP(nr_irqs + 1, 32),
 					       sizeof(*handler->enable_save), GFP_KERNEL);
 		if (!handler->enable_save) {
 			error = -ENOMEM;
-- 
2.51.0

Re: [PATCH v3 1/2] irqchip/sifive-plic: Fix wrong nr_irqs handling

[Thomas Gleixner]

On Wed, Feb 04 2026 at 01:21, Yangyu Chen wrote:
> @@ -351,7 +351,7 @@ static int plic_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
>  	if (ret)
>  		return ret;
>  
> -	for (i = 0; i < nr_irqs; i++) {
> +	for (i = 1; i <= nr_irqs; i++) {
>  		ret = plic_irqdomain_map(domain, virq + i, hwirq + i);

That's just wrong and clearly untested.

@virq and @nr_irqs are provided by the core code and you cannot
manipulate them just because.

This instance of nr_irqs has absolutely nothing to do with the problem
you are trying to solve. The core invokes this to map

    $N (@nr_irqs) Linux interrupt numbers starting from @virq to
    hardware interrupt numbers.

The fwspec argument (@arg) is used to retrieve the hardware interrupt
number from the device tree:

    plic_irq_domain_translate(....);

The device tree better contains the real hardware interrupt number and
not a 0 based enumeration.

>  static irq_hw_number_t cp100_get_hwirq(struct plic_handler *handler, void __iomem *claim)
>  {
> -	int nr_irq_groups = DIV_ROUND_UP(handler->priv->nr_irqs, 32);
> +	int nr_irq_groups = DIV_ROUND_UP(handler->priv->nr_irqs + 1, 32);

Requiring this '+1' muck all over the place is a guarantee for more
disaster.

It's not really hard to sit back and think about it instead of
mindlessly changing things until it looks about right. I'm tired of
wasting my time with reviewing botched up stuff like that.

Untested, but defintely correct patch below. If you find a bug, I owe
you a beer at the next conference.

Thanks,

        tglx
---
Subject: irqchip/sifive-plic: Handle number of hardware interrupts correctly
From: Thomas Gleixner <tglx@kernel.org>
Date: Tue, 03 Feb 2026 20:16:12 +0100

The driver is inconsistently handling the number of hardware interrupts.

The reason is that the firmware enumerates the maximum number of device
interrupts, but the actual number of hardware interrupts is one more
because hardware interrupt 0 is reserved.

There are two loop variants where this matters:

  1) Iterating over the device interrupts

     for (irq = 1; irq < total_irqs; irq++)

  2) Iterating over the number of interrupt register groups

     for (grp = 0; grp < irq_groups; grp++)

The current code stores the number of device interrupts and that requires
to write the loops as:

  1) for (irq = 1; irq <= device_irqs; irq++)

  2) for (grp = 0; grp < DIV_ROUND_UP(device_irqs + 1); grp++)

But the code gets it wrong all over the place. Just fixing up the
conditions and off by ones is not a sustainable solution as the next changes
will reintroduce the same bugs over and over.

Sanitize it by storing the total number of hardware interrupts during probe
and precalculating the number of groups. To future proof it mark
priv::total_irqs __private, provide a correct iterator macro and adjust the
code to this.

Marking it private allows sparse (C=1 build) to catch direct access to this
member:

  drivers/irqchip/irq-sifive-plic.c:270:9: warning: dereference of noderef expression

That should prevent at least the most obvious future damage in that area.

Fixes: e80f0b6a2cf3 ("irqchip/irq-sifive-plic: Add syscore callbacks for hibernation")
Reported-by: Yangyu Chen <cyy@cyyself.name>
Signed-off-by: Thomas Gleixner <tglx@kernel.org>
---
 drivers/irqchip/irq-sifive-plic.c |   82 ++++++++++++++++++++------------------
 1 file changed, 45 insertions(+), 37 deletions(-)

--- a/drivers/irqchip/irq-sifive-plic.c
+++ b/drivers/irqchip/irq-sifive-plic.c
@@ -68,15 +68,17 @@
 #define PLIC_QUIRK_CP100_CLAIM_REGISTER_ERRATUM	1
 
 struct plic_priv {
-	struct fwnode_handle *fwnode;
-	struct cpumask lmask;
-	struct irq_domain *irqdomain;
-	void __iomem *regs;
-	unsigned long plic_quirks;
-	unsigned int nr_irqs;
-	unsigned long *prio_save;
-	u32 gsi_base;
-	int acpi_plic_id;
+	struct fwnode_handle	*fwnode;
+	struct cpumask		lmask;
+	struct irq_domain	*irqdomain;
+	void __iomem		*regs;
+	unsigned long		plic_quirks;
+	/* @device_irqs + 1 to compensate for the reserved hwirq 0 */
+	unsigned int __private	total_irqs;
+	unsigned int		irq_groups;
+	unsigned long		*prio_save;
+	u32			gsi_base;
+	int			acpi_plic_id;
 };
 
 struct plic_handler {
@@ -91,6 +93,12 @@ struct plic_handler {
 	u32			*enable_save;
 	struct plic_priv	*priv;
 };
+
+/*
+ * Macro to deal with the insanity of hardware interrupt 0 being reserved */
+#define for_each_device_irq(iter, priv)	\
+	for (unsigned int iter = 1; iter < ACCESS_PRIVATE(priv, total_irqs); iter++)
+
 static int plic_parent_irq __ro_after_init;
 static bool plic_global_setup_done __ro_after_init;
 static DEFINE_PER_CPU(struct plic_handler, plic_handlers);
@@ -257,14 +265,11 @@ static int plic_irq_set_type(struct irq_
 
 static int plic_irq_suspend(void *data)
 {
-	struct plic_priv *priv;
-
-	priv = per_cpu_ptr(&plic_handlers, smp_processor_id())->priv;
+	struct plic_priv *priv = this_cpu_ptr(&plic_handlers)->priv;
 
-	/* irq ID 0 is reserved */
-	for (unsigned int i = 1; i < priv->nr_irqs; i++) {
-		__assign_bit(i, priv->prio_save,
-			     readl(priv->regs + PRIORITY_BASE + i * PRIORITY_PER_ID));
+	for_each_device_irq(irq, priv) {
+		__assign_bit(irq, priv->prio_save,
+			     readl(priv->regs + PRIORITY_BASE + irq * PRIORITY_PER_ID));
 	}
 
 	return 0;
@@ -272,18 +277,15 @@ static int plic_irq_suspend(void *data)
 
 static void plic_irq_resume(void *data)
 {
-	unsigned int i, index, cpu;
+	struct plic_priv *priv = this_cpu_ptr(&plic_handlers)->priv;
+	unsigned int index, cpu;
 	unsigned long flags;
 	u32 __iomem *reg;
-	struct plic_priv *priv;
-
-	priv = per_cpu_ptr(&plic_handlers, smp_processor_id())->priv;
 
-	/* irq ID 0 is reserved */
-	for (i = 1; i < priv->nr_irqs; i++) {
-		index = BIT_WORD(i);
-		writel((priv->prio_save[index] & BIT_MASK(i)) ? 1 : 0,
-		       priv->regs + PRIORITY_BASE + i * PRIORITY_PER_ID);
+	for_each_device_irq(irq, priv) {
+		index = BIT_WORD(irq);
+		writel((priv->prio_save[index] & BIT_MASK(irq)) ? 1 : 0,
+		       priv->regs + PRIORITY_BASE + irq * PRIORITY_PER_ID);
 	}
 
 	for_each_present_cpu(cpu) {
@@ -293,7 +295,7 @@ static void plic_irq_resume(void *data)
 			continue;
 
 		raw_spin_lock_irqsave(&handler->enable_lock, flags);
-		for (i = 0; i < DIV_ROUND_UP(priv->nr_irqs, 32); i++) {
+		for (unsigned int i = 0; i < priv->irq_groups; i++) {
 			reg = handler->enable_base + i * sizeof(u32);
 			writel(handler->enable_save[i], reg);
 		}
@@ -431,7 +433,7 @@ static u32 cp100_isolate_pending_irq(int
 
 static irq_hw_number_t cp100_get_hwirq(struct plic_handler *handler, void __iomem *claim)
 {
-	int nr_irq_groups = DIV_ROUND_UP(handler->priv->nr_irqs, 32);
+	int nr_irq_groups = handler->priv->irq_groups;
 	u32 __iomem *enable = handler->enable_base;
 	irq_hw_number_t hwirq = 0;
 	u32 iso_mask;
@@ -614,7 +616,6 @@ static int plic_probe(struct fwnode_hand
 	struct plic_handler *handler;
 	u32 nr_irqs, parent_hwirq;
 	struct plic_priv *priv;
-	irq_hw_number_t hwirq;
 	void __iomem *regs;
 	int id, context_id;
 	u32 gsi_base;
@@ -647,7 +648,16 @@ static int plic_probe(struct fwnode_hand
 
 	priv->fwnode = fwnode;
 	priv->plic_quirks = plic_quirks;
-	priv->nr_irqs = nr_irqs;
+	/*
+	 * The firmware provides the number of device interrupts. As
+	 * hardware interrupt 0 is reserved, the number of total interrupts
+	 * is nr_irqs + 1.
+	 */
+	nr_irqs++;
+	ACCESS_PRIVATE(priv, total_irqs) = nr_irqs;
+	/* Precalculate the number of register groups */
+	priv->irq_groups = DIV_ROUND_UP(nr_irqs, 32);
+
 	priv->regs = regs;
 	priv->gsi_base = gsi_base;
 	priv->acpi_plic_id = id;
@@ -686,7 +696,7 @@ static int plic_probe(struct fwnode_hand
 				u32 __iomem *enable_base = priv->regs +	CONTEXT_ENABLE_BASE +
 							   i * CONTEXT_ENABLE_SIZE;
 
-				for (int j = 0; j <= nr_irqs / 32; j++)
+				for (int j = 0; j < priv->irq_groups; j++)
 					writel(0, enable_base + j);
 			}
 			continue;
@@ -718,23 +728,21 @@ static int plic_probe(struct fwnode_hand
 			context_id * CONTEXT_ENABLE_SIZE;
 		handler->priv = priv;
 
-		handler->enable_save = kcalloc(DIV_ROUND_UP(nr_irqs, 32),
-					       sizeof(*handler->enable_save), GFP_KERNEL);
+		handler->enable_save = kcalloc(priv->irq_groups, sizeof(*handler->enable_save),
+					       GFP_KERNEL);
 		if (!handler->enable_save) {
 			error = -ENOMEM;
 			goto fail_cleanup_contexts;
 		}
 done:
-		for (hwirq = 1; hwirq <= nr_irqs; hwirq++) {
+		for_each_device_irq(hwirq, priv) {
 			plic_toggle(handler, hwirq, 0);
-			writel(1, priv->regs + PRIORITY_BASE +
-				  hwirq * PRIORITY_PER_ID);
+			writel(1, priv->regs + PRIORITY_BASE + hwirq * PRIORITY_PER_ID);
 		}
 		nr_handlers++;
 	}
 
-	priv->irqdomain = irq_domain_create_linear(fwnode, nr_irqs + 1,
-						   &plic_irqdomain_ops, priv);
+	priv->irqdomain = irq_domain_create_linear(fwnode, nr_irqs, &plic_irqdomain_ops, priv);
 	if (WARN_ON(!priv->irqdomain)) {
 		error = -ENOMEM;
 		goto fail_cleanup_contexts;

Re: [PATCH v3 1/2] irqchip/sifive-plic: Fix wrong nr_irqs handling

[Yangyu Chen]

On 4/2/2026 04:40, Thomas Gleixner wrote:
> On Wed, Feb 04 2026 at 01:21, Yangyu Chen wrote:
>> @@ -351,7 +351,7 @@ static int plic_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
>>   	if (ret)
>>   		return ret;
>>   
>> -	for (i = 0; i < nr_irqs; i++) {
>> +	for (i = 1; i <= nr_irqs; i++) {
>>   		ret = plic_irqdomain_map(domain, virq + i, hwirq + i);
> 
> That's just wrong and clearly untested.
> 
> @virq and @nr_irqs are provided by the core code and you cannot
> manipulate them just because.
> 
> This instance of nr_irqs has absolutely nothing to do with the problem
> you are trying to solve. The core invokes this to map
> 
>      $N (@nr_irqs) Linux interrupt numbers starting from @virq to
>      hardware interrupt numbers.
> 
> The fwspec argument (@arg) is used to retrieve the hardware interrupt
> number from the device tree:
> 
>      plic_irq_domain_translate(....);
> 
> The device tree better contains the real hardware interrupt number and
> not a 0 based enumeration.
> 
>>   static irq_hw_number_t cp100_get_hwirq(struct plic_handler *handler, void __iomem *claim)
>>   {
>> -	int nr_irq_groups = DIV_ROUND_UP(handler->priv->nr_irqs, 32);
>> +	int nr_irq_groups = DIV_ROUND_UP(handler->priv->nr_irqs + 1, 32);
> 
> Requiring this '+1' muck all over the place is a guarantee for more
> disaster.
> 
> It's not really hard to sit back and think about it instead of
> mindlessly changing things until it looks about right. I'm tired of
> wasting my time with reviewing botched up stuff like that.
> 

Sorry for that. I haven't tested that on QEMU with interrupt enabled at 
the time of submission, only tested on XiangShan's NEMU [1] without 
interrupt (serial console is handled by sbi hvc pull) to see if the 
memory corruption bug is being resolved. Sorry for the missed coverage. 
And also sorry for my misunderstanding of irq_domain. I have tested it 
today on QEMU, and it even breaks serial interrupts.

[1] https://github.com/OpenXiangShan/NEMU

> Untested, but defintely correct patch below. If you find a bug, I owe
> you a beer at the next conference.
> 
> Thanks,
> 
>          tglx
> ---
> Subject: irqchip/sifive-plic: Handle number of hardware interrupts correctly
> From: Thomas Gleixner <tglx@kernel.org>
> Date: Tue, 03 Feb 2026 20:16:12 +0100
> 
> The driver is inconsistently handling the number of hardware interrupts.
> 
> The reason is that the firmware enumerates the maximum number of device
> interrupts, but the actual number of hardware interrupts is one more
> because hardware interrupt 0 is reserved.
> 
> There are two loop variants where this matters:
> 
>    1) Iterating over the device interrupts
> 
>       for (irq = 1; irq < total_irqs; irq++)
> 
>    2) Iterating over the number of interrupt register groups
> 
>       for (grp = 0; grp < irq_groups; grp++)
> 
> The current code stores the number of device interrupts and that requires
> to write the loops as:
> 
>    1) for (irq = 1; irq <= device_irqs; irq++)
> 
>    2) for (grp = 0; grp < DIV_ROUND_UP(device_irqs + 1); grp++)
> 
> But the code gets it wrong all over the place. Just fixing up the
> conditions and off by ones is not a sustainable solution as the next changes
> will reintroduce the same bugs over and over.
> 
> Sanitize it by storing the total number of hardware interrupts during probe
> and precalculating the number of groups. To future proof it mark
> priv::total_irqs __private, provide a correct iterator macro and adjust the
> code to this.
> 

Your idea is great to prevent future bugs from being produced. Thanks!

> Marking it private allows sparse (C=1 build) to catch direct access to this
> member:
> 
>    drivers/irqchip/irq-sifive-plic.c:270:9: warning: dereference of noderef expression
> 
> That should prevent at least the most obvious future damage in that area.
> 
> Fixes: e80f0b6a2cf3 ("irqchip/irq-sifive-plic: Add syscore callbacks for hibernation")
> Reported-by: Yangyu Chen <cyy@cyyself.name>
> Signed-off-by: Thomas Gleixner <tglx@kernel.org>
> ---
>   drivers/irqchip/irq-sifive-plic.c |   82 ++++++++++++++++++++------------------
>   1 file changed, 45 insertions(+), 37 deletions(-)
> 
> --- a/drivers/irqchip/irq-sifive-plic.c
> +++ b/drivers/irqchip/irq-sifive-plic.c
> @@ -68,15 +68,17 @@
>   #define PLIC_QUIRK_CP100_CLAIM_REGISTER_ERRATUM	1
>   
>   struct plic_priv {
> -	struct fwnode_handle *fwnode;
> -	struct cpumask lmask;
> -	struct irq_domain *irqdomain;
> -	void __iomem *regs;
> -	unsigned long plic_quirks;
> -	unsigned int nr_irqs;
> -	unsigned long *prio_save;
> -	u32 gsi_base;
> -	int acpi_plic_id;
> +	struct fwnode_handle	*fwnode;
> +	struct cpumask		lmask;
> +	struct irq_domain	*irqdomain;
> +	void __iomem		*regs;
> +	unsigned long		plic_quirks;
> +	/* @device_irqs + 1 to compensate for the reserved hwirq 0 */
> +	unsigned int __private	total_irqs;
> +	unsigned int		irq_groups;
> +	unsigned long		*prio_save;
> +	u32			gsi_base;
> +	int			acpi_plic_id;
>   };
>   
>   struct plic_handler {
> @@ -91,6 +93,12 @@ struct plic_handler {
>   	u32			*enable_save;
>   	struct plic_priv	*priv;
>   };
> +
> +/*
> + * Macro to deal with the insanity of hardware interrupt 0 being reserved */
> +#define for_each_device_irq(iter, priv)	\
> +	for (unsigned int iter = 1; iter < ACCESS_PRIVATE(priv, total_irqs); iter++)
> +
>   static int plic_parent_irq __ro_after_init;
>   static bool plic_global_setup_done __ro_after_init;
>   static DEFINE_PER_CPU(struct plic_handler, plic_handlers);
> @@ -257,14 +265,11 @@ static int plic_irq_set_type(struct irq_
>   
>   static int plic_irq_suspend(void *data)
>   {
> -	struct plic_priv *priv;
> -
> -	priv = per_cpu_ptr(&plic_handlers, smp_processor_id())->priv;
> +	struct plic_priv *priv = this_cpu_ptr(&plic_handlers)->priv;
>   
> -	/* irq ID 0 is reserved */
> -	for (unsigned int i = 1; i < priv->nr_irqs; i++) {
> -		__assign_bit(i, priv->prio_save,
> -			     readl(priv->regs + PRIORITY_BASE + i * PRIORITY_PER_ID));
> +	for_each_device_irq(irq, priv) {
> +		__assign_bit(irq, priv->prio_save,
> +			     readl(priv->regs + PRIORITY_BASE + irq * PRIORITY_PER_ID));
>   	}
>   
>   	return 0;
> @@ -272,18 +277,15 @@ static int plic_irq_suspend(void *data)
>   
>   static void plic_irq_resume(void *data)
>   {
> -	unsigned int i, index, cpu;
> +	struct plic_priv *priv = this_cpu_ptr(&plic_handlers)->priv;
> +	unsigned int index, cpu;
>   	unsigned long flags;
>   	u32 __iomem *reg;
> -	struct plic_priv *priv;
> -
> -	priv = per_cpu_ptr(&plic_handlers, smp_processor_id())->priv;
>   
> -	/* irq ID 0 is reserved */
> -	for (i = 1; i < priv->nr_irqs; i++) {
> -		index = BIT_WORD(i);
> -		writel((priv->prio_save[index] & BIT_MASK(i)) ? 1 : 0,
> -		       priv->regs + PRIORITY_BASE + i * PRIORITY_PER_ID);
> +	for_each_device_irq(irq, priv) {
> +		index = BIT_WORD(irq);
> +		writel((priv->prio_save[index] & BIT_MASK(irq)) ? 1 : 0,
> +		       priv->regs + PRIORITY_BASE + irq * PRIORITY_PER_ID);
>   	}
>   
>   	for_each_present_cpu(cpu) {
> @@ -293,7 +295,7 @@ static void plic_irq_resume(void *data)
>   			continue;
>   
>   		raw_spin_lock_irqsave(&handler->enable_lock, flags);
> -		for (i = 0; i < DIV_ROUND_UP(priv->nr_irqs, 32); i++) {
> +		for (unsigned int i = 0; i < priv->irq_groups; i++) {
>   			reg = handler->enable_base + i * sizeof(u32);
>   			writel(handler->enable_save[i], reg);
>   		}
> @@ -431,7 +433,7 @@ static u32 cp100_isolate_pending_irq(int
>   
>   static irq_hw_number_t cp100_get_hwirq(struct plic_handler *handler, void __iomem *claim)
>   {
> -	int nr_irq_groups = DIV_ROUND_UP(handler->priv->nr_irqs, 32);
> +	int nr_irq_groups = handler->priv->irq_groups;
>   	u32 __iomem *enable = handler->enable_base;
>   	irq_hw_number_t hwirq = 0;
>   	u32 iso_mask;
> @@ -614,7 +616,6 @@ static int plic_probe(struct fwnode_hand
>   	struct plic_handler *handler;
>   	u32 nr_irqs, parent_hwirq;
>   	struct plic_priv *priv;
> -	irq_hw_number_t hwirq;
>   	void __iomem *regs;
>   	int id, context_id;
>   	u32 gsi_base;
> @@ -647,7 +648,16 @@ static int plic_probe(struct fwnode_hand
>   
>   	priv->fwnode = fwnode;
>   	priv->plic_quirks = plic_quirks;
> -	priv->nr_irqs = nr_irqs;
> +	/*
> +	 * The firmware provides the number of device interrupts. As
> +	 * hardware interrupt 0 is reserved, the number of total interrupts
> +	 * is nr_irqs + 1.
> +	 */
> +	nr_irqs++;
> +	ACCESS_PRIVATE(priv, total_irqs) = nr_irqs;
> +	/* Precalculate the number of register groups */
> +	priv->irq_groups = DIV_ROUND_UP(nr_irqs, 32);
> +
>   	priv->regs = regs;
>   	priv->gsi_base = gsi_base;
>   	priv->acpi_plic_id = id;
> @@ -686,7 +696,7 @@ static int plic_probe(struct fwnode_hand
>   				u32 __iomem *enable_base = priv->regs +	CONTEXT_ENABLE_BASE +
>   							   i * CONTEXT_ENABLE_SIZE;
>   
> -				for (int j = 0; j <= nr_irqs / 32; j++)
> +				for (int j = 0; j < priv->irq_groups; j++)
>   					writel(0, enable_base + j);
>   			}
>   			continue;
> @@ -718,23 +728,21 @@ static int plic_probe(struct fwnode_hand
>   			context_id * CONTEXT_ENABLE_SIZE;
>   		handler->priv = priv;
>   
> -		handler->enable_save = kcalloc(DIV_ROUND_UP(nr_irqs, 32),
> -					       sizeof(*handler->enable_save), GFP_KERNEL);
> +		handler->enable_save = kcalloc(priv->irq_groups, sizeof(*handler->enable_save),
> +					       GFP_KERNEL);
>   		if (!handler->enable_save) {
>   			error = -ENOMEM;
>   			goto fail_cleanup_contexts;
>   		}
>   done:
> -		for (hwirq = 1; hwirq <= nr_irqs; hwirq++) {
> +		for_each_device_irq(hwirq, priv) {
>   			plic_toggle(handler, hwirq, 0);
> -			writel(1, priv->regs + PRIORITY_BASE +
> -				  hwirq * PRIORITY_PER_ID);
> +			writel(1, priv->regs + PRIORITY_BASE + hwirq * PRIORITY_PER_ID);
>   		}
>   		nr_handlers++;
>   	}
>   
> -	priv->irqdomain = irq_domain_create_linear(fwnode, nr_irqs + 1,
> -						   &plic_irqdomain_ops, priv);
> +	priv->irqdomain = irq_domain_create_linear(fwnode, nr_irqs, &plic_irqdomain_ops, priv);
>   	if (WARN_ON(!priv->irqdomain)) {
>   		error = -ENOMEM;
>   		goto fail_cleanup_contexts;

Tested-by: Yangyu Chen <cyy@cyyself.name>

Tested on both QEMU and NEMU, and it works great!

Maybe we can merge this along with the dt-binding updates.

Thanks,
Yangyu Chen

Re: [PATCH v3 1/2] irqchip/sifive-plic: Fix wrong nr_irqs handling

[Conor Dooley]

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

On Wed, Feb 04, 2026 at 01:21:16AM +0800, Yangyu Chen wrote:
> Since the first irq source is 1 instead of 0, when the number of
> irqs is multiple of 32, the last irq group will be ignored during
> allocation, saving, and restoring. This lead to memory corruption
> when accessing enable_save beyond allocated memory after commit
> 14ff9e54dd14 ("irqchip/sifive-plic: Cache the interrupt enable state")
> which will access enable_save for all sources during plic_probe.
> Thus, we should allocate irq_groups based on (nr_irqs + 1) instead of
> nr_irqs to avoid this issue. This commit also fixes related loops
> to have all consumer of nr_irqs consistent.
> 
> This is an long standing bug since Linux v5.6 but since the last irq
> source is rarely used, it may not be triggered in practice until commit

FWIW, on mpfs the 186th and last source is used by the hardware but it's
used by the platform's m-mode firmware not linux.

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

[PATCH v3 2/2] dt-binding: riscv: Clarify the riscv,ndev meaning in PLIC

[Yangyu Chen]

In PLIC, interrupt source 0 is reserved and should not be used.
Therefore, the valid interrupt sources are from 1 to riscv,ndev
inclusive. This commit updates the documentation to clarify this point.

Signed-off-by: Yangyu Chen <cyy@cyyself.name>
---
 .../bindings/interrupt-controller/sifive,plic-1.0.0.yaml        | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml b/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml
index 388fc2c620c0..df9578bcac89 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml
+++ b/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml
@@ -109,6 +109,8 @@ properties:
     $ref: /schemas/types.yaml#/definitions/uint32
     description:
       Specifies how many external interrupts are supported by this controller.
+      Note that source 0 is reserved in PLIC, so the valid interrupt sources
+      are 1 to riscv,ndev inclusive.
 
   clocks: true
 
-- 
2.51.0

Re: [PATCH v3 2/2] dt-binding: riscv: Clarify the riscv,ndev meaning in PLIC

[Conor Dooley]

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

On Wed, Feb 04, 2026 at 01:21:48AM +0800, Yangyu Chen wrote:
> In PLIC, interrupt source 0 is reserved and should not be used.
> Therefore, the valid interrupt sources are from 1 to riscv,ndev
> inclusive. This commit updates the documentation to clarify this point.
> 
> Signed-off-by: Yangyu Chen <cyy@cyyself.name>
> ---
>  .../bindings/interrupt-controller/sifive,plic-1.0.0.yaml        | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml b/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml
> index 388fc2c620c0..df9578bcac89 100644
> --- a/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml
> +++ b/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml
> @@ -109,6 +109,8 @@ properties:
>      $ref: /schemas/types.yaml#/definitions/uint32
>      description:
>        Specifies how many external interrupts are supported by this controller.
> +      Note that source 0 is reserved in PLIC, so the valid interrupt sources
> +      are 1 to riscv,ndev inclusive.

The trm for mpfs says "in the plic, global interrupt id 0 means 'no
interrupt'". That sounds subtly different to me than "reserved", but
/shrug
I think this could just be truncated to "Valid interrupt sources are
1 to riscv,ndev, inclusive.".
Acked-by: Conor Dooley <conor.dooley@microchip.com>

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

Re: [PATCH v3 0/2] irqchip/sifive-plic: Fix wrong nr_irqs handling

[patchwork-bot+linux-riscv]

Hello:

This series was applied to riscv/linux.git (fixes)
by Thomas Gleixner <tglx@kernel.org>:

On Wed,  4 Feb 2026 01:17:07 +0800 you wrote:
> This patch series fixes long standing bugs in sifive-plic driver regarding
> the handling of nr_irqs. Some code assumes the first irq source is 0 while
> some assumes it is 1. Since the first irq source is actually 1, this causes
> various issues including memory corruption when the number of irqs is
> multiple of 32. Also, some code assumes nr_irqs is the maximum irq source
> ID while some assumes it is the total number of irq sources including the
> reserved source 0. This patch series standardizes the handling of nr_irqs
> to be the maximum irq source ID, and the first irq source is 1.
> 
> [...]

Here is the summary with links:
  - [v3,1/2] irqchip/sifive-plic: Fix wrong nr_irqs handling
    (no matching commit)
  - [v3,2/2] dt-binding: riscv: Clarify the riscv,ndev meaning in PLIC
    https://git.kernel.org/riscv/c/889588d75050

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html