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@xxxxxxxxx>
> ---
>  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