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

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

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