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

[sashiko-bot@kernel.org]

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