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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] net: r8152: read_mii_word() propagates negative error codes to MII library
- [Medium] net: usb: r8152: silent divergence of software and hardware states on MII_ADVERTISE write failure
--

commit 1a66cb27853181290552776e26318bd7353533ff
Author: Chih Kai Hsu <hsu.chih.kai@realtek.com>
Subject: r8152: add TGPHY register access for RTL8157 and RTL8159

This commit introduces TGPHY command channel access for RTL8157 and RTL8159
chips, adding busy timeouts to register accesses. It updates the PHY accessors
to return integer error codes and propagates these statuses in several places,
while deferring other callers to a follow-up series.

> 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)
[ ... ]
> -static inline int r8152_mdio_read(struct r8152 *tp, u32 reg_addr)
> +static int r8152_mdio_read(struct r8152 *tp, u32 reg_addr)
>  {
> -	return ocp_reg_read(tp, OCP_BASE_MII + reg_addr * 2);
> +	u16 data;
> +	int ret;
> +
> +	ret = ocp_reg_read(tp, OCP_BASE_MII + reg_addr * 2, &data);
> +
> +	return ret < 0 ? ret : data;
>  }

[Severity: High]
Does this introduce a regression by propagating negative error codes to the
MII library before the follow-up series is merged?

If a TGPHY access times out, this will return a negative error code
(like -ETIMEDOUT). This propagates back to read_mii_word(), which is used
as the MII library's mdio_read callback.

The MII library callbacks (e.g., mii_ethtool_get_link_ksettings, mii_link_ok)
expect a 16-bit register value and do not check for negative errors. They will
silently cast the negative integer to u16. A negative error code will have
spurious bits set, causing the MII library to misinterpret the link state
(e.g., falsely assuming BMCR_ANENABLE is set or BMSR_LSTATUS is cleared),
leading to silent link state corruption.

[ ... ]
> @@ -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);

[ ... ]
> @@ -6628,7 +6810,11 @@ static int rtl8152_set_speed(struct r8152 *tp, u8 autoneg, u32 speed, u8 duplex,
>  		}
>  
>  		if (tp->mii.supports_gmii) {
> -			orig = r8152_mdio_read(tp, MII_CTRL1000);
> +			ret = r8152_mdio_read(tp, MII_CTRL1000);
> +			if (ret < 0)
> +				goto out;
> +
> +			orig = ret;
>  			new1 = orig & ~(ADVERTISE_1000FULL |
>  					ADVERTISE_1000HALF);

[Severity: Medium]
Are we missing error checks for the subsequent writes after these reads?

Looking at rtl8152_set_speed(), the return values of intermediate writes to
MII_ADVERTISE, MII_CTRL1000, and OCP_10GBT_CTRL are ignored:

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

If one of these writes fails due to a TGPHY timeout, the software state
variables (like tp->mii.advertising and tp->ups_info.speed_duplex) are still
unconditionally updated. 

If the final write to MII_BMCR succeeds, the function returns 0 (success).
This causes the hardware to restart autonegotiation with old settings while
the driver falsely reports the new settings to userspace via ethtool. Could
this lead to a silent divergence between software and hardware states?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008081556.47901-1-nic_swsd@realtek.com?part=8