Commit b4ab21f9 authored by David S. Miller's avatar David S. Miller
Browse files

Merge branch 'ksettings-locking-fixes' 



Andrew Lunn says:

====================
ksettings_{get|set} lock fixes

Walter Stoll <Walter.Stoll@duagon.com> reported a race condition
between "ethtool -s eth0 speed 100 duplex full autoneg off" and phylib
reading the current status from the PHY. Both ksetting_get and
ksetting_set fail the take the phydev mutex, and as a result, there is
a small window of time where the phydev members are not self
consistent.

Patch 1 fixes phy_ethtool_ksettings_get by adding the needed lock.
Patches 2 and 3 move code around and perform to refactoring, to allow
patch 4 to fix phy_ethtool_ksettings_set by added the lock.

Thanks go to Walter for the detailed origional report, suggested fix,
and testing of the proposed patches.
====================

Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
parents 95a359c9 af1a02aa

drivers/net/phy/phy.c

+81 −59
Original line number Diff line number Diff line
@@ -243,62 +243,10 @@ 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)

{

	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 +265,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);
@@ -751,7 +700,7 @@ static int phy_check_link_status(struct phy_device *phydev) 
}



/**

 * phy_start_aneg - start auto-negotiation for this PHY device

 * _phy_start_aneg - start auto-negotiation for this PHY device

 * @phydev: the phy_device struct

 *

 * Description: Sanitizes the settings (if we're not autonegotiating
@@ -759,25 +708,43 @@ static int phy_check_link_status(struct phy_device *phydev) 
 *   If the PHYCONTROL Layer is operating, we change the state to

 *   reflect the beginning of Auto-negotiation or forcing.

 */

int phy_start_aneg(struct phy_device *phydev)

static int _phy_start_aneg(struct phy_device *phydev)

{

	int err;



	lockdep_assert_held(&phydev->lock);



	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;

		return err;



	if (phy_is_started(phydev))

		err = phy_check_link_status(phydev);

out_unlock:



	return err;

}



/**

 * 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.

 */

int phy_start_aneg(struct phy_device *phydev)

{

	int err;



	mutex_lock(&phydev->lock);

	err = _phy_start_aneg(phydev);

	mutex_unlock(&phydev->lock);



	return err;
@@ -800,6 +767,61 @@ 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;



	mutex_lock(&phydev->lock);

	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);



	mutex_unlock(&phydev->lock);

	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