Re: [PATCH v2] net/mlx5: add support for ConnectX-5 NICs

[Adrien Mazarguil <adrien.mazarguil@6wind.com>]

Hi Koh,

A few remaining comments, please see below.

On Thu, Jan 05, 2017 at 03:52:54AM -0800, Yongseok Koh wrote:
> Add PCI device ID for ConnectX-5 and enable multi-packet send for PF and VF
> along with changing documentation and release note.
> 
> Signed-off-by: Yongseok Koh <yskoh@mellanox.com>
> ---
> 
> ConnectX-5 is a newly announced NIC of Mellanox. This patch includes basic
> enablement of ConnectX-5 as well as documentation.
> 
>  config/common_base                     |  2 +-
>  doc/guides/nics/mlx5.rst               | 37 +++++++++++++++++-------------
>  doc/guides/rel_notes/release_17_02.rst |  5 ++++
>  drivers/net/mlx5/mlx5.c                | 42 ++++++++++++++++++++++++++++++----
>  drivers/net/mlx5/mlx5.h                |  4 ++++
>  drivers/net/mlx5/mlx5_ethdev.c         |  7 ++----
>  drivers/net/mlx5/mlx5_txq.c            |  2 +-
>  7 files changed, 71 insertions(+), 28 deletions(-)
[...]
> diff --git a/doc/guides/nics/mlx5.rst b/doc/guides/nics/mlx5.rst
[...]
> @@ -185,8 +185,8 @@ Run-time configuration
>    save PCI bandwidth and improve performance at the cost of a slightly
>    higher CPU usage.
>  
> -  It is currently only supported on the ConnectX-4 Lx family of adapters.
> -  Enabled by default.
> +  It is currently only supported on the ConnectX-4 Lx and ConnectX-5
> +  families of adapters.  Enabled by default.

Minor nit, can you remove the double spacing here? I know there is another
one in the same document but it does break the style, it will be fixed
eventually.

[...]
> @@ -241,12 +242,16 @@ DPDK and must be installed separately:
>  
>  Currently supported by DPDK:
>  
> -- Mellanox OFED **3.4-1.0.0.0**.
> +- Mellanox OFED version:

An empty line is missing after this line. It causes the list to not display
properly once exported to other formats.

> +  - ConnectX-4: **3.4-1.0.0.0** or higher
> +  - ConnectX-4 Lx: **3.4-1.0.0.0** or higher
> +  - ConnectX-5: **4.0-0.0.8.1** or higher
> 
>  - firmware version:
>  
> -  - ConnectX-4: **12.17.1010**
> -  - ConnectX-4 Lx: **14.17.1010**
> +  - ConnectX-4: **12.17.1010** or higher
> +  - ConnectX-4 Lx: **14.17.1010** or higher
> +  - ConnectX-5: **16.18.0296** or higher

"or higher" was implicit until now, if you want to make it explicit I think
it's better to write it once in some other place (e.g. by clarifying
"Currently supported by DPDK"). Falls outside the scope of this patch
though.

>  Getting Mellanox OFED
>  ~~~~~~~~~~~~~~~~~~~~~
> @@ -288,8 +293,8 @@ behavior as librte_pmd_mlx4:
>  Usage example
>  -------------
>  
> -This section demonstrates how to launch **testpmd** with Mellanox ConnectX-4
> -devices managed by librte_pmd_mlx5.
> +This section demonstrates how to launch **testpmd** with Mellanox
> +ConnectX-4/ConnectX-5 devices managed by librte_pmd_mlx5.
>  
>  #. Load the kernel modules:
>  
> diff --git a/doc/guides/rel_notes/release_17_02.rst b/doc/guides/rel_notes/release_17_02.rst
> index 699487dfe..6da4d0f8d 100644
> --- a/doc/guides/rel_notes/release_17_02.rst
> +++ b/doc/guides/rel_notes/release_17_02.rst
> @@ -61,6 +61,11 @@ New Features
>    A new network PMD which supports Solarflare SFN7xxx and SFN8xxx family
>    of 10/40 Gbps adapters has been added.
>  
> +* **Added support for Mellanox ConnectX-5 adpaters (mlx5).**

Typo, "adpaters".

-- 
Adrien Mazarguil
6WIND