[PATCH net-next 0/4] ethtool: Fixes for RX NFC API

[PATCH net-next 0/4] ethtool: Fixes for RX NFC API

[Ben Hutchings]

This series fixes some minor flaws in the documentation and the API itself.

Ben.

Ben Hutchings (4):
  ethtool: Make struct ethtool_rxnfc kernel-doc more self-consistent
  ethtool: Explicitly state that RX NFC rule locations are priorities
  ethtool: Clean up definitions of rule location arrays in RX NFC
  ethtool: Update ethtool_rxnfc::rule_cnt on return from
    ETHTOOL_GRXCLSRLALL

 .../net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c    |    2 +-
 drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c    |    2 +-
 drivers/net/ethernet/freescale/gianfar_ethtool.c   |    5 ++-
 drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c   |    7 ++--
 drivers/net/ethernet/sfc/ethtool.c                 |    2 +-
 drivers/net/ethernet/sun/niu.c                     |    6 ++-
 drivers/net/vmxnet3/vmxnet3_ethtool.c              |    2 +-
 include/linux/ethtool.h                            |   33 +++++++++----------
 8 files changed, 31 insertions(+), 28 deletions(-)

-- 
1.7.4.4


-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

[PATCH net-next 1/4] ethtool: Make struct ethtool_rxnfc kernel-doc more self-consistent

[Ben Hutchings]

Refer consistently to 'classification rules' or just 'rules' rather
than 'filter specifications' or 'filter rules'.

Refer consistently to rule 'locations' and not 'indices'.

Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
 include/linux/ethtool.h |   23 +++++++++++------------
 1 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 3829712..dffab51 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -446,7 +446,7 @@ struct ethtool_flow_ext {
 };
 
 /**
- * struct ethtool_rx_flow_spec - specification for RX flow filter
+ * struct ethtool_rx_flow_spec - classification rule for RX flows
  * @flow_type: Type of match to perform, e.g. %TCP_V4_FLOW
  * @h_u: Flow fields to match (dependent on @flow_type)
  * @h_ext: Additional fields to match
@@ -456,7 +456,7 @@ struct ethtool_flow_ext {
  *	includes the %FLOW_EXT flag.
  * @ring_cookie: RX ring/queue index to deliver to, or %RX_CLS_FLOW_DISC
  *	if packets should be discarded
- * @location: Index of filter in hardware table
+ * @location: Location of rule in the table
  */
 struct ethtool_rx_flow_spec {
 	__u32		flow_type;
@@ -475,9 +475,9 @@ struct ethtool_rx_flow_spec {
  *	%ETHTOOL_GRXCLSRLALL, %ETHTOOL_SRXCLSRLDEL or %ETHTOOL_SRXCLSRLINS
  * @flow_type: Type of flow to be affected, e.g. %TCP_V4_FLOW
  * @data: Command-dependent value
- * @fs: Flow filter specification
+ * @fs: Flow classification rule
  * @rule_cnt: Number of rules to be affected
- * @rule_locs: Array of valid rule indices
+ * @rule_locs: Array of valid rule locations
  *
  * For %ETHTOOL_GRXFH and %ETHTOOL_SRXFH, @data is a bitmask indicating
  * the fields included in the flow hash, e.g. %RXH_IP_SRC.  The following
@@ -489,20 +489,19 @@ struct ethtool_rx_flow_spec {
  * For %ETHTOOL_GRXCLSRLCNT, @rule_cnt is set to the number of defined
  * rules on return.
  *
- * For %ETHTOOL_GRXCLSRULE, @fs.@location specifies the index of an
- * existing filter rule on entry and @fs contains the rule on return.
+ * For %ETHTOOL_GRXCLSRULE, @fs.@location specifies the location of an
+ * existing rule on entry and @fs contains the rule on return.
  *
  * For %ETHTOOL_GRXCLSRLALL, @rule_cnt specifies the array size of the
  * user buffer for @rule_locs on entry.  On return, @data is the size
- * of the filter table and @rule_locs contains the indices of the
+ * of the rule table and @rule_locs contains the locations of the
  * defined rules.
  *
- * For %ETHTOOL_SRXCLSRLINS, @fs specifies the filter rule to add or
- * update.  @fs.@location specifies the index to use and must not be
- * ignored.
+ * For %ETHTOOL_SRXCLSRLINS, @fs specifies the rule to add or update.
+ * @fs.@location specifies the location to use and must not be ignored.
  *
- * For %ETHTOOL_SRXCLSRLDEL, @fs.@location specifies the index of an
- * existing filter rule on entry.
+ * For %ETHTOOL_SRXCLSRLDEL, @fs.@location specifies the location of an
+ * existing rule on entry.
  *
  * Implementation of indexed classification rules generally requires a
  * TCAM.
-- 
1.7.4.4



-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

[PATCH net-next 2/4] ethtool: Explicitly state that RX NFC rule locations are priorities

[Ben Hutchings]

The location of an RX flow classification rule is needed to identify
it for retrieval, replacement or deletion.  However it also defines
the priority of the rule in the case that a flow is matched by
multiple rules.  This is what I intended to imply by referring to the
use of a TCAM, commonly used to implement that behaviour.

However there are other ways this can be done, and it is better to
specify this explicitly.  Further, I want to add the option for
automatic selection of rule locations.

Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
I hope we're all in agreement that this is how locations are supposed to
be numbered.

Ben.

 include/linux/ethtool.h |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index dffab51..58407e3 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -456,7 +456,9 @@ struct ethtool_flow_ext {
  *	includes the %FLOW_EXT flag.
  * @ring_cookie: RX ring/queue index to deliver to, or %RX_CLS_FLOW_DISC
  *	if packets should be discarded
- * @location: Location of rule in the table
+ * @location: Location of rule in the table.  Locations must be
+ *	numbered such that a flow matching multiple rules will be
+ *	classified according to the first (lowest numbered) rule.
  */
 struct ethtool_rx_flow_spec {
 	__u32		flow_type;
@@ -502,9 +504,6 @@ struct ethtool_rx_flow_spec {
  *
  * For %ETHTOOL_SRXCLSRLDEL, @fs.@location specifies the location of an
  * existing rule on entry.
- *
- * Implementation of indexed classification rules generally requires a
- * TCAM.
  */
 struct ethtool_rxnfc {
 	__u32				cmd;
-- 
1.7.4.4



-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

[PATCH net-next 3/4] ethtool: Clean up definitions of rule location arrays in RX NFC

[Ben Hutchings]

Correct the description of ethtool_rxnfc::rule_locs; it is an array
of currently used locations, not all possible valid locations.

Add note that drivers must not use ethtool_rxnfc::rule_locs.

The rule_locs argument to ethtool_ops::get_rxnfc is either NULL or a
pointer to an array of u32, so change the parameter type accordingly.

Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
 .../net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c    |    2 +-
 drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c    |    2 +-
 drivers/net/ethernet/freescale/gianfar_ethtool.c   |    4 ++--
 drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c   |    5 ++---
 drivers/net/ethernet/sfc/ethtool.c                 |    2 +-
 drivers/net/ethernet/sun/niu.c                     |    4 ++--
 drivers/net/vmxnet3/vmxnet3_ethtool.c              |    2 +-
 include/linux/ethtool.h                            |    7 ++++---
 8 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c
index 767c229..ce14f11 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c
@@ -2241,7 +2241,7 @@ static int bnx2x_set_phys_id(struct net_device *dev,
 }
 
 static int bnx2x_get_rxnfc(struct net_device *dev, struct ethtool_rxnfc *info,
-			   void *rules __always_unused)
+			   u32 *rules __always_unused)
 {
 	struct bnx2x *bp = netdev_priv(dev);
 
diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
index 90b4921..40b395f 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
@@ -1902,7 +1902,7 @@ static int set_rss_table(struct net_device *dev,
 }
 
 static int get_rxnfc(struct net_device *dev, struct ethtool_rxnfc *info,
-		     void *rules)
+		     u32 *rules)
 {
 	const struct port_info *pi = netdev_priv(dev);
 
diff --git a/drivers/net/ethernet/freescale/gianfar_ethtool.c b/drivers/net/ethernet/freescale/gianfar_ethtool.c
index 25a8c2a..4223830 100644
--- a/drivers/net/ethernet/freescale/gianfar_ethtool.c
+++ b/drivers/net/ethernet/freescale/gianfar_ethtool.c
@@ -1712,7 +1712,7 @@ static int gfar_set_nfc(struct net_device *dev, struct ethtool_rxnfc *cmd)
 }
 
 static int gfar_get_nfc(struct net_device *dev, struct ethtool_rxnfc *cmd,
-		void *rule_locs)
+		u32 *rule_locs)
 {
 	struct gfar_private *priv = netdev_priv(dev);
 	int ret = 0;
@@ -1728,7 +1728,7 @@ static int gfar_get_nfc(struct net_device *dev, struct ethtool_rxnfc *cmd,
 		ret = gfar_get_cls(priv, cmd);
 		break;
 	case ETHTOOL_GRXCLSRLALL:
-		ret = gfar_get_cls_all(priv, cmd, (u32 *) rule_locs);
+		ret = gfar_get_cls_all(priv, cmd, rule_locs);
 		break;
 	default:
 		ret = -EINVAL;
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
index 82d4244..bad2d27 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
@@ -2283,7 +2283,7 @@ static int ixgbe_get_ethtool_fdir_all(struct ixgbe_adapter *adapter,
 }
 
 static int ixgbe_get_rxnfc(struct net_device *dev, struct ethtool_rxnfc *cmd,
-			   void *rule_locs)
+			   u32 *rule_locs)
 {
 	struct ixgbe_adapter *adapter = netdev_priv(dev);
 	int ret = -EOPNOTSUPP;
@@ -2301,8 +2301,7 @@ static int ixgbe_get_rxnfc(struct net_device *dev, struct ethtool_rxnfc *cmd,
 		ret = ixgbe_get_ethtool_fdir_entry(adapter, cmd);
 		break;
 	case ETHTOOL_GRXCLSRLALL:
-		ret = ixgbe_get_ethtool_fdir_all(adapter, cmd,
-						 (u32 *)rule_locs);
+		ret = ixgbe_get_ethtool_fdir_all(adapter, cmd, rule_locs);
 		break;
 	default:
 		break;
diff --git a/drivers/net/ethernet/sfc/ethtool.c b/drivers/net/ethernet/sfc/ethtool.c
index 93f1fb9..9536925 100644
--- a/drivers/net/ethernet/sfc/ethtool.c
+++ b/drivers/net/ethernet/sfc/ethtool.c
@@ -824,7 +824,7 @@ static int efx_ethtool_reset(struct net_device *net_dev, u32 *flags)
 
 static int
 efx_ethtool_get_rxnfc(struct net_device *net_dev,
-		      struct ethtool_rxnfc *info, void *rules __always_unused)
+		      struct ethtool_rxnfc *info, u32 *rules __always_unused)
 {
 	struct efx_nic *efx = netdev_priv(net_dev);
 
diff --git a/drivers/net/ethernet/sun/niu.c b/drivers/net/ethernet/sun/niu.c
index 3c9ef1c..8037059 100644
--- a/drivers/net/ethernet/sun/niu.c
+++ b/drivers/net/ethernet/sun/niu.c
@@ -7306,7 +7306,7 @@ static int niu_get_ethtool_tcam_all(struct niu *np,
 }
 
 static int niu_get_nfc(struct net_device *dev, struct ethtool_rxnfc *cmd,
-		       void *rule_locs)
+		       u32 *rule_locs)
 {
 	struct niu *np = netdev_priv(dev);
 	int ret = 0;
@@ -7325,7 +7325,7 @@ static int niu_get_nfc(struct net_device *dev, struct ethtool_rxnfc *cmd,
 		ret = niu_get_ethtool_tcam_entry(np, cmd);
 		break;
 	case ETHTOOL_GRXCLSRLALL:
-		ret = niu_get_ethtool_tcam_all(np, cmd, (u32 *)rule_locs);
+		ret = niu_get_ethtool_tcam_all(np, cmd, rule_locs);
 		break;
 	default:
 		ret = -EINVAL;
diff --git a/drivers/net/vmxnet3/vmxnet3_ethtool.c b/drivers/net/vmxnet3/vmxnet3_ethtool.c
index 27400ed..e662cbc 100644
--- a/drivers/net/vmxnet3/vmxnet3_ethtool.c
+++ b/drivers/net/vmxnet3/vmxnet3_ethtool.c
@@ -558,7 +558,7 @@ out:
 
 static int
 vmxnet3_get_rxnfc(struct net_device *netdev, struct ethtool_rxnfc *info,
-		  void *rules)
+		  u32 *rules)
 {
 	struct vmxnet3_adapter *adapter = netdev_priv(netdev);
 	switch (info->cmd) {
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 58407e3..3df1f3b 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -479,7 +479,7 @@ struct ethtool_rx_flow_spec {
  * @data: Command-dependent value
  * @fs: Flow classification rule
  * @rule_cnt: Number of rules to be affected
- * @rule_locs: Array of valid rule locations
+ * @rule_locs: Array of used rule locations
  *
  * For %ETHTOOL_GRXFH and %ETHTOOL_SRXFH, @data is a bitmask indicating
  * the fields included in the flow hash, e.g. %RXH_IP_SRC.  The following
@@ -497,7 +497,8 @@ struct ethtool_rx_flow_spec {
  * For %ETHTOOL_GRXCLSRLALL, @rule_cnt specifies the array size of the
  * user buffer for @rule_locs on entry.  On return, @data is the size
  * of the rule table and @rule_locs contains the locations of the
- * defined rules.
+ * defined rules.  Drivers must use the second parameter to get_rxnfc()
+ * instead of @rule_locs.
  *
  * For %ETHTOOL_SRXCLSRLINS, @fs specifies the rule to add or update.
  * @fs.@location specifies the location to use and must not be ignored.
@@ -936,7 +937,7 @@ struct ethtool_ops {
 	int	(*set_priv_flags)(struct net_device *, u32);
 	int	(*get_sset_count)(struct net_device *, int);
 	int	(*get_rxnfc)(struct net_device *,
-			     struct ethtool_rxnfc *, void *);
+			     struct ethtool_rxnfc *, u32 *rule_locs);
 	int	(*set_rxnfc)(struct net_device *, struct ethtool_rxnfc *);
 	int	(*flash_device)(struct net_device *, struct ethtool_flash *);
 	int	(*reset)(struct net_device *, u32 *);
-- 
1.7.4.4



-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

[PATCH net-next 4/4] ethtool: Update ethtool_rxnfc::rule_cnt on return from ETHTOOL_GRXCLSRLALL

[Ben Hutchings]

A user-space process must use ETHTOOL_GRXCLSRLCNT to find the number
of classification rules, then allocate a buffer of the right size,
then use ETHTOOL_GRXCLSRLALL to fill the buffer.  If some other
process inserts or deletes a rule between those two operations,
the user buffer might turn out to be the wrong size.

If it's too small, the return value will be -EMSGSIZE.  But if it's
too large, there is no indication of this.  Fix this by updating
the rule_cnt field on return.

Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
The change to ixgbe and niu is compile-tested only.  The change to
gianfar isn't tested at all.

Ben.

 drivers/net/ethernet/freescale/gianfar_ethtool.c |    1 +
 drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c |    2 ++
 drivers/net/ethernet/sun/niu.c                   |    2 ++
 include/linux/ethtool.h                          |    6 +++---
 4 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/freescale/gianfar_ethtool.c b/drivers/net/ethernet/freescale/gianfar_ethtool.c
index 4223830..f30b96f 100644
--- a/drivers/net/ethernet/freescale/gianfar_ethtool.c
+++ b/drivers/net/ethernet/freescale/gianfar_ethtool.c
@@ -1676,6 +1676,7 @@ static int gfar_get_cls_all(struct gfar_private *priv,
 	}
 
 	cmd->data = MAX_FILER_IDX;
+	cmd->rule_cnt = i;
 
 	return 0;
 }
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
index bad2d27..34591c3 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
@@ -2279,6 +2279,8 @@ static int ixgbe_get_ethtool_fdir_all(struct ixgbe_adapter *adapter,
 		cnt++;
 	}
 
+	cmd->rule_cnt = cnt;
+
 	return 0;
 }
 
diff --git a/drivers/net/ethernet/sun/niu.c b/drivers/net/ethernet/sun/niu.c
index 8037059..fff0f8b 100644
--- a/drivers/net/ethernet/sun/niu.c
+++ b/drivers/net/ethernet/sun/niu.c
@@ -7302,6 +7302,8 @@ static int niu_get_ethtool_tcam_all(struct niu *np,
 	}
 	niu_unlock_parent(np, flags);
 
+	nfc->rule_cnt = cnt;
+
 	return ret;
 }
 
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 3df1f3b..ddd536d 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -496,9 +496,9 @@ struct ethtool_rx_flow_spec {
  *
  * For %ETHTOOL_GRXCLSRLALL, @rule_cnt specifies the array size of the
  * user buffer for @rule_locs on entry.  On return, @data is the size
- * of the rule table and @rule_locs contains the locations of the
- * defined rules.  Drivers must use the second parameter to get_rxnfc()
- * instead of @rule_locs.
+ * of the rule table, @rule_cnt is the number of defined rules, and
+ * @rule_locs contains the locations of the defined rules.  Drivers
+ * must use the second parameter to get_rxnfc() instead of @rule_locs.
  *
  * For %ETHTOOL_SRXCLSRLINS, @fs specifies the rule to add or update.
  * @fs.@location specifies the location to use and must not be ignored.
-- 
1.7.4.4


-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH net-next 0/4] ethtool: Fixes for RX NFC API

[David Miller]

From: Ben Hutchings <bhutchings@solarflare.com>
Date: Wed, 07 Sep 2011 00:44:53 +0100

> This series fixes some minor flaws in the documentation and the API itself.

All applied, thanks Ben.