Re: [PATCH for-next] RDMA/bnxt_re: Congestion control settings using debugfs hook

[Leon Romanovsky <leon@kernel.org>]

On Fri, Jan 17, 2025 at 08:29:04PM -0800, Selvin Xavier wrote:
> Implements routines to set and get different settings  of
> the congestion control. This will enable the users to modify
> the settings according to their network.
> 
> Currently supporting only GEN 0 version of the parameters.
> Reading these files queries the firmware and report the values
> currently programmed. Writing to the files sends commands that
> update the congestion control settings.
> 
> Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com>
> ---
>  drivers/infiniband/hw/bnxt_re/bnxt_re.h |   2 +
>  drivers/infiniband/hw/bnxt_re/debugfs.c | 215 +++++++++++++++++++++++++++++++-
>  drivers/infiniband/hw/bnxt_re/debugfs.h |  15 +++
>  3 files changed, 231 insertions(+), 1 deletion(-)

<...>

> +static const char * const bnxt_re_cc_gen0_name[] = {
> +	"enable_cc",
> +	"g",

????

> +	"num_phase_per_state",
> +	"init_cr",
> +	"init_tr",
> +	"tos_ecn",
> +	"tos_dscp",
> +	"alt_vlan_pcp",
> +	"alt_vlan_dscp",
> +	"rtt",
> +	"cc_mode",
> +	"tcp_cp",
> +	"tx_queue",
> +	"inactivity_cp",
> +};

<...>

> +static int map_cc_config_offset_gen0_ext0(u32 offset, struct bnxt_qplib_cc_param *ccparam, u32 *val)
> +{
> +	u64 map_offset;
> +
> +	map_offset = BIT(offset);
> +
> +	switch (map_offset) {
> +	case CMDQ_MODIFY_ROCE_CC_MODIFY_MASK_ENABLE_CC:
> +		*val = ccparam->enable;
> +		break;
> +	case CMDQ_MODIFY_ROCE_CC_MODIFY_MASK_G:
> +		*val = ccparam->g;
> +		break;
> +	case CMDQ_MODIFY_ROCE_CC_MODIFY_MASK_NUMPHASEPERSTATE:
> +		*val = ccparam->nph_per_state;
> +		break;
> +	case CMDQ_MODIFY_ROCE_CC_MODIFY_MASK_INIT_CR:
> +		*val = ccparam->init_cr;
> +		break;
> +	case CMDQ_MODIFY_ROCE_CC_MODIFY_MASK_INIT_TR:
> +	       *val = ccparam->init_tr;
> +		break;
> +	case CMDQ_MODIFY_ROCE_CC_MODIFY_MASK_TOS_ECN:
> +	       *val = ccparam->tos_ecn;
> +		break;
> +	case CMDQ_MODIFY_ROCE_CC_MODIFY_MASK_TOS_DSCP:
> +	       *val =  ccparam->tos_dscp;
> +	break;

Wrong indentations, above and below.

> +	case CMDQ_MODIFY_ROCE_CC_MODIFY_MASK_ALT_VLAN_PCP:
> +		*val = ccparam->alt_vlan_pcp;
> +		break;
> +	case CMDQ_MODIFY_ROCE_CC_MODIFY_MASK_ALT_TOS_DSCP:
> +		*val = ccparam->alt_tos_dscp;
> +		break;
> +	case CMDQ_MODIFY_ROCE_CC_MODIFY_MASK_RTT:
> +	       *val = ccparam->rtt;
> +		break;
> +	case CMDQ_MODIFY_ROCE_CC_MODIFY_MASK_CC_MODE:
> +	      *val = ccparam->cc_mode;
> +		break;
> +	case CMDQ_MODIFY_ROCE_CC_MODIFY_MASK_TCP_CP:
> +	     *val =  ccparam->tcp_cp;
> +		break;
> +	default:
> +	     return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static ssize_t bnxt_re_cc_config_get(struct file *filp, char __user *buffer,
> +				     size_t usr_buf_len, loff_t *ppos)
> +{
> +	struct bnxt_re_cc_param *dbg_cc_param = filp->private_data;
> +	struct bnxt_re_dev *rdev = dbg_cc_param->rdev;
> +	struct bnxt_qplib_cc_param ccparam = {};
> +	u32 offset = dbg_cc_param->offset;
> +	char buf[16];
> +	u32 val;
> +	int rc;
> +
> +	rc = bnxt_qplib_query_cc_param(&rdev->qplib_res, &ccparam);
> +	if (rc) {
> +		dev_err(rdev_to_dev(rdev),
> +			"%s: Failed to query CC parameters\n", __func__);

Let's not have debug print, and especially dev_err here.

> +		return -EINVAL;

bnxt_qplib_query_cc_param() can return ENOMEM, there is no need to
overwrite return value.

> +	}
> +
> +	rc = map_cc_config_offset_gen0_ext0(offset, &ccparam, &val);
> +	if (rc)
> +		return rc;
> +
> +	rc = snprintf(buf, sizeof(buf), "%d\n", val);
> +	if (rc < 0)
> +		return rc;
> +
> +	return simple_read_from_buffer(buffer, usr_buf_len, ppos, (u8 *)(buf), rc);
> +}
> +