[dpdk-dev] [PATCH v1 05/11] drivers/net/ipn3ke: add IPN3KE PMD driver

Xu, Rosen rosen.xu at intel.com
Mon Mar 11 14:09:08 CET 2019


Hi,

> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Wednesday, March 06, 2019 20:44
> To: Xu, Rosen <rosen.xu at intel.com>; dev at dpdk.org
> Cc: Zhang, Tianfei <tianfei.zhang at intel.com>; Wei, Dan
> <dan.wei at intel.com>; Pei, Andy <andy.pei at intel.com>; Yang, Qiming
> <qiming.yang at intel.com>; Wang, Haiyue <haiyue.wang at intel.com>; Chen,
> Santos <santos.chen at intel.com>; Zhang, Zhang <zhang.zhang at 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.xu at intel.com>
> > Signed-off-by: Andy Pei <andy.pei at intel.com>
> > Signed-off-by: Dan Wei <dan.wei at 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: *;
> > +};
> >



More information about the dev mailing list