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

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
r8152: add TGPHY register access for RTL8157 and RTL8159

This adds a TGPHY command channel backend for PHY register access on
RTL8157 and RTL8159. It is selected through new rtl_ops.phy_read and
phy_write hooks. The helpers built on ocp_reg_read()/ocp_reg_write() now
return int, and a subset of callers propagate the errors.

> This patch propagates the status only where an errno
> would otherwise be taken for register content or reported as
> success: the pause parameters, rtl8152_set_speed(),
> SIOCGMIIREG/SIOCSMIIREG, the PHY patch handshake and the PHY
> firmware version checks.
>
> 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.

[Severity: Medium]
Is this description accurate? Some of the callers listed as still
ignoring the status do act on it in this diff.

r8153_get_eee() returns an errno from each of its ocp_reg_read() calls:

	ret = ocp_reg_read(tp, OCP_EEE_ABLE, &val);
	if (ret < 0)
		goto out;

r8153_phy_status() now leaves its poll on the first failed read:

	if (ocp_reg_read(tp, OCP_PHY_STATUS, &data) < 0)
		break;

rtl8152_get_link_ksettings() is not in either list. It now fails the
whole request when the OCP_10GBT_CTRL or OCP_10GBT_STAT read fails.

rtl8153_in_nway(), r8156_mdio_force_mode(), r8156_eee_en() and
rtl8152_fw_phy_fixup() also return early on a read failure now.

Could the commit message be updated to cover these changes?

> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index 558ccb4682c27..7c0de56266963 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)

[ ... ]

> +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: Medium]
On RTL8157/RTL8159, can this pass -ETIMEDOUT to the generic MII library
as if it were a BMCR value? read_mii_word() is the mii_if_info.mdio_read
callback, and it returns the result of r8152_mdio_read() unchanged.

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. write_mii_word()
only filters RTL8152_INACCESSIBLE, and a TGPHY timeout does not set that
flag. So 0xff92 would be written to BMCR. That sets BMCR_RESET,
BMCR_LOOPBACK, BMCR_PDOWN and BMCR_ISOLATE, and the call still returns 0.

The path is:

rtl8152_nway_reset()
  mii_nway_restart()
    read_mii_word()
      r8152_mdio_read()
        r8157_phy_read()
          rtl_tgphy_access()
            wait_tgphy_cmd_ready() returns -ETIMEDOUT

rtl_ethtool_set_eee() also calls mii_nway_restart(). In addition,
mii_ethtool_get_link_ksettings() would decode link modes from the errno.

Before this patch, the only errno read_mii_word() could return was
-ENODEV, and write_mii_word() filters that case.

The commit message mentions this case, and the later patches in the series
do not seem to address it. rtl_ops_init() switches RTL_VER_16/17 to
r8157_phy_read() in this patch. Could read_mii_word()/write_mii_word()
handle the errno before or in this patch?

> @@ -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 TGPHY_CMD_BUSY stays set while USB transfers still work?
Each PHY access then waits up to 20ms here, or 40ms for a read. It also
prints another error that is not ratelimited. There is no recovery, and
RTL8152_INACCESSIBLE is never set.

Most callers ignore the status and keep going. r8157_hw_phy_cfg() makes
well over a hundred PHY accesses, and the firmware loaders do one write
per firmware word:

rtl8152_fw_phy_nc_apply() {
    ...
	for (i = 0; i < num; i++)
		ocp_reg_write(tp, OCP_SRAM_DATA, __le16_to_cpu(data[i]));
    ...
}

rtl8152_fw_phy_union_apply() has the same loop.

All of this runs with tp->control held in the probe, resume, open and
reset paths. Could this stall for seconds or minutes and flood the log?
Would ratelimiting the message, or stopping the sequence after the first
timeout, avoid that?

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

[Severity: Low]
This is a minor point. The status of the USB_TGPHY_DATA, ADDR and CMD
transfers is not checked here.

A misdirected command does not seem possible. r8152_control_msg() calls
rtl_set_inaccessible() when a control transfer fails, so a CMD write that
follows a failed ADDR write is never sent.

rtl_tgphy_access() can still return 0 once after a failed transfer,
either for a write or when the final USB_TGPHY_DATA read fails. Would it
be cleaner to return -ENODEV when RTL8152_INACCESSIBLE is set at the end
of the transaction?

> +
> +	if (!write) {
> +		ret = wait_tgphy_cmd_ready(tp);
> +		if (ret < 0)
> +			goto out;
> +
> +		*data = ocp_read_word(tp, MCU_TYPE_USB, USB_TGPHY_DATA);

[Severity: Medium]
This isn't a bug introduced by this patch, but ocp_read_word() ignores
the return value of generic_ocp_read():

ocp_read_word() {
    ...
	generic_ocp_read(tp, index, sizeof(tmp), &tmp, type | byen);

	data = __le32_to_cpu(tmp);
    ...
}

RTL8152_INACCESSIBLE could be set between the second
wait_tgphy_cmd_ready() and this read, for example by
rtl8152_disconnect()->rtl_set_unplug(). In that case generic_ocp_read()
returns -ENODEV early, tmp stays uninitialized, and rtl_tgphy_access()
returns 0. SIOCGMIIREG could then copy that stack data into
data->val_out.

dev_ioctl() requires CAP_NET_ADMIN for SIOCGMIIREG, so unprivileged users
cannot reach this. r8152_phy_read() already has the same flaw.

[ ... ]

> @@ -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]
On RTL8157/8159, one 20ms TGPHY busy timeout now ends this poll, which
previously ran for up to 500 x 20ms. The function then returns 0 or a
stale masked value. Is that safe for callers that switch on the result?

r8157_hw_phy_cfg() {
    ...
	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;
	}
    ...
}

A return of 0 falls into the default case. That calls
rtl8152_apply_firmware(tp, false) and skips the EXT_INIT release writes,
even when the PHY really is in PHY_STAT_EXT_INIT. r8159_hw_phy_cfg() and
r8157_init() follow the same pattern.

The later r8153_phy_status(tp, PHY_STAT_LAN_ON) call also stops after its
first failed read. The PFM, ALDPS and related settings are then
programmed before the PHY reaches LAN_ON, and WARN_ON_ONCE() fires.

Before this patch, the loop always ran to its full timeout on these
chips.

[ ... ]

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

[Severity: Medium]
When request is true, PATCH_REQUEST is already set in OCP_PHY_PATCH_CMD
by the time these two new return ret paths run. Do they leave it
asserted? The -ETIME path releases it with
rtl_phy_patch_request(tp, false, false), but the errno paths return
without doing that.

Callers treat any non-zero return as "not acquired" and do not release:

r8157_hw_phy_cfg() {
    ...
	if (rtl_phy_patch_request(tp, true, true))
		return;
    ...
}

r8159_hw_phy_cfg() and rtl_ram_code_speed_up() do the same. A TGPHY
timeout on the OCP_PHY_PATCH_STAT read would leave the PHY in patch
request mode. r8157_hw_phy_cfg() would also skip the rest of its
configuration.

[ ... ]

> @@ -6629,7 +6815,10 @@ static int rtl8152_set_speed(struct r8152 *tp, u8 autoneg, u32 speed, u8 duplex,
>  		}
>  
>  		if (tp->support_2500full || tp->support_5000full || tp->support_10000full) {
> -			orig = ocp_reg_read(tp, OCP_10GBT_CTRL);
> +			ret = ocp_reg_read(tp, OCP_10GBT_CTRL, &orig);
> +			if (ret < 0)
> +				goto out;
> +
>  			new1 = orig & ~(MDIO_AN_10GBT_CTRL_ADV2_5G | MDIO_AN_10GBT_CTRL_ADV5G
>  					| MDIO_AN_10GBT_CTRL_ADV10G);
>  

[Severity: Medium]
The reads are checked now, but the writes in rtl8152_set_speed() still
are not. The commit message names this function as one where an errno is
no longer reported as success:

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

If one of these writes times out on RTL8157/8159, tp->mii.advertising is
still updated. The function returns 0 as long as the BMCR write succeeds.
rtl8152_set_link_ksettings() then saves tp->advertising, tp->speed and
tp->duplex as if they had been applied.

The PHY patch handshake has the same gap. rtl_phy_patch_request() does
not check the result of the PATCH_REQUEST write:

	if (request) {
		ocp_reg_set_bits(tp, OCP_PHY_PATCH_CMD, PATCH_REQUEST);
		check = 0;
	} else {
		ocp_reg_clr_bits(tp, OCP_PHY_PATCH_CMD, PATCH_REQUEST);
		check = PATCH_READY;
	}

This has three effects:

- With wait false, a failed request or release returns 0. The firmware
  path uses this mode through rtl_pre_ram_code(..., !power_cut).
- With request true, a failed write polls for more than 5 seconds and
  then returns -ETIME instead of the real errno.
- With request false and wait true, a failed release runs all 5000
  iterations and still returns 0.

rtl_pre_ram_code() also turns any error into -ETIME, and
rtl8152_apply_firmware() ignores its return value.

Should these write results be checked as well?

> @@ -6660,20 +6849,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]
Is the pending PHY reset lost when this write fails? PHY_RESET is cleared
before the write. On RTL8157/8159, r8152_mdio_write() can now return
-ETIMEDOUT before anything is sent to the hardware:

rtl_tgphy_access() {
	...
	ret = wait_tgphy_cmd_ready(tp);
	if (ret < 0)
		goto out;
	...
}

This can happen if the preceding OCP_10GBT_CTRL write is still busy.

r8157_hw_phy_cfg() and r8159_hw_phy_cfg() end with
set_bit(PHY_RESET, &tp->flags) so that the next rtl8152_set_speed()
resets the PHY. If this write fails, BMCR_RESET never reaches the PHY and
the flag is not set again.

Should PHY_RESET be restored on this error path?

[ ... ]