Re: [PATCH net-next v2 1/4] r8152: Add support for 10Gbit Link Speeds and EEE
From: Jakub Kicinski <kuba@kernel.org>
Date: 2026-05-01 01:15:58
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 10Gbit Link Speeds and EEE This change adds support for 10 Gbit link speeds on RTL8159 via the ethtool link_ksettings interface and adds 10 Gbit EEE advertisement handling alongside the existing 2.5G/5G paths.
quoted hunk ↗ jump to hunk
diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c index 7337bf1b7d6a..01e65d845f87 100644 --- a/drivers/net/usb/r8152.c +++ b/drivers/net/usb/r8152.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -5773,6 +5779,11 @@ static void r8156_eee_en(struct r8152 *tp, bool enable) else config &= ~MDIO_EEE_5GT; + if (enable && (tp->eee_adv2 & MDIO_EEE_10GT)) + config |= MDIO_EEE_10GT; + else + config &= ~MDIO_EEE_10GT; + ocp_reg_write(tp, OCP_EEE_ADV2, config); }
Is MDIO_EEE_10GT actually a bit of the EEE advertisement 2 register (7.62, cap2 / OCP_EEE_ADV2)? MDIO_EEE_10GT is defined by IEEE 802.3 as a bit of the EEE advertisement register 7.60 (cap1 / OCP_EEE_ADV), not of register 7.62. The existing 10G EEE advertisement already flows through tp->eee_adv (populated by linkmode_to_mii_eee_cap1_t() in r8152_set_eee()) and is written to OCP_EEE_ADV by rtl_eee_enable(). tp->eee_adv2 is populated only by linkmode_to_mii_eee_cap2_t() in r8152_set_eee(), which covers 2500baseT and 5000baseT and cannot set MDIO_EEE_10GT, plus the chip-init assignments in rtl_ops_init() which also do not set this bit. With that in mind, is the enable && (tp->eee_adv2 & MDIO_EEE_10GT) condition ever true? The else branch will then unconditionally clear bit 3 of OCP_EEE_ADV2 on every call, which is a cap1 bit being written into the cap2 register. [ ... ]
quoted hunk ↗ jump to hunk
@@ -8968,6 +9008,13 @@ static int r8153_get_eee(struct r8152 *tp, struct ethtool_keee *eee) linkmode_set_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, common); } + if (tp->support_10000full) { + linkmode_set_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT, eee->supported); + + if (speed & _10000bps) + linkmode_set_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT, common); + } + eee->eee_enabled = tp->eee_en;
Should this hunk also extend the earlier gating inside r8153_get_eee()?
A few lines above, the cap2 register reads are still gated on:
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);
}
The equivalent disjunction in rtl8152_set_speed() and
rtl8152_get_link_ksettings() was updated to include support_10000full,
but r8153_get_eee() (and r8152_set_eee() at the analogous spot) still
only checks support_2500full || support_5000full.
A hypothetical chip that sets only support_10000full would bypass the
OCP_EEE_ADV2 / OCP_EEE_LPABLE2 reads and never reflect 10G EEE into
eee->advertised. Should the condition be made consistent with the
other sites?
quoted hunk ↗ jump to hunk
@@ -9982,6 +10029,11 @@ static int rtl8152_probe_once(struct usb_interface *intf, tp->speed = SPEED_5000; tp->advertising |= RTL_ADVERTISED_5000_FULL; } + if (tp->support_10000full && + tp->udev->speed >= USB_SPEED_SUPER) { + tp->speed = SPEED_10000; + tp->advertising |= RTL_ADVERTISED_10000_FULL; + } tp->advertising |= RTL_ADVERTISED_1000_FULL; } tp->duplex = DUPLEX_FULL;
Is USB_SPEED_SUPER the correct gate for defaulting to a 10 Gbit link? USB_SPEED_SUPER is USB 3.0 SuperSpeed at 5 Gbit/s signaling, which cannot carry a 10 Gbit Ethernet link. The minimum bus rate that can sustain 10GBASE-T is USB_SPEED_SUPER_PLUS (USB 3.1 Gen 2, 10 Gbit/s). With this check, a 10G-capable RTL8159 plugged into a USB 3.0 host will still default to tp->speed = SPEED_10000 and advertise RTL_ADVERTISED_10000_FULL, so the Ethernet link may negotiate at 10G while the USB bus is bottlenecked well below that, leading to misleading ethtool speed reporting and back-pressure on the bus. The same >= USB_SPEED_SUPER gate is reused from the 2.5G and 5G defaults, which fit inside the USB 3.0 envelope; should the 10G default instead use >= USB_SPEED_SUPER_PLUS? -- pw-bot: cr