Re: [PATCH net-next v3 3/8] r8152: refactor rtl8156_enable, rtl8156_up, and rtl8156_down

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential
issue(s) to consider:

Critical: 0 · High: 2 · Medium: 3 · Low: 0

- [High] Version guard collapsed in the wrong direction:
  `ocp_word_clr_bits(tp, MCU_TYPE_PLA, PLA_RCR1, BIT(3))` in…
- [High] During the split of rtl8156_up()/rtl8156_down(), the two power-
  management writes for RTL8157/RTL8159 (RTL_VER_16,…
- [Medium] The commit message describes a pure code-motion refactor ("Add
  dedicated rtl8156b_enable (VER_12/13/15), rtl8157_enable, rtl8157_up,…
- [Medium] rtl8156_down() (RTL_VER_10/11 and RTL_VER_12/13/15) and the
  new rtl8157_down() (RTL_VER_16/17) both add two OOB hand-off register…
- [Medium] New 'Clear_SDR' hardware programming is introduced with raw
  register addresses and no symbolic defines: `ocp_word_set_bits(tp,…

--

r8152: refactor rtl8156_enable, rtl8156_up, and rtl8156_down

This splits the shared RTL8156 enable/up/down handlers into per-family
variants: rtl8156b_enable() for RTL_VER_12/13/15 and rtl8157_enable(),
rtl8157_up(), rtl8157_down() for RTL_VER_16/17.  The inline version
guards inside rtl8156_enable(), rtl8156_up() and rtl8156_down() are
dropped and rtl_ops_init() is updated to install the new callbacks.

[Severity: Medium]
The changelog reads as a pure code-motion refactor:

  "Add dedicated rtl8156b_enable (VER_12/13/15), rtl8157_enable,
   rtl8157_up, and rtl8157_down (VER_16/17) instead of handling
   per-version differences with inline version guards."

But rtl8156_up() and rtl8156_down() themselves are also modified, and
several register writes change for every family involved:

  - PLA_RCR1 BIT(3) clear becomes unconditional in rtl8156_up(), so it
    is now a new write on RTL8156/RTL8156B.
  - a new "Enable Clear_SDR" write to USB 0xd3ca BIT(15) for
    RTL_VER_13/15 in rtl8156_up().
  - rtl8156_down() replaces the bare PLA_OOB_CTRL |= NOW_IS_OOB with
    PLA_BDC_CR |= ALDPS_PROXY_MODE plus
    PLA_OOB_CTRL |= NOW_IS_OOB | DIS_MCU_CLROOB.
  - rtl8157_up() performs the USB_SPEED_OPTION clear that the baseline
    skipped for RTL_VER_16+, drops the PLA_MCU_SPDWN_EN clear, and adds
    two new writes (USB 0xd378 BIT(7) set, USB 0xcd06 BIT(15) clear).
  - rtl8157_down() changes PLA_RMS/PLA_MTPS and drops the
    PLA_MCU_SPDWN_EN set.

Could the changelog describe these hardware programming changes, or could
they be split into separate patches so they can be reverted or bisected
independently of the refactor?

> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index f01f442fa71a0..6c189790b0bab 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c

[ ... ]

> @@ -6869,8 +6912,7 @@ static void rtl8156_up(struct r8152 *tp)
>  
>  	ocp_byte_clr_bits(tp, MCU_TYPE_PLA, PLA_OOB_CTRL, NOW_IS_OOB);
>  
> -	if (tp->version >= RTL_VER_16)
> -		ocp_word_clr_bits(tp, MCU_TYPE_PLA, PLA_RCR1, BIT(3));
> +	ocp_word_clr_bits(tp, MCU_TYPE_PLA, PLA_RCR1, BIT(3));

[Severity: High]
Was this guard collapsed in the wrong direction?  After the split,
rtl_ops_init() installs rtl8156_up() only for RTL_VER_10/11 and
RTL_VER_12/13/15, and gives RTL_VER_16/17 the new rtl8157_up():

	ops->up			= rtl8156_up;	/* VER_10/11, VER_12/13/15 */
	...
	ops->up			= rtl8157_up;	/* VER_16, VER_17_QFN68/100 */

So the "tp->version >= RTL_VER_16" condition can no longer be true in
rtl8156_up(), which suggests the write should have been removed together
with its guard rather than made unconditional.  The new rtl8157_up()
already contains the identical write:

	ocp_word_clr_bits(tp, MCU_TYPE_PLA, PLA_RCR1, BIT(3));

Does this mean RTL8156/RTL8156B now clear bit 3 of PLA_RCR1 (0xc012) on
every bring-up and resume, a write those parts never received before?
The driver only names OUTER_VLAN BIT(7) and INNER_VLAN BIT(6) for that
register, so the effect of BIT(3) on those chips is not obvious from the
code.

>  
>  	ocp_word_clr_bits(tp, MCU_TYPE_PLA, PLA_SFF_STS_7, MCU_BORW_EN);
>  
> @@ -6892,11 +6934,11 @@ static void rtl8156_up(struct r8152 *tp)
>  	ocp_word_w0w1(tp, MCU_TYPE_PLA, PLA_RXFIFO_FULL, RXFIFO_FULL_MASK,
>  		      0x08);
>  
> -	ocp_word_clr_bits(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL3, PLA_MCU_SPDWN_EN);
> +	ocp_word_clr_bits(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL3,
> +			  PLA_MCU_SPDWN_EN);
>  
> -	if (tp->version < RTL_VER_16)
> -		ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_SPEED_OPTION,
> -				  RG_PWRDN_EN | ALL_SPEED_OFF);
> +	ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_SPEED_OPTION,
> +			  RG_PWRDN_EN | ALL_SPEED_OFF);
>  
>  	ocp_write_dword(tp, MCU_TYPE_USB, USB_RX_BUF_TH, 0x00600400);
>  
> @@ -6906,10 +6948,19 @@ static void rtl8156_up(struct r8152 *tp)
>  	}
>  
>  	r8153_aldps_en(tp, true);
> -	if (tp->version < RTL_VER_16)
> -		r8153_u2p3en(tp, true);
> +	r8153_u2p3en(tp, true);
>  
> -	if (tp->version < RTL_VER_16 && tp->udev->speed >= USB_SPEED_SUPER)
> +	switch (tp->version) {
> +	case RTL_VER_13:
> +	case RTL_VER_15:
> +		/* Enable Clear_SDR */
> +		ocp_word_set_bits(tp, MCU_TYPE_USB, 0xd3ca, BIT(15));
> +		break;
> +	default:
> +		break;
> +	}

[Severity: Medium]
This "Clear_SDR" programming is new, has no baseline counterpart, and
uses a raw address with no #define.  Two questions about it:

The same USB address is accessed as a byte register elsewhere in this
file, in r8157_init() and r8159_init():

	ocp_byte_clr_bits(tp, MCU_TYPE_USB, 0xd3ca, BIT(0));

while the new code does a 16-bit read-modify-write covering
0xd3ca..0xd3cb and sets BIT(15).  Is the upper byte a plain read/write
field?  If any of it is W1C or status, can this read-modify-write clobber
unrelated state?

Also, RTL_VER_12 is RTL8156B silicon as well but is excluded here.  Is
VER_12 genuinely unaffected, or was it missed?

There is no matching clear of this bit in rtl8156_down().  Is the bit
expected to persist across an ifdown/ifup cycle?

> +
> +	if (tp->udev->speed >= USB_SPEED_SUPER)
>  		r8153b_u1u2en(tp, true);
>  }
>  
> @@ -6922,12 +6973,9 @@ static void rtl8156_down(struct r8152 *tp)
>  
>  	ocp_word_set_bits(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL3,
>  			  PLA_MCU_SPDWN_EN);
> -
>  	r8153b_u1u2en(tp, false);
> -	if (tp->version < RTL_VER_16) {
> -		r8153_u2p3en(tp, false);
> -		r8153b_power_cut_en(tp, false);
> -	}
> +	r8153_u2p3en(tp, false);
> +	r8153b_power_cut_en(tp, false);
>  	r8153_aldps_en(tp, false);
>  
>  	ocp_byte_clr_bits(tp, MCU_TYPE_PLA, PLA_OOB_CTRL, NOW_IS_OOB);
> @@ -6949,7 +6997,102 @@ static void rtl8156_down(struct r8152 *tp)
>  	 */
>  	ocp_write_word(tp, MCU_TYPE_PLA, PLA_TEREDO_WAKE_BASE, 0x00ff);
>  
> -	ocp_byte_set_bits(tp, MCU_TYPE_PLA, PLA_OOB_CTRL, NOW_IS_OOB);
> +	ocp_word_set_bits(tp, MCU_TYPE_PLA, PLA_BDC_CR, ALDPS_PROXY_MODE);
> +
> +	ocp_byte_set_bits(tp, MCU_TYPE_PLA, PLA_OOB_CTRL,
> +			  NOW_IS_OOB | DIS_MCU_CLROOB);

[Severity: Medium]
These two writes are new for the chips rtl8156_down() still serves
(RTL_VER_10/11 and RTL_VER_12/13/15 per rtl_ops_init()).  The baseline
ended the OOB entry with only:

	ocp_byte_set_bits(tp, MCU_TYPE_PLA, PLA_OOB_CTRL, NOW_IS_OOB);

ALDPS_PROXY_MODE in PLA_BDC_CR and DIS_MCU_CLROOB in PLA_OOB_CTRL change
who owns OOB state and how the MCU proxies during ALDPS, so does this
alter WoL and ARP-proxy behaviour on RTL8156/RTL8156B after an ifdown or
suspend?  The same two writes are added to rtl8157_down() below.

It looks like this brings 8156 in line with r8152b_enter_oob() and
r8153_enter_oob(), which do program both bits.  If that is the intent,
could it be stated in the changelog or split out so a WoL regression
report can be traced back to it?

> +
> +	ocp_word_set_bits(tp, MCU_TYPE_PLA, PLA_SFF_STS_7, MCU_BORW_EN);
> +
> +	rtl_rx_vlan_en(tp, true);
> +	rxdy_gated_en(tp, false);
> +
> +	ocp_dword_set_bits(tp, MCU_TYPE_PLA, PLA_RCR,
> +			   RCR_APM | RCR_AM | RCR_AB);
> +
> +	r8153_aldps_en(tp, true);
> +}
> +
> +static void rtl8157_up(struct r8152 *tp)
> +{

[ ... ]

> +	/* share FIFO settings */
> +	ocp_word_w0w1(tp, MCU_TYPE_PLA, PLA_RXFIFO_FULL, RXFIFO_FULL_MASK,
> +		      0x08);
> +
> +	ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_SPEED_OPTION,
> +			  RG_PWRDN_EN | ALL_SPEED_OFF);

[Severity: High]
Are the two power-management writes for RTL8157/RTL8159 swapped here?

The baseline guarded this USB_SPEED_OPTION clear specifically to exclude
these versions:

	if (tp->version < RTL_VER_16)
		ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_SPEED_OPTION,
				  RG_PWRDN_EN | ALL_SPEED_OFF);

and rtl8157_up() is installed only for RTL_VER_16 and
RTL_VER_17_QFN68/QFN100, so this now runs on exactly the parts the guard
excluded.  Does clearing RG_PWRDN_EN | ALL_SPEED_OFF change PHY power
management on 8157/8159?

In the other direction, the PLA_MCU_SPDWN_EN clear that rtl8156_up()
does just before this point is missing from rtl8157_up(), and the paired
set is missing from rtl8157_down().  The baseline did both
unconditionally for RTL_VER_16/17, and grepping the file after this patch
the remaining PLA_MCU_SPDWN_EN sites are rtl8153b_up/down, rtl8153c_up,
rtl8156_up/down, r8153b_init, r8156_init and r8156b_init - none of which
are reachable for VER_16/17, and r8157_init()/r8159_init() only clear it
"if (tp->version < RTL_VER_16)".

Does that leave PLA_MAC_PWR_CTRL3 PLA_MCU_SPDWN_EN at whatever value the
ROM, OOB firmware or a previous driver instance left while the interface
is up on those chips?

> +
> +	ocp_write_dword(tp, MCU_TYPE_USB, USB_RX_BUF_TH, 0x00600400);
> +
> +	if (tp->saved_wolopts != __rtl_get_wol(tp)) {
> +		netif_warn(tp, ifup, tp->netdev, "wol setting is changed\n");
> +		__rtl_set_wol(tp, tp->saved_wolopts);
> +	}
> +
> +	r8153_aldps_en(tp, true);
> +
> +	/* Clear_SDR */
> +	ocp_byte_set_bits(tp, MCU_TYPE_USB, 0xd378, BIT(7));
> +	ocp_word_clr_bits(tp, MCU_TYPE_USB, 0xcd06, BIT(15));

[Severity: Medium]
These two writes have no baseline counterpart, no symbolic defines, and
no matching undo in rtl8157_down().  Could they get names, and is the
comment "Clear_SDR" enough to explain what the two different addresses
do?

> +}
> +
> +static void rtl8157_down(struct r8152 *tp)
> +{

[ ... ]

> +	ocp_write_word(tp, MCU_TYPE_PLA, PLA_RMS, 1526);
> +	ocp_write_byte(tp, MCU_TYPE_PLA, PLA_MTPS, 10 * 1024 / 64);

[Severity: Medium]
The baseline down path applied 1522 and MTPS_DEFAULT for RTL_VER_16/17.
Is the change to 1526 and 10 * 1024 / 64 intentional, and could it be
mentioned in the changelog?

> +
> +	/* Clear teredo wake event. bit[15:8] is the teredo wakeup
> +	 * type. Set it to zero. bits[7:0] are the W1C bits about
> +	 * the events. Set them to all 1 to clear them.
> +	 */
> +	ocp_write_word(tp, MCU_TYPE_PLA, PLA_TEREDO_WAKE_BASE, 0x00ff);
> +
> +	ocp_word_set_bits(tp, MCU_TYPE_PLA, PLA_BDC_CR, ALDPS_PROXY_MODE);
> +
> +	ocp_byte_set_bits(tp, MCU_TYPE_PLA, PLA_OOB_CTRL,
> +			  NOW_IS_OOB | DIS_MCU_CLROOB);
>  
>  	ocp_word_set_bits(tp, MCU_TYPE_PLA, PLA_SFF_STS_7, MCU_BORW_EN);
>  
> @@ -10071,7 +10214,7 @@ static int rtl_ops_init(struct r8152 *tp)
>  		tp->eee_adv		= MDIO_EEE_1000T | MDIO_EEE_100TX;
>  		tp->eee_adv2		= MDIO_EEE_2_5GT;
>  		ops->init		= r8156b_init;
> -		ops->enable		= rtl8156_enable;
> +		ops->enable		= rtl8156b_enable;
>  		ops->disable		= rtl8153_disable;
>  		ops->up			= rtl8156_up;
>  		ops->down		= rtl8156_down;
> @@ -10110,10 +10253,10 @@ static int rtl_ops_init(struct r8152 *tp)
>  		tp->eee_adv		= MDIO_EEE_1000T | MDIO_EEE_100TX;
>  		tp->eee_adv2		= MDIO_EEE_2_5GT | MDIO_EEE_5GT;
>  		ops->init		= r8157_init;
> -		ops->enable		= rtl8156_enable;
> +		ops->enable		= rtl8157_enable;
>  		ops->disable		= rtl8153_disable;
> -		ops->up			= rtl8156_up;
> -		ops->down		= rtl8156_down;
> +		ops->up			= rtl8157_up;
> +		ops->down		= rtl8157_down;

[ ... ]

For reference, none of the above is changed later in the series; the
same code is still present in rtl8156_up(), rtl8156_down(),
rtl8157_up() and rtl8157_down() at the end of the series.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260908075634.23740-1-nic_swsd%40realtek.com