Re: [PATCH] PCI: let pci_request_irq properly deal with threaded interrupts

[Thomas Gleixner]

On Wed, 1 Aug 2018, Heiner Kallweit wrote:
> On 31.07.2018 08:15, Marc Zyngier wrote:
> > On Mon, 30 Jul 2018 23:36:57 +0100,
> > Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:
> >>
> >> Yes, request for pure threaded interrupts are rejected if the oneshot flag
> >> is not set. The reason is that this would be deadly especially with level
> >> triggered interrupts because the primary default handler just wakes the
> >> thread and then reenables interrupts, which will make the interrupt come
> >> back immediately and the thread won't have a chance to actually shut it up
> >> in the device.
> >>
> >> That made me look into that code again and I found that we added a flag for
> >> irq chips to tell the core that the interrupt is one shot safe, i.e. that
> >> it can be requested w/o IRQF_ONESHOT. That was initially added to optimize
> >> MSI based interrupts which are oneshot safe by implementation.
> >>
> >>   dc9b229a58dc ("genirq: Allow irq chips to mark themself oneshot safe")
> >>
> >> The original patch added that flag to the x86 MSI irqchip code, but that
> >> part was not applied for reasons which slipped from memory. It might be
> >> worthwhile to revisit that in order to avoid the mask/unmask overhead for
> >> such cases.
> > 
> > Yup, that would actually be beneficial to a range of interrupt
> > controllers (only an obscure GPIO driver makes use of this flag).
> > 
> 
> I'm not an expert on that area, but if MSI is oneshot-safe in general,
> then we could do the following in the framework instead of touching
> every MSI irq_chip. Opinions?
> I tested on X86 and at least my system is still running.
> 
> diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c
> index 4ca2fd46..ba6da943 100644
> --- a/kernel/irq/msi.c
> +++ b/kernel/irq/msi.c
> @@ -289,6 +289,9 @@ struct irq_domain *msi_create_irq_domain(struct fwnode_handle *fwnode,
>         if (info->flags & MSI_FLAG_USE_DEF_CHIP_OPS)
>                 msi_domain_update_chip_ops(info);
> 
> +       /* MSI is oneshot-safe in general */
> +       info->chip->flags |= IRQCHIP_ONESHOT_SAFE;
> +
>         domain = irq_domain_create_hierarchy(parent, IRQ_DOMAIN_FLAG_MSI, 0,

Looks about right, though there might be dragons. MSI is not always as sane
as it should be...

> > 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).
> > 
> 
> Would it be sufficient if one irq_chip in the hierarchy is oneshot-safe?
> Or what do we have to check for?

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.

Thanks,

	tglx