[PATCH 0/7] mlx5: Make eswitch_offloads a compile option

[PATCH 0/7] mlx5: Make eswitch_offloads a compile option

[Jes Sorensen]

Hi,

This changes the code to allow for eswitch_offloads to be compile time
option, reducing the size of the driver module for those who do not
need it.

The patches do it step by step by introducing stubs and then finally
gets rid of all the #ifdefs once the code can compile and link without
including eswitch_offloads.o

Unlike the patches that were discussed on the list back in January,
this does not try to remove eswitch support. It simply allows the
offloads to be disabled.

Cheers,
Jes


Jes Sorensen (7):
  mlx5: Allow support for eswitch offloads to be disabled
  mlx5: eswitch vlan functionality is only called if SRIOV_OFFLOADS is
    set
  mlx5: Disable {add,del}_offloaded_rule() code if eswitch offloads are
    disabled
  mlx5: Stub out eswitch offload vport functions
  mlx5: Stub out create_vport_rx_rule when eswitch_offloads disabled
  mlx5: Stub out sqs2vport functions
  mlx5: Do not build eswitch_offloads if CONFIG_MLX5_EN_ESWITCH_OFFLOADS
    is set

 drivers/net/ethernet/mellanox/mlx5/core/Kconfig    | 10 +++
 drivers/net/ethernet/mellanox/mlx5/core/Makefile   |  4 +-
 drivers/net/ethernet/mellanox/mlx5/core/eswitch.h  | 80 +++++++++++++++++++++-
 .../ethernet/mellanox/mlx5/core/eswitch_offloads.c |  1 +
 drivers/net/ethernet/mellanox/mlx5/core/main.c     |  2 +-
 5 files changed, 94 insertions(+), 3 deletions(-)

-- 
2.9.4

[PATCH 1/7] mlx5: Allow support for eswitch offloads to be disabled

[Jes Sorensen]

This allows users to disable eswitch offloads. Follow-on patches will
clean up how the eswitch_offloads code is being called and get rid of all
the #ifdefs.

Signed-off-by: Jes Sorensen <jsorensen@fb.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/Kconfig            | 10 ++++++++++
 drivers/net/ethernet/mellanox/mlx5/core/eswitch.h          | 11 +++++++++++
 drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c |  8 ++++++++
 drivers/net/ethernet/mellanox/mlx5/core/main.c             |  2 +-
 4 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/Kconfig b/drivers/net/ethernet/mellanox/mlx5/core/Kconfig
index 27251a7..27b409e 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/Kconfig
+++ b/drivers/net/ethernet/mellanox/mlx5/core/Kconfig
@@ -32,6 +32,16 @@ config MLX5_CORE_EN_DCB
 
 	  If unsure, set to Y
 
+config MLX5_CORE_EN_ESWITCH_OFFLOADS
+	bool "Enable support for Mellanox ESwitch Offload Support"
+	default y
+	depends on MLX5_CORE_EN
+	---help---
+	  Say Y here if you want to use Mellanox ESwitch offload support.
+	  If set to N, the driver will use the kernel's software implementation.
+
+	  If unsure, set to Y
+
 config MLX5_CORE_IPOIB
 	bool "Mellanox Technologies ConnectX-4 IPoIB offloads support"
 	depends on MLX5_CORE_EN
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
index b746f62..de4e5e8 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
@@ -243,8 +243,19 @@ struct mlx5_eswitch {
 	int                     mode;
 };
 
+#ifdef CONFIG_MLX5_EN_ESWITCH_OFFLOADS
 void esw_offloads_cleanup(struct mlx5_eswitch *esw, int nvports);
 int esw_offloads_init(struct mlx5_eswitch *esw, int nvports);
+#else
+static inline void esw_offloads_cleanup(struct mlx5_eswitch *esw, int nvports)
+{
+	return;
+}
+static inline int esw_offloads_init(struct mlx5_eswitch *esw, int nvports)
+{
+	return -EOPNOTSUPP;
+}
+#endif
 
 /* E-Switch API */
 int mlx5_eswitch_init(struct mlx5_core_dev *dev);
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
index f991f66..e78dec1 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
@@ -393,6 +393,7 @@ int mlx5_eswitch_sqs2vport_start(struct mlx5_eswitch *esw,
 	return err;
 }
 
+#ifdef CONFIG_MLX5_EN_ESWITCH_OFFLOADS
 static int esw_add_fdb_miss_rule(struct mlx5_eswitch *esw)
 {
 	struct mlx5_flow_act flow_act = {0};
@@ -425,6 +426,7 @@ static int esw_add_fdb_miss_rule(struct mlx5_eswitch *esw)
 	kvfree(spec);
 	return err;
 }
+#endif
 
 #define ESW_OFFLOADS_NUM_GROUPS  4
 
@@ -475,6 +477,7 @@ static void esw_destroy_offloads_fast_fdb_table(struct mlx5_eswitch *esw)
 
 #define MAX_PF_SQ 256
 
+#ifdef CONFIG_MLX5_EN_ESWITCH_OFFLOADS
 static int esw_create_offloads_fdb_tables(struct mlx5_eswitch *esw, int nvports)
 {
 	int inlen = MLX5_ST_SZ_BYTES(create_flow_group_in);
@@ -665,6 +668,7 @@ static void esw_destroy_vport_rx_group(struct mlx5_eswitch *esw)
 {
 	mlx5_destroy_flow_group(esw->offloads.vport_rx_group);
 }
+#endif
 
 struct mlx5_flow_handle *
 mlx5_eswitch_create_vport_rx_rule(struct mlx5_eswitch *esw, int vport, u32 tirn)
@@ -733,6 +737,7 @@ static int esw_offloads_start(struct mlx5_eswitch *esw)
 	return err;
 }
 
+#ifdef CONFIG_MLX5_EN_ESWITCH_OFFLOADS
 int esw_offloads_init(struct mlx5_eswitch *esw, int nvports)
 {
 	struct mlx5_eswitch_rep *rep;
@@ -791,6 +796,7 @@ int esw_offloads_init(struct mlx5_eswitch *esw, int nvports)
 
 	return err;
 }
+#endif
 
 static int esw_offloads_stop(struct mlx5_eswitch *esw)
 {
@@ -813,6 +819,7 @@ static int esw_offloads_stop(struct mlx5_eswitch *esw)
 	return err;
 }
 
+#ifdef CONFIG_MLX5_EN_ESWITCH_OFFLOADS
 void esw_offloads_cleanup(struct mlx5_eswitch *esw, int nvports)
 {
 	struct mlx5_eswitch_rep *rep;
@@ -829,6 +836,7 @@ void esw_offloads_cleanup(struct mlx5_eswitch *esw, int nvports)
 	esw_destroy_offloads_table(esw);
 	esw_destroy_offloads_fdb_tables(esw);
 }
+#endif
 
 static int esw_mode_from_devlink(u16 mode, u16 *mlx5_mode)
 {
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/main.c b/drivers/net/ethernet/mellanox/mlx5/core/main.c
index 0c123d5..3d8a41a 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/main.c
@@ -1275,7 +1275,7 @@ struct mlx5_core_event_handler {
 };
 
 static const struct devlink_ops mlx5_devlink_ops = {
-#ifdef CONFIG_MLX5_CORE_EN
+#ifdef CONFIG_MLX5_EN_ESWITCH_OFFLOADS
 	.eswitch_mode_set = mlx5_devlink_eswitch_mode_set,
 	.eswitch_mode_get = mlx5_devlink_eswitch_mode_get,
 	.eswitch_inline_mode_set = mlx5_devlink_eswitch_inline_mode_set,
-- 
2.9.4

[PATCH 2/7] mlx5: eswitch vlan functionality is only called if SRIOV_OFFLOADS is set

[Jes Sorensen]

This allows not compiling this code if eswitch offloads are disabled.

Signed-off-by: Jes Sorensen <jsorensen@fb.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/eswitch.h          | 13 +++++++++++++
 drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c |  2 ++
 2 files changed, 15 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
index de4e5e8..2bf5299 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
@@ -338,10 +338,23 @@ void mlx5_eswitch_unregister_vport_rep(struct mlx5_eswitch *esw,
 				       int vport_index);
 struct net_device *mlx5_eswitch_get_uplink_netdev(struct mlx5_eswitch *esw);
 
+#ifdef CONFIG_MLX5_EN_ESWITCH_OFFLOADS
 int mlx5_eswitch_add_vlan_action(struct mlx5_eswitch *esw,
 				 struct mlx5_esw_flow_attr *attr);
 int mlx5_eswitch_del_vlan_action(struct mlx5_eswitch *esw,
 				 struct mlx5_esw_flow_attr *attr);
+#else
+static inline int mlx5_eswitch_add_vlan_action(struct mlx5_eswitch *esw,
+					       struct mlx5_esw_flow_attr *attr)
+{
+	return -EOPNOTSUPP;
+}
+static inline int mlx5_eswitch_del_vlan_action(struct mlx5_eswitch *esw,
+					       struct mlx5_esw_flow_attr *attr)
+{
+	return -EOPNOTSUPP;
+}
+#endif
 int __mlx5_eswitch_set_vport_vlan(struct mlx5_eswitch *esw,
 				  int vport, u16 vlan, u8 qos, u8 set_flags);
 
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
index e78dec1..8d0af1f 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
@@ -122,6 +122,7 @@ mlx5_eswitch_del_offloaded_rule(struct mlx5_eswitch *esw,
 	esw->offloads.num_flows--;
 }
 
+#ifdef CONFIG_MLX5_EN_ESWITCH_OFFLOADS
 static int esw_set_global_vlan_pop(struct mlx5_eswitch *esw, u8 val)
 {
 	struct mlx5_eswitch_rep *rep;
@@ -301,6 +302,7 @@ int mlx5_eswitch_del_vlan_action(struct mlx5_eswitch *esw,
 out:
 	return err;
 }
+#endif
 
 static struct mlx5_flow_handle *
 mlx5_eswitch_add_send_to_vport_rule(struct mlx5_eswitch *esw, int vport, u32 sqn)
-- 
2.9.4

[PATCH 3/7] mlx5: Disable {add,del}_offloaded_rule() code if eswitch offloads are disabled

[Jes Sorensen]

This code will not be called if CONFIG_MLX5_EN_ESWITCH_OFFLOADS is disabled.

Signed-off-by: Jes Sorensen <jsorensen@fb.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/eswitch.h       | 17 +++++++++++++++++
 .../net/ethernet/mellanox/mlx5/core/eswitch_offloads.c  |  2 +-
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
index 2bf5299..c2b0c02 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
@@ -286,6 +286,7 @@ int mlx5_eswitch_get_vport_stats(struct mlx5_eswitch *esw,
 struct mlx5_flow_spec;
 struct mlx5_esw_flow_attr;
 
+#ifdef CONFIG_MLX5_EN_ESWITCH_OFFLOADS
 struct mlx5_flow_handle *
 mlx5_eswitch_add_offloaded_rule(struct mlx5_eswitch *esw,
 				struct mlx5_flow_spec *spec,
@@ -294,6 +295,22 @@ void
 mlx5_eswitch_del_offloaded_rule(struct mlx5_eswitch *esw,
 				struct mlx5_flow_handle *rule,
 				struct mlx5_esw_flow_attr *attr);
+#else
+static inline struct mlx5_flow_handle *
+mlx5_eswitch_add_offloaded_rule(struct mlx5_eswitch *esw,
+				struct mlx5_flow_spec *spec,
+				struct mlx5_esw_flow_attr *attr)
+{
+	return ERR_PTR(-EOPNOTSUPP);
+}
+static inline void
+mlx5_eswitch_del_offloaded_rule(struct mlx5_eswitch *esw,
+				struct mlx5_flow_handle *rule,
+				struct mlx5_esw_flow_attr *attr)
+{
+	return;
+}
+#endif
 
 struct mlx5_flow_handle *
 mlx5_eswitch_create_vport_rx_rule(struct mlx5_eswitch *esw, int vport, u32 tirn);
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
index 8d0af1f..12505ba 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
@@ -43,6 +43,7 @@ enum {
 	FDB_SLOW_PATH
 };
 
+#ifdef CONFIG_MLX5_EN_ESWITCH_OFFLOADS
 struct mlx5_flow_handle *
 mlx5_eswitch_add_offloaded_rule(struct mlx5_eswitch *esw,
 				struct mlx5_flow_spec *spec,
@@ -122,7 +123,6 @@ mlx5_eswitch_del_offloaded_rule(struct mlx5_eswitch *esw,
 	esw->offloads.num_flows--;
 }
 
-#ifdef CONFIG_MLX5_EN_ESWITCH_OFFLOADS
 static int esw_set_global_vlan_pop(struct mlx5_eswitch *esw, u8 val)
 {
 	struct mlx5_eswitch_rep *rep;
-- 
2.9.4

[PATCH 4/7] mlx5: Stub out eswitch offload vport functions

[Jes Sorensen]

This code isn't called if CONFIG_MLX5_EN_ESWITCH_OFFLOADS isn't enabled

Signed-off-by: Jes Sorensen <jsorensen@fb.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/eswitch.h    | 20 +++++++++++++++++++-
 .../ethernet/mellanox/mlx5/core/eswitch_offloads.c   |  2 ++
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
index c2b0c02..6397384 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
@@ -348,6 +348,7 @@ int mlx5_devlink_eswitch_inline_mode_get(struct devlink *devlink, u8 *mode);
 int mlx5_eswitch_inline_mode_get(struct mlx5_eswitch *esw, int nvfs, u8 *mode);
 int mlx5_devlink_eswitch_encap_mode_set(struct devlink *devlink, u8 encap);
 int mlx5_devlink_eswitch_encap_mode_get(struct devlink *devlink, u8 *encap);
+#ifdef CONFIG_MLX5_EN_ESWITCH_OFFLOADS
 void mlx5_eswitch_register_vport_rep(struct mlx5_eswitch *esw,
 				     int vport_index,
 				     struct mlx5_eswitch_rep *rep);
@@ -355,12 +356,29 @@ void mlx5_eswitch_unregister_vport_rep(struct mlx5_eswitch *esw,
 				       int vport_index);
 struct net_device *mlx5_eswitch_get_uplink_netdev(struct mlx5_eswitch *esw);
 
-#ifdef CONFIG_MLX5_EN_ESWITCH_OFFLOADS
 int mlx5_eswitch_add_vlan_action(struct mlx5_eswitch *esw,
 				 struct mlx5_esw_flow_attr *attr);
 int mlx5_eswitch_del_vlan_action(struct mlx5_eswitch *esw,
 				 struct mlx5_esw_flow_attr *attr);
 #else
+static inline void
+mlx5_eswitch_register_vport_rep(struct mlx5_eswitch *esw, int vport_index,
+				struct mlx5_eswitch_rep *rep)
+{
+	return;
+}
+static inline void
+mlx5_eswitch_unregister_vport_rep(struct mlx5_eswitch *esw, int vport_index)
+{
+	return;
+}
+
+static inline struct net_device *
+mlx5_eswitch_get_uplink_netdev(struct mlx5_eswitch *esw)
+{
+	return NULL;
+}
+
 static inline int mlx5_eswitch_add_vlan_action(struct mlx5_eswitch *esw,
 					       struct mlx5_esw_flow_attr *attr)
 {
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
index 12505ba..9278a33 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
@@ -1126,6 +1126,7 @@ int mlx5_devlink_eswitch_encap_mode_get(struct devlink *devlink, u8 *encap)
 	return 0;
 }
 
+#ifdef CONFIG_MLX5_EN_ESWITCH_OFFLOADS
 void mlx5_eswitch_register_vport_rep(struct mlx5_eswitch *esw,
 				     int vport_index,
 				     struct mlx5_eswitch_rep *__rep)
@@ -1170,3 +1171,4 @@ struct net_device *mlx5_eswitch_get_uplink_netdev(struct mlx5_eswitch *esw)
 	rep = &offloads->vport_reps[UPLINK_REP_INDEX];
 	return rep->netdev;
 }
+#endif
-- 
2.9.4

[PATCH 5/7] mlx5: Stub out create_vport_rx_rule when eswitch_offloads disabled

[Jes Sorensen]

One more vport related function to disable.

Signed-off-by: Jes Sorensen <jsorensen@fb.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/eswitch.h          | 11 ++++++++---
 drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c |  2 +-
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
index 6397384..9a395f3 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
@@ -295,6 +295,9 @@ void
 mlx5_eswitch_del_offloaded_rule(struct mlx5_eswitch *esw,
 				struct mlx5_flow_handle *rule,
 				struct mlx5_esw_flow_attr *attr);
+struct mlx5_flow_handle *
+mlx5_eswitch_create_vport_rx_rule(struct mlx5_eswitch *esw, int vport, u32 tirn);
+
 #else
 static inline struct mlx5_flow_handle *
 mlx5_eswitch_add_offloaded_rule(struct mlx5_eswitch *esw,
@@ -310,11 +313,13 @@ mlx5_eswitch_del_offloaded_rule(struct mlx5_eswitch *esw,
 {
 	return;
 }
+static inline struct mlx5_flow_handle *
+mlx5_eswitch_create_vport_rx_rule(struct mlx5_eswitch *esw, int vport, u32 tirn)
+{
+	return ERR_PTR(-EOPNOTSUPP);
+}
 #endif
 
-struct mlx5_flow_handle *
-mlx5_eswitch_create_vport_rx_rule(struct mlx5_eswitch *esw, int vport, u32 tirn);
-
 enum {
 	SET_VLAN_STRIP	= BIT(0),
 	SET_VLAN_INSERT	= BIT(1)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
index 9278a33..2bda8f5 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
@@ -670,7 +670,6 @@ static void esw_destroy_vport_rx_group(struct mlx5_eswitch *esw)
 {
 	mlx5_destroy_flow_group(esw->offloads.vport_rx_group);
 }
-#endif
 
 struct mlx5_flow_handle *
 mlx5_eswitch_create_vport_rx_rule(struct mlx5_eswitch *esw, int vport, u32 tirn)
@@ -710,6 +709,7 @@ mlx5_eswitch_create_vport_rx_rule(struct mlx5_eswitch *esw, int vport, u32 tirn)
 	kvfree(spec);
 	return flow_rule;
 }
+#endif
 
 static int esw_offloads_start(struct mlx5_eswitch *esw)
 {
-- 
2.9.4

[PATCH 6/7] mlx5: Stub out sqs2vport functions

[Jes Sorensen]

These aren't called if CONFIG_MLX5_EN_ESWITCH_OFFLOADS isn't enabled,
so do not build them.

Signed-off-by: Jes Sorensen <jsorensen@fb.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/eswitch.h          | 14 ++++++++++++++
 drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c |  2 --
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
index 9a395f3..f1a597b 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
@@ -340,11 +340,25 @@ struct mlx5_esw_flow_attr {
 	struct mlx5e_tc_flow_parse_attr *parse_attr;
 };
 
+#ifdef CONFIG_MLX5_EN_ESWITCH_OFFLOADS
 int mlx5_eswitch_sqs2vport_start(struct mlx5_eswitch *esw,
 				 struct mlx5_eswitch_rep *rep,
 				 u16 *sqns_array, int sqns_num);
 void mlx5_eswitch_sqs2vport_stop(struct mlx5_eswitch *esw,
 				 struct mlx5_eswitch_rep *rep);
+#else
+static inline int mlx5_eswitch_sqs2vport_start(struct mlx5_eswitch *esw,
+					       struct mlx5_eswitch_rep *rep,
+					       u16 *sqns_array, int sqns_num)
+{
+	return -EOPNOTSUPP;
+}
+static inline void mlx5_eswitch_sqs2vport_stop(struct mlx5_eswitch *esw,
+					       struct mlx5_eswitch_rep *rep)
+{
+	return;
+}
+#endif
 
 int mlx5_devlink_eswitch_mode_set(struct devlink *devlink, u16 mode);
 int mlx5_devlink_eswitch_mode_get(struct devlink *devlink, u16 *mode);
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
index 2bda8f5..638b84f 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
@@ -302,7 +302,6 @@ int mlx5_eswitch_del_vlan_action(struct mlx5_eswitch *esw,
 out:
 	return err;
 }
-#endif
 
 static struct mlx5_flow_handle *
 mlx5_eswitch_add_send_to_vport_rule(struct mlx5_eswitch *esw, int vport, u32 sqn)
@@ -395,7 +394,6 @@ int mlx5_eswitch_sqs2vport_start(struct mlx5_eswitch *esw,
 	return err;
 }
 
-#ifdef CONFIG_MLX5_EN_ESWITCH_OFFLOADS
 static int esw_add_fdb_miss_rule(struct mlx5_eswitch *esw)
 {
 	struct mlx5_flow_act flow_act = {0};
-- 
2.9.4

[PATCH 7/7] mlx5: Do not build eswitch_offloads if CONFIG_MLX5_EN_ESWITCH_OFFLOADS is set

[Jes Sorensen]

This gets rid of the temporary #ifdef spaghetti and allows the code to
compile without offload support enabled.

Signed-off-by: Jes Sorensen <jsorensen@fb.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/Makefile           | 4 +++-
 drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c | 9 ---------
 2 files changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/Makefile b/drivers/net/ethernet/mellanox/mlx5/core/Makefile
index 9e64461..5967b97 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/Makefile
+++ b/drivers/net/ethernet/mellanox/mlx5/core/Makefile
@@ -5,11 +5,13 @@ mlx5_core-y :=	main.o cmd.o debugfs.o fw.o eq.o uar.o pagealloc.o \
 		mad.o transobj.o vport.o sriov.o fs_cmd.o fs_core.o \
 		fs_counters.o rl.o lag.o dev.o
 
-mlx5_core-$(CONFIG_MLX5_CORE_EN) += wq.o eswitch.o eswitch_offloads.o \
+mlx5_core-$(CONFIG_MLX5_CORE_EN) += wq.o eswitch.o \
 		en_main.o en_common.o en_fs.o en_ethtool.o en_tx.o \
 		en_rx.o en_rx_am.o en_txrx.o en_clock.o vxlan.o \
 		en_tc.o en_arfs.o en_rep.o en_fs_ethtool.o en_selftest.o
 
 mlx5_core-$(CONFIG_MLX5_CORE_EN_DCB) +=  en_dcbnl.o
 
+mlx5_core-$(CONFIG_MLX5_EN_ESWITCH_OFFLOADS) +=  en_eswitch_offloads.o
+
 mlx5_core-$(CONFIG_MLX5_CORE_IPOIB) += ipoib.o
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
index 638b84f..320d1c5 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
@@ -43,7 +43,6 @@ enum {
 	FDB_SLOW_PATH
 };
 
-#ifdef CONFIG_MLX5_EN_ESWITCH_OFFLOADS
 struct mlx5_flow_handle *
 mlx5_eswitch_add_offloaded_rule(struct mlx5_eswitch *esw,
 				struct mlx5_flow_spec *spec,
@@ -426,7 +425,6 @@ static int esw_add_fdb_miss_rule(struct mlx5_eswitch *esw)
 	kvfree(spec);
 	return err;
 }
-#endif
 
 #define ESW_OFFLOADS_NUM_GROUPS  4
 
@@ -477,7 +475,6 @@ static void esw_destroy_offloads_fast_fdb_table(struct mlx5_eswitch *esw)
 
 #define MAX_PF_SQ 256
 
-#ifdef CONFIG_MLX5_EN_ESWITCH_OFFLOADS
 static int esw_create_offloads_fdb_tables(struct mlx5_eswitch *esw, int nvports)
 {
 	int inlen = MLX5_ST_SZ_BYTES(create_flow_group_in);
@@ -737,7 +734,6 @@ static int esw_offloads_start(struct mlx5_eswitch *esw)
 	return err;
 }
 
-#ifdef CONFIG_MLX5_EN_ESWITCH_OFFLOADS
 int esw_offloads_init(struct mlx5_eswitch *esw, int nvports)
 {
 	struct mlx5_eswitch_rep *rep;
@@ -796,7 +792,6 @@ int esw_offloads_init(struct mlx5_eswitch *esw, int nvports)
 
 	return err;
 }
-#endif
 
 static int esw_offloads_stop(struct mlx5_eswitch *esw)
 {
@@ -819,7 +814,6 @@ static int esw_offloads_stop(struct mlx5_eswitch *esw)
 	return err;
 }
 
-#ifdef CONFIG_MLX5_EN_ESWITCH_OFFLOADS
 void esw_offloads_cleanup(struct mlx5_eswitch *esw, int nvports)
 {
 	struct mlx5_eswitch_rep *rep;
@@ -836,7 +830,6 @@ void esw_offloads_cleanup(struct mlx5_eswitch *esw, int nvports)
 	esw_destroy_offloads_table(esw);
 	esw_destroy_offloads_fdb_tables(esw);
 }
-#endif
 
 static int esw_mode_from_devlink(u16 mode, u16 *mlx5_mode)
 {
@@ -1124,7 +1117,6 @@ int mlx5_devlink_eswitch_encap_mode_get(struct devlink *devlink, u8 *encap)
 	return 0;
 }
 
-#ifdef CONFIG_MLX5_EN_ESWITCH_OFFLOADS
 void mlx5_eswitch_register_vport_rep(struct mlx5_eswitch *esw,
 				     int vport_index,
 				     struct mlx5_eswitch_rep *__rep)
@@ -1169,4 +1161,3 @@ struct net_device *mlx5_eswitch_get_uplink_netdev(struct mlx5_eswitch *esw)
 	rep = &offloads->vport_reps[UPLINK_REP_INDEX];
 	return rep->netdev;
 }
-#endif
-- 
2.9.4

Re: [PATCH 7/7] mlx5: Do not build eswitch_offloads if CONFIG_MLX5_EN_ESWITCH_OFFLOADS is set

[Or Gerlitz]

On Sat, May 27, 2017 at 12:16 AM, Jes Sorensen <jes.sorensen@gmail.com> wrote:
> This gets rid of the temporary #ifdef spaghetti and allows the code to
> compile without offload support enabled.

Hi Jes,

I am pretty sure we can do that exercise you're up to without any
spaghetti cooking and even put more code under that CONFIG directive
(en_rep.c), I'll take that with Saeed.

Just wondering, you are motivated by a wish to put some mlx5
functionalities under their own CONFIG directives which could be
useful when backporting the latest upstream driver into older kernel
and being able not to deal with parts of it, right? in that respect,
are you using SRIOV but not the offloads mode?

Or.

Re: [PATCH 7/7] mlx5: Do not build eswitch_offloads if CONFIG_MLX5_EN_ESWITCH_OFFLOADS is set

[Jes Sorensen]

On 05/27/2017 05:02 PM, Or Gerlitz wrote:
> On Sat, May 27, 2017 at 12:16 AM, Jes Sorensen <jes.sorensen@gmail.com> wrote:
>> This gets rid of the temporary #ifdef spaghetti and allows the code to
>> compile without offload support enabled.
> 
> Hi Jes,
> 
> I am pretty sure we can do that exercise you're up to without any
> spaghetti cooking and even put more code under that CONFIG directive
> (en_rep.c), I'll take that with Saeed.

Hi Or,

I want to avoid adding #ifdef CONFIG_foo to the main code in order to 
keep it readable. I did it gradually to make sure I didn't break 
anything and to allow for it to be bisected in case something did break. 
If we can move out more code from places like en_rep.c into 
eswitch_offload.c and get it disabled that way that would be great, but 
I like to limit the number of #ifdefs we add to the actual code.

> Just wondering, you are motivated by a wish to put some mlx5
> functionalities under their own CONFIG directives which could be
> useful when backporting the latest upstream driver into older kernel
> and being able not to deal with parts of it, right? in that respect,
> are you using SRIOV but not the offloads mode?

The motivation is two-fold, the primary is to be able to disable 
features not being used for those who compile a custom kernel and who 
wish to reduce the codebase compiled. It also makes it more flexible 
when back porting the code to older kernels since it is easier to pick 
out a smaller subset. I was going to look into making TC support etc. 
optional next, but I wanted to have a discussion about this patchset first.

Cheers,
Jes

Re: [PATCH 7/7] mlx5: Do not build eswitch_offloads if CONFIG_MLX5_EN_ESWITCH_OFFLOADS is set

[Or Gerlitz]

On Sun, May 28, 2017 at 5:23 AM, Jes Sorensen <jes.sorensen@gmail.com> wrote:
> On 05/27/2017 05:02 PM, Or Gerlitz wrote:
>>
>> On Sat, May 27, 2017 at 12:16 AM, Jes Sorensen <jes.sorensen@gmail.com>
>> wrote:
>>>
>>> This gets rid of the temporary #ifdef spaghetti and allows the code to
>>> compile without offload support enabled.

>> I am pretty sure we can do that exercise you're up to without any
>> spaghetti cooking and even put more code under that CONFIG directive
>> (en_rep.c), I'll take that with Saeed.

> I want to avoid adding #ifdef CONFIG_foo to the main code in order to keep
> it readable. I did it gradually to make sure I didn't break anything and to
> allow for it to be bisected in case something did break. If we can move out
> more code from places like en_rep.c into eswitch_offload.c and get it
> disabled that way that would be great, but I like to limit the number of
> #ifdefs we add to the actual code.

FWIW (see below), squashing your seven patches to one resulted in a
fairly simple/clear
patch, so if we go that way, no need to have seven commits just for this piece.

>> Just wondering, you are motivated by a wish to put some mlx5
>> functionalities under their own CONFIG directives which could be
>> useful when backporting the latest upstream driver into older kernel
>> and being able not to deal with parts of it, right? in that respect,
>> are you using SRIOV but not the offloads mode?

> The motivation is two-fold, the primary is to be able to disable features
> not being used for those who compile a custom kernel and who wish to reduce
> the codebase compiled. It also makes it more flexible when back porting the
> code to older kernels since it is easier to pick out a smaller subset. I was
> going to look into making TC support etc. optional next, but I wanted to
> have a discussion about this patchset first.

OKay, I got you.

Re SRIOV, I don't think it would be correct to break the support info few
CONFIG directives. If we want to allow someone to build the driver w.o
SRIOV that's fine, but I don't think we should further go down and disable
some of the SRIOV sub-modes.

Re TC offload support, that's make sense.

Or.

Re: [PATCH 7/7] mlx5: Do not build eswitch_offloads if CONFIG_MLX5_EN_ESWITCH_OFFLOADS is set

[Jes Sorensen]

On 05/28/2017 02:03 AM, Or Gerlitz wrote:
> On Sun, May 28, 2017 at 5:23 AM, Jes Sorensen <jes.sorensen@gmail.com> wrote:
>> On 05/27/2017 05:02 PM, Or Gerlitz wrote:
>>>
>>> On Sat, May 27, 2017 at 12:16 AM, Jes Sorensen <jes.sorensen@gmail.com>
>>> wrote:
>>>>
>>>> This gets rid of the temporary #ifdef spaghetti and allows the code to
>>>> compile without offload support enabled.
> 
>>> I am pretty sure we can do that exercise you're up to without any
>>> spaghetti cooking and even put more code under that CONFIG directive
>>> (en_rep.c), I'll take that with Saeed.
> 
>> I want to avoid adding #ifdef CONFIG_foo to the main code in order to keep
>> it readable. I did it gradually to make sure I didn't break anything and to
>> allow for it to be bisected in case something did break. If we can move out
>> more code from places like en_rep.c into eswitch_offload.c and get it
>> disabled that way that would be great, but I like to limit the number of
>> #ifdefs we add to the actual code.
> 
> FWIW (see below), squashing your seven patches to one resulted in a
> fairly simple/clear
> patch, so if we go that way, no need to have seven commits just for this piece.

Squashing patches into jumbo patches is inherently broken and bad coding 
practice! It makes it way more complicated to debug and bisect in case a 
minor detail broke in the process.

>>> Just wondering, you are motivated by a wish to put some mlx5
>>> functionalities under their own CONFIG directives which could be
>>> useful when backporting the latest upstream driver into older kernel
>>> and being able not to deal with parts of it, right? in that respect,
>>> are you using SRIOV but not the offloads mode?
> 
>> The motivation is two-fold, the primary is to be able to disable features
>> not being used for those who compile a custom kernel and who wish to reduce
>> the codebase compiled. It also makes it more flexible when back porting the
>> code to older kernels since it is easier to pick out a smaller subset. I was
>> going to look into making TC support etc. optional next, but I wanted to
>> have a discussion about this patchset first.
> 
> OKay, I got you.
> 
> Re SRIOV, I don't think it would be correct to break the support info few
> CONFIG directives. If we want to allow someone to build the driver w.o
> SRIOV that's fine, but I don't think we should further go down and disable
> some of the SRIOV sub-modes.
> 
> Re TC offload support, that's make sense.

OK, so disabling SRIOV and disabling TC makes sense - I'll look at that.

Jes

Re: [PATCH 7/7] mlx5: Do not build eswitch_offloads if CONFIG_MLX5_EN_ESWITCH_OFFLOADS is set

[Or Gerlitz]

On Fri, Jun 2, 2017 at 11:22 PM, Jes Sorensen <jsorensen@fb.com> wrote:
> On 05/28/2017 02:03 AM, Or Gerlitz wrote:
>>
>> On Sun, May 28, 2017 at 5:23 AM, Jes Sorensen <jes.sorensen@gmail.com>
>> wrote:
>>>
>>> On 05/27/2017 05:02 PM, Or Gerlitz wrote:
>>>>
>>>>
>>>> On Sat, May 27, 2017 at 12:16 AM, Jes Sorensen <jes.sorensen@gmail.com>
>>>> wrote:
>>>>>
>>>>>
>>>>> This gets rid of the temporary #ifdef spaghetti and allows the code to
>>>>> compile without offload support enabled.
>>
>>
>>>> I am pretty sure we can do that exercise you're up to without any
>>>> spaghetti cooking and even put more code under that CONFIG directive
>>>> (en_rep.c), I'll take that with Saeed.
>>
>>
>>> I want to avoid adding #ifdef CONFIG_foo to the main code in order to
>>> keep
>>> it readable. I did it gradually to make sure I didn't break anything and
>>> to
>>> allow for it to be bisected in case something did break. If we can move
>>> out
>>> more code from places like en_rep.c into eswitch_offload.c and get it
>>> disabled that way that would be great, but I like to limit the number of
>>> #ifdefs we add to the actual code.
>>
>>
>> FWIW (see below), squashing your seven patches to one resulted in a
>> fairly simple/clear
>> patch, so if we go that way, no need to have seven commits just for this
>> piece.
>
>
> Squashing patches into jumbo patches is inherently broken and bad coding
> practice! It makes it way more complicated to debug and bisect in case a
> minor detail broke in the process.

Not that pure LOC ##-s is the only/deep measurement, but your overall
changes in the the seven patch series account to:

 5 files changed, 94 insertions(+), 3 deletions(-)

and by no mean this is jumbo or inherently broken and bad coded, so
please slow down please, I looked with care on the resulted patch and
said it's basically ok.


>> Re SRIOV, I don't think it would be correct to break the support info few
>> CONFIG directives. If we want to allow someone to build the driver w.o
>> SRIOV that's fine, but I don't think we should further go down and disable
>> some of the SRIOV sub-modes.

>> Re TC offload support, that's make sense.

> OK, so disabling SRIOV and disabling TC makes sense - I'll look at that.

I think Saeed wants us to conduct that exercise, let me check with him
and get back to you

Re: [PATCH 7/7] mlx5: Do not build eswitch_offloads if CONFIG_MLX5_EN_ESWITCH_OFFLOADS is set

[Saeed Mahameed]

On Sat, Jun 3, 2017 at 10:37 PM, Or Gerlitz <gerlitz.or@gmail.com> wrote:
> On Fri, Jun 2, 2017 at 11:22 PM, Jes Sorensen <jsorensen@fb.com> wrote:
>> On 05/28/2017 02:03 AM, Or Gerlitz wrote:
>>>
>>> On Sun, May 28, 2017 at 5:23 AM, Jes Sorensen <jes.sorensen@gmail.com>
>>> wrote:
>>>>
>>>> On 05/27/2017 05:02 PM, Or Gerlitz wrote:
>>>>>
>>>>>
>>>>> On Sat, May 27, 2017 at 12:16 AM, Jes Sorensen <jes.sorensen@gmail.com>
>>>>> wrote:
>>>>>>
>>>>>>
>>>>>> This gets rid of the temporary #ifdef spaghetti and allows the code to
>>>>>> compile without offload support enabled.
>>>
>>>
>>>>> I am pretty sure we can do that exercise you're up to without any
>>>>> spaghetti cooking and even put more code under that CONFIG directive
>>>>> (en_rep.c), I'll take that with Saeed.
>>>
>>>
>>>> I want to avoid adding #ifdef CONFIG_foo to the main code in order to
>>>> keep
>>>> it readable. I did it gradually to make sure I didn't break anything and
>>>> to
>>>> allow for it to be bisected in case something did break. If we can move
>>>> out
>>>> more code from places like en_rep.c into eswitch_offload.c and get it
>>>> disabled that way that would be great, but I like to limit the number of
>>>> #ifdefs we add to the actual code.
>>>
>>>
>>> FWIW (see below), squashing your seven patches to one resulted in a
>>> fairly simple/clear
>>> patch, so if we go that way, no need to have seven commits just for this
>>> piece.
>>
>>
>> Squashing patches into jumbo patches is inherently broken and bad coding
>> practice! It makes it way more complicated to debug and bisect in case a
>> minor detail broke in the process.
>
> Not that pure LOC ##-s is the only/deep measurement, but your overall
> changes in the the seven patch series account to:
>
>  5 files changed, 94 insertions(+), 3 deletions(-)
>
> and by no mean this is jumbo or inherently broken and bad coded, so
> please slow down please, I looked with care on the resulted patch and
> said it's basically ok.
>
>
>>> Re SRIOV, I don't think it would be correct to break the support info few
>>> CONFIG directives. If we want to allow someone to build the driver w.o
>>> SRIOV that's fine, but I don't think we should further go down and disable
>>> some of the SRIOV sub-modes.
>
>>> Re TC offload support, that's make sense.
>
>> OK, so disabling SRIOV and disabling TC makes sense - I'll look at that.
>
> I think Saeed wants us to conduct that exercise, let me check with him
> and get back to you

disabling SRIOV in all the kernel can do the trick, but we want
something more flexible, yet simple.
eswitch is required __ONLY__ for SRIOV, in the light of that fact we
can have CONFIG_MLX5_SRIOV which depends on kernel SRIOV
kconfig directive, that will eliminate  MLX5 sriov support (basically
compile out sriov.c, eswitch.c, eswitch_offloads.c and en_rep.c).

and stub out some sriov NDOs and some little API calls from the main
flows to the above mlx5 sub-modules.
Also we will need to compile out some en_tc.c offloads which meant to
program the eswitch they also call the eswitch_offloads API.

Re: [PATCH 7/7] mlx5: Do not build eswitch_offloads if CONFIG_MLX5_EN_ESWITCH_OFFLOADS is set

[Or Gerlitz]

On Sat, Jun 3, 2017 at 10:37 PM, Or Gerlitz <gerlitz.or@gmail.com> wrote:
> On Fri, Jun 2, 2017 at 11:22 PM, Jes Sorensen <jsorensen@fb.com> wrote:
>> On 05/28/2017 02:03 AM, Or Gerlitz wrote:
>>>
>>> On Sun, May 28, 2017 at 5:23 AM, Jes Sorensen <jes.sorensen@gmail.com>
>>> wrote:
>>>>
>>>> On 05/27/2017 05:02 PM, Or Gerlitz wrote:
>>>>>
>>>>>
>>>>> On Sat, May 27, 2017 at 12:16 AM, Jes Sorensen <jes.sorensen@gmail.com>
>>>>> wrote:
>>>>>>
>>>>>>
>>>>>> This gets rid of the temporary #ifdef spaghetti and allows the code to
>>>>>> compile without offload support enabled.
>>>
>>>
>>>>> I am pretty sure we can do that exercise you're up to without any
>>>>> spaghetti cooking and even put more code under that CONFIG directive
>>>>> (en_rep.c), I'll take that with Saeed.
>>>
>>>
>>>> I want to avoid adding #ifdef CONFIG_foo to the main code in order to
>>>> keep
>>>> it readable. I did it gradually to make sure I didn't break anything and
>>>> to
>>>> allow for it to be bisected in case something did break. If we can move
>>>> out
>>>> more code from places like en_rep.c into eswitch_offload.c and get it
>>>> disabled that way that would be great, but I like to limit the number of
>>>> #ifdefs we add to the actual code.
>>>
>>>
>>> FWIW (see below), squashing your seven patches to one resulted in a
>>> fairly simple/clear
>>> patch, so if we go that way, no need to have seven commits just for this
>>> piece.
>>
>>
>> Squashing patches into jumbo patches is inherently broken and bad coding
>> practice! It makes it way more complicated to debug and bisect in case a
>> minor detail broke in the process.
>
> Not that pure LOC ##-s is the only/deep measurement, but your overall
> changes in the the seven patch series account to:
>
>  5 files changed, 94 insertions(+), 3 deletions(-)
>
> and by no mean this is jumbo or inherently broken and bad coded, so
> please slow down please, I looked with care on the resulted patch and
> said it's basically ok.
>
>
>>> Re SRIOV, I don't think it would be correct to break the support info few
>>> CONFIG directives. If we want to allow someone to build the driver w.o
>>> SRIOV that's fine, but I don't think we should further go down and disable
>>> some of the SRIOV sub-modes.
>
>>> Re TC offload support, that's make sense.
>
>> OK, so disabling SRIOV and disabling TC makes sense - I'll look at that.
>
> I think Saeed wants us to conduct that exercise, let me check with him
> and get back to you


Jes, we will do the exercise, there will be a config directive for TC
offloads and another
one for eswitch/sriov support.

Or.

Re: [PATCH 7/7] mlx5: Do not build eswitch_offloads if CONFIG_MLX5_EN_ESWITCH_OFFLOADS is set

[Jes Sorensen]

On 06/03/2017 03:37 PM, Or Gerlitz wrote:
> On Fri, Jun 2, 2017 at 11:22 PM, Jes Sorensen <jsorensen@fb.com> wrote:
>> On 05/28/2017 02:03 AM, Or Gerlitz wrote:
>>>
>>> On Sun, May 28, 2017 at 5:23 AM, Jes Sorensen <jes.sorensen@gmail.com>
>>> wrote:
>>>>
>>>> On 05/27/2017 05:02 PM, Or Gerlitz wrote:
>>>>>
>>>>>
>>>>> On Sat, May 27, 2017 at 12:16 AM, Jes Sorensen <jes.sorensen@gmail.com>
>>>>> wrote:
>>>>>>
>>>>>>
>>>>>> This gets rid of the temporary #ifdef spaghetti and allows the code to
>>>>>> compile without offload support enabled.
>>>
>>>
>>>>> I am pretty sure we can do that exercise you're up to without any
>>>>> spaghetti cooking and even put more code under that CONFIG directive
>>>>> (en_rep.c), I'll take that with Saeed.
>>>
>>>
>>>> I want to avoid adding #ifdef CONFIG_foo to the main code in order to
>>>> keep
>>>> it readable. I did it gradually to make sure I didn't break anything and
>>>> to
>>>> allow for it to be bisected in case something did break. If we can move
>>>> out
>>>> more code from places like en_rep.c into eswitch_offload.c and get it
>>>> disabled that way that would be great, but I like to limit the number of
>>>> #ifdefs we add to the actual code.
>>>
>>>
>>> FWIW (see below), squashing your seven patches to one resulted in a
>>> fairly simple/clear
>>> patch, so if we go that way, no need to have seven commits just for this
>>> piece.
>>
>>
>> Squashing patches into jumbo patches is inherently broken and bad coding
>> practice! It makes it way more complicated to debug and bisect in case a
>> minor detail broke in the process.
> 
> Not that pure LOC ##-s is the only/deep measurement, but your overall
> changes in the the seven patch series account to:
> 
>   5 files changed, 94 insertions(+), 3 deletions(-)
> 
> and by no mean this is jumbo or inherently broken and bad coded, so
> please slow down please, I looked with care on the resulted patch and
> said it's basically ok.

Squashing patches for the sake of squashing patches is inherently broken 
and bad. So please calm down and stop this mangling of other peoples' 
patches.

If you want an alternative, put up a proposal and look at it for 
comparison somewhere.

Jes

Re: [PATCH 7/7] mlx5: Do not build eswitch_offloads if CONFIG_MLX5_EN_ESWITCH_OFFLOADS is set

[Saeed Mahameed]

On Mon, Jun 5, 2017 at 11:51 PM, Jes Sorensen <jsorensen@fb.com> wrote:
> On 06/03/2017 03:37 PM, Or Gerlitz wrote:
>>
>> On Fri, Jun 2, 2017 at 11:22 PM, Jes Sorensen <jsorensen@fb.com> wrote:
>>>
>>> On 05/28/2017 02:03 AM, Or Gerlitz wrote:
>>>>
>>>>
>>>> On Sun, May 28, 2017 at 5:23 AM, Jes Sorensen <jes.sorensen@gmail.com>
>>>> wrote:
>>>>>
>>>>>
>>>>> On 05/27/2017 05:02 PM, Or Gerlitz wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On Sat, May 27, 2017 at 12:16 AM, Jes Sorensen
>>>>>> <jes.sorensen@gmail.com>
>>>>>> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> This gets rid of the temporary #ifdef spaghetti and allows the code
>>>>>>> to
>>>>>>> compile without offload support enabled.
>>>>
>>>>
>>>>
>>>>>> I am pretty sure we can do that exercise you're up to without any
>>>>>> spaghetti cooking and even put more code under that CONFIG directive
>>>>>> (en_rep.c), I'll take that with Saeed.
>>>>
>>>>
>>>>
>>>>> I want to avoid adding #ifdef CONFIG_foo to the main code in order to
>>>>> keep
>>>>> it readable. I did it gradually to make sure I didn't break anything
>>>>> and
>>>>> to
>>>>> allow for it to be bisected in case something did break. If we can move
>>>>> out
>>>>> more code from places like en_rep.c into eswitch_offload.c and get it
>>>>> disabled that way that would be great, but I like to limit the number
>>>>> of
>>>>> #ifdefs we add to the actual code.
>>>>
>>>>
>>>>
>>>> FWIW (see below), squashing your seven patches to one resulted in a
>>>> fairly simple/clear
>>>> patch, so if we go that way, no need to have seven commits just for this
>>>> piece.
>>>
>>>
>>>
>>> Squashing patches into jumbo patches is inherently broken and bad coding
>>> practice! It makes it way more complicated to debug and bisect in case a
>>> minor detail broke in the process.
>>
>>
>> Not that pure LOC ##-s is the only/deep measurement, but your overall
>> changes in the the seven patch series account to:
>>
>>   5 files changed, 94 insertions(+), 3 deletions(-)
>>
>> and by no mean this is jumbo or inherently broken and bad coded, so
>> please slow down please, I looked with care on the resulted patch and
>> said it's basically ok.
>
>
> Squashing patches for the sake of squashing patches is inherently broken and
> bad. So please calm down and stop this mangling of other peoples' patches.
>
> If you want an alternative, put up a proposal and look at it for comparison
> somewhere.
>
> Jes
>

Hey Jes,

It is not just about squashing patches, I am working on a series of
patches to allow compiling out eswitch/eswitch_offloads/en_rep.c/en_tc
altogether, it will come out cleaner as it will remove all ethernet
sriov/eswitch VF representors and eswitch tc offloads stuff with one
kconfig flag, and yet preserve standard QoS functionality from en_tc.

BTW today you can just remove eswitch from driver and non sriov
configuration will perfectly work with no issues.
Even multi PF configuration will also work, but without l2 mac table,
which means PFs can only see packets with their own static (permanent)
mac addresses, user configured macs will not work on Multi PF
configuration.

For that i will take the l2 table (ConnectX PF mac table) logic out of
eswitch as it is not really an eswitch logic, and move it to core
driver to allow Multi PF configuration to work without eswitch.

I will post some patches for you to review by end of week.

Thanks,
Saeed.

Re: [PATCH 7/7] mlx5: Do not build eswitch_offloads if CONFIG_MLX5_EN_ESWITCH_OFFLOADS is set

[Jes Sorensen]

On 06/05/2017 05:53 PM, Saeed Mahameed wrote:
> On Mon, Jun 5, 2017 at 11:51 PM, Jes Sorensen <jsorensen@fb.com> wrote:
>> On 06/03/2017 03:37 PM, Or Gerlitz wrote:
>>>
>>> On Fri, Jun 2, 2017 at 11:22 PM, Jes Sorensen <jsorensen@fb.com> wrote:
>>>>
>>>> On 05/28/2017 02:03 AM, Or Gerlitz wrote:
>>>>>
>>>>>
>>>>> On Sun, May 28, 2017 at 5:23 AM, Jes Sorensen <jes.sorensen@gmail.com>
>>>>> wrote:
>>>>>>
>>>>>>
>>>>>> On 05/27/2017 05:02 PM, Or Gerlitz wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On Sat, May 27, 2017 at 12:16 AM, Jes Sorensen
>>>>>>> <jes.sorensen@gmail.com>
>>>>>>> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> This gets rid of the temporary #ifdef spaghetti and allows the code
>>>>>>>> to
>>>>>>>> compile without offload support enabled.
>>>>>
>>>>>
>>>>>
>>>>>>> I am pretty sure we can do that exercise you're up to without any
>>>>>>> spaghetti cooking and even put more code under that CONFIG directive
>>>>>>> (en_rep.c), I'll take that with Saeed.
>>>>>
>>>>>
>>>>>
>>>>>> I want to avoid adding #ifdef CONFIG_foo to the main code in order to
>>>>>> keep
>>>>>> it readable. I did it gradually to make sure I didn't break anything
>>>>>> and
>>>>>> to
>>>>>> allow for it to be bisected in case something did break. If we can move
>>>>>> out
>>>>>> more code from places like en_rep.c into eswitch_offload.c and get it
>>>>>> disabled that way that would be great, but I like to limit the number
>>>>>> of
>>>>>> #ifdefs we add to the actual code.
>>>>>
>>>>>
>>>>>
>>>>> FWIW (see below), squashing your seven patches to one resulted in a
>>>>> fairly simple/clear
>>>>> patch, so if we go that way, no need to have seven commits just for this
>>>>> piece.
>>>>
>>>>
>>>>
>>>> Squashing patches into jumbo patches is inherently broken and bad coding
>>>> practice! It makes it way more complicated to debug and bisect in case a
>>>> minor detail broke in the process.
>>>
>>>
>>> Not that pure LOC ##-s is the only/deep measurement, but your overall
>>> changes in the the seven patch series account to:
>>>
>>>    5 files changed, 94 insertions(+), 3 deletions(-)
>>>
>>> and by no mean this is jumbo or inherently broken and bad coded, so
>>> please slow down please, I looked with care on the resulted patch and
>>> said it's basically ok.
>>
>>
>> Squashing patches for the sake of squashing patches is inherently broken and
>> bad. So please calm down and stop this mangling of other peoples' patches.
>>
>> If you want an alternative, put up a proposal and look at it for comparison
>> somewhere.
>
> Hey Jes,
> 
> It is not just about squashing patches, I am working on a series of
> patches to allow compiling out eswitch/eswitch_offloads/en_rep.c/en_tc
> altogether, it will come out cleaner as it will remove all ethernet
> sriov/eswitch VF representors and eswitch tc offloads stuff with one
> kconfig flag, and yet preserve standard QoS functionality from en_tc.

Saeed,

I realize it is not just about squashing patches, however doing that to 
someone else's patches is just broken. The Linux kernel way is to build 
on top of patches, if they are valid, rather than throwing them all away 
and doing it from scratch again bottom up. If there was something 
actually wrong with my patches, and I would love to understand if that 
is the case, since I don't know 1/100th of the hardware details that you 
know, then please share those details.

> BTW today you can just remove eswitch from driver and non sriov
> configuration will perfectly work with no issues.
> Even multi PF configuration will also work, but without l2 mac table,
> which means PFs can only see packets with their own static (permanent)
> mac addresses, user configured macs will not work on Multi PF
> configuration.

It sounds like this shakes up things a little and we will have things 
moved to where they actually belong in the hierarchy so that will be a 
good thing in the end :)

> For that i will take the l2 table (ConnectX PF mac table) logic out of
> eswitch as it is not really an eswitch logic, and move it to core
> driver to allow Multi PF configuration to work without eswitch.

Sounds good.

> I will post some patches for you to review by end of week.

Could we please start seeing this stuff happen in a public git tree so 
it is possible to follow and contribute to the development? It is very 
frustrating having to wait for things to appear and and not knowing 
whether a patch is integrated or needs to be revised when you have 
things building on top of it.

Jes

Re: [PATCH 7/7] mlx5: Do not build eswitch_offloads if CONFIG_MLX5_EN_ESWITCH_OFFLOADS is set

[Saeed Mahameed]

On Wed, Jun 7, 2017 at 12:46 AM, Jes Sorensen <jsorensen@fb.com> wrote:
> On 06/05/2017 05:53 PM, Saeed Mahameed wrote:
>>
>> On Mon, Jun 5, 2017 at 11:51 PM, Jes Sorensen <jsorensen@fb.com> wrote:
>>>
>>> On 06/03/2017 03:37 PM, Or Gerlitz wrote:
>>>>
>>>>
>>>> On Fri, Jun 2, 2017 at 11:22 PM, Jes Sorensen <jsorensen@fb.com> wrote:
>>>>>
>>>>>
>>>>> On 05/28/2017 02:03 AM, Or Gerlitz wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On Sun, May 28, 2017 at 5:23 AM, Jes Sorensen <jes.sorensen@gmail.com>
>>>>>> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 05/27/2017 05:02 PM, Or Gerlitz wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On Sat, May 27, 2017 at 12:16 AM, Jes Sorensen
>>>>>>>> <jes.sorensen@gmail.com>
>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> This gets rid of the temporary #ifdef spaghetti and allows the code
>>>>>>>>> to
>>>>>>>>> compile without offload support enabled.
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>>> I am pretty sure we can do that exercise you're up to without any
>>>>>>>> spaghetti cooking and even put more code under that CONFIG directive
>>>>>>>> (en_rep.c), I'll take that with Saeed.
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>> I want to avoid adding #ifdef CONFIG_foo to the main code in order to
>>>>>>> keep
>>>>>>> it readable. I did it gradually to make sure I didn't break anything
>>>>>>> and
>>>>>>> to
>>>>>>> allow for it to be bisected in case something did break. If we can
>>>>>>> move
>>>>>>> out
>>>>>>> more code from places like en_rep.c into eswitch_offload.c and get it
>>>>>>> disabled that way that would be great, but I like to limit the number
>>>>>>> of
>>>>>>> #ifdefs we add to the actual code.
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> FWIW (see below), squashing your seven patches to one resulted in a
>>>>>> fairly simple/clear
>>>>>> patch, so if we go that way, no need to have seven commits just for
>>>>>> this
>>>>>> piece.
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> Squashing patches into jumbo patches is inherently broken and bad
>>>>> coding
>>>>> practice! It makes it way more complicated to debug and bisect in case
>>>>> a
>>>>> minor detail broke in the process.
>>>>
>>>>
>>>>
>>>> Not that pure LOC ##-s is the only/deep measurement, but your overall
>>>> changes in the the seven patch series account to:
>>>>
>>>>    5 files changed, 94 insertions(+), 3 deletions(-)
>>>>
>>>> and by no mean this is jumbo or inherently broken and bad coded, so
>>>> please slow down please, I looked with care on the resulted patch and
>>>> said it's basically ok.
>>>
>>>
>>>
>>> Squashing patches for the sake of squashing patches is inherently broken
>>> and
>>> bad. So please calm down and stop this mangling of other peoples'
>>> patches.
>>>
>>> If you want an alternative, put up a proposal and look at it for
>>> comparison
>>> somewhere.
>>
>>
>> Hey Jes,
>>
>> It is not just about squashing patches, I am working on a series of
>> patches to allow compiling out eswitch/eswitch_offloads/en_rep.c/en_tc
>> altogether, it will come out cleaner as it will remove all ethernet
>> sriov/eswitch VF representors and eswitch tc offloads stuff with one
>> kconfig flag, and yet preserve standard QoS functionality from en_tc.
>
>
> Saeed,
>
> I realize it is not just about squashing patches, however doing that to
> someone else's patches is just broken. The Linux kernel way is to build on
> top of patches, if they are valid, rather than throwing them all away and
> doing it from scratch again bottom up. If there was something actually wrong
> with my patches, and I would love to understand if that is the case, since I
> don't know 1/100th of the hardware details that you know, then please share
> those details.

Hey Jes,

Sorry for the inconvenience, I am working on a very similar patches,
even before you posted yours.
Your patches are fine, but as i said before, removing eswitch as is
will introduce a small regression in Multi-PF configuration.

the issue is that lately we are having tons of discussions exactly
about this and how to do the driver breakdown
that makes everyone happy, so things are moving relatively slow, but
my work on eswitch is converging.

>
>> BTW today you can just remove eswitch from driver and non sriov
>> configuration will perfectly work with no issues.
>> Even multi PF configuration will also work, but without l2 mac table,
>> which means PFs can only see packets with their own static (permanent)
>> mac addresses, user configured macs will not work on Multi PF
>> configuration.
>
>
> It sounds like this shakes up things a little and we will have things moved
> to where they actually belong in the hierarchy so that will be a good thing
> in the end :)
>
>> For that i will take the l2 table (ConnectX PF mac table) logic out of
>> eswitch as it is not really an eswitch logic, and move it to core
>> driver to allow Multi PF configuration to work without eswitch.
>
>
> Sounds good.
>
>> I will post some patches for you to review by end of week.
>
>
> Could we please start seeing this stuff happen in a public git tree so it is
> possible to follow and contribute to the development? It is very frustrating
> having to wait for things to appear and and not knowing whether a patch is
> integrated or needs to be revised when you have things building on top of
> it.
>

Sure, I will post some patches later today.
I believe they will be fully ready by for submission by End of week.
Again sorry about this.

> Jes

Re: [PATCH 7/7] mlx5: Do not build eswitch_offloads if CONFIG_MLX5_EN_ESWITCH_OFFLOADS is set

[Jes Sorensen]

On 06/07/2017 12:06 AM, Saeed Mahameed wrote:
> On Wed, Jun 7, 2017 at 12:46 AM, Jes Sorensen <jsorensen@fb.com> wrote:
>>> Hey Jes,
>>>
>>> It is not just about squashing patches, I am working on a series of
>>> patches to allow compiling out eswitch/eswitch_offloads/en_rep.c/en_tc
>>> altogether, it will come out cleaner as it will remove all ethernet
>>> sriov/eswitch VF representors and eswitch tc offloads stuff with one
>>> kconfig flag, and yet preserve standard QoS functionality from en_tc.
>>
>>
>> Saeed,
>>
>> I realize it is not just about squashing patches, however doing that to
>> someone else's patches is just broken. The Linux kernel way is to build on
>> top of patches, if they are valid, rather than throwing them all away and
>> doing it from scratch again bottom up. If there was something actually wrong
>> with my patches, and I would love to understand if that is the case, since I
>> don't know 1/100th of the hardware details that you know, then please share
>> those details.
> 
> Hey Jes,
> 
> Sorry for the inconvenience, I am working on a very similar patches,
> even before you posted yours.
> Your patches are fine, but as i said before, removing eswitch as is
> will introduce a small regression in Multi-PF configuration.
> 
> the issue is that lately we are having tons of discussions exactly
> about this and how to do the driver breakdown
> that makes everyone happy, so things are moving relatively slow, but
> my work on eswitch is converging.

Gotcha. I deliberately didn't disable eswitch itself in my patch, but 
only the offloading functionality, because of the old discussion 
mentioning that the eswitch needing to be initialized.

>> Sounds good.
>>
>>> I will post some patches for you to review by end of week.
>>
>>
>> Could we please start seeing this stuff happen in a public git tree so it is
>> possible to follow and contribute to the development? It is very frustrating
>> having to wait for things to appear and and not knowing whether a patch is
>> integrated or needs to be revised when you have things building on top of
>> it.
> 
> Sure, I will post some patches later today.
> I believe they will be fully ready by for submission by End of week.
> Again sorry about this.

Awesome!

Jes