Re: [PATCH net-next v8 06/11] r8152: add new init writes for RTL8156/8156B/8157/8159

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 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] r8156_init() (RTL_TEST_01/VER_10/11/12/13/15) and r8157_init()
  (VER_16/17) now always clear RMT_WAKE_EN (BIT(0) of PLA 0xe0c8).
- [Low] The commit message lists each new register write but explains
  only one of them (RX_END_TRANSFER_EN).

--

r8152: add new init writes for RTL8156/8156B/8157/8159

This commit adds register writes to r8156_init() and r8157_init() so they
match the vendor init sequence. Both functions now clear PREBOOT_OPTION and
RMT_WAKE_EN, and RTL8156B gets a u2phy backup. r8157_init() also disables
interrupt mitigation and AUTO_SPEEDUP, clears the reset event flag, adds
VER_17 specific writes, and clears RX_END_TRANSFER_EN for all chips.

> Bring the per-generation init functions in line with the vendor init
> sequence by adding writes the shared r8156_init() never performed.

[Severity: Low]
This isn't a bug, but could the commit message say why most of these writes
are needed? Only RX_END_TRANSFER_EN is explained. The rest are listed with
no stated purpose or effect:

  - clearing PREBOOT_OPTION in PLA_INDICATE_FALG
  - clearing RMT_WAKE_EN in PLA 0xe0c8
  - the r8156b_u2phy_backup() values written to USB 0xd3ce..0xd3dc
  - clearing USB 0xcf04 bits 0/1/2/7 (interrupt mitigation)
  - clearing USB 0xcd06 BIT(11) (Warm RST / Bus RST event flag)
  - clearing USB 0xb9a6 BIT(0) and writing 0x0448 to 0xb9a2 on VER_17
  - clearing AUTO_SPEEDUP

Several of these look like fixes for specific failures. Examples would be
problems after a warm or bus reset, after a USB2/USB3 PHY power cut, or
with interrupt mitigation enabled. Other patches in the series are marked
as fixes.

Is there a user-visible symptom behind any of these writes? If so, could
it be described, with Fixes: tags added? That would show whether this
belongs in net and stable rather than net-next.

Could the message also say how clearing RMT_WAKE_EN relates to WoL and to
remote wakeup from runtime autosuspend?

> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index 364a0573f6cd1..a1c2e1e08b161 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c

[ ... ]

> @@ -8528,11 +8543,24 @@ static void r8156_init(struct r8152 *tp)

[ ... ]

>  	r8153b_power_cut_en(tp, false);
>  	r8156_ups_en(tp, false);
>  	r8153_queue_wake(tp, false);
>  	rtl_runtime_suspend_enable(tp, false);
>  
> +	ocp_byte_clr_bits(tp, MCU_TYPE_PLA, PLA_INDICATE_FALG, PREBOOT_OPTION);
> +
> +	ocp_byte_clr_bits(tp, MCU_TYPE_PLA, PLA_RMT_WAKE, RMT_WAKE_EN);

[Severity: Medium]
Does this leave RMT_WAKE_EN permanently off on these chips? r8156_init()
(TEST_01, VER_10/11/12/13/15) clears it every time, and so does
r8157_init() (VER_16/17). Nothing else in the driver uses PLA_RMT_WAKE or
RMT_WAKE_EN, so the bit is never set again.

init() runs at probe, right after the wake state is read:

drivers/net/usb/r8152.c:rtl8152_probe() {
    ...
	intf->needs_remote_wakeup = 1;

	if (!rtl_can_wakeup(tp))
		__rtl_set_wol(tp, 0);
	else
		tp->saved_wolopts = __rtl_get_wol(tp);

	tp->rtl_ops.init(tp);
    ...
}

It also runs again on every rtl8152_reset_resume()->tp->rtl_ops.init().

The WoL arming path, __rtl_set_wol(), only writes PLA_CONFIG34, PLA_CONFIG5
and PLA_CFG_WOL, then calls device_set_wakeup_enable(). The
runtime-suspend paths through rtl_runtime_suspend_enable() and
r8153_queue_wake() never touch PLA_RMT_WAKE either.

What happens if RMT_WAKE_EN controls whether the MAC can send a USB remote
wakeup? Then WoL from system suspend and resume from runtime autosuspend on
a link change would stop working on RTL8156/8156B/8157/8159. Software would
still report wake as armed through saved_wolopts and needs_remote_wakeup.

If the bit is only leftover state from a UEFI/PXE preboot driver, which
its placement next to PREBOOT_OPTION suggests, the clear is harmless.
Neither the code nor the changelog says which case applies. Can this be
confirmed?

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