Re: [net-next,v2] r8152: Add 2500baseT EEE status/configuration support
From: Birger Koblitz
Date: Wed Feb 25 2026 - 17:02:48 EST
Hi Simon,
Thanks for the thorough analysis!
On 25/02/2026 8:48 pm, Simon Horman wrote:
I do not understand what the AI is complaining about, here. The lines in the v2 patch are in exactly the order you requested:
@@ -8922,7 +8924,8 @@ static void rtl8152_get_strings(struct net_device *dev, u32 stringset, u8 *data)
static int r8152_get_eee(struct r8152 *tp, struct ethtool_keee *eee)
{
- __ETHTOOL_DECLARE_LINK_MODE_MASK(common);
+ __ETHTOOL_DECLARE_LINK_MODE_MASK(common) = {};
+ u16 speed = rtl8152_get_speed(tp);
u16 val;
val = r8152_mmd_read(tp, MDIO_MMD_PCS, MDIO_PCS_EEE_ABLE);
This isn't a bug, but the variable declarations are not in reverse
Christmas tree order. Simon Horman's AI review raised this in v1, you
acknowledged it and said it would be fixed in v2, but the ordering issue
remains. The longest line should come first, followed by shorter lines.
Should be:
__ETHTOOL_DECLARE_LINK_MODE_MASK(common) = {};
u16 speed = rtl8152_get_speed(tp);
u16 val;
- __ETHTOOL_DECLARE_LINK_MODE_MASK(common);
+ __ETHTOOL_DECLARE_LINK_MODE_MASK(common) = {};
+ u16 speed = rtl8152_get_speed(tp);
u16 val;
gives
__ETHTOOL_DECLARE_LINK_MODE_MASK(common) = {};
u16 speed = rtl8152_get_speed(tp);
u16 val;
(Reference: https://lore.kernel.org/all/20260224170206.146491-3-horms@xxxxxxxxxx/)Same argument as above.
[ ... ]
@@ -8956,7 +8968,8 @@ static int r8152_set_eee(struct r8152 *tp, struct ethtool_keee *eee)
static int r8153_get_eee(struct r8152 *tp, struct ethtool_keee *eee)
{
- __ETHTOOL_DECLARE_LINK_MODE_MASK(common);
+ __ETHTOOL_DECLARE_LINK_MODE_MASK(common) = {};
+ u16 speed = rtl8152_get_speed(tp);
u16 val;
val = ocp_reg_read(tp, OCP_EEE_ABLE);
Same reverse Christmas tree order issue here. The same pattern should be
applied as in r8152_get_eee().
This could make sense, but I tried to make this as minimally intrusive as possible and was already at a doubt whether I should even fix the RTL8152 version of get_eee() to correctly depend on the actual, not the desired speed. Then I decided for fixing this incorrect behavior and have consistency. The style of the existing code has plenty of duplicate code for the different chip-versions and is already difficult to follow to this point depending on the chip version. Personally, I find the readability improved if the few lines of code are repeated instead of separating this out into yet another nested function, and readability also helps code maintenance. Plus, it is quite likely that these functions further diverge with e.g. support for the RTL8157 (5GBit/s).
[ ... ]
@@ -8968,10 +8981,29 @@ static int r8153_get_eee(struct r8152 *tp, struct ethtool_keee *eee)
val = ocp_reg_read(tp, OCP_EEE_LPABLE);
mii_eee_cap1_mod_linkmode_t(eee->lp_advertised, val);
+ if (tp->support_2500full) {
+ linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, eee->supported);
+
+ val = ocp_reg_read(tp, OCP_EEE_ADV2);
+ mii_eee_cap2_mod_linkmode_adv_t(eee->advertised, val);
+
+ val = ocp_reg_read(tp, OCP_EEE_LPABLE2);
+ mii_eee_cap2_mod_linkmode_adv_t(eee->lp_advertised, val);
+
+ if (speed & _2500bps)
+ linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, common);
+ }
+
eee->eee_enabled = tp->eee_en;
- linkmode_and(common, eee->advertised, eee->lp_advertised);
- eee->eee_active = phy_check_valid(tp->speed, tp->duplex, common);
+ if (speed & _1000bps)
+ linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT, common);
+ if (speed & _100bps)
+ linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT, common);
+
+ linkmode_and(common, common, eee->advertised);
+ linkmode_and(common, common, eee->lp_advertised);
+ eee->eee_active = !linkmode_empty(common);
return 0;
}
The eee_active determination logic is now duplicated between
r8152_get_eee() and r8153_get_eee(). Both functions convert speed
register bits to linkmode bits, AND with advertised and lp_advertised,
and test emptiness in the same way. Would it make sense to factor this
into a shared helper to reduce maintenance burden?
VER 15 supports 2.5GBit links. The existing code enabled 2.5GBit EEE unconditionally in rtl_eee_enable():
@@ -9512,6 +9544,7 @@ static int rtl_ops_init(struct r8152 *tp)
case RTL_VER_11:
tp->eee_en = true;
tp->eee_adv = MDIO_EEE_1000T | MDIO_EEE_100TX;
+ tp->eee_adv2 = MDIO_EEE_2_5GT;
fallthrough;
case RTL_VER_10:
ops->init = r8156_init;
[ ... ]
@@ -9537,6 +9570,7 @@ static int rtl_ops_init(struct r8152 *tp)
case RTL_VER_15:
tp->eee_en = true;
tp->eee_adv = MDIO_EEE_1000T | MDIO_EEE_100TX;
+ tp->eee_adv2 = MDIO_EEE_2_5GT;
ops->init = r8156b_init;
ops->enable = rtl8156b_enable;
ops->disable = rtl8153_disable;
Does RTL_VER_15 support 2.5G links? Looking at the code, only RTL_VER_12
and RTL_VER_13 set tp->support_2500full before falling through, but
RTL_VER_15 does not. With this change, tp->eee_adv2 = MDIO_EEE_2_5GT is
set for VER_15.
case RTL_VER_10:
case RTL_VER_11:
case RTL_VER_12:
case RTL_VER_13:
case RTL_VER_15:
if (enable) {
r8156_eee_en(tp, true);
ocp_reg_write(tp, OCP_EEE_ADV, tp->eee_adv);
} else {
r8156_eee_en(tp, false);
ocp_reg_write(tp, OCP_EEE_ADV, 0);
}
break;
Note that previously r8156_eee_en() did not conditionally enable the 2.5GBit advertisement depending on tp->eee_adv2, but always enabled it, also for VER 15.
No, see above.
When r8156_eee_en() is called for a VER_15 device with EEE enabled:
static void r8156_eee_en(struct r8152 *tp, bool enable)
{
u16 config;
config = ocp_reg_read(tp, OCP_EEE_ADV2);
if (enable && (tp->eee_adv2 & MDIO_EEE_2_5GT))
config |= MDIO_EEE_2_5GT;
The condition (tp->eee_adv2 & MDIO_EEE_2_5GT) will be true, causing
MDIO_EEE_2_5GT to be written into the OCP_EEE_ADV2 hardware register.
This would advertise 2.5G EEE capability to the link partner on a device
that does not support 2.5Gbit links.
Should the eee_adv2 initialization be moved into the RTL_VER_12/13 block
(before the fallthrough), or should RTL_VER_15 be given eee_adv2 = 0 to
remain consistent with its lack of support_2500full?
Birger