netdev - Question about setting speed and duplex failed after auto-negotiation disabled on marvell phy

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 1 Mar 2019 11:26:14 +0000
From:   liweihang <liweihang@...ilicon.com>
To:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>
CC:     "davem@...emloft.net" <davem@...emloft.net>,
        linyunsheng <linyunsheng@...wei.com>,
        "Zhuangyuzeng (Yisen)" <yisen.zhuang@...wei.com>,
        Salil Mehta <salil.mehta@...wei.com>,
        "lipeng (Y)" <lipeng321@...wei.com>,
        "shenjian (K)" <shenjian15@...wei.com>
Subject: Question about setting speed and duplex failed after
 auto-negotiation disabled on marvell phy

Hi all,

We encountered a problem that if there are two devices in kernel v5.0 with
marvell phy 88E1510 connect with each other directly, one with autoneg on and
the other off. The one who has disabled auto-negotiation will failed on setting 
speed and duplex mode. Their speed and duplex mode will go to 10M/half. And
no matter what speed and duplex we set by ethtool -s ethx speed XX duplex full,
they will go to 10M/half. If we disable auto-negotiation on both sides and set
same speed and duplex, there speed and duplex mode will go to Unknown.

I found that m88e1121_config_aneg() has been modified by commit
d6ab93364734 net: phy: marvell: Avoid unnecessary soft reset
And in that patch, genphy_soft_reset() was moved below genphy_config_aneg(),
which caused value of speed and duplex in MII_BMCR was cleared. And then they
will go to 10M/half.

static int m88e1121_config_aneg(struct phy_device *phydev)
{
+       int changed = 0;
       int err = 0;

        if (phy_interface_is_rgmii(phydev)) {
@@ -487,15 +455,26 @@  static int m88e1121_config_aneg(struct phy_device *phydev)
                          return err;
       }

-        err = genphy_soft_reset(phydev);
+       err = marvell_set_polarity(phydev, phydev->mdix_ctrl);
       if (err < 0)
                return err;

-        err = marvell_set_polarity(phydev, phydev->mdix_ctrl);
+       changed = err;
+
+       err = genphy_config_aneg(phydev);
       if (err < 0)
                return err;

-        return genphy_config_aneg(phydev);
+       if (phydev->autoneg != autoneg || changed) {
+                /* A software reset is used to ensure a "commit" of the
+                * changes is done.
+                */
+                err = genphy_soft_reset(phydev);
+                if (err < 0)
+                          return err;
+       }
+
+       return 0;
}

I moved genphy_soft_reset back and it tested ok. And in my opinion, if we have to do
soft reset after genphy_config_aneg(phydev), it should be like this:

         if (phydev->autoneg != AUTONEG_ENABLE) {
                   int bmcr;

                   bmcr = phy_read(phydev, MII_BMCR);
                   if (bmcr < 0)
                            return bmcr;

                   err = phy_write(phydev, MII_BMCR, bmcr | BMCR_RESET);
                   if (err < 0)
                          return err;
       }

The above code has been mentioned in commit 3438634456c4
net: phy: marvell: Use core genphy_soft_reset(), phy_write() was replaced by
genphy_soft_reset() in that patch.

I think this issue will affect devices with Marvell PHY ID 88E1112/1111/1121/
1318/1240/1510/1540/1545/6390.

If there are any better info or suggestion regarding this problem, it would be very
helpful, thanks in advance.

reference:
[1] https://lore.kernel.org/patchwork/patch/991682/
[2] https://patchwork.ozlabs.org/patch/795435/