[RFC] net: phy: allow ethtool to set any supported fixed speed

phylib restricts the fixed speed to 1000, 100 or 10Mbps, even if the
PHY supports other speeds, or doesn't even support these speeds.
Validate the fixed speed against the PHY capabilities, and return an
error if we are unable to find a match.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
NOTE: is this correct behaviour - or should we do something like:

	s = phy_lookup_setting(speed, duplex, phydev->support, false);
	if (!s)
		return -EINVAL;

	phydev->speed = speed;
	phydev->duplex = duplex;

IOW, set either an exact match, or a slower supported speed than was
requested, or the slowest?  That's how phy_sanitize_settings() is
implemented, which I replicated for phylink's ethtool implementation.

Another issue here is with the validation of the settings that the
user passed in - this looks very racy.  Consider the following:

- another thread calls phy_ethtool_ksettings_set(), which sets
  phydev->speed and phydev->duplex, and disables autoneg.
- the phylib state machine is running, and overwrites the
  phydev->speed and phydev->duplex settings
- phy_ethtool_ksettings_set() then calls phy_start_aneg() which
  sets the PHY up with the phylib-read settings rather than the
  settings the user requested.

IMHO, phylib needs to keep the user requested settings separate from
the readback state from the PHY.

Yet another issue is what to do when a PHY doesn't support disabled
autoneg (or it's not known how to make it work) - the PHY driver
doesn't get a look-in to validate the settings, phylib just expects
every PHY out there to support it.  The best the PHY driver can do
is to cause it's config_aneg() method to return -EINVAL, dropping
the PHY state machine into PHY_HALTED mode via phy_error() - which
will then provoke a nice big stack dump in phy_stop() when the
network device is downed as phy_is_started() will return false.
Clearly not a good user experience on any level (API or kernel
behaviour.)

 drivers/net/phy/phy.c | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

On Fri, Nov 22, 2019 at 03:18:10PM +0000, Russell King wrote:
> phylib restricts the fixed speed to 1000, 100 or 10Mbps, even if the
> PHY supports other speeds, or doesn't even support these speeds.
> Validate the fixed speed against the PHY capabilities, and return an
> error if we are unable to find a match.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
> NOTE: is this correct behaviour - or should we do something like:
> 
> 	s = phy_lookup_setting(speed, duplex, phydev->support, false);
> 	if (!s)
> 		return -EINVAL;
> 
> 	phydev->speed = speed;
> 	phydev->duplex = duplex;

Sorry, that should've been:

	phydev->speed = s->speed;
	phydev->duplex = s->duplex;

> 
> IOW, set either an exact match, or a slower supported speed than was
> requested, or the slowest?  That's how phy_sanitize_settings() is
> implemented, which I replicated for phylink's ethtool implementation.
> 
> Another issue here is with the validation of the settings that the
> user passed in - this looks very racy.  Consider the following:
> 
> - another thread calls phy_ethtool_ksettings_set(), which sets
>   phydev->speed and phydev->duplex, and disables autoneg.
> - the phylib state machine is running, and overwrites the
>   phydev->speed and phydev->duplex settings
> - phy_ethtool_ksettings_set() then calls phy_start_aneg() which
>   sets the PHY up with the phylib-read settings rather than the
>   settings the user requested.
> 
> IMHO, phylib needs to keep the user requested settings separate from
> the readback state from the PHY.
> 
> Yet another issue is what to do when a PHY doesn't support disabled
> autoneg (or it's not known how to make it work) - the PHY driver
> doesn't get a look-in to validate the settings, phylib just expects
> every PHY out there to support it.  The best the PHY driver can do
> is to cause it's config_aneg() method to return -EINVAL, dropping
> the PHY state machine into PHY_HALTED mode via phy_error() - which
> will then provoke a nice big stack dump in phy_stop() when the
> network device is downed as phy_is_started() will return false.
> Clearly not a good user experience on any level (API or kernel
> behaviour.)
> 
>  drivers/net/phy/phy.c | 29 +++++++++++++++--------------
>  1 file changed, 15 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 9e431b9f9d87..75d11c48afce 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -270,31 +270,32 @@ int phy_ethtool_ksettings_set(struct phy_device *phydev,
>  	linkmode_and(advertising, advertising, phydev->supported);
>  
>  	/* Verify the settings we care about. */
> -	if (autoneg != AUTONEG_ENABLE && autoneg != AUTONEG_DISABLE)
> -		return -EINVAL;
> +	switch (autoneg) {
> +	case AUTONEG_ENABLE:
> +		if (linkmode_empty(advertising))
> +			return -EINVAL;
> +		break;
> +	
> +	case AUTONEG_DISABLE:
> +		if (duplex != DUPLEX_HALF && duplex != DUPLEX_FULL)
> +			return -EINVAL;
>  
> -	if (autoneg == AUTONEG_ENABLE && linkmode_empty(advertising))
> -		return -EINVAL;
> +		if (!phy_lookup_setting(speed, duplex, phydev->supported, true))
> +			return -EINVAL;
> +		break;
>  
> -	if (autoneg == AUTONEG_DISABLE &&
> -	    ((speed != SPEED_1000 &&
> -	      speed != SPEED_100 &&
> -	      speed != SPEED_10) ||
> -	     (duplex != DUPLEX_HALF &&
> -	      duplex != DUPLEX_FULL)))
> +	default:
>  		return -EINVAL;
> +	}
>  
>  	phydev->autoneg = autoneg;
> -
> +	phydev->duplex = duplex;
>  	phydev->speed = speed;
>  
>  	linkmode_copy(phydev->advertising, advertising);
> -
>  	linkmode_mod_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
>  			 phydev->advertising, autoneg == AUTONEG_ENABLE);
>  
> -	phydev->duplex = duplex;
> -
>  	phydev->mdix_ctrl = cmd->base.eth_tp_mdix_ctrl;
>  
>  	/* Restart the PHY */
> -- 
> 2.20.1
> 
>