On 06/13/2017 01:58 PM, Saeed Mahameed wrote:
On Mon, Jun 12, 2017 at 9:20 PM, Jes Sorensen <jsoren...@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 <sae...@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
