[PATCH RFC net-next 0/4] Mellanox, MLX5 E-Switch break down

[PATCH RFC net-next 0/4] Mellanox, MLX5 E-Switch break down

[Saeed Mahameed]

Based on Jes Sorensen work to make eswitch_offloads optional at
compilation time, here is an initial RFC series that provides similar
behavior, but here we do more accurate zoom in into eswitch and we break
it into two HW components (MPFS & ESWITCH) to not break any functionality,
and provide more modular separation.

In this series we provide the following modular separation
and give the option of compiling out:

MLX5_MPFS: Multi-Physical Function Switch (MPFs) needed only for Multi-PF configurations
MLX5_ESWITCH: SRIOV Ethernet Switch

With this separation, E-Switch becomes responsible only for ethernet SRIOV and needed only
when SRIOV in enabled, otherwise it is not required.

The two patches at the beginning are for preparation and cleanup only.

Thanks,
Saeed.

Saeed Mahameed (4):
  net/mlx5e: Rearrange netdevice ops structures
  net/mlx5e: NIC netdev init flow cleanup
  net/mlx5: Separate between eswitch and MPFS l2 table logic
  net/mlx5: Add CONFIG_MLX5_ESWITCH Kconfig

 drivers/net/ethernet/mellanox/mlx5/core/Kconfig   |  15 ++
 drivers/net/ethernet/mellanox/mlx5/core/Makefile  |   8 +-
 drivers/net/ethernet/mellanox/mlx5/core/en_fs.c   |  14 +-
 drivers/net/ethernet/mellanox/mlx5/core/en_main.c |  69 ++++----
 drivers/net/ethernet/mellanox/mlx5/core/en_rep.c  |  13 ++
 drivers/net/ethernet/mellanox/mlx5/core/en_rep.h  |   9 +
 drivers/net/ethernet/mellanox/mlx5/core/en_rx.c   |   2 +
 drivers/net/ethernet/mellanox/mlx5/core/en_tc.h   |   7 +
 drivers/net/ethernet/mellanox/mlx5/core/eswitch.c | 189 ++++----------------
 drivers/net/ethernet/mellanox/mlx5/core/eswitch.h |  78 ++------
 drivers/net/ethernet/mellanox/mlx5/core/main.c    |  34 ++--
 drivers/net/ethernet/mellanox/mlx5/core/mpfs.c    | 205 ++++++++++++++++++++++
 drivers/net/ethernet/mellanox/mlx5/core/mpfs.h    |  98 +++++++++++
 include/linux/mlx5/driver.h                       |   2 +
 14 files changed, 473 insertions(+), 270 deletions(-)
 create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/mpfs.c
 create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/mpfs.h

-- 
2.11.0

[PATCH RFC net-next 1/4] net/mlx5e: Rearrange netdevice ops structures

[Saeed Mahameed]

Since we are going to allow compiling the driver without eswitch,
it means we will have to compile out mlx5e_netdev_ops_sriov and will
replace it with mlx5e_netdev_ops_basic.

Move missing vxlan ndos into mlx5e_netdev_ops_basic.

Rearrange some ndos in mlx5e_netdev_ops_sriov and put vf/eswitch related
ndos towards the end of the struct.

Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
index cdff04b2aea1..931347b797c4 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
@@ -3646,6 +3646,9 @@ static const struct net_device_ops mlx5e_netdev_ops_basic = {
 	.ndo_change_mtu          = mlx5e_change_mtu,
 	.ndo_do_ioctl            = mlx5e_ioctl,
 	.ndo_set_tx_maxrate      = mlx5e_set_tx_maxrate,
+	.ndo_udp_tunnel_add      = mlx5e_add_vxlan_port,
+	.ndo_udp_tunnel_del      = mlx5e_del_vxlan_port,
+	.ndo_features_check      = mlx5e_features_check,
 #ifdef CONFIG_RFS_ACCEL
 	.ndo_rx_flow_steer	 = mlx5e_rx_flow_steer,
 #endif
@@ -3670,13 +3673,19 @@ static const struct net_device_ops mlx5e_netdev_ops_sriov = {
 	.ndo_set_features        = mlx5e_set_features,
 	.ndo_change_mtu          = mlx5e_change_mtu,
 	.ndo_do_ioctl            = mlx5e_ioctl,
+	.ndo_set_tx_maxrate      = mlx5e_set_tx_maxrate,
 	.ndo_udp_tunnel_add	 = mlx5e_add_vxlan_port,
 	.ndo_udp_tunnel_del	 = mlx5e_del_vxlan_port,
-	.ndo_set_tx_maxrate      = mlx5e_set_tx_maxrate,
 	.ndo_features_check      = mlx5e_features_check,
 #ifdef CONFIG_RFS_ACCEL
 	.ndo_rx_flow_steer	 = mlx5e_rx_flow_steer,
 #endif
+	.ndo_tx_timeout          = mlx5e_tx_timeout,
+	.ndo_xdp		 = mlx5e_xdp,
+#ifdef CONFIG_NET_POLL_CONTROLLER
+	.ndo_poll_controller     = mlx5e_netpoll,
+#endif
+	/* SRIOV E-Switch NDOs */
 	.ndo_set_vf_mac          = mlx5e_set_vf_mac,
 	.ndo_set_vf_vlan         = mlx5e_set_vf_vlan,
 	.ndo_set_vf_spoofchk     = mlx5e_set_vf_spoofchk,
@@ -3685,11 +3694,6 @@ static const struct net_device_ops mlx5e_netdev_ops_sriov = {
 	.ndo_get_vf_config       = mlx5e_get_vf_config,
 	.ndo_set_vf_link_state   = mlx5e_set_vf_link_state,
 	.ndo_get_vf_stats        = mlx5e_get_vf_stats,
-	.ndo_tx_timeout          = mlx5e_tx_timeout,
-	.ndo_xdp		 = mlx5e_xdp,
-#ifdef CONFIG_NET_POLL_CONTROLLER
-	.ndo_poll_controller     = mlx5e_netpoll,
-#endif
 	.ndo_has_offload_stats	 = mlx5e_has_offload_stats,
 	.ndo_get_offload_stats	 = mlx5e_get_offload_stats,
 };
-- 
2.11.0

Re: [PATCH RFC net-next 1/4] net/mlx5e: Rearrange netdevice ops structures

[Or Gerlitz]

On Thu, Jun 8, 2017 at 2:42 AM, Saeed Mahameed <saeedm@mellanox.com> wrote:
> Since we are going to allow compiling the driver without eswitch,
> it means we will have to compile out mlx5e_netdev_ops_sriov and will
> replace it with mlx5e_netdev_ops_basic.
>
> Move missing vxlan ndos into mlx5e_netdev_ops_basic.

preferred to be more price and use the term udp tunnel (missing udp tunnel) ndos

> Rearrange some ndos in mlx5e_netdev_ops_sriov and put vf/eswitch related
> ndos towards the end of the struct.

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

[PATCH RFC net-next 2/4] net/mlx5e: NIC netdev init flow cleanup

[Saeed Mahameed]

Remove redundant mlx5_eswitch_unregister_vport_rep in mlx5e_add error
flow.

Hide mlx5e_rep_priv and eswitch internal structures from en_main.c in
preparation for downstream patches which allows compiling the driver
without en_rep and eswitch.

Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 20 ++++++--------------
 drivers/net/ethernet/mellanox/mlx5/core/en_rep.c  | 13 +++++++++++++
 drivers/net/ethernet/mellanox/mlx5/core/en_rep.h  |  1 +
 3 files changed, 20 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
index 931347b797c4..1514027a5c51 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
@@ -4357,32 +4357,28 @@ static void mlx5e_detach(struct mlx5_core_dev *mdev, void *vpriv)
 
 static void *mlx5e_add(struct mlx5_core_dev *mdev)
 {
-	struct mlx5_eswitch *esw = mdev->priv.eswitch;
-	int total_vfs = MLX5_TOTAL_VPORTS(mdev);
-	struct mlx5e_rep_priv *rpriv = NULL;
+	struct net_device *netdev;
+	void *rpriv = NULL;
 	void *priv;
-	int vport;
 	int err;
-	struct net_device *netdev;
 
 	err = mlx5e_check_required_hca_cap(mdev);
 	if (err)
 		return NULL;
 
 	if (MLX5_CAP_GEN(mdev, vport_group_manager)) {
-		rpriv = kzalloc(sizeof(*rpriv), GFP_KERNEL);
+		rpriv = mlx5e_alloc_nic_rep_priv(mdev);
 		if (!rpriv) {
-			mlx5_core_warn(mdev,
-				       "Not creating net device, Failed to alloc rep priv data\n");
+			mlx5_core_warn(mdev, "Failed to alloc NIC rep priv data\n");
 			return NULL;
 		}
-		rpriv->rep = &esw->offloads.vport_reps[0];
 	}
 
 	netdev = mlx5e_create_netdev(mdev, &mlx5e_nic_profile, rpriv);
 	if (!netdev) {
 		mlx5_core_err(mdev, "mlx5e_create_netdev failed\n");
-		goto err_unregister_reps;
+		kfree(rpriv);
+		return NULL;
 	}
 
 	priv = netdev_priv(netdev);
@@ -4407,10 +4403,6 @@ static void *mlx5e_add(struct mlx5_core_dev *mdev)
 err_destroy_netdev:
 	mlx5e_destroy_netdev(priv);
 
-err_unregister_reps:
-	for (vport = 1; vport < total_vfs; vport++)
-		mlx5_eswitch_unregister_vport_rep(esw, vport);
-
 	kfree(rpriv);
 	return NULL;
 }
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c b/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c
index 79462c0368a0..8d673d61cd11 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c
@@ -1089,3 +1089,16 @@ void mlx5e_unregister_vport_reps(struct mlx5e_priv *priv)
 	mlx5e_rep_unregister_vf_vports(priv); /* VFs vports */
 	mlx5_eswitch_unregister_vport_rep(esw, 0); /* UPLINK PF*/
 }
+
+void* mlx5e_alloc_nic_rep_priv(struct mlx5_core_dev *mdev)
+{
+	struct mlx5_eswitch *esw = mdev->priv.eswitch;
+	struct mlx5e_rep_priv *rpriv;
+
+	rpriv = kzalloc(sizeof(*rpriv), GFP_KERNEL);
+	if (!rpriv)
+		return NULL;
+
+	rpriv->rep = &esw->offloads.vport_reps[0];
+	return rpriv;
+}
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rep.h b/drivers/net/ethernet/mellanox/mlx5/core/en_rep.h
index a0a1a7a1d6c0..23e43bbf928d 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_rep.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rep.h
@@ -123,6 +123,7 @@ struct mlx5e_encap_entry {
 	int encap_size;
 };
 
+void *mlx5e_alloc_nic_rep_priv(struct mlx5_core_dev *mdev);
 void mlx5e_register_vport_reps(struct mlx5e_priv *priv);
 void mlx5e_unregister_vport_reps(struct mlx5e_priv *priv);
 bool mlx5e_is_uplink_rep(struct mlx5e_priv *priv);
-- 
2.11.0

Re: [PATCH RFC net-next 2/4] net/mlx5e: NIC netdev init flow cleanup

[Or Gerlitz]

On Thu, Jun 8, 2017 at 2:42 AM, Saeed Mahameed <saeedm@mellanox.com> wrote:
> Remove redundant mlx5_eswitch_unregister_vport_rep in mlx5e_add error flow.

Maybe have point patch that makes it clear we fix commit 26e59d807 ?


> Hide mlx5e_rep_priv and eswitch internal structures from en_main.c in
> preparation for downstream patches which allows compiling the driver
> without en_rep and eswitch.

otherwise than the about comment and another one re how to do that,
patch seems fine

>  static void *mlx5e_add(struct mlx5_core_dev *mdev)
>  {
> -       struct mlx5_eswitch *esw = mdev->priv.eswitch;
> -       int total_vfs = MLX5_TOTAL_VPORTS(mdev);
> -       struct mlx5e_rep_priv *rpriv = NULL;
> +       struct net_device *netdev;
> +       void *rpriv = NULL;
>         void *priv;
> -       int vport;
>         int err;
> -       struct net_device *netdev;
>
>         err = mlx5e_check_required_hca_cap(mdev);
>         if (err)
>                 return NULL;
>
>         if (MLX5_CAP_GEN(mdev, vport_group_manager)) {
> -               rpriv = kzalloc(sizeof(*rpriv), GFP_KERNEL);
> +               rpriv = mlx5e_alloc_nic_rep_priv(mdev);
>                 if (!rpriv) {
> -                       mlx5_core_warn(mdev,
> -                                      "Not creating net device, Failed to alloc rep priv data\n");
> +                       mlx5_core_warn(mdev, "Failed to alloc NIC rep priv data\n");
>                         return NULL;
>                 }
> -               rpriv->rep = &esw->offloads.vport_reps[0];
>         }
>
>         netdev = mlx5e_create_netdev(mdev, &mlx5e_nic_profile, rpriv);
>         if (!netdev) {
>                 mlx5_core_err(mdev, "mlx5e_create_netdev failed\n");
> -               goto err_unregister_reps;
> +               kfree(rpriv);
> +               return NULL;

nit - would be better to just do goto err_something and avoid
repeating the kfree/return we already have there

>         }
>
>         priv = netdev_priv(netdev);
> @@ -4407,10 +4403,6 @@ static void *mlx5e_add(struct mlx5_core_dev *mdev)
>  err_destroy_netdev:
>         mlx5e_destroy_netdev(priv);
>
> -err_unregister_reps:
> -       for (vport = 1; vport < total_vfs; vport++)
> -               mlx5_eswitch_unregister_vport_rep(esw, vport);
> -
+err_something:
>         kfree(rpriv);
>         return NULL;
>  }

[PATCH RFC net-next 3/4] net/mlx5: Separate between eswitch and MPFS l2 table logic

[Saeed Mahameed]

Multi-Physical Function Switch (MPFs) is required for when multi-PF
configuration is enabled to allow passing user configured unicast MAC
addresses to the requesting PF.

Before this patch eswitch.c used to manage the HW MPFS l2 table,
eswitch always enabled vport(0) (NIC PF) vport's contexts update on unicast
mac address list changes, to populate the PF's MPFS L2 table accordingly,
even if SRIOV was not enabled.

In downstream patch we would like to allow compiling the driver without
eswitch functionalities, for that we move MPFS l2 table logic out
of eswitch.c into its own file, and provide Kconfig flag (MLX5_MPFS) to
allow compiling out MPFS for those who don't want Multi-PF support

NIC PF netdevice will now directly update MPFS l2 table via the new MPFS
API. VF netdevice has no access to MPFS L2 table, so e-Switch will remain
responsible of updating its PF MPFS l2 table on behalf of its VFs.

Due to this change we also don't require enabling vport(0) (PF vport)
unicast mac changes events anymore, for when SRIOV is not enabled.
Which means eswitch is now activated only on SRIOV activation, and not
required otherwise.

Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/Kconfig   |   8 +
 drivers/net/ethernet/mellanox/mlx5/core/Makefile  |   2 +
 drivers/net/ethernet/mellanox/mlx5/core/en_fs.c   |  14 +-
 drivers/net/ethernet/mellanox/mlx5/core/eswitch.c | 189 ++++----------------
 drivers/net/ethernet/mellanox/mlx5/core/eswitch.h |  55 +-----
 drivers/net/ethernet/mellanox/mlx5/core/main.c    |  26 ++-
 drivers/net/ethernet/mellanox/mlx5/core/mpfs.c    | 205 ++++++++++++++++++++++
 drivers/net/ethernet/mellanox/mlx5/core/mpfs.h    |  98 +++++++++++
 include/linux/mlx5/driver.h                       |   2 +
 9 files changed, 376 insertions(+), 223 deletions(-)
 create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/mpfs.c
 create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/mpfs.h

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/Kconfig b/drivers/net/ethernet/mellanox/mlx5/core/Kconfig
index cf1ef48bfd8d..49559e11af86 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/Kconfig
+++ b/drivers/net/ethernet/mellanox/mlx5/core/Kconfig
@@ -30,6 +30,14 @@ config MLX5_CORE_EN
 	---help---
 	  Ethernet support in Mellanox Technologies ConnectX-4 NIC.
 
+config MLX5_MPFS
+        bool "Mellanox Technologies MLX5 MPFS support"
+        depends on MLX5_CORE_EN
+	default y
+        ---help---
+	  Mellanox Technologies Ethernet Multi-Physical Function Switch (MPFS)
+          support in ConnectX NIC.
+
 config MLX5_CORE_EN_DCB
 	bool "Data Center Bridging (DCB) Support"
 	default y
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/Makefile b/drivers/net/ethernet/mellanox/mlx5/core/Makefile
index 12556c03eec4..8c493aeec392 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/Makefile
+++ b/drivers/net/ethernet/mellanox/mlx5/core/Makefile
@@ -13,6 +13,8 @@ mlx5_core-$(CONFIG_MLX5_CORE_EN) += wq.o eswitch.o eswitch_offloads.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_MPFS) += mpfs.o
+
 mlx5_core-$(CONFIG_MLX5_CORE_EN_DCB) +=  en_dcbnl.o
 
 mlx5_core-$(CONFIG_MLX5_CORE_IPOIB) += ipoib.o
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_fs.c b/drivers/net/ethernet/mellanox/mlx5/core/en_fs.c
index 7acc4fba7ece..5a782618f1b5 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_fs.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_fs.c
@@ -36,6 +36,7 @@
 #include <linux/tcp.h>
 #include <linux/mlx5/fs.h>
 #include "en.h"
+#include "mpfs.h"
 
 static int mlx5e_add_l2_flow_rule(struct mlx5e_priv *priv,
 				  struct mlx5e_l2_rule *ai, int type);
@@ -363,17 +364,28 @@ static void mlx5e_del_vlan_rules(struct mlx5e_priv *priv)
 static void mlx5e_execute_l2_action(struct mlx5e_priv *priv,
 				    struct mlx5e_l2_hash_node *hn)
 {
-	switch (hn->action) {
+	u8 action = hn->action;
+	int l2_err = 0;
+
+	switch (action) {
 	case MLX5E_ACTION_ADD:
 		mlx5e_add_l2_flow_rule(priv, &hn->ai, MLX5E_FULLMATCH);
+		/* mlx5_mpfs_add_mac will skip mc addresses */
+		l2_err = mlx5_mpfs_add_mac(priv->mdev, hn->ai.addr);
 		hn->action = MLX5E_ACTION_NONE;
 		break;
 
 	case MLX5E_ACTION_DEL:
+		/* mlx5_mpfs_del_mac will skip mc addresses */
+		l2_err = mlx5_mpfs_del_mac(priv->mdev, hn->ai.addr);
 		mlx5e_del_l2_flow_rule(priv, &hn->ai);
 		mlx5e_del_l2_from_hash(hn);
 		break;
 	}
+
+	if (l2_err)
+		netdev_warn(priv->netdev, "L2 table, failed to %s mac %pM, err(%d)\n",
+			    action == MLX5E_ACTION_ADD ? "add" : "del", hn->ai.addr, l2_err);
 }
 
 static void mlx5e_sync_netdev_addr(struct mlx5e_priv *priv)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
index 81dfcd90b1f5..33e3b9a2f966 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
@@ -46,13 +46,6 @@ enum {
 	MLX5_ACTION_DEL  = 2,
 };
 
-/* E-Switch UC L2 table hash node */
-struct esw_uc_addr {
-	struct l2addr_node node;
-	u32                table_index;
-	u32                vport;
-};
-
 /* Vport UC/MC hash node */
 struct vport_addr {
 	struct l2addr_node     node;
@@ -154,81 +147,6 @@ static int modify_esw_vport_cvlan(struct mlx5_core_dev *dev, u32 vport,
 	return modify_esw_vport_context_cmd(dev, vport, in, sizeof(in));
 }
 
-/* HW L2 Table (MPFS) management */
-static int set_l2_table_entry_cmd(struct mlx5_core_dev *dev, u32 index,
-				  u8 *mac, u8 vlan_valid, u16 vlan)
-{
-	u32 in[MLX5_ST_SZ_DW(set_l2_table_entry_in)]   = {0};
-	u32 out[MLX5_ST_SZ_DW(set_l2_table_entry_out)] = {0};
-	u8 *in_mac_addr;
-
-	MLX5_SET(set_l2_table_entry_in, in, opcode,
-		 MLX5_CMD_OP_SET_L2_TABLE_ENTRY);
-	MLX5_SET(set_l2_table_entry_in, in, table_index, index);
-	MLX5_SET(set_l2_table_entry_in, in, vlan_valid, vlan_valid);
-	MLX5_SET(set_l2_table_entry_in, in, vlan, vlan);
-
-	in_mac_addr = MLX5_ADDR_OF(set_l2_table_entry_in, in, mac_address);
-	ether_addr_copy(&in_mac_addr[2], mac);
-
-	return mlx5_cmd_exec(dev, in, sizeof(in), out, sizeof(out));
-}
-
-static int del_l2_table_entry_cmd(struct mlx5_core_dev *dev, u32 index)
-{
-	u32 in[MLX5_ST_SZ_DW(delete_l2_table_entry_in)]   = {0};
-	u32 out[MLX5_ST_SZ_DW(delete_l2_table_entry_out)] = {0};
-
-	MLX5_SET(delete_l2_table_entry_in, in, opcode,
-		 MLX5_CMD_OP_DELETE_L2_TABLE_ENTRY);
-	MLX5_SET(delete_l2_table_entry_in, in, table_index, index);
-	return mlx5_cmd_exec(dev, in, sizeof(in), out, sizeof(out));
-}
-
-static int alloc_l2_table_index(struct mlx5_l2_table *l2_table, u32 *ix)
-{
-	int err = 0;
-
-	*ix = find_first_zero_bit(l2_table->bitmap, l2_table->size);
-	if (*ix >= l2_table->size)
-		err = -ENOSPC;
-	else
-		__set_bit(*ix, l2_table->bitmap);
-
-	return err;
-}
-
-static void free_l2_table_index(struct mlx5_l2_table *l2_table, u32 ix)
-{
-	__clear_bit(ix, l2_table->bitmap);
-}
-
-static int set_l2_table_entry(struct mlx5_core_dev *dev, u8 *mac,
-			      u8 vlan_valid, u16 vlan,
-			      u32 *index)
-{
-	struct mlx5_l2_table *l2_table = &dev->priv.eswitch->l2_table;
-	int err;
-
-	err = alloc_l2_table_index(l2_table, index);
-	if (err)
-		return err;
-
-	err = set_l2_table_entry_cmd(dev, *index, mac, vlan_valid, vlan);
-	if (err)
-		free_l2_table_index(l2_table, *index);
-
-	return err;
-}
-
-static void del_l2_table_entry(struct mlx5_core_dev *dev, u32 index)
-{
-	struct mlx5_l2_table *l2_table = &dev->priv.eswitch->l2_table;
-
-	del_l2_table_entry_cmd(dev, index);
-	free_l2_table_index(l2_table, index);
-}
-
 /* E-Switch FDB */
 static struct mlx5_flow_handle *
 __esw_fdb_set_vport_rule(struct mlx5_eswitch *esw, u32 vport, bool rx_rule,
@@ -455,65 +373,58 @@ typedef int (*vport_addr_action)(struct mlx5_eswitch *esw,
 
 static int esw_add_uc_addr(struct mlx5_eswitch *esw, struct vport_addr *vaddr)
 {
-	struct hlist_head *hash = esw->l2_table.l2_hash;
-	struct esw_uc_addr *esw_uc;
 	u8 *mac = vaddr->node.addr;
 	u32 vport = vaddr->vport;
 	int err;
 
-	esw_uc = l2addr_hash_find(hash, mac, struct esw_uc_addr);
-	if (esw_uc) {
+	/* Skip l2_table_add for PFs,
+	 * it is already done by the PF netdev in mlx5e_execute_l2_action
+	 */
+	if (!vport)
+		goto fdb_add;
+
+	err = mlx5_mpfs_add_mac(esw->dev, mac);
+	if (err) {
 		esw_warn(esw->dev,
-			 "Failed to set L2 mac(%pM) for vport(%d), mac is already in use by vport(%d)\n",
-			 mac, vport, esw_uc->vport);
-		return -EEXIST;
+			 "Failed to add L2 table mac(%pM) for vport(%d), err(%d)\n",
+			 mac, vport, err);
+		return err;
 	}
 
-	esw_uc = l2addr_hash_add(hash, mac, struct esw_uc_addr, GFP_KERNEL);
-	if (!esw_uc)
-		return -ENOMEM;
-	esw_uc->vport = vport;
-
-	err = set_l2_table_entry(esw->dev, mac, 0, 0, &esw_uc->table_index);
-	if (err)
-		goto abort;
-
+fdb_add:
 	/* SRIOV is enabled: Forward UC MAC to vport */
 	if (esw->fdb_table.fdb && esw->mode == SRIOV_LEGACY)
 		vaddr->flow_rule = esw_fdb_set_vport_rule(esw, mac, vport);
 
-	esw_debug(esw->dev, "\tADDED UC MAC: vport[%d] %pM index:%d fr(%p)\n",
-		  vport, mac, esw_uc->table_index, vaddr->flow_rule);
-	return err;
-abort:
-	l2addr_hash_del(esw_uc);
+	esw_debug(esw->dev, "\tADDED UC MAC: vport[%d] %pM fr(%p)\n",
+		  vport, mac, vaddr->flow_rule);
+
 	return err;
 }
 
 static int esw_del_uc_addr(struct mlx5_eswitch *esw, struct vport_addr *vaddr)
 {
-	struct hlist_head *hash = esw->l2_table.l2_hash;
-	struct esw_uc_addr *esw_uc;
 	u8 *mac = vaddr->node.addr;
 	u32 vport = vaddr->vport;
+	int err = 0;
 
-	esw_uc = l2addr_hash_find(hash, mac, struct esw_uc_addr);
-	if (!esw_uc || esw_uc->vport != vport) {
-		esw_debug(esw->dev,
-			  "MAC(%pM) doesn't belong to vport (%d)\n",
-			  mac, vport);
-		return -EINVAL;
-	}
-	esw_debug(esw->dev, "\tDELETE UC MAC: vport[%d] %pM index:%d fr(%p)\n",
-		  vport, mac, esw_uc->table_index, vaddr->flow_rule);
+	/* Skip l2_table_del for PFs,
+	 * it is already done by the PF netdev in mlx5e_execute_l2_action
+	 */
+	if (!vport)
+		goto fdb_del;
 
-	del_l2_table_entry(esw->dev, esw_uc->table_index);
+	err = mlx5_mpfs_del_mac(esw->dev, mac);
+	if (err)
+		esw_warn(esw->dev,
+			 "Failed to del L2 table mac(%pM) for vport(%d), err(%d)\n",
+			 mac, vport, err);
 
+fdb_del:
 	if (vaddr->flow_rule)
 		mlx5_del_flow_rules(vaddr->flow_rule);
 	vaddr->flow_rule = NULL;
 
-	l2addr_hash_del(esw_uc);
 	return 0;
 }
 
@@ -1635,7 +1546,6 @@ int mlx5_eswitch_enable_sriov(struct mlx5_eswitch *esw, int nvfs, int mode)
 
 	esw_info(esw->dev, "E-Switch enable SRIOV: nvfs(%d) mode (%d)\n", nvfs, mode);
 	esw->mode = mode;
-	esw_disable_vport(esw, 0);
 
 	if (mode == SRIOV_LEGACY)
 		err = esw_create_legacy_fdb_table(esw, nvfs + 1);
@@ -1648,7 +1558,11 @@ int mlx5_eswitch_enable_sriov(struct mlx5_eswitch *esw, int nvfs, int mode)
 	if (err)
 		esw_warn(esw->dev, "Failed to create eswitch TSAR");
 
-	enabled_events = (mode == SRIOV_LEGACY) ? SRIOV_VPORT_EVENTS : UC_ADDR_CHANGE;
+	/* Don't enable vport events when in SRIOV_OFFLOADS mode, since:
+	 * 1. L2 table (MPFS) is programmed by PF/VF representors netdevs set_rx_mode
+	 * 2. FDB/Eswitch is programmed by user space tools
+	 */
+	enabled_events = (mode == SRIOV_LEGACY) ? SRIOV_VPORT_EVENTS : 0;
 	for (i = 0; i <= nvfs; i++)
 		esw_enable_vport(esw, i, enabled_events);
 
@@ -1657,7 +1571,6 @@ int mlx5_eswitch_enable_sriov(struct mlx5_eswitch *esw, int nvfs, int mode)
 	return 0;
 
 abort:
-	esw_enable_vport(esw, 0, UC_ADDR_CHANGE);
 	esw->mode = SRIOV_NONE;
 	return err;
 }
@@ -1692,32 +1605,10 @@ void mlx5_eswitch_disable_sriov(struct mlx5_eswitch *esw)
 		esw_offloads_cleanup(esw, nvports);
 
 	esw->mode = SRIOV_NONE;
-	/* VPORT 0 (PF) must be enabled back with non-sriov configuration */
-	esw_enable_vport(esw, 0, UC_ADDR_CHANGE);
-}
-
-void mlx5_eswitch_attach(struct mlx5_eswitch *esw)
-{
-	if (!esw || !MLX5_CAP_GEN(esw->dev, vport_group_manager) ||
-	    MLX5_CAP_GEN(esw->dev, port_type) != MLX5_CAP_PORT_TYPE_ETH)
-		return;
-
-	esw_enable_vport(esw, 0, UC_ADDR_CHANGE);
-	/* VF Vports will be enabled when SRIOV is enabled */
-}
-
-void mlx5_eswitch_detach(struct mlx5_eswitch *esw)
-{
-	if (!esw || !MLX5_CAP_GEN(esw->dev, vport_group_manager) ||
-	    MLX5_CAP_GEN(esw->dev, port_type) != MLX5_CAP_PORT_TYPE_ETH)
-		return;
-
-	esw_disable_vport(esw, 0);
 }
 
 int mlx5_eswitch_init(struct mlx5_core_dev *dev)
 {
-	int l2_table_size = 1 << MLX5_CAP_GEN(dev, log_max_l2_table);
 	int total_vports = MLX5_TOTAL_VPORTS(dev);
 	struct mlx5_eswitch *esw;
 	int vport_num;
@@ -1728,8 +1619,8 @@ int mlx5_eswitch_init(struct mlx5_core_dev *dev)
 		return 0;
 
 	esw_info(dev,
-		 "Total vports %d, l2 table size(%d), per vport: max uc(%d) max mc(%d)\n",
-		 total_vports, l2_table_size,
+		 "Total vports %d, per vport: max uc(%d) max mc(%d)\n",
+		 total_vports,
 		 MLX5_MAX_UC_PER_VPORT(dev),
 		 MLX5_MAX_MC_PER_VPORT(dev));
 
@@ -1739,14 +1630,6 @@ int mlx5_eswitch_init(struct mlx5_core_dev *dev)
 
 	esw->dev = dev;
 
-	esw->l2_table.bitmap = kcalloc(BITS_TO_LONGS(l2_table_size),
-				   sizeof(uintptr_t), GFP_KERNEL);
-	if (!esw->l2_table.bitmap) {
-		err = -ENOMEM;
-		goto abort;
-	}
-	esw->l2_table.size = l2_table_size;
-
 	esw->work_queue = create_singlethread_workqueue("mlx5_esw_wq");
 	if (!esw->work_queue) {
 		err = -ENOMEM;
@@ -1796,7 +1679,6 @@ int mlx5_eswitch_init(struct mlx5_core_dev *dev)
 abort:
 	if (esw->work_queue)
 		destroy_workqueue(esw->work_queue);
-	kfree(esw->l2_table.bitmap);
 	kfree(esw->vports);
 	kfree(esw->offloads.vport_reps);
 	kfree(esw);
@@ -1813,7 +1695,6 @@ void mlx5_eswitch_cleanup(struct mlx5_eswitch *esw)
 
 	esw->dev->priv.eswitch = NULL;
 	destroy_workqueue(esw->work_queue);
-	kfree(esw->l2_table.bitmap);
 	kfree(esw->offloads.vport_reps);
 	kfree(esw->vports);
 	kfree(esw);
@@ -1838,7 +1719,7 @@ void mlx5_eswitch_vport_event(struct mlx5_eswitch *esw, struct mlx5_eqe *eqe)
 
 /* Vport Administration */
 #define ESW_ALLOWED(esw) \
-	(esw && MLX5_CAP_GEN(esw->dev, vport_group_manager) && mlx5_core_is_pf(esw->dev))
+	(esw && MLX5_VPORT_MANAGER(esw->dev))
 #define LEGAL_VPORT(esw, vport) (vport >= 0 && vport < esw->total_vports)
 
 int mlx5_eswitch_set_vport_mac(struct mlx5_eswitch *esw,
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
index b746f62c8c79..a0203c7f6cf6 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
@@ -37,6 +37,7 @@
 #include <linux/if_link.h>
 #include <net/devlink.h>
 #include <linux/mlx5/device.h>
+#include "mpfs.h"
 
 #define MLX5_MAX_UC_PER_VPORT(dev) \
 	(1 << MLX5_CAP_GEN(dev, log_max_current_uc_list))
@@ -44,9 +45,6 @@
 #define MLX5_MAX_MC_PER_VPORT(dev) \
 	(1 << MLX5_CAP_GEN(dev, log_max_current_mc_list))
 
-#define MLX5_L2_ADDR_HASH_SIZE (BIT(BITS_PER_BYTE))
-#define MLX5_L2_ADDR_HASH(addr) (addr[5])
-
 #define FDB_UPLINK_VPORT 0xffff
 
 #define MLX5_MIN_BW_SHARE 1
@@ -54,48 +52,6 @@
 #define MLX5_RATE_TO_BW_SHARE(rate, divider, limit) \
 	min_t(u32, max_t(u32, (rate) / (divider), MLX5_MIN_BW_SHARE), limit)
 
-/* L2 -mac address based- hash helpers */
-struct l2addr_node {
-	struct hlist_node hlist;
-	u8                addr[ETH_ALEN];
-};
-
-#define for_each_l2hash_node(hn, tmp, hash, i) \
-	for (i = 0; i < MLX5_L2_ADDR_HASH_SIZE; i++) \
-		hlist_for_each_entry_safe(hn, tmp, &hash[i], hlist)
-
-#define l2addr_hash_find(hash, mac, type) ({                \
-	int ix = MLX5_L2_ADDR_HASH(mac);                    \
-	bool found = false;                                 \
-	type *ptr = NULL;                                   \
-							    \
-	hlist_for_each_entry(ptr, &hash[ix], node.hlist)    \
-		if (ether_addr_equal(ptr->node.addr, mac)) {\
-			found = true;                       \
-			break;                              \
-		}                                           \
-	if (!found)                                         \
-		ptr = NULL;                                 \
-	ptr;                                                \
-})
-
-#define l2addr_hash_add(hash, mac, type, gfp) ({            \
-	int ix = MLX5_L2_ADDR_HASH(mac);                    \
-	type *ptr = NULL;                                   \
-							    \
-	ptr = kzalloc(sizeof(type), gfp);                   \
-	if (ptr) {                                          \
-		ether_addr_copy(ptr->node.addr, mac);       \
-		hlist_add_head(&ptr->node.hlist, &hash[ix]);\
-	}                                                   \
-	ptr;                                                \
-})
-
-#define l2addr_hash_del(ptr) ({                             \
-	hlist_del(&ptr->node.hlist);                        \
-	kfree(ptr);                                         \
-})
-
 struct vport_ingress {
 	struct mlx5_flow_table *acl;
 	struct mlx5_flow_group *allow_untagged_spoofchk_grp;
@@ -150,12 +106,6 @@ struct mlx5_vport {
 	u16                     enabled_events;
 };
 
-struct mlx5_l2_table {
-	struct hlist_head l2_hash[MLX5_L2_ADDR_HASH_SIZE];
-	u32                  size;
-	unsigned long        *bitmap;
-};
-
 struct mlx5_eswitch_fdb {
 	void *fdb;
 	union {
@@ -221,7 +171,6 @@ struct esw_mc_addr { /* SRIOV only */
 
 struct mlx5_eswitch {
 	struct mlx5_core_dev    *dev;
-	struct mlx5_l2_table    l2_table;
 	struct mlx5_eswitch_fdb fdb_table;
 	struct hlist_head       mc_table[MLX5_L2_ADDR_HASH_SIZE];
 	struct workqueue_struct *work_queue;
@@ -249,8 +198,6 @@ int esw_offloads_init(struct mlx5_eswitch *esw, int nvports);
 /* E-Switch API */
 int mlx5_eswitch_init(struct mlx5_core_dev *dev);
 void mlx5_eswitch_cleanup(struct mlx5_eswitch *esw);
-void mlx5_eswitch_attach(struct mlx5_eswitch *esw);
-void mlx5_eswitch_detach(struct mlx5_eswitch *esw);
 void mlx5_eswitch_vport_event(struct mlx5_eswitch *esw, struct mlx5_eqe *eqe);
 int mlx5_eswitch_enable_sriov(struct mlx5_eswitch *esw, int nvfs, int mode);
 void mlx5_eswitch_disable_sriov(struct mlx5_eswitch *esw);
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/main.c b/drivers/net/ethernet/mellanox/mlx5/core/main.c
index 361cd112bb5b..70db4257930e 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/main.c
@@ -53,6 +53,7 @@
 #include <net/devlink.h>
 #include "mlx5_core.h"
 #include "fs_core.h"
+#include "mpfs.h"
 #ifdef CONFIG_MLX5_CORE_EN
 #include "eswitch.h"
 #endif
@@ -941,11 +942,17 @@ static int mlx5_init_once(struct mlx5_core_dev *dev, struct mlx5_priv *priv)
 		goto err_tables_cleanup;
 	}
 
+	err = mlx5_mpfs_init(dev);
+	if (err) {
+		dev_err(&pdev->dev, "Failed to init l2 table %d\n", err);
+		goto err_rl_cleanup;
+	}
+
 #ifdef CONFIG_MLX5_CORE_EN
 	err = mlx5_eswitch_init(dev);
 	if (err) {
 		dev_err(&pdev->dev, "Failed to init eswitch %d\n", err);
-		goto err_rl_cleanup;
+		goto err_mpfs_cleanup;
 	}
 #endif
 
@@ -960,11 +967,11 @@ static int mlx5_init_once(struct mlx5_core_dev *dev, struct mlx5_priv *priv)
 err_eswitch_cleanup:
 #ifdef CONFIG_MLX5_CORE_EN
 	mlx5_eswitch_cleanup(dev->priv.eswitch);
-
-err_rl_cleanup:
+err_mpfs_cleanup:
 #endif
+	mlx5_mpfs_cleanup(dev);
+err_rl_cleanup:
 	mlx5_cleanup_rl_table(dev);
-
 err_tables_cleanup:
 	mlx5_cleanup_mkey_table(dev);
 	mlx5_cleanup_srq_table(dev);
@@ -984,6 +991,7 @@ static void mlx5_cleanup_once(struct mlx5_core_dev *dev)
 #ifdef CONFIG_MLX5_CORE_EN
 	mlx5_eswitch_cleanup(dev->priv.eswitch);
 #endif
+	mlx5_mpfs_cleanup(dev);
 	mlx5_cleanup_rl_table(dev);
 	mlx5_cleanup_mkey_table(dev);
 	mlx5_cleanup_srq_table(dev);
@@ -1137,10 +1145,6 @@ static int mlx5_load_one(struct mlx5_core_dev *dev, struct mlx5_priv *priv,
 		goto err_fs;
 	}
 
-#ifdef CONFIG_MLX5_CORE_EN
-	mlx5_eswitch_attach(dev->priv.eswitch);
-#endif
-
 	err = mlx5_sriov_attach(dev);
 	if (err) {
 		dev_err(&pdev->dev, "sriov init failed %d\n", err);
@@ -1174,9 +1178,6 @@ static int mlx5_load_one(struct mlx5_core_dev *dev, struct mlx5_priv *priv,
 	mlx5_sriov_detach(dev);
 
 err_sriov:
-#ifdef CONFIG_MLX5_CORE_EN
-	mlx5_eswitch_detach(dev->priv.eswitch);
-#endif
 	mlx5_cleanup_fs(dev);
 
 err_fs:
@@ -1248,9 +1249,6 @@ static int mlx5_unload_one(struct mlx5_core_dev *dev, struct mlx5_priv *priv,
 		mlx5_detach_device(dev);
 
 	mlx5_sriov_detach(dev);
-#ifdef CONFIG_MLX5_CORE_EN
-	mlx5_eswitch_detach(dev->priv.eswitch);
-#endif
 	mlx5_cleanup_fs(dev);
 	mlx5_irq_clear_affinity_hints(dev);
 	free_comp_eqs(dev);
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/mpfs.c b/drivers/net/ethernet/mellanox/mlx5/core/mpfs.c
new file mode 100644
index 000000000000..af5e49f3482f
--- /dev/null
+++ b/drivers/net/ethernet/mellanox/mlx5/core/mpfs.c
@@ -0,0 +1,205 @@
+/*
+ * Copyright (c) 2017, Mellanox Technologies. All rights reserved.
+ *
+ * This software is available to you under a choice of one of two
+ * licenses.  You may choose to be licensed under the terms of the GNU
+ * General Public License (GPL) Version 2, available from the file
+ * COPYING in the main directory of this source tree, or the
+ * OpenIB.org BSD license below:
+ *
+ *     Redistribution and use in source and binary forms, with or
+ *     without modification, are permitted provided that the following
+ *     conditions are met:
+ *
+ *      - Redistributions of source code must retain the above
+ *        copyright notice, this list of conditions and the following
+ *        disclaimer.
+ *
+ *      - Redistributions in binary form must reproduce the above
+ *        copyright notice, this list of conditions and the following
+ *        disclaimer in the documentation and/or other materials
+ *        provided with the distribution.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
+ * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
+ * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
+ * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+#include <linux/etherdevice.h>
+#include <linux/mlx5/driver.h>
+#include <linux/mlx5/mlx5_ifc.h>
+#include "mlx5_core.h"
+#include "mpfs.h"
+
+/* HW L2 Table (MPFS) management */
+static int set_l2table_entry_cmd(struct mlx5_core_dev *dev, u32 index, u8 *mac)
+{
+	u32 in[MLX5_ST_SZ_DW(set_l2_table_entry_in)]   = {0};
+	u32 out[MLX5_ST_SZ_DW(set_l2_table_entry_out)] = {0};
+	u8 *in_mac_addr;
+
+	MLX5_SET(set_l2_table_entry_in, in, opcode, MLX5_CMD_OP_SET_L2_TABLE_ENTRY);
+	MLX5_SET(set_l2_table_entry_in, in, table_index, index);
+
+	in_mac_addr = MLX5_ADDR_OF(set_l2_table_entry_in, in, mac_address);
+	ether_addr_copy(&in_mac_addr[2], mac);
+
+	return mlx5_cmd_exec(dev, in, sizeof(in), out, sizeof(out));
+}
+
+static int del_l2table_entry_cmd(struct mlx5_core_dev *dev, u32 index)
+{
+	u32 in[MLX5_ST_SZ_DW(delete_l2_table_entry_in)]   = {0};
+	u32 out[MLX5_ST_SZ_DW(delete_l2_table_entry_out)] = {0};
+
+	MLX5_SET(delete_l2_table_entry_in, in, opcode, MLX5_CMD_OP_DELETE_L2_TABLE_ENTRY);
+	MLX5_SET(delete_l2_table_entry_in, in, table_index, index);
+	return mlx5_cmd_exec(dev, in, sizeof(in), out, sizeof(out));
+}
+
+/* UC L2 table hash node */
+struct l2table_node {
+	struct l2addr_node node;
+	u32                index; /* index in HW l2 table */
+};
+
+struct mlx5_mpfs {
+	struct hlist_head    hash[MLX5_L2_ADDR_HASH_SIZE];
+	struct mutex         lock; /* Synchronize l2 table access */
+	u32                  size;
+	unsigned long        *bitmap;
+};
+
+static int alloc_l2table_index(struct mlx5_mpfs *l2table, u32 *ix)
+{
+	int err = 0;
+
+	*ix = find_first_zero_bit(l2table->bitmap, l2table->size);
+	if (*ix >= l2table->size)
+		err = -ENOSPC;
+	else
+		__set_bit(*ix, l2table->bitmap);
+
+	return err;
+}
+
+static void free_l2table_index(struct mlx5_mpfs *l2table, u32 ix)
+{
+	__clear_bit(ix, l2table->bitmap);
+}
+
+int mlx5_mpfs_init(struct mlx5_core_dev *dev)
+{
+	int l2table_size = 1 << MLX5_CAP_GEN(dev, log_max_l2_table);
+	struct mlx5_mpfs *mpfs;
+
+	if (!MLX5_VPORT_MANAGER(dev))
+		return 0;
+
+	mpfs = kzalloc(sizeof(*mpfs), GFP_KERNEL);
+	if (!mpfs)
+		return -ENOMEM;
+
+	mutex_init(&mpfs->lock);
+	mpfs->size   = l2table_size;
+	mpfs->bitmap = kcalloc(BITS_TO_LONGS(l2table_size),
+			       sizeof(uintptr_t), GFP_KERNEL);
+	if (!mpfs->bitmap) {
+		kfree(mpfs);
+		return -ENOMEM;
+	}
+
+	dev->priv.mpfs = mpfs;
+	return 0;
+}
+
+void mlx5_mpfs_cleanup(struct mlx5_core_dev *dev)
+{
+	struct mlx5_mpfs *mpfs = dev->priv.mpfs;
+
+	if (!MLX5_VPORT_MANAGER(dev))
+		return;
+
+	WARN_ON(!hlist_empty(mpfs->hash));
+	kfree(mpfs->bitmap);
+	kfree(mpfs);
+}
+
+int mlx5_mpfs_add_mac(struct mlx5_core_dev *dev, u8 *mac)
+{
+	struct mlx5_mpfs *mpfs = dev->priv.mpfs;
+	struct l2table_node *l2addr;
+	u32 index;
+	int err;
+
+	if (!MLX5_VPORT_MANAGER(dev))
+		return 0;
+
+	if (is_multicast_ether_addr(mac))
+		return 0;
+
+	mutex_lock(&mpfs->lock);
+
+	l2addr = l2addr_hash_find(mpfs->hash, mac, struct l2table_node);
+	if (l2addr) {
+		err = -EEXIST;
+		goto abort;
+	}
+
+	err = alloc_l2table_index(mpfs, &index);
+	if (err)
+		goto abort;
+
+	l2addr = l2addr_hash_add(mpfs->hash, mac, struct l2table_node, GFP_KERNEL);
+	if (!l2addr) {
+		free_l2table_index(mpfs, index);
+		err = -ENOMEM;
+		goto abort;
+	}
+
+	l2addr->index = index;
+	err = set_l2table_entry_cmd(dev, index, mac);
+	if (err) {
+		l2addr_hash_del(l2addr);
+		free_l2table_index(mpfs, index);
+	}
+
+abort:
+	mutex_unlock(&mpfs->lock);
+	return err;
+}
+
+int mlx5_mpfs_del_mac(struct mlx5_core_dev *dev, u8 *mac)
+{
+	struct mlx5_mpfs *mpfs = dev->priv.mpfs;
+	struct l2table_node *l2addr;
+	u32 index;
+	int err;
+
+	if (!MLX5_VPORT_MANAGER(dev))
+		return 0;
+
+	if (is_multicast_ether_addr(mac))
+		return 0;
+
+	mutex_lock(&mpfs->lock);
+
+	l2addr = l2addr_hash_find(mpfs->hash, mac, struct l2table_node);
+	if (!l2addr) {
+		err = -ENOENT;
+		goto unlock;
+	}
+
+	index = l2addr->index;
+	del_l2table_entry_cmd(dev, index);
+	l2addr_hash_del(l2addr);
+	free_l2table_index(mpfs, index);
+unlock:
+	mutex_unlock(&mpfs->lock);
+	return err;
+}
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/mpfs.h b/drivers/net/ethernet/mellanox/mlx5/core/mpfs.h
new file mode 100644
index 000000000000..1db560f689a6
--- /dev/null
+++ b/drivers/net/ethernet/mellanox/mlx5/core/mpfs.h
@@ -0,0 +1,98 @@
+/*
+ * Copyright (c) 2017, Mellanox Technologies, Ltd.  All rights reserved.
+ *
+ * This software is available to you under a choice of one of two
+ * licenses.  You may choose to be licensed under the terms of the GNU
+ * General Public License (GPL) Version 2, available from the file
+ * COPYING in the main directory of this source tree, or the
+ * OpenIB.org BSD license below:
+ *
+ *     Redistribution and use in source and binary forms, with or
+ *     without modification, are permitted provided that the following
+ *     conditions are met:
+ *
+ *      - Redistributions of source code must retain the above
+ *        copyright notice, this list of conditions and the following
+ *        disclaimer.
+ *
+ *      - Redistributions in binary form must reproduce the above
+ *        copyright notice, this list of conditions and the following
+ *        disclaimer in the documentation and/or other materials
+ *        provided with the distribution.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
+ * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
+ * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
+ * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+#ifndef __MLX5_MPFS_H__
+#define __MLX5_MPFS_H__
+
+#include <linux/if_ether.h>
+#include <linux/mlx5/device.h>
+
+#define MLX5_VPORT_MANAGER(mdev) \
+	(MLX5_CAP_GEN(mdev, vport_group_manager) && mlx5_core_is_pf(mdev))
+
+/* L2 -mac address based- hash helpers */
+#define MLX5_L2_ADDR_HASH_SIZE (BIT(BITS_PER_BYTE))
+#define MLX5_L2_ADDR_HASH(addr) (addr[5])
+
+struct l2addr_node {
+	struct hlist_node hlist;
+	u8                addr[ETH_ALEN];
+};
+
+#define for_each_l2hash_node(hn, tmp, hash, i) \
+	for (i = 0; i < MLX5_L2_ADDR_HASH_SIZE; i++) \
+		hlist_for_each_entry_safe(hn, tmp, &(hash)[i], hlist)
+
+#define l2addr_hash_find(hash, mac, type) ({                \
+	int ix = MLX5_L2_ADDR_HASH(mac);                    \
+	bool found = false;                                 \
+	type *ptr = NULL;                                   \
+							    \
+	hlist_for_each_entry(ptr, &(hash)[ix], node.hlist)  \
+		if (ether_addr_equal(ptr->node.addr, mac)) {\
+			found = true;                       \
+			break;                              \
+		}                                           \
+	if (!found)                                         \
+		ptr = NULL;                                 \
+	ptr;                                                \
+})
+
+#define l2addr_hash_add(hash, mac, type, gfp) ({            \
+	int ix = MLX5_L2_ADDR_HASH(mac);                    \
+	type *ptr = NULL;                                   \
+							    \
+	ptr = kzalloc(sizeof(type), gfp);                   \
+	if (ptr) {                                          \
+		ether_addr_copy(ptr->node.addr, mac);       \
+		hlist_add_head(&ptr->node.hlist, &(hash)[ix]);\
+	}                                                   \
+	ptr;                                                \
+})
+
+#define l2addr_hash_del(ptr) ({                             \
+	hlist_del(&(ptr)->node.hlist);                      \
+	kfree(ptr);                                         \
+})
+
+#ifdef CONFIG_MLX5_MPFS
+int  mlx5_mpfs_init(struct mlx5_core_dev *dev);
+void mlx5_mpfs_cleanup(struct mlx5_core_dev *dev);
+int  mlx5_mpfs_add_mac(struct mlx5_core_dev *dev, u8 *mac);
+int  mlx5_mpfs_del_mac(struct mlx5_core_dev *dev, u8 *mac);
+#else /* #ifndef CONFIG_MLX5_MPFS */
+static inline int  mlx5_mpfs_init(struct mlx5_core_dev *dev) { return 0; }
+static inline void mlx5_mpfs_cleanup(struct mlx5_core_dev *dev) {}
+static inline int  mlx5_mpfs_add_mac(struct mlx5_core_dev *dev, u8 *mac) { return 0; }
+static inline int  mlx5_mpfs_del_mac(struct mlx5_core_dev *dev, u8 *mac) { return 0; }
+#endif
+#endif
diff --git a/include/linux/mlx5/driver.h b/include/linux/mlx5/driver.h
index 6ea2f5734e37..382f688b7231 100644
--- a/include/linux/mlx5/driver.h
+++ b/include/linux/mlx5/driver.h
@@ -545,6 +545,7 @@ struct mlx5_fc_stats {
 	unsigned long sampling_interval; /* jiffies */
 };
 
+struct mlx5_mpfs;
 struct mlx5_eswitch;
 struct mlx5_lag;
 struct mlx5_pagefault;
@@ -642,6 +643,7 @@ struct mlx5_priv {
 	spinlock_t              ctx_lock;
 
 	struct mlx5_flow_steering *steering;
+	struct mlx5_mpfs        *mpfs;
 	struct mlx5_eswitch     *eswitch;
 	struct mlx5_core_sriov	sriov;
 	struct mlx5_lag		*lag;
-- 
2.11.0

Re: [PATCH RFC net-next 3/4] net/mlx5: Separate between eswitch and MPFS l2 table logic

[Or Gerlitz]

On Thu, Jun 8, 2017 at 2:42 AM, Saeed Mahameed <saeedm@mellanox.com> wrote:
[...]
>  9 files changed, 376 insertions(+), 223 deletions(-)

a bit of heavy duty for this sort of sensitive change, should
investigate if it can be broken a bit

> +int mlx5_mpfs_init(struct mlx5_core_dev *dev)
> +{
> +       int l2table_size = 1 << MLX5_CAP_GEN(dev, log_max_l2_table);
> +       struct mlx5_mpfs *mpfs;
> +
> +       if (!MLX5_VPORT_MANAGER(dev))
> +               return 0;

In commit 955bc48081805c053 we added

+ static bool mlx5e_is_eswitch_vport_mngr(struct mlx5_core_dev *mdev)
+{
+       return (MLX5_CAP_GEN(mdev, vport_group_manager) &&
+               MLX5_CAP_GEN(mdev, port_type) == MLX5_CAP_PORT_TYPE_ETH);
+}
+

and now you add

+#define MLX5_VPORT_MANAGER(mdev) \
+       (MLX5_CAP_GEN(mdev, vport_group_manager) && mlx5_core_is_pf(mdev))
+

would be good to align things across the place with a pre-patch and
then use the whatever check
we need to apply

BTW  mlx5e_is_eswitch_vport_mngr() should return false in the down
stream patch when eswitch is compiled out, right?

> +void mlx5_mpfs_cleanup(struct mlx5_core_dev *dev)
> +{
> +       struct mlx5_mpfs *mpfs = dev->priv.mpfs;
> +
> +       if (!MLX5_VPORT_MANAGER(dev))
> +               return;
> +
> +       WARN_ON(!hlist_empty(mpfs->hash));

I don't see any line with WARN_ON that was deleted by this patch, why add that?

> +       kfree(mpfs->bitmap);
> +       kfree(mpfs);
> +}
> +
> +int mlx5_mpfs_add_mac(struct mlx5_core_dev *dev, u8 *mac)
> +{

> +       if (!MLX5_VPORT_MANAGER(dev))
> +               return 0;
> +
> +       if (is_multicast_ether_addr(mac))
> +               return 0;

It would have been much better reviewed and maintained if we can code
things in a manner under which we don't get here if not appropriate and
not simulate successful return.

> +int mlx5_mpfs_del_mac(struct mlx5_core_dev *dev, u8 *mac)
> +{

> +       if (!MLX5_VPORT_MANAGER(dev))
> +               return 0;
> +
> +       if (is_multicast_ether_addr(mac))
> +               return 0;

same comment as for the add_mac call

Re: [PATCH RFC net-next 3/4] net/mlx5: Separate between eswitch and MPFS l2 table logic

[Saeed Mahameed]

On Thu, Jun 8, 2017 at 12:13 PM, Or Gerlitz <gerlitz.or@gmail.com> wrote:
> On Thu, Jun 8, 2017 at 2:42 AM, Saeed Mahameed <saeedm@mellanox.com> wrote:
> [...]
>>  9 files changed, 376 insertions(+), 223 deletions(-)
>
> a bit of heavy duty for this sort of sensitive change, should
> investigate if it can be broken a bit
>

yes, hence the RFC.

>> +int mlx5_mpfs_init(struct mlx5_core_dev *dev)
>> +{
>> +       int l2table_size = 1 << MLX5_CAP_GEN(dev, log_max_l2_table);
>> +       struct mlx5_mpfs *mpfs;
>> +
>> +       if (!MLX5_VPORT_MANAGER(dev))
>> +               return 0;
>
> In commit 955bc48081805c053 we added
>
> + static bool mlx5e_is_eswitch_vport_mngr(struct mlx5_core_dev *mdev)
> +{
> +       return (MLX5_CAP_GEN(mdev, vport_group_manager) &&
> +               MLX5_CAP_GEN(mdev, port_type) == MLX5_CAP_PORT_TYPE_ETH);
> +}
> +
>
> and now you add
>
> +#define MLX5_VPORT_MANAGER(mdev) \
> +       (MLX5_CAP_GEN(mdev, vport_group_manager) && mlx5_core_is_pf(mdev))
> +
>
> would be good to align things across the place with a pre-patch and
> then use the whatever check
> we need to apply
>

Sure.

> BTW  mlx5e_is_eswitch_vport_mngr() should return false in the down
> stream patch when eswitch is compiled out, right?
>

No, vport manager is a vport manager whether it wants to initialize
the eswitch or not.
it is up to the stub function to decide what to do when eswitch is compiled out.

>> +void mlx5_mpfs_cleanup(struct mlx5_core_dev *dev)
>> +{
>> +       struct mlx5_mpfs *mpfs = dev->priv.mpfs;
>> +
>> +       if (!MLX5_VPORT_MANAGER(dev))
>> +               return;
>> +
>> +       WARN_ON(!hlist_empty(mpfs->hash));
>
> I don't see any line with WARN_ON that was deleted by this patch, why add that?
>

Just to make sure we freed anything on cleanup, for now i  will keep
this, i will consider removing this on
final submission.

>> +       kfree(mpfs->bitmap);
>> +       kfree(mpfs);
>> +}
>> +
>> +int mlx5_mpfs_add_mac(struct mlx5_core_dev *dev, u8 *mac)
>> +{
>
>> +       if (!MLX5_VPORT_MANAGER(dev))
>> +               return 0;
>> +
>> +       if (is_multicast_ether_addr(mac))
>> +               return 0;
>
> It would have been much better reviewed and maintained if we can code
> things in a manner under which we don't get here if not appropriate and
> not simulate successful return.
>

Sure, will consider that, i just wanted to avoid ugly indentations in
mlx5e set_rx_mode en_fs.c

>> +int mlx5_mpfs_del_mac(struct mlx5_core_dev *dev, u8 *mac)
>> +{
>
>> +       if (!MLX5_VPORT_MANAGER(dev))
>> +               return 0;
>> +
>> +       if (is_multicast_ether_addr(mac))
>> +               return 0;
>
> same comment as for the add_mac call

Re: [PATCH RFC net-next 3/4] net/mlx5: Separate between eswitch and MPFS l2 table logic

[Or Gerlitz]

On Thu, Jun 8, 2017 at 1:26 PM, Saeed Mahameed
<saeedm@dev.mellanox.co.il> wrote:
> On Thu, Jun 8, 2017 at 12:13 PM, Or Gerlitz <gerlitz.or@gmail.com> wrote:


>>> +int mlx5_mpfs_add_mac(struct mlx5_core_dev *dev, u8 *mac)
>>> +{
>>
>>> +       if (!MLX5_VPORT_MANAGER(dev))
>>> +               return 0;
>>> +
>>> +       if (is_multicast_ether_addr(mac))
>>> +               return 0;

>> It would have been much better reviewed and maintained if we can code
>> things in a manner under which we don't get here if not appropriate and
>> not simulate successful return.

> Sure, will consider that, i just wanted to avoid ugly indentations in
> mlx5e set_rx_mode en_fs.c

I understand your concern re ugly indentations, but hopefully we can avoid them

Re: [PATCH RFC net-next 3/4] net/mlx5: Separate between eswitch and MPFS l2 table logic

[Or Gerlitz]

On Thu, Jun 8, 2017 at 2:42 AM, Saeed Mahameed <saeedm@mellanox.com> wrote:

> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_fs.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_fs.c

> @@ -363,17 +364,28 @@ static void mlx5e_del_vlan_rules(struct mlx5e_priv *priv)
>  static void mlx5e_execute_l2_action(struct mlx5e_priv *priv,
>                                     struct mlx5e_l2_hash_node *hn)
>  {
> -       switch (hn->action) {
> +       u8 action = hn->action;
> +       int l2_err = 0;
> +
> +       switch (action) {
>         case MLX5E_ACTION_ADD:
>                 mlx5e_add_l2_flow_rule(priv, &hn->ai, MLX5E_FULLMATCH);
> +               /* mlx5_mpfs_add_mac will skip mc addresses */
> +               l2_err = mlx5_mpfs_add_mac(priv->mdev, hn->ai.addr);
>                 hn->action = MLX5E_ACTION_NONE;
>                 break;
>
>         case MLX5E_ACTION_DEL:
> +               /* mlx5_mpfs_del_mac will skip mc addresses */
> +               l2_err = mlx5_mpfs_del_mac(priv->mdev, hn->ai.addr);
>                 mlx5e_del_l2_flow_rule(priv, &hn->ai);
>                 mlx5e_del_l2_from_hash(hn);
>                 break;
>         }

> --- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c

>  static int esw_add_uc_addr(struct mlx5_eswitch *esw, struct vport_addr *vaddr)
>  {
> -       struct hlist_head *hash = esw->l2_table.l2_hash;
> -       struct esw_uc_addr *esw_uc;
>         u8 *mac = vaddr->node.addr;
>         u32 vport = vaddr->vport;
>         int err;
>
> -       esw_uc = l2addr_hash_find(hash, mac, struct esw_uc_addr);
> -       if (esw_uc) {
> +       /* Skip l2_table_add for PFs,
> +        * it is already done by the PF netdev in mlx5e_execute_l2_action
> +        */
> +       if (!vport)
> +               goto fdb_add;
> +

why !vport means we should go there?

> +       err = mlx5_mpfs_add_mac(esw->dev, mac);
> +       if (err) {
>                 esw_warn(esw->dev,
> -                        "Failed to set L2 mac(%pM) for vport(%d), mac is already in use by vport(%d)\n",
> -                        mac, vport, esw_uc->vport);
> -               return -EEXIST;
> +                        "Failed to add L2 table mac(%pM) for vport(%d), err(%d)\n",
> +                        mac, vport, err);
> +               return err;
>         }
>
> -       esw_uc = l2addr_hash_add(hash, mac, struct esw_uc_addr, GFP_KERNEL);
> -       if (!esw_uc)
> -               return -ENOMEM;
> -       esw_uc->vport = vport;
> -
> -       err = set_l2_table_entry(esw->dev, mac, 0, 0, &esw_uc->table_index);
> -       if (err)
> -               goto abort;
> -
> +fdb_add:
>         /* SRIOV is enabled: Forward UC MAC to vport */
>         if (esw->fdb_table.fdb && esw->mode == SRIOV_LEGACY)
>                 vaddr->flow_rule = esw_fdb_set_vport_rule(esw, mac, vport);
>
> -       esw_debug(esw->dev, "\tADDED UC MAC: vport[%d] %pM index:%d fr(%p)\n",
> -                 vport, mac, esw_uc->table_index, vaddr->flow_rule);
> -       return err;
> -abort:
> -       l2addr_hash_del(esw_uc);
> +       esw_debug(esw->dev, "\tADDED UC MAC: vport[%d] %pM fr(%p)\n",
> +                 vport, mac, vaddr->flow_rule);
> +
>         return err;
>  }


>  static int esw_del_uc_addr(struct mlx5_eswitch *esw, struct vport_addr *vaddr)
>  {
> -       struct hlist_head *hash = esw->l2_table.l2_hash;
> -       struct esw_uc_addr *esw_uc;
>         u8 *mac = vaddr->node.addr;
>         u32 vport = vaddr->vport;
> +       int err = 0;
>
> -       esw_uc = l2addr_hash_find(hash, mac, struct esw_uc_addr);
> -       if (!esw_uc || esw_uc->vport != vport) {
> -               esw_debug(esw->dev,
> -                         "MAC(%pM) doesn't belong to vport (%d)\n",
> -                         mac, vport);
> -               return -EINVAL;
> -       }
> -       esw_debug(esw->dev, "\tDELETE UC MAC: vport[%d] %pM index:%d fr(%p)\n",
> -                 vport, mac, esw_uc->table_index, vaddr->flow_rule);
> +       /* Skip l2_table_del for PFs,
> +        * it is already done by the PF netdev in mlx5e_execute_l2_action
> +        */
> +       if (!vport)
> +               goto fdb_del;
>
> -       del_l2_table_entry(esw->dev, esw_uc->table_index);
> +       err = mlx5_mpfs_del_mac(esw->dev, mac);
> +       if (err)
> +               esw_warn(esw->dev,
> +                        "Failed to del L2 table mac(%pM) for vport(%d), err(%d)\n",
> +                        mac, vport, err);
>
> +fdb_del:
>         if (vaddr->flow_rule)
>                 mlx5_del_flow_rules(vaddr->flow_rule);
>         vaddr->flow_rule = NULL;
>
> -       l2addr_hash_del(esw_uc);
>         return 0;
>  }
>

can we somehow put these areas into a pre-patch?

Re: [PATCH RFC net-next 3/4] net/mlx5: Separate between eswitch and MPFS l2 table logic

[Jes Sorensen]

On 06/07/2017 07:42 PM, Saeed Mahameed wrote:
> Multi-Physical Function Switch (MPFs) is required for when multi-PF
> configuration is enabled to allow passing user configured unicast MAC
> addresses to the requesting PF.
> 
> Before this patch eswitch.c used to manage the HW MPFS l2 table,
> eswitch always enabled vport(0) (NIC PF) vport's contexts update on unicast
> mac address list changes, to populate the PF's MPFS L2 table accordingly,
> even if SRIOV was not enabled.
> 
> In downstream patch we would like to allow compiling the driver without
> eswitch functionalities, for that we move MPFS l2 table logic out
> of eswitch.c into its own file, and provide Kconfig flag (MLX5_MPFS) to
> allow compiling out MPFS for those who don't want Multi-PF support
> 
> NIC PF netdevice will now directly update MPFS l2 table via the new MPFS
> API. VF netdevice has no access to MPFS L2 table, so e-Switch will remain
> responsible of updating its PF MPFS l2 table on behalf of its VFs.
> 
> Due to this change we also don't require enabling vport(0) (PF vport)
> unicast mac changes events anymore, for when SRIOV is not enabled.
> Which means eswitch is now activated only on SRIOV activation, and not
> required otherwise.


On overall it looks good - one nit.

> +static int alloc_l2table_index(struct mlx5_mpfs *l2table, u32 *ix)
> +{
> +	int err = 0;
> +
> +	*ix = find_first_zero_bit(l2table->bitmap, l2table->size);
> +	if (*ix >= l2table->size)
> +		err = -ENOSPC;
> +	else
> +		__set_bit(*ix, l2table->bitmap);
> +
> +	return err;
> +}

You pass in a pointer for ix but you don't modify it, why not just pass 
in the value?.

> +static void free_l2table_index(struct mlx5_mpfs *l2table, u32 ix)
> +{
> +	__clear_bit(ix, l2table->bitmap);
> +}

Here you stick to the u32.

Jes

Re: [PATCH RFC net-next 3/4] net/mlx5: Separate between eswitch and MPFS l2 table logic

[Saeed Mahameed]

On Mon, Jun 12, 2017 at 8:52 PM, Jes Sorensen <jsorensen@fb.com> wrote:
> On 06/07/2017 07:42 PM, Saeed Mahameed wrote:
>>
>> Multi-Physical Function Switch (MPFs) is required for when multi-PF
>> configuration is enabled to allow passing user configured unicast MAC
>> addresses to the requesting PF.
>>
>> Before this patch eswitch.c used to manage the HW MPFS l2 table,
>> eswitch always enabled vport(0) (NIC PF) vport's contexts update on
>> unicast
>> mac address list changes, to populate the PF's MPFS L2 table accordingly,
>> even if SRIOV was not enabled.
>>
>> In downstream patch we would like to allow compiling the driver without
>> eswitch functionalities, for that we move MPFS l2 table logic out
>> of eswitch.c into its own file, and provide Kconfig flag (MLX5_MPFS) to
>> allow compiling out MPFS for those who don't want Multi-PF support
>>
>> NIC PF netdevice will now directly update MPFS l2 table via the new MPFS
>> API. VF netdevice has no access to MPFS L2 table, so e-Switch will remain
>> responsible of updating its PF MPFS l2 table on behalf of its VFs.
>>
>> Due to this change we also don't require enabling vport(0) (PF vport)
>> unicast mac changes events anymore, for when SRIOV is not enabled.
>> Which means eswitch is now activated only on SRIOV activation, and not
>> required otherwise.
>
>
>
> On overall it looks good - one nit.
>
>> +static int alloc_l2table_index(struct mlx5_mpfs *l2table, u32 *ix)
>> +{
>> +       int err = 0;
>> +
>> +       *ix = find_first_zero_bit(l2table->bitmap, l2table->size);
>> +       if (*ix >= l2table->size)
>> +               err = -ENOSPC;
>> +       else
>> +               __set_bit(*ix, l2table->bitmap);
>> +
>> +       return err;
>> +}
>
>
> You pass in a pointer for ix but you don't modify it, why not just pass in
> the value?.
>

we do modify ix:
   *ix = find_first_zero_bit(l2table->bitmap, l2table->size);

The idea is to find the next free index and return it to the caller

>> +static void free_l2table_index(struct mlx5_mpfs *l2table, u32 ix)
>> +{
>> +       __clear_bit(ix, l2table->bitmap);
>> +}
>
>
> Here you stick to the u32.

to free the index we allocated from before

>
> Jes

Re: [PATCH RFC net-next 3/4] net/mlx5: Separate between eswitch and MPFS l2 table logic

[Jes Sorensen]

On 06/13/2017 01:49 PM, Saeed Mahameed wrote:
> On Mon, Jun 12, 2017 at 8:52 PM, Jes Sorensen <jsorensen@fb.com> wrote:
>> On 06/07/2017 07:42 PM, Saeed Mahameed wrote:
>>>
>>> Multi-Physical Function Switch (MPFs) is required for when multi-PF
>>> configuration is enabled to allow passing user configured unicast MAC
>>> addresses to the requesting PF.
>>>
>>> Before this patch eswitch.c used to manage the HW MPFS l2 table,
>>> eswitch always enabled vport(0) (NIC PF) vport's contexts update on
>>> unicast
>>> mac address list changes, to populate the PF's MPFS L2 table accordingly,
>>> even if SRIOV was not enabled.
>>>
>>> In downstream patch we would like to allow compiling the driver without
>>> eswitch functionalities, for that we move MPFS l2 table logic out
>>> of eswitch.c into its own file, and provide Kconfig flag (MLX5_MPFS) to
>>> allow compiling out MPFS for those who don't want Multi-PF support
>>>
>>> NIC PF netdevice will now directly update MPFS l2 table via the new MPFS
>>> API. VF netdevice has no access to MPFS L2 table, so e-Switch will remain
>>> responsible of updating its PF MPFS l2 table on behalf of its VFs.
>>>
>>> Due to this change we also don't require enabling vport(0) (PF vport)
>>> unicast mac changes events anymore, for when SRIOV is not enabled.
>>> Which means eswitch is now activated only on SRIOV activation, and not
>>> required otherwise.
>>
>>
>>
>> On overall it looks good - one nit.
>>
>>> +static int alloc_l2table_index(struct mlx5_mpfs *l2table, u32 *ix)
>>> +{
>>> +       int err = 0;
>>> +
>>> +       *ix = find_first_zero_bit(l2table->bitmap, l2table->size);
>>> +       if (*ix >= l2table->size)
>>> +               err = -ENOSPC;
>>> +       else
>>> +               __set_bit(*ix, l2table->bitmap);
>>> +
>>> +       return err;
>>> +}
>>
>>
>> You pass in a pointer for ix but you don't modify it, why not just pass in
>> the value?.
>>
> 
> we do modify ix:
>     *ix = find_first_zero_bit(l2table->bitmap, l2table->size);
> 
> The idea is to find the next free index and return it to the caller

I clearly need new glasses :(

Jes

[PATCH RFC net-next 4/4] net/mlx5: Add CONFIG_MLX5_ESWITCH Kconfig

[Saeed Mahameed]

This patch gives the option to chose whether to compile the driver with or
without eswitch/eswitch_offloads(switchdev mode)/en_rep(VF representors)
and en_tc offloads.

It also removes most of the above modules headers declarations and stubs
out the API functions which are used outside these modules.

Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/Kconfig   |  7 +++++
 drivers/net/ethernet/mellanox/mlx5/core/Makefile  |  6 +++--
 drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 33 +++++++++++++++--------
 drivers/net/ethernet/mellanox/mlx5/core/en_rep.h  |  8 ++++++
 drivers/net/ethernet/mellanox/mlx5/core/en_rx.c   |  2 ++
 drivers/net/ethernet/mellanox/mlx5/core/en_tc.h   |  7 +++++
 drivers/net/ethernet/mellanox/mlx5/core/eswitch.h | 23 +++++++++++-----
 drivers/net/ethernet/mellanox/mlx5/core/main.c    | 10 +------
 8 files changed, 68 insertions(+), 28 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/Kconfig b/drivers/net/ethernet/mellanox/mlx5/core/Kconfig
index 49559e11af86..4024069139a2 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/Kconfig
+++ b/drivers/net/ethernet/mellanox/mlx5/core/Kconfig
@@ -38,6 +38,13 @@ config MLX5_MPFS
 	  Mellanox Technologies Ethernet Multi-Physical Function Switch (MPFS)
           support in ConnectX NIC.
 
+config MLX5_ESWITCH
+	bool "Mellanox Technologies MLX5 SRIOV E-Switch support"
+	depends on MLX5_CORE_EN
+	default y
+	---help---
+	  Mellanox Technologies Ethernet SRIOV E-Switch support in ConnectX NIC.
+
 config MLX5_CORE_EN_DCB
 	bool "Data Center Bridging (DCB) Support"
 	default y
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/Makefile b/drivers/net/ethernet/mellanox/mlx5/core/Makefile
index 8c493aeec392..2ca2dc1b1147 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/Makefile
+++ b/drivers/net/ethernet/mellanox/mlx5/core/Makefile
@@ -8,13 +8,15 @@ mlx5_core-y :=	main.o cmd.o debugfs.o fw.o eq.o uar.o pagealloc.o \
 
 mlx5_core-$(CONFIG_MLX5_FPGA) += fpga/cmd.o fpga/core.o
 
-mlx5_core-$(CONFIG_MLX5_CORE_EN) += wq.o eswitch.o eswitch_offloads.o \
+mlx5_core-$(CONFIG_MLX5_CORE_EN) += wq.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
+		en_arfs.o en_fs_ethtool.o en_selftest.o
 
 mlx5_core-$(CONFIG_MLX5_MPFS) += mpfs.o
 
+mlx5_core-$(CONFIG_MLX5_ESWITCH) += eswitch.o eswitch_offloads.o en_rep.o en_tc.o
+
 mlx5_core-$(CONFIG_MLX5_CORE_EN_DCB) +=  en_dcbnl.o
 
 mlx5_core-$(CONFIG_MLX5_CORE_IPOIB) += ipoib.o
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
index 1514027a5c51..1f0a1b78f609 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
@@ -2961,9 +2961,8 @@ static int mlx5e_modify_channels_vsd(struct mlx5e_channels *chs, bool vsd)
 	return 0;
 }
 
-static int mlx5e_setup_tc(struct net_device *netdev, u8 tc)
+static int mlx5e_setup_tc(struct mlx5e_priv *priv, u8 tc)
 {
-	struct mlx5e_priv *priv = netdev_priv(netdev);
 	struct mlx5e_channels new_channels = {};
 	int err = 0;
 
@@ -2995,6 +2994,7 @@ static int mlx5e_ndo_setup_tc(struct net_device *dev, u32 handle,
 {
 	struct mlx5e_priv *priv = netdev_priv(dev);
 
+#ifdef CONFIG_MLX5_ESWITCH
 	if (TC_H_MAJ(handle) != TC_H_MAJ(TC_H_INGRESS))
 		goto mqprio;
 
@@ -3013,12 +3013,13 @@ static int mlx5e_ndo_setup_tc(struct net_device *dev, u32 handle,
 	}
 
 mqprio:
+#endif
 	if (tc->type != TC_SETUP_MQPRIO)
-		return -EINVAL;
+		return -EOPNOTSUPP;
 
 	tc->mqprio->hw = TC_MQPRIO_HW_OFFLOAD_TCS;
 
-	return mlx5e_setup_tc(dev, tc->mqprio->num_tc);
+	return mlx5e_setup_tc(priv, tc->mqprio->num_tc);
 }
 
 static void
@@ -3316,6 +3317,7 @@ static int mlx5e_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
 	}
 }
 
+#ifdef CONFIG_MLX5_ESWITCH
 static int mlx5e_set_vf_mac(struct net_device *dev, int vf, u8 *mac)
 {
 	struct mlx5e_priv *priv = netdev_priv(dev);
@@ -3418,6 +3420,7 @@ static int mlx5e_get_vf_stats(struct net_device *dev,
 	return mlx5_eswitch_get_vport_stats(mdev->priv.eswitch, vf + 1,
 					    vf_stats);
 }
+#endif
 
 static void mlx5e_add_vxlan_port(struct net_device *netdev,
 				 struct udp_tunnel_info *ti)
@@ -3659,6 +3662,7 @@ static const struct net_device_ops mlx5e_netdev_ops_basic = {
 #endif
 };
 
+#ifdef CONFIG_MLX5_ESWITCH
 static const struct net_device_ops mlx5e_netdev_ops_sriov = {
 	.ndo_open                = mlx5e_open,
 	.ndo_stop                = mlx5e_close,
@@ -3697,6 +3701,7 @@ static const struct net_device_ops mlx5e_netdev_ops_sriov = {
 	.ndo_has_offload_stats	 = mlx5e_has_offload_stats,
 	.ndo_get_offload_stats	 = mlx5e_get_offload_stats,
 };
+#endif
 
 static int mlx5e_check_required_hca_cap(struct mlx5_core_dev *mdev)
 {
@@ -3923,9 +3928,11 @@ static void mlx5e_set_netdev_dev_addr(struct net_device *netdev)
 	}
 }
 
+#if IS_ENABLED(CONFIG_NET_SWITCHDEV) && IS_ENABLED(CONFIG_MLX5_ESWITCH)
 static const struct switchdev_ops mlx5e_switchdev_ops = {
 	.switchdev_port_attr_get	= mlx5e_attr_get,
 };
+#endif
 
 static void mlx5e_build_nic_netdev(struct net_device *netdev)
 {
@@ -3936,15 +3943,17 @@ static void mlx5e_build_nic_netdev(struct net_device *netdev)
 
 	SET_NETDEV_DEV(netdev, &mdev->pdev->dev);
 
-	if (MLX5_CAP_GEN(mdev, vport_group_manager)) {
-		netdev->netdev_ops = &mlx5e_netdev_ops_sriov;
 #ifdef CONFIG_MLX5_CORE_EN_DCB
-		if (MLX5_CAP_GEN(mdev, qos))
-			netdev->dcbnl_ops = &mlx5e_dcbnl_ops;
+	if (MLX5_CAP_GEN(mdev, vport_group_manager) && MLX5_CAP_GEN(mdev, qos))
+		netdev->dcbnl_ops = &mlx5e_dcbnl_ops;
+#endif
+
+#ifdef CONFIG_MLX5_ESWITCH
+	if (MLX5_CAP_GEN(mdev, vport_group_manager))
+		netdev->netdev_ops = &mlx5e_netdev_ops_sriov;
+	else
 #endif
-	} else {
 		netdev->netdev_ops = &mlx5e_netdev_ops_basic;
-	}
 
 	netdev->watchdog_timeo    = 15 * HZ;
 
@@ -4016,7 +4025,7 @@ static void mlx5e_build_nic_netdev(struct net_device *netdev)
 
 	mlx5e_set_netdev_dev_addr(netdev);
 
-#ifdef CONFIG_NET_SWITCHDEV
+#if IS_ENABLED(CONFIG_NET_SWITCHDEV) && IS_ENABLED(CONFIG_MLX5_ESWITCH)
 	if (MLX5_CAP_GEN(mdev, vport_group_manager))
 		netdev->switchdev_ops = &mlx5e_switchdev_ops;
 #endif
@@ -4366,6 +4375,7 @@ static void *mlx5e_add(struct mlx5_core_dev *mdev)
 	if (err)
 		return NULL;
 
+#ifdef CONFIG_MLX5_ESWITCH
 	if (MLX5_CAP_GEN(mdev, vport_group_manager)) {
 		rpriv = mlx5e_alloc_nic_rep_priv(mdev);
 		if (!rpriv) {
@@ -4373,6 +4383,7 @@ static void *mlx5e_add(struct mlx5_core_dev *mdev)
 			return NULL;
 		}
 	}
+#endif
 
 	netdev = mlx5e_create_netdev(mdev, &mlx5e_nic_profile, rpriv);
 	if (!netdev) {
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rep.h b/drivers/net/ethernet/mellanox/mlx5/core/en_rep.h
index 23e43bbf928d..5659ed9f51e6 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_rep.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rep.h
@@ -38,6 +38,7 @@
 #include "eswitch.h"
 #include "en.h"
 
+#ifdef CONFIG_MLX5_ESWITCH
 struct mlx5e_neigh_update_table {
 	struct rhashtable       neigh_ht;
 	/* Save the neigh hash entries in a list in addition to the hash table
@@ -142,5 +143,12 @@ void mlx5e_rep_encap_entry_detach(struct mlx5e_priv *priv,
 				  struct mlx5e_encap_entry *e);
 
 void mlx5e_rep_queue_neigh_stats_work(struct mlx5e_priv *priv);
+#else /* CONFIG_MLX5_ESWITCH */
+static inline void mlx5e_register_vport_reps(struct mlx5e_priv *priv) {}
+static inline void mlx5e_unregister_vport_reps(struct mlx5e_priv *priv) {}
+static inline bool mlx5e_is_uplink_rep(struct mlx5e_priv *priv) { return false; }
+static inline int mlx5e_add_sqs_fwd_rules(struct mlx5e_priv *priv) { return 0; }
+static inline void mlx5e_remove_sqs_fwd_rules(struct mlx5e_priv *priv) {}
+#endif
 
 #endif /* __MLX5E_REP_H__ */
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
index 66b5fec15313..7d2860252dce 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
@@ -806,6 +806,7 @@ void mlx5e_handle_rx_cqe(struct mlx5e_rq *rq, struct mlx5_cqe64 *cqe)
 		       &wqe->next.next_wqe_index);
 }
 
+#ifdef CONFIG_MLX5_ESWITCH
 void mlx5e_handle_rx_cqe_rep(struct mlx5e_rq *rq, struct mlx5_cqe64 *cqe)
 {
 	struct net_device *netdev = rq->netdev;
@@ -838,6 +839,7 @@ void mlx5e_handle_rx_cqe_rep(struct mlx5e_rq *rq, struct mlx5_cqe64 *cqe)
 	mlx5_wq_ll_pop(&rq->wq, wqe_counter_be,
 		       &wqe->next.next_wqe_index);
 }
+#endif
 
 static inline void mlx5e_mpwqe_fill_rx_skb(struct mlx5e_rq *rq,
 					   struct mlx5_cqe64 *cqe,
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.h b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.h
index ecbe30d808ae..36473ec65ce8 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.h
@@ -35,6 +35,7 @@
 
 #define MLX5E_TC_FLOW_ID_MASK 0x0000ffff
 
+#ifdef CONFIG_MLX5_ESWITCH
 int mlx5e_tc_init(struct mlx5e_priv *priv);
 void mlx5e_tc_cleanup(struct mlx5e_priv *priv);
 
@@ -60,4 +61,10 @@ static inline int mlx5e_tc_num_filters(struct mlx5e_priv *priv)
 	return atomic_read(&priv->fs.tc.ht.nelems);
 }
 
+#else /* CONFIG_MLX5_ESWITCH */
+static inline int  mlx5e_tc_init(struct mlx5e_priv *priv) { return 0; }
+static inline void mlx5e_tc_cleanup(struct mlx5e_priv *priv) {}
+static inline int  mlx5e_tc_num_filters(struct mlx5e_priv *priv) { return 0; }
+#endif
+
 #endif /* __MLX5_EN_TC_H__ */
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
index a0203c7f6cf6..4806e81dec03 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
@@ -39,6 +39,14 @@
 #include <linux/mlx5/device.h>
 #include "mpfs.h"
 
+enum {
+	SRIOV_NONE,
+	SRIOV_LEGACY,
+	SRIOV_OFFLOADS
+};
+
+#ifdef CONFIG_MLX5_ESWITCH
+
 #define MLX5_MAX_UC_PER_VPORT(dev) \
 	(1 << MLX5_CAP_GEN(dev, log_max_current_uc_list))
 
@@ -125,12 +133,6 @@ struct mlx5_eswitch_fdb {
 	};
 };
 
-enum {
-	SRIOV_NONE,
-	SRIOV_LEGACY,
-	SRIOV_OFFLOADS
-};
-
 struct mlx5_esw_sq {
 	struct mlx5_flow_handle	*send_to_vport_rule;
 	struct list_head	 list;
@@ -291,4 +293,13 @@ int __mlx5_eswitch_set_vport_vlan(struct mlx5_eswitch *esw,
 
 #define esw_debug(dev, format, ...)				\
 	mlx5_core_dbg_mask(dev, MLX5_DEBUG_ESWITCH_MASK, format, ##__VA_ARGS__)
+#else  /* CONFIG_MLX5_ESWITCH */
+/* eswitch API stubs */
+static inline int  mlx5_eswitch_init(struct mlx5_core_dev *dev) { return 0; }
+static inline void mlx5_eswitch_cleanup(struct mlx5_eswitch *esw) {}
+static inline void mlx5_eswitch_vport_event(struct mlx5_eswitch *esw, struct mlx5_eqe *eqe) {}
+static inline int  mlx5_eswitch_enable_sriov(struct mlx5_eswitch *esw, int nvfs, int mode) { return -EOPNOTSUPP; }
+static inline void mlx5_eswitch_disable_sriov(struct mlx5_eswitch *esw) {}
+#endif /* CONFIG_MLX5_ESWITCH */
+
 #endif /* __MLX5_ESWITCH_H__ */
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/main.c b/drivers/net/ethernet/mellanox/mlx5/core/main.c
index 70db4257930e..8b78925f8850 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/main.c
@@ -54,9 +54,7 @@
 #include "mlx5_core.h"
 #include "fs_core.h"
 #include "mpfs.h"
-#ifdef CONFIG_MLX5_CORE_EN
 #include "eswitch.h"
-#endif
 #include "fpga/core.h"
 
 MODULE_AUTHOR("Eli Cohen <eli@mellanox.com>");
@@ -948,13 +946,11 @@ static int mlx5_init_once(struct mlx5_core_dev *dev, struct mlx5_priv *priv)
 		goto err_rl_cleanup;
 	}
 
-#ifdef CONFIG_MLX5_CORE_EN
 	err = mlx5_eswitch_init(dev);
 	if (err) {
 		dev_err(&pdev->dev, "Failed to init eswitch %d\n", err);
 		goto err_mpfs_cleanup;
 	}
-#endif
 
 	err = mlx5_sriov_init(dev);
 	if (err) {
@@ -965,10 +961,8 @@ static int mlx5_init_once(struct mlx5_core_dev *dev, struct mlx5_priv *priv)
 	return 0;
 
 err_eswitch_cleanup:
-#ifdef CONFIG_MLX5_CORE_EN
 	mlx5_eswitch_cleanup(dev->priv.eswitch);
 err_mpfs_cleanup:
-#endif
 	mlx5_mpfs_cleanup(dev);
 err_rl_cleanup:
 	mlx5_cleanup_rl_table(dev);
@@ -988,9 +982,7 @@ static int mlx5_init_once(struct mlx5_core_dev *dev, struct mlx5_priv *priv)
 static void mlx5_cleanup_once(struct mlx5_core_dev *dev)
 {
 	mlx5_sriov_cleanup(dev);
-#ifdef CONFIG_MLX5_CORE_EN
 	mlx5_eswitch_cleanup(dev->priv.eswitch);
-#endif
 	mlx5_mpfs_cleanup(dev);
 	mlx5_cleanup_rl_table(dev);
 	mlx5_cleanup_mkey_table(dev);
@@ -1283,7 +1275,7 @@ struct mlx5_core_event_handler {
 };
 
 static const struct devlink_ops mlx5_devlink_ops = {
-#ifdef CONFIG_MLX5_CORE_EN
+#ifdef CONFIG_MLX5_ESWITCH
 	.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.11.0

Re: [PATCH RFC net-next 4/4] net/mlx5: Add CONFIG_MLX5_ESWITCH Kconfig

[Or Gerlitz]

On Thu, Jun 8, 2017 at 2:42 AM, Saeed Mahameed <saeedm@mellanox.com> wrote:

> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> @@ -2961,9 +2961,8 @@ static int mlx5e_modify_channels_vsd(struct mlx5e_channels *chs, bool vsd)
>         return 0;
>  }
>
> -static int mlx5e_setup_tc(struct net_device *netdev, u8 tc)
> +static int mlx5e_setup_tc(struct mlx5e_priv *priv, u8 tc)
>  {
> -       struct mlx5e_priv *priv = netdev_priv(netdev);
>         struct mlx5e_channels new_channels = {};
>         int err = 0;
>
> @@ -2995,6 +2994,7 @@ static int mlx5e_ndo_setup_tc(struct net_device *dev, u32 handle,
>  {
>         struct mlx5e_priv *priv = netdev_priv(dev);
>
> +#ifdef CONFIG_MLX5_ESWITCH
>         if (TC_H_MAJ(handle) != TC_H_MAJ(TC_H_INGRESS))
>                 goto mqprio;
>
> @@ -3013,12 +3013,13 @@ static int mlx5e_ndo_setup_tc(struct net_device *dev, u32 handle,
>         }
>
>  mqprio:
> +#endif
>         if (tc->type != TC_SETUP_MQPRIO)
> -               return -EINVAL;
> +               return -EOPNOTSUPP;

why change this corner in this patch? we're doing enough changes

>
>         tc->mqprio->hw = TC_MQPRIO_HW_OFFLOAD_TCS;
>
> -       return mlx5e_setup_tc(dev, tc->mqprio->num_tc);
> +       return mlx5e_setup_tc(priv, tc->mqprio->num_tc);
>  }

same comment

> --- a/drivers/net/ethernet/mellanox/mlx5/core/main.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/main.c

> @@ -948,13 +946,11 @@ static int mlx5_init_once(struct mlx5_core_dev *dev, struct mlx5_priv *priv)
>                 goto err_rl_cleanup;
>         }
>
> -#ifdef CONFIG_MLX5_CORE_EN
>         err = mlx5_eswitch_init(dev);
>         if (err) {
>                 dev_err(&pdev->dev, "Failed to init eswitch %d\n", err);
>                 goto err_mpfs_cleanup;
>         }
> -#endif

why? before this patch we were doing it only if Ethernet
(MLX5_CORE_EN) was defined into the build,
and now we are returning blindly zero if another config isn't there
(CONFIG_MLX5_ESWITCH) - is that
fully equivalent? do we want to be fully equiv?

> @@ -965,10 +961,8 @@ static int mlx5_init_once(struct mlx5_core_dev *dev, struct mlx5_priv *priv)
>         return 0;
>
>  err_eswitch_cleanup:
> -#ifdef CONFIG_MLX5_CORE_EN
>         mlx5_eswitch_cleanup(dev->priv.eswitch);
>  err_mpfs_cleanup:
> -#endif

same comment/question

Re: [PATCH RFC net-next 4/4] net/mlx5: Add CONFIG_MLX5_ESWITCH Kconfig

[Saeed Mahameed]

On Thu, Jun 8, 2017 at 12:35 PM, Or Gerlitz <gerlitz.or@gmail.com> wrote:
> On Thu, Jun 8, 2017 at 2:42 AM, Saeed Mahameed <saeedm@mellanox.com> wrote:
>
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>> @@ -2961,9 +2961,8 @@ static int mlx5e_modify_channels_vsd(struct mlx5e_channels *chs, bool vsd)
>>         return 0;
>>  }
>>
>> -static int mlx5e_setup_tc(struct net_device *netdev, u8 tc)
>> +static int mlx5e_setup_tc(struct mlx5e_priv *priv, u8 tc)
>>  {
>> -       struct mlx5e_priv *priv = netdev_priv(netdev);
>>         struct mlx5e_channels new_channels = {};
>>         int err = 0;
>>
>> @@ -2995,6 +2994,7 @@ static int mlx5e_ndo_setup_tc(struct net_device *dev, u32 handle,
>>  {
>>         struct mlx5e_priv *priv = netdev_priv(dev);
>>
>> +#ifdef CONFIG_MLX5_ESWITCH
>>         if (TC_H_MAJ(handle) != TC_H_MAJ(TC_H_INGRESS))
>>                 goto mqprio;
>>
>> @@ -3013,12 +3013,13 @@ static int mlx5e_ndo_setup_tc(struct net_device *dev, u32 handle,
>>         }
>>
>>  mqprio:
>> +#endif
>>         if (tc->type != TC_SETUP_MQPRIO)
>> -               return -EINVAL;
>> +               return -EOPNOTSUPP;
>
> why change this corner in this patch? we're doing enough changes
>
>>
>>         tc->mqprio->hw = TC_MQPRIO_HW_OFFLOAD_TCS;
>>
>> -       return mlx5e_setup_tc(dev, tc->mqprio->num_tc);
>> +       return mlx5e_setup_tc(priv, tc->mqprio->num_tc);
>>  }
>
> same comment
>

Ok will change both.

>> --- a/drivers/net/ethernet/mellanox/mlx5/core/main.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/main.c
>
>> @@ -948,13 +946,11 @@ static int mlx5_init_once(struct mlx5_core_dev *dev, struct mlx5_priv *priv)
>>                 goto err_rl_cleanup;
>>         }
>>
>> -#ifdef CONFIG_MLX5_CORE_EN
>>         err = mlx5_eswitch_init(dev);
>>         if (err) {
>>                 dev_err(&pdev->dev, "Failed to init eswitch %d\n", err);
>>                 goto err_mpfs_cleanup;
>>         }
>> -#endif
>
> why? before this patch we were doing it only if Ethernet
> (MLX5_CORE_EN) was defined into the build,
> and now we are returning blindly zero if another config isn't there
> (CONFIG_MLX5_ESWITCH) - is that
> fully equivalent? do we want to be fully equiv?
>

Fully equivalent and more correct behavior and design.
eswitch is now separate from MPFS and EN and fully independent and has
its own CONFIG_MLX5_ESWITCH,
you can choose to have it or not (CONFIG_MLX5_ESWITCH) it won't
affects other components.

>> @@ -965,10 +961,8 @@ static int mlx5_init_once(struct mlx5_core_dev *dev, struct mlx5_priv *priv)
>>         return 0;
>>
>>  err_eswitch_cleanup:
>> -#ifdef CONFIG_MLX5_CORE_EN
>>         mlx5_eswitch_cleanup(dev->priv.eswitch);
>>  err_mpfs_cleanup:
>> -#endif
>
> same comment/question

Same answer.

Re: [PATCH RFC net-next 4/4] net/mlx5: Add CONFIG_MLX5_ESWITCH Kconfig

[Jes Sorensen]

On 06/07/2017 07:42 PM, Saeed Mahameed wrote:
> This patch gives the option to chose whether to compile the driver with or
> without eswitch/eswitch_offloads(switchdev mode)/en_rep(VF representors)
> and en_tc offloads.
> 
> It also removes most of the above modules headers declarations and stubs
> out the API functions which are used outside these modules.
> 
> Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
> ---
>   drivers/net/ethernet/mellanox/mlx5/core/Kconfig   |  7 +++++
>   drivers/net/ethernet/mellanox/mlx5/core/Makefile  |  6 +++--
>   drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 33 +++++++++++++++--------
>   drivers/net/ethernet/mellanox/mlx5/core/en_rep.h  |  8 ++++++
>   drivers/net/ethernet/mellanox/mlx5/core/en_rx.c   |  2 ++
>   drivers/net/ethernet/mellanox/mlx5/core/en_tc.h   |  7 +++++
>   drivers/net/ethernet/mellanox/mlx5/core/eswitch.h | 23 +++++++++++-----
>   drivers/net/ethernet/mellanox/mlx5/core/main.c    | 10 +------
>   8 files changed, 68 insertions(+), 28 deletions(-)

Overall good, a few nits

> @@ -3316,6 +3317,7 @@ static int mlx5e_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
>   	}
>   }
>   
> +#ifdef CONFIG_MLX5_ESWITCH
>   static int mlx5e_set_vf_mac(struct net_device *dev, int vf, u8 *mac)
>   {
>   	struct mlx5e_priv *priv = netdev_priv(dev);
> @@ -3418,6 +3420,7 @@ static int mlx5e_get_vf_stats(struct net_device *dev,
>   	return mlx5_eswitch_get_vport_stats(mdev->priv.eswitch, vf + 1,
>   					    vf_stats);
>   }
> +#endif
>   
>   static void mlx5e_add_vxlan_port(struct net_device *netdev,
>   				 struct udp_tunnel_info *ti)
> @@ -3659,6 +3662,7 @@ static const struct net_device_ops mlx5e_netdev_ops_basic = {
>   #endif
>   };
>   
> +#ifdef CONFIG_MLX5_ESWITCH
>   static const struct net_device_ops mlx5e_netdev_ops_sriov = {
>   	.ndo_open                = mlx5e_open,
>   	.ndo_stop                = mlx5e_close,
> @@ -3697,6 +3701,7 @@ static const struct net_device_ops mlx5e_netdev_ops_sriov = {
>   	.ndo_has_offload_stats	 = mlx5e_has_offload_stats,
>   	.ndo_get_offload_stats	 = mlx5e_get_offload_stats,
>   };
> +#endif
>   
>   static int mlx5e_check_required_hca_cap(struct mlx5_core_dev *mdev)
>   {
> @@ -3923,9 +3928,11 @@ static void mlx5e_set_netdev_dev_addr(struct net_device *netdev)
>   	}
>   }
>   
> +#if IS_ENABLED(CONFIG_NET_SWITCHDEV) && IS_ENABLED(CONFIG_MLX5_ESWITCH)
>   static const struct switchdev_ops mlx5e_switchdev_ops = {
>   	.switchdev_port_attr_get	= mlx5e_attr_get,
>   };
> +#endif
>   
>   static void mlx5e_build_nic_netdev(struct net_device *netdev)
>   {

Why not move these functions and the struct into one of the files that 
is being compiled out. The less #ifdefs we leave in the code the better.

> @@ -3936,15 +3943,17 @@ static void mlx5e_build_nic_netdev(struct net_device *netdev)
>   
>   	SET_NETDEV_DEV(netdev, &mdev->pdev->dev);
>   
> -	if (MLX5_CAP_GEN(mdev, vport_group_manager)) {
> -		netdev->netdev_ops = &mlx5e_netdev_ops_sriov;
>   #ifdef CONFIG_MLX5_CORE_EN_DCB
> -		if (MLX5_CAP_GEN(mdev, qos))
> -			netdev->dcbnl_ops = &mlx5e_dcbnl_ops;
> +	if (MLX5_CAP_GEN(mdev, vport_group_manager) && MLX5_CAP_GEN(mdev, qos))
> +		netdev->dcbnl_ops = &mlx5e_dcbnl_ops;
> +#endif
> +
> +#ifdef CONFIG_MLX5_ESWITCH
> +	if (MLX5_CAP_GEN(mdev, vport_group_manager))
> +		netdev->netdev_ops = &mlx5e_netdev_ops_sriov;
> +	else
>   #endif
> -	} else {
>   		netdev->netdev_ops = &mlx5e_netdev_ops_basic;
> -	}
>   
>   	netdev->watchdog_timeo    = 15 * HZ;

This kind of #ifdef is always bad, it's hard to read and easy to get 
wrong. Why not have MLX5_CAP_GEN return 0 if MLX5_ESWITCH is not enabled 
and have a dummy pointer?

> @@ -4016,7 +4025,7 @@ static void mlx5e_build_nic_netdev(struct net_device *netdev)
>   
>   	mlx5e_set_netdev_dev_addr(netdev);
>   
> -#ifdef CONFIG_NET_SWITCHDEV
> +#if IS_ENABLED(CONFIG_NET_SWITCHDEV) && IS_ENABLED(CONFIG_MLX5_ESWITCH)
>   	if (MLX5_CAP_GEN(mdev, vport_group_manager))
>   		netdev->switchdev_ops = &mlx5e_switchdev_ops;
>   #endif

Can MLX5_ESWITCH be enabled without NET_SWITCHDEV?

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> index 66b5fec15313..7d2860252dce 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> @@ -806,6 +806,7 @@ void mlx5e_handle_rx_cqe(struct mlx5e_rq *rq, struct mlx5_cqe64 *cqe)
>   		       &wqe->next.next_wqe_index);
>   }
>   
> +#ifdef CONFIG_MLX5_ESWITCH
>   void mlx5e_handle_rx_cqe_rep(struct mlx5e_rq *rq, struct mlx5_cqe64 *cqe)
>   {
>   	struct net_device *netdev = rq->netdev;
> @@ -838,6 +839,7 @@ void mlx5e_handle_rx_cqe_rep(struct mlx5e_rq *rq, struct mlx5_cqe64 *cqe)
>   	mlx5_wq_ll_pop(&rq->wq, wqe_counter_be,
>   		       &wqe->next.next_wqe_index);
>   }
> +#endif
>   
>   static inline void mlx5e_mpwqe_fill_rx_skb(struct mlx5e_rq *rq,
>   					   struct mlx5_cqe64 *cqe,

Another case of moving it to one of the disabled files.

Cheers,
Jes

Re: [PATCH RFC net-next 4/4] net/mlx5: Add CONFIG_MLX5_ESWITCH Kconfig

[Saeed Mahameed]

On Mon, Jun 12, 2017 at 9:20 PM, Jes Sorensen <jsorensen@fb.com> wrote:
> On 06/07/2017 07:42 PM, Saeed Mahameed wrote:
>>
>> This patch gives the option to chose whether to compile the driver with or
>> without eswitch/eswitch_offloads(switchdev mode)/en_rep(VF representors)
>> and en_tc offloads.
>>
>> It also removes most of the above modules headers declarations and stubs
>> out the API functions which are used outside these modules.
>>
>> Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
>> ---
>>   drivers/net/ethernet/mellanox/mlx5/core/Kconfig   |  7 +++++
>>   drivers/net/ethernet/mellanox/mlx5/core/Makefile  |  6 +++--
>>   drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 33
>> +++++++++++++++--------
>>   drivers/net/ethernet/mellanox/mlx5/core/en_rep.h  |  8 ++++++
>>   drivers/net/ethernet/mellanox/mlx5/core/en_rx.c   |  2 ++
>>   drivers/net/ethernet/mellanox/mlx5/core/en_tc.h   |  7 +++++
>>   drivers/net/ethernet/mellanox/mlx5/core/eswitch.h | 23 +++++++++++-----
>>   drivers/net/ethernet/mellanox/mlx5/core/main.c    | 10 +------
>>   8 files changed, 68 insertions(+), 28 deletions(-)
>
>
> Overall good, a few nits
>
>
>> @@ -3316,6 +3317,7 @@ static int mlx5e_ioctl(struct net_device *dev,
>> struct ifreq *ifr, int cmd)
>>         }
>>   }
>>   +#ifdef CONFIG_MLX5_ESWITCH
>>   static int mlx5e_set_vf_mac(struct net_device *dev, int vf, u8 *mac)
>>   {
>>         struct mlx5e_priv *priv = netdev_priv(dev);
>> @@ -3418,6 +3420,7 @@ static int mlx5e_get_vf_stats(struct net_device
>> *dev,
>>         return mlx5_eswitch_get_vport_stats(mdev->priv.eswitch, vf + 1,
>>                                             vf_stats);
>>   }
>> +#endif
>>     static void mlx5e_add_vxlan_port(struct net_device *netdev,
>>                                  struct udp_tunnel_info *ti)
>> @@ -3659,6 +3662,7 @@ static const struct net_device_ops
>> mlx5e_netdev_ops_basic = {
>>   #endif
>>   };
>>   +#ifdef CONFIG_MLX5_ESWITCH
>>   static const struct net_device_ops mlx5e_netdev_ops_sriov = {
>>         .ndo_open                = mlx5e_open,
>>         .ndo_stop                = mlx5e_close,
>> @@ -3697,6 +3701,7 @@ static const struct net_device_ops
>> mlx5e_netdev_ops_sriov = {
>>         .ndo_has_offload_stats   = mlx5e_has_offload_stats,
>>         .ndo_get_offload_stats   = mlx5e_get_offload_stats,
>>   };
>> +#endif
>>     static int mlx5e_check_required_hca_cap(struct mlx5_core_dev *mdev)
>>   {
>> @@ -3923,9 +3928,11 @@ static void mlx5e_set_netdev_dev_addr(struct
>> net_device *netdev)
>>         }
>>   }
>>   +#if IS_ENABLED(CONFIG_NET_SWITCHDEV) && IS_ENABLED(CONFIG_MLX5_ESWITCH)
>>   static const struct switchdev_ops mlx5e_switchdev_ops = {
>>         .switchdev_port_attr_get        = mlx5e_attr_get,
>>   };
>> +#endif
>>     static void mlx5e_build_nic_netdev(struct net_device *netdev)
>>   {
>
>
> Why not move these functions and the struct into one of the files that is
> being compiled out. The less #ifdefs we leave in the code the better.
>

eswitch is independent from netdev, and we want to keep netdev ndos
local to netdev files.

>> @@ -3936,15 +3943,17 @@ static void mlx5e_build_nic_netdev(struct
>> net_device *netdev)
>>         SET_NETDEV_DEV(netdev, &mdev->pdev->dev);
>>   -     if (MLX5_CAP_GEN(mdev, vport_group_manager)) {
>> -               netdev->netdev_ops = &mlx5e_netdev_ops_sriov;
>>   #ifdef CONFIG_MLX5_CORE_EN_DCB
>> -               if (MLX5_CAP_GEN(mdev, qos))
>> -                       netdev->dcbnl_ops = &mlx5e_dcbnl_ops;
>> +       if (MLX5_CAP_GEN(mdev, vport_group_manager) && MLX5_CAP_GEN(mdev,
>> qos))
>> +               netdev->dcbnl_ops = &mlx5e_dcbnl_ops;
>> +#endif
>> +
>> +#ifdef CONFIG_MLX5_ESWITCH
>> +       if (MLX5_CAP_GEN(mdev, vport_group_manager))
>> +               netdev->netdev_ops = &mlx5e_netdev_ops_sriov;
>> +       else
>>   #endif
>> -       } else {
>>                 netdev->netdev_ops = &mlx5e_netdev_ops_basic;
>> -       }
>>         netdev->watchdog_timeo    = 15 * HZ;
>
>
> This kind of #ifdef is always bad, it's hard to read and easy to get wrong.
> Why not have MLX5_CAP_GEN return 0 if MLX5_ESWITCH is not enabled and have a
> dummy pointer?
>

i know ifdef is ugly, but we have to provide basic ndos (not dummy
pointers) when eswitch is not avaialbe, also we want the code to be as
much as free from empty functions that all they do is to return
-EOPNOTSUPP;

for my taste this way is cleaner and more readable, from these line
you can understand when SRIOV/Eswitch is not supported. you don't need
to backtrack the function pointers.

>> @@ -4016,7 +4025,7 @@ static void mlx5e_build_nic_netdev(struct net_device
>> *netdev)
>>         mlx5e_set_netdev_dev_addr(netdev);
>>   -#ifdef CONFIG_NET_SWITCHDEV
>> +#if IS_ENABLED(CONFIG_NET_SWITCHDEV) && IS_ENABLED(CONFIG_MLX5_ESWITCH)
>>         if (MLX5_CAP_GEN(mdev, vport_group_manager))
>>                 netdev->switchdev_ops = &mlx5e_switchdev_ops;
>>   #endif
>
>
> Can MLX5_ESWITCH be enabled without NET_SWITCHDEV?
>

Yes (Legacy SRIOV mode), but not the other way around.

>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
>> b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
>> index 66b5fec15313..7d2860252dce 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
>> @@ -806,6 +806,7 @@ void mlx5e_handle_rx_cqe(struct mlx5e_rq *rq, struct
>> mlx5_cqe64 *cqe)
>>                        &wqe->next.next_wqe_index);
>>   }
>>   +#ifdef CONFIG_MLX5_ESWITCH
>>   void mlx5e_handle_rx_cqe_rep(struct mlx5e_rq *rq, struct mlx5_cqe64
>> *cqe)
>>   {
>>         struct net_device *netdev = rq->netdev;
>> @@ -838,6 +839,7 @@ void mlx5e_handle_rx_cqe_rep(struct mlx5e_rq *rq,
>> struct mlx5_cqe64 *cqe)
>>         mlx5_wq_ll_pop(&rq->wq, wqe_counter_be,
>>                        &wqe->next.next_wqe_index);
>>   }
>> +#endif
>>     static inline void mlx5e_mpwqe_fill_rx_skb(struct mlx5e_rq *rq,
>>                                            struct mlx5_cqe64 *cqe,
>
>
> Another case of moving it to one of the disabled files.
>

this means that we need to export some data path RX helper functions,
also we would like to keep all RX path local to en_rx.c file for
performance considerations, and this is the price we have to pay.

> Cheers,
> Jes

Re: [PATCH RFC net-next 4/4] net/mlx5: Add CONFIG_MLX5_ESWITCH Kconfig

[Jes Sorensen]

On 06/13/2017 01:58 PM, Saeed Mahameed wrote:
> On Mon, Jun 12, 2017 at 9:20 PM, Jes Sorensen <jsorensen@fb.com> wrote:
>> On 06/07/2017 07:42 PM, Saeed Mahameed wrote:
>>>
>>> This patch gives the option to chose whether to compile the driver with or
>>> without eswitch/eswitch_offloads(switchdev mode)/en_rep(VF representors)
>>> and en_tc offloads.
>>>
>>> It also removes most of the above modules headers declarations and stubs
>>> out the API functions which are used outside these modules.
>>>
>>> Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
>>> ---
>>>    drivers/net/ethernet/mellanox/mlx5/core/Kconfig   |  7 +++++
>>>    drivers/net/ethernet/mellanox/mlx5/core/Makefile  |  6 +++--
>>>    drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 33
>>> +++++++++++++++--------
>>>    drivers/net/ethernet/mellanox/mlx5/core/en_rep.h  |  8 ++++++
>>>    drivers/net/ethernet/mellanox/mlx5/core/en_rx.c   |  2 ++
>>>    drivers/net/ethernet/mellanox/mlx5/core/en_tc.h   |  7 +++++
>>>    drivers/net/ethernet/mellanox/mlx5/core/eswitch.h | 23 +++++++++++-----
>>>    drivers/net/ethernet/mellanox/mlx5/core/main.c    | 10 +------
>>>    8 files changed, 68 insertions(+), 28 deletions(-)
>>
>>
>> Overall good, a few nits
>>
>>
>>> @@ -3316,6 +3317,7 @@ static int mlx5e_ioctl(struct net_device *dev,
>>> struct ifreq *ifr, int cmd)
>>>          }
>>>    }
>>>    +#ifdef CONFIG_MLX5_ESWITCH
>>>    static int mlx5e_set_vf_mac(struct net_device *dev, int vf, u8 *mac)
>>>    {
>>>          struct mlx5e_priv *priv = netdev_priv(dev);
>>> @@ -3418,6 +3420,7 @@ static int mlx5e_get_vf_stats(struct net_device
>>> *dev,
>>>          return mlx5_eswitch_get_vport_stats(mdev->priv.eswitch, vf + 1,
>>>                                              vf_stats);
>>>    }
>>> +#endif
>>>      static void mlx5e_add_vxlan_port(struct net_device *netdev,
>>>                                   struct udp_tunnel_info *ti)
>>> @@ -3659,6 +3662,7 @@ static const struct net_device_ops
>>> mlx5e_netdev_ops_basic = {
>>>    #endif
>>>    };
>>>    +#ifdef CONFIG_MLX5_ESWITCH
>>>    static const struct net_device_ops mlx5e_netdev_ops_sriov = {
>>>          .ndo_open                = mlx5e_open,
>>>          .ndo_stop                = mlx5e_close,
>>> @@ -3697,6 +3701,7 @@ static const struct net_device_ops
>>> mlx5e_netdev_ops_sriov = {
>>>          .ndo_has_offload_stats   = mlx5e_has_offload_stats,
>>>          .ndo_get_offload_stats   = mlx5e_get_offload_stats,
>>>    };
>>> +#endif
>>>      static int mlx5e_check_required_hca_cap(struct mlx5_core_dev *mdev)
>>>    {
>>> @@ -3923,9 +3928,11 @@ static void mlx5e_set_netdev_dev_addr(struct
>>> net_device *netdev)
>>>          }
>>>    }
>>>    +#if IS_ENABLED(CONFIG_NET_SWITCHDEV) && IS_ENABLED(CONFIG_MLX5_ESWITCH)
>>>    static const struct switchdev_ops mlx5e_switchdev_ops = {
>>>          .switchdev_port_attr_get        = mlx5e_attr_get,
>>>    };
>>> +#endif
>>>      static void mlx5e_build_nic_netdev(struct net_device *netdev)
>>>    {
>>
>>
>> Why not move these functions and the struct into one of the files that is
>> being compiled out. The less #ifdefs we leave in the code the better.
>>
> 
> eswitch is independent from netdev, and we want to keep netdev ndos
> local to netdev files.

Not the end of the World then.

>>> @@ -3936,15 +3943,17 @@ static void mlx5e_build_nic_netdev(struct
>>> net_device *netdev)
>>>          SET_NETDEV_DEV(netdev, &mdev->pdev->dev);
>>>    -     if (MLX5_CAP_GEN(mdev, vport_group_manager)) {
>>> -               netdev->netdev_ops = &mlx5e_netdev_ops_sriov;
>>>    #ifdef CONFIG_MLX5_CORE_EN_DCB
>>> -               if (MLX5_CAP_GEN(mdev, qos))
>>> -                       netdev->dcbnl_ops = &mlx5e_dcbnl_ops;
>>> +       if (MLX5_CAP_GEN(mdev, vport_group_manager) && MLX5_CAP_GEN(mdev,
>>> qos))
>>> +               netdev->dcbnl_ops = &mlx5e_dcbnl_ops;
>>> +#endif
>>> +
>>> +#ifdef CONFIG_MLX5_ESWITCH
>>> +       if (MLX5_CAP_GEN(mdev, vport_group_manager))
>>> +               netdev->netdev_ops = &mlx5e_netdev_ops_sriov;
>>> +       else
>>>    #endif
>>> -       } else {
>>>                  netdev->netdev_ops = &mlx5e_netdev_ops_basic;
>>> -       }
>>>          netdev->watchdog_timeo    = 15 * HZ;
>>
>>
>> This kind of #ifdef is always bad, it's hard to read and easy to get wrong.
>> Why not have MLX5_CAP_GEN return 0 if MLX5_ESWITCH is not enabled and have a
>> dummy pointer?
>>
> 
> i know ifdef is ugly, but we have to provide basic ndos (not dummy
> pointers) when eswitch is not avaialbe, also we want the code to be as
> much as free from empty functions that all they do is to return
> -EOPNOTSUPP;
> 
> for my taste this way is cleaner and more readable, from these line
> you can understand when SRIOV/Eswitch is not supported. you don't need
> to backtrack the function pointers.

This is not a good way of doing it, with #ifdef's mangling an if() 
statement. It really should be avoided. Any problem having 
MLX5_CAP_GEN() return 0 when ESWITCH is not enabled?

>>> @@ -4016,7 +4025,7 @@ static void mlx5e_build_nic_netdev(struct net_device
>>> *netdev)
>>>          mlx5e_set_netdev_dev_addr(netdev);
>>>    -#ifdef CONFIG_NET_SWITCHDEV
>>> +#if IS_ENABLED(CONFIG_NET_SWITCHDEV) && IS_ENABLED(CONFIG_MLX5_ESWITCH)
>>>          if (MLX5_CAP_GEN(mdev, vport_group_manager))
>>>                  netdev->switchdev_ops = &mlx5e_switchdev_ops;
>>>    #endif
>>
>>
>> Can MLX5_ESWITCH be enabled without NET_SWITCHDEV?
>>
> 
> Yes (Legacy SRIOV mode), but not the other way around.
> 
>>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
>>> b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
>>> index 66b5fec15313..7d2860252dce 100644
>>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
>>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
>>> @@ -806,6 +806,7 @@ void mlx5e_handle_rx_cqe(struct mlx5e_rq *rq, struct
>>> mlx5_cqe64 *cqe)
>>>                         &wqe->next.next_wqe_index);
>>>    }
>>>    +#ifdef CONFIG_MLX5_ESWITCH
>>>    void mlx5e_handle_rx_cqe_rep(struct mlx5e_rq *rq, struct mlx5_cqe64
>>> *cqe)
>>>    {
>>>          struct net_device *netdev = rq->netdev;
>>> @@ -838,6 +839,7 @@ void mlx5e_handle_rx_cqe_rep(struct mlx5e_rq *rq,
>>> struct mlx5_cqe64 *cqe)
>>>          mlx5_wq_ll_pop(&rq->wq, wqe_counter_be,
>>>                         &wqe->next.next_wqe_index);
>>>    }
>>> +#endif
>>>      static inline void mlx5e_mpwqe_fill_rx_skb(struct mlx5e_rq *rq,
>>>                                             struct mlx5_cqe64 *cqe,
>>
>>
>> Another case of moving it to one of the disabled files.
>>
> 
> this means that we need to export some data path RX helper functions,
> also we would like to keep all RX path local to en_rx.c file for
> performance considerations, and this is the price we have to pay.

The linker takes care of this - if you want them inlined, you should 
explicitly inline them. Otherwise there is no cost here.

Please move them over in this case.

Jes

Re: [PATCH RFC net-next 4/4] net/mlx5: Add CONFIG_MLX5_ESWITCH Kconfig

[Saeed Mahameed]

On Tue, Jun 13, 2017 at 9:21 PM, Jes Sorensen <jsorensen@fb.com> wrote:
> On 06/13/2017 01:58 PM, Saeed Mahameed wrote:
>>
>> On Mon, Jun 12, 2017 at 9:20 PM, Jes Sorensen <jsorensen@fb.com> wrote:
>>>
>>> On 06/07/2017 07:42 PM, Saeed Mahameed wrote:
>>>>
>>>>
>>>> This patch gives the option to chose whether to compile the driver with
>>>> or
>>>> without eswitch/eswitch_offloads(switchdev mode)/en_rep(VF representors)
>>>> and en_tc offloads.
>>>>
>>>> It also removes most of the above modules headers declarations and stubs
>>>> out the API functions which are used outside these modules.
>>>>
>>>> Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
>>>> ---
>>>>    drivers/net/ethernet/mellanox/mlx5/core/Kconfig   |  7 +++++
>>>>    drivers/net/ethernet/mellanox/mlx5/core/Makefile  |  6 +++--
>>>>    drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 33
>>>> +++++++++++++++--------
>>>>    drivers/net/ethernet/mellanox/mlx5/core/en_rep.h  |  8 ++++++
>>>>    drivers/net/ethernet/mellanox/mlx5/core/en_rx.c   |  2 ++
>>>>    drivers/net/ethernet/mellanox/mlx5/core/en_tc.h   |  7 +++++
>>>>    drivers/net/ethernet/mellanox/mlx5/core/eswitch.h | 23
>>>> +++++++++++-----
>>>>    drivers/net/ethernet/mellanox/mlx5/core/main.c    | 10 +------
>>>>    8 files changed, 68 insertions(+), 28 deletions(-)
>>>
>>>
>>>
>>> Overall good, a few nits
>>>
>>>
>>>> @@ -3316,6 +3317,7 @@ static int mlx5e_ioctl(struct net_device *dev,
>>>> struct ifreq *ifr, int cmd)
>>>>          }
>>>>    }
>>>>    +#ifdef CONFIG_MLX5_ESWITCH
>>>>    static int mlx5e_set_vf_mac(struct net_device *dev, int vf, u8 *mac)
>>>>    {
>>>>          struct mlx5e_priv *priv = netdev_priv(dev);
>>>> @@ -3418,6 +3420,7 @@ static int mlx5e_get_vf_stats(struct net_device
>>>> *dev,
>>>>          return mlx5_eswitch_get_vport_stats(mdev->priv.eswitch, vf + 1,
>>>>                                              vf_stats);
>>>>    }
>>>> +#endif
>>>>      static void mlx5e_add_vxlan_port(struct net_device *netdev,
>>>>                                   struct udp_tunnel_info *ti)
>>>> @@ -3659,6 +3662,7 @@ static const struct net_device_ops
>>>> mlx5e_netdev_ops_basic = {
>>>>    #endif
>>>>    };
>>>>    +#ifdef CONFIG_MLX5_ESWITCH
>>>>    static const struct net_device_ops mlx5e_netdev_ops_sriov = {
>>>>          .ndo_open                = mlx5e_open,
>>>>          .ndo_stop                = mlx5e_close,
>>>> @@ -3697,6 +3701,7 @@ static const struct net_device_ops
>>>> mlx5e_netdev_ops_sriov = {
>>>>          .ndo_has_offload_stats   = mlx5e_has_offload_stats,
>>>>          .ndo_get_offload_stats   = mlx5e_get_offload_stats,
>>>>    };
>>>> +#endif
>>>>      static int mlx5e_check_required_hca_cap(struct mlx5_core_dev *mdev)
>>>>    {
>>>> @@ -3923,9 +3928,11 @@ static void mlx5e_set_netdev_dev_addr(struct
>>>> net_device *netdev)
>>>>          }
>>>>    }
>>>>    +#if IS_ENABLED(CONFIG_NET_SWITCHDEV) &&
>>>> IS_ENABLED(CONFIG_MLX5_ESWITCH)
>>>>    static const struct switchdev_ops mlx5e_switchdev_ops = {
>>>>          .switchdev_port_attr_get        = mlx5e_attr_get,
>>>>    };
>>>> +#endif
>>>>      static void mlx5e_build_nic_netdev(struct net_device *netdev)
>>>>    {
>>>
>>>
>>>
>>> Why not move these functions and the struct into one of the files that is
>>> being compiled out. The less #ifdefs we leave in the code the better.
>>>
>>
>> eswitch is independent from netdev, and we want to keep netdev ndos
>> local to netdev files.
>
>
> Not the end of the World then.
>
>
>>>> @@ -3936,15 +3943,17 @@ static void mlx5e_build_nic_netdev(struct
>>>> net_device *netdev)
>>>>          SET_NETDEV_DEV(netdev, &mdev->pdev->dev);
>>>>    -     if (MLX5_CAP_GEN(mdev, vport_group_manager)) {
>>>> -               netdev->netdev_ops = &mlx5e_netdev_ops_sriov;
>>>>    #ifdef CONFIG_MLX5_CORE_EN_DCB
>>>> -               if (MLX5_CAP_GEN(mdev, qos))
>>>> -                       netdev->dcbnl_ops = &mlx5e_dcbnl_ops;
>>>> +       if (MLX5_CAP_GEN(mdev, vport_group_manager) &&
>>>> MLX5_CAP_GEN(mdev,
>>>> qos))
>>>> +               netdev->dcbnl_ops = &mlx5e_dcbnl_ops;
>>>> +#endif
>>>> +
>>>> +#ifdef CONFIG_MLX5_ESWITCH
>>>> +       if (MLX5_CAP_GEN(mdev, vport_group_manager))
>>>> +               netdev->netdev_ops = &mlx5e_netdev_ops_sriov;
>>>> +       else
>>>>    #endif
>>>> -       } else {
>>>>                  netdev->netdev_ops = &mlx5e_netdev_ops_basic;
>>>> -       }
>>>>          netdev->watchdog_timeo    = 15 * HZ;
>>>
>>>
>>>
>>> This kind of #ifdef is always bad, it's hard to read and easy to get
>>> wrong.
>>> Why not have MLX5_CAP_GEN return 0 if MLX5_ESWITCH is not enabled and
>>> have a
>>> dummy pointer?
>>>
>>
>> i know ifdef is ugly, but we have to provide basic ndos (not dummy
>> pointers) when eswitch is not avaialbe, also we want the code to be as
>> much as free from empty functions that all they do is to return
>> -EOPNOTSUPP;
>>
>> for my taste this way is cleaner and more readable, from these line
>> you can understand when SRIOV/Eswitch is not supported. you don't need
>> to backtrack the function pointers.
>
>
> This is not a good way of doing it, with #ifdef's mangling an if()
> statement. It really should be avoided. Any problem having MLX5_CAP_GEN()
> return 0 when ESWITCH is not enabled?
>

if you inset i can completely remove the need of having two ndo
structs and stub out the missing eswitch API functions needed by the
sriov ndos.

MLX5_CAP_GEN has a very clear purpose to report actual device
capabilities, regardless of what driver want to see. if you don't want
eswitch, you shouldn't even think of testing eswitch capabilities :).

>
>>>> @@ -4016,7 +4025,7 @@ static void mlx5e_build_nic_netdev(struct
>>>> net_device
>>>> *netdev)
>>>>          mlx5e_set_netdev_dev_addr(netdev);
>>>>    -#ifdef CONFIG_NET_SWITCHDEV
>>>> +#if IS_ENABLED(CONFIG_NET_SWITCHDEV) && IS_ENABLED(CONFIG_MLX5_ESWITCH)
>>>>          if (MLX5_CAP_GEN(mdev, vport_group_manager))
>>>>                  netdev->switchdev_ops = &mlx5e_switchdev_ops;
>>>>    #endif
>>>
>>>
>>>
>>> Can MLX5_ESWITCH be enabled without NET_SWITCHDEV?
>>>
>>
>> Yes (Legacy SRIOV mode), but not the other way around.
>>
>>>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
>>>> b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
>>>> index 66b5fec15313..7d2860252dce 100644
>>>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
>>>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
>>>> @@ -806,6 +806,7 @@ void mlx5e_handle_rx_cqe(struct mlx5e_rq *rq, struct
>>>> mlx5_cqe64 *cqe)
>>>>                         &wqe->next.next_wqe_index);
>>>>    }
>>>>    +#ifdef CONFIG_MLX5_ESWITCH
>>>>    void mlx5e_handle_rx_cqe_rep(struct mlx5e_rq *rq, struct mlx5_cqe64
>>>> *cqe)
>>>>    {
>>>>          struct net_device *netdev = rq->netdev;
>>>> @@ -838,6 +839,7 @@ void mlx5e_handle_rx_cqe_rep(struct mlx5e_rq *rq,
>>>> struct mlx5_cqe64 *cqe)
>>>>          mlx5_wq_ll_pop(&rq->wq, wqe_counter_be,
>>>>                         &wqe->next.next_wqe_index);
>>>>    }
>>>> +#endif
>>>>      static inline void mlx5e_mpwqe_fill_rx_skb(struct mlx5e_rq *rq,
>>>>                                             struct mlx5_cqe64 *cqe,
>>>
>>>
>>>
>>> Another case of moving it to one of the disabled files.
>>>
>>
>> this means that we need to export some data path RX helper functions,
>> also we would like to keep all RX path local to en_rx.c file for
>> performance considerations, and this is the price we have to pay.
>
>
> The linker takes care of this - if you want them inlined, you should
> explicitly inline them. Otherwise there is no cost here.
>
> Please move them over in this case.
>

we are talking about a big chunk of en_rx file, i don't feel
comfortable disrupting this file structure at the moment, or even do
any small structural change just to avoid one #ifdef

let's keep it as is for now.

> Jes
>