[net,v2,0/1] PHY interruptus horribilis

Message

Lukas Wunner June 8, 2022, 9:52 a.m. UTC

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.

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?

Side note: suspend_device_irqs() and resume_device_irqs() have been
exported since forever even though there's no module user...

Thanks!

Changes since v1:
* Extend rationale in the commit message.
* Drop Fixes tag, add Tested-by tag (Marek).

Link to v1:
https://lore.kernel.org/netdev/[e-mail]/

Lukas Wunner (1):
  net: phy: Don't trigger state machine while in suspend

 drivers/net/phy/phy.c        | 23 +++++++++++++++++++++++
 drivers/net/phy/phy_device.c | 23 +++++++++++++++++++++++
 include/linux/phy.h          |  6 ++++++
 3 files changed, 52 insertions(+)

Comments

Rafael J. Wysocki June 8, 2022, 10:35 a.m. UTC | #1

On Wed, Jun 8, 2022 at 11:52 AM Lukas Wunner <[e-mail]> 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.

> Side note: suspend_device_irqs() and resume_device_irqs() have been
> exported since forever even though there's no module user...

Well, that's a mistake.

Rafael J. Wysocki June 8, 2022, 11:09 a.m. UTC | #2

On Wed, Jun 8, 2022 at 12:35 PM Rafael J. Wysocki <[e-mail]> wrote:
>
> On Wed, Jun 8, 2022 at 11:52 AM Lukas Wunner <[e-mail]> 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?