From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id CBA3154AE for ; Wed, 6 Mar 2019 13:44:33 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga004.jf.intel.com ([10.7.209.38]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 06 Mar 2019 04:44:32 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.58,447,1544515200"; d="scan'208";a="280221761" Received: from fyigit-mobl.ger.corp.intel.com (HELO [10.252.14.113]) ([10.252.14.113]) by orsmga004.jf.intel.com with ESMTP; 06 Mar 2019 04:44:29 -0800 To: Rosen Xu , dev@dpdk.org Cc: tianfei.zhang@intel.com, dan.wei@intel.com, andy.pei@intel.com, qiming.yang@intel.com, haiyue.wang@intel.com, santos.chen@intel.com, zhang.zhang@intel.com References: <1551338000-120348-1-git-send-email-rosen.xu@intel.com> <1551338000-120348-6-git-send-email-rosen.xu@intel.com> From: Ferruh Yigit Openpgp: preference=signencrypt Autocrypt: addr=ferruh.yigit@intel.com; prefer-encrypt=mutual; keydata= mQINBFXZCFABEADCujshBOAaqPZpwShdkzkyGpJ15lmxiSr3jVMqOtQS/sB3FYLT0/d3+bvy qbL9YnlbPyRvZfnP3pXiKwkRoR1RJwEo2BOf6hxdzTmLRtGtwWzI9MwrUPj6n/ldiD58VAGQ +iR1I/z9UBUN/ZMksElA2D7Jgg7vZ78iKwNnd+vLBD6I61kVrZ45Vjo3r+pPOByUBXOUlxp9 GWEKKIrJ4eogqkVNSixN16VYK7xR+5OUkBYUO+sE6etSxCr7BahMPKxH+XPlZZjKrxciaWQb +dElz3Ab4Opl+ZT/bK2huX+W+NJBEBVzjTkhjSTjcyRdxvS1gwWRuXqAml/sh+KQjPV1PPHF YK5LcqLkle+OKTCa82OvUb7cr+ALxATIZXQkgmn+zFT8UzSS3aiBBohg3BtbTIWy51jNlYdy ezUZ4UxKSsFuUTPt+JjHQBvF7WKbmNGS3fCid5Iag4tWOfZoqiCNzxApkVugltxoc6rG2TyX CmI2rP0mQ0GOsGXA3+3c1MCdQFzdIn/5tLBZyKy4F54UFo35eOX8/g7OaE+xrgY/4bZjpxC1 1pd66AAtKb3aNXpHvIfkVV6NYloo52H+FUE5ZDPNCGD0/btFGPWmWRmkPybzColTy7fmPaGz cBcEEqHK4T0aY4UJmE7Ylvg255Kz7s6wGZe6IR3N0cKNv++O7QARAQABtCVGZXJydWggWWln aXQgPGZlcnJ1aC55aWdpdEBpbnRlbC5jb20+iQJVBBMBAgA/AhsDBgsJCAcDAgYVCAIJCgsE FgIDAQIeAQIXgBYhBNI2U4dCLsKE45mBx/kz60PfE2EfBQJbughWBQkHwjOGAAoJEPkz60Pf E2Eft84QAIbKWqhgqRfoiw/BbXbA1+qm2o4UgkCRQ0yJgt9QsnbpOmPKydHH0ixCliNz1J8e mRXCkMini1bTpnzp7spOjQGLeAFkNFz6BMq8YF2mVWbGEDE9WgnAxZdi0eLY7ZQnHbE6AxKL SXmpe9INb6z3ztseFt7mqje/W/6DWYIMnH3Yz9KzxujFWDcq8UCAvPkxVQXLTMpauhFgYeEx Nub5HbvhxTfUkapLwRQsSd/HbywzqZ3s/bbYMjj5JO3tgMiM9g9HOjv1G2f1dQjHi5YQiTZl 1eIIqQ3pTic6ROaiZqNmQFXPsoOOFfXF8nN2zg8kl/sSdoXWHhama5hbwwtl1vdaygQYlmdK H2ueiFh/UvT3WG3waNv2eZiEbHV8Rk52Xyn2w1G90lV0fYC6Ket1Xjoch7kjwbx793Kz/RfQ rmBY8/S4DTGn3oq3dMdQY+b6+7VMUeLMMh2CXYO9ErkOq+qNTD1IY+cBAkXnaDbQfz0zbste ZGWH74FAZ9nCpDOqbRTrBL42aMGhfOWEyeA1x7+hl6JZfabBWAuf4nnCXuorKHzBXTrf7u7p fXsKQClWRW77PF1VmzrtKNVSytQAmlCWApQIw20AarFipXmVdIjHmJPU611WoyxZPb4JTOxx 5cv9B+nr/RIB+v5dcStyHCCwO1be7nBDdCgd4F6kTQPLuQINBFfWTL4BEACnNA29e8TarUsB L5n6eLZHXcFvVwNLVlirWOClHXf44o2KnN3ww+eBEmKVfEFo9MSuGDNHS8Zw1NiGMYxLIUgd U6gGrVVs/VrQWL82pbMk6jCj98N+BXIri+6K1z+AImz7ax7iF1kDgRAnFWU0znWWBgM2mM8Y gDjcxfXk4sCKnvf6Gjo08Ey5zmqx7dekAKU2EEp8Q1EJY3jbymLdZWRP4AFFMTS1rGMk0/tt v71NBg1GobCcbNfn9chK/jhqxYhAJqq86RdJQkt3/9x1U1Oq0vXCt4JVVHmkxePtUiuWTTt+ aYlUAsKYZsWvncExvw77x2ArYDmaK0yfjh37wp0lY7DOJHFxoyT8tyWZlLci/VMRG2Ja33xj 0CN4C1yBg+QDeV3QFxQo42iA/ykdXPUR3ezmsND3XKvVLTC4DNb3V/EZQ7jBj64+bEK0VW4G B31VP00ApNQvSoczsIOAKdk97RNbpmPw6q10ILIB+9T1xbnFYzshzGF17oC0/GENIHATx8vZ masOZoDiOZQpeneLgnFE9JfzhLTxv6wNZcc/HLXRQVTkDsQr8ERtkAoHCf1E5+b5Yr7pfnE4 YuhET746o25S53ELUYPIs49qoJsEJL34/oexMfPGyPIlrbufiNyty5jc/1MRwUlhJlJ5IOHy ZUa+6CLR7GdImusFkPJUJwARAQABiQI8BBgBAgAmAhsMFiEE0jZTh0IuwoTjmYHH+TPrQ98T YR8FAlu6CHAFCQXE7zIACgkQ+TPrQ98TYR9nXxAAqNBgkYNyGuWUuy0GwDQCbu3iiMyH1+D7 llafPcK4NYy1Z4AYuVwC9nmLaoj+ozdqS3ncRo57ncRsKEJC46nDJJZYZ5LSJVn63Y3NBF86 lxQAgjj2oyZEwaLKtKbAFsXL43jv1pUGgSvWwYtDwHITXXFQto9rZEuUDRFSx4sg9OR+Q6/6 LY+nQQ3OdHlBkflzYMPcWgDcvcTAO6yasLEUf7UcYoSWTyMYjLB4QuNlXzTswzGVMssJF/vo V8lD1eqqaSUWG3STF6GVLQOr1NLvN5+kUBiEStHFxBpgSCvYY9sNV8FS6N24CAWMBl+10W+D 2h1yiiP5dOdPcBDYKsgqDD91/sP0WdyMJkwdQJtD49f9f+lYloxHnSAxMleOpyscg1pldw+i mPaUY1bmIknLhhkqfMmjywQOXpac5LRMibAAYkcB8v7y3kwELnt8mhqqZy6LUsqcWygNbH/W K3GGt5tRpeIXeJ25x8gg5EBQ0Jnvp/IbBYQfPLtXH0Myq2QuAhk/1q2yEIbVjS+7iowEZNyE 56K63WBJxsJPB2mvmLgn98GqB4G6GufP1ndS0XDti/2K0o8rep9xoY/JDGi0n0L0tk9BHyoP Y7kaEpu7UyY3nVdRLe5H1/MnFG8hdJ97WqnPS0buYZlrbTV0nRFL/NI2VABl18vEEXvNQiO+ vM8= Message-ID: Date: Wed, 6 Mar 2019 12:44:28 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101 Thunderbird/60.5.2 MIME-Version: 1.0 In-Reply-To: <1551338000-120348-6-git-send-email-rosen.xu@intel.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH v1 05/11] drivers/net/ipn3ke: add IPN3KE PMD driver X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 06 Mar 2019 12:44:34 -0000 On 2/28/2019 7:13 AM, Rosen Xu wrote: > Add Intel FPGA Acceleration NIC IPN3KE PMD driver. > > Signed-off-by: Rosen Xu > Signed-off-by: Andy Pei > Signed-off-by: Dan Wei > --- > drivers/net/Makefile | 1 + > drivers/net/ipn3ke/Makefile | 33 + > drivers/net/ipn3ke/ipn3ke_ethdev.c | 814 +++++++++ > drivers/net/ipn3ke/ipn3ke_ethdev.h | 742 +++++++++ > drivers/net/ipn3ke/ipn3ke_flow.c | 1407 ++++++++++++++++ > drivers/net/ipn3ke/ipn3ke_flow.h | 104 ++ > drivers/net/ipn3ke/ipn3ke_logs.h | 30 + > drivers/net/ipn3ke/ipn3ke_representor.c | 890 ++++++++++ > drivers/net/ipn3ke/ipn3ke_tm.c | 2217 +++++++++++++++++++++++++ > drivers/net/ipn3ke/ipn3ke_tm.h | 135 ++ > drivers/net/ipn3ke/meson.build | 9 + > drivers/net/ipn3ke/rte_pmd_ipn3ke_version.map | 4 + Can you please split this patch into multiple patch, at least I think ethdev, flow support and tm support can be seperated. <...> > @@ -32,6 +32,7 @@ DIRS-$(CONFIG_RTE_LIBRTE_FM10K_PMD) += fm10k > DIRS-$(CONFIG_RTE_LIBRTE_I40E_PMD) += i40e > DIRS-$(CONFIG_RTE_LIBRTE_ICE_PMD) += ice > DIRS-$(CONFIG_RTE_LIBRTE_IXGBE_PMD) += ixgbe > +DIRS-$(CONFIG_RTE_LIBRTE_IPN3KE_PMD) += ipn3ke Can you please add alphatecally sorted, you are almost there, but not quite J <...> > @@ -0,0 +1,33 @@ > +# SPDX-License-Identifier: BSD-3-Clause > +# Copyright(c) 2019 Intel Corporation > + > +include $(RTE_SDK)/mk/rte.vars.mk > + > +# > +# library name > +# > +LIB = librte_pmd_ipn3ke.a > + > +CFLAGS += -DALLOW_EXPERIMENTAL_API Can you please add the experimenatal APIs called from this PMD as comments here, that can help a lot to trace when to remove the flag, or it is really required? > +CFLAGS += -O3 > +#CFLAGS += $(WERROR_FLAGS) > +CFLAGS += -I$(RTE_SDK)/drivers/bus/ifpga > +CFLAGS += -I$(RTE_SDK)/drivers/raw/ifpga_rawdev > +LDLIBS += -lrte_eal -lrte_mbuf -lrte_mempool -lrte_ring > +LDLIBS += -lrte_ethdev -lrte_net -lrte_kvargs > +LDLIBS += -lrte_bus_pci > +LDLIBS += -lrte_bus_vdev Can you please double check if you really have a dependency to both pci and vdev? And don't you need bus_ifpga ? > + > +EXPORT_MAP := rte_pmd_ipn3ke_version.map > + > +LIBABIVER := 1 > + > +# > +# all source are stored in SRCS-y > +# > +SRCS-y += ipn3ke_ethdev.c They are same thing, but for consistency can you please use: SRC-$(CONFIG_RTE_LIBRTE_IPN3KE_PMD) += ipn3ke_ethdev.c <...> > +static const struct rte_afu_uuid afu_uuid_ipn3ke_map[] = { > + { MAP_UUID_10G_LOW, MAP_UUID_10G_HIGH }, > + { IPN3KE_UUID_10G_LOW, IPN3KE_UUID_10G_HIGH }, > + { IPN3KE_UUID_25G_LOW, IPN3KE_UUID_25G_HIGH }, > + { 0, 0 /* sentinel */ }, > +}; > + > +struct ipn3ke_hw_cap hw_cap; Do you need this forward decleration? "ipn3ke_ethdev.h" is already included. > + > +static int ipn3ke_indirect_read(struct ipn3ke_hw *hw, > + uint32_t *rd_data, > + uint32_t addr, > + uint32_t mac_num, > + uint32_t dev_sel, > + uint32_t eth_wrapper_sel) According our coding convention, it should be like: static int ipn3ke_indirect_read(struct ipn3ke_hw *hw, uint32_t *rd_data, uint32_t addr, uint32_t mac_num, uint32_t dev_sel, uint32_t eth_wrapper_sel) Since this is a new file, why now follow the project coding convention? https://doc.dpdk.org/guides/contributing/coding_style.html <...> > +static int > +ipn3ke_hw_init_vbng(struct rte_afu_device *afu_dev, > + struct ipn3ke_hw *hw) > +{ > + struct rte_rawdev *rawdev; > + int ret; > + int i; > + uint32_t val; > + > + rawdev = afu_dev->rawdev; > + > + hw->afu_id.uuid.uuid_low = afu_dev->id.uuid.uuid_low; > + hw->afu_id.uuid.uuid_high = afu_dev->id.uuid.uuid_high; > + hw->afu_id.port = afu_dev->id.port; > + hw->hw_addr = (uint8_t *)(afu_dev->mem_resource[0].addr); > + if ((afu_dev->id.uuid.uuid_low == MAP_UUID_10G_LOW) && > + (afu_dev->id.uuid.uuid_high == MAP_UUID_10G_HIGH)) { > + hw->f_mac_read = map_indirect_mac_read; > + hw->f_mac_write = map_indirect_mac_write; > + } else { > + hw->f_mac_read = ipn3ke_indirect_mac_read; > + hw->f_mac_write = ipn3ke_indirect_mac_write; > + } > + hw->rawdev = rawdev; > + rawdev->dev_ops->attr_get(rawdev, > + "retimer_info", > + (uint64_t *)&hw->retimer); What do you think casting to "uintptr_t" instead of "uint64_t *" ? <...> > + ret = rte_eth_switch_domain_alloc(&hw->switch_domain_id); > + if (ret) > + IPN3KE_AFU_PMD_WARN("failed to allocate switch domain for device %d", > + ret); > + > + ret = ipn3ke_hw_tm_init(hw); function is defined in another .c file, need to declared for the usage, otherwise causing build warning: .../drivers/net/ipn3ke/ipn3ke_ethdev.c: In function ‘ipn3ke_hw_init_vbng’: .../drivers/net/ipn3ke/ipn3ke_ethdev.c:443:8: warning: implicit declaration of function ‘ipn3ke_hw_tm_init’; did you mean ‘ipn3ke_hw_cap_init’? [-Wimplicit-function-declaration] ret = ipn3ke_hw_tm_init(hw); ^~~~~~~~~~~~~~~~~ <...> > + > +RTE_INIT(ipn3ke_afu_init_log); > +static void > +ipn3ke_afu_init_log(void) Can combine both: RTE_INIT(ipn3ke_afu_init_log) { Just for consistency, can you move this function to the buttom of the file? > +{ > + ipn3ke_afu_logtype = rte_log_register("driver.afu.ipn3ke"); "pmd.net.ipn3ke" <...> > + if (!hw) { > + IPN3KE_AFU_PMD_LOG(ERR, > + "failed to allocate hardwart data"); > + retval = -ENOMEM; > + return -ENOMEM; There is already 'IPN3KE_AFU_PMD_ERR' and variant macros defined if you prefer to use. > + } > + afu_dev->shared.data = hw; > + > + rte_spinlock_init(&afu_dev->shared.lock); Getting following warning with clang [1], worth chekcing, is 'struct rte_afu_device' really needs to be packed? [1] .../dpdk/drivers/net/ipn3ke/ipn3ke_ethdev.c:536:22: warning: taking address of packed member 'shared' of class or structure 'rte_afu_device' may result in an unaligned pointer value [-Waddress-of-pack ed-member] rte_spinlock_init(&afu_dev->shared.lock); ^~~~~~~~~~~~~~~~~~~~ > + } else > + hw = (struct ipn3ke_hw *)afu_dev->shared.data; > + > +#if IPN3KE_HW_BASE_ENABLE > + retval = ipn3ke_hw_init_base(afu_dev, hw); > + if (retval) > + return retval; > +#endif Is this macro defined anywhere? It looks like not, so is it used like "#if 0", if so removed the macro and the wrapped code please. > + > + retval = ipn3ke_hw_init_vbng(afu_dev, hw); > + if (retval) > + return retval; > + > + /* probe representor ports */ > + for (i = 0; i < hw->port_num; i++) { > + struct ipn3ke_rpst rpst = { > + .port_id = i, > + .switch_domain_id = hw->switch_domain_id, > + .hw = hw > + }; > + > + /* representor port net_bdf_port */ > + snprintf(name, sizeof(name), "net_%s_representor_%d", > + afu_dev->device.name, i); > + > + retval = rte_eth_dev_create(&afu_dev->device, name, > + sizeof(struct ipn3ke_rpst), NULL, NULL, > + ipn3ke_rpst_init, &rpst); Similar allignment warning as above: .../dpdk/drivers/net/ipn3ke/ipn3ke_ethdev.c:562:32: warning: taking address of packed member 'device' of class or structure 'rte_afu_device' may result in an unaligned pointer value [-Waddress-of-pac$ ed-member] retval = rte_eth_dev_create(&afu_dev->device, name, ^~~~~~~~~~~~~~~ > + > + if (retval) > + IPN3KE_AFU_PMD_LOG(ERR, "failed to create ipn3ke " > + "representor %s.", name); No need to split the message. <...> > + ret = rte_eth_switch_domain_free(hw->switch_domain_id); > + if (ret) > + IPN3KE_AFU_PMD_LOG(WARNING, > + "failed to free switch domain: %d", > + ret); Please fix the indentation. > + > + /* flow uninit*/> + > + ipn3ke_hw_uninit(hw); Is the comment for "hw_uninit" ? > + > + return 0; > +} > + > +static struct rte_afu_driver afu_ipn3ke_driver = { > + .id_table = afu_uuid_ipn3ke_map, > + .probe = ipn3ke_vswitch_probe, > + .remove = ipn3ke_vswitch_remove, > +}; > + > +RTE_PMD_REGISTER_AFU(net_ipn3ke_afu, afu_ipn3ke_driver); So this file is has two drivers, one vdev driver and one afu driver. Is vdev one only to get the device arguments? If so why not get device arguments directly to afu driver and remove the vdev device/driver? And since this is an AFU driver, should we have a drivers/ifpga/* folder and put these drivers in it? What do you think? > +RTE_PMD_REGISTER_AFU_ALIAS(net_ipn3ke_afu, afu_dev); Isn't 'afu_dev' so genereic for being alias? Also are you taking 'alias' field into account? I checked the ifpga bus code but not able to find, can you please double check? Did you test this alias? > +RTE_PMD_REGISTER_PARAM_STRING(net_ipn3ke_afu, > + "bdf= " > + "port= " > + "uudi_high= " > + "uuid_low= " > + "path= " > + "pr_enable=" > + "debug="); Are these the net driver device arguments? Where they are defined/parsed/used? > + > +static const char * const valid_args[] = { > +#define IPN3KE_AFU_NAME "afu" > + IPN3KE_AFU_NAME, > +#define IPN3KE_FPGA_ACCELERATION_LIST "fpga_acc" > + IPN3KE_FPGA_ACCELERATION_LIST, > +#define IPN3KE_I40E_PF_LIST "i40e_pf" > + IPN3KE_I40E_PF_LIST, > + NULL > +}; > +static int > +ipn3ke_cfg_parse_acc_list(const char *afu_name, > +const char *acc_list_name) Please fix the indentation. <...> > + if (rte_kvargs_count(kvlist, IPN3KE_FPGA_ACCELERATION_LIST) == 1) { > + if (rte_kvargs_process(kvlist, IPN3KE_FPGA_ACCELERATION_LIST, > + &rte_ifpga_get_string_arg, > + &acc_name) < 0) { > + IPN3KE_AFU_PMD_ERR("error to parse %s", > + IPN3KE_FPGA_ACCELERATION_LIST); > + goto end; > + } > + ret = ipn3ke_cfg_parse_acc_list(afu_name, acc_name); > + if (ret) > + goto end; > + } else { > + IPN3KE_AFU_PMD_INFO("arg %s is optional for ipn3ke, using i40e acc", > + IPN3KE_FPGA_ACCELERATION_LIST); > + } > + > + if (rte_kvargs_count(kvlist, IPN3KE_I40E_PF_LIST) == 1) { > + if (rte_kvargs_process(kvlist, IPN3KE_I40E_PF_LIST, > + &rte_ifpga_get_string_arg, > + &pf_name) < 0) { > + IPN3KE_AFU_PMD_ERR("error to parse %s", > + IPN3KE_I40E_PF_LIST); > + goto end; > + } > + ret = ipn3ke_cfg_parse_i40e_pf_ethdev(afu_name, pf_name); You shouldn't rely on to the order of the devargs, what if 'afu_name' is not provided yet when 'pf_name' arg is provided? Same concern for above 'ipn3ke_cfg_parse_acc_list()'. It is better to get all the args first, later call the proper APIs. <...> > +static int > +ipn3ke_cfg_remove(struct rte_vdev_device *vdev) > +{ > + IPN3KE_AFU_PMD_INFO("Remove ipn3ke_cfg %p", > + vdev); > + > + return 0; Nothing to be done for remove? > +} > + > +static struct rte_vdev_driver ipn3ke_cfg_driver = { > + .probe = ipn3ke_cfg_probe, > + .remove = ipn3ke_cfg_remove, > +}; > + > +RTE_PMD_REGISTER_VDEV(ipn3ke_cfg, ipn3ke_cfg_driver); > +RTE_PMD_REGISTER_ALIAS(ipn3ke_cfg, ipn3ke_cfg); > +RTE_PMD_REGISTER_PARAM_STRING(ipn3ke_cfg, > + "afu= " > + "fpga_acc=" > + "i40e="); Isn't this "i40e_pf", using defined macros can prevent mistakes. <...> > +static inline uint32_t _ipn3ke_indrct_read(struct ipn3ke_hw *hw, > + uint32_t addr) > +{ > + uint64_t word_offset = 0; > + uint64_t read_data = 0; > + uint64_t indirect_value = 0; > + volatile void *indirect_addrs = 0; > + > + word_offset = (addr & 0x1FFFFFF) >> 2; > + indirect_value = RCMD | word_offset << 32; > + indirect_addrs = (volatile void *)(hw->hw_addr + > + (uint32_t)(UPL_BASE | 0x10)); > + > + usleep(10); Required head for usleep seems not included, causing build warning: In file included from .../drivers/net/ipn3ke/ipn3ke_representor.c:27: .../drivers/net/ipn3ke/ipn3ke_ethdev.h: In function ‘_ipn3ke_indrct_read’: .../drivers/net/ipn3ke/ipn3ke_ethdev.h:220:2: warning: implicit declaration of function ‘usleep’; did you mean ‘fseek’? [-Wimplicit-function-declaration] usleep(10); ^~~~~~ Same warning for all occurances. <...> > +static int > +ipn3ke_flow_hw_update(struct ipn3ke_hw *hw, > + struct rte_flow *flow, uint32_t is_add) > +{ > + uint32_t *pdata = NULL; > + uint32_t data; > + uint32_t time_out = MHL_COMMAND_TIME_COUNT; > + > +#ifdef DEBUG_IPN3KE_FLOW In many places in this patchset, there are define which are not part of PMD configuration. So most probably need to enable/disable them manually in the code. This approach is very open the errors. The code in the ifdef block is escaped from all compilers, when someone needs it, it is common that they even don't compile and work. I suggest removing them all. If a define is really necessary add it as PMD config option. > + uint32_t i; > + > + printf("IPN3KE flow dump\n"); > + > + pdata = (uint32_t *)flow->rule.key; > + printf(" - key :"); > + > + for (i = 0; i < RTE_DIM(flow->rule.key); i++) > + printf(" %02x", flow->rule.key[i]); > + > + for (i = 0; i < 4; i++) > + printf(" %02x", __SWAP32(pdata[3 - i])); > + printf("\n"); > + > + pdata = (uint32_t *)flow->rule.result; > + printf(" - result:"); > + > + for (i = 0; i < RTE_DIM(flow->rule.result); i++) > + printf(" %02x", flow->rule.result[i]); > + > + for (i = 0; i < 1; i++) > + printf(" %02x", pdata[i]); > + printf("\n"); > +#endif > + > + pdata = (uint32_t *)flow->rule.key; > + > + IPN3KE_MASK_WRITE_REG(hw, > + IPN3KE_CLF_MHL_KEY_0, > + 0, > + __SWAP32(pdata[3]), > + IPN3KE_CLF_MHL_KEY_MASK); > + > + IPN3KE_MASK_WRITE_REG(hw, > + IPN3KE_CLF_MHL_KEY_1, > + 0, > + __SWAP32(pdata[2]), > + IPN3KE_CLF_MHL_KEY_MASK); > + > + IPN3KE_MASK_WRITE_REG(hw, > + IPN3KE_CLF_MHL_KEY_2, > + 0, > + __SWAP32(pdata[1]), > + IPN3KE_CLF_MHL_KEY_MASK); > + > + IPN3KE_MASK_WRITE_REG(hw, > + IPN3KE_CLF_MHL_KEY_3, > + 0, > + __SWAP32(pdata[0]), > + IPN3KE_CLF_MHL_KEY_MASK); > + > + pdata = (uint32_t *)flow->rule.result; > + IPN3KE_MASK_WRITE_REG(hw, > + IPN3KE_CLF_MHL_RES, > + 0, > + __SWAP32(pdata[0]), > + IPN3KE_CLF_MHL_RES_MASK); > + > + /* insert/delete the key and result */ > + data = 0; > + data = IPN3KE_MASK_READ_REG(hw, > + IPN3KE_CLF_MHL_MGMT_CTRL, > + 0, > + 0x80000000); > + time_out = MHL_COMMAND_TIME_COUNT; > + while (IPN3KE_BIT_ISSET(data, IPN3KE_CLF_MHL_MGMT_CTRL_BIT_BUSY) && > + (time_out > 0)) { > + data = IPN3KE_MASK_READ_REG(hw, > + IPN3KE_CLF_MHL_MGMT_CTRL, > + 0, > + 0x80000000); > + time_out--; > + rte_delay_us(MHL_COMMAND_TIME_INTERVAL_US); This is giving missing decleration warning for cross compile [1], most probably a heder is missing. [1] .../drivers/net/ipn3ke/ipn3ke_flow.c:1084:3: warning: implicit declaration of function ‘rte_delay_us’; did you mean ‘rte_realloc’? [-Wimplicit-function-declaration] rte_delay_us(MHL_COMMAND_TIME_INTERVAL_US); ^~~~~~~~~~~~ <...> > +static int > +ipn3ke_retimer_conf_link(struct rte_rawdev *rawdev, > + uint16_t port, > + uint8_t force_speed, > + bool is_up) > +{ > + struct ifpga_rawdevg_link_info linfo; > + > + linfo.port = port; > + linfo.link_up = is_up; > + linfo.link_speed = force_speed; > + > + return rawdev->dev_ops->attr_set(rawdev, > + "retimer_linkstatus", > + (uint64_t)&linfo); You are saving the pointer of a local variable converting it to uint64_t. Can you please elaborate why link info needs to saved as attribute? And when it is retrieved back will the memory be still valid? > +} > + > +static void > +ipn3ke_update_link(struct rte_rawdev *rawdev, > + uint16_t port, > + struct rte_eth_link *link) > +{ > + struct ifpga_rawdevg_link_info linfo; > + > + rawdev->dev_ops->attr_get(rawdev, > + "retimer_linkstatus", > + (uint64_t *)&linfo); > + /* Parse the link status */ > + link->link_status = linfo.link_up; > + switch (linfo.link_speed) { > + case IFPGA_RAWDEV_LINK_SPEED_10GB: > + link->link_speed = ETH_SPEED_NUM_10G; > + break; > + case IFPGA_RAWDEV_LINK_SPEED_25GB: > + link->link_speed = ETH_SPEED_NUM_25G; > + break; > + default: > + IPN3KE_AFU_PMD_LOG(ERR, "Unknown link speed info %u", > + linfo.link_speed); > + break; > + } > +} > + > +/* > + * Set device link up. > + */ > +int > +ipn3ke_rpst_dev_set_link_up(struct rte_eth_dev *dev) > +{ > + uint8_t link_speed = IFPGA_RAWDEV_LINK_SPEED_UNKNOWN; > + struct ipn3ke_hw *hw = IPN3KE_DEV_PRIVATE_TO_HW(dev); > + struct ipn3ke_rpst *rpst = IPN3KE_DEV_PRIVATE_TO_RPST(dev); > + struct rte_rawdev *rawdev; > + struct rte_eth_conf *conf = &dev->data->dev_conf; > + > + rawdev = hw->rawdev; > + if (conf->link_speeds == ETH_LINK_SPEED_AUTONEG) { > + conf->link_speeds = ETH_LINK_SPEED_25G | > + ETH_LINK_SPEED_10G; > + } > + > + if (conf->link_speeds & ETH_LINK_SPEED_10G) > + link_speed = IFPGA_RAWDEV_LINK_SPEED_25GB; > + else if (conf->link_speeds & ETH_LINK_SPEED_25G) > + link_speed = IFPGA_RAWDEV_LINK_SPEED_10GB; > + else > + link_speed = IFPGA_RAWDEV_LINK_SPEED_UNKNOWN; > + > + if (rpst->i40e_pf_eth) > + rte_eth_dev_set_link_up(rpst->i40e_pf_eth_port_id); > + > + return ipn3ke_retimer_conf_link(rawdev, > + rpst->port_id, > + link_speed, > + true); Just a reminder that this is not doing anything useful, although it looks like . 'ipn3ke_retimer_conf_link()' calls 'rawdev->dev_ops->attr_set()' which doesn't do anything functional. <...> > +struct eth_dev_ops ipn3ke_rpst_dev_ops = { > + .dev_infos_get = ipn3ke_rpst_dev_infos_get, > + > + .dev_configure = ipn3ke_rpst_dev_configure, > + .dev_start = ipn3ke_rpst_dev_start, > + .dev_stop = ipn3ke_rpst_dev_stop, > + .dev_close = ipn3ke_rpst_dev_close, > + .dev_reset = ipn3ke_rpst_dev_reset, > + > + .stats_get = ipn3ke_rpst_stats_get, > + .xstats_get = ipn3ke_rpst_xstats_get, > + .xstats_get_names = ipn3ke_rpst_xstats_get_names, > + .stats_reset = ipn3ke_rpst_stats_reset, > + .xstats_reset = ipn3ke_rpst_stats_reset, > + > + .filter_ctrl = ipn3ke_afu_filter_ctrl, > + > + .rx_queue_start = ipn3ke_rpst_rx_queue_start, > + .rx_queue_stop = ipn3ke_rpst_rx_queue_stop, > + .tx_queue_start = ipn3ke_rpst_tx_queue_start, > + .tx_queue_stop = ipn3ke_rpst_tx_queue_stop, > + .rx_queue_setup = ipn3ke_rpst_rx_queue_setup, > + .rx_queue_release = ipn3ke_rpst_rx_queue_release, > + .tx_queue_setup = ipn3ke_rpst_tx_queue_setup, > + .tx_queue_release = ipn3ke_rpst_tx_queue_release, > + > + .dev_set_link_up = ipn3ke_rpst_dev_set_link_up, > + .dev_set_link_down = ipn3ke_rpst_dev_set_link_down, > + .link_update = ipn3ke_rpst_link_update, > + > + .promiscuous_enable = ipn3ke_rpst_promiscuous_enable, > + .promiscuous_disable = ipn3ke_rpst_promiscuous_disable, > + .allmulticast_enable = ipn3ke_rpst_allmulticast_enable, > + .allmulticast_disable = ipn3ke_rpst_allmulticast_disable, > + .mac_addr_set = ipn3ke_rpst_mac_addr_set, > + /*.get_reg = ipn3ke_get_regs,*/ > + .mtu_set = ipn3ke_rpst_mtu_set, > + > + /** > + * .rxq_info_get = ipn3ke_rxq_info_get, > + * .txq_info_get = ipn3ke_txq_info_get, > + * .fw_version_get = , > + * .get_module_info = ipn3ke_get_module_info, > + */ Please remove commented code. > + > + .tm_ops_get = ipn3ke_tm_ops_get, > +}; Is this understanding correct: These dev_ops are for representor ports and to control the ethernet ports via FPGA interface. If so can the ports be configured via regular device drivers? Is there possible conflict between two controls and is there any syncronization required? <...> > +static int > +ipn3ke_tm_hierarchy_commit_check(struct rte_eth_dev *dev, > + struct rte_tm_error *error) > +{ > + struct ipn3ke_tm_internals *tm = IPN3KE_DEV_PRIVATE_TO_TM(dev); > + uint32_t tm_id; > + struct ipn3ke_tm_node_list *nl; > + struct ipn3ke_tm_node *n, *parent_node; > + enum ipn3ke_tm_node_state node_state; > + > + tm_id = tm->tm_id; > + > + nl = &tm->h.cos_commit_node_list; > + TAILQ_FOREACH(n, nl, node) { > + node_state = n->node_state; > + parent_node = n->parent_node; > + if (n->node_state == IPN3KE_TM_NODE_STATE_CONFIGURED_ADD) { > + if ((n->parent_node_id == RTE_TM_NODE_ID_NULL) || > + (n->level != IPN3KE_TM_NODE_LEVEL_COS) || > + (n->tm_id != tm_id) || > + (parent_node == NULL) || > + (parent_node && > + (parent_node->node_state == > + IPN3KE_TM_NODE_STATE_CONFIGURED_DEL)) || > + (parent_node && > + (parent_node->node_state == > + IPN3KE_TM_NODE_STATE_IDLE)) || > + (n->shaper_profile.valid == 0)) > + return -rte_tm_error_set(error, > + EINVAL, > + RTE_TM_ERROR_TYPE_UNSPECIFIED, > + NULL, > + rte_strerror(EINVAL)); > + } else if (n->node_state == IPN3KE_TM_NODE_STATE_CONFIGURED_DEL) > + if ((n->level != IPN3KE_TM_NODE_LEVEL_COS) || > + (n->n_children != 0)) > + return -rte_tm_error_set(error, > + EINVAL, > + RTE_TM_ERROR_TYPE_UNSPECIFIED, > + NULL, > + rte_strerror(EINVAL)); > + else > + return -rte_tm_error_set(error, > + EINVAL, > + RTE_TM_ERROR_TYPE_UNSPECIFIED, > + NULL, > + rte_strerror(EINVAL)); It is easy to confuse last 'else', also I belive its indentation is wrong, so it may be already confused, can you please use braces to clarify, related clang warning: ../dpdk/drivers/net/ipn3ke/ipn3ke_tm.c:1551:3: warning: add explicit braces to avoid dangling else [-Wdangling-else] else ^ > + } > + > + nl = &tm->h.vt_commit_node_list; > + TAILQ_FOREACH(n, nl, node) { > + node_state = n->node_state; > + parent_node = n->parent_node; > + if (n->node_state == IPN3KE_TM_NODE_STATE_CONFIGURED_ADD) { > + if ((n->parent_node_id == RTE_TM_NODE_ID_NULL) || > + (n->level != IPN3KE_TM_NODE_LEVEL_VT) || > + (n->tm_id != tm_id) || > + (parent_node == NULL) || > + (parent_node && > + (parent_node->node_state == > + IPN3KE_TM_NODE_STATE_CONFIGURED_DEL)) || > + (parent_node && > + (parent_node->node_state == > + IPN3KE_TM_NODE_STATE_IDLE)) || > + (n->shaper_profile.valid == 0)) > + return -rte_tm_error_set(error, > + EINVAL, > + RTE_TM_ERROR_TYPE_UNSPECIFIED, > + NULL, > + rte_strerror(EINVAL)); > + } else if (n->node_state == IPN3KE_TM_NODE_STATE_CONFIGURED_DEL) > + if ((n->level != IPN3KE_TM_NODE_LEVEL_VT) || > + (n->n_children != 0)) > + return -rte_tm_error_set(error, > + EINVAL, > + RTE_TM_ERROR_TYPE_UNSPECIFIED, > + NULL, > + rte_strerror(EINVAL)); > + else > + return -rte_tm_error_set(error, > + EINVAL, > + RTE_TM_ERROR_TYPE_UNSPECIFIED, > + NULL, > + rte_strerror(EINVAL)); Same as above for this 'else', clang warning: .../dpdk/drivers/net/ipn3ke/ipn3ke_tm.c:1588:3: warning: add explicit braces to avoid dangling else [-Wdangling-else] else ^ <...> > +static int > +ipn3ke_tm_hierarchy_hw_commit(struct rte_eth_dev *dev, > + struct rte_tm_error *error) > +{ > + struct ipn3ke_hw *hw = IPN3KE_DEV_PRIVATE_TO_HW(dev); > + struct ipn3ke_tm_internals *tm = IPN3KE_DEV_PRIVATE_TO_TM(dev); > + struct ipn3ke_tm_node_list *nl; > + struct ipn3ke_tm_node *n, *nn, *parent_node; > + > + n = tm->h.port_commit_node; > + if (n) { > + if (n->node_state == IPN3KE_TM_NODE_STATE_CONFIGURED_ADD) { > + tm->h.port_commit_node = NULL; > + > + n->node_state = IPN3KE_TM_NODE_STATE_COMMITTED; > + } else if (n->node_state == > + IPN3KE_TM_NODE_STATE_CONFIGURED_DEL) { > + tm->h.port_commit_node = NULL; > + > + n->node_state = IPN3KE_TM_NODE_STATE_IDLE; > + n->priority = IPN3KE_TM_NODE_PRIORITY_NORMAL0; > + n->weight = 0; > + n->tm_id = RTE_TM_NODE_ID_NULL; > + } else > + return -rte_tm_error_set(error, > + EINVAL, > + RTE_TM_ERROR_TYPE_UNSPECIFIED, > + NULL, > + rte_strerror(EINVAL)); > + ipn3ke_hw_tm_node_wr(hw, n); > + } > + > + nl = &tm->h.vt_commit_node_list; > + for (n = TAILQ_FIRST(nl); n; nn) { what is the point of last 'nn'? clang warning: .../dpdk/drivers/net/ipn3ke/ipn3ke_tm.c:1811:31: warning: expression result unused [-Wunused-value] for (n = TAILQ_FIRST(nl); n; nn) { ^~ there are multiple similar usage, am I missing something, if not can you please clean them: .../dpdk/drivers/net/ipn3ke/ipn3ke_tm.c:1841:31: warning: expression result unused [-Wunused-value] .../dpdk/drivers/net/ipn3ke/ipn3ke_tm.c:1897:31: warning: expression result unused [-Wunused-value] .../dpdk/drivers/net/ipn3ke/ipn3ke_tm.c:1914:31: warning: expression result unused [-Wunused-value] <...> > +static void > +ipn3ke_tm_show(struct rte_eth_dev *dev) > +{ > + struct ipn3ke_tm_internals *tm = IPN3KE_DEV_PRIVATE_TO_TM(dev); > + uint32_t tm_id; > + struct ipn3ke_tm_node_list *vt_nl, *cos_nl; > + struct ipn3ke_tm_node *port_n, *vt_n, *cos_n; > + char *str_state[IPN3KE_TM_NODE_STATE_MAX] = {"Idle", > + "CfgAdd", > + "CfgDel", > + "Committed"}; > + > + tm_id = tm->tm_id; > + > + printf("*************HQoS Tree(%d)*************\n", tm_id); Please don't use 'printf' in drivers and libs, instead prefer logging functions. <...> > +/* TM Levels */ > +enum ipn3ke_tm_node_level { > + IPN3KE_TM_NODE_LEVEL_PORT = 0, No need to provide initial '0' value to an enum, that is the default value. <...> > @@ -0,0 +1,4 @@ > +DPDK_2.0 { Please fix tag with correct DPDK version. > + > + local: *; > +}; >