[PATCH net-next v3 00/10] Adding permanent config get/set to devlink

[PATCH net-next v3 00/10] Adding permanent config get/set to devlink

[Steve Lin]

Changes since v2:

* Removed references to "NVRAM" in comments / commits.
* Add parameter descriptions to header file.
* Split bnxt patch into infrastructure, then one patch
  for each new parameter.
* Cleaned up goofs (unused parameters leftover from v1)
* Used enum rather than u32 in prototype for perm_config_get()
  and _set().
* Defined DEVLINK_ATTR_PERM_CONFIG_TYPE so future parameters
  can use arbitrary types (not just U32 and smaller).
* Reverse Christmas tree local variable definitions.
* No longer return original/previous value of parameter in
  response to set.
* Check parameter within enum limits before using it as array
  index.

I have NOT implemented the following suggested changes:

* Have driver report what parameters and parameter options it
  supports.  Could be done in future patch by defining new
  devlink op.
* Support parameters spread across multi-part netlink messages.
  See discussion on list - this doesn't seem necessary even
  for devices with large number of parameters.
* Support specifying per-VF config, if the VFs don't have
  pci b/d/f associated with them.  See discussion on list;
  if/when this support is required, could add
  DEVLINK_ATTR_PERM_CONFIG_VF_INDEX to describe, without
  breaking UAPI.
* Rolling back previously set parameters in a collection of
  sets, when one fails.  Instead, we report back to user
  which sets were successful, so they know which were set
  and which weren't, and can decide how to proceed.

--

Adds a devlink command for getting & setting permanent /
persistent device configuration parameters, and enumerates
the parameters as nested devlink attributes.

bnxt driver patches makes use of these new devlink cmds.

Steve Lin (10):
  devlink: Add permanent config parameter get/set operations
  devlink: Adding SR-IOV enablement perm config param
  devlink: Adding num VFs per PF permanent config param
  devlink: Adding max PF MSI-X vectors perm config param
  devlink: Adding num MSI-X vectors per VF perm config param
  bnxt: Add devlink support for config get/set
  bnxt: Adding SR-IOV enablement permanent cfg param
  bnxt: Adding num VFs per PF perm config param
  bnxt: Adding max PF MSI-X vectors perm config param
  bnxt: Adding num MSI-X vectors per VF perm config param

 drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 281 +++++++++++++++++++-
 drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h |  17 ++
 drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h     | 100 ++++++++
 include/net/devlink.h                             |   6 +
 include/uapi/linux/devlink.h                      |  33 +++
 net/core/devlink.c                                | 299 ++++++++++++++++++++++
 6 files changed, 730 insertions(+), 6 deletions(-)

-- 
2.7.4

[PATCH net-next v3 01/10] devlink: Add permanent config parameter get/set operations

[Steve Lin]

Add support for permanent config parameter get/set commands. Used
for persistent device configuration parameters.

Signed-off-by: Steve Lin <steven.lin1@broadcom.com>
Acked-by: Andy Gospodarek <gospo@broadcom.com>
---
 include/net/devlink.h        |   6 +
 include/uapi/linux/devlink.h |  18 +++
 net/core/devlink.c           | 295 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 319 insertions(+)

diff --git a/include/net/devlink.h b/include/net/devlink.h
index b9654e1..eb86031 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -270,6 +270,12 @@ struct devlink_ops {
 	int (*eswitch_inline_mode_set)(struct devlink *devlink, u8 inline_mode);
 	int (*eswitch_encap_mode_get)(struct devlink *devlink, u8 *p_encap_mode);
 	int (*eswitch_encap_mode_set)(struct devlink *devlink, u8 encap_mode);
+	int (*perm_config_get)(struct devlink *devlink,
+			       enum devlink_perm_config_param param, u8 type,
+			       void *value);
+	int (*perm_config_set)(struct devlink *devlink,
+			       enum devlink_perm_config_param param, u8 type,
+			       void *value, u8 *restart_reqd);
 };
 
 static inline void *devlink_priv(struct devlink *devlink)
diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
index 0cbca96..28ea961 100644
--- a/include/uapi/linux/devlink.h
+++ b/include/uapi/linux/devlink.h
@@ -70,6 +70,10 @@ enum devlink_command {
 	DEVLINK_CMD_DPIPE_HEADERS_GET,
 	DEVLINK_CMD_DPIPE_TABLE_COUNTERS_SET,
 
+	/* Permanent device config get/set */
+	DEVLINK_CMD_PERM_CONFIG_GET,
+	DEVLINK_CMD_PERM_CONFIG_SET,
+
 	/* add new commands above here */
 	__DEVLINK_CMD_MAX,
 	DEVLINK_CMD_MAX = __DEVLINK_CMD_MAX - 1
@@ -202,6 +206,14 @@ enum devlink_attr {
 
 	DEVLINK_ATTR_ESWITCH_ENCAP_MODE,	/* u8 */
 
+	/* Permanent Configuration Parameters */
+	DEVLINK_ATTR_PERM_CONFIGS,			/* nested */
+	DEVLINK_ATTR_PERM_CONFIG,			/* nested */
+	DEVLINK_ATTR_PERM_CONFIG_PARAMETER,		/* u32 */
+	DEVLINK_ATTR_PERM_CONFIG_TYPE,			/* u8 */
+	DEVLINK_ATTR_PERM_CONFIG_VALUE,			/* dynamic */
+	DEVLINK_ATTR_PERM_CONFIG_RESTART_REQUIRED,	/* u8 */
+
 	/* add new attributes above here, update the policy in devlink.c */
 
 	__DEVLINK_ATTR_MAX,
@@ -244,4 +256,10 @@ enum devlink_dpipe_header_id {
 	DEVLINK_DPIPE_HEADER_IPV6,
 };
 
+/* Permanent config parameters */
+enum devlink_perm_config_param {
+	__DEVLINK_PERM_CONFIG_MAX,
+	DEVLINK_PERM_CONFIG_MAX = __DEVLINK_PERM_CONFIG_MAX - 1
+};
+
 #endif /* _UAPI_LINUX_DEVLINK_H_ */
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 7d430c1..4deb4da 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -1566,6 +1566,285 @@ static int devlink_nl_cmd_eswitch_set_doit(struct sk_buff *skb,
 	return 0;
 }
 
+static const struct nla_policy devlink_nl_policy[DEVLINK_ATTR_MAX + 1];
+
+static const u8 devlink_perm_cfg_param_types[DEVLINK_PERM_CONFIG_MAX + 1] = {
+};
+
+static int devlink_nl_single_param_get(struct sk_buff *msg,
+				       struct devlink *devlink,
+				       u32 param, u8 type)
+{
+	const struct devlink_ops *ops = devlink->ops;
+	struct nlattr *param_attr;
+	void *value;
+	u32 val;
+	int err;
+
+	/* Allocate buffer for parameter value */
+	switch (type) {
+	case NLA_U8:
+		value = kmalloc(sizeof(u8), GFP_KERNEL);
+		break;
+	case NLA_U16:
+		value = kmalloc(sizeof(u16), GFP_KERNEL);
+		break;
+	case NLA_U32:
+		value = kmalloc(sizeof(u32), GFP_KERNEL);
+		break;
+	default:
+		return -EINVAL; /* Unsupported Type */
+	}
+
+	if (!value)
+		return -ENOMEM;
+
+	err = ops->perm_config_get(devlink, param, type, value);
+	if (err)
+		return err;
+
+	param_attr = nla_nest_start(msg, DEVLINK_ATTR_PERM_CONFIG);
+	if (!param_attr)
+		goto nonest_err;
+
+	if (nla_put_u32(msg, DEVLINK_ATTR_PERM_CONFIG_PARAMETER, param) ||
+	    nla_put_u8(msg, DEVLINK_ATTR_PERM_CONFIG_TYPE, type))
+		goto nest_err;
+
+	switch (type) {
+	case NLA_U8:
+		val = *((u8 *)value);
+		if (nla_put_u8(msg, DEVLINK_ATTR_PERM_CONFIG_VALUE, val))
+			goto nest_err;
+		break;
+	case NLA_U16:
+		val = *((u16 *)value);
+		if (nla_put_u16(msg, DEVLINK_ATTR_PERM_CONFIG_VALUE, val))
+			goto nest_err;
+		break;
+	case NLA_U32:
+		val = *((u32 *)value);
+		if (nla_put_u32(msg, DEVLINK_ATTR_PERM_CONFIG_VALUE, val))
+			goto nest_err;
+		break;
+	}
+	nla_nest_end(msg, param_attr);
+
+	kfree(value);
+
+	return err;
+
+nest_err:
+	nla_nest_cancel(msg, param_attr);
+nonest_err:
+	kfree(value);
+
+	return -EMSGSIZE;
+}
+
+static int devlink_nl_config_get_fill(struct sk_buff *msg,
+				      struct devlink *devlink,
+				      enum devlink_command cmd,
+				      struct genl_info *info)
+{
+	struct nlattr *tb[DEVLINK_ATTR_MAX + 1];
+	struct nlattr *cfgparam_attr;
+	struct nlattr *attr;
+	void *hdr;
+	u32 param;
+	int err;
+	int rem;
+	u8 type;
+
+	hdr = genlmsg_put(msg, info->snd_portid, info->snd_seq,
+			  &devlink_nl_family, 0, cmd);
+	if (!hdr)
+		return -EMSGSIZE;
+
+	err = devlink_nl_put_handle(msg, devlink);
+	if (err)
+		goto nla_put_failure;
+
+	if (!info->attrs[DEVLINK_ATTR_PERM_CONFIGS]) {
+		err = -EINVAL;
+		goto nla_put_failure;
+	}
+
+	cfgparam_attr = nla_nest_start(msg, DEVLINK_ATTR_PERM_CONFIGS);
+
+	nla_for_each_nested(attr, info->attrs[DEVLINK_ATTR_PERM_CONFIGS],
+			    rem) {
+		err = nla_parse_nested(tb, DEVLINK_ATTR_MAX, attr,
+				       devlink_nl_policy, NULL);
+		if (err)
+			goto nla_nest_failure;
+		if (!tb[DEVLINK_ATTR_PERM_CONFIG_PARAMETER] ||
+		    !tb[DEVLINK_ATTR_PERM_CONFIG_TYPE])
+			continue;
+
+		param = nla_get_u32(tb[DEVLINK_ATTR_PERM_CONFIG_PARAMETER]);
+		type = nla_get_u8(tb[DEVLINK_ATTR_PERM_CONFIG_TYPE]);
+		if (param > DEVLINK_PERM_CONFIG_MAX ||
+		    type > NLA_TYPE_MAX) {
+			continue;
+		}
+		/* Note if single_param_get fails, that param won't be in
+		 * response msg, so caller will know which param(s) failed to
+		 * get.
+		 */
+		devlink_nl_single_param_get(msg, devlink, param, type);
+	}
+
+	nla_nest_end(msg, cfgparam_attr);
+
+	genlmsg_end(msg, hdr);
+	return 0;
+
+nla_nest_failure:
+	nla_nest_cancel(msg, cfgparam_attr);
+nla_put_failure:
+	genlmsg_cancel(msg, hdr);
+	return err;
+}
+
+static int devlink_nl_cmd_perm_config_get_doit(struct sk_buff *skb,
+					       struct genl_info *info)
+{
+	struct devlink *devlink = info->user_ptr[0];
+	struct sk_buff *msg;
+	int err;
+
+	if (!devlink->ops || !devlink->ops->perm_config_get)
+		return -EOPNOTSUPP;
+
+	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
+	if (!msg)
+		return -ENOMEM;
+
+	err = devlink_nl_config_get_fill(msg, devlink,
+					 DEVLINK_CMD_PERM_CONFIG_GET, info);
+
+	if (err) {
+		nlmsg_free(msg);
+		return err;
+	}
+
+	return genlmsg_reply(msg, info);
+}
+
+static int devlink_nl_single_param_set(struct sk_buff *msg,
+				       struct devlink *devlink,
+				       u32 param, u8 type, void *value)
+{
+	const struct devlink_ops *ops = devlink->ops;
+	struct nlattr *cfgparam_attr;
+	u8 need_restart;
+	int err;
+
+	/* Now set parameter */
+	err = ops->perm_config_set(devlink, param, type, value, &need_restart);
+	if (err)
+		return err;
+
+	cfgparam_attr = nla_nest_start(msg, DEVLINK_ATTR_PERM_CONFIG);
+	/* Update restart reqd - if any param needs restart, should be set */
+	if (need_restart) {
+		err = nla_put_u8(msg,
+				 DEVLINK_ATTR_PERM_CONFIG_RESTART_REQUIRED, 1);
+		if (err)
+			goto nest_fail;
+	}
+
+	/* Since set was successful, write attr back to msg */
+	err = nla_put_u32(msg, DEVLINK_ATTR_PERM_CONFIG_PARAMETER, param);
+	if (err)
+		goto nest_fail;
+
+	nla_nest_end(msg, cfgparam_attr);
+
+	return 0;
+
+nest_fail:
+	nla_nest_cancel(msg, cfgparam_attr);
+	return err;
+}
+
+static int devlink_nl_cmd_perm_config_set_doit(struct sk_buff *skb,
+					       struct genl_info *info)
+{
+	struct devlink *devlink = info->user_ptr[0];
+	struct nlattr *tb[DEVLINK_ATTR_MAX + 1];
+	struct nlattr *cfgparam_attr;
+	struct sk_buff *msg;
+	struct nlattr *attr;
+	void *value;
+	void *hdr;
+	u32 param;
+	u8 type;
+	int rem;
+	int err;
+
+	if (!devlink->ops || !devlink->ops->perm_config_get ||
+	    !devlink->ops->perm_config_set)
+		return -EOPNOTSUPP;
+
+	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
+	if (!msg)
+		return -ENOMEM;
+
+	hdr = genlmsg_put(msg, info->snd_portid, info->snd_seq,
+			  &devlink_nl_family, 0, DEVLINK_CMD_PERM_CONFIG_SET);
+	if (!hdr) {
+		err = -EMSGSIZE;
+		goto nla_msg_failure;
+	}
+
+	err = devlink_nl_put_handle(msg, devlink);
+	if (err)
+		goto nla_put_failure;
+
+	cfgparam_attr = nla_nest_start(msg, DEVLINK_ATTR_PERM_CONFIGS);
+
+	nla_for_each_nested(attr, info->attrs[DEVLINK_ATTR_PERM_CONFIGS], rem) {
+		err = nla_parse_nested(tb, DEVLINK_ATTR_MAX, attr,
+				       devlink_nl_policy, NULL);
+		if (err)
+			goto nla_nest_failure;
+
+		if (!tb[DEVLINK_ATTR_PERM_CONFIG_PARAMETER] ||
+		    !tb[DEVLINK_ATTR_PERM_CONFIG_TYPE] ||
+		    !tb[DEVLINK_ATTR_PERM_CONFIG_VALUE])
+			continue;
+
+		param = nla_get_u32(tb[DEVLINK_ATTR_PERM_CONFIG_PARAMETER]);
+		type = nla_get_u8(tb[DEVLINK_ATTR_PERM_CONFIG_TYPE]);
+		if (param > DEVLINK_PERM_CONFIG_MAX ||
+		    type > NLA_TYPE_MAX) {
+			continue;
+		}
+
+		value = nla_data(tb[DEVLINK_ATTR_PERM_CONFIG_VALUE]);
+
+		/* Note if single_param_set fails, that param won't be in
+		 * response msg, so caller will know which param(s) failed to
+		 * set.
+		 */
+		devlink_nl_single_param_set(msg, devlink, param, type, value);
+	}
+
+	nla_nest_end(msg, cfgparam_attr);
+
+	genlmsg_end(msg, hdr);
+	return genlmsg_reply(msg, info);
+
+nla_nest_failure:
+	nla_nest_cancel(msg, cfgparam_attr);
+nla_put_failure:
+	genlmsg_cancel(msg, hdr);
+nla_msg_failure:
+	return err;
+}
+
 int devlink_dpipe_match_put(struct sk_buff *skb,
 			    struct devlink_dpipe_match *match)
 {
@@ -2291,6 +2570,8 @@ static const struct nla_policy devlink_nl_policy[DEVLINK_ATTR_MAX + 1] = {
 	[DEVLINK_ATTR_ESWITCH_ENCAP_MODE] = { .type = NLA_U8 },
 	[DEVLINK_ATTR_DPIPE_TABLE_NAME] = { .type = NLA_NUL_STRING },
 	[DEVLINK_ATTR_DPIPE_TABLE_COUNTERS_ENABLED] = { .type = NLA_U8 },
+	[DEVLINK_ATTR_PERM_CONFIG_PARAMETER] = { .type = NLA_U32 },
+	[DEVLINK_ATTR_PERM_CONFIG_TYPE] = { .type = NLA_U8 },
 };
 
 static const struct genl_ops devlink_nl_ops[] = {
@@ -2451,6 +2732,20 @@ static const struct genl_ops devlink_nl_ops[] = {
 		.flags = GENL_ADMIN_PERM,
 		.internal_flags = DEVLINK_NL_FLAG_NEED_DEVLINK,
 	},
+	{
+		.cmd = DEVLINK_CMD_PERM_CONFIG_GET,
+		.doit = devlink_nl_cmd_perm_config_get_doit,
+		.policy = devlink_nl_policy,
+		.flags = GENL_ADMIN_PERM,
+		.internal_flags = DEVLINK_NL_FLAG_NEED_DEVLINK,
+	},
+	{
+		.cmd = DEVLINK_CMD_PERM_CONFIG_SET,
+		.doit = devlink_nl_cmd_perm_config_set_doit,
+		.policy = devlink_nl_policy,
+		.flags = GENL_ADMIN_PERM,
+		.internal_flags = DEVLINK_NL_FLAG_NEED_DEVLINK,
+	},
 };
 
 static struct genl_family devlink_nl_family __ro_after_init = {
-- 
2.7.4

RE: [PATCH net-next v3 01/10] devlink: Add permanent config parameter get/set operations

[Yuval Mintz]

> Add support for permanent config parameter get/set commands. Used
> for persistent device configuration parameters.
> 
...
> +	int (*perm_config_get)(struct devlink *devlink,
> +			       enum devlink_perm_config_param param, u8
> type,
> +			       void *value);
> +	int (*perm_config_set)(struct devlink *devlink,
> +			       enum devlink_perm_config_param param, u8
> type,
> +			       void *value, u8 *restart_reqd);
>  };
> +static int devlink_nl_single_param_get(struct sk_buff *msg,
> +				       struct devlink *devlink,
> +				       u32 param, u8 type)
> +{
> +	const struct devlink_ops *ops = devlink->ops;
> +	struct nlattr *param_attr;
> +	void *value;
> +	u32 val;
> +	int err;
> +
> +	/* Allocate buffer for parameter value */
> +	switch (type) {
> +	case NLA_U8:
> +		value = kmalloc(sizeof(u8), GFP_KERNEL);
> +		break;
> +	case NLA_U16:
> +		value = kmalloc(sizeof(u16), GFP_KERNEL);
> +		break;
> +	case NLA_U32:
> +		value = kmalloc(sizeof(u32), GFP_KERNEL);
> +		break;
> +	default:
> +		return -EINVAL; /* Unsupported Type */
> +	}
> +
> +	if (!value)
> +		return -ENOMEM;
> +
> +	err = ops->perm_config_get(devlink, param, type, value);
> +	if (err)
> +		return err;

I suspect this logic might be risky - its dependent on the driver to cast the
'value' into the proper type or else, E.g., the following switch might break
for BE platforms.
Is there any reason to have the devlink <-> driver API be based on void*
and not on some typed data [of sufficient size]?
...
> +	switch (type) {
> +	case NLA_U8:
> +		val = *((u8 *)value);
> +		if (nla_put_u8(msg, DEVLINK_ATTR_PERM_CONFIG_VALUE,
> val))
> +			goto nest_err;
> +		break;
> +	case NLA_U16:
> +		val = *((u16 *)value);
> +		if (nla_put_u16(msg,
> DEVLINK_ATTR_PERM_CONFIG_VALUE, val))
> +			goto nest_err;
> +		break;
> +	case NLA_U32:
> +		val = *((u32 *)value);
> +		if (nla_put_u32(msg,
> DEVLINK_ATTR_PERM_CONFIG_VALUE, val))
> +			goto nest_err;
> +		break;
> +	}

...
> +static int devlink_nl_single_param_set(struct sk_buff *msg,
> +				       struct devlink *devlink,
> +				       u32 param, u8 type, void *value)
> +{
> +	const struct devlink_ops *ops = devlink->ops;
> +	struct nlattr *cfgparam_attr;
> +	u8 need_restart;
> +	int err;
> +
> +	/* Now set parameter */
> +	err = ops->perm_config_set(devlink, param, type, value,
> &need_restart);
> +	if (err)
> +		return err;

Likewise

Re: [PATCH net-next v3 01/10] devlink: Add permanent config parameter get/set operations

[Steve Lin]

On Tue, Oct 24, 2017 at 5:22 PM, Yuval Mintz <yuvalm@mellanox.com> wrote:
>> Add support for permanent config parameter get/set commands. Used
>> for persistent device configuration parameters.
>>
> ...
>> +     int (*perm_config_get)(struct devlink *devlink,
>> +                            enum devlink_perm_config_param param, u8
>> type,
>> +                            void *value);
>> +     int (*perm_config_set)(struct devlink *devlink,
>> +                            enum devlink_perm_config_param param, u8
>> type,
>> +                            void *value, u8 *restart_reqd);
>>  };
>> +static int devlink_nl_single_param_get(struct sk_buff *msg,
>> +                                    struct devlink *devlink,
>> +                                    u32 param, u8 type)
>> +{
>> +     const struct devlink_ops *ops = devlink->ops;
>> +     struct nlattr *param_attr;
>> +     void *value;
>> +     u32 val;
>> +     int err;
>> +
>> +     /* Allocate buffer for parameter value */
>> +     switch (type) {
>> +     case NLA_U8:
>> +             value = kmalloc(sizeof(u8), GFP_KERNEL);
>> +             break;
>> +     case NLA_U16:
>> +             value = kmalloc(sizeof(u16), GFP_KERNEL);
>> +             break;
>> +     case NLA_U32:
>> +             value = kmalloc(sizeof(u32), GFP_KERNEL);
>> +             break;
>> +     default:
>> +             return -EINVAL; /* Unsupported Type */
>> +     }
>> +
>> +     if (!value)
>> +             return -ENOMEM;
>> +
>> +     err = ops->perm_config_get(devlink, param, type, value);
>> +     if (err)
>> +             return err;
>
> I suspect this logic might be risky - its dependent on the driver to cast the
> 'value' into the proper type or else, E.g., the following switch might break
> for BE platforms.
> Is there any reason to have the devlink <-> driver API be based on void*
> and not on some typed data [of sufficient size]?
> ...
>> +     switch (type) {
>> +     case NLA_U8:
>> +             val = *((u8 *)value);
>> +             if (nla_put_u8(msg, DEVLINK_ATTR_PERM_CONFIG_VALUE,
>> val))
>> +                     goto nest_err;
>> +             break;
>> +     case NLA_U16:
>> +             val = *((u16 *)value);
>> +             if (nla_put_u16(msg,
>> DEVLINK_ATTR_PERM_CONFIG_VALUE, val))
>> +                     goto nest_err;
>> +             break;
>> +     case NLA_U32:
>> +             val = *((u32 *)value);
>> +             if (nla_put_u32(msg,
>> DEVLINK_ATTR_PERM_CONFIG_VALUE, val))
>> +                     goto nest_err;
>> +             break;
>> +     }

Why might this break on a BE system?  It's not as though driver will
be compiled LE and kernel BE or vice versa - as long as driver and
kernel are same endian-ness, I would think it should be okay?

In general, the issue is that the parameter could be any of the
netlink types (per Jiri's suggestion to the previous version of this
patch).  So, we allocate some space, tell the driver the type we're
expecting (the type argument to the perm_config_get() function), and
yes, we rely on the driver to write something of the type we request
to the pointer we provide.  Are you suggesting defining a union of
U8/U16/U32, and passing a pointer to that for the driver to fill in?
The issue is that whatever the types we support now, we want future
parameters to be able to be of arbitrary types.  Defining the
interface to use the void pointer means that some future parameter can
be of some other type, without having to update all the drivers using
this API...

Or did I misunderstand your suggestion?

RE: [PATCH net-next v3 01/10] devlink: Add permanent config parameter get/set operations

[Yuval Mintz]

> On Tue, Oct 24, 2017 at 5:22 PM, Yuval Mintz <yuvalm@mellanox.com>
> wrote:
> >> Add support for permanent config parameter get/set commands. Used
> >> for persistent device configuration parameters.
> >>
> > ...
> >> +     int (*perm_config_get)(struct devlink *devlink,
> >> +                            enum devlink_perm_config_param param, u8
> >> type,
> >> +                            void *value);
> >> +     int (*perm_config_set)(struct devlink *devlink,
> >> +                            enum devlink_perm_config_param param, u8
> >> type,
> >> +                            void *value, u8 *restart_reqd);
> >>  };
> >> +static int devlink_nl_single_param_get(struct sk_buff *msg,
> >> +                                    struct devlink *devlink,
> >> +                                    u32 param, u8 type)
> >> +{
> >> +     const struct devlink_ops *ops = devlink->ops;
> >> +     struct nlattr *param_attr;
> >> +     void *value;
> >> +     u32 val;
> >> +     int err;
> >> +
> >> +     /* Allocate buffer for parameter value */
> >> +     switch (type) {
> >> +     case NLA_U8:
> >> +             value = kmalloc(sizeof(u8), GFP_KERNEL);
> >> +             break;
> >> +     case NLA_U16:
> >> +             value = kmalloc(sizeof(u16), GFP_KERNEL);
> >> +             break;
> >> +     case NLA_U32:
> >> +             value = kmalloc(sizeof(u32), GFP_KERNEL);
> >> +             break;
> >> +     default:
> >> +             return -EINVAL; /* Unsupported Type */
> >> +     }
> >> +
> >> +     if (!value)
> >> +             return -ENOMEM;
> >> +
> >> +     err = ops->perm_config_get(devlink, param, type, value);
> >> +     if (err)
> >> +             return err;
> >
> > I suspect this logic might be risky - its dependent on the driver to cast the
> > 'value' into the proper type or else, E.g., the following switch might break
> > for BE platforms.
> > Is there any reason to have the devlink <-> driver API be based on void*
> > and not on some typed data [of sufficient size]?
> > ...
> >> +     switch (type) {
> >> +     case NLA_U8:
> >> +             val = *((u8 *)value);
> >> +             if (nla_put_u8(msg, DEVLINK_ATTR_PERM_CONFIG_VALUE,
> >> val))
> >> +                     goto nest_err;
> >> +             break;
> >> +     case NLA_U16:
> >> +             val = *((u16 *)value);
> >> +             if (nla_put_u16(msg,
> >> DEVLINK_ATTR_PERM_CONFIG_VALUE, val))
> >> +                     goto nest_err;
> >> +             break;
> >> +     case NLA_U32:
> >> +             val = *((u32 *)value);
> >> +             if (nla_put_u32(msg,
> >> DEVLINK_ATTR_PERM_CONFIG_VALUE, val))
> >> +                     goto nest_err;
> >> +             break;
> >> +     }
> 
> Why might this break on a BE system?  It's not as though driver will
> be compiled LE and kernel BE or vice versa - as long as driver and
> kernel are same endian-ness, I would think it should be okay?

It depends on the driver implementation to cast your pointer to the right type.
E.g., driver needs to fill in a u8 data in *value for a given parameter.
If the driver casted the pointer to (u8*) everything is fine. But if he casted
it to (u32*) [naïve implementation that doesn't care about the type]
and filled it, then on a LE machine value[0] would contain the data while on
BE value[3] would contain it.


> 
> In general, the issue is that the parameter could be any of the
> netlink types (per Jiri's suggestion to the previous version of this
> patch).  So, we allocate some space, tell the driver the type we're
> expecting (the type argument to the perm_config_get() function), and
> yes, we rely on the driver to write something of the type we request
> to the pointer we provide.  Are you suggesting defining a union of
> U8/U16/U32, and passing a pointer to that for the driver to fill in?

Problem is that the driver-side could always use the biggest data-type
as long as it's working on a LE machine, but that approach would break
if same driver would be tried on a BE machine.
And the developer would have no way of knowing other than via code-review.

> The issue is that whatever the types we support now, we want future
> parameters to be able to be of arbitrary types.  Defining the
> interface to use the void pointer means that some future parameter can
> be of some other type, without having to update all the drivers using
> this API...
> 
> Or did I misunderstand your suggestion?

Re: [PATCH net-next v3 01/10] devlink: Add permanent config parameter get/set operations

[David Miller]

From: Steve Lin <steven.lin1@broadcom.com>
Date: Tue, 24 Oct 2017 16:12:33 -0400

> +	switch (type) {
> +	case NLA_U8:
> +		val = *((u8 *)value);
> +		if (nla_put_u8(msg, DEVLINK_ATTR_PERM_CONFIG_VALUE, val))
> +			goto nest_err;
> +		break;
> +	case NLA_U16:
> +		val = *((u16 *)value);
> +		if (nla_put_u16(msg, DEVLINK_ATTR_PERM_CONFIG_VALUE, val))
> +			goto nest_err;
> +		break;
> +	case NLA_U32:
> +		val = *((u32 *)value);
> +		if (nla_put_u32(msg, DEVLINK_ATTR_PERM_CONFIG_VALUE, val))
> +			goto nest_err;
> +		break;
> +	}
> +	nla_nest_end(msg, param_attr);
> +
> +	kfree(value);

You have to get the endianness right on these things.  Netlink could
theoretically be done over a network, so just saying "device and
system endianness match" is not a valid argument.

Typing and endianness is so important for interfaces like this, so
please contruct the interfaces such that the compiler and 'sparse' can
help us make sure it is done properly.

Thanks.

Re: [PATCH net-next v3 01/10] devlink: Add permanent config parameter get/set operations

[Jiri Pirko]

Tue, Oct 24, 2017 at 10:12:33PM CEST, steven.lin1@broadcom.com wrote:
>Add support for permanent config parameter get/set commands. Used
>for persistent device configuration parameters.
>
>Signed-off-by: Steve Lin <steven.lin1@broadcom.com>
>Acked-by: Andy Gospodarek <gospo@broadcom.com>
>---
> include/net/devlink.h        |   6 +
> include/uapi/linux/devlink.h |  18 +++
> net/core/devlink.c           | 295 +++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 319 insertions(+)
>
>diff --git a/include/net/devlink.h b/include/net/devlink.h
>index b9654e1..eb86031 100644
>--- a/include/net/devlink.h
>+++ b/include/net/devlink.h
>@@ -270,6 +270,12 @@ struct devlink_ops {
> 	int (*eswitch_inline_mode_set)(struct devlink *devlink, u8 inline_mode);
> 	int (*eswitch_encap_mode_get)(struct devlink *devlink, u8 *p_encap_mode);
> 	int (*eswitch_encap_mode_set)(struct devlink *devlink, u8 encap_mode);
>+	int (*perm_config_get)(struct devlink *devlink,
>+			       enum devlink_perm_config_param param, u8 type,
>+			       void *value);

Please don't use "void *" here. use union instead.


>+	int (*perm_config_set)(struct devlink *devlink,
>+			       enum devlink_perm_config_param param, u8 type,
>+			       void *value, u8 *restart_reqd);
> };
> 
> static inline void *devlink_priv(struct devlink *devlink)
>diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
>index 0cbca96..28ea961 100644
>--- a/include/uapi/linux/devlink.h
>+++ b/include/uapi/linux/devlink.h
>@@ -70,6 +70,10 @@ enum devlink_command {
> 	DEVLINK_CMD_DPIPE_HEADERS_GET,
> 	DEVLINK_CMD_DPIPE_TABLE_COUNTERS_SET,
> 
>+	/* Permanent device config get/set */
>+	DEVLINK_CMD_PERM_CONFIG_GET,
>+	DEVLINK_CMD_PERM_CONFIG_SET,
>+
> 	/* add new commands above here */
> 	__DEVLINK_CMD_MAX,
> 	DEVLINK_CMD_MAX = __DEVLINK_CMD_MAX - 1
>@@ -202,6 +206,14 @@ enum devlink_attr {
> 
> 	DEVLINK_ATTR_ESWITCH_ENCAP_MODE,	/* u8 */
> 
>+	/* Permanent Configuration Parameters */
>+	DEVLINK_ATTR_PERM_CONFIGS,			/* nested */
>+	DEVLINK_ATTR_PERM_CONFIG,			/* nested */
>+	DEVLINK_ATTR_PERM_CONFIG_PARAMETER,		/* u32 */
>+	DEVLINK_ATTR_PERM_CONFIG_TYPE,			/* u8 */
>+	DEVLINK_ATTR_PERM_CONFIG_VALUE,			/* dynamic */
>+	DEVLINK_ATTR_PERM_CONFIG_RESTART_REQUIRED,	/* u8 */
>+
> 	/* add new attributes above here, update the policy in devlink.c */
> 
> 	__DEVLINK_ATTR_MAX,
>@@ -244,4 +256,10 @@ enum devlink_dpipe_header_id {
> 	DEVLINK_DPIPE_HEADER_IPV6,
> };
> 
>+/* Permanent config parameters */
>+enum devlink_perm_config_param {
>+	__DEVLINK_PERM_CONFIG_MAX,
>+	DEVLINK_PERM_CONFIG_MAX = __DEVLINK_PERM_CONFIG_MAX - 1
>+};
>+
> #endif /* _UAPI_LINUX_DEVLINK_H_ */
>diff --git a/net/core/devlink.c b/net/core/devlink.c
>index 7d430c1..4deb4da 100644
>--- a/net/core/devlink.c
>+++ b/net/core/devlink.c
>@@ -1566,6 +1566,285 @@ static int devlink_nl_cmd_eswitch_set_doit(struct sk_buff *skb,
> 	return 0;
> }
> 
>+static const struct nla_policy devlink_nl_policy[DEVLINK_ATTR_MAX + 1];
>+
>+static const u8 devlink_perm_cfg_param_types[DEVLINK_PERM_CONFIG_MAX + 1] = {
>+};
>+
>+static int devlink_nl_single_param_get(struct sk_buff *msg,
>+				       struct devlink *devlink,
>+				       u32 param, u8 type)
>+{
>+	const struct devlink_ops *ops = devlink->ops;
>+	struct nlattr *param_attr;
>+	void *value;
>+	u32 val;
>+	int err;
>+
>+	/* Allocate buffer for parameter value */
>+	switch (type) {
>+	case NLA_U8:
>+		value = kmalloc(sizeof(u8), GFP_KERNEL);
>+		break;
>+	case NLA_U16:
>+		value = kmalloc(sizeof(u16), GFP_KERNEL);
>+		break;
>+	case NLA_U32:
>+		value = kmalloc(sizeof(u32), GFP_KERNEL);
>+		break;
>+	default:
>+		return -EINVAL; /* Unsupported Type */
>+	}
>+
>+	if (!value)
>+		return -ENOMEM;
>+
>+	err = ops->perm_config_get(devlink, param, type, value);
>+	if (err)
>+		return err;
>+
>+	param_attr = nla_nest_start(msg, DEVLINK_ATTR_PERM_CONFIG);
>+	if (!param_attr)
>+		goto nonest_err;
>+
>+	if (nla_put_u32(msg, DEVLINK_ATTR_PERM_CONFIG_PARAMETER, param) ||
>+	    nla_put_u8(msg, DEVLINK_ATTR_PERM_CONFIG_TYPE, type))
>+		goto nest_err;
>+
>+	switch (type) {
>+	case NLA_U8:
>+		val = *((u8 *)value);
>+		if (nla_put_u8(msg, DEVLINK_ATTR_PERM_CONFIG_VALUE, val))
>+			goto nest_err;
>+		break;
>+	case NLA_U16:
>+		val = *((u16 *)value);
>+		if (nla_put_u16(msg, DEVLINK_ATTR_PERM_CONFIG_VALUE, val))
>+			goto nest_err;
>+		break;
>+	case NLA_U32:
>+		val = *((u32 *)value);
>+		if (nla_put_u32(msg, DEVLINK_ATTR_PERM_CONFIG_VALUE, val))
>+			goto nest_err;
>+		break;
>+	}
>+	nla_nest_end(msg, param_attr);
>+
>+	kfree(value);
>+
>+	return err;
>+
>+nest_err:
>+	nla_nest_cancel(msg, param_attr);
>+nonest_err:
>+	kfree(value);
>+
>+	return -EMSGSIZE;
>+}
>+
>+static int devlink_nl_config_get_fill(struct sk_buff *msg,
>+				      struct devlink *devlink,
>+				      enum devlink_command cmd,
>+				      struct genl_info *info)
>+{
>+	struct nlattr *tb[DEVLINK_ATTR_MAX + 1];
>+	struct nlattr *cfgparam_attr;
>+	struct nlattr *attr;
>+	void *hdr;
>+	u32 param;
>+	int err;
>+	int rem;
>+	u8 type;
>+
>+	hdr = genlmsg_put(msg, info->snd_portid, info->snd_seq,
>+			  &devlink_nl_family, 0, cmd);
>+	if (!hdr)
>+		return -EMSGSIZE;
>+
>+	err = devlink_nl_put_handle(msg, devlink);
>+	if (err)
>+		goto nla_put_failure;
>+
>+	if (!info->attrs[DEVLINK_ATTR_PERM_CONFIGS]) {
>+		err = -EINVAL;
>+		goto nla_put_failure;
>+	}
>+
>+	cfgparam_attr = nla_nest_start(msg, DEVLINK_ATTR_PERM_CONFIGS);
>+
>+	nla_for_each_nested(attr, info->attrs[DEVLINK_ATTR_PERM_CONFIGS],
>+			    rem) {
>+		err = nla_parse_nested(tb, DEVLINK_ATTR_MAX, attr,
>+				       devlink_nl_policy, NULL);
>+		if (err)
>+			goto nla_nest_failure;
>+		if (!tb[DEVLINK_ATTR_PERM_CONFIG_PARAMETER] ||
>+		    !tb[DEVLINK_ATTR_PERM_CONFIG_TYPE])
>+			continue;
>+
>+		param = nla_get_u32(tb[DEVLINK_ATTR_PERM_CONFIG_PARAMETER]);
>+		type = nla_get_u8(tb[DEVLINK_ATTR_PERM_CONFIG_TYPE]);
>+		if (param > DEVLINK_PERM_CONFIG_MAX ||
>+		    type > NLA_TYPE_MAX) {
>+			continue;
>+		}
>+		/* Note if single_param_get fails, that param won't be in
>+		 * response msg, so caller will know which param(s) failed to
>+		 * get.
>+		 */
>+		devlink_nl_single_param_get(msg, devlink, param, type);
>+	}
>+
>+	nla_nest_end(msg, cfgparam_attr);
>+
>+	genlmsg_end(msg, hdr);
>+	return 0;
>+
>+nla_nest_failure:
>+	nla_nest_cancel(msg, cfgparam_attr);
>+nla_put_failure:
>+	genlmsg_cancel(msg, hdr);
>+	return err;
>+}
>+
>+static int devlink_nl_cmd_perm_config_get_doit(struct sk_buff *skb,
>+					       struct genl_info *info)
>+{
>+	struct devlink *devlink = info->user_ptr[0];
>+	struct sk_buff *msg;
>+	int err;
>+
>+	if (!devlink->ops || !devlink->ops->perm_config_get)
>+		return -EOPNOTSUPP;
>+
>+	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
>+	if (!msg)
>+		return -ENOMEM;
>+
>+	err = devlink_nl_config_get_fill(msg, devlink,
>+					 DEVLINK_CMD_PERM_CONFIG_GET, info);
>+
>+	if (err) {
>+		nlmsg_free(msg);
>+		return err;
>+	}
>+
>+	return genlmsg_reply(msg, info);
>+}
>+
>+static int devlink_nl_single_param_set(struct sk_buff *msg,
>+				       struct devlink *devlink,
>+				       u32 param, u8 type, void *value)
>+{
>+	const struct devlink_ops *ops = devlink->ops;
>+	struct nlattr *cfgparam_attr;
>+	u8 need_restart;
>+	int err;
>+
>+	/* Now set parameter */
>+	err = ops->perm_config_set(devlink, param, type, value, &need_restart);
>+	if (err)
>+		return err;
>+
>+	cfgparam_attr = nla_nest_start(msg, DEVLINK_ATTR_PERM_CONFIG);
>+	/* Update restart reqd - if any param needs restart, should be set */
>+	if (need_restart) {
>+		err = nla_put_u8(msg,
>+				 DEVLINK_ATTR_PERM_CONFIG_RESTART_REQUIRED, 1);
>+		if (err)
>+			goto nest_fail;
>+	}
>+
>+	/* Since set was successful, write attr back to msg */
>+	err = nla_put_u32(msg, DEVLINK_ATTR_PERM_CONFIG_PARAMETER, param);
>+	if (err)
>+		goto nest_fail;
>+
>+	nla_nest_end(msg, cfgparam_attr);
>+
>+	return 0;
>+
>+nest_fail:
>+	nla_nest_cancel(msg, cfgparam_attr);
>+	return err;
>+}
>+
>+static int devlink_nl_cmd_perm_config_set_doit(struct sk_buff *skb,
>+					       struct genl_info *info)
>+{
>+	struct devlink *devlink = info->user_ptr[0];
>+	struct nlattr *tb[DEVLINK_ATTR_MAX + 1];
>+	struct nlattr *cfgparam_attr;
>+	struct sk_buff *msg;
>+	struct nlattr *attr;
>+	void *value;
>+	void *hdr;
>+	u32 param;
>+	u8 type;
>+	int rem;
>+	int err;
>+
>+	if (!devlink->ops || !devlink->ops->perm_config_get ||
>+	    !devlink->ops->perm_config_set)
>+		return -EOPNOTSUPP;
>+
>+	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
>+	if (!msg)
>+		return -ENOMEM;
>+
>+	hdr = genlmsg_put(msg, info->snd_portid, info->snd_seq,
>+			  &devlink_nl_family, 0, DEVLINK_CMD_PERM_CONFIG_SET);
>+	if (!hdr) {
>+		err = -EMSGSIZE;
>+		goto nla_msg_failure;
>+	}
>+
>+	err = devlink_nl_put_handle(msg, devlink);
>+	if (err)
>+		goto nla_put_failure;
>+
>+	cfgparam_attr = nla_nest_start(msg, DEVLINK_ATTR_PERM_CONFIGS);
>+
>+	nla_for_each_nested(attr, info->attrs[DEVLINK_ATTR_PERM_CONFIGS], rem) {
>+		err = nla_parse_nested(tb, DEVLINK_ATTR_MAX, attr,
>+				       devlink_nl_policy, NULL);
>+		if (err)
>+			goto nla_nest_failure;
>+
>+		if (!tb[DEVLINK_ATTR_PERM_CONFIG_PARAMETER] ||
>+		    !tb[DEVLINK_ATTR_PERM_CONFIG_TYPE] ||
>+		    !tb[DEVLINK_ATTR_PERM_CONFIG_VALUE])
>+			continue;
>+
>+		param = nla_get_u32(tb[DEVLINK_ATTR_PERM_CONFIG_PARAMETER]);
>+		type = nla_get_u8(tb[DEVLINK_ATTR_PERM_CONFIG_TYPE]);
>+		if (param > DEVLINK_PERM_CONFIG_MAX ||
>+		    type > NLA_TYPE_MAX) {
>+			continue;
>+		}
>+
>+		value = nla_data(tb[DEVLINK_ATTR_PERM_CONFIG_VALUE]);
>+
>+		/* Note if single_param_set fails, that param won't be in
>+		 * response msg, so caller will know which param(s) failed to
>+		 * set.
>+		 */
>+		devlink_nl_single_param_set(msg, devlink, param, type, value);
>+	}
>+
>+	nla_nest_end(msg, cfgparam_attr);
>+
>+	genlmsg_end(msg, hdr);
>+	return genlmsg_reply(msg, info);
>+
>+nla_nest_failure:
>+	nla_nest_cancel(msg, cfgparam_attr);
>+nla_put_failure:
>+	genlmsg_cancel(msg, hdr);
>+nla_msg_failure:
>+	return err;
>+}
>+
> int devlink_dpipe_match_put(struct sk_buff *skb,
> 			    struct devlink_dpipe_match *match)
> {
>@@ -2291,6 +2570,8 @@ static const struct nla_policy devlink_nl_policy[DEVLINK_ATTR_MAX + 1] = {
> 	[DEVLINK_ATTR_ESWITCH_ENCAP_MODE] = { .type = NLA_U8 },
> 	[DEVLINK_ATTR_DPIPE_TABLE_NAME] = { .type = NLA_NUL_STRING },
> 	[DEVLINK_ATTR_DPIPE_TABLE_COUNTERS_ENABLED] = { .type = NLA_U8 },
>+	[DEVLINK_ATTR_PERM_CONFIG_PARAMETER] = { .type = NLA_U32 },
>+	[DEVLINK_ATTR_PERM_CONFIG_TYPE] = { .type = NLA_U8 },
> };
> 
> static const struct genl_ops devlink_nl_ops[] = {
>@@ -2451,6 +2732,20 @@ static const struct genl_ops devlink_nl_ops[] = {
> 		.flags = GENL_ADMIN_PERM,
> 		.internal_flags = DEVLINK_NL_FLAG_NEED_DEVLINK,
> 	},
>+	{
>+		.cmd = DEVLINK_CMD_PERM_CONFIG_GET,
>+		.doit = devlink_nl_cmd_perm_config_get_doit,
>+		.policy = devlink_nl_policy,
>+		.flags = GENL_ADMIN_PERM,
>+		.internal_flags = DEVLINK_NL_FLAG_NEED_DEVLINK,
>+	},
>+	{
>+		.cmd = DEVLINK_CMD_PERM_CONFIG_SET,
>+		.doit = devlink_nl_cmd_perm_config_set_doit,
>+		.policy = devlink_nl_policy,
>+		.flags = GENL_ADMIN_PERM,
>+		.internal_flags = DEVLINK_NL_FLAG_NEED_DEVLINK,
>+	},
> };
> 
> static struct genl_family devlink_nl_family __ro_after_init = {
>-- 
>2.7.4
>

[PATCH net-next v3 02/10] devlink: Adding SR-IOV enablement perm config param

[Steve Lin]

Adding DEVLINK_PERM_CONFIG_SRIOV_ENABLED permanent config
parameter.  Value is permanent, so becomes the new default
value for this device.

  0 = Disable SR-IOV
  1 = Enable SR-IOV

Signed-off-by: Steve Lin <steven.lin1@broadcom.com>
Acked-by: Andy Gospodarek <gospo@broadcom.com>
---
 include/uapi/linux/devlink.h | 8 +++++++-
 net/core/devlink.c           | 1 +
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
index 28ea961..ed520e7 100644
--- a/include/uapi/linux/devlink.h
+++ b/include/uapi/linux/devlink.h
@@ -256,8 +256,14 @@ enum devlink_dpipe_header_id {
 	DEVLINK_DPIPE_HEADER_IPV6,
 };
 
-/* Permanent config parameters */
+/* Permanent config parameters:
+ * DEVLINK_PERM_CONFIG_SRIOV_ENABLED:
+ *   0 = disable SR-IOV
+ *   1 = enable SR-IOV
+ */
 enum devlink_perm_config_param {
+	DEVLINK_PERM_CONFIG_SRIOV_ENABLED,
+
 	__DEVLINK_PERM_CONFIG_MAX,
 	DEVLINK_PERM_CONFIG_MAX = __DEVLINK_PERM_CONFIG_MAX - 1
 };
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 4deb4da..58ba715 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -1569,6 +1569,7 @@ static int devlink_nl_cmd_eswitch_set_doit(struct sk_buff *skb,
 static const struct nla_policy devlink_nl_policy[DEVLINK_ATTR_MAX + 1];
 
 static const u8 devlink_perm_cfg_param_types[DEVLINK_PERM_CONFIG_MAX + 1] = {
+	[DEVLINK_PERM_CONFIG_SRIOV_ENABLED] = NLA_U8,
 };
 
 static int devlink_nl_single_param_get(struct sk_buff *msg,
-- 
2.7.4

RE: [PATCH net-next v3 02/10] devlink: Adding SR-IOV enablement perm config param

[Yuval Mintz]

> Adding DEVLINK_PERM_CONFIG_SRIOV_ENABLED permanent config
> parameter.  Value is permanent, so becomes the new default
> value for this device.
> 
>   0 = Disable SR-IOV
>   1 = Enable SR-IOV

Does this imposes a requirement on the PCIe specifics, E.g., that the device
should no longer expose the SRIOV PCie capability?
Or does any implementation that would prevent a user from activating
SR-IOV is sufficient?

Re: [PATCH net-next v3 02/10] devlink: Adding SR-IOV enablement perm config param

[Jiri Pirko]

Tue, Oct 24, 2017 at 10:12:34PM CEST, steven.lin1@broadcom.com wrote:
>Adding DEVLINK_PERM_CONFIG_SRIOV_ENABLED permanent config
>parameter.  Value is permanent, so becomes the new default
>value for this device.
>
>  0 = Disable SR-IOV
>  1 = Enable SR-IOV
>
>Signed-off-by: Steve Lin <steven.lin1@broadcom.com>
>Acked-by: Andy Gospodarek <gospo@broadcom.com>
>---
> include/uapi/linux/devlink.h | 8 +++++++-
> net/core/devlink.c           | 1 +
> 2 files changed, 8 insertions(+), 1 deletion(-)
>
>diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
>index 28ea961..ed520e7 100644
>--- a/include/uapi/linux/devlink.h
>+++ b/include/uapi/linux/devlink.h
>@@ -256,8 +256,14 @@ enum devlink_dpipe_header_id {
> 	DEVLINK_DPIPE_HEADER_IPV6,
> };
> 
>-/* Permanent config parameters */
>+/* Permanent config parameters:
>+ * DEVLINK_PERM_CONFIG_SRIOV_ENABLED:
>+ *   0 = disable SR-IOV
>+ *   1 = enable SR-IOV

Enum please. Also, I would like to see more detailed description,
including description how exactly the driver is expected to behave.
Current description is still quite vague.


>+ */
> enum devlink_perm_config_param {
>+	DEVLINK_PERM_CONFIG_SRIOV_ENABLED,
>+
> 	__DEVLINK_PERM_CONFIG_MAX,
> 	DEVLINK_PERM_CONFIG_MAX = __DEVLINK_PERM_CONFIG_MAX - 1
> };
>diff --git a/net/core/devlink.c b/net/core/devlink.c
>index 4deb4da..58ba715 100644
>--- a/net/core/devlink.c
>+++ b/net/core/devlink.c
>@@ -1569,6 +1569,7 @@ static int devlink_nl_cmd_eswitch_set_doit(struct sk_buff *skb,
> static const struct nla_policy devlink_nl_policy[DEVLINK_ATTR_MAX + 1];
> 
> static const u8 devlink_perm_cfg_param_types[DEVLINK_PERM_CONFIG_MAX + 1] = {
>+	[DEVLINK_PERM_CONFIG_SRIOV_ENABLED] = NLA_U8,
> };
> 
> static int devlink_nl_single_param_get(struct sk_buff *msg,
>-- 
>2.7.4
>

[PATCH net-next v3 03/10] devlink: Adding num VFs per PF permanent config param

[Steve Lin]

Adding DEVLINK_PERM_CONFIG_NUM_VF_PER_PF permanent config
parameter.  Value is permanent, so becomes the new default
value for this device.

The value sets the number of VFs per PF in SR-IOV mode.

Signed-off-by: Steve Lin <steven.lin1@broadcom.com>
Acked-by: Andy Gospodarek <gospo@broadcom.com>
---
 include/uapi/linux/devlink.h | 3 +++
 net/core/devlink.c           | 1 +
 2 files changed, 4 insertions(+)

diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
index ed520e7..db512c5 100644
--- a/include/uapi/linux/devlink.h
+++ b/include/uapi/linux/devlink.h
@@ -260,9 +260,12 @@ enum devlink_dpipe_header_id {
  * DEVLINK_PERM_CONFIG_SRIOV_ENABLED:
  *   0 = disable SR-IOV
  *   1 = enable SR-IOV
+ * DEVLINK_PERM_CONFIG_NUM_VF_PER_PF:
+ *   # of VFs per PF in SR-IOV mode
  */
 enum devlink_perm_config_param {
 	DEVLINK_PERM_CONFIG_SRIOV_ENABLED,
+	DEVLINK_PERM_CONFIG_NUM_VF_PER_PF,
 
 	__DEVLINK_PERM_CONFIG_MAX,
 	DEVLINK_PERM_CONFIG_MAX = __DEVLINK_PERM_CONFIG_MAX - 1
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 58ba715..18f2600 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -1570,6 +1570,7 @@ static const struct nla_policy devlink_nl_policy[DEVLINK_ATTR_MAX + 1];
 
 static const u8 devlink_perm_cfg_param_types[DEVLINK_PERM_CONFIG_MAX + 1] = {
 	[DEVLINK_PERM_CONFIG_SRIOV_ENABLED] = NLA_U8,
+	[DEVLINK_PERM_CONFIG_NUM_VF_PER_PF] = NLA_U32,
 };
 
 static int devlink_nl_single_param_get(struct sk_buff *msg,
-- 
2.7.4

RE: [PATCH net-next v3 03/10] devlink: Adding num VFs per PF permanent config param

[Yuval Mintz]

> Adding DEVLINK_PERM_CONFIG_NUM_VF_PER_PF permanent config
> parameter.  Value is permanent, so becomes the new default
> value for this device.
> 
> The value sets the number of VFs per PF in SR-IOV mode.

Assuming it's meant to directly control the PCIe capability value
I think you should mention it explicitly in the commit message.

[PATCH net-next v3 04/10] devlink: Adding max PF MSI-X vectors perm config param

[Steve Lin]

Adding DEVLINK_PERM_CONFIG_MAX_NUM_PF_MSIX_VECT permanent config
parameter.  Value is permanent, so becomes the new default value
for this device.

Value sets the maximum number of PF MSI-X vectors.

Signed-off-by: Steve Lin <steven.lin1@broadcom.com>
Acked-by: Andy Gospodarek <gospo@broadcom.com>
---
 include/uapi/linux/devlink.h | 3 +++
 net/core/devlink.c           | 1 +
 2 files changed, 4 insertions(+)

diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
index db512c5..5877ff9 100644
--- a/include/uapi/linux/devlink.h
+++ b/include/uapi/linux/devlink.h
@@ -262,10 +262,13 @@ enum devlink_dpipe_header_id {
  *   1 = enable SR-IOV
  * DEVLINK_PERM_CONFIG_NUM_VF_PER_PF:
  *   # of VFs per PF in SR-IOV mode
+ * DEVLINK_PERM_CONFIG_MAX_NUM_PF_MSIX_VECT:
+ *   Max # of MSI-X vectors per PF
  */
 enum devlink_perm_config_param {
 	DEVLINK_PERM_CONFIG_SRIOV_ENABLED,
 	DEVLINK_PERM_CONFIG_NUM_VF_PER_PF,
+	DEVLINK_PERM_CONFIG_MAX_NUM_PF_MSIX_VECT,
 
 	__DEVLINK_PERM_CONFIG_MAX,
 	DEVLINK_PERM_CONFIG_MAX = __DEVLINK_PERM_CONFIG_MAX - 1
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 18f2600..284e167 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -1571,6 +1571,7 @@ static const struct nla_policy devlink_nl_policy[DEVLINK_ATTR_MAX + 1];
 static const u8 devlink_perm_cfg_param_types[DEVLINK_PERM_CONFIG_MAX + 1] = {
 	[DEVLINK_PERM_CONFIG_SRIOV_ENABLED] = NLA_U8,
 	[DEVLINK_PERM_CONFIG_NUM_VF_PER_PF] = NLA_U32,
+	[DEVLINK_PERM_CONFIG_MAX_NUM_PF_MSIX_VECT] = NLA_U32,
 };
 
 static int devlink_nl_single_param_get(struct sk_buff *msg,
-- 
2.7.4

[PATCH net-next v3 05/10] devlink: Adding num MSI-X vectors per VF perm config param

[Steve Lin]

Adding DEVLINK_PERM_CONFIG_MSIX_VECTORS_PER_VF permanent config
parameter.  Value is permanent, so becomes the new default value
for this device.

Value defines number of MSI-X vectors allocated per VF.

Signed-off-by: Steve Lin <steven.lin1@broadcom.com>
Acked-by: Andy Gospodarek <gospo@broadcom.com>
---
 include/uapi/linux/devlink.h | 3 +++
 net/core/devlink.c           | 1 +
 2 files changed, 4 insertions(+)

diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
index 5877ff9..1b76e8f 100644
--- a/include/uapi/linux/devlink.h
+++ b/include/uapi/linux/devlink.h
@@ -264,11 +264,14 @@ enum devlink_dpipe_header_id {
  *   # of VFs per PF in SR-IOV mode
  * DEVLINK_PERM_CONFIG_MAX_NUM_PF_MSIX_VECT:
  *   Max # of MSI-X vectors per PF
+ * DEVLINK_PERM_CONFIG_MSIX_VECTORS_PER_VF:
+ *   # of MSI-X vectors per VF
  */
 enum devlink_perm_config_param {
 	DEVLINK_PERM_CONFIG_SRIOV_ENABLED,
 	DEVLINK_PERM_CONFIG_NUM_VF_PER_PF,
 	DEVLINK_PERM_CONFIG_MAX_NUM_PF_MSIX_VECT,
+	DEVLINK_PERM_CONFIG_MSIX_VECTORS_PER_VF,
 
 	__DEVLINK_PERM_CONFIG_MAX,
 	DEVLINK_PERM_CONFIG_MAX = __DEVLINK_PERM_CONFIG_MAX - 1
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 284e167..07d64da 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -1572,6 +1572,7 @@ static const u8 devlink_perm_cfg_param_types[DEVLINK_PERM_CONFIG_MAX + 1] = {
 	[DEVLINK_PERM_CONFIG_SRIOV_ENABLED] = NLA_U8,
 	[DEVLINK_PERM_CONFIG_NUM_VF_PER_PF] = NLA_U32,
 	[DEVLINK_PERM_CONFIG_MAX_NUM_PF_MSIX_VECT] = NLA_U32,
+	[DEVLINK_PERM_CONFIG_MSIX_VECTORS_PER_VF] = NLA_U32,
 };
 
 static int devlink_nl_single_param_get(struct sk_buff *msg,
-- 
2.7.4

[PATCH net-next v3 06/10] bnxt: Add devlink support for config get/set

[Steve Lin]

Implements get and set of configuration parameters using new devlink
config get/set API.  Parameters themselves defined in later patches.

Signed-off-by: Steve Lin <steven.lin1@broadcom.com>
Acked-by: Andy Gospodarek <gospo@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 262 +++++++++++++++++++++-
 drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h |  17 ++
 drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h     | 100 +++++++++
 3 files changed, 373 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
index f3f6aa8..81ab77e 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
@@ -14,11 +14,261 @@
 #include "bnxt_vfr.h"
 #include "bnxt_devlink.h"
 
-static const struct devlink_ops bnxt_dl_ops = {
+struct bnxt_drv_cfgparam bnxt_drv_cfgparam_list[] = {
+};
+
+#define BNXT_NUM_DRV_CFGPARAM ARRAY_SIZE(bnxt_drv_cfgparam_list)
+
+static int bnxt_nvm_read(struct bnxt *bp, int nvm_param, int idx,
+			 void *buf, int size)
+{
+	struct hwrm_nvm_get_variable_input req = {0};
+	dma_addr_t dest_data_dma_addr;
+	void *dest_data_addr = NULL;
+	int bytesize;
+	int rc;
+
+	bytesize = (size + 7) / BITS_PER_BYTE;
+	dest_data_addr = dma_alloc_coherent(&bp->pdev->dev, bytesize,
+					    &dest_data_dma_addr, GFP_KERNEL);
+	if (!dest_data_addr) {
+		netdev_err(bp->dev, "dma_alloc_coherent failure\n");
+		return -ENOMEM;
+	}
+
+	bnxt_hwrm_cmd_hdr_init(bp, &req, HWRM_NVM_GET_VARIABLE, -1, -1);
+	req.dest_data_addr = cpu_to_le64(dest_data_dma_addr);
+	req.data_len = cpu_to_le16(size);
+	req.option_num = cpu_to_le16(nvm_param);
+	req.index_0 = cpu_to_le16(idx);
+	if (idx != 0)
+		req.dimensions = cpu_to_le16(1);
+
+	rc = _hwrm_send_message(bp, &req, sizeof(req), HWRM_CMD_TIMEOUT);
+
+	memcpy(buf, dest_data_addr, bytesize);
+
+	dma_free_coherent(&bp->pdev->dev, bytesize, dest_data_addr,
+			  dest_data_dma_addr);
+
+	return rc;
+}
+
+static int bnxt_nvm_write(struct bnxt *bp, int nvm_param, int idx,
+			  const void *buf, int size)
+{
+	struct hwrm_nvm_set_variable_input req = {0};
+	dma_addr_t src_data_dma_addr;
+	void *src_data_addr = NULL;
+	int bytesize;
+	int rc;
+
+	bytesize = (size + 7) / BITS_PER_BYTE;
+
+	src_data_addr = dma_alloc_coherent(&bp->pdev->dev, bytesize,
+					   &src_data_dma_addr, GFP_KERNEL);
+	if (!src_data_addr) {
+		netdev_err(bp->dev, "dma_alloc_coherent failure\n");
+		return -ENOMEM;
+	}
+
+	memcpy(src_data_addr, buf, bytesize);
+
+	bnxt_hwrm_cmd_hdr_init(bp, &req, HWRM_NVM_SET_VARIABLE, -1, -1);
+	req.src_data_addr = cpu_to_le64(src_data_dma_addr);
+	req.data_len = cpu_to_le16(size);
+	req.option_num = cpu_to_le16(nvm_param);
+	req.index_0 = cpu_to_le16(idx);
+	if (idx != 0)
+		req.dimensions = cpu_to_le16(1);
+
+	rc = _hwrm_send_message(bp, &req, sizeof(req), HWRM_CMD_TIMEOUT);
+
+	dma_free_coherent(&bp->pdev->dev, bytesize, src_data_addr,
+			  src_data_dma_addr);
+
+	return 0;
+}
+
+static int bnxt_dl_perm_config_set(struct devlink *devlink,
+				   enum devlink_perm_config_param param,
+				   u8 type, void *value, u8 *restart_reqd)
+{
+	struct bnxt *bp = bnxt_get_bp_from_dl(devlink);
+	struct bnxt_drv_cfgparam *entry;
+	int idx = 0;
+	int ret = 0;
+	u32 bytesize;
+	u32 val32;
+	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 (i == BNXT_NUM_DRV_CFGPARAM)
+		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 = (entry->bitlength + 7) / BITS_PER_BYTE;
+
+	/* If passed in type matches bytesize, pass value directly */
+	if ((bytesize == 1 && type == NLA_U8) ||
+	    (bytesize == 2 && type == NLA_U16) ||
+	    (bytesize == 4 && type == NLA_U32)) {
+		ret = bnxt_nvm_write(bp, entry->nvm_param, idx, value,
+				     entry->bitlength);
+	} else {
+		/* Otherwise copy value to u32, then to driver type */
+		if (type == NLA_U8) {
+			memcpy(&val8, value, sizeof(val8));
+			val32 = val8;
+		} else if (type == NLA_U16) {
+			memcpy(&val16, value, sizeof(val16));
+			val32 = val16;
+		} else if (type == NLA_U32) {
+			memcpy(&val32, value, sizeof(val32));
+		} else {
+			/* Unsupported type */
+			return -EINVAL;
+		}
+
+		if (bytesize == 1) {
+			val8 = val32;
+			ret = bnxt_nvm_write(bp, entry->nvm_param, idx, &val8,
+					     entry->bitlength);
+		} else if (bytesize == 2) {
+			val16 = val32;
+			ret = bnxt_nvm_write(bp, entry->nvm_param, idx, &val16,
+					     entry->bitlength);
+		} else {
+			ret = bnxt_nvm_write(bp, entry->nvm_param, idx, &val32,
+					     entry->bitlength);
+		}
+	}
+
+	/* Restart required for all nvm parameter writes */
+	*restart_reqd = 1;
+
+	return ret;
+}
+
+static int bnxt_dl_perm_config_get(struct devlink *devlink,
+				   enum devlink_perm_config_param param,
+				   u8 type, void *value)
+{
+	struct bnxt *bp = bnxt_get_bp_from_dl(devlink);
+	struct bnxt_drv_cfgparam *entry;
+	u32 bytesize;
+	int idx = 0;
+	int err = 0;
+	void *data;
+	u32 val32;
+	u16 val16;
+	u8 val8;
+	int i;
+
+	/* 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 (i == BNXT_NUM_DRV_CFGPARAM)
+		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;
+	}
+
+	/* Allocate space, retrieve value, and copy to result */
+	bytesize = (entry->bitlength + 7) / BITS_PER_BYTE;
+	data = kmalloc(bytesize, GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+	err = bnxt_nvm_read(bp, entry->nvm_param, idx, data, entry->bitlength);
+	if (err)
+		goto err_data;
+
+	/* if bytesize matches type requested, just copy data */
+	if ((bytesize == 1 && type == NLA_U8) ||
+	    (bytesize == 2 && type == NLA_U16) ||
+	    (bytesize == 4 && type == NLA_U32)) {
+		memcpy(value, data, bytesize);
+	} else {
+		/* Otherwise copy data to u32, then to requested type */
+		if (bytesize == 1) {
+			memcpy(&val8, data, sizeof(val8));
+			val32 = val8;
+		} else if (bytesize == 2) {
+			memcpy(&val16, data, sizeof(val16));
+			val32 = val16;
+		} else {
+			memcpy(&val32, data, sizeof(val32));
+		}
+
+		if (type == NLA_U8) {
+			val8 = val32;
+			memcpy(value, &val8, sizeof(val8));
+		} else if (type == NLA_U16) {
+			val16 = val32;
+			memcpy(value, &val16, sizeof(val16));
+		} else if (type == NLA_U32) {
+			memcpy(value, &val32, sizeof(val32));
+		} else {
+			/* Unsupported type */
+			err = -EINVAL;
+		}
+	}
+
+err_data:
+	kfree(data);
+
+	return err;
+}
+
+static struct devlink_ops bnxt_dl_ops = {
 #ifdef CONFIG_BNXT_SRIOV
 	.eswitch_mode_set = bnxt_dl_eswitch_mode_set,
 	.eswitch_mode_get = bnxt_dl_eswitch_mode_get,
 #endif /* CONFIG_BNXT_SRIOV */
+	.perm_config_get = bnxt_dl_perm_config_get,
+	.perm_config_set = bnxt_dl_perm_config_set,
 };
 
 int bnxt_dl_register(struct bnxt *bp)
@@ -26,12 +276,12 @@ int bnxt_dl_register(struct bnxt *bp)
 	struct devlink *dl;
 	int rc;
 
-	if (!pci_find_ext_capability(bp->pdev, PCI_EXT_CAP_ID_SRIOV))
-		return 0;
-
-	if (bp->hwrm_spec_code < 0x10800) {
+	if ((!pci_find_ext_capability(bp->pdev, PCI_EXT_CAP_ID_SRIOV)) ||
+	    bp->hwrm_spec_code < 0x10800) {
+		/* eswitch switchdev mode not supported */
+		bnxt_dl_ops.eswitch_mode_set = NULL;
+		bnxt_dl_ops.eswitch_mode_get = NULL;
 		netdev_warn(bp->dev, "Firmware does not support SR-IOV E-Switch SWITCHDEV mode.\n");
-		return -ENOTSUPP;
 	}
 
 	dl = devlink_alloc(&bnxt_dl_ops, sizeof(struct bnxt_dl));
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h
index e92a35d..d843a81 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h
@@ -10,6 +10,23 @@
 #ifndef BNXT_DEVLINK_H
 #define BNXT_DEVLINK_H
 
+#define BNXT_DRV_PF 1
+#define BNXT_DRV_VF 2
+
+enum bnxt_drv_appl {
+	BNXT_DRV_APPL_SHARED,
+	BNXT_DRV_APPL_PORT,
+	BNXT_DRV_APPL_FUNCTION
+};
+
+struct bnxt_drv_cfgparam {
+	enum devlink_perm_config_param	param;
+	u8			func; /* BNXT_DRV_PF | BNXT_DRV_VF */
+	enum bnxt_drv_appl	appl; /* applicability (shared, func, port) */
+	u32			bitlength; /* length, in bits */
+	u32			nvm_param;
+};
+
 /* Struct to hold housekeeping info needed by devlink interface */
 struct bnxt_dl {
 	struct bnxt *bp;	/* back ptr to the controlling dev */
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h
index cb04cc7..8c25731 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h
@@ -5676,6 +5676,106 @@ struct hwrm_nvm_install_update_cmd_err {
 	u8 unused_0[7];
 };
 
+/* hwrm_nvm_get_variable */
+/* Input (40 bytes) */
+struct hwrm_nvm_get_variable_input {
+	__le16 req_type;
+	__le16 cmpl_ring;
+	__le16 seq_id;
+	__le16 target_id;
+	__le64 resp_addr;
+	__le64 dest_data_addr;
+	__le16 data_len;
+	__le16 option_num;
+	#define NVM_GET_VARIABLE_REQ_OPTION_NUM_RSVD_0		   0x0UL
+	#define NVM_GET_VARIABLE_REQ_OPTION_NUM_RSVD_FFFF	   0xffffUL
+	__le16 dimensions;
+	__le16 index_0;
+	__le16 index_1;
+	__le16 index_2;
+	__le16 index_3;
+	u8 flags;
+	#define NVM_GET_VARIABLE_REQ_FLAGS_FACTORY_DFLT	    0x1UL
+	u8 unused_0;
+};
+
+/* Output (16 bytes) */
+struct hwrm_nvm_get_variable_output {
+	__le16 error_code;
+	__le16 req_type;
+	__le16 seq_id;
+	__le16 resp_len;
+	__le16 data_len;
+	__le16 option_num;
+	#define NVM_GET_VARIABLE_RESP_OPTION_NUM_RSVD_0	   0x0UL
+	#define NVM_GET_VARIABLE_RESP_OPTION_NUM_RSVD_FFFF	   0xffffUL
+	u8 unused_0;
+	u8 unused_1;
+	u8 unused_2;
+	u8 valid;
+};
+
+/* Command specific Error Codes (8 bytes) */
+struct hwrm_nvm_get_variable_cmd_err {
+	u8 code;
+	#define NVM_GET_VARIABLE_CMD_ERR_CODE_UNKNOWN		   0x0UL
+	#define NVM_GET_VARIABLE_CMD_ERR_CODE_VAR_NOT_EXIST       0x1UL
+	#define NVM_GET_VARIABLE_CMD_ERR_CODE_CORRUPT_VAR	   0x2UL
+	#define NVM_GET_VARIABLE_CMD_ERR_CODE_LEN_TOO_SHORT       0x3UL
+	u8 unused_0[7];
+};
+
+/* hwrm_nvm_set_variable */
+/* Input (40 bytes) */
+struct hwrm_nvm_set_variable_input {
+	__le16 req_type;
+	__le16 cmpl_ring;
+	__le16 seq_id;
+	__le16 target_id;
+	__le64 resp_addr;
+	__le64 src_data_addr;
+	__le16 data_len;
+	__le16 option_num;
+	#define NVM_SET_VARIABLE_REQ_OPTION_NUM_RSVD_0		   0x0UL
+	#define NVM_SET_VARIABLE_REQ_OPTION_NUM_RSVD_FFFF	   0xffffUL
+	__le16 dimensions;
+	__le16 index_0;
+	__le16 index_1;
+	__le16 index_2;
+	__le16 index_3;
+	u8 flags;
+	#define NVM_SET_VARIABLE_REQ_FLAGS_FORCE_FLUSH		    0x1UL
+	#define NVM_SET_VARIABLE_REQ_FLAGS_ENCRYPT_MODE_MASK       0xeUL
+	#define NVM_SET_VARIABLE_REQ_FLAGS_ENCRYPT_MODE_SFT	    1
+	#define NVM_SET_VARIABLE_REQ_FLAGS_ENCRYPT_MODE_NONE      (0x0UL << 1)
+	#define NVM_SET_VARIABLE_REQ_FLAGS_ENCRYPT_MODE_HMAC_SHA1 (0x1UL << 1)
+	#define NVM_SET_VARIABLE_REQ_FLAGS_ENCRYPT_MODE_LAST \
+		NVM_SET_VARIABLE_REQ_FLAGS_ENCRYPT_MODE_HMAC_SHA1
+	u8 unused_0;
+};
+
+/* Output (16 bytes) */
+struct hwrm_nvm_set_variable_output {
+	__le16 error_code;
+	__le16 req_type;
+	__le16 seq_id;
+	__le16 resp_len;
+	__le32 unused_0;
+	u8 unused_1;
+	u8 unused_2;
+	u8 unused_3;
+	u8 valid;
+};
+
+/* Command specific Error Codes (8 bytes) */
+struct hwrm_nvm_set_variable_cmd_err {
+	u8 code;
+	#define NVM_SET_VARIABLE_CMD_ERR_CODE_UNKNOWN		   0x0UL
+	#define NVM_SET_VARIABLE_CMD_ERR_CODE_VAR_NOT_EXIST       0x1UL
+	#define NVM_SET_VARIABLE_CMD_ERR_CODE_CORRUPT_VAR	   0x2UL
+	u8 unused_0[7];
+};
+
 /* hwrm_selftest_qlist */
 /* Input (16 bytes) */
 struct hwrm_selftest_qlist_input {
-- 
2.7.4

Re: [PATCH net-next v3 06/10] bnxt: Add devlink support for config get/set

[Michael Chan]

On Tue, Oct 24, 2017 at 1:12 PM, Steve Lin <steven.lin1@broadcom.com> wrote:
> Implements get and set of configuration parameters using new devlink
> config get/set API.  Parameters themselves defined in later patches.
>
> Signed-off-by: Steve Lin <steven.lin1@broadcom.com>
> Acked-by: Andy Gospodarek <gospo@broadcom.com>
> ---
>  drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 262 +++++++++++++++++++++-
>  drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h |  17 ++
>  drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h     | 100 +++++++++
>  3 files changed, 373 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
> index f3f6aa8..81ab77e 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
> @@ -14,11 +14,261 @@
>  #include "bnxt_vfr.h"
>  #include "bnxt_devlink.h"
>
> -static const struct devlink_ops bnxt_dl_ops = {
> +struct bnxt_drv_cfgparam bnxt_drv_cfgparam_list[] = {
> +};
> +
> +#define BNXT_NUM_DRV_CFGPARAM ARRAY_SIZE(bnxt_drv_cfgparam_list)
> +
> +static int bnxt_nvm_read(struct bnxt *bp, int nvm_param, int idx,
> +                        void *buf, int size)
> +{
> +       struct hwrm_nvm_get_variable_input req = {0};
> +       dma_addr_t dest_data_dma_addr;
> +       void *dest_data_addr = NULL;
> +       int bytesize;
> +       int rc;
> +
> +       bytesize = (size + 7) / BITS_PER_BYTE;
> +       dest_data_addr = dma_alloc_coherent(&bp->pdev->dev, bytesize,
> +                                           &dest_data_dma_addr, GFP_KERNEL);
> +       if (!dest_data_addr) {
> +               netdev_err(bp->dev, "dma_alloc_coherent failure\n");
> +               return -ENOMEM;
> +       }
> +
> +       bnxt_hwrm_cmd_hdr_init(bp, &req, HWRM_NVM_GET_VARIABLE, -1, -1);
> +       req.dest_data_addr = cpu_to_le64(dest_data_dma_addr);
> +       req.data_len = cpu_to_le16(size);
> +       req.option_num = cpu_to_le16(nvm_param);
> +       req.index_0 = cpu_to_le16(idx);
> +       if (idx != 0)
> +               req.dimensions = cpu_to_le16(1);
> +
> +       rc = _hwrm_send_message(bp, &req, sizeof(req), HWRM_CMD_TIMEOUT);

This won't have the proper mutex protection.  You should call
hwrm_send_message() instead.

Re: [PATCH net-next v3 06/10] bnxt: Add devlink support for config get/set

[Steve Lin]

On Tue, Oct 24, 2017 at 4:52 PM, Michael Chan <michael.chan@broadcom.com> wrote:
> On Tue, Oct 24, 2017 at 1:12 PM, Steve Lin <steven.lin1@broadcom.com> wrote:
>> Implements get and set of configuration parameters using new devlink
>> config get/set API.  Parameters themselves defined in later patches.
>>
>> Signed-off-by: Steve Lin <steven.lin1@broadcom.com>
>> Acked-by: Andy Gospodarek <gospo@broadcom.com>
>> ---
>>  drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 262 +++++++++++++++++++++-
>>  drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h |  17 ++
>>  drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h     | 100 +++++++++
>>  3 files changed, 373 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
>> index f3f6aa8..81ab77e 100644
>> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
>> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
>> @@ -14,11 +14,261 @@
>>  #include "bnxt_vfr.h"
>>  #include "bnxt_devlink.h"
>>
>> -static const struct devlink_ops bnxt_dl_ops = {
>> +struct bnxt_drv_cfgparam bnxt_drv_cfgparam_list[] = {
>> +};
>> +
>> +#define BNXT_NUM_DRV_CFGPARAM ARRAY_SIZE(bnxt_drv_cfgparam_list)
>> +
>> +static int bnxt_nvm_read(struct bnxt *bp, int nvm_param, int idx,
>> +                        void *buf, int size)
>> +{
>> +       struct hwrm_nvm_get_variable_input req = {0};
>> +       dma_addr_t dest_data_dma_addr;
>> +       void *dest_data_addr = NULL;
>> +       int bytesize;
>> +       int rc;
>> +
>> +       bytesize = (size + 7) / BITS_PER_BYTE;
>> +       dest_data_addr = dma_alloc_coherent(&bp->pdev->dev, bytesize,
>> +                                           &dest_data_dma_addr, GFP_KERNEL);
>> +       if (!dest_data_addr) {
>> +               netdev_err(bp->dev, "dma_alloc_coherent failure\n");
>> +               return -ENOMEM;
>> +       }
>> +
>> +       bnxt_hwrm_cmd_hdr_init(bp, &req, HWRM_NVM_GET_VARIABLE, -1, -1);
>> +       req.dest_data_addr = cpu_to_le64(dest_data_dma_addr);
>> +       req.data_len = cpu_to_le16(size);
>> +       req.option_num = cpu_to_le16(nvm_param);
>> +       req.index_0 = cpu_to_le16(idx);
>> +       if (idx != 0)
>> +               req.dimensions = cpu_to_le16(1);
>> +
>> +       rc = _hwrm_send_message(bp, &req, sizeof(req), HWRM_CMD_TIMEOUT);
>
> This won't have the proper mutex protection.  You should call
> hwrm_send_message() instead.

Ok, thanks, I'll fix that.

RE: [PATCH net-next v3 06/10] bnxt: Add devlink support for config get/set

[Yuval Mintz]

> +static int bnxt_nvm_read(struct bnxt *bp, int nvm_param, int idx,
> +			 void *buf, int size)
> +{
> +	struct hwrm_nvm_get_variable_input req = {0};
> +	dma_addr_t dest_data_dma_addr;
> +	void *dest_data_addr = NULL;
> +	int bytesize;
> +	int rc;
> +
> +	bytesize = (size + 7) / BITS_PER_BYTE;
roundup?

..

+static int bnxt_nvm_write(struct bnxt *bp, int nvm_param, int idx,
> +			  const void *buf, int size)
> +{
> +	struct hwrm_nvm_set_variable_input req = {0};
> +	dma_addr_t src_data_dma_addr;
> +	void *src_data_addr = NULL;
> +	int bytesize;
> +	int rc;
> +
> +	bytesize = (size + 7) / BITS_PER_BYTE;
Likewise

> +
> +	src_data_addr = dma_alloc_coherent(&bp->pdev->dev, bytesize,
> +					   &src_data_dma_addr,
> GFP_KERNEL);
> +	if (!src_data_addr) {
> +		netdev_err(bp->dev, "dma_alloc_coherent failure\n");

Won't you see an oom? Why do you need the print?

> +static int bnxt_dl_perm_config_set(struct devlink *devlink,
> +				   enum devlink_perm_config_param param,
> +				   u8 type, void *value, u8 *restart_reqd)
> +{
> +	struct bnxt *bp = bnxt_get_bp_from_dl(devlink);
> +	struct bnxt_drv_cfgparam *entry;
> +	int idx = 0;
> +	int ret = 0;
> +	u32 bytesize;
> +	u32 val32;
> +	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 (i == BNXT_NUM_DRV_CFGPARAM)
> +		return -EINVAL;
> +
Looks cleaner to check whether entry is set instead
...

> +	bytesize = (entry->bitlength + 7) / BITS_PER_BYTE;

Roundup?

...

> +		if (bytesize == 1) {
> +			val8 = val32;

Don't you need explicit castings for these kind of assignments
to prevent warnings?

> +			ret = bnxt_nvm_write(bp, entry->nvm_param, idx,
> &val8,
> +					     entry->bitlength);
> +		} else if (bytesize == 2) {
> +			val16 = val32;
> +			ret = bnxt_nvm_write(bp, entry->nvm_param, idx,
> &val16,
> +					     entry->bitlength);
> +		} else {
> +			ret = bnxt_nvm_write(bp, entry->nvm_param, idx,
> &val32,
> +					     entry->bitlength);
> +		}
> +	}
> +
> +	/* Restart required for all nvm parameter writes */
> +	*restart_reqd = 1;
> +
> +	return ret;
> +}
> +
> +static int bnxt_dl_perm_config_get(struct devlink *devlink,
> +				   enum devlink_perm_config_param param,
> +				   u8 type, void *value)
> +{
Same comments as for the setter
...

> -	if (!pci_find_ext_capability(bp->pdev, PCI_EXT_CAP_ID_SRIOV))
> -		return 0;
> -
> -	if (bp->hwrm_spec_code < 0x10800) {
> +	if ((!pci_find_ext_capability(bp->pdev, PCI_EXT_CAP_ID_SRIOV)) ||
> +	    bp->hwrm_spec_code < 0x10800) {
> +		/* eswitch switchdev mode not supported */
> +		bnxt_dl_ops.eswitch_mode_set = NULL;
> +		bnxt_dl_ops.eswitch_mode_get = NULL;

Why would you need to tie this interface to the presence of SRIOV in PCIe?
Also, Assuming the ability to disable sriov in #2 would cause this capability
not to be exposed after reboot, isn't this a one-way ticket?

Re: [PATCH net-next v3 06/10] bnxt: Add devlink support for config get/set

[Michael Chan]

On Tue, Oct 24, 2017 at 10:55 PM, Yuval Mintz <yuvalm@mellanox.com> wrote:

> ...
>
>> -     if (!pci_find_ext_capability(bp->pdev, PCI_EXT_CAP_ID_SRIOV))
>> -             return 0;
>> -
>> -     if (bp->hwrm_spec_code < 0x10800) {
>> +     if ((!pci_find_ext_capability(bp->pdev, PCI_EXT_CAP_ID_SRIOV)) ||
>> +         bp->hwrm_spec_code < 0x10800) {
>> +             /* eswitch switchdev mode not supported */
>> +             bnxt_dl_ops.eswitch_mode_set = NULL;
>> +             bnxt_dl_ops.eswitch_mode_get = NULL;
>
> Why would you need to tie this interface to the presence of SRIOV in PCIe?
> Also, Assuming the ability to disable sriov in #2 would cause this capability
> not to be exposed after reboot, isn't this a one-way ticket?
>

No, he is modifying the existing devlink logic for the switchdev
eswitch mode.  That logic controls the SRIOV eswitch mode and depends
on SRIOV.

But I agree the patch makes it look cumbersome.  It will be better to
check for SRIOV and proper firmware support and then setup the
eswitch_mode devlink methods.

[PATCH net-next v3 07/10] bnxt: Adding SR-IOV enablement permanent cfg param

[Steve Lin]

Adding permanent config parameter for SR-IOV enablement, using
devlink API for get/set operation.

0 = SR-IOV disabled
1 = SR-IOV enabled

Signed-off-by: Steve Lin <steven.lin1@broadcom.com>
Acked-by: Andy Gospodarek <gospo@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
index 81ab77e..6eb80c1 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
@@ -14,7 +14,14 @@
 #include "bnxt_vfr.h"
 #include "bnxt_devlink.h"
 
+/* Permanent config parameters from devlink.h:
+ * DEVLINK_PERM_CONFIG_SRIOV_ENABLED:
+ *   0 = disable SR-IOV
+ *   1 = enable SR-IOV
+ */
 struct bnxt_drv_cfgparam bnxt_drv_cfgparam_list[] = {
+	{DEVLINK_PERM_CONFIG_SRIOV_ENABLED, BNXT_DRV_PF,
+		BNXT_DRV_APPL_SHARED, 1, 401},
 };
 
 #define BNXT_NUM_DRV_CFGPARAM ARRAY_SIZE(bnxt_drv_cfgparam_list)
-- 
2.7.4

[PATCH net-next v3 08/10] bnxt: Adding num VFs per PF perm config param

[Steve Lin]

Adding permanent config parameter for number of VFs per PF,
using devlink API for get/set operation.

Value sets the number of VFs per PF in SR-IOV mode.

Signed-off-by: Steve Lin <steven.lin1@broadcom.com>
Acked-by: Andy Gospodarek <gospo@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
index 6eb80c1..55913c4 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
@@ -18,10 +18,14 @@
  * DEVLINK_PERM_CONFIG_SRIOV_ENABLED:
  *   0 = disable SR-IOV
  *   1 = enable SR-IOV
+ * DEVLINK_PERM_CONFIG_NUM_VF_PER_PF:
+ *   # of VFs per PF in SR-IOV mode
  */
 struct bnxt_drv_cfgparam bnxt_drv_cfgparam_list[] = {
 	{DEVLINK_PERM_CONFIG_SRIOV_ENABLED, BNXT_DRV_PF,
 		BNXT_DRV_APPL_SHARED, 1, 401},
+	{DEVLINK_PERM_CONFIG_NUM_VF_PER_PF, BNXT_DRV_PF,
+		BNXT_DRV_APPL_FUNCTION, 8, 404},
 };
 
 #define BNXT_NUM_DRV_CFGPARAM ARRAY_SIZE(bnxt_drv_cfgparam_list)
-- 
2.7.4

[PATCH net-next v3 09/10] bnxt: Adding max PF MSI-X vectors perm config param

[Steve Lin]

Adding permanent config parameter for maximum number of PF
MSI-X vectors.

Signed-off-by: Steve Lin <steven.lin1@broadcom.com>
Acked-by: Andy Gospodarek <gospo@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
index 55913c4..c6e670c 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
@@ -20,12 +20,16 @@
  *   1 = enable SR-IOV
  * DEVLINK_PERM_CONFIG_NUM_VF_PER_PF:
  *   # of VFs per PF in SR-IOV mode
+ * DEVLINK_PERM_CONFIG_MAX_NUM_PF_MSIX_VECT:
+ *   Max # of MSI-X vectors per PF
  */
 struct bnxt_drv_cfgparam bnxt_drv_cfgparam_list[] = {
 	{DEVLINK_PERM_CONFIG_SRIOV_ENABLED, BNXT_DRV_PF,
 		BNXT_DRV_APPL_SHARED, 1, 401},
 	{DEVLINK_PERM_CONFIG_NUM_VF_PER_PF, BNXT_DRV_PF,
 		BNXT_DRV_APPL_FUNCTION, 8, 404},
+	{DEVLINK_PERM_CONFIG_MAX_NUM_PF_MSIX_VECT, BNXT_DRV_PF,
+		BNXT_DRV_APPL_SHARED, 10, 108},
 };
 
 #define BNXT_NUM_DRV_CFGPARAM ARRAY_SIZE(bnxt_drv_cfgparam_list)
-- 
2.7.4

[PATCH net-next v3 10/10] bnxt: Adding num MSI-X vectors per VF perm config param

[Steve Lin]

Adding permanent config parameter for number MSI-X vectors
per VF, using devlink API for get/set operation.

Signed-off-by: Steve Lin <steven.lin1@broadcom.com>
Acked-by: Andy Gospodarek <gospo@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
index c6e670c..620a207 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
@@ -22,6 +22,8 @@
  *   # of VFs per PF in SR-IOV mode
  * DEVLINK_PERM_CONFIG_MAX_NUM_PF_MSIX_VECT:
  *   Max # of MSI-X vectors per PF
+ * DEVLINK_PERM_CONFIG_MSIX_VECTORS_PER_VF:
+ *   # of MSI-X vectors per VF
  */
 struct bnxt_drv_cfgparam bnxt_drv_cfgparam_list[] = {
 	{DEVLINK_PERM_CONFIG_SRIOV_ENABLED, BNXT_DRV_PF,
@@ -30,6 +32,8 @@ struct bnxt_drv_cfgparam bnxt_drv_cfgparam_list[] = {
 		BNXT_DRV_APPL_FUNCTION, 8, 404},
 	{DEVLINK_PERM_CONFIG_MAX_NUM_PF_MSIX_VECT, BNXT_DRV_PF,
 		BNXT_DRV_APPL_SHARED, 10, 108},
+	{DEVLINK_PERM_CONFIG_MSIX_VECTORS_PER_VF, BNXT_DRV_PF,
+		BNXT_DRV_APPL_FUNCTION, 10, 406},
 };
 
 #define BNXT_NUM_DRV_CFGPARAM ARRAY_SIZE(bnxt_drv_cfgparam_list)
-- 
2.7.4