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

[Chih Kai Hsu]

> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · 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).
> 
> - [Medium] If the new backup-restore poll in r8157_ups_en(tp, false)
>   times out during runtime resume on RTL_VER_16 or RTL_VER_17_QFN68,
> the…
> 
> --
> 
> r8152: enable UPS for RTL8157 and RTL8159 QFN68
> 
> This patch adds r8157_ups_en() and calls it from rtl8157_runtime_enable() for
> RTL_VER_16 and RTL_VER_17_QFN68. When UPS is disabled and PCUT_STATUS
> is set, the new disable path polls for backup-restore to finish. It also extends
> r8156_ups_flags() with per-speed EEE flags and the 5G/10G speed encodings.
> 
> > diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c index
> > b2e4b071d84f0..baac964fea80f 100644
> > --- a/drivers/net/usb/r8152.c
> > +++ b/drivers/net/usb/r8152.c
> 
> [ ... ]
> 
> > @@ -4486,6 +4524,49 @@ static void r8156_ups_en(struct r8152 *tp, bool
> enable)
> >       }
> >  }
> >
> > +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);
> > +
> > +             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) {
> > +                     u32 ocp_data;
> > +                     int ret;
> > +
> > +                     ret = read_poll_timeout(ocp_read_word,
> ocp_data,
> > +                                             ocp_data &
> BACKUP_RESTRORE, 200,
> > +                                             20000, false, tp,
> MCU_TYPE_USB,
> > +                                             USB_GPHY_CTRL);
> > +                     if (ret) {
> > +                             rtl_set_inaccessible(tp);
> > +                             dev_err(&tp->intf->dev,
> > +                                     "backup-restore timed out\n");
> > +                             return;
> > +                     }
> 
> [Severity: Medium]
> If this read_poll_timeout() times out during a runtime resume on
> RTL_VER_16 or RTL_VER_17_QFN68, does anything recover the device?
> 
> The path looks like this:
> 
> rtl8152_resume()
>   rtl8152_runtime_resume()
>     tp->rtl_ops.autosuspend_en(tp, false)
>       rtl8157_runtime_enable(tp, false)
>         r8157_ups_en(tp, false)
>           rtl_set_inaccessible(tp)
> 
> r8157_ups_en() returns void, so rtl8152_runtime_resume() keeps going and
> returns 0. The reset fallback in rtl8152_resume() skips the runtime case:
> 
>         if (!runtime_resume && test_bit(RTL8152_INACCESSIBLE, &tp->flags))
>                 usb_reset_device(tp->udev);
> 
> The register reads in the poll succeeded, so r8152_control_msg() never queued
> a reset. After that, every access returns early before
> r8152_control_msg() gets to its reset logic:
> 
>         if (test_bit(RTL8152_INACCESSIBLE, &tp->flags))
>                 return -ENODEV;
> 
> r8152_submit_rx(), the interrupt callback and bottom_half() also return early
> while the flag is set.
> 
> Would the interface then stay unusable until some unrelated reset, such as
> rtl8152_tx_timeout() when TX traffic stalls, or a replug? An idle interface would
> get neither.
> 
> Before this patch these chips never entered UPS on runtime suspend, so this
> resume failure looks new.
> 
> Could this path queue a reset, or otherwise pass the error back, in the runtime
> resume case?

Will do.

> 
> > +
> > +                     /* clear USB fw_ver_reg */
> > +                     ocp_write_byte(tp, MCU_TYPE_USB,
> USB_FW_USB_VER,
> > + 0);
> > +
> > +                     tp->rtl_ops.hw_phy_cfg(tp);
> > +
> > +                     rtl8152_set_speed(tp, tp->autoneg, tp->speed,
> > +                                       tp->duplex, tp->advertising);
> > +             }
> > +     }
> > +}
> 
> [ ... ]
> 
> > @@ -4644,9 +4725,28 @@ static void rtl8157_runtime_enable(struct r8152
> > *tp, bool enable)
> 
> [ ... ]
> 
> >       } 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;
> > +             }
> > +
> >               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/20261001091144.28191-1-
> nic_swsd%40realtek.com

Best Regards,
Chih-Kai