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

[Luiz Angelo Daros de Luca]

Thanks Oleksij for your patch.

> 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

I tested this patch and tried to reproduce the corner case Sashiko pointed out.
My device has a RTL8367S switch, using a straight-through cable
directly connecting lan1 and lan3.

# 1. Force speed to 100/full and disable autoneg on both ports
ethtool -s lan1 autoneg off speed 100 duplex full
ethtool -s lan3 autoneg off speed 100 duplex full
# 2. Establish a working link by setting MDI-X on lan1 and MDI on lan3
ethtool -s lan1 mdix on
ethtool -s lan3 mdix off
# (At this point, the link is UP)
# 3. Force MDI off on lan1
ethtool -s lan1 mdix off
# 4. The link goes down as expected
# ethtool lan1
Settings for lan1:
        Supported ports: [ TP MII ]
        Supported link modes:   10baseT/Half 10baseT/Full
                                100baseT/Half 100baseT/Full
                                1000baseT/Full
        Supported pause frame use: Symmetric Receive-only
        Supports auto-negotiation: Yes
        Supported FEC modes: Not reported
        Advertised link modes:  100baseT/Full
        Advertised pause frame use: Symmetric Receive-only
        Advertised auto-negotiation: No
        Advertised FEC modes: Not reported
        Speed: 100Mb/s
        Duplex: Full
        Port: Twisted Pair
        PHYAD: 3
        Transceiver: external
        Auto-negotiation: off
        MDI-X: off (forced)
        Supports Wake-on: d
        Wake-on: d
        Link detected: no
# ethtool -s lan1 mdix on
# 5. and it is back up
# ethtool lan1
Settings for lan1:
        Supported ports: [ TP MII ]
        Supported link modes:   10baseT/Half 10baseT/Full
                                100baseT/Half 100baseT/Full
                                1000baseT/Full
        Supported pause frame use: Symmetric Receive-only
        Supports auto-negotiation: Yes
        Supported FEC modes: Not reported
        Advertised link modes:  100baseT/Full
        Advertised pause frame use: Symmetric Receive-only
        Advertised auto-negotiation: No
        Advertised FEC modes: Not reported
        Speed: 100Mb/s
        Duplex: Full
        Port: Twisted Pair
        PHYAD: 3
        Transceiver: external
        Auto-negotiation: off
        MDI-X: on (forced)
        Supports Wake-on: d
        Wake-on: d
        Link detected: yes

At least for me, I couldn't reproduce any issue.

Tested-by: Luiz Angelo Daros de Luca <luizluca@xxxxxxxxx>