[PATCH v2 1/2] net/cpfl: fix check for opcodes of received ctlq messages

[PATCH v2 1/2] net/cpfl: fix check for opcodes of received ctlq messages

[Soumyadeep Hore]

cpfl_process_rx_ctlq_msg() is used to check error status
returned for specific opcodes and return error messages
accordingly.

Fixes: db042ef09d26 ("net/cpfl: implement FXP rule creation and destroying")
Cc: stable@dpdk.org

Signed-off-by: Soumyadeep Hore <soumyadeep.hore@intel.com>
---
 drivers/net/cpfl/cpfl_fxp_rule.c | 52 ++++++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)

diff --git a/drivers/net/cpfl/cpfl_fxp_rule.c b/drivers/net/cpfl/cpfl_fxp_rule.c
index 0e710a007b..4232b192ed 100644
--- a/drivers/net/cpfl/cpfl_fxp_rule.c
+++ b/drivers/net/cpfl/cpfl_fxp_rule.c
@@ -60,6 +60,52 @@ cpfl_send_ctlq_msg(struct idpf_hw *hw, struct idpf_ctlq_info *cq, u16 num_q_msg,
 	return ret;
 }
 
+static int
+cpfl_process_rx_ctlq_msg(u16 msg_opcode, u16 msg_status)
+{
+	int ret = CPFL_CFG_PKT_ERR_OK;
+
+	if (msg_status &&
+		msg_opcode == cpfl_ctlq_sem_query_rule_hash_addr)
+		return ret;
+
+	switch (msg_status) {
+	case CPFL_CFG_PKT_ERR_EEXIST:
+		PMD_INIT_LOG(ERR, "The rule has confliction with already existed one");
+		ret = CPFL_CFG_PKT_ERR_EEXIST;
+		break;
+	case CPFL_CFG_PKT_ERR_ENOSPC:
+		PMD_INIT_LOG(ERR, "No space left in the table");
+		ret = CPFL_CFG_PKT_ERR_ENOSPC;
+		break;
+	case CPFL_CFG_PKT_ERR_ESRCH:
+		PMD_INIT_LOG(ERR, "Bad opcode");
+		ret = CPFL_CFG_PKT_ERR_ESRCH;
+		break;
+	case CPFL_CFG_PKT_ERR_ERANGE:
+		PMD_INIT_LOG(ERR, "Parameter are out of");
+		ret = CPFL_CFG_PKT_ERR_ERANGE;
+		break;
+	case CPFL_CFG_PKT_ERR_ESBCOMP:
+		PMD_INIT_LOG(ERR, "Completion error");
+		ret = CPFL_CFG_PKT_ERR_ESBCOMP;
+		break;
+	case CPFL_CFG_PKT_ERR_ENOPIN:
+		PMD_INIT_LOG(ERR, "Entry cannot be pinned in the cache");
+		ret = CPFL_CFG_PKT_ERR_ENOPIN;
+		break;
+	case CPFL_CFG_PKT_ERR_ENOTFND:
+		PMD_INIT_LOG(ERR, "Entry does not exists");
+		ret = CPFL_CFG_PKT_ERR_ENOTFND;
+		break;
+	case CPFL_CFG_PKT_ERR_EMAXCOL:
+		PMD_INIT_LOG(ERR, "Maximum number of hash collisions reached");
+		ret = CPFL_CFG_PKT_ERR_EMAXCOL;
+		break;
+	}
+	return ret;
+}
+
 int
 cpfl_receive_ctlq_msg(struct idpf_hw *hw, struct idpf_ctlq_info *cq, u16 num_q_msg,
 		      struct idpf_ctlq_msg q_msg[])
@@ -92,6 +138,12 @@ cpfl_receive_ctlq_msg(struct idpf_hw *hw, struct idpf_ctlq_info *cq, u16 num_q_m
 
 		/* TODO - process rx controlq message */
 		for (i = 0; i < num_q_msg; i++) {
+			ret = cpfl_process_rx_ctlq_msg(q_msg[i].opcode, q_msg[i].status);
+			if (ret) {
+				PMD_INIT_LOG(ERR, "failed to process rx_ctrlq msg");
+				return ret;
+			}
+
 			if (q_msg[i].data_len > 0)
 				dma = q_msg[i].ctx.indirect.payload;
 			else
-- 
2.43.0

[PATCH v2 2/2] net/cpfl: fix +ve error codes for received ctlq messages

[Soumyadeep Hore]

Previously error codes were only -ve for
cpfl_receive_ctlq_msg() but now there are +ve error codes.
Hence code changes are made accordingly.

Fixes: db042ef09d26 ("net/cpfl: implement FXP rule creation and destroying")
Cc: stable@dpdk.org

Signed-off-by: Soumyadeep Hore <soumyadeep.hore@intel.com>
---
 drivers/net/cpfl/cpfl_flow_engine_fxp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/cpfl/cpfl_flow_engine_fxp.c b/drivers/net/cpfl/cpfl_flow_engine_fxp.c
index 39a281fa61..983b4188e4 100644
--- a/drivers/net/cpfl/cpfl_flow_engine_fxp.c
+++ b/drivers/net/cpfl/cpfl_flow_engine_fxp.c
@@ -95,7 +95,7 @@ cpfl_fxp_create(struct rte_eth_dev *dev,
 
 	ret = cpfl_rule_process(itf, ad->ctlqp[cpq_id], ad->ctlqp[cpq_id + 1],
 				rim->rules, rim->rule_num, true);
-	if (ret < 0) {
+	if (ret) {
 		rte_flow_error_set(error, EINVAL, RTE_FLOW_ERROR_TYPE_HANDLE, NULL,
 				   "cpfl filter create flow fail");
 		rte_free(rim);
-- 
2.43.0

[PATCH v3 1/2] net/cpfl: fix check for opcodes of received ctlq messages

[Soumyadeep Hore]

cpfl_process_rx_ctlq_msg() is used to check error status
returned for specific opcodes and return error messages
accordingly.

Fixes: db042ef09d26 ("net/cpfl: implement FXP rule creation and destroying")
Cc: stable@dpdk.org

Signed-off-by: Soumyadeep Hore <soumyadeep.hore@intel.com>
---
 drivers/net/cpfl/cpfl_fxp_rule.c | 52 ++++++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)

diff --git a/drivers/net/cpfl/cpfl_fxp_rule.c b/drivers/net/cpfl/cpfl_fxp_rule.c
index 0e710a007b..4232b192ed 100644
--- a/drivers/net/cpfl/cpfl_fxp_rule.c
+++ b/drivers/net/cpfl/cpfl_fxp_rule.c
@@ -60,6 +60,52 @@ cpfl_send_ctlq_msg(struct idpf_hw *hw, struct idpf_ctlq_info *cq, u16 num_q_msg,
 	return ret;
 }
 
+static int
+cpfl_process_rx_ctlq_msg(u16 msg_opcode, u16 msg_status)
+{
+	int ret = CPFL_CFG_PKT_ERR_OK;
+
+	if (msg_status &&
+		msg_opcode == cpfl_ctlq_sem_query_rule_hash_addr)
+		return ret;
+
+	switch (msg_status) {
+	case CPFL_CFG_PKT_ERR_EEXIST:
+		PMD_INIT_LOG(ERR, "The rule has confliction with already existed one");
+		ret = CPFL_CFG_PKT_ERR_EEXIST;
+		break;
+	case CPFL_CFG_PKT_ERR_ENOSPC:
+		PMD_INIT_LOG(ERR, "No space left in the table");
+		ret = CPFL_CFG_PKT_ERR_ENOSPC;
+		break;
+	case CPFL_CFG_PKT_ERR_ESRCH:
+		PMD_INIT_LOG(ERR, "Bad opcode");
+		ret = CPFL_CFG_PKT_ERR_ESRCH;
+		break;
+	case CPFL_CFG_PKT_ERR_ERANGE:
+		PMD_INIT_LOG(ERR, "Parameter are out of");
+		ret = CPFL_CFG_PKT_ERR_ERANGE;
+		break;
+	case CPFL_CFG_PKT_ERR_ESBCOMP:
+		PMD_INIT_LOG(ERR, "Completion error");
+		ret = CPFL_CFG_PKT_ERR_ESBCOMP;
+		break;
+	case CPFL_CFG_PKT_ERR_ENOPIN:
+		PMD_INIT_LOG(ERR, "Entry cannot be pinned in the cache");
+		ret = CPFL_CFG_PKT_ERR_ENOPIN;
+		break;
+	case CPFL_CFG_PKT_ERR_ENOTFND:
+		PMD_INIT_LOG(ERR, "Entry does not exists");
+		ret = CPFL_CFG_PKT_ERR_ENOTFND;
+		break;
+	case CPFL_CFG_PKT_ERR_EMAXCOL:
+		PMD_INIT_LOG(ERR, "Maximum number of hash collisions reached");
+		ret = CPFL_CFG_PKT_ERR_EMAXCOL;
+		break;
+	}
+	return ret;
+}
+
 int
 cpfl_receive_ctlq_msg(struct idpf_hw *hw, struct idpf_ctlq_info *cq, u16 num_q_msg,
 		      struct idpf_ctlq_msg q_msg[])
@@ -92,6 +138,12 @@ cpfl_receive_ctlq_msg(struct idpf_hw *hw, struct idpf_ctlq_info *cq, u16 num_q_m
 
 		/* TODO - process rx controlq message */
 		for (i = 0; i < num_q_msg; i++) {
+			ret = cpfl_process_rx_ctlq_msg(q_msg[i].opcode, q_msg[i].status);
+			if (ret) {
+				PMD_INIT_LOG(ERR, "failed to process rx_ctrlq msg");
+				return ret;
+			}
+
 			if (q_msg[i].data_len > 0)
 				dma = q_msg[i].ctx.indirect.payload;
 			else
-- 
2.43.0

[PATCH v3 2/2] net/cpfl: fix +ve error codes for received ctlq messages

[Soumyadeep Hore]

Previously error codes were only -ve for
cpfl_receive_ctlq_msg() but now there are +ve error codes.
Hence code changes are made accordingly.

Fixes: db042ef09d26 ("net/cpfl: implement FXP rule creation and destroying")
Cc: stable@dpdk.org

Signed-off-by: Soumyadeep Hore <soumyadeep.hore@intel.com>
---
 drivers/net/cpfl/cpfl_flow_engine_fxp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/cpfl/cpfl_flow_engine_fxp.c b/drivers/net/cpfl/cpfl_flow_engine_fxp.c
index 39a281fa61..b9e825ef57 100644
--- a/drivers/net/cpfl/cpfl_flow_engine_fxp.c
+++ b/drivers/net/cpfl/cpfl_flow_engine_fxp.c
@@ -95,7 +95,7 @@ cpfl_fxp_create(struct rte_eth_dev *dev,
 
 	ret = cpfl_rule_process(itf, ad->ctlqp[cpq_id], ad->ctlqp[cpq_id + 1],
 				rim->rules, rim->rule_num, true);
-	if (ret < 0) {
+	if (ret != 0) {
 		rte_flow_error_set(error, EINVAL, RTE_FLOW_ERROR_TYPE_HANDLE, NULL,
 				   "cpfl filter create flow fail");
 		rte_free(rim);
-- 
2.43.0

Re: [PATCH v3 1/2] net/cpfl: fix check for opcodes of received ctlq messages

[Bruce Richardson]

On Fri, Jul 05, 2024 at 08:30:31AM +0000, Soumyadeep Hore wrote:
> cpfl_process_rx_ctlq_msg() is used to check error status
> returned for specific opcodes and return error messages
> accordingly.
> 
> Fixes: db042ef09d26 ("net/cpfl: implement FXP rule creation and destroying")
> Cc: stable@dpdk.org

Thanks for splitting the patches. While the other patch definitely looks
like a fix, is this an enhancement or a fix?

More comments inline below.

> 
> Signed-off-by: Soumyadeep Hore <soumyadeep.hore@intel.com>
> ---
>  drivers/net/cpfl/cpfl_fxp_rule.c | 52 ++++++++++++++++++++++++++++++++
>  1 file changed, 52 insertions(+)
> 
> diff --git a/drivers/net/cpfl/cpfl_fxp_rule.c b/drivers/net/cpfl/cpfl_fxp_rule.c
> index 0e710a007b..4232b192ed 100644
> --- a/drivers/net/cpfl/cpfl_fxp_rule.c
> +++ b/drivers/net/cpfl/cpfl_fxp_rule.c
> @@ -60,6 +60,52 @@ cpfl_send_ctlq_msg(struct idpf_hw *hw, struct idpf_ctlq_info *cq, u16 num_q_msg,
>  	return ret;
>  }
>  
> +static int
> +cpfl_process_rx_ctlq_msg(u16 msg_opcode, u16 msg_status)
> +{
> +	int ret = CPFL_CFG_PKT_ERR_OK;
> +
> +	if (msg_status &&
> +		msg_opcode == cpfl_ctlq_sem_query_rule_hash_addr)
> +		return ret;
> +
> +	switch (msg_status) {
> +	case CPFL_CFG_PKT_ERR_EEXIST:
> +		PMD_INIT_LOG(ERR, "The rule has confliction with already existed one");

"The rule conflicts with an existing one"

> +		ret = CPFL_CFG_PKT_ERR_EEXIST;
> +		break;
> +	case CPFL_CFG_PKT_ERR_ENOSPC:
> +		PMD_INIT_LOG(ERR, "No space left in the table");
> +		ret = CPFL_CFG_PKT_ERR_ENOSPC;
> +		break;
> +	case CPFL_CFG_PKT_ERR_ESRCH:
> +		PMD_INIT_LOG(ERR, "Bad opcode");
> +		ret = CPFL_CFG_PKT_ERR_ESRCH;
> +		break;
> +	case CPFL_CFG_PKT_ERR_ERANGE:
> +		PMD_INIT_LOG(ERR, "Parameter are out of");
> +		ret = CPFL_CFG_PKT_ERR_ERANGE;
> +		break;
> +	case CPFL_CFG_PKT_ERR_ESBCOMP:
> +		PMD_INIT_LOG(ERR, "Completion error");
> +		ret = CPFL_CFG_PKT_ERR_ESBCOMP;
> +		break;
> +	case CPFL_CFG_PKT_ERR_ENOPIN:
> +		PMD_INIT_LOG(ERR, "Entry cannot be pinned in the cache");
> +		ret = CPFL_CFG_PKT_ERR_ENOPIN;
> +		break;
> +	case CPFL_CFG_PKT_ERR_ENOTFND:
> +		PMD_INIT_LOG(ERR, "Entry does not exists");
> +		ret = CPFL_CFG_PKT_ERR_ENOTFND;
> +		break;
> +	case CPFL_CFG_PKT_ERR_EMAXCOL:
> +		PMD_INIT_LOG(ERR, "Maximum number of hash collisions reached");
> +		ret = CPFL_CFG_PKT_ERR_EMAXCOL;
> +		break;
> +	}
> +	return ret;
> +}

This function seems overly long, and doesn't need any branching statements.
Would be shorter rewritten as (not tested, just to give the idea):

{
	const char *errmsgs[] = {
		[CPFL_CFG_PKT_ERR_ESRCH] = "Bad opcode",
		[CPFL_CFG_PKT_ERR_EEXIST] = "The rule conflicts with an existing one"
		....
	};

	if (msg_status == CPFL_CFG_PKT_ERR_OK || msg_opcode =
			cpfl_ctlq_sem_query_rule_hash_addr)
		return 0;

	PMD_INIT_LOG(ERR, "%s", errmsgs[msg_status]);
	return msg_status;
}

While something like above should work, if you don't want to use the
designated initializers for the error messages you can just define them in
a regular array, since the error code are sequential from 1-9.

Also, another suggestion: since we can shorten the code this much, do we
actually need a separate function just to print the error messages. Maybe
define the descriptive text for each error message in cpfl_rules.h beside
the enum and put the error printing directly in cpfl_receive_ctlq_msg()
function.

> +
>  int
>  cpfl_receive_ctlq_msg(struct idpf_hw *hw, struct idpf_ctlq_info *cq, u16 num_q_msg,
>  		      struct idpf_ctlq_msg q_msg[])
> @@ -92,6 +138,12 @@ cpfl_receive_ctlq_msg(struct idpf_hw *hw, struct idpf_ctlq_info *cq, u16 num_q_m
>  
>  		/* TODO - process rx controlq message */
>  		for (i = 0; i < num_q_msg; i++) {
> +			ret = cpfl_process_rx_ctlq_msg(q_msg[i].opcode, q_msg[i].status);
> +			if (ret) {
> +				PMD_INIT_LOG(ERR, "failed to process rx_ctrlq msg");
> +				return ret;
> +			}
> +

Another argument in favour of handling the error message here directly is
that we can avoid having two separate PMD_INIT_LOG error lines. Using a
global array of error messages we can collapse the two into one, which
would read better:

	PMD_INIT_LOG(ERR, "Failed to process rx_ctrlq_msg, %s", errmsg(q_msg[i].status));

giving the nice single-line output e.g.

<PREFIX>: Failed to process rx_ctrlq_msg, bad opcode

NOTE: watch the capitalization for the error message strings in this case.

>  			if (q_msg[i].data_len > 0)
>  				dma = q_msg[i].ctx.indirect.payload;
>  			else
> -- 
> 

Regards,
/Bruce

[PATCH v4 1/2] net/cpfl: fix check for opcodes of received ctlq messages

[Soumyadeep Hore]

Include checks for error status returned for specific
opcodes and display error messages accordingly.

Fixes: db042ef09d26 ("net/cpfl: implement FXP rule creation and destroying")
Cc: stable@dpdk.org

Signed-off-by: Soumyadeep Hore <soumyadeep.hore@intel.com>
---
 drivers/net/cpfl/cpfl_fxp_rule.c |  8 ++++++++
 drivers/net/cpfl/cpfl_rules.h    | 11 +++++++++++
 2 files changed, 19 insertions(+)

diff --git a/drivers/net/cpfl/cpfl_fxp_rule.c b/drivers/net/cpfl/cpfl_fxp_rule.c
index 0e710a007b..f48ecd5656 100644
--- a/drivers/net/cpfl/cpfl_fxp_rule.c
+++ b/drivers/net/cpfl/cpfl_fxp_rule.c
@@ -92,6 +92,14 @@ cpfl_receive_ctlq_msg(struct idpf_hw *hw, struct idpf_ctlq_info *cq, u16 num_q_m
 
 		/* TODO - process rx controlq message */
 		for (i = 0; i < num_q_msg; i++) {
+			ret = q_msg[i].status;
+			if (ret &&
+				q_msg[i].opcode != cpfl_ctlq_sem_query_del_rule_hash_addr) {
+				PMD_INIT_LOG(ERR, "Failed to process rx_ctrlq msg: %s",
+					cpfl_cfg_pkt_errormsg[ret]);
+				return ret;
+			}
+
 			if (q_msg[i].data_len > 0)
 				dma = q_msg[i].ctx.indirect.payload;
 			else
diff --git a/drivers/net/cpfl/cpfl_rules.h b/drivers/net/cpfl/cpfl_rules.h
index d23eae8e91..10569b1fdc 100644
--- a/drivers/net/cpfl/cpfl_rules.h
+++ b/drivers/net/cpfl/cpfl_rules.h
@@ -62,6 +62,17 @@ enum cpfl_cfg_pkt_error_code {
 	CPFL_CFG_PKT_ERR_EMAXCOL = 9    /* Max Hash Collision */
 };
 
+static const char * const cpfl_cfg_pkt_errormsg[] = {
+	[CPFL_CFG_PKT_ERR_ESRCH] = "Bad opcode",
+	[CPFL_CFG_PKT_ERR_EEXIST] = "The rule conflicts with already existed one",
+	[CPFL_CFG_PKT_ERR_ENOSPC] = "No space left in the table",
+	[CPFL_CFG_PKT_ERR_ERANGE] = "Parameter out of range",
+	[CPFL_CFG_PKT_ERR_ESBCOMP] = "Completion error",
+	[CPFL_CFG_PKT_ERR_ENOPIN] = "Entry cannot be pinned in cache",
+	[CPFL_CFG_PKT_ERR_ENOTFND] = "Entry does not exist",
+	[CPFL_CFG_PKT_ERR_EMAXCOL] = "Maximum Hash Collisions reached",
+};
+
 /* macros for creating context for rule descriptor */
 #define MEV_RULE_VSI_ID_S		0
 #define MEV_RULE_VSI_ID_M		\
-- 
2.43.0

[PATCH v4 2/2] net/cpfl: fix +ve error codes for received ctlq messages

[Soumyadeep Hore]

Previously error codes were only -ve for
cpfl_receive_ctlq_msg() but now there are +ve error codes.
Hence code changes are made accordingly.

Fixes: db042ef09d26 ("net/cpfl: implement FXP rule creation and destroying")
Cc: stable@dpdk.org

Signed-off-by: Soumyadeep Hore <soumyadeep.hore@intel.com>
---
 drivers/net/cpfl/cpfl_flow_engine_fxp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/cpfl/cpfl_flow_engine_fxp.c b/drivers/net/cpfl/cpfl_flow_engine_fxp.c
index 39a281fa61..b9e825ef57 100644
--- a/drivers/net/cpfl/cpfl_flow_engine_fxp.c
+++ b/drivers/net/cpfl/cpfl_flow_engine_fxp.c
@@ -95,7 +95,7 @@ cpfl_fxp_create(struct rte_eth_dev *dev,
 
 	ret = cpfl_rule_process(itf, ad->ctlqp[cpq_id], ad->ctlqp[cpq_id + 1],
 				rim->rules, rim->rule_num, true);
-	if (ret < 0) {
+	if (ret != 0) {
 		rte_flow_error_set(error, EINVAL, RTE_FLOW_ERROR_TYPE_HANDLE, NULL,
 				   "cpfl filter create flow fail");
 		rte_free(rim);
-- 
2.43.0

Re: [PATCH v4 1/2] net/cpfl: fix check for opcodes of received ctlq messages

[Bruce Richardson]

On Fri, Jul 05, 2024 at 01:05:14PM +0000, Soumyadeep Hore wrote:
> Include checks for error status returned for specific
> opcodes and display error messages accordingly.
> 
> Fixes: db042ef09d26 ("net/cpfl: implement FXP rule creation and destroying")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Soumyadeep Hore <soumyadeep.hore@intel.com>
> ---
>  drivers/net/cpfl/cpfl_fxp_rule.c |  8 ++++++++
>  drivers/net/cpfl/cpfl_rules.h    | 11 +++++++++++
>  2 files changed, 19 insertions(+)
> 
> diff --git a/drivers/net/cpfl/cpfl_fxp_rule.c b/drivers/net/cpfl/cpfl_fxp_rule.c
> index 0e710a007b..f48ecd5656 100644
> --- a/drivers/net/cpfl/cpfl_fxp_rule.c
> +++ b/drivers/net/cpfl/cpfl_fxp_rule.c
> @@ -92,6 +92,14 @@ cpfl_receive_ctlq_msg(struct idpf_hw *hw, struct idpf_ctlq_info *cq, u16 num_q_m
>  
>  		/* TODO - process rx controlq message */
>  		for (i = 0; i < num_q_msg; i++) {
> +			ret = q_msg[i].status;
> +			if (ret &&

DPDK style guide recommends doing explicit comparisons for conditionals,
rather than relying on non-zero being true. Therefore this would be better
as "if (ret != CPFL_CFG_PKT_ERR_OK &&"

> +				q_msg[i].opcode != cpfl_ctlq_sem_query_del_rule_hash_addr) {

The indentation here is problematic as the line continuation aligns with
the conditional body. Looking at the rest of this file, the continuation
style is that of aligning with opening brackets so that should be used here
too.

> +				PMD_INIT_LOG(ERR, "Failed to process rx_ctrlq msg: %s",
> +					cpfl_cfg_pkt_errormsg[ret]);
> +				return ret;
> +			}
> +
>  			if (q_msg[i].data_len > 0)
>  				dma = q_msg[i].ctx.indirect.payload;
>  			else

If there is no objection, I'll fix both these comments on patch apply.

/Bruce