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?
