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

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

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

- [Medium] Incomplete version dispatch: the four new switch statements
  added by this patch enumerate only RTL_VER_16 and RTL_VER_17_QFN68 and…

--

r8152: support UPS for RTL8157 and RTL8159

This adds r8157_ups_en() for the RTL8157/RTL8159 UPS enable sequence,
extends r8156_ups_flags() with per-speed EEE flags and 5G/10G speed
encodings, and switches r8157_init()/r8159_init() over to the new
enable helper.

[ ... ]

[Severity: Medium]

Should RTL_VER_17_QFN100 be listed here as well?

In rtl_ops_init() the two RTL8159 packages share one identical block:

	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->hw_phy_cfg		= r8159_hw_phy_cfg;
		ops->autosuspend_en	= rtl8157_runtime_enable;

So QFN100 advertises the same 100M through 10G EEE capabilities, but with
only RTL_VER_17_QFN68 matched above it takes the default arm and gets the
old UPS_FLAGS_EN_EEE encoding that this patch replaces for this chip
generation.

[Severity: Medium]

Same question for the lite_mode suppression: RTL_VER_17_QFN100 is not
listed, so it still gets the bit 5/6 lite_mode field that this hunk treats
as not applicable to this generation.

+		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;
 	}
 

[ ... ]

[Severity: Medium]

Does this leave RTL_VER_17_QFN100 without UPS entirely?

rtl_ops_init() assigns ops->autosuspend_en = rtl8157_runtime_enable for
both RTL_VER_17_QFN68 and RTL_VER_17_QFN100, and __rtl_get_pkg_det()
results of 0x18/0x1a select RTL_VER_17_QFN100. On runtime suspend the
chain is:

	rtl8152_runtime_suspend()
	  -> tp->rtl_ops.autosuspend_en(tp, true)
	     == rtl8157_runtime_enable(tp, true)
	        -> switch (tp->version) ... default: break;

so r8157_ups_en() is never reached for that package, while the commit
message says "support UPS for RTL8157 and RTL8159".

 	} 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]

Mirror of the above on the resume path.

Note that r8159_init(), shared by both RTL8159 packages, was converted in
this same patch:

	r8157_power_cut_en(tp, false);
	r8157_ups_en(tp, false);

so the QFN100 path does get the new disable call at probe/init time, but
never the enable/disable dispatch during runtime suspend and resume.

There is a QFN68-only quirk elsewhere in r8159_init() ("Enable u2phy
backup restore patch"), so a package-specific restriction is plausible.
Should case RTL_VER_17_QFN100 be added to all four new switches, or should
the commit message be scoped to the QFN68 package with a note on the
hardware difference?

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