Mon, Oct 30, 2017 at 03:46:12PM CET, steven.lin1@broadcom.com wrote:

[...]

+static int bnxt_dl_perm_config_set(struct devlink *devlink,
+				   enum devlink_perm_config_param param,
+				   enum devlink_perm_config_type type,
+				   union devlink_perm_config_value *value,
+				   bool *restart_reqd)
+{
+	struct bnxt *bp = bnxt_get_bp_from_dl(devlink);
+	struct bnxt_drv_cfgparam *entry = NULL;
+	u32 value_int;
+	u32 bytesize;
+	int idx = 0;
+	int ret = 0;
+	u16 val16;
+	u8 val8;
+	int i;
+
+	*restart_reqd = 0;
+
+	/* Find parameter in table */
+	for (i = 0; i < BNXT_NUM_DRV_CFGPARAM; i++) {
+		if (param == bnxt_drv_cfgparam_list[i].param) {
+			entry = &bnxt_drv_cfgparam_list[i];
+			break;
+		}
+	}
+
+	/* Not found */
+	if (!entry)
+		return -EINVAL;
+
+	/* Check to see if this func type can access variable */
+	if (BNXT_PF(bp) && !(entry->func & BNXT_DRV_PF))
+		return -EOPNOTSUPP;
+	if (BNXT_VF(bp) && !(entry->func & BNXT_DRV_VF))
+		return -EOPNOTSUPP;
+
+	/* If parameter is per port or function, compute index */
+	if (entry->appl == BNXT_DRV_APPL_PORT) {
+		idx = bp->pf.port_id;
+	} else if (entry->appl == BNXT_DRV_APPL_FUNCTION) {
+		if (BNXT_PF(bp))
+			idx = bp->pf.fw_fid - BNXT_FIRST_PF_FID;
	}

+	bytesize = roundup(entry->bitlength, BITS_PER_BYTE) / BITS_PER_BYTE;
+
+	/* Type expected by device may or may not be the same as type passed
+	 * in from devlink API.  So first convert to an interval u32 value,
+	 * then to type needed by device.
+	 */
+	if (type == DEVLINK_PERM_CONFIG_TYPE_U8) {
+		value_int = value->value8;
+	} else if (type == DEVLINK_PERM_CONFIG_TYPE_U16) {
+		value_int = value->value16;
+	} else if (type == DEVLINK_PERM_CONFIG_TYPE_U32) {
+		value_int = value->value32;
+	} else {
+		/* Unsupported type */
+		return -EINVAL;
+	}
+
+	if (bytesize == 1) {
+		val8 = value_int;

+		ret = bnxt_nvm_write(bp, entry->nvm_param, idx, &val8,
+				     entry->bitlength);
+	} else if (bytesize == 2) {
+		val16 = value_int;
+		ret = bnxt_nvm_write(bp, entry->nvm_param, idx, &val16,
+				     entry->bitlength);
+	} else {
+		ret = bnxt_nvm_write(bp, entry->nvm_param, idx, &value_int,
+				     entry->bitlength);
+	}
+
+	/* Restart required for all nvm parameter writes */
+	*restart_reqd = 1;
+
+	return ret;
+}

I don't like this swissknife approach for setters and getters. It's
messy, and hard to read. There should be clean get/set pair function
per parameter defined in array.
Something like:

static int bnxt_param_sriov_enabled_get(struct devlink *devlink, bool *p_value)
{
	...
}

static int bnxt_param_sriov_enabled_set(struct devlink *devlink, bool value,
					bool *restart_reqd)
{
	...
}

static int bnxt_param_num_vf_per_pf_get(struct devlink *devlink, u32 *p_value)
{
	...
}

static int bnxt_param_num_vf_per_pf_set(struct devlink *devlink, u32 value,
					bool *restart_reqd)
{
	...
}

static const struct devlink_config_param bnxt_params[] = {
	DEVLINK_CONFIG_PARAM_BOOL(DEVLINK_PERM_CONFIG_SRIOV_ENABLED,
				  bnxt_param_sriov_enabled_get,
				  bnxt_param_sriov_enabled_set),
	DEVLINK_CONFIG_PARAM_U32(DEVLINK_PERM_CONFIG_NUM_VF_PER_PF,
				 bnxt_param_num_vf_per_pf_get,
				 bnxt_param_num_vf_per_pf_set),
};

and then in init:

err = devlink_config_param_register(devlink, bnxt_params,
				    ARRAY_SIZE(bnxt_params));

The register function will check types against the internal devlink
param->type table.

Also, the restart_reqd could be signalized by some pre-defined positive
setter return value.