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

[Simon Horman <horms@kernel.org>]

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