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

["Liu, Mingxia" <mingxia.liu@intel.com>]

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@amd.com>
> Sent: Tuesday, February 28, 2023 5:44 AM
> To: Liu, Mingxia <mingxia.liu@intel.com>; Xing, Beilei <beilei.xing@intel.com>;
> Zhang, Yuying <yuying.zhang@intel.com>
> Cc: dev@dpdk.org; Yigit, Ferruh <ferruh.yigit@amd.com>
> Subject: Re: [PATCH v7 01/21] net/cpfl: support device initialization
> 
> 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.
> 
[Liu, Mingxia] ok, got it! Thanks!

> >  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.
> 
[Liu, Mingxia] ok, will update it, thanks!

> >  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
> 
[Liu, Mingxia] ok, will update it, thanks!

> >  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.
> 
[Liu, Mingxia] ok, will add it, thanks!
> 
> > +
> > +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"?
> 
[Liu, Mingxia] Yes, refer to ice and i40e .rst.

> > +
> > +- ``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?
> 
[beilei] we’re using this ID to map physical port to vport as default receive and transmit interface.

> 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?
>
 [beilei] sure, will add the description.

> <...>
> 
> > 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.
> 
[Liu, Mingxia] ok, thanks will update it.
> <...>
> 
> > +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;
>
[Liu, Mingxia] Thanks, good idea, will update. 

> <...>
> 
> > +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.
> 
[Liu, Mingxia] Thanks, good idea, will update.
> <...>
> 
> > +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"
> 
[beilei] We won’t support this case. Will add check in idpf_parse_devargs.

> <...>
> 
> > +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()'?
> 
[beilei] Yes, thanks for the catch, will fix it in next version.

> > +		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.
> 
[beilei] OK, will update the log in the next version.

> > +			ret = -EINVAL;
> > +			goto bail;
> > +		}
> > +	}
> 
> same here, both for 'cpfl_args->req_vport_nb' & 'cpfl_args->req_vports[]',
> they are not updated yet.
>
[beilei] Yes, thanks for the catch, will fix it in next version.

> <...>
> 
> > +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.
> 
[Liu, Mingxia] ok, got it, I'll update them, for your comments.

> <...>
> 
> > +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.
> 
[Liu, Mingxia] ok, thanks!

> <...>
> 
> > +
> > +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.
> 
[beilei] Yes, need to update the two fields.
> <...>
> 
> > +
> > +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?
> 
[Liu, Mingxia] It's related to runtime creating vport in the future, but now the version doesn't support runtime.
> <...>
> 
> > +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][,..]\]"
> 
[Liu, Mingxia] Good idea, thanks!

> <...>
> 
> > +
> > +#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?
> 
> 
[Liu, Mingxia] yes, we'll try.

> > +#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.
> 
[Liu, Mingxia] ok, thanks! Will delete it.

> <...>
> 
> > @@ -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'?
>
[Liu, Mingxia] Good idea, I'll simplify the code.

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