Re: [PATCH 7/8] net: intel: igb: Use linkmode helpers for EEE

[Maxime Chevallier]

Hello Andrew,

On Sun, 04 Feb 2024 17:40:24 -0600
Andrew Lunn <andrew@xxxxxxx> wrote:

> Make use of the existing linkmode helpers for converting PHY EEE
> register values into links modes, now that ethtool_keee uses link
> modes, rather than u32 values.
> 
> Signed-off-by: Andrew Lunn <andrew@xxxxxxx>

[...]

> @@ -3109,6 +3111,8 @@ static int igb_set_eee(struct net_device *netdev,
>  		       struct ethtool_keee *edata)
>  {
>  	struct igb_adapter *adapter = netdev_priv(netdev);
> +	__ETHTOOL_DECLARE_LINK_MODE_MASK(supported) = {};
> +	__ETHTOOL_DECLARE_LINK_MODE_MASK(tmp) = {};
>  	struct e1000_hw *hw = &adapter->hw;
>  	struct ethtool_keee eee_curr;
>  	bool adv1g_eee = true, adv100m_eee = true;
> @@ -3138,14 +3142,21 @@ static int igb_set_eee(struct net_device *netdev,
>  			return -EINVAL;
>  		}
>  
> -		if (!edata->advertised_u32 || (edata->advertised_u32 &
> -		    ~(ADVERTISE_100_FULL | ADVERTISE_1000_FULL))) {
> +		linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT,
> +				 supported);
> +		linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT,
> +				 supported);
> +		if (linkmode_andnot(tmp, edata->advertised, supported)) {
>  			dev_err(&adapter->pdev->dev,
>  				"EEE Advertisement supports only 100Tx and/or 100T full duplex\n");
>  			return -EINVAL;
>  		}
> -		adv100m_eee = !!(edata->advertised_u32 & ADVERTISE_100_FULL);
> -		adv1g_eee = !!(edata->advertised_u32 & ADVERTISE_1000_FULL);
> +		adv100m_eee = linkmode_test_bit(
> +			ETHTOOL_LINK_MODE_100baseT_Full_BIT,
> +			edata->advertised);
> +		adv1g_eee = linkmode_test_bit(
> +			ETHTOOL_LINK_MODE_100baseT_Full_BIT,

Probably a typo, I think it should be ETHTOOL_LINK_MODE_1000baseT_Full_BIT
here :)

> +			edata->advertised);
>  
>  	} else if (!edata->eee_enabled) {
>  		dev_err(&adapter->pdev->dev,
> @@ -3153,7 +3164,7 @@ static int igb_set_eee(struct net_device *netdev,
>  		return -EINVAL;
>  	}
>  
> -	adapter->eee_advert = ethtool_adv_to_mmd_eee_adv_t(edata->advertised_u32);
> +	adapter->eee_advert = linkmode_to_mii_eee_cap1_t(edata->advertised);
>  	if (hw->dev_spec._82575.eee_disable != !edata->eee_enabled) {
>  		hw->dev_spec._82575.eee_disable = !edata->eee_enabled;
>  		adapter->flags |= IGB_FLAG_EEE;
> 

Thanks,

Maxime