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: sta...@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.h...@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
