[PATCH net-next v2] r8152: Add 2500baseT EEE status/configuration support

[PATCH net-next v2] r8152: Add 2500baseT EEE status/configuration support

[Birger Koblitz]

The r8152 driver supports the RTL8156, which is a 2.5Gbit Ethernet controller for
USB 3.0, for which support is added for configuring and displaying the EEE
advertisement status for 2.5GBit connections.

The patch also corrects the determination of whether EEE is active to include
the 2.5GBit connection status and make the determination dependent not on the
desired speed configuration (tp->speed), but on the actual speed used by
the controller. For consistency, this is corrected also for the RTL8152/3.

This was tested on an Edimax EU-4307 V1.0 USB-Ethernet adapter with RTL8156,
and a SECOMP Value 12.99.1115 USB-C 3.1 Ethernet converter with RTL8153.

Signed-off-by: Birger Koblitz <mail@birger-koblitz.de>
---
Changes in v2:
- Added forgotten writing of tp->eee_adv2 into OCP_EEE_ADV2 
- Initialize common link mode mask to 0 on declaration
- Link to v1: https://lore.kernel.org/r/20260223-b4-eee2g5-v1-1-7006b537b144@birger-koblitz.de
---
 drivers/net/usb/r8152.c | 50 +++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 42 insertions(+), 8 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 49433301e7b1d5c98fc32c72f821e31335bc3527..ca65bf450c89a05313474fbaa4350b375a1a171d 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -213,6 +213,7 @@
 #define OCP_EEE_LPABLE		0xa5d2
 #define OCP_10GBT_CTRL		0xa5d4
 #define OCP_10GBT_STAT		0xa5d6
+#define OCP_EEE_LPABLE2		0xa6d0
 #define OCP_EEE_ADV2		0xa6d4
 #define OCP_PHY_STATE		0xa708		/* nway state for 8153 */
 #define OCP_PHY_PATCH_STAT	0xb800
@@ -954,6 +955,7 @@ struct r8152 {
 	u16 ocp_base;
 	u16 speed;
 	u16 eee_adv;
+	u16 eee_adv2;
 	u8 *intr_buff;
 	u8 version;
 	u8 duplex;
@@ -5397,7 +5399,7 @@ static void r8156_eee_en(struct r8152 *tp, bool enable)
 
 	config = ocp_reg_read(tp, OCP_EEE_ADV2);
 
-	if (enable)
+	if (enable && (tp->eee_adv2 & MDIO_EEE_2_5GT))
 		config |= MDIO_EEE_2_5GT;
 	else
 		config &= ~MDIO_EEE_2_5GT;
@@ -8927,7 +8929,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);
@@ -8941,8 +8944,14 @@ static int r8152_get_eee(struct r8152 *tp, struct ethtool_keee *eee)
 
 	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;
 }
@@ -8953,7 +8962,10 @@ static int r8152_set_eee(struct r8152 *tp, struct ethtool_keee *eee)
 
 	tp->eee_en = eee->eee_enabled;
 	tp->eee_adv = val;
-
+	if (tp->support_2500full) {
+		val = linkmode_to_mii_eee_cap2_t(eee->advertised);
+		tp->eee_adv2 = val;
+	}
 	rtl_eee_enable(tp, tp->eee_en);
 
 	return 0;
@@ -8961,7 +8973,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);
@@ -8973,10 +8986,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;
 }
@@ -9517,6 +9549,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;
@@ -9542,6 +9575,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;

---
base-commit: 89ed47f863c259e0c639d3b8c62cde2c30485c6b
change-id: 20260223-b4-eee2g5-20d7c6fd4a88

Best regards,
-- 
Birger Koblitz <mail@birger-koblitz.de>

Re: [net-next,v2] r8152: Add 2500baseT EEE status/configuration support

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
r8152: Add 2500baseT EEE status/configuration support

The r8152 driver supports the RTL8156, which is a 2.5Gbit Ethernet
controller for USB 3.0, for which support is added for configuring and
displaying the EEE advertisement status for 2.5GBit connections. The
patch also corrects the determination of whether EEE is active to include
the 2.5GBit connection status and make the determination dependent not on
the desired speed configuration (tp->speed), but on the actual speed used
by the controller.

> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index 8d36162f36df..3df1a1f4a350 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c

[ ... ]

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

(Reference: https://lore.kernel.org/all/20260224170206.146491-3-horms@kernel.org/)

[ ... ]

> @@ -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().

[ ... ]

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

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

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?

Re: [net-next,v2] r8152: Add 2500baseT EEE status/configuration support

[Birger Koblitz]

Hi Simon,

Thanks for the thorough analysis!

On 25/02/2026 8:48 pm, Simon Horman wrote:

> 
>> @@ -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;
> 
I do not understand what the AI is complaining about, here. The lines in 
the v2 patch are in exactly the order you requested:
-	__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@kernel.org/)
> 
> [ ... ]
> 
>> @@ -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().
Same argument as above.

> 
> [ ... ]
> 
>> @@ -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?
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).

> 
>> @@ -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.
VER 15 supports 2.5GBit links. The existing code enabled 2.5GBit EEE 
unconditionally in rtl_eee_enable():
	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.

> 
> 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?
No, see above.

   Birger

Re: [net-next,v2] r8152: Add 2500baseT EEE status/configuration support

[Simon Horman]

On Wed, Feb 25, 2026 at 11:02:17PM +0100, Birger Koblitz wrote:
> Hi Simon,
> 
> Thanks for the thorough analysis!
> 
> On 25/02/2026 8:48 pm, Simon Horman wrote:
> 
> > 
> > > @@ -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;
> > 
> I do not understand what the AI is complaining about, here. The lines in the
> v2 patch are in exactly the order you requested:
> -	__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;

Sorry, there was supposed to be some commentary from me included in
my previous email. But somehow it got lost.

I agree the AI is wrong here.

> 
> > (Reference: https://lore.kernel.org/all/20260224170206.146491-3-horms@kernel.org/)
> > 
> > [ ... ]
> > 
> > > @@ -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().
> Same argument as above.

Agreed.

> > [ ... ]
> > 
> > > @@ -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?
> 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).

Understood. I did think this point was worth raising.
And I appreciate your feedback. We can let this one go for now.

> 
> > 
> > > @@ -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.
> VER 15 supports 2.5GBit links. The existing code enabled 2.5GBit EEE
> unconditionally in rtl_eee_enable():
> 	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.

Sorry, I missed that when reviewing the AI generated feedback.

> > 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?
> No, see above.

Ack.

It looks like the AI was all false positives this time.
And I'll mark it accordingly in the system. Thanks for looking over it.

Re: [PATCH net-next v2] r8152: Add 2500baseT EEE status/configuration support

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Tue, 24 Feb 2026 18:40:14 +0100 you wrote:
> The r8152 driver supports the RTL8156, which is a 2.5Gbit Ethernet controller for
> USB 3.0, for which support is added for configuring and displaying the EEE
> advertisement status for 2.5GBit connections.
> 
> The patch also corrects the determination of whether EEE is active to include
> the 2.5GBit connection status and make the determination dependent not on the
> desired speed configuration (tp->speed), but on the actual speed used by
> the controller. For consistency, this is corrected also for the RTL8152/3.
> 
> [...]

Here is the summary with links:
  - [net-next,v2] r8152: Add 2500baseT EEE status/configuration support
    https://git.kernel.org/netdev/net-next/c/e8e83b67960c

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html