From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 36069A00C3; Mon, 3 Oct 2022 15:44:05 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id C7DD140DFB; Mon, 3 Oct 2022 15:44:04 +0200 (CEST) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id 5BC4640695 for ; Mon, 3 Oct 2022 15:44:03 +0200 (CEST) Received: from [192.168.38.17] (aros.oktetlabs.ru [192.168.38.17]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by shelob.oktetlabs.ru (Postfix) with ESMTPSA id CA30366; Mon, 3 Oct 2022 16:44:02 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru CA30366 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1664804642; bh=GfZAfzMkJWt/YddaCa0R7PwrrRa+C+JIDl8/LneqsAA=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=DTApBQdQEw/dIhwSJHAsF9XLgvP+/E3FiJP3ns2NhIj2gIG3mOPnU9WWF0EUy9eCW HbQCI+aphw+wUtbwXwwv/P5KfEXc8rTVVN6RLStiDqSNsIyjLEe/aYFGFixfuqxXGA /Ssolvp0BNW086jPFoyoUoBB/FeHhIHla62BRrs0= Message-ID: <77c843b5-5742-f1b8-bcf5-63eefe363479@oktetlabs.ru> Date: Mon, 3 Oct 2022 16:44:02 +0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.3.0 Subject: Re: [PATCH v2 03/14] net/idpf: add support for device initialization Content-Language: en-US To: Junfeng Guo , qi.z.zhang@intel.com, jingjing.wu@intel.com, beilei.xing@intel.com Cc: dev@dpdk.org, xiao.w.wang@intel.com, Xiaoyun Li References: <20220803113104.1184059-1-junfeng.guo@intel.com> <20220905105828.3190335-1-junfeng.guo@intel.com> <20220905105828.3190335-4-junfeng.guo@intel.com> From: Andrew Rybchenko Organization: OKTET Labs In-Reply-To: <20220905105828.3190335-4-junfeng.guo@intel.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org 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). > > Signed-off-by: Beilei Xing > Signed-off-by: Xiaoyun Li > Signed-off-by: Xiao Wang > Signed-off-by: Junfeng Guo > --- > 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 > +#include > +#include > +#include > +#include > +#include > + > +#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. [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. > + 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? > +) > + > +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. > 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? > 'igc', > 'ionic', > 'ipn3ke',