[net-next 00/11][pull request] 40GbE Intel Wired LAN Driver Updates 2019-10-23

[net-next 00/11][pull request] 40GbE Intel Wired LAN Driver Updates 2019-10-23

[Jeff Kirsher]

This series contains updates to i40e only.  Several are fixes that could
go to 'net', but were intended for 'net-next'.

Sylwia changes how the driver function to read the NVM module data, so
that it is able to read the LLDP agent configuration to allow for
persistent LLDP.

Arkadiusz provides extended statistics for PF interfaces in i40e.

Jaroslaw resolves an issue where the incorrect FEC settings were being
displayed in ethtool, by setting the proper FEC bits.

Piotr moves the hardware flags detection into a separate function, so
that the specific flags can be set based on the MAC and NVM.  Also
extends the PHY access function to include a command flag to let the
firmware know it should not change the page while accessing a OSFP module.
Updates the driver to display the driver and firmware version when in
recovery mode.

Aleksandr cleans up the code by removing an unneeded macro by calling
the i40e_update_vfid_in_stats() directly.  Refactored the VF MAC filters
accounting since an untrusted VF was able to delete but not add a MAC
filter, so refactor the code to have more consistency and improved
logging.

Nicholas updates the driver to use a default interval of 50 usecs,
instead of the current 100 usecs which was causing some regression
performance issues.

Damian resolved LED blinking issues for X710T*L devices by adding
specific flows for these devices in the LED operations.

Navid Emamdoost found where allocated memory is not being properly freed
upon a failure in setting up MAC VLANs, so added the missing kfree().

The following are changes since commit 406715df933ad6a1b8b0545e7689aa5f4ac27922:
  fq_codel: do not include <linux/jhash.h>
and are available in the git repository at:
  git://git.kernel.org/pub/scm/linux/kernel/git/jkirsher/next-queue 40GbE

Aleksandr Loktionov (2):
  i40e: remove the macro with it's argument reuse
  i40e: Refactoring VF MAC filters counting to make more reliable

Arkadiusz Grubba (1):
  i40e: Add ability to display VF stats along with PF core stats

Damian Milosek (1):
  i40e: Fix LED blinking flow for X710T*L devices.

Jaroslaw Gawin (1):
  i40e: Wrong 'Advertised FEC modes' after set FEC to AUTO

Navid Emamdoost (1):
  i40e: prevent memory leak in i40e_setup_macvlans

Nicholas Nunley (1):
  i40e: initialize ITRN registers with correct values

Piotr Azarewicz (2):
  i40e: Extract detection of HW flags into a function
  i40e: Extend PHY access with page change flag

Piotr Kwapulinski (1):
  i40e: allow ethtool to report SW and FW versions in recovery mode

Sylwia Wnuczko (1):
  i40e: Fix for persistent lldp support

 drivers/net/ethernet/intel/i40e/i40e.h        |   1 +
 drivers/net/ethernet/intel/i40e/i40e_adminq.c |  71 +++++--
 .../net/ethernet/intel/i40e/i40e_adminq_cmd.h |   8 +-
 drivers/net/ethernet/intel/i40e/i40e_common.c | 116 +++++++++---
 drivers/net/ethernet/intel/i40e/i40e_dcb.c    |   4 +-
 drivers/net/ethernet/intel/i40e/i40e_dcb.h    |   3 +
 drivers/net/ethernet/intel/i40e/i40e_devids.h |   2 +
 .../net/ethernet/intel/i40e/i40e_ethtool.c    | 174 ++++++++++++++++--
 drivers/net/ethernet/intel/i40e/i40e_main.c   |  30 ++-
 drivers/net/ethernet/intel/i40e/i40e_nvm.c    |  61 +++---
 .../net/ethernet/intel/i40e/i40e_prototype.h  |  36 ++--
 drivers/net/ethernet/intel/i40e/i40e_type.h   |   1 +
 .../ethernet/intel/i40e/i40e_virtchnl_pf.c    |  45 ++---
 .../ethernet/intel/i40e/i40e_virtchnl_pf.h    |   1 -
 14 files changed, 422 insertions(+), 131 deletions(-)

-- 
2.21.0

[net-next 01/11] i40e: Fix for persistent lldp support

[Jeff Kirsher]

From: Sylwia Wnuczko <sylwia.wnuczko@intel.com>

This patch fixes function to read NVM module data and uses it to
read current LLDP agent configuration from NVM API version 1.8.

Signed-off-by: Sylwia Wnuczko <sylwia.wnuczko@intel.com>
Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 drivers/net/ethernet/intel/i40e/i40e_dcb.c    |  4 +-
 drivers/net/ethernet/intel/i40e/i40e_dcb.h    |  3 +
 drivers/net/ethernet/intel/i40e/i40e_nvm.c    | 61 ++++++++++---------
 .../net/ethernet/intel/i40e/i40e_prototype.h  | 10 +--
 4 files changed, 44 insertions(+), 34 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_dcb.c b/drivers/net/ethernet/intel/i40e/i40e_dcb.c
index 200a1cb3b536..9de503c5f99b 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_dcb.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_dcb.c
@@ -889,7 +889,9 @@ i40e_status i40e_init_dcb(struct i40e_hw *hw, bool enable_mib_change)
 
 		ret = i40e_read_nvm_module_data(hw,
 						I40E_SR_EMP_SR_SETTINGS_PTR,
-						offset, 1,
+						offset,
+						I40E_LLDP_CURRENT_STATUS_OFFSET,
+						I40E_LLDP_CURRENT_STATUS_SIZE,
 						&lldp_cfg.adminstatus);
 	} else {
 		ret = i40e_read_lldp_cfg(hw, &lldp_cfg);
diff --git a/drivers/net/ethernet/intel/i40e/i40e_dcb.h b/drivers/net/ethernet/intel/i40e/i40e_dcb.h
index 2a80c5daa376..ba86ad833bee 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_dcb.h
+++ b/drivers/net/ethernet/intel/i40e/i40e_dcb.h
@@ -32,6 +32,9 @@
 #define I40E_CEE_MAX_FEAT_TYPE		3
 #define I40E_LLDP_CURRENT_STATUS_XL710_OFFSET	0x2B
 #define I40E_LLDP_CURRENT_STATUS_X722_OFFSET	0x31
+#define I40E_LLDP_CURRENT_STATUS_OFFSET		1
+#define I40E_LLDP_CURRENT_STATUS_SIZE		1
+
 /* Defines for LLDP TLV header */
 #define I40E_LLDP_TLV_LEN_SHIFT		0
 #define I40E_LLDP_TLV_LEN_MASK		(0x01FF << I40E_LLDP_TLV_LEN_SHIFT)
diff --git a/drivers/net/ethernet/intel/i40e/i40e_nvm.c b/drivers/net/ethernet/intel/i40e/i40e_nvm.c
index e4d8d20baf3b..7164f4ad8120 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_nvm.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_nvm.c
@@ -323,20 +323,24 @@ i40e_status i40e_read_nvm_word(struct i40e_hw *hw, u16 offset,
 
 /**
  * i40e_read_nvm_module_data - Reads NVM Buffer to specified memory location
- * @hw: pointer to the HW structure
+ * @hw: Pointer to the HW structure
  * @module_ptr: Pointer to module in words with respect to NVM beginning
- * @offset: offset in words from module start
+ * @module_offset: Offset in words from module start
+ * @data_offset: Offset in words from reading data area start
  * @words_data_size: Words to read from NVM
  * @data_ptr: Pointer to memory location where resulting buffer will be stored
  **/
-i40e_status i40e_read_nvm_module_data(struct i40e_hw *hw,
-				      u8 module_ptr, u16 offset,
-				      u16 words_data_size,
-				      u16 *data_ptr)
+enum i40e_status_code i40e_read_nvm_module_data(struct i40e_hw *hw,
+						u8 module_ptr,
+						u16 module_offset,
+						u16 data_offset,
+						u16 words_data_size,
+						u16 *data_ptr)
 {
 	i40e_status status;
+	u16 specific_ptr = 0;
 	u16 ptr_value = 0;
-	u32 flat_offset;
+	u32 offset = 0;
 
 	if (module_ptr != 0) {
 		status = i40e_read_nvm_word(hw, module_ptr, &ptr_value);
@@ -352,36 +356,35 @@ i40e_status i40e_read_nvm_module_data(struct i40e_hw *hw,
 
 	/* Pointer not initialized */
 	if (ptr_value == I40E_NVM_INVALID_PTR_VAL ||
-	    ptr_value == I40E_NVM_INVALID_VAL)
+	    ptr_value == I40E_NVM_INVALID_VAL) {
+		i40e_debug(hw, I40E_DEBUG_ALL, "Pointer not initialized.\n");
 		return I40E_ERR_BAD_PTR;
+	}
 
 	/* Check whether the module is in SR mapped area or outside */
 	if (ptr_value & I40E_PTR_TYPE) {
 		/* Pointer points outside of the Shared RAM mapped area */
-		ptr_value &= ~I40E_PTR_TYPE;
+		i40e_debug(hw, I40E_DEBUG_ALL,
+			   "Reading nvm data failed. Pointer points outside of the Shared RAM mapped area.\n");
 
-		/* PtrValue in 4kB units, need to convert to words */
-		ptr_value /= 2;
-		flat_offset = ((u32)ptr_value * 0x1000) + (u32)offset;
-		status = i40e_acquire_nvm(hw, I40E_RESOURCE_READ);
-		if (!status) {
-			status = i40e_aq_read_nvm(hw, 0, 2 * flat_offset,
-						  2 * words_data_size,
-						  data_ptr, true, NULL);
-			i40e_release_nvm(hw);
-			if (status) {
-				i40e_debug(hw, I40E_DEBUG_ALL,
-					   "Reading nvm aq failed.Error code: %d.\n",
-					   status);
-				return I40E_ERR_NVM;
-			}
-		} else {
-			return I40E_ERR_NVM;
-		}
+		return I40E_ERR_PARAM;
 	} else {
 		/* Read from the Shadow RAM */
-		status = i40e_read_nvm_buffer(hw, ptr_value + offset,
-					      &words_data_size, data_ptr);
+
+		status = i40e_read_nvm_word(hw, ptr_value + module_offset,
+					    &specific_ptr);
+		if (status) {
+			i40e_debug(hw, I40E_DEBUG_ALL,
+				   "Reading nvm word failed.Error code: %d.\n",
+				   status);
+			return I40E_ERR_NVM;
+		}
+
+		offset = ptr_value + module_offset + specific_ptr +
+			data_offset;
+
+		status = i40e_read_nvm_buffer(hw, offset, &words_data_size,
+					      data_ptr);
 		if (status) {
 			i40e_debug(hw, I40E_DEBUG_ALL,
 				   "Reading nvm buffer failed.Error code: %d.\n",
diff --git a/drivers/net/ethernet/intel/i40e/i40e_prototype.h b/drivers/net/ethernet/intel/i40e/i40e_prototype.h
index 5250441bf75b..7effe5010e32 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_prototype.h
+++ b/drivers/net/ethernet/intel/i40e/i40e_prototype.h
@@ -315,10 +315,12 @@ i40e_status i40e_acquire_nvm(struct i40e_hw *hw,
 void i40e_release_nvm(struct i40e_hw *hw);
 i40e_status i40e_read_nvm_word(struct i40e_hw *hw, u16 offset,
 					 u16 *data);
-i40e_status i40e_read_nvm_module_data(struct i40e_hw *hw,
-				      u8 module_ptr, u16 offset,
-				      u16 words_data_size,
-				      u16 *data_ptr);
+enum i40e_status_code i40e_read_nvm_module_data(struct i40e_hw *hw,
+						u8 module_ptr,
+						u16 module_offset,
+						u16 data_offset,
+						u16 words_data_size,
+						u16 *data_ptr);
 i40e_status i40e_read_nvm_buffer(struct i40e_hw *hw, u16 offset,
 				 u16 *words, u16 *data);
 i40e_status i40e_update_nvm_checksum(struct i40e_hw *hw);
-- 
2.21.0

[net-next 02/11] i40e: Add ability to display VF stats along with PF core stats

[Jeff Kirsher]

From: Arkadiusz Grubba <arkadiusz.grubba@intel.com>

This change introduces the ability to display extended (enhanced)
statistics for PF interfaces.

The patch introduces new arrays defined for these
extra stats (in i40e_ethtool.c file) and enhances/extends ethtool ops
functions intended for dealing with PF stats (i.e.: i40e_get_stats_count(),
i40e_get_ethtool_stats(), i40e_get_stat_strings() ).

There have also been introduced the new build flag named
"I40E_PF_EXTRA_STATS_OFF" to exclude from the driver code all code snippets
associated with these extra stats.

Signed-off-by: Arkadiusz Grubba <arkadiusz.grubba@intel.com>
Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 .../net/ethernet/intel/i40e/i40e_ethtool.c    | 149 ++++++++++++++++++
 1 file changed, 149 insertions(+)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
index 41e1240acaea..c814c756b4bb 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
@@ -389,6 +389,7 @@ static const struct i40e_stats i40e_gstrings_pfc_stats[] = {
 
 #define I40E_GLOBAL_STATS_LEN	ARRAY_SIZE(i40e_gstrings_stats)
 
+/* Length (number) of PF core stats only (i.e. without queues / extra stats): */
 #define I40E_PF_STATS_LEN	(I40E_GLOBAL_STATS_LEN + \
 				 I40E_PFC_STATS_LEN + \
 				 I40E_VEB_STATS_LEN + \
@@ -397,6 +398,44 @@ static const struct i40e_stats i40e_gstrings_pfc_stats[] = {
 /* Length of stats for a single queue */
 #define I40E_QUEUE_STATS_LEN	ARRAY_SIZE(i40e_gstrings_queue_stats)
 
+#define I40E_STATS_NAME_VFID_EXTRA "vf___."
+#define I40E_STATS_NAME_VFID_EXTRA_LEN (sizeof(I40E_STATS_NAME_VFID_EXTRA) - 1)
+
+static struct i40e_stats i40e_gstrings_eth_stats_extra[] = {
+	I40E_VSI_STAT(I40E_STATS_NAME_VFID_EXTRA
+		      "rx_bytes", eth_stats.rx_bytes),
+	I40E_VSI_STAT(I40E_STATS_NAME_VFID_EXTRA
+		      "rx_unicast", eth_stats.rx_unicast),
+	I40E_VSI_STAT(I40E_STATS_NAME_VFID_EXTRA
+		      "rx_multicast", eth_stats.rx_multicast),
+	I40E_VSI_STAT(I40E_STATS_NAME_VFID_EXTRA
+		      "rx_broadcast", eth_stats.rx_broadcast),
+	I40E_VSI_STAT(I40E_STATS_NAME_VFID_EXTRA
+		      "rx_discards", eth_stats.rx_discards),
+	I40E_VSI_STAT(I40E_STATS_NAME_VFID_EXTRA
+		      "rx_unknown_protocol", eth_stats.rx_unknown_protocol),
+	I40E_VSI_STAT(I40E_STATS_NAME_VFID_EXTRA
+		      "tx_bytes", eth_stats.tx_bytes),
+	I40E_VSI_STAT(I40E_STATS_NAME_VFID_EXTRA
+		      "tx_unicast", eth_stats.tx_unicast),
+	I40E_VSI_STAT(I40E_STATS_NAME_VFID_EXTRA
+		      "tx_multicast", eth_stats.tx_multicast),
+	I40E_VSI_STAT(I40E_STATS_NAME_VFID_EXTRA
+		      "tx_broadcast", eth_stats.tx_broadcast),
+	I40E_VSI_STAT(I40E_STATS_NAME_VFID_EXTRA
+		      "tx_discards", eth_stats.tx_discards),
+	I40E_VSI_STAT(I40E_STATS_NAME_VFID_EXTRA
+		      "tx_errors", eth_stats.tx_errors),
+};
+
+#define I40E_STATS_EXTRA_COUNT	128  /* as for now only I40E_MAX_VF_COUNT */
+/* Following length value does not include the length values for queues stats */
+#define I40E_STATS_EXTRA_LEN	ARRAY_SIZE(i40e_gstrings_eth_stats_extra)
+/* Length (number) of PF extra stats only (i.e. without core stats / queues): */
+#define I40E_PF_STATS_EXTRA_LEN (I40E_STATS_EXTRA_COUNT * I40E_STATS_EXTRA_LEN)
+/* Length (number) of enhanced/all PF stats (i.e. core with extra stats): */
+#define I40E_PF_STATS_ENHANCE_LEN (I40E_PF_STATS_LEN + I40E_PF_STATS_EXTRA_LEN)
+
 enum i40e_ethtool_test_id {
 	I40E_ETH_TEST_REG = 0,
 	I40E_ETH_TEST_EEPROM,
@@ -2190,6 +2229,9 @@ static int i40e_get_stats_count(struct net_device *netdev)
 	 */
 	stats_len += I40E_QUEUE_STATS_LEN * 2 * netdev->num_tx_queues;
 
+	if (vsi == pf->vsi[pf->lan_vsi] && pf->hw.partition_id == 1)
+		stats_len += I40E_PF_STATS_EXTRA_LEN;
+
 	return stats_len;
 }
 
@@ -2258,6 +2300,10 @@ static void i40e_get_ethtool_stats(struct net_device *netdev,
 	struct i40e_vsi *vsi = np->vsi;
 	struct i40e_pf *pf = vsi->back;
 	struct i40e_veb *veb = NULL;
+	unsigned int vsi_idx;
+	unsigned int vf_idx;
+	unsigned int vf_id;
+	bool is_vf_valid;
 	unsigned int i;
 	bool veb_stats;
 	u64 *p = data;
@@ -2307,11 +2353,109 @@ static void i40e_get_ethtool_stats(struct net_device *netdev,
 		i40e_add_ethtool_stats(&data, &pfc, i40e_gstrings_pfc_stats);
 	}
 
+	/* As for now, we only process the SRIOV type VSIs (as extra stats to
+	 * PF core stats) which are correlated with VF LAN VSI (hence below,
+	 * in this for-loop instruction block, only VF's LAN VSIs are currently
+	 * processed).
+	 */
+	for (vf_id = 0; vf_id < pf->num_alloc_vfs; vf_id++) {
+		is_vf_valid = true;
+		for (vf_idx = 0; vf_idx < pf->num_alloc_vfs; vf_idx++)
+			if (pf->vf[vf_idx].vf_id == vf_id)
+				break;
+		if (vf_idx >= pf->num_alloc_vfs) {
+			dev_info(&pf->pdev->dev,
+				 "In the PF's array, there is no VF instance with VF_ID identifier %d or it is not set/initialized correctly yet\n",
+				 vf_id);
+			is_vf_valid = false;
+			goto check_vf;
+		}
+		vsi_idx = pf->vf[vf_idx].lan_vsi_idx;
+
+		vsi = pf->vsi[vsi_idx];
+		if (!vsi) {
+			/* It means empty field in the PF VSI array... */
+			dev_info(&pf->pdev->dev,
+				 "No LAN VSI instance referenced by VF %d or it is not set/initialized correctly yet\n",
+				 vf_id);
+			is_vf_valid = false;
+			goto check_vf;
+		}
+		if (vsi->vf_id != vf_id) {
+			dev_info(&pf->pdev->dev,
+				 "In the PF's array, there is incorrectly set/initialized LAN VSI or reference to it from VF %d is not set/initialized correctly yet\n",
+				 vf_id);
+			is_vf_valid = false;
+			goto check_vf;
+		}
+		if (vsi->vf_id != pf->vf[vf_idx].vf_id ||
+		    !i40e_find_vsi_from_id(pf, pf->vf[vsi->vf_id].lan_vsi_id)) {
+			/* Disjointed identifiers or broken references VF-VSI */
+			dev_warn(&pf->pdev->dev,
+				 "SRIOV LAN VSI (index %d in PF VSI array) with invalid VF Identifier %d (referenced by VF %d, ordered as %d in VF array)\n",
+				 vsi_idx, pf->vsi[vsi_idx]->vf_id,
+				 pf->vf[vf_idx].vf_id, vf_idx);
+			is_vf_valid = false;
+		}
+check_vf:
+		if (!is_vf_valid) {
+			i40e_add_ethtool_stats(&data, NULL,
+					       i40e_gstrings_eth_stats_extra);
+		} else {
+			i40e_update_eth_stats(vsi);
+			i40e_add_ethtool_stats(&data, vsi,
+					       i40e_gstrings_eth_stats_extra);
+		}
+	}
+	for (; vf_id < I40E_STATS_EXTRA_COUNT; vf_id++)
+		i40e_add_ethtool_stats(&data, NULL,
+				       i40e_gstrings_eth_stats_extra);
+
 check_data_pointer:
 	WARN_ONCE(data - p != i40e_get_stats_count(netdev),
 		  "ethtool stats count mismatch!");
 }
 
+/**
+ * __i40e_update_vfid_in_stats_strings - print VF num to stats names
+ * @stats_extra: array of stats structs with stats name strings
+ * @strings_num: number of stats name strings in array above (length)
+ * @vf_id: VF number to update stats name strings with
+ *
+ * Helper function to i40e_get_stat_strings() in case of extra stats.
+ **/
+static inline void
+__i40e_update_vfid_in_stats_strings(struct i40e_stats stats_extra[],
+				    int strings_num, int vf_id)
+{
+	int i;
+
+	for (i = 0; i < strings_num; i++) {
+		snprintf(stats_extra[i].stat_string,
+			 I40E_STATS_NAME_VFID_EXTRA_LEN, "vf%03d", vf_id);
+		stats_extra[i].stat_string[I40E_STATS_NAME_VFID_EXTRA_LEN -
+								       1] = '.';
+	}
+}
+
+/**
+ * i40e_update_vfid_in_stats - print VF num to stat names
+ * @stats_extra: array of stats structs with stats name strings
+ * @vf_id: VF number to update stats name strings with
+ *
+ * Helper macro to i40e_get_stat_strings() to ease use of
+ * __i40e_update_vfid_in_stats_strings() function due to extra stats.
+ *
+ * Macro to ease the use of __i40e_update_vfid_in_stats_strings by taking
+ * a static constant stats array and passing the ARRAY_SIZE(). This avoids typos
+ * by ensuring that we pass the size associated with the given stats array.
+ *
+ * The parameter @stats_extra is evaluated twice, so parameters with side
+ * effects should be avoided.
+ **/
+#define i40e_update_vfid_in_stats(stats_extra, vf_id) \
+__i40e_update_vfid_in_stats_strings(stats_extra, ARRAY_SIZE(stats_extra), vf_id)
+
 /**
  * i40e_get_stat_strings - copy stat strings into supplied buffer
  * @netdev: the netdev to collect strings for
@@ -2354,6 +2498,11 @@ static void i40e_get_stat_strings(struct net_device *netdev, u8 *data)
 	for (i = 0; i < I40E_MAX_USER_PRIORITY; i++)
 		i40e_add_stat_strings(&data, i40e_gstrings_pfc_stats, i);
 
+	for (i = 0; i < I40E_STATS_EXTRA_COUNT; i++) {
+		i40e_update_vfid_in_stats(i40e_gstrings_eth_stats_extra, i);
+		i40e_add_stat_strings(&data, i40e_gstrings_eth_stats_extra);
+	}
+
 check_data_pointer:
 	WARN_ONCE(data - p != i40e_get_stats_count(netdev) * ETH_GSTRING_LEN,
 		  "stat strings count mismatch!");
-- 
2.21.0

[net-next 03/11] i40e: Wrong 'Advertised FEC modes' after set FEC to AUTO

[Jeff Kirsher]

From: Jaroslaw Gawin <jaroslawx.gawin@intel.com>

Fix display of parameters "Configured FEC encodings:" and "Advertised
FEC modes:" in ethtool.  Implemented by setting proper FEC bits in
“advertising” bitmask of link_modes struct and “fec” bitmask in
ethtool_fecparam struct. Without this patch wrong FEC settings
can be shown.

Signed-off-by: Jaroslaw Gawin <jaroslawx.gawin@intel.com>
Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 drivers/net/ethernet/intel/i40e/i40e_common.c | 13 ++++++--
 .../net/ethernet/intel/i40e/i40e_ethtool.c    | 32 +++++++++----------
 2 files changed, 26 insertions(+), 19 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_common.c b/drivers/net/ethernet/intel/i40e/i40e_common.c
index d37c6e0e5f08..f1d67267c983 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_common.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_common.c
@@ -2570,9 +2570,16 @@ noinline_for_stack i40e_status i40e_update_link_info(struct i40e_hw *hw)
 		if (status)
 			return status;
 
-		hw->phy.link_info.req_fec_info =
-			abilities.fec_cfg_curr_mod_ext_info &
-			(I40E_AQ_REQUEST_FEC_KR | I40E_AQ_REQUEST_FEC_RS);
+		if (abilities.fec_cfg_curr_mod_ext_info &
+		    I40E_AQ_ENABLE_FEC_AUTO)
+			hw->phy.link_info.req_fec_info =
+				(I40E_AQ_REQUEST_FEC_KR |
+				 I40E_AQ_REQUEST_FEC_RS);
+		else
+			hw->phy.link_info.req_fec_info =
+				abilities.fec_cfg_curr_mod_ext_info &
+				(I40E_AQ_REQUEST_FEC_KR |
+				 I40E_AQ_REQUEST_FEC_RS);
 
 		memcpy(hw->phy.link_info.module_type, &abilities.module_type,
 		       sizeof(hw->phy.link_info.module_type));
diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
index c814c756b4bb..765bc5174ead 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
@@ -761,7 +761,14 @@ static void i40e_get_settings_link_up_fec(u8 req_fec_info,
 	ethtool_link_ksettings_add_link_mode(ks, supported, FEC_RS);
 	ethtool_link_ksettings_add_link_mode(ks, supported, FEC_BASER);
 
-	if (I40E_AQ_SET_FEC_REQUEST_RS & req_fec_info) {
+	if ((I40E_AQ_SET_FEC_REQUEST_RS & req_fec_info) &&
+	    (I40E_AQ_SET_FEC_REQUEST_KR & req_fec_info)) {
+		ethtool_link_ksettings_add_link_mode(ks, advertising,
+						     FEC_NONE);
+		ethtool_link_ksettings_add_link_mode(ks, advertising,
+						     FEC_BASER);
+		ethtool_link_ksettings_add_link_mode(ks, advertising, FEC_RS);
+	} else if (I40E_AQ_SET_FEC_REQUEST_RS & req_fec_info) {
 		ethtool_link_ksettings_add_link_mode(ks, advertising, FEC_RS);
 	} else if (I40E_AQ_SET_FEC_REQUEST_KR & req_fec_info) {
 		ethtool_link_ksettings_add_link_mode(ks, advertising,
@@ -769,12 +776,6 @@ static void i40e_get_settings_link_up_fec(u8 req_fec_info,
 	} else {
 		ethtool_link_ksettings_add_link_mode(ks, advertising,
 						     FEC_NONE);
-		if (I40E_AQ_SET_FEC_AUTO & req_fec_info) {
-			ethtool_link_ksettings_add_link_mode(ks, advertising,
-							     FEC_RS);
-			ethtool_link_ksettings_add_link_mode(ks, advertising,
-							     FEC_BASER);
-		}
 	}
 }
 
@@ -1476,6 +1477,7 @@ static int i40e_get_fec_param(struct net_device *netdev,
 	struct i40e_hw *hw = &pf->hw;
 	i40e_status status = 0;
 	int err = 0;
+	u8 fec_cfg;
 
 	/* Get the current phy config */
 	memset(&abilities, 0, sizeof(abilities));
@@ -1487,18 +1489,16 @@ static int i40e_get_fec_param(struct net_device *netdev,
 	}
 
 	fecparam->fec = 0;
-	if (abilities.fec_cfg_curr_mod_ext_info & I40E_AQ_SET_FEC_AUTO)
+	fec_cfg = abilities.fec_cfg_curr_mod_ext_info;
+	if (fec_cfg & I40E_AQ_SET_FEC_AUTO)
 		fecparam->fec |= ETHTOOL_FEC_AUTO;
-	if ((abilities.fec_cfg_curr_mod_ext_info &
-	     I40E_AQ_SET_FEC_REQUEST_RS) ||
-	    (abilities.fec_cfg_curr_mod_ext_info &
-	     I40E_AQ_SET_FEC_ABILITY_RS))
+	else if (fec_cfg & (I40E_AQ_SET_FEC_REQUEST_RS |
+		 I40E_AQ_SET_FEC_ABILITY_RS))
 		fecparam->fec |= ETHTOOL_FEC_RS;
-	if ((abilities.fec_cfg_curr_mod_ext_info &
-	     I40E_AQ_SET_FEC_REQUEST_KR) ||
-	    (abilities.fec_cfg_curr_mod_ext_info & I40E_AQ_SET_FEC_ABILITY_KR))
+	else if (fec_cfg & (I40E_AQ_SET_FEC_REQUEST_KR |
+		 I40E_AQ_SET_FEC_ABILITY_KR))
 		fecparam->fec |= ETHTOOL_FEC_BASER;
-	if (abilities.fec_cfg_curr_mod_ext_info == 0)
+	if (fec_cfg == 0)
 		fecparam->fec |= ETHTOOL_FEC_OFF;
 
 	if (hw->phy.link_info.fec_info & I40E_AQ_CONFIG_FEC_KR_ENA)
-- 
2.21.0

[net-next 04/11] i40e: Extract detection of HW flags into a function

[Jeff Kirsher]

From: Piotr Azarewicz <piotr.azarewicz@intel.com>

Move code detecting HW flags based on device type and FW API version
into a single function.

Signed-off-by: Piotr Azarewicz <piotr.azarewicz@intel.com>
Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 drivers/net/ethernet/intel/i40e/i40e_adminq.c | 71 +++++++++++++++----
 drivers/net/ethernet/intel/i40e/i40e_common.c |  4 --
 2 files changed, 58 insertions(+), 17 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_adminq.c b/drivers/net/ethernet/intel/i40e/i40e_adminq.c
index 72c04881d290..9f0a4e92a231 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_adminq.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_adminq.c
@@ -507,6 +507,59 @@ static i40e_status i40e_shutdown_arq(struct i40e_hw *hw)
 	return ret_code;
 }
 
+/**
+ *  i40e_set_hw_flags - set HW flags
+ *  @hw: pointer to the hardware structure
+ **/
+static void i40e_set_hw_flags(struct i40e_hw *hw)
+{
+	struct i40e_adminq_info *aq = &hw->aq;
+
+	hw->flags = 0;
+
+	switch (hw->mac.type) {
+	case I40E_MAC_XL710:
+		if (aq->api_maj_ver > 1 ||
+		    (aq->api_maj_ver == 1 &&
+		     aq->api_min_ver >= I40E_MINOR_VER_GET_LINK_INFO_XL710)) {
+			hw->flags |= I40E_HW_FLAG_AQ_PHY_ACCESS_CAPABLE;
+			hw->flags |= I40E_HW_FLAG_FW_LLDP_STOPPABLE;
+			/* The ability to RX (not drop) 802.1ad frames */
+			hw->flags |= I40E_HW_FLAG_802_1AD_CAPABLE;
+		}
+		break;
+	case I40E_MAC_X722:
+		hw->flags |= I40E_HW_FLAG_AQ_SRCTL_ACCESS_ENABLE |
+			     I40E_HW_FLAG_NVM_READ_REQUIRES_LOCK;
+
+		if (aq->api_maj_ver > 1 ||
+		    (aq->api_maj_ver == 1 &&
+		     aq->api_min_ver >= I40E_MINOR_VER_FW_LLDP_STOPPABLE_X722))
+			hw->flags |= I40E_HW_FLAG_FW_LLDP_STOPPABLE;
+		/* fall through */
+	default:
+		break;
+	}
+
+	/* Newer versions of firmware require lock when reading the NVM */
+	if (aq->api_maj_ver > 1 ||
+	    (aq->api_maj_ver == 1 &&
+	     aq->api_min_ver >= 5))
+		hw->flags |= I40E_HW_FLAG_NVM_READ_REQUIRES_LOCK;
+
+	if (aq->api_maj_ver > 1 ||
+	    (aq->api_maj_ver == 1 &&
+	     aq->api_min_ver >= 8)) {
+		hw->flags |= I40E_HW_FLAG_FW_LLDP_PERSISTENT;
+		hw->flags |= I40E_HW_FLAG_DROP_MODE;
+	}
+
+	if (aq->api_maj_ver > 1 ||
+	    (aq->api_maj_ver == 1 &&
+	     aq->api_min_ver >= 9))
+		hw->flags |= I40E_HW_FLAG_AQ_PHY_ACCESS_EXTENDED;
+}
+
 /**
  *  i40e_init_adminq - main initialization routine for Admin Queue
  *  @hw: pointer to the hardware structure
@@ -571,6 +624,11 @@ i40e_status i40e_init_adminq(struct i40e_hw *hw)
 	if (ret_code != I40E_SUCCESS)
 		goto init_adminq_free_arq;
 
+	/* Some features were introduced in different FW API version
+	 * for different MAC type.
+	 */
+	i40e_set_hw_flags(hw);
+
 	/* get the NVM version info */
 	i40e_read_nvm_word(hw, I40E_SR_NVM_DEV_STARTER_VERSION,
 			   &hw->nvm.version);
@@ -596,25 +654,12 @@ i40e_status i40e_init_adminq(struct i40e_hw *hw)
 		hw->flags |= I40E_HW_FLAG_FW_LLDP_STOPPABLE;
 	}
 
-	/* Newer versions of firmware require lock when reading the NVM */
-	if (hw->aq.api_maj_ver > 1 ||
-	    (hw->aq.api_maj_ver == 1 &&
-	     hw->aq.api_min_ver >= 5))
-		hw->flags |= I40E_HW_FLAG_NVM_READ_REQUIRES_LOCK;
-
 	/* The ability to RX (not drop) 802.1ad frames was added in API 1.7 */
 	if (hw->aq.api_maj_ver > 1 ||
 	    (hw->aq.api_maj_ver == 1 &&
 	     hw->aq.api_min_ver >= 7))
 		hw->flags |= I40E_HW_FLAG_802_1AD_CAPABLE;
 
-	if (hw->aq.api_maj_ver > 1 ||
-	    (hw->aq.api_maj_ver == 1 &&
-	     hw->aq.api_min_ver >= 8)) {
-		hw->flags |= I40E_HW_FLAG_FW_LLDP_PERSISTENT;
-		hw->flags |= I40E_HW_FLAG_DROP_MODE;
-	}
-
 	if (hw->aq.api_maj_ver > I40E_FW_API_VERSION_MAJOR) {
 		ret_code = I40E_ERR_FIRMWARE_API_VERSION;
 		goto init_adminq_free_arq;
diff --git a/drivers/net/ethernet/intel/i40e/i40e_common.c b/drivers/net/ethernet/intel/i40e/i40e_common.c
index f1d67267c983..fe553bb23d7a 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_common.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_common.c
@@ -933,10 +933,6 @@ i40e_status i40e_init_shared_code(struct i40e_hw *hw)
 	else
 		hw->pf_id = (u8)(func_rid & 0x7);
 
-	if (hw->mac.type == I40E_MAC_X722)
-		hw->flags |= I40E_HW_FLAG_AQ_SRCTL_ACCESS_ENABLE |
-			     I40E_HW_FLAG_NVM_READ_REQUIRES_LOCK;
-
 	status = i40e_init_nvm(hw);
 	return status;
 }
-- 
2.21.0

[net-next 05/11] i40e: Extend PHY access with page change flag

[Jeff Kirsher]

From: Piotr Azarewicz <piotr.azarewicz@intel.com>

Currently FW use MDIO I/F number corresponded with current PF for PHY
access. This code allow to specify used MDIO I/F number.

Add new field - command flags with only one flag for now. Added flag
tells FW that it shouldn't change page while accessing QSFP module, as
it was set manually.

Signed-off-by: Piotr Azarewicz <piotr.azarewicz@intel.com>
Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 .../net/ethernet/intel/i40e/i40e_adminq_cmd.h |  8 ++-
 drivers/net/ethernet/intel/i40e/i40e_common.c | 70 +++++++++++++++----
 .../net/ethernet/intel/i40e/i40e_ethtool.c    |  8 +--
 .../net/ethernet/intel/i40e/i40e_prototype.h  | 26 ++++---
 drivers/net/ethernet/intel/i40e/i40e_type.h   |  1 +
 5 files changed, 87 insertions(+), 26 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_adminq_cmd.h b/drivers/net/ethernet/intel/i40e/i40e_adminq_cmd.h
index 530613f31527..a23f89fb33ee 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_adminq_cmd.h
+++ b/drivers/net/ethernet/intel/i40e/i40e_adminq_cmd.h
@@ -2249,7 +2249,13 @@ struct i40e_aqc_phy_register_access {
 #define I40E_AQ_PHY_REG_ACCESS_EXTERNAL	1
 #define I40E_AQ_PHY_REG_ACCESS_EXTERNAL_MODULE	2
 	u8	dev_address;
-	u8	reserved1[2];
+	u8	cmd_flags;
+#define I40E_AQ_PHY_REG_ACCESS_DONT_CHANGE_QSFP_PAGE	0x01
+#define I40E_AQ_PHY_REG_ACCESS_SET_MDIO_IF_NUMBER	0x02
+#define I40E_AQ_PHY_REG_ACCESS_MDIO_IF_NUMBER_SHIFT	2
+#define I40E_AQ_PHY_REG_ACCESS_MDIO_IF_NUMBER_MASK	(0x3 << \
+		I40E_AQ_PHY_REG_ACCESS_MDIO_IF_NUMBER_SHIFT)
+	u8	reserved1;
 	__le32	reg_address;
 	__le32	reg_value;
 	u8	reserved2[4];
diff --git a/drivers/net/ethernet/intel/i40e/i40e_common.c b/drivers/net/ethernet/intel/i40e/i40e_common.c
index fe553bb23d7a..ed8608b874f4 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_common.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_common.c
@@ -5046,7 +5046,7 @@ static enum i40e_status_code i40e_led_get_reg(struct i40e_hw *hw, u16 led_addr,
 		status =
 		       i40e_aq_get_phy_register(hw,
 						I40E_AQ_PHY_REG_ACCESS_EXTERNAL,
-						I40E_PHY_COM_REG_PAGE,
+						I40E_PHY_COM_REG_PAGE, true,
 						I40E_PHY_LED_PROV_REG_1,
 						reg_val, NULL);
 	} else {
@@ -5079,7 +5079,7 @@ static enum i40e_status_code i40e_led_set_reg(struct i40e_hw *hw, u16 led_addr,
 		status =
 		       i40e_aq_set_phy_register(hw,
 						I40E_AQ_PHY_REG_ACCESS_EXTERNAL,
-						I40E_PHY_COM_REG_PAGE,
+						I40E_PHY_COM_REG_PAGE, true,
 						I40E_PHY_LED_PROV_REG_1,
 						reg_val, NULL);
 	} else {
@@ -5118,7 +5118,7 @@ i40e_status i40e_led_get_phy(struct i40e_hw *hw, u16 *led_addr,
 		status =
 		      i40e_aq_get_phy_register(hw,
 					       I40E_AQ_PHY_REG_ACCESS_EXTERNAL,
-					       I40E_PHY_COM_REG_PAGE,
+					       I40E_PHY_COM_REG_PAGE, true,
 					       I40E_PHY_LED_PROV_REG_1,
 					       &reg_val_aq, NULL);
 		if (status == I40E_SUCCESS)
@@ -5323,20 +5323,49 @@ void i40e_write_rx_ctl(struct i40e_hw *hw, u32 reg_addr, u32 reg_val)
 }
 
 /**
- * i40e_aq_set_phy_register
+ * i40e_mdio_if_number_selection - MDIO I/F number selection
+ * @hw: pointer to the hw struct
+ * @set_mdio: use MDIO I/F number specified by mdio_num
+ * @mdio_num: MDIO I/F number
+ * @cmd: pointer to PHY Register command structure
+ **/
+static void i40e_mdio_if_number_selection(struct i40e_hw *hw, bool set_mdio,
+					  u8 mdio_num,
+					  struct i40e_aqc_phy_register_access *cmd)
+{
+	if (set_mdio && cmd->phy_interface == I40E_AQ_PHY_REG_ACCESS_EXTERNAL) {
+		if (hw->flags & I40E_HW_FLAG_AQ_PHY_ACCESS_EXTENDED)
+			cmd->cmd_flags |=
+				I40E_AQ_PHY_REG_ACCESS_SET_MDIO_IF_NUMBER |
+				((mdio_num <<
+				I40E_AQ_PHY_REG_ACCESS_MDIO_IF_NUMBER_SHIFT) &
+				I40E_AQ_PHY_REG_ACCESS_MDIO_IF_NUMBER_MASK);
+		else
+			i40e_debug(hw, I40E_DEBUG_PHY,
+				   "MDIO I/F number selection not supported by current FW version.\n");
+	}
+}
+
+/**
+ * i40e_aq_set_phy_register_ext
  * @hw: pointer to the hw struct
  * @phy_select: select which phy should be accessed
  * @dev_addr: PHY device address
+ * @set_mdio: use MDIO I/F number specified by mdio_num
+ * @mdio_num: MDIO I/F number
  * @reg_addr: PHY register address
  * @reg_val: new register value
  * @cmd_details: pointer to command details structure or NULL
  *
  * Write the external PHY register.
+ * NOTE: In common cases MDIO I/F number should not be changed, thats why you
+ * may use simple wrapper i40e_aq_set_phy_register.
  **/
-i40e_status i40e_aq_set_phy_register(struct i40e_hw *hw,
-				     u8 phy_select, u8 dev_addr,
-				     u32 reg_addr, u32 reg_val,
-				     struct i40e_asq_cmd_details *cmd_details)
+enum i40e_status_code i40e_aq_set_phy_register_ext(struct i40e_hw *hw,
+			     u8 phy_select, u8 dev_addr, bool page_change,
+			     bool set_mdio, u8 mdio_num,
+			     u32 reg_addr, u32 reg_val,
+			     struct i40e_asq_cmd_details *cmd_details)
 {
 	struct i40e_aq_desc desc;
 	struct i40e_aqc_phy_register_access *cmd =
@@ -5351,26 +5380,36 @@ i40e_status i40e_aq_set_phy_register(struct i40e_hw *hw,
 	cmd->reg_address = cpu_to_le32(reg_addr);
 	cmd->reg_value = cpu_to_le32(reg_val);
 
+	i40e_mdio_if_number_selection(hw, set_mdio, mdio_num, cmd);
+
+	if (!page_change)
+		cmd->cmd_flags = I40E_AQ_PHY_REG_ACCESS_DONT_CHANGE_QSFP_PAGE;
+
 	status = i40e_asq_send_command(hw, &desc, NULL, 0, cmd_details);
 
 	return status;
 }
 
 /**
- * i40e_aq_get_phy_register
+ * i40e_aq_get_phy_register_ext
  * @hw: pointer to the hw struct
  * @phy_select: select which phy should be accessed
  * @dev_addr: PHY device address
+ * @set_mdio: use MDIO I/F number specified by mdio_num
+ * @mdio_num: MDIO I/F number
  * @reg_addr: PHY register address
  * @reg_val: read register value
  * @cmd_details: pointer to command details structure or NULL
  *
  * Read the external PHY register.
+ * NOTE: In common cases MDIO I/F number should not be changed, thats why you
+ * may use simple wrapper i40e_aq_get_phy_register.
  **/
-i40e_status i40e_aq_get_phy_register(struct i40e_hw *hw,
-				     u8 phy_select, u8 dev_addr,
-				     u32 reg_addr, u32 *reg_val,
-				     struct i40e_asq_cmd_details *cmd_details)
+enum i40e_status_code i40e_aq_get_phy_register_ext(struct i40e_hw *hw,
+			     u8 phy_select, u8 dev_addr, bool page_change,
+			     bool set_mdio, u8 mdio_num,
+			     u32 reg_addr, u32 *reg_val,
+			     struct i40e_asq_cmd_details *cmd_details)
 {
 	struct i40e_aq_desc desc;
 	struct i40e_aqc_phy_register_access *cmd =
@@ -5384,6 +5423,11 @@ i40e_status i40e_aq_get_phy_register(struct i40e_hw *hw,
 	cmd->dev_address = dev_addr;
 	cmd->reg_address = cpu_to_le32(reg_addr);
 
+	i40e_mdio_if_number_selection(hw, set_mdio, mdio_num, cmd);
+
+	if (!page_change)
+		cmd->cmd_flags = I40E_AQ_PHY_REG_ACCESS_DONT_CHANGE_QSFP_PAGE;
+
 	status = i40e_asq_send_command(hw, &desc, NULL, 0, cmd_details);
 	if (!status)
 		*reg_val = le32_to_cpu(cmd->reg_value);
diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
index 765bc5174ead..67a6bd52eb95 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
@@ -5261,7 +5261,7 @@ static int i40e_get_module_info(struct net_device *netdev,
 	case I40E_MODULE_TYPE_SFP:
 		status = i40e_aq_get_phy_register(hw,
 				I40E_AQ_PHY_REG_ACCESS_EXTERNAL_MODULE,
-				I40E_I2C_EEPROM_DEV_ADDR,
+				I40E_I2C_EEPROM_DEV_ADDR, true,
 				I40E_MODULE_SFF_8472_COMP,
 				&sff8472_comp, NULL);
 		if (status)
@@ -5269,7 +5269,7 @@ static int i40e_get_module_info(struct net_device *netdev,
 
 		status = i40e_aq_get_phy_register(hw,
 				I40E_AQ_PHY_REG_ACCESS_EXTERNAL_MODULE,
-				I40E_I2C_EEPROM_DEV_ADDR,
+				I40E_I2C_EEPROM_DEV_ADDR, true,
 				I40E_MODULE_SFF_8472_SWAP,
 				&sff8472_swap, NULL);
 		if (status)
@@ -5301,7 +5301,7 @@ static int i40e_get_module_info(struct net_device *netdev,
 		/* Read from memory page 0. */
 		status = i40e_aq_get_phy_register(hw,
 				I40E_AQ_PHY_REG_ACCESS_EXTERNAL_MODULE,
-				0,
+				0, true,
 				I40E_MODULE_REVISION_ADDR,
 				&sff8636_rev, NULL);
 		if (status)
@@ -5372,7 +5372,7 @@ static int i40e_get_module_eeprom(struct net_device *netdev,
 
 		status = i40e_aq_get_phy_register(hw,
 				I40E_AQ_PHY_REG_ACCESS_EXTERNAL_MODULE,
-				addr, offset, &value, NULL);
+				true, addr, offset, &value, NULL);
 		if (status)
 			return -EIO;
 		data[i] = value;
diff --git a/drivers/net/ethernet/intel/i40e/i40e_prototype.h b/drivers/net/ethernet/intel/i40e/i40e_prototype.h
index 7effe5010e32..bbb478f09093 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_prototype.h
+++ b/drivers/net/ethernet/intel/i40e/i40e_prototype.h
@@ -411,14 +411,24 @@ i40e_status i40e_aq_rx_ctl_write_register(struct i40e_hw *hw,
 				u32 reg_addr, u32 reg_val,
 				struct i40e_asq_cmd_details *cmd_details);
 void i40e_write_rx_ctl(struct i40e_hw *hw, u32 reg_addr, u32 reg_val);
-i40e_status i40e_aq_set_phy_register(struct i40e_hw *hw,
-				     u8 phy_select, u8 dev_addr,
-				     u32 reg_addr, u32 reg_val,
-				     struct i40e_asq_cmd_details *cmd_details);
-i40e_status i40e_aq_get_phy_register(struct i40e_hw *hw,
-				     u8 phy_select, u8 dev_addr,
-				     u32 reg_addr, u32 *reg_val,
-				     struct i40e_asq_cmd_details *cmd_details);
+enum i40e_status_code
+i40e_aq_set_phy_register_ext(struct i40e_hw *hw,
+			     u8 phy_select, u8 dev_addr, bool page_change,
+			     bool set_mdio, u8 mdio_num,
+			     u32 reg_addr, u32 reg_val,
+			     struct i40e_asq_cmd_details *cmd_details);
+enum i40e_status_code
+i40e_aq_get_phy_register_ext(struct i40e_hw *hw,
+			     u8 phy_select, u8 dev_addr, bool page_change,
+			     bool set_mdio, u8 mdio_num,
+			     u32 reg_addr, u32 *reg_val,
+			     struct i40e_asq_cmd_details *cmd_details);
+
+/* Convenience wrappers for most common use case */
+#define i40e_aq_set_phy_register(hw, ps, da, pc, ra, rv, cd)		\
+	i40e_aq_set_phy_register_ext(hw, ps, da, pc, false, 0, ra, rv, cd)
+#define i40e_aq_get_phy_register(hw, ps, da, pc, ra, rv, cd)		\
+	i40e_aq_get_phy_register_ext(hw, ps, da, pc, false, 0, ra, rv, cd)
 
 i40e_status i40e_read_phy_register_clause22(struct i40e_hw *hw,
 					    u16 reg, u8 phy_addr, u16 *value);
diff --git a/drivers/net/ethernet/intel/i40e/i40e_type.h b/drivers/net/ethernet/intel/i40e/i40e_type.h
index b43ec94a0f29..6ea2867ff60f 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_type.h
+++ b/drivers/net/ethernet/intel/i40e/i40e_type.h
@@ -624,6 +624,7 @@ struct i40e_hw {
 #define I40E_HW_FLAG_NVM_READ_REQUIRES_LOCK BIT_ULL(3)
 #define I40E_HW_FLAG_FW_LLDP_STOPPABLE      BIT_ULL(4)
 #define I40E_HW_FLAG_FW_LLDP_PERSISTENT     BIT_ULL(5)
+#define I40E_HW_FLAG_AQ_PHY_ACCESS_EXTENDED BIT_ULL(6)
 #define I40E_HW_FLAG_DROP_MODE              BIT_ULL(7)
 	u64 flags;
 
-- 
2.21.0

[net-next 06/11] i40e: remove the macro with it's argument reuse

[Jeff Kirsher]

From: Aleksandr Loktionov <aleksandr.loktionov@intel.com>

Remove macro and call i40e_update_vfid_in_stats() function directly
to avoid checkpatch.pl complains about macro argument reuse and
possible side effects.

Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 .../net/ethernet/intel/i40e/i40e_ethtool.c    | 28 ++++---------------
 1 file changed, 6 insertions(+), 22 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
index 67a6bd52eb95..883b43ac9816 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
@@ -2417,7 +2417,7 @@ static void i40e_get_ethtool_stats(struct net_device *netdev,
 }
 
 /**
- * __i40e_update_vfid_in_stats_strings - print VF num to stats names
+ * i40e_update_vfid_in_stats - print VF num to stats names
  * @stats_extra: array of stats structs with stats name strings
  * @strings_num: number of stats name strings in array above (length)
  * @vf_id: VF number to update stats name strings with
@@ -2425,8 +2425,8 @@ static void i40e_get_ethtool_stats(struct net_device *netdev,
  * Helper function to i40e_get_stat_strings() in case of extra stats.
  **/
 static inline void
-__i40e_update_vfid_in_stats_strings(struct i40e_stats stats_extra[],
-				    int strings_num, int vf_id)
+i40e_update_vfid_in_stats(struct i40e_stats stats_extra[],
+			  int strings_num, int vf_id)
 {
 	int i;
 
@@ -2438,24 +2438,6 @@ __i40e_update_vfid_in_stats_strings(struct i40e_stats stats_extra[],
 	}
 }
 
-/**
- * i40e_update_vfid_in_stats - print VF num to stat names
- * @stats_extra: array of stats structs with stats name strings
- * @vf_id: VF number to update stats name strings with
- *
- * Helper macro to i40e_get_stat_strings() to ease use of
- * __i40e_update_vfid_in_stats_strings() function due to extra stats.
- *
- * Macro to ease the use of __i40e_update_vfid_in_stats_strings by taking
- * a static constant stats array and passing the ARRAY_SIZE(). This avoids typos
- * by ensuring that we pass the size associated with the given stats array.
- *
- * The parameter @stats_extra is evaluated twice, so parameters with side
- * effects should be avoided.
- **/
-#define i40e_update_vfid_in_stats(stats_extra, vf_id) \
-__i40e_update_vfid_in_stats_strings(stats_extra, ARRAY_SIZE(stats_extra), vf_id)
-
 /**
  * i40e_get_stat_strings - copy stat strings into supplied buffer
  * @netdev: the netdev to collect strings for
@@ -2499,7 +2481,9 @@ static void i40e_get_stat_strings(struct net_device *netdev, u8 *data)
 		i40e_add_stat_strings(&data, i40e_gstrings_pfc_stats, i);
 
 	for (i = 0; i < I40E_STATS_EXTRA_COUNT; i++) {
-		i40e_update_vfid_in_stats(i40e_gstrings_eth_stats_extra, i);
+		i40e_update_vfid_in_stats
+			(i40e_gstrings_eth_stats_extra,
+			 ARRAY_SIZE(i40e_gstrings_eth_stats_extra), i);
 		i40e_add_stat_strings(&data, i40e_gstrings_eth_stats_extra);
 	}
 
-- 
2.21.0

[net-next 07/11] i40e: initialize ITRN registers with correct values

[Jeff Kirsher]

From: Nicholas Nunley <nicholas.d.nunley@intel.com>

Since commit 92418fb14750 ("i40e/i40evf: Use usec value instead of reg
value for ITR defines") the driver tracks the interrupt throttling
intervals in single usec units, although the actual ITRN/ITR0 registers are
programmed in 2 usec units. Most register programming flows in the driver
correctly handle the conversion, although it is currently not applied when
the registers are initialized to their default values. Most of the time
this doesn't present a problem since the default values are usually
immediately overwritten through the standard adaptive throttling mechanism,
or updated manually by the user, but if adaptive throttling is disabled and
the interval values are left alone then the incorrect value will persist.

Since the intended default interval of 50 usecs (vs. 100 usecs as
programmed) performs better for most traffic workloads, this can lead to
performance regressions.

This patch adds the correct conversion when writing the initial values to
the ITRN registers.

Signed-off-by: Nicholas Nunley <nicholas.d.nunley@intel.com>
Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 drivers/net/ethernet/intel/i40e/i40e_main.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index 6031223eafab..339925af0206 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -3534,14 +3534,14 @@ static void i40e_vsi_configure_msix(struct i40e_vsi *vsi)
 		q_vector->rx.target_itr =
 			ITR_TO_REG(vsi->rx_rings[i]->itr_setting);
 		wr32(hw, I40E_PFINT_ITRN(I40E_RX_ITR, vector - 1),
-		     q_vector->rx.target_itr);
+		     q_vector->rx.target_itr >> 1);
 		q_vector->rx.current_itr = q_vector->rx.target_itr;
 
 		q_vector->tx.next_update = jiffies + 1;
 		q_vector->tx.target_itr =
 			ITR_TO_REG(vsi->tx_rings[i]->itr_setting);
 		wr32(hw, I40E_PFINT_ITRN(I40E_TX_ITR, vector - 1),
-		     q_vector->tx.target_itr);
+		     q_vector->tx.target_itr >> 1);
 		q_vector->tx.current_itr = q_vector->tx.target_itr;
 
 		wr32(hw, I40E_PFINT_RATEN(vector - 1),
@@ -3646,11 +3646,11 @@ static void i40e_configure_msi_and_legacy(struct i40e_vsi *vsi)
 	/* set the ITR configuration */
 	q_vector->rx.next_update = jiffies + 1;
 	q_vector->rx.target_itr = ITR_TO_REG(vsi->rx_rings[0]->itr_setting);
-	wr32(hw, I40E_PFINT_ITR0(I40E_RX_ITR), q_vector->rx.target_itr);
+	wr32(hw, I40E_PFINT_ITR0(I40E_RX_ITR), q_vector->rx.target_itr >> 1);
 	q_vector->rx.current_itr = q_vector->rx.target_itr;
 	q_vector->tx.next_update = jiffies + 1;
 	q_vector->tx.target_itr = ITR_TO_REG(vsi->tx_rings[0]->itr_setting);
-	wr32(hw, I40E_PFINT_ITR0(I40E_TX_ITR), q_vector->tx.target_itr);
+	wr32(hw, I40E_PFINT_ITR0(I40E_TX_ITR), q_vector->tx.target_itr >> 1);
 	q_vector->tx.current_itr = q_vector->tx.target_itr;
 
 	i40e_enable_misc_int_causes(pf);
@@ -11396,7 +11396,7 @@ static int i40e_setup_misc_vector(struct i40e_pf *pf)
 
 	/* associate no queues to the misc vector */
 	wr32(hw, I40E_PFINT_LNKLST0, I40E_QUEUE_END_OF_LIST);
-	wr32(hw, I40E_PFINT_ITR0(I40E_RX_ITR), I40E_ITR_8K);
+	wr32(hw, I40E_PFINT_ITR0(I40E_RX_ITR), I40E_ITR_8K >> 1);
 
 	i40e_flush(hw);
 
-- 
2.21.0

[net-next 08/11] i40e: allow ethtool to report SW and FW versions in recovery mode

[Jeff Kirsher]

From: Piotr Kwapulinski <piotr.kwapulinski@intel.com>

Let ethtool print driver and firmware versions when NIC is in
recovery mode.  Assign i40e_get_drvinfo() operation to ethtool
recovery mode operations.  Previously ethtool did not report
driver and firmware versions when NIC was in recovery mode.

Signed-off-by: Piotr Kwapulinski <piotr.kwapulinski@intel.com>
Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 drivers/net/ethernet/intel/i40e/i40e_ethtool.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
index 883b43ac9816..001a2fad297d 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
@@ -5375,6 +5375,7 @@ static int i40e_set_eee(struct net_device *netdev, struct ethtool_eee *edata)
 }
 
 static const struct ethtool_ops i40e_ethtool_recovery_mode_ops = {
+	.get_drvinfo		= i40e_get_drvinfo,
 	.set_eeprom		= i40e_set_eeprom,
 	.get_eeprom_len		= i40e_get_eeprom_len,
 	.get_eeprom		= i40e_get_eeprom,
-- 
2.21.0

[net-next 09/11] i40e: Fix LED blinking flow for X710T*L devices.

[Jeff Kirsher]

From: Damian Milosek <damian.milosek@intel.com>

Add X710T*L device specific operations (in port LED detection and
handling of GLGEN_GPIO_CTL.PIN_FUNC field) to enable LED blinking.

Signed-off-by: Damian Milosek <damian.milosek@intel.com>
Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 drivers/net/ethernet/intel/i40e/i40e_common.c | 29 +++++++++++++++++--
 drivers/net/ethernet/intel/i40e/i40e_devids.h |  2 ++
 2 files changed, 28 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_common.c b/drivers/net/ethernet/intel/i40e/i40e_common.c
index ed8608b874f4..8b25a6d9c81d 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_common.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_common.c
@@ -1437,9 +1437,9 @@ static u32 i40e_led_is_mine(struct i40e_hw *hw, int idx)
 	u32 gpio_val = 0;
 	u32 port;
 
-	if (!hw->func_caps.led[idx])
+	if (!I40E_IS_X710TL_DEVICE(hw->device_id) &&
+	    !hw->func_caps.led[idx])
 		return 0;
-
 	gpio_val = rd32(hw, I40E_GLGEN_GPIO_CTL(idx));
 	port = (gpio_val & I40E_GLGEN_GPIO_CTL_PRT_NUM_MASK) >>
 		I40E_GLGEN_GPIO_CTL_PRT_NUM_SHIFT;
@@ -1458,8 +1458,15 @@ static u32 i40e_led_is_mine(struct i40e_hw *hw, int idx)
 #define I40E_FILTER_ACTIVITY 0xE
 #define I40E_LINK_ACTIVITY 0xC
 #define I40E_MAC_ACTIVITY 0xD
+#define I40E_FW_LED BIT(4)
+#define I40E_LED_MODE_VALID (I40E_GLGEN_GPIO_CTL_LED_MODE_MASK >> \
+			     I40E_GLGEN_GPIO_CTL_LED_MODE_SHIFT)
+
 #define I40E_LED0 22
 
+#define I40E_PIN_FUNC_SDP 0x0
+#define I40E_PIN_FUNC_LED 0x1
+
 /**
  * i40e_led_get - return current on/off mode
  * @hw: pointer to the hw struct
@@ -1504,8 +1511,10 @@ void i40e_led_set(struct i40e_hw *hw, u32 mode, bool blink)
 {
 	int i;
 
-	if (mode & 0xfffffff0)
+	if (mode & ~I40E_LED_MODE_VALID) {
 		hw_dbg(hw, "invalid mode passed in %X\n", mode);
+		return;
+	}
 
 	/* as per the documentation GPIO 22-29 are the LED
 	 * GPIO pins named LED0..LED7
@@ -1515,6 +1524,20 @@ void i40e_led_set(struct i40e_hw *hw, u32 mode, bool blink)
 
 		if (!gpio_val)
 			continue;
+
+		if (I40E_IS_X710TL_DEVICE(hw->device_id)) {
+			u32 pin_func = 0;
+
+			if (mode & I40E_FW_LED)
+				pin_func = I40E_PIN_FUNC_SDP;
+			else
+				pin_func = I40E_PIN_FUNC_LED;
+
+			gpio_val &= ~I40E_GLGEN_GPIO_CTL_PIN_FUNC_MASK;
+			gpio_val |= ((pin_func <<
+				     I40E_GLGEN_GPIO_CTL_PIN_FUNC_SHIFT) &
+				     I40E_GLGEN_GPIO_CTL_PIN_FUNC_MASK);
+		}
 		gpio_val &= ~I40E_GLGEN_GPIO_CTL_LED_MODE_MASK;
 		/* this & is a bit of paranoia, but serves as a range check */
 		gpio_val |= ((mode << I40E_GLGEN_GPIO_CTL_LED_MODE_SHIFT) &
diff --git a/drivers/net/ethernet/intel/i40e/i40e_devids.h b/drivers/net/ethernet/intel/i40e/i40e_devids.h
index bac4da031f9b..bf15a868292f 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_devids.h
+++ b/drivers/net/ethernet/intel/i40e/i40e_devids.h
@@ -23,6 +23,8 @@
 #define I40E_DEV_ID_10G_BASE_T_BC	0x15FF
 #define I40E_DEV_ID_10G_B		0x104F
 #define I40E_DEV_ID_10G_SFP		0x104E
+#define I40E_IS_X710TL_DEVICE(d) \
+	((d) == I40E_DEV_ID_10G_BASE_T_BC)
 #define I40E_DEV_ID_KX_X722		0x37CE
 #define I40E_DEV_ID_QSFP_X722		0x37CF
 #define I40E_DEV_ID_SFP_X722		0x37D0
-- 
2.21.0

[net-next 10/11] i40e: Refactoring VF MAC filters counting to make more reliable

[Jeff Kirsher]

From: Aleksandr Loktionov <aleksandr.loktionov@intel.com>

This patch prepares ground for the next VF MAC address change fix.
It lets untrusted VF to delete any VF mac filter, but it still
doesn't let untrusted VF to add mac filter not setup by PF.
It removes information duplication in num_mac mac filters counter.
And improves exact h/w mac filters usage checking in the
i40e_check_vf_permission() function by counting mac2add_cnt.
It also improves logging because now all mac addresses will be validated
first and corresponding messages will be logged.

Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
---
 drivers/net/ethernet/intel/i40e/i40e.h        |  1 +
 drivers/net/ethernet/intel/i40e/i40e_main.c   | 19 ++++++++
 .../ethernet/intel/i40e/i40e_virtchnl_pf.c    | 45 ++++++++-----------
 .../ethernet/intel/i40e/i40e_virtchnl_pf.h    |  1 -
 4 files changed, 39 insertions(+), 27 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e.h b/drivers/net/ethernet/intel/i40e/i40e.h
index 2af9f6308f84..cb6367334ca7 100644
--- a/drivers/net/ethernet/intel/i40e/i40e.h
+++ b/drivers/net/ethernet/intel/i40e/i40e.h
@@ -1118,6 +1118,7 @@ struct i40e_mac_filter *i40e_add_mac_filter(struct i40e_vsi *vsi,
 					    const u8 *macaddr);
 int i40e_del_mac_filter(struct i40e_vsi *vsi, const u8 *macaddr);
 bool i40e_is_vsi_in_vlan(struct i40e_vsi *vsi);
+int i40e_count_filters(struct i40e_vsi *vsi);
 struct i40e_mac_filter *i40e_find_mac(struct i40e_vsi *vsi, const u8 *macaddr);
 void i40e_vlan_stripping_enable(struct i40e_vsi *vsi);
 #ifdef CONFIG_I40E_DCB
diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index 339925af0206..2e4df0bd8d37 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -1109,6 +1109,25 @@ void i40e_update_stats(struct i40e_vsi *vsi)
 	i40e_update_vsi_stats(vsi);
 }
 
+/**
+ * i40e_count_filters - counts VSI mac filters
+ * @vsi: the VSI to be searched
+ *
+ * Returns count of mac filters
+ **/
+int i40e_count_filters(struct i40e_vsi *vsi)
+{
+	struct i40e_mac_filter *f;
+	struct hlist_node *h;
+	int bkt;
+	int cnt = 0;
+
+	hash_for_each_safe(vsi->mac_filter_hash, bkt, h, f, hlist)
+		++cnt;
+
+	return cnt;
+}
+
 /**
  * i40e_find_filter - Search VSI filter list for specific mac/vlan filter
  * @vsi: the VSI to be searched
diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
index 3d2440838822..a2710664d653 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
@@ -955,7 +955,6 @@ static void i40e_free_vf_res(struct i40e_vf *vf)
 		i40e_vsi_release(pf->vsi[vf->lan_vsi_idx]);
 		vf->lan_vsi_idx = 0;
 		vf->lan_vsi_id = 0;
-		vf->num_mac = 0;
 	}
 
 	/* do the accounting and remove additional ADq VSI's */
@@ -2548,20 +2547,12 @@ static inline int i40e_check_vf_permission(struct i40e_vf *vf,
 					   struct virtchnl_ether_addr_list *al)
 {
 	struct i40e_pf *pf = vf->pf;
+	struct i40e_vsi *vsi = pf->vsi[vf->lan_vsi_idx];
+	int mac2add_cnt = 0;
 	int i;
 
-	/* If this VF is not privileged, then we can't add more than a limited
-	 * number of addresses. Check to make sure that the additions do not
-	 * push us over the limit.
-	 */
-	if (!test_bit(I40E_VIRTCHNL_VF_CAP_PRIVILEGE, &vf->vf_caps) &&
-	    (vf->num_mac + al->num_elements) > I40E_VC_MAX_MAC_ADDR_PER_VF) {
-		dev_err(&pf->pdev->dev,
-			"Cannot add more MAC addresses, VF is not trusted, switch the VF to trusted to add more functionality\n");
-		return -EPERM;
-	}
-
 	for (i = 0; i < al->num_elements; i++) {
+		struct i40e_mac_filter *f;
 		u8 *addr = al->list[i].addr;
 
 		if (is_broadcast_ether_addr(addr) ||
@@ -2585,8 +2576,24 @@ static inline int i40e_check_vf_permission(struct i40e_vf *vf,
 				"VF attempting to override administratively set MAC address, bring down and up the VF interface to resume normal operation\n");
 			return -EPERM;
 		}
+
+		/*count filters that really will be added*/
+		f = i40e_find_mac(vsi, addr);
+		if (!f)
+			++mac2add_cnt;
 	}
 
+	/* If this VF is not privileged, then we can't add more than a limited
+	 * number of addresses. Check to make sure that the additions do not
+	 * push us over the limit.
+	 */
+	if (!test_bit(I40E_VIRTCHNL_VF_CAP_PRIVILEGE, &vf->vf_caps) &&
+	    (i40e_count_filters(vsi) + mac2add_cnt) >
+		    I40E_VC_MAX_MAC_ADDR_PER_VF) {
+		dev_err(&pf->pdev->dev,
+			"Cannot add more MAC addresses, VF is not trusted, switch the VF to trusted to add more functionality\n");
+		return -EPERM;
+	}
 	return 0;
 }
 
@@ -2640,8 +2647,6 @@ static int i40e_vc_add_mac_addr_msg(struct i40e_vf *vf, u8 *msg)
 				ret = I40E_ERR_PARAM;
 				spin_unlock_bh(&vsi->mac_filter_hash_lock);
 				goto error_param;
-			} else {
-				vf->num_mac++;
 			}
 		}
 	}
@@ -2689,16 +2694,6 @@ static int i40e_vc_del_mac_addr_msg(struct i40e_vf *vf, u8 *msg)
 			ret = I40E_ERR_INVALID_MAC_ADDR;
 			goto error_param;
 		}
-
-		if (vf->pf_set_mac &&
-		    ether_addr_equal(al->list[i].addr,
-				     vf->default_lan_addr.addr)) {
-			dev_err(&pf->pdev->dev,
-				"MAC addr %pM has been set by PF, cannot delete it for VF %d, reset VF to change MAC addr\n",
-				vf->default_lan_addr.addr, vf->vf_id);
-			ret = I40E_ERR_PARAM;
-			goto error_param;
-		}
 	}
 	vsi = pf->vsi[vf->lan_vsi_idx];
 
@@ -2709,8 +2704,6 @@ static int i40e_vc_del_mac_addr_msg(struct i40e_vf *vf, u8 *msg)
 			ret = I40E_ERR_INVALID_MAC_ADDR;
 			spin_unlock_bh(&vsi->mac_filter_hash_lock);
 			goto error_param;
-		} else {
-			vf->num_mac--;
 		}
 
 	spin_unlock_bh(&vsi->mac_filter_hash_lock);
diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.h b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.h
index 7164b9bb294f..1ce06240a702 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.h
+++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.h
@@ -101,7 +101,6 @@ struct i40e_vf {
 	bool link_up;		/* only valid if VF link is forced */
 	bool queues_enabled;	/* true if the VF queues are enabled */
 	bool spoofchk;
-	u16 num_mac;
 	u16 num_vlan;
 
 	/* ADq related variables */
-- 
2.21.0

[net-next 11/11] i40e: prevent memory leak in i40e_setup_macvlans

[Jeff Kirsher]

From: Navid Emamdoost <navid.emamdoost@gmail.com>

In i40e_setup_macvlans if i40e_setup_channel fails the allocated memory
for ch should be released.

Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com>
Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 drivers/net/ethernet/intel/i40e/i40e_main.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index 2e4df0bd8d37..141575ada588 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -7187,6 +7187,7 @@ static int i40e_setup_macvlans(struct i40e_vsi *vsi, u16 macvlan_cnt, u16 qcnt,
 		ch->num_queue_pairs = qcnt;
 		if (!i40e_setup_channel(pf, vsi, ch)) {
 			ret = -EINVAL;
+			kfree(ch);
 			goto err_free;
 		}
 		ch->parent_vsi = vsi;
-- 
2.21.0

Re: [net-next 02/11] i40e: Add ability to display VF stats along with PF core stats

[Jakub Kicinski]

On Wed, 23 Oct 2019 11:24:17 -0700, Jeff Kirsher wrote:
> From: Arkadiusz Grubba <arkadiusz.grubba@intel.com>
> 
> This change introduces the ability to display extended (enhanced)
> statistics for PF interfaces.
> 
> The patch introduces new arrays defined for these
> extra stats (in i40e_ethtool.c file) and enhances/extends ethtool ops
> functions intended for dealing with PF stats (i.e.: i40e_get_stats_count(),
> i40e_get_ethtool_stats(), i40e_get_stat_strings() ).

This commit message doesn't explain _what_ stats your adding, and _why_.

From glancing at the code you're dumping 128 * 12 stats, which are
basic netdev stats per-VF. 

These are trivially exposed on representors in modern designs.

> There have also been introduced the new build flag named
> "I40E_PF_EXTRA_STATS_OFF" to exclude from the driver code all code snippets
> associated with these extra stats.

And this doesn't even exist in the patch.

> Signed-off-by: Arkadiusz Grubba <arkadiusz.grubba@intel.com>
> Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>

RE: [net-next 02/11] i40e: Add ability to display VF stats along with PF core stats

[Grubba, Arkadiusz]

[-- Attachment #1: Type: text/plain, Size: 5765 bytes --]

Hi,

The main info about _what_ and _why_ , as you wrote, is explained in the first (i.e. title) line.
Namely, this change was introduced to "Add ability to display VF stats along with PF core stats" (for i40e equipment as prefix "i40e:" stands for it in the title).

(And if it was about general issues, i.e. why we introduce such changes, then, of course, they usually result mostly from the needs reported, e.g. by users using a given solution, although this does not change the nature/significance of the change from a technical point of view.)

As for further comments, that's right, you rightly notice here that the basic VF statistics are displayed and there may actually be an alternative possibility to check them (or other, newer solutions may appear that may enable it). The solution introduced here is simply one of the options (and maybe also the basis for further development of it).
But I don't know exactly for what specific purpose you mention it here?
What is the question? ...
(but for sure, if I guess right what you would like to ask, it's good to keep in mind that no tool is perfectly well in itself to the full extent of all use cases or... preferences - that's why we have alternatives and generally good to have them.)

[But also, such considerations already fall, for example, into the area of user preferences. And of course, the role of this patch is not to want to influence someone's preferences but only to provide some opportunity (as opposed to limiting the possibility of using various solutions, which should probably not be our goal...)
because among others here, this particular change is to be made available in connection with the exact and targeted needs raised by the users of the equipment affected by this code.]

As for the last point, this is indeed some oversight - yes, the last sentence is now unnecessary after rearranging this patch to meet the final requirements / agreements for the upstream (in-tree) version of it (as I also mentioned in my previous email - see attachment).
I think that instead of this last sentence in the commit message discussed here, and if you think it is important here, we may add (copy) from the original commit message this part of the text regarding description of displayed statistics:

+Testing hints:
+
+Use ethtool -S with this PF interface and check the output.
+Extra lines with the prefix "vf" should be displayed, e.g.:
+"
+     vf012.rx_bytes: 69264721849
+     vf012.rx_unicast: 45629259
+     vf012.rx_multicast: 9
+     vf012.rx_broadcast: 1
+     vf012.rx_discards: 2958
+     vf012.rx_unknown_protocol: 0
+     vf012.tx_bytes: 93048734
+     vf012.tx_unicast: 1409700
+     vf012.tx_multicast: 11
+     vf012.tx_broadcast: 0
+     vf012.tx_discards: 0
+     vf012.tx_errors: 0
+"
+(it's an example of a whole stats block for one VF).
+
+(For more specific tests:
+Create some VF interfaces, link them and give them IP addresses.
+Generate same network traffic and then follow the instructions above.)


(but for me it's really not certain whether in this particular case a larger description means better, especially since it is not so important from the point of view of the functioning of the kernel / driver or the system or interaction with them.)

Best Regards
A.G.


-----Original Message-----
From: Jakub Kicinski [mailto:jakub.kicinski@netronome.com] 
Sent: Thursday, October 24, 2019 5:42 AM
To: Kirsher, Jeffrey T <jeffrey.t.kirsher@intel.com>
Cc: davem@davemloft.net; Grubba, Arkadiusz <arkadiusz.grubba@intel.com>; netdev@vger.kernel.org; nhorman@redhat.com; sassmann@redhat.com; Bowers, AndrewX <andrewx.bowers@intel.com>
Subject: Re: [net-next 02/11] i40e: Add ability to display VF stats along with PF core stats

On Wed, 23 Oct 2019 11:24:17 -0700, Jeff Kirsher wrote:
> From: Arkadiusz Grubba <arkadiusz.grubba@intel.com>
> 
> This change introduces the ability to display extended (enhanced) 
> statistics for PF interfaces.
> 
> The patch introduces new arrays defined for these extra stats (in 
> i40e_ethtool.c file) and enhances/extends ethtool ops functions 
> intended for dealing with PF stats (i.e.: i40e_get_stats_count(), 
> i40e_get_ethtool_stats(), i40e_get_stat_strings() ).

This commit message doesn't explain _what_ stats your adding, and _why_.

From glancing at the code you're dumping 128 * 12 stats, which are basic netdev stats per-VF. 

These are trivially exposed on representors in modern designs.

> There have also been introduced the new build flag named 
> "I40E_PF_EXTRA_STATS_OFF" to exclude from the driver code all code 
> snippets associated with these extra stats.

And this doesn't even exist in the patch.

> Signed-off-by: Arkadiusz Grubba <arkadiusz.grubba@intel.com>
> Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>

--------------------------------------------------------------------

Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.

Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek
przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by
others is strictly prohibited.

[-- Attachment #2: email.txt --]
[-- Type: text/plain, Size: 9983 bytes --]

From: Grubba, Arkadiusz 
Sent: Tuesday, September 10, 2019 11:58 PM
To: Michael, Alice <alice.michael@intel.com>; e1000-patches@eclists.intel.com
Subject: RE: [e1000-patches] [next PATCH S10 02/11] i40e: Add ability to display VF stats along with PF core stats

Hi Alice,

The last sentence in the commit message should be deleted because it is unnecessary/unrelated to this particular case. (I mean the sentence about the build flag.) And originally there was also a comment (five lines) to the code in the function i40e_get_stats_count() ...
Apart from this, ACK.

Thanks
Arek


-----Original Message-----
From: e1000-patches-request@eclists.intel.com [mailto:e1000-patches-request@eclists.intel.com] On Behalf Of Michael, Alice
Sent: Tuesday, September 10, 2019 10:00 PM
To: Michael, Alice <alice.michael@intel.com>; e1000-patches@eclists.intel.com
Cc: Grubba, Arkadiusz <arkadiusz.grubba@intel.com>
Subject: [e1000-patches] [next PATCH S10 02/11] i40e: Add ability to display VF stats along with PF core stats

From: Arkadiusz Grubba <arkadiusz.grubba@intel.com>

This change introduces the ability to display extended (enhanced) statistics for PF interfaces (in accordance to the new build flags also introduced here).

The patch introduces new arrays and preprocessor symbols defined for these extra stats (in i40e_ethtool.c file) and enhances/extends ethtool ops functions intended for dealing with PF stats (i.e.: i40e_get_stats_count(), i40e_get_ethtool_stats(), i40e_get_stat_strings() ).

There have also been introduced the new build flag named "I40E_PF_EXTRA_STATS_OFF" to exclude from the driver code all code snippets associated with these extra stats.

Signed-off-by: Arkadiusz Grubba <arkadiusz.grubba@intel.com>
---
 .../net/ethernet/intel/i40e/i40e_ethtool.c    | 149 ++++++++++++++++++
 1 file changed, 149 insertions(+)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
index 41e1240acaea..c814c756b4bb 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
@@ -389,6 +389,7 @@ static const struct i40e_stats i40e_gstrings_pfc_stats[] = {
 
 #define I40E_GLOBAL_STATS_LEN	ARRAY_SIZE(i40e_gstrings_stats)
 
+/* Length (number) of PF core stats only (i.e. without queues / extra
+stats): */
 #define I40E_PF_STATS_LEN	(I40E_GLOBAL_STATS_LEN + \
 				 I40E_PFC_STATS_LEN + \
 				 I40E_VEB_STATS_LEN + \
@@ -397,6 +398,44 @@ static const struct i40e_stats i40e_gstrings_pfc_stats[] = {
 /* Length of stats for a single queue */
 #define I40E_QUEUE_STATS_LEN	ARRAY_SIZE(i40e_gstrings_queue_stats)
 
+#define I40E_STATS_NAME_VFID_EXTRA "vf___."
+#define I40E_STATS_NAME_VFID_EXTRA_LEN
+(sizeof(I40E_STATS_NAME_VFID_EXTRA) - 1)
+
+static struct i40e_stats i40e_gstrings_eth_stats_extra[] = {
+	I40E_VSI_STAT(I40E_STATS_NAME_VFID_EXTRA
+		      "rx_bytes", eth_stats.rx_bytes),
+	I40E_VSI_STAT(I40E_STATS_NAME_VFID_EXTRA
+		      "rx_unicast", eth_stats.rx_unicast),
+	I40E_VSI_STAT(I40E_STATS_NAME_VFID_EXTRA
+		      "rx_multicast", eth_stats.rx_multicast),
+	I40E_VSI_STAT(I40E_STATS_NAME_VFID_EXTRA
+		      "rx_broadcast", eth_stats.rx_broadcast),
+	I40E_VSI_STAT(I40E_STATS_NAME_VFID_EXTRA
+		      "rx_discards", eth_stats.rx_discards),
+	I40E_VSI_STAT(I40E_STATS_NAME_VFID_EXTRA
+		      "rx_unknown_protocol", eth_stats.rx_unknown_protocol),
+	I40E_VSI_STAT(I40E_STATS_NAME_VFID_EXTRA
+		      "tx_bytes", eth_stats.tx_bytes),
+	I40E_VSI_STAT(I40E_STATS_NAME_VFID_EXTRA
+		      "tx_unicast", eth_stats.tx_unicast),
+	I40E_VSI_STAT(I40E_STATS_NAME_VFID_EXTRA
+		      "tx_multicast", eth_stats.tx_multicast),
+	I40E_VSI_STAT(I40E_STATS_NAME_VFID_EXTRA
+		      "tx_broadcast", eth_stats.tx_broadcast),
+	I40E_VSI_STAT(I40E_STATS_NAME_VFID_EXTRA
+		      "tx_discards", eth_stats.tx_discards),
+	I40E_VSI_STAT(I40E_STATS_NAME_VFID_EXTRA
+		      "tx_errors", eth_stats.tx_errors), };
+
+#define I40E_STATS_EXTRA_COUNT	128  /* as for now only I40E_MAX_VF_COUNT */
+/* Following length value does not include the length values for queues stats */
+#define I40E_STATS_EXTRA_LEN	ARRAY_SIZE(i40e_gstrings_eth_stats_extra)
+/* Length (number) of PF extra stats only (i.e. without core stats /
+queues): */ #define I40E_PF_STATS_EXTRA_LEN (I40E_STATS_EXTRA_COUNT *
+I40E_STATS_EXTRA_LEN)
+/* Length (number) of enhanced/all PF stats (i.e. core with extra
+stats): */ #define I40E_PF_STATS_ENHANCE_LEN (I40E_PF_STATS_LEN +
+I40E_PF_STATS_EXTRA_LEN)
+
 enum i40e_ethtool_test_id {
 	I40E_ETH_TEST_REG = 0,
 	I40E_ETH_TEST_EEPROM,
@@ -2190,6 +2229,9 @@ static int i40e_get_stats_count(struct net_device *netdev)
 	 */
 	stats_len += I40E_QUEUE_STATS_LEN * 2 * netdev->num_tx_queues;
 
+	if (vsi == pf->vsi[pf->lan_vsi] && pf->hw.partition_id == 1)
+		stats_len += I40E_PF_STATS_EXTRA_LEN;
+
 	return stats_len;
 }
 
@@ -2258,6 +2300,10 @@ static void i40e_get_ethtool_stats(struct net_device *netdev,
 	struct i40e_vsi *vsi = np->vsi;
 	struct i40e_pf *pf = vsi->back;
 	struct i40e_veb *veb = NULL;
+	unsigned int vsi_idx;
+	unsigned int vf_idx;
+	unsigned int vf_id;
+	bool is_vf_valid;
 	unsigned int i;
 	bool veb_stats;
 	u64 *p = data;
@@ -2307,11 +2353,109 @@ static void i40e_get_ethtool_stats(struct net_device *netdev,
 		i40e_add_ethtool_stats(&data, &pfc, i40e_gstrings_pfc_stats);
 	}
 
+	/* As for now, we only process the SRIOV type VSIs (as extra stats to
+	 * PF core stats) which are correlated with VF LAN VSI (hence below,
+	 * in this for-loop instruction block, only VF's LAN VSIs are currently
+	 * processed).
+	 */
+	for (vf_id = 0; vf_id < pf->num_alloc_vfs; vf_id++) {
+		is_vf_valid = true;
+		for (vf_idx = 0; vf_idx < pf->num_alloc_vfs; vf_idx++)
+			if (pf->vf[vf_idx].vf_id == vf_id)
+				break;
+		if (vf_idx >= pf->num_alloc_vfs) {
+			dev_info(&pf->pdev->dev,
+				 "In the PF's array, there is no VF instance with VF_ID identifier %d or it is not set/initialized correctly yet\n",
+				 vf_id);
+			is_vf_valid = false;
+			goto check_vf;
+		}
+		vsi_idx = pf->vf[vf_idx].lan_vsi_idx;
+
+		vsi = pf->vsi[vsi_idx];
+		if (!vsi) {
+			/* It means empty field in the PF VSI array... */
+			dev_info(&pf->pdev->dev,
+				 "No LAN VSI instance referenced by VF %d or it is not set/initialized correctly yet\n",
+				 vf_id);
+			is_vf_valid = false;
+			goto check_vf;
+		}
+		if (vsi->vf_id != vf_id) {
+			dev_info(&pf->pdev->dev,
+				 "In the PF's array, there is incorrectly set/initialized LAN VSI or reference to it from VF %d is not set/initialized correctly yet\n",
+				 vf_id);
+			is_vf_valid = false;
+			goto check_vf;
+		}
+		if (vsi->vf_id != pf->vf[vf_idx].vf_id ||
+		    !i40e_find_vsi_from_id(pf, pf->vf[vsi->vf_id].lan_vsi_id)) {
+			/* Disjointed identifiers or broken references VF-VSI */
+			dev_warn(&pf->pdev->dev,
+				 "SRIOV LAN VSI (index %d in PF VSI array) with invalid VF Identifier %d (referenced by VF %d, ordered as %d in VF array)\n",
+				 vsi_idx, pf->vsi[vsi_idx]->vf_id,
+				 pf->vf[vf_idx].vf_id, vf_idx);
+			is_vf_valid = false;
+		}
+check_vf:
+		if (!is_vf_valid) {
+			i40e_add_ethtool_stats(&data, NULL,
+					       i40e_gstrings_eth_stats_extra);
+		} else {
+			i40e_update_eth_stats(vsi);
+			i40e_add_ethtool_stats(&data, vsi,
+					       i40e_gstrings_eth_stats_extra);
+		}
+	}
+	for (; vf_id < I40E_STATS_EXTRA_COUNT; vf_id++)
+		i40e_add_ethtool_stats(&data, NULL,
+				       i40e_gstrings_eth_stats_extra);
+
 check_data_pointer:
 	WARN_ONCE(data - p != i40e_get_stats_count(netdev),
 		  "ethtool stats count mismatch!");
 }
 
+/**
+ * __i40e_update_vfid_in_stats_strings - print VF num to stats names
+ * @stats_extra: array of stats structs with stats name strings
+ * @strings_num: number of stats name strings in array above (length)
+ * @vf_id: VF number to update stats name strings with
+ *
+ * Helper function to i40e_get_stat_strings() in case of extra stats.
+ **/
+static inline void
+__i40e_update_vfid_in_stats_strings(struct i40e_stats stats_extra[],
+				    int strings_num, int vf_id)
+{
+	int i;
+
+	for (i = 0; i < strings_num; i++) {
+		snprintf(stats_extra[i].stat_string,
+			 I40E_STATS_NAME_VFID_EXTRA_LEN, "vf%03d", vf_id);
+		stats_extra[i].stat_string[I40E_STATS_NAME_VFID_EXTRA_LEN -
+								       1] = '.';
+	}
+}
+
+/**
+ * i40e_update_vfid_in_stats - print VF num to stat names
+ * @stats_extra: array of stats structs with stats name strings
+ * @vf_id: VF number to update stats name strings with
+ *
+ * Helper macro to i40e_get_stat_strings() to ease use of
+ * __i40e_update_vfid_in_stats_strings() function due to extra stats.
+ *
+ * Macro to ease the use of __i40e_update_vfid_in_stats_strings by 
+taking
+ * a static constant stats array and passing the ARRAY_SIZE(). This 
+avoids typos
+ * by ensuring that we pass the size associated with the given stats array.
+ *
+ * The parameter @stats_extra is evaluated twice, so parameters with 
+side
+ * effects should be avoided.
+ **/
+#define i40e_update_vfid_in_stats(stats_extra, vf_id) \ 
+__i40e_update_vfid_in_stats_strings(stats_extra,
+ARRAY_SIZE(stats_extra), vf_id)
+
 /**
  * i40e_get_stat_strings - copy stat strings into supplied buffer
  * @netdev: the netdev to collect strings for @@ -2354,6 +2498,11 @@ static void i40e_get_stat_strings(struct net_device *netdev, u8 *data)
 	for (i = 0; i < I40E_MAX_USER_PRIORITY; i++)
 		i40e_add_stat_strings(&data, i40e_gstrings_pfc_stats, i);
 
+	for (i = 0; i < I40E_STATS_EXTRA_COUNT; i++) {
+		i40e_update_vfid_in_stats(i40e_gstrings_eth_stats_extra, i);
+		i40e_add_stat_strings(&data, i40e_gstrings_eth_stats_extra);
+	}
+
 check_data_pointer:
 	WARN_ONCE(data - p != i40e_get_stats_count(netdev) * ETH_GSTRING_LEN,
 		  "stat strings count mismatch!");
--
2.21.0

Re: [net-next 02/11] i40e: Add ability to display VF stats along with PF core stats

[Jakub Kicinski]

On Mon, 28 Oct 2019 13:51:07 +0000, Grubba, Arkadiusz wrote:
> Hi,

Please don't top post.

> The main info about _what_ and _why_ , as you wrote, is explained in the first (i.e. title) line.
> Namely, this change was introduced to "Add ability to display VF stats along with PF core stats" (for i40e equipment as prefix "i40e:" stands for it in the title).
> 
> (And if it was about general issues, i.e. why we introduce such changes, then, of course, they usually result mostly from the needs reported, e.g. by users using a given solution, although this does not change the nature/significance of the change from a technical point of view.)
> 
> As for further comments, that's right, you rightly notice here that the basic VF statistics are displayed and there may actually be an alternative possibility to check them (or other, newer solutions may appear that may enable it). The solution introduced here is simply one of the options (and maybe also the basis for further development of it).
> But I don't know exactly for what specific purpose you mention it here?
> What is the question? ...
> (but for sure, if I guess right what you would like to ask, it's good to keep in mind that no tool is perfectly well in itself to the full extent of all use cases or... preferences - that's why we have alternatives and generally good to have them.)
> 
> [But also, such considerations already fall, for example, into the area of user preferences. And of course, the role of this patch is not to want to influence someone's preferences but only to provide some opportunity (as opposed to limiting the possibility of using various solutions, which should probably not be our goal...)
> because among others here, this particular change is to be made available in connection with the exact and targeted needs raised by the users of the equipment affected by this code.]

It's not a matter of preference. I object to abuse of free-form APIs
for things which have proper, modern interfaces.

You're adding 12 * 128 = 1536 statistics to ethtool -S. That's
going to make the output absolutely unreadable for a human being.

> As for the last point, this is indeed some oversight - yes, the last sentence is now unnecessary after rearranging this patch to meet the final requirements / agreements for the upstream (in-tree) version of it (as I also mentioned in my previous email - see attachment).
> I think that instead of this last sentence in the commit message discussed here, and if you think it is important here, we may add (copy) from the original commit message this part of the text regarding description of displayed statistics:
> 
> +Testing hints:
> +
> +Use ethtool -S with this PF interface and check the output.
> +Extra lines with the prefix "vf" should be displayed, e.g.:
> +"
> +     vf012.rx_bytes: 69264721849
> +     vf012.rx_unicast: 45629259
> +     vf012.rx_multicast: 9
> +     vf012.rx_broadcast: 1
> +     vf012.rx_discards: 2958
> +     vf012.rx_unknown_protocol: 0
> +     vf012.tx_bytes: 93048734
> +     vf012.tx_unicast: 1409700
> +     vf012.tx_multicast: 11
> +     vf012.tx_broadcast: 0
> +     vf012.tx_discards: 0
> +     vf012.tx_errors: 0
> +"
> +(it's an example of a whole stats block for one VF).
> +
> +(For more specific tests:
> +Create some VF interfaces, link them and give them IP addresses.
> +Generate same network traffic and then follow the instructions above.)
> 
> 
> (but for me it's really not certain whether in this particular case a larger description means better, especially since it is not so important from the point of view of the functioning of the kernel / driver or the system or interaction with them.)
> 
> Best Regards
> A.G.
> 
> 
> -----Original Message-----
> From: Jakub Kicinski [mailto:jakub.kicinski@netronome.com] 
> Sent: Thursday, October 24, 2019 5:42 AM
> To: Kirsher, Jeffrey T <jeffrey.t.kirsher@intel.com>
> Cc: davem@davemloft.net; Grubba, Arkadiusz <arkadiusz.grubba@intel.com>; netdev@vger.kernel.org; nhorman@redhat.com; sassmann@redhat.com; Bowers, AndrewX <andrewx.bowers@intel.com>
> Subject: Re: [net-next 02/11] i40e: Add ability to display VF stats along with PF core stats
> 
> On Wed, 23 Oct 2019 11:24:17 -0700, Jeff Kirsher wrote:
> > From: Arkadiusz Grubba <arkadiusz.grubba@intel.com>
> > 
> > This change introduces the ability to display extended (enhanced) 
> > statistics for PF interfaces.
> > 
> > The patch introduces new arrays defined for these extra stats (in 
> > i40e_ethtool.c file) and enhances/extends ethtool ops functions 
> > intended for dealing with PF stats (i.e.: i40e_get_stats_count(), 
> > i40e_get_ethtool_stats(), i40e_get_stat_strings() ).  
> 
> This commit message doesn't explain _what_ stats your adding, and _why_.
> 
> From glancing at the code you're dumping 128 * 12 stats, which are basic netdev stats per-VF. 
> 
> These are trivially exposed on representors in modern designs.
> 
> > There have also been introduced the new build flag named 
> > "I40E_PF_EXTRA_STATS_OFF" to exclude from the driver code all code 
> > snippets associated with these extra stats.  
> 
> And this doesn't even exist in the patch.
> 
> > Signed-off-by: Arkadiusz Grubba <arkadiusz.grubba@intel.com>
> > Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
> > Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>  
> 
> --------------------------------------------------------------------
> 
> Intel Technology Poland sp. z o.o.
> ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.
> 
> Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek
> przegladanie lub rozpowszechnianie jest zabronione.
> This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by
> others is strictly prohibited.

Re: [net-next 02/11] i40e: Add ability to display VF stats along with PF core stats

[David Miller]

From: Jakub Kicinski <jakub.kicinski@netronome.com>
Date: Mon, 28 Oct 2019 10:30:47 -0700

> It's not a matter of preference. I object to abuse of free-form APIs
> for things which have proper, modern interfaces.

+1

Re: [net-next 02/11] i40e: Add ability to display VF stats along with PF core stats

[Alexander Duyck]

On Thu, Oct 24, 2019 at 2:08 AM Jeff Kirsher
<jeffrey.t.kirsher@intel.com> wrote:
>
> From: Arkadiusz Grubba <arkadiusz.grubba@intel.com>
>
> This change introduces the ability to display extended (enhanced)
> statistics for PF interfaces.
>
> The patch introduces new arrays defined for these
> extra stats (in i40e_ethtool.c file) and enhances/extends ethtool ops
> functions intended for dealing with PF stats (i.e.: i40e_get_stats_count(),
> i40e_get_ethtool_stats(), i40e_get_stat_strings() ).
>
> There have also been introduced the new build flag named
> "I40E_PF_EXTRA_STATS_OFF" to exclude from the driver code all code snippets
> associated with these extra stats.
>
> Signed-off-by: Arkadiusz Grubba <arkadiusz.grubba@intel.com>
> Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>

So this patch is bad. It is overwriting the statistics strings for
each VF separately. In addition the code isn't really easy to follow
for the stats update as it seems like it is doing a bunch of extra
work and generating far more noise then it needs to.

>  .../net/ethernet/intel/i40e/i40e_ethtool.c    | 149 ++++++++++++++++++
>  1 file changed, 149 insertions(+)
>
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
> index 41e1240acaea..c814c756b4bb 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
> @@ -389,6 +389,7 @@ static const struct i40e_stats i40e_gstrings_pfc_stats[] = {
>
>  #define I40E_GLOBAL_STATS_LEN  ARRAY_SIZE(i40e_gstrings_stats)
>
> +/* Length (number) of PF core stats only (i.e. without queues / extra stats): */
>  #define I40E_PF_STATS_LEN      (I40E_GLOBAL_STATS_LEN + \
>                                  I40E_PFC_STATS_LEN + \
>                                  I40E_VEB_STATS_LEN + \
> @@ -397,6 +398,44 @@ static const struct i40e_stats i40e_gstrings_pfc_stats[] = {
>  /* Length of stats for a single queue */
>  #define I40E_QUEUE_STATS_LEN   ARRAY_SIZE(i40e_gstrings_queue_stats)
>
> +#define I40E_STATS_NAME_VFID_EXTRA "vf___."
> +#define I40E_STATS_NAME_VFID_EXTRA_LEN (sizeof(I40E_STATS_NAME_VFID_EXTRA) - 1)
> +

Why bother with this? If you are just going to skip over it in
__i40e_update_vfid_in_stats_strings() anyway why waste the memory on 5
characters per stat? It would simplify your code to just skip it here
since you are inserting it later anyway.

> +static struct i40e_stats i40e_gstrings_eth_stats_extra[] = {

This should be const.

> +       I40E_VSI_STAT(I40E_STATS_NAME_VFID_EXTRA
> +                     "rx_bytes", eth_stats.rx_bytes),
> +       I40E_VSI_STAT(I40E_STATS_NAME_VFID_EXTRA
> +                     "rx_unicast", eth_stats.rx_unicast),
> +       I40E_VSI_STAT(I40E_STATS_NAME_VFID_EXTRA
> +                     "rx_multicast", eth_stats.rx_multicast),
> +       I40E_VSI_STAT(I40E_STATS_NAME_VFID_EXTRA
> +                     "rx_broadcast", eth_stats.rx_broadcast),
> +       I40E_VSI_STAT(I40E_STATS_NAME_VFID_EXTRA
> +                     "rx_discards", eth_stats.rx_discards),
> +       I40E_VSI_STAT(I40E_STATS_NAME_VFID_EXTRA
> +                     "rx_unknown_protocol", eth_stats.rx_unknown_protocol),
> +       I40E_VSI_STAT(I40E_STATS_NAME_VFID_EXTRA
> +                     "tx_bytes", eth_stats.tx_bytes),
> +       I40E_VSI_STAT(I40E_STATS_NAME_VFID_EXTRA
> +                     "tx_unicast", eth_stats.tx_unicast),
> +       I40E_VSI_STAT(I40E_STATS_NAME_VFID_EXTRA
> +                     "tx_multicast", eth_stats.tx_multicast),
> +       I40E_VSI_STAT(I40E_STATS_NAME_VFID_EXTRA
> +                     "tx_broadcast", eth_stats.tx_broadcast),
> +       I40E_VSI_STAT(I40E_STATS_NAME_VFID_EXTRA
> +                     "tx_discards", eth_stats.tx_discards),
> +       I40E_VSI_STAT(I40E_STATS_NAME_VFID_EXTRA
> +                     "tx_errors", eth_stats.tx_errors),
> +};
> +
> +#define I40E_STATS_EXTRA_COUNT 128  /* as for now only I40E_MAX_VF_COUNT */
> +/* Following length value does not include the length values for queues stats */
> +#define I40E_STATS_EXTRA_LEN   ARRAY_SIZE(i40e_gstrings_eth_stats_extra)
> +/* Length (number) of PF extra stats only (i.e. without core stats / queues): */
> +#define I40E_PF_STATS_EXTRA_LEN (I40E_STATS_EXTRA_COUNT * I40E_STATS_EXTRA_LEN)
> +/* Length (number) of enhanced/all PF stats (i.e. core with extra stats): */
> +#define I40E_PF_STATS_ENHANCE_LEN (I40E_PF_STATS_LEN + I40E_PF_STATS_EXTRA_LEN)
> +
>  enum i40e_ethtool_test_id {
>         I40E_ETH_TEST_REG = 0,
>         I40E_ETH_TEST_EEPROM,
> @@ -2190,6 +2229,9 @@ static int i40e_get_stats_count(struct net_device *netdev)
>          */
>         stats_len += I40E_QUEUE_STATS_LEN * 2 * netdev->num_tx_queues;
>
> +       if (vsi == pf->vsi[pf->lan_vsi] && pf->hw.partition_id == 1)
> +               stats_len += I40E_PF_STATS_EXTRA_LEN;
> +
>         return stats_len;
>  }
>

This bit is just wasteful. You already have this check up above in
this function. Why not just add this to I40E_PF_STATS_LEN and be done
with it?

> @@ -2258,6 +2300,10 @@ static void i40e_get_ethtool_stats(struct net_device *netdev,
>         struct i40e_vsi *vsi = np->vsi;
>         struct i40e_pf *pf = vsi->back;
>         struct i40e_veb *veb = NULL;
> +       unsigned int vsi_idx;
> +       unsigned int vf_idx;
> +       unsigned int vf_id;
> +       bool is_vf_valid;
>         unsigned int i;
>         bool veb_stats;
>         u64 *p = data;
> @@ -2307,11 +2353,109 @@ static void i40e_get_ethtool_stats(struct net_device *netdev,
>                 i40e_add_ethtool_stats(&data, &pfc, i40e_gstrings_pfc_stats);
>         }
>
> +       /* As for now, we only process the SRIOV type VSIs (as extra stats to
> +        * PF core stats) which are correlated with VF LAN VSI (hence below,
> +        * in this for-loop instruction block, only VF's LAN VSIs are currently
> +        * processed).
> +        */
> +       for (vf_id = 0; vf_id < pf->num_alloc_vfs; vf_id++) {
> +               is_vf_valid = true;
> +               for (vf_idx = 0; vf_idx < pf->num_alloc_vfs; vf_idx++)
> +                       if (pf->vf[vf_idx].vf_id == vf_id)
> +                               break;
> +               if (vf_idx >= pf->num_alloc_vfs) {
> +                       dev_info(&pf->pdev->dev,
> +                                "In the PF's array, there is no VF instance with VF_ID identifier %d or it is not set/initialized correctly yet\n",
> +                                vf_id);
> +                       is_vf_valid = false;
> +                       goto check_vf;
> +               }
> +               vsi_idx = pf->vf[vf_idx].lan_vsi_idx;
> +

Okay so this whole block here is just ugly.Why bother with trying to
output this all in-order? We have the stats you need to output as a
giant array, and you should know the base index of that array. So
instead of making this way more complicated and expensive then it
needs to be why not just determine the offset  that you need to output
the stats to based off of the vf_id? It would be much more readable
then the approach you have taken.

> +               vsi = pf->vsi[vsi_idx];
> +               if (!vsi) {
> +                       /* It means empty field in the PF VSI array... */
> +                       dev_info(&pf->pdev->dev,
> +                                "No LAN VSI instance referenced by VF %d or it is not set/initialized correctly yet\n",
> +                                vf_id);
> +                       is_vf_valid = false;
> +                       goto check_vf;
> +               }

This is getting noisy really quick. Do you really need to dump
information if you cannot collect stats on a given VF? There are way
too many messages in here in my opinion.

> +               if (vsi->vf_id != vf_id) {
> +                       dev_info(&pf->pdev->dev,
> +                                "In the PF's array, there is incorrectly set/initialized LAN VSI or reference to it from VF %d is not set/initialized correctly yet\n",
> +                                vf_id);
> +                       is_vf_valid = false;
> +                       goto check_vf;
> +               }

This is more noise. It concerns me that you need all these checks. Is
this something you can actually encounter. If so then maybe these
should be wrapped in some sort of reader/writer lock like what we have
for the netdev queue statistics.

> +               if (vsi->vf_id != pf->vf[vf_idx].vf_id ||
> +                   !i40e_find_vsi_from_id(pf, pf->vf[vsi->vf_id].lan_vsi_id)) {
> +                       /* Disjointed identifiers or broken references VF-VSI */
> +                       dev_warn(&pf->pdev->dev,
> +                                "SRIOV LAN VSI (index %d in PF VSI array) with invalid VF Identifier %d (referenced by VF %d, ordered as %d in VF array)\n",
> +                                vsi_idx, pf->vsi[vsi_idx]->vf_id,
> +                                pf->vf[vf_idx].vf_id, vf_idx);
> +                       is_vf_valid = false;
> +               }

Here we finally get to any productive work.

So as I mentioned before there is a much simpler way to deal with all
of this. What you can do is zero out all of the stats, and then when
you hit this part you just have to access "data + (vf_id *
I40E_STATS_EXTRA_LEN)".

> +check_vf:
> +               if (!is_vf_valid) {
> +                       i40e_add_ethtool_stats(&data, NULL,
> +                                              i40e_gstrings_eth_stats_extra);
> +               } else {
> +                       i40e_update_eth_stats(vsi);
> +                       i40e_add_ethtool_stats(&data, vsi,
> +                                              i40e_gstrings_eth_stats_extra);
> +               }
> +       }
> +       for (; vf_id < I40E_STATS_EXTRA_COUNT; vf_id++)
> +               i40e_add_ethtool_stats(&data, NULL,
> +                                      i40e_gstrings_eth_stats_extra);
> +
>  check_data_pointer:
>         WARN_ONCE(data - p != i40e_get_stats_count(netdev),
>                   "ethtool stats count mismatch!");
>  }
>
> +/**
> + * __i40e_update_vfid_in_stats_strings - print VF num to stats names
> + * @stats_extra: array of stats structs with stats name strings
> + * @strings_num: number of stats name strings in array above (length)
> + * @vf_id: VF number to update stats name strings with
> + *
> + * Helper function to i40e_get_stat_strings() in case of extra stats.
> + **/
> +static inline void
> +__i40e_update_vfid_in_stats_strings(struct i40e_stats stats_extra[],
> +                                   int strings_num, int vf_id)
> +{
> +       int i;
> +
> +       for (i = 0; i < strings_num; i++) {
> +               snprintf(stats_extra[i].stat_string,
> +                        I40E_STATS_NAME_VFID_EXTRA_LEN, "vf%03d", vf_id);
> +               stats_extra[i].stat_string[I40E_STATS_NAME_VFID_EXTRA_LEN -
> +                                                                      1] = '.';
> +       }
> +}
> +

So this is just ugly on so many levels. Actually now that I have
looked into it a bit more why couldn't you simply re-purpose the
__i40e-add_stat_strings() code? You could pre-format your VF strings
and then just have them all get inserted with the correct names and
indexes later.

> +/**
> + * i40e_update_vfid_in_stats - print VF num to stat names
> + * @stats_extra: array of stats structs with stats name strings
> + * @vf_id: VF number to update stats name strings with
> + *
> + * Helper macro to i40e_get_stat_strings() to ease use of
> + * __i40e_update_vfid_in_stats_strings() function due to extra stats.
> + *
> + * Macro to ease the use of __i40e_update_vfid_in_stats_strings by taking
> + * a static constant stats array and passing the ARRAY_SIZE(). This avoids typos
> + * by ensuring that we pass the size associated with the given stats array.
> + *
> + * The parameter @stats_extra is evaluated twice, so parameters with side
> + * effects should be avoided.
> + **/
> +#define i40e_update_vfid_in_stats(stats_extra, vf_id) \
> +__i40e_update_vfid_in_stats_strings(stats_extra, ARRAY_SIZE(stats_extra), vf_id)
> +
>  /**
>   * i40e_get_stat_strings - copy stat strings into supplied buffer
>   * @netdev: the netdev to collect strings for
> @@ -2354,6 +2498,11 @@ static void i40e_get_stat_strings(struct net_device *netdev, u8 *data)
>         for (i = 0; i < I40E_MAX_USER_PRIORITY; i++)
>                 i40e_add_stat_strings(&data, i40e_gstrings_pfc_stats, i);
>
> +       for (i = 0; i < I40E_STATS_EXTRA_COUNT; i++) {
> +               i40e_update_vfid_in_stats(i40e_gstrings_eth_stats_extra, i);
> +               i40e_add_stat_strings(&data, i40e_gstrings_eth_stats_extra);
> +       }
> +

Okay, now this is officially a hard NAK. I hadn't noticed this until
now but you are overwriting the i40e_gstrings_eth_stats_extra? That
should be a const value.

My advice is that this should work like the Tx/Rx rings and PFC stats
do. You cannot be rewriting the strings for every VF. It makes much
more sense to simply use them as an input string and out put the
formatted string into the destination.