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

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

[Prashant Sengar]

Use phylink's Wake-on-LAN helpers to handle PHY and MAC
wakeup capabilities, allowing MAC-based WoL configuration
when no PHY is attached.

Add a MAC WoL callback and advertise the supported MAC wakeup
modes through phylink. Synchronize the effective WoL settings
with the driver state used during suspend.

Preserve the default magic-packet wakeup configuration and
restore the previous USB wakeup state if WoL configuration
fails.

Tested with QEMU Raw Gadget emulating a PHY-less LAN7801.
Verified WoL configuration, unsupported mode rejection,
and USB wakeup state transitions.

Signed-off-by: Prashant Sengar <prashantsengar25031@gmail.com>
---
 drivers/net/usb/lan78xx.c | 84 +++++++++++++++++++++++++++++++--------
 1 file changed, 67 insertions(+), 17 deletions(-)

diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index d73002b2c21a..7d67dd150271 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -1765,13 +1765,6 @@ static void lan78xx_get_wol(struct net_device *netdev,
 	struct lan78xx_net *dev = netdev_priv(netdev);
 	int ret;
 	u32 buf;
-	struct lan78xx_priv *pdata = (struct lan78xx_priv *)(dev->data[0]);
-
-	if (!netdev->phydev) {
-		wol->supported = 0;
-		wol->wolopts = 0;
-		return;
-	}
 
 	if (usb_autopm_get_interface(dev->intf) < 0)
 		return;
@@ -1783,8 +1776,7 @@ static void lan78xx_get_wol(struct net_device *netdev,
 		wol->wolopts = 0;
 	} else {
 		if (buf & USB_CFG_RMT_WKP_) {
-			wol->supported = WAKE_ALL;
-			wol->wolopts = pdata->wol;
+			phylink_ethtool_get_wol(dev->phylink, wol);
 		} else {
 			wol->supported = 0;
 			wol->wolopts = 0;
@@ -1799,25 +1791,45 @@ static int lan78xx_set_wol(struct net_device *netdev,
 {
 	struct lan78xx_net *dev = netdev_priv(netdev);
 	struct lan78xx_priv *pdata = (struct lan78xx_priv *)(dev->data[0]);
+	struct ethtool_wolinfo actual = { .cmd = ETHTOOL_GWOL };
+	bool old_wakeup, enable;
+	int rollback_ret;
 	int ret;
 
 	if (wol->wolopts & ~WAKE_ALL)
 		return -EINVAL;
 
-	if (!netdev->phydev)
-		return -EOPNOTSUPP;
-
 	ret = usb_autopm_get_interface(dev->intf);
 	if (ret < 0)
 		return ret;
 
-	pdata->wol = wol->wolopts;
-
-	ret = device_set_wakeup_enable(&dev->udev->dev, (bool)wol->wolopts);
-	if (ret < 0)
+	phylink_ethtool_get_wol(dev->phylink, &actual);
+	if (wol->wolopts & ~actual.supported) {
+		ret = -EINVAL;
 		goto exit_pm_put;
+	}
+
+	old_wakeup = device_may_wakeup(&dev->udev->dev);
+	enable = !!wol->wolopts;
 
-	ret = phy_ethtool_set_wol(netdev->phydev, wol);
+	if (old_wakeup != enable) {
+		ret = device_set_wakeup_enable(&dev->udev->dev, enable);
+		if (ret < 0)
+			goto exit_pm_put;
+	}
+
+	ret = phylink_ethtool_set_wol(dev->phylink, wol);
+	if (ret < 0) {
+		if (old_wakeup != enable) {
+			rollback_ret = device_set_wakeup_enable(&dev->udev->dev, old_wakeup);
+			if (rollback_ret)
+				netdev_warn(dev->net, "USB wakeup rollback failed: %d\n",
+					    rollback_ret);
+		}
+		goto exit_pm_put;
+	}
+	phylink_ethtool_get_wol(dev->phylink, &actual);
+	pdata->wol = actual.wolopts;
 
 exit_pm_put:
 	usb_autopm_put_interface(dev->intf);
@@ -2617,12 +2629,24 @@ static int lan78xx_mac_enable_tx_lpi(struct phylink_config *config, u32 timer,
 	return lan78xx_mac_eee_enable(dev, true);
 }
 
+static int lan78xx_mac_wol_set(struct phylink_config *config,
+			       u32 wolopts, const u8 *sopass)
+{
+	struct lan78xx_net *dev = netdev_priv(to_net_dev(config->dev));
+	struct lan78xx_priv *pdata = (struct lan78xx_priv *)dev->data[0];
+
+	pdata->wol = wolopts;
+
+	return 0;
+}
+
 static const struct phylink_mac_ops lan78xx_phylink_mac_ops = {
 	.mac_config = lan78xx_mac_config,
 	.mac_link_down = lan78xx_mac_link_down,
 	.mac_link_up = lan78xx_mac_link_up,
 	.mac_disable_tx_lpi = lan78xx_mac_disable_tx_lpi,
 	.mac_enable_tx_lpi = lan78xx_mac_enable_tx_lpi,
+	.mac_wol_set = lan78xx_mac_wol_set,
 };
 
 /**
@@ -2848,6 +2872,10 @@ static int lan78xx_phylink_setup(struct lan78xx_net *dev)
 	       dev->phylink_config.supported_interfaces,
 	       sizeof(dev->phylink_config.lpi_interfaces));
 
+	pc->wol_mac_support = WAKE_MAGIC | WAKE_UCAST | WAKE_BCAST |
+		      WAKE_MCAST | WAKE_ARP;
+	pc->wol_phy_legacy = true;
+
 	phylink = phylink_create(pc, dev->net->dev.fwnode,
 				 dev->interface, &lan78xx_phylink_mac_ops);
 	if (IS_ERR(phylink))
@@ -4671,6 +4699,10 @@ static void lan78xx_stat_monitor(struct timer_list *t)
 static int lan78xx_probe(struct usb_interface *intf,
 			 const struct usb_device_id *id)
 {
+	struct ethtool_wolinfo wol = {
+		.cmd = ETHTOOL_SWOL,
+		.wolopts = WAKE_MAGIC,
+	};
 	struct usb_host_endpoint *ep_blkin, *ep_blkout, *ep_intr;
 	struct lan78xx_net *dev;
 	struct net_device *netdev;
@@ -4803,6 +4835,24 @@ static int lan78xx_probe(struct usb_interface *intf,
 	if (ret < 0)
 		goto free_urbs;
 
+	/* Preserve the default magic-packet wakeup configuration. */
+	rtnl_lock();
+
+	ret = phylink_ethtool_set_wol(dev->phylink, &wol);
+	if (!ret) {
+		struct lan78xx_priv *pdata = (struct lan78xx_priv *)dev->data[0];
+
+		wol.cmd = ETHTOOL_GWOL;
+		phylink_ethtool_get_wol(dev->phylink, &wol);
+		pdata->wol = wol.wolopts;
+		netdev->ethtool->wol_enabled = !!wol.wolopts;
+	}
+
+	rtnl_unlock();
+
+	if (ret)
+		netdev_warn(netdev, "failed to initialize WoL: %d\n", ret);
+
 	ret = register_netdev(netdev);
 	if (ret != 0) {
 		netif_err(dev, probe, netdev, "couldn't register the device\n");
-- 
2.55.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [High] net: usb: lan78xx: phylink MAC WoL breaks suspend and
  lan78xx_resume()
- [Medium] net: usb: lan78xx: missing WAKE_PHY in
  lan78xx_phylink_setup() breaks PHY WoL
- [Medium] net: usb: lan78xx: pdata->wol overwrite conflicts with
  phylink PHY WoL
- [Medium] net: usb: lan78xx: inconsistent WoL state in lan78xx_probe()
  and lan78xx_set_wol()
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [High] net: usb: lan78xx: missing rtnl_lock() in lan78xx_phy_uninit()
  [1]
--

commit 2aa41b74c1faf1ee028bdb9727321bf174890b23
Author: Prashant Sengar <prashantsengar25031@gmail.com>

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

This commit converts lan78xx Wake-on-LAN handling to use phylink helpers
and adds a phylink .mac_wol_set callback so MAC-based Wake-on-LAN can be
configured with or without an attached PHY.

> diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
> index d73002b2c21ac..7d67dd150271b 100644
> --- a/drivers/net/usb/lan78xx.c
> +++ b/drivers/net/usb/lan78xx.c
[ ... ]
> @@ -1799,25 +1791,45 @@ static int lan78xx_set_wol(struct net_device *netdev,
>  {
>  	struct lan78xx_net *dev = netdev_priv(netdev);
>  	struct lan78xx_priv *pdata = (struct lan78xx_priv *)(dev->data[0]);
> +	struct ethtool_wolinfo actual = { .cmd = ETHTOOL_GWOL };
> +	bool old_wakeup, enable;
> +	int rollback_ret;
>  	int ret;
>  
>  	if (wol->wolopts & ~WAKE_ALL)
>  		return -EINVAL;
>  
> -	if (!netdev->phydev)
> -		return -EOPNOTSUPP;
> -
>  	ret = usb_autopm_get_interface(dev->intf);
>  	if (ret < 0)
>  		return ret;
>  
> -	pdata->wol = wol->wolopts;
> -
> -	ret = device_set_wakeup_enable(&dev->udev->dev, (bool)wol->wolopts);
> -	if (ret < 0)
> +	phylink_ethtool_get_wol(dev->phylink, &actual);
> +	if (wol->wolopts & ~actual.supported) {
> +		ret = -EINVAL;
>  		goto exit_pm_put;
> +	}
> +
> +	old_wakeup = device_may_wakeup(&dev->udev->dev);
> +	enable = !!wol->wolopts;
>  
> -	ret = phy_ethtool_set_wol(netdev->phydev, wol);
> +	if (old_wakeup != enable) {
> +		ret = device_set_wakeup_enable(&dev->udev->dev, enable);
> +		if (ret < 0)
> +			goto exit_pm_put;
> +	}
> +
> +	ret = phylink_ethtool_set_wol(dev->phylink, wol);
> +	if (ret < 0) {
> +		if (old_wakeup != enable) {
> +			rollback_ret = device_set_wakeup_enable(&dev->udev->dev, old_wakeup);
> +			if (rollback_ret)
> +				netdev_warn(dev->net, "USB wakeup rollback failed: %d\n",
> +					    rollback_ret);
> +		}
> +		goto exit_pm_put;
> +	}
> +	phylink_ethtool_get_wol(dev->phylink, &actual);
> +	pdata->wol = actual.wolopts;

[Severity: Medium]
Does overwriting pdata->wol with actual.wolopts here (and in
lan78xx_probe()) desynchronize pdata->wol from phylink's pl->wolopts_mac
when an attached PHY handles Wake-on-LAN?

With pc->wol_phy_legacy set, phylink_ethtool_set_wol() first passes the
requested options to phy_ethtool_set_wol() and strips any bits reported by
phy_ethtool_get_wol() before calling lan78xx_mac_wol_set() and recording
pl->wolopts_mac.  On a LAN7801 with an external RGMII PHY whose driver
implements .set_wol and .get_wol for WAKE_MAGIC, phylink delegates
WAKE_MAGIC to the PHY and sets pl->wolopts_mac to 0, but reading back
phylink_ethtool_get_wol() overwrites pdata->wol with the combined PHY+MAC
mask (WAKE_MAGIC).  On suspend, phylink_suspend() then sees
pl->wolopts_mac == 0 and stops the MAC via phylink_stop(), while
lan78xx_set_suspend() programs WUCSR_MPEN_ on the stopped MAC without
enabling PMT_CTL_PHY_WAKE_EN_ for the PHY wakeup interrupt.

In addition, checking wol->wolopts & ~WAKE_ALL at the top of
lan78xx_set_wol() rejects PHY-supported modes outside WAKE_ALL (such as
WAKE_MAGICSECURE or WAKE_FILTER) even when lan78xx_get_wol() advertises
them via phylink_ethtool_get_wol().

> @@ -2617,12 +2629,24 @@ static int lan78xx_mac_enable_tx_lpi(struct phylink_config *config, u32 timer,
>  	return lan78xx_mac_eee_enable(dev, true);
>  }
>  
> +static int lan78xx_mac_wol_set(struct phylink_config *config,
> +			       u32 wolopts, const u8 *sopass)
> +{
> +	struct lan78xx_net *dev = netdev_priv(to_net_dev(config->dev));
> +	struct lan78xx_priv *pdata = (struct lan78xx_priv *)dev->data[0];
> +
> +	pdata->wol = wolopts;
> +
> +	return 0;
> +}
> +
>  static const struct phylink_mac_ops lan78xx_phylink_mac_ops = {
>  	.mac_config = lan78xx_mac_config,
>  	.mac_link_down = lan78xx_mac_link_down,
>  	.mac_link_up = lan78xx_mac_link_up,
>  	.mac_disable_tx_lpi = lan78xx_mac_disable_tx_lpi,
>  	.mac_enable_tx_lpi = lan78xx_mac_enable_tx_lpi,
> +	.mac_wol_set = lan78xx_mac_wol_set,
>  };

[Severity: High]
Does populating .mac_wol_set break link recovery across lan78xx_suspend()
and lan78xx_resume()?

Once .mac_wol_set is implemented, phylink_mac_supports_wol() returns true
and phylink_suspend() ignores its mac_wol parameter in favor of
!!pl->wolopts_mac:

drivers/net/phy/phylink.c:phylink_suspend() {
	ASSERT_RTNL();

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

	if (mac_wol) {
		/* Wake-on-Lan enabled, MAC handling */
		mutex_lock(&pl->state_mutex);
		/* Stop theWatchdog, but keep link up */
		__set_bit(PHYLINK_DISABLE_MAC_WOL, &pl->phylink_disable_state);
		mutex_unlock(&pl->state_mutex);

		netif_carrier_off(pl->netdev);
	} else {
		phylink_stop(pl);
	}
}

Because lan78xx_probe() enables WAKE_MAGIC by default, phylink_suspend() in
lan78xx_suspend() (which runs on both runtime autosuspend after the 10s
idle delay and system suspend) now sets PHYLINK_DISABLE_MAC_WOL and drops
carrier instead of calling phylink_stop().  However, lan78xx_resume() never
calls phylink_resume() (only lan78xx_reset_resume() does), and
PHYLINK_DISABLE_MAC_WOL is only cleared by phylink_resume().  After the
first runtime autoresume or non-reset system resume, PHYLINK_DISABLE_MAC_WOL
remains permanently set, leaving carrier off, dropping deferred TX packets
in lan78xx_submit_deferred_urbs(), and preventing phylink_resolve() from
bringing the link back up even if the interface is cycled through
lan78xx_stop() and lan78xx_open().

Skipping phylink_stop() during lan78xx_suspend() also leaves the phylib
state machine running in PHY_POLL mode on LAN7800/LAN7850 (since
pc->mac_managed_pm is true), so phy_state_machine() fires 1 second into USB
runtime autosuspend and calls lan78xx_mdiobus_read() ->
usb_autopm_get_interface(), waking the device back up.

> @@ -2848,6 +2872,10 @@ static int lan78xx_phylink_setup(struct lan78xx_net *dev)
>  	       dev->phylink_config.supported_interfaces,
>  	       sizeof(dev->phylink_config.lpi_interfaces));
>  
> +	pc->wol_mac_support = WAKE_MAGIC | WAKE_UCAST | WAKE_BCAST |
> +		      WAKE_MCAST | WAKE_ARP;
> +	pc->wol_phy_legacy = true;
> +
>  	phylink = phylink_create(pc, dev->net->dev.fwnode,
>  				 dev->interface, &lan78xx_phylink_mac_ops);

[Severity: Medium]
Does omitting WAKE_PHY from pc->wol_mac_support regress link-change
Wake-on-LAN on LAN7800 and LAN7850?

Previously lan78xx_get_wol() advertised WAKE_ALL and lan78xx_set_wol()
accepted WAKE_PHY, which lan78xx_set_suspend() uses to program
PMT_CTL_PHY_WAKE_EN_ in the MAC's PMT_CTL register.  The integrated PHY on
LAN7800 and LAN7850 uses the Microchip LAN88xx driver
(drivers/net/phy/microchip.c), which implements .set_wol = lan88xx_set_wol
but does not implement .get_wol.  Because phy_ethtool_get_wol() leaves
wol->supported untouched when .get_wol is NULL, phylink_ethtool_get_wol()
only reports pc->wol_mac_support, so WAKE_PHY is no longer advertised by
lan78xx_get_wol() and is rejected with -EINVAL by lan78xx_set_wol().

[ ... ]

> @@ -4803,6 +4835,24 @@ static int lan78xx_probe(struct usb_interface *intf,
>  	if (ret < 0)
>  		goto free_urbs;
>  
> +	/* Preserve the default magic-packet wakeup configuration. */
> +	rtnl_lock();
> +
> +	ret = phylink_ethtool_set_wol(dev->phylink, &wol);
> +	if (!ret) {
> +		struct lan78xx_priv *pdata = (struct lan78xx_priv *)dev->data[0];
> +
> +		wol.cmd = ETHTOOL_GWOL;
> +		phylink_ethtool_get_wol(dev->phylink, &wol);
> +		pdata->wol = wol.wolopts;
> +		netdev->ethtool->wol_enabled = !!wol.wolopts;
> +	}
> +
> +	rtnl_unlock();
> +
> +	if (ret)
> +		netdev_warn(netdev, "failed to initialize WoL: %d\n", ret);
> +
>  	ret = register_netdev(netdev);

[Severity: Medium]
Can this leave the driver's Wake-on-LAN state inconsistent with the
hardware and USB wakeup settings?

First, lan78xx_bind() pre-sets pdata->wol = WAKE_MAGIC, and after
register_netdev() succeeds, lan78xx_probe() unconditionally calls
device_set_wakeup_enable(&udev->dev, true).  If phylink_ethtool_set_wol()
fails here, phylink and netdev->ethtool->wol_enabled treat WoL as disabled,
while pdata->wol remains WAKE_MAGIC and udev->dev wakeup is still enabled;
calling device_set_wakeup_enable() after register_netdev() also races with
userspace changing WoL immediately after netdev registration.

Second, lan78xx_get_wol() checks USB_CFG_RMT_WKP_ in USB_CFG0 and reports
wol->supported = 0 and wol->wolopts = 0 when clear, but neither
lan78xx_probe() nor lan78xx_set_wol() checks USB_CFG_RMT_WKP_.  When
USB_CFG_RMT_WKP_ is not set, lan78xx_probe() and lan78xx_set_wol() still
enable WoL in phylink, pdata->wol, and udev->dev while lan78xx_get_wol()
reports WoL as unsupported and disabled.

[Severity: High]
This problem wasn't introduced by this patch, but if register_netdev()
fails right below (or if lan78xx_phy_init() fails after
lan78xx_phylink_setup()), the error path calls lan78xx_phy_uninit() ->
phylink_disconnect_phy() without holding rtnl_lock() [1], triggering
ASSERT_RTNL() in phylink_disconnect_phy().

[1] https://sashiko.dev/#/bug/linux-f22d0679-ab9c-4543-b756-3c93ec69c79d

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261108180137.3488023-1-prashantsengar25031@gmail.com?part=1

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

[netdev-bot+sashiko]

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.

> diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
> index d73002b2c21ac..7d67dd150271b 100644
> --- a/drivers/net/usb/lan78xx.c
> +++ b/drivers/net/usb/lan78xx.c

[ ... ]

> @@ -2617,12 +2629,24 @@ static int lan78xx_mac_enable_tx_lpi(struct phylink_config *config, u32 timer,
>  	return lan78xx_mac_eee_enable(dev, true);
>  }
>  
> +static int lan78xx_mac_wol_set(struct phylink_config *config,
> +			       u32 wolopts, const u8 *sopass)
> +{
> +	struct lan78xx_net *dev = netdev_priv(to_net_dev(config->dev));
> +	struct lan78xx_priv *pdata = (struct lan78xx_priv *)dev->data[0];
> +
> +	pdata->wol = wolopts;

[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?

> +
> +	return 0;
> +}
> +
>  static const struct phylink_mac_ops lan78xx_phylink_mac_ops = {
>  	.mac_config = lan78xx_mac_config,
>  	.mac_link_down = lan78xx_mac_link_down,
>  	.mac_link_up = lan78xx_mac_link_up,
>  	.mac_disable_tx_lpi = lan78xx_mac_disable_tx_lpi,
>  	.mac_enable_tx_lpi = lan78xx_mac_enable_tx_lpi,
> +	.mac_wol_set = lan78xx_mac_wol_set,

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

>  };

[ ... ]

> @@ -2848,6 +2872,10 @@ static int lan78xx_phylink_setup(struct lan78xx_net *dev)
>  	       dev->phylink_config.supported_interfaces,
>  	       sizeof(dev->phylink_config.lpi_interfaces));
>  
> +	pc->wol_mac_support = WAKE_MAGIC | WAKE_UCAST | WAKE_BCAST |
> +		      WAKE_MCAST | WAKE_ARP;

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

[ ... ]

> @@ -4803,6 +4835,24 @@ static int lan78xx_probe(struct usb_interface *intf,
>  	if (ret < 0)
>  		goto free_urbs;
>  
> +	/* Preserve the default magic-packet wakeup configuration. */
> +	rtnl_lock();
> +
> +	ret = phylink_ethtool_set_wol(dev->phylink, &wol);
> +	if (!ret) {
> +		struct lan78xx_priv *pdata = (struct lan78xx_priv *)dev->data[0];
> +
> +		wol.cmd = ETHTOOL_GWOL;
> +		phylink_ethtool_get_wol(dev->phylink, &wol);
> +		pdata->wol = wol.wolopts;
> +		netdev->ethtool->wol_enabled = !!wol.wolopts;

[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?

> +	}
> +
> +	rtnl_unlock();
> +
> +	if (ret)
> +		netdev_warn(netdev, "failed to initialize WoL: %d\n", ret);

[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

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

[Prashant Sengar]

Thanks for the detailed review.

The review identified valid concerns with PHY/MAC WoL ownership,
supported modes, and the default configuration. I'll address these in
a v2.

I also noted that the normal phylink resume issue has been fixed
upstream by commit be9587c47db1. I'll account for that fix and check
the remaining reset-resume interaction.

pw-bot: cr

On Tue, Nov 10, 2026 at 12:13 AM <netdev-bot+sashiko@kernel.org> wrote:
>
> 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.
>
> > diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
> > index d73002b2c21ac..7d67dd150271b 100644
> > --- a/drivers/net/usb/lan78xx.c
> > +++ b/drivers/net/usb/lan78xx.c
>
> [ ... ]
>
> > @@ -2617,12 +2629,24 @@ static int lan78xx_mac_enable_tx_lpi(struct phylink_config *config, u32 timer,
> >       return lan78xx_mac_eee_enable(dev, true);
> >  }
> >
> > +static int lan78xx_mac_wol_set(struct phylink_config *config,
> > +                            u32 wolopts, const u8 *sopass)
> > +{
> > +     struct lan78xx_net *dev = netdev_priv(to_net_dev(config->dev));
> > +     struct lan78xx_priv *pdata = (struct lan78xx_priv *)dev->data[0];
> > +
> > +     pdata->wol = wolopts;
>
> [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?
>
> > +
> > +     return 0;
> > +}
> > +
> >  static const struct phylink_mac_ops lan78xx_phylink_mac_ops = {
> >       .mac_config = lan78xx_mac_config,
> >       .mac_link_down = lan78xx_mac_link_down,
> >       .mac_link_up = lan78xx_mac_link_up,
> >       .mac_disable_tx_lpi = lan78xx_mac_disable_tx_lpi,
> >       .mac_enable_tx_lpi = lan78xx_mac_enable_tx_lpi,
> > +     .mac_wol_set = lan78xx_mac_wol_set,
>
> [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.
>
> >  };
>
> [ ... ]
>
> > @@ -2848,6 +2872,10 @@ static int lan78xx_phylink_setup(struct lan78xx_net *dev)
> >              dev->phylink_config.supported_interfaces,
> >              sizeof(dev->phylink_config.lpi_interfaces));
> >
> > +     pc->wol_mac_support = WAKE_MAGIC | WAKE_UCAST | WAKE_BCAST |
> > +                   WAKE_MCAST | WAKE_ARP;
>
> [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;
> > +
>
> [ ... ]
>
> > @@ -4803,6 +4835,24 @@ static int lan78xx_probe(struct usb_interface *intf,
> >       if (ret < 0)
> >               goto free_urbs;
> >
> > +     /* Preserve the default magic-packet wakeup configuration. */
> > +     rtnl_lock();
> > +
> > +     ret = phylink_ethtool_set_wol(dev->phylink, &wol);
> > +     if (!ret) {
> > +             struct lan78xx_priv *pdata = (struct lan78xx_priv *)dev->data[0];
> > +
> > +             wol.cmd = ETHTOOL_GWOL;
> > +             phylink_ethtool_get_wol(dev->phylink, &wol);
> > +             pdata->wol = wol.wolopts;
> > +             netdev->ethtool->wol_enabled = !!wol.wolopts;
>
> [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?
>
> > +     }
> > +
> > +     rtnl_unlock();
> > +
> > +     if (ret)
> > +             netdev_warn(netdev, "failed to initialize WoL: %d\n", ret);
>
> [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