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

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · 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).

- [High] r8157_ups_en()'s UPS-exit (disable) path in
  drivers/net/usb/r8152.c starts reprogramming the chip as soon as it…
- [Medium] In rtl8157_runtime_enable()'s disable (resume) branch the new
  UPS exit is sequenced last instead of first: line 4722 calls…

--

r8152: enable UPS for RTL8157 and RTL8159 QFN68

This adds r8157_ups_en() and calls it from rtl8157_runtime_enable() for
RTL_VER_16 and RTL_VER_17_QFN68, so UPS is now toggled across runtime
suspend/resume for those chips.  It also extends r8156_ups_flags() with
per-speed EEE flags and two new ups_flags_speed() entries.

Two questions below about the new UPS exit path.

> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index 05b2b79ab3e31..ebe9cc9322b3c 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c

[ ... ]

> @@ -4484,6 +4522,35 @@ static void r8156_ups_en(struct r8152 *tp, bool enable)
>  	}
>  }
>  
> +static void r8157_ups_en(struct r8152 *tp, bool enable)
> +{
> +	if (enable) {
> +		r8156_ups_flags(tp);
> +
> +		ocp_byte_set_bits(tp, MCU_TYPE_USB, USB_POWER_CUT,
> +				  UPS_EN | USP_PREWAKE | PHASE2_EN);
> +
> +		ocp_byte_set_bits(tp, MCU_TYPE_USB, USB_MISC_2,
> +				  UPS_FORCE_PWR_DOWN);
> +	} 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);
> +
> +		if (ocp_read_word(tp, MCU_TYPE_USB, USB_MISC_0) & PCUT_STATUS) {
> +			/* clear USB fw_ver_reg */
> +			ocp_write_byte(tp, MCU_TYPE_USB, USB_FW_USE_VER, 0);
> +
> +			tp->rtl_ops.hw_phy_cfg(tp);
> +
> +			rtl8152_set_speed(tp, tp->autoneg, tp->speed,
> +					  tp->duplex, tp->advertising);
> +		}
> +	}
> +}

[Severity: High]
Should this PCUT_STATUS branch wait for autoload to complete before
reprogramming the PHY?

The two UPS helpers that are actually reached from a runtime resume after a
power cut, r8153b_ups_en() and r8153c_ups_en(), gate the same sequence on
wait_autoload_done():

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

		tp->rtl_ops.hw_phy_cfg(tp);

		rtl8152_set_speed(tp, tp->autoneg, tp->speed,
				  tp->duplex, tp->advertising);
	}

r8156_ups_en(), from which the new helper was copied, has no such wait, but
its branch appears to be dead: both callers (r8156_init() and r8156b_init())
run r8153b_power_cut_en(tp, false) first, which clears PCUT_STATUS.  With
this patch the unguarded branch becomes live via

  rtl8152_runtime_resume() -> tp->rtl_ops.autosuspend_en(tp, false)
    -> rtl8157_runtime_enable(tp, false) -> r8157_ups_en(tp, false)

and PCUT_STATUS is exactly the expected state there, because the suspend side
just ran r8157_ups_en(tp, true).  Can hw_phy_cfg() and the following link
renegotiation then run while the chip is still reloading its post-power-cut
configuration?

wait_autoload_done() also returns -ENODEV when RTL8152_INACCESSIBLE is set,
so the peer helpers bail out on a disconnect during resume.  Without that
check, does a disconnect here run the whole hw_phy_cfg() sequence and then
the BMCR_RESET poll in rtl8152_set_speed(), which can spin 50 * msleep(20)
on failing register reads?

For RTL_VER_17_QFN68 there is a second handshake that this path skips.
r8159_init() treats a missing backup-restore as fatal whenever PCUT_STATUS is
set:

	if (r8159_wait_backup_restore(tp)) {
		rtl_set_inaccessible(tp);
		dev_err(&tp->intf->dev,
			"init failed, backup-restore timed out\n");
		return;
	}

r8159_wait_backup_restore() polls USB_GPHY_CTRL for BACKUP_RESTRORE only when
PCUT_STATUS is set.  On the new runtime resume path, r8159_hw_phy_cfg() only
calls r8156b_wait_loading_flash() and then clears PCUT_STATUS before starting
firmware/PHY programming, so nothing waits for BACKUP_RESTRORE.  Is that
handshake needed here too?

>  static void r8153_power_cut_en(struct r8152 *tp, bool enable)
>  {
>  	if (enable)

[ ... ]

> @@ -4642,9 +4709,28 @@ static void rtl8157_runtime_enable(struct r8152 *tp, bool enable)
>  		r8153b_u1u2en(tp, false);
>  		r8157_u2p3en(tp, false);
>  		rtl_runtime_suspend_enable(tp, true);
> +
> +		switch (tp->version) {
> +		case RTL_VER_16:
> +		case RTL_VER_17_QFN68:
> +			r8157_ups_en(tp, true);
> +			break;
> +		default:
> +			break;
> +		}
>  	} else {
>  		r8153_queue_wake(tp, false);
>  		rtl_runtime_suspend_enable(tp, false);
> +
> +		switch (tp->version) {
> +		case RTL_VER_16:
> +		case RTL_VER_17_QFN68:
> +			r8157_ups_en(tp, false);
> +			break;
> +		default:
> +			break;
> +		}
> +

[Severity: Medium]
Is the ordering here intentional?  In the resume branch the UPS exit runs
after the PLA writes, while the other UPS-capable variants exit UPS first.

rtl8153b_runtime_enable():

	} else {
		r8153b_ups_en(tp, false);
		r8153_queue_wake(tp, false);
		rtl_runtime_suspend_enable(tp, false);

rtl8153c_runtime_enable() has the same order with r8153c_ups_en(tp, false)
first.

Here, r8153_queue_wake(tp, false) and rtl_runtime_suspend_enable(tp, false)
touch PLA registers (PLA_INDICATE_FALG/UPCOMING_RUNTIME_D3, PLA_SUSPEND_FLAG,
PLA_EXTRA_STATUS, and the CRWECR-gated PLA_CONFIG34/LINK_OFF_WAKE_EN plus
__rtl_set_wol(tp->saved_wolopts)) while UPS_EN | USP_PREWAKE and
UPS_FORCE_PWR_DOWN are still set, and r8157_ups_en(tp, false) only clears
those bits afterwards.

Since r8153b_ups_en()/r8153c_ups_en() poll PLA_BOOT_CTRL for AUTOLOAD_DONE
right after clearing the same two bits, the PLA block seems to still be
reloading its defaults at that point.  Can the WOL settings restored from
tp->saved_wolopts and the cleared UPCOMING_RUNTIME_D3 flag be overwritten by
that reload, leaving the device unable to wake or with link-off-wake still
armed?  The new helper has no AUTOLOAD_DONE gate either, so nothing in this
resume sequence waits for the reload.

>  		r8157_u2p3en(tp, true);
>  		if (tp->udev->speed >= USB_SPEED_SUPER)
>  			r8153b_u1u2en(tp, true);

[ ... ]

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