Re: [PATCH v2 2/3] x86/irq: Use irq_chip_retrigger_hierarchy() in fixup_irqs()

[Naman Jain <namjain@linux.microsoft.com>]

On 8/18/2026 10:40 AM, Michael Kelley wrote:
> From: Naman Jain <namjain@linux.microsoft.com> 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 <tglx@kernel.org>
>> Reviewed-by: Shradha Gupta <shradhagupta@linux.microsoft.com>
>> Signed-off-by: Naman Jain <namjain@linux.microsoft.com>
>> ---
>>   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