Re: [PATCH net] net: phy: marvell: keep WOL_EIE across interrupt reconfiguration

[netdev-bot+sashiko]

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

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

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

- [High] m88e1318_config_intr() deliberately keeps
  MII_88E1318S_PHY_CSIER_WOL_EIE set when phydev->interrupts ==…
- [Medium] Commit message misdescribes the trigger path.
- [Medium] After this patch no code in drivers/net/phy/marvell.c can ever
  clear MII_88E1318S_PHY_CSIER_WOL_EIE (BIT(7) of copper page register…

--

net: phy: marvell: keep WOL_EIE across interrupt reconfiguration

Copper page register 0x12 on the 88E1318S/88E1510 is both the interrupt
mask and the CSIER register holding the WoL interrupt enable bit, so the
patch gives those two PHYs a dedicated config_intr that preserves
MII_88E1318S_PHY_CSIER_WOL_EIE with a read-modify-write, plus a matching
handle_interrupt. Other Marvell PHYs keep marvell_config_intr() and
marvell_handle_interrupt().

A few questions below, first about the changelog and then about the new
disable path.

> On the 88E1318S/88E1510, copper page register 0x12 is both the
> MII_M1011_IMASK interrupt mask used by marvell_config_intr() and the
> CSIER register in which m88e1318_set_wol() arms the WoL interrupt
> (MII_88E1318S_PHY_CSIER_WOL_EIE). marvell_config_intr() rewrote the
> whole register, so any interrupt reconfiguration (for example the
> phy_disable_interrupts() on link down behind mvneta) silently cleared
> WOL_EIE

[Severity: Medium]
Is "the phy_disable_interrupts() on link down behind mvneta" accurate?
phy_disable_interrupts() has only two callers:

drivers/net/phy/phy_device.c:phy_probe()
drivers/net/phy/phy.c:phy_free_interrupt()

	void phy_free_interrupt(struct phy_device *phydev)
	{
		phy_disable_interrupts(phydev);
		free_irq(phydev->irq, phydev);
	}

and for mvneta the only route into phy_free_interrupt() is
mvneta_stop() -> mvneta_mdio_remove() -> phylink_disconnect_phy() ->
phy_disconnect().  Link state changes go through the phylink resolve /
mac_link_down path and do not call config_intr at all.

So is the reproducer ifdown/ifup (and probe) rather than a link flap?
Note also that the enable side, phy_request_interrupt() ->
phy_enable_interrupts(), rewrites the same register, which the changelog
does not mention.

> diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
> index f71cffa884062..0897cfcd6ab70 100644
> --- a/drivers/net/phy/marvell.c
> +++ b/drivers/net/phy/marvell.c
> @@ -425,6 +425,68 @@ static irqreturn_t marvell_handle_interrupt(struct phy_device *phydev)
>  	return IRQ_HANDLED;
>  }
>  
> +/*
> + * On the 88E1318S/88E1510, copper page register 0x12 serves two
> + * masters: it is the MII_M1011_IMASK interrupt mask for the generic
> + * Marvell interrupt handling, and m88e1318_set_wol() sets the WoL
> + * interrupt enable bit (MII_88E1318S_PHY_CSIER_WOL_EIE) in it. The
> + * interrupt routines below therefore preserve that bit, so reconfiguring
> + * the PHY interrupts cannot disarm Wake-on-LAN behind the user's back.
> + */

[Severity: Medium]
Same framing question here as in the changelog: since this callback only
runs from phy_probe(), phy_request_interrupt() and phy_free_interrupt(),
would it be clearer to say the bit is preserved across probe, ifup and
ifdown instead of "reconfiguring the PHY interrupts"?

> +static int m88e1318_config_intr(struct phy_device *phydev)
> +{
> +	int val, err;
> +
> +	val = phy_read(phydev, MII_88E1318S_PHY_CSIER);
> +	if (val < 0)
> +		return val;
> +
> +	if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
> +		err = marvell_ack_interrupt(phydev);
> +		if (err < 0)
> +			return err;
> +
> +		err = phy_write(phydev, MII_88E1318S_PHY_CSIER,
> +				MII_M1011_IMASK_INIT |
> +				(val & MII_88E1318S_PHY_CSIER_WOL_EIE));
> +	} else {
> +		/* Disable the PHY interrupts, but keep WOL_EIE set so an
> +		 * armed magic packet still asserts INTn while the
> +		 * interface is down or the machine is suspended.
> +		 */
> +		err = phy_write(phydev, MII_88E1318S_PHY_CSIER,
> +				val & MII_88E1318S_PHY_CSIER_WOL_EIE);

[Severity: High]
Is it safe to leave WOL_EIE armed in the PHY_INTERRUPT_DISABLED branch,
given what phylib does right after calling it?

	void phy_free_interrupt(struct phy_device *phydev)
	{
		phy_disable_interrupts(phydev);
		free_irq(phydev->irq, phydev);
	}

After ifdown the WoL matcher is still enabled (m88e1318_set_wol() only
clears MAGIC_PACKET_MATCH_ENABLE when the user disables WoL), so a magic
packet can assert INTn once the handler is gone.  The existing comment in
m88e1318_set_wol() says that assertion is latched:

	/* If WOL event happened once, the LED[2] interrupt pin
	 * will not be cleared unless we reading the interrupt status
	 * register.

and reading MII_M1011_IEVENT is exactly what only the freed handler does.

Since phy_request_interrupt() asks for the line with

	request_threaded_irq(phydev->irq, NULL, phy_interrupt,
			     IRQF_ONESHOT | IRQF_SHARED,
			     phydev_name(phydev), phydev);

can a shared level-triggered line end up permanently asserted here, with
the co-user's handler returning IRQ_NONE until genirq's "nobody cared"
logic disables the line for that other device too?

Related question about the probe side: phy_probe() also calls
phy_disable_interrupts() before any IRQ has been requested, so this branch
now carries over a WOL_EIE left set by the bootloader or a previous module
load.  Was that intended?

Since WoL across suspend goes through phy_suspend()/drv->suspend rather
than config_intr, could the bit be preserved only where the WoL arming
needs it, instead of in the generic interrupt-disable callback?

[Severity: Medium]
With both branches writing back "val & MII_88E1318S_PHY_CSIER_WOL_EIE", is
there anything left in marvell.c that can clear bit 7 of copper register
0x12?

m88e1318_set_wol() only ever sets it, inside the wolopts branch:

	err = __phy_set_bits(phydev, MII_88E1318S_PHY_CSIER,
			     MII_88E1318S_PHY_CSIER_WOL_EIE);

and the WoL-disable branches touch only MII_88E1318S_PHY_WOL_CTRL on the
WoL page.  The unconditional write in marvell_config_intr():

	err = phy_write(phydev, MII_M1011_IMASK,
			MII_M1011_IMASK_CLEAR);

was the only path that returned bit 7 to zero, and these two PHYs no
longer use it.

So after a single "ethtool -s ethX wol g" does WOL_EIE stay set for the
lifetime of the PHY, including after "ethtool -s ethX wol d"?
m88e1318_get_wol() derives wolopts from the WoL-page WOL_CTRL bits only:

	ret = phy_read_paged(phydev, MII_MARVELL_WOL_PAGE,
			     MII_88E1318S_PHY_WOL_CTRL);

so userspace would report wol=d while CSIER bit 7 remains armed.  Should
m88e1318_set_wol()'s disable path clear WOL_EIE so the preservation in
config_intr has a matching clear?

> +		if (err < 0)
> +			return err;
> +
> +		err = marvell_ack_interrupt(phydev);
> +	}
> +
> +	return err;
> +}
> +

[ ... ]

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