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

[netdev-bot+sashiko@kernel.org]

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

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

- [Medium] The new arm/disarm pairing for the LED[2]/INTn pin mux keeps
  its bookkeeping (priv->wol_led_armed, priv->wol_led_tcr) in devm-…
- [Low] The comment added at drivers/net/phy/marvell.c:2097-2103 makes
  three statements the adjacent code does not implement.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it directly
  undermines the register-state guarantee the patch adds and should be…

--

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 restores the LED[2]/INTn pin mux to
the value that was read when WoL was first armed. The saved value and an
"armed" flag are kept in struct marvell_priv.

A few questions about the new save/restore mechanism below.

> diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
> index f71cffa884062..64862f12eb42f 100644
> --- a/drivers/net/phy/marvell.c
> +++ b/drivers/net/phy/marvell.c
> @@ -354,6 +354,8 @@ struct marvell_priv {
>  	u32 step;
>  	s8 pair;
>  	u8 vct_phase;
> +	u16 wol_led_tcr;
> +	bool wol_led_armed;
>  };
>  
>  static int marvell_read_page(struct phy_device *phydev)

[ ... ]

> @@ -2000,6 +2003,22 @@ static int m88e1318_set_wol(struct phy_device *phydev,
>  		if (err < 0)
>  			goto error;
>  
> +		/* Remember the LED[2]/INTn mux before forcing the pin to
> +		 * INTn, so it can be restored when WoL is disabled again.
> +		 * Only save it once, or a re-enable would overwrite the
> +		 * value read before WoL was first armed.
> +		 */
> +		if (!priv->wol_led_armed) {
> +			err = __phy_read(phydev, MII_88E1318S_PHY_LED_TCR);
> +			if (err < 0)
> +				goto error;
> +
> +			priv->wol_led_tcr = err &
> +				(MII_88E1318S_PHY_LED_TCR_INTn_ENABLE |
> +				 MII_88E1318S_PHY_LED_TCR_INT_ACTIVE_LOW);
> +			priv->wol_led_armed = true;
> +		}

[Severity: Medium]
Can the saved mux value survive an unbind while WoL is still armed?

priv is devm-allocated in marvell_probe():

	priv = devm_kzalloc(&phydev->mdio.dev, sizeof(*priv), GFP_KERNEL);
	if (!priv)
		return -ENOMEM;

	phydev->priv = priv;

marvell.c has no .remove callback in any of its phy_driver entries, and
m88e1318_set_wol() is only reachable from phy_ethtool_set_wol(), never
from detach or unbind with wolopts == 0.

So for the sequence "ethtool -s eth0 wol g", then unbind the PHY driver
(sysfs unbind or module unload), then bind again:

  - CSIER.WOL_EIE stays set and LED[2] stays muxed to INTn in hardware
  - priv, and with it wol_led_tcr / wol_led_armed, is freed
  - the fresh priv from devm_kzalloc() has wol_led_armed == false

The next "wol g" then snapshots the already-forced
INTn_ENABLE | INT_ACTIVE_LOW value as the pristine one, and the following
"wol d" restores exactly the forced value, so the pin can no longer be
returned to its LED function.

Should the saved state be re-derived from hardware, or the WoL setup
disarmed at driver teardown, so the arm/disarm pairing is balanced over the
device lifetime?

Also, priv->wol_led_armed = true is set before the __phy_modify() below
that actually forces the pin. If that MDIO write fails, the flag stays
latched true and the if (!priv->wol_led_armed) guard above prevents the
snapshot from ever being refreshed. Is that intentional?

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

[ ... ]

> @@ -2074,6 +2093,41 @@ 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. The
> +		 * LED[2]/INTn pin mux is only touched when it was forced
> +		 * to INTn here, and is restored to the value read at
> +		 * arming time; PHYs serving the MAC interrupt, straps, or
> +		 * marvell,reg-init values are left alone.
> +		 */

[Severity: Low]
This isn't a bug in the register programming, but does this comment match
what the code does?

"only touched when it was forced to INTn here": priv->wol_led_armed is set
unconditionally in the arm block, not only when the pin actually had to be
switched. On a PHY where m88e1318_config_init() already selected the INTn
function:

	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);

the disable path still writes LED_TCR. The written value happens to match
what config_init programmed, so only the comment seems off.

"restored to the value read at arming time" and "marvell,reg-init values
are left alone" do not hold for MII_88E1318S_PHY_LED_TCR_FORCE_INT, see
the restore below.

> +		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;

[Severity: High]
This isn't a bug introduced by this patch, but it seems to defeat the
register-state guarantee the patch is adding, so perhaps it needs to be
resolved together with it.

Copper page register 0x12 has two names in this driver, MII_M1011_IMASK
and MII_88E1318S_PHY_CSIER, and no single owner. m88e1318_set_wol()
touches only BIT(7) with a read-modify-write, while
marvell_config_intr() rewrites the whole register:

	if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
		...
		err = phy_write(phydev, MII_M1011_IMASK,
				MII_M1011_IMASK_INIT);
	} else {
		err = phy_write(phydev, MII_M1011_IMASK,
				MII_M1011_IMASK_CLEAR);

Neither MII_M1011_IMASK_INIT (0x6400) nor MII_M1011_IMASK_CLEAR (0x0000)
contains MII_88E1318S_PHY_CSIER_WOL_EIE.

On an interrupt-driven 88E1510 (for example behind mvneta, .config_intr =
marvell_config_intr), can this drop WoL arming behind the user's back?

  ethtool -s eth0 wol g
    m88e1318_set_wol()        /* sets CSIER.WOL_EIE, forces LED[2] */

  ip link set eth0 down
    mvneta_stop()
      mvneta_mdio_remove()
        phylink_disconnect_phy()
          phy_disconnect()
            phy_free_interrupt()
              phy_disable_interrupts()
                phy_config_interrupt(PHY_INTERRUPT_DISABLED)
                  marvell_config_intr()   /* writes 0x0000 to reg 0x12 */

After that, the WOL page magic packet matcher is still enabled and
m88e1318_get_wol() still reports WAKE_MAGIC, but WOL_EIE is gone, so a
matched magic packet is no longer routed to INTn/LED[2] and the board does
not wake.

Two consequences for this patch: the new clear of WOL_EIE here is a no-op
on those PHYs, so the "clean state" the comment promises only holds for
polled and MAC-interrupt PHYs. And marvell_handle_interrupt() masks
MII_M1011_IEVENT with MII_M1011_IMASK_INIT:

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

so a WoL event bit outside 0x6400 is never claimed anyway.

Would it make sense for marvell_config_intr() to preserve WOL_EIE with a
read-modify-write, the same way this patch preserves the LED_TCR bits it
does not own, or for set_wol to not depend on that bit?

> +
> +		if (!priv->wol_led_armed)
> +			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,
> +				   priv->wol_led_tcr);

[Severity: Low]
Continuing the comment question above: the write mask here includes
MII_88E1318S_PHY_LED_TCR_FORCE_INT, but the snapshot taken in the arm path
deliberately excludes it:

	priv->wol_led_tcr = err &
		(MII_88E1318S_PHY_LED_TCR_INTn_ENABLE |
		 MII_88E1318S_PHY_LED_TCR_INT_ACTIVE_LOW);

So FORCE_INT is driven to 0 unconditionally rather than to "the value read
at arming time". The arm path already cleared FORCE_INT before this patch,
and the commit message explains why FORCE_INT is not re-set, so the
register content does not look newly damaged; it is the comment's
guarantee about that bit, and about straps and marvell,reg-init being left
alone, that appears inaccurate.

Could the comment be reworded to say which LED_TCR bits the WoL path
actually owns?

> +		if (err < 0)
> +			goto error;
> +
> +		priv->wol_led_armed = false;
> +	}
> +
>  error:
>  	return phy_restore_page(phydev, oldpage, err);
>  }

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