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

[Birger Koblitz]

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.
>
> > 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
>
> [ ... ]
>
> > @@ -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.

> > -	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.

> [ ... ]
>
> > @@ -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) {
>
> [ ... ]
>
> > +	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.

> > @@ -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