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

[Thomas Gleixner <tglx@xxxxxxxxxxxxx>]

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@xxxxxxxxxxxxx

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