Re: [PATCH net-next] net: usb: lan78xx: use phylink for Wake-on-LAN

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

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

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] Adding `.mac_wol_set` makes phylink_mac_supports_wol() true, so
  phylink_suspend() ignores the driver's `mac_wol=false` argument and…
- [Medium] This patch removes WAKE_PHY (`ethtool -s ethX wol p`) for
  lan78xx devices that have a PHY, and the commit message does not…
- [Medium] The commit message says probe 'preserves' the default magic-
  packet configuration, but the behaviour changes.
- [Low] `pdata->wol` has writers that disagree on what it means.
- [Low] If phylink_ethtool_set_wol() fails in lan78xx_probe(), for
  example because an external PHY's set_wol returns an error other than…

--

net: usb: lan78xx: use phylink for Wake-on-LAN

This patch moves the lan78xx ethtool WoL handlers to
phylink_ethtool_get_wol() and phylink_ethtool_set_wol(). It adds a
mac_wol_set callback and advertises MAC wake modes through
phylink_config. At probe, it now sends a default WAKE_MAGIC configuration
through phylink and sets netdev->ethtool->wol_enabled.

[ ... ]

[Severity: Low]
What is pdata->wol supposed to hold? The mac_wol_set kernel-doc in
include/linux/phylink.h says options the PHY can handle will already have
been removed from wolopts, so this stores the MAC-only set.

Both callers of phylink_ethtool_set_wol() overwrite pdata->wol right
afterwards with the merged PHY+MAC set:

lan78xx_set_wol() {
    ...
	ret = phylink_ethtool_set_wol(dev->phylink, wol);
	...
	phylink_ethtool_get_wol(dev->phylink, &actual);
	pdata->wol = actual.wolopts;
}

and phylink_ethtool_get_wol() does:

	wol->wolopts |= pl->wolopts_mac;

So is this store always dead?

lan78xx_suspend() passes the merged value to lan78xx_set_suspend().
phylink_suspend() decides from the MAC-only set instead:

	mac_wol = !!pl->wolopts_mac;

Take an external PHY on LAN7801 whose driver implements get_wol and
handles WAKE_MAGIC. There, wolopts_mac would be 0, so phylink calls
phylink_stop(), and lan78xx_mac_link_down() stops RX/TX and resets the
MAC.

The driver would still arm MAC magic-packet detection (WUCSR_MPEN_,
SUS_MODE_3) for a mode the PHY handles. It also would not set
PMT_CTL_PHY_WAKE_EN_ to pass on the PHY's wake.

Can the driver and phylink end up disagreeing about who handles WoL and
whether the MAC needs to keep receiving?

[Severity: High]
Does setting mac_wol_set leave the link permanently down after a normal
system resume?

With mac_wol_set set, phylink_mac_supports_wol() returns true, and
phylink_suspend() ignores the mac_wol argument from the driver:

phylink_suspend() {
	...
	if (phylink_mac_supports_wol(pl))
		mac_wol = !!pl->wolopts_mac;

	if (mac_wol && (!pl->netdev || pl->netdev->ethtool->wol_enabled)) {
		...
		__set_bit(PHYLINK_DISABLE_MAC_WOL, &pl->phylink_disable_state);
	...
}

By default, the new probe code sets pl->wolopts_mac = WAKE_MAGIC (lan88xx
has no get_wol, so all of it goes to the MAC) and sets wol_enabled to
true.

So with the interface open, every system suspend through
lan78xx_suspend()->phylink_suspend(dev->phylink, false) takes this
branch. It sets PHYLINK_DISABLE_MAC_WOL and turns carrier off, but does
not stop phylink or phylib.

Only phylink_resume() clears that bit, and lan78xx calls it only from
lan78xx_reset_resume(). lan78xx_resume() is used when the device keeps
power across sleep, and it has no phylink call.

After that, phylink_resolve() sees a non-zero phylink_disable_state and
forces the link down:

	if (pl->phylink_disable_state) {
		pl->link_failed = false;
		link_state = pl->link_config;
		link_state.link = false;

Before this patch, ip link set down/up recovered from this state, because
phylink_stop() sets STOPPED and phylink_start() clears it. phylink_start()
only clears PHYLINK_DISABLE_STOPPED, so the MAC_WOL bit now survives
ifdown/ifup and ethtool wol d.

Won't the interface stay without carrier until a reset_resume or a driver
rebind?

On the reset_resume path, lan78xx_reset() hard-resets the PHY through
PMT_CTL_PHY_RST_. Before, phylink_resume()->phylink_start()->phy_start()
brought phylib back up and re-ran aneg configuration.

Now phylink_resume() takes the MAC-WoL branch (mac_initial_config plus a
re-resolve). phylib was never halted, so it doesn't reprogram the freshly
reset PHY.

Are user-configured advertisement or forced link modes lost in that case?

Runtime autosuspend doesn't trigger this, since lan78xx_open() holds a
usb_autopm reference until lan78xx_stop(). The system suspend path alone
is enough.

 };

[ ... ]

[Severity: Medium]
Is WAKE_PHY meant to be left out here? The driver still includes WAKE_PHY
in WAKE_ALL, and lan78xx_set_suspend() still handles it:

	if (wol & WAKE_PHY) {
		temp_pmt_ctl |= PMT_CTL_PHY_WAKE_EN_;

Before the patch, lan78xx_get_wol() reported supported = WAKE_ALL. Now
the supported mask comes from phylink_ethtool_get_wol(), which is the PHY
driver's get_wol result OR'd with wol_mac_support.

The internal PHY uses the lan88xx driver in drivers/net/phy/microchip.c.
It has set_wol but no get_wol, so it adds nothing, and WAKE_PHY is never
advertised.

ethnl_set_wol() then rejects it:

	if (wol.wolopts & ~wol.supported) {

The new wol->wolopts & ~actual.supported check in lan78xx_set_wol() also
rejects it with -EINVAL.

So ethtool -s ethX wol p, wol pg, and systemd-networkd WakeOnLan=phy all
worked before and now fail. The PMT_CTL_PHY_WAKE_EN_ path in
lan78xx_set_suspend() can no longer be reached.

The commit message doesn't mention this change. The PHY-less LAN7801
testing wouldn't exercise this path.

Should WAKE_PHY be added to wol_mac_support, since the MAC implements it?

+	pc->wol_phy_legacy = true;
+

[ ... ]

[Severity: Medium]
The commit message says:

  Preserve the default magic-packet wakeup configuration

Does this code change the default PHY power behaviour, though?

Before the patch, lan78xx_bind() only set pdata->wol = WAKE_MAGIC. The
PHY was never told about WoL, and netdev->ethtool->wol_enabled stayed
false.

With wol_phy_legacy = true, phylink_phy_supports_wol() returns true
whenever a phydev is attached, so this call reaches
phy_ethtool_set_wol(). lan88xx_set_wol() then sets
priv->wolopts = WAKE_MAGIC, and external PHYs on LAN7801 get their WoL
hardware programmed.

With wol_enabled forced to true, phy_suspend() returns -EBUSY:

	phydev->wol_enabled = phy_may_wakeup(phydev) ||
			      (netdev && netdev->ethtool->wol_enabled);
	if (phydev->wol_enabled && !(phydrv->flags & PHY_ALWAYS_CALL_SUSPEND))
		return -EBUSY;

and lan88xx_suspend() skips genphy_suspend() while priv->wolopts is set:

	if (!priv->wolopts)
		genphy_suspend(phydev);

So the PHY would no longer be powered down when
phylink_stop()->phy_stop() halts it, including on a plain ip link set
down. In that case the !dev_open branch of lan78xx_suspend() disables all
wake sources anyway, so keeping the PHY powered doesn't seem to help.

Is this new default intended for every lan78xx device with a PHY?

[Severity: Low]
What happens to pdata->wol if phylink_ethtool_set_wol() fails here? One
example is an external PHY on LAN7801 whose set_wol returns an error
other than -EOPNOTSUPP, such as an MDIO or USB control transfer failure.

pdata->wol keeps the WAKE_MAGIC value set in lan78xx_bind(). Meanwhile
pl->wolopts_mac stays 0 and netdev->ethtool->wol_enabled stays false.

lan78xx_get_wol() would then report no active WoL through phylink, but
lan78xx_suspend() still does:

	ret = lan78xx_set_suspend(dev, pdata->wol);

and arms magic-packet wake (WUCSR_MPEN_).

Should pdata->wol be reset on this failure path so it matches what
phylink reports? This can't happen with the internal lan88xx PHY, whose
set_wol always returns 0.

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