[patch net-next 0/4] net: sched: block callbacks follow-up

From: Jiri Pirko <redacted>

This patchset does a bit of cleanup of leftovers after block callbacks
patchset. The main part is patch 2, which restores the original handling
of tc offload feature flag.

Jiri Pirko (4):
  net: sched: remove unused tc_should_offload helper
  net: sched: move the can_offload check from binding phase to rule
    insertion phase
  net: sched: remove tc_can_offload check from egdev call
  net: sched: remove ndo_setup_tc check from tc_can_offload

 drivers/net/ethernet/broadcom/bnxt/bnxt.c           |  2 +-
 drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c       |  3 +++
 drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c     |  3 +++
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c       |  3 +++
 drivers/net/ethernet/mellanox/mlx5/core/en_main.c   |  3 +++
 drivers/net/ethernet/mellanox/mlx5/core/en_rep.c    |  3 +++
 drivers/net/ethernet/mellanox/mlxsw/spectrum.c      |  3 +++
 drivers/net/ethernet/netronome/nfp/bpf/main.c       |  3 +++
 drivers/net/ethernet/netronome/nfp/flower/offload.c |  3 +++
 include/net/pkt_cls.h                               | 13 +------------
 net/dsa/slave.c                                     |  3 +++
 net/sched/cls_api.c                                 |  4 ++--
 12 files changed, 31 insertions(+), 15 deletions(-)

-- 
2.9.5

From: Jiri Pirko <redacted>

tc_should_offload is no longer used, remove it.

Signed-off-by: Jiri Pirko <redacted>
---
 include/net/pkt_cls.h | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index 04caa24..0d24d22 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h

@@ -621,13 +621,6 @@ static inline bool tc_skip_hw(u32 flags)
 	return (flags & TCA_CLS_FLAGS_SKIP_HW) ? true : false;
 }
 
-static inline bool tc_should_offload(const struct net_device *dev, u32 flags)
-{
-	if (tc_skip_hw(flags))
-		return false;
-	return tc_can_offload(dev);
-}
-
 static inline bool tc_skip_sw(u32 flags)
 {
 	return (flags & TCA_CLS_FLAGS_SKIP_SW) ? true : false;

-- 
2.9.5

From: Jiri Pirko <redacted>

This restores the original behaviour before the block callbacks were
introduced. Allow the drivers to do binding of block always, no matter
if the NETIF_F_HW_TC feature is on or off. Move the check to the block
callback which is called for rule insertion.

Reported-by: Alexander Duyck <redacted>
Signed-off-by: Jiri Pirko <redacted>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c           | 2 +-
 drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c       | 3 +++
 drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c     | 3 +++
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c       | 3 +++
 drivers/net/ethernet/mellanox/mlx5/core/en_main.c   | 3 +++
 drivers/net/ethernet/mellanox/mlx5/core/en_rep.c    | 3 +++
 drivers/net/ethernet/mellanox/mlxsw/spectrum.c      | 3 +++
 drivers/net/ethernet/netronome/nfp/bpf/main.c       | 3 +++
 drivers/net/ethernet/netronome/nfp/flower/offload.c | 3 +++
 net/dsa/slave.c                                     | 3 +++
 net/sched/cls_api.c                                 | 2 +-
 11 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 24d55724..e193232 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c

@@ -7325,7 +7325,7 @@ static int bnxt_setup_tc_block_cb(enum tc_setup_type type, void *type_data,
 {
 	struct bnxt *bp = cb_priv;
 
-	if (BNXT_VF(bp))
+	if (!tc_can_offload(bp->dev) || BNXT_VF(bp))
 		return -EOPNOTSUPP;
 
 	switch (type) {

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c
index 4ae9359..2fa614f 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c

@@ -124,6 +124,9 @@ 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 (!tc_can_offload(bp->dev))
+		return -EOPNOTSUPP;
+
 	switch (type) {
 	case TC_SETUP_CLSFLOWER:
 		return bnxt_tc_setup_flower(bp, vf_fid, type_data);

diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
index e16078d..b07eecf 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c

@@ -2935,6 +2935,9 @@ 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);

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 7f503d3..3520f28 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c

@@ -9392,6 +9392,9 @@ 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);

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
index 560b208..28ae00b 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c

@@ -3106,6 +3106,9 @@ 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 0edb706..2c43606 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c

@@ -682,6 +682,9 @@ 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);

diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
index 8f8db6c..71aa603 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c

@@ -1738,6 +1738,9 @@ static int mlxsw_sp_setup_tc_block_cb(enum tc_setup_type type, void *type_data,
 {
 	struct mlxsw_sp_port *mlxsw_sp_port = cb_priv;
 
+	if (!tc_can_offload(mlxsw_sp_port->dev))
+		return -EOPNOTSUPP;
+
 	switch (type) {
 	case TC_SETUP_CLSMATCHALL:
 		return mlxsw_sp_setup_tc_cls_matchall(mlxsw_sp_port, type_data,

diff --git a/drivers/net/ethernet/netronome/nfp/bpf/main.c b/drivers/net/ethernet/netronome/nfp/bpf/main.c
index fa0ac90..55fca38 100644
--- a/drivers/net/ethernet/netronome/nfp/bpf/main.c
+++ b/drivers/net/ethernet/netronome/nfp/bpf/main.c

@@ -120,6 +120,9 @@ static int nfp_bpf_setup_tc_block_cb(enum tc_setup_type type,
 	struct tc_cls_bpf_offload *cls_bpf = type_data;
 	struct nfp_net *nn = cb_priv;
 
+	if (!tc_can_offload(nn->dp.netdev))
+		return -EOPNOTSUPP;
+
 	switch (type) {
 	case TC_SETUP_CLSBPF:
 		if (!nfp_net_ebpf_capable(nn) ||

diff --git a/drivers/net/ethernet/netronome/nfp/flower/offload.c b/drivers/net/ethernet/netronome/nfp/flower/offload.c
index c47753f..7c6cab1 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/offload.c
+++ b/drivers/net/ethernet/netronome/nfp/flower/offload.c

@@ -470,6 +470,9 @@ static int nfp_flower_setup_tc_block_cb(enum tc_setup_type type,
 {
 	struct nfp_net *nn = cb_priv;
 
+	if (!tc_can_offload(nn->dp.netdev))
+		return -EOPNOTSUPP;
+
 	switch (type) {
 	case TC_SETUP_CLSFLOWER:
 		return nfp_flower_repr_offload(nn->app, nn->port->netdev,

diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index d0ae701..3481fd6 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c

@@ -799,6 +799,9 @@ static int dsa_slave_setup_tc_block_cb(enum tc_setup_type type, void *type_data,
 {
 	struct net_device *dev = cb_priv;
 
+	if (!tc_can_offload(dev))
+		return -EOPNOTSUPP;
+
 	switch (type) {
 	case TC_SETUP_CLSMATCHALL:
 		return dsa_slave_setup_tc_cls_matchall(dev, type_data, ingress);

diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 0e96cda..0636c19 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c

@@ -247,7 +247,7 @@ static void tcf_block_offload_cmd(struct tcf_block *block, struct Qdisc *q,
 	struct net_device *dev = q->dev_queue->dev;
 	struct tc_block_offload bo = {};
 
-	if (!tc_can_offload(dev))
+	if (!dev->netdev_ops->ndo_setup_tc)
 		return;
 	bo.command = command;
 	bo.binder_type = ei->binder_type;

-- 
2.9.5

From: Jiri Pirko <redacted>

Since the only user, mlx5 driver does the check in
mlx5e_setup_tc_block_cb, no need to check here.

Signed-off-by: Jiri Pirko <redacted>
---
 net/sched/cls_api.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 0636c19..81e1eb3 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c

@@ -1194,7 +1194,7 @@ static int tc_exts_setup_cb_egdev_call(struct tcf_exts *exts,
 		if (!a->ops->get_dev)
 			continue;
 		dev = a->ops->get_dev(a);
-		if (!dev || !tc_can_offload(dev))
+		if (!dev)
 			continue;
 		ret = tc_setup_cb_egdev_call(dev, type, type_data, err_stop);
 		if (ret < 0)

-- 
2.9.5

From: Jiri Pirko <redacted>

Since tc_can_offload is always called from block callback or egdev
callback, no need to check if ndo_setup_tc exists.

Signed-off-by: Jiri Pirko <redacted>
---
 include/net/pkt_cls.h | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index 0d24d22..4631f1a 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h

@@ -609,11 +609,7 @@ struct tc_cls_u32_offload {
 
 static inline bool tc_can_offload(const struct net_device *dev)
 {
-	if (!(dev->features & NETIF_F_HW_TC))
-		return false;
-	if (!dev->netdev_ops->ndo_setup_tc)
-		return false;
-	return true;
+	return dev->features & NETIF_F_HW_TC;
 }
 
 static inline bool tc_skip_hw(u32 flags)

-- 
2.9.5

On Wed, 25 Oct 2017 16:34:58 +0200, Jiri Pirko wrote:

From: Jiri Pirko <redacted>

This restores the original behaviour before the block callbacks were
introduced. Allow the drivers to do binding of block always, no matter
if the NETIF_F_HW_TC feature is on or off. Move the check to the block
callback which is called for rule insertion.

Reported-by: Alexander Duyck <redacted>
Signed-off-by: Jiri Pirko <redacted>

Could you explain why not add this check to the core?  IIUC every
driver will have to duplicate it if we want to keep the behaviour
of tc offloads we have today.

From: Jiri Pirko <jiri@resnulli.us>
Date: Wed, 25 Oct 2017 16:34:58 +0200

From: Jiri Pirko <redacted>

This restores the original behaviour before the block callbacks were
introduced. Allow the drivers to do binding of block always, no matter
if the NETIF_F_HW_TC feature is on or off. Move the check to the block
callback which is called for rule insertion.

Reported-by: Alexander Duyck <redacted>
Signed-off-by: Jiri Pirko <redacted>

I agree with Jakub's feedback, if every callback has to make this check
why not just do it in the core where the callback is invoked?

Thu, Oct 26, 2017 at 03:01:31AM CEST, davem@davemloft.net wrote:

From: Jiri Pirko <jiri@resnulli.us>
Date: Wed, 25 Oct 2017 16:34:58 +0200

quoted

From: Jiri Pirko <redacted>

This restores the original behaviour before the block callbacks were
introduced. Allow the drivers to do binding of block always, no matter
if the NETIF_F_HW_TC feature is on or off. Move the check to the block
callback which is called for rule insertion.

Reported-by: Alexander Duyck <redacted>
Signed-off-by: Jiri Pirko <redacted>

I agree with Jakub's feedback, if every callback has to make this check
why not just do it in the core where the callback is invoked?

We cannot add it to the core. The idea of the block callbacks is to get
independent of the "dev". For the block sharing, driver registers one
block callback for multiple devs. Core has no access to "dev".

On Wed, Oct 25, 2017 at 11:07 PM, Jiri Pirko [off-list ref] wrote:

Thu, Oct 26, 2017 at 03:01:31AM CEST, davem@davemloft.net wrote:

quoted

From: Jiri Pirko <jiri@resnulli.us>
Date: Wed, 25 Oct 2017 16:34:58 +0200

quoted

From: Jiri Pirko <redacted>

This restores the original behaviour before the block callbacks were
introduced. Allow the drivers to do binding of block always, no matter
if the NETIF_F_HW_TC feature is on or off. Move the check to the block
callback which is called for rule insertion.

Reported-by: Alexander Duyck <redacted>
Signed-off-by: Jiri Pirko <redacted>

I agree with Jakub's feedback, if every callback has to make this check
why not just do it in the core where the callback is invoked?

We cannot add it to the core. The idea of the block callbacks is to get
independent of the "dev". For the block sharing, driver registers one
block callback for multiple devs. Core has no access to "dev".

If the plan is to have one block represent multiple devices are we
then looking at the offload flag potentially being tied together for
all those devices then at some point in the future? I assume that
means that this workaround will have to eventually go away for some of
the Mellanox devices since it wouldn't make sense to have a netdev
pointer if the context block supports multiple netdevices. Do I have
that right? If so, is the idea to just assume the offload is always-on
(fixed) or do you have some other idea in mind for how to deal with a
feature that is controlled globally for the device versus a port?

- Alex

Thu, Oct 26, 2017 at 05:29:08PM CEST, alexander.duyck@gmail.com wrote:

On Wed, Oct 25, 2017 at 11:07 PM, Jiri Pirko [off-list ref] wrote:

quoted

Thu, Oct 26, 2017 at 03:01:31AM CEST, davem@davemloft.net wrote:

quoted

From: Jiri Pirko <jiri@resnulli.us>
Date: Wed, 25 Oct 2017 16:34:58 +0200

quoted

From: Jiri Pirko <redacted>

This restores the original behaviour before the block callbacks were
introduced. Allow the drivers to do binding of block always, no matter
if the NETIF_F_HW_TC feature is on or off. Move the check to the block
callback which is called for rule insertion.

Reported-by: Alexander Duyck <redacted>
Signed-off-by: Jiri Pirko <redacted>

I agree with Jakub's feedback, if every callback has to make this check
why not just do it in the core where the callback is invoked?

We cannot add it to the core. The idea of the block callbacks is to get
independent of the "dev". For the block sharing, driver registers one
block callback for multiple devs. Core has no access to "dev".

If the plan is to have one block represent multiple devices are we
then looking at the offload flag potentially being tied together for
all those devices then at some point in the future? I assume that
means that this workaround will have to eventually go away for some of
the Mellanox devices since it wouldn't make sense to have a netdev
pointer if the context block supports multiple netdevices. Do I have
that right? If so, is the idea to just assume the offload is always-on

You do.

(fixed) or do you have some other idea in mind for how to deal with a
feature that is controlled globally for the device versus a port?

I plan to check the offload feature flag for all ports that share the
block and allow the offload only in case all are on.
Also, there would be forbidden to turn the offload flag off in case
there is at least one offloaded rule for the block. This is how mlx5
does it.

On Thu, 26 Oct 2017 17:40:19 +0200, Jiri Pirko wrote:

Thu, Oct 26, 2017 at 05:29:08PM CEST, alexander.duyck@gmail.com wrote:

quoted

On Wed, Oct 25, 2017 at 11:07 PM, Jiri Pirko [off-list ref] wrote:  

quoted

Thu, Oct 26, 2017 at 03:01:31AM CEST, davem@davemloft.net wrote:  

quoted

From: Jiri Pirko <jiri@resnulli.us>
Date: Wed, 25 Oct 2017 16:34:58 +0200
 

quoted

From: Jiri Pirko <redacted>

This restores the original behaviour before the block callbacks were
introduced. Allow the drivers to do binding of block always, no matter
if the NETIF_F_HW_TC feature is on or off. Move the check to the block
callback which is called for rule insertion.

Reported-by: Alexander Duyck <redacted>
Signed-off-by: Jiri Pirko <redacted>  

I agree with Jakub's feedback, if every callback has to make this check
why not just do it in the core where the callback is invoked?  

We cannot add it to the core. The idea of the block callbacks is to get
independent of the "dev". For the block sharing, driver registers one
block callback for multiple devs. Core has no access to "dev".  

If the plan is to have one block represent multiple devices are we
then looking at the offload flag potentially being tied together for
all those devices then at some point in the future? I assume that
means that this workaround will have to eventually go away for some of
the Mellanox devices since it wouldn't make sense to have a netdev
pointer if the context block supports multiple netdevices. Do I have
that right? If so, is the idea to just assume the offload is always-on  

You do.

quoted

(fixed) or do you have some other idea in mind for how to deal with a
feature that is controlled globally for the device versus a port?  

I plan to check the offload feature flag for all ports that share the
block and allow the offload only in case all are on.

Clearly we draw the line between what should the responsibility of the
driver do and what belongs higher in the stack differently :)

Also, there would be forbidden to turn the offload flag off in case
there is at least one offloaded rule for the block. This is how mlx5
does it.

My grep-foo fails me I only see that check in nfp.  I though John
already did that in ixgbe.  And then everyone copied, since it's
sort-of UAPI?  Or at least users may come to expect certain
consistent behaviour.  Same story for phys_port_name for representors,
and OpenStack expect certain names by now.  That copying of behaviour
concerns me, not necessarily multiplication of code.

From: Jiri Pirko <jiri@resnulli.us>
Date: Wed, 25 Oct 2017 16:34:56 +0200

This patchset does a bit of cleanup of leftovers after block callbacks
patchset. The main part is patch 2, which restores the original handling
of tc offload feature flag.

Jiri, this series does not apply cleanly to net-next.

Also, you should really work out the block offload semantics
with Jakub, continuing the discussion of patch #2.

Thanks.

Sun, Oct 29, 2017 at 10:36:32AM CET, davem@davemloft.net wrote:

From: Jiri Pirko <jiri@resnulli.us>
Date: Wed, 25 Oct 2017 16:34:56 +0200

quoted

This patchset does a bit of cleanup of leftovers after block callbacks
patchset. The main part is patch 2, which restores the original handling
of tc offload feature flag.

Jiri, this series does not apply cleanly to net-next.

I will respin.

Also, you should really work out the block offload semantics
with Jakub, continuing the discussion of patch #2.

I have some ideas. I could let driver to enable/disable already
registered callback. Will look at it.

Sun, Oct 29, 2017 at 12:17:14PM CET, jiri@resnulli.us wrote:

Sun, Oct 29, 2017 at 10:36:32AM CET, davem@davemloft.net wrote:

quoted

From: Jiri Pirko <jiri@resnulli.us>
Date: Wed, 25 Oct 2017 16:34:56 +0200

quoted

This patchset does a bit of cleanup of leftovers after block callbacks
patchset. The main part is patch 2, which restores the original handling
of tc offload feature flag.

Jiri, this series does not apply cleanly to net-next.

I will respin.

quoted

Also, you should really work out the block offload semantics
with Jakub, continuing the discussion of patch #2.

I have some ideas. I could let driver to enable/disable already
registered callback. Will look at it.

The cb enable disable according to NETIF_F_HW_TC ethtool flag could be
done. It will require some additional work in many drivers. Most of the
drivers do not effect the flag change.

I will rebase and repost this patchset for now to fix the regression
and work on NETIF_F_HW_TC handling in a follow-up.