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

[PATCH net-next] 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>
---
 drivers/net/usb/r8152.c | 42 +++++++++++++++++++++++++++++++++++++-----
 1 file changed, 37 insertions(+), 5 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 49433301e7b1d5c98fc32c72f821e31335bc3527..0bc3b1b93017c80e1761b00767904ff1c2e6ac87 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;
@@ -8929,6 +8931,7 @@ static int r8152_get_eee(struct r8152 *tp, struct ethtool_keee *eee)
 {
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(common);
 	u16 val;
+	u16 speed = rtl8152_get_speed(tp);
 
 	val = r8152_mmd_read(tp, MDIO_MMD_PCS, MDIO_PCS_EEE_ABLE);
 	mii_eee_cap1_mod_linkmode_t(eee->supported, val);
@@ -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;
@@ -8963,6 +8975,7 @@ static int r8153_get_eee(struct r8152 *tp, struct ethtool_keee *eee)
 {
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(common);
 	u16 val;
+	u16 speed = rtl8152_get_speed(tp);
 
 	val = ocp_reg_read(tp, OCP_EEE_ABLE);
 	mii_eee_cap1_mod_linkmode_t(eee->supported, val);
@@ -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;
 }

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

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

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

[Daniel Golle]

On Mon, Feb 23, 2026 at 08:20:35PM +0100, Birger Koblitz 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.
> 
> 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>
> ---
>  drivers/net/usb/r8152.c | 42 +++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 37 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index 49433301e7b1d5c98fc32c72f821e31335bc3527..0bc3b1b93017c80e1761b00767904ff1c2e6ac87 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;
> ...
> @@ -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;

eee_adv2 set here is never actually written to the hardware.
ie. you are never doing
ocp_reg_write(tp, OCP_EEE_ADV2, tp->eee_adv2);
for capable NICs.

Re: [net-next] 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..a831acd385cd 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c

[ ... ]

> @@ -8924,6 +8926,7 @@ static int r8152_get_eee(struct r8152 *tp, struct ethtool_keee *eee)
>  {
>  	__ETHTOOL_DECLARE_LINK_MODE_MASK(common);
>  	u16 val;
> +	u16 speed = rtl8152_get_speed(tp);

Simon says: please declare local variables in reverse xmas tree order -
            longest line to shortest.

>
>  	val = r8152_mmd_read(tp, MDIO_MMD_PCS, MDIO_PCS_EEE_ABLE);
>  	mii_eee_cap1_mod_linkmode_t(eee->supported, val);
> @@ -8936,8 +8939,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);

Does this code read uninitialized memory? The 'common' bitmap is declared
via __ETHTOOL_DECLARE_LINK_MODE_MASK(common), which expands to
DECLARE_BITMAP -- an uninitialized stack array:

unsigned long common[BITS_TO_LONGS(__ETHTOOL_LINK_MODE_MASK_NBITS)];

The new code conditionally sets individual bits via linkmode_set_bit(),
which calls __set_bit() to set a single bit position. This leaves all
other bits in the array containing whatever garbage was on the stack.

Then linkmode_and(common, common, eee->advertised) reads all the
uninitialized bits when performing the bitwise AND operation.

The old code was safe because it used linkmode_and(common,
eee->advertised, eee->lp_advertised), which fully wrote 'common' from two
initialized bitmaps before reading it.

With uninitialized stack data, eee_active could be incorrectly reported as
true if garbage bits happen to overlap with the advertised and
lp_advertised bitmasks.

>
>  	return 0;
>  }
> @@ -8948,7 +8957,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);

[Simon says: I note that Daniel Golle flagged this issue already]

Is the user's 2500baseT EEE configuration actually applied to hardware?
The code stores the user's preference in tp->eee_adv2, but
rtl_eee_enable() never writes this value to the OCP_EEE_ADV2 register.

For RTL_VER_10-15 (the 2500full-capable chips), rtl_eee_enable() calls
r8156_eee_en() which unconditionally sets MDIO_EEE_2_5GT rather than using
tp->eee_adv2:

drivers/net/usb/r8152.c:r8156_eee_en() {
    ...
    if (enable) {
        ...
        set |= MDIO_EEE_2_5GT;
    } else {
        ...
        clr |= MDIO_EEE_2_5GT;
    }
    ocp_data = (ocp_data & ~clr) | set;
    ocp_reg_write(tp, OCP_EEE_ADV2, ocp_data);
}

Then rtl_eee_enable() writes only tp->eee_adv to OCP_EEE_ADV:

drivers/net/usb/r8152.c:rtl_eee_enable() {
    ...
    case RTL_VER_10: ... case RTL_VER_15:
        r8156_eee_en(tp, enable);
        ...
        ocp_reg_write(tp, OCP_EEE_ADV, tp->eee_adv);
        break;
}

The OCP_EEE_ADV2 register is never updated with tp->eee_adv2. This means
user attempts to selectively control 2500baseT EEE advertisement via
ethtool are silently ignored.

The cap1 path correctly writes tp->eee_adv to OCP_EEE_ADV. Shouldn't the
cap2 path similarly write tp->eee_adv2 to OCP_EEE_ADV2?

>
>  	return 0;
>  }
> @@ -8958,6 +8970,7 @@ static int r8153_get_eee(struct r8152 *tp, struct ethtool_keee *eee)
>  {
>  	__ETHTOOL_DECLARE_LINK_MODE_MASK(common);
>  	u16 val;
> +	u16 speed = rtl8152_get_speed(tp);
>
>  	val = ocp_reg_read(tp, OCP_EEE_ABLE);
>  	mii_eee_cap1_mod_linkmode_t(eee->supported, val);

[ ... ]

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

The same uninitialized memory issue exists here in r8153_get_eee. The
'common' bitmap declared via __ETHTOOL_DECLARE_LINK_MODE_MASK is never
zeroed before individual speed bits are conditionally set with
linkmode_set_bit(). Then linkmode_and() reads the uninitialized garbage
bits.

This function affects RTL_VER_03 through RTL_VER_15, which is the majority
of supported devices.

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

[Birger Koblitz]

Hi Daniel,

On 24/02/2026 2:33 am, Daniel Golle wrote:
>>   	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;
> 
> eee_adv2 set here is never actually written to the hardware.
> ie. you are never doing
> ocp_reg_write(tp, OCP_EEE_ADV2, tp->eee_adv2);
> for capable NICs.
thanks for the feedback! You are of course right, the writing part was 
forgotten when editing the patch. I will send a corrected v2.

Birger

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

[Birger Koblitz]

Hi Simon,

On 24/02/2026 6:02 pm, Simon Horman wrote:
> 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..a831acd385cd 100644
>> --- a/drivers/net/usb/r8152.c
>> +++ b/drivers/net/usb/r8152.c
> 
> [ ... ]
> 
>> @@ -8924,6 +8926,7 @@ static int r8152_get_eee(struct r8152 *tp, struct ethtool_keee *eee)
>>   {
>>   	__ETHTOOL_DECLARE_LINK_MODE_MASK(common);
>>   	u16 val;
>> +	u16 speed = rtl8152_get_speed(tp);
> 
> Simon says: please declare local variables in reverse xmas tree order -
>              longest line to shortest.
Will fix in v2.

> Does this code read uninitialized memory? The 'common' bitmap is declared
> via __ETHTOOL_DECLARE_LINK_MODE_MASK(common), which expands to
> DECLARE_BITMAP -- an uninitialized stack array:
I will fix this in v2 by initializing common to an empty bitmask.

> [Simon says: I note that Daniel Golle flagged this issue already]
This was forgotten in the edited patch and will be part of v2.

> The same uninitialized memory issue exists here in r8153_get_eee. The
> 'common' bitmap declared via __ETHTOOL_DECLARE_LINK_MODE_MASK is never
> zeroed before individual speed bits are conditionally set with
> linkmode_set_bit(). Then linkmode_and() reads the uninitialized garbage
> bits.
> 
> This function affects RTL_VER_03 through RTL_VER_15, which is the majority
> of supported devices.
I will fix this in v2.

Birger