[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <aefc6b92-4458-4697-a8ae-4625d610d00b@intel.com>
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
>>
>
Powered by blists - more mailing lists