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); > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
