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