Re: [PATCH net-next v4 2/2] r8152: Add support for the RTL8157 hardware

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

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 the RTL8157 hardware
> 
> This commit adds support for the RTL8157 chip, including its new 16-byte
> descriptor format, SRAM/ADV interfaces, and PHY setup. It reuses existing
> initialization functions where possible while branching for chip-specific
> logic.
> 
>> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
>> index bab8e10e5f99..010126e6dae4 100644
>> --- a/drivers/net/usb/r8152.c
>> +++ b/drivers/net/usb/r8152.c
>> @@ -1629,6 +1862,28 @@ static u16 sram_read(struct r8152 *tp, u16 addr)
>>   	return ocp_reg_read(tp, OCP_SRAM_DATA);
>>   }
>>   
>> +static void sram_write_w0w1(struct r8152 *tp, u16 addr, u16 clear, u16 set)
>> +{
>> +	u16 data = sram_read(tp, addr);
>> +
>> +	data = (data & ~clear) | set;
>> +	ocp_reg_write(tp, OCP_SRAM_DATA, data);
>> +}
> 
> Does this sequence write the modified data back to the wrong address?
> 
> Indirect SRAM access ports typically auto-increment their internal address
> pointer upon read. Since sram_read() reads OCP_SRAM_DATA, the address
> pointer likely advances, causing the subsequent ocp_reg_write() to corrupt
> the adjacent register instead of the intended one. Should this use
> sram_write(tp, addr, data) to reset the address before writing?
> 
The SRAM access does not have this alleged behaviour. This optimization is
found in the out-of-tree driver by Realtek. I also read back the values written
into the SRAM address as well as the following address, and the code is correct.

> 
> Is the same auto-increment issue present here with OCP_SRAM2_DATA?
No. Again, the optimization is also found in the out-of-tree driver and I explicitly
verified the data written.


> 
> [ ... ]
> 
>> @@ -4080,6 +4473,14 @@ static void rtl8156_runtime_enable(struct r8152 *tp, bool enable)
>>   	}
>>   }
>>   
>> +static void rtl8157_runtime_enable(struct r8152 *tp, bool enable)
>> +{
>> +	r8153_queue_wake(tp, enable);
>> +	r8153b_u1u2en(tp, false);
>> +	r8157_u2p3en(tp, false);
>> +	rtl_runtime_suspend_enable(tp, enable);
>> +}
> 
> Does this function break power management upon the first suspend and resume
> cycle? Unlike rtl8156_runtime_enable() which checks the enable parameter,
> this implementation unconditionally calls r8153b_u1u2en(tp, false) and
> r8157_u2p3en(tp, false) even when resuming.
This appears to be a bug in the out-of-tree driver. I will change this
function to follow the behaviour of rtl8156_runtime_enable() in v5. I have also traced
the PM functions while entering and leaving S3 sleep, and the behavior is now correct.


> 
> [ ... ]
> 
>> @@ -6693,7 +7052,7 @@ static void rtl8156_up(struct r8152 *tp)
>>   	r8153_aldps_en(tp, true);
>>   	r8153_u2p3en(tp, true);
>>   
>> -	if (tp->udev->speed >= USB_SPEED_SUPER)
>> +	if (tp->version != RTL_VER_16 && tp->udev->speed >= USB_SPEED_SUPER)
>>   		r8153b_u1u2en(tp, true);
>>   }
> 
> Because r8153_u2p3en(tp, true) is called unconditionally just before the
> modified lines, will RTL8157 incorrectly write to the legacy USB_U2P3_CTRL
> register instead of using its specific r8157_u2p3en() function?
This was a mistake. I will fix this to call r8153_u2p3en() only for non-RTL8157
chips.


> 
> [ ... ]
> 
>> @@ -6706,13 +7065,13 @@ static void rtl8156_down(struct r8152 *tp)
>>   		return;
>>   	}
>>   
>> -	ocp_data = ocp_read_word(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL3);
>> -	ocp_data |= PLA_MCU_SPDWN_EN;
>> -	ocp_write_word(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL3, ocp_data);
>> +	ocp_word_set_bits(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL3, PLA_MCU_SPDWN_EN);
>>   
>>   	r8153b_u1u2en(tp, false);
>> -	r8153_u2p3en(tp, false);
>> -	r8153b_power_cut_en(tp, false);
>> +	if (tp->version != RTL_VER_16) {
>> +		r8153_u2p3en(tp, false);
>> +		r8153b_power_cut_en(tp, false);
>> +	}
>>   	r8153_aldps_en(tp, false);
> 
> While the legacy registers are correctly avoided for RTL_VER_16 here, are we
> missing calls to r8157_u2p3en(tp, false) and r8157_power_cut_en(tp, false)?
> If omitted, these power states might remain permanently enabled when the
> interface goes down.
These calls are not in the out-of-tree driver. It appears the r8157_u2p3en()
is not used in the _up and _down functions. I measured the USB power
consumption with an interposed USB dongle on the port, and the power use is
better than for the RTL8153 when the link is down.

> 
> [ ... ]
> 
>> @@ -8362,10 +8825,21 @@ static void r8156b_init(struct r8152 *tp)
>>   
>>   	set_bit(GREEN_ETHERNET, &tp->flags);
>>   
>> -	/* rx aggregation */
>> -	ocp_data = ocp_read_word(tp, MCU_TYPE_USB, USB_USB_CTRL);
>> -	ocp_data &= ~(RX_AGG_DISABLE | RX_ZERO_EN);
>> -	ocp_write_word(tp, MCU_TYPE_USB, USB_USB_CTRL, ocp_data);
>> +	/* rx aggregation / 16 bytes Rx descriptor */
>> +	if (tp->version == RTL_VER_16)
>> +		ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_USB_CTRL, RX_AGG_DISABLE | RX_DESC_16B);
> 
> Is it intended to clear the RX_DESC_16B bit for RTL_VER_16? The driver uses
> sizeof(struct rx_desc_v2) which is a 16-byte descriptor for RTL8157.
> Clearing this bit might configure the hardware to send 8-byte descriptors
> instead, leading to packet parsing corruption in rx_bottom().
The code is correct as per the out-of-tree driver. I also tested packet
transmission in various stress-tests.

> 
> [ ... ]
> 
>> @@ -9647,6 +10158,29 @@ static int rtl_ops_init(struct r8152 *tp)
>>   		tp->rx_buf_sz		= 32 * 1024;
>>   		tp->eee_en		= true;
>>   		tp->eee_adv		= MDIO_EEE_1000T | MDIO_EEE_100TX;
>> +		r8152_desc_init(tp);
>> +		break;
>> +
>> +	case RTL_VER_16:
>> +		tp->eee_en		= true;
>> +		tp->eee_adv		= MDIO_EEE_1000T | MDIO_EEE_100TX;
>> +		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->unload		= rtl8153_unload;
> 
> Will using rtl8153_unload for RTL_VER_16 result in incorrect power cut
> teardown? rtl8153_unload calls the legacy r8153_power_cut_en(tp, false).
> RTL8157 seems to require r8157_power_cut_en(tp, false) to properly clear
> USB_MISC_2 bit 1 and PCUT_STATUS on module unload.
This behaviour is also found in the out-of-tree driver by Realtek.
The out-of-tree function reads:

static void rtl8157_unload(struct r8152 *tp)
{
	if (test_bit(RTL8152_UNPLUG, &tp->flags))
		return;

	r8153_power_cut_en(tp, false);

	/* Disable Interrupt Mitigation */
	if (ocp_byte_clr_bits(tp, MCU_TYPE_USB, 0xcf04,
			      BIT(0) | BIT(1) | BIT(2) | BIT(7)) < 0)
		return;
}

However, I had forgotten to add the "Disable Interrupt Mitigation" part.
I therefore added it, depending on RTL_VER_16.

I tested unloading and re-loading the driver, again while measuring the USB power
consumption with an interposed USB dongle on the port, and the wattage
in the state where the driver is unloaded for the RTL8157 is lower than the
one for the RTL8153 (280mW vs 450mW). The driver behavior now appear
correct.