RE: [PATCH net-next v3 5/8] r8152: support rtl8157_unload and rtl8157_change_mtu

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

Critical: 0 · High: 0 · Medium: 0 · Low: 1

- [Low] Commit message understates the impact of the change and carries
  no Fixes: tag, even though it silently alters the register programming…

--

r8152: support rtl8157_unload and rtl8157_change_mtu

This adds rtl8157_unload() and rtl8157_change_mtu() and wires them into the
RTL_VER_16 (RTL8157) and RTL_VER_17 (RTL8159) op tables, replacing
rtl8153_unload() and rtl8156_change_mtu().  The interrupt mitigation clear
of USB register 0xcf04 is moved out of rtl8153_unload() into the new callback.

quoted

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c index

679aead731f73..06fad895fce09 100644

--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c

@@ -7024,6 +7024,28 @@ static void rtl8156_down(struct r8152 *tp)
      r8153_aldps_en(tp, true);
 }

+static void rtl8157_change_mtu(struct r8152 *tp) {
+     u32 max_pkt_size = mtu_to_size(tp->netdev->mtu);
+     u32 ocp_data;
+
+     ocp_write_word(tp, MCU_TYPE_PLA, PLA_RMS, max_pkt_size);
+
+     /* Use at least 10K for MTPS */
+     ocp_data = max_t(u32, max_pkt_size, 10 * 1024) / 64;
+
+     /* 16 * 1024 / 64 = 0x100, so the max is 0xff for 8 bits data */
+     ocp_data = min_t(u32, ocp_data, 0xff);
+
+     ocp_write_byte(tp, MCU_TYPE_PLA, PLA_MTPS, ocp_data);
+     r8156_fc_parameter(tp);

[ ... ]

quoted

@@ -10174,11 +10196,6 @@ static void rtl8153_unload(struct r8152 *tp)
              return;

      r8153_power_cut_en(tp, false);
-
-     if (tp->version >= RTL_VER_16) {
-             /* Disable Interrupt Mitigation */
-             ocp_byte_clr_bits(tp, MCU_TYPE_USB, 0xcf04, BIT(0) | BIT(1)

| BIT(2) | BIT(7));

quoted

-     }
 }

[ ... ]

quoted

+static void rtl8157_unload(struct r8152 *tp) {
+     if (test_bit(RTL8152_INACCESSIBLE, &tp->flags))
+             return;
+
+     r8157_power_cut_en(tp, false);
+     ocp_byte_clr_bits(tp, MCU_TYPE_USB, 0xcf04,
+                       BIT(0) | BIT(1) | BIT(2) | BIT(7)); }

[Severity: Low]
Could the changelog say what misbehaves today on these chips, and carry
Fixes: tags?  RTL_VER_16 and RTL_VER_17 are already supported in-tree
(added by fd3c7d080df531 and ad2a55b63b41af), and both assigned
ops->unload = rtl8153_unload and ops->change_mtu = rtl8156_change_mtu,
ops->so
this changes the register programming of shipped hardware rather than
enabling something new.

The commit message says the dedicated unload is needed "to disable interrupt
mitigation", but rtl8153_unload() already did that for
tp->version >= RTL_VER_16:

        if (tp->version >= RTL_VER_16) {
                /* Disable Interrupt Mitigation */
                ocp_byte_clr_bits(tp, MCU_TYPE_USB, 0xcf04, BIT(0) |
BIT(1) | BIT(2) | BIT(7));
        }

so that part is only relocated.  The behaviour that actually changes is the
power cut disable sequence:

r8153_power_cut_en(tp, false):
        ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_POWER_CUT, PWR_EN
| PHASE2_EN);
        ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_MISC_0, PCUT_STATUS);

r8157_power_cut_en(tp, false):
        ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_POWER_CUT, PWR_EN);
        ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_MISC_0, PCUT_STATUS);
        ocp_byte_clr_bits(tp, MCU_TYPE_USB, USB_MISC_2, BIT(1));

PHASE2_EN is now left set on the unload/disconnect path for these chips.
Is that intentional, and is there an observable symptom with the old sequence?

It's intentional. RTL8157 and RTL8159 have different sequences to disable power cut.

Similarly, rtl8157_change_mtu() replaces the fixed MTPS_JUMBO (12 * 1024 /
64 = 0xc0) that rtl8156_change_mtu() programmed with an MTU derived value:
at the default 1500 byte MTU, max_t(u32, mtu_to_size(1500), 10 * 1024) / 64
gives 0xa0, and it saturates at 0xff for a 16K MTU.  Both paths are live at
runtime, via rtl8157_up() on every bring-up and via rtl8152_change_mtu() ->
tp->rtl_ops.change_mtu() on every MTU change.  What does the old MTPS
tp->value
break on these chips?

RTL8157 and RTL8159 support a new formula to compute PLA_MTPS.

As written it is hard to tell whether this is net-next tuning or a fix for a user
visible defect on released kernels, and there is nothing for stable backports to
key on.  Note that this wording is unchanged at the end of the series, so no
later patch adds the missing justification.

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

Best,
Chih-Kai