Date:   Thu, 16 Sep 2021 13:08:21 +0000
From:   Walter Stoll <Walter.Stoll@...gon.com>
To:     "andrew@...n.ch" <andrew@...n.ch>,
        "f.fainelli@...il.com" <f.fainelli@...il.com>,
        "hkallweit1@...il.com" <hkallweit1@...il.com>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: [BUG] net/phy: ethtool versus phy_state_machine race condition

Effect observed
---------------

During final test of one of our products, we use ethtool to set the mode of
the ethernet port eth0 as follows:

ethtool -s eth0 speed 100 duplex full autoneg off

However, very rarely the command does not work as expected. Even though the
command executes without error, the port is not set accordingly. As a result,
the test fails.

We observed the effect with kernel version v5.4.138-rt62. However, we think
that the most recent kernel exhibits the same behavior because the structure of
the sources in question (see below) did not change. This also holds for the non
realtime kernel.


Root cause
----------

We found that there exists a race condition between ethtool and the PHY state-
machine.

Execution of the ethtool command involves the phy_ethtool_ksettings_set()
function being executed, see 
https://elixir.bootlin.com/linux/v5.4.138/source/drivers/net/phy/phy.c#L315

Here the mode is stored in the phydev structure. The phy_start_aneg() function
then takes the mode from the phydev structure and finally stores the mode into
the PHY.

However, the phy_ethtool_ksettings_set() function can be interrupted by the
phy_state_machine() worker thread. If this happens just before the
phy_start_aneg() function is called, then the new settings are overwritten by
the current settings of the PHY. This is because the phy_state_machine()
function reads back the PHY settings, see
https://elixir.bootlin.com/linux/v5.4.138/source/drivers/net/phy/phy.c#L918

This is exactly what happens in our case. We were able to proof this by
inserting various dev_info() calls in the code. 

Note, that we operate the PHY in polling mode. We are not sure if the effect
can occur also in interrupt mode.


Bug fix proposal
----------------

The phydev structure accessed inside the phy_ethtool_ksettings_set() function
must be locked. We applied a patch (see below), which resolved the issue.

Please note the following:
- Our code does not execute the phy_ethtool_sset() function, but we think there
    is exactly the same issue.
- The mutex should possibly be locked earlier (after variable definition ?).
- The phy_start_aneg() function cannot be called anymore (deadlock) inside the
    phy_ethtool_ksettings_set() and phy_ethtool_sset() functions.
- We replaced phy_start_aneg() by phy_config_aneg(). This might be too simple.
- The phy_ethtool_ksettings_get() function should possibly also lock the mutex.


diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index b0b8a3ce82b6..a21508da3848 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -252,6 +252,20 @@ static void phy_sanitize_settings(struct phy_device *phydev)
 	}
 }
 
+static int phy_config_aneg(struct phy_device *phydev)
+{
+	if (phydev->drv->config_aneg)
+		return phydev->drv->config_aneg(phydev);
+
+	/* Clause 45 PHYs that don't implement Clause 22 registers are not
+	 * allowed to call genphy_config_aneg()
+	 */
+	if (phydev->is_c45 && !(phydev->c45_ids.devices_in_package & BIT(0)))
+		return genphy_c45_config_aneg(phydev);
+
+	return genphy_config_aneg(phydev);
+}
+
 /**
  * phy_ethtool_sset - generic ethtool sset function, handles all the details
  * @phydev: target phy_device struct
@@ -292,6 +306,8 @@ int phy_ethtool_sset(struct phy_device *phydev, struct ethtool_cmd *cmd)
 	      cmd->duplex != DUPLEX_FULL)))
 		return -EINVAL;
 
+	mutex_lock(&phydev->lock);
+
 	phydev->autoneg = cmd->autoneg;
 
 	phydev->speed = speed;
@@ -306,7 +322,9 @@ int phy_ethtool_sset(struct phy_device *phydev, struct ethtool_cmd *cmd)
 	phydev->mdix_ctrl = cmd->eth_tp_mdix_ctrl;
 
 	/* Restart the PHY */
-	phy_start_aneg(phydev);
+	phy_config_aneg(phydev);
+
+	mutex_unlock(&phydev->lock);
 
 	return 0;
 }
@@ -343,6 +361,8 @@ int phy_ethtool_ksettings_set(struct phy_device *phydev,
 	      duplex != DUPLEX_FULL)))
 		return -EINVAL;
 
+	mutex_lock(&phydev->lock);
+
 	phydev->autoneg = autoneg;
 
 	if (autoneg == AUTONEG_DISABLE) {
@@ -358,7 +378,9 @@ int phy_ethtool_ksettings_set(struct phy_device *phydev,
 	phydev->mdix_ctrl = cmd->base.eth_tp_mdix_ctrl;
 
 	/* Restart the PHY */
-	phy_start_aneg(phydev);
+	phy_config_aneg(phydev);
+
+	mutex_unlock(&phydev->lock);
 
 	return 0;
 }
@@ -504,20 +526,6 @@ static void phy_trigger_machine(struct phy_device *phydev)
 	phy_queue_state_machine(phydev, 0);
 }
 
-static int phy_config_aneg(struct phy_device *phydev)
-{
-	if (phydev->drv->config_aneg)
-		return phydev->drv->config_aneg(phydev);
-
-	/* Clause 45 PHYs that don't implement Clause 22 registers are not
-	 * allowed to call genphy_config_aneg()
-	 */
-	if (phydev->is_c45 && !(phydev->c45_ids.devices_in_package & BIT(0)))
-		return genphy_c45_config_aneg(phydev);
-
-	return genphy_config_aneg(phydev);
-}
-
 /**
  * phy_check_link_status - check link status and set state accordingly
  * @phydev: the phy_device struct

Powered by blists - more mailing lists