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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] `irq_chip_retrigger_hierarchy()` skips the outermost interrupt chip, breaking retriggering for interrupts where the top-level chip directly implements `.irq_retrigger` (e.g., single-level domains).
--

--- Patch [2]: [PATCH v2 2/3] x86/irq: Use irq_chip_retrigger_hierarchy() in fixup_irqs() ---
commit 51de6b45bc61b20c750e5760473230c207ce5194
Author: Naman Jain <[email protected]>

x86/irq: Use irq_chip_retrigger_hierarchy() in fixup_irqs()

This commit updates fixup_irqs() to use a scoped guard for the
irq_desc lock and replaces the direct invocation of the outermost
chip's irq_retrigger() callback with irq_chip_retrigger_hierarchy().

> diff --git a/arch/x86/kernel/irq.c b/arch/x86/kernel/irq.c
> --- a/arch/x86/kernel/irq.c
> +++ b/arch/x86/kernel/irq.c
[ ... ]
> @@ -489,22 +484,18 @@ void fixup_irqs(void)
[ ... ]
>  		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]
Does this skip the outermost interrupt chip for single-level domains?

Looking at irq_chip_retrigger_hierarchy() in kernel/irq/chip.c, the loop
initialization immediately advances 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;
}

For domains where the outermost chip directly implements the retrigger 
callback (such as legacy interrupts in x86_vector_domain), data->parent_data 
will be NULL or lack the retrigger logic. Will this cause the hierarchy walk 
to just return 0 and silently drop the pending interrupt during CPU hot-unplug?

The generic try_retrigger() in kernel/irq/resend.c handles this by explicitly 
checking desc->irq_data.chip->irq_retrigger before falling back to the 
hierarchy walk. Should fixup_irqs() follow that same pattern to avoid skipping
the top-level chip?

>  				__this_cpu_write(vector_irq[vector], VECTOR_RETRIGGERED);
> -			}
> -			raw_spin_unlock(&desc->lock);
>  		}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2