Re: [PATCH net-next v3 6/8] r8152: add TGPHY access support

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

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

- [Medium] r8157_phy_read() (drivers/net/usb/r8152.c:2086) and
  r8157_phy_write() (:2095) discard rtl_tgphy_access()'s return value.
- [Medium] wait_tgphy_cmd_ready() (drivers/net/usb/r8152.c:2045) makes
  its loop-exit decision on the value returned by ocp_read_word(), which…

--

r8152: add TGPHY access support

This adds a TGPHY register access path for RTL8157/RTL8159 using
USB_TGPHY_CMD/ADDR/DATA, so PHY registers can be reached without
reprogramming the PLA OCP base. It also introduces phy_read/phy_write
function pointers in struct rtl_ops so each chip gets the right access
method, with ocp_reg_read()/ocp_reg_write() becoming thin dispatchers.

A couple of questions about error propagation in the new path below.

[ ... ]

[ ... ]

[ ... ]

[Severity: Medium]

wait_tgphy_cmd_ready() makes its loop-exit decision purely on the value
returned by ocp_read_word(), which is indeterminate if the underlying
transfer never happens:

  ocp_read_word() {
	u32 data;
	__le32 tmp;
	...
	generic_ocp_read(tp, index, sizeof(tmp), &tmp, type | byen);
	data = __le32_to_cpu(tmp);
	...
  }

generic_ocp_read() starts with:

	if (test_bit(RTL8152_INACCESSIBLE, &tp->flags))
		return -ENODEV;

so tmp is left untouched and the u16 handed to the poll loop is stale
stack content.  If bit 0 happens to be set, does this helper then sleep
for its whole 20 ms budget instead of returning right away?

The USB-error case looks deterministic rather than random, since
get_registers() does:

	if (ret < 0)
		memset(data, 0xff, size);

USB_TGPHY_CMD then reads back as 0xffff, TGPHY_CMD_BUSY always appears
asserted, and every TGPHY access consumes the full timeout.

Since every PHY register access on RTL8157/RTL8159 now funnels through
rtl_tgphy_access() (one wait per write, two per read), would a device
marked inaccessible mid-sequence make r8157_hw_phy_cfg()/hw_phy_work, the
up/down paths and the mdio ioctl sleep 20-40 ms per register while holding
tp->control?  Would an is-accessible check in the helper be preferable to
polling a value that may never have been read?

+
+static int rtl_tgphy_access(struct r8152 *tp, u16 addr, u16 *data, bool write)
+{
+	u16 cmd = 0;
+	int ret;
+
+	ret = wait_tgphy_cmd_ready(tp);
+	if (ret < 0)
+		goto out;
+
+	if (write) {
+		cmd |= TGPHY_CMD_WR;
+		ocp_write_word(tp, MCU_TYPE_USB, USB_TGPHY_DATA, *data);
+	}
+
+	ocp_write_word(tp, MCU_TYPE_USB, USB_TGPHY_ADDR, addr);
+
+	cmd |= TGPHY_CMD_BUSY;
+	ocp_write_word(tp, MCU_TYPE_USB, USB_TGPHY_CMD, cmd);
+
+	if (!write) {
+		ret = wait_tgphy_cmd_ready(tp);
+		if (ret < 0)
+			goto out;
+
+		*data = ocp_read_word(tp, MCU_TYPE_USB, USB_TGPHY_DATA);
+	}
+
+out:
+	return ret;
+}
+
+static u16 r8157_phy_read(struct r8152 *tp, u16 addr)
+{
+	u16 data = 0;
+
+	rtl_tgphy_access(tp, addr, &data, false);
+
+	return data;
+}
+
+static void r8157_phy_write(struct r8152 *tp, u16 addr, u16 data)
+{
+	rtl_tgphy_access(tp, addr, &data, true);
+}

[Severity: Medium]

Both wrappers discard rtl_tgphy_access()'s status.  On a
wait_tgphy_cmd_ready() timeout, r8157_phy_read() returns the
pre-initialised 0, which is indistinguishable from real register content.

Can that fabricated 0 get written back to the PHY?  ocp_reg_read() now
dispatches to r8157_phy_read() for RTL8157/RTL8159, and the
read-modify-write helpers use it directly:

  ocp_reg_w0w1() {
	u16 data;

	data = ocp_reg_read(tp, addr);
	data = (data & ~clear) | set;
	ocp_reg_write(tp, addr, data);
  }

so a timed-out read makes the driver write (0 & ~clear) | set, dropping
every other bit of the MII/PHY register (advertisement, EEE, PLL/ALDPS).
r8152_mdio_set_bit(), r8152_mdio_test_and_clr_bit() and sram_write_w0w1()
have the same shape.

For the write direction, a first-wait timeout jumps to out before any of
the ADDR/DATA/CMD writes are issued, so the PHY write is dropped
entirely.  With the void signature there is no way for the caller to
notice, and no dev_err()/dev_warn() is emitted, so should this at least
be logged?

Note that the existing global mitigation does not cover a plain BUSY
timeout: rtl_set_inaccessible() is only reached when a USB control
transfer fails in r8152_control_msg(), so here the control transfers
succeed and generic_ocp_write() still pushes the clobbered value to the
chip.

The sibling ADV interface in the same file returns int for exactly this
reason:

  static int ocp_adv_read(struct r8152 *tp, u16 cmd, u16 addr, u32 *data)
  ...
  static int rtl_bmu_w0w1(struct r8152 *tp, u16 addr, u32 clear, u32 set)
  {
	ret = rtl_bmu_read(tp, addr, &bmu);
	if (ret < 0)
		goto out;
	...
  }

and wait_tgphy_cmd_ready() duplicates wait_cmd_ready() almost verbatim.
Would it make sense for the new phy_read/phy_write ops in struct rtl_ops
to carry a status back, or to reuse the existing wait_cmd_ready() helper,
so the TGPHY timeout can stop the read-modify-write sequence instead of
being swallowed?

[ ... ]

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