[PATCH net-next] net: phy: marvell: undo WoL setup when Wake-on-LAN is disabled

[PATCH net-next] net: phy: marvell: undo WoL setup when Wake-on-LAN is disabled

[Rosen Penev]

Fully disabling Wake-on-LAN on the 88E1318S/88E1510 used to leave
the WoL interrupt enable bit (CSIER.WOL_EIE) set and LED[2]
configured as the INTn pin. A magic packet arriving after "wol d"
would still assert INTn, and a later "wol g" re-enabled WoL from a
dirty register state.

Undo the WoL interrupt setup once all WoL options are cleared:
clear CSIER.WOL_EIE and restore LED[2] to its prior use - the INTn
output on interrupt-driven PHYs, the plain LED function otherwise.
Never set the FORCE_INT bit while disposing of the pin: it holds
the interrupt line asserted and turns the shared PHY interrupt into
a "nobody cared" IRQ storm when the PHY interrupt is in use.

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

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index f71cffa88406..6f7c190880f2 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -2074,6 +2074,35 @@ static int m88e1318_set_wol(struct phy_device *phydev,
 			goto error;
 	}
 
+	if (!(wol->wolopts & (WAKE_MAGIC | WAKE_PHY))) {
+		/* Fully disabled: undo the WoL interrupt setup done above,
+		 * so a later re-enable starts from a clean state.
+		 */
+		err = marvell_write_page(phydev, MII_MARVELL_COPPER_PAGE);
+		if (err < 0)
+			goto error;
+
+		err = __phy_clear_bits(phydev, MII_88E1318S_PHY_CSIER,
+				       MII_88E1318S_PHY_CSIER_WOL_EIE);
+		if (err < 0)
+			goto error;
+
+		err = marvell_write_page(phydev, MII_MARVELL_LED_PAGE);
+		if (err < 0)
+			goto error;
+
+		err = __phy_modify(phydev, MII_88E1318S_PHY_LED_TCR,
+				   MII_88E1318S_PHY_LED_TCR_FORCE_INT |
+				   MII_88E1318S_PHY_LED_TCR_INTn_ENABLE |
+				   MII_88E1318S_PHY_LED_TCR_INT_ACTIVE_LOW,
+				   phy_interrupt_is_valid(phydev) ?
+					MII_88E1318S_PHY_LED_TCR_INTn_ENABLE |
+					MII_88E1318S_PHY_LED_TCR_INT_ACTIVE_LOW :
+					0);
+		if (err < 0)
+			goto error;
+	}
+
 error:
 	return phy_restore_page(phydev, oldpage, err);
 }
-- 
2.55.0

Re: [PATCH net-next] net: phy: marvell: undo WoL setup when Wake-on-LAN is disabled

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 0 · Low: 1

- [High] The new 'undo' block in m88e1318_set_wol()
  (drivers/net/phy/marvell.c:2094-2103) unconditionally rewrites the…
- [Low] The patch presents itself as a fix for released behaviour
  (CSIER.WOL_EIE left set and LED[2] left muxed as INTn after a full…

--

net: phy: marvell: undo WoL setup when Wake-on-LAN is disabled

When all WoL options are cleared, m88e1318_set_wol() now clears
CSIER.WOL_EIE on the copper page and rewrites the LED[2]/INTn pin control
bits in page 3 register 0x12 (MII_88E1318S_PHY_LED_TCR), setting
INTn_ENABLE and INT_ACTIVE_LOW when phy_interrupt_is_valid() is true and
clearing them otherwise.  FORCE_INT is always cleared.

Two questions below, one about the LED_TCR rewrite and one about missing
provenance metadata.

[Severity: Low]

>     Fully disabling Wake-on-LAN on the 88E1318S/88E1510 used to leave
>     the WoL interrupt enable bit (CSIER.WOL_EIE) set and LED[2]
>     configured as the INTn pin. A magic packet arriving after "wol d"
>     would still assert INTn, and a later "wol g" re-enabled WoL from a
>     dirty register state.

This describes a defect in released behaviour, but there is no Fixes: tag.

The leftover state comes from the original 88E1318S WoL support, commit
3871c3876f80 ("mv643xx_eth with 88E1318S: support Wake on LAN"), which
added m88e1318_set_wol() with the CSIER.WOL_EIE and LED[2]-as-INTn setup
and no matching teardown in the disable branch.  m88e1318_set_wol() is the
.set_wol callback for both the 88E1318S and the 88E1510 entries.

Would it make sense to add:

  Fixes: 3871c3876f80 ("mv643xx_eth with 88E1318S: support Wake on LAN")

or, if no backport is wanted, to state in the changelog that the residual
register state is cosmetic?

> diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
> index f71cffa884062..6f7c190880f2e 100644
> --- a/drivers/net/phy/marvell.c
> +++ b/drivers/net/phy/marvell.c
> @@ -2074,6 +2074,35 @@ static int m88e1318_set_wol(struct phy_device *phydev,
>  			goto error;
>  	}
>  
> +	if (!(wol->wolopts & (WAKE_MAGIC | WAKE_PHY))) {
> +		/* Fully disabled: undo the WoL interrupt setup done above,
> +		 * so a later re-enable starts from a clean state.
> +		 */
> +		err = marvell_write_page(phydev, MII_MARVELL_COPPER_PAGE);
> +		if (err < 0)
> +			goto error;
> +
> +		err = __phy_clear_bits(phydev, MII_88E1318S_PHY_CSIER,
> +				       MII_88E1318S_PHY_CSIER_WOL_EIE);
> +		if (err < 0)
> +			goto error;
> +
> +		err = marvell_write_page(phydev, MII_MARVELL_LED_PAGE);
> +		if (err < 0)
> +			goto error;
> +
> +		err = __phy_modify(phydev, MII_88E1318S_PHY_LED_TCR,
> +				   MII_88E1318S_PHY_LED_TCR_FORCE_INT |
> +				   MII_88E1318S_PHY_LED_TCR_INTn_ENABLE |
> +				   MII_88E1318S_PHY_LED_TCR_INT_ACTIVE_LOW,
> +				   phy_interrupt_is_valid(phydev) ?
> +					MII_88E1318S_PHY_LED_TCR_INTn_ENABLE |
> +					MII_88E1318S_PHY_LED_TCR_INT_ACTIVE_LOW :
> +					0);
> +		if (err < 0)
> +			goto error;
> +	}

[Severity: High]

Can this de-mux an INTn pin that is still in use on PHY_MAC_INTERRUPT
devices?

phy_interrupt_is_valid() in include/linux/phy.h is false for both PHY_POLL
and PHY_MAC_INTERRUPT:

  static inline bool phy_interrupt_is_valid(struct phy_device *phydev)
  {
	  return phydev->irq != PHY_POLL && phydev->irq != PHY_MAC_INTERRUPT;
  }

For PHY_MAC_INTERRUPT the PHY does drive its interrupt output, it is just
serviced by the MAC driver rather than by phylib.  In that case this
__phy_modify() clears INTn_ENABLE (the LED[2] pin mux) and INT_ACTIVE_LOW
(the polarity), so the pin stops being the interrupt output.

Does anything put those bits back afterwards?  m88e1318_config_init(),
which the 88E1510 reaches through m88e1510_config_init(), programs them
only in the interrupt-valid case:

  static int m88e1318_config_init(struct phy_device *phydev)
  {
	  if (phy_interrupt_is_valid(phydev)) {
		  int err = phy_modify_paged(
			  phydev, MII_MARVELL_LED_PAGE,
			  MII_88E1318S_PHY_LED_TCR,
			  MII_88E1318S_PHY_LED_TCR_FORCE_INT,
			  MII_88E1318S_PHY_LED_TCR_INTn_ENABLE |
			  MII_88E1318S_PHY_LED_TCR_INT_ACTIVE_LOW);
  ...

And PHY_MAC_INTERRUPT is not polled either, since marvell.c defines no
.update_stats and phy_polling_mode() then only returns true for PHY_POLL.
So link state changes would be neither interrupt-delivered nor polled
until the next config_init or hardware reset.

The block also runs for calls that never armed WoL at all, for example a
plain "ethtool -s ethX wol d" on a fresh interface, or a MAC driver
passing wolopts == 0 on shutdown or resume:

  ethtool_set_wol() -> MAC .set_wol -> phy_ethtool_set_wol() ->
  m88e1318_set_wol(wolopts == 0)

so the pin function can be changed on a device where this driver never
touched LED_TCR.  The same applies to a LED_TCR value set by strap or
bootloader, or by the DT "marvell,reg-init" property, which
marvell_of_reg_init() applies as arbitrary <page reg mask value> tuples and
can legitimately target page 3 register 0x12.

Related to that, the changelog says the patch will "restore LED[2] to its
prior use", but the enable path only does:

  /* Setup LED[2] as interrupt pin (active low) */
  err = __phy_modify(phydev, MII_88E1318S_PHY_LED_TCR,
		     MII_88E1318S_PHY_LED_TCR_FORCE_INT,
		     MII_88E1318S_PHY_LED_TCR_INTn_ENABLE |
		     MII_88E1318S_PHY_LED_TCR_INT_ACTIVE_LOW);

with no read of the previous value, so the undo path writes an assumed
value rather than the prior one.

Would it work to read and save the INTn_ENABLE/INT_ACTIVE_LOW bits when
WoL is armed and write those saved bits back here, or alternatively to
restrict the undo to the bits this driver itself set?

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