[PATCH 1/3] ethtool: Support ETHTOOL_GSTATS2 command.

[PATCH 1/3] ethtool: Support ETHTOOL_GSTATS2 command.

[greearb]

From: Ben Greear <greearb@candelatech.com>

This is similar to ETHTOOL_GSTATS, but it allows you to specify
flags.  These flags can be used by the driver to decrease the
amount of stats refreshed.  In particular, this helps with ath10k
since getting the firmware stats can be slow.

Signed-off-by: Ben Greear <greearb@candelatech.com>
---
 include/linux/ethtool.h      | 12 ++++++++++++
 include/uapi/linux/ethtool.h | 10 ++++++++++
 net/core/ethtool.c           | 40 +++++++++++++++++++++++++++++++++++-----
 3 files changed, 57 insertions(+), 5 deletions(-)

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index ebe4181..a4aa11f 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -243,6 +243,15 @@ bool ethtool_convert_link_mode_to_legacy_u32(u32 *legacy_u32,
  * @get_ethtool_stats: Return extended statistics about the device.
  *	This is only useful if the device maintains statistics not
  *	included in &struct rtnl_link_stats64.
+ * @get_ethtool_stats2: Return extended statistics about the device.
+ *	This is only useful if the device maintains statistics not
+ *	included in &struct rtnl_link_stats64.
+ *      Takes a flags argument:  0 means all (same as get_ethtool_stats),
+ *      0x1 (ETHTOOL_GS2_SKIP_FW) means skip firmware stats.
+ *      Other flags are reserved for now.
+ *      Same number of stats will be returned, but some of them might
+ *      not be as accurate/refreshed.  This is to allow not querying
+ *      firmware or other expensive-to-read stats, for instance.
  * @begin: Function to be called before any other operation.  Returns a
  *	negative error code or zero.
  * @complete: Function to be called after any other operation except
@@ -355,6 +364,9 @@ struct ethtool_ops {
 	int	(*set_phys_id)(struct net_device *, enum ethtool_phys_id_state);
 	void	(*get_ethtool_stats)(struct net_device *,
 				     struct ethtool_stats *, u64 *);
+	void	(*get_ethtool_stats2)(struct net_device *dev,
+				      struct ethtool_stats *gstats, u64 *data,
+				      u32 flags);
 	int	(*begin)(struct net_device *);
 	void	(*complete)(struct net_device *);
 	u32	(*get_priv_flags)(struct net_device *);
diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index 4ca65b5..1c74f3e 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -1396,11 +1396,21 @@ enum ethtool_fec_config_bits {
 #define ETHTOOL_PHY_STUNABLE	0x0000004f /* Set PHY tunable configuration */
 #define ETHTOOL_GFECPARAM	0x00000050 /* Get FEC settings */
 #define ETHTOOL_SFECPARAM	0x00000051 /* Set FEC settings */
+#define ETHTOOL_GSTATS2		0x00000052 /* get NIC-specific statistics
+					    * with ability to specify flags.
+					    * See ETHTOOL_GS2* below.
+					    */
 
 /* compatibility with older code */
 #define SPARC_ETH_GSET		ETHTOOL_GSET
 #define SPARC_ETH_SSET		ETHTOOL_SSET
 
+/* GSTATS2 flags */
+#define ETHTOOL_GS2_SKIP_NONE (0)    /* default is to update all stats */
+#define ETHTOOL_GS2_SKIP_FW   (1<<0) /* Skip reading stats that probe firmware,
+				      * and thus are slow/expensive.
+				      */
+
 /* Link mode bit indices */
 enum ethtool_link_mode_bit_indices {
 	ETHTOOL_LINK_MODE_10baseT_Half_BIT	= 0,
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 03416e6..6ec3413 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -1952,16 +1952,14 @@ static int ethtool_phys_id(struct net_device *dev, void __user *useraddr)
 	return rc;
 }
 
-static int ethtool_get_stats(struct net_device *dev, void __user *useraddr)
+static int _ethtool_get_stats(struct net_device *dev, void __user *useraddr,
+			      u32 flags)
 {
 	struct ethtool_stats stats;
 	const struct ethtool_ops *ops = dev->ethtool_ops;
 	u64 *data;
 	int ret, n_stats;
 
-	if (!ops->get_ethtool_stats || !ops->get_sset_count)
-		return -EOPNOTSUPP;
-
 	n_stats = ops->get_sset_count(dev, ETH_SS_STATS);
 	if (n_stats < 0)
 		return n_stats;
@@ -1976,7 +1974,10 @@ static int ethtool_get_stats(struct net_device *dev, void __user *useraddr)
 	if (n_stats && !data)
 		return -ENOMEM;
 
-	ops->get_ethtool_stats(dev, &stats, data);
+	if (flags != ETHTOOL_GS2_SKIP_NONE)
+		ops->get_ethtool_stats2(dev, &stats, data, flags);
+	else
+		ops->get_ethtool_stats(dev, &stats, data);
 
 	ret = -EFAULT;
 	if (copy_to_user(useraddr, &stats, sizeof(stats)))
@@ -1991,6 +1992,31 @@ static int ethtool_get_stats(struct net_device *dev, void __user *useraddr)
 	return ret;
 }
 
+static int ethtool_get_stats(struct net_device *dev, void __user *useraddr)
+{
+	const struct ethtool_ops *ops = dev->ethtool_ops;
+
+	if (!ops->get_ethtool_stats || !ops->get_sset_count)
+		return -EOPNOTSUPP;
+	return _ethtool_get_stats(dev, useraddr, ETHTOOL_GS2_SKIP_NONE);
+}
+
+static int ethtool_get_stats2(struct net_device *dev, void __user *useraddr)
+{
+	struct ethtool_stats stats;
+	const struct ethtool_ops *ops = dev->ethtool_ops;
+	u32 flags = 0;
+
+	if (!ops->get_ethtool_stats2 || !ops->get_sset_count)
+		return -EOPNOTSUPP;
+
+	if (copy_from_user(&stats, useraddr, sizeof(stats)))
+		return -EFAULT;
+
+	flags = stats.n_stats;
+	return _ethtool_get_stats(dev, useraddr, flags);
+}
+
 static int ethtool_get_phy_stats(struct net_device *dev, void __user *useraddr)
 {
 	struct ethtool_stats stats;
@@ -2632,6 +2658,7 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
 	case ETHTOOL_GSSET_INFO:
 	case ETHTOOL_GSTRINGS:
 	case ETHTOOL_GSTATS:
+	case ETHTOOL_GSTATS2:
 	case ETHTOOL_GPHYSTATS:
 	case ETHTOOL_GTSO:
 	case ETHTOOL_GPERMADDR:
@@ -2742,6 +2769,9 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
 	case ETHTOOL_GSTATS:
 		rc = ethtool_get_stats(dev, useraddr);
 		break;
+	case ETHTOOL_GSTATS2:
+		rc = ethtool_get_stats2(dev, useraddr);
+		break;
 	case ETHTOOL_GPERMADDR:
 		rc = ethtool_get_perm_addr(dev, useraddr);
 		break;
-- 
2.4.11

[PATCH 1/3] ethtool: Support ETHTOOL_GSTATS2 command.

[greearb]

From: Ben Greear <greearb@candelatech.com>

This is similar to ETHTOOL_GSTATS, but it allows you to specify
flags.  These flags can be used by the driver to decrease the
amount of stats refreshed.  In particular, this helps with ath10k
since getting the firmware stats can be slow.

Signed-off-by: Ben Greear <greearb@candelatech.com>
---
 include/linux/ethtool.h      | 12 ++++++++++++
 include/uapi/linux/ethtool.h | 10 ++++++++++
 net/core/ethtool.c           | 40 +++++++++++++++++++++++++++++++++++-----
 3 files changed, 57 insertions(+), 5 deletions(-)

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index ebe4181..a4aa11f 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -243,6 +243,15 @@ bool ethtool_convert_link_mode_to_legacy_u32(u32 *legacy_u32,
  * @get_ethtool_stats: Return extended statistics about the device.
  *	This is only useful if the device maintains statistics not
  *	included in &struct rtnl_link_stats64.
+ * @get_ethtool_stats2: Return extended statistics about the device.
+ *	This is only useful if the device maintains statistics not
+ *	included in &struct rtnl_link_stats64.
+ *      Takes a flags argument:  0 means all (same as get_ethtool_stats),
+ *      0x1 (ETHTOOL_GS2_SKIP_FW) means skip firmware stats.
+ *      Other flags are reserved for now.
+ *      Same number of stats will be returned, but some of them might
+ *      not be as accurate/refreshed.  This is to allow not querying
+ *      firmware or other expensive-to-read stats, for instance.
  * @begin: Function to be called before any other operation.  Returns a
  *	negative error code or zero.
  * @complete: Function to be called after any other operation except
@@ -355,6 +364,9 @@ struct ethtool_ops {
 	int	(*set_phys_id)(struct net_device *, enum ethtool_phys_id_state);
 	void	(*get_ethtool_stats)(struct net_device *,
 				     struct ethtool_stats *, u64 *);
+	void	(*get_ethtool_stats2)(struct net_device *dev,
+				      struct ethtool_stats *gstats, u64 *data,
+				      u32 flags);
 	int	(*begin)(struct net_device *);
 	void	(*complete)(struct net_device *);
 	u32	(*get_priv_flags)(struct net_device *);
diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index 4ca65b5..1c74f3e 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -1396,11 +1396,21 @@ enum ethtool_fec_config_bits {
 #define ETHTOOL_PHY_STUNABLE	0x0000004f /* Set PHY tunable configuration */
 #define ETHTOOL_GFECPARAM	0x00000050 /* Get FEC settings */
 #define ETHTOOL_SFECPARAM	0x00000051 /* Set FEC settings */
+#define ETHTOOL_GSTATS2		0x00000052 /* get NIC-specific statistics
+					    * with ability to specify flags.
+					    * See ETHTOOL_GS2* below.
+					    */
 
 /* compatibility with older code */
 #define SPARC_ETH_GSET		ETHTOOL_GSET
 #define SPARC_ETH_SSET		ETHTOOL_SSET
 
+/* GSTATS2 flags */
+#define ETHTOOL_GS2_SKIP_NONE (0)    /* default is to update all stats */
+#define ETHTOOL_GS2_SKIP_FW   (1<<0) /* Skip reading stats that probe firmware,
+				      * and thus are slow/expensive.
+				      */
+
 /* Link mode bit indices */
 enum ethtool_link_mode_bit_indices {
 	ETHTOOL_LINK_MODE_10baseT_Half_BIT	= 0,
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 03416e6..6ec3413 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -1952,16 +1952,14 @@ static int ethtool_phys_id(struct net_device *dev, void __user *useraddr)
 	return rc;
 }
 
-static int ethtool_get_stats(struct net_device *dev, void __user *useraddr)
+static int _ethtool_get_stats(struct net_device *dev, void __user *useraddr,
+			      u32 flags)
 {
 	struct ethtool_stats stats;
 	const struct ethtool_ops *ops = dev->ethtool_ops;
 	u64 *data;
 	int ret, n_stats;
 
-	if (!ops->get_ethtool_stats || !ops->get_sset_count)
-		return -EOPNOTSUPP;
-
 	n_stats = ops->get_sset_count(dev, ETH_SS_STATS);
 	if (n_stats < 0)
 		return n_stats;
@@ -1976,7 +1974,10 @@ static int ethtool_get_stats(struct net_device *dev, void __user *useraddr)
 	if (n_stats && !data)
 		return -ENOMEM;
 
-	ops->get_ethtool_stats(dev, &stats, data);
+	if (flags != ETHTOOL_GS2_SKIP_NONE)
+		ops->get_ethtool_stats2(dev, &stats, data, flags);
+	else
+		ops->get_ethtool_stats(dev, &stats, data);
 
 	ret = -EFAULT;
 	if (copy_to_user(useraddr, &stats, sizeof(stats)))
@@ -1991,6 +1992,31 @@ static int ethtool_get_stats(struct net_device *dev, void __user *useraddr)
 	return ret;
 }
 
+static int ethtool_get_stats(struct net_device *dev, void __user *useraddr)
+{
+	const struct ethtool_ops *ops = dev->ethtool_ops;
+
+	if (!ops->get_ethtool_stats || !ops->get_sset_count)
+		return -EOPNOTSUPP;
+	return _ethtool_get_stats(dev, useraddr, ETHTOOL_GS2_SKIP_NONE);
+}
+
+static int ethtool_get_stats2(struct net_device *dev, void __user *useraddr)
+{
+	struct ethtool_stats stats;
+	const struct ethtool_ops *ops = dev->ethtool_ops;
+	u32 flags = 0;
+
+	if (!ops->get_ethtool_stats2 || !ops->get_sset_count)
+		return -EOPNOTSUPP;
+
+	if (copy_from_user(&stats, useraddr, sizeof(stats)))
+		return -EFAULT;
+
+	flags = stats.n_stats;
+	return _ethtool_get_stats(dev, useraddr, flags);
+}
+
 static int ethtool_get_phy_stats(struct net_device *dev, void __user *useraddr)
 {
 	struct ethtool_stats stats;
@@ -2632,6 +2658,7 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
 	case ETHTOOL_GSSET_INFO:
 	case ETHTOOL_GSTRINGS:
 	case ETHTOOL_GSTATS:
+	case ETHTOOL_GSTATS2:
 	case ETHTOOL_GPHYSTATS:
 	case ETHTOOL_GTSO:
 	case ETHTOOL_GPERMADDR:
@@ -2742,6 +2769,9 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
 	case ETHTOOL_GSTATS:
 		rc = ethtool_get_stats(dev, useraddr);
 		break;
+	case ETHTOOL_GSTATS2:
+		rc = ethtool_get_stats2(dev, useraddr);
+		break;
 	case ETHTOOL_GPERMADDR:
 		rc = ethtool_get_perm_addr(dev, useraddr);
 		break;
-- 
2.4.11


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

[PATCH 1/3] ethtool: Support ETHTOOL_GSTATS2 command.

[greearb-my8/4N5VtI7c+919tysfdA]

From: Ben Greear <greearb-my8/4N5VtI7c+919tysfdA@public.gmane.org>

This is similar to ETHTOOL_GSTATS, but it allows you to specify
flags.  These flags can be used by the driver to decrease the
amount of stats refreshed.  In particular, this helps with ath10k
since getting the firmware stats can be slow.

Signed-off-by: Ben Greear <greearb-my8/4N5VtI7c+919tysfdA@public.gmane.org>
---
 include/linux/ethtool.h      | 12 ++++++++++++
 include/uapi/linux/ethtool.h | 10 ++++++++++
 net/core/ethtool.c           | 40 +++++++++++++++++++++++++++++++++++-----
 3 files changed, 57 insertions(+), 5 deletions(-)

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index ebe4181..a4aa11f 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -243,6 +243,15 @@ bool ethtool_convert_link_mode_to_legacy_u32(u32 *legacy_u32,
  * @get_ethtool_stats: Return extended statistics about the device.
  *	This is only useful if the device maintains statistics not
  *	included in &struct rtnl_link_stats64.
+ * @get_ethtool_stats2: Return extended statistics about the device.
+ *	This is only useful if the device maintains statistics not
+ *	included in &struct rtnl_link_stats64.
+ *      Takes a flags argument:  0 means all (same as get_ethtool_stats),
+ *      0x1 (ETHTOOL_GS2_SKIP_FW) means skip firmware stats.
+ *      Other flags are reserved for now.
+ *      Same number of stats will be returned, but some of them might
+ *      not be as accurate/refreshed.  This is to allow not querying
+ *      firmware or other expensive-to-read stats, for instance.
  * @begin: Function to be called before any other operation.  Returns a
  *	negative error code or zero.
  * @complete: Function to be called after any other operation except
@@ -355,6 +364,9 @@ struct ethtool_ops {
 	int	(*set_phys_id)(struct net_device *, enum ethtool_phys_id_state);
 	void	(*get_ethtool_stats)(struct net_device *,
 				     struct ethtool_stats *, u64 *);
+	void	(*get_ethtool_stats2)(struct net_device *dev,
+				      struct ethtool_stats *gstats, u64 *data,
+				      u32 flags);
 	int	(*begin)(struct net_device *);
 	void	(*complete)(struct net_device *);
 	u32	(*get_priv_flags)(struct net_device *);
diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index 4ca65b5..1c74f3e 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -1396,11 +1396,21 @@ enum ethtool_fec_config_bits {
 #define ETHTOOL_PHY_STUNABLE	0x0000004f /* Set PHY tunable configuration */
 #define ETHTOOL_GFECPARAM	0x00000050 /* Get FEC settings */
 #define ETHTOOL_SFECPARAM	0x00000051 /* Set FEC settings */
+#define ETHTOOL_GSTATS2		0x00000052 /* get NIC-specific statistics
+					    * with ability to specify flags.
+					    * See ETHTOOL_GS2* below.
+					    */
 
 /* compatibility with older code */
 #define SPARC_ETH_GSET		ETHTOOL_GSET
 #define SPARC_ETH_SSET		ETHTOOL_SSET
 
+/* GSTATS2 flags */
+#define ETHTOOL_GS2_SKIP_NONE (0)    /* default is to update all stats */
+#define ETHTOOL_GS2_SKIP_FW   (1<<0) /* Skip reading stats that probe firmware,
+				      * and thus are slow/expensive.
+				      */
+
 /* Link mode bit indices */
 enum ethtool_link_mode_bit_indices {
 	ETHTOOL_LINK_MODE_10baseT_Half_BIT	= 0,
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 03416e6..6ec3413 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -1952,16 +1952,14 @@ static int ethtool_phys_id(struct net_device *dev, void __user *useraddr)
 	return rc;
 }
 
-static int ethtool_get_stats(struct net_device *dev, void __user *useraddr)
+static int _ethtool_get_stats(struct net_device *dev, void __user *useraddr,
+			      u32 flags)
 {
 	struct ethtool_stats stats;
 	const struct ethtool_ops *ops = dev->ethtool_ops;
 	u64 *data;
 	int ret, n_stats;
 
-	if (!ops->get_ethtool_stats || !ops->get_sset_count)
-		return -EOPNOTSUPP;
-
 	n_stats = ops->get_sset_count(dev, ETH_SS_STATS);
 	if (n_stats < 0)
 		return n_stats;
@@ -1976,7 +1974,10 @@ static int ethtool_get_stats(struct net_device *dev, void __user *useraddr)
 	if (n_stats && !data)
 		return -ENOMEM;
 
-	ops->get_ethtool_stats(dev, &stats, data);
+	if (flags != ETHTOOL_GS2_SKIP_NONE)
+		ops->get_ethtool_stats2(dev, &stats, data, flags);
+	else
+		ops->get_ethtool_stats(dev, &stats, data);
 
 	ret = -EFAULT;
 	if (copy_to_user(useraddr, &stats, sizeof(stats)))
@@ -1991,6 +1992,31 @@ static int ethtool_get_stats(struct net_device *dev, void __user *useraddr)
 	return ret;
 }
 
+static int ethtool_get_stats(struct net_device *dev, void __user *useraddr)
+{
+	const struct ethtool_ops *ops = dev->ethtool_ops;
+
+	if (!ops->get_ethtool_stats || !ops->get_sset_count)
+		return -EOPNOTSUPP;
+	return _ethtool_get_stats(dev, useraddr, ETHTOOL_GS2_SKIP_NONE);
+}
+
+static int ethtool_get_stats2(struct net_device *dev, void __user *useraddr)
+{
+	struct ethtool_stats stats;
+	const struct ethtool_ops *ops = dev->ethtool_ops;
+	u32 flags = 0;
+
+	if (!ops->get_ethtool_stats2 || !ops->get_sset_count)
+		return -EOPNOTSUPP;
+
+	if (copy_from_user(&stats, useraddr, sizeof(stats)))
+		return -EFAULT;
+
+	flags = stats.n_stats;
+	return _ethtool_get_stats(dev, useraddr, flags);
+}
+
 static int ethtool_get_phy_stats(struct net_device *dev, void __user *useraddr)
 {
 	struct ethtool_stats stats;
@@ -2632,6 +2658,7 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
 	case ETHTOOL_GSSET_INFO:
 	case ETHTOOL_GSTRINGS:
 	case ETHTOOL_GSTATS:
+	case ETHTOOL_GSTATS2:
 	case ETHTOOL_GPHYSTATS:
 	case ETHTOOL_GTSO:
 	case ETHTOOL_GPERMADDR:
@@ -2742,6 +2769,9 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
 	case ETHTOOL_GSTATS:
 		rc = ethtool_get_stats(dev, useraddr);
 		break;
+	case ETHTOOL_GSTATS2:
+		rc = ethtool_get_stats2(dev, useraddr);
+		break;
 	case ETHTOOL_GPERMADDR:
 		rc = ethtool_get_perm_addr(dev, useraddr);
 		break;
-- 
2.4.11

[PATCH 2/3] mac80211: Add support for ethtool gstats2 API.

[greearb]

From: Ben Greear <greearb@candelatech.com>

This enables users to request fewer stats to be refreshed
in cases where firmware does not need to be probed.

Signed-off-by: Ben Greear <greearb@candelatech.com>
---
 include/net/mac80211.h    |  6 ++++++
 net/mac80211/driver-ops.h |  9 +++++++--
 net/mac80211/ethtool.c    | 18 +++++++++++++-----
 3 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index d2279b2..4854f33 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -3361,6 +3361,8 @@ enum ieee80211_reconfig_type {
  *
  * @get_et_stats:  Ethtool API to get a set of u64 stats.
  *
+ * @get_et_stats2:  Ethtool API to get a set of u64 stats, with flags.
+ *
  * @get_et_strings:  Ethtool API to get a set of strings to describe stats
  *	and perhaps other supported types of ethtool data-sets.
  *
@@ -3692,6 +3694,10 @@ struct ieee80211_ops {
 	void	(*get_et_stats)(struct ieee80211_hw *hw,
 				struct ieee80211_vif *vif,
 				struct ethtool_stats *stats, u64 *data);
+	void	(*get_et_stats2)(struct ieee80211_hw *hw,
+				 struct ieee80211_vif *vif,
+				 struct ethtool_stats *stats, u64 *data,
+				 u32 flags);
 	void	(*get_et_strings)(struct ieee80211_hw *hw,
 				  struct ieee80211_vif *vif,
 				  u32 sset, u8 *data);
diff --git a/net/mac80211/driver-ops.h b/net/mac80211/driver-ops.h
index 4d82fe7..519d2db 100644
--- a/net/mac80211/driver-ops.h
+++ b/net/mac80211/driver-ops.h
@@ -58,10 +58,15 @@ static inline void drv_get_et_strings(struct ieee80211_sub_if_data *sdata,
 
 static inline void drv_get_et_stats(struct ieee80211_sub_if_data *sdata,
 				    struct ethtool_stats *stats,
-				    u64 *data)
+				    u64 *data, u32 flags)
 {
 	struct ieee80211_local *local = sdata->local;
-	if (local->ops->get_et_stats) {
+	if (local->ops->get_et_stats2) {
+		trace_drv_get_et_stats(local);
+		local->ops->get_et_stats2(&local->hw, &sdata->vif, stats, data,
+					  flags);
+		trace_drv_return_void(local);
+	} else if (local->ops->get_et_stats) {
 		trace_drv_get_et_stats(local);
 		local->ops->get_et_stats(&local->hw, &sdata->vif, stats, data);
 		trace_drv_return_void(local);
diff --git a/net/mac80211/ethtool.c b/net/mac80211/ethtool.c
index 9cc986d..b67520e 100644
--- a/net/mac80211/ethtool.c
+++ b/net/mac80211/ethtool.c
@@ -61,9 +61,9 @@ static int ieee80211_get_sset_count(struct net_device *dev, int sset)
 	return rv;
 }
 
-static void ieee80211_get_stats(struct net_device *dev,
-				struct ethtool_stats *stats,
-				u64 *data)
+static void ieee80211_get_stats2(struct net_device *dev,
+				 struct ethtool_stats *stats,
+				 u64 *data, u32 flags)
 {
 	struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
 	struct ieee80211_chanctx_conf *chanctx_conf;
@@ -199,7 +199,14 @@ static void ieee80211_get_stats(struct net_device *dev,
 	if (WARN_ON(i != STA_STATS_LEN))
 		return;
 
-	drv_get_et_stats(sdata, stats, &(data[STA_STATS_LEN]));
+	drv_get_et_stats(sdata, stats, &data[STA_STATS_LEN], flags);
+}
+
+static void ieee80211_get_stats(struct net_device *dev,
+				struct ethtool_stats *stats,
+				u64 *data)
+{
+	ieee80211_get_stats2(dev, stats, data, 0);
 }
 
 static void ieee80211_get_strings(struct net_device *dev, u32 sset, u8 *data)
@@ -211,7 +218,7 @@ static void ieee80211_get_strings(struct net_device *dev, u32 sset, u8 *data)
 		sz_sta_stats = sizeof(ieee80211_gstrings_sta_stats);
 		memcpy(data, ieee80211_gstrings_sta_stats, sz_sta_stats);
 	}
-	drv_get_et_strings(sdata, sset, &(data[sz_sta_stats]));
+	drv_get_et_strings(sdata, sset, &data[sz_sta_stats]);
 }
 
 static int ieee80211_get_regs_len(struct net_device *dev)
@@ -238,5 +245,6 @@ const struct ethtool_ops ieee80211_ethtool_ops = {
 	.set_ringparam = ieee80211_set_ringparam,
 	.get_strings = ieee80211_get_strings,
 	.get_ethtool_stats = ieee80211_get_stats,
+	.get_ethtool_stats2 = ieee80211_get_stats2,
 	.get_sset_count = ieee80211_get_sset_count,
 };
-- 
2.4.11

[PATCH 2/3] mac80211: Add support for ethtool gstats2 API.

[greearb]

From: Ben Greear <greearb@candelatech.com>

This enables users to request fewer stats to be refreshed
in cases where firmware does not need to be probed.

Signed-off-by: Ben Greear <greearb@candelatech.com>
---
 include/net/mac80211.h    |  6 ++++++
 net/mac80211/driver-ops.h |  9 +++++++--
 net/mac80211/ethtool.c    | 18 +++++++++++++-----
 3 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index d2279b2..4854f33 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -3361,6 +3361,8 @@ enum ieee80211_reconfig_type {
  *
  * @get_et_stats:  Ethtool API to get a set of u64 stats.
  *
+ * @get_et_stats2:  Ethtool API to get a set of u64 stats, with flags.
+ *
  * @get_et_strings:  Ethtool API to get a set of strings to describe stats
  *	and perhaps other supported types of ethtool data-sets.
  *
@@ -3692,6 +3694,10 @@ struct ieee80211_ops {
 	void	(*get_et_stats)(struct ieee80211_hw *hw,
 				struct ieee80211_vif *vif,
 				struct ethtool_stats *stats, u64 *data);
+	void	(*get_et_stats2)(struct ieee80211_hw *hw,
+				 struct ieee80211_vif *vif,
+				 struct ethtool_stats *stats, u64 *data,
+				 u32 flags);
 	void	(*get_et_strings)(struct ieee80211_hw *hw,
 				  struct ieee80211_vif *vif,
 				  u32 sset, u8 *data);
diff --git a/net/mac80211/driver-ops.h b/net/mac80211/driver-ops.h
index 4d82fe7..519d2db 100644
--- a/net/mac80211/driver-ops.h
+++ b/net/mac80211/driver-ops.h
@@ -58,10 +58,15 @@ static inline void drv_get_et_strings(struct ieee80211_sub_if_data *sdata,
 
 static inline void drv_get_et_stats(struct ieee80211_sub_if_data *sdata,
 				    struct ethtool_stats *stats,
-				    u64 *data)
+				    u64 *data, u32 flags)
 {
 	struct ieee80211_local *local = sdata->local;
-	if (local->ops->get_et_stats) {
+	if (local->ops->get_et_stats2) {
+		trace_drv_get_et_stats(local);
+		local->ops->get_et_stats2(&local->hw, &sdata->vif, stats, data,
+					  flags);
+		trace_drv_return_void(local);
+	} else if (local->ops->get_et_stats) {
 		trace_drv_get_et_stats(local);
 		local->ops->get_et_stats(&local->hw, &sdata->vif, stats, data);
 		trace_drv_return_void(local);
diff --git a/net/mac80211/ethtool.c b/net/mac80211/ethtool.c
index 9cc986d..b67520e 100644
--- a/net/mac80211/ethtool.c
+++ b/net/mac80211/ethtool.c
@@ -61,9 +61,9 @@ static int ieee80211_get_sset_count(struct net_device *dev, int sset)
 	return rv;
 }
 
-static void ieee80211_get_stats(struct net_device *dev,
-				struct ethtool_stats *stats,
-				u64 *data)
+static void ieee80211_get_stats2(struct net_device *dev,
+				 struct ethtool_stats *stats,
+				 u64 *data, u32 flags)
 {
 	struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
 	struct ieee80211_chanctx_conf *chanctx_conf;
@@ -199,7 +199,14 @@ static void ieee80211_get_stats(struct net_device *dev,
 	if (WARN_ON(i != STA_STATS_LEN))
 		return;
 
-	drv_get_et_stats(sdata, stats, &(data[STA_STATS_LEN]));
+	drv_get_et_stats(sdata, stats, &data[STA_STATS_LEN], flags);
+}
+
+static void ieee80211_get_stats(struct net_device *dev,
+				struct ethtool_stats *stats,
+				u64 *data)
+{
+	ieee80211_get_stats2(dev, stats, data, 0);
 }
 
 static void ieee80211_get_strings(struct net_device *dev, u32 sset, u8 *data)
@@ -211,7 +218,7 @@ static void ieee80211_get_strings(struct net_device *dev, u32 sset, u8 *data)
 		sz_sta_stats = sizeof(ieee80211_gstrings_sta_stats);
 		memcpy(data, ieee80211_gstrings_sta_stats, sz_sta_stats);
 	}
-	drv_get_et_strings(sdata, sset, &(data[sz_sta_stats]));
+	drv_get_et_strings(sdata, sset, &data[sz_sta_stats]);
 }
 
 static int ieee80211_get_regs_len(struct net_device *dev)
@@ -238,5 +245,6 @@ const struct ethtool_ops ieee80211_ethtool_ops = {
 	.set_ringparam = ieee80211_set_ringparam,
 	.get_strings = ieee80211_get_strings,
 	.get_ethtool_stats = ieee80211_get_stats,
+	.get_ethtool_stats2 = ieee80211_get_stats2,
 	.get_sset_count = ieee80211_get_sset_count,
 };
-- 
2.4.11


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

[PATCH 3/3] ath10k: Support ethtool gstats2 API.

[greearb]

From: Ben Greear <greearb@candelatech.com>

Skip a firmware stats update when calling
code indicates the stats refresh is not needed.

Signed-off-by: Ben Greear <greearb@candelatech.com>
---
 drivers/net/wireless/ath/ath10k/debug.c | 18 +++++++++++++++---
 drivers/net/wireless/ath/ath10k/debug.h |  4 ++++
 drivers/net/wireless/ath/ath10k/mac.c   |  1 +
 3 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c
index bac832c..d559a3f 100644
--- a/drivers/net/wireless/ath/ath10k/debug.c
+++ b/drivers/net/wireless/ath/ath10k/debug.c
@@ -1159,9 +1159,10 @@ int ath10k_debug_get_et_sset_count(struct ieee80211_hw *hw,
 	return 0;
 }
 
-void ath10k_debug_get_et_stats(struct ieee80211_hw *hw,
-			       struct ieee80211_vif *vif,
-			       struct ethtool_stats *stats, u64 *data)
+void ath10k_debug_get_et_stats2(struct ieee80211_hw *hw,
+				struct ieee80211_vif *vif,
+				struct ethtool_stats *stats, u64 *data,
+				u32 flags)
 {
 	struct ath10k *ar = hw->priv;
 	static const struct ath10k_fw_stats_pdev zero_stats = {};
@@ -1170,6 +1171,9 @@ void ath10k_debug_get_et_stats(struct ieee80211_hw *hw,
 
 	mutex_lock(&ar->conf_mutex);
 
+	if (flags & ETHTOOL_GS2_SKIP_FW)
+		goto skip_query_fw_stats;
+
 	if (ar->state == ATH10K_STATE_ON) {
 		ret = ath10k_debug_fw_stats_request(ar);
 		if (ret) {
@@ -1180,6 +1184,7 @@ void ath10k_debug_get_et_stats(struct ieee80211_hw *hw,
 		}
 	}
 
+skip_query_fw_stats:
 	pdev_stats = list_first_entry_or_null(&ar->debug.fw_stats.pdevs,
 					      struct ath10k_fw_stats_pdev,
 					      list);
@@ -1244,6 +1249,13 @@ void ath10k_debug_get_et_stats(struct ieee80211_hw *hw,
 	WARN_ON(i != ATH10K_SSTATS_LEN);
 }
 
+void ath10k_debug_get_et_stats(struct ieee80211_hw *hw,
+			       struct ieee80211_vif *vif,
+			       struct ethtool_stats *stats, u64 *data)
+{
+	ath10k_debug_get_et_stats2(hw, vif, stats, data, 0);
+}
+
 static const struct file_operations fops_fw_dbglog = {
 	.read = ath10k_read_fw_dbglog,
 	.write = ath10k_write_fw_dbglog,
diff --git a/drivers/net/wireless/ath/ath10k/debug.h b/drivers/net/wireless/ath/ath10k/debug.h
index 0afca5c..595d964 100644
--- a/drivers/net/wireless/ath/ath10k/debug.h
+++ b/drivers/net/wireless/ath/ath10k/debug.h
@@ -117,6 +117,10 @@ int ath10k_debug_get_et_sset_count(struct ieee80211_hw *hw,
 void ath10k_debug_get_et_stats(struct ieee80211_hw *hw,
 			       struct ieee80211_vif *vif,
 			       struct ethtool_stats *stats, u64 *data);
+void ath10k_debug_get_et_stats2(struct ieee80211_hw *hw,
+				struct ieee80211_vif *vif,
+				struct ethtool_stats *stats, u64 *data,
+				u32 level);
 
 static inline u64 ath10k_debug_get_fw_dbglog_mask(struct ath10k *ar)
 {
diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index bf05a36..27b793c 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -7734,6 +7734,7 @@ static const struct ieee80211_ops ath10k_ops = {
 	.ampdu_action			= ath10k_ampdu_action,
 	.get_et_sset_count		= ath10k_debug_get_et_sset_count,
 	.get_et_stats			= ath10k_debug_get_et_stats,
+	.get_et_stats2			= ath10k_debug_get_et_stats2,
 	.get_et_strings			= ath10k_debug_get_et_strings,
 	.add_chanctx			= ath10k_mac_op_add_chanctx,
 	.remove_chanctx			= ath10k_mac_op_remove_chanctx,
-- 
2.4.11

[PATCH 3/3] ath10k: Support ethtool gstats2 API.

[greearb]

From: Ben Greear <greearb@candelatech.com>

Skip a firmware stats update when calling
code indicates the stats refresh is not needed.

Signed-off-by: Ben Greear <greearb@candelatech.com>
---
 drivers/net/wireless/ath/ath10k/debug.c | 18 +++++++++++++++---
 drivers/net/wireless/ath/ath10k/debug.h |  4 ++++
 drivers/net/wireless/ath/ath10k/mac.c   |  1 +
 3 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c
index bac832c..d559a3f 100644
--- a/drivers/net/wireless/ath/ath10k/debug.c
+++ b/drivers/net/wireless/ath/ath10k/debug.c
@@ -1159,9 +1159,10 @@ int ath10k_debug_get_et_sset_count(struct ieee80211_hw *hw,
 	return 0;
 }
 
-void ath10k_debug_get_et_stats(struct ieee80211_hw *hw,
-			       struct ieee80211_vif *vif,
-			       struct ethtool_stats *stats, u64 *data)
+void ath10k_debug_get_et_stats2(struct ieee80211_hw *hw,
+				struct ieee80211_vif *vif,
+				struct ethtool_stats *stats, u64 *data,
+				u32 flags)
 {
 	struct ath10k *ar = hw->priv;
 	static const struct ath10k_fw_stats_pdev zero_stats = {};
@@ -1170,6 +1171,9 @@ void ath10k_debug_get_et_stats(struct ieee80211_hw *hw,
 
 	mutex_lock(&ar->conf_mutex);
 
+	if (flags & ETHTOOL_GS2_SKIP_FW)
+		goto skip_query_fw_stats;
+
 	if (ar->state == ATH10K_STATE_ON) {
 		ret = ath10k_debug_fw_stats_request(ar);
 		if (ret) {
@@ -1180,6 +1184,7 @@ void ath10k_debug_get_et_stats(struct ieee80211_hw *hw,
 		}
 	}
 
+skip_query_fw_stats:
 	pdev_stats = list_first_entry_or_null(&ar->debug.fw_stats.pdevs,
 					      struct ath10k_fw_stats_pdev,
 					      list);
@@ -1244,6 +1249,13 @@ void ath10k_debug_get_et_stats(struct ieee80211_hw *hw,
 	WARN_ON(i != ATH10K_SSTATS_LEN);
 }
 
+void ath10k_debug_get_et_stats(struct ieee80211_hw *hw,
+			       struct ieee80211_vif *vif,
+			       struct ethtool_stats *stats, u64 *data)
+{
+	ath10k_debug_get_et_stats2(hw, vif, stats, data, 0);
+}
+
 static const struct file_operations fops_fw_dbglog = {
 	.read = ath10k_read_fw_dbglog,
 	.write = ath10k_write_fw_dbglog,
diff --git a/drivers/net/wireless/ath/ath10k/debug.h b/drivers/net/wireless/ath/ath10k/debug.h
index 0afca5c..595d964 100644
--- a/drivers/net/wireless/ath/ath10k/debug.h
+++ b/drivers/net/wireless/ath/ath10k/debug.h
@@ -117,6 +117,10 @@ int ath10k_debug_get_et_sset_count(struct ieee80211_hw *hw,
 void ath10k_debug_get_et_stats(struct ieee80211_hw *hw,
 			       struct ieee80211_vif *vif,
 			       struct ethtool_stats *stats, u64 *data);
+void ath10k_debug_get_et_stats2(struct ieee80211_hw *hw,
+				struct ieee80211_vif *vif,
+				struct ethtool_stats *stats, u64 *data,
+				u32 level);
 
 static inline u64 ath10k_debug_get_fw_dbglog_mask(struct ath10k *ar)
 {
diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index bf05a36..27b793c 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -7734,6 +7734,7 @@ static const struct ieee80211_ops ath10k_ops = {
 	.ampdu_action			= ath10k_ampdu_action,
 	.get_et_sset_count		= ath10k_debug_get_et_sset_count,
 	.get_et_stats			= ath10k_debug_get_et_stats,
+	.get_et_stats2			= ath10k_debug_get_et_stats2,
 	.get_et_strings			= ath10k_debug_get_et_strings,
 	.add_chanctx			= ath10k_mac_op_add_chanctx,
 	.remove_chanctx			= ath10k_mac_op_remove_chanctx,
-- 
2.4.11


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH 3/3] ath10k: Support ethtool gstats2 API.

[kbuild test robot]

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

Hi Ben,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on mac80211/master]
[also build test ERROR on v4.17-rc1 next-20180419]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/greearb-candelatech-com/ethtool-Support-ETHTOOL_GSTATS2-command/20180419-105301
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211.git master
config: x86_64-randconfig-ne0-04191514 (attached as .config)
compiler: gcc-6 (Debian 6.4.0-9) 6.4.0 20171026
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

>> drivers/net/wireless/ath/ath10k/mac.c:7706:21: error: 'ath10k_debug_get_et_stats2' undeclared here (not in a function)
     .get_et_stats2   = ath10k_debug_get_et_stats2,
                        ^~~~~~~~~~~~~~~~~~~~~~~~~~

vim +/ath10k_debug_get_et_stats2 +7706 drivers/net/wireless/ath/ath10k/mac.c

  7672	
  7673	static const struct ieee80211_ops ath10k_ops = {
  7674		.tx				= ath10k_mac_op_tx,
  7675		.wake_tx_queue			= ath10k_mac_op_wake_tx_queue,
  7676		.start				= ath10k_start,
  7677		.stop				= ath10k_stop,
  7678		.config				= ath10k_config,
  7679		.add_interface			= ath10k_add_interface,
  7680		.remove_interface		= ath10k_remove_interface,
  7681		.configure_filter		= ath10k_configure_filter,
  7682		.bss_info_changed		= ath10k_bss_info_changed,
  7683		.set_coverage_class		= ath10k_mac_op_set_coverage_class,
  7684		.hw_scan			= ath10k_hw_scan,
  7685		.cancel_hw_scan			= ath10k_cancel_hw_scan,
  7686		.set_key			= ath10k_set_key,
  7687		.set_default_unicast_key        = ath10k_set_default_unicast_key,
  7688		.sta_state			= ath10k_sta_state,
  7689		.conf_tx			= ath10k_conf_tx,
  7690		.remain_on_channel		= ath10k_remain_on_channel,
  7691		.cancel_remain_on_channel	= ath10k_cancel_remain_on_channel,
  7692		.set_rts_threshold		= ath10k_set_rts_threshold,
  7693		.set_frag_threshold		= ath10k_mac_op_set_frag_threshold,
  7694		.flush				= ath10k_flush,
  7695		.tx_last_beacon			= ath10k_tx_last_beacon,
  7696		.set_antenna			= ath10k_set_antenna,
  7697		.get_antenna			= ath10k_get_antenna,
  7698		.reconfig_complete		= ath10k_reconfig_complete,
  7699		.get_survey			= ath10k_get_survey,
  7700		.set_bitrate_mask		= ath10k_mac_op_set_bitrate_mask,
  7701		.sta_rc_update			= ath10k_sta_rc_update,
  7702		.offset_tsf			= ath10k_offset_tsf,
  7703		.ampdu_action			= ath10k_ampdu_action,
  7704		.get_et_sset_count		= ath10k_debug_get_et_sset_count,
  7705		.get_et_stats			= ath10k_debug_get_et_stats,
> 7706		.get_et_stats2			= ath10k_debug_get_et_stats2,
  7707		.get_et_strings			= ath10k_debug_get_et_strings,
  7708		.add_chanctx			= ath10k_mac_op_add_chanctx,
  7709		.remove_chanctx			= ath10k_mac_op_remove_chanctx,
  7710		.change_chanctx			= ath10k_mac_op_change_chanctx,
  7711		.assign_vif_chanctx		= ath10k_mac_op_assign_vif_chanctx,
  7712		.unassign_vif_chanctx		= ath10k_mac_op_unassign_vif_chanctx,
  7713		.switch_vif_chanctx		= ath10k_mac_op_switch_vif_chanctx,
  7714		.sta_pre_rcu_remove		= ath10k_mac_op_sta_pre_rcu_remove,
  7715		.sta_statistics			= ath10k_sta_statistics,
  7716	
  7717		CFG80211_TESTMODE_CMD(ath10k_tm_cmd)
  7718	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 30420 bytes --]

Re: [PATCH 3/3] ath10k: Support ethtool gstats2 API.

[kbuild test robot]

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

Hi Ben,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on mac80211/master]
[also build test ERROR on v4.17-rc1 next-20180419]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/greearb-candelatech-com/ethtool-Support-ETHTOOL_GSTATS2-command/20180419-105301
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211.git master
config: x86_64-randconfig-ne0-04191514 (attached as .config)
compiler: gcc-6 (Debian 6.4.0-9) 6.4.0 20171026
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

>> drivers/net/wireless/ath/ath10k/mac.c:7706:21: error: 'ath10k_debug_get_et_stats2' undeclared here (not in a function)
     .get_et_stats2   = ath10k_debug_get_et_stats2,
                        ^~~~~~~~~~~~~~~~~~~~~~~~~~

vim +/ath10k_debug_get_et_stats2 +7706 drivers/net/wireless/ath/ath10k/mac.c

  7672	
  7673	static const struct ieee80211_ops ath10k_ops = {
  7674		.tx				= ath10k_mac_op_tx,
  7675		.wake_tx_queue			= ath10k_mac_op_wake_tx_queue,
  7676		.start				= ath10k_start,
  7677		.stop				= ath10k_stop,
  7678		.config				= ath10k_config,
  7679		.add_interface			= ath10k_add_interface,
  7680		.remove_interface		= ath10k_remove_interface,
  7681		.configure_filter		= ath10k_configure_filter,
  7682		.bss_info_changed		= ath10k_bss_info_changed,
  7683		.set_coverage_class		= ath10k_mac_op_set_coverage_class,
  7684		.hw_scan			= ath10k_hw_scan,
  7685		.cancel_hw_scan			= ath10k_cancel_hw_scan,
  7686		.set_key			= ath10k_set_key,
  7687		.set_default_unicast_key        = ath10k_set_default_unicast_key,
  7688		.sta_state			= ath10k_sta_state,
  7689		.conf_tx			= ath10k_conf_tx,
  7690		.remain_on_channel		= ath10k_remain_on_channel,
  7691		.cancel_remain_on_channel	= ath10k_cancel_remain_on_channel,
  7692		.set_rts_threshold		= ath10k_set_rts_threshold,
  7693		.set_frag_threshold		= ath10k_mac_op_set_frag_threshold,
  7694		.flush				= ath10k_flush,
  7695		.tx_last_beacon			= ath10k_tx_last_beacon,
  7696		.set_antenna			= ath10k_set_antenna,
  7697		.get_antenna			= ath10k_get_antenna,
  7698		.reconfig_complete		= ath10k_reconfig_complete,
  7699		.get_survey			= ath10k_get_survey,
  7700		.set_bitrate_mask		= ath10k_mac_op_set_bitrate_mask,
  7701		.sta_rc_update			= ath10k_sta_rc_update,
  7702		.offset_tsf			= ath10k_offset_tsf,
  7703		.ampdu_action			= ath10k_ampdu_action,
  7704		.get_et_sset_count		= ath10k_debug_get_et_sset_count,
  7705		.get_et_stats			= ath10k_debug_get_et_stats,
> 7706		.get_et_stats2			= ath10k_debug_get_et_stats2,
  7707		.get_et_strings			= ath10k_debug_get_et_strings,
  7708		.add_chanctx			= ath10k_mac_op_add_chanctx,
  7709		.remove_chanctx			= ath10k_mac_op_remove_chanctx,
  7710		.change_chanctx			= ath10k_mac_op_change_chanctx,
  7711		.assign_vif_chanctx		= ath10k_mac_op_assign_vif_chanctx,
  7712		.unassign_vif_chanctx		= ath10k_mac_op_unassign_vif_chanctx,
  7713		.switch_vif_chanctx		= ath10k_mac_op_switch_vif_chanctx,
  7714		.sta_pre_rcu_remove		= ath10k_mac_op_sta_pre_rcu_remove,
  7715		.sta_statistics			= ath10k_sta_statistics,
  7716	
  7717		CFG80211_TESTMODE_CMD(ath10k_tm_cmd)
  7718	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 30420 bytes --]

[-- Attachment #3: Type: text/plain, Size: 146 bytes --]

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH 1/3] ethtool: Support ETHTOOL_GSTATS2 command.

[Florian Fainelli]

On 04/17/2018 06:49 PM, greearb@candelatech.com wrote:
> From: Ben Greear <greearb@candelatech.com>
> 
> This is similar to ETHTOOL_GSTATS, but it allows you to specify
> flags.  These flags can be used by the driver to decrease the
> amount of stats refreshed.  In particular, this helps with ath10k
> since getting the firmware stats can be slow.

You can configure the statistics refresh rate through the ethtool
coalescing parameter stats_block_coalesce_usecs, not sure if this is
exactly what you had in mind, but if it works, then you might as well
want to use it.

> 
> Signed-off-by: Ben Greear <greearb@candelatech.com>
> ---
>  include/linux/ethtool.h      | 12 ++++++++++++
>  include/uapi/linux/ethtool.h | 10 ++++++++++
>  net/core/ethtool.c           | 40 +++++++++++++++++++++++++++++++++++-----
>  3 files changed, 57 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index ebe4181..a4aa11f 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -243,6 +243,15 @@ bool ethtool_convert_link_mode_to_legacy_u32(u32 *legacy_u32,
>   * @get_ethtool_stats: Return extended statistics about the device.
>   *	This is only useful if the device maintains statistics not
>   *	included in &struct rtnl_link_stats64.
> + * @get_ethtool_stats2: Return extended statistics about the device.
> + *	This is only useful if the device maintains statistics not
> + *	included in &struct rtnl_link_stats64.
> + *      Takes a flags argument:  0 means all (same as get_ethtool_stats),
> + *      0x1 (ETHTOOL_GS2_SKIP_FW) means skip firmware stats.
> + *      Other flags are reserved for now.
> + *      Same number of stats will be returned, but some of them might
> + *      not be as accurate/refreshed.  This is to allow not querying
> + *      firmware or other expensive-to-read stats, for instance.
>   * @begin: Function to be called before any other operation.  Returns a
>   *	negative error code or zero.
>   * @complete: Function to be called after any other operation except
> @@ -355,6 +364,9 @@ struct ethtool_ops {
>  	int	(*set_phys_id)(struct net_device *, enum ethtool_phys_id_state);
>  	void	(*get_ethtool_stats)(struct net_device *,
>  				     struct ethtool_stats *, u64 *);
> +	void	(*get_ethtool_stats2)(struct net_device *dev,
> +				      struct ethtool_stats *gstats, u64 *data,
> +				      u32 flags);
>  	int	(*begin)(struct net_device *);
>  	void	(*complete)(struct net_device *);
>  	u32	(*get_priv_flags)(struct net_device *);
> diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
> index 4ca65b5..1c74f3e 100644
> --- a/include/uapi/linux/ethtool.h
> +++ b/include/uapi/linux/ethtool.h
> @@ -1396,11 +1396,21 @@ enum ethtool_fec_config_bits {
>  #define ETHTOOL_PHY_STUNABLE	0x0000004f /* Set PHY tunable configuration */
>  #define ETHTOOL_GFECPARAM	0x00000050 /* Get FEC settings */
>  #define ETHTOOL_SFECPARAM	0x00000051 /* Set FEC settings */
> +#define ETHTOOL_GSTATS2		0x00000052 /* get NIC-specific statistics
> +					    * with ability to specify flags.
> +					    * See ETHTOOL_GS2* below.
> +					    */
>  
>  /* compatibility with older code */
>  #define SPARC_ETH_GSET		ETHTOOL_GSET
>  #define SPARC_ETH_SSET		ETHTOOL_SSET
>  
> +/* GSTATS2 flags */
> +#define ETHTOOL_GS2_SKIP_NONE (0)    /* default is to update all stats */
> +#define ETHTOOL_GS2_SKIP_FW   (1<<0) /* Skip reading stats that probe firmware,
> +				      * and thus are slow/expensive.
> +				      */
> +
>  /* Link mode bit indices */
>  enum ethtool_link_mode_bit_indices {
>  	ETHTOOL_LINK_MODE_10baseT_Half_BIT	= 0,
> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> index 03416e6..6ec3413 100644
> --- a/net/core/ethtool.c
> +++ b/net/core/ethtool.c
> @@ -1952,16 +1952,14 @@ static int ethtool_phys_id(struct net_device *dev, void __user *useraddr)
>  	return rc;
>  }
>  
> -static int ethtool_get_stats(struct net_device *dev, void __user *useraddr)
> +static int _ethtool_get_stats(struct net_device *dev, void __user *useraddr,
> +			      u32 flags)
>  {
>  	struct ethtool_stats stats;
>  	const struct ethtool_ops *ops = dev->ethtool_ops;
>  	u64 *data;
>  	int ret, n_stats;
>  
> -	if (!ops->get_ethtool_stats || !ops->get_sset_count)
> -		return -EOPNOTSUPP;
> -
>  	n_stats = ops->get_sset_count(dev, ETH_SS_STATS);
>  	if (n_stats < 0)
>  		return n_stats;
> @@ -1976,7 +1974,10 @@ static int ethtool_get_stats(struct net_device *dev, void __user *useraddr)
>  	if (n_stats && !data)
>  		return -ENOMEM;
>  
> -	ops->get_ethtool_stats(dev, &stats, data);
> +	if (flags != ETHTOOL_GS2_SKIP_NONE)
> +		ops->get_ethtool_stats2(dev, &stats, data, flags);
> +	else
> +		ops->get_ethtool_stats(dev, &stats, data);
>  
>  	ret = -EFAULT;
>  	if (copy_to_user(useraddr, &stats, sizeof(stats)))
> @@ -1991,6 +1992,31 @@ static int ethtool_get_stats(struct net_device *dev, void __user *useraddr)
>  	return ret;
>  }
>  
> +static int ethtool_get_stats(struct net_device *dev, void __user *useraddr)
> +{
> +	const struct ethtool_ops *ops = dev->ethtool_ops;
> +
> +	if (!ops->get_ethtool_stats || !ops->get_sset_count)
> +		return -EOPNOTSUPP;
> +	return _ethtool_get_stats(dev, useraddr, ETHTOOL_GS2_SKIP_NONE);
> +}
> +
> +static int ethtool_get_stats2(struct net_device *dev, void __user *useraddr)
> +{
> +	struct ethtool_stats stats;
> +	const struct ethtool_ops *ops = dev->ethtool_ops;
> +	u32 flags = 0;
> +
> +	if (!ops->get_ethtool_stats2 || !ops->get_sset_count)
> +		return -EOPNOTSUPP;
> +
> +	if (copy_from_user(&stats, useraddr, sizeof(stats)))
> +		return -EFAULT;
> +
> +	flags = stats.n_stats;
> +	return _ethtool_get_stats(dev, useraddr, flags);
> +}
> +
>  static int ethtool_get_phy_stats(struct net_device *dev, void __user *useraddr)
>  {
>  	struct ethtool_stats stats;
> @@ -2632,6 +2658,7 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
>  	case ETHTOOL_GSSET_INFO:
>  	case ETHTOOL_GSTRINGS:
>  	case ETHTOOL_GSTATS:
> +	case ETHTOOL_GSTATS2:
>  	case ETHTOOL_GPHYSTATS:
>  	case ETHTOOL_GTSO:
>  	case ETHTOOL_GPERMADDR:
> @@ -2742,6 +2769,9 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
>  	case ETHTOOL_GSTATS:
>  		rc = ethtool_get_stats(dev, useraddr);
>  		break;
> +	case ETHTOOL_GSTATS2:
> +		rc = ethtool_get_stats2(dev, useraddr);
> +		break;
>  	case ETHTOOL_GPERMADDR:
>  		rc = ethtool_get_perm_addr(dev, useraddr);
>  		break;
> 


-- 
Florian

Re: [PATCH 1/3] ethtool: Support ETHTOOL_GSTATS2 command.

[Florian Fainelli]

On 04/17/2018 06:49 PM, greearb@candelatech.com wrote:
> From: Ben Greear <greearb@candelatech.com>
> 
> This is similar to ETHTOOL_GSTATS, but it allows you to specify
> flags.  These flags can be used by the driver to decrease the
> amount of stats refreshed.  In particular, this helps with ath10k
> since getting the firmware stats can be slow.

You can configure the statistics refresh rate through the ethtool
coalescing parameter stats_block_coalesce_usecs, not sure if this is
exactly what you had in mind, but if it works, then you might as well
want to use it.

> 
> Signed-off-by: Ben Greear <greearb@candelatech.com>
> ---
>  include/linux/ethtool.h      | 12 ++++++++++++
>  include/uapi/linux/ethtool.h | 10 ++++++++++
>  net/core/ethtool.c           | 40 +++++++++++++++++++++++++++++++++++-----
>  3 files changed, 57 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index ebe4181..a4aa11f 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -243,6 +243,15 @@ bool ethtool_convert_link_mode_to_legacy_u32(u32 *legacy_u32,
>   * @get_ethtool_stats: Return extended statistics about the device.
>   *	This is only useful if the device maintains statistics not
>   *	included in &struct rtnl_link_stats64.
> + * @get_ethtool_stats2: Return extended statistics about the device.
> + *	This is only useful if the device maintains statistics not
> + *	included in &struct rtnl_link_stats64.
> + *      Takes a flags argument:  0 means all (same as get_ethtool_stats),
> + *      0x1 (ETHTOOL_GS2_SKIP_FW) means skip firmware stats.
> + *      Other flags are reserved for now.
> + *      Same number of stats will be returned, but some of them might
> + *      not be as accurate/refreshed.  This is to allow not querying
> + *      firmware or other expensive-to-read stats, for instance.
>   * @begin: Function to be called before any other operation.  Returns a
>   *	negative error code or zero.
>   * @complete: Function to be called after any other operation except
> @@ -355,6 +364,9 @@ struct ethtool_ops {
>  	int	(*set_phys_id)(struct net_device *, enum ethtool_phys_id_state);
>  	void	(*get_ethtool_stats)(struct net_device *,
>  				     struct ethtool_stats *, u64 *);
> +	void	(*get_ethtool_stats2)(struct net_device *dev,
> +				      struct ethtool_stats *gstats, u64 *data,
> +				      u32 flags);
>  	int	(*begin)(struct net_device *);
>  	void	(*complete)(struct net_device *);
>  	u32	(*get_priv_flags)(struct net_device *);
> diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
> index 4ca65b5..1c74f3e 100644
> --- a/include/uapi/linux/ethtool.h
> +++ b/include/uapi/linux/ethtool.h
> @@ -1396,11 +1396,21 @@ enum ethtool_fec_config_bits {
>  #define ETHTOOL_PHY_STUNABLE	0x0000004f /* Set PHY tunable configuration */
>  #define ETHTOOL_GFECPARAM	0x00000050 /* Get FEC settings */
>  #define ETHTOOL_SFECPARAM	0x00000051 /* Set FEC settings */
> +#define ETHTOOL_GSTATS2		0x00000052 /* get NIC-specific statistics
> +					    * with ability to specify flags.
> +					    * See ETHTOOL_GS2* below.
> +					    */
>  
>  /* compatibility with older code */
>  #define SPARC_ETH_GSET		ETHTOOL_GSET
>  #define SPARC_ETH_SSET		ETHTOOL_SSET
>  
> +/* GSTATS2 flags */
> +#define ETHTOOL_GS2_SKIP_NONE (0)    /* default is to update all stats */
> +#define ETHTOOL_GS2_SKIP_FW   (1<<0) /* Skip reading stats that probe firmware,
> +				      * and thus are slow/expensive.
> +				      */
> +
>  /* Link mode bit indices */
>  enum ethtool_link_mode_bit_indices {
>  	ETHTOOL_LINK_MODE_10baseT_Half_BIT	= 0,
> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> index 03416e6..6ec3413 100644
> --- a/net/core/ethtool.c
> +++ b/net/core/ethtool.c
> @@ -1952,16 +1952,14 @@ static int ethtool_phys_id(struct net_device *dev, void __user *useraddr)
>  	return rc;
>  }
>  
> -static int ethtool_get_stats(struct net_device *dev, void __user *useraddr)
> +static int _ethtool_get_stats(struct net_device *dev, void __user *useraddr,
> +			      u32 flags)
>  {
>  	struct ethtool_stats stats;
>  	const struct ethtool_ops *ops = dev->ethtool_ops;
>  	u64 *data;
>  	int ret, n_stats;
>  
> -	if (!ops->get_ethtool_stats || !ops->get_sset_count)
> -		return -EOPNOTSUPP;
> -
>  	n_stats = ops->get_sset_count(dev, ETH_SS_STATS);
>  	if (n_stats < 0)
>  		return n_stats;
> @@ -1976,7 +1974,10 @@ static int ethtool_get_stats(struct net_device *dev, void __user *useraddr)
>  	if (n_stats && !data)
>  		return -ENOMEM;
>  
> -	ops->get_ethtool_stats(dev, &stats, data);
> +	if (flags != ETHTOOL_GS2_SKIP_NONE)
> +		ops->get_ethtool_stats2(dev, &stats, data, flags);
> +	else
> +		ops->get_ethtool_stats(dev, &stats, data);
>  
>  	ret = -EFAULT;
>  	if (copy_to_user(useraddr, &stats, sizeof(stats)))
> @@ -1991,6 +1992,31 @@ static int ethtool_get_stats(struct net_device *dev, void __user *useraddr)
>  	return ret;
>  }
>  
> +static int ethtool_get_stats(struct net_device *dev, void __user *useraddr)
> +{
> +	const struct ethtool_ops *ops = dev->ethtool_ops;
> +
> +	if (!ops->get_ethtool_stats || !ops->get_sset_count)
> +		return -EOPNOTSUPP;
> +	return _ethtool_get_stats(dev, useraddr, ETHTOOL_GS2_SKIP_NONE);
> +}
> +
> +static int ethtool_get_stats2(struct net_device *dev, void __user *useraddr)
> +{
> +	struct ethtool_stats stats;
> +	const struct ethtool_ops *ops = dev->ethtool_ops;
> +	u32 flags = 0;
> +
> +	if (!ops->get_ethtool_stats2 || !ops->get_sset_count)
> +		return -EOPNOTSUPP;
> +
> +	if (copy_from_user(&stats, useraddr, sizeof(stats)))
> +		return -EFAULT;
> +
> +	flags = stats.n_stats;
> +	return _ethtool_get_stats(dev, useraddr, flags);
> +}
> +
>  static int ethtool_get_phy_stats(struct net_device *dev, void __user *useraddr)
>  {
>  	struct ethtool_stats stats;
> @@ -2632,6 +2658,7 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
>  	case ETHTOOL_GSSET_INFO:
>  	case ETHTOOL_GSTRINGS:
>  	case ETHTOOL_GSTATS:
> +	case ETHTOOL_GSTATS2:
>  	case ETHTOOL_GPHYSTATS:
>  	case ETHTOOL_GTSO:
>  	case ETHTOOL_GPERMADDR:
> @@ -2742,6 +2769,9 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
>  	case ETHTOOL_GSTATS:
>  		rc = ethtool_get_stats(dev, useraddr);
>  		break;
> +	case ETHTOOL_GSTATS2:
> +		rc = ethtool_get_stats2(dev, useraddr);
> +		break;
>  	case ETHTOOL_GPERMADDR:
>  		rc = ethtool_get_perm_addr(dev, useraddr);
>  		break;
> 


-- 
Florian

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH 1/3] ethtool: Support ETHTOOL_GSTATS2 command.

[Johannes Berg]

On Tue, 2018-04-17 at 18:49 -0700, greearb@candelatech.com wrote:
> 
> + * @get_ethtool_stats2: Return extended statistics about the device.
> + *	This is only useful if the device maintains statistics not
> + *	included in &struct rtnl_link_stats64.
> + *      Takes a flags argument:  0 means all (same as get_ethtool_stats),
> + *      0x1 (ETHTOOL_GS2_SKIP_FW) means skip firmware stats.
> + *      Other flags are reserved for now.

It'd be pretty hard to know which flags are firmware stats?

Anyway, there's no way I'm going to take this patch, so you need to
float it on netdev first (best CC us here) and get it applied there
before we can do anything on the wifi side.

johannes

Re: [PATCH 1/3] ethtool: Support ETHTOOL_GSTATS2 command.

[Johannes Berg]

On Tue, 2018-04-17 at 18:49 -0700, greearb@candelatech.com wrote:
> 
> + * @get_ethtool_stats2: Return extended statistics about the device.
> + *	This is only useful if the device maintains statistics not
> + *	included in &struct rtnl_link_stats64.
> + *      Takes a flags argument:  0 means all (same as get_ethtool_stats),
> + *      0x1 (ETHTOOL_GS2_SKIP_FW) means skip firmware stats.
> + *      Other flags are reserved for now.

It'd be pretty hard to know which flags are firmware stats?

Anyway, there's no way I'm going to take this patch, so you need to
float it on netdev first (best CC us here) and get it applied there
before we can do anything on the wifi side.

johannes

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH 1/3] ethtool: Support ETHTOOL_GSTATS2 command.

[Ben Greear]

On 04/18/2018 02:26 PM, Johannes Berg wrote:
> On Tue, 2018-04-17 at 18:49 -0700, greearb@candelatech.com wrote:
>>
>> + * @get_ethtool_stats2: Return extended statistics about the device.
>> + *	This is only useful if the device maintains statistics not
>> + *	included in &struct rtnl_link_stats64.
>> + *      Takes a flags argument:  0 means all (same as get_ethtool_stats),
>> + *      0x1 (ETHTOOL_GS2_SKIP_FW) means skip firmware stats.
>> + *      Other flags are reserved for now.
>
> It'd be pretty hard to know which flags are firmware stats?

Yes, it is, but ethtool stats are difficult to understand in a generic
manner anyway, so someone using them is already likely aware of low-level
details of the driver(s) they are using.

In my case, I have lots of virtual stations (or APs), and I want stats
for them as well as for the 'radio', so I would probe the first vdev with
flags of 'skip-none' to get all stats, including radio (firmware) stats.

And then the rest I would just probe the non-firmware stats.

To be honest, I was slightly amused that anyone expressed interest in
this patch originally, but maybe other people have similar use case
and/or drivers with slow-to-acquire stats.

> Anyway, there's no way I'm going to take this patch, so you need to
> float it on netdev first (best CC us here) and get it applied there
> before we can do anything on the wifi side.

I posted the patches to netdev, ath10k and linux-wireless.  If I had only
posted them individually to different lists I figure I'd be hearing about how
the netdev patch is useless because it has no driver support, etc.

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH 1/3] ethtool: Support ETHTOOL_GSTATS2 command.

[Ben Greear]

On 04/18/2018 02:26 PM, Johannes Berg wrote:
> On Tue, 2018-04-17 at 18:49 -0700, greearb@candelatech.com wrote:
>>
>> + * @get_ethtool_stats2: Return extended statistics about the device.
>> + *	This is only useful if the device maintains statistics not
>> + *	included in &struct rtnl_link_stats64.
>> + *      Takes a flags argument:  0 means all (same as get_ethtool_stats),
>> + *      0x1 (ETHTOOL_GS2_SKIP_FW) means skip firmware stats.
>> + *      Other flags are reserved for now.
>
> It'd be pretty hard to know which flags are firmware stats?

Yes, it is, but ethtool stats are difficult to understand in a generic
manner anyway, so someone using them is already likely aware of low-level
details of the driver(s) they are using.

In my case, I have lots of virtual stations (or APs), and I want stats
for them as well as for the 'radio', so I would probe the first vdev with
flags of 'skip-none' to get all stats, including radio (firmware) stats.

And then the rest I would just probe the non-firmware stats.

To be honest, I was slightly amused that anyone expressed interest in
this patch originally, but maybe other people have similar use case
and/or drivers with slow-to-acquire stats.

> Anyway, there's no way I'm going to take this patch, so you need to
> float it on netdev first (best CC us here) and get it applied there
> before we can do anything on the wifi side.

I posted the patches to netdev, ath10k and linux-wireless.  If I had only
posted them individually to different lists I figure I'd be hearing about how
the netdev patch is useless because it has no driver support, etc.

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH 1/3] ethtool: Support ETHTOOL_GSTATS2 command.

[Johannes Berg]

On Wed, 2018-04-18 at 14:51 -0700, Ben Greear wrote:

> > It'd be pretty hard to know which flags are firmware stats?
> 
> Yes, it is, but ethtool stats are difficult to understand in a generic
> manner anyway, so someone using them is already likely aware of low-level
> details of the driver(s) they are using.

Right. Come to think of it though,

> + * @get_ethtool_stats2: Return extended statistics about the device.
> + *     This is only useful if the device maintains statistics not
> + *     included in &struct rtnl_link_stats64.
> + *      Takes a flags argument:  0 means all (same as get_ethtool_stats),
> + *      0x1 (ETHTOOL_GS2_SKIP_FW) means skip firmware stats.
> + *      Other flags are reserved for now.
> + *      Same number of stats will be returned, but some of them might
> + *      not be as accurate/refreshed.  This is to allow not querying
> + *      firmware or other expensive-to-read stats, for instance.

"skip" vs. "don't refresh" is a bit ambiguous - I'd argue better to
either really skip and not return the non-refreshed ones (also helps
with the identifying), or rename the flag.

Also, wrt. the rest of the patch, I'd argue that it'd be worthwhile to
write the spatch and just add the flags argument to "get_ethtool_stats"
instead of adding a separate method - internally to the kernel it's not
that hard to change.

> I posted the patches to netdev, ath10k and linux-wireless.  If I had only
> posted them individually to different lists I figure I'd be hearing about how
> the netdev patch is useless because it has no driver support, etc.

Sure. I missed netdev, perhaps because it was in To, or more likely
because I was too sleepy. Sorry for the noise.

johannes

Re: [PATCH 1/3] ethtool: Support ETHTOOL_GSTATS2 command.

[Johannes Berg]

On Wed, 2018-04-18 at 14:51 -0700, Ben Greear wrote:

> > It'd be pretty hard to know which flags are firmware stats?
> 
> Yes, it is, but ethtool stats are difficult to understand in a generic
> manner anyway, so someone using them is already likely aware of low-level
> details of the driver(s) they are using.

Right. Come to think of it though,

> + * @get_ethtool_stats2: Return extended statistics about the device.
> + *     This is only useful if the device maintains statistics not
> + *     included in &struct rtnl_link_stats64.
> + *      Takes a flags argument:  0 means all (same as get_ethtool_stats),
> + *      0x1 (ETHTOOL_GS2_SKIP_FW) means skip firmware stats.
> + *      Other flags are reserved for now.
> + *      Same number of stats will be returned, but some of them might
> + *      not be as accurate/refreshed.  This is to allow not querying
> + *      firmware or other expensive-to-read stats, for instance.

"skip" vs. "don't refresh" is a bit ambiguous - I'd argue better to
either really skip and not return the non-refreshed ones (also helps
with the identifying), or rename the flag.

Also, wrt. the rest of the patch, I'd argue that it'd be worthwhile to
write the spatch and just add the flags argument to "get_ethtool_stats"
instead of adding a separate method - internally to the kernel it's not
that hard to change.

> I posted the patches to netdev, ath10k and linux-wireless.  If I had only
> posted them individually to different lists I figure I'd be hearing about how
> the netdev patch is useless because it has no driver support, etc.

Sure. I missed netdev, perhaps because it was in To, or more likely
because I was too sleepy. Sorry for the noise.

johannes

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH 1/3] ethtool: Support ETHTOOL_GSTATS2 command.

[Ben Greear]

On 04/18/2018 11:38 PM, Johannes Berg wrote:
> On Wed, 2018-04-18 at 14:51 -0700, Ben Greear wrote:
>
>>> It'd be pretty hard to know which flags are firmware stats?
>>
>> Yes, it is, but ethtool stats are difficult to understand in a generic
>> manner anyway, so someone using them is already likely aware of low-level
>> details of the driver(s) they are using.
>
> Right. Come to think of it though,
>
>> + * @get_ethtool_stats2: Return extended statistics about the device.
>> + *     This is only useful if the device maintains statistics not
>> + *     included in &struct rtnl_link_stats64.
>> + *      Takes a flags argument:  0 means all (same as get_ethtool_stats),
>> + *      0x1 (ETHTOOL_GS2_SKIP_FW) means skip firmware stats.
>> + *      Other flags are reserved for now.
>> + *      Same number of stats will be returned, but some of them might
>> + *      not be as accurate/refreshed.  This is to allow not querying
>> + *      firmware or other expensive-to-read stats, for instance.
>
> "skip" vs. "don't refresh" is a bit ambiguous - I'd argue better to
> either really skip and not return the non-refreshed ones (also helps
> with the identifying), or rename the flag.

In order to efficiently parse lots of stats over and over again, I probe
the stat names once on startup, map them to the variable I am trying to use
(since different drivers may have different names for the same basic stat),
and then I store the stat index.

On subsequent stat reads, I just grab stats and go right to the index to
store the stat.

If the stats indexes change, that will complicate my logic quite a bit.

Maybe the flag could be called:  ETHTOOL_GS2_NO_REFRESH_FW ?

>
> Also, wrt. the rest of the patch, I'd argue that it'd be worthwhile to
> write the spatch and just add the flags argument to "get_ethtool_stats"
> instead of adding a separate method - internally to the kernel it's not
> that hard to change.

Maybe this could be in followup patches?  It's going to touch a lot of files,
and might be hell to get merged all at once, and I've never used spatch, so
just maybe someone else will volunteer that part :)

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH 1/3] ethtool: Support ETHTOOL_GSTATS2 command.

[Ben Greear]

On 04/18/2018 11:38 PM, Johannes Berg wrote:
> On Wed, 2018-04-18 at 14:51 -0700, Ben Greear wrote:
>
>>> It'd be pretty hard to know which flags are firmware stats?
>>
>> Yes, it is, but ethtool stats are difficult to understand in a generic
>> manner anyway, so someone using them is already likely aware of low-level
>> details of the driver(s) they are using.
>
> Right. Come to think of it though,
>
>> + * @get_ethtool_stats2: Return extended statistics about the device.
>> + *     This is only useful if the device maintains statistics not
>> + *     included in &struct rtnl_link_stats64.
>> + *      Takes a flags argument:  0 means all (same as get_ethtool_stats),
>> + *      0x1 (ETHTOOL_GS2_SKIP_FW) means skip firmware stats.
>> + *      Other flags are reserved for now.
>> + *      Same number of stats will be returned, but some of them might
>> + *      not be as accurate/refreshed.  This is to allow not querying
>> + *      firmware or other expensive-to-read stats, for instance.
>
> "skip" vs. "don't refresh" is a bit ambiguous - I'd argue better to
> either really skip and not return the non-refreshed ones (also helps
> with the identifying), or rename the flag.

In order to efficiently parse lots of stats over and over again, I probe
the stat names once on startup, map them to the variable I am trying to use
(since different drivers may have different names for the same basic stat),
and then I store the stat index.

On subsequent stat reads, I just grab stats and go right to the index to
store the stat.

If the stats indexes change, that will complicate my logic quite a bit.

Maybe the flag could be called:  ETHTOOL_GS2_NO_REFRESH_FW ?

>
> Also, wrt. the rest of the patch, I'd argue that it'd be worthwhile to
> write the spatch and just add the flags argument to "get_ethtool_stats"
> instead of adding a separate method - internally to the kernel it's not
> that hard to change.

Maybe this could be in followup patches?  It's going to touch a lot of files,
and might be hell to get merged all at once, and I've never used spatch, so
just maybe someone else will volunteer that part :)

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH 1/3] ethtool: Support ETHTOOL_GSTATS2 command.

[Johannes Berg]

On Thu, 2018-04-19 at 08:25 -0700, Ben Greear wrote:
> 
> In order to efficiently parse lots of stats over and over again, I probe
> the stat names once on startup, map them to the variable I am trying to use
> (since different drivers may have different names for the same basic stat),
> and then I store the stat index.
> 
> On subsequent stat reads, I just grab stats and go right to the index to
> store the stat.
> 
> If the stats indexes change, that will complicate my logic quite a bit.

That's a good point.

> Maybe the flag could be called:  ETHTOOL_GS2_NO_REFRESH_FW ?

Sounds more to the point to me, yeah.

> > 
> > Also, wrt. the rest of the patch, I'd argue that it'd be worthwhile to
> > write the spatch and just add the flags argument to "get_ethtool_stats"
> > instead of adding a separate method - internally to the kernel it's not
> > that hard to change.
> 
> Maybe this could be in followup patches?  It's going to touch a lot of files,
> and might be hell to get merged all at once, and I've never used spatch, so
> just maybe someone else will volunteer that part :)

I guess you'll have to ask davem. :)

johannes

Re: [PATCH 1/3] ethtool: Support ETHTOOL_GSTATS2 command.

[Johannes Berg]

On Thu, 2018-04-19 at 08:25 -0700, Ben Greear wrote:
> 
> In order to efficiently parse lots of stats over and over again, I probe
> the stat names once on startup, map them to the variable I am trying to use
> (since different drivers may have different names for the same basic stat),
> and then I store the stat index.
> 
> On subsequent stat reads, I just grab stats and go right to the index to
> store the stat.
> 
> If the stats indexes change, that will complicate my logic quite a bit.

That's a good point.

> Maybe the flag could be called:  ETHTOOL_GS2_NO_REFRESH_FW ?

Sounds more to the point to me, yeah.

> > 
> > Also, wrt. the rest of the patch, I'd argue that it'd be worthwhile to
> > write the spatch and just add the flags argument to "get_ethtool_stats"
> > instead of adding a separate method - internally to the kernel it's not
> > that hard to change.
> 
> Maybe this could be in followup patches?  It's going to touch a lot of files,
> and might be hell to get merged all at once, and I've never used spatch, so
> just maybe someone else will volunteer that part :)

I guess you'll have to ask davem. :)

johannes

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH 1/3] ethtool: Support ETHTOOL_GSTATS2 command.

[Johannes Berg]

On Thu, 2018-04-19 at 08:25 -0700, Ben Greear wrote:
> 
> In order to efficiently parse lots of stats over and over again, I probe
> the stat names once on startup, map them to the variable I am trying to use
> (since different drivers may have different names for the same basic stat),
> and then I store the stat index.
> 
> On subsequent stat reads, I just grab stats and go right to the index to
> store the stat.
> 
> If the stats indexes change, that will complicate my logic quite a bit.

That's a good point.

> Maybe the flag could be called:  ETHTOOL_GS2_NO_REFRESH_FW ?

Sounds more to the point to me, yeah.

> > 
> > Also, wrt. the rest of the patch, I'd argue that it'd be worthwhile to
> > write the spatch and just add the flags argument to "get_ethtool_stats"
> > instead of adding a separate method - internally to the kernel it's not
> > that hard to change.
> 
> Maybe this could be in followup patches?  It's going to touch a lot of files,
> and might be hell to get merged all at once, and I've never used spatch, so
> just maybe someone else will volunteer that part :)

I guess you'll have to ask davem. :)

johannes

Re: [PATCH 1/3] ethtool: Support ETHTOOL_GSTATS2 command.

[David Miller]

From: Johannes Berg <johannes@sipsolutions.net>
Date: Thu, 19 Apr 2018 17:26:57 +0200

> On Thu, 2018-04-19 at 08:25 -0700, Ben Greear wrote:
>> 
>> Maybe this could be in followup patches?  It's going to touch a lot of files,
>> and might be hell to get merged all at once, and I've never used spatch, so
>> just maybe someone else will volunteer that part :)
> 
> I guess you'll have to ask davem. :)

Well, first of all, I really don't like this.

The first reason is that every time I see interface foo become foo2,
foo3 is never far behind it.

If foo was not extensible enough such that we needed foo2, we beter
design the new thing with explicitly better extensibility in mind.

Furthermore, what you want here is a specific filter.  Someone else
will want to filter on another criteria, and the next person will
want yet another.

This needs to be properly generalized.

And frankly if we had moved to ethtool netlink/devlink by now, we
could just add a netlink attribute for filtering and not even be
having this conversation.

Re: [PATCH 1/3] ethtool: Support ETHTOOL_GSTATS2 command.

[David Miller]

From: Johannes Berg <johannes@sipsolutions.net>
Date: Thu, 19 Apr 2018 17:26:57 +0200

> On Thu, 2018-04-19 at 08:25 -0700, Ben Greear wrote:
>> 
>> Maybe this could be in followup patches?  It's going to touch a lot of files,
>> and might be hell to get merged all at once, and I've never used spatch, so
>> just maybe someone else will volunteer that part :)
> 
> I guess you'll have to ask davem. :)

Well, first of all, I really don't like this.

The first reason is that every time I see interface foo become foo2,
foo3 is never far behind it.

If foo was not extensible enough such that we needed foo2, we beter
design the new thing with explicitly better extensibility in mind.

Furthermore, what you want here is a specific filter.  Someone else
will want to filter on another criteria, and the next person will
want yet another.

This needs to be properly generalized.

And frankly if we had moved to ethtool netlink/devlink by now, we
could just add a netlink attribute for filtering and not even be
having this conversation.

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH 1/3] ethtool: Support ETHTOOL_GSTATS2 command.

[David Miller]

From: Johannes Berg <johannes-cdvu00un1VgdHxzADdlk8Q@public.gmane.org>
Date: Thu, 19 Apr 2018 17:26:57 +0200

> On Thu, 2018-04-19 at 08:25 -0700, Ben Greear wrote:
>> 
>> Maybe this could be in followup patches?  It's going to touch a lot of files,
>> and might be hell to get merged all at once, and I've never used spatch, so
>> just maybe someone else will volunteer that part :)
> 
> I guess you'll have to ask davem. :)

Well, first of all, I really don't like this.

The first reason is that every time I see interface foo become foo2,
foo3 is never far behind it.

If foo was not extensible enough such that we needed foo2, we beter
design the new thing with explicitly better extensibility in mind.

Furthermore, what you want here is a specific filter.  Someone else
will want to filter on another criteria, and the next person will
want yet another.

This needs to be properly generalized.

And frankly if we had moved to ethtool netlink/devlink by now, we
could just add a netlink attribute for filtering and not even be
having this conversation.

Re: [PATCH 1/3] ethtool: Support ETHTOOL_GSTATS2 command.

[Roopa Prabhu]

On Sun, Apr 22, 2018 at 11:54 AM, David Miller <davem@davemloft.net> wrote:
> From: Johannes Berg <johannes@sipsolutions.net>
> Date: Thu, 19 Apr 2018 17:26:57 +0200
>
>> On Thu, 2018-04-19 at 08:25 -0700, Ben Greear wrote:
>>>
>>> Maybe this could be in followup patches?  It's going to touch a lot of files,
>>> and might be hell to get merged all at once, and I've never used spatch, so
>>> just maybe someone else will volunteer that part :)
>>
>> I guess you'll have to ask davem. :)
>
> Well, first of all, I really don't like this.
>
> The first reason is that every time I see interface foo become foo2,
> foo3 is never far behind it.
>
> If foo was not extensible enough such that we needed foo2, we beter
> design the new thing with explicitly better extensibility in mind.
>
> Furthermore, what you want here is a specific filter.  Someone else
> will want to filter on another criteria, and the next person will
> want yet another.
>
> This needs to be properly generalized.
>
> And frankly if we had moved to ethtool netlink/devlink by now, we
> could just add a netlink attribute for filtering and not even be
> having this conversation.


+1.

Also, the RTM_GETSTATS api was added to improve stats query efficiency
(with filters).
 we should look at it  to see if this fits there. Keeping all stats
queries in one place will help.

Re: [PATCH 1/3] ethtool: Support ETHTOOL_GSTATS2 command.

[Roopa Prabhu]

On Sun, Apr 22, 2018 at 11:54 AM, David Miller <davem@davemloft.net> wrote:
> From: Johannes Berg <johannes@sipsolutions.net>
> Date: Thu, 19 Apr 2018 17:26:57 +0200
>
>> On Thu, 2018-04-19 at 08:25 -0700, Ben Greear wrote:
>>>
>>> Maybe this could be in followup patches?  It's going to touch a lot of files,
>>> and might be hell to get merged all at once, and I've never used spatch, so
>>> just maybe someone else will volunteer that part :)
>>
>> I guess you'll have to ask davem. :)
>
> Well, first of all, I really don't like this.
>
> The first reason is that every time I see interface foo become foo2,
> foo3 is never far behind it.
>
> If foo was not extensible enough such that we needed foo2, we beter
> design the new thing with explicitly better extensibility in mind.
>
> Furthermore, what you want here is a specific filter.  Someone else
> will want to filter on another criteria, and the next person will
> want yet another.
>
> This needs to be properly generalized.
>
> And frankly if we had moved to ethtool netlink/devlink by now, we
> could just add a netlink attribute for filtering and not even be
> having this conversation.


+1.

Also, the RTM_GETSTATS api was added to improve stats query efficiency
(with filters).
 we should look at it  to see if this fits there. Keeping all stats
queries in one place will help.

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH 1/3] ethtool: Support ETHTOOL_GSTATS2 command.

[Roopa Prabhu]

On Sun, Apr 22, 2018 at 11:54 AM, David Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org> wrote:
> From: Johannes Berg <johannes-cdvu00un1VgdHxzADdlk8Q@public.gmane.org>
> Date: Thu, 19 Apr 2018 17:26:57 +0200
>
>> On Thu, 2018-04-19 at 08:25 -0700, Ben Greear wrote:
>>>
>>> Maybe this could be in followup patches?  It's going to touch a lot of files,
>>> and might be hell to get merged all at once, and I've never used spatch, so
>>> just maybe someone else will volunteer that part :)
>>
>> I guess you'll have to ask davem. :)
>
> Well, first of all, I really don't like this.
>
> The first reason is that every time I see interface foo become foo2,
> foo3 is never far behind it.
>
> If foo was not extensible enough such that we needed foo2, we beter
> design the new thing with explicitly better extensibility in mind.
>
> Furthermore, what you want here is a specific filter.  Someone else
> will want to filter on another criteria, and the next person will
> want yet another.
>
> This needs to be properly generalized.
>
> And frankly if we had moved to ethtool netlink/devlink by now, we
> could just add a netlink attribute for filtering and not even be
> having this conversation.


+1.

Also, the RTM_GETSTATS api was added to improve stats query efficiency
(with filters).
 we should look at it  to see if this fits there. Keeping all stats
queries in one place will help.

Re: [PATCH 1/3] ethtool: Support ETHTOOL_GSTATS2 command.

[Ben Greear]

On 04/22/2018 02:15 PM, Roopa Prabhu wrote:
> On Sun, Apr 22, 2018 at 11:54 AM, David Miller <davem@davemloft.net> wrote:
>> From: Johannes Berg <johannes@sipsolutions.net>
>> Date: Thu, 19 Apr 2018 17:26:57 +0200
>>
>>> On Thu, 2018-04-19 at 08:25 -0700, Ben Greear wrote:
>>>>
>>>> Maybe this could be in followup patches?  It's going to touch a lot of files,
>>>> and might be hell to get merged all at once, and I've never used spatch, so
>>>> just maybe someone else will volunteer that part :)
>>>
>>> I guess you'll have to ask davem. :)
>>
>> Well, first of all, I really don't like this.
>>
>> The first reason is that every time I see interface foo become foo2,
>> foo3 is never far behind it.
>>
>> If foo was not extensible enough such that we needed foo2, we beter
>> design the new thing with explicitly better extensibility in mind.
>>
>> Furthermore, what you want here is a specific filter.  Someone else
>> will want to filter on another criteria, and the next person will
>> want yet another.
>>
>> This needs to be properly generalized.
>>
>> And frankly if we had moved to ethtool netlink/devlink by now, we
>> could just add a netlink attribute for filtering and not even be
>> having this conversation.
>
>
> +1.
>
> Also, the RTM_GETSTATS api was added to improve stats query efficiency
> (with filters).
>  we should look at it  to see if this fits there. Keeping all stats
> queries in one place will help.

I like the ethtool API, so I'll be sticking with that for now.

Thanks,
Ben



-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH 1/3] ethtool: Support ETHTOOL_GSTATS2 command.

[Ben Greear]

On 04/22/2018 02:15 PM, Roopa Prabhu wrote:
> On Sun, Apr 22, 2018 at 11:54 AM, David Miller <davem@davemloft.net> wrote:
>> From: Johannes Berg <johannes@sipsolutions.net>
>> Date: Thu, 19 Apr 2018 17:26:57 +0200
>>
>>> On Thu, 2018-04-19 at 08:25 -0700, Ben Greear wrote:
>>>>
>>>> Maybe this could be in followup patches?  It's going to touch a lot of files,
>>>> and might be hell to get merged all at once, and I've never used spatch, so
>>>> just maybe someone else will volunteer that part :)
>>>
>>> I guess you'll have to ask davem. :)
>>
>> Well, first of all, I really don't like this.
>>
>> The first reason is that every time I see interface foo become foo2,
>> foo3 is never far behind it.
>>
>> If foo was not extensible enough such that we needed foo2, we beter
>> design the new thing with explicitly better extensibility in mind.
>>
>> Furthermore, what you want here is a specific filter.  Someone else
>> will want to filter on another criteria, and the next person will
>> want yet another.
>>
>> This needs to be properly generalized.
>>
>> And frankly if we had moved to ethtool netlink/devlink by now, we
>> could just add a netlink attribute for filtering and not even be
>> having this conversation.
>
>
> +1.
>
> Also, the RTM_GETSTATS api was added to improve stats query efficiency
> (with filters).
>  we should look at it  to see if this fits there. Keeping all stats
> queries in one place will help.

I like the ethtool API, so I'll be sticking with that for now.

Thanks,
Ben



-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH 1/3] ethtool: Support ETHTOOL_GSTATS2 command.

[Ben Greear]

On 04/22/2018 11:54 AM, David Miller wrote:
> From: Johannes Berg <johannes@sipsolutions.net>
> Date: Thu, 19 Apr 2018 17:26:57 +0200
>
>> On Thu, 2018-04-19 at 08:25 -0700, Ben Greear wrote:
>>>
>>> Maybe this could be in followup patches?  It's going to touch a lot of files,
>>> and might be hell to get merged all at once, and I've never used spatch, so
>>> just maybe someone else will volunteer that part :)
>>
>> I guess you'll have to ask davem. :)
>
> Well, first of all, I really don't like this.
>
> The first reason is that every time I see interface foo become foo2,
> foo3 is never far behind it.
>
> If foo was not extensible enough such that we needed foo2, we beter
> design the new thing with explicitly better extensibility in mind.
>
> Furthermore, what you want here is a specific filter.  Someone else
> will want to filter on another criteria, and the next person will
> want yet another.
>
> This needs to be properly generalized.
>
> And frankly if we had moved to ethtool netlink/devlink by now, we
> could just add a netlink attribute for filtering and not even be
> having this conversation.

Well, since there are un-defined flags, it would be simple enough to
extend the API further in the future (flag (1<<31) could mean expect
more input members, etc.  And, adding up to 30 more flags to filter on different
things won't change the API and should be backwards compatible.

But, if you don't want it, that is OK by me, I agree it is a fairly
obscure feature.  It would have saved me time if you had said you didn't
want it at the first RFC patch though...

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH 1/3] ethtool: Support ETHTOOL_GSTATS2 command.

[Ben Greear]

On 04/22/2018 11:54 AM, David Miller wrote:
> From: Johannes Berg <johannes@sipsolutions.net>
> Date: Thu, 19 Apr 2018 17:26:57 +0200
>
>> On Thu, 2018-04-19 at 08:25 -0700, Ben Greear wrote:
>>>
>>> Maybe this could be in followup patches?  It's going to touch a lot of files,
>>> and might be hell to get merged all at once, and I've never used spatch, so
>>> just maybe someone else will volunteer that part :)
>>
>> I guess you'll have to ask davem. :)
>
> Well, first of all, I really don't like this.
>
> The first reason is that every time I see interface foo become foo2,
> foo3 is never far behind it.
>
> If foo was not extensible enough such that we needed foo2, we beter
> design the new thing with explicitly better extensibility in mind.
>
> Furthermore, what you want here is a specific filter.  Someone else
> will want to filter on another criteria, and the next person will
> want yet another.
>
> This needs to be properly generalized.
>
> And frankly if we had moved to ethtool netlink/devlink by now, we
> could just add a netlink attribute for filtering and not even be
> having this conversation.

Well, since there are un-defined flags, it would be simple enough to
extend the API further in the future (flag (1<<31) could mean expect
more input members, etc.  And, adding up to 30 more flags to filter on different
things won't change the API and should be backwards compatible.

But, if you don't want it, that is OK by me, I agree it is a fairly
obscure feature.  It would have saved me time if you had said you didn't
want it at the first RFC patch though...

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH 1/3] ethtool: Support ETHTOOL_GSTATS2 command.

[Ben Greear]

On 04/22/2018 11:54 AM, David Miller wrote:
> From: Johannes Berg <johannes-cdvu00un1VgdHxzADdlk8Q@public.gmane.org>
> Date: Thu, 19 Apr 2018 17:26:57 +0200
>
>> On Thu, 2018-04-19 at 08:25 -0700, Ben Greear wrote:
>>>
>>> Maybe this could be in followup patches?  It's going to touch a lot of files,
>>> and might be hell to get merged all at once, and I've never used spatch, so
>>> just maybe someone else will volunteer that part :)
>>
>> I guess you'll have to ask davem. :)
>
> Well, first of all, I really don't like this.
>
> The first reason is that every time I see interface foo become foo2,
> foo3 is never far behind it.
>
> If foo was not extensible enough such that we needed foo2, we beter
> design the new thing with explicitly better extensibility in mind.
>
> Furthermore, what you want here is a specific filter.  Someone else
> will want to filter on another criteria, and the next person will
> want yet another.
>
> This needs to be properly generalized.
>
> And frankly if we had moved to ethtool netlink/devlink by now, we
> could just add a netlink attribute for filtering and not even be
> having this conversation.

Well, since there are un-defined flags, it would be simple enough to
extend the API further in the future (flag (1<<31) could mean expect
more input members, etc.  And, adding up to 30 more flags to filter on different
things won't change the API and should be backwards compatible.

But, if you don't want it, that is OK by me, I agree it is a fairly
obscure feature.  It would have saved me time if you had said you didn't
want it at the first RFC patch though...

Thanks,
Ben

-- 
Ben Greear <greearb-my8/4N5VtI7c+919tysfdA@public.gmane.org>
Candela Technologies Inc  http://www.candelatech.com