Re: [PATCH net-next v2 8/8] r8152: support UPS for RTL8157 and RTL8159

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 2 · Low: 0

- [High] The three new version switches added by this patch (in
  r8157_ups_en()'s two call sites inside rtl8157_runtime_enable(), and…
- [Medium] In the resume (!enable) path of rtl8157_runtime_enable(),
  r8157_ups_en(tp, false) is invoked only after r8153_queue_wake(tp,…
- [Medium] r8157_ups_en(tp, false) detects PCUT_STATUS (a UPS power cut
  occurred, so the PHY/GPHY domain was powered down and is re-loading)…

--

r8152: support UPS for RTL8157 and RTL8159

This adds r8157_ups_en() for the RTL8157/RTL8159 UPS enable sequence and
extends r8156_ups_flags() with per-speed EEE flags and 5G/10G speed
encodings. rtl8157_runtime_enable() now arms and disarms UPS around
runtime suspend, and the two init paths switch from r8156_ups_en() to the
new helper.

> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index 2b0972b96738..cc18b1c5a17c 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c

[ ... ]

> @@ -4178,8 +4184,27 @@ static void r8156_ups_flags(struct r8152 *tp)
>  	if (tp->ups_info.aldps)
>  		ups_flags |= UPS_FLAGS_EN_ALDPS;
>  
> -	if (tp->ups_info.eee)
> -		ups_flags |= UPS_FLAGS_EN_EEE;
> +	if (tp->ups_info.eee) {
> +		switch (tp->version) {
> +		case RTL_VER_17_QFN68:
> +			if (tp->eee_adv & MDIO_EEE_10GT)
> +				ups_flags |= UPS_FLAGS_EN_10G_EEE;
> +			fallthrough;
> +		case RTL_VER_16:
> +			if (tp->eee_adv & MDIO_EEE_100TX)
> +				ups_flags |= UPS_FLAGS_EN_100M_EEE;
> +			if (tp->eee_adv & MDIO_EEE_1000T)
> +				ups_flags |= UPS_FLAGS_EN_1000M_EEE;
> +			if (tp->eee_adv2 & MDIO_EEE_2_5GT)
> +				ups_flags |= UPS_FLAGS_EN_2500M_EEE;
> +			if (tp->eee_adv2 & MDIO_EEE_5GT)
> +				ups_flags |= UPS_FLAGS_EN_5000M_EEE;
> +			break;
> +		default:
> +			ups_flags |= UPS_FLAGS_EN_EEE;
> +			break;
> +		}
> +	}
>  
>  	if (tp->ups_info.flow_control)
>  		ups_flags |= UPS_FLAGS_EN_FLOW_CTR;

[ ... ]

> @@ -4230,20 +4255,33 @@ static void r8156_ups_flags(struct r8152 *tp)
>  	case NWAY_2500M_FULL:
>  		ups_flags |= ups_flags_speed(9);
>  		break;
> +	case NWAY_5000M_FULL:
> +		ups_flags |= ups_flags_speed(10);
> +		break;
> +	case NWAY_10000M_FULL:
> +		ups_flags |= ups_flags_speed(11);
> +		break;
>  	default:
>  		break;
>  	}
>  
> -	switch (tp->ups_info.lite_mode) {
> -	case 1:
> -		ups_flags |= 0 << 5;
> -		break;
> -	case 2:
> -		ups_flags |= 2 << 5;
> +	switch (tp->version) {
> +	case RTL_VER_16:
> +	case RTL_VER_17_QFN68:
>  		break;
> -	case 0:
>  	default:
> -		ups_flags |= 1 << 5;
> +		switch (tp->ups_info.lite_mode) {
> +		case 1:
> +			ups_flags |= 0 << 5;
> +			break;
> +		case 2:
> +			ups_flags |= 2 << 5;
> +			break;
> +		case 0:
> +		default:
> +			ups_flags |= 1 << 5;
> +			break;
> +		}
>  		break;
>  	}
>  

[ ... ]

> @@ -4415,6 +4453,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: Medium]
Should this PCUT_STATUS branch wait for autoload to complete before
touching the PHY?  The two peer helpers in the same driver guard the
identical branch:

drivers/net/usb/r8152.c:r8153b_ups_en() {
	...
	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);
	...
}

r8153c_ups_en() does the same.  r8157_ups_en() was copied from
r8156_ups_en(), which also lacks the wait, but for this family that branch
was previously unreachable: r8156_ups_en()/r8157_ups_en() were only ever
called with enable=false from the init paths, always after
r8157_power_cut_en(tp, false) had already cleared PCUT_STATUS.  This patch
adds the first call with enable=true in rtl8157_runtime_enable(), so the
branch becomes live on the first runtime resume that follows a real power
cut.

For RTL8159 there is a second wait that looks relevant:

drivers/net/usb/r8152.c:r8159_wait_backup_restore() {
	ocp_data = ocp_read_word(tp, MCU_TYPE_USB, USB_MISC_0);
	if (!(ocp_data & PCUT_STATUS))
		return 0;

	return poll_timeout_us(ocp_data = ocp_read_word(tp, MCU_TYPE_USB, USB_GPHY_CTRL),
			       ocp_data & BACKUP_RESTRORE, 200, 20000, false);
}

That poll is itself predicated on PCUT_STATUS, and r8159_init() treats a
timeout there as fatal ("init failed, backup-restore timed out").  Can
hw_phy_cfg() run here while the GPHY backup-restore engine is still
running, given hw_phy_cfg() itself sets GPHY_PATCH_DONE | BACKUP_RESTRORE?

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

[ ... ]

> @@ -4573,9 +4640,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;
> +		}

[Severity: High]
Is the omission of RTL_VER_17_QFN100 here intentional?

RTL_VER_17_QFN68 and RTL_VER_17_QFN100 are the two package variants
selected by __rtl_get_pkg_det() (0x1e/0x1c versus 0x18/0x1a), and
rtl_ops_init() gives them the same ops and the same EEE capabilities:

drivers/net/usb/r8152.c:rtl_ops_init() {
	case RTL_VER_17_QFN68:
	case RTL_VER_17_QFN100:
		tp->eee_en		= true;
		tp->eee_adv		= MDIO_EEE_100TX | MDIO_EEE_1000T | MDIO_EEE_10GT;
		tp->eee_adv2		= MDIO_EEE_2_5GT | MDIO_EEE_5GT;
		ops->init		= r8159_init;
	...
		ops->autosuspend_en	= rtl8157_runtime_enable;
}

Both also map to FIRMWARE_8159_1.  With the switch above, a QFN100 device
takes the default arm on both the suspend and the resume side, so
r8157_ups_en() is never called for it: UPS_EN | USP_PREWAKE | PHASE2_EN and
UPS_FORCE_PWR_DOWN are never programmed, USB_UPS_FLAGS is never written,
and the PCUT_STATUS recovery is never run on resume.  Does the commit
subject "support UPS for RTL8157 and RTL8159" then only cover one of the two
RTL8159 packages?

The same two-version lists appear in r8156_ups_flags(), both in the new EEE
switch and in the new lite-mode switch:

	switch (tp->version) {
	case RTL_VER_16:
	case RTL_VER_17_QFN68:
		break;
	default:
		switch (tp->ups_info.lite_mode) {

If RTL_VER_17_QFN100 is added to rtl8157_runtime_enable() later without
also updating r8156_ups_flags(), it would fall into the default arms and be
programmed with the legacy UPS_FLAGS_EN_EEE bit plus the lite-mode bits,
which is the encoding this patch declares wrong for this family.  Every
other chip-family switch in this driver lists all three versions together
(rtl_disable(), r8153_set_rx_early_timeout(), r8153_set_rx_early_size(),
rtl_reset_bmu(), r8156_fc_parameter() and the max_mtu switch), so the
two-version lists here stand out.

>  	} 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]
Should UPS be exited before the PLA wake and WOL registers are
reprogrammed?  The other two UPS-capable chips do it in the opposite
order:

drivers/net/usb/r8152.c:rtl8153b_runtime_enable() {
	} else {
		r8153b_ups_en(tp, false);
		r8153_queue_wake(tp, false);
		rtl_runtime_suspend_enable(tp, false);
	...
}

rtl8153c_runtime_enable() begins its resume path with
r8153c_ups_en(tp, false) as well.

As written, r8153_queue_wake(tp, false) and
rtl_runtime_suspend_enable(tp, false) write PLA_INDICATE_FALG,
PLA_SUSPEND_FLAG, PLA_EXTRA_STATUS and the CRWECR-protected
PLA_CONFIG34/WOL registers while USB_POWER_CUT UPS_EN | USP_PREWAKE and
USB_MISC_2 UPS_FORCE_PWR_DOWN are still set.

Can those PLA writes be dropped or overwritten while the part is still held
in UPS force-power-down?  If they are, the device stays armed with
__rtl_set_wol(WAKE_ANY) and LINK_OFF_WAKE_EN while tp->saved_wolopts makes
the driver believe the user's WOL setting was restored.

A related effect of this order: when PCUT_STATUS is set, the hw_phy_cfg()
and rtl8152_set_speed() calls inside r8157_ups_en() now run after the
wake/WOL restore rather than before it, which is the reverse of what the
8153b/8153c paths do.

>  		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/20260903072852.58695-1-nic_swsd%40realtek.com