RE: [PATCH net-next v4 1/8] r8152: split r8156_init per chip and add missing init writes

[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: 2 (1) · Low: 1
> 
> 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] Undocumented hardware behaviour change during the split:
>   r8157_init() (installed as ops->init for RTL_VER_16 in rtl_ops_init())…
> - [Medium] Silent reordering of hardware handshakes for RTL8157/RTL8159.
> - [Low] Commit message vs. code: the message names the FC-timer block
>   ("fenced to RTL_VER_12 <= version <= RTL_VER_15") as the example of a…
> 
> Pre-existing issues:
> - [Medium] An RTL8159 backup-restore timeout marks the device
>   inaccessible but cannot report the failure to its caller: r8159_init()…
> 
> --
> 
> r8152: split r8156_init per chip and add missing init writes
> 
> This splits the shared r8156_init() into r8156_init(), r8156b_init(),
> r8157_init() and r8159_init(), factors the AUTOLOAD_DONE polling loops into
> wait_autoload_done(), and adds a number of new OCP writes to the per-chip
> init sequences.  A few observations below.
> 
> First, about the motivation in the changelog:
> 
> > RTL8156, RTL8156B, RTL8157 and RTL8159 shared a single r8156_init(),
> > gated by a dense set of tp->version range checks. That made the init
> > sequence hard to follow, and some checks (e.g. the FC-timer block,
> > which was fenced to "RTL_VER_12 <= version <= RTL_VER_15") silently
> > skipped RTL8156 (VER_10/11) and RTL8157/RTL8159 (VER_16/17)
> altogether
> > even though nothing chip-specific justified the gap.
> 
> [Severity: Low]
> 
> This isn't a bug, but after the split the FC-timer block named here still exists
> only in r8156b_init().  The PLA_RCR SLOT_EN clear, PLA_CPCR
> FLOW_CTRL_EN, USB_FC_TIMER, FLOW_CTRL_PATCH_2 and USB_FW_TASK
> FC_PATCH_TASK writes are absent from r8156_init() (VER_10/11), r8157_init()
> (VER_16) and
> r8159_init() (VER_17), i.e. the exact gap the paragraph complains about is
> unchanged by this patch, and the "While splitting:" list does not claim to close
> it.
> 
> The gap is closed later in the series by "r8152: extract
> rtl_fc_pause_pkt_en() and apply it to RTL8156/8157/8159", which factors the
> block into rtl_fc_pause_pkt_en() with cases for RTL_VER_10/11,
> RTL_VER_12/13/15 and RTL_VER_16/17 and calls it from r8156_init(),
> r8156b_init() and rtl8157_enable().
> 
> Could this paragraph say that the FC-timer gap is addressed by a later patch in
> the series rather than by this one?

Sure. Will do.

> 
> > diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c index
> > af3d7dcb2f141..00b0fda306c30 100644
> > --- a/drivers/net/usb/r8152.c
> > +++ b/drivers/net/usb/r8152.c
> 
> [ ... ]
> 
> > @@ -542,6 +547,7 @@
> >  #define RX_AGG_DISABLE               0x0010
> >  #define RX_ZERO_EN           0x0080
> >  #define RX_DESC_16B          0x0400
> > +#define RX_END_TRANSFER_EN   BIT(11)
> >
> >  /* USB_U2P3_CTRL */
> >  #define U2P3_ENABLE          0x0001
> 
> [ ... ]
> 
> > @@ -8330,90 +8311,132 @@ static void r8157_hw_phy_cfg(struct r8152
> > *tp)
> 
> [ ... ]
> 
> >       switch (tp->version) {
> >       case RTL_VER_13:
> >       case RTL_VER_15:
> > -     case RTL_VER_16:
> > -     case RTL_VER_17:
> >               r8156b_wait_loading_flash(tp);
> >               break;
> >       default:
> >               break;
> >       }
> >
> > -     for (i = 0; i < 500; i++) {
> > -             if (ocp_read_word(tp, MCU_TYPE_PLA, PLA_BOOT_CTRL) &
> > -                 AUTOLOAD_DONE)
> > -                     break;
> > -
> > -             msleep(20);
> > -             if (test_bit(RTL8152_INACCESSIBLE, &tp->flags))
> > -                     return;
> > -     }
> > -
> > -     if (tp->version == RTL_VER_17 && r8159_wait_backup_restore(tp)) {
> > -             rtl_set_inaccessible(tp);
> > -             dev_err(&tp->intf->dev, "init failed, backup-restore timed
> out\n");
> > +     if (wait_autoload_done(tp))
> >               return;
> > -     }
> 
> [ ... ]
> 
> > @@ -8421,73 +8444,261 @@ static void r8156_init(struct r8152 *tp)
> 
> [ ... ]
> 
> >       set_bit(GREEN_ETHERNET, &tp->flags);
> >
> > -     /* RX aggregation / 16 bytes RX descriptor
> > -      * BIT(11) is specific to RTL8159, with unknown meaning
> > -      */
> > -     if (tp->version == RTL_VER_17)
> > -             ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_USB_CTRL,
> > -                               RX_AGG_DISABLE | RX_DESC_16B |
> BIT(11));
> > -     else if (tp->version == RTL_VER_16)
> > -             ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_USB_CTRL,
> RX_AGG_DISABLE | RX_DESC_16B);
> > +     ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_USB_CTRL,
> > +                       RX_AGG_DISABLE | RX_ZERO_EN);
> 
> [ ... ]
> 
> > +static void r8157_init(struct r8152 *tp) {
> > +     u16 data;
> > +
> > +     if (test_bit(RTL8152_INACCESSIBLE, &tp->flags))
> > +             return;
> > +
> > +     /* Enable SW reset */
> > +     ocp_byte_set_bits(tp, MCU_TYPE_USB, 0xcffe, BIT(3));
> > +     ocp_byte_clr_bits(tp, MCU_TYPE_USB, 0xd3ca, BIT(0));
> > +
> > +     ocp_byte_clr_bits(tp, MCU_TYPE_USB, USB_ECM_OP,
> EN_ALL_SPEED);
> > +
> > +     ocp_word_set_bits(tp, MCU_TYPE_USB, USB_ECM_OPTION,
> > + BYPASS_MAC_RESET);
> > +
> > +     r8153b_u1u2en(tp, false);
> > +
> > +     if (wait_autoload_done(tp))
> > +             return;
> > +
> > +     r8156b_wait_loading_flash(tp);
> 
> [Severity: Medium]
> 
> Is this reordering deliberate?  In the shared r8156_init() the flash-patch wait
> for VER_16/17 ran right after r8153b_u1u2en(tp, false) and before the
> AUTOLOAD_DONE poll:
> 
>         switch (tp->version) {
>         case RTL_VER_13:
>         case RTL_VER_15:
>         case RTL_VER_16:
>         case RTL_VER_17:
>                 r8156b_wait_loading_flash(tp);
>                 break;
>         ...
>         for (i = 0; i < 500; i++) {
>                 if (ocp_read_word(tp, MCU_TYPE_PLA, PLA_BOOT_CTRL)
> &
>                     AUTOLOAD_DONE)
>                         break;
> 
> r8156b_init() keeps that pre-poll placement for VER_13/15, so only the
> VER_16 and VER_17 paths change order here.
> 
> r8159_init() moves r8156b_wait_loading_flash() after
> r8159_wait_backup_restore() as well.  Both helpers poll USB_GPHY_CTRL
> (GPHY_PATCH_DONE versus BACKUP_RESTRORE), and the backup-restore poll
> has a
> 20 ms budget:
> 
>         return poll_timeout_us(ocp_data = ocp_read_word(tp,
> MCU_TYPE_USB, USB_GPHY_CTRL),
>                                ocp_data & BACKUP_RESTRORE, 200,
> 20000, false);
> 
> while r8156b_wait_loading_flash() allows roughly 100 ms for the patch load.
> If a pending flash load can delay BACKUP_RESTRORE, can this poll now time
> out where it previously could not, given that the flash wait used to complete
> first?  On timeout r8159_init() calls rtl_set_inaccessible() and abandons the
> rest of the bring-up.

The order doesn't matter.

> 
> > +
> > +     data = r8153_phy_status(tp, 0);
> > +     if (data == PHY_STAT_EXT_INIT) {
> > +             ocp_reg_clr_bits(tp, 0xa468, BIT(3) | BIT(1));
> > +             ocp_reg_clr_bits(tp, 0xa466, BIT(0));
> > +     }
> 
> [ ... ]
> 
> > +     set_bit(GREEN_ETHERNET, &tp->flags);
> >
> > +     /* RX aggregation / 16 bytes RX descriptor / Bulk In End transfer */
> > +     ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_USB_CTRL,
> > +                       RX_AGG_DISABLE | RX_DESC_16B |
> > + RX_END_TRANSFER_EN);
> 
> [Severity: Medium]
> 
> Should r8157_init() clear USB_USB_CTRL bit 11?  Before the split that bit
> was restricted to RTL_VER_17, with a comment saying it was RTL8159
> specific:
> 
>         /* RX aggregation / 16 bytes RX descriptor
>          * BIT(11) is specific to RTL8159, with unknown meaning
>          */
>         if (tp->version == RTL_VER_17)
>                 ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_USB_CTRL,
>                                   RX_AGG_DISABLE | RX_DESC_16B |
> BIT(11));
>         else if (tp->version == RTL_VER_16)
>                 ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_USB_CTRL,
> RX_AGG_DISABLE | RX_DESC_16B);
> 
> rtl_ops_init() installs r8157_init() as ops->init for RTL_VER_16, so every
> RTL8157 now has bit 11 (newly named RX_END_TRANSFER_EN) cleared on
> each probe and reset_resume.
> 
> Every other per-version divergence of the old function was reproduced exactly
> in the split (USB_SPEED_OPTION for versions below VER_16, RX_DETECT8 for
> VER_12 to VER_15, PLA_MCU_SPDWN_EN for versions below VER_16,
> ACT_ODMA for versions below VER_12, r8153_u2p3en() versus
> r8157_u2p3en(),
> r8153b_power_cut_en() versus r8157_power_cut_en()), and the changelog
> enumerates each intentionally added write without mentioning this one.
> 
> Can the changelog describe what bit 11 does on RTL8157, or keep the write
> limited to RTL8159 as before?

RTL8157 also needs to clear bit 11. While set, incoming packets are buffered in the RX FIFO and
Bulk-IN transfers are suspended. This bit is NOT self-clearing, so the driver must manually clear it 
to resume Bulk-IN transfers.

I will mention this in the next version.

> 
> [ ... ]
> 
> > +static int r8159_wait_backup_restore(struct r8152 *tp) {
> > +     u32 ocp_data;
> > +
> > +     ocp_data = ocp_read_word(tp, MCU_TYPE_USB, USB_MISC_0);
> > +     if (!(ocp_data & PCUT_STATUS))
> > +             return 0;
> > +
> > +     return poll_timeout_us(ocp_data = ocp_read_word(tp,
> MCU_TYPE_USB, USB_GPHY_CTRL),
> > +                            ocp_data & BACKUP_RESTRORE, 200,
> 20000,
> > +false); }
> > +
> > +static void r8159_init(struct r8152 *tp) {
> 
> [ ... ]
> 
> > +     if (wait_autoload_done(tp))
> > +             return;
> > +
> > +     if (r8159_wait_backup_restore(tp)) {
> > +             rtl_set_inaccessible(tp);
> > +             dev_err(&tp->intf->dev,
> > +                     "init failed, backup-restore timed out\n");
> > +             return;
> >       }
> 
> [Severity: Medium]
> 
> This is a pre-existing issue and not introduced by this patch, since the same
> abort path was in the shared r8156_init(), but as it is being moved
> here: how does this failure reach the caller?
> 
> rtl_set_inaccessible() only sets one flag:
> 
>         static void rtl_set_inaccessible(struct r8152 *tp)
>         {
>                 set_bit(RTL8152_INACCESSIBLE, &tp->flags);
>                 smp_mb__after_atomic();
>         }
> 
> PROBE_SHOULD_RETRY is set only by r8152_control_msg() on a failed control
> transfer, which a BACKUP_RESTRORE poll timeout is not, and
> rtl8152_probe_once() checks only that flag after calling
> tp->rtl_ops.init(tp):
> 
>         if (test_bit(PROBE_SHOULD_RETRY, &tp->flags))
>                 goto out2;
> 
> So probe continues through register_netdev() and returns success while the
> rest of r8159_init() was skipped and every subsequent
> generic_ocp_read()/generic_ocp_write() short-circuits to -ENODEV, leaving a
> registered netdev that cannot pass traffic until re-plug.  Would it make sense
> for this path to propagate an error or queue a device reset?
> 
> [ ... ]
> 
> --
> Sashiko AI review ·
> https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260917070637.62827-
> 1-nic_swsd%40realtek.com

Best Regards,
Chih-Kai