Re: [PATCH net-next v3 3/8] r8152: refactor rtl8156_enable, rtl8156_up, and rtl8156_down
From: netdev-bot+sashiko
Date: Sat Sep 12 2026 - 16:22:01 EST
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