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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] r8152: unchecked negative error from r8152_mdio_read() in
  set_carrier()
- [Medium] r8152: negative errnos returned by read_mii_word() to MII
  library
--

commit 1177dcaa1f0b492777f7f7b3e3e9ddb3fee5ac11
Author: Chih Kai Hsu <hsu.chih.kai@realtek.com>

r8152: add TGPHY register access for RTL8157 and RTL8159

This commit adds TGPHY register access hooks for RTL8157 and RTL8159 and
updates PHY register accessors and their callers to return and handle negative
error codes on failure.

> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index fad4473caf2a4..703ec5483778b 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c

[ ... ]

> @@ -1669,16 +1680,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 int ocp_reg_read(struct r8152 *tp, u16 addr, u16 *data)
> +{
> +	return tp->rtl_ops.phy_read(tp, addr, data);
> +}
> +
> +static int ocp_reg_write(struct r8152 *tp, u16 addr, u16 data)
> +{
> +	return tp->rtl_ops.phy_write(tp, addr, data);
>  }
>  
> -static inline void r8152_mdio_write(struct r8152 *tp, u32 reg_addr, u32 value)
> +static inline int r8152_mdio_write(struct r8152 *tp, u32 reg_addr, u32 value)
>  {
> -	ocp_reg_write(tp, OCP_BASE_MII + reg_addr * 2, value);
> +	return ocp_reg_write(tp, OCP_BASE_MII + reg_addr * 2, value);
>  }
>  
> -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: Medium]
Does set_carrier() also need to check for a negative return value from
r8152_mdio_read()?

drivers/net/usb/r8152.c:set_carrier() {
	...
	if (r8152_mdio_read(tp, MII_BMSR) & BMSR_LSTATUS) {
		if (!netif_carrier_ok(netdev)) {
			tp->rtl_ops.enable(tp);
			netif_stop_queue(netdev);
			napi_disable(napi);
			netif_carrier_on(netdev);
	...
}

When a TGPHY read of MII_BMSR fails on RTL8157 or RTL8159,
r8152_mdio_read() returns -ENODEV (-19, 0xffffffed) or -ETIMEDOUT (-110,
0xffffff92 on x86).  Because -ENODEV has BMSR_LSTATUS (0x0004) set,
set_carrier() treats an inaccessible device as having link up and calls
tp->rtl_ops.enable() and netif_carrier_on(), whereas a transient
-ETIMEDOUT with BMSR_LSTATUS clear tears down an active link.

[ ... ]

> @@ -1822,7 +1866,10 @@ static int read_mii_word(struct net_device *netdev, int phy_id, int reg)
>  
>  	ret = r8152_mdio_read(tp, reg);
>  
> -	return ret;
> +	/* The MII library takes the return value for register content,
> +	 * so a failed read must not be passed on as an errno.
> +	 */
> +	return ret < 0 ? 0 : ret;
>  }

[Severity: Medium]
As reported by Jakub Kicinski on v7 [1], can read_mii_word() still return
negative errnos to the MII library through its early checks?

drivers/net/usb/r8152.c:read_mii_word() {
	struct r8152 *tp = netdev_priv(netdev);
	int ret;

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

	if (phy_id != R8152_PHY_ID)
		return -EINVAL;
	...
}

If the first MII_BMCR read in mii_ethtool_get_link_ksettings() fails a
control transfer and latches RTL8152_INACCESSIBLE, read_mii_word() returns
0 for MII_BMCR, but the subsequent MII_BMSR, MII_CTRL1000, MII_STAT1000
and MII_ADVERTISE reads hit the early check and return -ENODEV (0xffffffed),
which the MII library decodes as register bits with link and advertising
flags set.  Likewise, calling mii_nway_restart() while RTL8152_INACCESSIBLE
is set sees BMCR_ANENABLE (0x1000) set in -ENODEV and returns 0 instead of
-EINVAL.

[1] https://lore.kernel.org/r/20261021005213.1255035-1-kuba@kernel.org

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