|SUCCESS| [v2] net/cpfl: add LEM block support in CPFL PMD

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

This review is for commit "net/cpfl: add LEM block support in CPFL PMD"

Author: Sushmita Hoskeri <sushmita.hoskeri at intel.com>

Subject: net/cpfl: add LEM block support in CPFL PMD

This patch adds LEM (Large Exact Match) block support to the CPFL PMD
by introducing new APIs and structures to handle LEM rule configuration.

> diff --git a/drivers/net/intel/cpfl/cpfl_flow_engine_fxp.c b/drivers/net/intel/cpfl/cpfl_flow_engine_fxp.c
> index 361827cb10..f42156e28f 100644
> --- a/drivers/net/intel/cpfl/cpfl_flow_engine_fxp.c
> +++ b/drivers/net/intel/cpfl/cpfl_flow_engine_fxp.c

[ ... ]

> @@ -180,6 +181,15 @@ cpfl_fxp_parse_pattern(const struct cpfl_flow_pr_action *pr_action,
>  		memcpy(rinfo->sem.key, pr_action->sem.cpfl_flow_pr_fv, rinfo->sem.key_byte_len);
>  		rinfo->sem.pin_to_cache = CPFL_PIN_TO_CACHE_DEF;
>  		rinfo->sem.fixed_fetch = CPFL_FIXED_FETCH_DEF;
> +	} else if (pr_action->type == CPFL_JS_PR_ACTION_TYPE_LEM) {
> +		struct cpfl_rule_info *rinfo = &rim->rules[i];

Does this code correctly increment i for the new LEM rule being added?
Looking at the SEM code path above, i should have already been used and
incremented, so this appears to reuse the same index. Should i be
incremented before this code block?

> +
> +		rinfo->type = CPFL_RULE_TYPE_LEM;
> +		rinfo->lem.prof_id = pr_action->lem.prof;
> +		rinfo->lem.key_byte_len = pr_action->lem.keysize;
> +		memcpy(rinfo->lem.key, pr_action->lem.cpfl_flow_pr_fv, rinfo->lem.key_byte_len);
> +		rinfo->lem.pin_to_cache = CPFL_PIN_TO_CACHE_CUST;
> +		rinfo->lem.fixed_fetch = CPFL_FIXED_FETCH_DEF;
>  	} else {
>  		PMD_DRV_LOG(ERR, "Invalid pattern item.");
>  		return false;
> diff --git a/drivers/net/intel/cpfl/cpfl_flow_parser.c b/drivers/net/intel/cpfl/cpfl_flow_parser.c
> index e7deb619ee..dfaddc9ec5 100644
> --- a/drivers/net/intel/cpfl/cpfl_flow_parser.c
> +++ b/drivers/net/intel/cpfl/cpfl_flow_parser.c

[ ... ]

> +static int
> +cpfl_flow_js_pattern_act_fv_lem(json_t *cjson_fv, struct cpfl_flow_js_pr_action *js_act)
> +{
> +	int len, i, ret;
> +
> +	len = json_array_size(cjson_fv);
> +	js_act->lem.fv = rte_malloc(NULL, sizeof(struct cpfl_flow_js_fv) * len, 0);
> +	if (!js_act->lem.fv) {
> +		PMD_DRV_LOG(ERR, "Failed to alloc memory.");
> +		return -ENOMEM;
> +	}

Does this code properly validate that len is within acceptable bounds
before allocating? Should there be a check against
CPFL_JS_LEM_FV_KEY_NUM_MAX to prevent excessive allocations?

> +	js_act->lem.fv_size = len;
> +	for (i = 0; i < len; i++) {
> +		struct cpfl_flow_js_fv *js_fv;
> +		json_t *object, *cjson_value;
> +		const char *type;
> +
> +		object = json_array_get(cjson_fv, i);
> +		js_fv = &js_act->lem.fv[i];
> +		ret = cpfl_json_t_to_uint16(object, "offset", &js_fv->offset);
> +		if (ret < 0) {
> +			PMD_DRV_LOG(ERR, "Can not parse 'offset'.");
> +			return -EINVAL;

Does this error path leak js_act->lem.fv? The function returns without
calling rte_free on the previously allocated memory.

> +		}
> +		type = cpfl_json_t_to_string(object, "type");
> +		if (!type) {
> +			PMD_DRV_LOG(ERR, "Can not parse 'type'.");
> +			return -EINVAL;

Does this error path also leak js_act->lem.fv?

> +		}
> +		cjson_value = json_object_get(object, "value");
> +		if (strcmp(type, "immediate") == 0) {
> +			js_fv->type = CPFL_FV_TYPE_IMMEDIATE;
> +			js_fv->immediate = json_integer_value(cjson_value);
> +		} else if (strcmp(type, "metadata") == 0) {
> +			js_fv->type = CPFL_FV_TYPE_METADATA;
> +			cpfl_flow_js_pattern_act_fv_metadata(cjson_value, js_fv);
> +		} else if (strcmp(type, "protocol") == 0) {
> +			js_fv->type = CPFL_FV_TYPE_PROTOCOL;
> +			cpfl_flow_js_pattern_act_fv_proto(cjson_value, js_fv);
> +		} else {
> +			PMD_DRV_LOG(ERR, "Not support this type: %s.", type);
> +			goto err;
> +		}
> +	}
> +	return 0;
> +err:
> +	rte_free(js_act->lem.fv);
> +	return -EINVAL;
> +}

[ ... ]

> @@ -458,6 +508,25 @@ cpfl_flow_js_pattern_per_act(json_t *ob_per_act, struct cpfl_flow_js_pr_action *
>  		ret = cpfl_flow_js_pattern_act_fv(ob_fvs, js_act);
>  		if (ret < 0)
>  			return ret;
> +	} else if (strcmp(type, "lem") == 0) {
> +		js_act->type = CPFL_JS_PR_ACTION_TYPE_LEM;
> +		json_t *cjson_fv, *ob_lem;
> +
> +		ob_lem = json_object_get(ob_per_act, "data");
> +		ret = cpfl_json_t_to_uint16(ob_lem, "profile", &js_act->lem.prof);
> +		if (ret < 0) {
> +			PMD_DRV_LOG(ERR, "Can not parse 'profile'.");
> +			return -EINVAL;
> +		}
> +		ret = cpfl_json_t_to_uint16(ob_lem, "keysize", &js_act->lem.keysize);
> +		if (ret < 0) {
> +			PMD_DRV_LOG(ERR, "Can not parse 'keysize'.");
> +			return -EINVAL;
> +		}
> +		cjson_fv = json_object_get(ob_lem, "fieldvectors");
> +		ret = cpfl_flow_js_pattern_act_fv_lem(cjson_fv, js_act);
> +		if (ret < 0)
> +			return ret;

Should js_act->lem.keysize be validated against CPFL_MAX_KEY_LEN before
calling cpfl_flow_js_pattern_act_fv_lem? This would prevent buffer
overflows in later memcpy operations.

[ ... ]

> @@ -1149,6 +1220,19 @@ cpfl_parse_pr_actions(struct cpfl_itf *itf,
>  			ret = cpfl_parse_fieldvectors(itf, sem->fv, sem->fv_size,
>  						      pr_action->sem.cpfl_flow_pr_fv, items);
>  			return ret;
> +		} else if (attr->group % 10 == 4 && type == CPFL_JS_PR_ACTION_TYPE_LEM) {
> +			struct cpfl_flow_js_pr_action_lem *lem = &pr_act->lem;
> +
> +			pr_action->type = CPFL_JS_PR_ACTION_TYPE_LEM;
> +			pr_action->lem.prof = lem->prof;
> +			pr_action->lem.keysize = lem->keysize;
> +			memset(pr_action->lem.cpfl_flow_pr_fv, 0,
> +					sizeof(pr_action->lem.cpfl_flow_pr_fv));

Does this memset align correctly with the coding style? The second line
of the memset call appears to be misaligned - should it align with the
opening parenthesis on the previous line?

> +			ret = cpfl_parse_fieldvectors(itf, lem->fv, lem->fv_size,
> +							  pr_action->lem.cpfl_flow_pr_fv, items);

Does this line have inconsistent indentation? The second line should
align with the opening parenthesis.

> +			if (ret < 0)
> +				return ret;
> +			continue;

[ ... ]

> diff --git a/drivers/net/intel/cpfl/cpfl_rules.h b/drivers/net/intel/cpfl/cpfl_rules.h
> index 10569b1fdc..2b65c7ecc8 100644
> --- a/drivers/net/intel/cpfl/cpfl_rules.h
> +++ b/drivers/net/intel/cpfl/cpfl_rules.h

[ ... ]

> @@ -46,6 +46,12 @@ enum cpfl_ctlq_rule_cfg_opc {
>  	cpfl_ctlq_sem_query_rule_hash_addr		= 0x1307,
>  	cpfl_ctlq_sem_query_del_rule_hash_addr		= 0x1308,
>  
> +	cpfl_ctlq_lem_add_update_rule			= 0x1343,
> +	cpfl_ctlq_lem_del_rule					= 0x1345,
> +	cpfl_ctlq_lem_query_rule				= 0x1346,
> +	cpfl_ctlq_lem_query_rule_hash_addr		= 0x1347,
> +	cpfl_ctlq_lem_query_del_rule_hash_addr	= 0x1348,

Does this enum use tabs for alignment consistently with the existing
code? The alignment here appears to use tabs while the surrounding code
may use different spacing.