Re: [PATCH net-next v9 3/7] r8169: add support for phylink

[netdev-bot+sashiko@kernel.org]

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

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

- [High] PM paths no longer stop/start the link state machine.
- [Medium] The commit message ("Transfer old framework to phylink...")
  does not mention the uAPI-visible behaviour changes the patch makes.
- [Medium] MAC Tx LPI (EEE) is silently disabled by default.
- [Medium] PHY-backed ethtool operations now fail or silently lose
  changes while the interface is down.
- [Low] Reported concern (assessed as resolved later in the series): at
  this commit the stated motivation ("Phylink can support fiber mode…

Pre-existing issues:
- [Medium] Reported concern (assessed as pre-existing and Medium,
  therefore discarded): rtl_mac_enable_tx_lpi() accepts phylink's…

--

r8169: add support for phylink

This converts r8169 from direct phylib use to phylink, adding a
phylink_config plus mac_link_up/mac_link_down/mac_config and Tx LPI
callbacks, and routes the ethtool link, pause and EEE operations through
phylink.  A few questions below about the PM paths, the EEE defaults and
the ethtool behaviour changes.

> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> index c60710f9bd21..823763e4f445 100644
> --- a/drivers/net/ethernet/realtek/r8169_main.c
> +++ b/drivers/net/ethernet/realtek/r8169_main.c

[ ... ]

> @@ -2253,7 +2256,7 @@ static int rtl8169_get_eee(struct net_device *dev, struct ethtool_keee *data)
>  	if (!rtl_supports_eee(tp))
>  		return -EOPNOTSUPP;
>  
> -	ret = phy_ethtool_get_eee(tp->phydev, data);
> +	ret = phylink_ethtool_get_eee(tp->phylink, data);
>  	if (ret)
>  		return ret;
>  
> @@ -2269,7 +2272,7 @@ static int rtl8169_set_eee(struct net_device *dev, struct ethtool_keee *data)
>  	if (!rtl_supports_eee(tp))
>  		return -EOPNOTSUPP;
>  
> -	return phy_ethtool_set_eee(tp->phydev, data);
> +	return phylink_ethtool_set_eee(tp->phylink, data);
>  }

[Severity: Medium]

Do these two callbacks still work while the interface is down?  tp->phydev
exists from probe onwards, but pl->phydev is only populated by
phylink_connect_phy() in rtl_open() and is cleared again by
phylink_disconnect_phy() in rtl8169_close().

phylink_ethtool_get_eee() leaves the return value at its initial value when
no PHY is attached:

drivers/net/phy/phylink.c:phylink_ethtool_get_eee() {
	int ret = -EOPNOTSUPP;
	...
	if (pl->phydev) {
		ret = phy_ethtool_get_eee(pl->phydev, eee);
	...
}

So after "ip link set ethX down", does "ethtool --show-eee ethX" (and
--set-eee) now return -EOPNOTSUPP where it previously operated on
tp->phydev directly?

The non-SFP path of rtl8169_set_link_ksettings() has a related question: with
no PHY attached, phylink_ethtool_ksettings_set() only updates phylink's own
link_config, and on the next open phylink_bringup_phy() rebuilds
config.advertising from phy->advertising.  Does a setting made while the
interface is down get silently dropped while still reporting success?

> @@ -2300,13 +2303,8 @@ static void rtl8169_get_pauseparam(struct net_device *dev,
>  				   struct ethtool_pauseparam *data)
>  {
>  	struct rtl8169_private *tp = netdev_priv(dev);
> -	bool tx_pause, rx_pause;
>  
> -	phy_get_pause(tp->phydev, &tx_pause, &rx_pause);
> -
> -	data->autoneg = tp->phydev->autoneg;
> -	data->tx_pause = tx_pause ? 1 : 0;
> -	data->rx_pause = rx_pause ? 1 : 0;
> +	phylink_ethtool_get_pauseparam(tp->phylink, data);
>  }
>  
>  static int rtl8169_set_pauseparam(struct net_device *dev,
> @@ -2314,12 +2312,10 @@ static int rtl8169_set_pauseparam(struct net_device *dev,
>  {
>  	struct rtl8169_private *tp = netdev_priv(dev);
>  
> -	if (dev->mtu > ETH_DATA_LEN)
> +	if (dev->mtu > ETH_DATA_LEN || data->autoneg == AUTONEG_DISABLE)
>  		return -EOPNOTSUPP;
>  
> -	phy_set_asym_pause(tp->phydev, data->rx_pause, data->tx_pause);
> -
> -	return 0;
> +	return phylink_ethtool_set_pauseparam(tp->phylink, data);
>  }

[Severity: Medium]

The new "|| data->autoneg == AUTONEG_DISABLE" test changes user visible
behaviour: "ethtool -A ethX autoneg off rx on tx on" previously ignored the
autoneg field, applied phy_set_asym_pause() and returned 0, and now returns
-EOPNOTSUPP.

The commit message says only:

    Transfer old framework to phylink. Phylink can support fiber mode card
    which can not get link status or link speed from standard phy registers.

Could the message describe this ethtool behaviour change?  The rationale
currently lives in the v9 changelog below the "---" separator, which is
stripped and does not become part of the git history.

The same applies to the jumbo pause restriction, which moves from
rtl_jumbo_config() (re-applied on every hardware start) to a one-shot
phylink_update_mac_pause_capabilities() call in rtl8169_change_mtu().  Is
the restriction still re-applied if the MAC is restarted without an MTU
transition?

[ ... ]

> @@ -2779,7 +2781,7 @@ static void rtl_prepare_power_down(struct rtl8169_private *tp)
>  		rtl_ephy_write(tp, 0x19, 0xff64);
>  
>  	if (device_may_wakeup(tp_to_dev(tp))) {
> -		phy_speed_down(tp->phydev, false);
> +		phylink_speed_down(tp->phylink, false);
>  		rtl_wol_enable_rx(tp);
>  	}
>  }

[ ... ]

> @@ -5007,8 +5001,6 @@ static void rtl8169_down(struct rtl8169_private *tp)
>  	/* Clear all task flags */
>  	bitmap_zero(tp->wk.flags, RTL_FLAG_MAX);
>  
> -	phy_stop(tp->phydev);
> -
>  	/* Reset SerDes PHY to bring down fiber link */
>  	if (tp->sfp_mode)
>  		rtl_sfp_reset(tp);
> @@ -5038,8 +5030,6 @@ static void rtl8169_up(struct rtl8169_private *tp)
>  	napi_enable(&tp->napi);
>  	enable_work(&tp->wk.work);
>  	rtl_reset_work(tp);
> -
> -	phy_start(tp->phydev);
>  }

[Severity: High]

phy_stop()/phy_start() are removed from rtl8169_down()/rtl8169_up(), but the
replacements phylink_stop()/phylink_start() are only added to
rtl8169_close()/rtl_open().  rtl8169_down()/rtl8169_up() are also the
suspend, runtime suspend, shutdown and resume paths:

rtl8169_suspend()      -> rtl8169_net_suspend() -> rtl8169_down()
rtl8169_runtime_suspend() -> rtl8169_net_suspend() -> rtl8169_down()
rtl_shutdown()         -> rtl8169_net_suspend() -> rtl8169_down()
rtl8169_resume()       -> rtl8169_up()
rtl8169_runtime_resume() -> rtl8169_up()

and there is no phylink_suspend()/phylink_resume() anywhere in the driver,
even though phylink_config.mac_managed_pm is set to true.  Two questions:

First, is the phylink resolve work still quiesced on the suspend path?
disable_work_sync(&tp->wk.work) only covers the driver's own work item, and
right at the end of rtl8169_down():

rtl_prepare_power_down() {
	if (device_may_wakeup(tp_to_dev(tp))) {
		phylink_speed_down(tp->phylink, false);

phylink_speed_down() -> phy_speed_down() -> phy_config_aneg() restarts
autoneg while phylink is still started, so phy_check_link_status() ->
phy_link_up() -> phylink_run_resolve() can queue a resolve.  The resolve
worker does not take RTNL, so can rtl_mac_link_up() (rtl_link_chg_patch()
ERI writes) or rtl_mac_enable_tx_lpi() (OCP/ERI writes) run after
rtl8169_cleanup()/pci_clear_master(), after the PCI core has put the device
in D3hot on the runtime PM path, or after clk_disable_unprepare(tp->clk) in
rtl8169_suspend()?

Second, is the chip specific link configuration re-applied after resume?
After rtl8169_up() the MAC has been reset by rtl_reset_work() ->
rtl_hw_start(), but phylib state is still PHY_RUNNING with phydev->link ==
1, so phy_check_link_status() takes neither branch and phy_link_up() is not
re-emitted; phylink therefore never re-runs mac_link_up().  Since
rtl_mac_link_up() is now the only caller of rtl_link_chg_patch(), and
rtl_enable_tx_lpi() is only driven from the mac_enable_tx_lpi/
mac_disable_tx_lpi callbacks, do the speed dependent ERI settings for
RTL_GIGA_MAC_VER_34..38 and the MAC Tx LPI bits stay lost after every
suspend/resume cycle?

>  static int rtl8169_close(struct net_device *dev)
> @@ -5049,13 +5039,14 @@ static int rtl8169_close(struct net_device *dev)
>  
>  	pm_runtime_get_sync(&pdev->dev);
>  
> +	phylink_stop(tp->phylink);
>  	netif_stop_queue(dev);
>  	rtl8169_down(tp);
>  	rtl8169_rx_clear(tp);
>  
>  	free_irq(tp->irq, tp);
>  
> -	phy_disconnect(tp->phydev);
> +	phylink_disconnect_phy(tp->phylink);

[ ... ]

> @@ -5117,6 +5108,7 @@ static int rtl_open(struct net_device *dev)
>  		goto err_free_irq;
>  
>  	rtl8169_up(tp);
> +	phylink_start(tp->phylink);
>  	rtl8169_init_counter_offsets(tp);
>  	netif_start_queue(dev);
>  out:

[ ... ]

> @@ -5474,16 +5467,6 @@ static int r8169_mdio_register(struct rtl8169_private *tp)
>  		return -EUNATCH;
>  	}
>  
> -	tp->phydev->mac_managed_pm = true;
> -	if (rtl_supports_eee(tp))
> -		phy_support_eee(tp->phydev);
> -	phy_support_asym_pause(tp->phydev);
> -
> -	/* mimic behavior of r8125/r8126 vendor drivers */
> -	if (tp->mac_version == RTL_GIGA_MAC_VER_61)
> -		phy_disable_eee_mode(tp->phydev,
> -				     ETHTOOL_LINK_MODE_2500baseT_Full_BIT);
> -

[Severity: Medium]

The unconditional phy_support_eee() call disappears here, but
rtl_init_phylink() below sets only lpi_capabilities, not
phylink_config.eee_enabled_default or lpi_timer_default.  phylink_create()
then leaves EEE off:

drivers/net/phy/phylink.c:phylink_create() {
	/* Set the default EEE configuration */
	pl->eee_cfg.eee_enabled = pl->config->eee_enabled_default;
	pl->eee_cfg.tx_lpi_enabled = pl->eee_cfg.eee_enabled;
	pl->eee_cfg.tx_lpi_timer = pl->config->lpi_timer_default;
}

and phylink_bringup_phy() skips phy_support_eee() and programs tx_lpi off:

drivers/net/phy/phylink.c:phylink_bringup_phy() {
	if (pl->mac_supports_eee) {
		if (pl->eee_cfg.eee_enabled)
			phy_support_eee(phy);

		phy->eee_cfg.tx_lpi_enabled = pl->eee_cfg.tx_lpi_enabled;
		phy->eee_cfg.tx_lpi_timer = pl->eee_cfg.tx_lpi_timer;
}

With phydev->eee_cfg.tx_lpi_enabled false, phydev->enable_tx_lpi is never
set, so phylink never calls mac_enable_tx_lpi().  Does that leave MAC Tx LPI
disabled by default on every EEE capable chip, and the newly added
rtl_mac_enable_tx_lpi() -> rtl_enable_tx_lpi() path unreachable until
userspace runs "ethtool --set-eee ethX eee on"?  Previously
phy_support_eee() set tx_lpi_enabled = true and r8169_phylink_handler()
called rtl_enable_tx_lpi(tp, tp->phydev->enable_tx_lpi).

Should rtl_init_phylink() set eee_enabled_default (and lpi_timer_default)
when rtl8169_get_lpi_caps() returns non-zero?

[ ... ]

> +static int rtl_mac_enable_tx_lpi(struct phylink_config *config,
> +				 u32 timer,
> +				 bool tx_clk_stop)
> +{
> +	struct rtl8169_private *tp = container_of(config,
> +						  struct rtl8169_private,
> +						  phylink_config);
> +
> +	rtl_enable_tx_lpi(tp, true);
> +
> +	return 0;
> +}

[Severity: Medium]

This isn't a bug introduced by this patch, but the mismatch is now visible
in the new callback signature: the "timer" argument that
phylink_activate_lpi() passes from pl->mac_tx_lpi_timer (the value set via
"ethtool --set-eee ethX tx-lpi-timer N") and "tx_clk_stop" are both dropped
while 0 is returned, and rtl8169_get_eee() then reports the unrelated MTU
derived value from r8169_get_tx_lpi_timer_us().  The old code had the same
behaviour through r8169_phylink_handler().  Should the callback either
program the requested timer or reject values it cannot honour?

[ ... ]

> +static int rtl_init_phylink(struct rtl8169_private *tp)
> +{
> +	struct phylink *pl;
> +	phy_interface_t phy_mode;
> +
> +	tp->phylink_config.dev = &tp->dev->dev;
> +	tp->phylink_config.type = PHYLINK_NETDEV;
> +	tp->phylink_config.mac_managed_pm = true;
> +	tp->phylink_config.lpi_capabilities = rtl8169_get_lpi_caps(tp);
> +	tp->phylink_config.mac_capabilities |= MAC_ASYM_PAUSE | MAC_SYM_PAUSE;
> +
> +	if (tp->sfp_mode) {
> +		phy_mode = PHY_INTERFACE_MODE_INTERNAL;
> +		tp->phylink_config.mac_capabilities |= MAC_10000FD;
> +	} else {

[Severity: Low]

At this commit the sfp_mode branch advertises only MAC_10000FD (plus pause),
while the unchanged sfp path of rtl8169_set_link_ksettings() still accepts
any speed valid for phydev->supported, including 1000.  Would a 1000BASE-X
fiber link then be missing from pl->supported and fail phylink_bringup_phy()
validation?

Related, the commit message says phylink "can support fiber mode card which
can not get link status or link speed from standard phy registers", but
rtl_mac_select_pcs() returns NULL and rtl_mac_config() is empty here, so
fiber link state still comes from the pseudo-PHY through phylib.

Both halves look resolved later in the series: at the end of the series
rtl_init_phylink() has a switch on enum rtl_sfp_mode where
RTL_SFP_8127_ATF adds MAC_1000FD | MAC_10000FD, sets 10GBASER and 1000BASEX
in supported_interfaces and installs real r8169_pcs_ops, and the sfp special
case in rtl8169_set_link_ksettings() is gone.  Would it help to word the
commit message so it does not promise the PCS behaviour that only arrives in
the later RTL8127atf/RTL8116af patches?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831053745.1197-1-javen_xu%40realsil.com.cn