Re: [PATCH net-next v2 5/7] usbnet: smsc95xx: Forward PHY interrupts to PHY driver to avoid polling

[Lukas Wunner <lukas@wunner.de>]

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@wunner.de> 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@wunner.de> 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@wunner.de/

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