Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
MIME-Version: 1.0
References: <cover.1654680790.git.lukas@wunner.de> <CAJZ5v0i1p=oReS5Ki69-uOLQNBu2Y=MkqDQ4fUU-ih=n_kshwQ@mail.gmail.com>
In-Reply-To: <CAJZ5v0i1p=oReS5Ki69-uOLQNBu2Y=MkqDQ4fUU-ih=n_kshwQ@mail.gmail.com>
From:   "Rafael J. Wysocki" <rafael@kernel.org>
Date:   Wed, 8 Jun 2022 13:09:50 +0200
Message-ID: <CAJZ5v0jocgtgZxA2wiDTbtOypQFy2WeHRCeNn9U4cvCZ_p48Bg@mail.gmail.com>
Subject: Re: [PATCH net v2 0/1] PHY interruptus horribilis
To:     Lukas Wunner <lukas@wunner.de>
Cc:     "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Eric Dumazet <edumazet@google.com>,
        Andrew Lunn <andrew@lunn.ch>,
        Heiner Kallweit <hkallweit1@gmail.com>,
        Russell King <linux@armlinux.org.uk>,
        Marek Szyprowski <m.szyprowski@samsung.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        "Rafael J. Wysocki" <rafael@kernel.org>,
        Len Brown <len.brown@intel.com>, Pavel Machek <pavel@ucw.cz>,
        netdev <netdev@vger.kernel.org>,
        Steve Glendinning <steve.glendinning@shawell.net>,
        UNGLinuxDriver@microchip.com, Oliver Neukum <oneukum@suse.com>,
        Andre Edich <andre.edich@microchip.com>,
        Oleksij Rempel <linux@rempel-privat.de>,
        Martyn Welch <martyn.welch@collabora.com>,
        Gabriel Hojda <ghojda@yo2urs.ro>,
        Christoph Fritz <chf.fritz@googlemail.com>,
        Lino Sanfilippo <LinoSanfilippo@gmx.de>,
        Philipp Rosenberger <p.rosenberger@kunbus.com>,
        Ferry Toth <fntoth@gmail.com>,
        Krzysztof Kozlowski <krzk@kernel.org>,
        Linux Samsung SoC <linux-samsung-soc@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Linux PM <linux-pm@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Jun 8, 2022 at 12:35 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Wed, Jun 8, 2022 at 11:52 AM Lukas Wunner <lukas@wunner.de> 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?