Re: [dpdk-dev] [PATCH v5 01/13] regex/mlx5: add RegEx PMD layer and mlx5 driver

[Ori Kam <orika@mellanox.com>]

Hi Thomas,

Thanks for the review.

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> 
> 19/07/2020 20:09, Ori Kam:
> > From: Yuval Avnery <yuvalav@mellanox.com>
> >
> > This commit introduce the RegEx pull mode drivers class, and
> > adds Mellanox RegEx PMD.
> [...]
> > --- /dev/null
> > +++ b/doc/guides/regexdevs/features_overview.rst
> > @@ -0,0 +1,100 @@
> > +..  SPDX-License-Identifier: BSD-3-Clause
> > +   Copyright 2020 Mellanox Technologies, Ltd
> 
> SPDX is not aligned with the Copyright.
> You should not have a double space before SPDX.
> 
O.K.

> [...]
> > --- /dev/null
> > +++ b/doc/guides/regexdevs/index.rst
> > @@ -0,0 +1,15 @@
> > +..  SPDX-License-Identifier: BSD-3-Clause
> > +    Copyright 2020 Mellanox Technologies, Ltd
> 
> Same here. RST annotations start at the third column.
> 
O.K
> > +
> > +REGEX Device Drivers
> > +====================
> > +
> > +The following are a list of RegEx (Regular Expression) device drivers,
> > +which can be used from an application through RegEx API.
> > +
> > +.. toctree::
> > +    :maxdepth: 2
> > +    :numbered:
> 
> same here
> 
O.K.

> [...]
> > +   Due to external dependencies, this driver is disabled in default
> > +   configuration of the "make" build. It can be enabled with
> > +   ``CONFIG_RTE_LIBRTE_MLX5_REGEX_PMD=y`` or by using "meson" build
> system which
> > +   will detect dependencies.
> 
> I would drop this part. I don't think it is an useful info,
> especially because make is going to be removed.
> 
I think it is a good comment at least for now, since the RegEx is going to be used by
costumers that are not used to Mellanox devices.
But if you wish I will remove.

> > +Mellanox mlx5 PCI device can be probed by number of different pci devices,
> for example
> 
> Inconsistency on the same line: "pci" vs "PCI".
>
Will fix.
 
> Better to break line after the comma and after a dot.
> 
Will fix

> > +net / vDPA / RegEx. To select the RegEx PMD ``class=regex`` should be
> specified
> > +as device parameter.
> 
> This does not explain if RegEx is exclusive with other classes.
> 
Will clarify.

> [...]
> > +- BlueField 2 running Mellonx supported kernel.
> 
> typo: Mellonx
> 
Will fix.

> > +- Enable the RegEx caps using system call from the BlueField 2.
> 
> This is a doc. Please write full words: "capabilities".
>
Will fix.

 
> [...]
> > +These options can be modified in the ``.config`` file.
> > +
> > +- ``CONFIG_RTE_LIBRTE_MLX5_REGEX_PMD`` (default **n**)
> > +
> > +  Toggle compilation of librte_pmd_mlx5 itself.
> 
> Better to drop this "make" part.
> [...]

O.K. but Make is still a build system, that is used, but I will remove.

> >  enum mlx5_class {
> >  	MLX5_CLASS_INVALID,
> >  	MLX5_CLASS_NET = RTE_BIT64(0),
> > +	MLX5_CLASS_REGEX = RTE_BIT64(2),
> >  	MLX5_CLASS_VDPA = RTE_BIT64(1),
> 
> I think regex would be better sorted after vDPA.
> 
I did this to have alphabetic ordering.

> [...]
> > +++ b/drivers/regex/meson.build
> > @@ -0,0 +1,9 @@
> > +# SPDX-License-Identifier: BSD-3-Clause
> > +# Copyright 2020 Mellanox Technologies, Ltd
> > +
> > +drivers = ['mlx5']
> > +std_deps = ['ethdev', 'kvargs'] # 'ethdev' also pulls in mbuf, net, eal etc
> > +std_deps += ['bus_pci']         # very many PMDs depend on PCI, so make std
> > +std_deps += ['bus_vdev']        # same with vdev bus
> 
> I disagree about making bus some standard deps.

O.K will remove

> 
>