On Tue, Jun 13, 2017 at 9:21 PM, Jes Sorensen <jsoren...@fb.com> wrote:
> 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?
>
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
>
