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

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

[Aleksandr Loktionov]

Add "EEE: Enabled/Disabled" to dmesg for supported X710 Base-T/KR/KX cards.
According to the IEEE standard report the EEE ability and and the
EEE Link Partner ability. Use the kernel's 'ethtool_keee' structure
and report EEE link modes.

Example:
dmesg | grep 'NIC Link is'
ethtool --show-eee <device>

Before:
	NIC Link is Up, 10 Gbps Full Duplex, Flow Control: None

        Supported EEE link modes:  Not reported
        Advertised EEE link modes:  Not reported
        Link partner advertised EEE link modes:  Not reported

After:
	NIC Link is Up, 10 Gbps Full Duplex, Flow Control: None, EEE: Enabled

        Supported EEE link modes:  100baseT/Full
                                   1000baseT/Full
                                   10000baseT/Full
        Advertised EEE link modes:  100baseT/Full
                                    1000baseT/Full
                                    10000baseT/Full
        Link partner advertised EEE link modes:  100baseT/Full
                                                 1000baseT/Full
                                                 10000baseT/Full

Reviewed-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
---
 drivers/net/ethernet/intel/i40e/i40e.h        |  1 +
 .../net/ethernet/intel/i40e/i40e_ethtool.c    | 37 ++++++++++++++++---
 drivers/net/ethernet/intel/i40e/i40e_main.c   | 25 +++++++++++--
 3 files changed, 55 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e.h b/drivers/net/ethernet/intel/i40e/i40e.h
index d546567..0f25a48 100644
--- a/drivers/net/ethernet/intel/i40e/i40e.h
+++ b/drivers/net/ethernet/intel/i40e/i40e.h
@@ -7,6 +7,7 @@
 #include <linux/pci.h>
 #include <linux/ptp_clock_kernel.h>
 #include <linux/types.h>
+#include <linux/linkmode.h>
 #include <linux/avf/virtchnl.h>
 #include <linux/net/intel/i40e_client.h>
 #include <net/devlink.h>
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_);
+	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)))
+			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;
 
 	/* Get initial PHY capabilities */
 	status = i40e_aq_get_phy_capabilities(hw, false, true, &phy_cfg, NULL);
 	if (status)
 		return -EAGAIN;
 
 	/* Check whether NIC configuration is compatible with Energy Efficient
 	 * Ethernet (EEE) mode.
 	 */
 	if (phy_cfg.eee_capability == 0)
 		return -EOPNOTSUPP;
 
+	i40e_eee_capability_to_kedata_supported(phy_cfg.eee_capability, edata->supported);
+	linkmode_copy(edata->lp_advertised, edata->supported);
+
 	/* Get current configuration */
 	status = i40e_aq_get_phy_capabilities(hw, false, false, &phy_cfg, NULL);
 	if (status)
 		return -EAGAIN;
 
+	linkmode_zero(edata->advertised);
+	if (phy_cfg.eee_capability)
+		linkmode_copy(edata->advertised, edata->supported);
 	edata->eee_enabled = !!phy_cfg.eee_capability;
 	edata->tx_lpi_enabled = pf->stats.tx_lpi_status;
 
 	edata->eee_active = pf->stats.tx_lpi_status && pf->stats.rx_lpi_status;
 
 	return 0;
 }
 
 static int i40e_is_eee_param_supported(struct net_device *netdev,
 				       struct ethtool_keee *edata)
 {
 	struct i40e_netdev_priv *np = netdev_priv(netdev);
 	struct i40e_vsi *vsi = np->vsi;
 	struct i40e_pf *pf = vsi->back;
 	struct i40e_ethtool_not_used {
-		u32 value;
+		int value;
 		const char *name;
 	} param[] = {
-		{edata->tx_lpi_timer, "tx-timer"},
+		{!!(edata->advertised[0] & ~edata->supported[0]), "advertise"},
+		{!!edata->tx_lpi_timer, "tx-timer"},
 		{edata->tx_lpi_enabled != pf->stats.tx_lpi_status, "tx-lpi"}
 	};
 	int i;
@@ -5710,7 +5737,7 @@ static int i40e_set_eee(struct net_device *netdev, struct ethtool_keee *edata)
 	struct i40e_pf *pf = vsi->back;
 	struct i40e_hw *hw = &pf->hw;
 	__le16 eee_capability;
-	int status = 0;
+	int status;
 
 	/* Deny parameters we don't support */
 	if (i40e_is_eee_param_supported(netdev, edata))
@@ -5754,7 +5781,7 @@ static int i40e_set_eee(struct net_device *netdev, struct ethtool_keee *edata)
 		config.eeer |= cpu_to_le32(I40E_PRTPM_EEER_TX_LPI_EN_MASK);
 	} else {
 		config.eee_capability = 0;
-		config.eeer &= cpu_to_le32(~I40E_PRTPM_EEER_TX_LPI_EN_MASK);
+		config.eeer &= ~cpu_to_le32(I40E_PRTPM_EEER_TX_LPI_EN_MASK);
 	}
 
 	/* Apply modified PHY configuration */
diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index cbcfada..55bbf0f 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -7263,6 +7263,22 @@ static int i40e_init_pf_dcb(struct i40e_pf *pf)
 	return err;
 }
 #endif /* CONFIG_I40E_DCB */
+static void i40e_print_link_message_eee(struct i40e_vsi *vsi, struct ethtool_keee *kedata,
+			    const char *speed, const char *fc)
+{
+	if (vsi->netdev->ethtool_ops->get_eee)
+		vsi->netdev->ethtool_ops->get_eee(vsi->netdev, kedata);
+
+	if (!linkmode_empty(kedata->supported))
+		netdev_info(vsi->netdev,
+			    "NIC Link is Up, %sbps Full Duplex, Flow Control: %s, EEE: %s\n",
+			    speed, fc,
+			    kedata->eee_enabled ? "Enabled" : "Disabled");
+	else
+		netdev_info(vsi->netdev,
+			    "NIC Link is Up, %sbps Full Duplex, Flow Control: %s\n",
+			    speed, fc);
+}
 
 /**
  * i40e_print_link_message - print link up or down
@@ -7395,9 +7411,12 @@ void i40e_print_link_message(struct i40e_vsi *vsi, bool isup)
 			    "NIC Link is Up, %sbps Full Duplex, Requested FEC: %s, Negotiated FEC: %s, Autoneg: %s, Flow Control: %s\n",
 			    speed, req_fec, fec, an, fc);
 	} else {
-		netdev_info(vsi->netdev,
-			    "NIC Link is Up, %sbps Full Duplex, Flow Control: %s\n",
-			    speed, fc);
+		struct ethtool_keee kedata;
+
+		linkmode_zero(kedata.supported);
+		kedata.eee_enabled = false;
+
+		i40e_print_link_message_eee(vsi, &kedata, speed, fc);
 	}
 
 }
-- 
2.25.1

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

[Simon Horman]

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))

> +			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.

>  
>  	/* Get initial PHY capabilities */
>  	status = i40e_aq_get_phy_capabilities(hw, false, true, &phy_cfg, NULL);
>  	if (status)
>  		return -EAGAIN;
>  
>  	/* Check whether NIC configuration is compatible with Energy Efficient
>  	 * Ethernet (EEE) mode.
>  	 */
>  	if (phy_cfg.eee_capability == 0)
>  		return -EOPNOTSUPP;
>  
> +	i40e_eee_capability_to_kedata_supported(phy_cfg.eee_capability, edata->supported);

Please line-wrap: Networking still prefers code to be 80 columns wide or less.

> +	linkmode_copy(edata->lp_advertised, edata->supported);
> +
>  	/* Get current configuration */
>  	status = i40e_aq_get_phy_capabilities(hw, false, false, &phy_cfg, NULL);
>  	if (status)
>  		return -EAGAIN;
>  
> +	linkmode_zero(edata->advertised);
> +	if (phy_cfg.eee_capability)
> +		linkmode_copy(edata->advertised, edata->supported);
>  	edata->eee_enabled = !!phy_cfg.eee_capability;
>  	edata->tx_lpi_enabled = pf->stats.tx_lpi_status;
>  
>  	edata->eee_active = pf->stats.tx_lpi_status && pf->stats.rx_lpi_status;
>  
>  	return 0;
>  }
>  
>  static int i40e_is_eee_param_supported(struct net_device *netdev,
>  				       struct ethtool_keee *edata)
>  {
>  	struct i40e_netdev_priv *np = netdev_priv(netdev);
>  	struct i40e_vsi *vsi = np->vsi;
>  	struct i40e_pf *pf = vsi->back;
>  	struct i40e_ethtool_not_used {
> -		u32 value;
> +		int value;

It is unclear to me that this type change is really related to the
subject of this patch. If not, please drop it. But if so, it
seems to me that bool would be the appropriate type.

>  		const char *name;
>  	} param[] = {
> -		{edata->tx_lpi_timer, "tx-timer"},
> +		{!!(edata->advertised[0] & ~edata->supported[0]), "advertise"},
> +		{!!edata->tx_lpi_timer, "tx-timer"},
>  		{edata->tx_lpi_enabled != pf->stats.tx_lpi_status, "tx-lpi"}
>  	};
>  	int i;
> @@ -5710,7 +5737,7 @@ static int i40e_set_eee(struct net_device *netdev, struct ethtool_keee *edata)
>  	struct i40e_pf *pf = vsi->back;
>  	struct i40e_hw *hw = &pf->hw;
>  	__le16 eee_capability;
> -	int status = 0;
> +	int status;

This change seems unrelated to the subject of this patch.
If so, please remove.

>  
>  	/* Deny parameters we don't support */
>  	if (i40e_is_eee_param_supported(netdev, edata))
> @@ -5754,7 +5781,7 @@ static int i40e_set_eee(struct net_device *netdev, struct ethtool_keee *edata)
>  		config.eeer |= cpu_to_le32(I40E_PRTPM_EEER_TX_LPI_EN_MASK);
>  	} else {
>  		config.eee_capability = 0;
> -		config.eeer &= cpu_to_le32(~I40E_PRTPM_EEER_TX_LPI_EN_MASK);
> +		config.eeer &= ~cpu_to_le32(I40E_PRTPM_EEER_TX_LPI_EN_MASK);

Ditto.

>  	}
>  
>  	/* Apply modified PHY configuration */
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
> index cbcfada..55bbf0f 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> @@ -7263,6 +7263,22 @@ static int i40e_init_pf_dcb(struct i40e_pf *pf)
>  	return err;
>  }
>  #endif /* CONFIG_I40E_DCB */
> +static void i40e_print_link_message_eee(struct i40e_vsi *vsi, struct ethtool_keee *kedata,
> +			    const char *speed, const char *fc)
> +{
> +	if (vsi->netdev->ethtool_ops->get_eee)
> +		vsi->netdev->ethtool_ops->get_eee(vsi->netdev, kedata);
> +
> +	if (!linkmode_empty(kedata->supported))
> +		netdev_info(vsi->netdev,
> +			    "NIC Link is Up, %sbps Full Duplex, Flow Control: %s, EEE: %s\n",
> +			    speed, fc,
> +			    kedata->eee_enabled ? "Enabled" : "Disabled");
> +	else
> +		netdev_info(vsi->netdev,
> +			    "NIC Link is Up, %sbps Full Duplex, Flow Control: %s\n",
> +			    speed, fc);
> +}
>  
>  /**
>   * i40e_print_link_message - print link up or down
> @@ -7395,9 +7411,12 @@ void i40e_print_link_message(struct i40e_vsi *vsi, bool isup)
>  			    "NIC Link is Up, %sbps Full Duplex, Requested FEC: %s, Negotiated FEC: %s, Autoneg: %s, Flow Control: %s\n",
>  			    speed, req_fec, fec, an, fc);
>  	} else {
> -		netdev_info(vsi->netdev,
> -			    "NIC Link is Up, %sbps Full Duplex, Flow Control: %s\n",
> -			    speed, fc);
> +		struct ethtool_keee kedata;
> +
> +		linkmode_zero(kedata.supported);
> +		kedata.eee_enabled = false;

Can the declaration of ethtool_keee be moved into
i40e_print_link_message_eee()? I suspect that would lead to
a cleaner implementation.

> +
> +		i40e_print_link_message_eee(vsi, &kedata, speed, fc);
>  	}
>  
>  }

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

[Przemek Kitszel]

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

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

[Loktionov, Aleksandr]

> -----Original Message-----
> From: Simon Horman <horms@kernel.org>
> Sent: Friday, August 9, 2024 5:26 PM
> To: Loktionov, Aleksandr <aleksandr.loktionov@intel.com>
> Cc: intel-wired-lan@lists.osuosl.org; Nguyen, Anthony L
> <anthony.l.nguyen@intel.com>; netdev@vger.kernel.org; Kubalewski,
> Arkadiusz <arkadiusz.kubalewski@intel.com>
> Subject: Re: [PATCH iwl-next v1] i40e: Add Energy Efficient Ethernet
> ability for X710 Base-T/KR/KX cards
> 
> 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.
> 
The value is got from FW which dictates endianness.
Const is const, explicit coding style helps understanding and compiler optimizations. 

> > +	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))
> 
Ok

> > +			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.
> 
But it fixes kernel coding style which checkpatch.pl may complain about.

> >
> >  	/* Get initial PHY capabilities */
> >  	status = i40e_aq_get_phy_capabilities(hw, false, true, &phy_cfg,
> NULL);
> >  	if (status)
> >  		return -EAGAIN;
> >
> >  	/* Check whether NIC configuration is compatible with Energy
> Efficient
> >  	 * Ethernet (EEE) mode.
> >  	 */
> >  	if (phy_cfg.eee_capability == 0)
> >  		return -EOPNOTSUPP;
> >
> > +	i40e_eee_capability_to_kedata_supported(phy_cfg.eee_capability,
> > +edata->supported);
> 
> Please line-wrap: Networking still prefers code to be 80 columns wide
> or less.
> 
As you wish.

> > +	linkmode_copy(edata->lp_advertised, edata->supported);
> > +
> >  	/* Get current configuration */
> >  	status = i40e_aq_get_phy_capabilities(hw, false, false,
> &phy_cfg, NULL);
> >  	if (status)
> >  		return -EAGAIN;
> >
> > +	linkmode_zero(edata->advertised);
> > +	if (phy_cfg.eee_capability)
> > +		linkmode_copy(edata->advertised, edata->supported);
> >  	edata->eee_enabled = !!phy_cfg.eee_capability;
> >  	edata->tx_lpi_enabled = pf->stats.tx_lpi_status;
> >
> >  	edata->eee_active = pf->stats.tx_lpi_status &&
> > pf->stats.rx_lpi_status;
> >
> >  	return 0;
> >  }
> >
> >  static int i40e_is_eee_param_supported(struct net_device *netdev,
> >  				       struct ethtool_keee *edata)  {
> >  	struct i40e_netdev_priv *np = netdev_priv(netdev);
> >  	struct i40e_vsi *vsi = np->vsi;
> >  	struct i40e_pf *pf = vsi->back;
> >  	struct i40e_ethtool_not_used {
> > -		u32 value;
> > +		int value;
> 
> It is unclear to me that this type change is really related to the
> subject of this patch. If not, please drop it. But if so, it seems to
> me that bool would be the appropriate type.
> 
> >  		const char *name;
> >  	} param[] = {
> > -		{edata->tx_lpi_timer, "tx-timer"},
> > +		{!!(edata->advertised[0] & ~edata->supported[0]),
> "advertise"},
> > +		{!!edata->tx_lpi_timer, "tx-timer"},
> >  		{edata->tx_lpi_enabled != pf->stats.tx_lpi_status, "tx-
> lpi"}
> >  	};
> >  	int i;
> > @@ -5710,7 +5737,7 @@ static int i40e_set_eee(struct net_device
> *netdev, struct ethtool_keee *edata)
> >  	struct i40e_pf *pf = vsi->back;
> >  	struct i40e_hw *hw = &pf->hw;
> >  	__le16 eee_capability;
> > -	int status = 0;
> > +	int status;
> 
> This change seems unrelated to the subject of this patch.
> If so, please remove.
> 
But it fixes kernel coding style which checkpatch.pl may complain about.

> >
> >  	/* Deny parameters we don't support */
> >  	if (i40e_is_eee_param_supported(netdev, edata)) @@ -5754,7
> +5781,7
> > @@ static int i40e_set_eee(struct net_device *netdev, struct
> ethtool_keee *edata)
> >  		config.eeer |=
> cpu_to_le32(I40E_PRTPM_EEER_TX_LPI_EN_MASK);
> >  	} else {
> >  		config.eee_capability = 0;
> > -		config.eeer &=
> cpu_to_le32(~I40E_PRTPM_EEER_TX_LPI_EN_MASK);
> > +		config.eeer &=
> ~cpu_to_le32(I40E_PRTPM_EEER_TX_LPI_EN_MASK);
> 
> Ditto.
> 
> >  	}
> >
> >  	/* Apply modified PHY configuration */ diff --git
> > a/drivers/net/ethernet/intel/i40e/i40e_main.c
> > b/drivers/net/ethernet/intel/i40e/i40e_main.c
> > index cbcfada..55bbf0f 100644
> > --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> > +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> > @@ -7263,6 +7263,22 @@ static int i40e_init_pf_dcb(struct i40e_pf
> *pf)
> >  	return err;
> >  }
> >  #endif /* CONFIG_I40E_DCB */
> > +static void i40e_print_link_message_eee(struct i40e_vsi *vsi,
> struct ethtool_keee *kedata,
> > +			    const char *speed, const char *fc) {
> > +	if (vsi->netdev->ethtool_ops->get_eee)
> > +		vsi->netdev->ethtool_ops->get_eee(vsi->netdev, kedata);
> > +
> > +	if (!linkmode_empty(kedata->supported))
> > +		netdev_info(vsi->netdev,
> > +			    "NIC Link is Up, %sbps Full Duplex, Flow
> Control: %s, EEE: %s\n",
> > +			    speed, fc,
> > +			    kedata->eee_enabled ? "Enabled" : "Disabled");
> > +	else
> > +		netdev_info(vsi->netdev,
> > +			    "NIC Link is Up, %sbps Full Duplex, Flow
> Control: %s\n",
> > +			    speed, fc);
> > +}
> >
> >  /**
> >   * i40e_print_link_message - print link up or down @@ -7395,9
> > +7411,12 @@ void i40e_print_link_message(struct i40e_vsi *vsi, bool
> isup)
> >  			    "NIC Link is Up, %sbps Full Duplex, Requested
> FEC: %s, Negotiated FEC: %s, Autoneg: %s, Flow Control: %s\n",
> >  			    speed, req_fec, fec, an, fc);
> >  	} else {
> > -		netdev_info(vsi->netdev,
> > -			    "NIC Link is Up, %sbps Full Duplex, Flow
> Control: %s\n",
> > -			    speed, fc);
> > +		struct ethtool_keee kedata;
> > +
> > +		linkmode_zero(kedata.supported);
> > +		kedata.eee_enabled = false;
> 
> Can the declaration of ethtool_keee be moved into
> i40e_print_link_message_eee()? I suspect that would lead to a cleaner
> implementation.
> 
Ok

> > +
> > +		i40e_print_link_message_eee(vsi, &kedata, speed, fc);
> >  	}
> >
> >  }

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

[Simon Horman]

On Mon, Aug 12, 2024 at 10:09:37AM +0200, Przemek Kitszel wrote:
> 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 ;)

Ok, I would vote for it being a separate patch.
But I won't push this one any further.