The RTL8157 supports 5GBit Link speeds. Add support for this speed
in the setup and setting/getting through ethtool. Also add 5GBit EEE.
Add functionality for setup and ethtool get/set methods.
Signed-off-by: Birger Koblitz <mail@birger-koblitz.de>
---
drivers/net/usb/r8152.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 53 insertions(+), 3 deletions(-)
diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 3b6d4252d34c63ead8f11a120e212325a5f7d505..bab8e10e5f99afcb332e333c2739ed7509b03419 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -604,6 +604,7 @@ enum spd_duplex {
FORCE_100M_FULL,
FORCE_1000M_FULL,
NWAY_2500M_FULL,
+ NWAY_5000M_FULL,
};
/* OCP_ALDPS_CONFIG */
@@ -725,6 +726,7 @@ enum spd_duplex {
#define BP4_SUPER_ONLY 0x1578 /* RTL_VER_04 only */
enum rtl_register_content {
+ _5000bps = BIT(12),
_2500bps = BIT(10),
_1250bps = BIT(9),
_500bps = BIT(8),
@@ -738,6 +740,7 @@ enum rtl_register_content {
};
#define is_speed_2500(_speed) (((_speed) & (_2500bps | LINK_STATUS)) == (_2500bps | LINK_STATUS))
+#define is_speed_5000(_speed) (((_speed) & (_5000bps | LINK_STATUS)) == (_5000bps | LINK_STATUS))
#define is_flow_control(_speed) (((_speed) & (_tx_flow | _rx_flow)) == (_tx_flow | _rx_flow))
#define RTL8152_MAX_TX 4
@@ -944,6 +947,7 @@ struct r8152 {
unsigned int pipe_in, pipe_out, pipe_intr, pipe_ctrl_in, pipe_ctrl_out;
u32 support_2500full:1;
+ u32 support_5000full:1;
u32 lenovo_macpassthru:1;
u32 dell_tb_rx_agg_bug:1;
u16 ocp_base;
@@ -1194,6 +1198,7 @@ enum tx_csum_stat {
#define RTL_ADVERTISED_1000_HALF BIT(4)
#define RTL_ADVERTISED_1000_FULL BIT(5)
#define RTL_ADVERTISED_2500_FULL BIT(6)
+#define RTL_ADVERTISED_5000_FULL BIT(7)
/* Maximum number of multicast addresses to filter (vs. Rx-all-multicast).
* The RTL chips use a 64 element hash table based on the Ethernet CRC.
@@ -5400,6 +5405,11 @@ static void r8156_eee_en(struct r8152 *tp, bool enable)
else
config &= ~MDIO_EEE_2_5GT;
+ if (enable && (tp->eee_adv2 & MDIO_EEE_5GT))
+ config |= MDIO_EEE_5GT;
+ else
+ config &= ~MDIO_EEE_5GT;
+
ocp_reg_write(tp, OCP_EEE_ADV2, config);
}
@@ -6298,6 +6308,9 @@ static int rtl8152_set_speed(struct r8152 *tp, u8 autoneg, u32 speed, u8 duplex,
if (tp->support_2500full)
support |= RTL_ADVERTISED_2500_FULL;
+
+ if (tp->support_5000full)
+ support |= RTL_ADVERTISED_5000_FULL;
}
if (!(advertising & support))
@@ -6343,15 +6356,20 @@ static int rtl8152_set_speed(struct r8152 *tp, u8 autoneg, u32 speed, u8 duplex,
r8152_mdio_write(tp, MII_CTRL1000, new1);
}
- if (tp->support_2500full) {
+ if (tp->support_2500full || tp->support_5000full) {
orig = ocp_reg_read(tp, OCP_10GBT_CTRL);
- new1 = orig & ~MDIO_AN_10GBT_CTRL_ADV2_5G;
+ new1 = orig & ~(MDIO_AN_10GBT_CTRL_ADV2_5G | MDIO_AN_10GBT_CTRL_ADV5G);
if (advertising & RTL_ADVERTISED_2500_FULL) {
new1 |= MDIO_AN_10GBT_CTRL_ADV2_5G;
tp->ups_info.speed_duplex = NWAY_2500M_FULL;
}
+ if (advertising & RTL_ADVERTISED_5000_FULL) {
+ new1 |= MDIO_AN_10GBT_CTRL_ADV5G;
+ tp->ups_info.speed_duplex = NWAY_5000M_FULL;
+ }
+
if (orig != new1)
ocp_reg_write(tp, OCP_10GBT_CTRL, new1);
}
@@ -8780,6 +8798,9 @@ int rtl8152_get_link_ksettings(struct net_device *netdev,
linkmode_mod_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT,
cmd->link_modes.supported, tp->support_2500full);
+ linkmode_mod_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT,
+ cmd->link_modes.supported, tp->support_5000full);
+
if (tp->support_2500full) {
linkmode_mod_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT,
cmd->link_modes.advertising,
@@ -8793,6 +8814,19 @@ int rtl8152_get_link_ksettings(struct net_device *netdev,
cmd->base.speed = SPEED_2500;
}
+ if (tp->support_5000full) {
+ linkmode_mod_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT,
+ cmd->link_modes.advertising,
+ ocp_reg_read(tp, OCP_10GBT_CTRL) & MDIO_AN_10GBT_CTRL_ADV5G);
+
+ linkmode_mod_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT,
+ cmd->link_modes.lp_advertising,
+ ocp_reg_read(tp, OCP_10GBT_STAT) & MDIO_AN_10GBT_STAT_LP5G);
+
+ if (is_speed_5000(rtl8152_get_speed(tp)))
+ cmd->base.speed = SPEED_5000;
+ }
+
mutex_unlock(&tp->control);
usb_autopm_put_interface(tp->intf);
@@ -8840,6 +8874,10 @@ static int rtl8152_set_link_ksettings(struct net_device *dev,
cmd->link_modes.advertising))
advertising |= RTL_ADVERTISED_2500_FULL;
+ if (test_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT,
+ cmd->link_modes.advertising))
+ advertising |= RTL_ADVERTISED_5000_FULL;
+
mutex_lock(&tp->control);
ret = rtl8152_set_speed(tp, cmd->base.autoneg, cmd->base.speed,
@@ -8957,7 +8995,7 @@ 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) {
+ if (tp->support_2500full || tp->support_5000full) {
val = linkmode_to_mii_eee_cap2_t(eee->advertised);
tp->eee_adv2 = val;
}
@@ -8994,6 +9032,13 @@ static int r8153_get_eee(struct r8152 *tp, struct ethtool_keee *eee)
linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, common);
}
+ if (tp->support_5000full) {
+ linkmode_set_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, eee->supported);
+
+ if (speed & _5000bps)
+ linkmode_set_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, common);
+ }
+
eee->eee_enabled = tp->eee_en;
if (speed & _1000bps)
@@ -9934,6 +9979,11 @@ static int rtl8152_probe_once(struct usb_interface *intf,
} else {
tp->speed = SPEED_1000;
}
+ if (tp->support_5000full &&
+ tp->udev->speed >= USB_SPEED_SUPER) {
+ tp->speed = SPEED_5000;
+ tp->advertising |= RTL_ADVERTISED_5000_FULL;
+ }
tp->advertising |= RTL_ADVERTISED_1000_FULL;
}
tp->duplex = DUPLEX_FULL;
--
2.47.3This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
r8152: Add support for 5Gbit Link Speeds and EEE
This commit adds support for 5GBit link speeds and EEE to the RTL8157
driver. It updates the speed and duplex settings, ethtool integration,
and EEE advertisement parameters to handle the new 5G capabilities.
> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index 3b6d4252d34c..bab8e10e5f99 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c
> @@ -604,6 +604,7 @@ enum spd_duplex {
> FORCE_100M_FULL,
> FORCE_1000M_FULL,
> NWAY_2500M_FULL,
> + NWAY_5000M_FULL,
> };
Are there missing switch cases for NWAY_5000M_FULL in the hardware UPS
flag configurations, such as r8156_ups_flags?
It looks like if a 5Gbps link is established, it might fall through to the
default case and leave the speed bits as 0. Since ups_flags_speed(0) maps
to FORCE_10M_HALF, this could cause the device's clock scaling and power
management features to be misconfigured for 10M Half-Duplex instead of the
5Gbps link.
[ ... ]
> @@ -725,6 +726,7 @@ enum spd_duplex {
> #define BP4_SUPER_ONLY 0x1578 /* RTL_VER_04 only */
>
> enum rtl_register_content {
> + _5000bps = BIT(12),
> _2500bps = BIT(10),
Is it intentional that the MAC idle speed down feature (IDLE_SPDWN_EN) is
disabled for _2500bps but remains enabled for _5000bps?
In functions like rtl8156_enable, the code checks if the speed is _2500bps
to disable the idle speed down feature, but the new _5000bps constant is
not included in those checks. This might cause latency spikes or instability
at 5Gbps if the power-saving feature is enabled.
[ ... ]
> @@ -6343,15 +6356,20 @@ static int rtl8152_set_speed(struct r8152 *tp, u8 autoneg, u32 speed, u8 duplex,
> r8152_mdio_write(tp, MII_CTRL1000, new1);
> }
>
> - if (tp->support_2500full) {
> + if (tp->support_2500full || tp->support_5000full) {
> orig = ocp_reg_read(tp, OCP_10GBT_CTRL);
> - new1 = orig & ~MDIO_AN_10GBT_CTRL_ADV2_5G;
> + new1 = orig & ~(MDIO_AN_10GBT_CTRL_ADV2_5G | MDIO_AN_10GBT_CTRL_ADV5G);
>
> if (advertising & RTL_ADVERTISED_2500_FULL) {
> new1 |= MDIO_AN_10GBT_CTRL_ADV2_5G;
> tp->ups_info.speed_duplex = NWAY_2500M_FULL;
> }
>
> + if (advertising & RTL_ADVERTISED_5000_FULL) {
> + new1 |= MDIO_AN_10GBT_CTRL_ADV5G;
> + tp->ups_info.speed_duplex = NWAY_5000M_FULL;
> + }
> +
Could this unintentionally advertise 5GbE on a 2.5GbE-only adapter if
userspace requests it?
The earlier check in this function:
if (!(advertising & support))
return -EINVAL;
only ensures that at least one supported speed is requested, but it does
not strip unsupported speeds from the advertising mask. Since this block
is now entered when support_2500full is true, checking the advertising mask
for RTL_ADVERTISED_5000_FULL without verifying tp->support_5000full could
erroneously write the 5G bit to the PHY and misconfigure the hardware
state.
[ ... ]
> @@ -8793,6 +8814,19 @@ int rtl8152_get_link_ksettings(struct net_device *netdev,
> cmd->base.speed = SPEED_2500;
> }
>
> + if (tp->support_5000full) {
> + linkmode_mod_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT,
> + cmd->link_modes.advertising,
> + ocp_reg_read(tp, OCP_10GBT_CTRL) & MDIO_AN_10GBT_CTRL_ADV5G);
> +
> + linkmode_mod_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT,
> + cmd->link_modes.lp_advertising,
> + ocp_reg_read(tp, OCP_10GBT_STAT) & MDIO_AN_10GBT_STAT_LP5G);
> +
> + if (is_speed_5000(rtl8152_get_speed(tp)))
> + cmd->base.speed = SPEED_5000;
> + }
Since 5G adapters like the RTL8157 will have both support_2500full and
support_5000full set, will this result in duplicate blocking USB control
transfers?
The ocp_reg_read calls for OCP_10GBT_CTRL and OCP_10GBT_STAT, along with
rtl8152_get_speed, are also executed in the preceding support_2500full
block. Reading these into local variables before the capability checks
would avoid the overhead of duplicate hardware accesses.
[ ... ]
> @@ -8994,6 +9032,13 @@ static int r8153_get_eee(struct r8152 *tp, struct ethtool_keee *eee)
> linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, common);
> }
>
> + if (tp->support_5000full) {
> + linkmode_set_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, eee->supported);
> +
> + if (speed & _5000bps)
> + linkmode_set_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, common);
> + }
In r8153_get_eee, the reading of OCP_EEE_ADV2 and OCP_EEE_LPABLE2 is
performed inside the if (tp->support_2500full) block that precedes this new
code.
Since those registers contain the EEE capability bits for both 2.5G
and 5G speeds, what happens if a device has support_5000full enabled but
support_2500full disabled? It looks like the registers would never be read,
and the 5Gbps EEE capabilities would fail to populate correctly.
--
pw-bot: cr
Hi Jakub,
thanks a lot for reviewing the patch!
On 3/29/26 21:34, Jakub Kicinski 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.
> ---
> r8152: Add support for 5Gbit Link Speeds and EEE
>
> This commit adds support for 5GBit link speeds and EEE to the RTL8157
> driver. It updates the speed and duplex settings, ethtool integration,
> and EEE advertisement parameters to handle the new 5G capabilities.
>
>> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
>> index 3b6d4252d34c..bab8e10e5f99 100644
>> --- a/drivers/net/usb/r8152.c
>> +++ b/drivers/net/usb/r8152.c
>> @@ -604,6 +604,7 @@ enum spd_duplex {
>> FORCE_100M_FULL,
>> FORCE_1000M_FULL,
>> NWAY_2500M_FULL,
>> + NWAY_5000M_FULL,
>> };
>
> Are there missing switch cases for NWAY_5000M_FULL in the hardware UPS
> flag configurations, such as r8156_ups_flags?
This question has now been raised the third time, see here:
https://lkml.org/lkml/2026/3/24/1902 and here https://lkml.org/lkml/2026/3/19/1765
However, while I share the concern, I cannot provide a better answer than
the first time or second time.
> Is it intentional that the MAC idle speed down feature (IDLE_SPDWN_EN) is
> disabled for _2500bps but remains enabled for _5000bps?
Yes, this is intentional. IDLE_SPDWN_EN is indeed only disabled for _2500bps
in the out-of-tree driver (and not for 5GBit or 10GBit).
>
>> @@ -6343,15 +6356,20 @@ static int rtl8152_set_speed(struct r8152 *tp, u8 autoneg, u32 speed, u8 duplex,
>> r8152_mdio_write(tp, MII_CTRL1000, new1);
>> }
>>
>> - if (tp->support_2500full) {
>> + if (tp->support_2500full || tp->support_5000full) {
>> orig = ocp_reg_read(tp, OCP_10GBT_CTRL);
>> - new1 = orig & ~MDIO_AN_10GBT_CTRL_ADV2_5G;
>> + new1 = orig & ~(MDIO_AN_10GBT_CTRL_ADV2_5G | MDIO_AN_10GBT_CTRL_ADV5G);
>>
>> if (advertising & RTL_ADVERTISED_2500_FULL) {
>> new1 |= MDIO_AN_10GBT_CTRL_ADV2_5G;
>> tp->ups_info.speed_duplex = NWAY_2500M_FULL;
>> }
>>
>> + if (advertising & RTL_ADVERTISED_5000_FULL) {
>> + new1 |= MDIO_AN_10GBT_CTRL_ADV5G;
>> + tp->ups_info.speed_duplex = NWAY_5000M_FULL;
>> + }
>> +
>
> Could this unintentionally advertise 5GbE on a 2.5GbE-only adapter if
> userspace requests it?
>
> The earlier check in this function:
>
> if (!(advertising & support))
> return -EINVAL;
>
> only ensures that at least one supported speed is requested, but it does
> not strip unsupported speeds from the advertising mask. Since this block
> is now entered when support_2500full is true, checking the advertising mask
> for RTL_ADVERTISED_5000_FULL without verifying tp->support_5000full could
> erroneously write the 5G bit to the PHY and misconfigure the hardware
> state.
I will change this to:
advertising &= support;
if (!advertising)
return -EINVAL;
in v5, to be on the safe side.
> [ ... ]
>
>> @@ -8793,6 +8814,19 @@ int rtl8152_get_link_ksettings(struct net_device *netdev,
>> cmd->base.speed = SPEED_2500;
>> }
>>
>> + if (tp->support_5000full) {
>> + linkmode_mod_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT,
>> + cmd->link_modes.advertising,
>> + ocp_reg_read(tp, OCP_10GBT_CTRL) & MDIO_AN_10GBT_CTRL_ADV5G);
>> +
>> + linkmode_mod_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT,
>> + cmd->link_modes.lp_advertising,
>> + ocp_reg_read(tp, OCP_10GBT_STAT) & MDIO_AN_10GBT_STAT_LP5G);
>> +
>> + if (is_speed_5000(rtl8152_get_speed(tp)))
>> + cmd->base.speed = SPEED_5000;
>> + }
>
> Since 5G adapters like the RTL8157 will have both support_2500full and
> support_5000full set, will this result in duplicate blocking USB control
> transfers?
You are right. Also in view of future 10GBit support, I will change this to:
if (tp->support_2500full || tp->support_5000full) {
u16 ocp_10bt_ctrl = ocp_reg_read(tp, OCP_10GBT_CTR)L;
u16 ocp_10bt_stat = ocp_reg_read(tp, OCP_10GBT_STAT);
if (tp->support_2500full) {
...
}
if (tp->support_5000full) {
...
}
}
> In r8153_get_eee, the reading of OCP_EEE_ADV2 and OCP_EEE_LPABLE2 is
> performed inside the if (tp->support_2500full) block that precedes this new
> code.
>
> Since those registers contain the EEE capability bits for both 2.5G
> and 5G speeds, what happens if a device has support_5000full enabled but
> support_2500full disabled? It looks like the registers would never be read,
> and the 5Gbps EEE capabilities would fail to populate correctly.
The driver supports RTL815x-chips, and all RTL815x-chips, which support 5GBit also
support 2.5gbit (the RTL8157 and the RTL8159). My understanding is, that the
802.3bz standard even requires this fallback. However, to be on the safe side and
to also support possible future chips, I will change this to:
if (tp->support_2500full || tp->support_5000full) {
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 (tp->support_2500full) {
linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, eee->supported);
if (speed & _2500bps)
linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, common);
}
if (tp->support_5000full) {
linkmode_set_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, eee->supported);
if (speed & _5000bps)
linkmode_set_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, common);
}
Cheers,
Birger
On Tue, 31 Mar 2026 17:38:02 +0200 Birger Koblitz wrote: > > Are there missing switch cases for NWAY_5000M_FULL in the hardware UPS > > flag configurations, such as r8156_ups_flags? > This question has now been raised the third time, see here: > https://lkml.org/lkml/2026/3/24/1902 and here https://lkml.org/lkml/2026/3/19/1765 > However, while I share the concern, I cannot provide a better answer than > the first time or second time. Apologies for the repeats, I tend to send out the full review as soon as I spot at least one chunk that looks plausible. I should do better but re-checking the AI reviews takes so much of my time already.. :(
On 4/1/26 02:56, Jakub Kicinski wrote: > On Tue, 31 Mar 2026 17:38:02 +0200 Birger Koblitz wrote: >>> Are there missing switch cases for NWAY_5000M_FULL in the hardware UPS >>> flag configurations, such as r8156_ups_flags? >> This question has now been raised the third time, see here: >> https://lkml.org/lkml/2026/3/24/1902 and here https://lkml.org/lkml/2026/3/19/1765 >> However, while I share the concern, I cannot provide a better answer than >> the first time or second time. > > Apologies for the repeats, I tend to send out the full review as > soon as I spot at least one chunk that looks plausible. > I should do better but re-checking the AI reviews takes so much > of my time already.. :( No problem, it also took me a couple of minutes to understand that it was in fact the same issue :-) I appreciate a lot the time and effort spent by reviewers going through patches, and reviewing AI output probably just adds on top of that. I see that some of the issues found would have been hard for a human to spot. On the other hand, some of the issues raised make little sense when giving them a second look, and would probably not have been brought up by a human reviewer. Still, they take quit some time to answer properly. One thing I noticed in particular is that the AI does not seem to take into account that the code actually works: suspecting broken SRAM-access or wrong descriptor-size settings would break the driver at such a fundamental level, it would not be able to work at all. Birger
> One thing I > noticed in particular is that the AI does not seem to take into account > that the code actually works: suspecting broken SRAM-access or wrong > descriptor-size settings would break the driver at such a fundamental level, > it would not be able to work at all. The flip side to this is that we get a lot of patches which are compile tested only. So assuming the patch has run on real hardware is a poor assumption to make. We humans however do have a feeling about who is submitting the patches and how likely they have run it on hardware. So we know to look for fundamental problems or not. Andrew
© 2016 - 2026 Red Hat, Inc.