Re: [dpdk-dev] [PATCH v2 5/5] net/fm10k: add switch management support

[Xiaojun Liu <xiaojun.liu@silicom.co.il>]

Hi Xiao,

Thank you! I will follow your comments.

Best regards,
Xiaojun
________________________________
From: Wang, Xiao W <xiao.w.wang@intel.com>
Sent: Tuesday, February 25, 2020 7:28 PM
To: Xiaojun Liu <xiaojun.liu@silicom.co.il>; Zhang, Qi Z <qi.z.zhang@intel.com>; Kwan, Ngai-mint <ngai-mint.kwan@intel.com>; Keller, Jacob E <jacob.e.keller@intel.com>
Cc: dev@dpdk.org <dev@dpdk.org>; Ye, Xiaolong <xiaolong.ye@intel.com>
Subject: RE: [PATCH v2 5/5] net/fm10k: add switch management support

Hi Xiaojun,

Gather some comments about the code changes to original file:

Based on the latest dpdk, "git am" will fail with the Makefile.

Add CONFIG_RTE_FM10K_MANAGEMENT=n as a default option into ./config is better. If user wants to turn on this switch feature, it's easier to just change "n" to "y".

No need to introduce another macro ENABLE_FM10K_MANAGEMENT, just use RTE_FM10K_MANAGEMENT to wrap your code.

For ethdev.c:
static int fm10k_switch_ready; This should be a per device value, right? I assume we may have >1 fm10k devices, then cannot share one state value.

DEV_RX_OFFLOAD_RSS_HASH is removed, it may affect current usage.

Log some debug info before return -EIO.

How about moving the commonly used variables like "struct fm10k_hw *hw" out of "ifdef else".

Put fm10k_link_update() at the end of dev_start().

For fm10k_stats_get(), it looks there's little code shared between #if and #else, you can just write a whole new function.

There's comment "It may not work for VF", have you addressed this uncertainty? We'd better not to leave this in upstream version.

BTW, when sending new versions, you need to document what has changed from last version in your cover letter, in below format:

V2:
* Fix what
* Fix what

Best Regards,
Xiao

> -----Original Message-----
> From: Xiaojun Liu <xiaojun.liu@silicom.co.il>
> Sent: Thursday, February 20, 2020 10:00 PM
> To: Wang, Xiao W <xiao.w.wang@intel.com>; Zhang, Qi Z
> <qi.z.zhang@intel.com>; Kwan, Ngai-mint <ngai-mint.kwan@intel.com>;
> Keller, Jacob E <jacob.e.keller@intel.com>
> Cc: dev@dpdk.org; Xiaojun Liu <xiaojun.liu@silicom.co.il>
> Subject: [PATCH v2 5/5] net/fm10k: add switch management support
>
> Split dev init to 2 parts.
> First only register the port in switch
> management; second init hook will be
> called after all the pf are registered
> and switch initialization. It will finish
> dev init. Also add switch interrupt support.
> Add fm10k_mirror_rule_set/fm10k_mirror_rule_reset
> to support mirror operation. Add fm10k_dev_filter_ctrl
> to support flow operation.
> Add dpdk port and pf mapping, so
> the dpdk port can map to a specific pf
> and 1 dpdk port can map to 2 pf to get
> total 100G throughput.
>
> To enable the switch management, you need add
> CONFIG_RTE_FM10K_MANAGEMENT=y in
> config/common_linux when building.
>
> Signed-off-by: Xiaojun Liu <xiaojun.liu@silicom.co.il>
> ---
[...]