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

[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] This patch is presented as splitting r8156_init() per chip,
>   but it also changes hardware behaviour for all four chip families, and…
> 
> --
> 
> r8152: split r8156_init per chip and add missing init writes
> 
> This commit splits the shared r8156_init() into separate init functions for
> RTL8156, RTL8156B, RTL8157 and RTL8159. It also moves the
> AUTOLOAD_DONE polling into a new wait_autoload_done() helper. While
> splitting, it adds several new register writes to the per-chip init paths, including
> clearing RX_END_TRANSFER_EN on RTL8157.
> 
> > diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c index
> > af3d7dcb2f141..4d8f0815d79d1 100644
> > --- a/drivers/net/usb/r8152.c
> > +++ b/drivers/net/usb/r8152.c
> 
> [ ... ]
> 
> > @@ -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) {
> 
> [ ... ]
> 
> > +     /* 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]
> Is this a functional fix for RTL8157 that should go in as its own patch with a
> Fixes: tag?
> 
> Before this patch, the RTL_VER_16 branch in r8156_init() cleared only
> RX_AGG_DISABLE | RX_DESC_16B. BIT(11) was cleared only for RTL_VER_17,
> and the comment said its meaning was unknown. r8157_init() now clears
> RX_END_TRANSFER_EN as well.
> 
> The commit message says that while this bit is set, "Bulk-in transfers are
> suspended", and that "the driver must manually clear it to resume Bulk-in
> transfers".
> 
> That sounds like an RX stall on RTL8157 that goes back to commit
> fd3c7d080df5 ("r8152: Add support for the RTL8157 hardware"). This fix has
> no Fixes: tag and is part of a refactor touching about 490 lines, so it seems
> unlikely to reach net or stable.
> 
> Later patches in this series do carry Fixes: tags: "r8152: fix up and down
> register settings for RTL8156/8156B/8157/8159" and "r8152: add
> rtl8157_unload and rtl8157_change_mtu".
> 
> Could this change be sent as a small standalone fix ahead of the split?

The hardware uses this bit to preserve packet integrity across runtime
suspend/resume, and it is not self-clearing. However, the driver doesn't
use this bit now; this is a defensive clearing.

> 
> The other new hardware writes raise the same question. The commit message
> lists them, but it doesn't say what problem or symptom each one
> fixes:
> 
>     PREBOOT_OPTION and RMT_WAKE_EN, cleared in all four init functions
>     r8156b_u2phy_backup() writing 0xd3ce/0xd3d0/0xd3d4/0xd3d8/0xd3dc
>     0xcf04 interrupt mitigation disable in r8157_init()/r8159_init()
>     0xcd06 BIT(11) in r8157_init()/r8159_init()
>     0xb9a6 BIT(0) and 0xb9a2 = 0x0448 in r8159_init()
> 
> Could these be split out, or have their purpose explained? If one of them
> causes a regression, a bisect would land on this large mixed commit.
> 
> r8156b_init() also moves the AUTO_SPEEDUP clear. It used to be part of the
> USB_FW_CTRL read-modify-write that ran after usb_enable_lpm() and the
> flow control setup:
> 
>     ocp_data = ocp_read_word(tp, MCU_TYPE_USB, USB_FW_CTRL);
>     ...
>     ocp_data &= ~AUTO_SPEEDUP;
>     ocp_write_word(tp, MCU_TYPE_USB, USB_FW_CTRL, ocp_data);
> 
> Now it runs right after r8153_u2p3en():
> 
>     r8153_u2p3en(tp, false);
> 
>     /* Disable Auto Speed up */
>     ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_FW_CTRL, AUTO_SPEEDUP);
> 
> That is before the MSC timer, r8156b_u2phy_backup(), power-cut, UPS and
> LPM setup. Is this reordering intended for RTL8156B? The commit message
> doesn't mention it.

Will fix.

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

Best Regards,
Chih-Kai