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.zh...@intel.com
> Cc: sta...@dpdk.org
>
> Signed-off-by: Vladimir Medvedkin <vladimir.medved...@intel.com>
Acked-by: Bruce Richardson <bruce.richard...@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
>
