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

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

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