[PATCH net-next 0/8] use tc_cls_can_offload_and_chain0() throughout the drivers

[PATCH net-next 0/8] use tc_cls_can_offload_and_chain0() throughout the drivers

[Jakub Kicinski]

Hi!

This set makes most drivers use a new tc_cls_can_offload_and_chain0()
helper which will set extack in case TC hw offload flag is disabled.
i40e patch will follow after net -> net-next merge.

I chose to keep the new helper which also looks at the chain but
renamed it more appropriately.  The rationale being that most drivers
don't accept chains other than 0 and since we have to pass extack
to the helper we can as well pass the entire struct tc_cls_common_offload
and perform the most common checks.  Jiri, please let me know if that's
acceptable for you.

This code makes the assumption that type_data in the callback can
be interpreted as struct tc_cls_common_offload, i.e. the real offload
structure has common part as the first member.  This allows us to
make the check once for all classifier types if driver supports
more than one.  This also means I've dropped the last patch of
the RFC (preventing use of common before type validation in nfp).

Jakub Kicinski (8):
  pkt_cls: add new tc cls helper to check offload flag and chain index
  bnxt: use tc_cls_can_offload_and_chain0()
  nfp: flower: use tc_cls_can_offload_and_chain0()
  cxgb4: use tc_cls_can_offload_and_chain0()
  ixgbe: use tc_cls_can_offload_and_chain0()
  mlx5: use tc_cls_can_offload_and_chain0()
  mlxsw: use tc_cls_can_offload_and_chain0()
  selftests/bpf: check for spurious extacks from the driver

 drivers/net/ethernet/broadcom/bnxt/bnxt.c          |  3 ++-
 drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c       |  3 ---
 drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c      |  3 ++-
 drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c    |  8 +------
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c      |  5 +---
 drivers/net/ethernet/mellanox/mlx5/core/en_main.c  |  5 +---
 drivers/net/ethernet/mellanox/mlx5/core/en_rep.c   |  5 +---
 drivers/net/ethernet/mellanox/mlxsw/spectrum.c     |  6 ++---
 drivers/net/ethernet/netronome/nfp/bpf/main.c      |  4 +---
 .../net/ethernet/netronome/nfp/flower/offload.c    |  7 +++---
 drivers/net/netdevsim/bpf.c                        |  5 +---
 include/net/pkt_cls.h                              | 12 ++++++++++
 tools/testing/selftests/bpf/test_offload.py        | 27 ++++++++++++++++++++++
 13 files changed, 54 insertions(+), 39 deletions(-)

-- 
2.15.1

[PATCH net-next 1/8] pkt_cls: add new tc cls helper to check offload flag and chain index

[Jakub Kicinski]

Very few (mlxsw) upstream drivers seem to allow offload of chains
other than 0.  Save driver developers typing and add a helper for
checking both if ethtool's TC offload flag is on and if chain is 0.
This helper will set the extack appropriately in both error cases.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Simon Horman <simon.horman@netronome.com>
---
 drivers/net/ethernet/netronome/nfp/bpf/main.c |  4 +---
 drivers/net/netdevsim/bpf.c                   |  5 +----
 include/net/pkt_cls.h                         | 12 ++++++++++++
 3 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/netronome/nfp/bpf/main.c b/drivers/net/ethernet/netronome/nfp/bpf/main.c
index b3206855535a..322027792fe8 100644
--- a/drivers/net/ethernet/netronome/nfp/bpf/main.c
+++ b/drivers/net/ethernet/netronome/nfp/bpf/main.c
@@ -130,7 +130,7 @@ static int nfp_bpf_setup_tc_block_cb(enum tc_setup_type type,
 				   "only offload of BPF classifiers supported");
 		return -EOPNOTSUPP;
 	}
-	if (!tc_can_offload_extack(nn->dp.netdev, cls_bpf->common.extack))
+	if (!tc_cls_can_offload_and_chain0(nn->dp.netdev, &cls_bpf->common))
 		return -EOPNOTSUPP;
 	if (!nfp_net_ebpf_capable(nn)) {
 		NL_SET_ERR_MSG_MOD(cls_bpf->common.extack,
@@ -142,8 +142,6 @@ static int nfp_bpf_setup_tc_block_cb(enum tc_setup_type type,
 				   "only ETH_P_ALL supported as filter protocol");
 		return -EOPNOTSUPP;
 	}
-	if (cls_bpf->common.chain_index)
-		return -EOPNOTSUPP;
 
 	/* Only support TC direct action */
 	if (!cls_bpf->exts_integrated ||
diff --git a/drivers/net/netdevsim/bpf.c b/drivers/net/netdevsim/bpf.c
index 8166f121bbcc..de73c1ff0939 100644
--- a/drivers/net/netdevsim/bpf.c
+++ b/drivers/net/netdevsim/bpf.c
@@ -135,7 +135,7 @@ int nsim_bpf_setup_tc_block_cb(enum tc_setup_type type,
 		return -EOPNOTSUPP;
 	}
 
-	if (!tc_can_offload_extack(ns->netdev, cls_bpf->common.extack))
+	if (!tc_cls_can_offload_and_chain0(ns->netdev, &cls_bpf->common))
 		return -EOPNOTSUPP;
 
 	if (cls_bpf->common.protocol != htons(ETH_P_ALL)) {
@@ -144,9 +144,6 @@ int nsim_bpf_setup_tc_block_cb(enum tc_setup_type type,
 		return -EOPNOTSUPP;
 	}
 
-	if (cls_bpf->common.chain_index)
-		return -EOPNOTSUPP;
-
 	if (!ns->bpf_tc_accept) {
 		NSIM_EA(cls_bpf->common.extack,
 			"netdevsim configured to reject BPF TC offload");
diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index 1a41513cec7f..4db08d7dd22c 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -656,6 +656,18 @@ static inline bool tc_can_offload_extack(const struct net_device *dev,
 	return can;
 }
 
+static inline bool
+tc_cls_can_offload_and_chain0(const struct net_device *dev,
+			      struct tc_cls_common_offload *common)
+{
+	if (common->chain_index) {
+		NL_SET_ERR_MSG(common->extack,
+			       "Driver supports only offload of chain 0");
+		return false;
+	}
+	return tc_can_offload_extack(dev, common->extack);
+}
+
 static inline bool tc_skip_hw(u32 flags)
 {
 	return (flags & TCA_CLS_FLAGS_SKIP_HW) ? true : false;
-- 
2.15.1

[PATCH net-next 2/8] cxgb4: use tc_cls_can_offload_and_chain0()

[Jakub Kicinski]

Make use of tc_cls_can_offload_and_chain0() to set extack msg in case
ethtool tc offload flag is not set or chain unsupported.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Simon Horman <simon.horman@netronome.com>
---
CC: Ganesh Goudar <ganeshgr@chelsio.com>
---
 drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
index f0fd2eba30c2..1e3cd8abc56d 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
@@ -2928,9 +2928,6 @@ static int cxgb_set_tx_maxrate(struct net_device *dev, int index, u32 rate)
 static int cxgb_setup_tc_flower(struct net_device *dev,
 				struct tc_cls_flower_offload *cls_flower)
 {
-	if (cls_flower->common.chain_index)
-		return -EOPNOTSUPP;
-
 	switch (cls_flower->command) {
 	case TC_CLSFLOWER_REPLACE:
 		return cxgb4_tc_flower_replace(dev, cls_flower);
@@ -2946,9 +2943,6 @@ static int cxgb_setup_tc_flower(struct net_device *dev,
 static int cxgb_setup_tc_cls_u32(struct net_device *dev,
 				 struct tc_cls_u32_offload *cls_u32)
 {
-	if (cls_u32->common.chain_index)
-		return -EOPNOTSUPP;
-
 	switch (cls_u32->command) {
 	case TC_CLSU32_NEW_KNODE:
 	case TC_CLSU32_REPLACE_KNODE:
@@ -2974,7 +2968,7 @@ static int cxgb_setup_tc_block_cb(enum tc_setup_type type, void *type_data,
 		return -EINVAL;
 	}
 
-	if (!tc_can_offload(dev))
+	if (!tc_cls_can_offload_and_chain0(dev, type_data))
 		return -EOPNOTSUPP;
 
 	switch (type) {
-- 
2.15.1

[PATCH net-next 3/8] mlx5: use tc_cls_can_offload_and_chain0()

[Jakub Kicinski]

Make use of tc_cls_can_offload_and_chain0() to set extack msg in case
ethtool tc offload flag is not set or chain unsupported.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Simon Horman <simon.horman@netronome.com>
---
CC: Saeed Mahameed <saeedm@mellanox.com>
CC: Or Gerlitz <ogerlitz@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 5 +----
 drivers/net/ethernet/mellanox/mlx5/core/en_rep.c  | 5 +----
 2 files changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
index 8530c770c873..47bab842c5ee 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
@@ -2944,9 +2944,6 @@ static int mlx5e_setup_tc_mqprio(struct net_device *netdev,
 static int mlx5e_setup_tc_cls_flower(struct mlx5e_priv *priv,
 				     struct tc_cls_flower_offload *cls_flower)
 {
-	if (cls_flower->common.chain_index)
-		return -EOPNOTSUPP;
-
 	switch (cls_flower->command) {
 	case TC_CLSFLOWER_REPLACE:
 		return mlx5e_configure_flower(priv, cls_flower);
@@ -2964,7 +2961,7 @@ int mlx5e_setup_tc_block_cb(enum tc_setup_type type, void *type_data,
 {
 	struct mlx5e_priv *priv = cb_priv;
 
-	if (!tc_can_offload(priv->netdev))
+	if (!tc_cls_can_offload_and_chain0(priv->netdev, type_data))
 		return -EOPNOTSUPP;
 
 	switch (type) {
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c b/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c
index 10fa6a18fcf9..363d8dcb7f17 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c
@@ -719,9 +719,6 @@ static int
 mlx5e_rep_setup_tc_cls_flower(struct mlx5e_priv *priv,
 			      struct tc_cls_flower_offload *cls_flower)
 {
-	if (cls_flower->common.chain_index)
-		return -EOPNOTSUPP;
-
 	switch (cls_flower->command) {
 	case TC_CLSFLOWER_REPLACE:
 		return mlx5e_configure_flower(priv, cls_flower);
@@ -739,7 +736,7 @@ static int mlx5e_rep_setup_tc_cb(enum tc_setup_type type, void *type_data,
 {
 	struct mlx5e_priv *priv = cb_priv;
 
-	if (!tc_can_offload(priv->netdev))
+	if (!tc_cls_can_offload_and_chain0(priv->netdev, type_data))
 		return -EOPNOTSUPP;
 
 	switch (type) {
-- 
2.15.1

[PATCH net-next 4/8] bnxt: use tc_cls_can_offload_and_chain0()

[Jakub Kicinski]

Make use of tc_cls_can_offload_and_chain0() to set extack msg in case
ethtool tc offload flag is not set or chain unsupported.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Simon Horman <simon.horman@netronome.com>
---
CC: Michael Chan <michael.chan@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c     | 3 ++-
 drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c  | 3 ---
 drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c | 3 ++-
 3 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 6b7e99675571..4b001d2050c2 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -7778,7 +7778,8 @@ static int bnxt_setup_tc_block_cb(enum tc_setup_type type, void *type_data,
 {
 	struct bnxt *bp = cb_priv;
 
-	if (!bnxt_tc_flower_enabled(bp) || !tc_can_offload(bp->dev))
+	if (!bnxt_tc_flower_enabled(bp) ||
+	    !tc_cls_can_offload_and_chain0(bp->dev, type_data))
 		return -EOPNOTSUPP;
 
 	switch (type) {
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
index 2ece1645f55d..fbe6e208e17b 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
@@ -1474,9 +1474,6 @@ int bnxt_tc_setup_flower(struct bnxt *bp, u16 src_fid,
 {
 	int rc = 0;
 
-	if (cls_flower->common.chain_index)
-		return -EOPNOTSUPP;
-
 	switch (cls_flower->command) {
 	case TC_CLSFLOWER_REPLACE:
 		rc = bnxt_tc_add_flow(bp, src_fid, cls_flower);
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c
index 2ca11be64182..26290403f38f 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c
@@ -124,7 +124,8 @@ static int bnxt_vf_rep_setup_tc_block_cb(enum tc_setup_type type,
 	struct bnxt *bp = vf_rep->bp;
 	int vf_fid = bp->pf.vf[vf_rep->vf_idx].fw_fid;
 
-	if (!bnxt_tc_flower_enabled(vf_rep->bp) || !tc_can_offload(bp->dev))
+	if (!bnxt_tc_flower_enabled(vf_rep->bp) ||
+	    !tc_cls_can_offload_and_chain0(bp->dev, type_data))
 		return -EOPNOTSUPP;
 
 	switch (type) {
-- 
2.15.1

[PATCH net-next 5/8] nfp: flower: use tc_cls_can_offload_and_chain0()

[Jakub Kicinski]

Make use of tc_cls_can_offload_and_chain0() to set extack msg in case
ethtool tc offload flag is not set or chain unsupported.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Simon Horman <simon.horman@netronome.com>
---
 drivers/net/ethernet/netronome/nfp/flower/offload.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/netronome/nfp/flower/offload.c b/drivers/net/ethernet/netronome/nfp/flower/offload.c
index 837134a9137c..08c4c6dc5f7f 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/offload.c
+++ b/drivers/net/ethernet/netronome/nfp/flower/offload.c
@@ -483,8 +483,7 @@ static int
 nfp_flower_repr_offload(struct nfp_app *app, struct net_device *netdev,
 			struct tc_cls_flower_offload *flower, bool egress)
 {
-	if (!eth_proto_is_802_3(flower->common.protocol) ||
-	    flower->common.chain_index)
+	if (!eth_proto_is_802_3(flower->common.protocol))
 		return -EOPNOTSUPP;
 
 	switch (flower->command) {
@@ -504,7 +503,7 @@ int nfp_flower_setup_tc_egress_cb(enum tc_setup_type type, void *type_data,
 {
 	struct nfp_repr *repr = cb_priv;
 
-	if (!tc_can_offload(repr->netdev))
+	if (!tc_cls_can_offload_and_chain0(repr->netdev, type_data))
 		return -EOPNOTSUPP;
 
 	switch (type) {
@@ -521,7 +520,7 @@ static int nfp_flower_setup_tc_block_cb(enum tc_setup_type type,
 {
 	struct nfp_repr *repr = cb_priv;
 
-	if (!tc_can_offload(repr->netdev))
+	if (!tc_cls_can_offload_and_chain0(repr->netdev, type_data))
 		return -EOPNOTSUPP;
 
 	switch (type) {
-- 
2.15.1

[PATCH net-next 6/8] ixgbe: use tc_cls_can_offload_and_chain0()

[Jakub Kicinski]

Make use of tc_cls_can_offload_and_chain0() to set extack msg in case
ethtool tc offload flag is not set or chain unsupported.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Simon Horman <simon.horman@netronome.com>
---
CC: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 722cc3153a99..bbb622f15a77 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -9303,9 +9303,6 @@ static int ixgbe_configure_clsu32(struct ixgbe_adapter *adapter,
 static int ixgbe_setup_tc_cls_u32(struct ixgbe_adapter *adapter,
 				  struct tc_cls_u32_offload *cls_u32)
 {
-	if (cls_u32->common.chain_index)
-		return -EOPNOTSUPP;
-
 	switch (cls_u32->command) {
 	case TC_CLSU32_NEW_KNODE:
 	case TC_CLSU32_REPLACE_KNODE:
@@ -9327,7 +9324,7 @@ static int ixgbe_setup_tc_block_cb(enum tc_setup_type type, void *type_data,
 {
 	struct ixgbe_adapter *adapter = cb_priv;
 
-	if (!tc_can_offload(adapter->netdev))
+	if (!tc_cls_can_offload_and_chain0(adapter->netdev, type_data))
 		return -EOPNOTSUPP;
 
 	switch (type) {
-- 
2.15.1

[PATCH net-next 7/8] mlxsw: use tc_cls_can_offload_and_chain0()

[Jakub Kicinski]

Make use of tc_cls_can_offload_and_chain0() to set extack msg in case
ethtool tc offload flag is not set or chain unsupported.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Simon Horman <simon.horman@netronome.com>
---
CC: Jiri Pirko <jiri@mellanox.com>
CC: Ido Schimmel <idosch@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlxsw/spectrum.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
index 833cd0a96fd9..3dcc58d61506 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
@@ -1738,9 +1738,6 @@ static int mlxsw_sp_setup_tc_cls_matchall(struct mlxsw_sp_port *mlxsw_sp_port,
 					  struct tc_cls_matchall_offload *f,
 					  bool ingress)
 {
-	if (f->common.chain_index)
-		return -EOPNOTSUPP;
-
 	switch (f->command) {
 	case TC_CLSMATCHALL_REPLACE:
 		return mlxsw_sp_port_add_cls_matchall(mlxsw_sp_port, f,
@@ -1780,7 +1777,8 @@ static int mlxsw_sp_setup_tc_block_cb_matchall(enum tc_setup_type type,
 
 	switch (type) {
 	case TC_SETUP_CLSMATCHALL:
-		if (!tc_can_offload(mlxsw_sp_port->dev))
+		if (!tc_cls_can_offload_and_chain0(mlxsw_sp_port->dev,
+						   type_data))
 			return -EOPNOTSUPP;
 
 		return mlxsw_sp_setup_tc_cls_matchall(mlxsw_sp_port, type_data,
-- 
2.15.1

[PATCH net-next 8/8] selftests/bpf: check for spurious extacks from the driver

[Jakub Kicinski]

Drivers should not report errors when offload is not forced.
Check stdout and stderr for familiar messages when with no
skip flags and with skip_hw.  Check for add, replace, and
destroy.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Simon Horman <simon.horman@netronome.com>
---
 tools/testing/selftests/bpf/test_offload.py | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/tools/testing/selftests/bpf/test_offload.py b/tools/testing/selftests/bpf/test_offload.py
index ae3eea3ab820..3a43fbc896db 100755
--- a/tools/testing/selftests/bpf/test_offload.py
+++ b/tools/testing/selftests/bpf/test_offload.py
@@ -543,6 +543,10 @@ netns = [] # net namespaces to be removed
 def check_extack_nsim(output, reference, args):
     check_extack(output, "Error: netdevsim: " + reference, args)
 
+def check_no_extack(res, needle):
+    fail((res[1] + res[2]).find(needle) != -1,
+         "Found '%s' in command output, leaky extack?" % (needle))
+
 def check_verifier_log(output, reference):
     lines = output.split("\n")
     for l in reversed(lines):
@@ -550,6 +554,18 @@ netns = [] # net namespaces to be removed
             return
     fail(True, "Missing or incorrect message from netdevsim in verifier log")
 
+def test_spurios_extack(sim, obj, skip_hw, needle):
+    res = sim.cls_bpf_add_filter(obj, prio=1, handle=1, skip_hw=skip_hw,
+                                 include_stderr=True)
+    check_no_extack(res, needle)
+    res = sim.cls_bpf_add_filter(obj, op="replace", prio=1, handle=1,
+                                 skip_hw=skip_hw, include_stderr=True)
+    check_no_extack(res, needle)
+    res = sim.cls_filter_op(op="delete", prio=1, handle=1, cls="bpf",
+                            include_stderr=True)
+    check_no_extack(res, needle)
+
+
 # Parse command line
 parser = argparse.ArgumentParser()
 parser.add_argument("--log", help="output verbose log to given file")
@@ -687,6 +703,17 @@ netns = []
                  (j))
         sim.cls_filter_op(op="delete", prio=1, handle=1, cls="bpf")
 
+    start_test("Test spurious extack from the driver...")
+    test_spurios_extack(sim, obj, False, "netdevsim")
+    test_spurios_extack(sim, obj, True, "netdevsim")
+
+    sim.set_ethtool_tc_offloads(False)
+
+    test_spurios_extack(sim, obj, False, "TC offload is disabled")
+    test_spurios_extack(sim, obj, True, "TC offload is disabled")
+
+    sim.set_ethtool_tc_offloads(True)
+
     sim.tc_flush_filters()
 
     start_test("Test TC offloads work...")
-- 
2.15.1

Re: [PATCH net-next 1/8] pkt_cls: add new tc cls helper to check offload flag and chain index

[Jiri Pirko]

Thu, Jan 25, 2018 at 01:17:46AM CET, jakub.kicinski@netronome.com wrote:
>Very few (mlxsw) upstream drivers seem to allow offload of chains
>other than 0.  Save driver developers typing and add a helper for
>checking both if ethtool's TC offload flag is on and if chain is 0.
>This helper will set the extack appropriately in both error cases.
>
>Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
>Reviewed-by: Simon Horman <simon.horman@netronome.com>
>---
> drivers/net/ethernet/netronome/nfp/bpf/main.c |  4 +---
> drivers/net/netdevsim/bpf.c                   |  5 +----
> include/net/pkt_cls.h                         | 12 ++++++++++++
> 3 files changed, 14 insertions(+), 7 deletions(-)
>
>diff --git a/drivers/net/ethernet/netronome/nfp/bpf/main.c b/drivers/net/ethernet/netronome/nfp/bpf/main.c
>index b3206855535a..322027792fe8 100644
>--- a/drivers/net/ethernet/netronome/nfp/bpf/main.c
>+++ b/drivers/net/ethernet/netronome/nfp/bpf/main.c
>@@ -130,7 +130,7 @@ static int nfp_bpf_setup_tc_block_cb(enum tc_setup_type type,
> 				   "only offload of BPF classifiers supported");
> 		return -EOPNOTSUPP;
> 	}
>-	if (!tc_can_offload_extack(nn->dp.netdev, cls_bpf->common.extack))
>+	if (!tc_cls_can_offload_and_chain0(nn->dp.netdev, &cls_bpf->common))
> 		return -EOPNOTSUPP;
> 	if (!nfp_net_ebpf_capable(nn)) {
> 		NL_SET_ERR_MSG_MOD(cls_bpf->common.extack,
>@@ -142,8 +142,6 @@ static int nfp_bpf_setup_tc_block_cb(enum tc_setup_type type,
> 				   "only ETH_P_ALL supported as filter protocol");
> 		return -EOPNOTSUPP;
> 	}
>-	if (cls_bpf->common.chain_index)
>-		return -EOPNOTSUPP;
> 
> 	/* Only support TC direct action */
> 	if (!cls_bpf->exts_integrated ||
>diff --git a/drivers/net/netdevsim/bpf.c b/drivers/net/netdevsim/bpf.c
>index 8166f121bbcc..de73c1ff0939 100644
>--- a/drivers/net/netdevsim/bpf.c
>+++ b/drivers/net/netdevsim/bpf.c
>@@ -135,7 +135,7 @@ int nsim_bpf_setup_tc_block_cb(enum tc_setup_type type,
> 		return -EOPNOTSUPP;
> 	}
> 
>-	if (!tc_can_offload_extack(ns->netdev, cls_bpf->common.extack))
>+	if (!tc_cls_can_offload_and_chain0(ns->netdev, &cls_bpf->common))
> 		return -EOPNOTSUPP;
> 
> 	if (cls_bpf->common.protocol != htons(ETH_P_ALL)) {
>@@ -144,9 +144,6 @@ int nsim_bpf_setup_tc_block_cb(enum tc_setup_type type,
> 		return -EOPNOTSUPP;
> 	}
> 
>-	if (cls_bpf->common.chain_index)
>-		return -EOPNOTSUPP;
>-

For nfp and netdevsim you do this in a patch that introduces the helper,
for the rest you have a separate patch? Why this inconsistency?
Again, from my perspective, this can be done in a single patch for all
drivers. All the same hunks.



> 	if (!ns->bpf_tc_accept) {
> 		NSIM_EA(cls_bpf->common.extack,
> 			"netdevsim configured to reject BPF TC offload");
>diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
>index 1a41513cec7f..4db08d7dd22c 100644
>--- a/include/net/pkt_cls.h
>+++ b/include/net/pkt_cls.h
>@@ -656,6 +656,18 @@ static inline bool tc_can_offload_extack(const struct net_device *dev,
> 	return can;
> }
> 
>+static inline bool
>+tc_cls_can_offload_and_chain0(const struct net_device *dev,
>+			      struct tc_cls_common_offload *common)
>+{
>+	if (common->chain_index) {
>+		NL_SET_ERR_MSG(common->extack,
>+			       "Driver supports only offload of chain 0");

No need for a line-wrap.


>+		return false;
>+	}
>+	return tc_can_offload_extack(dev, common->extack);
>+}
>+
> static inline bool tc_skip_hw(u32 flags)
> {
> 	return (flags & TCA_CLS_FLAGS_SKIP_HW) ? true : false;
>-- 
>2.15.1
>

Re: [PATCH net-next 3/8] mlx5: use tc_cls_can_offload_and_chain0()

[Or Gerlitz]

On Thu, Jan 25, 2018 at 2:17 AM, Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> Make use of tc_cls_can_offload_and_chain0() to set extack msg in case
> ethtool tc offload flag is not set or chain unsupported.
>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> Reviewed-by: Simon Horman <simon.horman@netronome.com>

Acked-by: Or Gerlitz <ogerlitz@mellanox.com>

Re: [PATCH net-next 1/8] pkt_cls: add new tc cls helper to check offload flag and chain index

[Marcelo Ricardo Leitner]

On Wed, Jan 24, 2018 at 04:17:46PM -0800, Jakub Kicinski wrote:
...
> +static inline bool
> +tc_cls_can_offload_and_chain0(const struct net_device *dev,
> +			      struct tc_cls_common_offload *common)
> +{
> +	if (common->chain_index) {
> +		NL_SET_ERR_MSG(common->extack,
> +			       "Driver supports only offload of chain 0");
> +		return false;
> +	}
> +	return tc_can_offload_extack(dev, common->extack);

I know that most of the drivers updated in this patchset checks it
this way, but considering both checks end up being done anyway in the
success case and that performance POV on error path is irrelevant
here, wouldn't it be better to swap both conditions here? I.e., first
check if the device can offload, to only then check what is being
offloaded?

Otherwise the first error would be implying that the device can
offload, just not the specified chain.

> +}
> +
>  static inline bool tc_skip_hw(u32 flags)
>  {
>  	return (flags & TCA_CLS_FLAGS_SKIP_HW) ? true : false;
> -- 
> 2.15.1
> 

Re: [PATCH net-next 1/8] pkt_cls: add new tc cls helper to check offload flag and chain index

[Jakub Kicinski]

On Thu, 25 Jan 2018 13:41:58 -0200, Marcelo Ricardo Leitner wrote:
> On Wed, Jan 24, 2018 at 04:17:46PM -0800, Jakub Kicinski wrote:
> ...
> > +static inline bool
> > +tc_cls_can_offload_and_chain0(const struct net_device *dev,
> > +			      struct tc_cls_common_offload *common)
> > +{
> > +	if (common->chain_index) {
> > +		NL_SET_ERR_MSG(common->extack,
> > +			       "Driver supports only offload of chain 0");
> > +		return false;
> > +	}
> > +	return tc_can_offload_extack(dev, common->extack);  
> 
> I know that most of the drivers updated in this patchset checks it
> this way, but considering both checks end up being done anyway in the
> success case and that performance POV on error path is irrelevant
> here, wouldn't it be better to swap both conditions here? I.e., first
> check if the device can offload, to only then check what is being
> offloaded?
> 
> Otherwise the first error would be implying that the device can
> offload, just not the specified chain.

Sure, can do.

Re: [PATCH net-next 1/8] pkt_cls: add new tc cls helper to check offload flag and chain index

[Jakub Kicinski]

On Thu, 25 Jan 2018 09:03:48 +0100, Jiri Pirko wrote:
> >@@ -144,9 +144,6 @@ int nsim_bpf_setup_tc_block_cb(enum tc_setup_type type,
> > 		return -EOPNOTSUPP;
> > 	}
> > 
> >-	if (cls_bpf->common.chain_index)
> >-		return -EOPNOTSUPP;
> >-  
> 
> For nfp and netdevsim you do this in a patch that introduces the helper,
> for the rest you have a separate patch? Why this inconsistency?
> Again, from my perspective, this can be done in a single patch for all
> drivers. All the same hunks.

Ah, indeed, slightly sloppy.  This patch used to remove the
tc_can_offload_extack() helper hence the conversion here.
Let me split them out.