Re: [PATCH v2] net/ice: fix how ice driver handles flows

[Bruce Richardson <bruce.richardson@intel.com>]

On Thu, Jan 30, 2025 at 04:14:42PM +0000, Vladimir Medvedkin wrote:
> Currently ICE PMD uses group attribute to select the appropriate HW engine
> to offload the flow. This behavior violates the rte_flow API, existing
> documentation/examples, and reveals hardware specific details.
> 
> This patch eliminates the use of the group attribute and runs each engine
> parser in the order they work in the HW pipeline.
> 
> Fixes: 9c5f0070fa3f ("net/ice: map group to pipeline stage")
> Cc: qi.z.zhang@intel.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Vladimir Medvedkin <vladimir.medvedkin@intel.com>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>

Applied to dpdk-next-net-intel,
Thanks,
/Bruce

PS: for future reference, minor feedback inline below.
> ---
>  drivers/net/intel/ice/ice_generic_flow.c | 30 ++++++++++++++----------
>  1 file changed, 17 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/intel/ice/ice_generic_flow.c b/drivers/net/intel/ice/ice_generic_flow.c
> index 50d760004f..5c34e0385f 100644
> --- a/drivers/net/intel/ice/ice_generic_flow.c
> +++ b/drivers/net/intel/ice/ice_generic_flow.c
> @@ -20,6 +20,8 @@
>  
>  #define ICE_FLOW_ENGINE_DISABLED(mask, type) ((mask) & BIT(type))
>  
> +#define ICE_FLOW_ENGINE_NB	3
> +
>  static struct ice_engine_list engine_list =
>  		TAILQ_HEAD_INITIALIZER(engine_list);
>  
> @@ -2295,21 +2297,23 @@ ice_flow_process_filter(struct rte_eth_dev *dev,
>  		return 0;
>  	}
>  
> -	parser = get_flow_parser(attr->group);
> -	if (parser == NULL) {
> -		rte_flow_error_set(error, EINVAL,
> -				   RTE_FLOW_ERROR_TYPE_ATTR,
> -				   NULL, "NULL attribute.");
> -		return -rte_errno;
> -	}
> +	for (int i = 0; i < ICE_FLOW_ENGINE_NB; i++) {
> +		parser = get_flow_parser(i);
> +		if (parser == NULL) {
> +			rte_flow_error_set(error, EINVAL,
> +					RTE_FLOW_ERROR_TYPE_ATTR,
> +					NULL, "NULL attribute.");

Since we allow up to 100 chars per line, this can be kept on two lines if
you want.

> +			return -rte_errno;
> +		}
>  
> -	if (ice_parse_engine(ad, flow, parser, attr->priority,
> -			     pattern, actions, error)) {
> -		*engine = parser->engine;
> -		return 0;
> -	} else {
> -		return -rte_errno;
> +		if (ice_parse_engine(ad, flow, parser, attr->priority,
> +				pattern, actions, error)) {

Similarly here, there is no need to wrap this statement - it all fits in
100 chars.

> +			*engine = parser->engine;
> +			return 0;
> +		}
>  	}
> +
> +	return -rte_errno;
>  }
>  
>  static int
> -- 
> 2.43.0
>