RE: [PATCH net-next v4 8/8] r8152: enable UPS for RTL8157 and RTL8159 QFN68

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

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

If you believe none of the issues are legitimate, please reply to each issue of
Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply (one
such reply per series is enough).

- [High] r8157_ups_en()'s UPS-exit (disable) path in
  drivers/net/usb/r8152.c starts reprogramming the chip as soon as it…
- [Medium] In rtl8157_runtime_enable()'s disable (resume) branch the new
  UPS exit is sequenced last instead of first: line 4722 calls…

--

r8152: enable UPS for RTL8157 and RTL8159 QFN68

This adds r8157_ups_en() and calls it from rtl8157_runtime_enable() for
RTL_VER_16 and RTL_VER_17_QFN68, so UPS is now toggled across runtime
suspend/resume for those chips.  It also extends r8156_ups_flags() with
per-speed EEE flags and two new ups_flags_speed() entries.

Two questions below about the new UPS exit path.

quoted

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

05b2b79ab3e31..ebe9cc9322b3c 100644

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

[ ... ]

quoted

@@ -4484,6 +4522,35 @@ static void r8156_ups_en(struct r8152 *tp, bool

enable)

quoted

      }
 }

+static void r8157_ups_en(struct r8152 *tp, bool enable) {
+     if (enable) {
+             r8156_ups_flags(tp);
+
+             ocp_byte_set_bits(tp, MCU_TYPE_USB, USB_POWER_CUT,
+                               UPS_EN | USP_PREWAKE |

PHASE2_EN);

quoted

+
+             ocp_byte_set_bits(tp, MCU_TYPE_USB, USB_MISC_2,
+                               UPS_FORCE_PWR_DOWN);
+     } else {
+             ocp_byte_clr_bits(tp, MCU_TYPE_USB, USB_POWER_CUT,
+                               UPS_EN | USP_PREWAKE);
+
+             ocp_byte_clr_bits(tp, MCU_TYPE_USB, USB_MISC_2,
+                               UPS_FORCE_PWR_DOWN);
+
+             if (ocp_read_word(tp, MCU_TYPE_USB, USB_MISC_0) &

PCUT_STATUS) {

quoted

+                     /* clear USB fw_ver_reg */
+                     ocp_write_byte(tp, MCU_TYPE_USB,

USB_FW_USE_VER,

quoted

+ 0);
+
+                     tp->rtl_ops.hw_phy_cfg(tp);
+
+                     rtl8152_set_speed(tp, tp->autoneg, tp->speed,
+                                       tp->duplex, tp->advertising);
+             }
+     }
+}

[Severity: High]
Should this PCUT_STATUS branch wait for autoload to complete before
reprogramming the PHY?

No, it shouldn't wait for autoload.

The two UPS helpers that are actually reached from a runtime resume after a
power cut, r8153b_ups_en() and r8153c_ups_en(), gate the same sequence on
wait_autoload_done():

        if (ocp_read_word(tp, MCU_TYPE_USB, USB_MISC_0) &
PCUT_STATUS) {
                if (wait_autoload_done(tp))
                        return;

                tp->rtl_ops.hw_phy_cfg(tp);

                rtl8152_set_speed(tp, tp->autoneg, tp->speed,
                                  tp->duplex, tp->advertising);
        }

r8156_ups_en(), from which the new helper was copied, has no such wait, but
its branch appears to be dead: both callers (r8156_init() and r8156b_init()) run
r8153b_power_cut_en(tp, false) first, which clears PCUT_STATUS.  With this
patch the unguarded branch becomes live via

  rtl8152_runtime_resume() -> tp->rtl_ops.autosuspend_en(tp, false)
    -> rtl8157_runtime_enable(tp, false) -> r8157_ups_en(tp, false)

and PCUT_STATUS is exactly the expected state there, because the suspend
side just ran r8157_ups_en(tp, true).  Can hw_phy_cfg() and the following
link renegotiation then run while the chip is still reloading its post-power-cut
configuration?

wait_autoload_done() also returns -ENODEV when RTL8152_INACCESSIBLE is
set, so the peer helpers bail out on a disconnect during resume.  Without
that check, does a disconnect here run the whole hw_phy_cfg() sequence and
then the BMCR_RESET poll in rtl8152_set_speed(), which can spin 50 *
msleep(20) on failing register reads?

Yes, it does. But it just wastes a little time.

For RTL_VER_17_QFN68 there is a second handshake that this path skips.
r8159_init() treats a missing backup-restore as fatal whenever PCUT_STATUS is
set:

        if (r8159_wait_backup_restore(tp)) {
                rtl_set_inaccessible(tp);
                dev_err(&tp->intf->dev,
                        "init failed, backup-restore timed out\n");
                return;
        }

r8159_wait_backup_restore() polls USB_GPHY_CTRL for BACKUP_RESTRORE
only when PCUT_STATUS is set.  On the new runtime resume path,
r8159_hw_phy_cfg() only calls r8156b_wait_loading_flash() and then clears
PCUT_STATUS before starting firmware/PHY programming, so nothing waits
for BACKUP_RESTRORE.  Is that handshake needed here too?

Yes, it is needed. I will add this in the next version.

quoted

 static void r8153_power_cut_en(struct r8152 *tp, bool enable)  {
      if (enable)

[ ... ]

quoted

@@ -4642,9 +4709,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;
+             }
      } 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]
Is the ordering here intentional?  In the resume branch the UPS exit runs
after the PLA writes, while the other UPS-capable variants exit UPS first.

Yes, it's intentional.

rtl8153b_runtime_enable():

        } else {
                r8153b_ups_en(tp, false);
                r8153_queue_wake(tp, false);
                rtl_runtime_suspend_enable(tp, false);

rtl8153c_runtime_enable() has the same order with r8153c_ups_en(tp, false)
first.

Here, r8153_queue_wake(tp, false) and rtl_runtime_suspend_enable(tp, false)
touch PLA registers (PLA_INDICATE_FALG/UPCOMING_RUNTIME_D3,
PLA_SUSPEND_FLAG, PLA_EXTRA_STATUS, and the CRWECR-gated
PLA_CONFIG34/LINK_OFF_WAKE_EN plus
__rtl_set_wol(tp->saved_wolopts)) while UPS_EN | USP_PREWAKE and
UPS_FORCE_PWR_DOWN are still set, and r8157_ups_en(tp, false) only clears
those bits afterwards.

Since r8153b_ups_en()/r8153c_ups_en() poll PLA_BOOT_CTRL for
AUTOLOAD_DONE right after clearing the same two bits, the PLA block seems
to still be reloading its defaults at that point.  Can the WOL settings restored
from
tp->saved_wolopts and the cleared UPCOMING_RUNTIME_D3 flag be
tp->overwritten by
that reload, leaving the device unable to wake or with link-off-wake still armed?
The new helper has no AUTOLOAD_DONE gate either, so nothing in this
resume sequence waits for the reload.

quoted

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

[ ... ]

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

Best Regards,
Chih-Kai