> -----Original Message-----
> From: Ferruh Yigit <ferruh.yi...@amd.com>
> Sent: Tuesday, February 28, 2023 5:44 AM
> To: Liu, Mingxia <mingxia....@intel.com>; Xing, Beilei
> <beilei.x...@intel.com>;
> Zhang, Yuying <yuying.zh...@intel.com>
> Cc: dev@dpdk.org; Yigit, Ferruh <ferruh.yi...@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....@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.zh...@intel.com>
> > +M: Beilei Xing <beilei.x...@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....@intel.com>
> > M: Simei Su <simei...@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().
