|SUCCESS| [v2] net/mlx5: add validation for indirect actions

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-04-17

Reviewing patch: net/mlx5: add validation for indirect actions

commit bca5b2769e..ec1cdbe1fa
Author: Rayane Boussanni <rboussanni at gmail.com>

    net/mlx5: add validation for indirect actions

This patch implements missing validation logic for RSS and Connection
Tracking (ConnTrack) indirect actions in the Hardware Steering (HWS)
flow engine.

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

[ ... ]

> @@ -346,6 +346,21 @@ mlx5_flow_ct_init(struct rte_eth_dev *dev,
>  		  uint32_t nb_conn_tracks,
>  		  uint16_t nb_queue);
>  
> +static int
> +mlx5_hw_validate_action_rss(struct rte_eth_dev *dev,
> +			    const struct rte_flow_action *action,
> +			    const struct rte_flow_action *mask,
> +			    const struct rte_flow_actions_template_attr *attr,
> +			    uint64_t action_flags,
> +			    struct rte_flow_error *error);
> +static int
> +mlx5_hw_validate_action_conntrack(struct rte_eth_dev *dev,
> +				  const struct rte_flow_action *action,
> +				  const struct rte_flow_action *mask,
> +				  const struct rte_flow_actions_template_attr *attr,
> +				  uint64_t action_flags,
> +				  struct rte_flow_error *error);
> +

Does the indentation here use tabs consistently? The existing
mlx5_flow_ct_init() declaration appears to use tabs for indentation,
while these new forward declarations appear to mix tabs and spaces for
alignment. This violates the DPDK coding style which requires tabs for
indentation and aligning function parameters.

> @@ -6604,6 +6619,8 @@ flow_hw_validate_action_meter_mark(struct rte_eth_dev *dev,
>   *   Pointer to the indirect action.
>   * @param[in] mask
>   *   Pointer to the indirect action mask.
> + * @param[in] attr
> + *   Pointer to the action template attributes.
>   * @param[in, out] action_flags
>   *   Holds the actions detected until now.
>   * @param[in, out] fixed_cnt

[ ... ]

> @@ -6637,11 +6655,17 @@ flow_hw_validate_action_indirect(struct rte_eth_dev *dev,
>  		*action_flags |= MLX5_FLOW_ACTION_METER;
>  		break;
>  	case RTE_FLOW_ACTION_TYPE_RSS:
> -		/* TODO: Validation logic (same as flow_hw_actions_validate) */
> +		ret = mlx5_hw_validate_action_rss(dev, action, mask, attr,
> +						  *action_flags, error);
> +		if (ret < 0)
> +			return ret;

Does this error path preserve the error details set by
mlx5_hw_validate_action_rss()? Since the error parameter is already
populated by the validator, this should be fine, but can you confirm
that mlx5_hw_validate_action_rss() properly sets error on all failure
paths?

>  		*action_flags |= MLX5_FLOW_ACTION_RSS;
>  		break;
>  	case RTE_FLOW_ACTION_TYPE_CONNTRACK:
> -		/* TODO: Validation logic (same as flow_hw_actions_validate) */
> +		ret = mlx5_hw_validate_action_conntrack(dev, action, mask, attr,
> +							*action_flags, error);
> +		if (ret < 0)
> +			return ret;

Same question for mlx5_hw_validate_action_conntrack() - does it properly
populate error on all failure paths?

>  		*action_flags |= MLX5_FLOW_ACTION_CT;
>  		break;
>  	case RTE_FLOW_ACTION_TYPE_COUNT:

[ ... ]

> @@ -7352,6 +7376,7 @@ mlx5_flow_hw_actions_validate(struct rte_eth_dev *dev,
>  		case RTE_FLOW_ACTION_TYPE_INDIRECT:
>  			ret = flow_hw_validate_action_indirect(dev, action,
>  							       mask,
> +							       attr,
>  							       &action_flags,
>  							       &fixed_cnt,
>  							       error);