On Mon, May 09, 2022 at 09:47:19AM +0100, Marc Zyngier wrote:
> On Fri, 06 May 2022 21:16:47 +0100, Lukas Wunner <lukas@xxxxxxxxx> wrote:
> > On Fri, May 06, 2022 at 11:58:43AM +0100, Marc Zyngier wrote:
> > > On Thu, 05 May 2022 19:53:28 +0100, Lukas Wunner <lukas@xxxxxxxxx> wrote:
> > > > generic_handle_domain_irq() warns unconditionally on !in_irq(),
> > > > unlike handle_irq_desc(), which constrains the warning to
> > > > handle_enforce_irqctx() (i.e. x86 APIC, arm GIC/GICv3).
> > > > Perhaps that's an oversight in generic_handle_domain_irq(),
> > > > unless __irq_resolve_mapping() becomes unsafe outside in_irq()
> > > > for some reason...
> > > >
> > > > In any case the unconditional in_irq() necessitates __irq_enter_raw()
> > > > here.
> > >
> > > Please don't directly use __irq_enter_raw() and similar things
> > > directly in driver code (it doesn't do anything related to RCU, for
> > > example, which could be problematic if used in arbitrary contexts).
> >
> > As I've pointed out above, it seems like an oversight that Mark
> > didn't make the WARN_ON_ONCE() conditional on handle_enforce_irqctx()
> > (as handle_irq_desc() does). Sadly you did not respond to that
> > observation.
>
> When did you make that observation? I can only see an email from you
> being sent *after* the one I am replying to.
I was referring to the above-quoted sentence:
"generic_handle_domain_irq() warns unconditionally on !in_irq(),
unlike handle_irq_desc(), which constrains the warning to
handle_enforce_irqctx() (i.e. x86 APIC, arm GIC/GICv3).
Perhaps that's an oversight in generic_handle_domain_irq(),
unless __irq_resolve_mapping() becomes unsafe outside in_irq()
for some reason..."
Never mind, let's focus on the problem at hand.
It's secondary who said what when.
> > Please clarify whether that is indeed erroneous.
> > Once handle_enforce_irqctx() is added to generic_handle_domain_irq(),
> > there's no need for me to call __irq_enter_raw(). Problem solved.
>
> I don't see it as an oversight. Drivers shouldn't rely on
> architectural quirks, and it is much clearer to simply forbid
> something that cannot be guaranteed across the board, specially for
> something that is as generic as USB.
Whether a warning is warranted is not dependent on the architecture,
but on the irqchip from which an interrupt normally originates:
* Interrupt normally originates from x86 APIC or arm GIC/GICv3,
but is synthesized in non-hardirq context: Warning is warranted.
* Interrupt normally originates from any other top-level irqchip,
such as irq-bcm2836.c, but is synthesized in non-hardirq context:
Warning is a false positive!
* Interrupt is always synthesized in non-hardirq context by a
USB irqchip: Warning is a false positive, regardless whether
the top-level irqchip is x86 APIC, arm GIC/GICv3 or anything else!
> > Should there be a valid reason for the missing handle_enforce_irqctx(),
> > then I propose adding a generic_handle_domain_irq_safe() function which
> > calls __irq_enter_raw() (or probably __irq_enter() to get accounting),
> > thereby resolving your objection to calling __irq_enter_raw() from a
> > driver.
>
> Feel free to submit a patch.
Done:
https://lore.kernel.org/lkml/c3caf60bfa78e5fdbdf483096b7174da65d1813a.1652168866.git.lukas@xxxxxxxxx/
I'm focussing on eliminating the false-positive warning for now.
Introducing a generic_handle_domain_irq_safe() wrapper which alleviates
drivers from calling local_irq_save() can be done in a separate step.
Thanks,
Lukas
