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

[netdev-bot+sashiko@kernel.org]

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 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.

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