[patch net-next v4 00/10] net: allow user specify TC action HW stats type

[patch net-next v4 00/10] net: allow user specify TC action HW stats type

[Jiri Pirko]

Currently, when user adds a TC action and the action gets offloaded,
the user expects the HW stats to be counted and included in stats dump.
However, since drivers may implement different types of counting, there
is no way to specify which one the user is interested in.

For example for mlx5, only delayed counters are available as the driver
periodically polls for updated stats.

In case of mlxsw, the counters are queried on dump time. However, the
HW resources for this type of counters is quite limited (couple of
thousands). This limits the amount of supported offloaded filters
significantly. Without counter assigned, the HW is capable to carry
millions of those.

On top of that, mlxsw HW is able to support delayed counters as well in
greater numbers. That is going to be added in a follow-up patch.

This patchset allows user to specify one of the following types of HW
stats for added action:
immediate - queried during dump time
delayed - polled from HW periodically or sent by HW in async manner
disabled - no stats needed

Note that if "hw_stats" option is not passed, user does not care about
the type, just expects any type of stats.

Examples:
$ tc filter add dev enp0s16np28 ingress proto ip handle 1 pref 1 flower skip_sw dst_ip 192.168.1.1 action drop hw_stats disabled
$ tc -s filter show dev enp0s16np28 ingress
filter protocol ip pref 1 flower chain 0
filter protocol ip pref 1 flower chain 0 handle 0x1
  eth_type ipv4
  dst_ip 192.168.1.1
  skip_sw
  in_hw in_hw_count 2
        action order 1: gact action drop
         random type none pass val 0
         index 1 ref 1 bind 1 installed 7 sec used 2 sec
        Action statistics:
        Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
        backlog 0b 0p requeues 0
        hw_stats disabled

$ tc filter add dev enp0s16np28 ingress proto ip handle 1 pref 1 flower skip_sw dst_ip 192.168.1.1 action drop hw_stats immediate
$ tc -s filter show dev enp0s16np28 ingress
filter protocol ip pref 1 flower chain 0
filter protocol ip pref 1 flower chain 0 handle 0x1
  eth_type ipv4
  dst_ip 192.168.1.1
  skip_sw
  in_hw in_hw_count 2
        action order 1: gact action drop
         random type none pass val 0
         index 1 ref 1 bind 1 installed 11 sec used 4 sec
        Action statistics:
        Sent 102 bytes 1 pkt (dropped 1, overlimits 0 requeues 0)
        Sent software 0 bytes 0 pkt
        Sent hardware 102 bytes 1 pkt
        backlog 0b 0p requeues 0
        hw_stats immediate

Jiri Pirko (10):
  flow_offload: Introduce offload of HW stats type
  ocelot_flower: use flow_offload_has_one_action() helper
  flow_offload: check for basic action hw stats type
  mlxsw: spectrum_flower: Do not allow mixing HW stats types for actions
  mlxsw: restrict supported HW stats type to "any"
  flow_offload: introduce "immediate" HW stats type and allow it in
    mlxsw
  flow_offload: introduce "delayed" HW stats type and allow it in mlx5
  mlxsw: spectrum_acl: Ask device for rule stats only if counter was
    created
  flow_offload: introduce "disabled" HW stats type and allow it in mlxsw
  sched: act: allow user to specify type of HW stats for a filter

 drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c  |  9 ++-
 .../ethernet/chelsio/cxgb4/cxgb4_tc_flower.c  |  8 ++-
 .../ethernet/chelsio/cxgb4/cxgb4_tc_flower.h  |  3 +-
 .../chelsio/cxgb4/cxgb4_tc_matchall.c         |  3 +-
 .../net/ethernet/marvell/mvpp2/mvpp2_cls.c    |  6 ++
 .../net/ethernet/mellanox/mlx5/core/en_tc.c   | 11 +++
 .../net/ethernet/mellanox/mlxsw/spectrum.h    |  3 +-
 .../ethernet/mellanox/mlxsw/spectrum_acl.c    | 26 ++++---
 .../ethernet/mellanox/mlxsw/spectrum_flower.c | 17 +++--
 drivers/net/ethernet/mscc/ocelot_flower.c     |  6 +-
 .../ethernet/netronome/nfp/flower/action.c    |  4 ++
 .../net/ethernet/qlogic/qede/qede_filter.c    | 10 ++-
 .../net/ethernet/stmicro/stmmac/stmmac_tc.c   |  9 ++-
 include/net/act_api.h                         |  4 ++
 include/net/flow_offload.h                    | 68 +++++++++++++++++++
 include/uapi/linux/pkt_cls.h                  | 22 ++++++
 net/dsa/slave.c                               |  4 ++
 net/sched/act_api.c                           | 36 ++++++++++
 net/sched/cls_api.c                           |  7 ++
 19 files changed, 230 insertions(+), 26 deletions(-)

-- 
2.21.1

[patch net-next v4 01/10] flow_offload: Introduce offload of HW stats type

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

Initially, pass "ANY" (struct is zeroed) to the drivers as that is the
current implicit value coming down to flow_offload. Add a bool
indicating that entries have mixed HW stats type.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
v3->v4:
- fixed member alignment
v2->v3:
- moved to bitfield
- removed "mixed" bool
v1->v2:
- moved to actions
- add mixed bool
---
 include/net/flow_offload.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
index cd3510ac66b0..93d17f37e980 100644
--- a/include/net/flow_offload.h
+++ b/include/net/flow_offload.h
@@ -154,6 +154,8 @@ enum flow_action_mangle_base {
 	FLOW_ACT_MANGLE_HDR_TYPE_UDP,
 };
 
+#define FLOW_ACTION_HW_STATS_TYPE_ANY 0
+
 typedef void (*action_destr)(void *priv);
 
 struct flow_action_cookie {
@@ -168,6 +170,7 @@ void flow_action_cookie_destroy(struct flow_action_cookie *cookie);
 
 struct flow_action_entry {
 	enum flow_action_id		id;
+	u8				hw_stats_type;
 	action_destr			destructor;
 	void				*destructor_priv;
 	union {
-- 
2.21.1

[patch net-next v4 02/10] ocelot_flower: use flow_offload_has_one_action() helper

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

Instead of directly checking number of action entries, use
flow_offload_has_one_action() helper.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
Acked-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v1->v2:
- new patch
---
 drivers/net/ethernet/mscc/ocelot_flower.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mscc/ocelot_flower.c b/drivers/net/ethernet/mscc/ocelot_flower.c
index 8993dadf063c..8986f209e981 100644
--- a/drivers/net/ethernet/mscc/ocelot_flower.c
+++ b/drivers/net/ethernet/mscc/ocelot_flower.c
@@ -14,7 +14,7 @@ static int ocelot_flower_parse_action(struct flow_cls_offload *f,
 	const struct flow_action_entry *a;
 	int i;
 
-	if (f->rule->action.num_entries != 1)
+	if (!flow_offload_has_one_action(&f->rule->action))
 		return -EOPNOTSUPP;
 
 	flow_action_for_each(i, a, &f->rule->action) {
-- 
2.21.1

[patch net-next v4 03/10] flow_offload: check for basic action hw stats type

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

Introduce flow_action_basic_hw_stats_types_check() helper and use it
in drivers. That sanitizes the drivers which do not have support
for action HW stats types.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
v3->v4:
- fixed entries iteration in check (s/0/i/)
- compare allowed type explicitly to 0 to avoid confusion
v2->v3:
- added flow_action_hw_stats_types_check() to pass allowed types
- "mixed" bool got remove, iterate entries in check
- added mlx5 checking instead of separate patches (will be changed by
  later patch to flow_action_hw_stats_types_check()
v1->v2:
- new patch
---
 drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c  |  9 ++-
 .../ethernet/chelsio/cxgb4/cxgb4_tc_flower.c  |  8 ++-
 .../ethernet/chelsio/cxgb4/cxgb4_tc_flower.h  |  3 +-
 .../chelsio/cxgb4/cxgb4_tc_matchall.c         |  3 +-
 .../net/ethernet/marvell/mvpp2/mvpp2_cls.c    |  6 ++
 .../net/ethernet/mellanox/mlx5/core/en_tc.c   |  9 +++
 drivers/net/ethernet/mscc/ocelot_flower.c     |  4 ++
 .../ethernet/netronome/nfp/flower/action.c    |  4 ++
 .../net/ethernet/qlogic/qede/qede_filter.c    | 10 ++-
 .../net/ethernet/stmicro/stmmac/stmmac_tc.c   |  9 ++-
 include/net/flow_offload.h                    | 61 +++++++++++++++++++
 net/dsa/slave.c                               |  4 ++
 12 files changed, 119 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
index 9bec256b0934..523bf4be43cc 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
@@ -279,7 +279,8 @@ bnxt_tc_parse_pedit(struct bnxt *bp, struct bnxt_tc_actions *actions,
 
 static int bnxt_tc_parse_actions(struct bnxt *bp,
 				 struct bnxt_tc_actions *actions,
-				 struct flow_action *flow_action)
+				 struct flow_action *flow_action,
+				 struct netlink_ext_ack *extack)
 {
 	/* Used to store the L2 rewrite mask for dmac (6 bytes) followed by
 	 * smac (6 bytes) if rewrite of both is specified, otherwise either
@@ -299,6 +300,9 @@ static int bnxt_tc_parse_actions(struct bnxt *bp,
 		return -EINVAL;
 	}
 
+	if (!flow_action_basic_hw_stats_types_check(flow_action, extack))
+		return -EOPNOTSUPP;
+
 	flow_action_for_each(i, act, flow_action) {
 		switch (act->id) {
 		case FLOW_ACTION_DROP:
@@ -491,7 +495,8 @@ static int bnxt_tc_parse_flow(struct bnxt *bp,
 		flow->tun_mask.tp_src = match.mask->src;
 	}
 
-	return bnxt_tc_parse_actions(bp, &flow->actions, &rule->action);
+	return bnxt_tc_parse_actions(bp, &flow->actions, &rule->action,
+				     tc_flow_cmd->common.extack);
 }
 
 static int bnxt_hwrm_cfa_flow_free(struct bnxt *bp,
diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c
index bb5513bdd293..cc46277e98de 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c
@@ -544,7 +544,8 @@ static bool valid_pedit_action(struct net_device *dev,
 }
 
 int cxgb4_validate_flow_actions(struct net_device *dev,
-				struct flow_action *actions)
+				struct flow_action *actions,
+				struct netlink_ext_ack *extack)
 {
 	struct flow_action_entry *act;
 	bool act_redir = false;
@@ -552,6 +553,9 @@ int cxgb4_validate_flow_actions(struct net_device *dev,
 	bool act_vlan = false;
 	int i;
 
+	if (!flow_action_basic_hw_stats_types_check(actions, extack))
+		return -EOPNOTSUPP;
+
 	flow_action_for_each(i, act, actions) {
 		switch (act->id) {
 		case FLOW_ACTION_ACCEPT:
@@ -642,7 +646,7 @@ int cxgb4_tc_flower_replace(struct net_device *dev,
 	struct filter_ctx ctx;
 	int fidx, ret;
 
-	if (cxgb4_validate_flow_actions(dev, &rule->action))
+	if (cxgb4_validate_flow_actions(dev, &rule->action, extack))
 		return -EOPNOTSUPP;
 
 	if (cxgb4_validate_flow_match(dev, cls))
diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.h b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.h
index e132516e9868..0a30c96b81ff 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.h
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.h
@@ -112,7 +112,8 @@ void cxgb4_process_flow_actions(struct net_device *in,
 				struct flow_action *actions,
 				struct ch_filter_specification *fs);
 int cxgb4_validate_flow_actions(struct net_device *dev,
-				struct flow_action *actions);
+				struct flow_action *actions,
+				struct netlink_ext_ack *extack);
 
 int cxgb4_tc_flower_replace(struct net_device *dev,
 			    struct flow_cls_offload *cls);
diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_matchall.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_matchall.c
index 1b7681a4eb32..d80dee4d316d 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_matchall.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_matchall.c
@@ -286,7 +286,8 @@ int cxgb4_tc_matchall_replace(struct net_device *dev,
 		}
 
 		ret = cxgb4_validate_flow_actions(dev,
-						  &cls_matchall->rule->action);
+						  &cls_matchall->rule->action,
+						  extack);
 		if (ret)
 			return ret;
 
diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_cls.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_cls.c
index 35478cba2aa5..0a0c6ec2336c 100644
--- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_cls.c
+++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_cls.c
@@ -1082,6 +1082,9 @@ static int mvpp2_port_c2_tcam_rule_add(struct mvpp2_port *port,
 	u8 qh, ql, pmap;
 	int index, ctx;
 
+	if (!flow_action_basic_hw_stats_types_check(&rule->flow->action, NULL))
+		return -EOPNOTSUPP;
+
 	memset(&c2, 0, sizeof(c2));
 
 	index = mvpp2_cls_c2_port_flow_index(port, rule->loc);
@@ -1305,6 +1308,9 @@ static int mvpp2_cls_rfs_parse_rule(struct mvpp2_rfs_rule *rule)
 	struct flow_rule *flow = rule->flow;
 	struct flow_action_entry *act;
 
+	if (!flow_action_basic_hw_stats_types_check(&rule->flow->action, NULL))
+		return -EOPNOTSUPP;
+
 	act = &flow->action.entries[0];
 	if (act->id != FLOW_ACTION_QUEUE && act->id != FLOW_ACTION_DROP)
 		return -EOPNOTSUPP;
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
index 4eb2f2392d2d..cfe393cb4026 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
@@ -2878,6 +2878,9 @@ static int parse_tc_nic_actions(struct mlx5e_priv *priv,
 	if (!flow_action_has_entries(flow_action))
 		return -EINVAL;
 
+	if (!flow_action_basic_hw_stats_types_check(flow_action, extack))
+		return -EOPNOTSUPP;
+
 	attr->flow_tag = MLX5_FS_DEFAULT_FLOW_TAG;
 
 	flow_action_for_each(i, act, flow_action) {
@@ -3330,6 +3333,9 @@ static int parse_tc_fdb_actions(struct mlx5e_priv *priv,
 	if (!flow_action_has_entries(flow_action))
 		return -EINVAL;
 
+	if (!flow_action_basic_hw_stats_types_check(flow_action, extack))
+		return -EOPNOTSUPP;
+
 	flow_action_for_each(i, act, flow_action) {
 		switch (act->id) {
 		case FLOW_ACTION_DROP:
@@ -4148,6 +4154,9 @@ static int scan_tc_matchall_fdb_actions(struct mlx5e_priv *priv,
 		return -EOPNOTSUPP;
 	}
 
+	if (!flow_action_basic_hw_stats_types_check(flow_action, extack))
+		return -EOPNOTSUPP;
+
 	flow_action_for_each(i, act, flow_action) {
 		switch (act->id) {
 		case FLOW_ACTION_POLICE:
diff --git a/drivers/net/ethernet/mscc/ocelot_flower.c b/drivers/net/ethernet/mscc/ocelot_flower.c
index 8986f209e981..6d84173373c7 100644
--- a/drivers/net/ethernet/mscc/ocelot_flower.c
+++ b/drivers/net/ethernet/mscc/ocelot_flower.c
@@ -17,6 +17,10 @@ static int ocelot_flower_parse_action(struct flow_cls_offload *f,
 	if (!flow_offload_has_one_action(&f->rule->action))
 		return -EOPNOTSUPP;
 
+	if (!flow_action_basic_hw_stats_types_check(&f->rule->action,
+						    f->common.extack))
+		return -EOPNOTSUPP;
+
 	flow_action_for_each(i, a, &f->rule->action) {
 		switch (a->id) {
 		case FLOW_ACTION_DROP:
diff --git a/drivers/net/ethernet/netronome/nfp/flower/action.c b/drivers/net/ethernet/netronome/nfp/flower/action.c
index c06600fb47ff..4aa7346cb040 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/action.c
+++ b/drivers/net/ethernet/netronome/nfp/flower/action.c
@@ -1207,6 +1207,10 @@ int nfp_flower_compile_action(struct nfp_app *app,
 	bool pkt_host = false;
 	u32 csum_updated = 0;
 
+	if (!flow_action_basic_hw_stats_types_check(&flow->rule->action,
+						    extack))
+		return -EOPNOTSUPP;
+
 	memset(nfp_flow->action_data, 0, NFP_FL_MAX_A_SIZ);
 	nfp_flow->meta.act_len = 0;
 	tun_type = NFP_FL_TUNNEL_NONE;
diff --git a/drivers/net/ethernet/qlogic/qede/qede_filter.c b/drivers/net/ethernet/qlogic/qede/qede_filter.c
index d1ce4531d01a..6505f7e2d1db 100644
--- a/drivers/net/ethernet/qlogic/qede/qede_filter.c
+++ b/drivers/net/ethernet/qlogic/qede/qede_filter.c
@@ -1746,7 +1746,8 @@ int qede_get_arfs_filter_count(struct qede_dev *edev)
 }
 
 static int qede_parse_actions(struct qede_dev *edev,
-			      struct flow_action *flow_action)
+			      struct flow_action *flow_action,
+			      struct netlink_ext_ack *extack)
 {
 	const struct flow_action_entry *act;
 	int i;
@@ -1756,6 +1757,9 @@ static int qede_parse_actions(struct qede_dev *edev,
 		return -EINVAL;
 	}
 
+	if (!flow_action_basic_hw_stats_types_check(flow_action, extack))
+		return -EOPNOTSUPP;
+
 	flow_action_for_each(i, act, flow_action) {
 		switch (act->id) {
 		case FLOW_ACTION_DROP:
@@ -1970,7 +1974,7 @@ int qede_add_tc_flower_fltr(struct qede_dev *edev, __be16 proto,
 	}
 
 	/* parse tc actions and get the vf_id */
-	if (qede_parse_actions(edev, &f->rule->action))
+	if (qede_parse_actions(edev, &f->rule->action, f->common.extack))
 		goto unlock;
 
 	if (qede_flow_find_fltr(edev, &t)) {
@@ -2038,7 +2042,7 @@ static int qede_flow_spec_validate(struct qede_dev *edev,
 		return -EINVAL;
 	}
 
-	if (qede_parse_actions(edev, flow_action))
+	if (qede_parse_actions(edev, flow_action, NULL))
 		return -EINVAL;
 
 	return 0;
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
index 7a01dee2f9a8..a0e6118444b0 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
@@ -367,7 +367,8 @@ static int tc_setup_cbs(struct stmmac_priv *priv,
 
 static int tc_parse_flow_actions(struct stmmac_priv *priv,
 				 struct flow_action *action,
-				 struct stmmac_flow_entry *entry)
+				 struct stmmac_flow_entry *entry,
+				 struct netlink_ext_ack *extack)
 {
 	struct flow_action_entry *act;
 	int i;
@@ -375,6 +376,9 @@ static int tc_parse_flow_actions(struct stmmac_priv *priv,
 	if (!flow_action_has_entries(action))
 		return -EINVAL;
 
+	if (!flow_action_basic_hw_stats_types_check(action, extack))
+		return -EOPNOTSUPP;
+
 	flow_action_for_each(i, act, action) {
 		switch (act->id) {
 		case FLOW_ACTION_DROP:
@@ -530,7 +534,8 @@ static int tc_add_flow(struct stmmac_priv *priv,
 			return -ENOENT;
 	}
 
-	ret = tc_parse_flow_actions(priv, &rule->action, entry);
+	ret = tc_parse_flow_actions(priv, &rule->action, entry,
+				    cls->common.extack);
 	if (ret)
 		return ret;
 
diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
index 93d17f37e980..8b40f612a565 100644
--- a/include/net/flow_offload.h
+++ b/include/net/flow_offload.h
@@ -3,6 +3,7 @@
 
 #include <linux/kernel.h>
 #include <linux/list.h>
+#include <linux/netlink.h>
 #include <net/flow_dissector.h>
 #include <linux/rhashtable.h>
 
@@ -251,6 +252,66 @@ static inline bool flow_offload_has_one_action(const struct flow_action *action)
 	return action->num_entries == 1;
 }
 
+static inline bool
+flow_action_mixed_hw_stats_types_check(const struct flow_action *action,
+				       struct netlink_ext_ack *extack)
+{
+	const struct flow_action_entry *action_entry;
+	u8 uninitialized_var(last_hw_stats_type);
+	int i;
+
+	if (flow_offload_has_one_action(action))
+		return true;
+
+	for (i = 0; i < action->num_entries; i++) {
+		action_entry = &action->entries[i];
+		if (i && action_entry->hw_stats_type != last_hw_stats_type) {
+			NL_SET_ERR_MSG_MOD(extack, "Mixing HW stats types for actions is not supported");
+			return false;
+		}
+		last_hw_stats_type = action_entry->hw_stats_type;
+	}
+	return true;
+}
+
+static inline const struct flow_action_entry *
+flow_action_first_entry_get(const struct flow_action *action)
+{
+	WARN_ON(!flow_action_has_entries(action));
+	return &action->entries[0];
+}
+
+static inline bool
+flow_action_hw_stats_types_check(const struct flow_action *action,
+				 struct netlink_ext_ack *extack,
+				 u8 allowed_hw_stats_type)
+{
+	const struct flow_action_entry *action_entry;
+
+	if (!flow_action_has_entries(action))
+		return true;
+	if (!flow_action_mixed_hw_stats_types_check(action, extack))
+		return false;
+	action_entry = flow_action_first_entry_get(action);
+	if (allowed_hw_stats_type == 0 &&
+	    action_entry->hw_stats_type != FLOW_ACTION_HW_STATS_TYPE_ANY) {
+		NL_SET_ERR_MSG_MOD(extack, "Driver supports only default HW stats type \"any\"");
+		return false;
+	} else if (allowed_hw_stats_type != 0 &&
+		   action_entry->hw_stats_type != allowed_hw_stats_type) {
+		NL_SET_ERR_MSG_MOD(extack, "Driver does not support selected HW stats type");
+		return false;
+	}
+	return true;
+}
+
+static inline bool
+flow_action_basic_hw_stats_types_check(const struct flow_action *action,
+				       struct netlink_ext_ack *extack)
+{
+	return flow_action_hw_stats_types_check(action, extack, 0);
+}
+
 #define flow_action_for_each(__i, __act, __actions)			\
         for (__i = 0, __act = &(__actions)->entries[0]; __i < (__actions)->num_entries; __act = &(__actions)->entries[++__i])
 
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 79d9b4384d7b..fca9bfa8437e 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -865,6 +865,10 @@ static int dsa_slave_add_cls_matchall(struct net_device *dev,
 	if (!flow_offload_has_one_action(&cls->rule->action))
 		return err;
 
+	if (!flow_action_basic_hw_stats_types_check(&cls->rule->action,
+						    cls->common.extack))
+		return err;
+
 	act = &cls->rule->action.entries[0];
 
 	if (act->id == FLOW_ACTION_MIRRED && protocol == htons(ETH_P_ALL)) {
-- 
2.21.1

[patch net-next v4 04/10] mlxsw: spectrum_flower: Do not allow mixing HW stats types for actions

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

As there is one set of counters for the whole action chain, forbid to
mix the HW stats types.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
v1->v2:
- new patch
---
 drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c
index 0011a71114e3..7435629c9e65 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c
@@ -26,6 +26,8 @@ static int mlxsw_sp_flower_parse_actions(struct mlxsw_sp *mlxsw_sp,
 
 	if (!flow_action_has_entries(flow_action))
 		return 0;
+	if (!flow_action_mixed_hw_stats_types_check(flow_action, extack))
+		return -EOPNOTSUPP;
 
 	/* Count action is inserted first */
 	err = mlxsw_sp_acl_rulei_act_count(mlxsw_sp, rulei, extack);
-- 
2.21.1

[patch net-next v4 05/10] mlxsw: restrict supported HW stats type to "any"

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

Currently don't allow actions with any other type to be inserted.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
v2->v3:
- moved to bitfield
v1->v2:
- move the code to the first action processing
---
 .../net/ethernet/mellanox/mlxsw/spectrum_flower.c  | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c
index 7435629c9e65..588d374531cc 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c
@@ -29,10 +29,16 @@ static int mlxsw_sp_flower_parse_actions(struct mlxsw_sp *mlxsw_sp,
 	if (!flow_action_mixed_hw_stats_types_check(flow_action, extack))
 		return -EOPNOTSUPP;
 
-	/* Count action is inserted first */
-	err = mlxsw_sp_acl_rulei_act_count(mlxsw_sp, rulei, extack);
-	if (err)
-		return err;
+	act = flow_action_first_entry_get(flow_action);
+	if (act->hw_stats_type == FLOW_ACTION_HW_STATS_TYPE_ANY) {
+		/* Count action is inserted first */
+		err = mlxsw_sp_acl_rulei_act_count(mlxsw_sp, rulei, extack);
+		if (err)
+			return err;
+	} else {
+		NL_SET_ERR_MSG_MOD(extack, "Unsupported action HW stats type");
+		return -EOPNOTSUPP;
+	}
 
 	flow_action_for_each(i, act, flow_action) {
 		switch (act->id) {
-- 
2.21.1

[patch net-next v4 06/10] flow_offload: introduce "immediate" HW stats type and allow it in mlxsw

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

Introduce new type for immediate HW stats and allow the value in
mlxsw offload.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
v2->v3:
- moved to bitfield
v1->v2:
- moved to action
---
 drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c | 3 ++-
 include/net/flow_offload.h                            | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c
index 588d374531cc..4bf3ac1cb20d 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c
@@ -30,7 +30,8 @@ static int mlxsw_sp_flower_parse_actions(struct mlxsw_sp *mlxsw_sp,
 		return -EOPNOTSUPP;
 
 	act = flow_action_first_entry_get(flow_action);
-	if (act->hw_stats_type == FLOW_ACTION_HW_STATS_TYPE_ANY) {
+	if (act->hw_stats_type == FLOW_ACTION_HW_STATS_TYPE_ANY ||
+	    act->hw_stats_type == FLOW_ACTION_HW_STATS_TYPE_IMMEDIATE) {
 		/* Count action is inserted first */
 		err = mlxsw_sp_acl_rulei_act_count(mlxsw_sp, rulei, extack);
 		if (err)
diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
index 8b40f612a565..6580c58b368f 100644
--- a/include/net/flow_offload.h
+++ b/include/net/flow_offload.h
@@ -155,7 +155,8 @@ enum flow_action_mangle_base {
 	FLOW_ACT_MANGLE_HDR_TYPE_UDP,
 };
 
-#define FLOW_ACTION_HW_STATS_TYPE_ANY 0
+#define FLOW_ACTION_HW_STATS_TYPE_IMMEDIATE BIT(0)
+#define FLOW_ACTION_HW_STATS_TYPE_ANY FLOW_ACTION_HW_STATS_TYPE_IMMEDIATE
 
 typedef void (*action_destr)(void *priv);
 
-- 
2.21.1

[patch net-next v4 07/10] flow_offload: introduce "delayed" HW stats type and allow it in mlx5

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

Introduce new type for delayed HW stats and allow the value in
mlx5 offload.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
v2->v3:
- converted to newly introduced flow_action_hw_stats_types_check()
- moved to bitfield
v1->v2:
- moved to action
- fixed c&p error in patch description
---
 drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 6 ++++--
 include/net/flow_offload.h                      | 4 +++-
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
index cfe393cb4026..cdc63dd59867 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
@@ -2878,7 +2878,8 @@ static int parse_tc_nic_actions(struct mlx5e_priv *priv,
 	if (!flow_action_has_entries(flow_action))
 		return -EINVAL;
 
-	if (!flow_action_basic_hw_stats_types_check(flow_action, extack))
+	if (!flow_action_hw_stats_types_check(flow_action, extack,
+					      FLOW_ACTION_HW_STATS_TYPE_DELAYED))
 		return -EOPNOTSUPP;
 
 	attr->flow_tag = MLX5_FS_DEFAULT_FLOW_TAG;
@@ -3333,7 +3334,8 @@ static int parse_tc_fdb_actions(struct mlx5e_priv *priv,
 	if (!flow_action_has_entries(flow_action))
 		return -EINVAL;
 
-	if (!flow_action_basic_hw_stats_types_check(flow_action, extack))
+	if (!flow_action_hw_stats_types_check(flow_action, extack,
+					      FLOW_ACTION_HW_STATS_TYPE_DELAYED))
 		return -EOPNOTSUPP;
 
 	flow_action_for_each(i, act, flow_action) {
diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
index 6580c58b368f..1b6500f0fbca 100644
--- a/include/net/flow_offload.h
+++ b/include/net/flow_offload.h
@@ -156,7 +156,9 @@ enum flow_action_mangle_base {
 };
 
 #define FLOW_ACTION_HW_STATS_TYPE_IMMEDIATE BIT(0)
-#define FLOW_ACTION_HW_STATS_TYPE_ANY FLOW_ACTION_HW_STATS_TYPE_IMMEDIATE
+#define FLOW_ACTION_HW_STATS_TYPE_DELAYED BIT(1)
+#define FLOW_ACTION_HW_STATS_TYPE_ANY (FLOW_ACTION_HW_STATS_TYPE_IMMEDIATE | \
+				       FLOW_ACTION_HW_STATS_TYPE_DELAYED)
 
 typedef void (*action_destr)(void *priv);
 
-- 
2.21.1

[patch net-next v4 08/10] mlxsw: spectrum_acl: Ask device for rule stats only if counter was created

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

Set a flag in case rule counter was created. Only query the device for
stats of a rule, which has the valid counter assigned.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
v2->v3:
- init current values to 0 in case of disabled counters.
v1->v2:
- new patch
---
 .../net/ethernet/mellanox/mlxsw/spectrum.h    |  3 ++-
 .../ethernet/mellanox/mlxsw/spectrum_acl.c    | 26 ++++++++++++-------
 2 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.h b/drivers/net/ethernet/mellanox/mlxsw/spectrum.h
index ff61cad74bb0..81801c6fb941 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.h
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.h
@@ -641,7 +641,8 @@ struct mlxsw_sp_acl_rule_info {
 	struct mlxsw_afa_block *act_block;
 	u8 action_created:1,
 	   ingress_bind_blocker:1,
-	   egress_bind_blocker:1;
+	   egress_bind_blocker:1,
+	   counter_valid:1;
 	unsigned int counter_index;
 };
 
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_acl.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_acl.c
index 36b264798f04..6f8d5005ff36 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_acl.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_acl.c
@@ -642,8 +642,14 @@ int mlxsw_sp_acl_rulei_act_count(struct mlxsw_sp *mlxsw_sp,
 				 struct mlxsw_sp_acl_rule_info *rulei,
 				 struct netlink_ext_ack *extack)
 {
-	return mlxsw_afa_block_append_counter(rulei->act_block,
-					      &rulei->counter_index, extack);
+	int err;
+
+	err = mlxsw_afa_block_append_counter(rulei->act_block,
+					     &rulei->counter_index, extack);
+	if (err)
+		return err;
+	rulei->counter_valid = true;
+	return 0;
 }
 
 int mlxsw_sp_acl_rulei_act_fid_set(struct mlxsw_sp *mlxsw_sp,
@@ -857,16 +863,18 @@ int mlxsw_sp_acl_rule_get_stats(struct mlxsw_sp *mlxsw_sp,
 
 {
 	struct mlxsw_sp_acl_rule_info *rulei;
-	u64 current_packets;
-	u64 current_bytes;
+	u64 current_packets = 0;
+	u64 current_bytes = 0;
 	int err;
 
 	rulei = mlxsw_sp_acl_rule_rulei(rule);
-	err = mlxsw_sp_flow_counter_get(mlxsw_sp, rulei->counter_index,
-					&current_packets, &current_bytes);
-	if (err)
-		return err;
-
+	if (rulei->counter_valid) {
+		err = mlxsw_sp_flow_counter_get(mlxsw_sp, rulei->counter_index,
+						&current_packets,
+						&current_bytes);
+		if (err)
+			return err;
+	}
 	*packets = current_packets - rule->last_packets;
 	*bytes = current_bytes - rule->last_bytes;
 	*last_use = rule->last_used;
-- 
2.21.1

[patch net-next v4 09/10] flow_offload: introduce "disabled" HW stats type and allow it in mlxsw

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

Introduce new type for disabled HW stats and allow the value in
mlxsw offload.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
v2->v3:
- moved to bitfield
v1->v2:
- moved to action
---
 drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c | 2 +-
 include/net/flow_offload.h                            | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c
index 4bf3ac1cb20d..88aa554415df 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c
@@ -36,7 +36,7 @@ static int mlxsw_sp_flower_parse_actions(struct mlxsw_sp *mlxsw_sp,
 		err = mlxsw_sp_acl_rulei_act_count(mlxsw_sp, rulei, extack);
 		if (err)
 			return err;
-	} else {
+	} else if (act->hw_stats_type != FLOW_ACTION_HW_STATS_TYPE_DISABLED) {
 		NL_SET_ERR_MSG_MOD(extack, "Unsupported action HW stats type");
 		return -EOPNOTSUPP;
 	}
diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
index 1b6500f0fbca..64807aa03cee 100644
--- a/include/net/flow_offload.h
+++ b/include/net/flow_offload.h
@@ -159,6 +159,7 @@ enum flow_action_mangle_base {
 #define FLOW_ACTION_HW_STATS_TYPE_DELAYED BIT(1)
 #define FLOW_ACTION_HW_STATS_TYPE_ANY (FLOW_ACTION_HW_STATS_TYPE_IMMEDIATE | \
 				       FLOW_ACTION_HW_STATS_TYPE_DELAYED)
+#define FLOW_ACTION_HW_STATS_TYPE_DISABLED 0
 
 typedef void (*action_destr)(void *priv);
 
-- 
2.21.1

[patch net-next v4 10/10] sched: act: allow user to specify type of HW stats for a filter

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

Currently, user who is adding an action expects HW to report stats,
however it does not have exact expectations about the stats types.
That is aligned with TCA_ACT_HW_STATS_TYPE_ANY.

Allow user to specify the type of HW stats for an action and require it.

Pass the information down to flow_offload layer.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
v2->v3:
- removed "mixed" bool init
- moved to bitfield
- removed "inline"
v1->v2:
- moved the stats attr from cls_flower (filter) to any action
- rebased on top of cookie offload changes
- adjusted the patch description a bit
---
 include/net/act_api.h        |  4 ++++
 include/uapi/linux/pkt_cls.h | 22 ++++++++++++++++++++++
 net/sched/act_api.c          | 36 ++++++++++++++++++++++++++++++++++++
 net/sched/cls_api.c          |  7 +++++++
 4 files changed, 69 insertions(+)

diff --git a/include/net/act_api.h b/include/net/act_api.h
index 71347a90a9d1..41337c7fc728 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -41,6 +41,7 @@ struct tc_action {
 	struct tc_cookie	__rcu *act_cookie;
 	struct tcf_chain	__rcu *goto_chain;
 	u32			tcfa_flags;
+	u8			hw_stats_type;
 };
 #define tcf_index	common.tcfa_index
 #define tcf_refcnt	common.tcfa_refcnt
@@ -52,6 +53,9 @@ struct tc_action {
 #define tcf_rate_est	common.tcfa_rate_est
 #define tcf_lock	common.tcfa_lock
 
+#define TCA_ACT_HW_STATS_TYPE_ANY (TCA_ACT_HW_STATS_TYPE_IMMEDIATE | \
+				   TCA_ACT_HW_STATS_TYPE_DELAYED)
+
 /* Update lastuse only if needed, to avoid dirtying a cache line.
  * We use a temp variable to avoid fetching jiffies twice.
  */
diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
index 449a63971451..81cc1a869588 100644
--- a/include/uapi/linux/pkt_cls.h
+++ b/include/uapi/linux/pkt_cls.h
@@ -17,6 +17,7 @@ enum {
 	TCA_ACT_PAD,
 	TCA_ACT_COOKIE,
 	TCA_ACT_FLAGS,
+	TCA_ACT_HW_STATS_TYPE,
 	__TCA_ACT_MAX
 };
 
@@ -24,6 +25,27 @@ enum {
 					 * actions stats.
 					 */
 
+/* tca HW stats type
+ * When user does not pass the attribute, he does not care.
+ * It is the same as if he would pass the attribute with
+ * all supported bits set.
+ * In case no bits are set, user is not interested in getting any HW statistics.
+ */
+#define TCA_ACT_HW_STATS_TYPE_IMMEDIATE (1 << 0) /* Means that in dump, user
+						  * gets the current HW stats
+						  * state from the device
+						  * queried at the dump time.
+						  */
+#define TCA_ACT_HW_STATS_TYPE_DELAYED (1 << 1) /* Means that in dump, user gets
+						* HW stats that might be out
+						* of date for some time, maybe
+						* couple of seconds. This is
+						* the case when driver polls
+						* stats updates periodically
+						* or when it gets async stats update
+						* from the device.
+						*/
+
 #define TCA_ACT_MAX __TCA_ACT_MAX
 #define TCA_OLD_COMPAT (TCA_ACT_MAX+1)
 #define TCA_ACT_MAX_PRIO 32
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 8c466a712cda..aa7b737fed2e 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -185,6 +185,7 @@ static size_t tcf_action_shared_attrs_size(const struct tc_action *act)
 	return  nla_total_size(0) /* action number nested */
 		+ nla_total_size(IFNAMSIZ) /* TCA_ACT_KIND */
 		+ cookie_len /* TCA_ACT_COOKIE */
+		+ nla_total_size(sizeof(struct nla_bitfield32)) /* TCA_ACT_HW_STATS_TYPE */
 		+ nla_total_size(0) /* TCA_ACT_STATS nested */
 		+ nla_total_size(sizeof(struct nla_bitfield32)) /* TCA_ACT_FLAGS */
 		/* TCA_STATS_BASIC */
@@ -788,6 +789,17 @@ tcf_action_dump_1(struct sk_buff *skb, struct tc_action *a, int bind, int ref)
 	}
 	rcu_read_unlock();
 
+	if (a->hw_stats_type != TCA_ACT_HW_STATS_TYPE_ANY) {
+		struct nla_bitfield32 hw_stats_type = {
+			a->hw_stats_type,
+			TCA_ACT_HW_STATS_TYPE_ANY,
+		};
+
+		if (nla_put(skb, TCA_ACT_HW_STATS_TYPE, sizeof(hw_stats_type),
+			    &hw_stats_type))
+			goto nla_put_failure;
+	}
+
 	if (a->tcfa_flags) {
 		struct nla_bitfield32 flags = { a->tcfa_flags,
 						a->tcfa_flags, };
@@ -854,7 +866,23 @@ static struct tc_cookie *nla_memdup_cookie(struct nlattr **tb)
 	return c;
 }
 
+static u8 tcf_action_hw_stats_type_get(struct nlattr *hw_stats_type_attr)
+{
+	struct nla_bitfield32 hw_stats_type_bf;
+
+	/* If the user did not pass the attr, that means he does
+	 * not care about the type. Return "any" in that case
+	 * which is setting on all supported types.
+	 */
+	if (!hw_stats_type_attr)
+		return TCA_ACT_HW_STATS_TYPE_ANY;
+	hw_stats_type_bf = nla_get_bitfield32(hw_stats_type_attr);
+	return hw_stats_type_bf.value;
+}
+
 static const u32 tca_act_flags_allowed = TCA_ACT_FLAGS_NO_PERCPU_STATS;
+static const u32 tca_act_hw_stats_type_allowed = TCA_ACT_HW_STATS_TYPE_ANY;
+
 static const struct nla_policy tcf_action_policy[TCA_ACT_MAX + 1] = {
 	[TCA_ACT_KIND]		= { .type = NLA_STRING },
 	[TCA_ACT_INDEX]		= { .type = NLA_U32 },
@@ -863,6 +891,8 @@ static const struct nla_policy tcf_action_policy[TCA_ACT_MAX + 1] = {
 	[TCA_ACT_OPTIONS]	= { .type = NLA_NESTED },
 	[TCA_ACT_FLAGS]		= { .type = NLA_BITFIELD32,
 				    .validation_data = &tca_act_flags_allowed },
+	[TCA_ACT_HW_STATS_TYPE]	= { .type = NLA_BITFIELD32,
+				    .validation_data = &tca_act_hw_stats_type_allowed },
 };
 
 struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
@@ -871,6 +901,7 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
 				    bool rtnl_held,
 				    struct netlink_ext_ack *extack)
 {
+	u8 hw_stats_type = TCA_ACT_HW_STATS_TYPE_ANY;
 	struct nla_bitfield32 flags = { 0, 0 };
 	struct tc_action *a;
 	struct tc_action_ops *a_o;
@@ -903,6 +934,8 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
 				goto err_out;
 			}
 		}
+		hw_stats_type =
+			tcf_action_hw_stats_type_get(tb[TCA_ACT_HW_STATS_TYPE]);
 		if (tb[TCA_ACT_FLAGS])
 			flags = nla_get_bitfield32(tb[TCA_ACT_FLAGS]);
 	} else {
@@ -953,6 +986,9 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
 	if (!name && tb[TCA_ACT_COOKIE])
 		tcf_set_action_cookie(&a->act_cookie, cookie);
 
+	if (!name)
+		a->hw_stats_type = hw_stats_type;
+
 	/* module count goes up only when brand new policy is created
 	 * if it exists and is only bound to in a_o->init() then
 	 * ACT_P_CREATED is not returned (a zero is).
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 4e766c5ab77a..e91448640a4f 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -3464,6 +3464,10 @@ int tc_setup_flow_action(struct flow_action *flow_action,
 	struct tc_action *act;
 	int i, j, k, err = 0;
 
+	BUILD_BUG_ON(TCA_ACT_HW_STATS_TYPE_ANY != FLOW_ACTION_HW_STATS_TYPE_ANY);
+	BUILD_BUG_ON(TCA_ACT_HW_STATS_TYPE_IMMEDIATE != FLOW_ACTION_HW_STATS_TYPE_IMMEDIATE);
+	BUILD_BUG_ON(TCA_ACT_HW_STATS_TYPE_DELAYED != FLOW_ACTION_HW_STATS_TYPE_DELAYED);
+
 	if (!exts)
 		return 0;
 
@@ -3476,6 +3480,9 @@ int tc_setup_flow_action(struct flow_action *flow_action,
 		err = tcf_act_get_cookie(entry, act);
 		if (err)
 			goto err_out_locked;
+
+		entry->hw_stats_type = act->hw_stats_type;
+
 		if (is_tcf_gact_ok(act)) {
 			entry->id = FLOW_ACTION_ACCEPT;
 		} else if (is_tcf_gact_shot(act)) {
-- 
2.21.1

Re: [patch net-next v4 00/10] net: allow user specify TC action HW stats type

[David Miller]

From: Jiri Pirko <jiri@resnulli.us>
Date: Sat,  7 Mar 2020 12:40:10 +0100

> Currently, when user adds a TC action and the action gets offloaded,
> the user expects the HW stats to be counted and included in stats dump.
> However, since drivers may implement different types of counting, there
> is no way to specify which one the user is interested in.
> 
> For example for mlx5, only delayed counters are available as the driver
> periodically polls for updated stats.
> 
> In case of mlxsw, the counters are queried on dump time. However, the
> HW resources for this type of counters is quite limited (couple of
> thousands). This limits the amount of supported offloaded filters
> significantly. Without counter assigned, the HW is capable to carry
> millions of those.
> 
> On top of that, mlxsw HW is able to support delayed counters as well in
> greater numbers. That is going to be added in a follow-up patch.
> 
> This patchset allows user to specify one of the following types of HW
> stats for added action:
> immediate - queried during dump time
> delayed - polled from HW periodically or sent by HW in async manner
> disabled - no stats needed
> 
> Note that if "hw_stats" option is not passed, user does not care about
> the type, just expects any type of stats.
> 
> Examples:
 ...

Series applied, thanks Jiri.

RE: [patch net-next v4 03/10] flow_offload: check for basic action hw stats type

[Jose Abreu]

From: Jiri Pirko <jiri@resnulli.us>
Date: Mar/07/2020, 11:40:13 (UTC+00:00)

> Introduce flow_action_basic_hw_stats_types_check() helper and use it
> in drivers. That sanitizes the drivers which do not have support
> for action HW stats types.

Next time please cc driver maintainers because this broke L3/L4 selftests 
for stmmac.

Not to worry, I already have a patch to send :)

---
Thanks,
Jose Miguel Abreu

Re: [patch net-next v4 03/10] flow_offload: check for basic action hw stats type

[Jiri Pirko]

Mon, Mar 09, 2020 at 11:23:41AM CET, Jose.Abreu@synopsys.com wrote:
>From: Jiri Pirko <jiri@resnulli.us>
>Date: Mar/07/2020, 11:40:13 (UTC+00:00)
>
>> Introduce flow_action_basic_hw_stats_types_check() helper and use it
>> in drivers. That sanitizes the drivers which do not have support
>> for action HW stats types.
>
>Next time please cc driver maintainers because this broke L3/L4 selftests 
>for stmmac.

How exactly? This should not have any change for the existing users.


>
>Not to worry, I already have a patch to send :)
>
>---
>Thanks,
>Jose Miguel Abreu

RE: [patch net-next v4 03/10] flow_offload: check for basic action hw stats type

[Jose Abreu]

From: Jiri Pirko <jiri@resnulli.us>
Date: Mar/09/2020, 12:04:53 (UTC+00:00)

> How exactly? This should not have any change for the existing users.

The fix is here: https://patchwork.ozlabs.org/patch/1251573/

---
Thanks,
Jose Miguel Abreu

Re: [patch net-next v4 03/10] flow_offload: check for basic action hw stats type

[Jiri Pirko]

Mon, Mar 09, 2020 at 02:31:40PM CET, Jose.Abreu@synopsys.com wrote:
>From: Jiri Pirko <jiri@resnulli.us>
>Date: Mar/09/2020, 12:04:53 (UTC+00:00)
>
>> How exactly? This should not have any change for the existing users.
>
>The fix is here: https://patchwork.ozlabs.org/patch/1251573/

Ah! in-kernel selftest. Okay. That didn't occur to me. Thanks!


>
>---
>Thanks,
>Jose Miguel Abreu

Re: [patch net-next v4 01/10] flow_offload: Introduce offload of HW stats type

[Edward Cree]

On 07/03/2020 11:40, Jiri Pirko wrote:
> From: Jiri Pirko <jiri@mellanox.com>
>
> Initially, pass "ANY" (struct is zeroed) to the drivers as that is the
> current implicit value coming down to flow_offload. Add a bool
> indicating that entries have mixed HW stats type.
>
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
> ---
> v3->v4:
> - fixed member alignment
> v2->v3:
> - moved to bitfield
> - removed "mixed" bool
> v1->v2:
> - moved to actions
> - add mixed bool
> ---
>  include/net/flow_offload.h | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
> index cd3510ac66b0..93d17f37e980 100644
> --- a/include/net/flow_offload.h
> +++ b/include/net/flow_offload.h
> @@ -154,6 +154,8 @@ enum flow_action_mangle_base {
>  	FLOW_ACT_MANGLE_HDR_TYPE_UDP,
>  };
>  
> +#define FLOW_ACTION_HW_STATS_TYPE_ANY 0
I'm not quite sure why switching to a bit fieldapproach means these
 haveto become #defines rather than enums...

> +
>  typedef void (*action_destr)(void *priv);
>  
>  struct flow_action_cookie {
> @@ -168,6 +170,7 @@ void flow_action_cookie_destroy(struct flow_action_cookie *cookie);
>  
>  struct flow_action_entry {
>  	enum flow_action_id		id;
> +	u8				hw_stats_type;
... causing this to become a u8with nothing obviously preventing
 a HW_STATS_TYPE bigger than 255 getting defined.
An enum type seems safer.

-ed

>  	action_destr			destructor;
>  	void				*destructor_priv;
>  	union {

Re: [patch net-next v4 03/10] flow_offload: check for basic action hw stats type

[Edward Cree]

On 07/03/2020 11:40, Jiri Pirko wrote:
> From: Jiri Pirko <jiri@mellanox.com>
>
> Introduce flow_action_basic_hw_stats_types_check() helper and use it
> in drivers. That sanitizes the drivers which do not have support
> for action HW stats types.
>
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
> ---
> v3->v4:
> - fixed entries iteration in check (s/0/i/)
> - compare allowed type explicitly to 0 to avoid confusion
> v2->v3:
> - added flow_action_hw_stats_types_check() to pass allowed types
> - "mixed" bool got remove, iterate entries in check
> - added mlx5 checking instead of separate patches (will be changed by
>   later patch to flow_action_hw_stats_types_check()
> v1->v2:
> - new patch
> ---
> <snip>
> diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
> index 93d17f37e980..8b40f612a565 100644
> --- a/include/net/flow_offload.h
> +++ b/include/net/flow_offload.h
> @@ -3,6 +3,7 @@
>  
>  #include <linux/kernel.h>
>  #include <linux/list.h>
> +#include <linux/netlink.h>
>  #include <net/flow_dissector.h>
>  #include <linux/rhashtable.h>
>  
> @@ -251,6 +252,66 @@ static inline bool flow_offload_has_one_action(const struct flow_action *action)
>  	return action->num_entries == 1;
>  }
>  
> +static inline bool
> +flow_action_mixed_hw_stats_types_check(const struct flow_action *action,
> +				       struct netlink_ext_ack *extack)
> +{
> +	const struct flow_action_entry *action_entry;
> +	u8 uninitialized_var(last_hw_stats_type);
> +	int i;
> +
> +	if (flow_offload_has_one_action(action))
> +		return true;
> +
> +	for (i = 0; i < action->num_entries; i++) {
Any reason you didn't use flow_action_for_each() here?
-ed

> +		action_entry = &action->entries[i];
> +		if (i && action_entry->hw_stats_type != last_hw_stats_type) {
> +			NL_SET_ERR_MSG_MOD(extack, "Mixing HW stats types for actions is not supported");
> +			return false;
> +		}
> +		last_hw_stats_type = action_entry->hw_stats_type;
> +	}
> +	return true;
> +}
> +
> +static inline const struct flow_action_entry *
> +flow_action_first_entry_get(const struct flow_action *action)
> +{
> +	WARN_ON(!flow_action_has_entries(action));
> +	return &action->entries[0];
> +}
> +
> +static inline bool
> +flow_action_hw_stats_types_check(const struct flow_action *action,
> +				 struct netlink_ext_ack *extack,
> +				 u8 allowed_hw_stats_type)
> +{
> +	const struct flow_action_entry *action_entry;
> +
> +	if (!flow_action_has_entries(action))
> +		return true;
> +	if (!flow_action_mixed_hw_stats_types_check(action, extack))
> +		return false;
> +	action_entry = flow_action_first_entry_get(action);
> +	if (allowed_hw_stats_type == 0 &&
> +	    action_entry->hw_stats_type != FLOW_ACTION_HW_STATS_TYPE_ANY) {
> +		NL_SET_ERR_MSG_MOD(extack, "Driver supports only default HW stats type \"any\"");
> +		return false;
> +	} else if (allowed_hw_stats_type != 0 &&
> +		   action_entry->hw_stats_type != allowed_hw_stats_type) {
> +		NL_SET_ERR_MSG_MOD(extack, "Driver does not support selected HW stats type");
> +		return false;
> +	}
> +	return true;
> +}
> +
> +static inline bool
> +flow_action_basic_hw_stats_types_check(const struct flow_action *action,
> +				       struct netlink_ext_ack *extack)
> +{
> +	return flow_action_hw_stats_types_check(action, extack, 0);
> +}
> +
>  #define flow_action_for_each(__i, __act, __actions)			\
>          for (__i = 0, __act = &(__actions)->entries[0]; __i < (__actions)->num_entries; __act = &(__actions)->entries[++__i])
>  
> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> index 79d9b4384d7b..fca9bfa8437e 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -865,6 +865,10 @@ static int dsa_slave_add_cls_matchall(struct net_device *dev,
>  	if (!flow_offload_has_one_action(&cls->rule->action))
>  		return err;
>  
> +	if (!flow_action_basic_hw_stats_types_check(&cls->rule->action,
> +						    cls->common.extack))
> +		return err;
> +
>  	act = &cls->rule->action.entries[0];
>  
>  	if (act->id == FLOW_ACTION_MIRRED && protocol == htons(ETH_P_ALL)) {

Re: [patch net-next v4 03/10] flow_offload: check for basic action hw stats type

[Jiri Pirko]

Mon, Mar 09, 2020 at 05:52:47PM CET, ecree@solarflare.com wrote:
>On 07/03/2020 11:40, Jiri Pirko wrote:
>> From: Jiri Pirko <jiri@mellanox.com>
>>
>> Introduce flow_action_basic_hw_stats_types_check() helper and use it
>> in drivers. That sanitizes the drivers which do not have support
>> for action HW stats types.
>>
>> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
>> ---
>> v3->v4:
>> - fixed entries iteration in check (s/0/i/)
>> - compare allowed type explicitly to 0 to avoid confusion
>> v2->v3:
>> - added flow_action_hw_stats_types_check() to pass allowed types
>> - "mixed" bool got remove, iterate entries in check
>> - added mlx5 checking instead of separate patches (will be changed by
>>   later patch to flow_action_hw_stats_types_check()
>> v1->v2:
>> - new patch
>> ---
>> <snip>
>> diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
>> index 93d17f37e980..8b40f612a565 100644
>> --- a/include/net/flow_offload.h
>> +++ b/include/net/flow_offload.h
>> @@ -3,6 +3,7 @@
>>  
>>  #include <linux/kernel.h>
>>  #include <linux/list.h>
>> +#include <linux/netlink.h>
>>  #include <net/flow_dissector.h>
>>  #include <linux/rhashtable.h>
>>  
>> @@ -251,6 +252,66 @@ static inline bool flow_offload_has_one_action(const struct flow_action *action)
>>  	return action->num_entries == 1;
>>  }
>>  
>> +static inline bool
>> +flow_action_mixed_hw_stats_types_check(const struct flow_action *action,
>> +				       struct netlink_ext_ack *extack)
>> +{
>> +	const struct flow_action_entry *action_entry;
>> +	u8 uninitialized_var(last_hw_stats_type);
>> +	int i;
>> +
>> +	if (flow_offload_has_one_action(action))
>> +		return true;
>> +
>> +	for (i = 0; i < action->num_entries; i++) {
>Any reason you didn't use flow_action_for_each() here?

Nope. Will send a follow-up to change this.


>-ed
>
>> +		action_entry = &action->entries[i];
>> +		if (i && action_entry->hw_stats_type != last_hw_stats_type) {
>> +			NL_SET_ERR_MSG_MOD(extack, "Mixing HW stats types for actions is not supported");
>> +			return false;
>> +		}
>> +		last_hw_stats_type = action_entry->hw_stats_type;
>> +	}
>> +	return true;
>> +}
>> +
>> +static inline const struct flow_action_entry *
>> +flow_action_first_entry_get(const struct flow_action *action)
>> +{
>> +	WARN_ON(!flow_action_has_entries(action));
>> +	return &action->entries[0];
>> +}
>> +
>> +static inline bool
>> +flow_action_hw_stats_types_check(const struct flow_action *action,
>> +				 struct netlink_ext_ack *extack,
>> +				 u8 allowed_hw_stats_type)
>> +{
>> +	const struct flow_action_entry *action_entry;
>> +
>> +	if (!flow_action_has_entries(action))
>> +		return true;
>> +	if (!flow_action_mixed_hw_stats_types_check(action, extack))
>> +		return false;
>> +	action_entry = flow_action_first_entry_get(action);
>> +	if (allowed_hw_stats_type == 0 &&
>> +	    action_entry->hw_stats_type != FLOW_ACTION_HW_STATS_TYPE_ANY) {
>> +		NL_SET_ERR_MSG_MOD(extack, "Driver supports only default HW stats type \"any\"");
>> +		return false;
>> +	} else if (allowed_hw_stats_type != 0 &&
>> +		   action_entry->hw_stats_type != allowed_hw_stats_type) {
>> +		NL_SET_ERR_MSG_MOD(extack, "Driver does not support selected HW stats type");
>> +		return false;
>> +	}
>> +	return true;
>> +}
>> +
>> +static inline bool
>> +flow_action_basic_hw_stats_types_check(const struct flow_action *action,
>> +				       struct netlink_ext_ack *extack)
>> +{
>> +	return flow_action_hw_stats_types_check(action, extack, 0);
>> +}
>> +
>>  #define flow_action_for_each(__i, __act, __actions)			\
>>          for (__i = 0, __act = &(__actions)->entries[0]; __i < (__actions)->num_entries; __act = &(__actions)->entries[++__i])
>>  
>> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
>> index 79d9b4384d7b..fca9bfa8437e 100644
>> --- a/net/dsa/slave.c
>> +++ b/net/dsa/slave.c
>> @@ -865,6 +865,10 @@ static int dsa_slave_add_cls_matchall(struct net_device *dev,
>>  	if (!flow_offload_has_one_action(&cls->rule->action))
>>  		return err;
>>  
>> +	if (!flow_action_basic_hw_stats_types_check(&cls->rule->action,
>> +						    cls->common.extack))
>> +		return err;
>> +
>>  	act = &cls->rule->action.entries[0];
>>  
>>  	if (act->id == FLOW_ACTION_MIRRED && protocol == htons(ETH_P_ALL)) {
>

Re: [patch net-next v4 01/10] flow_offload: Introduce offload of HW stats type

[Jiri Pirko]

Mon, Mar 09, 2020 at 05:52:16PM CET, ecree@solarflare.com wrote:
>On 07/03/2020 11:40, Jiri Pirko wrote:
>> From: Jiri Pirko <jiri@mellanox.com>
>>
>> Initially, pass "ANY" (struct is zeroed) to the drivers as that is the
>> current implicit value coming down to flow_offload. Add a bool
>> indicating that entries have mixed HW stats type.
>>
>> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
>> ---
>> v3->v4:
>> - fixed member alignment
>> v2->v3:
>> - moved to bitfield
>> - removed "mixed" bool
>> v1->v2:
>> - moved to actions
>> - add mixed bool
>> ---
>>  include/net/flow_offload.h | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
>> index cd3510ac66b0..93d17f37e980 100644
>> --- a/include/net/flow_offload.h
>> +++ b/include/net/flow_offload.h
>> @@ -154,6 +154,8 @@ enum flow_action_mangle_base {
>>  	FLOW_ACT_MANGLE_HDR_TYPE_UDP,
>>  };
>>  
>> +#define FLOW_ACTION_HW_STATS_TYPE_ANY 0
>I'm not quite sure why switching to a bit fieldapproach means these
> haveto become #defines rather than enums...
>
>> +
>>  typedef void (*action_destr)(void *priv);
>>  
>>  struct flow_action_cookie {
>> @@ -168,6 +170,7 @@ void flow_action_cookie_destroy(struct flow_action_cookie *cookie);
>>  
>>  struct flow_action_entry {
>>  	enum flow_action_id		id;
>> +	u8				hw_stats_type;
>... causing this to become a u8with nothing obviously preventing
> a HW_STATS_TYPE bigger than 255 getting defined.

I don't follow.


>An enum type seems safer.

Well, it's is a bitfield, how do you envision to implement it. Have enum
value for every combination? I don't get it.


>
>-ed
>
>>  	action_destr			destructor;
>>  	void				*destructor_priv;
>>  	union {
>

Re: [patch net-next v4 01/10] flow_offload: Introduce offload of HW stats type

[Edward Cree]

On 09/03/2020 17:34, Jiri Pirko wrote:
> Mon, Mar 09, 2020 at 05:52:16PM CET, ecree@solarflare.com wrote:
>> An enum type seems safer.
> Well, it's is a bitfield, how do you envision to implement it. Have enum
> value for every combination? I don't get it.
enum flow_action_stats_type {
    FLOW_ACTION_HW_STATS_TYPE_DISABLED=0,
    FLOW_ACTION_HW_STATS_TYPE_IMMEDIATE=BIT(0),
    FLOW_ACTION_HW_STATS_TYPE_DELAYED=BIT(1),
    FLOW_ACTION_HW_STATS_TYPE_ANY=(FLOW_ACTION_HW_STATS_TYPE_IMMEDIATE |
                                   FLOW_ACTION_HW_STATS_TYPE_DELAYED),
};

It's not a requirement of the language for every value used withan
 enumeration to be a defined enumerator value, so if someone ends up
 putting (FLOW_ACTION_HW_STATS_TYPE_FOO | FLOW_ACTION_HW_STATS_TYPE_BAR)
 into (say) a driver that supports only FOO and BAR, that will work
 just fine.  I don't see what problem you expect to occur here.

-ed

Re: [patch net-next v4 01/10] flow_offload: Introduce offload of HW stats type

[Jakub Kicinski]

On Mon, 9 Mar 2020 16:52:16 +0000 Edward Cree wrote:
> On 07/03/2020 11:40, Jiri Pirko wrote:
> > From: Jiri Pirko <jiri@mellanox.com>
> >
> > Initially, pass "ANY" (struct is zeroed) to the drivers as that is the
> > current implicit value coming down to flow_offload. Add a bool
> > indicating that entries have mixed HW stats type.
> >
> > Signed-off-by: Jiri Pirko <jiri@mellanox.com>
> > ---
> > v3->v4:
> > - fixed member alignment
> > v2->v3:
> > - moved to bitfield
> > - removed "mixed" bool
> > v1->v2:
> > - moved to actions
> > - add mixed bool
> > ---
> >  include/net/flow_offload.h | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
> > index cd3510ac66b0..93d17f37e980 100644
> > --- a/include/net/flow_offload.h
> > +++ b/include/net/flow_offload.h
> > @@ -154,6 +154,8 @@ enum flow_action_mangle_base {
> >  	FLOW_ACT_MANGLE_HDR_TYPE_UDP,
> >  };
> >  
> > +#define FLOW_ACTION_HW_STATS_TYPE_ANY 0  
> I'm not quite sure why switching to a bit fieldapproach means these
>  haveto become #defines rather than enums...

Perhaps having enum defining the values could be argued...

> > +
> >  typedef void (*action_destr)(void *priv);
> >  
> >  struct flow_action_cookie {
> > @@ -168,6 +170,7 @@ void flow_action_cookie_destroy(struct flow_action_cookie *cookie);
> >  
> >  struct flow_action_entry {
> >  	enum flow_action_id		id;
> > +	u8				hw_stats_type;  
> ... causing this to become a u8with nothing obviously preventing
>  a HW_STATS_TYPE bigger than 255 getting defined.
> An enum type seems safer.

...but using the type for fields which are very likely to contain
values from outside of the enumeration seems confusing, IMHO.

Driver author can understandably try to simply handle all the values 
in a switch statement and be unpleasantly surprised.

Re: [patch net-next v4 01/10] flow_offload: Introduce offload of HW stats type

[Edward Cree]

On 09/03/2020 21:36, Jakub Kicinski wrote:
> ...but using the type for fields which are very likely to contain
> values from outside of the enumeration seems confusing, IMHO.
It's the only way I can see to (a) ensure that the field is the correct
 size and (b) document the connection between the field and the type.

Now, I'm not saying that using the enums actually buys any real type-
 safety — the C standard enums are far too weak for that, being
 basically ints with hair — but the mythical Sufficiently Smart
 Compiler might be able to get some mileage out of it to issue better
 diagnostics.

> Driver author can understandably try to simply handle all the values 
> in a switch statement and be unpleasantly surprised.
In my experience, unenumerated enum values of this kind are fully
 idiomatic C; and a driver author looking at the header file to see
 what enumeration constants are defined will necessarily see all the
 calls to BIT() and the bitwise-or construction of _ANY.
I'm not sure I believe a naïve switch() implementation is an
 "understandable" error.

How about if we also rename the field "hw_stats_types", or squeeze
 the word "mask" in there somewhere?

-ed

Re: [patch net-next v4 01/10] flow_offload: Introduce offload of HW stats type

[Jakub Kicinski]

On Mon, 9 Mar 2020 22:27:59 +0000 Edward Cree wrote:
> > Driver author can understandably try to simply handle all the values 
> > in a switch statement and be unpleasantly surprised.  
> In my experience, unenumerated enum values of this kind are fully
>  idiomatic C; and a driver author looking at the header file to see
>  what enumeration constants are defined will necessarily see all the
>  calls to BIT() and the bitwise-or construction of _ANY.
> I'm not sure I believe a naïve switch() implementation is an
>  "understandable" error.

Could be my slight exposure to HDLs that makes me strongly averse to
encoding state outside of a enumeration into a value of that type :)

> How about if we also rename the field "hw_stats_types", or squeeze
>  the word "mask" in there somewhere?

That'd make things slightly better.

Re: [patch net-next v4 01/10] flow_offload: Introduce offload of HW stats type

[Jiri Pirko]

Mon, Mar 09, 2020 at 06:46:57PM CET, ecree@solarflare.com wrote:
>On 09/03/2020 17:34, Jiri Pirko wrote:
>> Mon, Mar 09, 2020 at 05:52:16PM CET, ecree@solarflare.com wrote:
>>> An enum type seems safer.
>> Well, it's is a bitfield, how do you envision to implement it. Have enum
>> value for every combination? I don't get it.
>enum flow_action_stats_type {
>    FLOW_ACTION_HW_STATS_TYPE_DISABLED=0,
>    FLOW_ACTION_HW_STATS_TYPE_IMMEDIATE=BIT(0),
>    FLOW_ACTION_HW_STATS_TYPE_DELAYED=BIT(1),
>    FLOW_ACTION_HW_STATS_TYPE_ANY=(FLOW_ACTION_HW_STATS_TYPE_IMMEDIATE |
>                                   FLOW_ACTION_HW_STATS_TYPE_DELAYED),
>};
>
>It's not a requirement of the language for every value used withan
> enumeration to be a defined enumerator value, so if someone ends up
> putting (FLOW_ACTION_HW_STATS_TYPE_FOO | FLOW_ACTION_HW_STATS_TYPE_BAR)
> into (say) a driver that supports only FOO and BAR, that will work
> just fine.  I don't see what problem you expect to occur here.

Okay. Will do. Thanks!


>
>-ed

Re: [patch net-next v4 01/10] flow_offload: Introduce offload of HW stats type

[Jiri Pirko]

Mon, Mar 09, 2020 at 11:38:17PM CET, kuba@kernel.org wrote:
>On Mon, 9 Mar 2020 22:27:59 +0000 Edward Cree wrote:
>> > Driver author can understandably try to simply handle all the values 
>> > in a switch statement and be unpleasantly surprised.  
>> In my experience, unenumerated enum values of this kind are fully
>>  idiomatic C; and a driver author looking at the header file to see
>>  what enumeration constants are defined will necessarily see all the
>>  calls to BIT() and the bitwise-or construction of _ANY.
>> I'm not sure I believe a naïve switch() implementation is an
>>  "understandable" error.
>
>Could be my slight exposure to HDLs that makes me strongly averse to
>encoding state outside of a enumeration into a value of that type :)
>
>> How about if we also rename the field "hw_stats_types", or squeeze
>>  the word "mask" in there somewhere?
>
>That'd make things slightly better.

It is not a mask though...