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