[PATCH] genirq: Consider domain hierarchy when checking for IRQCHIP_ONESHOT_SAFE

[PATCH] genirq: Consider domain hierarchy when checking for IRQCHIP_ONESHOT_SAFE

[Heiner Kallweit]

In case of a domain hierarchy we may miss the IRQCHIP_ONESHOT_SAFE
flag because we look at top of the stack only. See also discussion
here: https://marc.info/?l=linux-kernel&m=153301773524685&w=2

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
Uups, PATCH tag was missing.
---
 kernel/irq/manage.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index a66c58f9..1a28f068 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1156,6 +1156,16 @@ setup_irq_thread(struct irqaction *new, unsigned int irq, bool secondary)
 	return 0;
 }
 
+static bool irq_data_oneshot_safe(struct irq_data *data)
+{
+#ifdef	CONFIG_IRQ_DOMAIN_HIERARCHY
+	/* check topmost irq_chip only */
+	while (data->parent_data)
+		data = data->parent_data;
+#endif
+	return !!(data->chip->flags & IRQCHIP_ONESHOT_SAFE);
+}
+
 /*
  * Internal function to register an irqaction - typically used to
  * allocate special interrupts that are part of the architecture.
@@ -1243,7 +1253,7 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 	 * chip flags, so we can avoid the unmask dance at the end of
 	 * the threaded handler for those.
 	 */
-	if (desc->irq_data.chip->flags & IRQCHIP_ONESHOT_SAFE)
+	if (irq_data_oneshot_safe(&desc->irq_data))
 		new->flags &= ~IRQF_ONESHOT;
 
 	/*
-- 
2.18.0

Re: [PATCH] genirq: Consider domain hierarchy when checking for IRQCHIP_ONESHOT_SAFE

[Heiner Kallweit]

On 03.08.2018 22:00, Thomas Gleixner wrote:
> On Fri, 3 Aug 2018, Heiner Kallweit wrote:
> 
>> In case of a domain hierarchy we may miss the IRQCHIP_ONESHOT_SAFE
>> flag because we look at top of the stack only. See also discussion
>> here: https://marc.info/?l=linux-kernel&m=153301773524685&w=2
> 
> I think you misunderstood:
> 
>> I think the top most chip is the key, the rest of the hierarchy is
>> irrelevant because the top most chip is the one which is responsible for
>> not creating an interrupt storm after the interrupt got acknowledged.
> 
> The top most chip in the hierarchy, e.g. the PCI MSI one, is the key. If
> that one is badly implemented and starts to resend after ack/eoi then the
> interrupt storm happens. The lower layers in the hierarchy down to the
> vector domain are just transporting what the top level chip does. So it is
> actively wrong to flag the lower layers.
> 
>  	 if (desc->irq_data.chip->flags & IRQCHIP_ONESHOT_SAFE)
> 
> is the correct check as it looks at the topmost irq chip which is the one
> which initiates the interrupt.
> 

So you're saying it's correct as it is now in __setup_irq(). Then I don't
really understand Marc's following comment from earlier in the discussion.
What else needs to be done if it is correct already?

"We could also consider extending this to support interrupt
hierarchies, as __setup_irq() seems only concerned with the top of the
stack (an IRQ provided by a generic MSI stack and backed by an irqchip
providing IRQCHIP_ONESHOT_SAFE would go unnoticed)."

> Thanks,
> 
> 	tglx
> 

Re: [PATCH] genirq: Consider domain hierarchy when checking for IRQCHIP_ONESHOT_SAFE

[Thomas Gleixner]

On Fri, 3 Aug 2018, Heiner Kallweit wrote:
> On 03.08.2018 22:00, Thomas Gleixner wrote:
> > On Fri, 3 Aug 2018, Heiner Kallweit wrote:
> > 
> >> In case of a domain hierarchy we may miss the IRQCHIP_ONESHOT_SAFE
> >> flag because we look at top of the stack only. See also discussion
> >> here: https://marc.info/?l=linux-kernel&m=153301773524685&w=2
> > 
> > I think you misunderstood:
> > 
> >> I think the top most chip is the key, the rest of the hierarchy is
> >> irrelevant because the top most chip is the one which is responsible for
> >> not creating an interrupt storm after the interrupt got acknowledged.
> > 
> > The top most chip in the hierarchy, e.g. the PCI MSI one, is the key. If
> > that one is badly implemented and starts to resend after ack/eoi then the
> > interrupt storm happens. The lower layers in the hierarchy down to the
> > vector domain are just transporting what the top level chip does. So it is
> > actively wrong to flag the lower layers.
> > 
> >  	 if (desc->irq_data.chip->flags & IRQCHIP_ONESHOT_SAFE)
> > 
> > is the correct check as it looks at the topmost irq chip which is the one
> > which initiates the interrupt.
> > 
> 
> So you're saying it's correct as it is now in __setup_irq(). Then I don't
> really understand Marc's following comment from earlier in the discussion.
> What else needs to be done if it is correct already?
> 
> "We could also consider extending this to support interrupt
> hierarchies, as __setup_irq() seems only concerned with the top of the
> stack (an IRQ provided by a generic MSI stack and backed by an irqchip
> providing IRQCHIP_ONESHOT_SAFE would go unnoticed)."

I think Marc is wrong there, but I'm not sure what he had in mind.

Thanks,

	tglx