Re: [PATCH net-next v2 2/4] r8152: Add support for the RTL8159 chip

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

On 01/05/2026 3:15 am, Jakub Kicinski wrote:
> The commit subject says RTL8159 is being added, but the body reads
> "The RTL8157 re-uses the packet descriptor format introduced with the
> RTL8157 ..." and later "Add support for the USB device ID of Realtek
> RTL8157-based adapters." Should these occurrences of RTL8157 actually
> be RTL8159?

Both are typos and RTL8159 was meant. Will be fixed in v3.

> 
>>      Most of the addiitonal code is added in r8157_hw_phy_cfg() to configure
>                  ^^^^^^^^^^
> 
> Typo: addiitonal.

Will fix.

> 
>>      Add support for the USB device ID of Realtek RTL8157-based adapters. Detect
>>      the RTL8159 as RTL_VER_17 and set it up.
> 
> The new entry in rtl8152_table is 0x815a, which breaks the earlier
> 0x815N-for-RTL815N naming convention (0x8157 for RTL8157, etc.). Could
> the commit message explain why the RTL8159 ships with product id
> 0x815a?

Will fix.

> 
>> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
>> index 01e65d845f87..2a07dde289e2 100644
>> --- a/drivers/net/usb/r8152.c
>> +++ b/drivers/net/usb/r8152.c
> 
> [ ... ]
> 
>> @@ -6894,7 +6903,7 @@ static void rtl8156_down(struct r8152 *tp)
>>   			  PLA_MCU_SPDWN_EN);
>>   
>>   	r8153b_u1u2en(tp, false);
>> -	if (tp->version != RTL_VER_16) {
>> +	if (tp->version < RTL_VER_16) {
>>   		r8153_u2p3en(tp, false);
>>   		r8153b_power_cut_en(tp, false);
>>   	}
> 
> rtl8156_down() is converted from "!= RTL_VER_16" to "< RTL_VER_16" so
> that RTL_VER_17 takes the same path as RTL_VER_16. rtl8156_up() is
> wired as ops->up for RTL_VER_17 in rtl_ops_init() but still uses
> "!= RTL_VER_16" / "== RTL_VER_16" checks that were not converted.

This was simply forgotten. The code-path for the RTL8159, i.e. 
RTL_VER_17 should be the same as for the RTL8157 (RTL_VER_16), will fix 
rtl8156_up() accordingly.

> The rest of the driver organizes helpers by chip family (r8152_,
> r8153_, r8156_, r8157_) and the commit message describes the RTL8159
> as reusing the RTL8157 scaffolding. Is the r8159_ prefix on this
> single helper intentional, or would r8157_wait_backup_restore() (or
> similar) fit better?

The prefix is intentional, since the function is only used for the
RTL8159.

> 
> Also, poll_timeout_us() is invoked with a 200 us poll interval and a
> 2000 us overall timeout, giving roughly ten iterations before the
> error path fires. Other GPHY-related waits in the driver allow up to
> the order of seconds. Is 2 ms known to be sufficient for the
> backup-restore handshake on RTL8159 across suspend/resume and on
> slower hosts?

Experimentally, the function returns always on the first iteration, 
which is the reason for the short poll interval. A safety factor of 10 
appears to be relatively generous. The out-of-tree code was busy waiting 
for 100 iterations without any delay. The polling in wait_cmd_ready() 
waits for up-to 20ms. Since it does not hurt and to be on the safe side, 
I will change the total poll time to 20ms, here, too.

> 
>> @@ -8184,14 +8389,14 @@ static void r8156_init(struct r8152 *tp)

> Later in the same function:
> 
> 	if (tp->version != RTL_VER_16)
> 		ocp_word_clr_bits(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL3,
> 				  PLA_MCU_SPDWN_EN);
> 
> Was this site meant to be converted to "< RTL_VER_16" along with the
> others? As written, RTL_VER_17 clears PLA_MCU_SPDWN_EN during init,
> while RTL_VER_16 does not.

Indeed, this was an oversight, it should have been also converted to 
"<". Will fix.

> 
> [ ... ]
> 
>> @@ -8294,7 +8505,10 @@ static void r8156_init(struct r8152 *tp)
>>   	set_bit(GREEN_ETHERNET, &tp->flags);
>>   
>>   	/* rx aggregation / 16 bytes Rx descriptor */
>> -	if (tp->version == RTL_VER_16)
>> +	if (tp->version == RTL_VER_17)
>> +		ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_USB_CTRL,
>> +				  RX_AGG_DISABLE | RX_DESC_16B | BIT(11));
>> +	else if (tp->version == RTL_VER_16)
>>   		ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_USB_CTRL, RX_AGG_DISABLE | RX_DESC_16B);
> 
> Every other USB_USB_CTRL bit written in this driver uses a named
> macro (CDC_ECM_EN, RX_AGG_DISABLE, RX_ZERO_EN, RX_DESC_16B). Could
> BIT(11) be given a symbolic name so future multi-version handling of
> this register stays self-describing?

Unfortunately, BIT(11) just appears here, for the RTL8159 in the 
out-of-tree code, without any further explanation. For BIT(10) = 
RX_DESC_16B I guessed the name from the comment. That bit appears for 
both chips, but BIT(11) just is there for the RTL8159, while the comment 
stays the same. Any guess could turn out to be misleading.

> 
> [ ... ]
> 
>> @@ -9690,6 +9904,29 @@ static int rtl_ops_init(struct r8152 *tp)
>>   		r8157_desc_init(tp);
>>   		break;
>>   
>> +	case RTL_VER_17:
>> +		tp->eee_en		= true;
>> +		tp->eee_adv		= MDIO_EEE_100TX | MDIO_EEE_1000T | MDIO_EEE_10GT;
>> +		tp->eee_adv2		= MDIO_EEE_2_5GT | MDIO_EEE_5GT;
>> +		ops->init		= r8156_init;
>> +		ops->enable		= rtl8156_enable;
>> +		ops->disable		= rtl8153_disable;
>> +		ops->up			= rtl8156_up;
>> +		ops->down		= rtl8156_down;
> 
> ops->up is wired to rtl8156_up() here, but rtl8156_up() still checks
> for RTL_VER_16 exclusively at the sites noted above. See the
> rtl8156_down() hunk for the pattern that appears to be intended.

This is fixed, see above.

> RTL_VER_17 sets rx_buf_sz to 48 KiB and enables support_10000full,
> but the max_mtu switch in rtl8152_probe() does not include
> RTL_VER_17:
> 
> 	case RTL_VER_12:
> 	case RTL_VER_13:
> 	case RTL_VER_15:
> 	case RTL_VER_16:
> 		netdev->max_mtu = size_to_mtu(16 * 1024);
> 		break;
> 	...
> 	default:
> 		netdev->max_mtu = ETH_DATA_LEN;
> 		break;
> 
> Should RTL_VER_17 also select the 16 KiB max_mtu case so jumbo frames
> are available on the new 10 Gbit chip?

Yes, this was an oversight. Will be fixed.

> 
> Separately, rtl8156_enable() is set as ops->enable for RTL_VER_17 but
> the USB_RX_AGGR_NUM reset is guarded by
> "tp->version >= RTL_VER_12 && tp->version <= RTL_VER_16", so
> RTL_VER_17 does not execute it. Is that intended given RTL_VER_17
> otherwise shares the RTL_VER_16 descriptor/aggregation plumbing
> (r8157_desc_init(), and the USB_USB_CTRL aggregation bits cleared in
> r8156_init() for RTL_VER_17)?
> 
> [ ... ]
RTL_VER_17 should be included in that range. Will fix.