From c10a485c3de5ccbf1fff65a382cebcb2730c6b06 Mon Sep 17 00:00:00 2001 From: Andrew Lunn Date: Sun, 24 Oct 2021 21:48:02 +0200 Subject: [PATCH 1/4] phy: phy_ethtool_ksettings_get: Lock the phy for consistency The PHY structure should be locked while copying information out if it, otherwise there is no guarantee of self consistency. Without the lock the PHY state machine could be updating the structure. Fixes: 2d55173e71b0 ("phy: add generic function to support ksetting support") Signed-off-by: Andrew Lunn Signed-off-by: David S. Miller --- drivers/net/phy/phy.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c index f124a8a58bd4..8457b829667e 100644 --- a/drivers/net/phy/phy.c +++ b/drivers/net/phy/phy.c @@ -299,6 +299,7 @@ EXPORT_SYMBOL(phy_ethtool_ksettings_set); void phy_ethtool_ksettings_get(struct phy_device *phydev, struct ethtool_link_ksettings *cmd) { + mutex_lock(&phydev->lock); linkmode_copy(cmd->link_modes.supported, phydev->supported); linkmode_copy(cmd->link_modes.advertising, phydev->advertising); linkmode_copy(cmd->link_modes.lp_advertising, phydev->lp_advertising); @@ -317,6 +318,7 @@ void phy_ethtool_ksettings_get(struct phy_device *phydev, cmd->base.autoneg = phydev->autoneg; cmd->base.eth_tp_mdix_ctrl = phydev->mdix_ctrl; cmd->base.eth_tp_mdix = phydev->mdix; + mutex_unlock(&phydev->lock); } EXPORT_SYMBOL(phy_ethtool_ksettings_get); From 64cd92d5e8180c2ded3fdea76862de6f596ae2c9 Mon Sep 17 00:00:00 2001 From: Andrew Lunn Date: Sun, 24 Oct 2021 21:48:03 +0200 Subject: [PATCH 2/4] phy: phy_ethtool_ksettings_set: Move after phy_start_aneg This allows it to make use of a helper which assume the PHY is already locked. Fixes: 2d55173e71b0 ("phy: add generic function to support ksetting support") Signed-off-by: Andrew Lunn Signed-off-by: David S. Miller --- drivers/net/phy/phy.c | 106 +++++++++++++++++++++--------------------- 1 file changed, 53 insertions(+), 53 deletions(-) diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c index 8457b829667e..ee584fa8b76d 100644 --- a/drivers/net/phy/phy.c +++ b/drivers/net/phy/phy.c @@ -243,59 +243,6 @@ static void phy_sanitize_settings(struct phy_device *phydev) } } -int phy_ethtool_ksettings_set(struct phy_device *phydev, - const struct ethtool_link_ksettings *cmd) -{ - __ETHTOOL_DECLARE_LINK_MODE_MASK(advertising); - u8 autoneg = cmd->base.autoneg; - u8 duplex = cmd->base.duplex; - u32 speed = cmd->base.speed; - - if (cmd->base.phy_address != phydev->mdio.addr) - return -EINVAL; - - linkmode_copy(advertising, cmd->link_modes.advertising); - - /* We make sure that we don't pass unsupported values in to the PHY */ - linkmode_and(advertising, advertising, phydev->supported); - - /* Verify the settings we care about. */ - if (autoneg != AUTONEG_ENABLE && autoneg != AUTONEG_DISABLE) - return -EINVAL; - - if (autoneg == AUTONEG_ENABLE && linkmode_empty(advertising)) - return -EINVAL; - - if (autoneg == AUTONEG_DISABLE && - ((speed != SPEED_1000 && - speed != SPEED_100 && - speed != SPEED_10) || - (duplex != DUPLEX_HALF && - duplex != DUPLEX_FULL))) - return -EINVAL; - - phydev->autoneg = autoneg; - - if (autoneg == AUTONEG_DISABLE) { - phydev->speed = speed; - phydev->duplex = duplex; - } - - linkmode_copy(phydev->advertising, advertising); - - linkmode_mod_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, - phydev->advertising, autoneg == AUTONEG_ENABLE); - - phydev->master_slave_set = cmd->base.master_slave_cfg; - phydev->mdix_ctrl = cmd->base.eth_tp_mdix_ctrl; - - /* Restart the PHY */ - phy_start_aneg(phydev); - - return 0; -} -EXPORT_SYMBOL(phy_ethtool_ksettings_set); - void phy_ethtool_ksettings_get(struct phy_device *phydev, struct ethtool_link_ksettings *cmd) { @@ -802,6 +749,59 @@ static int phy_poll_aneg_done(struct phy_device *phydev) return ret < 0 ? ret : 0; } +int phy_ethtool_ksettings_set(struct phy_device *phydev, + const struct ethtool_link_ksettings *cmd) +{ + __ETHTOOL_DECLARE_LINK_MODE_MASK(advertising); + u8 autoneg = cmd->base.autoneg; + u8 duplex = cmd->base.duplex; + u32 speed = cmd->base.speed; + + if (cmd->base.phy_address != phydev->mdio.addr) + return -EINVAL; + + linkmode_copy(advertising, cmd->link_modes.advertising); + + /* We make sure that we don't pass unsupported values in to the PHY */ + linkmode_and(advertising, advertising, phydev->supported); + + /* Verify the settings we care about. */ + if (autoneg != AUTONEG_ENABLE && autoneg != AUTONEG_DISABLE) + return -EINVAL; + + if (autoneg == AUTONEG_ENABLE && linkmode_empty(advertising)) + return -EINVAL; + + if (autoneg == AUTONEG_DISABLE && + ((speed != SPEED_1000 && + speed != SPEED_100 && + speed != SPEED_10) || + (duplex != DUPLEX_HALF && + duplex != DUPLEX_FULL))) + return -EINVAL; + + phydev->autoneg = autoneg; + + if (autoneg == AUTONEG_DISABLE) { + phydev->speed = speed; + phydev->duplex = duplex; + } + + linkmode_copy(phydev->advertising, advertising); + + linkmode_mod_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, + phydev->advertising, autoneg == AUTONEG_ENABLE); + + phydev->master_slave_set = cmd->base.master_slave_cfg; + phydev->mdix_ctrl = cmd->base.eth_tp_mdix_ctrl; + + /* Restart the PHY */ + phy_start_aneg(phydev); + + return 0; +} +EXPORT_SYMBOL(phy_ethtool_ksettings_set); + /** * phy_speed_down - set speed to lowest speed supported by both link partners * @phydev: the phy_device struct From 707293a56f95f8e7e0cfae008010c7933fb68973 Mon Sep 17 00:00:00 2001 From: Andrew Lunn Date: Sun, 24 Oct 2021 21:48:04 +0200 Subject: [PATCH 3/4] phy: phy_start_aneg: Add an unlocked version Split phy_start_aneg into a wrapper which takes the PHY lock, and a helper doing the real work. This will be needed when phy_ethtook_ksettings_set takes the lock. Fixes: 2d55173e71b0 ("phy: add generic function to support ksetting support") Signed-off-by: Andrew Lunn Signed-off-by: David S. Miller --- drivers/net/phy/phy.c | 46 ++++++++++++++++++++++++++++++------------- 1 file changed, 32 insertions(+), 14 deletions(-) diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c index ee584fa8b76d..d845afab1af7 100644 --- a/drivers/net/phy/phy.c +++ b/drivers/net/phy/phy.c @@ -699,6 +699,37 @@ static int phy_check_link_status(struct phy_device *phydev) return 0; } +/** + * _phy_start_aneg - start auto-negotiation for this PHY device + * @phydev: the phy_device struct + * + * Description: Sanitizes the settings (if we're not autonegotiating + * them), and then calls the driver's config_aneg function. + * If the PHYCONTROL Layer is operating, we change the state to + * reflect the beginning of Auto-negotiation or forcing. + */ +static int _phy_start_aneg(struct phy_device *phydev) +{ + int err; + + lockdep_assert_held(&phydev->lock); + + if (!phydev->drv) + return -EIO; + + if (AUTONEG_DISABLE == phydev->autoneg) + phy_sanitize_settings(phydev); + + err = phy_config_aneg(phydev); + if (err < 0) + return err; + + if (phy_is_started(phydev)) + err = phy_check_link_status(phydev); + + return err; +} + /** * phy_start_aneg - start auto-negotiation for this PHY device * @phydev: the phy_device struct @@ -712,21 +743,8 @@ int phy_start_aneg(struct phy_device *phydev) { int err; - if (!phydev->drv) - return -EIO; - mutex_lock(&phydev->lock); - - if (AUTONEG_DISABLE == phydev->autoneg) - phy_sanitize_settings(phydev); - - err = phy_config_aneg(phydev); - if (err < 0) - goto out_unlock; - - if (phy_is_started(phydev)) - err = phy_check_link_status(phydev); -out_unlock: + err = _phy_start_aneg(phydev); mutex_unlock(&phydev->lock); return err; From af1a02aa23c37045e6adfcf074cf7dbac167a403 Mon Sep 17 00:00:00 2001 From: Andrew Lunn Date: Sun, 24 Oct 2021 21:48:05 +0200 Subject: [PATCH 4/4] phy: phy_ethtool_ksettings_set: Lock the PHY while changing settings There is a race condition where the PHY state machine can change members of the phydev structure at the same time userspace requests a change via ethtool. To prevent this, have phy_ethtool_ksettings_set take the PHY lock. Fixes: 2d55173e71b0 ("phy: add generic function to support ksetting support") Reported-by: Walter Stoll Suggested-by: Walter Stoll Tested-by: Walter Stoll Signed-off-by: Andrew Lunn Signed-off-by: David S. Miller --- drivers/net/phy/phy.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c index d845afab1af7..a3bfb156c83d 100644 --- a/drivers/net/phy/phy.c +++ b/drivers/net/phy/phy.c @@ -798,6 +798,7 @@ 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) { @@ -814,8 +815,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_start_aneg(phydev); + mutex_unlock(&phydev->lock); return 0; } EXPORT_SYMBOL(phy_ethtool_ksettings_set);