Re: [PATCH net-next v1 2/7] net: phy: extend link mode support to 48 bits

[Ben Hutchings <ben@decadent.org.uk>]

[-- Attachment #1: Type: text/plain, Size: 2158 bytes --]

On Sun, 2015-01-04 at 12:56 -0800, David Decotigny wrote:
> From: David Decotigny <decot@googlers.com>
> 
> Signed-off-by: David Decotigny <decot@googlers.com>
> ---
>  drivers/net/phy/phy.c        | 29 ++++++++++++++---------------
>  drivers/net/phy/phy_device.c |  4 ++--
>  include/linux/phy.h          | 10 +++++-----
>  3 files changed, 21 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 767cd11..e9c8499 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
[...]
> @@ -245,7 +246,7 @@ static inline unsigned int phy_find_valid(unsigned int idx, u32 features)
>   */
>  static void phy_sanitize_settings(struct phy_device *phydev)
>  {
> -	u32 features = phydev->supported;
> +	ethtool_link_mode_mask_t features = phydev->supported;
>  	unsigned int idx;
>  
>  	/* Sanitize settings based on PHY capabilities */
> @@ -279,13 +280,13 @@ int phy_ethtool_sset(struct phy_device *phydev, struct ethtool_cmd *cmd)
>  		return -EINVAL;
>  
>  	/* We make sure that we don't pass unsupported values in to the PHY */
> -	cmd->advertising &= phydev->supported;
> +	phydev->advertising = ethtool_cmd_advertising(cmd) & phydev->supported;

phydev->advertising should not be changed until after the following
validation.  Use a local variable for this.

>  	/* Verify the settings we care about. */
>  	if (cmd->autoneg != AUTONEG_ENABLE && cmd->autoneg != AUTONEG_DISABLE)
>  		return -EINVAL;
>  
> -	if (cmd->autoneg == AUTONEG_ENABLE && cmd->advertising == 0)
> +	if (cmd->autoneg == AUTONEG_ENABLE && phydev->advertising == 0)
>  		return -EINVAL;
>  
>  	if (cmd->autoneg == AUTONEG_DISABLE &&
> @@ -300,8 +301,6 @@ int phy_ethtool_sset(struct phy_device *phydev, struct ethtool_cmd *cmd)
>  
>  	phydev->speed = speed;
>  
> -	phydev->advertising = cmd->advertising;
> -
>  	if (AUTONEG_ENABLE == cmd->autoneg)
>  		phydev->advertising |= ADVERTISED_Autoneg;
>  	else
[...]

The assignment to phydev->advertising should probably remain here.

Ben.

-- 
Ben Hutchings
This sentence contradicts itself - no actually it doesn't.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]