[Intel-wired-lan] [PATCH v6 0/2] fix ethtool statistics patches

All of lore.kernel.org
 help / color / mirror / Atom feed

* [Intel-wired-lan] [PATCH v6 0/2] fix ethtool statistics patches
@ 2016-03-04 23:37 Jacob Keller
  2016-03-04 23:37 ` [Intel-wired-lan] [PATCH v6 1/2] fm10k: add helper functions to set strings and data for ethtool stats Jacob Keller
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Jacob Keller @ 2016-03-04 23:37 UTC (permalink / raw)
  To: intel-wired-lan

This series fixes the ethtool patches on Jeff's next-queue. First, I
fixed a bug in the fm10k ethtool stat helpers which had assumed the
allocated memory was already zeroed. The new code assumes it is not, and
always rights 0s. In addition, I fixed Bruce's comment about the
newline. Since I had to change the first patch, I rebased them both
against the top of Jeff's queue and merged some of the code. The first
patch in this series now handles the per-queue stats, while the second
patch does the work to remove the incorrect support for
debug-statistics.

The first patch is a direct replacement for 1256b40cb7b4 ("fm10k: add
helper functions to set strings and data for ethtool stats", 2016-03-04)

The second patch is a direct replacement for 2d6c14d75442 ("fm10k:
cleanup fm10k stats and remove debug-statistics", 2016-03-04)

The interdiff between the previous version of these patches and the
current version of the patches (to help with review) is:

diff --git c/drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c w/drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c
index 3941b80ab2d2..a41a35082ddb 100644
--- c/drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c
+++ w/drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c
@@ -130,7 +130,6 @@ static const struct fm10k_stats fm10k_gstrings_queue_stats[] = {
 				FM10K_NETDEV_STATS_LEN + \
 				FM10K_MBX_STATS_LEN)
 
-
 static const char fm10k_gstrings_test[][ETH_GSTRING_LEN] = {
 	"Mailbox test (on/offline)"
 };
@@ -246,9 +245,10 @@ static void fm10k_add_ethtool_stats(u64 **data, void *pointer,
 	unsigned int i;
 	char *p;
 
-	/* simply skip forward if we were not given a valid pointer */
 	if (!pointer) {
-		*data += size;
+		/* memory is not zero allocated so we have to clear it */
+		for (i = 0; i < size; i++)
+			*((*data)++) = 0;
 		return;
 	}
 
Jacob Keller (2):
  fm10k: add helper functions to set strings and data for ethtool stats
  fm10k: remove remove debug-statistics support

 drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c | 259 +++++++++--------------
 1 file changed, 100 insertions(+), 159 deletions(-)

-- 
2.7.1.429.g45cd78e


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [Intel-wired-lan] [PATCH v6 1/2] fm10k: add helper functions to set strings and data for ethtool stats
  2016-03-04 23:37 [Intel-wired-lan] [PATCH v6 0/2] fix ethtool statistics patches Jacob Keller
@ 2016-03-04 23:37 ` Jacob Keller
  2016-03-30 22:51   ` Singh, Krishneil K
  2016-03-04 23:37 ` [Intel-wired-lan] [PATCH v6 2/2] fm10k: remove remove debug-statistics support Jacob Keller
  2016-03-04 23:38 ` [Intel-wired-lan] [PATCH v6 0/2] fix ethtool statistics patches Keller, Jacob E
  2 siblings, 1 reply; 6+ messages in thread
From: Jacob Keller @ 2016-03-04 23:37 UTC (permalink / raw)
  To: intel-wired-lan

Reduce duplicate code and the amount of indentation by adding
fm10k_add_stat_strings and fm10k_add_ethtool_stats functions which help
add fm10k_stat structures to the ethtool stats callbacks. This helps
increase ease of use for future stat additions, and increases code
readability.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---

Notes:
    - v6
    * since this is now part of series, label it v6
    * fix fm10k_add_ethtool_stats to properly zero the data
    * handle the queue stats here as well, instead of in a separate patch
    
    This is a direct replacement for 1256b40cb7b4 ("fm10k: add helper functions to
    set strings and data for ethtool stats", 2016-03-04), and thus the previous
    version should be dropped from the queue.

 drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c | 219 ++++++++++++-----------
 1 file changed, 115 insertions(+), 104 deletions(-)

diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c b/drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c
index c23b682ad27e..93f26c8b520c 100644
--- a/drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c
+++ b/drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c
@@ -121,13 +121,22 @@ static const struct fm10k_stats fm10k_gstrings_mbx_stats[] = {
 	FM10K_MBX_STAT("mbx_rx_mbmem_pushed", rx_mbmem_pushed),
 };
 
+#define FM10K_QUEUE_STAT(_name, _stat) { \
+	.stat_string = _name, \
+	.sizeof_stat = FIELD_SIZEOF(struct fm10k_ring, _stat), \
+	.stat_offset = offsetof(struct fm10k_ring, _stat) \
+}
+
+static const struct fm10k_stats fm10k_gstrings_queue_stats[] = {
+	FM10K_QUEUE_STAT("packets", stats.packets),
+	FM10K_QUEUE_STAT("bytes", stats.packets),
+};
+
 #define FM10K_GLOBAL_STATS_LEN ARRAY_SIZE(fm10k_gstrings_global_stats)
 #define FM10K_DEBUG_STATS_LEN ARRAY_SIZE(fm10k_gstrings_debug_stats)
 #define FM10K_PF_STATS_LEN ARRAY_SIZE(fm10k_gstrings_pf_stats)
 #define FM10K_MBX_STATS_LEN ARRAY_SIZE(fm10k_gstrings_mbx_stats)
-
-#define FM10K_QUEUE_STATS_LEN(_n) \
-	((_n) * 2 * (sizeof(struct fm10k_queue_stats) / sizeof(u64)))
+#define FM10K_QUEUE_STATS_LEN ARRAY_SIZE(fm10k_gstrings_queue_stats)
 
 #define FM10K_STATIC_STATS_LEN (FM10K_GLOBAL_STATS_LEN + \
 				FM10K_NETDEV_STATS_LEN + \
@@ -153,69 +162,66 @@ static const char fm10k_prv_flags[FM10K_PRV_FLAG_LEN][ETH_GSTRING_LEN] = {
 	"debug-statistics",
 };
 
+static void fm10k_add_stat_strings(char **p, const char *prefix,
+				   const struct fm10k_stats stats[],
+				   const unsigned int size)
+{
+	unsigned int i;
+
+	for (i = 0; i < size; i++) {
+		snprintf(*p, ETH_GSTRING_LEN, "%s%s",
+			 prefix, stats[i].stat_string);
+		*p += ETH_GSTRING_LEN;
+	}
+}
+
 static void fm10k_get_stat_strings(struct net_device *dev, u8 *data)
 {
 	struct fm10k_intfc *interface = netdev_priv(dev);
 	struct fm10k_iov_data *iov_data = interface->iov_data;
 	char *p = (char *)data;
 	unsigned int i;
-	unsigned int j;
 
-	for (i = 0; i < FM10K_NETDEV_STATS_LEN; i++) {
-		memcpy(p, fm10k_gstrings_net_stats[i].stat_string,
-		       ETH_GSTRING_LEN);
-		p += ETH_GSTRING_LEN;
-	}
+	fm10k_add_stat_strings(&p, "", fm10k_gstrings_net_stats,
+			       FM10K_NETDEV_STATS_LEN);
 
-	for (i = 0; i < FM10K_GLOBAL_STATS_LEN; i++) {
-		memcpy(p, fm10k_gstrings_global_stats[i].stat_string,
-		       ETH_GSTRING_LEN);
-		p += ETH_GSTRING_LEN;
-	}
+	fm10k_add_stat_strings(&p, "", fm10k_gstrings_global_stats,
+			       FM10K_GLOBAL_STATS_LEN);
 
-	if (interface->flags & FM10K_FLAG_DEBUG_STATS) {
-		for (i = 0; i < FM10K_DEBUG_STATS_LEN; i++) {
-			memcpy(p, fm10k_gstrings_debug_stats[i].stat_string,
-			       ETH_GSTRING_LEN);
-			p += ETH_GSTRING_LEN;
-		}
-	}
+	if (interface->flags & FM10K_FLAG_DEBUG_STATS)
+		fm10k_add_stat_strings(&p, "", fm10k_gstrings_debug_stats,
+				       FM10K_DEBUG_STATS_LEN);
 
-	for (i = 0; i < FM10K_MBX_STATS_LEN; i++) {
-		memcpy(p, fm10k_gstrings_mbx_stats[i].stat_string,
-		       ETH_GSTRING_LEN);
-		p += ETH_GSTRING_LEN;
-	}
+	fm10k_add_stat_strings(&p, "", fm10k_gstrings_mbx_stats,
+			       FM10K_MBX_STATS_LEN);
 
-	if (interface->hw.mac.type != fm10k_mac_vf) {
-		for (i = 0; i < FM10K_PF_STATS_LEN; i++) {
-			memcpy(p, fm10k_gstrings_pf_stats[i].stat_string,
-			       ETH_GSTRING_LEN);
-			p += ETH_GSTRING_LEN;
-		}
-	}
+	if (interface->hw.mac.type != fm10k_mac_vf)
+		fm10k_add_stat_strings(&p, "", fm10k_gstrings_pf_stats,
+				       FM10K_PF_STATS_LEN);
 
 	if ((interface->flags & FM10K_FLAG_DEBUG_STATS) && iov_data) {
 		for (i = 0; i < iov_data->num_vfs; i++) {
-			for (j = 0; j < FM10K_MBX_STATS_LEN; j++) {
-				snprintf(p,
-					 ETH_GSTRING_LEN,
-					 "vf_%u_%s", i,
-					 fm10k_gstrings_mbx_stats[j].stat_string);
-				p += ETH_GSTRING_LEN;
-			}
+			char prefix[ETH_GSTRING_LEN];
+
+			snprintf(prefix, ETH_GSTRING_LEN, "vf_%u_", i);
+			fm10k_add_stat_strings(&p, prefix,
+					       fm10k_gstrings_mbx_stats,
+					       FM10K_MBX_STATS_LEN);
 		}
 	}
 
 	for (i = 0; i < interface->hw.mac.max_queues; i++) {
-		snprintf(p, ETH_GSTRING_LEN, "tx_queue_%u_packets", i);
-		p += ETH_GSTRING_LEN;
-		snprintf(p, ETH_GSTRING_LEN, "tx_queue_%u_bytes", i);
-		p += ETH_GSTRING_LEN;
-		snprintf(p, ETH_GSTRING_LEN, "rx_queue_%u_packets", i);
-		p += ETH_GSTRING_LEN;
-		snprintf(p, ETH_GSTRING_LEN, "rx_queue_%u_bytes", i);
-		p += ETH_GSTRING_LEN;
+		char prefix[ETH_GSTRING_LEN];
+
+		snprintf(prefix, ETH_GSTRING_LEN, "tx_queue_%u_", i);
+		fm10k_add_stat_strings(&p, prefix,
+				       fm10k_gstrings_queue_stats,
+				       FM10K_QUEUE_STATS_LEN);
+
+		snprintf(prefix, ETH_GSTRING_LEN, "rx_queue_%u_", i);
+		fm10k_add_stat_strings(&p, prefix,
+				       fm10k_gstrings_queue_stats,
+				       FM10K_QUEUE_STATS_LEN);
 	}
 }
 
@@ -250,7 +256,7 @@ static int fm10k_get_sset_count(struct net_device *dev, int sset)
 	case ETH_SS_TEST:
 		return FM10K_TEST_LEN;
 	case ETH_SS_STATS:
-		stats_len += FM10K_QUEUE_STATS_LEN(hw->mac.max_queues);
+		stats_len += hw->mac.max_queues * 2 * FM10K_QUEUE_STATS_LEN;
 
 		if (hw->mac.type != fm10k_mac_vf)
 			stats_len += FM10K_PF_STATS_LEN;
@@ -271,55 +277,72 @@ static int fm10k_get_sset_count(struct net_device *dev, int sset)
 	}
 }
 
+static void fm10k_add_ethtool_stats(u64 **data, void *pointer,
+				    const struct fm10k_stats stats[],
+				    const unsigned int size)
+{
+	unsigned int i;
+	char *p;
+
+	if (!pointer) {
+		/* memory is not zero allocated so we have to clear it */
+		for (i = 0; i < size; i++)
+			*((*data)++) = 0;
+		return;
+	}
+
+	for (i = 0; i < size; i++) {
+		p = (char *)pointer + stats[i].stat_offset;
+
+		switch (stats[i].sizeof_stat) {
+		case sizeof(u64):
+			*((*data)++) = *(u64 *)p;
+			break;
+		case sizeof(u32):
+			*((*data)++) = *(u32 *)p;
+			break;
+		case sizeof(u16):
+			*((*data)++) = *(u16 *)p;
+			break;
+		case sizeof(u8):
+			*((*data)++) = *(u8 *)p;
+			break;
+		default:
+			*((*data)++) = 0;
+		}
+	}
+}
+
 static void fm10k_get_ethtool_stats(struct net_device *netdev,
 				    struct ethtool_stats __always_unused *stats,
 				    u64 *data)
 {
-	const int stat_count = sizeof(struct fm10k_queue_stats) / sizeof(u64);
 	struct fm10k_intfc *interface = netdev_priv(netdev);
 	struct fm10k_iov_data *iov_data = interface->iov_data;
 	struct net_device_stats *net_stats = &netdev->stats;
-	char *p;
-	int i, j;
+	int i;
 
 	fm10k_update_stats(interface);
 
-	for (i = 0; i < FM10K_NETDEV_STATS_LEN; i++) {
-		p = (char *)net_stats + fm10k_gstrings_net_stats[i].stat_offset;
-		*(data++) = (fm10k_gstrings_net_stats[i].sizeof_stat ==
-			sizeof(u64)) ? *(u64 *)p : *(u32 *)p;
-	}
+	fm10k_add_ethtool_stats(&data, net_stats, fm10k_gstrings_net_stats,
+				FM10K_NETDEV_STATS_LEN);
 
-	for (i = 0; i < FM10K_GLOBAL_STATS_LEN; i++) {
-		p = (char *)interface +
-		    fm10k_gstrings_global_stats[i].stat_offset;
-		*(data++) = (fm10k_gstrings_global_stats[i].sizeof_stat ==
-			sizeof(u64)) ? *(u64 *)p : *(u32 *)p;
-	}
+	fm10k_add_ethtool_stats(&data, interface, fm10k_gstrings_global_stats,
+				FM10K_GLOBAL_STATS_LEN);
 
-	if (interface->flags & FM10K_FLAG_DEBUG_STATS) {
-		for (i = 0; i < FM10K_DEBUG_STATS_LEN; i++) {
-			p = (char *)interface +
-				fm10k_gstrings_debug_stats[i].stat_offset;
-			*(data++) = (fm10k_gstrings_debug_stats[i].sizeof_stat ==
-				     sizeof(u64)) ? *(u64 *)p : *(u32 *)p;
-		}
-	}
+	if (interface->flags & FM10K_FLAG_DEBUG_STATS)
+		fm10k_add_ethtool_stats(&data, interface,
+					fm10k_gstrings_debug_stats,
+					FM10K_DEBUG_STATS_LEN);
 
-	for (i = 0; i < FM10K_MBX_STATS_LEN; i++) {
-		p = (char *)&interface->hw.mbx +
-			fm10k_gstrings_mbx_stats[i].stat_offset;
-		*(data++) = (fm10k_gstrings_mbx_stats[i].sizeof_stat ==
-			sizeof(u64)) ? *(u64 *)p : *(u32 *)p;
-	}
+	fm10k_add_ethtool_stats(&data, &interface->hw.mbx,
+				fm10k_gstrings_mbx_stats,
+				FM10K_MBX_STATS_LEN);
 
 	if (interface->hw.mac.type != fm10k_mac_vf) {
-		for (i = 0; i < FM10K_PF_STATS_LEN; i++) {
-			p = (char *)interface +
-			    fm10k_gstrings_pf_stats[i].stat_offset;
-			*(data++) = (fm10k_gstrings_pf_stats[i].sizeof_stat ==
-				     sizeof(u64)) ? *(u64 *)p : *(u32 *)p;
-		}
+		fm10k_add_ethtool_stats(&data, interface,
+					fm10k_gstrings_pf_stats,
+					FM10K_PF_STATS_LEN);
 	}
 
 	if ((interface->flags & FM10K_FLAG_DEBUG_STATS) && iov_data) {
@@ -328,36 +351,24 @@ static void fm10k_get_ethtool_stats(struct net_device *netdev,
 
 			vf_info = &iov_data->vf_info[i];
 
-			/* skip stats if we don't have a vf info */
-			if (!vf_info) {
-				data += FM10K_MBX_STATS_LEN;
-				continue;
-			}
-
-			for (j = 0; j < FM10K_MBX_STATS_LEN; j++) {
-				p = (char *)&vf_info->mbx +
-					fm10k_gstrings_mbx_stats[j].stat_offset;
-				*(data++) = (fm10k_gstrings_mbx_stats[j].sizeof_stat ==
-					     sizeof(u64)) ? *(u64 *)p : *(u32 *)p;
-			}
+			fm10k_add_ethtool_stats(&data, &vf_info->mbx,
+						fm10k_gstrings_mbx_stats,
+						FM10K_MBX_STATS_LEN);
 		}
 	}
 
 	for (i = 0; i < interface->hw.mac.max_queues; i++) {
 		struct fm10k_ring *ring;
-		u64 *queue_stat;
 
 		ring = interface->tx_ring[i];
-		if (ring)
-			queue_stat = (u64 *)&ring->stats;
-		for (j = 0; j < stat_count; j++)
-			*(data++) = ring ? queue_stat[j] : 0;
+		fm10k_add_ethtool_stats(&data, ring,
+					fm10k_gstrings_queue_stats,
+					FM10K_QUEUE_STATS_LEN);
 
 		ring = interface->rx_ring[i];
-		if (ring)
-			queue_stat = (u64 *)&ring->stats;
-		for (j = 0; j < stat_count; j++)
-			*(data++) = ring ? queue_stat[j] : 0;
+		fm10k_add_ethtool_stats(&data, ring,
+					fm10k_gstrings_queue_stats,
+					FM10K_QUEUE_STATS_LEN);
 	}
 }
 
-- 
2.7.1.429.g45cd78e


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [Intel-wired-lan] [PATCH v6 1/2] fm10k: add helper functions to set strings and data for ethtool stats
  2016-03-04 23:37 ` [Intel-wired-lan] [PATCH v6 1/2] fm10k: add helper functions to set strings and data for ethtool stats Jacob Keller
@ 2016-03-30 22:51   ` Singh, Krishneil K
  0 siblings, 0 replies; 6+ messages in thread
From: Singh, Krishneil K @ 2016-03-30 22:51 UTC (permalink / raw)
  To: intel-wired-lan


-----Original Message-----
From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lists.osuosl.org] On Behalf Of Jacob Keller
Sent: Friday, March 4, 2016 3:38 PM
To: Intel Wired LAN <intel-wired-lan@lists.osuosl.org>
Subject: [Intel-wired-lan] [PATCH v6 1/2] fm10k: add helper functions to set strings and data for ethtool stats

Reduce duplicate code and the amount of indentation by adding fm10k_add_stat_strings and fm10k_add_ethtool_stats functions which help add fm10k_stat structures to the ethtool stats callbacks. This helps increase ease of use for future stat additions, and increases code readability.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---


Tested-by: Krishneil Singh <Krishneil.k.singh@intel.com>



^ permalink raw reply	[flat|nested] 6+ messages in thread

* [Intel-wired-lan] [PATCH v6 2/2] fm10k: remove remove debug-statistics support
  2016-03-04 23:37 [Intel-wired-lan] [PATCH v6 0/2] fix ethtool statistics patches Jacob Keller
  2016-03-04 23:37 ` [Intel-wired-lan] [PATCH v6 1/2] fm10k: add helper functions to set strings and data for ethtool stats Jacob Keller
@ 2016-03-04 23:37 ` Jacob Keller
  2016-03-30 22:50   ` Singh, Krishneil K
  2016-03-04 23:38 ` [Intel-wired-lan] [PATCH v6 0/2] fix ethtool statistics patches Keller, Jacob E
  2 siblings, 1 reply; 6+ messages in thread
From: Jacob Keller @ 2016-03-04 23:37 UTC (permalink / raw)
  To: intel-wired-lan

This change fixes an (ab)use of the ethtool stats API, which could
result in corrupt memory or misleading stat output. The ethtool stats
API is not robust enough to handle varying number of statistics due to
how it requests the size and allocates memory. Remove the poorly conceived
support originally added for extra debug statistics. In the future,
a new stats api may open up the ability to display these statistics.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---

Notes:
    - v6
    * move the queue stats changes to the first patch in this series
    * rework commit message to explain why we need to remove this feature
    
    This is a direct replacement for 2d6c14d75442 ("fm10k: cleanup fm10k stats and
    remove debug-statistics", 2016-03-04) which was on the queue. The previous
    version has a different title, but should be removed from the queue before
    applying this version.

 drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c | 72 +-----------------------
 1 file changed, 1 insertion(+), 71 deletions(-)

diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c b/drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c
index 93f26c8b520c..a41a35082ddb 100644
--- a/drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c
+++ b/drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c
@@ -81,17 +81,6 @@ static const struct fm10k_stats fm10k_gstrings_global_stats[] = {
 	FM10K_STAT("tx_hwtstamp_timeouts", tx_hwtstamp_timeouts),
 };
 
-static const struct fm10k_stats fm10k_gstrings_debug_stats[] = {
-	FM10K_STAT("hw_sm_mbx_full", hw_sm_mbx_full),
-	FM10K_STAT("hw_csum_tx_good", hw_csum_tx_good),
-	FM10K_STAT("hw_csum_rx_good", hw_csum_rx_good),
-	FM10K_STAT("rx_switch_errors", rx_switch_errors),
-	FM10K_STAT("rx_drops", rx_drops),
-	FM10K_STAT("rx_pp_errors", rx_pp_errors),
-	FM10K_STAT("rx_link_errors", rx_link_errors),
-	FM10K_STAT("rx_length_errors", rx_length_errors),
-};
-
 static const struct fm10k_stats fm10k_gstrings_pf_stats[] = {
 	FM10K_STAT("timeout", stats.timeout.count),
 	FM10K_STAT("ur", stats.ur.count),
@@ -133,7 +122,6 @@ static const struct fm10k_stats fm10k_gstrings_queue_stats[] = {
 };
 
 #define FM10K_GLOBAL_STATS_LEN ARRAY_SIZE(fm10k_gstrings_global_stats)
-#define FM10K_DEBUG_STATS_LEN ARRAY_SIZE(fm10k_gstrings_debug_stats)
 #define FM10K_PF_STATS_LEN ARRAY_SIZE(fm10k_gstrings_pf_stats)
 #define FM10K_MBX_STATS_LEN ARRAY_SIZE(fm10k_gstrings_mbx_stats)
 #define FM10K_QUEUE_STATS_LEN ARRAY_SIZE(fm10k_gstrings_queue_stats)
@@ -154,12 +142,10 @@ enum fm10k_self_test_types {
 };
 
 enum {
-	FM10K_PRV_FLAG_DEBUG_STATS,
 	FM10K_PRV_FLAG_LEN,
 };
 
 static const char fm10k_prv_flags[FM10K_PRV_FLAG_LEN][ETH_GSTRING_LEN] = {
-	"debug-statistics",
 };
 
 static void fm10k_add_stat_strings(char **p, const char *prefix,
@@ -178,7 +164,6 @@ static void fm10k_add_stat_strings(char **p, const char *prefix,
 static void fm10k_get_stat_strings(struct net_device *dev, u8 *data)
 {
 	struct fm10k_intfc *interface = netdev_priv(dev);
-	struct fm10k_iov_data *iov_data = interface->iov_data;
 	char *p = (char *)data;
 	unsigned int i;
 
@@ -188,10 +173,6 @@ static void fm10k_get_stat_strings(struct net_device *dev, u8 *data)
 	fm10k_add_stat_strings(&p, "", fm10k_gstrings_global_stats,
 			       FM10K_GLOBAL_STATS_LEN);
 
-	if (interface->flags & FM10K_FLAG_DEBUG_STATS)
-		fm10k_add_stat_strings(&p, "", fm10k_gstrings_debug_stats,
-				       FM10K_DEBUG_STATS_LEN);
-
 	fm10k_add_stat_strings(&p, "", fm10k_gstrings_mbx_stats,
 			       FM10K_MBX_STATS_LEN);
 
@@ -199,17 +180,6 @@ static void fm10k_get_stat_strings(struct net_device *dev, u8 *data)
 		fm10k_add_stat_strings(&p, "", fm10k_gstrings_pf_stats,
 				       FM10K_PF_STATS_LEN);
 
-	if ((interface->flags & FM10K_FLAG_DEBUG_STATS) && iov_data) {
-		for (i = 0; i < iov_data->num_vfs; i++) {
-			char prefix[ETH_GSTRING_LEN];
-
-			snprintf(prefix, ETH_GSTRING_LEN, "vf_%u_", i);
-			fm10k_add_stat_strings(&p, prefix,
-					       fm10k_gstrings_mbx_stats,
-					       FM10K_MBX_STATS_LEN);
-		}
-	}
-
 	for (i = 0; i < interface->hw.mac.max_queues; i++) {
 		char prefix[ETH_GSTRING_LEN];
 
@@ -248,7 +218,6 @@ static void fm10k_get_strings(struct net_device *dev,
 static int fm10k_get_sset_count(struct net_device *dev, int sset)
 {
 	struct fm10k_intfc *interface = netdev_priv(dev);
-	struct fm10k_iov_data *iov_data = interface->iov_data;
 	struct fm10k_hw *hw = &interface->hw;
 	int stats_len = FM10K_STATIC_STATS_LEN;
 
@@ -261,14 +230,6 @@ static int fm10k_get_sset_count(struct net_device *dev, int sset)
 		if (hw->mac.type != fm10k_mac_vf)
 			stats_len += FM10K_PF_STATS_LEN;
 
-		if (interface->flags & FM10K_FLAG_DEBUG_STATS) {
-			stats_len += FM10K_DEBUG_STATS_LEN;
-
-			if (iov_data)
-				stats_len += FM10K_MBX_STATS_LEN *
-					iov_data->num_vfs;
-		}
-
 		return stats_len;
 	case ETH_SS_PRIV_FLAGS:
 		return FM10K_PRV_FLAG_LEN;
@@ -318,7 +279,6 @@ static void fm10k_get_ethtool_stats(struct net_device *netdev,
 				    u64 *data)
 {
 	struct fm10k_intfc *interface = netdev_priv(netdev);
-	struct fm10k_iov_data *iov_data = interface->iov_data;
 	struct net_device_stats *net_stats = &netdev->stats;
 	int i;
 
@@ -330,11 +290,6 @@ static void fm10k_get_ethtool_stats(struct net_device *netdev,
 	fm10k_add_ethtool_stats(&data, interface, fm10k_gstrings_global_stats,
 				FM10K_GLOBAL_STATS_LEN);
 
-	if (interface->flags & FM10K_FLAG_DEBUG_STATS)
-		fm10k_add_ethtool_stats(&data, interface,
-					fm10k_gstrings_debug_stats,
-					FM10K_DEBUG_STATS_LEN);
-
 	fm10k_add_ethtool_stats(&data, &interface->hw.mbx,
 				fm10k_gstrings_mbx_stats,
 				FM10K_MBX_STATS_LEN);
@@ -345,18 +300,6 @@ static void fm10k_get_ethtool_stats(struct net_device *netdev,
 					FM10K_PF_STATS_LEN);
 	}
 
-	if ((interface->flags & FM10K_FLAG_DEBUG_STATS) && iov_data) {
-		for (i = 0; i < iov_data->num_vfs; i++) {
-			struct fm10k_vf_info *vf_info;
-
-			vf_info = &iov_data->vf_info[i];
-
-			fm10k_add_ethtool_stats(&data, &vf_info->mbx,
-						fm10k_gstrings_mbx_stats,
-						FM10K_MBX_STATS_LEN);
-		}
-	}
-
 	for (i = 0; i < interface->hw.mac.max_queues; i++) {
 		struct fm10k_ring *ring;
 
@@ -1015,27 +958,14 @@ static void fm10k_self_test(struct net_device *dev,
 
 static u32 fm10k_get_priv_flags(struct net_device *netdev)
 {
-	struct fm10k_intfc *interface = netdev_priv(netdev);
-	u32 priv_flags = 0;
-
-	if (interface->flags & FM10K_FLAG_DEBUG_STATS)
-		priv_flags |= BIT(FM10K_PRV_FLAG_DEBUG_STATS);
-
-	return priv_flags;
+	return 0;
 }
 
 static int fm10k_set_priv_flags(struct net_device *netdev, u32 priv_flags)
 {
-	struct fm10k_intfc *interface = netdev_priv(netdev);
-
 	if (priv_flags >= BIT(FM10K_PRV_FLAG_LEN))
 		return -EINVAL;
 
-	if (priv_flags & BIT(FM10K_PRV_FLAG_DEBUG_STATS))
-		interface->flags |= FM10K_FLAG_DEBUG_STATS;
-	else
-		interface->flags &= ~FM10K_FLAG_DEBUG_STATS;
-
 	return 0;
 }
 
-- 
2.7.1.429.g45cd78e


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [Intel-wired-lan] [PATCH v6 2/2] fm10k: remove remove debug-statistics support
  2016-03-04 23:37 ` [Intel-wired-lan] [PATCH v6 2/2] fm10k: remove remove debug-statistics support Jacob Keller
@ 2016-03-30 22:50   ` Singh, Krishneil K
  0 siblings, 0 replies; 6+ messages in thread
From: Singh, Krishneil K @ 2016-03-30 22:50 UTC (permalink / raw)
  To: intel-wired-lan

-----Original Message-----
From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lists.osuosl.org] On Behalf Of Jacob Keller
Sent: Friday, March 4, 2016 3:38 PM
To: Intel Wired LAN <intel-wired-lan@lists.osuosl.org>
Subject: [Intel-wired-lan] [PATCH v6 2/2] fm10k: remove remove debug-statistics support

This change fixes an (ab)use of the ethtool stats API, which could result in corrupt memory or misleading stat output. The ethtool stats API is not robust enough to handle varying number of statistics due to how it requests the size and allocates memory. Remove the poorly conceived support originally added for extra debug statistics. In the future, a new stats api may open up the ability to display these statistics.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---

Tested-by: Krishneil Singh <Krishneil.k.singh@intel.com>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [Intel-wired-lan] [PATCH v6 0/2] fix ethtool statistics patches
  2016-03-04 23:37 [Intel-wired-lan] [PATCH v6 0/2] fix ethtool statistics patches Jacob Keller
  2016-03-04 23:37 ` [Intel-wired-lan] [PATCH v6 1/2] fm10k: add helper functions to set strings and data for ethtool stats Jacob Keller
  2016-03-04 23:37 ` [Intel-wired-lan] [PATCH v6 2/2] fm10k: remove remove debug-statistics support Jacob Keller
@ 2016-03-04 23:38 ` Keller, Jacob E
  2 siblings, 0 replies; 6+ messages in thread
From: Keller, Jacob E @ 2016-03-04 23:38 UTC (permalink / raw)
  To: intel-wired-lan

On Fri, 2016-03-04 at 15:37 -0800, Jacob Keller wrote:
> This series fixes the ethtool patches on Jeff's next-queue. First, I
> fixed a bug in the fm10k ethtool stat helpers which had assumed the
> allocated memory was already zeroed. The new code assumes it is not,
> and
> always rights 0s. In addition, I fixed Bruce's comment about the
> newline. Since I had to change the first patch, I rebased them both
> against the top of Jeff's queue and merged some of the code. The
> first
> patch in this series now handles the per-queue stats, while the
> second
> patch does the work to remove the incorrect support for
> debug-statistics.
> 
> The first patch is a direct replacement for 1256b40cb7b4 ("fm10k: add
> helper functions to set strings and data for ethtool stats", 2016-03-
> 04)
> 
> The second patch is a direct replacement for 2d6c14d75442 ("fm10k:
> cleanup fm10k stats and remove debug-statistics", 2016-03-04)

This series fixes Krishniel's issue, and resolves the extra blank line.
It also is a bit easier to follow each patch, since they are more
clearly separated. Both patches listed above need to be dropped and
then this series applied in order.

Thanks,
Jake

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2016-03-30 22:51 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-03-04 23:37 [Intel-wired-lan] [PATCH v6 0/2] fix ethtool statistics patches Jacob Keller
2016-03-04 23:37 ` [Intel-wired-lan] [PATCH v6 1/2] fm10k: add helper functions to set strings and data for ethtool stats Jacob Keller
2016-03-30 22:51   ` Singh, Krishneil K
2016-03-04 23:37 ` [Intel-wired-lan] [PATCH v6 2/2] fm10k: remove remove debug-statistics support Jacob Keller
2016-03-30 22:50   ` Singh, Krishneil K
2016-03-04 23:38 ` [Intel-wired-lan] [PATCH v6 0/2] fix ethtool statistics patches Keller, Jacob E

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.