Re: [PATCH net-next v6 08/10] r8152: add TGPHY register access for RTL8157 and RTL8159

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 3 · Medium: 8 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] On the MII library path, a TGPHY timeout can write a corrupted
  BMCR value to the PHY.
- [High] rtl_phy_patch_request() still ignores whether the PATCH_REQUEST
  write succeeded, even though the commit message lists 'the PHY patch…
- [High] rtl8152_set_speed() still throws away the status of its
  advertisement writes, although the commit message lists it among the…
- [Medium] The two new `return ret;` error paths in
  rtl_phy_patch_request() leave PATCH_REQUEST asserted.
- [Medium] rtl8152_set_speed() now returns early after it has already
  changed hardware and driver state, and it can lose a pending PHY reset.
- [Medium] r8153_phy_status() now gives up its PHY readiness wait (up to
  500 × 20 ms) on the first failed ocp_reg_read().
- [Medium] On a read failure, rtl8153_in_nway() now returns false, which
  permits autosuspend while the PHY may be negotiating.
- [Medium] PHY SRAM is programmed by writing OCP_SRAM_ADDR once and then
  streaming consecutive words to OCP_SRAM_DATA, with the hardware auto-…
- [Medium] The patch moves RTL8157/RTL8159 off a PHY access path that
  cannot fail (r8152_phy_read()/r8152_phy_write()) onto the TGPHY…
- [Medium] A stuck TGPHY channel turns PHY bring-up and firmware loading
  into a long stall that floods the kernel log.
- [Medium] Several statements in the commit message do not match the
  code.

--

r8152: add TGPHY register access for RTL8157 and RTL8159

This patch adds a TGPHY command channel (USB_TGPHY_ADDR/DATA/CMD) for PHY
register access on RTL8157 and RTL8159. It turns ocp_reg_read() and
ocp_reg_write() into wrappers over new per-chip rtl_ops.phy_read and
rtl_ops.phy_write hooks. The PHY accessors and the helpers built on them now
return int, and the status is propagated in a subset of callers.

> RTL8157 and RTL8159 can reach PHY registers through the same
> OCP-base-switching path as earlier chips, but they also provide a
> TGPHY command channel (USB_TGPHY_ADDR/DATA/CMD) that needs no base
> switching.  Use it for these chips.

[Severity: Medium]
Is there a hardware reason these chips need the TGPHY channel?

The existing r8152_phy_read()/r8152_phy_write() path cannot fail. The TGPHY
path can, and the commit message says the MII callbacks will take errnos as
register values until a follow-up series lands.

A TGPHY read is at least 5 control transfers (busy poll, ADDR, CMD, busy
poll, DATA), and a write is at least 4. The old path needs 1 or 2. That cost
applies to every hw_phy_cfg step and every firmware word.

Could the switch in rtl_ops_init() land after the callers are hardened? If
not, could the commit message describe the hardware benefit?

> form a single transaction.  Like the ADV command channel, it only
> logs the timeout and does not mark the device inaccessible.  If the

[Severity: Medium]
Is this comparison accurate? wait_cmd_ready() is a bare poll_timeout_us()
with no message, and ocp_adv_read()/ocp_adv_write() don't log either.

> Not handled here, and left to a follow-up series: the remaining
> callers, notably the hw_phy_cfg and init sequences,
> r8153_phy_status(), r8152_mmd_read(), EEE handling and the MII
> library callbacks, still ignore the status.

Several of these do change in this patch:

  - r8153_get_eee() now returns errnos to ethtool.
  - r8156_eee_en() now returns early on a failed OCP_EEE_ADV2 read.
  - r8153_phy_status() now stops polling on the first error.
  - rtl8152_get_link_ksettings() now returns PHY errnos to userspace.
  - rtl8153_in_nway(), r8156_mdio_force_mode() and rtl8152_fw_phy_fixup()
    gain early returns.

Could the message describe these changes?

> Older chips are unaffected, as r8152_phy_read() always succeeds.

r8156_hw_phy_cfg() (RTL8156) now reads PHY register 0xd068 before the
MID_REVERSE test. Previously that test skipped the read. The extra read is
harmless, but the statement isn't quite accurate.

> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index b7c201ac6d160..023e7af4c5fa6 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c

[ ... ]

> @@ -1664,16 +1675,33 @@ static void ocp_reg_write(struct r8152 *tp, u16 addr, u16 data)
>  
>  	ocp_index = (addr & 0x0fff) | 0xb000;
>  	ocp_write_word(tp, MCU_TYPE_PLA, ocp_index, data);
> +
> +	return 0;
>  }
>  
> -static inline void r8152_mdio_write(struct r8152 *tp, u32 reg_addr, u32 value)
> +static int ocp_reg_read(struct r8152 *tp, u16 addr, u16 *data)
>  {
> -	ocp_reg_write(tp, OCP_BASE_MII + reg_addr * 2, value);
> +	return tp->rtl_ops.phy_read(tp, addr, data);
>  }
>  
> -static inline int r8152_mdio_read(struct r8152 *tp, u32 reg_addr)
> +static int ocp_reg_write(struct r8152 *tp, u16 addr, u16 data)
>  {
> -	return ocp_reg_read(tp, OCP_BASE_MII + reg_addr * 2);
> +	return tp->rtl_ops.phy_write(tp, addr, data);
> +}
> +
> +static inline int r8152_mdio_write(struct r8152 *tp, u32 reg_addr, u32 value)
> +{
> +	return ocp_reg_write(tp, OCP_BASE_MII + reg_addr * 2, value);
> +}
> +
> +static int r8152_mdio_read(struct r8152 *tp, u32 reg_addr)
> +{
> +	u16 data;
> +	int ret;
> +
> +	ret = ocp_reg_read(tp, OCP_BASE_MII + reg_addr * 2, &data);
> +
> +	return ret < 0 ? ret : data;
>  }

[Severity: High]
read_mii_word() is still registered as tp->mii.mdio_read, and it passes this
return value through unchanged. On RTL8157/8159, can a TGPHY busy timeout
here end up writing a bogus value to BMCR?

mii_nway_restart() in drivers/net/mii.c does:

	bmcr = mii->mdio_read(mii->dev, mii->phy_id, MII_BMCR);

	if (bmcr & BMCR_ANENABLE) {
		bmcr |= BMCR_ANRESTART;
		mii->mdio_write(mii->dev, mii->phy_id, MII_BMCR, bmcr);
		r = 0;
	}

-ETIMEDOUT is 0xffffff92, which has BMCR_ANENABLE set.
wait_tgphy_cmd_ready() leaves RTL8152_INACCESSIBLE clear, so write_mii_word()
goes ahead. r8152_mdio_write() then writes 0xff92 to MII_BMCR.

0xff92 sets BMCR_RESET, BMCR_LOOPBACK, BMCR_PDOWN and BMCR_ISOLATE. The write
does its own 20 ms busy wait first, so a channel that recovers within that
window will accept it. ethtool -r (rtl8152_nway_reset()) and ethtool
--set-eee (rtl_ethtool_set_eee()) would then return 0.

mii_ethtool_get_link_ksettings() has a similar problem. It stores the errno
in u16 bmcr/bmsr/ctrl1000/stat1000, reports made-up link modes, and sets
mii->full_duplex from them.

The commit message defers this, and read_mii_word() is unchanged at the end
of the series. Should the switch to the TGPHY accessors wait until
read_mii_word() filters errors?

[ ... ]

> @@ -1906,100 +1950,217 @@ static void ocp_byte_set_bits(struct r8152 *tp, u16 type, u16 index, u8 set)

[ ... ]

> +static int wait_tgphy_cmd_ready(struct r8152 *tp)
> +{
> +	u16 ocp_data;
> +	int ret;
> +
> +	ret = read_poll_timeout(ocp_read_word, ocp_data,
> +				test_bit(RTL8152_INACCESSIBLE, &tp->flags) ||
> +				!(ocp_data & TGPHY_CMD_BUSY),
> +				2000, 20000, false, tp,
> +				MCU_TYPE_USB, USB_TGPHY_CMD);
> +
> +	if (ret)
> +		dev_err(&tp->intf->dev, "TGPHY cmd busy timeout\n");

[Severity: Medium]
What happens if the TGPHY channel stays busy? Most callers ignore the
status, including:

  - the per-word OCP_SRAM_DATA loops in rtl8152_fw_phy_union_apply() and
    rtl8152_fw_phy_nc_apply()
  - the hw_phy_cfg and init sequences

Each access would wait 20 ms and print this message, all while holding
tp->control. An N-word firmware block would stall for about N * 20 ms and
print N errors. Meanwhile ethtool, the ioctls and runtime PM are blocked.

Would a rate-limited message or some sticky failure state help here?

> +
> +	return test_bit(RTL8152_INACCESSIBLE, &tp->flags) ? -ENODEV : ret;
> +}
> +
> +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;

[Severity: Medium]
On a write, this early return drops the word without any visible effect.
The PHY SRAM loaders write OCP_SRAM_ADDR once, then stream words to
OCP_SRAM_DATA with auto-increment, and they ignore each status:

rtl8152_fw_phy_union_apply() / rtl8152_fw_phy_nc_apply():
	ocp_reg_write(tp, OCP_SRAM_ADDR, __le16_to_cpu(phy->fw_reg));
	for (i = 0; i < num; i++)
		ocp_reg_write(tp, OCP_SRAM_DATA, __le16_to_cpu(data[i]));

If one word is dropped, would every remaining word land one slot early?

The breakpoint and bp_en writes would then be applied on top of the shifted
image, and "successfully applied" would still be logged. The inline tables
in r8157_hw_phy_cfg() and r8159_hw_phy_cfg() follow the same pattern.
rtl8157-1.fw and rtl8159-1.fw are configured for these chips, so this path
is reachable.

[ ... ]

> @@ -4177,11 +4338,13 @@ static void r8153b_green_en(struct r8152 *tp, bool enable)
>  
>  static u16 r8153_phy_status(struct r8152 *tp, u16 desired)
>  {
> -	u16 data;
> +	u16 data = 0;
>  	int i;
>  
>  	for (i = 0; i < 500; i++) {
> -		data = ocp_reg_read(tp, OCP_PHY_STATUS);
> +		if (ocp_reg_read(tp, OCP_PHY_STATUS, &data) < 0)
> +			break;
> +

[Severity: Medium]
Previously this loop kept polling for up to 500 * 20 ms. Now one failed read
ends the wait. If that read is the first, the function returns 0, which
matches no PHY_STAT_* value. r8157_init() does:

	data = r8153_phy_status(tp, 0);
	if (data == PHY_STAT_EXT_INIT) {
		ocp_reg_clr_bits(tp, 0xa468, BIT(3) | BIT(1));
		ocp_reg_clr_bits(tp, 0xa466, BIT(0));
	}

Can a transient timeout during PHY boot skip this EXT_INIT release? The
PHY_STAT_LAN_ON wait that follows could end early the same way.

Would it be safer to keep polling on error, as before?

[ ... ]

> @@ -4587,7 +4750,8 @@ static inline void rtl_reset_ocp_base(struct r8152 *tp)
>  static int rtl_phy_patch_request(struct r8152 *tp, bool request, bool wait)
>  {
>  	u16 check;
> -	int i;
> +	u16 ocp_data;
> +	int i, ret;
>  
>  	if (request) {
>  		ocp_reg_set_bits(tp, OCP_PHY_PATCH_CMD, PATCH_REQUEST);

[Severity: High]
This read-modify-write still discards its status, and so does the
ocp_reg_clr_bits() in the release branch. If rtl_tgphy_access() drops the
PATCH_REQUEST update on a busy timeout, what does the handshake report?

With wait == false (the power_cut path through rtl_pre_ram_code() and
rtl_post_ram_code()), it returns 0 straight away.

With request == false and wait == true, PATCH_READY stays set. So
(ocp_data & PATCH_READY) ^ check is always 0 and the loop runs all 5000
iterations. The final check only runs when request && wait, so the function
then returns 0 even though the PHY never left patch mode.

With request == true, the dropped write only shows up as -ETIME after the
full poll.

The commit message lists the PHY patch handshake among the places that now
propagate status. Should these writes be checked as well?

> @@ -4598,25 +4762,29 @@ static int rtl_phy_patch_request(struct r8152 *tp, bool request, bool wait)
>  	}
>  
>  	for (i = 0; wait && i < 5000; i++) {
> -		u32 ocp_data;
> -
>  		if (test_bit(RTL8152_INACCESSIBLE, &tp->flags))
>  			return -ENODEV;
>  
>  		usleep_range(1000, 2000);
> -		ocp_data = ocp_reg_read(tp, OCP_PHY_PATCH_STAT);
> -		if ((ocp_data & PATCH_READY) ^ check)
> +		ret = ocp_reg_read(tp, OCP_PHY_PATCH_STAT, &ocp_data);
> +		if (ret < 0)
> +			return ret;
> +		else if ((ocp_data & PATCH_READY) ^ check)
>  			break;
>  	}
>  
> -	if (request && wait &&
> -	    !(ocp_reg_read(tp, OCP_PHY_PATCH_STAT) & PATCH_READY)) {
> -		dev_err(&tp->intf->dev, "PHY patch request fail\n");
> -		rtl_phy_patch_request(tp, false, false);
> -		return -ETIME;
> -	} else {
> -		return 0;
> +	if (request && wait) {
> +		ret = ocp_reg_read(tp, OCP_PHY_PATCH_STAT, &ocp_data);
> +		if (ret < 0) {
> +			return ret;
> +		} else if (!(ocp_data & PATCH_READY)) {
> +			dev_err(&tp->intf->dev, "PHY patch request fail\n");
> +			rtl_phy_patch_request(tp, false, false);
> +			return -ETIME;
> +		}
>  	}
> +
> +	return 0;
>  }

[Severity: Medium]
When request == true, both new return ret paths run after PATCH_REQUEST has
been set. Unlike the -ETIME path, they don't call
rtl_phy_patch_request(tp, false, false).

Callers such as r8157_hw_phy_cfg(), r8159_hw_phy_cfg() and
rtl_ram_code_speed_up() do:

	if (rtl_phy_patch_request(tp, true, true))
		return;

They never release the request. Can a single TGPHY timeout on the
OCP_PHY_PATCH_STAT read leave the PHY MCU halted in patch-request mode?

In r8157_hw_phy_cfg(), this early return also skips the green, EEE and ALDPS
setup and set_bit(PHY_RESET).

[ ... ]

> @@ -6601,7 +6779,11 @@ static int rtl8152_set_speed(struct r8152 *tp, u8 autoneg, u32 speed, u8 duplex,
>  		if (!advertising)
>  			return -EINVAL;
>  
> -		orig = r8152_mdio_read(tp, MII_ADVERTISE);
> +		ret = r8152_mdio_read(tp, MII_ADVERTISE);
> +		if (ret < 0)
> +			goto out;
> +
> +		orig = ret;
>  		new1 = orig & ~(ADVERTISE_10HALF | ADVERTISE_10FULL |
>  				ADVERTISE_100HALF | ADVERTISE_100FULL);
>  		if (advertising & RTL_ADVERTISED_10_HALF) {

[Severity: High]
The reads are now checked, but the advertisement writes further down still
ignore the result:

		if (orig != new1) {
			r8152_mdio_write(tp, MII_ADVERTISE, new1);
			tp->mii.advertising = new1;
		}

The MII_CTRL1000 write and ocp_reg_write(tp, OCP_10GBT_CTRL, new1) do the
same.

Suppose one of these TGPHY writes times out and the later accesses succeed.
ret is overwritten and the function returns 0.
rtl8152_set_link_ksettings() then saves tp->autoneg, tp->speed, tp->duplex
and tp->advertising as applied, and tp->mii.advertising is updated, while
the PHY keeps the old advertisement.

The commit message names rtl8152_set_speed() as covered. Should these writes
be checked too?

[ ... ]

> @@ -6673,20 +6862,23 @@ static int rtl8152_set_speed(struct r8152 *tp, u8 autoneg, u32 speed, u8 duplex,
>  	if (test_and_clear_bit(PHY_RESET, &tp->flags))
>  		bmcr |= BMCR_RESET;
>  
> -	r8152_mdio_write(tp, MII_BMCR, bmcr);
> +	ret = r8152_mdio_write(tp, MII_BMCR, bmcr);
> +	if (ret < 0)
> +		goto out;

[Severity: Medium]
PHY_RESET is cleared before this write. r8157_hw_phy_cfg() and
r8159_hw_phy_cfg() request the reset with set_bit(PHY_RESET). If the TGPHY
write is dropped on a timeout, is that reset lost? rtl_hw_phy_work_func_t()
ignores the return value of rtl8152_set_speed(), so nothing retries it.

There is a second case. The new goto out after the MII_CTRL1000 or
OCP_10GBT_CTRL read can run after MII_ADVERTISE, tp->mii.advertising and
tp->ups_info.speed_duplex have already been updated. The cached
tp->autoneg, tp->speed, tp->duplex and tp->advertising then no longer match
the partly programmed PHY. ups_info.speed_duplex is also used by
r8156_ups_flags().

[ ... ]

> @@ -7136,7 +7328,12 @@ static bool rtl8152_in_nway(struct r8152 *tp)
>  
>  static bool rtl8153_in_nway(struct r8152 *tp)
>  {
> -	u16 phy_state = ocp_reg_read(tp, OCP_PHY_STATE) & 0xff;
> +	u16 phy_state;
> +
> +	if (ocp_reg_read(tp, OCP_PHY_STATE, &phy_state) < 0)
> +		return false;

[Severity: Medium]
delay_autosuspend() only blocks suspend when in_nway() returns true:

	if (!sw_linking && tp->rtl_ops.in_nway(tp))
		return true;

Returning false on a read failure tells delay_autosuspend() that the PHY is
not negotiating. Its own comment says that suspending during nway may miss
the link change event, and the device then won't wake when the link comes
up.

On RTL8157/8159, can a TGPHY timeout here let the device runtime-suspend in
the middle of autonegotiation? Would returning true on error be the safer
default?

[ ... ]

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