Re: [PATCH net-next v2 3/6] r8169: add support for phylink
From: Jakub Kicinski <kuba@kernel.org>
Date: 2026-06-13 22:06:25
Also in:
lkml
This is an AI-generated review of your patch. The human sending this email has considered the AI review valid, or at least plausible. --- r8169: add support for phylink This commit transfers the r8169 driver from the phylib framework to phylink, in order to support fiber-mode cards whose link status/speed cannot be read from standard PHY registers.
quoted hunk ↗ jump to hunk
diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c index 560f987437b6..615bd4107359 100644 --- a/drivers/net/ethernet/realtek/r8169_main.c +++ b/drivers/net/ethernet/realtek/r8169_main.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -2661,13 +2672,10 @@ static void rtl_jumbo_config(struct rtl8169_private *tp) 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); - } + if (jumbo) + tp->phylink_config.mac_capabilities &= ~(MAC_SYM_PAUSE | MAC_ASYM_PAUSE); + else + tp->phylink_config.mac_capabilities |= (MAC_SYM_PAUSE | MAC_ASYM_PAUSE); }
[High] Does this still enforce the "Chip doesn't support pause in jumbo mode" constraint that the comment describes? Looking at phylink_create() in drivers/net/phy/phylink.c, pl->supported is initialized once via linkmode_fill() + phylink_validate() and is not re-derived from config->mac_capabilities afterwards. Pause negotiation is driven by pl->link_config.pause and the PHY's advertising bits, neither of which is updated when rtl_jumbo_config() toggles mac_capabilities. In rtl8169_change_mtu() the phylink_stop()/phylink_start() cycle re-runs phylink_mac_initial_config() against the cached pl->link_config, so Pause/Asym_Pause remain advertised after MTU is raised above ETH_DATA_LEN. Compared to the old code which actively cleared Pause/Asym_Pause from tp->phydev->advertising and called phy_start_aneg() to force renegotiation, should this branch instead be updating something that is actually consumed on the next link-up, so that PAUSE is no longer advertised in jumbo mode? [ ... ]
quoted hunk ↗ jump to hunk
@@ -5010,7 +4995,7 @@ static void rtl8169_down(struct rtl8169_private *tp) /* Clear all task flags */ bitmap_zero(tp->wk.flags, RTL_FLAG_MAX); - phy_stop(tp->phydev); + phylink_stop(tp->phylink);
[Medium] Is the phylink conversion complete here? After this commit the PHY is owned by phylink (connected via phylink_connect_phy() / phylink_disconnect_phy()), but net_device_ops.ndo_eth_ioctl still points at phy_do_ioctl_running, so SIOCGMIIREG / SIOCSMIIREG / SIOCGMIIPHY continue to go directly to the PHY and bypass phylink. phylink exports phylink_mii_ioctl() for exactly this case. Mixing direct phydev ioctl access with phylink-managed link state can let phylink's cached state (pl->link_config, pl->phy_state) drift out of sync with the actual PHY register state. Should ndo_eth_ioctl be switched to phylink_mii_ioctl() as part of this conversion? [ ... ]
quoted hunk ↗ jump to hunk
@@ -5644,6 +5621,132 @@ static bool rtl_aspm_is_safe(struct rtl8169_private *tp) return false; }
[ ... ]
+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;
+ phy_mode = PHY_INTERFACE_MODE_INTERNAL;
+
+ 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;
+ }
+
+ __set_bit(phy_mode, tp->phylink_config.supported_interfaces);
+ pl = phylink_create(&tp->phylink_config, tp_to_dev(tp)->fwnode,
+ phy_mode, &rtl_phylink_mac_ops);[Medium] Is PHY_INTERFACE_MODE_INTERNAL the intended interface mode here? Both the sfp_mode branch and the copper branch set phy_mode to PHY_INTERFACE_MODE_INTERNAL, which makes the if/else look like a copy-paste. The previous r8169_phy_connect() picked PHY_INTERFACE_MODE_GMII when supports_gmii was set and PHY_INTERFACE_MODE_MII otherwise: phy_mode = tp->supports_gmii ? PHY_INTERFACE_MODE_GMII : PHY_INTERFACE_MODE_MII; PHY_INTERFACE_MODE_INTERNAL is conventionally used for PHYs integrated on the same die as the MAC, whereas r8169's MAC-to-PHY link is logically GMII/MII. Phylink also derives interface-allowed link capabilities through phy_caps_from_interface() in phylink_get_capabilities(), so the chosen interface mode shapes which speeds and duplex modes are validated, and may affect PHY-side EEE/clock-stop handling and rate matching. Could the commit message explain why INTERNAL was selected for both arms, or should the copper branch keep the GMII/MII selection?