RE: [PATCH net-next v4 3/8] r8152: split rtl8156_enable/up/down into per-chip-family functions

[Chih Kai Hsu <hsu.chih.kai@realtek.com>]

> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 3 · 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] For RTL8157/RTL8159 (RTL_VER_16, RTL_VER_17_QFN68,
>   RTL_VER_17_QFN100) the newly extracted rtl8157_up() / rtl8157_down()…
> - [Medium] The patch is presented as a refactor ("split
>   rtl8156_enable/up/down into per-chip-family functions") but it…
> - [Medium] Suspicious bitwise clear for Clear_SDR feature in rtl8157_up()
> 
> --
> 
> r8152: split rtl8156_enable/up/down into per-chip-family functions
> 
> This splits the version-guarded rtl8156_enable()/rtl8156_up()/rtl8156_down()
> into per-chip-family callbacks: rtl8156b_enable() for VER_12/13/15 and
> rtl8157_enable()/rtl8157_up()/rtl8157_down() for VER_16/17, while
> rtl8156_up()/rtl8156_down() keep serving VER_10..VER_15. Along the way
> several register writes that were previously version guarded become
> unconditional for one family or another.
> 
> > diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c index
> > 9ce072c38b5b5..c513c286c5499 100644
> > --- a/drivers/net/usb/r8152.c
> > +++ b/drivers/net/usb/r8152.c
> 
> [ ... ]
> 
> > @@ -6849,8 +6892,7 @@ static void rtl8156_up(struct r8152 *tp)
> >
> >       ocp_byte_clr_bits(tp, MCU_TYPE_PLA, PLA_OOB_CTRL,
> NOW_IS_OOB);
> >
> > -     if (tp->version >= RTL_VER_16)
> > -             ocp_word_clr_bits(tp, MCU_TYPE_PLA, PLA_RCR1, BIT(3));
> > +     ocp_word_clr_bits(tp, MCU_TYPE_PLA, PLA_RCR1, BIT(3));
> 
> [Severity: Medium]
> 
> Should the behaviour changes to already-supported chips be a separate patch
> from the mechanical split?
> 
> The subject describes a split, but this hunk starts clearing PLA_RCR1
> BIT(3) on every ifup for RTL8156/8156B, which previously only happened for
> VER_16 and later. The same patch also:
> 
> - adds a Clear_SDR write for VER_13/15 (USB 0xd3ca BIT(15)) in
>   rtl8156_up()
> - clears USB_SPEED_OPTION RG_PWRDN_EN|ALL_SPEED_OFF for
> RTL8157/8159 in
>   rtl8157_up(), which was excluded for them before
> - adds PLA_BDC_CR ALDPS_PROXY_MODE and PLA_OOB_CTRL
> DIS_MCU_CLROOB to OOB
>   entry in rtl8156_down() and rtl8157_down()
> - changes RTL8157/8159 OOB sizing to PLA_RMS=1526 and PLA_MTPS=10K in
>   rtl8157_down()
> 
> The commit message says:
> 
>   "Per the datasheet, this uncovers real register differences that the
>    old version guards had wrong or missing"
> 
> If the old guards were wrong, then shipping RTL8156/8156B dongles are
> mis-programmed today, in particular for the OOB/wake-on-LAN path after
> ifdown. Can the message name the observed symptom, add Fixes: tags for the
> commits that introduced the wrong guards (195aae321c82, fd3c7d080df5,
> ad2a55b63b41), and state which chips were tested?
> 
> Splitting the refactor from the register fixes would also let the fixes be
> backported and bisected on their own.

Ok, I will add a new patch to split them.

> 
> >
> >       ocp_word_clr_bits(tp, MCU_TYPE_PLA, PLA_SFF_STS_7,
> MCU_BORW_EN);
> >
> > @@ -6872,11 +6914,11 @@ static void rtl8156_up(struct r8152 *tp)
> >       ocp_word_w0w1(tp, MCU_TYPE_PLA, PLA_RXFIFO_FULL,
> RXFIFO_FULL_MASK,
> >                     0x08);
> >
> > -     ocp_word_clr_bits(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL3,
> PLA_MCU_SPDWN_EN);
> > +     ocp_word_clr_bits(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL3,
> > +                       PLA_MCU_SPDWN_EN);
> >
> > -     if (tp->version < RTL_VER_16)
> > -             ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_SPEED_OPTION,
> > -                               RG_PWRDN_EN | ALL_SPEED_OFF);
> > +     ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_SPEED_OPTION,
> > +                       RG_PWRDN_EN | ALL_SPEED_OFF);
> 
> [ ... ]
> 
> > @@ -6929,7 +6977,102 @@ static void rtl8156_down(struct r8152 *tp)
> 
> [ ... ]
> 
> > +static void rtl8157_up(struct r8152 *tp) {
> > +     if (test_bit(RTL8152_INACCESSIBLE, &tp->flags))
> > +             return;
> > +
> > +     r8153b_u1u2en(tp, false);
> > +     r8153_aldps_en(tp, false);
> 
> [ ... ]
> 
> > +     /* share FIFO settings */
> > +     ocp_word_w0w1(tp, MCU_TYPE_PLA, PLA_RXFIFO_FULL,
> RXFIFO_FULL_MASK,
> > +                   0x08);
> > +
> > +     ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_SPEED_OPTION,
> > +                       RG_PWRDN_EN | ALL_SPEED_OFF);
> 
> [Severity: Medium]
> 
> Is dropping the PLA_MCU_SPDWN_EN programming intentional for
> RTL8157/RTL8159?

Yes, RTL8157/RTL8159 don't need to toggle this bit upon up/down.

> 
> Before the split, VER_16/VER_17 ran rtl8156_up(), which cleared the bit
> unconditionally at exactly this spot in the sequence:
> 
>         /* share FIFO settings */
>         ocp_word_w0w1(tp, MCU_TYPE_PLA, PLA_RXFIFO_FULL,
> RXFIFO_FULL_MASK,
>                       0x08);
> 
>         ocp_word_clr_bits(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL3,
> PLA_MCU_SPDWN_EN);
> 
>         if (tp->version < RTL_VER_16)
>                 ocp_word_clr_bits(tp, MCU_TYPE_USB,
> USB_SPEED_OPTION,
>                                   RG_PWRDN_EN | ALL_SPEED_OFF);
> 
> and the old rtl8156_down() had
> 
>         ocp_word_set_bits(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL3,
>                           PLA_MCU_SPDWN_EN);
> 
> as its first register write for these chips. Neither rtl8157_up() nor
> rtl8157_down() touches PLA_MAC_PWR_CTRL3.
> 
> Grepping PLA_MCU_SPDWN_EN after the patch leaves only
> rtl8153b_up/down, rtl8153c_up, rtl8156_up/down, r8153b_init, r8156_init
> and r8156b_init.
> r8157_init() and r8159_init() do not clear it either, and r8156_mac_clk_spd(),
> which r8157_init() does call, only writes PLA_MAC_PWR_CTRL and
> PLA_MAC_PWR_CTRL2.
> 
> So for VER_16 and both VER_17 variants the MCU speed-down enable is now
> never written: while the interface is up it keeps whatever the ROM/OOB
> firmware or a previous driver instance left, and the set on OOB entry is gone.
> The commit message enumerates the other register deltas but not this one.
> Was the paired clear-on-up / set-on-down meant to be carried over into
> rtl8157_up()/rtl8157_down()?
> 
> The omission is also still present at the end of the series
> (eac8e24403901016050a115a3102a0c0b46ddbbd), where the later added
> rtl8157_unload() does not add it back.
> 
> > +
> > +     ocp_write_dword(tp, MCU_TYPE_USB, USB_RX_BUF_TH,
> 0x00600400);
> > +
> > +     if (tp->saved_wolopts != __rtl_get_wol(tp)) {
> > +             netif_warn(tp, ifup, tp->netdev, "wol setting is changed\n");
> > +             __rtl_set_wol(tp, tp->saved_wolopts);
> > +     }
> > +
> > +     r8153_aldps_en(tp, true);
> > +
> > +     /* Clear_SDR */
> > +     ocp_byte_set_bits(tp, MCU_TYPE_USB, 0xd378, BIT(7));
> > +     ocp_word_clr_bits(tp, MCU_TYPE_USB, 0xcd06, BIT(15)); }
> > +
> > +static void rtl8157_down(struct r8152 *tp) {
> > +     if (test_bit(RTL8152_INACCESSIBLE, &tp->flags)) {
> > +             rtl_drop_queued_tx(tp);
> > +             return;
> > +     }
> > +
> > +     r8153b_u1u2en(tp, false);
> > +     r8153_aldps_en(tp, false);
> 
> [Severity: Medium]
> Is the clear (rather than set) of USB 0xcd06 BIT(15) here what you meant?

Sure. While USB 0xd378[7] == 1 and USB 0xcd06[15] == 0, the SDR will be cleared automatically.

> 
> The commit message says RTL8157/8159 "gain their own Clear_SDR enables
> (USB 0xd378 BIT(7), USB 0xcd06 BIT(15))", and the comment above the two
> writes just says "Clear_SDR". The first write uses ocp_byte_set_bits() to set
> 0xd378 BIT(7), which matches "enable". The second uses
> ocp_word_clr_bits() to zero 0xcd06 BIT(15), which is the opposite direction,
> and it is the only Clear_SDR write in the series that clears rather than sets: the
> RTL8156B path added in this same patch does
> 
>         ocp_word_set_bits(tp, MCU_TYPE_USB, 0xd3ca, BIT(15));
> 
> for its Clear_SDR enable. If 0xcd06 BIT(15) is an active-low or disable-type bit,
> that would explain the asymmetry, but nothing in the code or the changelog
> says so, and a plain clr/set typo here would silently leave Clear_SDR off on
> RTL8157/8159 with no visible error.
> 
> Since these are bare magic addresses with no register defines, please either
> give the bit a name (a #define alongside the other USB_*
> registers) or extend the comment to state the polarity, e.g. that
> BIT(15) of 0xcd06 must be 0 to enable Clear_SDR. Can you confirm against the
> datasheet which of set/clear the hardware needs for that bit?

I will give the bit a name.

> 
> [ ... ]
> 
> > +     ocp_write_word(tp, MCU_TYPE_PLA, PLA_RMS, 1526);
> > +     ocp_write_byte(tp, MCU_TYPE_PLA, PLA_MTPS, 10 * 1024 / 64);
> 
> [ ... ]
> 
> > @@ -10090,10 +10233,10 @@ static int rtl_ops_init(struct r8152 *tp)
> >               tp->eee_adv             = MDIO_EEE_1000T |
> MDIO_EEE_100TX;
> >               tp->eee_adv2            = MDIO_EEE_2_5GT |
> MDIO_EEE_5GT;
> >               ops->init               = r8157_init;
> > -             ops->enable             = rtl8156_enable;
> > +             ops->enable             = rtl8157_enable;
> >               ops->disable            = rtl8153_disable;
> > -             ops->up                 = rtl8156_up;
> > -             ops->down               = rtl8156_down;
> > +             ops->up                 = rtl8157_up;
> > +             ops->down               = rtl8157_down;
> 
> [ ... ]
> 
> --
> Sashiko AI review ·
> https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260917070637.62827-
> 1-nic_swsd%40realtek.com

Best Regards,
Chih-Kai