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

[netdev-bot+sashiko@kernel.org]

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