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


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[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

[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


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

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

[Troy Mitchell]

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

On Thu, 12 Nov 2026 08:35:16 +0000, sashiko-bot@kernel.org wrote:
> This fixes a bug in the two-step vector move synchronization and local
> retry timer.  Should it include a Fixes tag?

Yes, will add.

[...]

> Does calling imsic_remote_kick(mvec->cpu) here wake the destination CPU
> out of cpuidle right after it takes over the broadcast timer?
Yes, by design. Only the new CPU clears move_prev, so it has to run
once after the old CPU is done. The retry timer did the same a jiffy
later.

> 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.
The pending IPI ends idle at once, so the window is the idle exit
latency. The broadcast IRQ then stays on CPU1, whose handler still
wakes the third CPU in time.

> kicking new_vec->cpu twice.
irq_work_queue_on() skips a work that is still pending, so this costs
at most one extra IPI.

-- 
Troy Mitchell

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

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

[Thomas Gleixner]

On Thu, Nov 12 2026 at 07:54, Troy Mitchell wrote:
> 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.

When that timer expires _before_ the earliest programmed timer on the
remote CPU then that remote CPU is kicked out of idle to reevaluate.

So your problem description does not make any sense at all.

If the timer code fails to kick that remote CPU out for re-evaluation
then this needs to be fixed and not worked around somewhere else with
dubious claims.

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

What's the problem with that? The notification to cleanup can be
arbitrarily long delayed. See below.

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

That's even more bogus. The old CPU does not queue anything.

The issue with these completely misdesigned interrupt controllers is
that the only safe way to migrate an interrupt is to do the following:

   1) Defer the affinity change to the interrupt context of the original
      target CPU.

   2) When the next interrupt arrives on the original target CPU switch
      the affinity over to the new target CPU (carefully...)

   3) If the interrupt is pending again on the original target CPU after
      switching affinity send an IPI to the new target to kick that
      vector.

   4) When the new target CPU receives the first interrupt (it does not
      matter whether it's from the device or from the IPI in #3) it
      tells the original target CPU to cleanup the stale
      associations. That's what the timer is for.

So the requirement is that the new target CPU has to receive an
interrupt to be able to release the associations on the original target
CPU and to re-enable affinity settings. This can obviously cause
arbitrarily long delays if #3 is not the case as the cleanup has to wait
for #4.

That has absolutely nothing to do with the timer and changing that to an
IPI as that only shortens the cleanup delay from _ONE_ jiffies to almost
instant. Who the hell cares?

Why is that requirement there in the first place?

  Because there neither is a guarantee that the affinity change takes
  place immediately when the update happens nor there is a guarantee
  that an interrupt message which was sent to the old CPU is delivered
  before the pending check in #3 happens. There are arbitrary device
  specific delays between the point where the device reads the target
  message and the point where it is sent and fabrics have delays too.

A lot of people spent a massive amount of time and head scratching to
come up with a solution for that especially for non-maskable MSI
interrupts.

Coming back to your change.

Of course this can be done with an IPI from the _new_ target CPU to
the _old_ target CPU, but only under one condition:

    The IPI vector cannot be higher prioritized than the to be cleaned
    up device vector.

Why?

Contrary to level interrupts which stay asserted until the interrupt
handler resets the condition, edge interrupts (e.g. PCI/MSI[-X]) are
from a device POV fire and forget. There is no guarantee that a device
waits for handler response before sending the next one.

So due to this and the arbitrary hardware (device/fabric) delays you can
end up in a situation where there is an interrupt pending either in the
old target or in the new target after #3 above or on both.

For the on both case assume that the new target CPU wins the race to
handle it and it sends the IPI to the old target CPU, which is currently
in an interupt disabled region. Once the old target CPU re-enables
interrupts it handles the highest priority interrupt first, which is the
IPI. Now the IPI handler detects that the lower priority device interrupt is
pending and retriggers the cleanup IPI on itself ---> livelock.

Sure, you can make that so that it ignores that situation and waits for
the new target CPU to re-issue the clenaup IPI on the next interrupt,
but that's still not solving the arbitrary delay problem, i.e. waiting
for that next interrupt which might take forever....

I'm still puzzled that at least _two_ decades after it was publicly
known that the x86 APIC is a design disaster hardware people still
insisted on making the same mistakes over and over. It's not only RISCV
folks, there are more insanities like that in other architectures...

But please read back on the initial IMSIC discussion which led up to
merging this driver. There is a ton of more context there which I'm not
going to repeat.

Thanks,

        tglx

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

[Thomas Gleixner]

On Thu, Nov 12 2026 at 07:54, Troy Mitchell wrote:
> 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.

When that timer expires _before_ the earliest programmed timer on the
remote CPU then that remote CPU is kicked out of idle to reevaluate.

So your problem description does not make any sense at all.

If the timer code fails to kick that remote CPU out for re-evaluation
then this needs to be fixed and not worked around somewhere else with
dubious claims.

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

What's the problem with that? The notification to cleanup can be
arbitrarily long delayed. See below.

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

That's even more bogus. The old CPU does not queue anything.

The issue with these completely misdesigned interrupt controllers is
that the only safe way to migrate an interrupt is to do the following:

   1) Defer the affinity change to the interrupt context of the original
      target CPU.

   2) When the next interrupt arrives on the original target CPU switch
      the affinity over to the new target CPU (carefully...)

   3) If the interrupt is pending again on the original target CPU after
      switching affinity send an IPI to the new target to kick that
      vector.

   4) When the new target CPU receives the first interrupt (it does not
      matter whether it's from the device or from the IPI in #3) it
      tells the original target CPU to cleanup the stale
      associations. That's what the timer is for.

So the requirement is that the new target CPU has to receive an
interrupt to be able to release the associations on the original target
CPU and to re-enable affinity settings. This can obviously cause
arbitrarily long delays if #3 is not the case as the cleanup has to wait
for #4.

That has absolutely nothing to do with the timer and changing that to an
IPI as that only shortens the cleanup delay from _ONE_ jiffies to almost
instant. Who the hell cares?

Why is that requirement there in the first place?

  Because there neither is a guarantee that the affinity change takes
  place immediately when the update happens nor there is a guarantee
  that an interrupt message which was sent to the old CPU is delivered
  before the pending check in #3 happens. There are arbitrary device
  specific delays between the point where the device reads the target
  message and the point where it is sent and fabrics have delays too.

A lot of people spent a massive amount of time and head scratching to
come up with a solution for that especially for non-maskable MSI
interrupts.

Coming back to your change.

Of course this can be done with an IPI from the _new_ target CPU to
the _old_ target CPU, but only under one condition:

    The IPI vector cannot be higher prioritized than the to be cleaned
    up device vector.

Why?

Contrary to level interrupts which stay asserted until the interrupt
handler resets the condition, edge interrupts (e.g. PCI/MSI[-X]) are
from a device POV fire and forget. There is no guarantee that a device
waits for handler response before sending the next one.

So due to this and the arbitrary hardware (device/fabric) delays you can
end up in a situation where there is an interrupt pending either in the
old target or in the new target after #3 above or on both.

For the on both case assume that the new target CPU wins the race to
handle it and it sends the IPI to the old target CPU, which is currently
in an interupt disabled region. Once the old target CPU re-enables
interrupts it handles the highest priority interrupt first, which is the
IPI. Now the IPI handler detects that the lower priority device interrupt is
pending and retriggers the cleanup IPI on itself ---> livelock.

Sure, you can make that so that it ignores that situation and waits for
the new target CPU to re-issue the clenaup IPI on the next interrupt,
but that's still not solving the arbitrary delay problem, i.e. waiting
for that next interrupt which might take forever....

I'm still puzzled that at least _two_ decades after it was publicly
known that the x86 APIC is a design disaster hardware people still
insisted on making the same mistakes over and over. It's not only RISCV
folks, there are more insanities like that in other architectures...

But please read back on the initial IMSIC discussion which led up to
merging this driver. There is a ton of more context there which I'm not
going to repeat.

Thanks,

        tglx




_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv