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

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

[javen]

From: Javen Xu <javen_xu@realsil.com.cn>

This series patch adds support for phylink. RTL8116af is a fiber mode
card, link status and speed can not be read from standard phy reg. So
we read link status and speed from serdes reg by pcs. So as RTL8127atf.

Javen Xu (7):
  r8169: add speed in private struct
  net: phy: phylink: add helper to modify pause
  r8169: add support for phylink
  r8169: add support for RTL8116af
  r8169: add support for RTL8127atf
  r8169: add ltr support for RTL8117 series
  r8169: fix RTL8116af can not enter s0idle and c10

 drivers/net/ethernet/realtek/Kconfig      |   2 +-
 drivers/net/ethernet/realtek/r8169_main.c | 705 +++++++++++++++++-----
 drivers/net/phy/phylink.c                 | 171 ++++--
 drivers/net/phy/realtek/realtek_main.c    |  54 --
 include/linux/phylink.h                   |   2 +
 include/net/phy/realtek_phy.h             |   7 -
 6 files changed, 684 insertions(+), 257 deletions(-)
 delete mode 100644 include/net/phy/realtek_phy.h

-- 
2.43.0

[PATCH net-next v9 1/7] r8169: add speed in private struct

[javen]

From: Javen Xu <javen_xu@realsil.com.cn>

This patch adds speed in private struct in order to decouple
from phydev in the following patch supporting for phylink.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Javen Xu <javen_xu@realsil.com.cn>
---
Changes in v2:
 - repalce current_speed with speed

Changes in v3:
 - update tp->speed in rtl8169_set_link_ksettings()

Changes in v4:
 - no changes

Changes in v5:
 - no changes

Changes in v6:
 - no changes

Changes in v7:
 - no changes

Changes in v8:
 - no changes

Changes in v9:
 - no changes
---
 drivers/net/ethernet/realtek/r8169_main.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index ec4fc21fa21f..c60710f9bd21 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c
@@ -750,6 +750,7 @@ struct rtl8169_private {
 	u32 irq_mask;
 	int irq;
 	struct clk *clk;
+	int speed;
 
 	struct {
 		DECLARE_BITMAP(flags, RTL_FLAG_MAX);
@@ -1673,16 +1674,14 @@ static void rtl8169_irq_mask_and_ack(struct rtl8169_private *tp)
 	rtl_pci_commit(tp);
 }
 
-static void rtl_link_chg_patch(struct rtl8169_private *tp)
+static void rtl_link_chg_patch(struct rtl8169_private *tp, int speed)
 {
-	struct phy_device *phydev = tp->phydev;
-
 	if (tp->mac_version == RTL_GIGA_MAC_VER_34 ||
 	    tp->mac_version == RTL_GIGA_MAC_VER_38) {
-		if (phydev->speed == SPEED_1000) {
+		if (speed == SPEED_1000) {
 			rtl_eri_write(tp, 0x1bc, ERIAR_MASK_1111, 0x00000011);
 			rtl_eri_write(tp, 0x1dc, ERIAR_MASK_1111, 0x00000005);
-		} else if (phydev->speed == SPEED_100) {
+		} else if (speed == SPEED_100) {
 			rtl_eri_write(tp, 0x1bc, ERIAR_MASK_1111, 0x0000001f);
 			rtl_eri_write(tp, 0x1dc, ERIAR_MASK_1111, 0x00000005);
 		} else {
@@ -1692,7 +1691,7 @@ static void rtl_link_chg_patch(struct rtl8169_private *tp)
 		rtl_reset_packet_filter(tp);
 	} else if (tp->mac_version == RTL_GIGA_MAC_VER_35 ||
 		   tp->mac_version == RTL_GIGA_MAC_VER_36) {
-		if (phydev->speed == SPEED_1000) {
+		if (speed == SPEED_1000) {
 			rtl_eri_write(tp, 0x1bc, ERIAR_MASK_1111, 0x00000011);
 			rtl_eri_write(tp, 0x1dc, ERIAR_MASK_1111, 0x00000005);
 		} else {
@@ -1700,7 +1699,7 @@ static void rtl_link_chg_patch(struct rtl8169_private *tp)
 			rtl_eri_write(tp, 0x1dc, ERIAR_MASK_1111, 0x0000003f);
 		}
 	} else if (tp->mac_version == RTL_GIGA_MAC_VER_37) {
-		if (phydev->speed == SPEED_10) {
+		if (speed == SPEED_10) {
 			rtl_eri_write(tp, 0x1d0, ERIAR_MASK_0011, 0x4d02);
 			rtl_eri_write(tp, 0x1dc, ERIAR_MASK_0011, 0x0060a);
 		} else {
@@ -2074,11 +2073,11 @@ rtl_coalesce_info(struct rtl8169_private *tp)
 		ci = rtl_coalesce_info_8168_8136;
 
 	/* if speed is unknown assume highest one */
-	if (tp->phydev->speed == SPEED_UNKNOWN)
+	if (tp->speed == SPEED_UNKNOWN)
 		return ci;
 
 	for (; ci->speed; ci++) {
-		if (tp->phydev->speed == ci->speed)
+		if (tp->speed == ci->speed)
 			return ci;
 	}
 
@@ -2236,7 +2235,7 @@ static void rtl_set_eee_txidle_timer(struct rtl8169_private *tp)
 
 static unsigned int r8169_get_tx_lpi_timer_us(struct rtl8169_private *tp)
 {
-	unsigned int speed = tp->phydev->speed;
+	unsigned int speed = tp->speed;
 	unsigned int timer = tp->tx_lpi_timer;
 
 	if (!timer || speed == SPEED_UNKNOWN)
@@ -4968,8 +4967,9 @@ static void r8169_phylink_handler(struct net_device *ndev)
 	struct rtl8169_private *tp = netdev_priv(ndev);
 	struct device *d = tp_to_dev(tp);
 
+	tp->speed = tp->phydev->speed;
 	if (netif_carrier_ok(ndev)) {
-		rtl_link_chg_patch(tp);
+		rtl_link_chg_patch(tp, tp->speed);
 		rtl_enable_tx_lpi(tp, tp->phydev->enable_tx_lpi);
 		pm_request_resume(d);
 	} else {
@@ -5667,6 +5667,7 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 				     ext_xid_str, xid);
 	tp->mac_version = chip->mac_version;
 	tp->fw_name = chip->fw_name;
+	tp->speed = SPEED_UNKNOWN;
 
 	/* Disable ASPM L1 as that cause random device stop working
 	 * problems as well as full system hangs for some PCIe devices users.
-- 
2.43.0

[PATCH net-next v9 2/7] net: phy: phylink: add helper to modify pause

[javen]

From: Javen Xu <javen_xu@realsil.com.cn>

For Realtek nics, when we enable jumbo, pause are not supported. So we
must check the pause capabilities from ourself and lp.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Javen Xu <javen_xu@realsil.com.cn>
---
Changes in v5:
 - no changes, new file

Changes in v6:
 - rename phylink_update_mac_pause_capabilities(), this function only
   changes mac pause capability
 - set asym pause and pause according to config->pause tx and rx
 - add phydev->lock when change pl->phydev->advertising

Changes in v7:
 - modify the logic of phylink_update_mac_pause_capabilities
   funciton
 - extract a helper function to update phylink from phylink_ethtool_set_pauseparam

Changes in v8:
 - no changes

Changes in v9:
 - no changes
---
 drivers/net/phy/phylink.c | 171 +++++++++++++++++++++++++++-----------
 include/linux/phylink.h   |   2 +
 2 files changed, 123 insertions(+), 50 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 5b8e956902fb..31d908105479 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -1829,6 +1829,126 @@ int phylink_set_fixed_link(struct phylink *pl,
 }
 EXPORT_SYMBOL_GPL(phylink_set_fixed_link);
 
+/**
+ * phylink_update_pause_state() - Update the phylink pause frame configuration
+ * @pl: a pointer to a &struct phylink instance
+ * @pause_state: bitmask indicating the new pause state
+ *
+ * Update the MAC pause frame (flow control) state for the phylink instance.
+ */
+static void phylink_update_pause_state(struct phylink *pl, int pause_state)
+{
+	struct phylink_link_state *config = &pl->link_config;
+	bool tx_pause = !!(pause_state & MLO_PAUSE_TX);
+	bool rx_pause = !!(pause_state & MLO_PAUSE_RX);
+	bool manual_changed;
+
+	mutex_lock(&pl->state_mutex);
+
+	/*
+	 * See the comments for linkmode_set_pause(), wrt the deficiencies
+	 * with the current implementation.  A solution to this issue would
+	 * be:
+	 * ethtool  Local device
+	 *  rx  tx  Pause AsymDir
+	 *  0   0   0     0
+	 *  1   0   1     1
+	 *  0   1   0     1
+	 *  1   1   1     1
+	 * and then use the ethtool rx/tx enablement status to mask the
+	 * rx/tx pause resolution.
+	 */
+	linkmode_set_pause(config->advertising, tx_pause,
+			   rx_pause);
+
+	manual_changed = (config->pause ^ pause_state) & MLO_PAUSE_AN ||
+			 (!(pause_state & MLO_PAUSE_AN) &&
+			   (config->pause ^ pause_state) & MLO_PAUSE_TXRX_MASK);
+
+	config->pause = pause_state;
+
+	/* Update our in-band advertisement, triggering a renegotiation if
+	 * the advertisement changed.
+	 */
+	if (!pl->phydev)
+		phylink_change_inband_advert(pl);
+
+	mutex_unlock(&pl->state_mutex);
+
+	/* If we have a PHY, a change of the pause frame advertisement will
+	 * cause phylib to renegotiate (if AN is enabled) which will in turn
+	 * call our phylink_phy_change() and trigger a resolve.  Note that
+	 * we can't hold our state mutex while calling phy_set_asym_pause().
+	 */
+	if (pl->phydev)
+		phy_set_asym_pause(pl->phydev, rx_pause, tx_pause);
+
+	/* If the manual pause settings changed, make sure we trigger a
+	 * resolve to update their state; we can not guarantee that the
+	 * link will cycle.
+	 */
+	if (manual_changed) {
+		pl->link_failed = true;
+		phylink_run_resolve(pl);
+	}
+}
+
+/**
+ * phylink_update_mac_pause_capabilities() - Dynamically update MAC pause
+ * @pl: a pointer to a &struct phylink returned from phylink_create()
+ * @mac_pause: the new MAC pause capabilities mask
+ *
+ * This function allows a MAC driver to dynamically change its pause state,
+ * such as losing/gaining Pause frame support based on MTU size.
+ * It recalculates supported link modes and triggers renegotiation if needed.
+ */
+void phylink_update_mac_pause_capabilities(struct phylink *pl, unsigned long mac_pause)
+{
+	struct phylink_link_state *config = &pl->link_config;
+	unsigned long old_pause;
+	int pause_state;
+
+	ASSERT_RTNL();
+
+	if (mac_pause & ~(MAC_SYM_PAUSE | MAC_ASYM_PAUSE)) {
+		phylink_err(pl, "Attempted to dynamically change non-pause MAC capabilities\n");
+		return;
+	}
+
+	old_pause = pl->config->mac_capabilities & (MAC_SYM_PAUSE | MAC_ASYM_PAUSE);
+	if (old_pause == mac_pause)
+		return;
+
+	mutex_lock(&pl->state_mutex);
+
+	pl->config->mac_capabilities &= ~(MAC_SYM_PAUSE | MAC_ASYM_PAUSE);
+	pl->config->mac_capabilities |= mac_pause;
+
+	phylink_set(pl->supported, Pause);
+	phylink_set(pl->supported, Asym_Pause);
+
+	if (pl->phydev)
+		linkmode_and(pl->supported, pl->supported, pl->phydev->supported);
+	else if (pl->sfp_bus)
+		linkmode_and(pl->supported, pl->supported, pl->sfp_support);
+
+	phylink_validate(pl, pl->supported, config);
+
+	pause_state = config->pause;
+
+	if (!phylink_test(pl->supported, Pause)) {
+		pause_state &= ~(MLO_PAUSE_RX | MLO_PAUSE_TX);
+	} else if (!phylink_test(pl->supported, Asym_Pause)) {
+		if ((pause_state & MLO_PAUSE_RX) ^ (pause_state & MLO_PAUSE_TX))
+			pause_state &= ~(MLO_PAUSE_RX | MLO_PAUSE_TX);
+	}
+
+	mutex_unlock(&pl->state_mutex);
+
+	phylink_update_pause_state(pl, pause_state);
+}
+EXPORT_SYMBOL_GPL(phylink_update_mac_pause_capabilities);
+
 /**
  * phylink_create() - create a phylink instance
  * @config: a pointer to the target &struct phylink_config
@@ -3188,8 +3308,6 @@ EXPORT_SYMBOL_GPL(phylink_ethtool_get_pauseparam);
 int phylink_ethtool_set_pauseparam(struct phylink *pl,
 				   struct ethtool_pauseparam *pause)
 {
-	struct phylink_link_state *config = &pl->link_config;
-	bool manual_changed;
 	int pause_state;
 
 	ASSERT_RTNL();
@@ -3213,54 +3331,7 @@ int phylink_ethtool_set_pauseparam(struct phylink *pl,
 	if (pause->tx_pause)
 		pause_state |= MLO_PAUSE_TX;
 
-	mutex_lock(&pl->state_mutex);
-	/*
-	 * See the comments for linkmode_set_pause(), wrt the deficiencies
-	 * with the current implementation.  A solution to this issue would
-	 * be:
-	 * ethtool  Local device
-	 *  rx  tx  Pause AsymDir
-	 *  0   0   0     0
-	 *  1   0   1     1
-	 *  0   1   0     1
-	 *  1   1   1     1
-	 * and then use the ethtool rx/tx enablement status to mask the
-	 * rx/tx pause resolution.
-	 */
-	linkmode_set_pause(config->advertising, pause->tx_pause,
-			   pause->rx_pause);
-
-	manual_changed = (config->pause ^ pause_state) & MLO_PAUSE_AN ||
-			 (!(pause_state & MLO_PAUSE_AN) &&
-			   (config->pause ^ pause_state) & MLO_PAUSE_TXRX_MASK);
-
-	config->pause = pause_state;
-
-	/* Update our in-band advertisement, triggering a renegotiation if
-	 * the advertisement changed.
-	 */
-	if (!pl->phydev)
-		phylink_change_inband_advert(pl);
-
-	mutex_unlock(&pl->state_mutex);
-
-	/* If we have a PHY, a change of the pause frame advertisement will
-	 * cause phylib to renegotiate (if AN is enabled) which will in turn
-	 * call our phylink_phy_change() and trigger a resolve.  Note that
-	 * we can't hold our state mutex while calling phy_set_asym_pause().
-	 */
-	if (pl->phydev)
-		phy_set_asym_pause(pl->phydev, pause->rx_pause,
-				   pause->tx_pause);
-
-	/* If the manual pause settings changed, make sure we trigger a
-	 * resolve to update their state; we can not guarantee that the
-	 * link will cycle.
-	 */
-	if (manual_changed) {
-		pl->link_failed = true;
-		phylink_run_resolve(pl);
-	}
+	phylink_update_pause_state(pl, pause_state);
 
 	return 0;
 }
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index 1dda5c7ed5f1..3a88a69882a6 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -843,4 +843,6 @@ void phylink_replay_link_begin(struct phylink *pl);
 
 void phylink_replay_link_end(struct phylink *pl);
 
+void phylink_update_mac_pause_capabilities(struct phylink *pl, unsigned long mac_pause);
+
 #endif
-- 
2.43.0

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

[javen]

From: Javen Xu <javen_xu@realsil.com.cn>

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

Signed-off-by: Javen Xu <javen_xu@realsil.com.cn>
---
Changes in v2:
 - merge patch v1 3/6 and v1 4/6.
 - add helper rtl_mac_enable_tx_lpi(), rtl_mac_disable_tx_lpi()
   and rtl8169_get_lpi_caps()

Changes in v3:
 - use phylink_ethtool_set_pauseparam to set pause status when change
   mtu
 - replace phy_do_ioctl_running with rtl8169_ioctl
 - recover phy_mode according to tp->supports_gmii for 1G nics

Changes in v4:
 - set lpi_interface if support eee
 - Only if jumbo changes, set pause

Changes in v5:
 - no changes

Changes in v6:
 - remove PHYLIB in KConfig
 - remove dead code, saved_pause and jumbo_pause_saved
 - move phylink_start to rtl8169_open, phylink_stop to rtl8169_close

Changes in v7:
 - no changes

Changes in v8:
 - only pass capabilities with pause bit to
   phylink_update_mac_pause_capabilities()

Changes in v9:
 - add comments on rtl_mac_link_up() for Realtek chips don't support
   force tx/rx pause when autoneg is disabled
 - return -EOPNOTSUPP in rtl8169_set_pauseparam() when autoneg is
   disabled
---
 drivers/net/ethernet/realtek/Kconfig      |   2 +-
 drivers/net/ethernet/realtek/r8169_main.c | 296 ++++++++++++++++------
 2 files changed, 221 insertions(+), 77 deletions(-)

diff --git a/drivers/net/ethernet/realtek/Kconfig b/drivers/net/ethernet/realtek/Kconfig
index 9b0f4f9631db..5818a9a500a8 100644
--- a/drivers/net/ethernet/realtek/Kconfig
+++ b/drivers/net/ethernet/realtek/Kconfig
@@ -86,8 +86,8 @@ config R8169
 	depends on PCI
 	select FW_LOADER
 	select CRC32
-	select PHYLIB
 	select REALTEK_PHY
+	select PHYLINK
 	help
 	  Say Y here if you have a Realtek Ethernet adapter belonging to
 	  the following families:
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
@@ -26,6 +26,7 @@
 #include <linux/dma-mapping.h>
 #include <linux/pm_runtime.h>
 #include <linux/bitfield.h>
+#include <linux/phylink.h>
 #include <linux/prefetch.h>
 #include <linux/ipv6.h>
 #include <linux/unaligned.h>
@@ -775,6 +776,8 @@ struct rtl8169_private {
 	struct r8169_led_classdev *leds;
 
 	u32 ocp_base;
+	struct phylink *phylink;
+	struct phylink_config phylink_config;
 };
 
 typedef void (*rtl_generic_fct)(struct rtl8169_private *tp);
@@ -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);
 }
 
 static void rtl8169_get_ringparam(struct net_device *dev,
@@ -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);
 }
 
 static void rtl8169_get_eth_mac_stats(struct net_device *dev,
@@ -2385,6 +2381,14 @@ static void rtl8169_get_eth_ctrl_stats(struct net_device *dev,
 		le32_to_cpu(tp->counters->rx_unknown_opcode);
 }
 
+static int rtl8169_get_link_ksettings(struct net_device *ndev,
+				      struct ethtool_link_ksettings *cmd)
+{
+	struct rtl8169_private *tp = netdev_priv(ndev);
+
+	return phylink_ethtool_ksettings_get(tp->phylink, cmd);
+}
+
 static int rtl8169_set_link_ksettings(struct net_device *ndev,
 				      const struct ethtool_link_ksettings *cmd)
 {
@@ -2394,7 +2398,7 @@ static int rtl8169_set_link_ksettings(struct net_device *ndev,
 	int speed = cmd->base.speed;
 
 	if (!tp->sfp_mode)
-		return phy_ethtool_ksettings_set(phydev, cmd);
+		return phylink_ethtool_ksettings_set(tp->phylink, cmd);
 
 	if (cmd->base.autoneg != AUTONEG_DISABLE)
 		return -EINVAL;
@@ -2415,6 +2419,13 @@ static int rtl8169_set_link_ksettings(struct net_device *ndev,
 	return 0;
 }
 
+static int rtl8169_nway_reset(struct net_device *dev)
+{
+	struct rtl8169_private *tp = netdev_priv(dev);
+
+	return phylink_ethtool_nway_reset(tp->phylink);
+}
+
 static const struct ethtool_ops rtl8169_ethtool_ops = {
 	.supported_coalesce_params = ETHTOOL_COALESCE_USECS |
 				     ETHTOOL_COALESCE_MAX_FRAMES,
@@ -2430,10 +2441,10 @@ static const struct ethtool_ops rtl8169_ethtool_ops = {
 	.get_sset_count		= rtl8169_get_sset_count,
 	.get_ethtool_stats	= rtl8169_get_ethtool_stats,
 	.get_ts_info		= ethtool_op_get_ts_info,
-	.nway_reset		= phy_ethtool_nway_reset,
+	.nway_reset		= rtl8169_nway_reset,
 	.get_eee		= rtl8169_get_eee,
 	.set_eee		= rtl8169_set_eee,
-	.get_link_ksettings	= phy_ethtool_get_link_ksettings,
+	.get_link_ksettings	= rtl8169_get_link_ksettings,
 	.set_link_ksettings	= rtl8169_set_link_ksettings,
 	.get_ringparam		= rtl8169_get_ringparam,
 	.get_pause_stats	= rtl8169_get_pause_stats,
@@ -2656,15 +2667,6 @@ static void rtl_jumbo_config(struct rtl8169_private *tp)
 
 	if (pci_is_pcie(tp->pci_dev) && tp->supports_gmii)
 		pcie_set_readrq(tp->pci_dev, readrq);
-
-	/* Chip doesn't support pause in jumbo mode */
-	if (jumbo) {
-		linkmode_clear_bit(ETHTOOL_LINK_MODE_Pause_BIT,
-				   tp->phydev->advertising);
-		linkmode_clear_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
-				   tp->phydev->advertising);
-		phy_start_aneg(tp->phydev);
-	}
 }
 
 DECLARE_RTL_COND(rtl_chipcmd_cond)
@@ -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);
 	}
 }
@@ -2831,6 +2833,16 @@ static void rtl8169_set_magic_reg(struct rtl8169_private *tp)
 	RTL_W32(tp, 0x7c, val);
 }
 
+static int rtl8169_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
+{
+	struct rtl8169_private *tp = netdev_priv(dev);
+
+	if (!netif_running(dev))
+		return -ENODEV;
+
+	return phylink_mii_ioctl(tp->phylink, ifr, cmd);
+}
+
 static void rtl_set_rx_mode(struct net_device *dev)
 {
 	u32 rx_mode = AcceptBroadcast | AcceptMyPhys | AcceptMulticast;
@@ -4138,12 +4150,23 @@ static void rtl_hw_start(struct  rtl8169_private *tp)
 static int rtl8169_change_mtu(struct net_device *dev, int new_mtu)
 {
 	struct rtl8169_private *tp = netdev_priv(dev);
+	bool jumbo_before = dev->mtu > ETH_DATA_LEN;
+	bool jumbo_after = new_mtu > ETH_DATA_LEN;
 
 	WRITE_ONCE(dev->mtu, new_mtu);
 	netdev_update_features(dev);
 	rtl_jumbo_config(tp);
 	rtl_set_eee_txidle_timer(tp);
 
+	if (jumbo_before != jumbo_after) {
+		unsigned long caps = 0;
+
+		if (!jumbo_after)
+			caps |= (MAC_SYM_PAUSE | MAC_ASYM_PAUSE);
+
+		phylink_update_mac_pause_capabilities(tp->phylink, caps);
+	}
+
 	return 0;
 }
 
@@ -4929,9 +4952,6 @@ static int rtl8169_poll(struct napi_struct *napi, int budget)
 
 static void rtl_enable_tx_lpi(struct rtl8169_private *tp, bool enable)
 {
-	if (!rtl_supports_eee(tp))
-		return;
-
 	switch (tp->mac_version) {
 	case RTL_GIGA_MAC_VER_34 ... RTL_GIGA_MAC_VER_52:
 		/* Adjust EEE LED frequency */
@@ -4962,41 +4982,15 @@ static void rtl_enable_tx_lpi(struct rtl8169_private *tp, bool enable)
 	}
 }
 
-static void r8169_phylink_handler(struct net_device *ndev)
-{
-	struct rtl8169_private *tp = netdev_priv(ndev);
-	struct device *d = tp_to_dev(tp);
-
-	tp->speed = tp->phydev->speed;
-	if (netif_carrier_ok(ndev)) {
-		rtl_link_chg_patch(tp, tp->speed);
-		rtl_enable_tx_lpi(tp, tp->phydev->enable_tx_lpi);
-		pm_request_resume(d);
-	} else {
-		pm_runtime_idle(d);
-	}
-
-	phy_print_status(tp->phydev);
-}
-
 static int r8169_phy_connect(struct rtl8169_private *tp)
 {
-	struct phy_device *phydev = tp->phydev;
-	phy_interface_t phy_mode;
 	int ret;
 
-	phy_mode = tp->supports_gmii ? PHY_INTERFACE_MODE_GMII :
-		   PHY_INTERFACE_MODE_MII;
-
-	ret = phy_connect_direct(tp->dev, phydev, r8169_phylink_handler,
-				 phy_mode);
-	if (ret)
+	ret = phylink_connect_phy(tp->phylink, tp->phydev);
+	if (ret) {
+		netdev_err(tp->dev, "failed to connect phy\n");
 		return ret;
-
-	if (!tp->supports_gmii)
-		phy_set_max_speed(phydev, SPEED_100);
-
-	phy_attached_info(phydev);
+	}
 
 	return 0;
 }
@@ -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);
 }
 
 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);
 
 	dma_free_coherent(&pdev->dev, R8169_RX_RING_BYTES, tp->RxDescArray,
 			  tp->RxPhyAddr);
@@ -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:
@@ -5288,6 +5280,7 @@ static void rtl_remove_one(struct pci_dev *pdev)
 		r8169_remove_leds(tp->leds);
 
 	unregister_netdev(tp->dev);
+	phylink_destroy(tp->phylink);
 
 	if (tp->dash_type != RTL_DASH_NONE)
 		rtl8168_driver_stop(tp);
@@ -5310,7 +5303,7 @@ static const struct net_device_ops rtl_netdev_ops = {
 	.ndo_fix_features	= rtl8169_fix_features,
 	.ndo_set_features	= rtl8169_set_features,
 	.ndo_set_mac_address	= rtl_set_mac_address,
-	.ndo_eth_ioctl		= phy_do_ioctl_running,
+	.ndo_eth_ioctl		= rtl8169_ioctl,
 	.ndo_set_rx_mode	= rtl_set_rx_mode,
 #ifdef CONFIG_NET_POLL_CONTROLLER
 	.ndo_poll_controller	= rtl8169_netpoll,
@@ -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);
-
 	/* PHY will be woken up in rtl_open() */
 	phy_suspend(tp->phydev);
 
@@ -5599,6 +5582,159 @@ static bool rtl_aspm_is_safe(struct rtl8169_private *tp)
 	return false;
 }
 
+static void rtl_mac_link_down(struct phylink_config *config, unsigned int mode,
+			      phy_interface_t interface)
+{
+	struct rtl8169_private *tp = container_of(config,
+						  struct rtl8169_private,
+						  phylink_config);
+
+	tp->speed = SPEED_UNKNOWN;
+	pm_runtime_idle(tp_to_dev(tp));
+}
+
+static void rtl_mac_link_up(struct phylink_config *config,
+			    struct phy_device *phydev,
+			    unsigned int mode,
+			    phy_interface_t interface,
+			    int speed, int duplex,
+			    bool tx_pause, bool rx_pause)
+{
+	struct rtl8169_private *tp = container_of(config,
+						  struct rtl8169_private,
+						  phylink_config);
+	struct device *d = tp_to_dev(tp);
+
+	tp->speed = speed;
+	rtl_link_chg_patch(tp, speed);
+
+	/*
+	 * Note: This hardware does not support forcing flow control.
+	 * Tx/Rx pause is completely hardwired: if autoneg is ON, hardware
+	 * handles it automatically based on PHY result; if autoneg is OFF,
+	 * hardware forces pause to be disabled. No software override exists.
+	 */
+	pm_request_resume(d);
+}
+
+static struct phylink_pcs *rtl_mac_select_pcs(struct phylink_config *config,
+					      phy_interface_t interface)
+{
+	return NULL;
+}
+
+static void rtl_mac_config(struct phylink_config *config, unsigned int mode,
+			   const struct phylink_link_state *state)
+{
+}
+
+static void rtl_mac_disable_tx_lpi(struct phylink_config *config)
+{
+	struct rtl8169_private *tp = container_of(config,
+						  struct rtl8169_private,
+						  phylink_config);
+
+	rtl_enable_tx_lpi(tp, false);
+}
+
+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;
+}
+
+static const struct phylink_mac_ops rtl_phylink_mac_ops = {
+	.mac_select_pcs = rtl_mac_select_pcs,
+	.mac_config = rtl_mac_config,
+	.mac_link_down  = rtl_mac_link_down,
+	.mac_link_up    = rtl_mac_link_up,
+	.mac_disable_tx_lpi = rtl_mac_disable_tx_lpi,
+	.mac_enable_tx_lpi = rtl_mac_enable_tx_lpi,
+};
+
+static unsigned long rtl8169_get_lpi_caps(struct rtl8169_private *tp)
+{
+	unsigned long caps = 0;
+
+	if (!rtl_supports_eee(tp))
+		return 0;
+
+	caps |= MAC_100FD | MAC_1000FD;
+
+	/* mimic behavior of r8125/r8126 vendor drivers
+	 * RTL_GIGA_MAC_VER_61 doesn't support 2.5G eee
+	 */
+	if (tp->mac_version >= RTL_GIGA_MAC_VER_63)
+		caps |= MAC_2500FD;
+	if (tp->mac_version >= RTL_GIGA_MAC_VER_70)
+		caps |= MAC_5000FD;
+	if (tp->mac_version == RTL_GIGA_MAC_VER_80)
+		caps |= MAC_10000FD;
+
+	return caps;
+}
+
+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 {
+		tp->phylink_config.mac_capabilities |= MAC_10 | MAC_100;
+
+		if (tp->mac_version == RTL_GIGA_MAC_VER_80)
+			tp->phylink_config.mac_capabilities |= MAC_1000FD |
+							       MAC_2500FD |
+							       MAC_5000FD |
+							       MAC_10000FD;
+		else if (tp->mac_version == RTL_GIGA_MAC_VER_70)
+			tp->phylink_config.mac_capabilities |= MAC_1000FD |
+							       MAC_2500FD |
+							       MAC_5000FD;
+		else if (tp->mac_version >= RTL_GIGA_MAC_VER_61)
+			tp->phylink_config.mac_capabilities |= MAC_1000FD |
+							       MAC_2500FD;
+		else
+			if (tp->supports_gmii)
+				tp->phylink_config.mac_capabilities |= MAC_1000FD;
+
+		if (tp->mac_version < RTL_GIGA_MAC_VER_61)
+			phy_mode = tp->supports_gmii ? PHY_INTERFACE_MODE_GMII :
+				   PHY_INTERFACE_MODE_MII;
+		else
+			phy_mode = PHY_INTERFACE_MODE_INTERNAL;
+	}
+
+	__set_bit(phy_mode, tp->phylink_config.supported_interfaces);
+	if (tp->phylink_config.lpi_capabilities)
+		__set_bit(phy_mode, tp->phylink_config.lpi_interfaces);
+
+	pl = phylink_create(&tp->phylink_config, tp_to_dev(tp)->fwnode,
+			    phy_mode, &rtl_phylink_mac_ops);
+	if (IS_ERR(pl))
+		return PTR_ERR(pl);
+
+	tp->phylink = pl;
+
+	return 0;
+}
+
 static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 {
 	const struct rtl_chip_info *chip;
@@ -5789,13 +5925,21 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	pci_set_drvdata(pdev, tp);
 
-	rc = r8169_mdio_register(tp);
+	rc = rtl_init_phylink(tp);
 	if (rc)
 		return rc;
 
+	rc = r8169_mdio_register(tp);
+	if (rc) {
+		phylink_destroy(tp->phylink);
+		return rc;
+	}
+
 	rc = register_netdev(dev);
-	if (rc)
+	if (rc) {
+		phylink_destroy(tp->phylink);
 		return rc;
+	}
 
 	if (IS_ENABLED(CONFIG_R8169_LEDS)) {
 		if (rtl_is_8125(tp))
-- 
2.43.0

[PATCH net-next v9 4/7] r8169: add support for RTL8116af

[javen]

From: Javen Xu <javen_xu@realsil.com.cn>

RTL8116af is sfp mode. Phylink uses pcs to get the link status from its
serdes reg, instead of standard phy reg. Speed and duplex are hardcoded
to 1000Mbps Full-Duplex. Also, RTL8116af doesn't have internal phy, so
we add some checks to ensure that tp->phydev is not empty when we need it.
In rtl_hw_start_8117(), the MAC calibration for register 0xd412 relies
on reading the internal PHY register 0x0c42. Since RTL8116af does not
have an internal PHY, this calibration step is intentionally bypassed.

Signed-off-by: Javen Xu <javen_xu@realsil.com.cn>
---
Changes in v2:
 - replace some magic numbers with macro

Changes in v3:
 - change commit message
 - add lock when we do rtl8169_sds_read
 - use phylink_mii_c22_pcs_decode_state to get status
 - add phylink_mac_change for RTL8116af for it doesn't have phy

Changes in v4:
 - if tp->pcs.ops is not initial, just return NULL in rtl_mac_select_pcs

Changes in v5:
 - no changes

Changes in v6:
 - no changes

Changes in v7:
 - no changes

Changes in v8
 - no changes

Changes in v9:
 - no changes
---
 drivers/net/ethernet/realtek/r8169_main.c | 192 ++++++++++++++++++----
 1 file changed, 158 insertions(+), 34 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index 823763e4f445..a04acb42e995 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c
@@ -97,6 +97,18 @@
 #define JUMBO_9K	(9 * SZ_1K - VLAN_ETH_HLEN - ETH_FCS_LEN)
 #define JUMBO_16K	(SZ_16K - VLAN_ETH_HLEN - ETH_FCS_LEN)
 
+#define OCP_SDS_ADDR_REG	0xeb10
+#define OCP_SDS_CMD_REG		0xeb0e
+#define OCP_SDS_DATA_REG	0xeb14
+#define SDS_CMD_READ		0x0001
+#define RTL_SDS_C22_BASE	0x40
+#define RTL_PKG_DETECT		0xdc00
+#define RTL_PKG_DETECT_MASK	0x0078
+#define RTL_PKG_DETECT_8116AF	0x0030
+#define RTL_INT_HW_ID		0xd006
+#define RTL_INT_HW_ID_MASK	0x00ff
+#define RTL_INT_HW_ID_8116AF	0x0000
+
 static const struct rtl_chip_info {
 	u32 mask;
 	u32 val;
@@ -729,6 +741,12 @@ enum rtl_dash_type {
 	RTL_DASH_25_BP,
 };
 
+enum rtl_sfp_mode {
+	RTL_SFP_NONE,
+	RTL_SFP_8168_AF,
+	RTL_SFP_8127_ATF,
+};
+
 struct rtl8169_private {
 	void __iomem *mmio_addr;	/* memory map physical address */
 	struct pci_dev *pci_dev;
@@ -737,6 +755,7 @@ struct rtl8169_private {
 	struct napi_struct napi;
 	enum mac_version mac_version;
 	enum rtl_dash_type dash_type;
+	enum rtl_sfp_mode sfp_mode;
 	u32 cur_rx; /* Index into the Rx descriptor buffer of next Rx pkt. */
 	u32 cur_tx; /* Index into the Tx descriptor buffer of next Rx pkt. */
 	u32 dirty_tx;
@@ -764,7 +783,6 @@ struct rtl8169_private {
 	unsigned supports_gmii:1;
 	unsigned aspm_manageable:1;
 	unsigned dash_enabled:1;
-	bool sfp_mode:1;
 	dma_addr_t counters_phys_addr;
 	struct rtl8169_counters *counters;
 	struct rtl8169_tc_offsets tc_offset;
@@ -778,6 +796,7 @@ struct rtl8169_private {
 	u32 ocp_base;
 	struct phylink *phylink;
 	struct phylink_config phylink_config;
+	struct phylink_pcs pcs;
 };
 
 typedef void (*rtl_generic_fct)(struct rtl8169_private *tp);
@@ -1133,7 +1152,8 @@ static int r8168_phy_ocp_read(struct rtl8169_private *tp, u32 reg)
 		return 0;
 
 	/* Return dummy MII_PHYSID2 in SFP mode to match SFP PHY driver */
-	if (tp->sfp_mode && reg == (OCP_STD_PHY_BASE + 2 * MII_PHYSID2))
+	if (tp->sfp_mode == RTL_SFP_8127_ATF &&
+	    reg == (OCP_STD_PHY_BASE + 2 * MII_PHYSID2))
 		return PHY_ID_RTL_DUMMY_SFP & 0xffff;
 
 	RTL_W32(tp, GPHY_OCP, reg << 15);
@@ -1287,6 +1307,15 @@ static void mac_mcu_write(struct rtl8169_private *tp, int reg, int value)
 	r8168_mac_ocp_write(tp, tp->ocp_base + reg, value);
 }
 
+static bool rtl_is_8116af(struct rtl8169_private *tp)
+{
+	return tp->mac_version == RTL_GIGA_MAC_VER_52 &&
+		(r8168_mac_ocp_read(tp, RTL_PKG_DETECT) & RTL_PKG_DETECT_MASK) ==
+		RTL_PKG_DETECT_8116AF &&
+		(r8168_mac_ocp_read(tp, RTL_INT_HW_ID) & RTL_INT_HW_ID_MASK) ==
+		RTL_INT_HW_ID_8116AF;
+}
+
 static int mac_mcu_read(struct rtl8169_private *tp, int reg)
 {
 	return r8168_mac_ocp_read(tp, tp->ocp_base + reg);
@@ -1582,6 +1611,20 @@ static bool rtl_dash_is_enabled(struct rtl8169_private *tp)
 	}
 }
 
+static enum rtl_sfp_mode rtl_get_sfp_mode(struct rtl8169_private *tp)
+{
+	if (rtl_is_8125(tp)) {
+		u16 data = r8168_mac_ocp_read(tp, RTL_INT_HW_ID);
+
+		if ((data & 0xff) == 0x07)
+			return RTL_SFP_8127_ATF;
+	} else if (rtl_is_8116af(tp)) {
+		return RTL_SFP_8168_AF;
+	}
+
+	return RTL_SFP_NONE;
+}
+
 static enum rtl_dash_type rtl_get_dash_type(struct rtl8169_private *tp)
 {
 	switch (tp->mac_version) {
@@ -2397,7 +2440,7 @@ static int rtl8169_set_link_ksettings(struct net_device *ndev,
 	int duplex = cmd->base.duplex;
 	int speed = cmd->base.speed;
 
-	if (!tp->sfp_mode)
+	if (tp->sfp_mode != RTL_SFP_8127_ATF)
 		return phylink_ethtool_ksettings_set(tp->phylink, cmd);
 
 	if (cmd->base.autoneg != AUTONEG_DISABLE)
@@ -2509,9 +2552,10 @@ void r8169_apply_firmware(struct rtl8169_private *tp)
 		tp->ocp_base = OCP_STD_PHY_BASE;
 
 		/* PHY soft reset may still be in progress */
-		phy_read_poll_timeout(tp->phydev, MII_BMCR, val,
-				      !(val & BMCR_RESET),
-				      50000, 600000, true);
+		if (tp->phydev)
+			phy_read_poll_timeout(tp->phydev, MII_BMCR, val,
+					      !(val & BMCR_RESET),
+					      50000, 600000, true);
 	}
 }
 
@@ -2548,6 +2592,8 @@ static void rtl_schedule_task(struct rtl8169_private *tp, enum rtl_flag flag)
 
 static void rtl8169_init_phy(struct rtl8169_private *tp)
 {
+	phy_init_hw(tp->phydev);
+	phy_resume(tp->phydev);
 	r8169_hw_phy_config(tp, tp->phydev, tp->mac_version);
 
 	if (tp->mac_version <= RTL_GIGA_MAC_VER_06) {
@@ -2562,7 +2608,7 @@ static void rtl8169_init_phy(struct rtl8169_private *tp)
 	    tp->pci_dev->subsystem_device == 0xe000)
 		phy_write_paged(tp->phydev, 0x0001, 0x10, 0xf01b);
 
-	if (tp->sfp_mode)
+	if (tp->sfp_mode == RTL_SFP_8127_ATF)
 		rtl_sfp_init(tp);
 
 	/* We may have called phy_speed_down before */
@@ -3704,12 +3750,14 @@ static void rtl_hw_start_8117(struct rtl8169_private *tp)
 
 	rtl_pcie_state_l2l3_disable(tp);
 
-	rg_saw_cnt = phy_read_paged(tp->phydev, 0x0c42, 0x13) & 0x3fff;
-	if (rg_saw_cnt > 0) {
-		u16 sw_cnt_1ms_ini;
+	if (tp->phydev) {
+		rg_saw_cnt = phy_read_paged(tp->phydev, 0x0c42, 0x13) & 0x3fff;
+		if (rg_saw_cnt > 0) {
+			u16 sw_cnt_1ms_ini;
 
-		sw_cnt_1ms_ini = (16000000 / rg_saw_cnt) & 0x0fff;
-		r8168_mac_ocp_modify(tp, 0xd412, 0x0fff, sw_cnt_1ms_ini);
+			sw_cnt_1ms_ini = (16000000 / rg_saw_cnt) & 0x0fff;
+			r8168_mac_ocp_modify(tp, 0xd412, 0x0fff, sw_cnt_1ms_ini);
+		}
 	}
 
 	r8168_mac_ocp_modify(tp, 0xe056, 0x00f0, 0x0000);
@@ -4891,8 +4939,13 @@ static irqreturn_t rtl8169_interrupt(int irq, void *dev_instance)
 		goto out;
 	}
 
-	if (status & LinkChg)
-		phy_mac_interrupt(tp->phydev);
+	if (status & LinkChg) {
+		if (tp->phydev)
+			phy_mac_interrupt(tp->phydev);
+		else if (tp->sfp_mode == RTL_SFP_8168_AF)
+			phylink_mac_change(tp->phylink,
+					   !!(RTL_R8(tp, PHYstatus) & LinkStatus));
+	}
 
 	rtl_irq_disable(tp);
 	napi_schedule(&tp->napi);
@@ -5002,7 +5055,7 @@ static void rtl8169_down(struct rtl8169_private *tp)
 	bitmap_zero(tp->wk.flags, RTL_FLAG_MAX);
 
 	/* Reset SerDes PHY to bring down fiber link */
-	if (tp->sfp_mode)
+	if (tp->sfp_mode == RTL_SFP_8127_ATF)
 		rtl_sfp_reset(tp);
 
 	rtl8169_update_counters(tp);
@@ -5024,9 +5077,9 @@ static void rtl8169_up(struct rtl8169_private *tp)
 		rtl8168_driver_start(tp);
 
 	pci_set_master(tp->pci_dev);
-	phy_init_hw(tp->phydev);
-	phy_resume(tp->phydev);
-	rtl8169_init_phy(tp);
+	if (tp->phydev)
+		rtl8169_init_phy(tp);
+
 	napi_enable(&tp->napi);
 	enable_work(&tp->wk.work);
 	rtl_reset_work(tp);
@@ -5103,10 +5156,11 @@ static int rtl_open(struct net_device *dev)
 	if (retval < 0)
 		goto err_release_fw_2;
 
-	retval = r8169_phy_connect(tp);
-	if (retval)
-		goto err_free_irq;
-
+	if (tp->phydev) {
+		retval = r8169_phy_connect(tp);
+		if (retval)
+			goto err_free_irq;
+	}
 	rtl8169_up(tp);
 	phylink_start(tp->phylink);
 	rtl8169_init_counter_offsets(tp);
@@ -5620,6 +5674,16 @@ static void rtl_mac_link_up(struct phylink_config *config,
 static struct phylink_pcs *rtl_mac_select_pcs(struct phylink_config *config,
 					      phy_interface_t interface)
 {
+	struct rtl8169_private *tp = container_of(config,
+						  struct rtl8169_private,
+						  phylink_config);
+
+	if (!tp->pcs.ops)
+		return NULL;
+
+	if (interface == PHY_INTERFACE_MODE_1000BASEX)
+		return &tp->pcs;
+
 	return NULL;
 }
 
@@ -5628,6 +5692,53 @@ static void rtl_mac_config(struct phylink_config *config, unsigned int mode,
 {
 }
 
+static u16 rtl8169_sds_read(struct rtl8169_private *tp, u16 sds_reg)
+{
+	unsigned long flags;
+	u16 val = 0;
+
+	raw_spin_lock_irqsave(&tp->mac_ocp_lock, flags);
+	__r8168_mac_ocp_write(tp, OCP_SDS_ADDR_REG, sds_reg);
+	__r8168_mac_ocp_write(tp, OCP_SDS_CMD_REG, SDS_CMD_READ);
+	val = __r8168_mac_ocp_read(tp, OCP_SDS_DATA_REG);
+	raw_spin_unlock_irqrestore(&tp->mac_ocp_lock, flags);
+
+	return val;
+}
+
+static void rtl8169_pcs_get_state(struct phylink_pcs *pcs,
+				  unsigned int neg_mode,
+				  struct phylink_link_state *state)
+{
+	struct rtl8169_private *tp = container_of(pcs, struct rtl8169_private,
+						  pcs);
+	u16 bmsr, lpa;
+
+	bmsr = rtl8169_sds_read(tp, RTL_SDS_C22_BASE + MII_BMSR);
+	lpa = rtl8169_sds_read(tp, RTL_SDS_C22_BASE + MII_LPA);
+
+	phylink_mii_c22_pcs_decode_state(state, neg_mode, bmsr, lpa);
+}
+
+static int rtl8169_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
+			      phy_interface_t interface,
+			      const unsigned long *advertising,
+			      bool permit_pause_to_mac)
+{
+	return 0;
+}
+
+static int rtl8169_pcs_validate(struct phylink_pcs *pcs,
+				unsigned long *supported,
+				const struct phylink_link_state *state)
+{
+	return 0;
+}
+
+static void rtl8169_pcs_an_restart(struct phylink_pcs *pcs)
+{
+}
+
 static void rtl_mac_disable_tx_lpi(struct phylink_config *config)
 {
 	struct rtl8169_private *tp = container_of(config,
@@ -5681,6 +5792,13 @@ static unsigned long rtl8169_get_lpi_caps(struct rtl8169_private *tp)
 	return caps;
 }
 
+static const struct phylink_pcs_ops r8169_pcs_ops = {
+	.pcs_validate = rtl8169_pcs_validate,
+	.pcs_get_state = rtl8169_pcs_get_state,
+	.pcs_config = rtl8169_pcs_config,
+	.pcs_an_restart = rtl8169_pcs_an_restart,
+};
+
 static int rtl_init_phylink(struct rtl8169_private *tp)
 {
 	struct phylink *pl;
@@ -5692,10 +5810,18 @@ static int rtl_init_phylink(struct rtl8169_private *tp)
 	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) {
+	switch (tp->sfp_mode) {
+	case RTL_SFP_8168_AF:
+		tp->pcs.ops = &r8169_pcs_ops;
+		tp->phylink_config.default_an_inband = true;
+		phy_mode = PHY_INTERFACE_MODE_1000BASEX;
+		tp->phylink_config.mac_capabilities |= MAC_1000FD;
+		break;
+	case RTL_SFP_8127_ATF:
 		phy_mode = PHY_INTERFACE_MODE_INTERNAL;
 		tp->phylink_config.mac_capabilities |= MAC_10000FD;
-	} else {
+		break;
+	default:
 		tp->phylink_config.mac_capabilities |= MAC_10 | MAC_100;
 
 		if (tp->mac_version == RTL_GIGA_MAC_VER_80)
@@ -5719,6 +5845,7 @@ static int rtl_init_phylink(struct rtl8169_private *tp)
 				   PHY_INTERFACE_MODE_MII;
 		else
 			phy_mode = PHY_INTERFACE_MODE_INTERNAL;
+		break;
 	}
 
 	__set_bit(phy_mode, tp->phylink_config.supported_interfaces);
@@ -5816,12 +5943,7 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	}
 	tp->aspm_manageable = !rc;
 
-	if (rtl_is_8125(tp)) {
-		u16 data = r8168_mac_ocp_read(tp, 0xd006);
-
-		if ((data & 0xff) == 0x07)
-			tp->sfp_mode = true;
-	}
+	tp->sfp_mode = rtl_get_sfp_mode(tp);
 
 	tp->dash_type = rtl_get_dash_type(tp);
 	tp->dash_enabled = rtl_dash_is_enabled(tp);
@@ -5929,10 +6051,12 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if (rc)
 		return rc;
 
-	rc = r8169_mdio_register(tp);
-	if (rc) {
-		phylink_destroy(tp->phylink);
-		return rc;
+	if (tp->sfp_mode != RTL_SFP_8168_AF) {
+		rc = r8169_mdio_register(tp);
+		if (rc) {
+			phylink_destroy(tp->phylink);
+			return rc;
+		}
 	}
 
 	rc = register_netdev(dev);
-- 
2.43.0

[PATCH net-next v9 5/7] r8169: add support for RTL8127atf

[javen]

From: Javen Xu <javen_xu@realsil.com.cn>

RTL8127atf is also a fiber mode card, but its sds reg base addr is
0x0080, which is different from RTL8116af. Add 10g and 1g support for
RTL8127atf in this patch.

Signed-off-by: Javen Xu <javen_xu@realsil.com.cn>
---
Changes in v3:
 - No changes. New file.

Changes in v4:
 - remove DUMMY_PHY in driver/net/phy/realtek/realtek_main.c and related
   function

Changes in v5:
 - no changes, 1G support will be submitted by a following patch

Changes in v6:
 - no changes, merge patch 7/8 and 8/8 in v5
 - r8127_sds_read/write, return -ETIMEDOUT when time out
 - rtl8169_pcs_get_state set speed according to state->interface
 - register PHY_INTERFACE_MODE_10GBASER and PHY_INTERFACE_MODE_1000BASEX
   to supported_interfaces for RTL8127atf

Changes in v7:
 - no changes

Changes in v8:
 - no changes

Changes in v9:
 - no changes
---
 drivers/net/ethernet/realtek/r8169_main.c | 180 ++++++++++++++++------
 drivers/net/phy/realtek/realtek_main.c    |  54 -------
 include/net/phy/realtek_phy.h             |   7 -
 3 files changed, 132 insertions(+), 109 deletions(-)
 delete mode 100644 include/net/phy/realtek_phy.h

diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index a04acb42e995..eecde26d9de2 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c
@@ -32,7 +32,6 @@
 #include <linux/unaligned.h>
 #include <net/ip6_checksum.h>
 #include <net/netdev_queues.h>
-#include <net/phy/realtek_phy.h>
 
 #include "r8169.h"
 #include "r8169_firmware.h"
@@ -97,11 +96,18 @@
 #define JUMBO_9K	(9 * SZ_1K - VLAN_ETH_HLEN - ETH_FCS_LEN)
 #define JUMBO_16K	(SZ_16K - VLAN_ETH_HLEN - ETH_FCS_LEN)
 
+#define R8127_SDS_CMD		0x2348
+#define R8127_SDS_ADDR		0x234a
+#define R8127_SDS_DATA_IN	0x234c
+#define R8127_SDS_DATA_OUT	0x234e
+#define R8127_SDS_CMD_EXEC	BIT(0)
+#define R8127_SDS_CMD_WE	BIT(1)
 #define OCP_SDS_ADDR_REG	0xeb10
 #define OCP_SDS_CMD_REG		0xeb0e
 #define OCP_SDS_DATA_REG	0xeb14
 #define SDS_CMD_READ		0x0001
 #define RTL_SDS_C22_BASE	0x40
+#define RTL_SDS_C45_BASE	0x0080
 #define RTL_PKG_DETECT		0xdc00
 #define RTL_PKG_DETECT_MASK	0x0078
 #define RTL_PKG_DETECT_8116AF	0x0030
@@ -1151,11 +1157,6 @@ static int r8168_phy_ocp_read(struct rtl8169_private *tp, u32 reg)
 	if (rtl_ocp_reg_failure(reg))
 		return 0;
 
-	/* Return dummy MII_PHYSID2 in SFP mode to match SFP PHY driver */
-	if (tp->sfp_mode == RTL_SFP_8127_ATF &&
-	    reg == (OCP_STD_PHY_BASE + 2 * MII_PHYSID2))
-		return PHY_ID_RTL_DUMMY_SFP & 0xffff;
-
 	RTL_W32(tp, GPHY_OCP, reg << 15);
 
 	return rtl_loop_wait_high(tp, &rtl_ocp_gphy_cond, 25, 10) ?
@@ -1223,6 +1224,75 @@ static void r8127_sfp_sds_phy_reset(struct rtl8169_private *tp)
 	usleep_range(10, 20);
 }
 
+DECLARE_RTL_COND(r8127_sds_cmd_cond)
+{
+	return RTL_R16(tp, R8127_SDS_CMD) & R8127_SDS_CMD_EXEC;
+}
+
+static int r8127_sds_read(struct rtl8169_private *tp, u16 index,
+			  u16 page, u16 reg)
+{
+	u16 addr = (index << 11) | (page << 5) | reg;
+
+	RTL_W16(tp, R8127_SDS_ADDR, addr);
+	RTL_W16(tp, R8127_SDS_CMD, R8127_SDS_CMD_EXEC);
+
+	if (rtl_loop_wait_low(tp, &r8127_sds_cmd_cond, 10, 100))
+		return RTL_R16(tp, R8127_SDS_DATA_OUT);
+
+	return -ETIMEDOUT;
+}
+
+static int r8127_sds_write(struct rtl8169_private *tp, u16 index, u16 page,
+			   u16 reg, u16 val)
+{
+	u16 addr = (index << 11) | (page << 5) | reg;
+
+	RTL_W16(tp, R8127_SDS_DATA_IN, val);
+	RTL_W16(tp, R8127_SDS_ADDR, addr);
+	RTL_W16(tp, R8127_SDS_CMD, R8127_SDS_CMD_EXEC | R8127_SDS_CMD_WE);
+
+	if (rtl_loop_wait_low(tp, &r8127_sds_cmd_cond, 10, 100))
+		return 0;
+
+	return -ETIMEDOUT;
+}
+
+static void r8127_sds_modify(struct rtl8169_private *tp, u16 index, u16 page,
+			     u16 reg, u16 clearmask, u16 setmask)
+{
+	int val = r8127_sds_read(tp, index, page, reg);
+
+	if (val < 0)
+		return;
+
+	val = (val & ~clearmask) | setmask;
+	r8127_sds_write(tp, index, page, reg, val);
+}
+
+static void r8127_sfp_init_1g(struct rtl8169_private *tp)
+{
+	int val;
+
+	r8127_sfp_sds_phy_reset(tp);
+
+	r8127_sds_modify(tp, 0, 1, 31, 0, BIT(3));
+	r8127_sds_modify(tp, 0, 2,  0, BIT(13) | BIT(12) | BIT(6),
+			 BIT(12) | BIT(6));
+	r8127_sds_modify(tp, 0, 0,  4, 0, BIT(2));
+
+	RTL_W16(tp, 0x233a, 0x8004);
+	RTL_W16(tp, 0x233e, (RTL_R16(tp, 0x233e) & ~0x3003) | 0x0002);
+
+	r8168_phy_ocp_write(tp, 0xc40a, 0x0000);
+	r8168_phy_ocp_write(tp, 0xc466, 0x0000);
+	r8168_phy_ocp_write(tp, 0xc808, 0x0000);
+	r8168_phy_ocp_write(tp, 0xc80a, 0x0000);
+
+	val = r8168_phy_ocp_read(tp, 0xc804);
+	r8168_phy_ocp_write(tp, 0xc804, (val & ~0x000f) | 0x000c);
+}
+
 static void r8127_sfp_init_10g(struct rtl8169_private *tp)
 {
 	int val;
@@ -1241,12 +1311,6 @@ static void r8127_sfp_init_10g(struct rtl8169_private *tp)
 	r8168_phy_ocp_write(tp, 0xc804, (val & ~0x000f) | 0x000c);
 }
 
-static void rtl_sfp_init(struct rtl8169_private *tp)
-{
-	if (tp->mac_version == RTL_GIGA_MAC_VER_80)
-		r8127_sfp_init_10g(tp);
-}
-
 static void rtl_sfp_reset(struct rtl8169_private *tp)
 {
 	if (tp->mac_version == RTL_GIGA_MAC_VER_80)
@@ -2436,30 +2500,8 @@ static int rtl8169_set_link_ksettings(struct net_device *ndev,
 				      const struct ethtool_link_ksettings *cmd)
 {
 	struct rtl8169_private *tp = netdev_priv(ndev);
-	struct phy_device *phydev = tp->phydev;
-	int duplex = cmd->base.duplex;
-	int speed = cmd->base.speed;
-
-	if (tp->sfp_mode != RTL_SFP_8127_ATF)
-		return phylink_ethtool_ksettings_set(tp->phylink, cmd);
-
-	if (cmd->base.autoneg != AUTONEG_DISABLE)
-		return -EINVAL;
-
-	if (!phy_check_valid(speed, duplex, phydev->supported))
-		return -EINVAL;
-
-	mutex_lock(&phydev->lock);
 
-	phydev->autoneg = AUTONEG_DISABLE;
-	phydev->speed = speed;
-	phydev->duplex = duplex;
-
-	rtl_sfp_init(tp);
-
-	mutex_unlock(&phydev->lock);
-
-	return 0;
+	return phylink_ethtool_ksettings_set(tp->phylink, cmd);
 }
 
 static int rtl8169_nway_reset(struct net_device *dev)
@@ -2608,9 +2650,6 @@ static void rtl8169_init_phy(struct rtl8169_private *tp)
 	    tp->pci_dev->subsystem_device == 0xe000)
 		phy_write_paged(tp->phydev, 0x0001, 0x10, 0xf01b);
 
-	if (tp->sfp_mode == RTL_SFP_8127_ATF)
-		rtl_sfp_init(tp);
-
 	/* We may have called phy_speed_down before */
 	phy_speed_up(tp->phydev);
 
@@ -4942,7 +4981,7 @@ static irqreturn_t rtl8169_interrupt(int irq, void *dev_instance)
 	if (status & LinkChg) {
 		if (tp->phydev)
 			phy_mac_interrupt(tp->phydev);
-		else if (tp->sfp_mode == RTL_SFP_8168_AF)
+		else if (tp->sfp_mode)
 			phylink_mac_change(tp->phylink,
 					   !!(RTL_R8(tp, PHYstatus) & LinkStatus));
 	}
@@ -5681,7 +5720,8 @@ static struct phylink_pcs *rtl_mac_select_pcs(struct phylink_config *config,
 	if (!tp->pcs.ops)
 		return NULL;
 
-	if (interface == PHY_INTERFACE_MODE_1000BASEX)
+	if (interface == PHY_INTERFACE_MODE_1000BASEX ||
+	    interface == PHY_INTERFACE_MODE_10GBASER)
 		return &tp->pcs;
 
 	return NULL;
@@ -5712,12 +5752,32 @@ static void rtl8169_pcs_get_state(struct phylink_pcs *pcs,
 {
 	struct rtl8169_private *tp = container_of(pcs, struct rtl8169_private,
 						  pcs);
-	u16 bmsr, lpa;
 
-	bmsr = rtl8169_sds_read(tp, RTL_SDS_C22_BASE + MII_BMSR);
-	lpa = rtl8169_sds_read(tp, RTL_SDS_C22_BASE + MII_LPA);
+	if (tp->sfp_mode == RTL_SFP_8127_ATF) {
+		u16 stat1;
+
+		stat1 = rtl8169_sds_read(tp, RTL_SDS_C45_BASE + MDIO_STAT1);
 
-	phylink_mii_c22_pcs_decode_state(state, neg_mode, bmsr, lpa);
+		if (!(stat1 & MDIO_STAT1_LSTATUS))
+			stat1 = rtl8169_sds_read(tp, RTL_SDS_C45_BASE + MDIO_STAT1);
+
+		state->link = !!(stat1 & MDIO_STAT1_LSTATUS);
+		if (!state->link)
+			return;
+
+		if (state->interface == PHY_INTERFACE_MODE_1000BASEX)
+			state->speed = SPEED_1000;
+		else
+			state->speed = SPEED_10000;
+
+		state->duplex = DUPLEX_FULL;
+	} else {
+		u16 bmsr, lpa;
+
+		bmsr = rtl8169_sds_read(tp, RTL_SDS_C22_BASE + MII_BMSR);
+		lpa = rtl8169_sds_read(tp, RTL_SDS_C22_BASE + MII_LPA);
+		phylink_mii_c22_pcs_decode_state(state, neg_mode, bmsr, lpa);
+	}
 }
 
 static int rtl8169_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
@@ -5725,6 +5785,24 @@ static int rtl8169_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
 			      const unsigned long *advertising,
 			      bool permit_pause_to_mac)
 {
+	struct rtl8169_private *tp = container_of(pcs, struct rtl8169_private,
+						  pcs);
+
+	if (tp->sfp_mode == RTL_SFP_8127_ATF) {
+		switch (interface) {
+		case PHY_INTERFACE_MODE_10GBASER:
+			r8127_sfp_init_10g(tp);
+			break;
+		case PHY_INTERFACE_MODE_1000BASEX:
+			r8127_sfp_init_1g(tp);
+			break;
+		default:
+			netdev_err(tp->dev, "Unsupported SFP interface mode: %s\n",
+				   phy_modes(interface));
+			return -EOPNOTSUPP;
+		}
+	}
+
 	return 0;
 }
 
@@ -5774,7 +5852,7 @@ static unsigned long rtl8169_get_lpi_caps(struct rtl8169_private *tp)
 {
 	unsigned long caps = 0;
 
-	if (!rtl_supports_eee(tp))
+	if (!rtl_supports_eee(tp) || tp->sfp_mode == RTL_SFP_8127_ATF)
 		return 0;
 
 	caps |= MAC_100FD | MAC_1000FD;
@@ -5818,8 +5896,14 @@ static int rtl_init_phylink(struct rtl8169_private *tp)
 		tp->phylink_config.mac_capabilities |= MAC_1000FD;
 		break;
 	case RTL_SFP_8127_ATF:
-		phy_mode = PHY_INTERFACE_MODE_INTERNAL;
-		tp->phylink_config.mac_capabilities |= MAC_10000FD;
+		tp->pcs.ops = &r8169_pcs_ops;
+		phy_mode = PHY_INTERFACE_MODE_10GBASER;
+		tp->phylink_config.default_an_inband = true;
+		tp->phylink_config.mac_capabilities |= MAC_1000FD | MAC_10000FD;
+		__set_bit(PHY_INTERFACE_MODE_10GBASER,
+			  tp->phylink_config.supported_interfaces);
+		__set_bit(PHY_INTERFACE_MODE_1000BASEX,
+			  tp->phylink_config.supported_interfaces);
 		break;
 	default:
 		tp->phylink_config.mac_capabilities |= MAC_10 | MAC_100;
@@ -6051,7 +6135,7 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if (rc)
 		return rc;
 
-	if (tp->sfp_mode != RTL_SFP_8168_AF) {
+	if (tp->sfp_mode == RTL_SFP_NONE) {
 		rc = r8169_mdio_register(tp);
 		if (rc) {
 			phylink_destroy(tp->phylink);
diff --git a/drivers/net/phy/realtek/realtek_main.c b/drivers/net/phy/realtek/realtek_main.c
index 177b62a7b2d1..27e31a799e2c 100644
--- a/drivers/net/phy/realtek/realtek_main.c
+++ b/drivers/net/phy/realtek/realtek_main.c
@@ -19,7 +19,6 @@
 #include <linux/delay.h>
 #include <linux/clk.h>
 #include <linux/string_choices.h>
-#include <net/phy/realtek_phy.h>
 
 #include "../phylib.h"
 #include "realtek.h"
@@ -3044,45 +3043,6 @@ static irqreturn_t rtl8221b_handle_interrupt(struct phy_device *phydev)
 	return IRQ_HANDLED;
 }
 
-static int rtlgen_sfp_get_features(struct phy_device *phydev)
-{
-	linkmode_set_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT,
-			 phydev->supported);
-
-	/* set default mode */
-	phydev->speed = SPEED_10000;
-	phydev->duplex = DUPLEX_FULL;
-
-	phydev->port = PORT_FIBRE;
-
-	return 0;
-}
-
-static int rtlgen_sfp_read_status(struct phy_device *phydev)
-{
-	int val, err;
-
-	err = genphy_update_link(phydev);
-	if (err)
-		return err;
-
-	if (!phydev->link)
-		return 0;
-
-	val = phy_read(phydev, RTL_PHYSR);
-	if (val < 0)
-		return val;
-
-	rtlgen_decode_physr(phydev, val);
-
-	return 0;
-}
-
-static int rtlgen_sfp_config_aneg(struct phy_device *phydev)
-{
-	return 0;
-}
-
 static struct phy_driver realtek_drvs[] = {
 	{
 		PHY_ID_MATCH_EXACT(0x00008201),
@@ -3332,20 +3292,6 @@ static struct phy_driver realtek_drvs[] = {
 		.write_page	= rtl821x_write_page,
 		.read_mmd	= rtl822x_read_mmd,
 		.write_mmd	= rtl822x_write_mmd,
-	}, {
-		PHY_ID_MATCH_EXACT(PHY_ID_RTL_DUMMY_SFP),
-		.name		= "Realtek SFP PHY Mode",
-		.flags		= PHY_IS_INTERNAL,
-		.probe		= rtl822x_probe,
-		.get_features	= rtlgen_sfp_get_features,
-		.config_aneg	= rtlgen_sfp_config_aneg,
-		.read_status	= rtlgen_sfp_read_status,
-		.suspend	= genphy_suspend,
-		.resume		= rtlgen_resume,
-		.read_page	= rtl821x_read_page,
-		.write_page	= rtl821x_write_page,
-		.read_mmd	= rtl822x_read_mmd,
-		.write_mmd	= rtl822x_write_mmd,
 	}, {
 		PHY_ID_MATCH_EXACT(0x001ccad0),
 		.name		= "RTL8224 2.5Gbps PHY",
diff --git a/include/net/phy/realtek_phy.h b/include/net/phy/realtek_phy.h
deleted file mode 100644
index d683bc1b0659..000000000000
--- a/include/net/phy/realtek_phy.h
+++ /dev/null
@@ -1,7 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 */
-#ifndef _REALTEK_PHY_H
-#define _REALTEK_PHY_H
-
-#define	PHY_ID_RTL_DUMMY_SFP	0x001ccbff
-
-#endif /* _REALTEK_PHY_H */
-- 
2.43.0

[PATCH net-next v9 6/7] r8169: add ltr support for RTL8117 series

[javen]

From: Javen Xu <javen_xu@realsil.com.cn>

This patch adds ltr support for RTL8117 series, enables RTL8117 series
enter l1.2 state. This makes sense for the system to enter c10 state.

This patch drops the COMBO_LTR_EXTEND_EN write for VER_52. This is
intentional, as this bit is not required for RTL8116af.

Signed-off-by: Javen Xu <javen_xu@realsil.com.cn>
---
Changes in v2:
 - no changes

Changes in v3:
 - no changes

Changes in v4:
 - no changes

Changes in v5:
 - no changes

Changes in v6:
 - no changes

Changes in v7:
 - no changes

Changes in v8:
 - no changes

Changes in v9:
 - no changes
---
 drivers/net/ethernet/realtek/r8169_main.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index eecde26d9de2..5dbbc3a48c09 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c
@@ -353,11 +353,13 @@ enum rtl_registers {
 	ALDPS_LTR	= 0xe0a2,
 	LTR_OBFF_LOCK	= 0xe032,
 	LTR_SNOOP	= 0xe034,
+	SEND_LTR_MSG	= 0xe038,
 
 #define ALDPS_LTR_EN			BIT(0)
 #define LTR_OBFF_LOCK_EN		BIT(0)
 #define LINK_SPEED_CHANGE_EN		BIT(14)
 #define LTR_SNOOP_EN			GENMASK(15, 14)
+#define LTR_MSG_EN			BIT(0)
 };
 
 enum rtl8168_8101_registers {
@@ -3194,8 +3196,23 @@ static void rtl_enable_ltr(struct rtl8169_private *tp)
 		r8168_mac_ocp_write(tp, 0xcdf2, 0x9003);
 		r8168_mac_ocp_modify(tp, LTR_OBFF_LOCK, 0x0000, LINK_SPEED_CHANGE_EN);
 		break;
-	case RTL_GIGA_MAC_VER_46 ... RTL_GIGA_MAC_VER_48:
 	case RTL_GIGA_MAC_VER_52:
+		r8168_mac_ocp_write(tp, 0xcdd0, 0x9003);
+		r8168_mac_ocp_modify(tp, LTR_SNOOP, 0x0000, LTR_SNOOP_EN);
+		r8168_mac_ocp_write(tp, 0xe02c, 0x1880);
+		r8168_mac_ocp_write(tp, 0xe02e, 0x4880);
+		r8168_mac_ocp_modify(tp, ALDPS_LTR, 0x0000, ALDPS_LTR_EN);
+		r8168_mac_ocp_write(tp, 0xcdd8, 0x9003);
+		r8168_mac_ocp_write(tp, 0xcdda, 0x9003);
+		r8168_mac_ocp_write(tp, 0xcddc, 0x9003);
+		r8168_mac_ocp_write(tp, 0xcdd2, 0x883c);
+		r8168_mac_ocp_write(tp, 0xcdd4, 0x8c12);
+		r8168_mac_ocp_write(tp, 0xcdd6, 0x9003);
+		r8168_mac_ocp_write(tp, 0xe0a6, 0x9003);
+		r8168_mac_ocp_write(tp, 0xe0a8, 0x9003);
+		r8168_mac_ocp_modify(tp, LTR_OBFF_LOCK, 0x0000, LINK_SPEED_CHANGE_EN);
+		break;
+	case RTL_GIGA_MAC_VER_46 ... RTL_GIGA_MAC_VER_48:
 		r8168_mac_ocp_modify(tp, ALDPS_LTR, 0x0000, ALDPS_LTR_EN);
 		RTL_W8(tp, COMBO_LTR_EXTEND, RTL_R8(tp, COMBO_LTR_EXTEND) | COMBO_LTR_EXTEND_EN);
 		fallthrough;
@@ -3215,6 +3232,7 @@ static void rtl_enable_ltr(struct rtl8169_private *tp)
 	}
 	/* chip can trigger LTR */
 	r8168_mac_ocp_modify(tp, LTR_OBFF_LOCK, 0x0003, LTR_OBFF_LOCK_EN);
+	r8168_mac_ocp_modify(tp, SEND_LTR_MSG, 0x0000, LTR_MSG_EN);
 }
 
 static void rtl_hw_aspm_clkreq_enable(struct rtl8169_private *tp, bool enable)
@@ -3248,6 +3266,7 @@ static void rtl_hw_aspm_clkreq_enable(struct rtl8169_private *tp, bool enable)
 		rtl_enable_ltr(tp);
 		switch (tp->mac_version) {
 		case RTL_GIGA_MAC_VER_46 ... RTL_GIGA_MAC_VER_48:
+		case RTL_GIGA_MAC_VER_52:
 		case RTL_GIGA_MAC_VER_61 ... RTL_GIGA_MAC_VER_LAST:
 			/* reset ephy tx/rx disable timer */
 			r8168_mac_ocp_modify(tp, 0xe094, 0xff00, 0);
@@ -3260,6 +3279,7 @@ static void rtl_hw_aspm_clkreq_enable(struct rtl8169_private *tp, bool enable)
 	} else {
 		switch (tp->mac_version) {
 		case RTL_GIGA_MAC_VER_46 ... RTL_GIGA_MAC_VER_48:
+		case RTL_GIGA_MAC_VER_52:
 		case RTL_GIGA_MAC_VER_61 ... RTL_GIGA_MAC_VER_LAST:
 			r8168_mac_ocp_modify(tp, 0xe092, 0x00ff, 0);
 			break;
-- 
2.43.0

[PATCH net-next v9 7/7] r8169: fix RTL8116af can not enter s0idle and c10

[javen]

From: Javen Xu <javen_xu@realsil.com.cn>

RTL8116AF is a multi-function device. Functions 2 to 7 are hidden from
the PCI core and return an all-ones response when their vendor ID is read,
so they are not enumerated as normal PCI functions.

However, these hidden functions can still affect platform power
management. If they are left in D0 or keep ASPM disabled, the platform may
fail to enter the low-power s0ix state and the CPU package may fail to
enter Package C10.

Put functions 2 to 7 into D3hot and enable ASPM on their PCIe link control
register. Since these functions are hidden, access their configuration
space through pci_bus_read_config_dword() / pci_bus_write_config_dword()
using the same slot and the target function numbers.

Ignore functions that return a PCI error response when reading their
configuration space.

Signed-off-by: Javen Xu <javen_xu@realsil.com.cn>
---
Changes in v2:
 - no changes

Changes in v3:
 - no changes

Changes in v4:
 - add gate for rtl_lowpower_hidden_functions, only for RTL8116af

Changes in v5:
 - no changes

Changes in v6:
 - no changes

Changes in v7:
 - replace PCI_D3HOT with RTL_PM_CTRL_D3HOT, PCI_D3HOT has type
   restricted pci_power_t, but we only need unsigned int type

Changes in v8:
 - no changes

Changes in v9:
 - no changes
---
 drivers/net/ethernet/realtek/r8169_main.c | 42 +++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index 5dbbc3a48c09..5415ff62a286 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c
@@ -114,6 +114,7 @@
 #define RTL_INT_HW_ID		0xd006
 #define RTL_INT_HW_ID_MASK	0x00ff
 #define RTL_INT_HW_ID_8116AF	0x0000
+#define RTL_PM_CTRL_D3HOT	0x03
 
 static const struct rtl_chip_info {
 	u32 mask;
@@ -3773,6 +3774,41 @@ static void rtl_hw_start_8168ep_3(struct rtl8169_private *tp)
 	r8168_mac_ocp_modify(tp, 0xe860, 0x0000, 0x0080);
 }
 
+static void rtl_lowpower_hidden_functions(struct pci_dev *pdev)
+{
+	unsigned int slot = PCI_SLOT(pdev->devfn);
+	struct pci_bus *bus = pdev->bus;
+	int func, pos;
+	u16 val;
+
+	for (func = 2; func < 8; func++) {
+		unsigned int devfn = PCI_DEVFN(slot, func);
+
+		pos = pci_bus_find_capability(bus, devfn, PCI_CAP_ID_EXP);
+		if (pos) {
+			pci_bus_read_config_word(bus, devfn, pos + PCI_EXP_LNKCTL, &val);
+
+			if (PCI_POSSIBLE_ERROR(val))
+				continue;
+
+			val |= (PCI_EXP_LNKCTL_ASPMC | PCI_EXP_LNKCTL_CLKREQ_EN);
+			pci_bus_write_config_word(bus, devfn, pos + PCI_EXP_LNKCTL, val);
+		}
+
+		pos = pci_bus_find_capability(bus, devfn, PCI_CAP_ID_PM);
+		if (pos) {
+			pci_bus_read_config_word(bus, devfn, pos + PCI_PM_CTRL, &val);
+
+			if (PCI_POSSIBLE_ERROR(val))
+				continue;
+
+			val &= ~PCI_PM_CTRL_STATE_MASK;
+			val |= (RTL_PM_CTRL_D3HOT | PCI_PM_CTRL_PME_STATUS);
+			pci_bus_write_config_word(bus, devfn, pos + PCI_PM_CTRL, val);
+		}
+	}
+}
+
 static void rtl_hw_start_8117(struct rtl8169_private *tp)
 {
 	static const struct ephy_info e_info_8117[] = {
@@ -5326,6 +5362,9 @@ static int rtl8169_resume(struct device *device)
 	/* Some chip versions may truncate packets without this initialization */
 	rtl_init_rxcfg(tp);
 
+	if (rtl_is_8116af(tp))
+		rtl_lowpower_hidden_functions(tp->pci_dev);
+
 	return rtl8169_runtime_resume(device);
 }
 
@@ -6190,6 +6229,9 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 		rtl8168_driver_start(tp);
 	}
 
+	if (rtl_is_8116af(tp))
+		rtl_lowpower_hidden_functions(tp->pci_dev);
+
 	if (pci_dev_run_wake(pdev))
 		pm_runtime_put_sync(&pdev->dev);
 
-- 
2.43.0

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

[Andrew Lunn]

On Mon, Aug 31, 2026 at 01:37:38PM +0800, javen wrote:
> From: Javen Xu <javen_xu@realsil.com.cn>
> 
> This series patch adds support for phylink. RTL8116af is a fiber mode
> card, link status and speed can not be read from standard phy reg. So
> we read link status and speed from serdes reg by pcs. So as RTL8127atf.

I think there is still too much code accessing the PHY, but lets get
this merged, so it is easier to see what is left, and figure out how
it can be cleaned up.

	Andrew

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

[Andrew Lunn]

On Mon, Aug 31, 2026 at 01:37:41PM +0800, javen wrote:
> From: Javen Xu <javen_xu@realsil.com.cn>
> 
> Transfer old framework to phylink. Phylink can support fiber mode card
> which can not get link status or link speed from standard phy registers.
> 
> Signed-off-by: Javen Xu <javen_xu@realsil.com.cn>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next v9 4/7] r8169: add support for RTL8116af

[Andrew Lunn]

On Mon, Aug 31, 2026 at 01:37:42PM +0800, javen wrote:
> From: Javen Xu <javen_xu@realsil.com.cn>
> 
> RTL8116af is sfp mode. Phylink uses pcs to get the link status from its
> serdes reg, instead of standard phy reg. Speed and duplex are hardcoded
> to 1000Mbps Full-Duplex. Also, RTL8116af doesn't have internal phy, so
> we add some checks to ensure that tp->phydev is not empty when we need it.
> In rtl_hw_start_8117(), the MAC calibration for register 0xd412 relies
> on reading the internal PHY register 0x0c42. Since RTL8116af does not
> have an internal PHY, this calibration step is intentionally bypassed.
> 
> Signed-off-by: Javen Xu <javen_xu@realsil.com.cn>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next v9 5/7] r8169: add support for RTL8127atf

[Andrew Lunn]

On Mon, Aug 31, 2026 at 01:37:43PM +0800, javen wrote:
> From: Javen Xu <javen_xu@realsil.com.cn>
> 
> RTL8127atf is also a fiber mode card, but its sds reg base addr is
> 0x0080, which is different from RTL8116af. Add 10g and 1g support for
> RTL8127atf in this patch.
> 
> Signed-off-by: Javen Xu <javen_xu@realsil.com.cn>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next v9 6/7] r8169: add ltr support for RTL8117 series

[Andrew Lunn]

On Mon, Aug 31, 2026 at 01:37:44PM +0800, javen wrote:
> From: Javen Xu <javen_xu@realsil.com.cn>
> 
> This patch adds ltr support for RTL8117 series, enables RTL8117 series
> enter l1.2 state. This makes sense for the system to enter c10 state.
> 
> This patch drops the COMBO_LTR_EXTEND_EN write for VER_52. This is
> intentional, as this bit is not required for RTL8116af.
> 
> Signed-off-by: Javen Xu <javen_xu@realsil.com.cn>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next v9 7/7] r8169: fix RTL8116af can not enter s0idle and c10

[Andrew Lunn]

On Mon, Aug 31, 2026 at 01:37:45PM +0800, javen wrote:
> From: Javen Xu <javen_xu@realsil.com.cn>
> 
> RTL8116AF is a multi-function device. Functions 2 to 7 are hidden from
> the PCI core and return an all-ones response when their vendor ID is read,
> so they are not enumerated as normal PCI functions.
> 
> However, these hidden functions can still affect platform power
> management. If they are left in D0 or keep ASPM disabled, the platform may
> fail to enter the low-power s0ix state and the CPU package may fail to
> enter Package C10.
> 
> Put functions 2 to 7 into D3hot and enable ASPM on their PCIe link control
> register. Since these functions are hidden, access their configuration
> space through pci_bus_read_config_dword() / pci_bus_write_config_dword()
> using the same slot and the target function numbers.
> 
> Ignore functions that return a PCI error response when reading their
> configuration space.
> 
> Signed-off-by: Javen Xu <javen_xu@realsil.com.cn>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

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

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Mon, 31 Aug 2026 13:37:38 +0800 you wrote:
> From: Javen Xu <javen_xu@realsil.com.cn>
> 
> This series patch adds support for phylink. RTL8116af is a fiber mode
> card, link status and speed can not be read from standard phy reg. So
> we read link status and speed from serdes reg by pcs. So as RTL8127atf.
> 
> Javen Xu (7):
>   r8169: add speed in private struct
>   net: phy: phylink: add helper to modify pause
>   r8169: add support for phylink
>   r8169: add support for RTL8116af
>   r8169: add support for RTL8127atf
>   r8169: add ltr support for RTL8117 series
>   r8169: fix RTL8116af can not enter s0idle and c10
> 
> [...]

Here is the summary with links:
  - [net-next,v9,1/7] r8169: add speed in private struct
    https://git.kernel.org/netdev/net-next/c/ddffc67d89c8
  - [net-next,v9,2/7] net: phy: phylink: add helper to modify pause
    https://git.kernel.org/netdev/net-next/c/9f8cb9cbcdc8
  - [net-next,v9,3/7] r8169: add support for phylink
    https://git.kernel.org/netdev/net-next/c/8a3c76523e44
  - [net-next,v9,4/7] r8169: add support for RTL8116af
    https://git.kernel.org/netdev/net-next/c/dab515216def
  - [net-next,v9,5/7] r8169: add support for RTL8127atf
    https://git.kernel.org/netdev/net-next/c/97dee72abc37
  - [net-next,v9,6/7] r8169: add ltr support for RTL8117 series
    https://git.kernel.org/netdev/net-next/c/6e445f3d9538
  - [net-next,v9,7/7] r8169: fix RTL8116af can not enter s0idle and c10
    https://git.kernel.org/netdev/net-next/c/a306cbd79e07

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net-next v9 1/7] r8169: add speed in private struct

[netdev-bot+sashiko]

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

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

- [Low] Claimed stale-cache regression: the patch converts
  rtl_coalesce_info() (r8169_main.c:2076,2080) and…

--

r8169: add speed in private struct

This adds an int speed field to struct rtl8169_private, initializes it to
SPEED_UNKNOWN in rtl_init_one() and updates it from r8169_phylink_handler().
The consumers rtl_link_chg_patch(), rtl_coalesce_info() and
r8169_get_tx_lpi_timer_us() are switched from tp->phydev->speed to the new
cached value.

No Link: tags in the commit header.

> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> index ec4fc21fa21f..c60710f9bd21 100644
> --- a/drivers/net/ethernet/realtek/r8169_main.c
> +++ b/drivers/net/ethernet/realtek/r8169_main.c
> @@ -750,6 +750,7 @@ struct rtl8169_private {
>  	u32 irq_mask;
>  	int irq;
>  	struct clk *clk;
> +	int speed;
>  
>  	struct {
>  		DECLARE_BITMAP(flags, RTL_FLAG_MAX);

[ ... ]

> @@ -2074,11 +2073,11 @@ rtl_coalesce_info(struct rtl8169_private *tp)
>  		ci = rtl_coalesce_info_8168_8136;
>  
>  	/* if speed is unknown assume highest one */
> -	if (tp->phydev->speed == SPEED_UNKNOWN)
> +	if (tp->speed == SPEED_UNKNOWN)
>  		return ci;
>  
>  	for (; ci->speed; ci++) {
> -		if (tp->phydev->speed == ci->speed)
> +		if (tp->speed == ci->speed)
>  			return ci;
>  	}
>  
> @@ -2236,7 +2235,7 @@ static void rtl_set_eee_txidle_timer(struct rtl8169_private *tp)
>  
>  static unsigned int r8169_get_tx_lpi_timer_us(struct rtl8169_private *tp)
>  {
> -	unsigned int speed = tp->phydev->speed;
> +	unsigned int speed = tp->speed;
>  	unsigned int timer = tp->tx_lpi_timer;
>  
>  	if (!timer || speed == SPEED_UNKNOWN)
> @@ -4968,8 +4967,9 @@ static void r8169_phylink_handler(struct net_device *ndev)
>  	struct rtl8169_private *tp = netdev_priv(ndev);
>  	struct device *d = tp_to_dev(tp);
>  
> +	tp->speed = tp->phydev->speed;

[Severity: Low]
This isn't a bug, but the v3 entry in the changelog below the --- line says:

  Changes in v3:
   - update tp->speed in rtl8169_set_link_ksettings()

That hunk does not appear in this diff. After this patch the only runtime
writer of tp->speed is r8169_phylink_handler(), while the SFP branch of
rtl8169_set_link_ksettings() still writes phydev->speed directly and
reprograms the SerDes without touching tp->speed:

	if (!phy_check_valid(speed, duplex, phydev->supported))
		return -EINVAL;

	mutex_lock(&phydev->lock);

	phydev->autoneg = AUTONEG_DISABLE;
	phydev->speed = speed;
	phydev->duplex = duplex;

	rtl_sfp_init(tp);

	mutex_unlock(&phydev->lock);

Should the changelog entry be dropped, or was the intended
rtl8169_set_link_ksettings() update accidentally lost in a rebase?

Following the two converted consumers, the divergence does not look
observable today. tp->sfp_mode is only assigned inside
if (rtl_is_8125(tp)) in rtl_init_one(), and both rtl_get_coalesce() and
rtl_set_coalesce() start with:

	if (rtl_is_8125(tp))
		return -EOPNOTSUPP;

so rtl_coalesce_info() cannot be reached on the chips that take the SFP
write path. The SFP branch is also gated by phy_check_valid() against
phydev->supported, and rtlgen_sfp_get_features() in
drivers/net/phy/realtek/realtek_main.c only sets
ETHTOOL_LINK_MODE_10000baseT_Full_BIT, so no other speed can be forced
there, and r8169_get_tx_lpi_timer_us() keeps seeing the real link rate that
rtlgen_sfp_read_status() decodes from RTL_PHYSR.

The construct is also replaced later in this series by "r8169: add support
for phylink", where rtl8169_set_link_ksettings() only calls
phylink_ethtool_ksettings_set() and tp->speed is written from
rtl_mac_link_up() and rtl_mac_link_down(), so only the changelog/diff
mismatch remains.

>  	if (netif_carrier_ok(ndev)) {
> -		rtl_link_chg_patch(tp);
> +		rtl_link_chg_patch(tp, tp->speed);
>  		rtl_enable_tx_lpi(tp, tp->phydev->enable_tx_lpi);
>  		pm_request_resume(d);
>  	} else {
> @@ -5667,6 +5667,7 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>  				     ext_xid_str, xid);
>  	tp->mac_version = chip->mac_version;
>  	tp->fw_name = chip->fw_name;
> +	tp->speed = SPEED_UNKNOWN;

[ ... ]

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

Re: [PATCH net-next v9 2/7] net: phy: phylink: add helper to modify pause

[netdev-bot+sashiko]

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

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

- [High] phylink_update_mac_pause_capabilities() destroys, rather than
  masks, the requested pause configuration, so the "pause capability…
- [Medium] phylink_update_mac_pause_capabilities() mutates the live
  pl->supported mask in place: it unconditionally sets Pause and…
- [Medium] phylink_update_mac_pause_capabilities() has no guaranteed way
  to re-program the MAC after a pause-capability change: it relies on…
- [Medium] phylink_update_mac_pause_capabilities() conveys the capability
  change only by writing pl->config->mac_capabilities (lines 1923-1924)…
- [Medium] In phylink_update_mac_pause_capabilities(), the asymmetric-
  request test is written as '(pause_state & MLO_PAUSE_RX) ^…
- [Medium] phylink_update_mac_pause_capabilities() treats an asymmetric-
  only MAC capability as if pause were entirely unsupported: the first…
- [Medium] phylink_update_mac_pause_capabilities() omits the MLO_AN_FIXED
  guard that phylink_ethtool_set_pauseparam() applies before reaching…
- [Low] phylink_update_mac_pause_capabilities() updates
  pl->config->mac_capabilities and pl->supported inside one…
- [Low] The commit message describes only the Realtek motivation ("when
  we enable jumbo, pause are not supported.

--

net: phy: phylink: add helper to modify pause

This patch factors the body of phylink_ethtool_set_pauseparam() out into a
new static helper phylink_update_pause_state(), and adds a new exported
core API, phylink_update_mac_pause_capabilities(), which lets a MAC driver
withdraw or restore MAC_SYM_PAUSE/MAC_ASYM_PAUSE at runtime.  The stated
motivation is Realtek hardware that cannot do flow control while jumbo
frames are enabled.

No Link: tags are present in the commit header.

>     For Realtek nics, when we enable jumbo, pause are not supported. So we
>     must check the pause capabilities from ourself and lp.

[Severity: Low]

Could the commit message describe what the patch does, in addition to why?

Two things happen here that a reader of the log would want to know about:
phylink_update_pause_state() is split out of the existing ethtool path as a
behaviour-preserving refactor, and a new exported core API,
phylink_update_mac_pause_capabilities(), is added which mutates the
driver-owned pl->config->mac_capabilities, the phylink supported mask and
pl->link_config.

The only caller is added later in the same series, in rtl8169_change_mtu(),
which is the usual API-then-user layout, so that part is fine.

> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 3ec3bb439109..646b8e1e727b 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -1829,6 +1829,126 @@ int phylink_set_fixed_link(struct phylink *pl,
>  }
>  EXPORT_SYMBOL_GPL(phylink_set_fixed_link);
>  
> +/**
> + * phylink_update_pause_state() - Update the phylink pause frame configuration
> + * @pl: a pointer to a &struct phylink instance
> + * @pause_state: bitmask indicating the new pause state
> + *
> + * Update the MAC pause frame (flow control) state for the phylink instance.
> + */
> +static void phylink_update_pause_state(struct phylink *pl, int pause_state)
> +{
> +	struct phylink_link_state *config = &pl->link_config;
> +	bool tx_pause = !!(pause_state & MLO_PAUSE_TX);
> +	bool rx_pause = !!(pause_state & MLO_PAUSE_RX);
> +	bool manual_changed;
> +
> +	mutex_lock(&pl->state_mutex);
> +
> +	/*
> +	 * See the comments for linkmode_set_pause(), wrt the deficiencies
> +	 * with the current implementation.  A solution to this issue would
> +	 * be:
> +	 * ethtool  Local device
> +	 *  rx  tx  Pause AsymDir
> +	 *  0   0   0     0
> +	 *  1   0   1     1
> +	 *  0   1   0     1
> +	 *  1   1   1     1
> +	 * and then use the ethtool rx/tx enablement status to mask the
> +	 * rx/tx pause resolution.
> +	 */
> +	linkmode_set_pause(config->advertising, tx_pause,
> +			   rx_pause);
> +
> +	manual_changed = (config->pause ^ pause_state) & MLO_PAUSE_AN ||
> +			 (!(pause_state & MLO_PAUSE_AN) &&
> +			   (config->pause ^ pause_state) & MLO_PAUSE_TXRX_MASK);

[Severity: Medium]

This manual_changed test is the only thing that forces pl->link_failed and
phylink_run_resolve(), and it was written for the ethtool path where
MLO_PAUSE_AN can toggle.

When phylink_update_mac_pause_capabilities() calls in, only MLO_PAUSE_RX
and MLO_PAUSE_TX are ever cleared and MLO_PAUSE_AN never changes, so with
pause autoneg enabled (the phylink default, link_config.pause =
MLO_PAUSE_AN) manual_changed is false and no resolve is queued.

For an MLO_AN_INBAND SGMII setup with no phydev, is there anything left to
re-run mac_link_up()?  phylink_change_inband_advert() ends up in
phylink_mii_c22_pcs_encode_advertisement(), where the SGMII family has no
pause bits at all:

	case PHY_INTERFACE_MODE_SGMII:
	case PHY_INTERFACE_MODE_PSGMII:
	case PHY_INTERFACE_MODE_QSGMII:
		return 0x0001;

so pcs_config() reports no change, no AN restart happens and the link does
not cycle.  Does the MAC then keep the tx_pause/rx_pause programming from
the previous mac_link_up() call while phylink believes pause is
unsupported, which is the jumbo-plus-pause combination this series is
trying to avoid?  The capability-regained direction has the same gap, and
so does the PHY case when phydev->autoneg is clear, since
phy_set_asym_pause() only renegotiates when autoneg is set.

> +
> +	config->pause = pause_state;
> +
> +	/* Update our in-band advertisement, triggering a renegotiation if
> +	 * the advertisement changed.
> +	 */
> +	if (!pl->phydev)
> +		phylink_change_inband_advert(pl);
> +
> +	mutex_unlock(&pl->state_mutex);
> +
> +	/* If we have a PHY, a change of the pause frame advertisement will
> +	 * cause phylib to renegotiate (if AN is enabled) which will in turn
> +	 * call our phylink_phy_change() and trigger a resolve.  Note that
> +	 * we can't hold our state mutex while calling phy_set_asym_pause().
> +	 */
> +	if (pl->phydev)
> +		phy_set_asym_pause(pl->phydev, rx_pause, tx_pause);
> +
> +	/* If the manual pause settings changed, make sure we trigger a
> +	 * resolve to update their state; we can not guarantee that the
> +	 * link will cycle.
> +	 */
> +	if (manual_changed) {
> +		pl->link_failed = true;
> +		phylink_run_resolve(pl);
> +	}
> +}
> +
> +/**
> + * phylink_update_mac_pause_capabilities() - Dynamically update MAC pause
> + * @pl: a pointer to a &struct phylink returned from phylink_create()
> + * @mac_pause: the new MAC pause capabilities mask
> + *
> + * This function allows a MAC driver to dynamically change its pause state,
> + * such as losing/gaining Pause frame support based on MTU size.
> + * It recalculates supported link modes and triggers renegotiation if needed.
> + */
> +void phylink_update_mac_pause_capabilities(struct phylink *pl, unsigned long mac_pause)
> +{
> +	struct phylink_link_state *config = &pl->link_config;
> +	unsigned long old_pause;
> +	int pause_state;
> +
> +	ASSERT_RTNL();
> +
> +	if (mac_pause & ~(MAC_SYM_PAUSE | MAC_ASYM_PAUSE)) {
> +		phylink_err(pl, "Attempted to dynamically change non-pause MAC capabilities\n");
> +		return;
> +	}

[Severity: Medium]

The sibling entry point into the same (now shared) code guards it with:

phylink_ethtool_set_pauseparam()
	if (pl->req_link_an_mode == MLO_AN_FIXED)
		return -EOPNOTSUPP;

	if (!phylink_test(pl->supported, Pause) &&
	    !phylink_test(pl->supported, Asym_Pause))
		return -EOPNOTSUPP;

Should the new exported helper carry the MLO_AN_FIXED check too?  An MTU
change is independent of the AN mode, so a fixed-link MAC can reach here,
and phylink_update_pause_state() will then run
linkmode_set_pause(config->advertising, tx, rx) with tx/rx derived from
config->pause, which defaults to MLO_PAUSE_AN, i.e. false/false, clearing
ETHTOOL_LINK_MODE_Pause_BIT and Asym_Pause_BIT from
pl->link_config.advertising.

That mask is what fixed-link pause resolution consumes:

phylink_get_fixed_state()
	state->pause = MLO_PAUSE_NONE;
	phylink_resolve_an_pause(state);

with lp_advertising seeded from the DT properties in
phylink_parse_fixedlink():

	if (fwnode_property_read_bool(fixed_node, "pause"))
		__set_bit(ETHTOOL_LINK_MODE_Pause_BIT,
			  pl->link_config.lp_advertising);

Can this silently disable the flow control described in firmware, with no
error returned to the caller since the helper is void?

> +
> +	old_pause = pl->config->mac_capabilities & (MAC_SYM_PAUSE | MAC_ASYM_PAUSE);
> +	if (old_pause == mac_pause)
> +		return;
> +
> +	mutex_lock(&pl->state_mutex);
> +
> +	pl->config->mac_capabilities &= ~(MAC_SYM_PAUSE | MAC_ASYM_PAUSE);
> +	pl->config->mac_capabilities |= mac_pause;

[Severity: Medium]

Does writing config->mac_capabilities have any effect for a MAC that
implements mac_get_caps()?  phylink_validate_mac_and_pcs() prefers the
callback:

	if (pl->mac_ops->mac_get_caps)
		capabilities = pl->mac_ops->mac_get_caps(pl->config,
							 state->interface);
	else
		capabilities = pl->config->mac_capabilities;

and the mac_get_caps kernel-doc says config->mac_capabilities is only used
when the callback is absent.  stmmac_mac_get_caps() goes further and
overwrites the field the helper just wrote:

	config->mac_capabilities = priv->hw->link.caps;
	return priv->hw->link.caps;

For such a driver, would the phylink_test() checks below both still pass,
leaving pause_state unmasked while the advertisement is nevertheless
rewritten by phylink_update_pause_state()?  The helper is void and prints
no warning, so the driver cannot tell the update was a no-op.

Also, is the core expected to write into the driver-owned struct
phylink_config after phylink_create()?

> +
> +	phylink_set(pl->supported, Pause);
> +	phylink_set(pl->supported, Asym_Pause);
> +
> +	if (pl->phydev)
> +		linkmode_and(pl->supported, pl->supported, pl->phydev->supported);
> +	else if (pl->sfp_bus)
> +		linkmode_and(pl->supported, pl->supported, pl->sfp_support);
> +
> +	phylink_validate(pl, pl->supported, config);

[Severity: Medium]

Is it safe to force Pause and Asym_Pause into the live pl->supported mask
and then ignore the phylink_validate() return value?  The bits are only
removed again inside phylink_validate_mask_caps(), that is, on the success
path.

phylink_validate() has early returns that leave the caller's mask
untouched:

	if (!test_bit(state->interface, interfaces))
		return -EINVAL;

as do the mac_select_pcs() ERR_PTR, PCS interface mismatch and
pcs_validate() failures.  On those paths pl->supported keeps the
force-set Pause|Asym_Pause bits, so the phylink_test() checks just below
conclude pause is supported, the requested restriction is not applied and
ethtool keeps reporting pause the MAC has just declared unsupported.

Such a failure is reachable in exactly the case this API is for, since
phylink_get_capabilities() drops RATE_MATCH_PAUSE support when either
pause capability is missing:

		if (!(mac_capabilities & MAC_SYM_PAUSE) ||
		    !(mac_capabilities & MAC_ASYM_PAUSE))
			break;

On the state->interface == PHY_INTERFACE_MODE_NA path,
phylink_validate_mask() copies a possibly empty accumulator into supported
before returning -EINVAL, so can pl->supported end up empty and
pl->link_config.advertising rewritten, with no rollback and no way for the
void caller to notice?

Related question: every other phylink_validate*() call site validates into
a local mask, for example phylink_bringup_phy():

	ret = phylink_validate_phy(pl, phy, supported, &config);
	if (ret) { ... return ret; }

Validating here against the single pl->link_config.interface rather than
over phy->possible_interfaces or pl->sfp_interfaces looks like it would
permanently prune modes only reachable via other interface modes on c45
PHYs and multi-interface SFPs.  Is that intended?

> +
> +	pause_state = config->pause;

[Severity: High]

Does this destroy the requested pause configuration rather than mask it,
so that the capability-regained direction can never restore pause?

pause_state is read from config->pause, masked here, and then written back
into the same field by phylink_update_pause_state():

	linkmode_set_pause(config->advertising, tx_pause,
			   rx_pause);
	...
	config->pause = pause_state;
	...
	if (pl->phydev)
		phy_set_asym_pause(pl->phydev, rx_pause, tx_pause);

There is no saved copy of the original request, and the advertisement is
re-programmed from the masked value.

In the default configuration phylink_create() sets

	pl->link_config.pause = MLO_PAUSE_AN;

with no MLO_PAUSE_RX/MLO_PAUSE_TX, while phylink_bringup_phy() advertises
pause via phy_support_asym_pause() and copies those bits into
pl->link_config.advertising.  So even a call that grants
MAC_SYM_PAUSE|MAC_ASYM_PAUSE ends up running

	linkmode_set_pause(adv, false, false);
	phy_set_asym_pause(phydev, false, false);

and linkmode_set_pause() clears both bits for false/false:

	linkmode_mod_bit(ETHTOOL_LINK_MODE_Pause_BIT, advertisement, rx);
	linkmode_mod_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, advertisement,
			 rx ^ tx);

With the r8169 caller added later in the series that gives:

	ip link set eth0 mtu 9000
	  rtl8169_change_mtu()
	    phylink_update_mac_pause_capabilities(pl, 0)
	      -> pause bits cleared in phydev->advertising
	ip link set eth0 mtu 1500
	  rtl8169_change_mtu()
	    phylink_update_mac_pause_capabilities(pl, MAC_SYM_PAUSE|MAC_ASYM_PAUSE)
	      -> pause_state is still MLO_PAUSE_AN, bits cleared again

Nothing re-runs phy_support_asym_pause() or otherwise re-seeds the
advertisement, so is flow control lost for good after the first
transition?  Would keeping the user request separately and masking it only
at resolve time avoid this?

> +
> +	if (!phylink_test(pl->supported, Pause)) {
> +		pause_state &= ~(MLO_PAUSE_RX | MLO_PAUSE_TX);

[Severity: Medium]

Should this branch also consider Asym_Pause?  MAC_ASYM_PAUSE is accepted
independently of MAC_SYM_PAUSE, and phylink_caps_to_linkmodes() sets only
ETHTOOL_LINK_MODE_Asym_Pause_BIT for it, so pl->supported can legitimately
hold Asym_Pause without Pause.

The capability table in include/linux/phylink.h documents tx-only pause as
valid in that state, and phylink_ethtool_set_pauseparam() allows it:

	if (!phylink_test(pl->supported, Pause) &&
	    !phylink_test(pl->supported, Asym_Pause))
		return -EOPNOTSUPP;

After phylink_update_mac_pause_capabilities(pl, MAC_ASYM_PAUSE), does an
existing tx-only setting get cleared here and then un-advertised by
phylink_update_pause_state(), even though the MAC still supports it?

> +	} else if (!phylink_test(pl->supported, Asym_Pause)) {
> +		if ((pause_state & MLO_PAUSE_RX) ^ (pause_state & MLO_PAUSE_TX))
> +			pause_state &= ~(MLO_PAUSE_RX | MLO_PAUSE_TX);

[Severity: Medium]

Does this XOR detect an asymmetric request?  MLO_PAUSE_RX and MLO_PAUSE_TX
are different bit positions:

	MLO_PAUSE_RX = BIT(0),
	MLO_PAUSE_TX = BIT(1),

so 0x1 ^ 0x2 == 0x3 and the condition is non-zero whenever either bit is
set, including the symmetric rx=1/tx=1 case.  It looks equivalent to
pause_state & MLO_PAUSE_TXRX_MASK, so after "ethtool -A eth0 rx on tx on"
a symmetric-pause-capable MAC would have flow control cleared entirely.

The peer check in phylink_ethtool_set_pauseparam() expresses the intended
rule as a plain comparison:

	if (!phylink_test(pl->supported, Asym_Pause) &&
	    pause->rx_pause != pause->tx_pause)
		return -EINVAL;

Would !!(pause_state & MLO_PAUSE_RX) != !!(pause_state & MLO_PAUSE_TX) be
what is wanted here?

> +	}
> +
> +	mutex_unlock(&pl->state_mutex);
> +
> +	phylink_update_pause_state(pl, pause_state);

[Severity: Low]

The capability update and the dependent pause state are committed in two
separate pl->state_mutex sections: mac_capabilities and pl->supported are
written above, the mutex is dropped here, and phylink_update_pause_state()
re-acquires it to publish link_config.pause and link_config.advertising.

phylink_resolve() runs from system_power_efficient_wq without RTNL and
takes pl->state_mutex, so it can run in that window and see pl->supported
already saying pause is unsupported while link_config.pause still has
MLO_PAUSE_RX|MLO_PAUSE_TX, then call mac_ops->mac_link_up() with pause
enabled.  The observed combination equals the pre-call state, so this is a
transient window rather than a lasting one, but would doing both updates
under a single acquisition be simpler to reason about?

> +}
> +EXPORT_SYMBOL_GPL(phylink_update_mac_pause_capabilities);
> +

[ ... ]

> diff --git a/include/linux/phylink.h b/include/linux/phylink.h
> index 1dda5c7ed5f1..3a88a69882a6 100644
> --- a/include/linux/phylink.h
> +++ b/include/linux/phylink.h
> @@ -843,4 +843,6 @@ void phylink_replay_link_begin(struct phylink *pl);
>  
>  void phylink_replay_link_end(struct phylink *pl);
>  
> +void phylink_update_mac_pause_capabilities(struct phylink *pl, unsigned long mac_pause);
> +
>  #endif

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

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

[netdev-bot+sashiko]

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

Re: [PATCH net-next v9 4/7] r8169: add support for RTL8116af

[netdev-bot+sashiko]

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

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

- [Medium] For RTL_SFP_8168_AF the driver registers a phylink PCS and
  declares in-band 1000BASE-X autonegotiation (tp->pcs.ops =…
- [Medium] The phylink conversion in this series removed phy_stop() from
  rtl8169_down() and phy_start() from rtl8169_up() (both present at…
- [Medium] The newly added PCS callback rtl8169_pcs_get_state() (via
  rtl8169_sds_read()) performs indirect OCP/SerDes MMIO accesses with no…
- [Low] For the new no-PHY 1000BASEX mode the driver still advertises
  EEE/LPI capabilities derived from the copper-PHY version check:…

--

r8169: add support for RTL8116af

This adds detection of the RTL8116af SFP variant of RTL_GIGA_MAC_VER_52,
registers a phylink PCS that reads link state from the SerDes registers,
and guards the existing internal-PHY code paths against a NULL
tp->phydev.  A few review questions below.

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

[ ... ]

> @@ -4891,8 +4939,13 @@ static irqreturn_t rtl8169_interrupt(int irq, void *dev_instance)
>  		goto out;
>  	}
>  
> -	if (status & LinkChg)
> -		phy_mac_interrupt(tp->phydev);
> +	if (status & LinkChg) {
> +		if (tp->phydev)
> +			phy_mac_interrupt(tp->phydev);
> +		else if (tp->sfp_mode == RTL_SFP_8168_AF)
> +			phylink_mac_change(tp->phylink,
> +					   !!(RTL_R8(tp, PHYstatus) & LinkStatus));
> +	}
>  
>  	rtl_irq_disable(tp);
>  	napi_schedule(&tp->napi);

[ ... ]

> @@ -5002,7 +5055,7 @@ static void rtl8169_down(struct rtl8169_private *tp)
>  	bitmap_zero(tp->wk.flags, RTL_FLAG_MAX);
>  
>  	/* Reset SerDes PHY to bring down fiber link */
> -	if (tp->sfp_mode)
> +	if (tp->sfp_mode == RTL_SFP_8127_ATF)
>  		rtl_sfp_reset(tp);
>  
>  	rtl8169_update_counters(tp);

[Severity: Medium]
Should the PM paths also quiesce phylink here?

rtl8169_down() masks interrupts, runs rtl8169_cleanup(), pci_clear_master()
and rtl_prepare_power_down(), but nothing calls phylink_stop() or
phylink_suspend() on this path.  phylink_stop() is only reached from
rtl8169_close(), while rtl8169_suspend(), rtl8169_runtime_suspend() and
rtl_shutdown() all reach rtl8169_down() through rtl8169_net_suspend():

rtl8169_net_suspend()
    netif_device_detach(tp->dev);
    if (netif_running(tp->dev))
        rtl8169_down(tp);

Since pl->phylink_disable_state is only set by phylink_stop() and
phylink_suspend(), phylink_run_resolve() is still free to queue
phylink_resolve() on system_power_efficient_wq, including a resolve queued
from the rtl8169_interrupt() -> phylink_mac_change() call added above.  The
suspend callbacks drop rtnl_lock before the PCI core moves the device to
D3hot, so RTNL does not exclude it.

Can that resolve then run rtl8169_pcs_get_state() -> rtl8169_sds_read()
MMIO/OCP reads, or rtl_mac_link_up() -> rtl_link_chg_patch() ERI/MMIO
writes, against a device that is already powered down?

> @@ -5024,9 +5077,9 @@ static void rtl8169_up(struct rtl8169_private *tp)
>  		rtl8168_driver_start(tp);
>  
>  	pci_set_master(tp->pci_dev);
> -	phy_init_hw(tp->phydev);
> -	phy_resume(tp->phydev);
> -	rtl8169_init_phy(tp);
> +	if (tp->phydev)
> +		rtl8169_init_phy(tp);
> +
>  	napi_enable(&tp->napi);
>  	enable_work(&tp->wk.work);
>  	rtl_reset_work(tp);

[Severity: Medium]
Same question from the resume side: rtl8169_up() is the counterpart of
rtl8169_down(), and neither phylink_resume() nor phylink_start() is called
from rtl8169_resume() or rtl8169_runtime_resume().  Is the start/stop
pairing intentionally asymmetric, with phylink_start()/phylink_stop() only
in rtl_open()/rtl8169_close()?  For the copper case, phy_init_hw() and
phy_resume() are also re-run from rtl8169_init_phy() without phylink
knowing about it.

[ ... ]

> @@ -5628,6 +5692,53 @@ static void rtl_mac_config(struct phylink_config *config, unsigned int mode,
>  {
>  }
>  
> +static u16 rtl8169_sds_read(struct rtl8169_private *tp, u16 sds_reg)
> +{
> +	unsigned long flags;
> +	u16 val = 0;
> +
> +	raw_spin_lock_irqsave(&tp->mac_ocp_lock, flags);
> +	__r8168_mac_ocp_write(tp, OCP_SDS_ADDR_REG, sds_reg);
> +	__r8168_mac_ocp_write(tp, OCP_SDS_CMD_REG, SDS_CMD_READ);
> +	val = __r8168_mac_ocp_read(tp, OCP_SDS_DATA_REG);
> +	raw_spin_unlock_irqrestore(&tp->mac_ocp_lock, flags);
> +
> +	return val;
> +}
> +
> +static void rtl8169_pcs_get_state(struct phylink_pcs *pcs,
> +				  unsigned int neg_mode,
> +				  struct phylink_link_state *state)
> +{
> +	struct rtl8169_private *tp = container_of(pcs, struct rtl8169_private,
> +						  pcs);
> +	u16 bmsr, lpa;
> +
> +	bmsr = rtl8169_sds_read(tp, RTL_SDS_C22_BASE + MII_BMSR);
> +	lpa = rtl8169_sds_read(tp, RTL_SDS_C22_BASE + MII_LPA);
> +
> +	phylink_mii_c22_pcs_decode_state(state, neg_mode, bmsr, lpa);
> +}

[Severity: Medium]
Does this need a runtime PM guard before touching the SerDes registers?

For RTL_SFP_8168_AF there is no phydev, so phylink's mode is MLO_AN_INBAND
with pl->phydev == NULL, and phylink_ethtool_ksettings_get() takes the
unconditional PCS read path in that case:

	case MLO_AN_INBAND:
		if (pl->phydev)
			break;

		phylink_mac_pcs_get_state(pl, &link_state);

rtl8169_get_link_ksettings() is a bare wrapper around
phylink_ethtool_ksettings_get() with no pm_runtime_get*/pm_runtime_active()
check, unlike rtl8169_get_stats64() which brackets its hardware access.
phylink_mii_ioctl() with SIOCGMIIREG reaches the same code.

If a plain "ethtool ethX" runs while the device is runtime suspended (after
rtl8169_close(), or after rtl_mac_link_down() -> pm_runtime_idle() and
rtl8169_runtime_idle() -> pm_schedule_suspend()), the reads return 0xffff,
so bmsr has BMSR_LSTATUS and BMSR_ANEGCOMPLETE set and
phylink_mii_c22_pcs_decode_state() reports a bogus 1000/Full link.  On
platforms that do not decode MMIO in D3hot the access can raise an
unsupported-request instead.  Should this path take a runtime PM reference,
or bail out when the device is not active?

> +
> +static int rtl8169_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
> +			      phy_interface_t interface,
> +			      const unsigned long *advertising,
> +			      bool permit_pause_to_mac)
> +{
> +	return 0;
> +}
> +
> +static int rtl8169_pcs_validate(struct phylink_pcs *pcs,
> +				unsigned long *supported,
> +				const struct phylink_link_state *state)
> +{
> +	return 0;
> +}
> +
> +static void rtl8169_pcs_an_restart(struct phylink_pcs *pcs)
> +{
> +}

[Severity: Medium]
These two callbacks are registered for a mode that declares in-band
1000BASE-X autonegotiation, but neither of them touches the hardware.

include/linux/phylink.h documents the contract:

 * For 1000BASE-X, the advertisement should be programmed into the PCS.
 ...
 * When an autonegotiation restart is required for 802.3z modes,
 * .pcs_config() should return a positive non-zero integer (e.g. 1)

rtl8169_pcs_config() discards neg_mode, interface, advertising and
permit_pause_to_mac and returns 0, rtl8169_pcs_an_restart() is empty, and
rtl8169_sds_read() is the only SerDes accessor added, so there is no write
path at all.

Does this mean ethtool -s advertise=..., rtl8169_set_pauseparam() ->
phylink_ethtool_set_pauseparam() and rtl8169_nway_reset() ->
phylink_ethtool_nway_reset() all return success while the SerDes keeps
advertising whatever reset or firmware left there?  Since
phylink_resolve_an_pause() uses phylink's software advertisement, the
resolved pause can also differ from what the peer actually sees.

If autonegotiation really is fixed or firmware-managed on this part, would
it be clearer to express that through pcs_inband_caps or by rejecting the
unsupported configuration, rather than acknowledging the request?

> +
>  static void rtl_mac_disable_tx_lpi(struct phylink_config *config)
>  {
>  	struct rtl8169_private *tp = container_of(config,

[ ... ]

> @@ -5692,10 +5810,18 @@ static int rtl_init_phylink(struct rtl8169_private *tp)
>  	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) {
> +	switch (tp->sfp_mode) {
> +	case RTL_SFP_8168_AF:
> +		tp->pcs.ops = &r8169_pcs_ops;
> +		tp->phylink_config.default_an_inband = true;
> +		phy_mode = PHY_INTERFACE_MODE_1000BASEX;
> +		tp->phylink_config.mac_capabilities |= MAC_1000FD;
> +		break;

[Severity: Low]
This isn't a functional bug, but should RTL_SFP_8168_AF be excluded from
rtl8169_get_lpi_caps() as well?

rtl8169_get_lpi_caps() only gates on rtl_supports_eee() and
tp->sfp_mode == RTL_SFP_8127_ATF:

	if (!rtl_supports_eee(tp) || tp->sfp_mode == RTL_SFP_8127_ATF)
		return 0;

	caps |= MAC_100FD | MAC_1000FD;

RTL_GIGA_MAC_VER_52 passes rtl_supports_eee(), so lpi_capabilities ends up
as MAC_100FD | MAC_1000FD and PHY_INTERFACE_MODE_1000BASEX is added to
lpi_interfaces further down, even though mac_capabilities for this mode is
only MAC_1000FD and there is no phydev.  Nothing can act on it, since
phylink_activate_lpi() is only reached when pl->phy_enable_tx_lpi is set
from a connected PHY and phylink_ethtool_get_eee()/set_eee() return
-EOPNOTSUPP without pl->phydev, but the advertised mask still lists a speed
this interface cannot run at.

> +	case RTL_SFP_8127_ATF:
>  		phy_mode = PHY_INTERFACE_MODE_INTERNAL;
>  		tp->phylink_config.mac_capabilities |= MAC_10000FD;
> -	} else {
> +		break;
> +	default:
>  		tp->phylink_config.mac_capabilities |= MAC_10 | MAC_100;
>  
>  		if (tp->mac_version == RTL_GIGA_MAC_VER_80)

[ ... ]

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

Re: [PATCH net-next v9 5/7] r8169: add support for RTL8127atf

[netdev-bot+sashiko]

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

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

- [High] The 1 Gb/s support the commit claims to add is dead code.
- [High] RTL8127atf loses its SerDes/PCS re-initialization across
  suspend/resume, so the fiber link cannot come back up.
- [Medium] The commit message ("RTL8127atf is also a fiber mode card, but
  its sds reg base addr is 0x0080 ... Add 10g and 1g support") does not…
- [Medium] In the newly added r8127_sfp_init_1g() the return value of
  r8168_phy_ocp_read(tp, 0xc804) is stored in `int val` and immediately…
- [Medium] r8127_sds_modify() silently swallows both failure modes of the
  new 0x2348 SDS command interface: it returns void, aborts without any…
- [Medium] The phylink resolve worker is never quiesced in the
  suspend/runtime-suspend/shutdown paths, yet this patch makes those…
- [Medium] rtl8169_pcs_config() unconditionally performs a full SerDes
  PHY hardware reset (r8127_sfp_sds_phy_reset(): 0x2350 bit0…
- [Low] Concern (validated as a FALSE POSITIVE): "RTL8127ATF is
  registered as an in-band 1000BASE-X PCS but its PCS callbacks do not…

--

r8169: add support for RTL8127atf

The patch wires RTL8127atf into phylink as a 10GBASE-R/1000BASE-X PCS,
adds a new SerDes command window (0x2348..0x234e) with read/write/modify
helpers, and adds a 1G SerDes init sequence. It also removes the dummy
SFP PHY driver entry from the realtek phylib driver and deletes
include/net/phy/realtek_phy.h.

The commit message says:

  "RTL8127atf is also a fiber mode card, but its sds reg base addr is
   0x0080, which is different from RTL8116af. Add 10g and 1g support for
   RTL8127atf in this patch."

[Severity: Medium]
Could the message describe the rest of what the patch does?  Beyond the
SerDes base address and the 10g/1g init sequences it also:

 - removes the "Realtek SFP PHY Mode" phy_driver entry together with
   rtlgen_sfp_get_features(), rtlgen_sfp_read_status() and
   rtlgen_sfp_config_aneg() from drivers/net/phy/realtek/realtek_main.c

 - deletes the global header include/net/phy/realtek_phy.h and the dummy
   MII_PHYSID2 hack in r8168_phy_ocp_read()

 - stops registering an MDIO bus and a phydev for every SFP mode, by
   changing rtl_init_one() from "if (tp->sfp_mode != RTL_SFP_8168_AF)" to
   "if (tp->sfp_mode == RTL_SFP_NONE)"

 - drops the RTL8127atf specific forced-speed path in
   rtl8169_set_link_ksettings()

 - introduces a second SerDes access mechanism (the MMIO window
   R8127_SDS_CMD/ADDR/DATA_IN/DATA_OUT with index/page/reg encoding),
   which is unrelated to the "base addr 0x0080" explanation

The phylib change in particular gives the phylib maintainers no notice
that their subsystem is touched.  Would it make sense to split the
phylib removal into its own patch, or at least mention it?

> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> index a04acb42e995d..eecde26d9de2c 100644
> --- a/drivers/net/ethernet/realtek/r8169_main.c
> +++ b/drivers/net/ethernet/realtek/r8169_main.c

[ ... ]

> @@ -1223,6 +1224,75 @@ static void r8127_sfp_sds_phy_reset(struct rtl8169_private *tp)
>  	usleep_range(10, 20);
>  }
>  
> +DECLARE_RTL_COND(r8127_sds_cmd_cond)
> +{
> +	return RTL_R16(tp, R8127_SDS_CMD) & R8127_SDS_CMD_EXEC;
> +}
> +
> +static int r8127_sds_read(struct rtl8169_private *tp, u16 index,
> +			  u16 page, u16 reg)
> +{
> +	u16 addr = (index << 11) | (page << 5) | reg;
> +
> +	RTL_W16(tp, R8127_SDS_ADDR, addr);
> +	RTL_W16(tp, R8127_SDS_CMD, R8127_SDS_CMD_EXEC);
> +
> +	if (rtl_loop_wait_low(tp, &r8127_sds_cmd_cond, 10, 100))
> +		return RTL_R16(tp, R8127_SDS_DATA_OUT);
> +
> +	return -ETIMEDOUT;
> +}
> +
> +static int r8127_sds_write(struct rtl8169_private *tp, u16 index, u16 page,
> +			   u16 reg, u16 val)
> +{
> +	u16 addr = (index << 11) | (page << 5) | reg;
> +
> +	RTL_W16(tp, R8127_SDS_DATA_IN, val);
> +	RTL_W16(tp, R8127_SDS_ADDR, addr);
> +	RTL_W16(tp, R8127_SDS_CMD, R8127_SDS_CMD_EXEC | R8127_SDS_CMD_WE);
> +
> +	if (rtl_loop_wait_low(tp, &r8127_sds_cmd_cond, 10, 100))
> +		return 0;
> +
> +	return -ETIMEDOUT;
> +}
> +
> +static void r8127_sds_modify(struct rtl8169_private *tp, u16 index, u16 page,
> +			     u16 reg, u16 clearmask, u16 setmask)
> +{
> +	int val = r8127_sds_read(tp, index, page, reg);
> +
> +	if (val < 0)
> +		return;
> +
> +	val = (val & ~clearmask) | setmask;
> +	r8127_sds_write(tp, index, page, reg, val);
> +}

[Severity: Medium]
The -ETIMEDOUT returns were added to r8127_sds_read()/r8127_sds_write()
on purpose, but r8127_sds_modify() is the only consumer and it drops
both: the read error returns silently and the write return value is
discarded.

Should the failure be propagated up instead?  As written, if one of the
three r8127_sds_modify() calls in r8127_sfp_init_1g() never reaches the
hardware, the following mode select writes still execute:

	r8127_sds_modify(tp, 0, 1, 31, 0, BIT(3));
	r8127_sds_modify(tp, 0, 2,  0, ...);
	r8127_sds_modify(tp, 0, 0,  4, 0, BIT(2));

	RTL_W16(tp, 0x233a, 0x8004);
	RTL_W16(tp, 0x233e, (RTL_R16(tp, 0x233e) & ~0x3003) | 0x0002);

so the SerDes lane keeps its post-reset settings while the mode select
says 1G.  r8127_sfp_init_1g() is void and rtl8169_pcs_config() returns 0
unconditionally, so phylink records the PCS as configured and will report
SPEED_1000/DUPLEX_FULL once MDIO_STAT1 reads link up.  rtl8169_pcs_config()
already returns -EOPNOTSUPP for an unsupported interface, so a real error
could be reported here.

> +
> +static void r8127_sfp_init_1g(struct rtl8169_private *tp)
> +{
> +	int val;
> +
> +	r8127_sfp_sds_phy_reset(tp);
> +
> +	r8127_sds_modify(tp, 0, 1, 31, 0, BIT(3));
> +	r8127_sds_modify(tp, 0, 2,  0, BIT(13) | BIT(12) | BIT(6),
> +			 BIT(12) | BIT(6));
> +	r8127_sds_modify(tp, 0, 0,  4, 0, BIT(2));
> +
> +	RTL_W16(tp, 0x233a, 0x8004);
> +	RTL_W16(tp, 0x233e, (RTL_R16(tp, 0x233e) & ~0x3003) | 0x0002);
> +
> +	r8168_phy_ocp_write(tp, 0xc40a, 0x0000);
> +	r8168_phy_ocp_write(tp, 0xc466, 0x0000);
> +	r8168_phy_ocp_write(tp, 0xc808, 0x0000);
> +	r8168_phy_ocp_write(tp, 0xc80a, 0x0000);
> +
> +	val = r8168_phy_ocp_read(tp, 0xc804);
> +	r8168_phy_ocp_write(tp, 0xc804, (val & ~0x000f) | 0x000c);
> +}

[Severity: Medium]
Can the unchecked r8168_phy_ocp_read() result corrupt the following
write?  r8168_phy_ocp_read() can return a negative value:

	return rtl_loop_wait_high(tp, &rtl_ocp_gphy_cond, 25, 10) ?
		(RTL_R32(tp, GPHY_OCP) & 0xffff) : -ETIMEDOUT;

With val == -ETIMEDOUT, (val & ~0x000f) | 0x000c is 0xffffff9c, and
r8168_phy_ocp_write() ORs data into the command word without masking it:

	RTL_W32(tp, GPHY_OCP, OCPAR_FLAG | (reg << 15) | data);

rtl_ocp_reg_failure() only validates reg, so every bit of the encoded
address field ends up set and the write lands somewhere other than
0xc804.  Would checking val < 0 and aborting the init be better here?
Note r8127_sfp_init_10g() has the same shape today; this adds a second
instance of it.

[ ... ]

> @@ -2608,9 +2650,6 @@ static void rtl8169_init_phy(struct rtl8169_private *tp)
>  	    tp->pci_dev->subsystem_device == 0xe000)
>  		phy_write_paged(tp->phydev, 0x0001, 0x10, 0xf01b);
>  
> -	if (tp->sfp_mode == RTL_SFP_8127_ATF)
> -		rtl_sfp_init(tp);
> -
>  	/* We may have called phy_speed_down before */
>  	phy_speed_up(tp->phydev);

[Severity: High]
Does RTL8127atf still come back after suspend/resume with this call
removed?

rtl8169_down() puts the SerDes PHY into reset and programs mode select
0x233a = 0x801f for this chip:

	if (tp->sfp_mode == RTL_SFP_8127_ATF)
		rtl_sfp_reset(tp);

and it is reached from rtl8169_close(), rtl8169_suspend(),
rtl8169_runtime_suspend() and rtl_shutdown() via rtl8169_net_suspend().
A runtime suspend also drops the device to D3hot.

Previously the re-programming happened on every bring-up through
rtl8169_up() -> rtl8169_init_phy() -> rtl_sfp_init().  After this patch
rtl8169_up() only does that under "if (tp->phydev)", and tp->phydev is
NULL for RTL8127atf because rtl_init_one() now registers the MDIO bus
only for "tp->sfp_mode == RTL_SFP_NONE".

The only remaining caller of r8127_sfp_init_10g()/r8127_sfp_init_1g() is
rtl8169_pcs_config(), which phylink invokes from a major config, i.e.
from phylink_start() in rtl_open() or on an interface change.  r8169
never calls phylink_suspend()/phylink_resume().  So after a resume the
SerDes appears to stay in the 0x801f reset mode while phylink believes
10GBASE-R is configured, and the link stays down until the interface is
taken down and up again.  Note rtl8169_runtime_idle() schedules a runtime
suspend 10s after the carrier drops, which is the normal state of a fiber
port with no module or no link.

[ ... ]

> @@ -4942,7 +4981,7 @@ static irqreturn_t rtl8169_interrupt(int irq, void *dev_instance)
>  	if (status & LinkChg) {
>  		if (tp->phydev)
>  			phy_mac_interrupt(tp->phydev);
> -		else if (tp->sfp_mode == RTL_SFP_8168_AF)
> +		else if (tp->sfp_mode)
>  			phylink_mac_change(tp->phylink,
>  					   !!(RTL_R8(tp, PHYstatus) & LinkStatus));
>  	}

[Severity: Medium]
Can the phylink resolve worker now run against a device that is being
powered down?

This call queues the phylink resolve work from hardirq for RTL8127atf,
including for the LinkChg caused by rtl_sfp_reset() itself.  Meanwhile
rtl8169_suspend(), rtl8169_runtime_suspend() and rtl_shutdown() take only
rtnl_lock() and go through rtl8169_net_suspend() -> rtl8169_down() ->
rtl_sfp_reset() -> rtl8169_cleanup(); a grep of phylink_ in this file
shows phylink_stop() only in rtl8169_close() and phylink_start() only in
rtl_open(), so nothing flushes pl->resolve on the PM paths.

The resolve worker holds only pl->state_mutex, and the suspend path holds
only RTNL, so the two lock sets are disjoint:

	rtl8169_down()				phylink resolve worker
	  rtl_sfp_reset()			  rtl8169_pcs_get_state()
	    r8127_sfp_sds_phy_reset()		    rtl8169_sds_read()
	  rtl_prepare_power_down()		  rtl_mac_link_up()/down()

Once the device is in D3, MMIO reads return all ones, so MDIO_STAT1 looks
like LSTATUS is set and phylink reports a 10 Gb/s link on a powered-down
device.  Should the PM paths call phylink_suspend()/phylink_resume(), or
at least phylink_stop()?

[ ... ]

> @@ -5725,6 +5785,24 @@ static int rtl8169_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
>  			      const unsigned long *advertising,
>  			      bool permit_pause_to_mac)
>  {
> +	struct rtl8169_private *tp = container_of(pcs, struct rtl8169_private,
> +						  pcs);
> +
> +	if (tp->sfp_mode == RTL_SFP_8127_ATF) {
> +		switch (interface) {
> +		case PHY_INTERFACE_MODE_10GBASER:
> +			r8127_sfp_init_10g(tp);
> +			break;
> +		case PHY_INTERFACE_MODE_1000BASEX:
> +			r8127_sfp_init_1g(tp);
> +			break;
> +		default:
> +			netdev_err(tp->dev, "Unsupported SFP interface mode: %s\n",
> +				   phy_modes(interface));
> +			return -EOPNOTSUPP;
> +		}
> +	}
> +
>  	return 0;
>  }

[Severity: Medium]
Does this bounce the fiber link on an advertisement-only change?  Both
init helpers start with r8127_sfp_sds_phy_reset(), which toggles 0x2350
bit 0 and rewrites 0x233a:

	RTL_W8(tp, 0x2350, RTL_R8(tp, 0x2350) & ~BIT(0));
	udelay(1);
	RTL_W16(tp, 0x233a, 0x801f);
	RTL_W8(tp, 0x2350, RTL_R8(tp, 0x2350) | BIT(0));

phylink calls pcs_config() from phylink_change_inband_advert() as well as
from a major config, and phylink_update_pause_state() does that
unconditionally when there is no PHY attached:

	if (!pl->phydev)
		phylink_change_inband_advert(pl);

tp->phydev is NULL for RTL8127atf after this patch, so an
"ethtool -A <dev> rx off tx off" would reach rtl8169_pcs_config() with an
unchanged interface and hard-reset the SerDes.  Should the helpers only
run when the interface actually changes?

[ ... ]

> @@ -5818,8 +5896,14 @@ static int rtl_init_phylink(struct rtl8169_private *tp)
>  		tp->phylink_config.mac_capabilities |= MAC_1000FD;
>  		break;
>  	case RTL_SFP_8127_ATF:
> -		phy_mode = PHY_INTERFACE_MODE_INTERNAL;
> -		tp->phylink_config.mac_capabilities |= MAC_10000FD;
> +		tp->pcs.ops = &r8169_pcs_ops;
> +		phy_mode = PHY_INTERFACE_MODE_10GBASER;
> +		tp->phylink_config.default_an_inband = true;
> +		tp->phylink_config.mac_capabilities |= MAC_1000FD | MAC_10000FD;
> +		__set_bit(PHY_INTERFACE_MODE_10GBASER,
> +			  tp->phylink_config.supported_interfaces);
> +		__set_bit(PHY_INTERFACE_MODE_1000BASEX,
> +			  tp->phylink_config.supported_interfaces);
>  		break;

[Severity: High]
How does the 1 Gb/s support announced in the commit message get selected?

phy_mode is fixed at PHY_INTERFACE_MODE_10GBASER here, so
phylink_create() -> phylink_parse_mode() derives pl->supported from
phy_caps_from_interface(10GBASER), i.e. 10 Gb/s link modes only, and
pl->link_config.interface stays 10GBASER.

There is no PHY (this patch stops registering an MDIO bus for SFP modes)
and no SFP bus (phylink_register_sfp() finds no "sfp" fwnode on a PCI
device), so the only PHY-less code that can re-select an interface never
runs:

drivers/net/phy/phylink.c:phylink_ethtool_ksettings_set() {
	...
	if (pl->sfp_bus) {
		if (kset->base.autoneg == AUTONEG_ENABLE)
			config.interface = phylink_sfp_select_interface(...);
		else
			config.interface = phylink_sfp_select_interface_speed(...);
	...
}

With link_config.interface pinned to 10GBASER, is any of the following
reachable?

 - the PHY_INTERFACE_MODE_1000BASEX case of rtl8169_pcs_config(), and
   therefore r8127_sfp_init_1g() in its entirety
 - the "state->interface == PHY_INTERFACE_MODE_1000BASEX" branch of
   rtl8169_pcs_get_state(), which otherwise always reports SPEED_10000
 - MAC_1000FD and the 1000BASEX supported_interfaces bit

"ethtool -s <dev> autoneg off speed 1000 duplex full" also looks like it
returns -EINVAL from phy_caps_lookup() because pl->supported only holds
10 Gb/s modes.  The patch removes the previous mechanism for forcing a
mode on this part (the RTL_SFP_8127_ATF case in
rtl8169_set_link_ksettings() calling rtl_sfp_init()) without adding a
replacement.

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

Re: [PATCH net-next v9 6/7] r8169: add ltr support for RTL8117 series

[netdev-bot+sashiko]

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

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

- [Medium] Undisclosed scope creep in rtl_enable_ltr(): the newly added
  `r8168_mac_ocp_modify(tp, SEND_LTR_MSG, 0x0000, LTR_MSG_EN)` (MAC-OCP…
- [Medium] The patch splits RTL_GIGA_MAC_VER_52 out of the VER_46..48
  branch of rtl_enable_ltr() into its own case (r8169_main.c:3199-3214)…

--

r8169: add ltr support for RTL8117 series

This adds a dedicated LTR programming sequence for RTL_GIGA_MAC_VER_52 in
rtl_enable_ltr(), adds VER_52 to the L1.2 enable/disable version lists in
rtl_hw_aspm_clkreq_enable(), and sets a new MAC OCP bit (0xe038 BIT(0)) at
the end of rtl_enable_ltr().

> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> index eecde26d9de2c..5dbbc3a48c09a 100644
> --- a/drivers/net/ethernet/realtek/r8169_main.c
> +++ b/drivers/net/ethernet/realtek/r8169_main.c
> @@ -353,11 +353,13 @@ enum rtl_registers {
>  	ALDPS_LTR	= 0xe0a2,
>  	LTR_OBFF_LOCK	= 0xe032,
>  	LTR_SNOOP	= 0xe034,
> +	SEND_LTR_MSG	= 0xe038,
>  
>  #define ALDPS_LTR_EN			BIT(0)
>  #define LTR_OBFF_LOCK_EN		BIT(0)
>  #define LINK_SPEED_CHANGE_EN		BIT(14)
>  #define LTR_SNOOP_EN			GENMASK(15, 14)
> +#define LTR_MSG_EN			BIT(0)
>  };
>  
>  enum rtl8168_8101_registers {

[ ... ]

> @@ -3194,8 +3196,23 @@ static void rtl_enable_ltr(struct rtl8169_private *tp)
>  		r8168_mac_ocp_write(tp, 0xcdf2, 0x9003);
>  		r8168_mac_ocp_modify(tp, LTR_OBFF_LOCK, 0x0000, LINK_SPEED_CHANGE_EN);
>  		break;
> -	case RTL_GIGA_MAC_VER_46 ... RTL_GIGA_MAC_VER_48:
>  	case RTL_GIGA_MAC_VER_52:
> +		r8168_mac_ocp_write(tp, 0xcdd0, 0x9003);
> +		r8168_mac_ocp_modify(tp, LTR_SNOOP, 0x0000, LTR_SNOOP_EN);
> +		r8168_mac_ocp_write(tp, 0xe02c, 0x1880);
> +		r8168_mac_ocp_write(tp, 0xe02e, 0x4880);
> +		r8168_mac_ocp_modify(tp, ALDPS_LTR, 0x0000, ALDPS_LTR_EN);
> +		r8168_mac_ocp_write(tp, 0xcdd8, 0x9003);
> +		r8168_mac_ocp_write(tp, 0xcdda, 0x9003);
> +		r8168_mac_ocp_write(tp, 0xcddc, 0x9003);
> +		r8168_mac_ocp_write(tp, 0xcdd2, 0x883c);
> +		r8168_mac_ocp_write(tp, 0xcdd4, 0x8c12);
> +		r8168_mac_ocp_write(tp, 0xcdd6, 0x9003);
> +		r8168_mac_ocp_write(tp, 0xe0a6, 0x9003);
> +		r8168_mac_ocp_write(tp, 0xe0a8, 0x9003);
> +		r8168_mac_ocp_modify(tp, LTR_OBFF_LOCK, 0x0000, LINK_SPEED_CHANGE_EN);
> +		break;
> +	case RTL_GIGA_MAC_VER_46 ... RTL_GIGA_MAC_VER_48:
>  		r8168_mac_ocp_modify(tp, ALDPS_LTR, 0x0000, ALDPS_LTR_EN);
>  		RTL_W8(tp, COMBO_LTR_EXTEND, RTL_R8(tp, COMBO_LTR_EXTEND) | COMBO_LTR_EXTEND_EN);
>  		fallthrough;

[Severity: Medium]
Splitting RTL_GIGA_MAC_VER_52 out of the VER_46..48 arm drops this write for
every VER_52 device:

	RTL_W8(tp, COMBO_LTR_EXTEND, RTL_R8(tp, COMBO_LTR_EXTEND) | COMBO_LTR_EXTEND_EN);

The changelog justifies the removal with "this bit is not required for
RTL8116af", but is RTL_GIGA_MAC_VER_52 equivalent to RTL8116af?

rtl_chip_infos maps both XIDs to the same mac_version:

	{ 0x7cf, 0x54b,	RTL_GIGA_MAC_VER_52, "RTL8168fp/RTL8117" },
	{ 0x7cf, 0x54a,	RTL_GIGA_MAC_VER_52, "RTL8168fp/RTL8117",
	  FIRMWARE_8168FP_3 },

and the af variant is only distinguished at runtime:

	static bool rtl_is_8116af(struct rtl8169_private *tp)
	{
		return tp->mac_version == RTL_GIGA_MAC_VER_52 &&
			(r8168_mac_ocp_read(tp, RTL_PKG_DETECT) & RTL_PKG_DETECT_MASK) ==
			RTL_PKG_DETECT_8116AF && ...

So plain RTL8117/RTL8168fp cards that have had the COMBO_LTR_EXTEND_EN write
since LTR support was added now lose it. Should the removal be gated with
rtl_is_8116af(), or should the changelog state that the bit is not needed on
any RTL8117 and explain the behaviour change for existing hardware?

The following patch in the series, "r8169: fix RTL8116af can not enter
s0idle and c10", does not touch rtl_enable_ltr(), so this stands at the end
of the series.

> @@ -3215,6 +3232,7 @@ static void rtl_enable_ltr(struct rtl8169_private *tp)
>  	}
>  	/* chip can trigger LTR */
>  	r8168_mac_ocp_modify(tp, LTR_OBFF_LOCK, 0x0003, LTR_OBFF_LOCK_EN);
> +	r8168_mac_ocp_modify(tp, SEND_LTR_MSG, 0x0000, LTR_MSG_EN);
>  }

[Severity: Medium]
Was this write meant to be limited to the new RTL_GIGA_MAC_VER_52 arm?

It sits in the shared epilogue after the switch in rtl_enable_ltr(), and
every non-default case ends in break (or falls through into VER_51), so it is
also executed for VER_46..48, VER_51, VER_61..66, VER_70 and VER_80 by way of
rtl_hw_aspm_clkreq_enable()->rtl_enable_ltr() on any chip >= VER_32 with
tp->aspm_manageable.

SEND_LTR_MSG (0xe038) and LTR_MSG_EN are introduced here and appear nowhere
else under drivers/net/ethernet/realtek/, so there is no in-tree precedent
for this bit on the 8168h/8168ep or 8125/8126/8127 generations, whose LTR
value tables in the same function are entirely different.

VER_51 in particular now gets LTR message generation enabled while it is
absent from both mac_version lists in rtl_hw_aspm_clkreq_enable(), so it
never receives the matching "chip can trigger L1.2" programming.

The rest of the patch is carefully version gated (a dedicated
case RTL_GIGA_MAC_VER_52 is split out, and VER_52 is added to both lists in
rtl_hw_aspm_clkreq_enable()). Could this write be moved into the
case RTL_GIGA_MAC_VER_52 block, or gated by an explicit version list, or the
changelog extended to cover the wider hardware change it makes?

> @@ -3248,6 +3266,7 @@ static void rtl_hw_aspm_clkreq_enable(struct rtl8169_private *tp, bool enable)
>  		rtl_enable_ltr(tp);
>  		switch (tp->mac_version) {
>  		case RTL_GIGA_MAC_VER_46 ... RTL_GIGA_MAC_VER_48:
> +		case RTL_GIGA_MAC_VER_52:
>  		case RTL_GIGA_MAC_VER_61 ... RTL_GIGA_MAC_VER_LAST:
>  			/* reset ephy tx/rx disable timer */
>  			r8168_mac_ocp_modify(tp, 0xe094, 0xff00, 0);
> @@ -3260,6 +3279,7 @@ static void rtl_hw_aspm_clkreq_enable(struct rtl8169_private *tp, bool enable)
>  	} else {
>  		switch (tp->mac_version) {
>  		case RTL_GIGA_MAC_VER_46 ... RTL_GIGA_MAC_VER_48:
> +		case RTL_GIGA_MAC_VER_52:
>  		case RTL_GIGA_MAC_VER_61 ... RTL_GIGA_MAC_VER_LAST:
>  			r8168_mac_ocp_modify(tp, 0xe092, 0x00ff, 0);
>  			break;

Related to the SEND_LTR_MSG question above: this disable path clears only
0xe092 and never LTR_MSG_EN, so once ASPM has been enabled once the new bit
stays set on all affected chips. Is that intended?

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

Re: [PATCH net-next v9 7/7] r8169: fix RTL8116af can not enter s0idle and c10

[netdev-bot+sashiko]

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

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

- [High] The hidden-function power/ASPM programming is never re-applied
  on the runtime-PM resume path, so the bug class the patch claims to…
- [Medium] rtl_lowpower_hidden_functions() unconditionally ORs
  PCI_EXP_LNKCTL_ASPMC (both L0s and L1) and PCI_EXP_LNKCTL_CLKREQ_EN…
- [Medium] rtl_lowpower_hidden_functions() writes PCI_EXP_LNKCTL (force
  ASPM L0s/L1 + CLKREQ_EN) and PCI_PM_CTRL (D3hot + RW1C PME_STATUS) to…
- [Low] Commit message names the wrong config-space accessors.

--

r8169: fix RTL8116af can not enter s0idle and c10

This adds rtl_lowpower_hidden_functions(), which walks functions 2 to 7 of
the NIC's slot and programs their PCIe Link Control and PM Control
registers to enable ASPM and put them into D3hot.  It is called from
rtl_init_one() and rtl8169_resume() when rtl_is_8116af() is true.

A few questions below.

>     Put functions 2 to 7 into D3hot and enable ASPM on their PCIe link control
>     register. Since these functions are hidden, access their configuration
>     space through pci_bus_read_config_dword() / pci_bus_write_config_dword()
>     using the same slot and the target function numbers.

[Severity: Low]
This isn't a bug, but the changelog names accessors that the patch never
calls.  All four config accesses in rtl_lowpower_hidden_functions() are
16-bit:

	pci_bus_read_config_word(bus, devfn, pos + PCI_EXP_LNKCTL, &val);
	pci_bus_write_config_word(bus, devfn, pos + PCI_EXP_LNKCTL, val);
	pci_bus_read_config_word(bus, devfn, pos + PCI_PM_CTRL, &val);
	pci_bus_write_config_word(bus, devfn, pos + PCI_PM_CTRL, val);

Could the description say pci_bus_read_config_word() /
pci_bus_write_config_word() instead?  It may also be worth mentioning that
whether a function is touched at all is decided by
pci_bus_find_capability(), i.e. by walking the capability list of an
un-enumerated function.

> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> index 5dbbc3a48c09a..5415ff62a2869 100644
> --- a/drivers/net/ethernet/realtek/r8169_main.c
> +++ b/drivers/net/ethernet/realtek/r8169_main.c

[ ... ]

> @@ -3773,6 +3774,41 @@ static void rtl_hw_start_8168ep_3(struct rtl8169_private *tp)
>  	r8168_mac_ocp_modify(tp, 0xe860, 0x0000, 0x0080);
>  }
>  
> +static void rtl_lowpower_hidden_functions(struct pci_dev *pdev)
> +{
> +	unsigned int slot = PCI_SLOT(pdev->devfn);
> +	struct pci_bus *bus = pdev->bus;
> +	int func, pos;
> +	u16 val;
> +
> +	for (func = 2; func < 8; func++) {
> +		unsigned int devfn = PCI_DEVFN(slot, func);
> +
> +		pos = pci_bus_find_capability(bus, devfn, PCI_CAP_ID_EXP);
> +		if (pos) {
> +			pci_bus_read_config_word(bus, devfn, pos + PCI_EXP_LNKCTL, &val);
> +
> +			if (PCI_POSSIBLE_ERROR(val))
> +				continue;
> +
> +			val |= (PCI_EXP_LNKCTL_ASPMC | PCI_EXP_LNKCTL_CLKREQ_EN);
> +			pci_bus_write_config_word(bus, devfn, pos + PCI_EXP_LNKCTL, val);
> +		}

[Severity: Medium]
Should this check PCI_EXP_LNKCAP before setting these bits?  Both ASPM
states plus Clock Power Management are enabled unconditionally, without
reading LNKCAP to see which of L0s/L1 and PCI_EXP_LNKCAP_CLKPM are
advertised.  The ASPM core encodes that rule explicitly:

drivers/pci/pcie/aspm.c:pcie_set_clkpm() {
	if (!link->clkpm_capable || link->clkpm_disable)
		enable = 0;
}

There is also a policy conflict on the same physical link.  rtl_init_one()
does:

	if (rtl_aspm_is_safe(tp)) {
		dev_info(&pdev->dev, "System vendor flags ASPM as safe\n");
		rc = 0;
	} else {
		rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L1);
	}
	tp->aspm_manageable = !rc;

and rtl8169_tx_timeout() can later call
pci_disable_link_state(tp->pci_dev, PCIE_LINK_STATE_L1 |
PCIE_LINK_STATE_L0S).  Neither of those can reach functions 2 to 7, since
pcie_config_aspm_link() and pcie_set_clkpm_nocheck() only walk
linkbus->devices, which contains enumerated pci_devs only.

So can functions 2 to 7 end up with L0s, L1 and CLKREQ# enabled on a link
where the administrator or a blacklist (pcie_aspm=off, sysfs link_state) or
the driver's own workaround asked for ASPM to be off?  aspm.c also notes
that spec sec 7.5.3.7 recommends the same ASPM Control value for all
functions of a multi-function device, which this diverges from.

> +
> +		pos = pci_bus_find_capability(bus, devfn, PCI_CAP_ID_PM);
> +		if (pos) {
> +			pci_bus_read_config_word(bus, devfn, pos + PCI_PM_CTRL, &val);
> +
> +			if (PCI_POSSIBLE_ERROR(val))
> +				continue;
> +
> +			val &= ~PCI_PM_CTRL_STATE_MASK;
> +			val |= (RTL_PM_CTRL_D3HOT | PCI_PM_CTRL_PME_STATUS);
> +			pci_bus_write_config_word(bus, devfn, pos + PCI_PM_CTRL, val);
> +		}
> +	}
> +}

[Severity: Medium]
What guarantees the devfn being written here is really one of the hidden
functions?  The loop derives devfn purely from PCI_SLOT(pdev->devfn), and
never reads PCI_VENDOR_ID, never looks for an existing struct pci_dev (for
example via pci_get_slot()), and never excludes PCI_FUNC(pdev->devfn).

The two guards present do not separate "hidden" from "present and owned by
another driver".  A non-zero pos only says the function answers config
reads, and PCI_POSSIBLE_ERROR(val) looks unreachable, because a
non-responding function already fails the capability walk with hdr_type
0xff:

drivers/pci/pci.c:__pci_bus_find_cap_start() {
	switch (hdr_type) {
	case PCI_HEADER_TYPE_NORMAL:
	case PCI_HEADER_TYPE_BRIDGE:
		return PCI_CAPABILITY_LIST;
	...
	return 0;
}

If some devfn in 2..7 of that slot is an enumerated device bound to another
driver (composed multi-function slots in a guest, for instance), does this
put it into D3hot while the PCI core still believes it is in PCI_D0, so
pci_set_power_state() will not repair the state?  And does the
unconditional PCI_PM_CTRL_PME_STATUS write discard a pending wake event of
a function nobody is tracking?

Related question: if the NIC itself were ever at function 2..7, the call
from rtl8169_resume() would put it into D3hot right before
rtl8169_runtime_resume() touches its registers.  Is that ruled out
somewhere?  The rtl_is_8116af() gate only checks the MAC version and the
MAC-OCP package/hardware IDs, not which PCI function the NIC occupies.

[ ... ]

> @@ -5326,6 +5362,9 @@ static int rtl8169_resume(struct device *device)
>  	/* Some chip versions may truncate packets without this initialization */
>  	rtl_init_rxcfg(tp);
>  
> +	if (rtl_is_8116af(tp))
> +		rtl_lowpower_hidden_functions(tp->pci_dev);
> +
>  	return rtl8169_runtime_resume(device);
>  }

[Severity: High]
Does the runtime-PM resume path need this too?  The only two call sites are
rtl_init_one() and rtl8169_resume(), but rtl8169_runtime_resume() is
registered as a peer callback:

static const struct dev_pm_ops rtl8169_pm_ops = {
	SYSTEM_SLEEP_PM_OPS(rtl8169_suspend, rtl8169_resume)
	RUNTIME_PM_OPS(rtl8169_runtime_suspend, rtl8169_runtime_resume,
		       rtl8169_runtime_idle)
};

and it does not re-apply the hidden-function programming:

static int rtl8169_runtime_resume(struct device *dev)
{
	struct rtl8169_private *tp = dev_get_drvdata(dev);

	rtl_rar_set(tp, tp->dev->dev_addr);
	__rtl8169_set_wol(tp, tp->saved_wolopts);

	if (tp->TxDescArray)
		rtl8169_up(tp);

	netif_device_attach(tp->dev);

	return 0;
}

Runtime suspend is a normal event for this driver: rtl_init_one() arms it
with

	if (pci_dev_run_wake(pdev))
		pm_runtime_put_sync(&pdev->dev);

and rtl_mac_link_down() -> pm_runtime_idle() plus rtl8169_runtime_idle()
schedule it on link down.  If the PCI core places the endpoint in D3cold
(parent bridge powered down), the hidden functions' PMCSR and Link Control
registers return to power-on defaults, D0 with ASPM disabled, and the core
only restores saved config space for enumerated pci_devs.  After that, is
there any path that re-programs functions 2..7?

The same gap seems to apply when the device stays runtime-suspended across
a system sleep (PCI direct_complete), where the core skips
rtl8169_resume() entirely and the device comes back through the runtime
callback.

Since rtl8169_resume() already tail-calls rtl8169_runtime_resume(), would
moving the rtl_is_8116af() call into rtl8169_runtime_resume() cover both
wake paths from a single site?

[ ... ]

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