[RFC net-next 0/8] use tc_can_offload_extack() throughout the drivers

[RFC net-next 0/8] use tc_can_offload_extack() throughout the drivers

[Jakub Kicinski]

Hi!

I have a number of patches for drivers to use tc_can_offload_extack()
as requested, but I wanted to ask for early comments and one specific
question - should we assume that type_data in block_cbs is always going
to contain struct tc_cls_common_offload as first argument?  In that
case we could just make it that struct instead of void?

It wasn't clear to me this assumption was safe therefore I pushed the
checks after enum tc_setup_type is checked against known values.  And
there is only one driver now (cxgb4) which supports two classifiers in
the same code base, so checking tc_can_offload before type_data is
cast doesn't buy much LOC.

Jiri, WDYT?

(i40e patch is queued up for later to avoid conflicts.)

Jakub Kicinski (8):
  pkt_cls: make tc_can_offload_extack() check chain index
  mlx5: use tc_can_offload_cls()
  mlxsw: use tc_can_offload_cls()
  cls_bpf: drivers: don't try to use extack before callback type is
    validated

 drivers/net/ethernet/broadcom/bnxt/bnxt.c           |  2 +-
 drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c        |  2 +-
 drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c       |  2 +-
 drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c     |  7 ++-----
 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      |  5 +----
 drivers/net/ethernet/netronome/nfp/bpf/main.c       |  9 ++-------
 drivers/net/ethernet/netronome/nfp/flower/offload.c | 11 +++--------
 drivers/net/netdevsim/bpf.c                         | 10 ++--------
 include/net/pkt_cls.h                               | 20 +++++++++++++-------
 12 files changed, 29 insertions(+), 54 deletions(-)

-- 
2.15.1

[RFC net-next 1/8] pkt_cls: make tc_can_offload_extack() check chain index

[Jakub Kicinski]

No upstream drivers seem to allow offload of chains other than 0.
Save driver developers typing and make tc_can_offload_extack()
check for that condition as well.  Rename the function to
tc_can_offload_cls() to better represent its application.

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

diff --git a/drivers/net/ethernet/netronome/nfp/bpf/main.c b/drivers/net/ethernet/netronome/nfp/bpf/main.c
index b3206855535a..552e2657b536 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_can_offload_cls(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..bbcb8ec42208 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_can_offload_cls(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..84932226da67 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -645,15 +645,21 @@ static inline bool tc_can_offload(const struct net_device *dev)
 	return dev->features & NETIF_F_HW_TC;
 }
 
-static inline bool tc_can_offload_extack(const struct net_device *dev,
-					 struct netlink_ext_ack *extack)
+static inline bool tc_can_offload_cls(const struct net_device *dev,
+				      struct tc_cls_common_offload *common)
 {
-	bool can = tc_can_offload(dev);
-
-	if (!can)
-		NL_SET_ERR_MSG(extack, "TC offload is disabled on net device");
+	if (common->chain_index) {
+		NL_SET_ERR_MSG(common->extack,
+			       "Driver supports only offload of chain 0");
+		return false;
+	}
+	if (!tc_can_offload(dev)) {
+		NL_SET_ERR_MSG(common->extack,
+			       "TC offload is disabled on net device");
+		return false;
+	}
 
-	return can;
+	return true;
 }
 
 static inline bool tc_skip_hw(u32 flags)
-- 
2.15.1

[RFC net-next 2/8] bnxt: use tc_can_offload_cls()

[Jakub Kicinski]

Make use of tc_can_offload_cls() to report error in case
ethtool tc offload flag is not set or chain unsupported.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c     | 2 +-
 drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c  | 2 +-
 drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 6b7e99675571..50e54b001af7 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -7778,7 +7778,7 @@ 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))
 		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..34721a9737a7 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
@@ -1474,7 +1474,7 @@ int bnxt_tc_setup_flower(struct bnxt *bp, u16 src_fid,
 {
 	int rc = 0;
 
-	if (cls_flower->common.chain_index)
+	if (!tc_can_offload_cls(bp->dev, &cls_flower->common))
 		return -EOPNOTSUPP;
 
 	switch (cls_flower->command) {
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c
index 2ca11be64182..c2c659ccb47d 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c
@@ -124,7 +124,7 @@ 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))
 		return -EOPNOTSUPP;
 
 	switch (type) {
-- 
2.15.1

[RFC net-next 3/8] nfp: flower: use tc_can_offload_cls()

[Jakub Kicinski]

Make use of tc_can_offload_cls() to report error in case
ethtool tc offload flag is not set or chain unsupported.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 drivers/net/ethernet/netronome/nfp/flower/offload.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/netronome/nfp/flower/offload.c b/drivers/net/ethernet/netronome/nfp/flower/offload.c
index 837134a9137c..74851d02c8b2 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/offload.c
+++ b/drivers/net/ethernet/netronome/nfp/flower/offload.c
@@ -483,8 +483,9 @@ 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 (!tc_can_offload_cls(netdev, &flower->common))
+		return -EOPNOTSUPP;
+	if (!eth_proto_is_802_3(flower->common.protocol))
 		return -EOPNOTSUPP;
 
 	switch (flower->command) {
@@ -504,9 +505,6 @@ 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))
-		return -EOPNOTSUPP;
-
 	switch (type) {
 	case TC_SETUP_CLSFLOWER:
 		return nfp_flower_repr_offload(repr->app, repr->netdev,
@@ -521,9 +519,6 @@ 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))
-		return -EOPNOTSUPP;
-
 	switch (type) {
 	case TC_SETUP_CLSFLOWER:
 		return nfp_flower_repr_offload(repr->app, repr->netdev,
-- 
2.15.1

[RFC net-next 4/8] cxgb4: use tc_can_offload_cls()

[Jakub Kicinski]

Make use of tc_can_offload_cls() to report error in case
ethtool tc offload flag is not set or chain unsupported.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
index 4716387830ef..bee357e1c147 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
@@ -2905,7 +2905,7 @@ 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)
+	if (!tc_can_offload_cls(dev, &cls_flower->common))
 		return -EOPNOTSUPP;
 
 	switch (cls_flower->command) {
@@ -2923,7 +2923,7 @@ 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)
+	if (!tc_can_offload_cls(dev, &cls_u32->common))
 		return -EOPNOTSUPP;
 
 	switch (cls_u32->command) {
@@ -2951,9 +2951,6 @@ static int cxgb_setup_tc_block_cb(enum tc_setup_type type, void *type_data,
 		return -EINVAL;
 	}
 
-	if (!tc_can_offload(dev))
-		return -EOPNOTSUPP;
-
 	switch (type) {
 	case TC_SETUP_CLSU32:
 		return cxgb_setup_tc_cls_u32(dev, type_data);
-- 
2.15.1

[RFC net-next 5/8] ixgbe: use tc_can_offload_cls()

[Jakub Kicinski]

Make use of tc_can_offload_cls() to report error in case
ethtool tc offload flag is not set or chain unsupported.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.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 4f28621b76e1..77682aee029e 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -9278,7 +9278,7 @@ 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)
+	if (!tc_can_offload_cls(adapter->netdev, &cls_u32->common))
 		return -EOPNOTSUPP;
 
 	switch (cls_u32->command) {
@@ -9302,9 +9302,6 @@ 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))
-		return -EOPNOTSUPP;
-
 	switch (type) {
 	case TC_SETUP_CLSU32:
 		return ixgbe_setup_tc_cls_u32(adapter, type_data);
-- 
2.15.1

[RFC net-next 6/8] mlx5: use tc_can_offload_cls()

[Jakub Kicinski]

Make use of tc_can_offload_cls() to report error in case
ethtool tc offload flag is not set or chain unsupported.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.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..6e4273a3da9b 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
@@ -2944,7 +2944,7 @@ 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)
+	if (!tc_can_offload_cls(priv->netdev, &cls_flower->common))
 		return -EOPNOTSUPP;
 
 	switch (cls_flower->command) {
@@ -2964,9 +2964,6 @@ 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))
-		return -EOPNOTSUPP;
-
 	switch (type) {
 	case TC_SETUP_CLSFLOWER:
 		return mlx5e_setup_tc_cls_flower(priv, type_data);
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c b/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c
index 10fa6a18fcf9..db05e8220e05 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c
@@ -719,7 +719,7 @@ 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)
+	if (!tc_can_offload_cls(priv->netdev, &cls_flower->common))
 		return -EOPNOTSUPP;
 
 	switch (cls_flower->command) {
@@ -739,9 +739,6 @@ 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))
-		return -EOPNOTSUPP;
-
 	switch (type) {
 	case TC_SETUP_CLSFLOWER:
 		return mlx5e_rep_setup_tc_cls_flower(priv, type_data);
-- 
2.15.1

[RFC net-next 7/8] mlxsw: use tc_can_offload_cls()

[Jakub Kicinski]

Make use of tc_can_offload_cls() to report error in case
ethtool tc offload flag is not set or chain unsupported.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 drivers/net/ethernet/mellanox/mlxsw/spectrum.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
index 833cd0a96fd9..1e4aac8471d5 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
@@ -1738,7 +1738,7 @@ 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)
+	if (!tc_can_offload_cls(mlxsw_sp_port->dev, &f->common))
 		return -EOPNOTSUPP;
 
 	switch (f->command) {
@@ -1780,9 +1780,6 @@ 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))
-			return -EOPNOTSUPP;
-
 		return mlxsw_sp_setup_tc_cls_matchall(mlxsw_sp_port, type_data,
 						      ingress);
 	case TC_SETUP_CLSFLOWER:
-- 
2.15.1

[RFC net-next 8/8] cls_bpf: drivers: don't try to use extack before callback type is validated

[Jakub Kicinski]

Although today only classifier offloads call block callbacks,
we shouldn't really assume that @type_data starts with
struct tc_cls_common_offload until we validated the type.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 drivers/net/ethernet/netronome/nfp/bpf/main.c | 5 +----
 drivers/net/netdevsim/bpf.c                   | 5 +----
 2 files changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/netronome/nfp/bpf/main.c b/drivers/net/ethernet/netronome/nfp/bpf/main.c
index 552e2657b536..165276b50b4c 100644
--- a/drivers/net/ethernet/netronome/nfp/bpf/main.c
+++ b/drivers/net/ethernet/netronome/nfp/bpf/main.c
@@ -125,11 +125,8 @@ static int nfp_bpf_setup_tc_block_cb(enum tc_setup_type type,
 	struct nfp_bpf_vnic *bv;
 	int err;
 
-	if (type != TC_SETUP_CLSBPF) {
-		NL_SET_ERR_MSG_MOD(cls_bpf->common.extack,
-				   "only offload of BPF classifiers supported");
+	if (type != TC_SETUP_CLSBPF)
 		return -EOPNOTSUPP;
-	}
 	if (!tc_can_offload_cls(nn->dp.netdev, &cls_bpf->common))
 		return -EOPNOTSUPP;
 	if (!nfp_net_ebpf_capable(nn)) {
diff --git a/drivers/net/netdevsim/bpf.c b/drivers/net/netdevsim/bpf.c
index bbcb8ec42208..5b2c5260624d 100644
--- a/drivers/net/netdevsim/bpf.c
+++ b/drivers/net/netdevsim/bpf.c
@@ -129,11 +129,8 @@ int nsim_bpf_setup_tc_block_cb(enum tc_setup_type type,
 	struct netdevsim *ns = cb_priv;
 	struct bpf_prog *oldprog;
 
-	if (type != TC_SETUP_CLSBPF) {
-		NSIM_EA(cls_bpf->common.extack,
-			"only offload of BPF classifiers supported");
+	if (type != TC_SETUP_CLSBPF)
 		return -EOPNOTSUPP;
-	}
 
 	if (!tc_can_offload_cls(ns->netdev, &cls_bpf->common))
 		return -EOPNOTSUPP;
-- 
2.15.1

Re: [RFC net-next 1/8] pkt_cls: make tc_can_offload_extack() check chain index

[Jiri Pirko]

Tue, Jan 23, 2018 at 10:33:33PM CET, jakub.kicinski@netronome.com wrote:
>No upstream drivers seem to allow offload of chains other than 0.

mlxsw does. And I know that Intel was talking about adding the support
to i40e iirc.


>Save driver developers typing and make tc_can_offload_extack()
>check for that condition as well.  Rename the function to
>tc_can_offload_cls() to better represent its application.
>
>Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
>---

[...]


>--- a/include/net/pkt_cls.h
>+++ b/include/net/pkt_cls.h
>@@ -645,15 +645,21 @@ static inline bool tc_can_offload(const struct net_device *dev)
> 	return dev->features & NETIF_F_HW_TC;
> }
> 
>-static inline bool tc_can_offload_extack(const struct net_device *dev,
>-					 struct netlink_ext_ack *extack)
>+static inline bool tc_can_offload_cls(const struct net_device *dev,
>+				      struct tc_cls_common_offload *common)
> {
>-	bool can = tc_can_offload(dev);
>-
>-	if (!can)
>-		NL_SET_ERR_MSG(extack, "TC offload is disabled on net device");
>+	if (common->chain_index) {

This check here is wrong. It is up to the driver if it supports
multichain offload or not, should not be checked in a generic code along
with tc_can_offload.


>+		NL_SET_ERR_MSG(common->extack,
>+			       "Driver supports only offload of chain 0");
>+		return false;
>+	}
>+	if (!tc_can_offload(dev)) {
>+		NL_SET_ERR_MSG(common->extack,
>+			       "TC offload is disabled on net device");
>+		return false;
>+	}
> 
>-	return can;
>+	return true;
> }
> 
> static inline bool tc_skip_hw(u32 flags)
>-- 
>2.15.1
>

Re: [RFC net-next 1/8] pkt_cls: make tc_can_offload_extack() check chain index

[Jakub Kicinski]

On Wed, 24 Jan 2018 09:28:44 +0100, Jiri Pirko wrote:
> Tue, Jan 23, 2018 at 10:33:33PM CET, jakub.kicinski@netronome.com wrote:
> >No upstream drivers seem to allow offload of chains other than 0.  
> 
> mlxsw does. And I know that Intel was talking about adding the support
> to i40e iirc.

An, I wrote that commit message before looking deeper into mlxsw.
My understanding is that you only use the simple tc_can_offload() 
check for matchall egress redirect.  For flower which actually makes
use of multiple chains, you also allow block sharing and have the
driver count the number of ports with tc offloads disabled.  Therefore
you don't use tc_can_offload() there.

Is that correct?

> >Save driver developers typing and make tc_can_offload_extack()
> >check for that condition as well.  Rename the function to
> >tc_can_offload_cls() to better represent its application.
> >
> >Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> >---  
> 
> [...]
> 
> >--- a/include/net/pkt_cls.h
> >+++ b/include/net/pkt_cls.h
> >@@ -645,15 +645,21 @@ static inline bool tc_can_offload(const struct net_device *dev)
> > 	return dev->features & NETIF_F_HW_TC;
> > }
> > 
> >-static inline bool tc_can_offload_extack(const struct net_device *dev,
> >-					 struct netlink_ext_ack *extack)
> >+static inline bool tc_can_offload_cls(const struct net_device *dev,
> >+				      struct tc_cls_common_offload *common)
> > {
> >-	bool can = tc_can_offload(dev);
> >-
> >-	if (!can)
> >-		NL_SET_ERR_MSG(extack, "TC offload is disabled on net device");
> >+	if (common->chain_index) {  
> 
> This check here is wrong. It is up to the driver if it supports
> multichain offload or not, should not be checked in a generic code along
> with tc_can_offload.

Mm.. maybe the name is a bit unfortunate now..  How about keeping
tc_can_offload_extack() as is and adding:

static inline bool tc_can_offload_simple(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);
}

"simple" standing for simple driver.

My gut feeling is that the drivers will fall into two categories:
simple drivers which don't share blocks and chains and advanced drivers
which do what mlxsw does and therefore doesn't use those helpers.  IOW
there will be no tc_can_offload_extack() callers.

CCing Intel folks.

Re: [RFC net-next 0/8] use tc_can_offload_extack() throughout the drivers

[Jiri Pirko]

Tue, Jan 23, 2018 at 10:33:32PM CET, jakub.kicinski@netronome.com wrote:
>Hi!
>
>I have a number of patches for drivers to use tc_can_offload_extack()
>as requested, but I wanted to ask for early comments and one specific
>question - should we assume that type_data in block_cbs is always going
>to contain struct tc_cls_common_offload as first argument?  In that
>case we could just make it that struct instead of void?

Yes, I believe so. I wanted to avoid lots of casts, that's why I'm
passing void. I agree it is probably a bit confusing.


>
>It wasn't clear to me this assumption was safe therefore I pushed the
>checks after enum tc_setup_type is checked against known values.  And
>there is only one driver now (cxgb4) which supports two classifiers in
>the same code base, so checking tc_can_offload before type_data is
>cast doesn't buy much LOC.

mlxsw also supports multiple classifiers. Others will come.


>
>Jiri, WDYT?
>
>(i40e patch is queued up for later to avoid conflicts.)
>
>Jakub Kicinski (8):
>  pkt_cls: make tc_can_offload_extack() check chain index
>  mlx5: use tc_can_offload_cls()
>  mlxsw: use tc_can_offload_cls()
>  cls_bpf: drivers: don't try to use extack before callback type is
>    validated
>
> drivers/net/ethernet/broadcom/bnxt/bnxt.c           |  2 +-
> drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c        |  2 +-
> drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c       |  2 +-
> drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c     |  7 ++-----
> 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      |  5 +----
> drivers/net/ethernet/netronome/nfp/bpf/main.c       |  9 ++-------
> drivers/net/ethernet/netronome/nfp/flower/offload.c | 11 +++--------
> drivers/net/netdevsim/bpf.c                         | 10 ++--------
> include/net/pkt_cls.h                               | 20 +++++++++++++-------
> 12 files changed, 29 insertions(+), 54 deletions(-)
>
>-- 
>2.15.1
>