Re: [PATCH net-next v1 1/1] net: phy: realtek: add MDI-X support for RTL8365MB-VC

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 1 · 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).

- [Medium] Changing MDI/MDI-X on a port that is forced to 10 or 100 Mb/s
  does not restart the link, so the new setting is not applied.

--

net: phy: realtek: add MDI-X support for RTL8365MB-VC

This commit adds config_aneg and read_status hooks for the RTL8365MB-VC
internal PHY. They let the MDI/MDI-X crossover mode be forced through a
chip-specific control register and report the resolved MDI-X state to
ethtool.

> diff --git a/drivers/net/phy/realtek/realtek_main.c b/drivers/net/phy/realtek/realtek_main.c
> index 3b3352e2cdc6a..e14960192e791 100644
> --- a/drivers/net/phy/realtek/realtek_main.c
> +++ b/drivers/net/phy/realtek/realtek_main.c

[ ... ]

> @@ -3058,6 +3068,74 @@ static irqreturn_t rtl8221b_handle_interrupt(struct phy_device *phydev)
>  	return IRQ_HANDLED;
>  }
>  
> +static int rtl8365mb_config_mdix(struct phy_device *phydev)
> +{

[ ... ]

> +	return phy_modify_changed(phydev, RTL8365MB_VC_PHYCR1,
> +				  RTL8365MB_VC_PHYCR1_MDIX_FORCE |
> +				  RTL8365MB_VC_PHYCR1_MDI, val);
> +}
> +
> +static int rtl8365mb_config_aneg(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	ret = rtl8365mb_config_mdix(phydev);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* The pair assignment is only evaluated while the link is brought up,
> +	 * so renegotiate if the crossover configuration changed.
> +	 */
> +	return __genphy_config_aneg(phydev, ret);

[Severity: Medium]
Does this renegotiate when the port is forced to 10 or 100 Mb/s?

When autoneg is disabled and the speed is below SPEED_1000,
__genphy_config_aneg() returns early and drops the changed argument:

drivers/net/phy/phy_device.c:__genphy_config_aneg() {
    ...
	} else if (phydev->speed < SPEED_1000) {
		return genphy_setup_forced(phydev);
	} else {
    ...
	return genphy_check_and_restart_aneg(phydev, changed);
}

genphy_setup_forced() only calls phy_modify() on MII_BMCR. If speed and
duplex are unchanged, __mdiobus_modify_changed() skips the write. Nothing
then restarts the link: there is no ANRESTART, no soft reset and no power
cycle.

For example, take a link that is up at a forced 100/full:

  ethtool -s <dev> autoneg off speed 100 duplex full
  ethtool -s <dev> mdix on

This goes through:

phy_ethtool_ksettings_set()->phy_start_aneg()->rtl8365mb_config_aneg()->
    __genphy_config_aneg()->genphy_setup_forced()

PHYCR1 bits 9:8 get updated. According to the comment above, though, the
pair assignment is not re-evaluated until the link drops for some other
reason. This part depends on the hardware behaviour that comment describes.

In the meantime, rtl8365mb_read_status() reads the new mode back from
PHYCR1 and reports it as mdix_ctrl. That no longer matches the pair
assignment actually in use.

The eth_tp_mdix_ctrl description in include/uapi/linux/ethtool.h says:
"When written successfully, the link should be renegotiated if necessary."

Would it help to handle the forced case the way marvell.c does? That
driver calls genphy_soft_reset() when "phydev->autoneg != AUTONEG_ENABLE ||
changed".

> +}
> +
> +static int rtl8365mb_read_status(struct phy_device *phydev)
> +{

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260929092007.1184843-1-o.rempel%40pengutronix.de