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

[netdev-bot+sashiko@kernel.org]

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

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

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] set_carrier() (drivers/net/usb/r8152.c:7468) was not converted
  to the new error contract.
- [Medium] The commit message says rtl8152_get_link_ksettings() now
  propagates the error.
- [Medium] rtl8153_in_nway() (line 7431) now returns false when the
  OCP_PHY_STATE read fails, which means 'the PHY is not in…
- [Medium] r8152: negative errnos returned by read_mii_word() to MII
  library
- [Low] rtl8152_set_speed() (line 6835) can now return early on a failed
  PHY access, after it has already written some advertisement registers…

--

r8152: add TGPHY register access for RTL8157 and RTL8159

This patch adds a TGPHY command channel for PHY register access on RTL8157
and RTL8159. Access goes through new rtl_ops.phy_read/phy_write hooks.
A TGPHY access can fail, so the PHY accessors and their helpers now return
int, and a set of callers is updated to act on errors.

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

[ ... ]

> -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]
The added comment says a failed read must not reach the MII library
as an errno, but the two early returns at the top of read_mii_word()
still pass one on. The RTL8152_INACCESSIBLE check returns -ENODEV and
the phy_id check returns -EINVAL. Neither goes through the new
ret < 0 ? 0 : ret conversion. Those returns are not new, but with this
patch they no longer match the stated contract.

-ENODEV is 0xffffffed, which has BMCR_ANENABLE, BMSR_LSTATUS and
BMSR_ANEGCOMPLETE set. -EINVAL is 0xffffffea, which also has
BMCR_ANENABLE set. The MII library always passes tp->mii.phy_id,
though, so the -EINVAL path is mostly theoretical.

mii_nway_restart() tests BMCR_ANENABLE directly on the mdio_read()
result. Suppose RTL8152_INACCESSIBLE is set when rtl8152_nway_reset()
or rtl_ethtool_set_eee() calls it. The -ENODEV then reads as autoneg
enabled, write_mii_word() drops the restart without reporting it, and
mii_nway_restart() returns 0.

Neither caller checks the flag first. On RTL8157/RTL8159,
r8152_set_eee() can latch it through rtl_eee_enable() right before the
mii_nway_restart() call. Userspace is then told the renegotiation
succeeded on a device that is gone.

In mii_ethtool_get_link_ksettings() the two conventions get mixed. The
TGPHY access that latches the flag makes the MII_BMCR read return 0.
The MII_BMSR, MII_CTRL1000, MII_STAT1000 and MII_ADVERTISE reads after
it hit the early return and come back as 0xffed.

Should these early returns also return 0, so read_mii_word() follows
its own comment? Or, since a 0 BMCR has problems of its own, would it
be better for the callers of the MII library to check
RTL8152_INACCESSIBLE before calling into it?

[Severity: High]
With this change, r8152_mdio_read() can return a negative errno on
RTL8157/RTL8159. The failure comes from
ocp_reg_read()->r8157_phy_read()->rtl_tgphy_access().

Was set_carrier() meant to be converted too? It still treats the return
value as register content:

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_carrier_on(netdev);
			rtl_start_rx(tp);
	...
	} else {
		if (netif_carrier_ok(netdev)) {
			netif_carrier_off(netdev);
			...
			tp->rtl_ops.disable(tp);
	...
		if (rtl8152_get_speed(tp) & LINK_STATUS) {
			set_bit(RTL8152_LINK_CHG, &tp->flags);
			schedule_delayed_work(&tp->schedule, 0);
		}
	}
}

-ENODEV is 0xffffffed, which has BMSR_LSTATUS (bit 2) set. If the
device is gone and the carrier is off, set_carrier() would see link up.
It would then call rtl_ops.enable(), netif_carrier_on() and
rtl_start_rx().

-ETIMEDOUT is 0xffffff92, which has BMSR_LSTATUS clear. One TGPHY busy
timeout from wait_tgphy_cmd_ready() while the carrier is up would read
as link down. The carrier would go off and rtl_ops.disable() would run.
rtl8152_get_speed() still reports LINK_STATUS, so LINK_CHG is
rescheduled. Wouldn't that give a carrier off/on flap?

set_carrier() runs from rtl_work_func_t() on every link change, and that
function checks RTL8152_INACCESSIBLE only on entry. set_carrier() is
missing from both lists in the commit message: the converted callers and
the callers left for the follow-up series.

[ ... ]

> @@ -6726,12 +6929,19 @@ static int rtl8152_set_speed(struct r8152 *tp, u8 autoneg, u32 speed, u8 duplex,
>  		}
>  
>  		if (orig != new1) {
> -			r8152_mdio_write(tp, MII_ADVERTISE, new1);
> +			ret = r8152_mdio_write(tp, MII_ADVERTISE, new1);
> +			if (ret < 0)
> +				goto out;
> +
>  			tp->mii.advertising = new1;
>  		}
>  
>  		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);

[ ... ]

> @@ -6776,20 +6995,26 @@ 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) {
> +		if (bmcr & BMCR_RESET)
> +			set_bit(PHY_RESET, &tp->flags);
> +		goto out;
> +	}

[Severity: Low]
The commit message says a failed write in rtl8152_set_speed() leaves the
cached advertising and a pending PHY reset untouched. Does that cover all
the state changed before these early exits?

In the autoneg branch, MII_ADVERTISE may already be written when the
MII_CTRL1000 or OCP_10GBT_CTRL access fails. tp->mii.advertising and
tp->ups_info.speed_duplex may already be updated too. The early return
skips BMCR_ANRESTART. The PHY then keeps the new 10/100 advertisement
together with the old 1000/multi-gig one, and the next renegotiation
uses that mix.

In the forced branch, tp->mii.force_media, tp->mii.full_duplex and
tp->ups_info.speed_duplex are set before the MII_BMCR write. If that
write fails, only PHY_RESET is restored, and the PHY is still
autonegotiating.

Since ret is non-zero, rtl8152_set_link_ksettings() keeps the old
tp->autoneg/speed/duplex/advertising. Later in the series,
r8157_ups_en() calls r8156_ups_flags(), which writes
tp->ups_info.speed_duplex into USB_UPS_FLAGS. The stale value would then
reach the UPS firmware as well.

Should this partly applied state be rolled back, or should the commit
message describe what is left behind?

[ ... ]

> @@ -7205,7 +7430,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]
Is false the safe answer when the read fails? RTL8157/RTL8159 use
rtl8153_in_nway() as ops->in_nway. delay_autosuspend() relies on it to
avoid suspending while the PHY is negotiating:

drivers/net/usb/r8152.c:delay_autosuspend() {
	...
	/* If the linking down is occurred by nway, the device may miss the
	 * linking change event. And it wouldn't wake when linking on.
	 */
	if (!sw_linking && tp->rtl_ops.in_nway(tp))
		return true;
	...
}

A TGPHY timeout while the PHY is mid-negotiation would let runtime
suspend go ahead. The adapter could then miss the link-up wake event
and stay suspended.

Before this patch, a failed control transfer filled the data with 0xff
through get_registers(). The low byte then read as "in nway", which
delayed suspend. Would returning true on a failed read be the safer
choice here?

[ ... ]

> @@ -9388,8 +9621,16 @@ int rtl8152_get_link_ksettings(struct net_device *netdev,
>  			 cmd->link_modes.supported, tp->support_10000full);
>  
>  	if (tp->support_2500full || tp->support_5000full || tp->support_10000full) {
> -		u16 ocp_10gbt_ctrl = ocp_reg_read(tp, OCP_10GBT_CTRL);
> -		u16 ocp_10gbt_stat = ocp_reg_read(tp, OCP_10GBT_STAT);
> +		u16 ocp_10gbt_ctrl;
> +		u16 ocp_10gbt_stat;
> +
> +		ret = ocp_reg_read(tp, OCP_10GBT_CTRL, &ocp_10gbt_ctrl);
> +		if (ret < 0)
> +			goto out_unlock;
> +
> +		ret = ocp_reg_read(tp, OCP_10GBT_STAT, &ocp_10gbt_stat);
> +		if (ret < 0)
> +			goto out_unlock;

[Severity: Medium]
The commit message says rtl8152_get_link_ksettings() propagates the
error. Does that apply only to these two OCP_10GBT reads?

Earlier in this function, mii_ethtool_get_link_ksettings() reads BMCR,
BMSR, CTRL1000, STAT1000, ADVERTISE and LPA through tp->mii.mdio_read,
which is read_mii_word(). After this patch, read_mii_word() turns a
failed read into 0:

	return ret < 0 ? 0 : ret;

A BMCR of 0 looks like autoneg off with forced 10M half duplex.
mii_ethtool_get_link_ksettings() also stores that duplex into
tp->mii.full_duplex. Suppose the MII reads fail but the 10GBT reads
succeed. Can ethtool then return success with a made-up forced 10M/half
configuration?

The same 0 makes mii_nway_restart() return -EINVAL (autoneg off) instead
of the I/O error. That affects rtl8152_nway_reset() and the eee_set path.

[ ... ]

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