> -----Original Message-----
> From: Andrew Rybchenko <andrew.rybche...@oktetlabs.ru>
> Sent: Monday, October 3, 2022 21:44
> To: Guo, Junfeng <junfeng....@intel.com>; Zhang, Qi Z
> <qi.z.zh...@intel.com>; Wu, Jingjing <jingjing...@intel.com>; Xing,
> Beilei <beilei.x...@intel.com>
> Cc: dev@dpdk.org; Wang, Xiao W <xiao.w.w...@intel.com>; Li, Xiaoyun
> <xiaoyun...@intel.com>
> Subject: Re: [PATCH v2 03/14] net/idpf: add support for device
> initialization
>
> On 9/5/22 13:58, Junfeng Guo wrote:
> > Support device init and the following dev ops:
> > - dev_configure
> > - dev_start
> > - dev_stop
> > - dev_close
>
> A bit strange set from my point of view since you can't start
> without setup queues. Each patch should be testable.
> It should be no dead code. It should be possible to stop after
> the patch, build, run, for example, testpmd and probe,
> configure, start, stop, close, unplug (depeneding on stage in
> the patch series).
>
> The patch is really big. It definitely worse to split
> configre/close and start/stop (and reorder to have start
> after queues setup supported).
Well, this patch mainly implemented the device initiation, as well
as adding the framework/skeleton of the config/start/stop/close
ops. As for the dev start ops, we decide to firstly add the vports
setup at this stage in this patch and later add other setups like
queues and queue irqs. So in this way, it may be reasonable that
the device could still start even without the queues setup.
And yes, this PMD contains too much code with so many INITs.
We will try to split them in the coming versions. Thanks!
>
> >
> > Signed-off-by: Beilei Xing <beilei.x...@intel.com>
> > Signed-off-by: Xiaoyun Li <xiaoyun...@intel.com>
> > Signed-off-by: Xiao Wang <xiao.w.w...@intel.com>
> > Signed-off-by: Junfeng Guo <junfeng....@intel.com>
> > ---
> > drivers/net/idpf/idpf_ethdev.c | 810
> +++++++++++++++++++++++++++++++++
> > drivers/net/idpf/idpf_ethdev.h | 229 ++++++++++
> > drivers/net/idpf/idpf_vchnl.c | 495 ++++++++++++++++++++
> > drivers/net/idpf/meson.build | 18 +
> > drivers/net/idpf/version.map | 3 +
> > drivers/net/meson.build | 1 +
> > 6 files changed, 1556 insertions(+)
> > create mode 100644 drivers/net/idpf/idpf_ethdev.c
> > create mode 100644 drivers/net/idpf/idpf_ethdev.h
> > create mode 100644 drivers/net/idpf/idpf_vchnl.c
> > create mode 100644 drivers/net/idpf/meson.build
> > create mode 100644 drivers/net/idpf/version.map
> >
> > diff --git a/drivers/net/idpf/idpf_ethdev.c
> b/drivers/net/idpf/idpf_ethdev.c
> > new file mode 100644
> > index 0000000000..f0452de09e
> > --- /dev/null
> > +++ b/drivers/net/idpf/idpf_ethdev.c
> > @@ -0,0 +1,810 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright(c) 2022 Intel Corporation
> > + */
> > +
> > +#include <rte_atomic.h>
> > +#include <rte_eal.h>
> > +#include <rte_ether.h>
> > +#include <rte_malloc.h>
> > +#include <rte_memzone.h>
> > +#include <rte_dev.h>
> > +
> > +#include "idpf_ethdev.h"
> > +
> > +#define IDPF_VPORT "vport"
> > +
> > +struct idpf_adapter_list adapter_list;
> > +bool adapter_list_init;
> > +
> > +uint64_t idpf_timestamp_dynflag;
> > +
> > +static const char * const idpf_valid_args[] = {
> > + IDPF_VPORT,
> > + NULL
> > +};
> > +
> > +static int idpf_dev_configure(struct rte_eth_dev *dev);
> > +static int idpf_dev_start(struct rte_eth_dev *dev);
> > +static int idpf_dev_stop(struct rte_eth_dev *dev);
> > +static int idpf_dev_close(struct rte_eth_dev *dev);
> > +
> > +static const struct eth_dev_ops idpf_eth_dev_ops = {
> > + .dev_configure = idpf_dev_configure,
> > + .dev_start = idpf_dev_start,
> > + .dev_stop = idpf_dev_stop,
> > + .dev_close = idpf_dev_close,
> > +};
> > +
> > +static int
> > +idpf_init_vport_req_info(struct rte_eth_dev *dev)
> > +{
> > + struct idpf_vport *vport = dev->data->dev_private;
> > + struct idpf_adapter *adapter = vport->adapter;
> > + struct virtchnl2_create_vport *vport_info;
> > + uint16_t idx = adapter->next_vport_idx;
> > +
> > + if (!adapter->vport_req_info[idx]) {
>
> DPDK coding style requires explicit comparison vs NULL [1].
> It is not applicable to base driver, but PMD itself should
> follow it. There is really huge number of places to fix.
>
> It is applicable to comparision integers vs 0 as well.
Will fix these in the coming versions. Thanks!
>
> [1] https://doc.dpdk.org/guides/contributing/coding_style.html#null-
> pointers
>
> [snip]
>
> > +static int
> > +idpf_dev_configure(__rte_unused struct rte_eth_dev *dev)
> > +{
>
> I'm really confusing that the configure as empty.
> It must validate configuration provided via
> rte_eth_dev_configure() and reject if something is not
> supported if ethdev does not the job.
Well, the initiation of some resources like vports are not put at this stage.
It is flexible to add the framework/skeleton of the config/start/stop/close
ops. Looks that it ok to implement like this... Thanks!
>
> > + return 0;
> > +}
>
> [snip]
>
> > diff --git a/drivers/net/idpf/meson.build b/drivers/net/idpf/meson.build
> > new file mode 100644
> > index 0000000000..7d776d3a15
> > --- /dev/null
> > +++ b/drivers/net/idpf/meson.build
> > @@ -0,0 +1,18 @@
> > +# SPDX-License-Identifier: BSD-3-Clause
> > +# Copyright(c) 2022 Intel Corporation
> > +
> > +if is_windows
> > + build = false
> > + reason = 'not supported on Windows'
> > + subdir_done()
> > +endif
> > +
> > +subdir('base')
> > +objs = [base_objs]
> > +
> > +sources = files(
> > + 'idpf_ethdev.c',
> > + 'idpf_vchnl.c',
>
> TAB vs spaces? Or is it just mail glitch?
Will double check for this, thanks!
>
> > +)
> > +
> > +includes += include_directories('base')
> > \ No newline at end of file
> > diff --git a/drivers/net/idpf/version.map b/drivers/net/idpf/version.map
> > new file mode 100644
> > index 0000000000..b7da224860
> > --- /dev/null
> > +++ b/drivers/net/idpf/version.map
> > @@ -0,0 +1,3 @@
> > +DPDK_22 {
> > + local: *;
> > +};
> > \ No newline at end of file
>
> It must be an empty line at the end of file.
Newline at EOF is not required for DPDK code.
This warning may be added by mistake.
Will double check for this, thanks!
>
> > diff --git a/drivers/net/meson.build b/drivers/net/meson.build
> > index e35652fe63..8faf8120c2 100644
> > --- a/drivers/net/meson.build
> > +++ b/drivers/net/meson.build
> > @@ -28,6 +28,7 @@ drivers = [
> > 'i40e',
> > 'iavf',
> > 'ice',
> > + 'idpf',
>
> TAB vs spaces? Or is it just mail glitch?
Will double check for this, thanks!
>
> > 'igc',
> > 'ionic',
> > 'ipn3ke',
