|SUCCESS| [v4] net/mlx5: prepend implicit items in sync flow creation path

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

Reviewing patch: net/mlx5: prepend implicit items in sync flow creation path

Commit: (from patch metadata)
Author: Maxime Peim <maxime.peim at gmail.com>
Subject: [PATCH v4] net/mlx5: prepend implicit items in sync flow creation path

This patch adds implicit item prepending to the sync flow creation path
to match the async template API behavior. It also refactors the pattern
adjustment logic into a new helper flow_hw_adjust_pattern() and fixes a
bug where metadata split failure returned a negative int cast to
uintptr_t.

> diff --git a/drivers/net/mlx5/mlx5_flow_hw.c b/drivers/net/mlx5/mlx5_flow_hw.c
> index bca5b2769e..6b3fcb43a7 100644
> --- a/drivers/net/mlx5/mlx5_flow_hw.c
> +++ b/drivers/net/mlx5/mlx5_flow_hw.c

[ ... ]

> +/*
> + * Validate the user-supplied items and, in eSwitch mode, prepend the implicit
> + * scoping item so the rule/template is bound to the current representor port:
> + *   - ingress -> RTE_FLOW_ITEM_TYPE_REPRESENTED_PORT (dev->data->port_id)
> + *   - egress  -> MLX5_RTE_FLOW_ITEM_TYPE_TAG on REG_C_0 (tx vport tag),
> + *                skipped when the user already supplied an SQ item.
> + *
> + * @param nt_flow
> + *   Selects between the two call paths that share this helper:
> + *     false -> pattern template creation (async API). The prepended item's
> + *              spec is left zeroed so mlx5dr matches any value; the live
> + *              port_id / tx-tag value is substituted later by
> + *              flow_hw_get_rule_items() at rule-create time.
> + *     true  -> sync (non-template) flow creation. The prepended item's spec
> + *              is filled immediately with the live values, and the flag is
> + *              forwarded to __flow_hw_pattern_validate() so that validation
> + *              paths gated on nt_flow (e.g. GENEVE_OPT TLV parser creation)
> + *              take the non-template branch.
> + *
> + * Return / ownership:
> + *   - NULL on validation or allocation failure (error populated).
> + *   - `items` unchanged when no prepending is required; *copied_items == NULL.
> + *   - A newly-allocated array otherwise; also stored in *copied_items. The
> + *     caller must mlx5_free(*copied_items) on every path (it is safe to call
> + *     with NULL). Do not free the returned pointer directly.
> + */
> +static const struct rte_flow_item *
> +flow_hw_adjust_pattern(struct rte_eth_dev *dev, const struct rte_flow_pattern_template_attr *attr,
> +		       bool nt_flow, const struct rte_flow_item *items, uint64_t *item_flags,
> +		       uint64_t *nb_items, struct rte_flow_item **copied_items,
> +		       struct rte_flow_error *error)

Does this parameter list exceed 80 columns per the coding style?

> +{
> +	struct mlx5_priv *priv = dev->data->dev_private;
> +	struct rte_flow_item_ethdev port_spec = {.port_id = dev->data->port_id};
>  	struct rte_flow_item port = {
>  		.type = RTE_FLOW_ITEM_TYPE_REPRESENTED_PORT,
>  		.mask = &rte_flow_item_ethdev_mask,

[ ... ]

> +	if (!copied_items || !item_flags || !nb_items)
> +		return NULL;

Should this set an error code in the error structure before returning
NULL? The callers expect a populated error on NULL return per the
function documentation.

> +
> +	if (nt_flow) {
> +		port.spec = &port_spec;
> +		tag_v.data = flow_hw_tx_tag_regc_value(dev);
> +	}
> +
> +	/*
> +	 * item_flags must be zero-initialized: __flow_hw_pattern_validate()
> +	 * OR-accumulates bits into it and reads it (MLX5_FLOW_LAYER_TUNNEL)
> +	 * on the first iteration.
> +	 */
> +	*item_flags = 0;
> +
>  	/* Validate application items only */
> -	rc = flow_hw_pattern_validate(dev, attr, items, &item_flags, error);
> +	rc = __flow_hw_pattern_validate(dev, attr, items, item_flags, nt_flow, error);
>  	if (rc < 0)
>  		return NULL;
> -	orig_item_nb = rc;
> -	if (priv->sh->config.dv_esw_en &&
> -	    attr->ingress && !attr->egress && !attr->transfer) {
> -		copied_items = flow_hw_prepend_item(items, orig_item_nb, &port, error);
> -		if (!copied_items)
> +	*nb_items = rc;

Is it safe to cast rc (negative errno on error, positive count on
success) to uint64_t via *nb_items? The check above only guards rc < 0.

> +
> +	if (priv->sh->config.dv_esw_en && attr->ingress && !attr->egress && !attr->transfer) {
> +		*copied_items = flow_hw_prepend_item(items, *nb_items, &port, error);
> +		if (!*copied_items)
>  			return NULL;
> -		tmpl_items = copied_items;
> -	} else if (priv->sh->config.dv_esw_en &&
> -		   !attr->ingress && attr->egress && !attr->transfer) {
> -		if (item_flags & MLX5_FLOW_ITEM_SQ) {
> -			DRV_LOG(DEBUG, "Port %u omitting implicit REG_C_0 match for egress "
> -				       "pattern template", dev->data->port_id);
> -			tmpl_items = items;
> -			goto setup_pattern_template;
> +		return *copied_items;
> +	} else if (priv->sh->config.dv_esw_en && !attr->ingress && attr->egress &&
> +		   !attr->transfer) {
> +		if (*item_flags & MLX5_FLOW_ITEM_SQ) {
> +			DRV_LOG(DEBUG,
> +				"Port %u: explicit SQ item present, omitting implicit "
> +				"REG_C_0 match for egress pattern",
> +				dev->data->port_id);
> +			return items;
>  		}
> -		copied_items = flow_hw_prepend_item(items, orig_item_nb, &tag, error);
> -		if (!copied_items)
> +		*copied_items = flow_hw_prepend_item(items, *nb_items, &tag, error);
> +		if (!*copied_items)
>  			return NULL;
> -		tmpl_items = copied_items;
> -	} else {
> -		tmpl_items = items;
> +		return *copied_items;
>  	}
> -setup_pattern_template:
> +	return items;
> +}

[ ... ]

> @@ -14289,20 +14345,27 @@ static uintptr_t flow_hw_list_create(struct rte_eth_dev *dev,
>  		.egress = attr->egress,
>  		.transfer = attr->transfer,
>  	};
> -
> -	/* Validate application items only */
> -	ret = __flow_hw_pattern_validate(dev, &pattern_template_attr, items,
> -						&item_flags, true, error);
> -	if (ret < 0)
> -		return 0;
> +	struct rte_flow_item *copied_items = NULL;
> +	const struct rte_flow_item *prepend_items;
> +	uint64_t orig_item_nb, item_flags;
>  
>  	RTE_SET_USED(encap_idx);
>  	if (!error)
>  		error = &shadow_error;
> +
> +	prepend_items = flow_hw_adjust_pattern(dev, &pattern_template_attr, true, items,
> +					       &orig_item_nb, &item_flags, &copied_items, error);
> +	if (!prepend_items)
> +		return 0;
> +
>  	split = mlx5_flow_nta_split_metadata(dev, attr, actions, qrss, action_flags,
>  					     actions_n, external, &resource, error);
> -	if (split < 0)
> -		return split;
> +	if (split < 0) {
> +		mlx5_free(copied_items);
> +		return 0;
> +	} else if (!split) {
> +		resource.suffix.items = prepend_items;
> +	}

Does this code have a logic issue? When split is false (0), the code
assigns resource.suffix.items = prepend_items. But looking at the rest
of the function:

> @@ -14343,12 +14409,14 @@ static uintptr_t flow_hw_list_create(struct rte_eth_dev *dev,
>  	if (flow) {
>  		flow->nt2hws->rix_mreg_copy = cpy_idx;
>  		cpy_idx = 0;
> -		if (!split)
> +		if (!split) {
> +			mlx5_free(copied_items);
>  			return (uintptr_t)flow;
> +		}
>  		/* Fall Through to prefix flow creation. */
>  	}
>  prefix_flow:
> -	ret = mlx5_flow_hw_create_flow(dev, type, attr, items, resource.prefix.actions,
> +	ret = mlx5_flow_hw_create_flow(dev, type, attr, prepend_items, resource.prefix.actions,
>  				       item_flags, action_flags, external, &prfx_flow, error);

When split is true, prefix flow creation uses prepend_items. Does
resource.suffix.items ever get used when split is false, or does the
early return mean it's not accessed? If the suffix path is taken when
split==false, would it use uninitialized resource.suffix.items?

> @@ -14313,23 +14376,26 @@ static uintptr_t flow_hw_list_create(struct rte_eth_dev *dev,
>  			goto free;
>  	}
>  	if (action_flags & MLX5_FLOW_ACTION_SAMPLE) {
> -		flow = mlx5_nta_sample_flow_list_create(dev, type, attr, items, actions,
> +		flow = mlx5_nta_sample_flow_list_create(dev, type, attr, prepend_items, actions,
>  							item_flags, action_flags, error);
> -		if (flow != NULL)
> +		if (flow != NULL) {
> +			mlx5_free(copied_items);
>  			return (uintptr_t)flow;
> +		}
>  		goto free;
>  	}

Per the coding style, should implicit comparisons use "if (flow)"
instead of "if (flow != NULL)"?

> @@ -14357,6 +14425,7 @@ static uintptr_t flow_hw_list_create(struct rte_eth_dev *dev,
>  		flow->nt2hws->chaned_flow = 1;
>  		SLIST_INSERT_AFTER(prfx_flow, flow, nt2hws->next);
>  		mlx5_flow_nta_split_resource_free(dev, &resource);
> +		mlx5_free(copied_items);
>  		return (uintptr_t)prfx_flow;
>  	}
>  free:
> @@ -14368,6 +14437,7 @@ static uintptr_t flow_hw_list_create(struct rte_eth_dev *dev,
>  		mlx5_flow_nta_del_copy_action(dev, cpy_idx);
>  	if (split > 0)
>  		mlx5_flow_nta_split_resource_free(dev, &resource);
> +	mlx5_free(copied_items);
>  	return 0;
>  }