Re: [PATCH net-next v3 4/8] r8152: refactor r8157_hw_phy_cfg

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

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

- [Medium] r8159_hw_phy_cfg() (drivers/net/usb/r8152.c:8332), newly bound
  as the hw_phy_cfg handler for RTL_VER_17_QFN68/RTL_VER_17_QFN100 in…
- [Low] The commit is presented as 'refactor r8157_hw_phy_cfg' with
  'update PHY parameters for RTL8156B/57/59', but it silently changes…

--

r8152: refactor r8157_hw_phy_cfg

This splits the shared r8157_hw_phy_cfg() into r8157_hw_phy_cfg() for
RTL_VER_16 and a new r8159_hw_phy_cfg() for RTL_VER_17_QFN68/QFN100,
rebinds the VER_17 ops entry to the new function, adds sram2_set_bits()
and sram2_clr_bits() helpers, and adjusts PHY parameters for
RTL8156B/57/59.

Two questions below.

[ ... ]

[Severity: Low]
This isn't a bug, but the commit message describes this as a refactor plus
an "update PHY parameters for RTL8156B/57/59", and some of the register
programming changes are hard to classify from the message alone.

In r8157_hw_phy_cfg() this 0xd3c8 BIT(0) clear was previously inside the
"case RTL_VER_17_QFN68/RTL_VER_17_QFN100:" arm only, and it is now above
the version switch, so RTL8157 (RTL_VER_16) executes it for the first
time.

In r8156b_hw_phy_cfg() the new "Nway DACONB parameters" write:

	ocp_reg_w0w1(tp, 0xa4ca, 0x6000, 0x0040);

retunes RTL8156B (RTL_VER_13/RTL_VER_15) in a patch whose subject is an
r8157 refactor.

Can the changelog say which of these register changes fix an observed
symptom (and therefore want a Fixes: tag and stable backporting) versus
which are vendor tuning?  Would the RTL8156B change be better as its own
patch?

 	switch (tp->version) {
 	case RTL_VER_16:
 		/* XG_INRX parameter */

[ ... ]

[ ... ]

+	set_bit(PHY_RESET, &tp->flags);
+}
+
+static void r8159_hw_phy_cfg(struct r8152 *tp)
+{
+	u16 data;
+
+	r8156b_wait_loading_flash(tp);
+
+	ocp_word_test_and_clr_bits(tp, MCU_TYPE_USB, USB_MISC_0, PCUT_STATUS);
+
+	data = r8153_phy_status(tp, 0);
+	switch (data) {
+	case PHY_STAT_EXT_INIT:
+		rtl8152_apply_firmware(tp, true);
+		ocp_reg_clr_bits(tp, 0xa466, BIT(0));
+		ocp_reg_clr_bits(tp, 0xa468, BIT(3) | BIT(1));
+		break;
+	case PHY_STAT_LAN_ON:
+	case PHY_STAT_PWRDN:
+	default:
+		rtl8152_apply_firmware(tp, false);
+		break;
+	}
 
+	r8152_mdio_test_and_clr_bit(tp, MII_BMCR, BMCR_PDOWN);
+
+	r8153_aldps_en(tp, false);
+
+	data = r8153_phy_status(tp, PHY_STAT_LAN_ON);
+	WARN_ON_ONCE(data != PHY_STAT_LAN_ON);

[Severity: Medium]
Was dropping the EEE disable here intentional for RTL8159?

Before the split, RTL_VER_17_QFN68/QFN100 went through the shared
r8157_hw_phy_cfg(), which quiesced both ALDPS and EEE before touching PHY
parameters, and the RTL_VER_16 half still does:

	r8153_aldps_en(tp, false);
	rtl_eee_enable(tp, false);

r8159_hw_phy_cfg() only calls r8153_aldps_en(tp, false) and then starts
reprogramming PHY/SRAM/OCP parameters, including the EEE related ones
(PLA_MAC_PWR_CTRL4 EEE_SPDWN_EN, OCP_DOWN_SPEED and OCP_POWER_CFG
EEE_CLKDIV_EN).  The other handlers in this driver comment on that
ordering explicitly, for example in r8156b_hw_phy_cfg():

	/* disable EEE before updating the PHY parameters */
	rtl_eee_enable(tp, false);

The remaining half of the pair is only the conditional enable at the tail
of r8159_hw_phy_cfg():

	if (tp->eee_en)
		rtl_eee_enable(tp, true);

With tp->eee_en set to false through r8152_set_eee(), nothing turns
hardware EEE off any more, so after a re-run of hw_phy_cfg (the
PCUT_STATUS resume paths, or rtl_hw_phy_change_work_func() calling
tp->rtl_ops.hw_phy_cfg(tp)) does the PHY come back with its default EEE
state while ups_info still records EEE as disabled, silently undoing an
earlier "ethtool --set-eee eee off"?

This still looks the same at the end of the series, so no later patch in
the set restores the disable.

+
+	/* PFM mode */
+	ocp_word_clr_bits(tp, MCU_TYPE_PLA, PLA_PHY_PWR, PFM_PWM_SWITCH);
+
+	/* Advanced Power Saving parameter */
+	ocp_reg_set_bits(tp, OCP_POWER_CFG, EN_10M_PLLOFF | EN_ALDPS_PLLOFF);
+
+	/* Disable ALDPS force mode */
+	ocp_reg_clr_bits(tp, 0xa44a, BIT(2));
+
+	/* Disable bypass_turn_off_clk_in_aldps */
+	ocp_byte_clr_bits(tp, MCU_TYPE_PLA, 0xd3c8, BIT(0));
+
+	switch (tp->version) {
 	case RTL_VER_17_QFN68:
 	case RTL_VER_17_QFN100:
-		/* Disable bypass turn off clk in ALDPS */
-		ocp_byte_clr_bits(tp, MCU_TYPE_PLA, 0xd3c8, BIT(0));
-
 		/* Power level tuning
 		 * test mode power level
 		 */

[ ... ]

These two hunks land in the RTL8159 epilogue and also change behaviour
rather than move code: EN_10M_CLKDIV goes from set to cleared with
ups_info._10m_ckdiv flipping from true to false (that field feeds the
UPS/suspend programming), and 0xa5ea BIT(2) is newly cleared.  Could the
changelog mention what these two changes do for RTL8159?

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