Re: [PATCH net-next v2 1/4] r8152: Add support for 10Gbit Link Speeds and EEE

[Birger Koblitz <mail@birger-koblitz.de>]

Thanks a lot for reviewing this patch, Jakub!
On 01/05/2026 3:15 am, Jakub Kicinski wrote:

>> +	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)?
> 
No, and it should have been clear to me. The entire bit of code is 
actually unneccessary, as the MDIO_EEE_10GT-BIT is part of OCP_EEE_ADV 
and is set in tp->eee_adv, so that rtl_eee_enable() correctly already 
sets it:
static void rtl_eee_enable(struct r8152 *tp, bool enable)
{
	case RTL_VER_17:
		if (enable) {
			r8156_eee_en(tp, true);
			ocp_reg_write(tp, OCP_EEE_ADV, tp->eee_adv);
		}
	...
}
Will be fixed in v3.

> 
> [ ... ]
> 
>> @@ -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?
The AI here actually contradicts itself with the previous issue. The 
10GBit modes are part of OCP_EEE_ADV (MMD 7.0x003C) and OCP_EEE_LPABLE 
(MMD 7.0x003D). A hypothetical chip that sets only support_10000full 
would still reflect the correct LP-ability 10GBit setting.

> 
>> @@ -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?
I believe this is the correct gate. In order to get the full speed out 
of the 10GBit Ethernet connection, a 20GBit USB link would be needed, 
anyway, with 10GBit USB-C, i.e. USB_SPEED_SUPER_PLUS, I see only about 
7.6GBit throughput. The difference of 10GBit wire-speed to 5GBit Speed 
for USB 3.0 is not that bad, and the overhead does not seem to make a 
problem.

But I think this is not the point: an important aspect of the 10GBit 
USB-Ethernet adapters is that they are used for testing 10GBit 
connections with network equipment. The RTL8159 is even put in 
connection with a media converter and an SFP+ cage in the same housing 
and advertised for testing 10GBit fiber lines. It should be possible to 
test and use 10GBit connectivity even on a 5GBit USB port.

Birger