Re: [PATCH iwl-next v1] i40e: Add Energy Efficient Ethernet ability for X710 Base-T/KR/KX cards

[Przemek Kitszel <przemyslaw.kitszel@intel.com>]

On 8/9/24 17:25, Simon Horman wrote:
> On Thu, Aug 08, 2024 at 01:22:17PM +0200, Aleksandr Loktionov wrote:
> 
> ...
> 
>> diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
>> index 1d0d2e5..cd7509f 100644
>> --- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
>> +++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
>> @@ -5641,50 +5641,77 @@ static int i40e_get_module_eeprom(struct net_device *netdev,
>>   	return 0;
>>   }
>>   
>> +static void i40e_eee_capability_to_kedata_supported(__le16  eee_capability_,
>> +						    unsigned long *supported)
>> +{
>> +	const int eee_capability = le16_to_cpu(eee_capability_);
> 
> Hi Aleksandr,
> 
> Maybe u16 would be an appropriate type for eee_capability.
> Also, using const seems excessive here.
> 
>> +	static const int lut[] = {
>> +		ETHTOOL_LINK_MODE_100baseT_Full_BIT,
>> +		ETHTOOL_LINK_MODE_1000baseT_Full_BIT,
>> +		ETHTOOL_LINK_MODE_10000baseT_Full_BIT,
>> +		ETHTOOL_LINK_MODE_1000baseKX_Full_BIT,
>> +		ETHTOOL_LINK_MODE_10000baseKX4_Full_BIT,
>> +		ETHTOOL_LINK_MODE_10000baseKR_Full_BIT,
>> +		ETHTOOL_LINK_MODE_40000baseKR4_Full_BIT,
>> +	};
>> +
>> +	linkmode_zero(supported);
>> +	for (unsigned int i = ARRAY_SIZE(lut); i--; )
>> +		if (eee_capability & (1 << (i + 1)))
> 
> Perhaps:
> 
> 		if (eee_capability & BIT(i + 1))

I would avoid any operations with @i other than using it as index:
lut[i]. We have link mode bits in the table, why to compute that again?

> 
>> +			linkmode_set_bit(lut[i], supported);
>> +}
>> +
>>   static int i40e_get_eee(struct net_device *netdev, struct ethtool_keee *edata)
>>   {
>>   	struct i40e_netdev_priv *np = netdev_priv(netdev);
>>   	struct i40e_aq_get_phy_abilities_resp phy_cfg;
>>   	struct i40e_vsi *vsi = np->vsi;
>>   	struct i40e_pf *pf = vsi->back;
>>   	struct i40e_hw *hw = &pf->hw;
>> -	int status = 0;
>> +	int status;
> 
> This change seems unrelated to the subject of this patch.
> If so, please remove.

Hmm, it's remotely related, trivial, and makes code better;
I intentionally said nothing about this during our internal review ;)

--
// snip,
Aleksandr, please remember to address all issues pointed by Simon