|SUCCESS| [RFC,8/8] net/txgbe: privatize legacy flow director types

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/164551

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-05-29

# Review of DPDK Legacy Flow Director Removal Patch Series

commit 8a1b5c4d2e3f9a6b7c8d9e0f1a2b3c4d5e6f7a8b (hypothetical SHA)
Author: Stephen Hemminger <stephen at networkplumber.org>

RFC: Remove legacy flow director API from testpmd and Intel PMDs

This series deprecates and removes legacy flow director artifacts from
testpmd, i40e, and ixgbe, in preparation for final removal in DPDK 26.11.

Below is a unified diff representing the series. Questions about potential
regressions appear inline near the relevant code sections.

[ ... ]

> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> index cc9c462498..a5022bde04 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -163,7 +163,7 @@ static void cmd_help_long_parsed(void *parsed_result,
>  			"Display:\n"
>  			"--------\n\n"
>  
> -			"show port (info|stats|summary|xstats|fdir|dcb_tc) (port_id|all)\n"
> +			"show port (info|stats|summary|xstats|dcb_tc) (port_id|all)\n"
>  			"    Display information for port_id, or all.\n\n"

The help string update looks correct.

> @@ -7510,11 +7510,6 @@ static void cmd_showportall_parsed(void *parsed_result,
>  	else if (!strcmp(res->what, "xstats"))
>  		RTE_ETH_FOREACH_DEV(i)
>  			nic_xstats_display(i);
> -#if defined(RTE_NET_I40E) || defined(RTE_NET_IXGBE)
> -	else if (!strcmp(res->what, "fdir"))
> -		RTE_ETH_FOREACH_DEV(i)
> -			fdir_get_infos(i);
> -#endif

Does this code properly handle the case where a user passes "fdir" to
the CLI after applying this patch? Is there a fallback error message
or validation that prevents silent no-op execution of an unrecognized
"what" string?

> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> index c950793aaf..12fc679250 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -7041,169 +7041,6 @@ flowtype_to_str(uint16_t flow_type)
>  	return NULL;
>  }
>  
> -#if defined(RTE_NET_I40E) || defined(RTE_NET_IXGBE)
> -
> -static inline void
> -print_fdir_flex_payload(struct rte_eth_fdir_flex_conf *flex_conf, uint32_t num)
> -{

Does the removal of print_fdir_flex_payload() and associated display
functions cause any memory leaks if they previously freed resources,
or were they purely display helpers with no cleanup responsibility?

[ ... ]

> diff --git a/drivers/net/intel/i40e/i40e_testpmd.c b/drivers/net/intel/i40e/i40e_testpmd.c
> index 21f596297b..3943287ca9 100644
> --- a/drivers/net/intel/i40e/i40e_testpmd.c
> +++ b/drivers/net/intel/i40e/i40e_testpmd.c
> @@ -464,149 +464,6 @@ static cmdline_parse_inst_t cmd_show_queue_region_info_all = {
>  	},
>  };
>  
> -/* *** deal with flow director filter *** */
> -struct cmd_flow_director_result {
> -	cmdline_fixed_string_t flow_director_filter;

Does the removal of cmd_flow_director_filter_parsed() properly clean up
any dynamically allocated resources referenced by cmd_flow_director_result,
particularly the filepath member and any internal conf.input.packet buffer
opened via open_file()?

> -static void
> -cmd_flow_director_filter_parsed(void *parsed_result,
> -	__rte_unused struct cmdline *cl, __rte_unused void *data)
> -{
> [...]
> -	conf.input.packet = open_file(res->filepath,
> -				&conf.input.length);
> -	if (!conf.input.packet)
> -		return;

Does this code leak conf.input.packet if the function returns early after
open_file() succeeds but before close_file() is called?

> -	ret = rte_pmd_i40e_flow_add_del_packet_template(port,
> -							&conf,
> -							add);
> -	if (ret < 0)
> -		fprintf(stderr, "flow director config error: (%s)\n",
> -			strerror(-ret));
> -	close_file(conf.input.packet);
> -}

Does this code leak conf.input.packet if
rte_pmd_i40e_flow_add_del_packet_template() fails but the error path
still calls close_file()? (Existing code appears correct but worth
confirming no double-free or missed free upon removal.)

[ ... ]

> diff --git a/drivers/net/intel/i40e/i40e_fdir.c b/drivers/net/intel/i40e/i40e_fdir.c
> index ad256a5a11..c1acf389bf 100644
> --- a/drivers/net/intel/i40e/i40e_fdir.c
> +++ b/drivers/net/intel/i40e/i40e_fdir.c
> @@ -1793,146 +1793,6 @@ i40e_fdir_flush(struct rte_eth_dev *dev)
>  	return 0;
>  }
>  
> -static inline void
> -i40e_fdir_info_get_flex_set(struct i40e_pf *pf,
> -			struct rte_eth_flex_payload_cfg *flex_set,
> -			uint16_t *num)

Does the removal of i40e_fdir_info_get_flex_set() cause any issue
with the pf->fdir.flex_set[] array, which appears to be read-only
here but might need cleanup elsewhere?

> -void
> -i40e_fdir_info_get(struct rte_eth_dev *dev,
> -			struct rte_eth_fdir_info *fdir)
> -{

Does the removal of i40e_fdir_info_get() and i40e_fdir_stats_get()
orphan any callers outside rte_pmd_i40e.c, or were these exclusively
called through the experimental PMD API functions being removed?

[ ... ]

> diff --git a/drivers/net/intel/ixgbe/ixgbe_fdir.c b/drivers/net/intel/ixgbe/ixgbe_fdir.c
> index b32dc54287..20cc72ffcd 100644
> --- a/drivers/net/intel/ixgbe/ixgbe_fdir.c
> +++ b/drivers/net/intel/ixgbe/ixgbe_fdir.c
> @@ -1202,123 +1202,6 @@ ixgbe_fdir_flush(struct rte_eth_dev *dev)
>  	return ret;
>  }
>  
> -#define FDIRENTRIES_NUM_SHIFT 10
> -void
> -ixgbe_fdir_info_get(struct rte_eth_dev *dev, struct rte_eth_fdir_info *fdir_info)

Does the removal of FDIRENTRIES_NUM_SHIFT break any macro dependencies
in ixgbe that might compute filter table sizes or offsets?

> -{
> -	struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> -	struct rte_eth_fdir_conf *fdir_conf = IXGBE_DEV_FDIR_CONF(dev);
> [...]
> -	if (fdir_info->mode >= RTE_FDIR_MODE_PERFECT &&
> -	    fdir_info->mode <= RTE_FDIR_MODE_PERFECT_TUNNEL)
> -		fdir_info->guarant_spc = max_num - fdir_stats->free;
> -	else if (fdir_info->mode == RTE_FDIR_MODE_SIGNATURE)
> -		fdir_info->guarant_spc = max_num * 4 - fdir_stats->free;

Does the code that calculates guarant_spc have any side effects on HW
registers or internal counters that might need explicit cleanup when
this function is removed?

[ ... ]

> diff --git a/drivers/net/intel/ixgbe/ixgbe_ethdev.c b/drivers/net/intel/ixgbe/ixgbe_ethdev.c
> index 9dc015dfff..20b7b91cc3 100644
> --- a/drivers/net/intel/ixgbe/ixgbe_ethdev.c
> +++ b/drivers/net/intel/ixgbe/ixgbe_ethdev.c
> @@ -1471,6 +1471,14 @@ static int ixgbe_fdir_filter_init(struct rte_eth_dev *eth_dev)
>  	/* drop queue is always fixed */
>  	IXGBE_DEV_FDIR_CONF(eth_dev)->drop_queue = IXGBE_FDIR_DROP_QUEUE;
>  
> +	/*
> +	 * Initialize pballoc to 64K. The base driver's
> +	 * enum ixgbe_fdir_pballoc_type uses NONE=0, 64K=1, ... so an
> +	 * uninitialized (zero) pballoc would be rejected by
> +	 * configure_fdir_flags(); set it explicitly here.
> +	 */
> +	IXGBE_DEV_FDIR_CONF(eth_dev)->pballoc = IXGBE_FDIR_PBALLOC_64K;

Does this explicit initialization of pballoc to IXGBE_FDIR_PBALLOC_64K
override any user-configured devargs or runtime settings that might
have expected a different allocation size?

[ ... ]

> diff --git a/drivers/net/intel/ixgbe/ixgbe_fdir.c b/drivers/net/intel/ixgbe/ixgbe_fdir.c
> index b32dc54287..69fd8c674c 100644
> --- a/drivers/net/intel/ixgbe/ixgbe_fdir.c
> +++ b/drivers/net/intel/ixgbe/ixgbe_fdir.c
> @@ -81,23 +81,23 @@
>  static int fdir_erase_filter_82599(struct ixgbe_hw *hw, uint32_t fdirhash);
>  static int fdir_set_input_mask_82599(struct ixgbe_adapter *adapter,
>  		const struct ixgbe_hw_fdir_mask *mask,
> -		enum rte_fdir_mode mode);
> +		enum ixgbe_fdir_mode mode);

Does the change from rte_fdir_mode to ixgbe_fdir_mode maintain
binary-compatible enum values, ensuring that any serialized or
persistent configuration structures don't break?

> @@ -164,20 +164,20 @@ fdir_enable_82599(struct ixgbe_hw *hw, uint32_t fdirctrl)
>   * flexbytes matching field, and drop queue (only for perfect matching mode).
>   */
>  static inline int
> -configure_fdir_flags(const struct rte_eth_fdir_conf *conf, uint32_t *fdirctrl)
> +configure_fdir_flags(const struct ixgbe_fdir_conf *conf, uint32_t *fdirctrl)
>  {
>  	*fdirctrl = 0;
>  
>  	switch (conf->pballoc) {
> -	case RTE_ETH_FDIR_PBALLOC_64K:
> +	case IXGBE_FDIR_PBALLOC_64K:

Does the switch from RTE_ETH_FDIR_PBALLOC_64K to IXGBE_FDIR_PBALLOC_64K
preserve the original enum integer values (0, 1, 2) to avoid breaking
the hardware programming logic that depends on these constants?

[ ... ]

> diff --git a/drivers/net/txgbe/txgbe_ethdev.c b/drivers/net/txgbe/txgbe_ethdev.c
> index 5d360f8305..258ec4d97c 100644
> --- a/drivers/net/txgbe/txgbe_ethdev.c
> +++ b/drivers/net/txgbe/txgbe_ethdev.c
> @@ -524,7 +524,7 @@ txgbe_handle_devarg(__rte_unused const char *key, const char *value,
>  static void
>  txgbe_parse_devargs(struct rte_eth_dev *dev)
>  {
> -	struct rte_eth_fdir_conf *fdir_conf = TXGBE_DEV_FDIR_CONF(dev);
> +	struct txgbe_fdir_conf *fdir_conf = TXGBE_DEV_FDIR_CONF(dev);
>  	struct rte_pci_device *pci_dev = RTE_ETH_DEV_TO_PCI(dev);
>  	struct rte_devargs *devargs = pci_dev->device.devargs;
>  	struct txgbe_hw *hw = TXGBE_DEV_HW(dev);

Does the change from rte_eth_fdir_conf to txgbe_fdir_conf in
txgbe_parse_devargs() maintain the same struct layout so that
existing parsing logic for fdir_conf->pballoc and other fields
remains correct?

> @@ -934,7 +934,7 @@ static int txgbe_l2_tn_filter_uninit(struct rte_eth_dev *eth_dev)
>  
>  int txgbe_fdir_filter_init(struct rte_eth_dev *eth_dev)
>  {
> -	struct rte_eth_fdir_conf *fdir_conf = TXGBE_DEV_FDIR_CONF(eth_dev);
> +	struct txgbe_fdir_conf *fdir_conf = TXGBE_DEV_FDIR_CONF(eth_dev);
>  	struct txgbe_hw_fdir_info *fdir_info = TXGBE_DEV_FDIR(eth_dev);
>  	char fdir_hash_name[RTE_HASH_NAMESIZE];
>  	u16 max_fdir_num = (1024 << (fdir_conf->pballoc + 1)) - 2;

Does this calculation (1024 << (fdir_conf->pballoc + 1)) - 2 still
produce the correct filter table size after switching to
txgbe_fdir_conf and enum txgbe_fdir_pballoc_type? The comment in
patch 8/8 states the enum values are 64K=0, 128K=1, 256