[PATCH net] net: stmmac: dwmac-rk: keep the GMAC powered when the PHY wakes the system

[PATCH net] net: stmmac: dwmac-rk: keep the GMAC powered when the PHY wakes the system

[Igor Velkov via B4 Relay]

From: Igor Velkov <iav@iav.lv>

Since commit 6911308d7d11 ("net: stmmac: convert to phylink-managed
Wake-on-Lan"), the MAC device gets its wakeup flag only when the MAC
handles Wake-on-LAN itself. When the PHY handles it, device_may_wakeup()
is false for the MAC, and rk_gmac_suspend() powers the GMAC down: its
clocks stop and phy-supply is released.

On Helios64 (RTL8211F, Wake-on-LAN in the PHY) the board then hangs on
the first or second suspend with a magic-packet wake.

Also keep the GMAC powered when phy_may_wakeup() is true, and remember
the decision for resume.

Fixes: 6911308d7d11 ("net: stmmac: convert to phylink-managed Wake-on-Lan")
Assisted-by: LLM
Signed-off-by: Igor Velkov <iav@iav.lv>
---
Tested on Helios64 with 7.3-rc5: 20 of 20 suspends woke on a magic
packet. The mainline Helios64 DT has no PHY node, so the test used a
DT that describes the PHY with its interrupt and wakeup-source (a DT
patch will follow), together with [1], which fixes an interrupt storm
on resume.

[1] https://lore.kernel.org/r/20260930-stmmac-irq-shut-v1-1-104d1a1dcb28@iav.lv
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
index 72bdbcb5e863..06faeb156b97 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
@@ -98,6 +98,7 @@ struct rk_priv_data {
 	bool integrated_phy;
 	bool supports_rgmii;
 	bool supports_rmii;
+	bool suspend_powerdown;
 
 	struct clk_bulk_data *clks;
 	int num_clks;
@@ -1530,10 +1531,13 @@ static int rk_set_clk_tx_rate(void *bsp_priv_, struct clk *clk_tx_i,
 
 static int rk_gmac_suspend(struct device *dev, void *bsp_priv_)
 {
+	struct net_device *ndev = dev_get_drvdata(dev);
 	struct rk_priv_data *bsp_priv = bsp_priv_;
 
-	/* Keep the PHY up if we use Wake-on-Lan. */
-	if (!device_may_wakeup(dev))
+	/* Keep the MAC and PHY powered if either of them wakes the system. */
+	bsp_priv->suspend_powerdown = !device_may_wakeup(dev) &&
+		!(ndev->phydev && phy_may_wakeup(ndev->phydev));
+	if (bsp_priv->suspend_powerdown)
 		rk_gmac_powerdown(bsp_priv);
 
 	return 0;
@@ -1543,8 +1547,7 @@ static int rk_gmac_resume(struct device *dev, void *bsp_priv_)
 {
 	struct rk_priv_data *bsp_priv = bsp_priv_;
 
-	/* The PHY was up for Wake-on-Lan. */
-	if (!device_may_wakeup(dev))
+	if (bsp_priv->suspend_powerdown)
 		rk_gmac_powerup(bsp_priv);
 
 	return 0;

---
base-commit: 99b43ede9e355ba35244cc9470bf1819774ce39d
change-id: 20260930-dwmac-rk-phy-wol-be1ea6a6066b

Best regards,
-- 
Igor Velkov <iav@iav.lv>

Re: [PATCH net] net: stmmac: dwmac-rk: keep the GMAC powered when the PHY wakes the system

[Andrew Lunn]

On Wed, Sep 30, 2026 at 07:34:45PM +0000, Igor Velkov via B4 Relay wrote:
> From: Igor Velkov <iav@iav.lv>
> 
> Since commit 6911308d7d11 ("net: stmmac: convert to phylink-managed
> Wake-on-Lan"), the MAC device gets its wakeup flag only when the MAC
> handles Wake-on-LAN itself. When the PHY handles it, device_may_wakeup()
> is false for the MAC, and rk_gmac_suspend() powers the GMAC down: its
> clocks stop and phy-supply is released.
> 
> On Helios64 (RTL8211F, Wake-on-LAN in the PHY) the board then hangs on
> the first or second suspend with a magic-packet wake.

You have not answered the question why is hangs? Given the current
description, this just sounds like a workaround, not a fix.

	Andrew

Re: [PATCH net] net: stmmac: dwmac-rk: keep the GMAC powered when the PHY wakes the system

[Igor Velkov]

On Wed, Sep 30, 2026 at 11:51:45PM +0200, Andrew Lunn wrote:
> You have not answered the question why is hangs? Given the current
> description, this just sounds like a workaround, not a fix.

It hangs because the PHY interrupt handler accesses the powered-down MAC.

stmmac sets mac_managed_pm, so mdio_bus_phy_suspend() returns before
setting irq_suspended and phy_interrupt() runs the handler at once.
rtl8211f_handle_interrupt() reads INSR over this GMAC's MDIO bus.
From rk_gmac_suspend() to suspend_late, and from resume_early to
rk_gmac_resume(), runtime PM is still enabled and that read reaches
the MAC with aclk_mac/pclk_mac gated.

On RK3399 such a read never completes: a test readl() right after
rk_gmac_powerdown() hung the board with no soft-lockup panic until
the hardware watchdog reset it (>5 min with the watchdog off).
Refusing MDIO access to the powered-down MAC: 10/10 suspends without
a hang; the same cycle script without it hung in 4 of 5 runs, each
time within three cycles. With the GMAC kept powered (v1): 45/45.

v2 will use device_wakeup_path(), and its Fixes tag will be
d65cb2e27e6e: that commit, not 6911308d7d11, stopped setting the MAC
wakeup flag.

pw-bot: cr

--
Igor Velkov

Re: [PATCH net] net: stmmac: dwmac-rk: keep the GMAC powered when the PHY wakes the system

[Andrew Lunn]

On Thu, Oct 01, 2026 at 06:11:09AM +0300, Igor Velkov wrote:
> On Wed, Sep 30, 2026 at 11:51:45PM +0200, Andrew Lunn wrote:
> > You have not answered the question why is hangs? Given the current
> > description, this just sounds like a workaround, not a fix.
> 
> It hangs because the PHY interrupt handler accesses the powered-down MAC.
> 
> stmmac sets mac_managed_pm, so mdio_bus_phy_suspend() returns before
> setting irq_suspended and phy_interrupt() runs the handler at once.

Why is the PHY interrupting? What interrupts are left enabled? Should
the mask be changed to disable everything except wol?

	Andrew

Re: [PATCH net] net: stmmac: dwmac-rk: keep the GMAC powered when the PHY wakes the system

[Igor Velkov]

On Thu, Oct 01, 2026 at 02:07:41PM +0200, Andrew Lunn wrote:
> Why is the PHY interrupting? What interrupts are left enabled? Should
> the mask be changed to disable everything except wol?

Only the WoL interrupt is left: rtl8211f_suspend() sets INER to PME
alone. The interrupt is the magic-packet wake itself.
resume_device_irqs() re-enables the line and it fires before the MAC
resumes; the handler reads INSR (0x80, PME) over MDIO with the GMAC
clocks off.

So the mask is fine; the problem is that with mac_managed_pm phylib
does not postpone the interrupt until the PHY resumes. Patch: [1].

No new version of this patch: the hang is a phylib issue, fixed by
[1]. With [1] and the GMAC powered down as today: 20/20 suspends (an
instrumented build logged no MDIO access to the powered-down GMAC).
Without it the same setup hung within 1 to 7 cycles in 14 of 16 runs,
some on instrumented builds; one run passed 25 cycles. What remains is that
rk_gmac_powerdown() disables phy-supply while the PHY is the wakeup
source; on Helios64 that regulator is always-on, so I cannot test a
fix for it.

[1] https://lore.kernel.org/r/20261002043548.1302145-1-iav@iav.lv

--
Igor Velkov

Re: [PATCH net] net: stmmac: dwmac-rk: keep the GMAC powered when the PHY wakes the system

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 (1) · Low: 2

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The new predicate in rk_gmac_suspend() takes 'ndev->phydev ==
  NULL' to mean 'no PHY needs to stay powered for wake'.
- [Low] The commit body says Helios64 (RTL8211F, WoL in the PHY) hangs
  after 6911308d7d11.
- [Low] rk_gmac_suspend() reads ndev->phydev, an RTNL-protected field,
  without RTNL and without READ_ONCE(), in `ndev->phydev &&…

Pre-existing issues:
- [Medium] rk_gmac_resume() ignores the return value of rk_gmac_powerup()
  and always returns 0.

--

net: stmmac: dwmac-rk: keep the GMAC powered when the PHY wakes the system

With this patch, rk_gmac_suspend() also skips rk_gmac_powerdown() when the
attached PHY reports phy_may_wakeup(). Before, it only checked
device_may_wakeup() on the MAC. The decision is stored in a new
suspend_powerdown field, and rk_gmac_resume() uses it to decide whether
to call rk_gmac_powerup().

> When the PHY handles it, device_may_wakeup()
> is false for the MAC, and rk_gmac_suspend() powers the GMAC down: its
> clocks stop and phy-supply is released.
>
> On Helios64 (RTL8211F, Wake-on-LAN in the PHY) the board then hangs on
> the first or second suspend with a magic-packet wake.

[Severity: Low]
Does this describe the in-tree Helios64 configuration?

The gmac node in rk3399-kobol-helios64.dts has no ethernet-phy child. So
the RTL8211F probe never calls device_set_wakeup_capable() on the PHY, and
rtl8211f_get_wol() reports no WoL support:

drivers/net/phy/realtek/realtek_main.c:rtl8211f_get_wol() {
    ...
	if (!device_can_wakeup(&dev->mdio.dev)) {
		wol->supported = 0;
		return;
	}
    ...
}

phylink then passes the request to stmmac_mac_wol_set(), which does:

	device_set_wakeup_enable(priv->device, !!wolopts);

So on the in-tree board, the old !device_may_wakeup(dev) check in
rk_gmac_suspend() already kept the GMAC powered.

The case fixed here only happens with a DT that gives the PHY node an
interrupt and wakeup-source. That requirement is mentioned only below the
--- line. The same goes for the need for the separate interrupt storm fix
[1] to get the 20 of 20 result. Both notes are dropped when the patch is
applied.

Also, on Helios64 phy-supply is vcc_lan, which is vcc3v3_sys_s3. That
regulator has regulator-always-on and regulator-on-in-suspend, so
releasing it does not cut PHY power on this board. That leaves the stopped
clocks as the only trigger the message describes.

Could the commit message say which configurations are affected, why the
board hangs, and that [1] is also needed?

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> index 72bdbcb5e863a..06faeb156b97d 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c

[ ... ]

> @@ -1530,10 +1531,13 @@ static int rk_set_clk_tx_rate(void *bsp_priv_, struct clk *clk_tx_i,
>  
>  static int rk_gmac_suspend(struct device *dev, void *bsp_priv_)
>  {
> +	struct net_device *ndev = dev_get_drvdata(dev);
>  	struct rk_priv_data *bsp_priv = bsp_priv_;
>  
> -	/* Keep the PHY up if we use Wake-on-Lan. */
> -	if (!device_may_wakeup(dev))
> +	/* Keep the MAC and PHY powered if either of them wakes the system. */
> +	bsp_priv->suspend_powerdown = !device_may_wakeup(dev) &&
> +		!(ndev->phydev && phy_may_wakeup(ndev->phydev));

[Severity: Medium]
What happens here if PHY WoL was enabled and the interface was brought
down before suspend?

With the link up, "ethtool -s ethX wol g" is taken by the wakeup-capable
PHY. phylink masks it out of the MAC request, so device_may_wakeup() stays
false for the MAC.

"ip link set ethX down" then runs:

stmmac_release()
  phylink_disconnect_phy()
    phy_disconnect()
      phy_detach()
        phy_suspend()     <- RTL8211F arms PME (PHY_ALWAYS_CALL_SUSPEND)
        phydev->attached_dev->phydev = NULL;

phylib keeps a detached PHY like this armed on purpose, as shown in
mdio_bus_phy_may_suspend():

	if (!netdev && phy_may_wakeup(phydev))
		return false;

On system suspend, stmmac_suspend() jumps to suspend_bsp because the
interface is not running:

	if (!ndev || !netif_running(ndev))
		goto suspend_bsp;

Here ndev->phydev is NULL, so suspend_powerdown becomes true.
rk_gmac_powerdown() then gates the GMAC clocks and drops phy-supply while
the PHY is still the wake source. How bad this is depends on the board:
on Helios64 only the clocks stop, but where phy-supply can be switched
off, the PHY loses power.

Before 6911308d7d11, the MAC wakeup flag stayed set across ifdown for PHY
WoL, so the glue kept the GMAC powered. Should the new check also cover
this case?

[Severity: Low]
Is it safe to read ndev->phydev here without rtnl_lock()?

stmmac_suspend() drops rtnl before it calls the glue hook:

	rtnl_lock();
	phylink_suspend(priv->phylink, !!priv->wolopts);
	rtnl_unlock();
	...
suspend_bsp:
	if (priv->plat->suspend)
		return priv->plat->suspend(dev, priv->plat->bsp_priv);

It also does not cancel the service task. stmmac_reset_subtask() runs on
priv->wq, which is not freezable. It takes rtnl_lock() and does:

	set_bit(STMMAC_DOWN, &priv->state);
	dev_close(priv->dev);
	dev_open(priv->dev, NULL);

This clears ndev->phydev in phy_detach() and sets it again from
__stmmac_open().

A reset queued by stmmac_global_err() can overlap with rk_gmac_suspend().
If it does, suspend_powerdown can be decided while the PHY is halfway
through detach or attach, and rk_gmac_resume() reuses that decision
without checking again. In theory, if the compiler reloads ndev->phydev
between the NULL test and the call, phy_may_wakeup() could also be passed
NULL.

Would it help to read the pointer once with READ_ONCE() into a local, or
to do the check under rtnl_lock()?

[ ... ]

> @@ -1543,8 +1547,7 @@ static int rk_gmac_resume(struct device *dev, void *bsp_priv_)
>  {
>  	struct rk_priv_data *bsp_priv = bsp_priv_;
>  
> -	/* The PHY was up for Wake-on-Lan. */
> -	if (!device_may_wakeup(dev))
> +	if (bsp_priv->suspend_powerdown)
>  		rk_gmac_powerup(bsp_priv);
>  
>  	return 0;

[Severity: Medium]
This is a pre-existing issue, but rk_gmac_resume() drops the return value
of rk_gmac_powerup() and always returns 0.

rk_gmac_powerup() can fail in rk_gmac_check_ops(), rk_get_phy_intf_sel(),
gmac_clk_enable(), a GRF write, or regulator_enable() in rk_phy_powerup().
In that case stmmac_resume() never takes its error path:

	if (priv->plat->resume) {
		ret = priv->plat->resume(dev, priv->plat->bsp_priv);
		if (ret)
			return ret;
	}

Instead it goes on to access MAC registers with the clocks off.

A later rk_gmac_powerdown() would then call pm_runtime_put_sync() without
a matching get, and regulator_disable() on a regulator that is already
disabled.

Should rk_gmac_resume() return the error from rk_gmac_powerup()?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260930-dwmac-rk-phy-wol-v1-1-9fdc50bd9ae4%40iav.lv