Re: [PATCH net v2 0/1] PHY interruptus horribilis

From: Rafael J. Wysocki
Date: Wed Jun 08 2022 - 07:11:48 EST


On Wed, Jun 8, 2022 at 12:35 PM Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote:
>
> On Wed, Jun 8, 2022 at 11:52 AM Lukas Wunner <lukas@xxxxxxxxx> wrote:
> >
> > Andrew Lunn (PHY maintainer) asked me to resend this patch and cc the
> > IRQ maintainer. I'm also cc'ing PM maintainers for good measure.
> >
> > The patch addresses an issue with PHY interrupts occurring during a
> > system sleep transition after the PHY has already been suspended.
> >
> > The IRQ subsystem uses an internal flag IRQD_WAKEUP_ARMED to avoid
> > handling such interrupts, but it's not set until suspend_device_irqs()
> > is called during the ->suspend_noirq() phase. That's too late in this
> > case as PHYs are suspended in the ->suspend() phase. And there's
> > no external interface to set the flag earlier.
>
> Yes, it is not there intentionally.
>
> Strictly speaking, IRQD_WAKEUP_ARMED is there to indicate to the IRQ
> subsystem that the given IRQ is a system wakeup one and has been left
> enabled specifically in order to signal system wakeup. It allows the
> IRQ to trigger between suspend_device_irqs() and resume_device_irqs()
> exactly once, which causes the system to wake up from suspend-to-idle
> (that's the primary use case for it) or aborts system suspends in
> progress.
>
> As you have noticed, it is set automatically by suspend_device_irqs()
> if the given IRQ has IRQD_WAKEUP_STATE which is the case when it has
> been enabled for system wakeup.
>
> > As I'm lacking access to the flag, I'm open coding its functionality
> > in this patch. Is this the correct approach or should I instead look
> > into providing an external interface to the flag?
>
> The idea is that the regular IRQ "action" handler will run before
> suspend_device_irqs(), so it should take care of signaling wakeup if
> need be. IRQD_WAKEUP_ARMED to trigger wakeup when IRQ "action"
> handlers don't run.

That said IMV there could be a wrapper around suspend_device_irq() to
mark a specific IRQ as "suspended" before running
suspend_device_irqs(), but that would require adding "suspend depth"
to struct irq_desc, so the IRQ is not "resumed" prematurely by
resume_device_irqs(). And there needs to be an analogous wrapper
around resume_irq() for the resume path.

Does the single prospective user justify this?