| Message ID | 20210721192650.687529735@linutronix.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | PCI/MSI, x86: Cure a couple of inconsistencies | expand |
On Wed, 21 Jul 2021 20:11:32 +0100, Thomas Gleixner <tglx@linutronix.de> wrote: > > X86 IO/APIC and MSI interrupts (when used without interrupts remapping) > require that the affinity setup on startup is done before the interrupt is > enabled for the first time as the non-remapped operation mode cannot safely > migrate enabled interrupts from arbitrary contexts. Provide a new irq chip > flag which allows affected hardware to request this. > > This has to be opt-in because there have been reports in the past that some > interrupt chips cannot handle affinity setting before startup. > > Fixes: 18404756765c ("genirq: Expose default irq affinity mask (take 3)") > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Marc Zyngier <maz@kernel.org> > Cc: Ingo Molnar <mingo@kernel.org> > --- > include/linux/irq.h | 2 ++ > kernel/irq/chip.c | 5 ++++- > 2 files changed, 6 insertions(+), 1 deletion(-) > > --- a/include/linux/irq.h > +++ b/include/linux/irq.h > @@ -569,6 +569,7 @@ struct irq_chip { > * IRQCHIP_SUPPORTS_NMI: Chip can deliver NMIs, only for root irqchips > * IRQCHIP_ENABLE_WAKEUP_ON_SUSPEND: Invokes __enable_irq()/__disable_irq() for wake irqs > * in the suspend path if they are in disabled state > + * IRQCHIP_AFFINITY_PRE_STARTUP: Default affinity update before startup > */ > enum { > IRQCHIP_SET_TYPE_MASKED = (1 << 0), > @@ -581,6 +582,7 @@ enum { > IRQCHIP_SUPPORTS_LEVEL_MSI = (1 << 7), > IRQCHIP_SUPPORTS_NMI = (1 << 8), > IRQCHIP_ENABLE_WAKEUP_ON_SUSPEND = (1 << 9), > + IRQCHIP_AFFINITY_PRE_STARTUP = (1 << 10), > }; > > #include <linux/irqdesc.h> > --- a/kernel/irq/chip.c > +++ b/kernel/irq/chip.c > @@ -265,8 +265,11 @@ int irq_startup(struct irq_desc *desc, b > } else { > switch (__irq_startup_managed(desc, aff, force)) { > case IRQ_STARTUP_NORMAL: > + if (d->chip->flags & IRQCHIP_AFFINITY_PRE_STARTUP) > + irq_setup_affinity(desc); How about moving this to activate instead? We already special-case the activation of MSIs for PCI (MSI_FLAG_ACTIVATE_EARLY), and this wouldn't look completely out of place. The startup mode could be an issue though... > ret = __irq_startup(desc); > - irq_setup_affinity(desc); > + if (!(d->chip->flags & IRQCHIP_AFFINITY_PRE_STARTUP)) > + irq_setup_affinity(desc); > break; > case IRQ_STARTUP_MANAGED: > irq_do_set_affinity(d, aff, false); Otherwise, looks good. M.
On Thu, Jul 22 2021 at 16:12, Marc Zyngier wrote: > On Wed, 21 Jul 2021 20:11:32 +0100, > Thomas Gleixner <tglx@linutronix.de> wrote: >> #include <linux/irqdesc.h> >> --- a/kernel/irq/chip.c >> +++ b/kernel/irq/chip.c >> @@ -265,8 +265,11 @@ int irq_startup(struct irq_desc *desc, b >> } else { >> switch (__irq_startup_managed(desc, aff, force)) { >> case IRQ_STARTUP_NORMAL: >> + if (d->chip->flags & IRQCHIP_AFFINITY_PRE_STARTUP) >> + irq_setup_affinity(desc); > > How about moving this to activate instead? We already special-case the > activation of MSIs for PCI (MSI_FLAG_ACTIVATE_EARLY), and this > wouldn't look completely out of place. The startup mode could be an > issue though... Yes, I thought about that, but the ordering here is: setup() early_activate() early activation just needs to program a valid message. Now later we have request_irq() invoking: activate() startup() So, yes. We could do that in activate, but then we still have the post startup variant in irq_startup() which makes the code hard to follow. There is another practical issue. Assume the irq is requested with IRQF_NOAUTOEN, then irq_startup() will be invoked when the driver calls enable_irq(), which might be way later and then the affinity setting might be completely different already. So I rather keep it there. Thanks, tglx
--- a/include/linux/irq.h +++ b/include/linux/irq.h @@ -569,6 +569,7 @@ struct irq_chip { * IRQCHIP_SUPPORTS_NMI: Chip can deliver NMIs, only for root irqchips * IRQCHIP_ENABLE_WAKEUP_ON_SUSPEND: Invokes __enable_irq()/__disable_irq() for wake irqs * in the suspend path if they are in disabled state + * IRQCHIP_AFFINITY_PRE_STARTUP: Default affinity update before startup */ enum { IRQCHIP_SET_TYPE_MASKED = (1 << 0), @@ -581,6 +582,7 @@ enum { IRQCHIP_SUPPORTS_LEVEL_MSI = (1 << 7), IRQCHIP_SUPPORTS_NMI = (1 << 8), IRQCHIP_ENABLE_WAKEUP_ON_SUSPEND = (1 << 9), + IRQCHIP_AFFINITY_PRE_STARTUP = (1 << 10), }; #include <linux/irqdesc.h> --- a/kernel/irq/chip.c +++ b/kernel/irq/chip.c @@ -265,8 +265,11 @@ int irq_startup(struct irq_desc *desc, b } else { switch (__irq_startup_managed(desc, aff, force)) { case IRQ_STARTUP_NORMAL: + if (d->chip->flags & IRQCHIP_AFFINITY_PRE_STARTUP) + irq_setup_affinity(desc); ret = __irq_startup(desc); - irq_setup_affinity(desc); + if (!(d->chip->flags & IRQCHIP_AFFINITY_PRE_STARTUP)) + irq_setup_affinity(desc); break; case IRQ_STARTUP_MANAGED: irq_do_set_affinity(d, aff, false);
X86 IO/APIC and MSI interrupts (when used without interrupts remapping) require that the affinity setup on startup is done before the interrupt is enabled for the first time as the non-remapped operation mode cannot safely migrate enabled interrupts from arbitrary contexts. Provide a new irq chip flag which allows affected hardware to request this. This has to be opt-in because there have been reports in the past that some interrupt chips cannot handle affinity setting before startup. Fixes: 18404756765c ("genirq: Expose default irq affinity mask (take 3)") Signed-off-by: Thomas Gleixner <tglx@linutronix.de> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Marc Zyngier <maz@kernel.org> Cc: Ingo Molnar <mingo@kernel.org> --- include/linux/irq.h | 2 ++ kernel/irq/chip.c | 5 ++++- 2 files changed, 6 insertions(+), 1 deletion(-)