|SUCCESS| net/mlx5: redirect LACP traffic for legacy E-Switch

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

Commit: 87e4384d2662 ("net/mlx5: fix condition of LACP miss flow")
Author: Dariusz Sosnowski <dsosnowski at nvidia.com>

net/mlx5: redirect LACP traffic for legacy E-Switch

Offending patch fixed the LACP miss rule logic for NICs where switchdev
is enabled. In this case, LACP miss rules should be inserted if and only
if started port is a main port on the embedded switch. Side effect of
that change was that LACP miss rules are not inserted when switchdev is
disabled and legacy SR-IOV switch mode is used.

This patch addresses that:

- Fix the LACP rule insertion condition.
- Move HWS table for LACP rule creation out of FDB rules, so they can be
  created separately.

[ ... ]

> diff --git a/drivers/net/mlx5/mlx5_flow.h b/drivers/net/mlx5/mlx5_flow.h
> index c9e72a33d6..3f5ba55bf9 100644
> --- a/drivers/net/mlx5/mlx5_flow.h
> +++ b/drivers/net/mlx5/mlx5_flow.h
> @@ -3031,6 +3031,13 @@ struct mlx5_flow_hw_ctrl_rx {
>  						[MLX5_FLOW_HW_CTRL_RX_EXPANDED_RSS_MAX];
>  };
>  
> +/* Contains all templates and table required for redirecting LACP traffic with HWS. */
> +struct mlx5_flow_hw_lacp_miss {
> +	struct rte_flow_pattern_template *lacp_rx_items_tmpl;
> +	struct rte_flow_actions_template *lacp_rx_actions_tmpl;
> +	struct rte_flow_template_table *hw_lacp_rx_tbl;
> +};

[ ... ]

> @@ -3735,6 +3739,36 @@ mlx5_indirect_list_handles_release(struct rte_eth_dev *dev);
>  
>  bool mlx5_flow_is_steering_disabled(void);
>  
> +/**
> + * Returns true if Rx control rule for LACP traffic is needed.
> + *
> + * mlx5 PMD needs to create a rule matching LACP traffic and forwarding it back to kernel if:
> + *
> + * - Underlying device is a bond interface.
> + * - User did not request to handle LACP traffic in user space.
> + *
> + * Creation of this rule is also controlled by the E-Switch mode:
> + *
> + * - It must be created in legacy mode.
> + * - It must be created only on proxy port in switchdev mode.
> + *
> + * @param[in] priv
> + *   Pointer to Ethernet device structure.

This comment says @param[in] priv but the function parameter is named
dev, not priv. Is the comment correct?

> + * @return
> + *   True if LACP rules must be created.
> + *   False otherwise.
> + */
> +static inline bool
> +mlx5_flow_lacp_miss_needed(struct rte_eth_dev *dev)
> +{
> +	struct mlx5_priv *priv = dev->data->dev_private;
> +
> +	return !priv->sh->config.lacp_by_user &&
> +	    priv->pf_bond >= 0 &&
> +	    (!priv->sh->esw_mode || (priv->sh->esw_mode && priv->master));

Coding style: The second and third lines here use aligned whitespace
indentation. According to the DPDK coding style guidelines, indentation
should be done with tabs and spaces should only be used for alignment
after the required indentation level with tabs. Can these lines be
indented consistently with tabs?

> +}

[ ... ]

> diff --git a/drivers/net/mlx5/mlx5_flow_hw.c b/drivers/net/mlx5/mlx5_flow_hw.c
> index b6bb9f12a6..c133230cb7 100644
> --- a/drivers/net/mlx5/mlx5_flow_hw.c
> +++ b/drivers/net/mlx5/mlx5_flow_hw.c

[ ... ]

> @@ -10898,6 +10889,99 @@ flow_hw_create_lacp_rx_table(struct rte_eth_dev *dev,
>  	return flow_hw_table_create(dev, &cfg, &it, 1, &at, 1, error);
>  }
>  
> +/*
> + * Clean up templates and table used for redirecting LACP traffic to kernel.
> + *
> + * @param dev
> + *   Pointer to Ethernet device.
> + */
> +static void
> +flow_hw_cleanup_lacp_miss_tables(struct rte_eth_dev *dev)
> +{
> +	struct mlx5_priv *priv = dev->data->dev_private;
> +	struct mlx5_flow_hw_lacp_miss *hw_lacp_miss;
> +
> +	if (priv->hw_lacp_miss == NULL)

Coding style: Implicit comparison against NULL. Can this be written as
!priv->hw_lacp_miss per the DPDK coding style?

> +		return;
> +
> +	hw_lacp_miss = priv->hw_lacp_miss;
> +
> +	if (hw_lacp_miss->hw_lacp_rx_tbl)
> +		claim_zero(flow_hw_table_destroy(dev, hw_lacp_miss->hw_lacp_rx_tbl, NULL));
> +	if (hw_lacp_miss->lacp_rx_actions_tmpl)
> +		claim_zero(flow_hw_actions_template_destroy(dev,
> +							    hw_lacp_miss->lacp_rx_actions_tmpl,
> +							    NULL));
> +	if (hw_lacp_miss->lacp_rx_items_tmpl)
> +		claim_zero(flow_hw_pattern_template_destroy(dev,
> +							    hw_lacp_miss->lacp_rx_items_tmpl,
> +							    NULL));
> +
> +	mlx5_free(hw_lacp_miss);
> +	priv->hw_lacp_miss = NULL;

Coding style: Implicit comparison against NULL. Can the three if
statements above hw_lacp_miss->hw_lacp_rx_tbl, etc. use explicit
comparisons?

> +}
> +
> +/*
> + * Create templates and table for redirecting LACP traffic to kernel.
> + *
> + * LACP traffic redirection is needed whenever LACP bond is managed by the kernel.
> + * Required rule has a following structure:
> + *
> + * - ingress rule on root table
> + * - match EtherType 0x8809
> + * - action DEFAULT_MISS
> + *
> + * @param dev
> + *   Pointer to Ethernet device.
> + *
> + * @return
> + *   0 on success. Negative errno otherwise.
> + */
> +static int
> +flow_hw_create_lacp_miss_tables(struct rte_eth_dev *dev)
> +{
> +	struct mlx5_priv *priv = dev->data->dev_private;
> +	struct mlx5_flow_hw_lacp_miss *hw_lacp_miss;
> +
> +	if (mlx5_flow_is_steering_disabled())
> +		return 0;
> +
> +	hw_lacp_miss = mlx5_malloc(MLX5_MEM_ZERO, sizeof(*hw_lacp_miss), 0, SOCKET_ID_ANY);
> +	if (!hw_lacp_miss) {
> +		DRV_LOG(ERR, "port %u Failed to allocate memory for LACP miss tables",
> +			dev->data->port_id);
> +		return -ENOMEM;
> +	}
> +	priv->hw_lacp_miss = hw_lacp_miss;
> +
> +	hw_lacp_miss->lacp_rx_items_tmpl = flow_hw_create_lacp_rx_pattern_template(dev, NULL);
> +	if (!hw_lacp_miss->lacp_rx_items_tmpl) {
> +		DRV_LOG(ERR, "port %u Failed to create pattern template for LACP Rx traffic",
> +			dev->data->port_id);
> +		goto error;
> +	}
> +	hw_lacp_miss->lacp_rx_actions_tmpl = flow_hw_create_lacp_rx_actions_template(dev, NULL);
> +	if (!hw_lacp_miss->lacp_rx_actions_tmpl) {
> +		DRV_LOG(ERR, "port %u Failed to create actions template for LACP Rx traffic",
> +			dev->data->port_id);
> +		goto error;
> +	}
> +	hw_lacp_miss->hw_lacp_rx_tbl =
> +		flow_hw_create_lacp_rx_table(dev, hw_lacp_miss->lacp_rx_items_tmpl,
> +					     hw_lacp_miss->lacp_rx_actions_tmpl, NULL);
> +	if (!hw_lacp_miss->hw_lacp_rx_tbl) {
> +		DRV_LOG(ERR, "port %u Failed to create template table for LACP Rx traffic",
> +			dev->data->port_id);
> +		goto error;
> +	}
> +
> +	return 0;
> +
> +error:
> +	flow_hw_cleanup_lacp_miss_tables(dev);

Does this error path leak hw_lacp_miss? The
flow_hw_create_lacp_miss_tables() function sets priv->hw_lacp_miss =
hw_lacp_miss early, then on error calls
flow_hw_cleanup_lacp_miss_tables() which frees hw_lacp_miss and clears
the pointer. However, if this error happens after
priv->hw_lacp_miss is set but before any template creation succeeds,
does the cleanup function handle the partially initialized structure
correctly? Can the cleanup logic operate safely when the structure
fields remain NULL?

> +	return -EINVAL;
> +}

[ ... ]

> @@ -12160,6 +12220,14 @@ __flow_hw_configure(struct rte_eth_dev *dev,
>  		if (ret)
>  			goto err;
>  	}
> +	if (mlx5_flow_lacp_miss_needed(dev)) {
> +		ret = flow_hw_create_lacp_miss_tables(dev);
> +		if (ret) {
> +			rte_flow_error_set(error, -ret, RTE_FLOW_ERROR_TYPE_UNSPECIFIED, NULL,
> +					   "Unable to create LACP miss flow tables");
> +			goto err;
> +		}
> +	}

[ ... ]

> @@ -16234,10 +16302,10 @@ mlx5_flow_hw_lacp_rx_flow(struct rte_eth_dev *dev)
>  		.type = MLX5_CTRL_FLOW_TYPE_LACP_RX,
>  	};
>  
> -	if (!priv->dr_ctx || !priv->hw_ctrl_fdb || !priv->hw_ctrl_fdb->hw_lacp_rx_tbl)
> +	if (!priv->dr_ctx || !priv->hw_lacp_miss || !priv->hw_lacp_miss->hw_lacp_rx_tbl)
>  		return 0;
>  	return flow_hw_create_ctrl_flow(dev, dev,
> -					priv->hw_ctrl_fdb->hw_lacp_rx_tbl,
> +					priv->hw_lacp_miss->hw_lacp_rx_tbl,
>  					eth_lacp, 0, miss_action, 0, &flow_info, false);
>  }