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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4e3602d2-6c6e-4311-b4fc-b3f8e2ce4da5@intel.com>
Date: Mon, 12 Aug 2024 10:09:37 +0200
From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
To: Simon Horman <horms@...nel.org>, Aleksandr Loktionov
	<aleksandr.loktionov@...el.com>
CC: <intel-wired-lan@...ts.osuosl.org>, <anthony.l.nguyen@...el.com>,
	<netdev@...r.kernel.org>, Arkadiusz Kubalewski
	<arkadiusz.kubalewski@...el.com>
Subject: Re: [PATCH iwl-next v1] i40e: Add Energy Efficient Ethernet ability
 for X710 Base-T/KR/KX cards

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