Re: [PATCH v7 01/21] net/cpfl: support device initialization

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

On 2/16/2023 12:29 AM, Mingxia Liu wrote:
> Support device init and add the following dev ops:
>  - dev_configure
>  - dev_close
>  - dev_infos_get
>  - link_update
>  - dev_supported_ptypes_get
> 
> Signed-off-by: Mingxia Liu <mingxia.liu@intel.com>
> ---
>  MAINTAINERS                            |   8 +
>  doc/guides/nics/cpfl.rst               |  66 +++

Need to add file to toctree (doc/guides/nics/index.rst) to make it visible.

>  doc/guides/nics/features/cpfl.ini      |  12 +
>  doc/guides/rel_notes/release_23_03.rst |   6 +
>  drivers/net/cpfl/cpfl_ethdev.c         | 768 +++++++++++++++++++++++++
>  drivers/net/cpfl/cpfl_ethdev.h         |  78 +++
>  drivers/net/cpfl/cpfl_logs.h           |  32 ++
>  drivers/net/cpfl/cpfl_rxtx.c           | 244 ++++++++
>  drivers/net/cpfl/cpfl_rxtx.h           |  25 +

cpfl_rxtx.[ch] not used at all in this patch,
'cpfl_tx_queue_setup()' is added in this patch and next patch (2/21)
looks a better place for it.

>  drivers/net/cpfl/meson.build           |  14 +
>  drivers/net/meson.build                |   1 +
>  11 files changed, 1254 insertions(+)
>  create mode 100644 doc/guides/nics/cpfl.rst
>  create mode 100644 doc/guides/nics/features/cpfl.ini
>  create mode 100644 drivers/net/cpfl/cpfl_ethdev.c
>  create mode 100644 drivers/net/cpfl/cpfl_ethdev.h
>  create mode 100644 drivers/net/cpfl/cpfl_logs.h
>  create mode 100644 drivers/net/cpfl/cpfl_rxtx.c
>  create mode 100644 drivers/net/cpfl/cpfl_rxtx.h
>  create mode 100644 drivers/net/cpfl/meson.build
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 9a0f416d2e..af80edaf6e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -783,6 +783,14 @@ F: drivers/common/idpf/
>  F: doc/guides/nics/idpf.rst
>  F: doc/guides/nics/features/idpf.ini
>  
> +Intel cpfl
> +M: Yuying Zhang <yuying.zhang@intel.com>
> +M: Beilei Xing <beilei.xing@intel.com>
> +T: git://dpdk.org/next/dpdk-next-net-intel
> +F: drivers/net/cpfl/
> +F: doc/guides/nics/cpfl.rst
> +F: doc/guides/nics/features/cpfl.ini
> +

Documentation mentions driver is experimental, can you please highlight
this in the maintainers file too, as:
Intel cpfl - EXPERIMENTAL

>  Intel igc
>  M: Junfeng Guo <junfeng.guo@intel.com>
>  M: Simei Su <simei.su@intel.com>
> diff --git a/doc/guides/nics/cpfl.rst b/doc/guides/nics/cpfl.rst
> new file mode 100644
> index 0000000000..7c5aff0789
> --- /dev/null
> +++ b/doc/guides/nics/cpfl.rst
> @@ -0,0 +1,66 @@
> +.. SPDX-License-Identifier: BSD-3-Clause
> +   Copyright(c) 2022 Intel Corporation.
> +

s/2022/2023/

> +.. include:: <isonum.txt>
> +
> +CPFL Poll Mode Driver
> +=====================
> +
> +The [*EXPERIMENTAL*] cpfl PMD (**librte_net_cpfl**) provides poll mode driver support
> +for Intel\ |reg| Infrastructure Processing Unit (Intel\ |reg| IPU) E2100.
> +

Can you please provide a link for the mentioned device?

So, interested users can evaluate, learn more about the mentioned hardware.


> +
> +Linux Prerequisites
> +-------------------
> +
> +Follow the DPDK :doc:`../linux_gsg/index` to setup the basic DPDK environment.
> +
> +To get better performance on Intel platforms,
> +please follow the :doc:`../linux_gsg/nic_perf_intel_platform`.
> +
> +
> +Pre-Installation Configuration
> +------------------------------
> +
> +Runtime Config Options
> +~~~~~~~~~~~~~~~~~~~~~~

Is "Runtime Config Options", a sub section of "Pre-Installation
Configuration"?

> +
> +- ``vport`` (default ``0``)
> +
> +  The PMD supports creation of multiple vports for one PCI device,
> +  each vport corresponds to a single ethdev.
> +  The user can specify the vports with specific ID to be created, for example::
> +
> +    -a ca:00.0,vport=[0,2,3]
> +
> +  Then the PMD will create 3 vports (ethdevs) for device ``ca:00.0``.
> +

Why need specific IDs?

other option is just provide number of requested vports and they get
sequential ids, but since vport ids are got from user instead there must
be some significance of them, can you please briefly document why ids
matter.

> +  If the parameter is not provided, the vport 0 will be created by default.
> +
> +- ``rx_single`` (default ``0``)
> +
> +  There are two queue modes supported by Intel\ |reg| IPU Ethernet E2100 Series,
> +  single queue mode and split queue mode for Rx queue.

Can you please describe in the documentation what 'split queue' and
'single queue' are and what is the difference between them?

<...>

> index 07914170a7..b0b23d1a44 100644
> --- a/doc/guides/rel_notes/release_23_03.rst
> +++ b/doc/guides/rel_notes/release_23_03.rst
> @@ -88,6 +88,12 @@ New Features
>    * Added timesync API support.
>    * Added packet pacing(launch time offloading) support.
>  
> +* **Added Intel cpfl driver.**
> +
> +  Added the new ``cpfl`` net driver
> +  for Intel\ |reg| Infrastructure Processing Unit (Intel\ |reg| IPU) E2100.
> +  See the :doc:`../nics/cpfl` NIC guide for more details on this new driver.
> +

"New Features" section is grouped, an that grouping is documented in the
section comment.

Can you please move the update to the proper location in the section.

<...>

> +static int
> +cpfl_dev_link_update(struct rte_eth_dev *dev,
> +		     __rte_unused int wait_to_complete)
> +{
> +	struct idpf_vport *vport = dev->data->dev_private;
> +	struct rte_eth_link new_link;
> +
> +	memset(&new_link, 0, sizeof(new_link));
> +
> +	switch (vport->link_speed) {
> +	case RTE_ETH_SPEED_NUM_10M:
> +		new_link.link_speed = RTE_ETH_SPEED_NUM_10M;
> +		break;
> +	case RTE_ETH_SPEED_NUM_100M:
> +		new_link.link_speed = RTE_ETH_SPEED_NUM_100M;
> +		break;
> +	case RTE_ETH_SPEED_NUM_1G:
> +		new_link.link_speed = RTE_ETH_SPEED_NUM_1G;
> +		break;
> +	case RTE_ETH_SPEED_NUM_10G:
> +		new_link.link_speed = RTE_ETH_SPEED_NUM_10G;
> +		break;
> +	case RTE_ETH_SPEED_NUM_20G:
> +		new_link.link_speed = RTE_ETH_SPEED_NUM_20G;
> +		break;
> +	case RTE_ETH_SPEED_NUM_25G:
> +		new_link.link_speed = RTE_ETH_SPEED_NUM_25G;
> +		break;
> +	case RTE_ETH_SPEED_NUM_40G:
> +		new_link.link_speed = RTE_ETH_SPEED_NUM_40G;
> +		break;
> +	case RTE_ETH_SPEED_NUM_50G:
> +		new_link.link_speed = RTE_ETH_SPEED_NUM_50G;
> +		break;
> +	case RTE_ETH_SPEED_NUM_100G:
> +		new_link.link_speed = RTE_ETH_SPEED_NUM_100G;
> +		break;
> +	case RTE_ETH_SPEED_NUM_200G:
> +		new_link.link_speed = RTE_ETH_SPEED_NUM_200G;
> +		break;

What about:
```
switch (vport->link_speed) {
case RTE_ETH_SPEED_NUM_10M:
case RTE_ETH_SPEED_NUM_100M:
...
case RTE_ETH_SPEED_NUM_200G:
	new_link.link_speed = vport->link_speed;
	break;
default:
	new_link.link_speed = RTE_ETH_SPEED_NUM_UNKNOWN;
```

OR

```
for (i = 0; i < RTE_DIM(supported_speeds); i++) {
	if (vport->link_speed == supported_speeds[i]) {
		new_link.link_speed = vport->link_speed;
		break;
	}
}

if (i == RTE_DIM(supported_speeds))
	new_link.link_speed = RTE_ETH_SPEED_NUM_UNKNOWN;
```

> +	default:
> +		new_link.link_speed = RTE_ETH_SPEED_NUM_NONE;

I think this should be :

if (link_up)
	new_link.link_speed = RTE_ETH_SPEED_NUM_UNKNOWN;
else
	new_link.link_speed = RTE_ETH_SPEED_NUM_NONE;

<...>

> +static int
> +insert_value(struct cpfl_devargs *devargs, uint16_t id)
> +{
> +	uint16_t i;
> +
> +	/* ignore duplicate */
> +	for (i = 0; i < devargs->req_vport_nb; i++) {
> +		if (devargs->req_vports[i] == id)
> +			return 0;
> +	}
> +
> +	if (devargs->req_vport_nb >= RTE_DIM(devargs->req_vports)) {
> +		PMD_INIT_LOG(ERR, "Total vport number can't be > %d",
> +			     CPFL_MAX_VPORT_NUM);

Check is using 'RTE_DIM(devargs->req_vports)' and log is using
'CPFL_MAX_VPORT_NUM', they are same value but better to stick to one of
them.

<...>

> +static int
> +parse_vport(const char *key, const char *value, void *args)
> +{
> +	struct cpfl_devargs *devargs = args;
> +	const char *pos = value;
> +
> +	devargs->req_vport_nb = 0;
> +

if "vport" can be provided multiple times, above assignment is wrong, like:
"vport=1,vport=3-5"

<...>

> +static int
> +cpfl_parse_devargs(struct rte_pci_device *pci_dev, struct cpfl_adapter_ext *adapter,
> +		   struct cpfl_devargs *cpfl_args)
> +{
> +	struct rte_devargs *devargs = pci_dev->device.devargs;
> +	struct rte_kvargs *kvlist;
> +	int i, ret;
> +
> +	cpfl_args->req_vport_nb = 0;
> +
> +	if (devargs == NULL)
> +		return 0;
> +
> +	kvlist = rte_kvargs_parse(devargs->args, cpfl_valid_args);
> +	if (kvlist == NULL) {
> +		PMD_INIT_LOG(ERR, "invalid kvargs key");
> +		return -EINVAL;
> +	}
> +
> +	/* check parsed devargs */
> +	if (adapter->cur_vport_nb + cpfl_args->req_vport_nb >
> +	    CPFL_MAX_VPORT_NUM) {

At this stage 'cpfl_args->req_vport_nb' is 0 since CPFL_VPORT is not
parsed yet, is the intention to do this check after 'rte_kvargs_processs()'?

> +		PMD_INIT_LOG(ERR, "Total vport number can't be > %d",
> +			     CPFL_MAX_VPORT_NUM);
> +		ret = -EINVAL;
> +		goto bail;
> +	}
> +
> +	for (i = 0; i < cpfl_args->req_vport_nb; i++) {> +		if (adapter->cur_vports & RTE_BIT32(cpfl_args->req_vports[i])) {
> +			PMD_INIT_LOG(ERR, "Vport %d has been created",
> +				     cpfl_args->req_vports[i]);

This is just argument parsing, nothing created yet, I suggest updating
log accordingly.

> +			ret = -EINVAL;
> +			goto bail;
> +		}
> +	}

same here, both for 'cpfl_args->req_vport_nb' &
'cpfl_args->req_vports[]', they are not updated yet.

<...>

> +static void
> +cpfl_handle_virtchnl_msg(struct cpfl_adapter_ext *adapter_ex)
> +{
> +	struct idpf_adapter *adapter = &adapter_ex->base;

Everywhere else, 'struct cpfl_adapter_ext' type variable name is 'adapter',
here it is 'adapter_ex' and 'struct idpf_adapter' type is 'adapter'.

As far as I understand 'struct cpfl_adapter_ext' is something like
"extended adapter" and extended version of 'struct idpf_adapter', so in
the context of this driver what do you think to refer:
'struct cpfl_adapter_ext' as 'adapter'
'struct idpf_adapter'     as 'base' (or 'adapter_base'), consistently.

<...>

> +static const struct eth_dev_ops cpfl_eth_dev_ops = {
> +	.dev_configure			= cpfl_dev_configure,
> +	.dev_close			= cpfl_dev_close,
> +	.dev_infos_get			= cpfl_dev_info_get,
> +	.link_update			= cpfl_dev_link_update,
> +	.dev_supported_ptypes_get	= cpfl_dev_supported_ptypes_get,
> +};

Can you please move the block just after 'cpfl_dev_close()', to group
dev_ops related code together.

<...>

> +
> +static int
> +cpfl_dev_vport_init(struct rte_eth_dev *dev, void *init_params)
> +{
> +	struct idpf_vport *vport = dev->data->dev_private;
> +	struct cpfl_vport_param *param = init_params;
> +	struct cpfl_adapter_ext *adapter = param->adapter;
> +	/* for sending create vport virtchnl msg prepare */
> +	struct virtchnl2_create_vport create_vport_info;
> +	int ret = 0;
> +
> +	dev->dev_ops = &cpfl_eth_dev_ops;
> +	vport->adapter = &adapter->base;
> +	vport->sw_idx = param->idx;
> +	vport->devarg_id = param->devarg_id;
> +	vport->dev = dev;
> +
> +	memset(&create_vport_info, 0, sizeof(create_vport_info));
> +	ret = idpf_vport_info_init(vport, &create_vport_info);
> +	if (ret != 0) {
> +		PMD_INIT_LOG(ERR, "Failed to init vport req_info.");
> +		goto err;
> +	}
> +
> +	ret = idpf_vport_init(vport, &create_vport_info, dev->data);
> +	if (ret != 0) {
> +		PMD_INIT_LOG(ERR, "Failed to init vports.");
> +		goto err;
> +	}
> +
> +	adapter->vports[param->idx] = vport;
> +	adapter->cur_vports |= RTE_BIT32(param->devarg_id);
> +	adapter->cur_vport_nb++;
> +
> +	dev->data->mac_addrs = rte_zmalloc(NULL, RTE_ETHER_ADDR_LEN, 0);
> +	if (dev->data->mac_addrs == NULL) {
> +		PMD_INIT_LOG(ERR, "Cannot allocate mac_addr memory.");
> +		ret = -ENOMEM;
> +		goto err_mac_addrs;
> +	}
> +
> +	rte_ether_addr_copy((struct rte_ether_addr *)vport->default_mac_addr,
> +			    &dev->data->mac_addrs[0]);
> +
> +	return 0;
> +
> +err_mac_addrs:
> +	adapter->vports[param->idx] = NULL;  /* reset */

shouln't update 'cur_vports' & 'cur_vport_nb' too in this error path.

<...>

> +
> +err:
> +	if (first_probe) {
> +		rte_spinlock_lock(&cpfl_adapter_lock);
> +		TAILQ_REMOVE(&cpfl_adapter_list, adapter, next);
> +		rte_spinlock_unlock(&cpfl_adapter_lock);
> +		cpfl_adapter_ext_deinit(adapter);
> +		rte_free(adapter);
> +	}


Why 'first_probe' is needed, it looks like it is for the case when
probe() called multiple time for same pci_dev, can this happen?

<...>

> +RTE_PMD_REGISTER_PCI(net_cpfl, rte_cpfl_pmd);
> +RTE_PMD_REGISTER_PCI_TABLE(net_cpfl, pci_id_cpfl_map);
> +RTE_PMD_REGISTER_KMOD_DEP(net_cpfl, "* igb_uio | vfio-pci");
> +RTE_PMD_REGISTER_PARAM_STRING(net_cpfl,
> +			      CPFL_TX_SINGLE_Q "=<0|1> "
> +			      CPFL_RX_SINGLE_Q "=<0|1> "
> +			      CPFL_VPORT "=[vport_set0,[vport_set1],...]");

What about:
"\[vport0_begin[-vport0_end][,vport1_begin[-vport1_end][,..]\]"

<...>

> +
> +#define CPFL_MAX_VPORT_NUM	8
> +
It looks like there is a dynamic max vport number
(adapter->base.caps.max_vports), and there is above hardcoded define,
for requested (devargs) vports.

The dynamic max is received via 'cpfl_adapter_ext_init()' before parsing
dev_arg, so can it be possible to remove this hardcoded max completely?


> +#define CPFL_INVALID_VPORT_IDX	0xffff
> +
> +#define CPFL_MIN_BUF_SIZE	1024
> +#define CPFL_MAX_FRAME_SIZE	9728
> +#define CPFL_DEFAULT_MTU	RTE_ETHER_MTU
> +
> +#define CPFL_NUM_MACADDR_MAX	64

The macro is not used, can you please add them when they are used.

<...>

> @@ -0,0 +1,32 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2023 Intel Corporation
> + */
> +
> +#ifndef _CPFL_LOGS_H_
> +#define _CPFL_LOGS_H_
> +
> +#include <rte_log.h>
> +
> +extern int cpfl_logtype_init;
> +extern int cpfl_logtype_driver;
> +
> +#define PMD_INIT_LOG(level, ...) \
> +	rte_log(RTE_LOG_ ## level, \
> +		cpfl_logtype_init, \
> +		RTE_FMT("%s(): " \
> +			RTE_FMT_HEAD(__VA_ARGS__,) "\n", \
> +			__func__, \
> +			RTE_FMT_TAIL(__VA_ARGS__,)))
> +
> +#define PMD_DRV_LOG_RAW(level, ...) \
> +	rte_log(RTE_LOG_ ## level, \
> +		cpfl_logtype_driver, \
> +		RTE_FMT("%s(): " \
> +			RTE_FMT_HEAD(__VA_ARGS__,) "\n", \
> +			__func__, \
> +			RTE_FMT_TAIL(__VA_ARGS__,)))
> +
> +#define PMD_DRV_LOG(level, fmt, args...) \
> +	PMD_DRV_LOG_RAW(level, fmt "\n", ## args)
> +

Is 'PMD_DRV_LOG_RAW' required at all, why not define 'PMD_DRV_LOG'
directly as it is done with 'PMD_INIT_LOG'?

Btw, both 'PMD_DRV_LOG_RAW' seems adding double '\n', one as part of
'fmt', other in the rte_log().