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

[Ferruh Yigit <ferruh.yigit@intel.com>]

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@intel.com>
> Signed-off-by: Andy Pei <andy.pei@intel.com>
> Signed-off-by: Dan Wei <dan.wei@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.

<...>

> @@ -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=<string> "
> +	"port=<int> "
> +	"uudi_high=<int64> "
> +	"uuid_low=<int64> "
> +	"path=<string> "
> +	"pr_enable=<int>"
> +	"debug=<int>");

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=<string> "
> +	"fpga_acc=<string>"
> +	"i40e=<string>");

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: *;
> +};
>