On 19/03/2026 5:51 pm, Simon Horman 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.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
r8152: Add support for the RTL8157 hardware

The RTL8157 uses a different packet descriptor format compared to the
previous generation of chips. The commit adds support for the new format,
SRAM and ADV access interfaces, and consolidates RTL8156/RTL8156B/RTL8157
initialization into a single function.

quoted

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index bab8e10e5f99..05531ea43791 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c

[ ... ]

quoted

@@ -6151,15 +6562,24 @@ static void rtl8156_enable(struct r8152 *tp)
  	u32 ocp_data;
  	u16 speed;

+	if (tp->version <= RTL_VER_16)
+		return rtl_enable(tp);
+
  	ocp_data = ocp_read_word(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL4);

The comparison operation is indeed wrong, it should be
   tp->version >= RTL_VER_16
Note that RTL_VER_17 will be used for the RTL8159, and the enable 
functions for these chips are identical, so we are already using >= here.

Before this patch, both rtl8156_enable (VER_10/11) and rtl8156b_enable
(VER_12/13/15) executed all of these operations unconditionally.

Indeed, this will be again the case with the correction to
tp->version >= 16

I also verified that the new enable() function for the RTL8157 as found 
in the out-of-tree driver is indeed as short as implemented by the early 
return from rtl8156_enable() function, here.

This will be fixed in v3.

quoted

-	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);
+	if (tp->version != RTL_VER_16)
+		ocp_byte_clr_bits(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL3, PLA_MCU_SPDWN_EN);

Is ocp_byte_clr_bits() the correct function here? PLA_MCU_SPDWN_EN is
defined as BIT(14) (0x4000), which requires 16-bit access. ocp_byte_clr_bits()
performs an 8-bit read/modify/write via ocp_read_byte/ocp_write_byte,
accessing only bits 0-7 of the register. Bit 14 is outside the byte range.

Indeed, this is wrong, it should be ocp_word_clr_bits. Will be fixed in v3.

The other hw_phy_cfg functions (r8156_hw_phy_cfg, r8156b_hw_phy_cfg)
use 'u16 data' and don't attempt error handling on r8153_phy_status().

The analysis is correct and the code will not correctly handle errors.
I will use the same approach as in the other hw_phy_cfg functions and 
not attempt error handling. Will be fixed in v3.

If the device becomes inaccessible during r8153_phy_status(), the function
continues to program PHY registers with potentially invalid values instead
of returning early.

The driver uses a different approach to error handling. Once the device 
becomes inaccessible, this state is detected and the device reset.

[ ... ]

quoted

@@ -8191,59 +8543,180 @@ static int r8156_init(struct r8152 *tp)

  	ocp_data = ocp_read_byte(tp, MCU_TYPE_USB, USB_MISC_1);
  	if (ocp_data & PCUT_STATUS) {

[ ... ]

quoted

+	if (tp->version != RTL_VER_16)
+		ocp_byte_clr_bits(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL3, PLA_MCU_SPDWN_EN);

Same issue as above - is ocp_byte_clr_bits() the correct function for
clearing BIT(14)? PLA_MCU_SPDWN_EN is 0x4000, which exceeds the 8-bit range.

This is the same passage as above, found a second time by the AI. Will 
be fixed in v3.

quoted

@@ -9541,6 +10056,7 @@ static void rtl_ops_init(struct r8152 *tp)
  	case RTL_VER_12:
  	case RTL_VER_13:
  	case RTL_VER_15:
+		ops->init		= r8156_init;
  		ops->enable		= rtl8156_enable;
  		ops->disable		= rtl8153_disable;

The ops->enable assignment changes from rtl8156b_enable to rtl8156_enable
for RTL_VER_12/13/15 (RTL8156B). Does the new rtl8156_enable provide the
same functionality?

As noted in the earlier issue, rtl8156_enable has an early return at line
6581 that causes it to skip the PLA_MAC_PWR_CTRL4, USB_L1_CTRL, and
FC_PATCH_TASK configuration for all current versions. The old
rtl8156b_enable performed these operations.

This will be fixed with the correct comparison in rtl8156_enable as 
mentioned before, for that function. The function will then provide the 
same functionality as before for RTL_VER_12/13/15, so this will be fixed 
in v3.

Birger