[PATCH] genirq: Deduplicate WARN_ON_ONCE() in generic_handle_domain_irq()

[PATCH] genirq: Deduplicate WARN_ON_ONCE() in generic_handle_domain_irq()

[Lukas Wunner]

Since commit 0953fb263714 ("irq: remove handle_domain_{irq,nmi}()"),
generic_handle_domain_irq() warns if called outside hardirq context,
even though the function calls down to handle_irq_desc(), which warns
about the same.

Moreover the newly added warning is false positive if the interrupt
originates from any other irqchip than x86 APIC or arm GIC/GICv3.
Those are the only ones for which handle_enforce_irqctx() returns true.
Per commit c16816acd086 ("genirq: Add protection against unsafe usage of
generic_handle_irq()"):

 "In general calling generic_handle_irq() with interrupts disabled from
  non interrupt context is harmless.  [But for] interrupt controllers
  like the x86 trainwrecks this is outright dangerous as it might
  corrupt state if an interrupt affinity change is pending."

An example for irqchips where the warning is false positive are
USB-attached GPIO controllers such as drivers/gpio/gpio-dln2.c:
USB gadgets are incapable of directly signaling an interrupt because
they cannot initiate a bus transaction by themselves.  All communication
on the bus is initiated by the host controller, which polls a gadget's
Interrupt Endpoint in regular intervals.  If an interrupt is pending,
that information is passed up the stack in softirq context, from which
a hardirq is synthesized via generic_handle_domain_irq().

Deduplicate the warning to eliminate false positives and speed up IRQ
handling.

Fixes: 0953fb263714 ("irq: remove handle_domain_{irq,nmi}()")
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Cc: stable@vger.kernel.org # v5.16+
Cc: Mark Rutland <mark.rutland@arm.com>
---
 kernel/irq/irqdesc.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
index 939d21c..da3e495 100644
--- a/kernel/irq/irqdesc.c
+++ b/kernel/irq/irqdesc.c
@@ -699,7 +699,6 @@ int generic_handle_irq_safe(unsigned int irq)
  */
 int generic_handle_domain_irq(struct irq_domain *domain, unsigned int hwirq)
 {
-	WARN_ON_ONCE(!in_hardirq());
 	return handle_irq_desc(irq_resolve_mapping(domain, hwirq));
 }
 EXPORT_SYMBOL_GPL(generic_handle_domain_irq);
-- 
2.35.2

Re: [PATCH] genirq: Deduplicate WARN_ON_ONCE() in generic_handle_domain_irq()

[Sebastian Andrzej Siewior]

On 2022-05-10 09:56:05 [+0200], Lukas Wunner wrote:
> An example for irqchips where the warning is false positive are
> USB-attached GPIO controllers such as drivers/gpio/gpio-dln2.c:

They are not false positives because…

> USB gadgets are incapable of directly signaling an interrupt because
> they cannot initiate a bus transaction by themselves.  All communication
> on the bus is initiated by the host controller, which polls a gadget's
> Interrupt Endpoint in regular intervals.  If an interrupt is pending,
> that information is passed up the stack in softirq context, from which
> a hardirq is synthesized via generic_handle_domain_irq().

they tell you that the context is wrong. From looking at gpio-dln2
this is called from USB URB's callback which is softirq. In the end
dln2_gpio_event() is invoked while dln2_dev::event_cb_lock is acquired.
That lock is acquired by disabling interrupts which is what gets the
locking right for generic_handle_domain_irq(). If that lock lifted to
spin_lock_bh() (because it is always in urb's calback context and all
HCDs complete in one context unlike now) then this breaks. And
PREEMPT_RT is broken already. Therefore, last week, I've been promoting
generic_handle_domain_irq_safe()
   https://lkml.kernel.org/r/YnkfWFzvusFFktSt@linutronix.de

and sadly I missed dln2. Please let me know if you have more users
similar to dln2. I will add those to my list once upstream buys that
interface.

Sebastian

Re: [PATCH] genirq: Deduplicate WARN_ON_ONCE() in generic_handle_domain_irq()

[Thomas Gleixner]

On Mon, May 16 2022 at 08:29, Sebastian Andrzej Siewior wrote:

> On 2022-05-10 09:56:05 [+0200], Lukas Wunner wrote:
>> An example for irqchips where the warning is false positive are
>> USB-attached GPIO controllers such as drivers/gpio/gpio-dln2.c:
>
> They are not false positives because…
>
>> USB gadgets are incapable of directly signaling an interrupt because
>> they cannot initiate a bus transaction by themselves.  All communication
>> on the bus is initiated by the host controller, which polls a gadget's
>> Interrupt Endpoint in regular intervals.  If an interrupt is pending,
>> that information is passed up the stack in softirq context, from which
>> a hardirq is synthesized via generic_handle_domain_irq().
>
> they tell you that the context is wrong.

Why? These handlers can be called from any context, really. Yes. They
need to be called with interrupts disabled, but that's it. The warning
is checking hard interrupt context unconditionally.

> From looking at gpio-dln2 this is called from USB URB's callback which
> is softirq. In the end dln2_gpio_event() is invoked while
> dln2_dev::event_cb_lock is acquired.  That lock is acquired by
> disabling interrupts which is what gets the locking right for
> generic_handle_domain_irq(). If that lock lifted to spin_lock_bh()
> (because it is always in urb's calback context and all HCDs complete
> in one context unlike now) then this breaks.

Yes, but that's a different problem.

> And PREEMPT_RT is broken already. Therefore, last week, I've been
> promoting generic_handle_domain_irq_safe()
> https://lkml.kernel.org/r/YnkfWFzvusFFktSt@linutronix.de

Well, that's just a wrapper which adds the local_irq_save(), so it's not
any different from having the local_irq_save() at the callsite, unless
I'm missing something.

Thanks,

        tglx

Re: [PATCH] genirq: Deduplicate WARN_ON_ONCE() in generic_handle_domain_irq()

[Sebastian Andrzej Siewior]

On 2022-05-16 08:53:29 [+0200], Thomas Gleixner wrote:
> > they tell you that the context is wrong.
> 
> Why? These handlers can be called from any context, really. Yes. They
> need to be called with interrupts disabled, but that's it. The warning
> is checking hard interrupt context unconditionally.

correct. If the context is wrong, the interrupts are usually not
disabled.

> > From looking at gpio-dln2 this is called from USB URB's callback which
> > is softirq. In the end dln2_gpio_event() is invoked while
> > dln2_dev::event_cb_lock is acquired.  That lock is acquired by
> > disabling interrupts which is what gets the locking right for
> > generic_handle_domain_irq(). If that lock lifted to spin_lock_bh()
> > (because it is always in urb's calback context and all HCDs complete
> > in one context unlike now) then this breaks.
> 
> Yes, but that's a different problem.
> 
> > And PREEMPT_RT is broken already. Therefore, last week, I've been
> > promoting generic_handle_domain_irq_safe()
> > https://lkml.kernel.org/r/YnkfWFzvusFFktSt@linutronix.de
> 
> Well, that's just a wrapper which adds the local_irq_save(), so it's not
> any different from having the local_irq_save() at the callsite, unless
> I'm missing something.

I haven't seen a local_irq_save() at the callsite. If it is, then it is
not any different.

> Thanks,
> 
>         tglx

Sebastian

Re: [PATCH] genirq: Deduplicate WARN_ON_ONCE() in generic_handle_domain_irq()

[Lukas Wunner]

On Mon, May 16, 2022 at 08:29:50AM +0200, Sebastian Andrzej Siewior wrote:
> From looking at gpio-dln2
> this is called from USB URB's callback which is softirq. In the end
> dln2_gpio_event() is invoked while dln2_dev::event_cb_lock is acquired.
> That lock is acquired by disabling interrupts which is what gets the
> locking right for generic_handle_domain_irq(). If that lock lifted to
> spin_lock_bh() (because it is always in urb's calback context and all
> HCDs complete in one context unlike now) then this breaks.

I think you want WARN_ON(!irqs_disabled) to catch that,
not WARN_ON_ONCE(!in_hardirq()).


> and sadly I missed dln2. Please let me know if you have more users
> similar to dln2. I will add those to my list once upstream buys that
> interface.

There's another USB GPIO irqchip queued up for 5.19 now:
https://git.kernel.org/netdev/net-next/c/1ce8b37241ed

And there's one more in drivers/hid/hid-cp2112.c, but that calls
handle_nested_irq(), which disables hardirqs.

Thanks,

Lukas