[PATCH] irqchip/riscv-imsic: Synchronize remote state with IRQ work

[PATCH] irqchip/riscv-imsic: Synchronize remote state with IRQ work

[Troy Mitchell]

Remote IMSIC state updates are deferred to a timer on the target CPU.
If that CPU has stopped its local clockevent and is waiting for a
broadcast interrupt, enabling the broadcast IRQ can depend on delivery
of that same interrupt. This can stall timer delivery when the broadcast
IRQ is unmasked or moved to another CPU, including during CPU hotplug.

Queue hard IRQ work to synchronize remote state independently of timer
delivery. Keep synchronous updates on the local CPU and the timer retry
for an unfinished vector move. Hard IRQ work also avoids depending on a
schedulable worker when PREEMPT_RT is enabled.

Fixes: 21a8f8a0eb35 ("irqchip: Add RISC-V incoming MSI controller early driver")
Signed-off-by: Troy Mitchell <troy.mitchell@linux.dev>
---
This was found while bringing up the SoC timer on SpacemiT K3 as a tick
broadcast device.

Without a real broadcast device, the tick code falls back to the hrtimer
based broadcast, which keeps one CPU's local timer running. Once the SoC
timer is registered it replaces the hrtimer broadcast, and every CPU may
stop its local timer in idle.

On K3 the SoC timer interrupt goes through APLIC in MSI mode to IMSIC.
When the CPU owning it goes offline, the interrupt is moved to a CPU
that may already be idle with its local timer stopped. IMSIC leaves the
enable on the new target to a timer on that CPU, and that timer can only
fire through the broadcast interrupt, which is not enabled yet. The
system stalls.

Tested on a K3 COM260 board by offlining the CPU owning the broadcast
interrupt, and then each CPU in turn, under a timerfd load:
- v7.4-rc1 with the SpacemiT SoC timer series and local timers forced
  to stop in idle
- a vendor 6.18 kernel with SBI cpuidle states that stop the local timer

Both stall without this patch and pass with it.
---
 drivers/irqchip/Kconfig                 |  1 +
 drivers/irqchip/irq-riscv-imsic-state.c | 19 +++++++++++++------
 drivers/irqchip/irq-riscv-imsic-state.h |  4 ++++
 3 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 136bab41f801..5cdc3f52198c 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -651,6 +651,7 @@ config RISCV_IMSIC
 	bool
 	depends on RISCV
 	select IRQ_DOMAIN_HIERARCHY
+	select IRQ_WORK if SMP
 	select GENERIC_IRQ_MATRIX_ALLOCATOR
 	select GENERIC_MSI_IRQ
 	select IRQ_MSI_LIB
diff --git a/drivers/irqchip/irq-riscv-imsic-state.c b/drivers/irqchip/irq-riscv-imsic-state.c
index 01620373b43a..f6373e4019a2 100644
--- a/drivers/irqchip/irq-riscv-imsic-state.c
+++ b/drivers/irqchip/irq-riscv-imsic-state.c
@@ -252,6 +252,11 @@ void imsic_local_delivery(bool enable)
 }
 
 #ifdef CONFIG_SMP
+static void imsic_local_sync_work(struct irq_work *work)
+{
+	imsic_local_sync_all(false);
+}
+
 static void imsic_local_timer_callback(struct timer_list *timer)
 {
 	imsic_local_sync_all(false);
@@ -268,18 +273,19 @@ static void __imsic_remote_sync(struct imsic_local_priv *lpriv, unsigned int cpu
 	 */
 
 	/*
-	 * We schedule a timer on the target CPU if the target CPU is not
-	 * same as the current CPU. An offline CPU will unconditionally
-	 * synchronize IDs through imsic_starting_cpu() when the
-	 * CPU is brought up.
+	 * A remote CPU may have stopped its local timer and be waiting for
+	 * the broadcast interrupt whose IMSIC state needs updating. Use an
+	 * IPI-backed work item so enabling that interrupt does not depend on
+	 * its delivery. An offline CPU synchronizes IDs in imsic_starting_cpu().
 	 */
 	if (cpu_online(cpu)) {
 		if (cpu == smp_processor_id()) {
 			if (__imsic_local_sync(lpriv))
 				return;
+			__imsic_local_timer_start(lpriv, cpu);
+		} else {
+			irq_work_queue_on(&lpriv->sync_work, cpu);
 		}
-
-		__imsic_local_timer_start(lpriv, cpu);
 	}
 }
 #else
@@ -510,6 +516,7 @@ static int __init imsic_local_init(void)
 #ifdef CONFIG_SMP
 		/* Setup lazy timer for synchronization */
 		timer_setup(&lpriv->timer, imsic_local_timer_callback, TIMER_PINNED);
+		lpriv->sync_work = IRQ_WORK_INIT_HARD(imsic_local_sync_work);
 #endif
 
 		/* Allocate vector array */
diff --git a/drivers/irqchip/irq-riscv-imsic-state.h b/drivers/irqchip/irq-riscv-imsic-state.h
index 189829b8673a..a36b9f0ef840 100644
--- a/drivers/irqchip/irq-riscv-imsic-state.h
+++ b/drivers/irqchip/irq-riscv-imsic-state.h
@@ -9,6 +9,7 @@
 
 #include <linux/irqchip/riscv-imsic.h>
 #include <linux/irqdomain.h>
+#include <linux/irq_work.h>
 #include <linux/fwnode.h>
 #include <linux/timer.h>
 
@@ -34,6 +35,9 @@ struct imsic_local_priv {
 	unsigned long				*dirty_bitmap;
 
 #ifdef CONFIG_SMP
+	/* Remote synchronization must not depend on a timer interrupt. */
+	struct irq_work				sync_work;
+
 	/* Local timer for synchronization */
 	struct timer_list			timer;
 #endif

---
base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4
change-id: 20261103-imsic-remote-sync-irqwork-9cfec6c410de

Best regards,
--  
Troy Mitchell <troy.mitchell@linux.dev>

Re: [PATCH] irqchip/riscv-imsic: Synchronize remote state with IRQ work

[Sebastian Andrzej Siewior]

On 2026-11-03 08:04:33 [+0000], Troy Mitchell wrote:
> Without a real broadcast device, the tick code falls back to the hrtimer
> based broadcast, which keeps one CPU's local timer running. Once the SoC
> timer is registered it replaces the hrtimer broadcast, and every CPU may
> stop its local timer in idle.
> 
> On K3 the SoC timer interrupt goes through APLIC in MSI mode to IMSIC.
> When the CPU owning it goes offline, the interrupt is moved to a CPU
> that may already be idle with its local timer stopped. IMSIC leaves the
> enable on the new target to a timer on that CPU, and that timer can only
> fire through the broadcast interrupt, which is not enabled yet. The
> system stalls.

So the broadcast timer is moved on CPU which is idle and waking the CPU
is done by a timer, which does not work if the CPU is idle.
This makes sense.

Sebastian

Re: [PATCH] irqchip/riscv-imsic: Synchronize remote state with IRQ work

[Anup Patel]

On Tue, Nov 3, 2026 at 1:34 PM Troy Mitchell <troy.mitchell@linux.dev> wrote:
>
> Remote IMSIC state updates are deferred to a timer on the target CPU.
> If that CPU has stopped its local clockevent and is waiting for a
> broadcast interrupt, enabling the broadcast IRQ can depend on delivery
> of that same interrupt. This can stall timer delivery when the broadcast
> IRQ is unmasked or moved to another CPU, including during CPU hotplug.
>
> Queue hard IRQ work to synchronize remote state independently of timer
> delivery. Keep synchronous updates on the local CPU and the timer retry
> for an unfinished vector move. Hard IRQ work also avoids depending on a
> schedulable worker when PREEMPT_RT is enabled.

Using timer retry for local CPU and IRQ work queue for remote CPUs looks
strange. Why not use IRQ work queue even for local CPU ??

>
> Fixes: 21a8f8a0eb35 ("irqchip: Add RISC-V incoming MSI controller early driver")
> Signed-off-by: Troy Mitchell <troy.mitchell@linux.dev>
> ---
> This was found while bringing up the SoC timer on SpacemiT K3 as a tick
> broadcast device.
>
> Without a real broadcast device, the tick code falls back to the hrtimer
> based broadcast, which keeps one CPU's local timer running. Once the SoC
> timer is registered it replaces the hrtimer broadcast, and every CPU may
> stop its local timer in idle.
>
> On K3 the SoC timer interrupt goes through APLIC in MSI mode to IMSIC.
> When the CPU owning it goes offline, the interrupt is moved to a CPU
> that may already be idle with its local timer stopped. IMSIC leaves the
> enable on the new target to a timer on that CPU, and that timer can only
> fire through the broadcast interrupt, which is not enabled yet. The
> system stalls.
>
> Tested on a K3 COM260 board by offlining the CPU owning the broadcast
> interrupt, and then each CPU in turn, under a timerfd load:
> - v7.4-rc1 with the SpacemiT SoC timer series and local timers forced
>   to stop in idle
> - a vendor 6.18 kernel with SBI cpuidle states that stop the local timer
>
> Both stall without this patch and pass with it.
> ---
>  drivers/irqchip/Kconfig                 |  1 +
>  drivers/irqchip/irq-riscv-imsic-state.c | 19 +++++++++++++------
>  drivers/irqchip/irq-riscv-imsic-state.h |  4 ++++
>  3 files changed, 18 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index 136bab41f801..5cdc3f52198c 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -651,6 +651,7 @@ config RISCV_IMSIC
>         bool
>         depends on RISCV
>         select IRQ_DOMAIN_HIERARCHY
> +       select IRQ_WORK if SMP
>         select GENERIC_IRQ_MATRIX_ALLOCATOR
>         select GENERIC_MSI_IRQ
>         select IRQ_MSI_LIB
> diff --git a/drivers/irqchip/irq-riscv-imsic-state.c b/drivers/irqchip/irq-riscv-imsic-state.c
> index 01620373b43a..f6373e4019a2 100644
> --- a/drivers/irqchip/irq-riscv-imsic-state.c
> +++ b/drivers/irqchip/irq-riscv-imsic-state.c
> @@ -252,6 +252,11 @@ void imsic_local_delivery(bool enable)
>  }
>
>  #ifdef CONFIG_SMP
> +static void imsic_local_sync_work(struct irq_work *work)
> +{
> +       imsic_local_sync_all(false);
> +}
> +
>  static void imsic_local_timer_callback(struct timer_list *timer)
>  {
>         imsic_local_sync_all(false);
> @@ -268,18 +273,19 @@ static void __imsic_remote_sync(struct imsic_local_priv *lpriv, unsigned int cpu
>          */
>
>         /*
> -        * We schedule a timer on the target CPU if the target CPU is not
> -        * same as the current CPU. An offline CPU will unconditionally
> -        * synchronize IDs through imsic_starting_cpu() when the
> -        * CPU is brought up.
> +        * A remote CPU may have stopped its local timer and be waiting for
> +        * the broadcast interrupt whose IMSIC state needs updating. Use an
> +        * IPI-backed work item so enabling that interrupt does not depend on
> +        * its delivery. An offline CPU synchronizes IDs in imsic_starting_cpu().
>          */
>         if (cpu_online(cpu)) {
>                 if (cpu == smp_processor_id()) {
>                         if (__imsic_local_sync(lpriv))
>                                 return;
> +                       __imsic_local_timer_start(lpriv, cpu);
> +               } else {
> +                       irq_work_queue_on(&lpriv->sync_work, cpu);
>                 }
> -
> -               __imsic_local_timer_start(lpriv, cpu);
>         }
>  }
>  #else
> @@ -510,6 +516,7 @@ static int __init imsic_local_init(void)
>  #ifdef CONFIG_SMP
>                 /* Setup lazy timer for synchronization */
>                 timer_setup(&lpriv->timer, imsic_local_timer_callback, TIMER_PINNED);
> +               lpriv->sync_work = IRQ_WORK_INIT_HARD(imsic_local_sync_work);
>  #endif
>
>                 /* Allocate vector array */
> diff --git a/drivers/irqchip/irq-riscv-imsic-state.h b/drivers/irqchip/irq-riscv-imsic-state.h
> index 189829b8673a..a36b9f0ef840 100644
> --- a/drivers/irqchip/irq-riscv-imsic-state.h
> +++ b/drivers/irqchip/irq-riscv-imsic-state.h
> @@ -9,6 +9,7 @@
>
>  #include <linux/irqchip/riscv-imsic.h>
>  #include <linux/irqdomain.h>
> +#include <linux/irq_work.h>
>  #include <linux/fwnode.h>
>  #include <linux/timer.h>
>
> @@ -34,6 +35,9 @@ struct imsic_local_priv {
>         unsigned long                           *dirty_bitmap;
>
>  #ifdef CONFIG_SMP
> +       /* Remote synchronization must not depend on a timer interrupt. */
> +       struct irq_work                         sync_work;
> +
>         /* Local timer for synchronization */
>         struct timer_list                       timer;
>  #endif
>
> ---
> base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4
> change-id: 20261103-imsic-remote-sync-irqwork-9cfec6c410de
>
> Best regards,
> --
> Troy Mitchell <troy.mitchell@linux.dev>
>

Regards,
Anup