Hi,
> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Wednesday, March 06, 2019 20:44
> To: Xu, Rosen <rosen...@intel.com>; dev@dpdk.org
> Cc: Zhang, Tianfei <tianfei.zh...@intel.com>; Wei, Dan
> <dan....@intel.com>; Pei, Andy <andy....@intel.com>; Yang, Qiming
> <qiming.y...@intel.com>; Wang, Haiyue <haiyue.w...@intel.com>; Chen,
> Santos <santos.c...@intel.com>; Zhang, Zhang <zhang.zh...@intel.com>
> Subject: Re: [PATCH v1 05/11] drivers/net/ipn3ke: add IPN3KE PMD driver
>
> On 2/28/2019 7:13 AM, Rosen Xu wrote:
> > Add Intel FPGA Acceleration NIC IPN3KE PMD driver.
> >
> > Signed-off-by: Rosen Xu <rosen...@intel.com>
> > Signed-off-by: Andy Pei <andy....@intel.com>
> > Signed-off-by: Dan Wei <dan....@intel.com>
> > ---
> > 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.
Good suggestion. I will apply it in patch v2.
> <...>
>
> > @@ -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
Okay.
> <...>
>
> > @@ -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?
Okay.
> > +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?
No dependency to pci, but we need vdev to take some configuration.
> And don't you need bus_ifpga ?
It need, I will apply it in patch v2.
> > +
> > +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
Okay, I will apply it in patch v2.
> <...>
>
> > +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.
hw_cap is only used in ipn3ke_ethdev.c, so I will remove the definition in
ipn3ke_ethdev.h
> > +
> > +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
Okay, absolutely follow coding convention.
>
> <...>
>
> > +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 *" ?
To fix compile warning and follow the definition of attr_get:
typedef int (*rawdev_get_attr_t)(struct rte_rawdev *dev,
const char *attr_name,
uint64_t *attr_value);
> <...>
>
> > + 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:
Okay, I will fix it in patch v2.
> .../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);
> ^~~~~~~~~~~~~~~~~
I will fix it in patch v2.
> <...>
>
> > +
> > +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?
Okay, I will fix it in patch v2.
> > +{
> > + 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.
I will replace it with 'IPN3KE_AFU_PMD_ERR'.
> > + }
> > + 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?
I will fix clang compile warning in patch v2.
> [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);
> ^~~~~~~~~~~~~~~~~~~~
I will fix it in patchv2.
> > + } 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.
This macro is just for debug with old FPGA bitstream, with newest bitstream we
don't need it.
So I will remove it in patch v2.
> > +
> > + 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,
> ^~~~~~~~~~~~~~~
I will fix it in patch v2.
>
> > +
> > + if (retval)
> > + IPN3KE_AFU_PMD_LOG(ERR, "failed to create
> ipn3ke "
> > + "representor %s.", name);
>
> No need to split the message.
Okay.
> <...>
>
> > + 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.
Okay.
> > +
> > + /* flow uninit*/> +
> > + ipn3ke_hw_uninit(hw);
>
> Is the comment for "hw_uninit" ?
Yes, I will fix it in patch v2, thanks.
> > +
> > + 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
Yes
> arguments directly to afu driver and remove the vdev device/driver?
Some special arguments for IPN3KE need to use the vdev to take configuration.
So we can't merge the new configuration vdev to afu afu 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?
From function point of view, it's a NIC driver, so we put it in net drivers
folder.
> > +RTE_PMD_REGISTER_AFU_ALIAS(net_ipn3ke_afu, afu_dev);
>
> Isn't 'afu_dev' so genereic for being alias?
No, I will remove it in patch v2. Thanks.
> 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?
This alias is no useful, I will remove it in patch v2. Thanks.
> > +RTE_PMD_REGISTER_PARAM_STRING(net_ipn3ke_afu,
> > + "bdf=<string> "
> > + "port=<int> "
> > + "uudi_high=<int64> "
> > + "uuid_low=<int64> "
> > + "path=<string> "
> > + "pr_enable=<int>"
> > + "debug=<int>");
No useful, I will remove it in patch v2. Thanks.
> Are these the net driver device arguments? Where they are
> defined/parsed/used?
No useful, I will remove it in patch v2. Thanks.
> > +
> > +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.
Okay.
> <...>
>
> > + 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?
Yes, I will fix order dependency in patch v2. But afu_name and pf_name are
mandatory.
> Same concern for above 'ipn3ke_cfg_parse_acc_list()'. It is better to get all
> the args first, later call the proper APIs.
Good suggestion, I will apply it in patch v2.
> <...>
>
> > +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?
I miss unbinding I40e and FPGA port mapping. I will fix it in patch v2.
> > +}
> > +
> > +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=<string> "
> > + "fpga_acc=<string>"
> > + "i40e=<string>");
>
> Isn't this "i40e_pf", using defined macros can prevent mistakes.
Okay, I will fix it in patch v2.
> <...>
>
> > +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);
> ^~~~~~
I will fix it in patch v2.
> Same warning for all occurances.
Apply in patch v2.
> <...>
>
> > +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.
Okay, apply in patch v2.
> If a define is really necessary add it as PMD config option.
Okay.
> > + 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.
I will apply it in patch v2.
> [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);
> ^~~~~~~~~~~~
Fix in patch v2.
> <...>
>
> > +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?
I agree with you, it's not suitable to save the pointer of a local variable
converting it to uing64_t.
This code is just for test, I will remove it. Thanks.
>
> > +}
> > +
> > +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.
> <...>
It's just for debug. I will remove it in patch v2. Thanks.
> > +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.
Okay.
> > +
> > + .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.
Yes
> If so can the ports be configured via regular device drivers? Is there
> possible
> conflict between two controls and is there any syncronization required?
If argument 'fpga_acc ' of vdev enable, representor is just for FPGA port
configuration.
Otherwise, representor is just for I40e PF port configuration.
> <...>
>
> > +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:
Okay
> ../dpdk/drivers/net/ipn3ke/ipn3ke_tm.c:1551:3: warning: add explicit braces
> to avoid dangling else [-Wdangling-else]
> else
> ^
Okay
> > + }
> > +
> > + 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
> ^
Apply in patch v2.
>
> <...>
>
> > +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:
Fix in patch v2.
> .../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] <...>
Okay, I will fix it in patch v2.
> > +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.
Okay.
> <...>
>
> > +/* 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.
Okay.
> <...>
>
> > @@ -0,0 +1,4 @@
> > +DPDK_2.0 {
>
> Please fix tag with correct DPDK version.
Okay
> > +
> > + local: *;
> > +};
> >
