On 8/18/2026 10:40 AM, Michael Kelley wrote:
From: Naman Jain <[email protected]> Sent: Monday, August 10, 2026
2:07 AM
fixup_irqs() re-injects a pending interrupt on its new target CPU by
looking at the outermost domain chip and invoking its irq_retrigger()
callback directly. That only works when the outermost chip happens to
install an irq_retrigger() callback, which is not guaranteed for every
irqchip and could lead to lost interrupts on CPU hot-unplug.
Use irq_chip_retrigger_hierarchy() instead, which walks up the interrupt
hierarchy until it finds a chip that implements irq_retrigger().
While at it, move the loop-local variables into the loop scope and use a
scoped guard for desc->lock.
Moving the loop-local variables into the loop scope seems to be churn
with no real benefit. I know Thomas' suggested code made that change
and he's the maintainer, so it's his call. But still . . .
Changing to use the scoped guard seems OK since that's a new pattern
we want to be using.
I can avoid moving this to loop-local variable in next version, if we
are moving ahead with changing the logic for below comments.
VECTOR_RETRIGGERED is now set only when the retrigger succeeds
(irq_chip_retrigger_hierarchy() returns non-zero) instead of
unconditionally. This is harmless today since apic_retrigger_irq()
always returns 1, and arguably more correct. No functional change
intended for chips which already provide an irq_retrigger() callback
on the outermost domain.
Suggested-by: Thomas Gleixner <[email protected]>
Reviewed-by: Shradha Gupta <[email protected]>
Signed-off-by: Naman Jain <[email protected]>
---
arch/x86/kernel/irq.c | 23 +++++++----------------
1 file changed, 7 insertions(+), 16 deletions(-)
diff --git a/arch/x86/kernel/irq.c b/arch/x86/kernel/irq.c
index 30122f0b3af96..ef1bdd3c4659a 100644
--- a/arch/x86/kernel/irq.c
+++ b/arch/x86/kernel/irq.c
@@ -466,11 +466,6 @@ DEFINE_IDTENTRY_SYSVEC(sysvec_posted_msi_notification)
/* A cpu has been removed from cpu_online_mask. Reset irq affinities. */
void fixup_irqs(void)
{
- unsigned int vector;
- struct irq_desc *desc;
- struct irq_data *data;
- struct irq_chip *chip;
-
irq_migrate_all_off_this_cpu();
/*
@@ -489,22 +484,18 @@ void fixup_irqs(void)
* vector_lock because the cpu is already marked !online, so
* nothing else will touch it.
*/
- for (vector = FIRST_EXTERNAL_VECTOR; vector < NR_VECTORS; vector++) {
- if (IS_ERR_OR_NULL(__this_cpu_read(vector_irq[vector])))
+ for (unsigned int vector = FIRST_EXTERNAL_VECTOR; vector < NR_VECTORS;
vector++) {
+ struct irq_desc *desc = __this_cpu_read(vector_irq[vector]);
+
+ if (IS_ERR_OR_NULL(desc))
continue;
if (is_vector_pending(vector)) {
- desc = __this_cpu_read(vector_irq[vector]);
-
- raw_spin_lock(&desc->lock);
- data = irq_desc_get_irq_data(desc);
- chip = irq_data_get_irq_chip(data);
- if (chip->irq_retrigger) {
- chip->irq_retrigger(data);
+ guard(raw_spinlock)(&desc->lock);
+ if (irq_chip_retrigger_hierarchy(&desc->irq_data))
The existing code uses irq_desc_get_irq_data() instead of directly
accessing the irq_data field. I *think* using the function is preferred,
but maybe I'm out-of-date.
I can switch to irq_desc_get_irq_data().
static inline struct irq_data *irq_desc_get_irq_data(struct irq_desc *desc)
{
return &desc->irq_data;
}
Sashiko pointed out that irq_chip_retrigger_hierarchy() doesn't check
the outermost domain -- it immediately goes the parent. In v1 of this patch
series, you categorized this as a false positive. But I don’t understand
your explanation. Even if there aren't currently any outermost domains
with a custom retrigger function, there could be at some point in the
future. So it seems wrong to skip it. But maybe I'm missing something.
Could you elaborate on your reasoning?
Michael
irq_chip_retrigger_hierarchy() deliberately skips the chip passed to it
and begins with its parent, i.e. apic_retrigger_irq(). The new code was
doing the same. However, if someone adds a custom .irq_retrigger, hoping
that it would get executed, it would not.
Sashiko's example was valid in principle, but it is not relevant to
these device interrupts because desc->irq_data is the outer
MSI/IOAPIC/Hyper-V chip. The LAPIC data is its parent, not the starting
data. I found no device IRQ in this path whose descriptor starts
directly at lapic_controller.
I am all in for adding this as a fallback -
+ if (chip->irq_retrigger)
+ ret = chip->irq_retrigger(data);
+ else
+ ret = irq_chip_retrigger_hierarchy(data);
This should solve this problem while still covering the chips which can
forget to add a retrigger function.
Please let me know if this looks good to you.
__this_cpu_write(vector_irq[vector],
VECTOR_RETRIGGERED);
- }
- raw_spin_unlock(&desc->lock);
}
+
if (__this_cpu_read(vector_irq[vector]) != VECTOR_RETRIGGERED)
__this_cpu_write(vector_irq[vector], VECTOR_UNUSED);
}
--
2.43.0
Regards,
Naman
