Re: [PATCH net-next v8 11/11] r8152: enable UPS for RTL8157 and RTL8159 QFN68

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 3 · 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] r8157_ups_en(tp, false) in drivers/net/usb/r8152.c runs the
  BACKUP_RESTRORE wait on RTL8157 (RTL_VER_16) as well as on RTL8159.
- [Medium] If the backup-restore wait in r8157_ups_en(tp, false) times
  out during runtime resume, the device is marked inaccessible and…
- [Medium] After a UPS power cut (PCUT_STATUS set), r8157_ups_en(tp,
  false) waits only for BACKUP_RESTRORE.
- [Low] r8157_ups_en() clears the USB MAC firmware version by writing 0
  to a hardcoded register, USB_FW_USB_VER (0xcfd7), and then calls…

--

r8152: enable UPS for RTL8157 and RTL8159 QFN68

This commit adds r8157_ups_en() and calls it from rtl8157_runtime_enable()
for RTL_VER_16 and RTL_VER_17_QFN68, so UPS is toggled on runtime suspend
and resume, and uses it in r8157_init(). It also extends r8156_ups_flags()
with per-speed EEE flags and 5G/10G speed encodings for these chips. On a
power cut, the disable path waits for BACKUP_RESTRORE, clears
USB_FW_USB_VER, and reapplies the PHY config and speed.

> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index 703ec5483778b..9c7031f938bc2 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c

[ ... ]

> @@ -4496,6 +4534,58 @@ static void r8156_ups_en(struct r8152 *tp, bool enable)
>  	}
>  }
>  
> +static void r8157_ups_en(struct r8152 *tp, bool enable)
> +{

[ ... ]

> +	} else {
> +		ocp_byte_clr_bits(tp, MCU_TYPE_USB, USB_POWER_CUT,
> +				  UPS_EN | USP_PREWAKE);
> +
> +		ocp_byte_clr_bits(tp, MCU_TYPE_USB, USB_MISC_2,
> +				  UPS_FORCE_PWR_DOWN);
> +
> +		ocp_data = ocp_read_word(tp, MCU_TYPE_USB, USB_MISC_0);
> +		if (test_bit(RTL8152_INACCESSIBLE, &tp->flags))
> +			return;
> +
> +		if (ocp_data & PCUT_STATUS) {
> +			ret = read_poll_timeout(ocp_read_word, ocp_data,
> +						test_bit(RTL8152_INACCESSIBLE,
> +							 &tp->flags) ||
> +						(ocp_data & BACKUP_RESTRORE),
> +						200, 20000, false, tp,
> +						MCU_TYPE_USB, USB_GPHY_CTRL);

[Severity: Medium]
Should this BACKUP_RESTRORE wait be limited to RTL_VER_17_QFN68?

rtl8157_runtime_enable() calls r8157_ups_en(tp, false) for both
RTL_VER_16 and RTL_VER_17_QFN68. Nothing here checks the version before
polling USB_GPHY_CTRL.

Everywhere else this wait is treated as RTL8159 only. r8157_init() does it
only for the VER_17 parts:

drivers/net/usb/r8152.c:r8157_init() {
    ...
	switch (tp->version) {
	case RTL_VER_17_QFN68:
	case RTL_VER_17_QFN100:
		if (r8159_wait_backup_restore(tp)) {
    ...
}

The u2phy/u3phy backup restore patch enables (0xb99c BIT(0), 0xb9a2) are
also set only for VER_17. The earlier r8156_init() gated this wait on
tp->version == RTL_VER_17. The "split r8156_init by generation" commit in
this series calls it "the RTL8159-only backup-restore wait".

The commit message says the timeout is handled "as r8157_init() does for
the same wait". However, r8157_init() never runs this wait on RTL_VER_16.

What happens if RTL8157 doesn't assert BACKUP_RESTRORE after a UPS power
cut? Wouldn't every runtime resume after a power cut time out here and
leave the NIC inaccessible until replug?

Could this be gated on RTL_VER_17_QFN68? Another option is to call
r8159_wait_backup_restore() under the same version check that r8157_init()
uses.

> +			if (test_bit(RTL8152_INACCESSIBLE, &tp->flags))
> +				return;
> +
> +			if (ret) {
> +				rtl_set_inaccessible(tp);
> +				dev_err(&tp->intf->dev,
> +					"backup-restore timed out\n");
> +				return;
> +			}

[Severity: Medium]
What recovers the device if this timeout hits during runtime resume?

r8157_ups_en() and rtl8157_runtime_enable() both return void.
rtl8152_runtime_resume() carries on and returns 0:

rtl8152_resume()
  rtl8152_runtime_resume()
    tp->rtl_ops.autosuspend_en(tp, false)
      rtl8157_runtime_enable(tp, false)
        r8157_ups_en(tp, false)
          rtl_set_inaccessible(tp)
    napi_enable(napi)
    usb_submit_urb(tp->intr_urb, GFP_NOIO)
    return 0

The existing recovery in rtl8152_resume() skips runtime resume:

	if (!runtime_resume && test_bit(RTL8152_INACCESSIBLE, &tp->flags))
		usb_reset_device(tp->udev);

So the PM core sees a successful resume and the netdev stays IFF_UP. Every
later register access is short-circuited by RTL8152_INACCESSIBLE. This
lasts until a replug or an unrelated reset, such as the TX watchdog.

The commit message acknowledges this and leaves recovery to a follow-up
series. Still, this patch is the one that adds the runtime resume failure
path. Could the recovery land together with this change, for example by
queuing a reset or passing the error back?

> +
> +			/* clear USB fw_ver_reg */
> +			ocp_write_byte(tp, MCU_TYPE_USB, USB_FW_USB_VER, 0);

[Severity: Low]
Is USB_FW_USB_VER guaranteed to match the fw_ver_reg in the loaded USB
firmware block?

The path is r8157_hw_phy_cfg()->rtl8152_apply_firmware()->
rtl8152_fw_mac_apply(). That function decides whether to skip the patch
using the register named in the firmware blob, not a fixed address:

drivers/net/usb/r8152.c:rtl8152_fw_mac_apply() {
    ...
	fw_ver_reg = __le16_to_cpu(mac->fw_ver_reg);
	if (fw_ver_reg && ocp_read_byte(tp, MCU_TYPE_USB, fw_ver_reg) >= mac->fw_ver_data) {
		dev_dbg(&tp->intf->dev, "%s firmware has been the newest\n", type ? "PLA" : "USB");
		return;
    ...
}

Suppose the FIRMWARE_8157_1 or FIRMWARE_8159_1 USB block names a
fw_ver_reg other than 0xcfd7. The USB MAC patch lost in the power cut would
then be skipped as already newest. This write would also land on an
unrelated register.

Could this use the fw_ver_reg parsed from the loaded firmware instead of a
hardcoded address?

> +
> +			tp->rtl_ops.hw_phy_cfg(tp);
> +
> +			rtl8152_set_speed(tp, tp->autoneg, tp->speed,
> +					  tp->duplex, tp->advertising);

[Severity: Medium]
Does this power cut recovery need a wait_autoload_done() before
hw_phy_cfg()?

The other UPS exit paths wait for autoload as soon as they see
PCUT_STATUS. For example, r8153b_ups_en() does this, and r8153c_ups_en()
does the same:

	if (ocp_read_word(tp, MCU_TYPE_USB, USB_MISC_0) & PCUT_STATUS) {
		if (wait_autoload_done(tp))
			return;

r8157_init(), for the same chip family, runs r8156b_wait_loading_flash()
and wait_autoload_done() before the backup restore wait and PHY setup.

r8156_ups_en(tp, false), which this function mirrors, also has no autoload
wait. But r8156_ups_en(tp, true) is never called, and r8156_init() clears
PCUT_STATUS first. That path has never run after a real power cut.

r8157_hw_phy_cfg() waits only for GPHY_PATCH_DONE, and only when
GPHY_FLASH is set and BYPASS_FLASH is clear.

If autoload is still running after the power cut, could the firmware and
PHY writes from hw_phy_cfg() and rtl8152_set_speed() race the hardware
reload? They might then be overwritten or only partly applied.

> +		}
> +	}
> +}

[ ... ]

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