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

On 7/28/2026 7:33 PM, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] The patch replaces the direct invocation of the outermost chip's irq_retrigger() callback with irq_chip_retrigger_hierarchy(). However, irq_chip_retrigger_hierarchy() initializes its loop with `data = data->parent_data`, completely skipping the outermost chip. This breaks interrupt retriggering for flat domains (like native lapic_controller) and any hierarchical domain where the outermost chip directly implements the retrigger callback.
> --
> 
> --- Patch [2]: [PATCH 2/3] x86/irq: Use irq_chip_retrigger_hierarchy() in fixup_irqs() ---
> commit f97e883de2f6626561de48c426194caa41c3e062
> Author: Naman Jain <[email protected]>
> x86/irq: Use irq_chip_retrigger_hierarchy() in fixup_irqs()
> 
> This commit modifies fixup_irqs() to use irq_chip_retrigger_hierarchy()
> instead of invoking the outermost domain chip's irq_retrigger() callback
> directly. It also refactors the loop to use scoped guards for the irq
> descriptor lock.
> 
>> 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
> [ ... ]
>> @@ -489,22 +484,18 @@ void fixup_irqs(void)
> [ ... ]
>> -	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))
> [Severity: High]
> Will this skip the outermost chip and cause a regression for flat domains?
> 
> Looking at kernel/irq/chip.c:irq_chip_retrigger_hierarchy(), the loop
> initializes by immediately moving to the parent_data:
> 
>      int irq_chip_retrigger_hierarchy(struct irq_data *data)
>      {
>          for (data = data->parent_data; data; data = data->parent_data)
>              if (data->chip && data->chip->irq_retrigger)
>                  return data->chip->irq_retrigger(data);
> 
>          return 0;
>      }
> 
> By passing &desc->irq_data directly to this function, it seems any retrigger
> callback on the outermost chip itself will be bypassed.
> 
> Could this lead to dropped hardware interrupts during CPU hot-unplug for
> flat domains (like native lapic_controller) or any hierarchical domain where
> the outermost chip implements the retrigger callback?
> 
>>   				__this_cpu_write(vector_irq[vector], VECTOR_RETRIGGERED);
>> -			}
>> -			raw_spin_unlock(&desc->lock);
>>   		}
> 

I think this is a false positive, considering we are not changing the 
behavior for any of the existing chip->irq_retrigger(data) which come in 
this path (irq_chip_retrigger_hierarchy()).

Regards,
Naman