netdev - Re: [PATCH net v2 6/6] igc: Fix Energy Efficient Ethernet support declaration

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]

Date: Wed, 5 Jun 2024 08:13:25 +0300
From: Sasha Neftin <sasha.neftin@...el.com>
To: Hariprasad Kelam <hkelam@...vell.com>, Jacob Keller
	<jacob.e.keller@...el.com>, Jakub Kicinski <kuba@...nel.org>, netdev
	<netdev@...r.kernel.org>, David Miller <davem@...emloft.net>
CC: Dima Ruinskiy <dima.ruinskiy@...el.com>, Naama Meir
	<naamax.meir@...ux.intel.com>
Subject: Re: [PATCH net v2 6/6] igc: Fix Energy Efficient Ethernet support
 declaration

On 04/06/2024 10:08, Hariprasad Kelam wrote:
> 
> 
>> From: Sasha Neftin <sasha.neftin@...el.com>
>>
>> The commit 01cf893bf0f4 ("net: intel: i40e/igc: Remove setting Autoneg in
>> EEE capabilities") removed SUPPORTED_Autoneg field but left inappropriate
>> ethtool_keee structure initialization. When "ethtool --show <device>"
>> (get_eee) invoke, the 'ethtool_keee' structure was accidentally overridden.
>> Remove the 'ethtool_keee' overriding and add EEE declaration as per IEEE
>> specification that allows reporting Energy Efficient Ethernet capabilities.
>>
>> Examples:
>> Before fix:
>> ethtool --show-eee enp174s0
>> EEE settings for enp174s0:
>> 	EEE status: not supported
>>
>> After fix:
>> EEE settings for enp174s0:
>> 	EEE status: disabled
>> 	Tx LPI: disabled
>> 	Supported EEE link modes:  100baseT/Full
>> 	                           1000baseT/Full
>> 	                           2500baseT/Full
>>
>> Fixes: 01cf893bf0f4 ("net: intel: i40e/igc: Remove setting Autoneg in EEE
>> capabilities")
>> Suggested-by: Dima Ruinskiy <dima.ruinskiy@...el.com>
>> Signed-off-by: Sasha Neftin <sasha.neftin@...el.com>
>> Tested-by: Naama Meir <naamax.meir@...ux.intel.com>
>> Signed-off-by: Jacob Keller <jacob.e.keller@...el.com>
>> ---
>>   drivers/net/ethernet/intel/igc/igc_ethtool.c | 9 +++++++--
>>   drivers/net/ethernet/intel/igc/igc_main.c    | 4 ++++
>>   2 files changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c
>> b/drivers/net/ethernet/intel/igc/igc_ethtool.c
>> index f2c4f1966bb0..0cd2bd695db1 100644
>> --- a/drivers/net/ethernet/intel/igc/igc_ethtool.c
>> +++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c
>> @@ -1629,12 +1629,17 @@ static int igc_ethtool_get_eee(struct net_device
>> *netdev,
>>   	struct igc_hw *hw = &adapter->hw;
>>   	u32 eeer;
>>
>> +	linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT,
>> +			 edata->supported);
>> +	linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT,
>> +			 edata->supported);
>> +	linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT,
>> +			 edata->supported);
>> +
>>   	if (hw->dev_spec._base.eee_enable)
>>   		mii_eee_cap1_mod_linkmode_t(edata->advertised,
>>   					    adapter->eee_advert);
>>
>> -	*edata = adapter->eee;
>> -
>>   	eeer = rd32(IGC_EEER);
>>
>>   	/* EEE status on negotiated link */
>> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c
>> b/drivers/net/ethernet/intel/igc/igc_main.c
>> index 12f004f46082..305e05294a26 100644
>> --- a/drivers/net/ethernet/intel/igc/igc_main.c
>> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
>> @@ -12,6 +12,7 @@
>>   #include <linux/bpf_trace.h>
>>   #include <net/xdp_sock_drv.h>
>>   #include <linux/pci.h>
>> +#include <linux/mdio.h>
>>
>>   #include <net/ipv6.h>
>>
>> @@ -4975,6 +4976,9 @@ void igc_up(struct igc_adapter *adapter)
>>   	/* start the watchdog. */
>>   	hw->mac.get_link_status = true;
>>   	schedule_work(&adapter->watchdog_task);
>> +
>> +	adapter->eee_advert = MDIO_EEE_100TX | MDIO_EEE_1000T |
>> +			      MDIO_EEE_2_5GT;
> 
>          Since advertised is supported here, does it make sense add below in " igc_ethtool_get_eee"

It is different. eee_advert is an internal field in the board-specific 
data structure. However, since i225/6 parts have only one PHY type 
(unlike the i210/1 parts family) and this field could be used only 
internally in a driver and FW, we probably can drop this assignment. I 
will double-check it and clean it up to eliminate duplication in a 
different patch.

> 
>          linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT,
>> +			 edata->advertisied);
> 
> Thanks,
> Hariprasad k
>>   }
>>
>>   /**
>>
>> --
>> 2.44.0.53.g0f9d4d28b7e6
>>
>