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

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

[Troy Mitchell]

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.

Patch 1 makes remote synchronization use hard IRQ work, which fixes the
stall.

A vector move can still take long to finish. The CPU entering idle
takes over the broadcast interrupt and syncs the new vector before the
old CPU has released the old one, so it arms a retry timer on itself
with its tick already stopped. The move then completes only on its next
interrupt, up to 0.5s later on an idle K3, and affinity changes of the
IRQ return -EBUSY until then. Patch 2 lets the old CPU kick the new CPU
instead and removes the retry timer.

---
Changes in v2:
- Patch 1 unchanged.
- New patch 2: once the old CPU has released the old vector it queues the
  sync work on the new CPU, which then completes the move from its own
  IRQ work. The per-CPU retry timer is dropped, since a move that has to
  wait for the old CPU could only complete on an interrupt driven by a
  timer that may itself be stopped.
- Link to v1: https://patch.msgid.link/20261103-imsic-remote-sync-irqwork-v1-1-8b5c951fc62a@linux.dev

To: Thomas Gleixner <tglx@kernel.org>
To: Radu Rendec <radu@rendec.net>
To: Anup Patel <anup@brainfault.org>
To: Paul Walmsley <pjw@kernel.org>
To: Palmer Dabbelt <palmer@dabbelt.com>
To: Albert Ou <aou@eecs.berkeley.edu>
To: Alexandre Ghiti <alex@ghiti.fr>
To: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
To: Clark Williams <clrkwllms@kernel.org>
To: Steven Rostedt <rostedt@goodmis.org>
To: Björn Töpel <bjorn@rivosinc.com>
Cc: Anup Patel <apatel@ventanamicro.com>
Cc: linux-kernel@vger.kernel.org
Cc: linux-riscv@lists.infradead.org
Cc: linux-rt-devel@lists.linux.dev

---
Troy Mitchell (2):
      irqchip/riscv-imsic: Synchronize remote state with IRQ work
      irqchip/riscv-imsic: Complete vector moves without a timer

 drivers/irqchip/Kconfig                 |  1 +
 drivers/irqchip/irq-riscv-imsic-early.c |  5 +-
 drivers/irqchip/irq-riscv-imsic-state.c | 87 ++++++++++++++-------------------
 drivers/irqchip/irq-riscv-imsic-state.h |  6 +--
 4 files changed, 42 insertions(+), 57 deletions(-)
---
base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4
change-id: 20261103-imsic-remote-sync-irqwork-9cfec6c410de

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

[PATCH v2 1/2] 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>
---
 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

-- 
2.56.0

[PATCH v2 2/2] irqchip/riscv-imsic: Complete vector moves without a timer

[Troy Mitchell]

When a vector moves, the new CPU clears move_prev only after the old CPU
has released the old vector. If the new CPU syncs first, it arms a timer
on itself and retries a jiffy later. That CPU is often the one entering
idle and taking over the broadcast interrupt, with its tick already
stopped, so the timer is not programmed. The move then completes only on
the next interrupt of that CPU, which took up to 0.5s on an idle
SpacemiT K3. Until then the IRQ is still marked as moving and further
affinity changes of it return -EBUSY.

Let the old CPU queue the sync work on the new CPU once it has released
the old vector, and remove the retry timer. Wait only while the old
vector still points to the new vector, since a released old vector may
already belong to another move.

IMSIC synchronization no longer depends on a timer.

Signed-off-by: Troy Mitchell <troy.mitchell@linux.dev>
---
 drivers/irqchip/irq-riscv-imsic-early.c |  5 +-
 drivers/irqchip/irq-riscv-imsic-state.c | 90 +++++++++++++--------------------
 drivers/irqchip/irq-riscv-imsic-state.h |  6 +--
 3 files changed, 37 insertions(+), 64 deletions(-)

diff --git a/drivers/irqchip/irq-riscv-imsic-early.c b/drivers/irqchip/irq-riscv-imsic-early.c
index 06726a062b75..06ee402d3f47 100644
--- a/drivers/irqchip/irq-riscv-imsic-early.c
+++ b/drivers/irqchip/irq-riscv-imsic-early.c
@@ -96,10 +96,7 @@ static void imsic_handle_irq(struct irq_desc *desc)
 	struct irq_chip *chip = irq_desc_get_chip(desc);
 	unsigned long local_id;
 
-	/*
-	 * Process pending local synchronization instead of waiting
-	 * for per-CPU local timer to expire.
-	 */
+	/* Process pending local synchronization first */
 	imsic_local_sync_all(false);
 
 	chained_irq_enter(chip, desc);
diff --git a/drivers/irqchip/irq-riscv-imsic-state.c b/drivers/irqchip/irq-riscv-imsic-state.c
index f6373e4019a2..36f4b7ede76a 100644
--- a/drivers/irqchip/irq-riscv-imsic-state.c
+++ b/drivers/irqchip/irq-riscv-imsic-state.c
@@ -125,11 +125,24 @@ void __imsic_eix_update(unsigned long base_id, unsigned long num_id, bool pend,
 	}
 }
 
-static bool __imsic_local_sync(struct imsic_local_priv *lpriv)
+#ifdef CONFIG_SMP
+static void imsic_remote_kick(unsigned int cpu)
+{
+	struct imsic_local_priv *lpriv = per_cpu_ptr(imsic->lpriv, cpu);
+
+	if (cpu_online(cpu))
+		irq_work_queue_on(&lpriv->sync_work, cpu);
+}
+#else
+static inline void imsic_remote_kick(unsigned int cpu)
+{
+}
+#endif
+
+static void __imsic_local_sync(struct imsic_local_priv *lpriv)
 {
 	struct imsic_local_config *tlocal, *mlocal;
 	struct imsic_vector *vec, *tvec, *mvec;
-	bool ret = true;
 	int i;
 
 	lockdep_assert_held(&lpriv->lock);
@@ -151,13 +164,13 @@ static bool __imsic_local_sync(struct imsic_local_priv *lpriv)
 		mvec = READ_ONCE(vec->move_prev);
 		if (mvec) {
 			/*
-			 * If the old vector has not been updated then
-			 * try again in the next sync-up call.
+			 * If the old vector has not been updated then keep
+			 * this vector dirty. The old CPU kicks this CPU once
+			 * it has updated the old vector. A released old vector
+			 * may already belong to another move.
 			 */
-			if (READ_ONCE(mvec->move_next)) {
-				ret = false;
+			if (READ_ONCE(mvec->move_next) == vec)
 				continue;
-			}
 
 			WRITE_ONCE(vec->move_prev, NULL);
 		}
@@ -199,30 +212,15 @@ static bool __imsic_local_sync(struct imsic_local_priv *lpriv)
 
 			WRITE_ONCE(vec->move_next, NULL);
 			imsic_vector_free(vec);
+
+			/* Let the new CPU finish the move it may be waiting for */
+			imsic_remote_kick(mvec->cpu);
 		}
 
 skip:
 		bitmap_clear(lpriv->dirty_bitmap, i, 1);
 	}
-
-	return ret;
-}
-
-#ifdef CONFIG_SMP
-static void __imsic_local_timer_start(struct imsic_local_priv *lpriv, unsigned int cpu)
-{
-	lockdep_assert_held(&lpriv->lock);
-
-	if (!timer_pending(&lpriv->timer)) {
-		lpriv->timer.expires = jiffies + 1;
-		add_timer_on(&lpriv->timer, cpu);
-	}
 }
-#else
-static inline void __imsic_local_timer_start(struct imsic_local_priv *lpriv, unsigned int cpu)
-{
-}
-#endif
 
 void imsic_local_sync_all(bool force_all)
 {
@@ -233,8 +231,7 @@ void imsic_local_sync_all(bool force_all)
 
 	if (force_all)
 		bitmap_fill(lpriv->dirty_bitmap, imsic->global.nr_ids + 1);
-	if (!__imsic_local_sync(lpriv))
-		__imsic_local_timer_start(lpriv, smp_processor_id());
+	__imsic_local_sync(lpriv);
 
 	raw_spin_unlock_irqrestore(&lpriv->lock, flags);
 }
@@ -257,11 +254,6 @@ 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);
-}
-
 static void __imsic_remote_sync(struct imsic_local_priv *lpriv, unsigned int cpu)
 {
 	lockdep_assert_held(&lpriv->lock);
@@ -273,20 +265,17 @@ static void __imsic_remote_sync(struct imsic_local_priv *lpriv, unsigned int cpu
 	 */
 
 	/*
-	 * 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().
+	 * A CPU that is not the current one is updated through hard IRQ
+	 * work so the update does not depend on a timer interrupt, which
+	 * may itself be the interrupt being updated. A sync that has to
+	 * wait for the old CPU of a vector move is completed when the old
+	 * CPU kicks this CPU. 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);
-		}
-	}
+	if (cpu == smp_processor_id())
+		__imsic_local_sync(lpriv);
+	else
+		imsic_remote_kick(cpu);
 }
 #else
 static void __imsic_remote_sync(struct imsic_local_priv *lpriv, unsigned int cpu)
@@ -514,8 +503,7 @@ static int __init imsic_local_init(void)
 			goto fail_local_cleanup;
 
 #ifdef CONFIG_SMP
-		/* Setup lazy timer for synchronization */
-		timer_setup(&lpriv->timer, imsic_local_timer_callback, TIMER_PINNED);
+		/* Setup IRQ work for remote synchronization */
 		lpriv->sync_work = IRQ_WORK_INIT_HARD(imsic_local_sync_work);
 #endif
 
@@ -557,14 +545,6 @@ void imsic_state_offline(void)
 	raw_spin_lock_irqsave(&imsic->matrix_lock, flags);
 	irq_matrix_offline(imsic->matrix);
 	raw_spin_unlock_irqrestore(&imsic->matrix_lock, flags);
-
-#ifdef CONFIG_SMP
-	struct imsic_local_priv *lpriv = this_cpu_ptr(imsic->lpriv);
-
-	raw_spin_lock_irqsave(&lpriv->lock, flags);
-	WARN_ON_ONCE(timer_delete_sync_try(&lpriv->timer) < 0);
-	raw_spin_unlock_irqrestore(&lpriv->lock, flags);
-#endif
 }
 
 static int __init imsic_matrix_init(void)
diff --git a/drivers/irqchip/irq-riscv-imsic-state.h b/drivers/irqchip/irq-riscv-imsic-state.h
index a36b9f0ef840..0bc4dccac238 100644
--- a/drivers/irqchip/irq-riscv-imsic-state.h
+++ b/drivers/irqchip/irq-riscv-imsic-state.h
@@ -11,7 +11,6 @@
 #include <linux/irqdomain.h>
 #include <linux/irq_work.h>
 #include <linux/fwnode.h>
-#include <linux/timer.h>
 
 #define IMSIC_IPI_ID				1
 
@@ -35,11 +34,8 @@ struct imsic_local_priv {
 	unsigned long				*dirty_bitmap;
 
 #ifdef CONFIG_SMP
-	/* Remote synchronization must not depend on a timer interrupt. */
+	/* IRQ work for synchronization from other CPUs */
 	struct irq_work				sync_work;
-
-	/* Local timer for synchronization */
-	struct timer_list			timer;
 #endif
 
 	/* Local vector table */

-- 
2.56.0

Re: [PATCH v2 2/2] irqchip/riscv-imsic: Complete vector moves without a timer

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] irqchip/riscv-imsic: imsic_remote_kick() in
  __imsic_local_sync() wakes idle CPU
- [Low] irqchip/riscv-imsic: missing Fixes trailer for commit
  0f67911e821c
--

commit 41f6c7fe40c982e58c49f5640f84619e2a018221
Author: Troy Mitchell <troy.mitchell@linux.dev>

> irqchip/riscv-imsic: Complete vector moves without a timer
>
> When a vector moves, the new CPU clears move_prev only after the old CPU
> has released the old vector. If the new CPU syncs first, it arms a timer
> on itself and retries a jiffy later. That CPU is often the one entering
> idle and taking over the broadcast interrupt, with its tick already
> stopped, so the timer is not programmed. The move then completes only on
> the next interrupt of that CPU, which took up to 0.5s on an idle
> SpacemiT K3. Until then the IRQ is still marked as moving and further
> affinity changes of it return -EBUSY.
>
> Let the old CPU queue the sync work on the new CPU once it has released
> the old vector, and remove the retry timer. Wait only while the old
> vector still points to the new vector, since a released old vector may
> already belong to another move.
>
> IMSIC synchronization no longer depends on a timer.
>
> Signed-off-by: Troy Mitchell <troy.mitchell@linux.dev>

[Severity: Low]
This fixes a bug in the two-step vector move synchronization and local
retry timer.  Should it include a Fixes tag?

  Fixes: 0f67911e821c ("irqchip/riscv-imsic: Separate next and previous pointers in IMSIC vector")

> diff --git a/drivers/irqchip/irq-riscv-imsic-state.c b/drivers/irqchip/irq-riscv-imsic-state.c
> index f6373e4019a22..36f4b7ede76a5 100644
> --- a/drivers/irqchip/irq-riscv-imsic-state.c
> +++ b/drivers/irqchip/irq-riscv-imsic-state.c

[ ... ]

> @@ -151,13 +164,13 @@ static bool __imsic_local_sync(struct imsic_local_priv *lpriv)
>  		mvec = READ_ONCE(vec->move_prev);
>  		if (mvec) {
>  			/*
> -			 * If the old vector has not been updated then
> -			 * try again in the next sync-up call.
> +			 * If the old vector has not been updated then keep
> +			 * this vector dirty. The old CPU kicks this CPU once
> +			 * it has updated the old vector. A released old vector
> +			 * may already belong to another move.
>  			 */
> -			if (READ_ONCE(mvec->move_next)) {
> -				ret = false;
> +			if (READ_ONCE(mvec->move_next) == vec)
>  				continue;
> -			}
>  
>  			WRITE_ONCE(vec->move_prev, NULL);
>  		}
> @@ -199,30 +212,15 @@ static bool __imsic_local_sync(struct imsic_local_priv *lpriv)
>  
>  			WRITE_ONCE(vec->move_next, NULL);
>  			imsic_vector_free(vec);
> +
> +			/* Let the new CPU finish the move it may be waiting for */
> +			imsic_remote_kick(mvec->cpu);
>  		}

[Severity: Medium]
Does calling imsic_remote_kick(mvec->cpu) here wake the destination CPU
out of cpuidle right after it takes over the broadcast timer?

When CPU1 enters a timer-stop idle state and moves the broadcast IRQ to
itself via cpuidle_enter_state() -> tick_broadcast_enter(), it runs
__imsic_local_sync() synchronously while CPU0 is still processing its
sync_work IPI:

CPU0                              CPU1
                                  cpuidle_enter_state()
                                    tick_broadcast_enter()
                                      ...
                                        imsic_vector_move(old_vec, new_vec)
                                          imsic_vector_move_update(old_lpriv, ...)
                                            imsic_remote_kick(CPU0)
                                          imsic_vector_move_update(new_lpriv, ...)
                                            __imsic_local_sync(new_lpriv)
                                              /* mvec->move_next == vec */
                                              /* leaves new_vec->move_prev set */
                                    target_state->enter()
imsic_local_sync_work()
  ...
    WRITE_ONCE(vec->move_next, NULL)
    imsic_vector_free(vec)
    imsic_remote_kick(CPU1)       <-- IPI wakes CPU1 from idle

CPU1 has already enabled new_vec->local_id in its local EIE CSR during its
own __imsic_local_sync(), so the IPI wakes CPU1 out of wfi or
riscv_sbi_hart_suspend() solely to clear new_vec->move_prev, forcing it
through tick_broadcast_exit() and another idle entry cycle.

In addition, cpuidle_enter_state() keeps local IRQs disabled on CPU1 from
before tick_broadcast_enter() until after target_state->enter() and
tick_broadcast_exit().  Until CPU1 re-enables IRQs and services the IPI,
new_vec->move_prev remains non-NULL, so if a third CPU enters
tick_broadcast_enter() with an earlier expiry during that window,
imsic_irq_set_affinity() still sees imsic_vector_get_move() != NULL and
returns -EBUSY.

Finally, when imsic_vector_move() runs on old_vec->cpu (such as from
tick_handle_oneshot_broadcast() or imsic_irq_ack() -> irq_move_irq()), the
first imsic_vector_move_update(old_lpriv, ...) runs __imsic_local_sync()
synchronously and calls imsic_remote_kick(new_vec->cpu) before the second
imsic_vector_move_update(new_lpriv, ...) has set new_vec->move_prev or
marked new_vec dirty, kicking new_vec->cpu twice.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112-imsic-remote-sync-irqwork-v2-0-680f65b24ff6@linux.dev?part=2