Re: [PATCH net-next v4 1/2] r8152: Add support for 5Gbit Link Speeds and EEE
From: Jakub Kicinski <kuba@kernel.org>
Date: 2026-03-29 19:34:57
Also in:
linux-usb, lkml
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.
quoted hunk ↗ jump to hunk
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. [ ... ]
quoted hunk ↗ jump to hunk
@@ -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. [ ... ]
quoted hunk ↗ jump to hunk
@@ -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. [ ... ]
quoted hunk ↗ jump to hunk
@@ -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. [ ... ]
quoted hunk ↗ jump to hunk
@@ -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