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