RE: [PATCH net-next v3 8/8] r8152: support UPS for RTL8157 and RTL8159

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

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

- [Medium] Incomplete version dispatch: the four new switch statements
  added by this patch enumerate only RTL_VER_16 and RTL_VER_17_QFN68
and…

--

r8152: support UPS for RTL8157 and RTL8159

This adds r8157_ups_en() for the RTL8157/RTL8159 UPS enable sequence,
extends r8156_ups_flags() with per-speed EEE flags and 5G/10G speed
encodings, and switches r8157_init()/r8159_init() over to the new enable
helper.

quoted

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

2b0972b967385..cc18b1c5a17c3 100644

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

[ ... ]

quoted

@@ -4178,8 +4184,27 @@ static void r8156_ups_flags(struct r8152 *tp)
      if (tp->ups_info.aldps)
              ups_flags |= UPS_FLAGS_EN_ALDPS;

-     if (tp->ups_info.eee)
-             ups_flags |= UPS_FLAGS_EN_EEE;
+     if (tp->ups_info.eee) {
+             switch (tp->version) {
+             case RTL_VER_17_QFN68:
+                     if (tp->eee_adv & MDIO_EEE_10GT)
+                             ups_flags |= UPS_FLAGS_EN_10G_EEE;
+                     fallthrough;
+             case RTL_VER_16:

[Severity: Medium]

Should RTL_VER_17_QFN100 be listed here as well?

No, VER_17_QFN100 doesn't support UPS.

In rtl_ops_init() the two RTL8159 packages share one identical block:

        case RTL_VER_17_QFN68:
        case RTL_VER_17_QFN100:
                tp->eee_en              = true;
                tp->eee_adv             = MDIO_EEE_100TX |
MDIO_EEE_1000T | MDIO_EEE_10GT;
                tp->eee_adv2            = MDIO_EEE_2_5GT |
MDIO_EEE_5GT;
                ops->init               = r8159_init;
                ...
                ops->hw_phy_cfg         = r8159_hw_phy_cfg;
                ops->autosuspend_en     = rtl8157_runtime_enable;

So QFN100 advertises the same 100M through 10G EEE capabilities, but with
only RTL_VER_17_QFN68 matched above it takes the default arm and gets the
old UPS_FLAGS_EN_EEE encoding that this patch replaces for this chip
generation.

quoted

+                     if (tp->eee_adv & MDIO_EEE_100TX)
+                             ups_flags |= UPS_FLAGS_EN_100M_EEE;
+                     if (tp->eee_adv & MDIO_EEE_1000T)
+                             ups_flags |=

UPS_FLAGS_EN_1000M_EEE;

quoted

+                     if (tp->eee_adv2 & MDIO_EEE_2_5GT)
+                             ups_flags |=

UPS_FLAGS_EN_2500M_EEE;

quoted

+                     if (tp->eee_adv2 & MDIO_EEE_5GT)
+                             ups_flags |=

UPS_FLAGS_EN_5000M_EEE;

quoted

+                     break;
+             default:
+                     ups_flags |= UPS_FLAGS_EN_EEE;
+                     break;
+             }
+     }

      if (tp->ups_info.flow_control)
              ups_flags |= UPS_FLAGS_EN_FLOW_CTR; @@ -4230,20

+4255,33

quoted

@@ static void r8156_ups_flags(struct r8152 *tp)
      case NWAY_2500M_FULL:
              ups_flags |= ups_flags_speed(9);
              break;
+     case NWAY_5000M_FULL:
+             ups_flags |= ups_flags_speed(10);
+             break;
+     case NWAY_10000M_FULL:
+             ups_flags |= ups_flags_speed(11);
+             break;
      default:
              break;
      }

-     switch (tp->ups_info.lite_mode) {
-     case 1:
-             ups_flags |= 0 << 5;
-             break;
-     case 2:
-             ups_flags |= 2 << 5;
+     switch (tp->version) {
+     case RTL_VER_16:
+     case RTL_VER_17_QFN68:
              break;
-     case 0:
      default:
-             ups_flags |= 1 << 5;

[Severity: Medium]

Same question for the lite_mode suppression: RTL_VER_17_QFN100 is not
listed, so it still gets the bit 5/6 lite_mode field that this hunk treats as not
applicable to this generation.

quoted

+             switch (tp->ups_info.lite_mode) {
+             case 1:
+                     ups_flags |= 0 << 5;
+                     break;
+             case 2:
+                     ups_flags |= 2 << 5;
+                     break;
+             case 0:
+             default:
+                     ups_flags |= 1 << 5;
+                     break;
+             }
              break;
      }

[ ... ]

quoted

@@ -4573,9 +4640,28 @@ static void rtl8157_runtime_enable(struct r8152

*tp, bool enable)

quoted

              r8153b_u1u2en(tp, false);
              r8157_u2p3en(tp, false);
              rtl_runtime_suspend_enable(tp, true);
+
+             switch (tp->version) {
+             case RTL_VER_16:
+             case RTL_VER_17_QFN68:
+                     r8157_ups_en(tp, true);
+                     break;
+             default:
+                     break;
+             }

[Severity: Medium]

Does this leave RTL_VER_17_QFN100 without UPS entirely?

rtl_ops_init() assigns ops->autosuspend_en = rtl8157_runtime_enable for both
RTL_VER_17_QFN68 and RTL_VER_17_QFN100, and __rtl_get_pkg_det()
results of 0x18/0x1a select RTL_VER_17_QFN100. On runtime suspend the
chain is:

        rtl8152_runtime_suspend()
          -> tp->rtl_ops.autosuspend_en(tp, true)
             == rtl8157_runtime_enable(tp, true)
                -> switch (tp->version) ... default: break;

so r8157_ups_en() is never reached for that package, while the commit
message says "support UPS for RTL8157 and RTL8159".

quoted

      } else {
              r8153_queue_wake(tp, false);
              rtl_runtime_suspend_enable(tp, false);
+
+             switch (tp->version) {
+             case RTL_VER_16:
+             case RTL_VER_17_QFN68:
+                     r8157_ups_en(tp, false);
+                     break;
+             default:
+                     break;
+             }

[Severity: Medium]

Mirror of the above on the resume path.

Note that r8159_init(), shared by both RTL8159 packages, was converted in this
same patch:

        r8157_power_cut_en(tp, false);
        r8157_ups_en(tp, false);

so the QFN100 path does get the new disable call at probe/init time, but never
the enable/disable dispatch during runtime suspend and resume.

There is a QFN68-only quirk elsewhere in r8159_init() ("Enable u2phy backup
restore patch"), so a package-specific restriction is plausible.
Should case RTL_VER_17_QFN100 be added to all four new switches, or
should the commit message be scoped to the QFN68 package with a note on
the hardware difference?

quoted

              r8157_u2p3en(tp, true);
              if (tp->udev->speed >= USB_SPEED_SUPER)
                      r8153b_u1u2en(tp, true);

[ ... ]

quoted

@@ -9130,7 +9216,7 @@ static void r8159_init(struct r8152 *tp)
      ocp_write_word(tp, MCU_TYPE_USB, USB_U1U2_TIMER, 500);

      r8157_power_cut_en(tp, false);
-     r8156_ups_en(tp, false);
+     r8157_ups_en(tp, false);
      r8153_queue_wake(tp, false);
      rtl_runtime_suspend_enable(tp, false);

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

Best,
Chih-Kai