[PATCH net-next] net: phy: marvell: wake the system only from the WoL event

[PATCH net-next] net: phy: marvell: wake the system only from the WoL event

[Rosen Penev]

Enable waking the system from a WoL event on the 88E1318S/88E1510.

When WoL is enabled, m88e1318_set_wol() now calls enable_irq_wake()
on the PHY's IRQ and marks the MDIO device as a wakeup source so the
underlying GPIO interrupt raises the system from suspend, and reverses
both on disable.

With WoL active the PHY must stay powered to detect a magic packet,
but without masking, phy_suspend() would leave every interrupt
enabled, so a link status change or any other PHY event would assert
INTn and spuriously wake the system. Set PHY_ALWAYS_CALL_SUSPEND on
the 88E1318S/88E1510 drivers so that phy_suspend() still calls their
suspend callbacks with WoL enabled, and have those callbacks keep the
PHY awake while writing the interrupt enable register (CSIER/IMASK)
down to only the WoL event bit.

marvell_config_intr() rewrites the whole CSIER register with
MII_M1011_IMASK_INIT on resume, clearing the WoL enable bit. Re-arm
WOL_EIE in the resume callbacks so the WoL interrupt stays active for
the next sleep cycle.

Assisted-by: LLM
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/net/phy/marvell.c | 145 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 138 insertions(+), 7 deletions(-)

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index f71cffa88406..70aaa09f2047 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -30,6 +30,8 @@
 #include <linux/ethtool_netlink.h>
 #include <linux/phy.h>
 #include <linux/phy_port.h>
+#include <linux/pm_wakeirq.h>
+#include <linux/property.h>
 #include <linux/marvell_phy.h>
 #include <linux/bitfield.h>
 #include <linux/of.h>
@@ -1902,6 +1904,61 @@ static int marvell_resume(struct phy_device *phydev)
 	return err;
 }
 
+/* marvell_wol_suspend_intrs
+ *
+ * With WoL enabled the PHY has to stay powered to keep detecting a WoL
+ * packet, so instead of entering low power mode, mask all interrupt
+ * sources except the WoL event. On the 88E1318S/88E1510 the interrupt
+ * mask (MII_M1011_IMASK) is the same register as the Copper Specific
+ * Interrupt Enable Register (MII_88E1318S_PHY_CSIER), so writing only
+ * the WoL event enable bit unmask just that event.
+ */
+static int marvell_wol_suspend_intrs(struct phy_device *phydev)
+{
+	int oldpage, ret;
+
+	oldpage = phy_save_page(phydev);
+	if (oldpage < 0)
+		return oldpage;
+
+	ret = marvell_write_page(phydev, MII_MARVELL_COPPER_PAGE);
+	if (ret < 0)
+		goto out;
+
+	ret = __phy_write(phydev, MII_88E1318S_PHY_CSIER,
+			  MII_88E1318S_PHY_CSIER_WOL_EIE);
+out:
+	return phy_restore_page(phydev, oldpage, ret);
+}
+
+/* marvell_wol_resume_intrs
+ *
+ * marvell_config_intr() rewrites the whole MII_M1011_IMASK register
+ * (which is MII_88E1318S_PHY_CSIER) with MII_M1011_IMASK_INIT on
+ * resume, clearing the WoL event interrupt enable bit. Re-arm it if
+ * WoL is still enabled so a later WoL event keeps waking the system.
+ */
+static int marvell_wol_resume_intrs(struct phy_device *phydev)
+{
+	int oldpage, ret;
+
+	if (!phydev->wol_enabled)
+		return 0;
+
+	oldpage = phy_save_page(phydev);
+	if (oldpage < 0)
+		return oldpage;
+
+	ret = marvell_write_page(phydev, MII_MARVELL_COPPER_PAGE);
+	if (ret < 0)
+		goto out;
+
+	ret = __phy_set_bits(phydev, MII_88E1318S_PHY_CSIER,
+			     MII_88E1318S_PHY_CSIER_WOL_EIE);
+out:
+	return phy_restore_page(phydev, oldpage, ret);
+}
+
 /* m88e1510_resume
  *
  * The 88e1510 PHY has an erratum where the phy downshift counter is not cleared
@@ -1934,9 +1991,25 @@ static int m88e1510_resume(struct phy_device *phydev)
 
 		/* downshift enabled, with previous counter value */
 		err = m88e1011_set_downshift(phydev, cnt);
+		if (err < 0)
+			return err;
 	}
 
-	return err;
+	return marvell_wol_resume_intrs(phydev);
+}
+
+/* m88e1510_suspend
+ *
+ * If WoL is enabled the PHY receiver has to keep running to detect a
+ * magic packet, so keep it awake and unmask only the WoL event at the
+ * PHY. Otherwise suspend both the fiber and copper interfaces as usual.
+ */
+static int m88e1510_suspend(struct phy_device *phydev)
+{
+	if (phydev->wol_enabled)
+		return marvell_wol_suspend_intrs(phydev);
+
+	return marvell_suspend(phydev);
 }
 
 static int marvell_aneg_done(struct phy_device *phydev)
@@ -1969,8 +2042,12 @@ static void m88e1318_get_wol(struct phy_device *phydev,
 static int m88e1318_set_wol(struct phy_device *phydev,
 			    struct ethtool_wolinfo *wol)
 {
+	struct device *dev = &phydev->mdio.dev;
+	bool wol_enable;
 	int err = 0, oldpage;
 
+	wol_enable = !!(wol->wolopts & (WAKE_MAGIC | WAKE_PHY));
+
 	oldpage = phy_save_page(phydev);
 	if (oldpage < 0)
 		goto error;
@@ -2075,7 +2152,49 @@ static int m88e1318_set_wol(struct phy_device *phydev,
 	}
 
 error:
-	return phy_restore_page(phydev, oldpage, err);
+	err = phy_restore_page(phydev, oldpage, err);
+	if (err < 0)
+		return err;
+
+	if (device_can_wakeup(dev) && wol_enable != device_may_wakeup(dev)) {
+		err = device_set_wakeup_enable(dev, wol_enable);
+		if (err < 0) {
+			/* Roll back the PHY WoL config if the PM state update failed */
+			struct ethtool_wolinfo wol_off = { .wolopts = 0 };
+			int rollback_err = m88e1318_set_wol(phydev, &wol_off);
+
+			if (rollback_err < 0)
+				phydev_err(phydev,
+					   "Failed to disable WoL after wakeup enable error %d\n",
+					   rollback_err);
+		}
+	}
+
+	return err;
+}
+
+/* m88e1318_suspend
+ *
+ * If WoL is enabled keep the PHY awake so it can detect a magic packet
+ * while the system is asleep, unmasking only the WoL event at the PHY.
+ * Otherwise suspend the PHY normally.
+ */
+static int m88e1318_suspend(struct phy_device *phydev)
+{
+	if (phydev->wol_enabled)
+		return marvell_wol_suspend_intrs(phydev);
+
+	return genphy_suspend(phydev);
+}
+
+static int m88e1318_resume(struct phy_device *phydev)
+{
+	int err = genphy_resume(phydev);
+
+	if (err < 0)
+		return err;
+
+	return marvell_wol_resume_intrs(phydev);
 }
 
 static int marvell_get_sset_count(struct phy_device *phydev)
@@ -3587,14 +3706,25 @@ static int m88e1318_led_hw_control_get(struct phy_device *phydev, u8 index,
 
 static int marvell_probe(struct phy_device *phydev)
 {
+	struct device *dev = &phydev->mdio.dev;
 	struct marvell_priv *priv;
 
-	priv = devm_kzalloc(&phydev->mdio.dev, sizeof(*priv), GFP_KERNEL);
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
 
 	phydev->priv = priv;
 
+	if (device_property_present(dev, "wakeup-source") &&
+	    phy_interrupt_is_valid(phydev)) {
+		int ret = devm_pm_set_wake_irq(dev, phydev->irq);
+
+		if (ret)
+			return ret;
+
+		device_set_wakeup_capable(dev, true);
+	}
+
 	return marvell_hwmon_probe(phydev);
 }
 
@@ -3813,6 +3943,7 @@ static struct phy_driver marvell_drivers[] = {
 		.phy_id_mask = MARVELL_PHY_ID_MASK,
 		.name = "Marvell 88E1318S",
 		/* PHY_GBIT_FEATURES */
+		.flags = PHY_ALWAYS_CALL_SUSPEND,
 		.probe = marvell_probe,
 		.config_init = m88e1318_config_init,
 		.config_aneg = m88e1318_config_aneg,
@@ -3821,8 +3952,8 @@ static struct phy_driver marvell_drivers[] = {
 		.handle_interrupt = marvell_handle_interrupt,
 		.get_wol = m88e1318_get_wol,
 		.set_wol = m88e1318_set_wol,
-		.resume = genphy_resume,
-		.suspend = genphy_suspend,
+		.resume = m88e1318_resume,
+		.suspend = m88e1318_suspend,
 		.read_page = marvell_read_page,
 		.write_page = marvell_write_page,
 		.get_sset_count = marvell_get_sset_count,
@@ -3920,7 +4051,7 @@ static struct phy_driver marvell_drivers[] = {
 		.name = "Marvell 88E1510",
 		.driver_data = DEF_MARVELL_HWMON_OPS(m88e1510_hwmon_ops),
 		.features = PHY_GBIT_FIBRE_FEATURES,
-		.flags = PHY_POLL_CABLE_TEST,
+		.flags = PHY_POLL_CABLE_TEST | PHY_ALWAYS_CALL_SUSPEND,
 		.probe = marvell_probe,
 		.config_init = m88e1510_config_init,
 		.config_aneg = m88e1510_config_aneg,
@@ -3930,7 +4061,7 @@ static struct phy_driver marvell_drivers[] = {
 		.get_wol = m88e1318_get_wol,
 		.set_wol = m88e1318_set_wol,
 		.resume = m88e1510_resume,
-		.suspend = marvell_suspend,
+		.suspend = m88e1510_suspend,
 		.read_page = marvell_read_page,
 		.write_page = marvell_write_page,
 		.get_sset_count = marvell_get_sset_count,
-- 
2.55.0

Re: [PATCH net-next] net: phy: marvell: wake the system only from the WoL event

[Maxime Chevallier]

Hi Rosen,

On 9/13/26 01:51, Rosen Penev wrote:
> Enable waking the system from a WoL event on the 88E1318S/88E1510.
> 
> When WoL is enabled, m88e1318_set_wol() now calls enable_irq_wake()
> on the PHY's IRQ and marks the MDIO device as a wakeup source so the
> underlying GPIO interrupt raises the system from suspend, and reverses
> both on disable.
> 
> With WoL active the PHY must stay powered to detect a magic packet,
> but without masking, phy_suspend() would leave every interrupt
> enabled, so a link status change or any other PHY event would assert
> INTn and spuriously wake the system. Set PHY_ALWAYS_CALL_SUSPEND on
> the 88E1318S/88E1510 drivers so that phy_suspend() still calls their
> suspend callbacks with WoL enabled, and have those callbacks keep the
> PHY awake while writing the interrupt enable register (CSIER/IMASK)
> down to only the WoL event bit.
> 
> marvell_config_intr() rewrites the whole CSIER register with
> MII_M1011_IMASK_INIT on resume, clearing the WoL enable bit. Re-arm
> WOL_EIE in the resume callbacks so the WoL interrupt stays active for
> the next sleep cycle.
> 
> Assisted-by: LLM
> Signed-off-by: Rosen Penev <rosenp@gmail.com>
> ---
>  drivers/net/phy/marvell.c | 145 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 138 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
> index f71cffa88406..70aaa09f2047 100644
> --- a/drivers/net/phy/marvell.c
> +++ b/drivers/net/phy/marvell.c
> @@ -30,6 +30,8 @@
>  #include <linux/ethtool_netlink.h>
>  #include <linux/phy.h>
>  #include <linux/phy_port.h>
> +#include <linux/pm_wakeirq.h>
> +#include <linux/property.h>
>  #include <linux/marvell_phy.h>
>  #include <linux/bitfield.h>
>  #include <linux/of.h>
> @@ -1902,6 +1904,61 @@ static int marvell_resume(struct phy_device *phydev)
>  	return err;
>  }
>  
> +/* marvell_wol_suspend_intrs
> + *
> + * With WoL enabled the PHY has to stay powered to keep detecting a WoL
> + * packet, so instead of entering low power mode, mask all interrupt
> + * sources except the WoL event. On the 88E1318S/88E1510 the interrupt
> + * mask (MII_M1011_IMASK) is the same register as the Copper Specific
> + * Interrupt Enable Register (MII_88E1318S_PHY_CSIER), so writing only
> + * the WoL event enable bit unmask just that event.
> + */

Please tone down the verbosity of the LLM used, this explanation is
repeated in lots of introduced comments. The code is explicit enough
TBH, I don't think these comments add much value.

Same goes on all function docs added in this patch :/

> +static int marvell_wol_suspend_intrs(struct phy_device *phydev)
> +{
> +	int oldpage, ret;
> +
> +	oldpage = phy_save_page(phydev);
> +	if (oldpage < 0)
> +		return oldpage;
> +
> +	ret = marvell_write_page(phydev, MII_MARVELL_COPPER_PAGE);
> +	if (ret < 0)
> +		goto out;
> +
> +	ret = __phy_write(phydev, MII_88E1318S_PHY_CSIER,
> +			  MII_88E1318S_PHY_CSIER_WOL_EIE);
> +out:
> +	return phy_restore_page(phydev, oldpage, ret);
> +}
> +
> +/* marvell_wol_resume_intrs
> + *
> + * marvell_config_intr() rewrites the whole MII_M1011_IMASK register
> + * (which is MII_88E1318S_PHY_CSIER) with MII_M1011_IMASK_INIT on
> + * resume, clearing the WoL event interrupt enable bit. Re-arm it if
> + * WoL is still enabled so a later WoL event keeps waking the system.
> + */
> +static int marvell_wol_resume_intrs(struct phy_device *phydev)
> +{
> +	int oldpage, ret;
> +
> +	if (!phydev->wol_enabled)
> +		return 0;
> +
> +	oldpage = phy_save_page(phydev);
> +	if (oldpage < 0)
> +		return oldpage;
> +
> +	ret = marvell_write_page(phydev, MII_MARVELL_COPPER_PAGE);
> +	if (ret < 0)
> +		goto out;
> +
> +	ret = __phy_set_bits(phydev, MII_88E1318S_PHY_CSIER,
> +			     MII_88E1318S_PHY_CSIER_WOL_EIE);
> +out:
> +	return phy_restore_page(phydev, oldpage, ret);
> +}
> +
>  /* m88e1510_resume
>   *
>   * The 88e1510 PHY has an erratum where the phy downshift counter is not cleared
> @@ -1934,9 +1991,25 @@ static int m88e1510_resume(struct phy_device *phydev)
>  
>  		/* downshift enabled, with previous counter value */
>  		err = m88e1011_set_downshift(phydev, cnt);
> +		if (err < 0)
> +			return err;
>  	}
>  
> -	return err;
> +	return marvell_wol_resume_intrs(phydev);
> +}
> +
> +/* m88e1510_suspend
> + *
> + * If WoL is enabled the PHY receiver has to keep running to detect a
> + * magic packet, so keep it awake and unmask only the WoL event at the
> + * PHY. Otherwise suspend both the fiber and copper interfaces as usual.
> + */
> +static int m88e1510_suspend(struct phy_device *phydev)
> +{
> +	if (phydev->wol_enabled)
> +		return marvell_wol_suspend_intrs(phydev);
> +
> +	return marvell_suspend(phydev);
>  }
>  
>  static int marvell_aneg_done(struct phy_device *phydev)
> @@ -1969,8 +2042,12 @@ static void m88e1318_get_wol(struct phy_device *phydev,
>  static int m88e1318_set_wol(struct phy_device *phydev,
>  			    struct ethtool_wolinfo *wol)
>  {
> +	struct device *dev = &phydev->mdio.dev;
> +	bool wol_enable;
>  	int err = 0, oldpage;
>  
> +	wol_enable = !!(wol->wolopts & (WAKE_MAGIC | WAKE_PHY));
> +
>  	oldpage = phy_save_page(phydev);
>  	if (oldpage < 0)
>  		goto error;
> @@ -2075,7 +2152,49 @@ static int m88e1318_set_wol(struct phy_device *phydev,
>  	}
>  
>  error:
> -	return phy_restore_page(phydev, oldpage, err);
> +	err = phy_restore_page(phydev, oldpage, err);
> +	if (err < 0)
> +		return err;
> +
> +	if (device_can_wakeup(dev) && wol_enable != device_may_wakeup(dev)) {
> +		err = device_set_wakeup_enable(dev, wol_enable);
> +		if (err < 0) {
> +			/* Roll back the PHY WoL config if the PM state update failed */
> +			struct ethtool_wolinfo wol_off = { .wolopts = 0 };
> +			int rollback_err = m88e1318_set_wol(phydev, &wol_off);
> +
> +			if (rollback_err < 0)
> +				phydev_err(phydev,
> +					   "Failed to disable WoL after wakeup enable error %d\n",
> +					   rollback_err);
> +		}
> +	}

It's unusual to recurse for error handling. If you need to run some
parts of this WoL config logic to disable WoL, put that logic in a separate
helper.

And this can be split into a dedicated patch, as this doesn't seem to be
related to the issue at stake here ?

Maxime

Re: [PATCH net-next] net: phy: marvell: wake the system only from the WoL event

[netdev-bot+sashiko]

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

Critical: 0 · High: 3 · Medium: 5 · Low: 0

- [High] The interrupt-masking mechanism the commit relies on is bypassed
  on the standard phylib PM route.
- [High] `marvell_wol_suspend_intrs()` overwrites the whole Copper
  Specific Interrupt Enable register with `__phy_write(phydev,…
- [High] Both new helpers, `marvell_wol_suspend_intrs()` and
  `marvell_wol_resume_intrs()`, return early when `phy_save_page()`…
- [Medium] Commit message does not match the code, and an undisclosed
  firmware/DT dependency gates the whole feature.
- [Medium] `marvell_wol_suspend_intrs()` rewrites the interrupt enable
  register (CSIER/IMASK, page 0 reg 0x12) but never reads…
- [Medium] `marvell_probe()` is the `.probe` of ~25 entries in
  `marvell_drivers[]`, but only the 88E1318S and 88E1510 entries gained…
- [Medium] The new suspend callbacks branch on `phydev->wol_enabled`, but
  the core defines that field as "the PHY *or the attached MAC* have…
- [Medium] The new wakeup-state check-then-update in `m88e1318_set_wol()`
  races with the MDIO device's `power/wakeup` sysfs control and, on a…

--

net: phy: marvell: wake the system only from the WoL event

This adds system wakeup support for WoL on the 88E1318S/88E1510 by
registering the PHY interrupt as a wake IRQ, marking the MDIO device as
wakeup capable, and adding suspend/resume callbacks that narrow the copper
specific interrupt enable register down to the WoL event bit.  A few
questions below about the interrupt enable handling, the MDIO bus lock, and
the commit message.

No Link: tags are present in the commit header.

The commit message says:

> When WoL is enabled, m88e1318_set_wol() now calls enable_irq_wake()
> on the PHY's IRQ and marks the MDIO device as a wakeup source so the
> underlying GPIO interrupt raises the system from suspend, and reverses
> both on disable.

[Severity: Medium]
Does m88e1318_set_wol() do either of those things?  In the patch it only
calls device_set_wakeup_enable(), and only when device_can_wakeup(dev) is
already true.  The wake IRQ is registered in marvell_probe() via
devm_pm_set_wake_irq(), gated on the firmware property "wakeup-source",
and device_set_wakeup_capable() is called there too.

Without "wakeup-source" in the PHY's DT node, device_can_wakeup() is false
and the new block in m88e1318_set_wol() is skipped entirely, so on existing
device trees nothing is armed.  Could the message mention the required DT
change?  The property is also not described in
Documentation/devicetree/bindings/net/ethernet-phy.yaml.

The message scopes the change to "the 88E1318S/88E1510", but marvell_probe()
is the .probe of every entry in marvell_drivers[].

> diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
> index f71cffa884062..70aaa09f20478 100644
> --- a/drivers/net/phy/marvell.c
> +++ b/drivers/net/phy/marvell.c

[ ... ]

> @@ -1902,6 +1904,61 @@ static int marvell_resume(struct phy_device *phydev)
>  	return err;
>  }
>  
> +/* marvell_wol_suspend_intrs
> + *
> + * With WoL enabled the PHY has to stay powered to keep detecting a WoL
> + * packet, so instead of entering low power mode, mask all interrupt
> + * sources except the WoL event. On the 88E1318S/88E1510 the interrupt
> + * mask (MII_M1011_IMASK) is the same register as the Copper Specific
> + * Interrupt Enable Register (MII_88E1318S_PHY_CSIER), so writing only
> + * the WoL event enable bit unmask just that event.
> + */
> +static int marvell_wol_suspend_intrs(struct phy_device *phydev)
> +{
> +	int oldpage, ret;
> +
> +	oldpage = phy_save_page(phydev);
> +	if (oldpage < 0)
> +		return oldpage;

[Severity: High]
Can this return leak the MDIO bus lock?  phy_save_page() takes the lock
before it reads the page register:

drivers/net/phy/phy-core.c:phy_save_page() {
	phy_lock_mdio_bus(phydev);
	return __phy_read_page(phydev);
}

Its kernel-doc states that phy_restore_page() must always be called after
this, irrespective of success or failure, and phy_restore_page() is the only
caller of phy_unlock_mdio_bus() (it already handles a negative oldpage).

__phy_read_page() can return -EOPNOTSUPP or propagate an MDIO transfer
error, which is not unlikely in the suspend/resume window.  If that happens
here, the same task then continues to issue MDIO accesses
(m88e1510_resume() -> marvell_resume(), downshift handling, the state
machine), so would it block forever on a mutex it cannot reacquire, taking
the rest of the bus with it?

The existing code in this file uses the documented idiom:

	oldpage = phy_save_page(phydev);
	if (oldpage < 0)
		goto error;

The same question applies to the identical early return in
marvell_wol_resume_intrs() below.

> +
> +	ret = marvell_write_page(phydev, MII_MARVELL_COPPER_PAGE);
> +	if (ret < 0)
> +		goto out;
> +
> +	ret = __phy_write(phydev, MII_88E1318S_PHY_CSIER,
> +			  MII_88E1318S_PHY_CSIER_WOL_EIE);

[Severity: High]
This is a full-register write to CSIER, which is the same register as
MII_M1011_IMASK, so it discards the MII_M1011_IMASK_INIT (0x6400, link
status change and aneg complete) enables programmed by
marvell_config_intr(), without saving them.  Is anything restoring them?

marvell_wol_resume_intrs() only ORs WOL_EIE back in.  drv->config_intr is
reached from phy_init_hw() and phy_config_interrupt() (attach/detach), not
from phy_start() -> __phy_resume(), and mdio_bus_phy_resume() returns early
when phydev->mac_managed_pm is set.

So after an ifdown/ifup cycle with WoL enabled, which now reaches the
suspend callback thanks to PHY_ALWAYS_CALL_SUSPEND:

	phy_stop()
	  _phy_state_machine()			/* PHY_STATE_WORK_SUSPEND */
	    _phy_state_machine_post_work()
	      phy_suspend()
	        m88e1510_suspend()
	          marvell_wol_suspend_intrs()	/* CSIER = WOL_EIE */

does the PHY come back with only WOL_EIE enabled while phydev->interrupts
still reads PHY_INTERRUPT_ENABLED and phy_polling_mode() is false?  In that
state marvell_handle_interrupt() bails out early:

	if (!(irq_status & MII_M1011_IMASK_INIT))
		return IRQ_NONE;

and INTn is not asserted for link events at all, so would link up/down go
undetected and the interface never regain carrier?  rtl8211f_suspend()
saves the enable register in priv->iner and restores it in rtl8211f_resume().

[Severity: Medium]
Should this sequence also read MII_M1011_IEVENT to clear already-latched
events before going to sleep?  marvell_config_intr() acks via
marvell_ack_interrupt() before every mask change, and the comment in
m88e1318_set_wol() says the LED[2] interrupt pin "will not be cleared unless
we reading the interrupt status register".

If a link status change latched while the interface was being taken down,
can INTn already be asserted low when the GPIO wake IRQ is armed, so the
suspend aborts or the system wakes immediately?  rtl8211f_suspend() does:

	/* Read the INSR to clear any pending interrupt */
	phy_read(phydev, RTL8211F_INSR);

> +out:
> +	return phy_restore_page(phydev, oldpage, ret);
> +}
> +
> +/* marvell_wol_resume_intrs
> + *
> + * marvell_config_intr() rewrites the whole MII_M1011_IMASK register
> + * (which is MII_88E1318S_PHY_CSIER) with MII_M1011_IMASK_INIT on
> + * resume, clearing the WoL event interrupt enable bit. Re-arm it if
> + * WoL is still enabled so a later WoL event keeps waking the system.
> + */
> +static int marvell_wol_resume_intrs(struct phy_device *phydev)
> +{
> +	int oldpage, ret;
> +
> +	if (!phydev->wol_enabled)
> +		return 0;
> +
> +	oldpage = phy_save_page(phydev);
> +	if (oldpage < 0)
> +		return oldpage;
> +
> +	ret = marvell_write_page(phydev, MII_MARVELL_COPPER_PAGE);
> +	if (ret < 0)
> +		goto out;
> +
> +	ret = __phy_set_bits(phydev, MII_88E1318S_PHY_CSIER,
> +			     MII_88E1318S_PHY_CSIER_WOL_EIE);
> +out:
> +	return phy_restore_page(phydev, oldpage, ret);
> +}
> +
>  /* m88e1510_resume
>   *
>   * The 88e1510 PHY has an erratum where the phy downshift counter is not cleared

[ ... ]

> @@ -1934,9 +1991,25 @@ static int m88e1510_resume(struct phy_device *phydev)
>  
>  		/* downshift enabled, with previous counter value */
>  		err = m88e1011_set_downshift(phydev, cnt);
> +		if (err < 0)
> +			return err;
>  	}
>  
> -	return err;
> +	return marvell_wol_resume_intrs(phydev);
> +}
> +
> +/* m88e1510_suspend
> + *
> + * If WoL is enabled the PHY receiver has to keep running to detect a
> + * magic packet, so keep it awake and unmask only the WoL event at the
> + * PHY. Otherwise suspend both the fiber and copper interfaces as usual.
> + */
> +static int m88e1510_suspend(struct phy_device *phydev)
> +{
> +	if (phydev->wol_enabled)
> +		return marvell_wol_suspend_intrs(phydev);

[Severity: Medium]
Is phydev->wol_enabled the right condition for "PHY WoL is configured"?
include/linux/phy.h documents it as "Set to true if the PHY or the attached
MAC have Wake-on-LAN enabled", and phy_suspend() assigns:

	phydev->wol_enabled = phy_may_wakeup(phydev) ||
			      (netdev && netdev->ethtool->wol_enabled);

With a MAC-only WoL configuration, does this branch write CSIER = WOL_EIE
even though m88e1318_set_wol() was never called, i.e. no magic packet
address and no LED[2]/INTn setup, masking the real PHY events and unmasking
a WoL source that is not armed?  rtl8211f_suspend() uses
device_may_wakeup(&phydev->mdio.dev) for this decision instead.

The same question applies to m88e1318_suspend() below.

> +
> +	return marvell_suspend(phydev);
>  }
>  
>  static int marvell_aneg_done(struct phy_device *phydev)

[ ... ]

> @@ -2075,7 +2152,49 @@ static int m88e1318_set_wol(struct phy_device *phydev,
>  	}
>  
>  error:
> -	return phy_restore_page(phydev, oldpage, err);
> +	err = phy_restore_page(phydev, oldpage, err);
> +	if (err < 0)
> +		return err;
> +
> +	if (device_can_wakeup(dev) && wol_enable != device_may_wakeup(dev)) {
> +		err = device_set_wakeup_enable(dev, wol_enable);
> +		if (err < 0) {
> +			/* Roll back the PHY WoL config if the PM state update failed */
> +			struct ethtool_wolinfo wol_off = { .wolopts = 0 };
> +			int rollback_err = m88e1318_set_wol(phydev, &wol_off);
> +
> +			if (rollback_err < 0)
> +				phydev_err(phydev,
> +					   "Failed to disable WoL after wakeup enable error %d\n",
> +					   rollback_err);
> +		}
> +	}

[Severity: Medium]
Can this check-then-update race with the MDIO device's power/wakeup sysfs
control?  phy_ethtool_set_wol() holds phydev->lock, but
drivers/base/power/sysfs.c:wakeup_store() calls device_set_wakeup_enable()
on the same device without that mutex.

The read of device_may_wakeup(dev) and the later
device_set_wakeup_enable(dev, true) are not one transaction, so if the sysfs
path enables wakeup in between, device_wakeup_enable() ->
device_wakeup_attach() returns -EEXIST.

Does the error handler then treat that as a real failure and recursively
call m88e1318_set_wol(phydev, &wol_off), tearing down the magic packet
matching and WOL_EIE that were just programmed?

> +
> +	return err;
> +}
> +
> +/* m88e1318_suspend
> + *
> + * If WoL is enabled keep the PHY awake so it can detect a magic packet
> + * while the system is asleep, unmasking only the WoL event at the PHY.
> + * Otherwise suspend the PHY normally.
> + */
> +static int m88e1318_suspend(struct phy_device *phydev)
> +{
> +	if (phydev->wol_enabled)
> +		return marvell_wol_suspend_intrs(phydev);
> +
> +	return genphy_suspend(phydev);
> +}
> +
> +static int m88e1318_resume(struct phy_device *phydev)
> +{
> +	int err = genphy_resume(phydev);
> +
> +	if (err < 0)
> +		return err;
> +
> +	return marvell_wol_resume_intrs(phydev);
>  }
>  
>  static int marvell_get_sset_count(struct phy_device *phydev)

[ ... ]

> @@ -3587,14 +3706,25 @@ static int m88e1318_led_hw_control_get(struct phy_device *phydev, u8 index,
>  
>  static int marvell_probe(struct phy_device *phydev)
>  {
> +	struct device *dev = &phydev->mdio.dev;
>  	struct marvell_priv *priv;
>  
> -	priv = devm_kzalloc(&phydev->mdio.dev, sizeof(*priv), GFP_KERNEL);
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>  	if (!priv)
>  		return -ENOMEM;
>  
>  	phydev->priv = priv;
>  
> +	if (device_property_present(dev, "wakeup-source") &&
> +	    phy_interrupt_is_valid(phydev)) {
> +		int ret = devm_pm_set_wake_irq(dev, phydev->irq);
> +
> +		if (ret)
> +			return ret;
> +
> +		device_set_wakeup_capable(dev, true);
> +	}

[Severity: Medium]
marvell_probe() is the .probe of every entry in marvell_drivers[], but only
the 88E1318S and 88E1510 entries get .set_wol, PHY_ALWAYS_CALL_SUSPEND and
the masking suspend callbacks.

For the other entries (88E1111, 88E1118, 88E1121R, 88E1116R, 88E1540,
88E1545, 88E3016, ...), a DT "wakeup-source" property now registers
phydev->irq as a dedicated wake IRQ and adds the power/wakeup sysfs
attributes via device_set_wakeup_capable() -> pm_wakeup_sysfs_add().

If root then writes "enabled" there, dev_pm_arm_wake_irq() arms INTn while
those drivers' genphy_suspend()/marvell_suspend() leave the enable register
at MII_M1011_IMASK_INIT:

	#define MII_M1011_IMASK_INIT		0x6400

Would any link event during sleep wake the system on those models, and does
phy_may_wakeup() also change for them from the register-derived state to
device_may_wakeup()?  Should this block be restricted to the models that
received the masking callbacks?

> +
>  	return marvell_hwmon_probe(phydev);
>  }
>  

[ ... ]

> @@ -3813,6 +3943,7 @@ static struct phy_driver marvell_drivers[] = {
>  		.phy_id_mask = MARVELL_PHY_ID_MASK,
>  		.name = "Marvell 88E1318S",
>  		/* PHY_GBIT_FEATURES */
> +		.flags = PHY_ALWAYS_CALL_SUSPEND,

[Severity: High]
PHY_ALWAYS_CALL_SUSPEND only affects the -EBUSY check inside phy_suspend().
On the mdio-bus PM route, is phy_suspend() reached at all when WoL is
enabled?

drivers/net/phy/phy_device.c:mdio_bus_phy_may_suspend() {
	...
	if (netdev->ethtool->wol_enabled)
		return false;
	...
}

mdio_bus_phy_suspend() calls phy_suspend() only when that helper returns
true, and both ethtool_set_wol() and ethnl_set_wol() do:

	dev->ethtool->wol_enabled = !!wol.wolopts;

after any successful ->set_wol, including PHY WoL configured with
ethtool -s ethX wol g, where the MAC delegates to phy_ethtool_set_wol() ->
m88e1318_set_wol().

So for the exact configuration this patch targets, do m88e1318_suspend() and
m88e1510_suspend() ever run from system suspend?  Meanwhile the wake side
still takes effect, since m88e1318_set_wol() sets device_set_wakeup_enable()
on the MDIO device and dpm_suspend_noirq() arms the wake IRQ registered by
devm_pm_set_wake_irq().  Does that leave an armed wake source on a PHY whose
link status change and aneg complete interrupts are still enabled?

The same applies to the 88E1510 entry below.

>  		.probe = marvell_probe,
>  		.config_init = m88e1318_config_init,
>  		.config_aneg = m88e1318_config_aneg,
> @@ -3821,8 +3952,8 @@ static struct phy_driver marvell_drivers[] = {
>  		.handle_interrupt = marvell_handle_interrupt,
>  		.get_wol = m88e1318_get_wol,
>  		.set_wol = m88e1318_set_wol,
> -		.resume = genphy_resume,
> -		.suspend = genphy_suspend,
> +		.resume = m88e1318_resume,
> +		.suspend = m88e1318_suspend,
>  		.read_page = marvell_read_page,
>  		.write_page = marvell_write_page,
>  		.get_sset_count = marvell_get_sset_count,
> @@ -3920,7 +4051,7 @@ static struct phy_driver marvell_drivers[] = {
>  		.name = "Marvell 88E1510",
>  		.driver_data = DEF_MARVELL_HWMON_OPS(m88e1510_hwmon_ops),
>  		.features = PHY_GBIT_FIBRE_FEATURES,
> -		.flags = PHY_POLL_CABLE_TEST,
> +		.flags = PHY_POLL_CABLE_TEST | PHY_ALWAYS_CALL_SUSPEND,

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260912235125.151370-1-rosenp%40gmail.com

Re: [PATCH net-next] net: phy: marvell: wake the system only from the WoL event

[Andrew Lunn]

> +static int marvell_wol_suspend_intrs(struct phy_device *phydev)
> +{
> +	int oldpage, ret;
> +
> +	oldpage = phy_save_page(phydev);
> +	if (oldpage < 0)
> +		return oldpage;
> +
> +	ret = marvell_write_page(phydev, MII_MARVELL_COPPER_PAGE);
> +	if (ret < 0)
> +		goto out;
> +
> +	ret = __phy_write(phydev, MII_88E1318S_PHY_CSIER,
> +			  MII_88E1318S_PHY_CSIER_WOL_EIE);
> +out:
> +	return phy_restore_page(phydev, oldpage, ret);

What is all the oldpage code for?

     Andrew