Re: [PATCH net-next v2 3/6] r8169: add support for phylink

[Jakub Kicinski]

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.

> 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

[ ... ]

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

[ ... ]

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

[ ... ]

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