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

[netdev-bot+sashiko]

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.

> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index 06fad895fce09..1fcb1cc5b4a18 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c

[ ... ]

> @@ -959,6 +966,8 @@ struct r8152 {
>  		void (*hw_phy_cfg)(struct r8152 *tp);
>  		void (*autosuspend_en)(struct r8152 *tp, bool enable);
>  		void (*change_mtu)(struct r8152 *tp);
> +		u16 (*phy_read)(struct r8152 *tp, u16 addr);
> +		void (*phy_write)(struct r8152 *tp, u16 addr, u16 data);
>  	} rtl_ops;

[ ... ]

> @@ -1687,6 +1696,16 @@ static void ocp_reg_write(struct r8152 *tp, u16 addr, u16 data)
>  	ocp_write_word(tp, MCU_TYPE_PLA, ocp_index, data);
>  }
>  
> +static u16 ocp_reg_read(struct r8152 *tp, u16 addr)
> +{
> +	return tp->rtl_ops.phy_read(tp, addr);
> +}
> +
> +static void ocp_reg_write(struct r8152 *tp, u16 addr, u16 data)
> +{
> +	tp->rtl_ops.phy_write(tp, addr, data);
> +}
> +

[ ... ]

> @@ -2023,6 +2042,61 @@ static int r8152_mdio_test_and_clr_bit(struct r8152 *tp, u16 addr, u16 clear)
>  	return data & clear;
>  }
>  
> +static int wait_tgphy_cmd_ready(struct r8152 *tp)
> +{
> +	u16 ocp_data;
> +
> +	return poll_timeout_us(ocp_data = ocp_read_word(tp, MCU_TYPE_USB,
> +							USB_TGPHY_CMD),
> +			       !(ocp_data & TGPHY_CMD_BUSY), 2000, 20000,
> +			       false);
> +}

[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