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

2017-06-13 Thread Saeed Mahameed 
On Tue, Jun 13, 2017 at 9:21 PM, Jes Sorensen  wrote:
> On 06/13/2017 01:58 PM, Saeed Mahameed wrote:
>>
>> On Mon, Jun 12, 2017 at 9:20 PM, Jes Sorensen  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 
 ---
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, >pdev->dev);
- if (MLX5_CAP_GEN(mdev, vport_group_manager)) {
 -   netdev->netdev_ops = _netdev_ops_sriov;
#ifdef CONFIG_MLX5_CORE_EN_DCB
 -   if (MLX5_CAP_GEN(mdev, qos))
 -   netdev->dcbnl_ops = _dcbnl_ops;
 +   if (MLX5_CAP_GEN(mdev, vport_group_manager) &&
 MLX5_CAP_GEN(mdev,
 qos))
 +   netdev->dcbnl_ops = _dcbnl_ops;
 +#endif
 +
 +#ifdef CONFIG_MLX5_ESWITCH
 +   if (MLX5_CAP_GEN(mdev, vport_group_manager))
 +   netdev->netdev_ops = _netdev_ops_sriov;
 +   else
#endif
 -   } else {
  netdev->netdev_ops = _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
>>

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

2017-06-13 Thread Jes Sorensen 

On 06/13/2017 01:58 PM, Saeed Mahameed wrote:

On Mon, Jun 12, 2017 at 9:20 PM, Jes Sorensen  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 
---
   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, >pdev->dev);
   - if (MLX5_CAP_GEN(mdev, vport_group_manager)) {
-   netdev->netdev_ops = _netdev_ops_sriov;
   #ifdef CONFIG_MLX5_CORE_EN_DCB
-   if (MLX5_CAP_GEN(mdev, qos))
-   netdev->dcbnl_ops = _dcbnl_ops;
+   if (MLX5_CAP_GEN(mdev, vport_group_manager) && MLX5_CAP_GEN(mdev,
qos))
+   netdev->dcbnl_ops = _dcbnl_ops;
+#endif
+
+#ifdef CONFIG_MLX5_ESWITCH
+   if (MLX5_CAP_GEN(mdev, vport_group_manager))
+   netdev->netdev_ops = _netdev_ops_sriov;
+   else
   #endif
-   } else {
 netdev->netdev_ops = _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 = _switchdev_ops;
   #endif



Can MLX5_ESWITCH be enabled without NET_SWITCHDEV?



Yes

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

2017-06-13 Thread Saeed Mahameed 
On Mon, Jun 12, 2017 at 9:20 PM, Jes Sorensen  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 
>> ---
>>   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, >pdev->dev);
>>   - if (MLX5_CAP_GEN(mdev, vport_group_manager)) {
>> -   netdev->netdev_ops = _netdev_ops_sriov;
>>   #ifdef CONFIG_MLX5_CORE_EN_DCB
>> -   if (MLX5_CAP_GEN(mdev, qos))
>> -   netdev->dcbnl_ops = _dcbnl_ops;
>> +   if (MLX5_CAP_GEN(mdev, vport_group_manager) && MLX5_CAP_GEN(mdev,
>> qos))
>> +   netdev->dcbnl_ops = _dcbnl_ops;
>> +#endif
>> +
>> +#ifdef CONFIG_MLX5_ESWITCH
>> +   if (MLX5_CAP_GEN(mdev, vport_group_manager))
>> +   netdev->netdev_ops = _netdev_ops_sriov;
>> +   else
>>   #endif
>> -   } else {
>> netdev->netdev_ops = _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 = _switchdev_ops;
>>   #endif
>
>
> Can MLX5_ESWITCH be enabled without NET_SWITCHDEV?
>

Yes (Legacy SRIOV mode),

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

2017-06-12 Thread 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 
---
  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, >pdev->dev);
  
-	if (MLX5_CAP_GEN(mdev, vport_group_manager)) {

-   netdev->netdev_ops = _netdev_ops_sriov;
  #ifdef CONFIG_MLX5_CORE_EN_DCB
-   if (MLX5_CAP_GEN(mdev, qos))
-   netdev->dcbnl_ops = _dcbnl_ops;
+   if (MLX5_CAP_GEN(mdev, vport_group_manager) && MLX5_CAP_GEN(mdev, qos))
+   netdev->dcbnl_ops = _dcbnl_ops;
+#endif
+
+#ifdef CONFIG_MLX5_ESWITCH
+   if (MLX5_CAP_GEN(mdev, vport_group_manager))
+   netdev->netdev_ops = _netdev_ops_sriov;
+   else
  #endif
-   } else {
netdev->netdev_ops = _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 = _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)
   >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(>wq, wqe_counter_be,
   >next.next_wqe_index);
  }
+#endif
  
  static inline void mlx5e_mpwqe_fill_rx_skb(struct mlx5e_rq *rq,

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

2017-06-08 Thread Saeed Mahameed 
On Thu, Jun 8, 2017 at 12:35 PM, Or Gerlitz  wrote:
> On Thu, Jun 8, 2017 at 2:42 AM, Saeed Mahameed  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(>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

2017-06-08 Thread Or Gerlitz 
On Thu, Jun 8, 2017 at 2:42 AM, Saeed Mahameed  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(>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