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

[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
> 
> - [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.
> 
> > 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
> 
> [ ... ]
> 
> > @@ -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.
> 
> > +                     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;
> > +                     if (tp->eee_adv2 & MDIO_EEE_2_5GT)
> > +                             ups_flags |=
> UPS_FLAGS_EN_2500M_EEE;
> > +                     if (tp->eee_adv2 & MDIO_EEE_5GT)
> > +                             ups_flags |=
> UPS_FLAGS_EN_5000M_EEE;
> > +                     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
> > @@ 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.
> 
> > +             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;
> >       }
> >
> 
> [ ... ]
> 
> > @@ -4573,9 +4640,28 @@ static void rtl8157_runtime_enable(struct r8152
> *tp, bool enable)
> >               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".
> 
> >       } 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?
> 
> >               r8157_u2p3en(tp, true);
> >               if (tp->udev->speed >= USB_SPEED_SUPER)
> >                       r8153b_u1u2en(tp, true);
> 
> [ ... ]
> 
> > @@ -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