Re: [dpdk-dev] [PATCH v5 03/14] drivers/net/ipn3ke: add IPN3KE ethdev PMD driver

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 4/3/2019 12:47 PM, Rosen Xu wrote:
> Add Intel FPGA Acceleration NIC IPN3KE ethdev PMD driver.
> 
> Signed-off-by: Rosen Xu <rosen.xu@intel.com>
> Signed-off-by: Andy Pei <andy.pei@intel.com>
> Signed-off-by: Dan Wei <dan.wei@intel.com>
<...>

> +Config File Options
> +~~~~~~~~~~~~~~~~~~~
> +
> +The following options can be modified in the ``config`` file.
> +Please note that enabling debugging options may affect system performance.
> +
> +- ``CONFIG_RTE_LIBRTE_IPN3KE_PMD`` (default ``n``)

It looks like enabled by default in config file, which one is the intention.

> +
> +  Toggle compilation of the ``librte_pmd_ipn3ke`` driver.
> +
> +Runtime Config Options
> +~~~~~~~~~~~~~~~~~~~~~~
> +
> +- ``AFU name``
> +
> +  AFU name identifies which AFU is used by IPN3KE.
> +
> +- ``FPGA Acceleration list``
> +
> +  For IPN3KE FPGA can provide different bitstream, different bitstream includes different
> +  Acceleration, so users need to identify which Acceleration is used.
> +
> +- ``I40e PF name list``
> +
> +  Users need to bind FPGA LineSidePort to FVL PF. So I40e PF name list should be involved in
> +  startup command.

Can you please document the actual options themselves here?

<...>

> +static int ipn3ke_indirect_read(struct ipn3ke_hw *hw,
> +					 uint32_t *rd_data,
> +					 uint32_t addr,
> +					 uint32_t dev_sel,
> +					 uint32_t eth_group_sel)

Can you please apply the coding convention:
static int
<function name> (parameters)
{

Also how parameters aligned here?
according coding convention it should be:

static int
ipn3ke_indirect_read(struct ipn3ke_hw *hw, uint32_t *rd_data, uint32_t addr,
	uint32_t dev_sel, uint32_t eth_group_sel)

Can you please fix this for all functions?

<...>

> +static int
> +ipn3ke_cfg_parse_i40e_pf_ethdev(const char *afu_name,
> +const char *pf_name)

Please fix indentation.

<...>

> +RTE_PMD_REGISTER_VDEV(ipn3ke_cfg, ipn3ke_cfg_driver);
> +RTE_PMD_REGISTER_ALIAS(ipn3ke_cfg, ipn3ke_cfg);

Creating alias with same name :) Please drop.

<...>

> +RTE_INIT(ipn3ke_afu_init_log);
> +static void
> +ipn3ke_afu_init_log(void)

Can merge these lines into single:
RTE_INIT(ipn3ke_afu_init_log)

It looks like I did same comment in previous versions as well, can you please
check previous reviews for missed comments.

> +{
> +	ipn3ke_afu_logtype = rte_log_register("driver.afu.ipn3ke");

"pmd.afu.ipn3ke"

<...>

> +#include <rte_cycles.h>
> +#include <rte_bus_ifpga.h>
> +#include <rte_tm_driver.h>
> +
> +struct ipn3ke_rpst;

Is this forward decleration needed?

<...>

> @@ -0,0 +1,6 @@
> +# SPDX-License-Identifier: BSD-3-Clause
> +# Copyright(c) 2019 Intel Corporation
> +
> +allow_experimental_apis = true

Can you please add list of experimental APIs called in a comment as done in makefile

<...>

> @@ -12,6 +12,8 @@
>  # The PCI base class for all devices
>  network_class = {'Class': '02', 'Vendor': None, 'Device': None,
>                      'SVendor': None, 'SDevice': None}
> +ifpga_class = {'Class': '12', 'Vendor': '8086', 'Device': 'bcc0,09c4,0b30',
> +                    'SVendor': None, 'SDevice': None}

Only '0x0B30' added in this patch, right?
If you can make this seperate patch, with older device ids, it becomes
backportable, and in this patch you can add new ids.

Should add VF ids, 0x0B31 etc... ?