[PATCH net-next RFC 0/7] Add configuration parameters support for devlink_port

[PATCH net-next RFC 0/7] Add configuration parameters support for devlink_port

[Vasundhara Volam]

This patchset adds support for configuration parameters setting through
devlink_port.  Each device registers supported configuration parameters
table.

The user can retrieve data on these parameters by
"devlink port param show" command and can set new value to a
parameter by "devlink port param set" command.
All configuration modes supported by devlink_dev are supported
by devlink_port also.

Command examples and output:

# devlink port param show
pci/0000:3b:00.0/0:
  name wake-on-lan type generic
    values:
      cmode permanent value false

pci/0000:3b:00.1/1:
  name wake-on-lan type generic
    values:
      cmode permanent value false

pci/0000:af:00.0/0:
  name wake-on-lan type generic
    values:
      cmode permanent value true

# devlink port param show pci/0000:3b:00.0/0 name wake-on-lan
pci/0000:3b:00.0/0:
  name wake-on-lan type generic
    values:
      cmode permanent value false

# devlink port param set pci/0000:3b:00.0/0 name wake-on-lan cmode permanent value true

Vasundhara Volam (7):
  devlink: Add devlink_param for port register and unregister
  devlink: Add port param get command
  devlink: Add port param set command
  devlink: Add support for get/set driverinit value for devlink_port
  devlink: Add devlink notifications support for port params
  devlink: Add a boolean generic port parameter
  bnxt_en: Add bnxt_en initial port params table and register it

 drivers/net/ethernet/broadcom/bnxt/bnxt.h         |   1 +
 drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c |  35 ++
 drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h |   1 +
 include/net/devlink.h                             |  74 ++++
 include/uapi/linux/devlink.h                      |   5 +
 net/core/devlink.c                                | 454 +++++++++++++++++++---
 6 files changed, 518 insertions(+), 52 deletions(-)

-- 
1.8.3.1

[PATCH net-next RFC 1/7] devlink: Add devlink_param for port register and unregister

[Vasundhara Volam]

Add functions to register and unregister for the driver supported
configuration parameters table per port.

Cc: Jiri Pirko <jiri@mellanox.com>
Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
---
 include/net/devlink.h |  29 +++++++++++
 net/core/devlink.c    | 130 ++++++++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 151 insertions(+), 8 deletions(-)

diff --git a/include/net/devlink.h b/include/net/devlink.h
index 67f4293..9b4d80b 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -48,6 +48,7 @@ struct devlink_port_attrs {
 
 struct devlink_port {
 	struct list_head list;
+	struct list_head param_list;
 	struct devlink *devlink;
 	unsigned index;
 	bool registered;
@@ -419,6 +420,13 @@ enum devlink_param_generic_id {
 	.validate = _validate,						\
 }
 
+enum devlink_port_param_generic_id {
+	/* add new param generic ids above here */
+	__DEVLINK_PORT_PARAM_GENERIC_ID_MAX,
+	DEVLINK_PORT_PARAM_GENERIC_ID_MAX =
+	       __DEVLINK_PORT_PARAM_GENERIC_ID_MAX - 1,
+};
+
 struct devlink_region;
 
 typedef void devlink_snapshot_data_dest_t(const void *data);
@@ -574,6 +582,12 @@ int devlink_param_driverinit_value_set(struct devlink *devlink, u32 param_id,
 void devlink_param_value_changed(struct devlink *devlink, u32 param_id);
 void devlink_param_value_str_fill(union devlink_param_value *dst_val,
 				  const char *src);
+int devlink_port_params_register(struct devlink_port *devlink_port,
+				 const struct devlink_param *params,
+				 size_t params_count);
+void devlink_port_params_unregister(struct devlink_port *devlink_port,
+				    const struct devlink_param *params,
+				    size_t params_count);
 struct devlink_region *devlink_region_create(struct devlink *devlink,
 					     const char *region_name,
 					     u32 region_max_snapshots,
@@ -816,6 +830,21 @@ static inline bool devlink_dpipe_table_counter_enabled(struct devlink *devlink,
 {
 }
 
+static inline int
+devlink_port_params_register(struct devlink_port *devlink_port,
+			     const struct devlink_param *params,
+			     size_t params_count)
+{
+	return 0;
+}
+
+static inline void
+devlink_port_params_unregister(struct devlink_port *devlink_port,
+			       const struct devlink_param *params,
+			       size_t params_count)
+{
+}
+
 static inline struct devlink_region *
 devlink_region_create(struct devlink *devlink,
 		      const char *region_name,
diff --git a/net/core/devlink.c b/net/core/devlink.c
index abb0da9..e194913 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -3147,12 +3147,12 @@ static int devlink_nl_cmd_param_set_doit(struct sk_buff *skb,
 }
 
 static int devlink_param_register_one(struct devlink *devlink,
+				      struct list_head *param_list,
 				      const struct devlink_param *param)
 {
 	struct devlink_param_item *param_item;
 
-	if (devlink_param_find_by_name(&devlink->param_list,
-				       param->name))
+	if (devlink_param_find_by_name(param_list, param->name))
 		return -EEXIST;
 
 	if (param->supported_cmodes == BIT(DEVLINK_PARAM_CMODE_DRIVERINIT))
@@ -3165,24 +3165,54 @@ static int devlink_param_register_one(struct devlink *devlink,
 		return -ENOMEM;
 	param_item->param = param;
 
-	list_add_tail(&param_item->list, &devlink->param_list);
+	list_add_tail(&param_item->list, param_list);
 	devlink_param_notify(devlink, param_item, DEVLINK_CMD_PARAM_NEW);
 	return 0;
 }
 
 static void devlink_param_unregister_one(struct devlink *devlink,
+					 struct list_head *param_list,
 					 const struct devlink_param *param)
 {
 	struct devlink_param_item *param_item;
 
-	param_item = devlink_param_find_by_name(&devlink->param_list,
-						param->name);
+	param_item = devlink_param_find_by_name(param_list, param->name);
 	WARN_ON(!param_item);
 	devlink_param_notify(devlink, param_item, DEVLINK_CMD_PARAM_DEL);
 	list_del(&param_item->list);
 	kfree(param_item);
 }
 
+static const struct devlink_param devlink_port_param_generic[] = {};
+
+static int devlink_port_param_generic_verify(const struct devlink_param *param)
+{
+	/* verify it matches generic parameter by id and name */
+	if (param->id > DEVLINK_PORT_PARAM_GENERIC_ID_MAX)
+		return -EINVAL;
+	if (strcmp(param->name, devlink_port_param_generic[param->id].name))
+		return -ENOENT;
+
+	WARN_ON(param->type != devlink_port_param_generic[param->id].type);
+
+	return 0;
+}
+
+static int devlink_port_param_driver_verify(const struct devlink_param *param)
+{
+	int i;
+
+	if (param->id <= DEVLINK_PORT_PARAM_GENERIC_ID_MAX)
+		return -EINVAL;
+	/* verify no such name in generic params */
+	for (i = 0; i <= DEVLINK_PORT_PARAM_GENERIC_ID_MAX; i++)
+		if (!strcmp(param->name,
+			    devlink_port_param_generic[param->id].name))
+			return -EEXIST;
+
+	return 0;
+}
+
 static int devlink_nl_region_snapshot_id_put(struct sk_buff *msg,
 					     struct devlink *devlink,
 					     struct devlink_snapshot *snapshot)
@@ -4503,7 +4533,8 @@ int devlink_params_register(struct devlink *devlink,
 			if (err)
 				goto rollback;
 		}
-		err = devlink_param_register_one(devlink, param);
+		err = devlink_param_register_one(devlink, &devlink->param_list,
+						 param);
 		if (err)
 			goto rollback;
 	}
@@ -4515,7 +4546,8 @@ int devlink_params_register(struct devlink *devlink,
 	if (!i)
 		goto unlock;
 	for (param--; i > 0; i--, param--)
-		devlink_param_unregister_one(devlink, param);
+		devlink_param_unregister_one(devlink, &devlink->param_list,
+					     param);
 unlock:
 	mutex_unlock(&devlink->lock);
 	return err;
@@ -4537,7 +4569,8 @@ void devlink_params_unregister(struct devlink *devlink,
 
 	mutex_lock(&devlink->lock);
 	for (i = 0; i < params_count; i++, param++)
-		devlink_param_unregister_one(devlink, param);
+		devlink_param_unregister_one(devlink, &devlink->param_list,
+					     param);
 	mutex_unlock(&devlink->lock);
 }
 EXPORT_SYMBOL_GPL(devlink_params_unregister);
@@ -4657,6 +4690,87 @@ void devlink_param_value_str_fill(union devlink_param_value *dst_val,
 EXPORT_SYMBOL_GPL(devlink_param_value_str_fill);
 
 /**
+ *	devlink_port_params_register - register port configuration parameters
+ *
+ *	@devlink_port: devlink port
+ *	@params: configuration parameters array
+ *	@params_count: number of parameters provided
+ *
+ *	Register the configuration parameters supported by the port.
+ */
+int devlink_port_params_register(struct devlink_port *devlink_port,
+				 const struct devlink_param *params,
+				 size_t params_count)
+{
+	struct devlink *devlink = devlink_port->devlink;
+	const struct devlink_param *param = params;
+	int i, err;
+
+	INIT_LIST_HEAD(&devlink_port->param_list);
+	mutex_lock(&devlink->lock);
+	for (i = 0; i < params_count; i++) {
+		if (!param || !param->name || !param->supported_cmodes) {
+			err = -EINVAL;
+			goto rollback;
+		}
+		if (param->generic) {
+			err = devlink_port_param_generic_verify(param);
+			if (err)
+				goto rollback;
+		} else {
+			err = devlink_port_param_driver_verify(param);
+			if (err)
+				goto rollback;
+		}
+		err = devlink_param_register_one(devlink_port->devlink,
+						 &devlink_port->param_list,
+						 param);
+		if (err)
+			goto rollback;
+	}
+
+	mutex_unlock(&devlink->lock);
+	return 0;
+
+rollback:
+	if (!i)
+		goto unlock;
+	for (param--; i > 0; i--, param--)
+		devlink_param_unregister_one(devlink_port->devlink,
+					     &devlink_port->param_list,
+					     param);
+unlock:
+	mutex_unlock(&devlink->lock);
+	return err;
+}
+EXPORT_SYMBOL_GPL(devlink_port_params_register);
+
+/**
+ *	devlink_port_params_unregister - unregister port configuration
+ *	parameters
+ *
+ *	@devlink_port: devlink port
+ *	@params: configuration parameters array
+ *	@params_count: number of parameters provided
+ */
+void devlink_port_params_unregister(struct devlink_port *devlink_port,
+				    const struct devlink_param *params,
+				    size_t params_count)
+{
+	struct devlink *devlink = devlink_port->devlink;
+	const struct devlink_param *param = params;
+	int i;
+
+	mutex_lock(&devlink->lock);
+	for (i = 0; i < params_count; i++, param++)
+		devlink_param_unregister_one(devlink_port->devlink,
+					     &devlink_port->param_list,
+					     param);
+	mutex_unlock(&devlink->lock);
+}
+EXPORT_SYMBOL_GPL(devlink_port_params_unregister);
+
+/**
  *	devlink_region_create - create a new address region
  *
  *	@devlink: devlink
-- 
1.8.3.1

Re: [PATCH net-next RFC 1/7] devlink: Add devlink_param for port register and unregister

[Jiri Pirko]

Wed, Dec 05, 2018 at 06:56:54AM CET, vasundhara-v.volam@broadcom.com wrote:
>Add functions to register and unregister for the driver supported
>configuration parameters table per port.
>
>Cc: Jiri Pirko <jiri@mellanox.com>
>Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
>---
> include/net/devlink.h |  29 +++++++++++
> net/core/devlink.c    | 130 ++++++++++++++++++++++++++++++++++++++++++++++----
> 2 files changed, 151 insertions(+), 8 deletions(-)
>
>diff --git a/include/net/devlink.h b/include/net/devlink.h
>index 67f4293..9b4d80b 100644
>--- a/include/net/devlink.h
>+++ b/include/net/devlink.h
>@@ -48,6 +48,7 @@ struct devlink_port_attrs {
> 
> struct devlink_port {
> 	struct list_head list;
>+	struct list_head param_list;
> 	struct devlink *devlink;
> 	unsigned index;
> 	bool registered;
>@@ -419,6 +420,13 @@ enum devlink_param_generic_id {
> 	.validate = _validate,						\
> }
> 
>+enum devlink_port_param_generic_id {
>+	/* add new param generic ids above here */
>+	__DEVLINK_PORT_PARAM_GENERIC_ID_MAX,
>+	DEVLINK_PORT_PARAM_GENERIC_ID_MAX =
>+	       __DEVLINK_PORT_PARAM_GENERIC_ID_MAX - 1,
>+};

I don't see the need for enum just for per-port params. The existing
params enum should be enough.


>+
> struct devlink_region;
> 
> typedef void devlink_snapshot_data_dest_t(const void *data);
>@@ -574,6 +582,12 @@ int devlink_param_driverinit_value_set(struct devlink *devlink, u32 param_id,
> void devlink_param_value_changed(struct devlink *devlink, u32 param_id);
> void devlink_param_value_str_fill(union devlink_param_value *dst_val,
> 				  const char *src);
>+int devlink_port_params_register(struct devlink_port *devlink_port,
>+				 const struct devlink_param *params,
>+				 size_t params_count);
>+void devlink_port_params_unregister(struct devlink_port *devlink_port,
>+				    const struct devlink_param *params,
>+				    size_t params_count);
> struct devlink_region *devlink_region_create(struct devlink *devlink,
> 					     const char *region_name,
> 					     u32 region_max_snapshots,
>@@ -816,6 +830,21 @@ static inline bool devlink_dpipe_table_counter_enabled(struct devlink *devlink,
> {
> }
> 
>+static inline int
>+devlink_port_params_register(struct devlink_port *devlink_port,
>+			     const struct devlink_param *params,
>+			     size_t params_count)
>+{
>+	return 0;
>+}
>+
>+static inline void
>+devlink_port_params_unregister(struct devlink_port *devlink_port,
>+			       const struct devlink_param *params,
>+			       size_t params_count)
>+{
>+}
>+
> static inline struct devlink_region *
> devlink_region_create(struct devlink *devlink,
> 		      const char *region_name,
>diff --git a/net/core/devlink.c b/net/core/devlink.c
>index abb0da9..e194913 100644
>--- a/net/core/devlink.c
>+++ b/net/core/devlink.c
>@@ -3147,12 +3147,12 @@ static int devlink_nl_cmd_param_set_doit(struct sk_buff *skb,
> }
> 
> static int devlink_param_register_one(struct devlink *devlink,
>+				      struct list_head *param_list,
> 				      const struct devlink_param *param)
> {
> 	struct devlink_param_item *param_item;
> 
>-	if (devlink_param_find_by_name(&devlink->param_list,
>-				       param->name))
>+	if (devlink_param_find_by_name(param_list, param->name))
> 		return -EEXIST;
> 
> 	if (param->supported_cmodes == BIT(DEVLINK_PARAM_CMODE_DRIVERINIT))
>@@ -3165,24 +3165,54 @@ static int devlink_param_register_one(struct devlink *devlink,
> 		return -ENOMEM;
> 	param_item->param = param;
> 
>-	list_add_tail(&param_item->list, &devlink->param_list);
>+	list_add_tail(&param_item->list, param_list);
> 	devlink_param_notify(devlink, param_item, DEVLINK_CMD_PARAM_NEW);
> 	return 0;
> }
> 
> static void devlink_param_unregister_one(struct devlink *devlink,
>+					 struct list_head *param_list,
> 					 const struct devlink_param *param)
> {
> 	struct devlink_param_item *param_item;
> 
>-	param_item = devlink_param_find_by_name(&devlink->param_list,
>-						param->name);
>+	param_item = devlink_param_find_by_name(param_list, param->name);
> 	WARN_ON(!param_item);
> 	devlink_param_notify(devlink, param_item, DEVLINK_CMD_PARAM_DEL);
> 	list_del(&param_item->list);
> 	kfree(param_item);
> }
> 
>+static const struct devlink_param devlink_port_param_generic[] = {};
>+
>+static int devlink_port_param_generic_verify(const struct devlink_param *param)
>+{
>+	/* verify it matches generic parameter by id and name */
>+	if (param->id > DEVLINK_PORT_PARAM_GENERIC_ID_MAX)
>+		return -EINVAL;
>+	if (strcmp(param->name, devlink_port_param_generic[param->id].name))
>+		return -ENOENT;
>+
>+	WARN_ON(param->type != devlink_port_param_generic[param->id].type);
>+
>+	return 0;
>+}
>+
>+static int devlink_port_param_driver_verify(const struct devlink_param *param)
>+{
>+	int i;
>+
>+	if (param->id <= DEVLINK_PORT_PARAM_GENERIC_ID_MAX)
>+		return -EINVAL;
>+	/* verify no such name in generic params */
>+	for (i = 0; i <= DEVLINK_PORT_PARAM_GENERIC_ID_MAX; i++)
>+		if (!strcmp(param->name,
>+			    devlink_port_param_generic[param->id].name))
>+			return -EEXIST;
>+
>+	return 0;
>+}
>+
> static int devlink_nl_region_snapshot_id_put(struct sk_buff *msg,
> 					     struct devlink *devlink,
> 					     struct devlink_snapshot *snapshot)
>@@ -4503,7 +4533,8 @@ int devlink_params_register(struct devlink *devlink,
> 			if (err)
> 				goto rollback;
> 		}
>-		err = devlink_param_register_one(devlink, param);
>+		err = devlink_param_register_one(devlink, &devlink->param_list,
>+						 param);
> 		if (err)
> 			goto rollback;
> 	}
>@@ -4515,7 +4546,8 @@ int devlink_params_register(struct devlink *devlink,
> 	if (!i)
> 		goto unlock;
> 	for (param--; i > 0; i--, param--)
>-		devlink_param_unregister_one(devlink, param);
>+		devlink_param_unregister_one(devlink, &devlink->param_list,
>+					     param);
> unlock:
> 	mutex_unlock(&devlink->lock);
> 	return err;
>@@ -4537,7 +4569,8 @@ void devlink_params_unregister(struct devlink *devlink,
> 
> 	mutex_lock(&devlink->lock);
> 	for (i = 0; i < params_count; i++, param++)
>-		devlink_param_unregister_one(devlink, param);
>+		devlink_param_unregister_one(devlink, &devlink->param_list,
>+					     param);
> 	mutex_unlock(&devlink->lock);
> }
> EXPORT_SYMBOL_GPL(devlink_params_unregister);
>@@ -4657,6 +4690,87 @@ void devlink_param_value_str_fill(union devlink_param_value *dst_val,
> EXPORT_SYMBOL_GPL(devlink_param_value_str_fill);
> 
> /**
>+ *	devlink_port_params_register - register port configuration parameters
>+ *
>+ *	@devlink_port: devlink port
>+ *	@params: configuration parameters array
>+ *	@params_count: number of parameters provided
>+ *
>+ *	Register the configuration parameters supported by the port.
>+ */
>+int devlink_port_params_register(struct devlink_port *devlink_port,
>+				 const struct devlink_param *params,
>+				 size_t params_count)
>+{
>+	struct devlink *devlink = devlink_port->devlink;
>+	const struct devlink_param *param = params;
>+	int i, err;
>+
>+	INIT_LIST_HEAD(&devlink_port->param_list);

Driver can call devlink_port_params_register multiple times. You need to
move the list init into devlink_port_register()


>+	mutex_lock(&devlink->lock);
>+	for (i = 0; i < params_count; i++) {
>+		if (!param || !param->name || !param->supported_cmodes) {
>+			err = -EINVAL;
>+			goto rollback;
>+		}
>+		if (param->generic) {
>+			err = devlink_port_param_generic_verify(param);
>+			if (err)
>+				goto rollback;
>+		} else {
>+			err = devlink_port_param_driver_verify(param);

This is duplicated code from devlink_params_register(). Once you use a 
single enum for all params, you can push this into a common function for
both devlink_params_register() and devlink_port_params_register()


>+			if (err)
>+				goto rollback;
>+		}
>+		err = devlink_param_register_one(devlink_port->devlink,
>+						 &devlink_port->param_list,
>+						 param);
>+		if (err)
>+			goto rollback;
>+	}
>+
>+	mutex_unlock(&devlink->lock);
>+	return 0;
>+
>+rollback:
>+	if (!i)
>+		goto unlock;
>+	for (param--; i > 0; i--, param--)
>+		devlink_param_unregister_one(devlink_port->devlink,
>+					     &devlink_port->param_list,
>+					     param);
>+unlock:
>+	mutex_unlock(&devlink->lock);
>+	return err;
>+}
>+EXPORT_SYMBOL_GPL(devlink_port_params_register);
>+
>+/**
>+ *	devlink_port_params_unregister - unregister port configuration
>+ *	parameters
>+ *
>+ *	@devlink_port: devlink port
>+ *	@params: configuration parameters array
>+ *	@params_count: number of parameters provided
>+ */
>+void devlink_port_params_unregister(struct devlink_port *devlink_port,
>+				    const struct devlink_param *params,
>+				    size_t params_count)
>+{
>+	struct devlink *devlink = devlink_port->devlink;
>+	const struct devlink_param *param = params;
>+	int i;
>+
>+	mutex_lock(&devlink->lock);
>+	for (i = 0; i < params_count; i++, param++)
>+		devlink_param_unregister_one(devlink_port->devlink,
>+					     &devlink_port->param_list,
>+					     param);

In this case, not so much of duplication, but still, put into a common
function (to be symmetric to devlink_params_register() and
devlink_port_params_register()


>+	mutex_unlock(&devlink->lock);
>+}
>+EXPORT_SYMBOL_GPL(devlink_port_params_unregister);
>+
>+/**
>  *	devlink_region_create - create a new address region
>  *
>  *	@devlink: devlink
>-- 
>1.8.3.1
>

Re: [PATCH net-next RFC 1/7] devlink: Add devlink_param for port register and unregister

[Vasundhara Volam]

Thank you reviewing the patches.

On Wed, Dec 5, 2018 at 5:24 PM Jiri Pirko <jiri@resnulli.us> wrote:
>
> Wed, Dec 05, 2018 at 06:56:54AM CET, vasundhara-v.volam@broadcom.com wrote:
> >Add functions to register and unregister for the driver supported
> >configuration parameters table per port.
> >
> >Cc: Jiri Pirko <jiri@mellanox.com>
> >Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
> >---
> > include/net/devlink.h |  29 +++++++++++
> > net/core/devlink.c    | 130 ++++++++++++++++++++++++++++++++++++++++++++++----
> > 2 files changed, 151 insertions(+), 8 deletions(-)
> >
> >diff --git a/include/net/devlink.h b/include/net/devlink.h
> >index 67f4293..9b4d80b 100644
> >--- a/include/net/devlink.h
> >+++ b/include/net/devlink.h
> >@@ -48,6 +48,7 @@ struct devlink_port_attrs {
> >
> > struct devlink_port {
> >       struct list_head list;
> >+      struct list_head param_list;
> >       struct devlink *devlink;
> >       unsigned index;
> >       bool registered;
> >@@ -419,6 +420,13 @@ enum devlink_param_generic_id {
> >       .validate = _validate,                                          \
> > }
> >
> >+enum devlink_port_param_generic_id {
> >+      /* add new param generic ids above here */
> >+      __DEVLINK_PORT_PARAM_GENERIC_ID_MAX,
> >+      DEVLINK_PORT_PARAM_GENERIC_ID_MAX =
> >+             __DEVLINK_PORT_PARAM_GENERIC_ID_MAX - 1,
> >+};
>
> I don't see the need for enum just for per-port params. The existing
> params enum should be enough.
In that case there won't be any differentiation between generic device
and port params.
Also, if enum is not needed, then separate struct
devlink_port_param_generic is also not needed.

Based on this, entire patchset needs a change.
>
>
> >+
> > struct devlink_region;
> >
> > typedef void devlink_snapshot_data_dest_t(const void *data);
> >@@ -574,6 +582,12 @@ int devlink_param_driverinit_value_set(struct devlink *devlink, u32 param_id,
> > void devlink_param_value_changed(struct devlink *devlink, u32 param_id);
> > void devlink_param_value_str_fill(union devlink_param_value *dst_val,
> >                                 const char *src);
> >+int devlink_port_params_register(struct devlink_port *devlink_port,
> >+                               const struct devlink_param *params,
> >+                               size_t params_count);
> >+void devlink_port_params_unregister(struct devlink_port *devlink_port,
> >+                                  const struct devlink_param *params,
> >+                                  size_t params_count);
> > struct devlink_region *devlink_region_create(struct devlink *devlink,
> >                                            const char *region_name,
> >                                            u32 region_max_snapshots,
> >@@ -816,6 +830,21 @@ static inline bool devlink_dpipe_table_counter_enabled(struct devlink *devlink,
> > {
> > }
> >
> >+static inline int
> >+devlink_port_params_register(struct devlink_port *devlink_port,
> >+                           const struct devlink_param *params,
> >+                           size_t params_count)
> >+{
> >+      return 0;
> >+}
> >+
> >+static inline void
> >+devlink_port_params_unregister(struct devlink_port *devlink_port,
> >+                             const struct devlink_param *params,
> >+                             size_t params_count)
> >+{
> >+}
> >+
> > static inline struct devlink_region *
> > devlink_region_create(struct devlink *devlink,
> >                     const char *region_name,
> >diff --git a/net/core/devlink.c b/net/core/devlink.c
> >index abb0da9..e194913 100644
> >--- a/net/core/devlink.c
> >+++ b/net/core/devlink.c
> >@@ -3147,12 +3147,12 @@ static int devlink_nl_cmd_param_set_doit(struct sk_buff *skb,
> > }
> >
> > static int devlink_param_register_one(struct devlink *devlink,
> >+                                    struct list_head *param_list,
> >                                     const struct devlink_param *param)
> > {
> >       struct devlink_param_item *param_item;
> >
> >-      if (devlink_param_find_by_name(&devlink->param_list,
> >-                                     param->name))
> >+      if (devlink_param_find_by_name(param_list, param->name))
> >               return -EEXIST;
> >
> >       if (param->supported_cmodes == BIT(DEVLINK_PARAM_CMODE_DRIVERINIT))
> >@@ -3165,24 +3165,54 @@ static int devlink_param_register_one(struct devlink *devlink,
> >               return -ENOMEM;
> >       param_item->param = param;
> >
> >-      list_add_tail(&param_item->list, &devlink->param_list);
> >+      list_add_tail(&param_item->list, param_list);
> >       devlink_param_notify(devlink, param_item, DEVLINK_CMD_PARAM_NEW);
> >       return 0;
> > }
> >
> > static void devlink_param_unregister_one(struct devlink *devlink,
> >+                                       struct list_head *param_list,
> >                                        const struct devlink_param *param)
> > {
> >       struct devlink_param_item *param_item;
> >
> >-      param_item = devlink_param_find_by_name(&devlink->param_list,
> >-                                              param->name);
> >+      param_item = devlink_param_find_by_name(param_list, param->name);
> >       WARN_ON(!param_item);
> >       devlink_param_notify(devlink, param_item, DEVLINK_CMD_PARAM_DEL);
> >       list_del(&param_item->list);
> >       kfree(param_item);
> > }
> >
> >+static const struct devlink_param devlink_port_param_generic[] = {};
> >+
> >+static int devlink_port_param_generic_verify(const struct devlink_param *param)
> >+{
> >+      /* verify it matches generic parameter by id and name */
> >+      if (param->id > DEVLINK_PORT_PARAM_GENERIC_ID_MAX)
> >+              return -EINVAL;
> >+      if (strcmp(param->name, devlink_port_param_generic[param->id].name))
> >+              return -ENOENT;
> >+
> >+      WARN_ON(param->type != devlink_port_param_generic[param->id].type);
> >+
> >+      return 0;
> >+}
> >+
> >+static int devlink_port_param_driver_verify(const struct devlink_param *param)
> >+{
> >+      int i;
> >+
> >+      if (param->id <= DEVLINK_PORT_PARAM_GENERIC_ID_MAX)
> >+              return -EINVAL;
> >+      /* verify no such name in generic params */
> >+      for (i = 0; i <= DEVLINK_PORT_PARAM_GENERIC_ID_MAX; i++)
> >+              if (!strcmp(param->name,
> >+                          devlink_port_param_generic[param->id].name))
> >+                      return -EEXIST;
> >+
> >+      return 0;
> >+}
> >+
> > static int devlink_nl_region_snapshot_id_put(struct sk_buff *msg,
> >                                            struct devlink *devlink,
> >                                            struct devlink_snapshot *snapshot)
> >@@ -4503,7 +4533,8 @@ int devlink_params_register(struct devlink *devlink,
> >                       if (err)
> >                               goto rollback;
> >               }
> >-              err = devlink_param_register_one(devlink, param);
> >+              err = devlink_param_register_one(devlink, &devlink->param_list,
> >+                                               param);
> >               if (err)
> >                       goto rollback;
> >       }
> >@@ -4515,7 +4546,8 @@ int devlink_params_register(struct devlink *devlink,
> >       if (!i)
> >               goto unlock;
> >       for (param--; i > 0; i--, param--)
> >-              devlink_param_unregister_one(devlink, param);
> >+              devlink_param_unregister_one(devlink, &devlink->param_list,
> >+                                           param);
> > unlock:
> >       mutex_unlock(&devlink->lock);
> >       return err;
> >@@ -4537,7 +4569,8 @@ void devlink_params_unregister(struct devlink *devlink,
> >
> >       mutex_lock(&devlink->lock);
> >       for (i = 0; i < params_count; i++, param++)
> >-              devlink_param_unregister_one(devlink, param);
> >+              devlink_param_unregister_one(devlink, &devlink->param_list,
> >+                                           param);
> >       mutex_unlock(&devlink->lock);
> > }
> > EXPORT_SYMBOL_GPL(devlink_params_unregister);
> >@@ -4657,6 +4690,87 @@ void devlink_param_value_str_fill(union devlink_param_value *dst_val,
> > EXPORT_SYMBOL_GPL(devlink_param_value_str_fill);
> >
> > /**
> >+ *    devlink_port_params_register - register port configuration parameters
> >+ *
> >+ *    @devlink_port: devlink port
> >+ *    @params: configuration parameters array
> >+ *    @params_count: number of parameters provided
> >+ *
> >+ *    Register the configuration parameters supported by the port.
> >+ */
> >+int devlink_port_params_register(struct devlink_port *devlink_port,
> >+                               const struct devlink_param *params,
> >+                               size_t params_count)
> >+{
> >+      struct devlink *devlink = devlink_port->devlink;
> >+      const struct devlink_param *param = params;
> >+      int i, err;
> >+
> >+      INIT_LIST_HEAD(&devlink_port->param_list);
>
> Driver can call devlink_port_params_register multiple times. You need to
> move the list init into devlink_port_register()
Agreed, will change it in next version.
>
>
> >+      mutex_lock(&devlink->lock);
> >+      for (i = 0; i < params_count; i++) {
> >+              if (!param || !param->name || !param->supported_cmodes) {
> >+                      err = -EINVAL;
> >+                      goto rollback;
> >+              }
> >+              if (param->generic) {
> >+                      err = devlink_port_param_generic_verify(param);
> >+                      if (err)
> >+                              goto rollback;
> >+              } else {
> >+                      err = devlink_port_param_driver_verify(param);
>
> This is duplicated code from devlink_params_register(). Once you use a
> single enum for all params, you can push this into a common function for
> both devlink_params_register() and devlink_port_params_register()
>
>
> >+                      if (err)
> >+                              goto rollback;
> >+              }
> >+              err = devlink_param_register_one(devlink_port->devlink,
> >+                                               &devlink_port->param_list,
> >+                                               param);
> >+              if (err)
> >+                      goto rollback;
> >+      }
> >+
> >+      mutex_unlock(&devlink->lock);
> >+      return 0;
> >+
> >+rollback:
> >+      if (!i)
> >+              goto unlock;
> >+      for (param--; i > 0; i--, param--)
> >+              devlink_param_unregister_one(devlink_port->devlink,
> >+                                           &devlink_port->param_list,
> >+                                           param);
> >+unlock:
> >+      mutex_unlock(&devlink->lock);
> >+      return err;
> >+}
> >+EXPORT_SYMBOL_GPL(devlink_port_params_register);
> >+
> >+/**
> >+ *    devlink_port_params_unregister - unregister port configuration
> >+ *    parameters
> >+ *
> >+ *    @devlink_port: devlink port
> >+ *    @params: configuration parameters array
> >+ *    @params_count: number of parameters provided
> >+ */
> >+void devlink_port_params_unregister(struct devlink_port *devlink_port,
> >+                                  const struct devlink_param *params,
> >+                                  size_t params_count)
> >+{
> >+      struct devlink *devlink = devlink_port->devlink;
> >+      const struct devlink_param *param = params;
> >+      int i;
> >+
> >+      mutex_lock(&devlink->lock);
> >+      for (i = 0; i < params_count; i++, param++)
> >+              devlink_param_unregister_one(devlink_port->devlink,
> >+                                           &devlink_port->param_list,
> >+                                           param);
>
> In this case, not so much of duplication, but still, put into a common
> function (to be symmetric to devlink_params_register() and
> devlink_port_params_register()
Okay.
>
>
> >+      mutex_unlock(&devlink->lock);
> >+}
> >+EXPORT_SYMBOL_GPL(devlink_port_params_unregister);
> >+
> >+/**
> >  *    devlink_region_create - create a new address region
> >  *
> >  *    @devlink: devlink
> >--
> >1.8.3.1
> >

Re: [PATCH net-next RFC 1/7] devlink: Add devlink_param for port register and unregister

[Jiri Pirko]

Thu, Dec 06, 2018 at 07:02:59AM CET, vasundhara-v.volam@broadcom.com wrote:
>Thank you reviewing the patches.
>
>On Wed, Dec 5, 2018 at 5:24 PM Jiri Pirko <jiri@resnulli.us> wrote:
>>
>> Wed, Dec 05, 2018 at 06:56:54AM CET, vasundhara-v.volam@broadcom.com wrote:
>> >Add functions to register and unregister for the driver supported
>> >configuration parameters table per port.
>> >
>> >Cc: Jiri Pirko <jiri@mellanox.com>
>> >Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
>> >---
>> > include/net/devlink.h |  29 +++++++++++
>> > net/core/devlink.c    | 130 ++++++++++++++++++++++++++++++++++++++++++++++----
>> > 2 files changed, 151 insertions(+), 8 deletions(-)
>> >
>> >diff --git a/include/net/devlink.h b/include/net/devlink.h
>> >index 67f4293..9b4d80b 100644
>> >--- a/include/net/devlink.h
>> >+++ b/include/net/devlink.h
>> >@@ -48,6 +48,7 @@ struct devlink_port_attrs {
>> >
>> > struct devlink_port {
>> >       struct list_head list;
>> >+      struct list_head param_list;
>> >       struct devlink *devlink;
>> >       unsigned index;
>> >       bool registered;
>> >@@ -419,6 +420,13 @@ enum devlink_param_generic_id {
>> >       .validate = _validate,                                          \
>> > }
>> >
>> >+enum devlink_port_param_generic_id {
>> >+      /* add new param generic ids above here */
>> >+      __DEVLINK_PORT_PARAM_GENERIC_ID_MAX,
>> >+      DEVLINK_PORT_PARAM_GENERIC_ID_MAX =
>> >+             __DEVLINK_PORT_PARAM_GENERIC_ID_MAX - 1,
>> >+};
>>
>> I don't see the need for enum just for per-port params. The existing
>> params enum should be enough.
>In that case there won't be any differentiation between generic device
>and port params.

Yes, you don't need that.

>Also, if enum is not needed, then separate struct
>devlink_port_param_generic is also not needed.

Yep.

>
>Based on this, entire patchset needs a change.

[...]

Re: [PATCH net-next RFC 1/7] devlink: Add devlink_param for port register and unregister

[Vasundhara Volam]

On Thu, Dec 6, 2018 at 12:43 PM Jiri Pirko <jiri@resnulli.us> wrote:
>
> Thu, Dec 06, 2018 at 07:02:59AM CET, vasundhara-v.volam@broadcom.com wrote:
> >Thank you reviewing the patches.
> >
> >On Wed, Dec 5, 2018 at 5:24 PM Jiri Pirko <jiri@resnulli.us> wrote:
> >>
> >> Wed, Dec 05, 2018 at 06:56:54AM CET, vasundhara-v.volam@broadcom.com wrote:
> >> >Add functions to register and unregister for the driver supported
> >> >configuration parameters table per port.
> >> >
> >> >Cc: Jiri Pirko <jiri@mellanox.com>
> >> >Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
> >> >---
> >> > include/net/devlink.h |  29 +++++++++++
> >> > net/core/devlink.c    | 130 ++++++++++++++++++++++++++++++++++++++++++++++----
> >> > 2 files changed, 151 insertions(+), 8 deletions(-)
> >> >
> >> >diff --git a/include/net/devlink.h b/include/net/devlink.h
> >> >index 67f4293..9b4d80b 100644
> >> >--- a/include/net/devlink.h
> >> >+++ b/include/net/devlink.h
> >> >@@ -48,6 +48,7 @@ struct devlink_port_attrs {
> >> >
> >> > struct devlink_port {
> >> >       struct list_head list;
> >> >+      struct list_head param_list;
> >> >       struct devlink *devlink;
> >> >       unsigned index;
> >> >       bool registered;
> >> >@@ -419,6 +420,13 @@ enum devlink_param_generic_id {
> >> >       .validate = _validate,                                          \
> >> > }
> >> >
> >> >+enum devlink_port_param_generic_id {
> >> >+      /* add new param generic ids above here */
> >> >+      __DEVLINK_PORT_PARAM_GENERIC_ID_MAX,
> >> >+      DEVLINK_PORT_PARAM_GENERIC_ID_MAX =
> >> >+             __DEVLINK_PORT_PARAM_GENERIC_ID_MAX - 1,
> >> >+};
> >>
> >> I don't see the need for enum just for per-port params. The existing
> >> params enum should be enough.
> >In that case there won't be any differentiation between generic device
> >and port params.
>
> Yes, you don't need that.
Okay, then let me modify and refactor the patchset. Thanks.
>
> >Also, if enum is not needed, then separate struct
> >devlink_port_param_generic is also not needed.
>
> Yep.
>
> >
> >Based on this, entire patchset needs a change.
>
> [...]

Re: [PATCH net-next RFC 1/7] devlink: Add devlink_param for port register and unregister

[Vasundhara Volam]

On Wed, Dec 5, 2018 at 5:24 PM Jiri Pirko <jiri@resnulli.us> wrote:
>
> Wed, Dec 05, 2018 at 06:56:54AM CET, vasundhara-v.volam@broadcom.com wrote:
> >Add functions to register and unregister for the driver supported
> >configuration parameters table per port.
> >
> >Cc: Jiri Pirko <jiri@mellanox.com>
> >Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
> >---
> > include/net/devlink.h |  29 +++++++++++
> > net/core/devlink.c    | 130 ++++++++++++++++++++++++++++++++++++++++++++++----
> > 2 files changed, 151 insertions(+), 8 deletions(-)
> >
> >diff --git a/include/net/devlink.h b/include/net/devlink.h
> >index 67f4293..9b4d80b 100644
> >--- a/include/net/devlink.h
> >+++ b/include/net/devlink.h
> >@@ -48,6 +48,7 @@ struct devlink_port_attrs {
> >
> > struct devlink_port {
> >       struct list_head list;
> >+      struct list_head param_list;
> >       struct devlink *devlink;
> >       unsigned index;
> >       bool registered;
> >@@ -419,6 +420,13 @@ enum devlink_param_generic_id {
> >       .validate = _validate,                                          \
> > }
> >
> >+enum devlink_port_param_generic_id {
> >+      /* add new param generic ids above here */
> >+      __DEVLINK_PORT_PARAM_GENERIC_ID_MAX,
> >+      DEVLINK_PORT_PARAM_GENERIC_ID_MAX =
> >+             __DEVLINK_PORT_PARAM_GENERIC_ID_MAX - 1,
> >+};
>
> I don't see the need for enum just for per-port params. The existing
> params enum should be enough.
>
>
> >+
> > struct devlink_region;
> >
> > typedef void devlink_snapshot_data_dest_t(const void *data);
> >@@ -574,6 +582,12 @@ int devlink_param_driverinit_value_set(struct devlink *devlink, u32 param_id,
> > void devlink_param_value_changed(struct devlink *devlink, u32 param_id);
> > void devlink_param_value_str_fill(union devlink_param_value *dst_val,
> >                                 const char *src);
> >+int devlink_port_params_register(struct devlink_port *devlink_port,
> >+                               const struct devlink_param *params,
> >+                               size_t params_count);
> >+void devlink_port_params_unregister(struct devlink_port *devlink_port,
> >+                                  const struct devlink_param *params,
> >+                                  size_t params_count);
> > struct devlink_region *devlink_region_create(struct devlink *devlink,
> >                                            const char *region_name,
> >                                            u32 region_max_snapshots,
> >@@ -816,6 +830,21 @@ static inline bool devlink_dpipe_table_counter_enabled(struct devlink *devlink,
> > {
> > }
> >
> >+static inline int
> >+devlink_port_params_register(struct devlink_port *devlink_port,
> >+                           const struct devlink_param *params,
> >+                           size_t params_count)
> >+{
> >+      return 0;
> >+}
> >+
> >+static inline void
> >+devlink_port_params_unregister(struct devlink_port *devlink_port,
> >+                             const struct devlink_param *params,
> >+                             size_t params_count)
> >+{
> >+}
> >+
> > static inline struct devlink_region *
> > devlink_region_create(struct devlink *devlink,
> >                     const char *region_name,
> >diff --git a/net/core/devlink.c b/net/core/devlink.c
> >index abb0da9..e194913 100644
> >--- a/net/core/devlink.c
> >+++ b/net/core/devlink.c
> >@@ -3147,12 +3147,12 @@ static int devlink_nl_cmd_param_set_doit(struct sk_buff *skb,
> > }
> >
> > static int devlink_param_register_one(struct devlink *devlink,
> >+                                    struct list_head *param_list,
> >                                     const struct devlink_param *param)
> > {
> >       struct devlink_param_item *param_item;
> >
> >-      if (devlink_param_find_by_name(&devlink->param_list,
> >-                                     param->name))
> >+      if (devlink_param_find_by_name(param_list, param->name))
> >               return -EEXIST;
> >
> >       if (param->supported_cmodes == BIT(DEVLINK_PARAM_CMODE_DRIVERINIT))
> >@@ -3165,24 +3165,54 @@ static int devlink_param_register_one(struct devlink *devlink,
> >               return -ENOMEM;
> >       param_item->param = param;
> >
> >-      list_add_tail(&param_item->list, &devlink->param_list);
> >+      list_add_tail(&param_item->list, param_list);
> >       devlink_param_notify(devlink, param_item, DEVLINK_CMD_PARAM_NEW);
> >       return 0;
> > }
> >
> > static void devlink_param_unregister_one(struct devlink *devlink,
> >+                                       struct list_head *param_list,
> >                                        const struct devlink_param *param)
> > {
> >       struct devlink_param_item *param_item;
> >
> >-      param_item = devlink_param_find_by_name(&devlink->param_list,
> >-                                              param->name);
> >+      param_item = devlink_param_find_by_name(param_list, param->name);
> >       WARN_ON(!param_item);
> >       devlink_param_notify(devlink, param_item, DEVLINK_CMD_PARAM_DEL);
> >       list_del(&param_item->list);
> >       kfree(param_item);
> > }
> >
> >+static const struct devlink_param devlink_port_param_generic[] = {};
> >+
> >+static int devlink_port_param_generic_verify(const struct devlink_param *param)
> >+{
> >+      /* verify it matches generic parameter by id and name */
> >+      if (param->id > DEVLINK_PORT_PARAM_GENERIC_ID_MAX)
> >+              return -EINVAL;
> >+      if (strcmp(param->name, devlink_port_param_generic[param->id].name))
> >+              return -ENOENT;
> >+
> >+      WARN_ON(param->type != devlink_port_param_generic[param->id].type);
> >+
> >+      return 0;
> >+}
> >+
> >+static int devlink_port_param_driver_verify(const struct devlink_param *param)
> >+{
> >+      int i;
> >+
> >+      if (param->id <= DEVLINK_PORT_PARAM_GENERIC_ID_MAX)
> >+              return -EINVAL;
> >+      /* verify no such name in generic params */
> >+      for (i = 0; i <= DEVLINK_PORT_PARAM_GENERIC_ID_MAX; i++)
> >+              if (!strcmp(param->name,
> >+                          devlink_port_param_generic[param->id].name))
> >+                      return -EEXIST;
> >+
> >+      return 0;
> >+}
> >+
> > static int devlink_nl_region_snapshot_id_put(struct sk_buff *msg,
> >                                            struct devlink *devlink,
> >                                            struct devlink_snapshot *snapshot)
> >@@ -4503,7 +4533,8 @@ int devlink_params_register(struct devlink *devlink,
> >                       if (err)
> >                               goto rollback;
> >               }
> >-              err = devlink_param_register_one(devlink, param);
> >+              err = devlink_param_register_one(devlink, &devlink->param_list,
> >+                                               param);
> >               if (err)
> >                       goto rollback;
> >       }
> >@@ -4515,7 +4546,8 @@ int devlink_params_register(struct devlink *devlink,
> >       if (!i)
> >               goto unlock;
> >       for (param--; i > 0; i--, param--)
> >-              devlink_param_unregister_one(devlink, param);
> >+              devlink_param_unregister_one(devlink, &devlink->param_list,
> >+                                           param);
> > unlock:
> >       mutex_unlock(&devlink->lock);
> >       return err;
> >@@ -4537,7 +4569,8 @@ void devlink_params_unregister(struct devlink *devlink,
> >
> >       mutex_lock(&devlink->lock);
> >       for (i = 0; i < params_count; i++, param++)
> >-              devlink_param_unregister_one(devlink, param);
> >+              devlink_param_unregister_one(devlink, &devlink->param_list,
> >+                                           param);
> >       mutex_unlock(&devlink->lock);
> > }
> > EXPORT_SYMBOL_GPL(devlink_params_unregister);
> >@@ -4657,6 +4690,87 @@ void devlink_param_value_str_fill(union devlink_param_value *dst_val,
> > EXPORT_SYMBOL_GPL(devlink_param_value_str_fill);
> >
> > /**
> >+ *    devlink_port_params_register - register port configuration parameters
> >+ *
> >+ *    @devlink_port: devlink port
> >+ *    @params: configuration parameters array
> >+ *    @params_count: number of parameters provided
> >+ *
> >+ *    Register the configuration parameters supported by the port.
> >+ */
> >+int devlink_port_params_register(struct devlink_port *devlink_port,
> >+                               const struct devlink_param *params,
> >+                               size_t params_count)
> >+{
> >+      struct devlink *devlink = devlink_port->devlink;
> >+      const struct devlink_param *param = params;
> >+      int i, err;
> >+
> >+      INIT_LIST_HEAD(&devlink_port->param_list);
>
> Driver can call devlink_port_params_register multiple times. You need to
> move the list init into devlink_port_register()
>
>
> >+      mutex_lock(&devlink->lock);
> >+      for (i = 0; i < params_count; i++) {
> >+              if (!param || !param->name || !param->supported_cmodes) {
> >+                      err = -EINVAL;
> >+                      goto rollback;
> >+              }
> >+              if (param->generic) {
> >+                      err = devlink_port_param_generic_verify(param);
> >+                      if (err)
> >+                              goto rollback;
> >+              } else {
> >+                      err = devlink_port_param_driver_verify(param);
>
> This is duplicated code from devlink_params_register(). Once you use a
> single enum for all params, you can push this into a common function for
> both devlink_params_register() and devlink_port_params_register()
Jiri, you are referring to move only param verification code into a
common function. right?
>
>
> >+                      if (err)
> >+                              goto rollback;
> >+              }
> >+              err = devlink_param_register_one(devlink_port->devlink,
> >+                                               &devlink_port->param_list,
> >+                                               param);
> >+              if (err)
> >+                      goto rollback;
> >+      }
> >+
> >+      mutex_unlock(&devlink->lock);
> >+      return 0;
> >+
> >+rollback:
> >+      if (!i)
> >+              goto unlock;
> >+      for (param--; i > 0; i--, param--)
> >+              devlink_param_unregister_one(devlink_port->devlink,
> >+                                           &devlink_port->param_list,
> >+                                           param);
> >+unlock:
> >+      mutex_unlock(&devlink->lock);
> >+      return err;
> >+}
> >+EXPORT_SYMBOL_GPL(devlink_port_params_register);
> >+
> >+/**
> >+ *    devlink_port_params_unregister - unregister port configuration
> >+ *    parameters
> >+ *
> >+ *    @devlink_port: devlink port
> >+ *    @params: configuration parameters array
> >+ *    @params_count: number of parameters provided
> >+ */
> >+void devlink_port_params_unregister(struct devlink_port *devlink_port,
> >+                                  const struct devlink_param *params,
> >+                                  size_t params_count)
> >+{
> >+      struct devlink *devlink = devlink_port->devlink;
> >+      const struct devlink_param *param = params;
> >+      int i;
> >+
> >+      mutex_lock(&devlink->lock);
> >+      for (i = 0; i < params_count; i++, param++)
> >+              devlink_param_unregister_one(devlink_port->devlink,
> >+                                           &devlink_port->param_list,
> >+                                           param);
>
> In this case, not so much of duplication, but still, put into a common
> function (to be symmetric to devlink_params_register() and
> devlink_port_params_register()
>
>
> >+      mutex_unlock(&devlink->lock);
> >+}
> >+EXPORT_SYMBOL_GPL(devlink_port_params_unregister);
> >+
> >+/**
> >  *    devlink_region_create - create a new address region
> >  *
> >  *    @devlink: devlink
> >--
> >1.8.3.1
> >

Re: [PATCH net-next RFC 1/7] devlink: Add devlink_param for port register and unregister

[Jiri Pirko]

Mon, Dec 10, 2018 at 10:21:03AM CET, vasundhara-v.volam@broadcom.com wrote:
>On Wed, Dec 5, 2018 at 5:24 PM Jiri Pirko <jiri@resnulli.us> wrote:
>>
>> Wed, Dec 05, 2018 at 06:56:54AM CET, vasundhara-v.volam@broadcom.com wrote:

[...]

>> >+      mutex_lock(&devlink->lock);
>> >+      for (i = 0; i < params_count; i++) {
>> >+              if (!param || !param->name || !param->supported_cmodes) {
>> >+                      err = -EINVAL;
>> >+                      goto rollback;
>> >+              }
>> >+              if (param->generic) {
>> >+                      err = devlink_port_param_generic_verify(param);
>> >+                      if (err)
>> >+                              goto rollback;
>> >+              } else {
>> >+                      err = devlink_port_param_driver_verify(param);
>>
>> This is duplicated code from devlink_params_register(). Once you use a
>> single enum for all params, you can push this into a common function for
>> both devlink_params_register() and devlink_port_params_register()
>Jiri, you are referring to move only param verification code into a
>common function. right?

Once you will have a single enum, you need only one verify() function.

[PATCH net-next RFC 2/7] devlink: Add port param get command

[Vasundhara Volam]

Add port param get command which gets data per parameter.
It also has option to dump the parameters data per port.

Cc: Jiri Pirko <jiri@mellanox.com>
Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
---
 include/uapi/linux/devlink.h |   2 +
 net/core/devlink.c           | 102 ++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 97 insertions(+), 7 deletions(-)

diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
index 6e52d36..f96e052 100644
--- a/include/uapi/linux/devlink.h
+++ b/include/uapi/linux/devlink.h
@@ -83,6 +83,8 @@ enum devlink_command {
 	DEVLINK_CMD_PARAM_NEW,
 	DEVLINK_CMD_PARAM_DEL,
 
+	DEVLINK_CMD_PORT_PARAM_GET,	/* can dump */
+
 	DEVLINK_CMD_REGION_GET,
 	DEVLINK_CMD_REGION_SET,
 	DEVLINK_CMD_REGION_NEW,
diff --git a/net/core/devlink.c b/net/core/devlink.c
index e194913..8653fb5 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -2843,6 +2843,7 @@ static int devlink_param_set(struct devlink *devlink,
 }
 
 static int devlink_nl_param_fill(struct sk_buff *msg, struct devlink *devlink,
+				 unsigned int port_index,
 				 struct devlink_param_item *param_item,
 				 enum devlink_command cmd,
 				 u32 portid, u32 seq, int flags)
@@ -2880,6 +2881,11 @@ static int devlink_nl_param_fill(struct sk_buff *msg, struct devlink *devlink,
 
 	if (devlink_nl_put_handle(msg, devlink))
 		goto genlmsg_cancel;
+
+	if (cmd == DEVLINK_CMD_PORT_PARAM_GET)
+		if (nla_put_u32(msg, DEVLINK_ATTR_PORT_INDEX, port_index))
+			goto genlmsg_cancel;
+
 	param_attr = nla_nest_start(msg, DEVLINK_ATTR_PARAM);
 	if (!param_attr)
 		goto genlmsg_cancel;
@@ -2933,7 +2939,7 @@ static void devlink_param_notify(struct devlink *devlink,
 	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
 	if (!msg)
 		return;
-	err = devlink_nl_param_fill(msg, devlink, param_item, cmd, 0, 0, 0);
+	err = devlink_nl_param_fill(msg, devlink, 0, param_item, cmd, 0, 0, 0);
 	if (err) {
 		nlmsg_free(msg);
 		return;
@@ -2962,7 +2968,7 @@ static int devlink_nl_cmd_param_get_dumpit(struct sk_buff *msg,
 				idx++;
 				continue;
 			}
-			err = devlink_nl_param_fill(msg, devlink, param_item,
+			err = devlink_nl_param_fill(msg, devlink, 0, param_item,
 						    DEVLINK_CMD_PARAM_GET,
 						    NETLINK_CB(cb->skb).portid,
 						    cb->nlh->nlmsg_seq,
@@ -3051,7 +3057,7 @@ static int devlink_nl_cmd_param_get_dumpit(struct sk_buff *msg,
 }
 
 static struct devlink_param_item *
-devlink_param_get_from_info(struct devlink *devlink,
+devlink_param_get_from_info(struct list_head *param_list,
 			    struct genl_info *info)
 {
 	char *param_name;
@@ -3060,7 +3066,7 @@ static int devlink_nl_cmd_param_get_dumpit(struct sk_buff *msg,
 		return NULL;
 
 	param_name = nla_data(info->attrs[DEVLINK_ATTR_PARAM_NAME]);
-	return devlink_param_find_by_name(&devlink->param_list, param_name);
+	return devlink_param_find_by_name(param_list, param_name);
 }
 
 static int devlink_nl_cmd_param_get_doit(struct sk_buff *skb,
@@ -3071,7 +3077,7 @@ static int devlink_nl_cmd_param_get_doit(struct sk_buff *skb,
 	struct sk_buff *msg;
 	int err;
 
-	param_item = devlink_param_get_from_info(devlink, info);
+	param_item = devlink_param_get_from_info(&devlink->param_list, info);
 	if (!param_item)
 		return -EINVAL;
 
@@ -3079,7 +3085,7 @@ static int devlink_nl_cmd_param_get_doit(struct sk_buff *skb,
 	if (!msg)
 		return -ENOMEM;
 
-	err = devlink_nl_param_fill(msg, devlink, param_item,
+	err = devlink_nl_param_fill(msg, devlink, 0, param_item,
 				    DEVLINK_CMD_PARAM_GET,
 				    info->snd_portid, info->snd_seq, 0);
 	if (err) {
@@ -3102,7 +3108,7 @@ static int devlink_nl_cmd_param_set_doit(struct sk_buff *skb,
 	union devlink_param_value value;
 	int err = 0;
 
-	param_item = devlink_param_get_from_info(devlink, info);
+	param_item = devlink_param_get_from_info(&devlink->param_list, info);
 	if (!param_item)
 		return -EINVAL;
 	param = param_item->param;
@@ -3213,6 +3219,80 @@ static int devlink_port_param_driver_verify(const struct devlink_param *param)
 	return 0;
 }
 
+static int devlink_nl_cmd_port_param_get_dumpit(struct sk_buff *msg,
+						struct netlink_callback *cb)
+{
+	struct devlink_param_item *param_item;
+	struct devlink_port *devlink_port;
+	struct devlink *devlink;
+	int start = cb->args[0];
+	int idx = 0;
+	int err;
+
+	mutex_lock(&devlink_mutex);
+	list_for_each_entry(devlink, &devlink_list, list) {
+		if (!net_eq(devlink_net(devlink), sock_net(msg->sk)))
+			continue;
+		mutex_lock(&devlink->lock);
+		list_for_each_entry(devlink_port, &devlink->port_list, list) {
+			list_for_each_entry(param_item,
+					    &devlink_port->param_list, list) {
+				if (idx < start) {
+					idx++;
+					continue;
+				}
+				err = devlink_nl_param_fill(msg,
+						devlink_port->devlink,
+						devlink_port->index, param_item,
+						DEVLINK_CMD_PORT_PARAM_GET,
+						NETLINK_CB(cb->skb).portid,
+						cb->nlh->nlmsg_seq,
+						NLM_F_MULTI);
+				if (err) {
+					mutex_unlock(&devlink->lock);
+					goto out;
+				}
+				idx++;
+			}
+		}
+		mutex_unlock(&devlink->lock);
+	}
+out:
+	mutex_unlock(&devlink_mutex);
+
+	cb->args[0] = idx;
+	return msg->len;
+}
+
+static int devlink_nl_cmd_port_param_get_doit(struct sk_buff *skb,
+					      struct genl_info *info)
+{
+	struct devlink_port *devlink_port = info->user_ptr[0];
+	struct devlink_param_item *param_item;
+	struct sk_buff *msg;
+	int err;
+
+	param_item = devlink_param_get_from_info(&devlink_port->param_list,
+						 info);
+	if (!param_item)
+		return -EINVAL;
+
+	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
+	if (!msg)
+		return -ENOMEM;
+
+	err = devlink_nl_param_fill(msg, devlink_port->devlink,
+				    devlink_port->index, param_item,
+				    DEVLINK_CMD_PORT_PARAM_GET,
+				    info->snd_portid, info->snd_seq, 0);
+	if (err) {
+		nlmsg_free(msg);
+		return err;
+	}
+
+	return genlmsg_reply(msg, info);
+}
+
 static int devlink_nl_region_snapshot_id_put(struct sk_buff *msg,
 					     struct devlink *devlink,
 					     struct devlink_snapshot *snapshot)
@@ -3851,6 +3931,14 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
 		.internal_flags = DEVLINK_NL_FLAG_NEED_DEVLINK,
 	},
 	{
+		.cmd = DEVLINK_CMD_PORT_PARAM_GET,
+		.doit = devlink_nl_cmd_port_param_get_doit,
+		.dumpit = devlink_nl_cmd_port_param_get_dumpit,
+		.policy = devlink_nl_policy,
+		.internal_flags = DEVLINK_NL_FLAG_NEED_PORT,
+		/* can be retrieved by unprivileged users */
+	},
+	{
 		.cmd = DEVLINK_CMD_REGION_GET,
 		.doit = devlink_nl_cmd_region_get_doit,
 		.dumpit = devlink_nl_cmd_region_get_dumpit,
-- 
1.8.3.1

Re: [PATCH net-next RFC 2/7] devlink: Add port param get command

[Jiri Pirko]

Wed, Dec 05, 2018 at 06:56:55AM CET, vasundhara-v.volam@broadcom.com wrote:
>Add port param get command which gets data per parameter.
>It also has option to dump the parameters data per port.
>
>Cc: Jiri Pirko <jiri@mellanox.com>
>Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
>---
> include/uapi/linux/devlink.h |   2 +
> net/core/devlink.c           | 102 ++++++++++++++++++++++++++++++++++++++++---
> 2 files changed, 97 insertions(+), 7 deletions(-)
>
>diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
>index 6e52d36..f96e052 100644
>--- a/include/uapi/linux/devlink.h
>+++ b/include/uapi/linux/devlink.h
>@@ -83,6 +83,8 @@ enum devlink_command {
> 	DEVLINK_CMD_PARAM_NEW,
> 	DEVLINK_CMD_PARAM_DEL,
> 
>+	DEVLINK_CMD_PORT_PARAM_GET,	/* can dump */

You need to add this to the end, otherwise you would break uapi.


>+
> 	DEVLINK_CMD_REGION_GET,
> 	DEVLINK_CMD_REGION_SET,
> 	DEVLINK_CMD_REGION_NEW,

[...]

The rest of the patch looks fine.

[PATCH net-next RFC 3/7] devlink: Add port param set command

[Vasundhara Volam]

Add port param set command to set the value for a parameter.
Value can be set to any of the supported configuration modes.

Cc: Jiri Pirko <jiri@mellanox.com>
Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
---
 include/uapi/linux/devlink.h |  1 +
 net/core/devlink.c           | 41 ++++++++++++++++++++++++++++++++++-------
 2 files changed, 35 insertions(+), 7 deletions(-)

diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
index f96e052..8f3c5dd 100644
--- a/include/uapi/linux/devlink.h
+++ b/include/uapi/linux/devlink.h
@@ -84,6 +84,7 @@ enum devlink_command {
 	DEVLINK_CMD_PARAM_DEL,
 
 	DEVLINK_CMD_PORT_PARAM_GET,	/* can dump */
+	DEVLINK_CMD_PORT_PARAM_SET,
 
 	DEVLINK_CMD_REGION_GET,
 	DEVLINK_CMD_REGION_SET,
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 8653fb5..10e1c45 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -3096,19 +3096,20 @@ static int devlink_nl_cmd_param_get_doit(struct sk_buff *skb,
 	return genlmsg_reply(msg, info);
 }
 
-static int devlink_nl_cmd_param_set_doit(struct sk_buff *skb,
-					 struct genl_info *info)
+static int __devlink_nl_cmd_param_set_doit(struct devlink *devlink,
+					   struct list_head *param_list,
+					   struct genl_info *info,
+					   enum devlink_command cmd)
 {
-	struct devlink *devlink = info->user_ptr[0];
+	struct devlink_param_item *param_item;
 	enum devlink_param_type param_type;
 	struct devlink_param_gset_ctx ctx;
-	enum devlink_param_cmode cmode;
-	struct devlink_param_item *param_item;
 	const struct devlink_param *param;
 	union devlink_param_value value;
+	enum devlink_param_cmode cmode;
 	int err = 0;
 
-	param_item = devlink_param_get_from_info(&devlink->param_list, info);
+	param_item = devlink_param_get_from_info(param_list, info);
 	if (!param_item)
 		return -EINVAL;
 	param = param_item->param;
@@ -3148,10 +3149,19 @@ static int devlink_nl_cmd_param_set_doit(struct sk_buff *skb,
 			return err;
 	}
 
-	devlink_param_notify(devlink, param_item, DEVLINK_CMD_PARAM_NEW);
+	devlink_param_notify(devlink, param_item, cmd);
 	return 0;
 }
 
+static int devlink_nl_cmd_param_set_doit(struct sk_buff *skb,
+					 struct genl_info *info)
+{
+	struct devlink *devlink = info->user_ptr[0];
+
+	return __devlink_nl_cmd_param_set_doit(devlink, &devlink->param_list,
+					       info, DEVLINK_CMD_PARAM_NEW);
+}
+
 static int devlink_param_register_one(struct devlink *devlink,
 				      struct list_head *param_list,
 				      const struct devlink_param *param)
@@ -3293,6 +3303,16 @@ static int devlink_nl_cmd_port_param_get_doit(struct sk_buff *skb,
 	return genlmsg_reply(msg, info);
 }
 
+static int devlink_nl_cmd_port_param_set_doit(struct sk_buff *skb,
+					      struct genl_info *info)
+{
+	struct devlink_port *devlink_port = info->user_ptr[0];
+
+	return __devlink_nl_cmd_param_set_doit(devlink_port->devlink,
+					       &devlink_port->param_list,
+					       info, 0);
+}
+
 static int devlink_nl_region_snapshot_id_put(struct sk_buff *msg,
 					     struct devlink *devlink,
 					     struct devlink_snapshot *snapshot)
@@ -3939,6 +3959,13 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
 		/* can be retrieved by unprivileged users */
 	},
 	{
+		.cmd = DEVLINK_CMD_PORT_PARAM_SET,
+		.doit = devlink_nl_cmd_port_param_set_doit,
+		.policy = devlink_nl_policy,
+		.flags = GENL_ADMIN_PERM,
+		.internal_flags = DEVLINK_NL_FLAG_NEED_PORT,
+	},
+	{
 		.cmd = DEVLINK_CMD_REGION_GET,
 		.doit = devlink_nl_cmd_region_get_doit,
 		.dumpit = devlink_nl_cmd_region_get_dumpit,
-- 
1.8.3.1

Re: [PATCH net-next RFC 3/7] devlink: Add port param set command

[Jiri Pirko]

Wed, Dec 05, 2018 at 06:56:56AM CET, vasundhara-v.volam@broadcom.com wrote:
>Add port param set command to set the value for a parameter.
>Value can be set to any of the supported configuration modes.
>
>Cc: Jiri Pirko <jiri@mellanox.com>
>Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
>---
> include/uapi/linux/devlink.h |  1 +
> net/core/devlink.c           | 41 ++++++++++++++++++++++++++++++++++-------
> 2 files changed, 35 insertions(+), 7 deletions(-)
>
>diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
>index f96e052..8f3c5dd 100644
>--- a/include/uapi/linux/devlink.h
>+++ b/include/uapi/linux/devlink.h
>@@ -84,6 +84,7 @@ enum devlink_command {
> 	DEVLINK_CMD_PARAM_DEL,
> 
> 	DEVLINK_CMD_PORT_PARAM_GET,	/* can dump */
>+	DEVLINK_CMD_PORT_PARAM_SET,

Same note as for the previous patch.

> 
> 	DEVLINK_CMD_REGION_GET,
> 	DEVLINK_CMD_REGION_SET,
>diff --git a/net/core/devlink.c b/net/core/devlink.c
>index 8653fb5..10e1c45 100644
>--- a/net/core/devlink.c
>+++ b/net/core/devlink.c
>@@ -3096,19 +3096,20 @@ static int devlink_nl_cmd_param_get_doit(struct sk_buff *skb,
> 	return genlmsg_reply(msg, info);
> }
> 
>-static int devlink_nl_cmd_param_set_doit(struct sk_buff *skb,
>-					 struct genl_info *info)
>+static int __devlink_nl_cmd_param_set_doit(struct devlink *devlink,
>+					   struct list_head *param_list,
>+					   struct genl_info *info,
>+					   enum devlink_command cmd)
> {
>-	struct devlink *devlink = info->user_ptr[0];
>+	struct devlink_param_item *param_item;
> 	enum devlink_param_type param_type;
> 	struct devlink_param_gset_ctx ctx;
>-	enum devlink_param_cmode cmode;
>-	struct devlink_param_item *param_item;
> 	const struct devlink_param *param;
> 	union devlink_param_value value;
>+	enum devlink_param_cmode cmode;

Don't move the things around for no good reason. In case you want to
make our reverse-Christmas-tree obsession satisfied, do it in
a separate patch :)

[...]

[PATCH net-next RFC 4/7] devlink: Add support for get/set driverinit value for devlink_port

[Vasundhara Volam]

Add support for "driverinit" configuration mode value for devlink_port
configuration parameters. Two additional functions added to help the
driver get/set the value from/to devlink:
devlink_port_param_driverinit_value_set() and
devlink_port_param_driverinit_value_get().

Also, move the common code to __devlink_param_driverinit_value_get/set()
to be used by both device and port params.

Cc: Jiri Pirko <jiri@mellanox.com>
Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
---
 include/net/devlink.h |  19 ++++++++
 net/core/devlink.c    | 118 +++++++++++++++++++++++++++++++++++++++-----------
 2 files changed, 112 insertions(+), 25 deletions(-)

diff --git a/include/net/devlink.h b/include/net/devlink.h
index 9b4d80b..c22b195 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -588,6 +588,9 @@ int devlink_port_params_register(struct devlink_port *devlink_port,
 void devlink_port_params_unregister(struct devlink_port *devlink_port,
 				    const struct devlink_param *params,
 				    size_t params_count);
+int devlink_port_param_driverinit_value_set(struct devlink_port *devlink_port,
+					    u32 param_id,
+					    union devlink_param_value init_val);
 struct devlink_region *devlink_region_create(struct devlink *devlink,
 					     const char *region_name,
 					     u32 region_max_snapshots,
@@ -845,6 +848,22 @@ static inline bool devlink_dpipe_table_counter_enabled(struct devlink *devlink,
 {
 }
 
+static inline int
+devlink_port_param_driverinit_value_get(struct devlink_port *devlink_port,
+					u32 param_id,
+					union devlink_param_value *init_val)
+{
+	return -EOPNOTSUPP;
+}
+
+static inline int
+devlink_port_param_driverinit_value_set(struct devlink_port *devlink_port,
+					u32 param_id,
+					union devlink_param_value init_val)
+{
+	return -EOPNOTSUPP;
+}
+
 static inline struct devlink_region *
 devlink_region_create(struct devlink *devlink,
 		      const char *region_name,
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 10e1c45..4f0052d 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -4690,6 +4690,29 @@ void devlink_params_unregister(struct devlink *devlink,
 }
 EXPORT_SYMBOL_GPL(devlink_params_unregister);
 
+static int
+__devlink_param_driverinit_value_get(struct list_head *param_list, u32 param_id,
+				     union devlink_param_value *init_val)
+{
+	struct devlink_param_item *param_item;
+
+	param_item = devlink_param_find_by_id(param_list, param_id);
+	if (!param_item)
+		return -EINVAL;
+
+	if (!param_item->driverinit_value_valid ||
+	    !devlink_param_cmode_is_supported(param_item->param,
+					      DEVLINK_PARAM_CMODE_DRIVERINIT))
+		return -EOPNOTSUPP;
+
+	if (param_item->param->type == DEVLINK_PARAM_TYPE_STRING)
+		strcpy(init_val->vstr, param_item->driverinit_value.vstr);
+	else
+		*init_val = param_item->driverinit_value;
+
+	return 0;
+}
+
 /**
  *	devlink_param_driverinit_value_get - get configuration parameter
  *					     value for driver initializing
@@ -4704,28 +4727,40 @@ void devlink_params_unregister(struct devlink *devlink,
 int devlink_param_driverinit_value_get(struct devlink *devlink, u32 param_id,
 				       union devlink_param_value *init_val)
 {
-	struct devlink_param_item *param_item;
-
 	if (!devlink->ops || !devlink->ops->reload)
 		return -EOPNOTSUPP;
 
+	return __devlink_param_driverinit_value_get(&devlink->param_list,
+						    param_id, init_val);
+}
+EXPORT_SYMBOL_GPL(devlink_param_driverinit_value_get);
+
+static int
+__devlink_param_driverinit_value_set(struct devlink *devlink,
+				     struct list_head *param_list,
+				     u32 param_id,
+				     union devlink_param_value init_val,
+				     enum devlink_command cmd)
+{
+	struct devlink_param_item *param_item;
+
 	param_item = devlink_param_find_by_id(&devlink->param_list, param_id);
 	if (!param_item)
 		return -EINVAL;
 
-	if (!param_item->driverinit_value_valid ||
-	    !devlink_param_cmode_is_supported(param_item->param,
+	if (!devlink_param_cmode_is_supported(param_item->param,
 					      DEVLINK_PARAM_CMODE_DRIVERINIT))
 		return -EOPNOTSUPP;
 
 	if (param_item->param->type == DEVLINK_PARAM_TYPE_STRING)
-		strcpy(init_val->vstr, param_item->driverinit_value.vstr);
+		strcpy(param_item->driverinit_value.vstr, init_val.vstr);
 	else
-		*init_val = param_item->driverinit_value;
+		param_item->driverinit_value = init_val;
+	param_item->driverinit_value_valid = true;
+	devlink_param_notify(devlink, param_item, cmd);
 
 	return 0;
 }
-EXPORT_SYMBOL_GPL(devlink_param_driverinit_value_get);
 
 /**
  *	devlink_param_driverinit_value_set - set value of configuration
@@ -4742,24 +4777,10 @@ int devlink_param_driverinit_value_get(struct devlink *devlink, u32 param_id,
 int devlink_param_driverinit_value_set(struct devlink *devlink, u32 param_id,
 				       union devlink_param_value init_val)
 {
-	struct devlink_param_item *param_item;
-
-	param_item = devlink_param_find_by_id(&devlink->param_list, param_id);
-	if (!param_item)
-		return -EINVAL;
-
-	if (!devlink_param_cmode_is_supported(param_item->param,
-					      DEVLINK_PARAM_CMODE_DRIVERINIT))
-		return -EOPNOTSUPP;
-
-	if (param_item->param->type == DEVLINK_PARAM_TYPE_STRING)
-		strcpy(param_item->driverinit_value.vstr, init_val.vstr);
-	else
-		param_item->driverinit_value = init_val;
-	param_item->driverinit_value_valid = true;
-
-	devlink_param_notify(devlink, param_item, DEVLINK_CMD_PARAM_NEW);
-	return 0;
+	return __devlink_param_driverinit_value_set(devlink,
+						    &devlink->param_list,
+						    param_id, init_val,
+						    DEVLINK_CMD_PARAM_NEW);
 }
 EXPORT_SYMBOL_GPL(devlink_param_driverinit_value_set);
 
@@ -4886,6 +4907,53 @@ void devlink_port_params_unregister(struct devlink_port *devlink_port,
 EXPORT_SYMBOL_GPL(devlink_port_params_unregister);
 
 /**
+ *	devlink_port_param_driverinit_value_get - get configuration parameter
+ *						value for driver initializing
+ *
+ *	@devlink_port: devlink_port
+ *	@param_id: parameter ID
+ *	@init_val: value of parameter in driverinit configuration mode
+ *
+ *	This function should be used by the driver to get driverinit
+ *	configuration for initialization after reload command.
+ */
+int devlink_port_param_driverinit_value_get(struct devlink_port *devlink_port,
+					    u32 param_id,
+					    union devlink_param_value *init_val)
+{
+	struct devlink *devlink = devlink_port->devlink;
+
+	if (!devlink->ops || !devlink->ops->reload)
+		return -EOPNOTSUPP;
+
+	return __devlink_param_driverinit_value_get(&devlink_port->param_list,
+						    param_id, init_val);
+}
+EXPORT_SYMBOL_GPL(devlink_port_param_driverinit_value_get);
+
+/**
+ *	devlink_port_param_driverinit_value_set - set value of configuration
+ *						  parameter for driverinit
+ *						  configuration mode
+ *
+ *	@devlink_port: devlink_port
+ *	@param_id: parameter ID
+ *	@init_val: value of parameter to set for driverinit configuration mode
+ *
+ *	This function should be used by the driver to set driverinit
+ *	configuration mode default value.
+ */
+int devlink_port_param_driverinit_value_set(struct devlink_port *devlink_port,
+					    u32 param_id,
+					    union devlink_param_value init_val)
+{
+	return __devlink_param_driverinit_value_set(devlink_port->devlink,
+						    &devlink_port->param_list,
+						    param_id, init_val, 0);
+}
+EXPORT_SYMBOL_GPL(devlink_port_param_driverinit_value_set);
+
+/**
  *	devlink_region_create - create a new address region
  *
  *	@devlink: devlink
-- 
1.8.3.1

Re: [PATCH net-next RFC 4/7] devlink: Add support for get/set driverinit value for devlink_port

[Jiri Pirko]

Wed, Dec 05, 2018 at 06:56:57AM CET, vasundhara-v.volam@broadcom.com wrote:
>Add support for "driverinit" configuration mode value for devlink_port
>configuration parameters. Two additional functions added to help the
>driver get/set the value from/to devlink:
>devlink_port_param_driverinit_value_set() and
>devlink_port_param_driverinit_value_get().
>
>Also, move the common code to __devlink_param_driverinit_value_get/set()
>to be used by both device and port params.
>
>Cc: Jiri Pirko <jiri@mellanox.com>
>Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
>---
> include/net/devlink.h |  19 ++++++++
> net/core/devlink.c    | 118 +++++++++++++++++++++++++++++++++++++++-----------
> 2 files changed, 112 insertions(+), 25 deletions(-)
>
>diff --git a/include/net/devlink.h b/include/net/devlink.h
>index 9b4d80b..c22b195 100644
>--- a/include/net/devlink.h
>+++ b/include/net/devlink.h
>@@ -588,6 +588,9 @@ int devlink_port_params_register(struct devlink_port *devlink_port,
> void devlink_port_params_unregister(struct devlink_port *devlink_port,
> 				    const struct devlink_param *params,
> 				    size_t params_count);
>+int devlink_port_param_driverinit_value_set(struct devlink_port *devlink_port,
>+					    u32 param_id,
>+					    union devlink_param_value init_val);
> struct devlink_region *devlink_region_create(struct devlink *devlink,
> 					     const char *region_name,
> 					     u32 region_max_snapshots,
>@@ -845,6 +848,22 @@ static inline bool devlink_dpipe_table_counter_enabled(struct devlink *devlink,
> {
> }
> 
>+static inline int
>+devlink_port_param_driverinit_value_get(struct devlink_port *devlink_port,
>+					u32 param_id,
>+					union devlink_param_value *init_val)
>+{
>+	return -EOPNOTSUPP;
>+}
>+
>+static inline int
>+devlink_port_param_driverinit_value_set(struct devlink_port *devlink_port,
>+					u32 param_id,
>+					union devlink_param_value init_val)
>+{
>+	return -EOPNOTSUPP;
>+}
>+
> static inline struct devlink_region *
> devlink_region_create(struct devlink *devlink,
> 		      const char *region_name,
>diff --git a/net/core/devlink.c b/net/core/devlink.c
>index 10e1c45..4f0052d 100644
>--- a/net/core/devlink.c
>+++ b/net/core/devlink.c
>@@ -4690,6 +4690,29 @@ void devlink_params_unregister(struct devlink *devlink,
> }
> EXPORT_SYMBOL_GPL(devlink_params_unregister);
> 
>+static int
>+__devlink_param_driverinit_value_get(struct list_head *param_list, u32 param_id,
>+				     union devlink_param_value *init_val)
>+{
>+	struct devlink_param_item *param_item;
>+
>+	param_item = devlink_param_find_by_id(param_list, param_id);
>+	if (!param_item)
>+		return -EINVAL;
>+
>+	if (!param_item->driverinit_value_valid ||
>+	    !devlink_param_cmode_is_supported(param_item->param,
>+					      DEVLINK_PARAM_CMODE_DRIVERINIT))
>+		return -EOPNOTSUPP;
>+
>+	if (param_item->param->type == DEVLINK_PARAM_TYPE_STRING)
>+		strcpy(init_val->vstr, param_item->driverinit_value.vstr);
>+	else
>+		*init_val = param_item->driverinit_value;
>+
>+	return 0;
>+}
>+
> /**
>  *	devlink_param_driverinit_value_get - get configuration parameter
>  *					     value for driver initializing
>@@ -4704,28 +4727,40 @@ void devlink_params_unregister(struct devlink *devlink,
> int devlink_param_driverinit_value_get(struct devlink *devlink, u32 param_id,
> 				       union devlink_param_value *init_val)
> {
>-	struct devlink_param_item *param_item;
>-
> 	if (!devlink->ops || !devlink->ops->reload)
> 		return -EOPNOTSUPP;
> 
>+	return __devlink_param_driverinit_value_get(&devlink->param_list,
>+						    param_id, init_val);
>+}
>+EXPORT_SYMBOL_GPL(devlink_param_driverinit_value_get);
>+
>+static int
>+__devlink_param_driverinit_value_set(struct devlink *devlink,
>+				     struct list_head *param_list,
>+				     u32 param_id,
>+				     union devlink_param_value init_val,
>+				     enum devlink_command cmd)
>+{
>+	struct devlink_param_item *param_item;
>+
> 	param_item = devlink_param_find_by_id(&devlink->param_list, param_id);
> 	if (!param_item)
> 		return -EINVAL;
> 
>-	if (!param_item->driverinit_value_valid ||
>-	    !devlink_param_cmode_is_supported(param_item->param,
>+	if (!devlink_param_cmode_is_supported(param_item->param,
> 					      DEVLINK_PARAM_CMODE_DRIVERINIT))
> 		return -EOPNOTSUPP;
> 
> 	if (param_item->param->type == DEVLINK_PARAM_TYPE_STRING)
>-		strcpy(init_val->vstr, param_item->driverinit_value.vstr);
>+		strcpy(param_item->driverinit_value.vstr, init_val.vstr);
> 	else
>-		*init_val = param_item->driverinit_value;
>+		param_item->driverinit_value = init_val;
>+	param_item->driverinit_value_valid = true;
>+	devlink_param_notify(devlink, param_item, cmd);
> 
> 	return 0;

Git does not understand what you do and it screwed thing up so the
review is not easy. I suggest to split this somehow, perhaps get and set
part separatelly.

Thanks.


> }
>-EXPORT_SYMBOL_GPL(devlink_param_driverinit_value_get);
> 

[...]

[PATCH net-next RFC 5/7] devlink: Add devlink notifications support for port params

[Vasundhara Volam]

Add notification call for devlink port param set, register and unregister
functions.
Add devlink_port_param_value_changed() function to enable the driver notify
devlink on value change. Driver should use this function after value was
changed on any configuration mode part to driverinit.

Cc: Jiri Pirko <jiri@mellanox.com>
Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
---
 include/net/devlink.h        |  9 ++++
 include/uapi/linux/devlink.h |  2 +
 net/core/devlink.c           | 97 ++++++++++++++++++++++++++++++++------------
 3 files changed, 83 insertions(+), 25 deletions(-)

diff --git a/include/net/devlink.h b/include/net/devlink.h
index c22b195..abb5b3a 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -591,6 +591,8 @@ void devlink_port_params_unregister(struct devlink_port *devlink_port,
 int devlink_port_param_driverinit_value_set(struct devlink_port *devlink_port,
 					    u32 param_id,
 					    union devlink_param_value init_val);
+void devlink_port_param_value_changed(struct devlink_port *devlink_port,
+				      u32 param_id);
 struct devlink_region *devlink_region_create(struct devlink *devlink,
 					     const char *region_name,
 					     u32 region_max_snapshots,
@@ -864,6 +866,13 @@ static inline bool devlink_dpipe_table_counter_enabled(struct devlink *devlink,
 	return -EOPNOTSUPP;
 }
 
+static inline void
+devlink_port_param_value_changed(struct devlink_port *devlink_port,
+				 u32 param_id)
+{
+	return -EOPNOTSUPP;
+}
+
 static inline struct devlink_region *
 devlink_region_create(struct devlink *devlink,
 		      const char *region_name,
diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
index 8f3c5dd..a024b25 100644
--- a/include/uapi/linux/devlink.h
+++ b/include/uapi/linux/devlink.h
@@ -85,6 +85,8 @@ enum devlink_command {
 
 	DEVLINK_CMD_PORT_PARAM_GET,	/* can dump */
 	DEVLINK_CMD_PORT_PARAM_SET,
+	DEVLINK_CMD_PORT_PARAM_NEW,
+	DEVLINK_CMD_PORT_PARAM_DEL,
 
 	DEVLINK_CMD_REGION_GET,
 	DEVLINK_CMD_REGION_SET,
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 4f0052d..7598da1 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -2882,7 +2882,9 @@ static int devlink_nl_param_fill(struct sk_buff *msg, struct devlink *devlink,
 	if (devlink_nl_put_handle(msg, devlink))
 		goto genlmsg_cancel;
 
-	if (cmd == DEVLINK_CMD_PORT_PARAM_GET)
+	if (cmd == DEVLINK_CMD_PORT_PARAM_GET ||
+	    cmd == DEVLINK_CMD_PORT_PARAM_NEW ||
+	    cmd == DEVLINK_CMD_PORT_PARAM_DEL)
 		if (nla_put_u32(msg, DEVLINK_ATTR_PORT_INDEX, port_index))
 			goto genlmsg_cancel;
 
@@ -2928,18 +2930,22 @@ static int devlink_nl_param_fill(struct sk_buff *msg, struct devlink *devlink,
 }
 
 static void devlink_param_notify(struct devlink *devlink,
+				 unsigned int port_index,
 				 struct devlink_param_item *param_item,
 				 enum devlink_command cmd)
 {
 	struct sk_buff *msg;
 	int err;
 
-	WARN_ON(cmd != DEVLINK_CMD_PARAM_NEW && cmd != DEVLINK_CMD_PARAM_DEL);
+	WARN_ON(cmd != DEVLINK_CMD_PARAM_NEW && cmd != DEVLINK_CMD_PARAM_DEL &&
+		cmd != DEVLINK_CMD_PORT_PARAM_NEW &&
+		cmd != DEVLINK_CMD_PORT_PARAM_DEL);
 
 	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
 	if (!msg)
 		return;
-	err = devlink_nl_param_fill(msg, devlink, 0, param_item, cmd, 0, 0, 0);
+	err = devlink_nl_param_fill(msg, devlink, port_index, param_item, cmd,
+				    0, 0, 0);
 	if (err) {
 		nlmsg_free(msg);
 		return;
@@ -3097,6 +3103,7 @@ static int devlink_nl_cmd_param_get_doit(struct sk_buff *skb,
 }
 
 static int __devlink_nl_cmd_param_set_doit(struct devlink *devlink,
+					   unsigned int port_index,
 					   struct list_head *param_list,
 					   struct genl_info *info,
 					   enum devlink_command cmd)
@@ -3149,7 +3156,7 @@ static int __devlink_nl_cmd_param_set_doit(struct devlink *devlink,
 			return err;
 	}
 
-	devlink_param_notify(devlink, param_item, cmd);
+	devlink_param_notify(devlink, port_index, param_item, cmd);
 	return 0;
 }
 
@@ -3158,13 +3165,15 @@ static int devlink_nl_cmd_param_set_doit(struct sk_buff *skb,
 {
 	struct devlink *devlink = info->user_ptr[0];
 
-	return __devlink_nl_cmd_param_set_doit(devlink, &devlink->param_list,
+	return __devlink_nl_cmd_param_set_doit(devlink, 0, &devlink->param_list,
 					       info, DEVLINK_CMD_PARAM_NEW);
 }
 
 static int devlink_param_register_one(struct devlink *devlink,
+				      unsigned int port_index,
 				      struct list_head *param_list,
-				      const struct devlink_param *param)
+				      const struct devlink_param *param,
+				      enum devlink_command cmd)
 {
 	struct devlink_param_item *param_item;
 
@@ -3182,19 +3191,21 @@ static int devlink_param_register_one(struct devlink *devlink,
 	param_item->param = param;
 
 	list_add_tail(&param_item->list, param_list);
-	devlink_param_notify(devlink, param_item, DEVLINK_CMD_PARAM_NEW);
+	devlink_param_notify(devlink, port_index, param_item, cmd);
 	return 0;
 }
 
 static void devlink_param_unregister_one(struct devlink *devlink,
+					 unsigned int port_index,
 					 struct list_head *param_list,
-					 const struct devlink_param *param)
+					 const struct devlink_param *param,
+					 enum devlink_command cmd)
 {
 	struct devlink_param_item *param_item;
 
 	param_item = devlink_param_find_by_name(param_list, param->name);
 	WARN_ON(!param_item);
-	devlink_param_notify(devlink, param_item, DEVLINK_CMD_PARAM_DEL);
+	devlink_param_notify(devlink, port_index, param_item, cmd);
 	list_del(&param_item->list);
 	kfree(param_item);
 }
@@ -3309,8 +3320,9 @@ static int devlink_nl_cmd_port_param_set_doit(struct sk_buff *skb,
 	struct devlink_port *devlink_port = info->user_ptr[0];
 
 	return __devlink_nl_cmd_param_set_doit(devlink_port->devlink,
-					       &devlink_port->param_list,
-					       info, 0);
+					       devlink_port->index,
+					       &devlink_port->param_list, info,
+					       DEVLINK_CMD_PORT_PARAM_NEW);
 }
 
 static int devlink_nl_region_snapshot_id_put(struct sk_buff *msg,
@@ -4648,8 +4660,9 @@ int devlink_params_register(struct devlink *devlink,
 			if (err)
 				goto rollback;
 		}
-		err = devlink_param_register_one(devlink, &devlink->param_list,
-						 param);
+		err = devlink_param_register_one(devlink, 0,
+						 &devlink->param_list, param,
+						 DEVLINK_CMD_PARAM_NEW);
 		if (err)
 			goto rollback;
 	}
@@ -4661,8 +4674,8 @@ int devlink_params_register(struct devlink *devlink,
 	if (!i)
 		goto unlock;
 	for (param--; i > 0; i--, param--)
-		devlink_param_unregister_one(devlink, &devlink->param_list,
-					     param);
+		devlink_param_unregister_one(devlink, 0, &devlink->param_list,
+					     param, DEVLINK_CMD_PARAM_DEL);
 unlock:
 	mutex_unlock(&devlink->lock);
 	return err;
@@ -4684,8 +4697,8 @@ void devlink_params_unregister(struct devlink *devlink,
 
 	mutex_lock(&devlink->lock);
 	for (i = 0; i < params_count; i++, param++)
-		devlink_param_unregister_one(devlink, &devlink->param_list,
-					     param);
+		devlink_param_unregister_one(devlink, 0, &devlink->param_list,
+					     param, DEVLINK_CMD_PARAM_DEL);
 	mutex_unlock(&devlink->lock);
 }
 EXPORT_SYMBOL_GPL(devlink_params_unregister);
@@ -4737,6 +4750,7 @@ int devlink_param_driverinit_value_get(struct devlink *devlink, u32 param_id,
 
 static int
 __devlink_param_driverinit_value_set(struct devlink *devlink,
+				     unsigned int port_index,
 				     struct list_head *param_list,
 				     u32 param_id,
 				     union devlink_param_value init_val,
@@ -4757,7 +4771,7 @@ int devlink_param_driverinit_value_get(struct devlink *devlink, u32 param_id,
 	else
 		param_item->driverinit_value = init_val;
 	param_item->driverinit_value_valid = true;
-	devlink_param_notify(devlink, param_item, cmd);
+	devlink_param_notify(devlink, port_index, param_item, cmd);
 
 	return 0;
 }
@@ -4777,7 +4791,7 @@ int devlink_param_driverinit_value_get(struct devlink *devlink, u32 param_id,
 int devlink_param_driverinit_value_set(struct devlink *devlink, u32 param_id,
 				       union devlink_param_value init_val)
 {
-	return __devlink_param_driverinit_value_set(devlink,
+	return __devlink_param_driverinit_value_set(devlink, 0,
 						    &devlink->param_list,
 						    param_id, init_val,
 						    DEVLINK_CMD_PARAM_NEW);
@@ -4804,7 +4818,7 @@ void devlink_param_value_changed(struct devlink *devlink, u32 param_id)
 	param_item = devlink_param_find_by_id(&devlink->param_list, param_id);
 	WARN_ON(!param_item);
 
-	devlink_param_notify(devlink, param_item, DEVLINK_CMD_PARAM_NEW);
+	devlink_param_notify(devlink, 0, param_item, DEVLINK_CMD_PARAM_NEW);
 }
 EXPORT_SYMBOL_GPL(devlink_param_value_changed);
 
@@ -4859,8 +4873,10 @@ int devlink_port_params_register(struct devlink_port *devlink_port,
 				goto rollback;
 		}
 		err = devlink_param_register_one(devlink_port->devlink,
+						 devlink_port->index,
 						 &devlink_port->param_list,
-						 param);
+						 param,
+						 DEVLINK_CMD_PORT_PARAM_NEW);
 		if (err)
 			goto rollback;
 	}
@@ -4873,8 +4889,9 @@ int devlink_port_params_register(struct devlink_port *devlink_port,
 		goto unlock;
 	for (param--; i > 0; i--, param--)
 		devlink_param_unregister_one(devlink_port->devlink,
-					     &devlink_port->param_list,
-					     param);
+					     devlink_port->index,
+					     &devlink_port->param_list, param,
+					     DEVLINK_CMD_PORT_PARAM_DEL);
 unlock:
 	mutex_unlock(&devlink->lock);
 	return err;
@@ -4900,8 +4917,9 @@ void devlink_port_params_unregister(struct devlink_port *devlink_port,
 	mutex_lock(&devlink->lock);
 	for (i = 0; i < params_count; i++, param++)
 		devlink_param_unregister_one(devlink_port->devlink,
+					     devlink_port->index,
 					     &devlink_port->param_list,
-					     param);
+					     param, DEVLINK_CMD_PORT_PARAM_DEL);
 	mutex_unlock(&devlink->lock);
 }
 EXPORT_SYMBOL_GPL(devlink_port_params_unregister);
@@ -4948,12 +4966,41 @@ int devlink_port_param_driverinit_value_set(struct devlink_port *devlink_port,
 					    union devlink_param_value init_val)
 {
 	return __devlink_param_driverinit_value_set(devlink_port->devlink,
+						    devlink_port->index,
 						    &devlink_port->param_list,
-						    param_id, init_val, 0);
+						    param_id, init_val,
+						    DEVLINK_CMD_PORT_PARAM_NEW);
 }
 EXPORT_SYMBOL_GPL(devlink_port_param_driverinit_value_set);
 
 /**
+ *	devlink_port_param_value_changed - notify devlink on a parameter's value
+ *					 change. Should be called by the driver
+ *					 right after the change.
+ *
+ *	@devlink_port: devlink_port
+ *	@param_id: parameter ID
+ *
+ *	This function should be used by the driver to notify devlink on value
+ *	change, excluding driverinit configuration mode.
+ *	For driverinit configuration mode driver should use the function
+ *	devlink_port_param_driverinit_value_set() instead.
+ */
+void devlink_port_param_value_changed(struct devlink_port *devlink_port,
+				      u32 param_id)
+{
+	struct devlink_param_item *param_item;
+
+	param_item = devlink_param_find_by_id(&devlink_port->param_list,
+					      param_id);
+	WARN_ON(!param_item);
+
+	devlink_param_notify(devlink_port->devlink, devlink_port->index,
+			     param_item, DEVLINK_CMD_PORT_PARAM_NEW);
+}
+EXPORT_SYMBOL_GPL(devlink_port_param_value_changed);
+
+/**
  *	devlink_region_create - create a new address region
  *
  *	@devlink: devlink
-- 
1.8.3.1

Re: [PATCH net-next RFC 5/7] devlink: Add devlink notifications support for port params

[Jiri Pirko]

Wed, Dec 05, 2018 at 06:56:58AM CET, vasundhara-v.volam@broadcom.com wrote:
>Add notification call for devlink port param set, register and unregister
>functions.
>Add devlink_port_param_value_changed() function to enable the driver notify
>devlink on value change. Driver should use this function after value was
>changed on any configuration mode part to driverinit.
>
>Cc: Jiri Pirko <jiri@mellanox.com>
>Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>

looks fine

[PATCH net-next RFC 6/7] devlink: Add a boolean generic port parameter

[Vasundhara Volam]

wake-on-lan - Enables Wake on Lan for this port when magic packet
is received with this port's MAC address using ACPI pattern.
If enabled, the controller asserts a wake pin upon reception of
WoL packet.  ACPI (Advanced Configuration and Power Interface)
is an industry specification for the efficient handling of power
consumption in desktop and mobile computers.

Cc: Jiri Pirko <jiri@mellanox.com>
Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
---
 include/net/devlink.h | 17 +++++++++++++++++
 net/core/devlink.c    |  8 +++++++-
 2 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/include/net/devlink.h b/include/net/devlink.h
index abb5b3a..7409076 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -421,12 +421,29 @@ enum devlink_param_generic_id {
 }
 
 enum devlink_port_param_generic_id {
+	DEVLINK_PORT_PARAM_GENERIC_ID_WOL,
+
 	/* add new param generic ids above here */
 	__DEVLINK_PORT_PARAM_GENERIC_ID_MAX,
 	DEVLINK_PORT_PARAM_GENERIC_ID_MAX =
 	       __DEVLINK_PORT_PARAM_GENERIC_ID_MAX - 1,
 };
 
+#define DEVLINK_PORT_PARAM_GENERIC_WOL_NAME "wake-on-lan"
+#define DEVLINK_PORT_PARAM_GENERIC_WOL_TYPE DEVLINK_PARAM_TYPE_BOOL
+
+#define DEVLINK_PORT_PARAM_GENERIC(_id, _cmodes, _get, _set, _validate)	\
+{									\
+	.id = DEVLINK_PORT_PARAM_GENERIC_ID_##_id,			\
+	.name = DEVLINK_PORT_PARAM_GENERIC_##_id##_NAME,		\
+	.type = DEVLINK_PORT_PARAM_GENERIC_##_id##_TYPE,		\
+	.generic = true,						\
+	.supported_cmodes = _cmodes,					\
+	.get = _get,							\
+	.set = _set,							\
+	.validate = _validate,						\
+}
+
 struct devlink_region;
 
 typedef void devlink_snapshot_data_dest_t(const void *data);
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 7598da1..24c2b9e 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -3210,7 +3210,13 @@ static void devlink_param_unregister_one(struct devlink *devlink,
 	kfree(param_item);
 }
 
-static const struct devlink_param devlink_port_param_generic[] = {};
+static const struct devlink_param devlink_port_param_generic[] = {
+	{
+		.id = DEVLINK_PORT_PARAM_GENERIC_ID_WOL,
+		.name = DEVLINK_PORT_PARAM_GENERIC_WOL_NAME,
+		.type = DEVLINK_PORT_PARAM_GENERIC_WOL_TYPE,
+	},
+};
 
 static int devlink_port_param_generic_verify(const struct devlink_param *param)
 {
-- 
1.8.3.1

Re: [PATCH net-next RFC 6/7] devlink: Add a boolean generic port parameter

[Jiri Pirko]

Wed, Dec 05, 2018 at 06:56:59AM CET, vasundhara-v.volam@broadcom.com wrote:
>wake-on-lan - Enables Wake on Lan for this port when magic packet
>is received with this port's MAC address using ACPI pattern.
>If enabled, the controller asserts a wake pin upon reception of
>WoL packet.  ACPI (Advanced Configuration and Power Interface)
>is an industry specification for the efficient handling of power
>consumption in desktop and mobile computers.
>
>Cc: Jiri Pirko <jiri@mellanox.com>
>Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
>---
> include/net/devlink.h | 17 +++++++++++++++++
> net/core/devlink.c    |  8 +++++++-
> 2 files changed, 24 insertions(+), 1 deletion(-)
>
>diff --git a/include/net/devlink.h b/include/net/devlink.h
>index abb5b3a..7409076 100644
>--- a/include/net/devlink.h
>+++ b/include/net/devlink.h
>@@ -421,12 +421,29 @@ enum devlink_param_generic_id {
> }
> 
> enum devlink_port_param_generic_id {
>+	DEVLINK_PORT_PARAM_GENERIC_ID_WOL,
>+
> 	/* add new param generic ids above here */
> 	__DEVLINK_PORT_PARAM_GENERIC_ID_MAX,
> 	DEVLINK_PORT_PARAM_GENERIC_ID_MAX =
> 	       __DEVLINK_PORT_PARAM_GENERIC_ID_MAX - 1,
> };
> 
>+#define DEVLINK_PORT_PARAM_GENERIC_WOL_NAME "wake-on-lan"
>+#define DEVLINK_PORT_PARAM_GENERIC_WOL_TYPE DEVLINK_PARAM_TYPE_BOOL
>+
>+#define DEVLINK_PORT_PARAM_GENERIC(_id, _cmodes, _get, _set, _validate)	\
>+{									\
>+	.id = DEVLINK_PORT_PARAM_GENERIC_ID_##_id,			\
>+	.name = DEVLINK_PORT_PARAM_GENERIC_##_id##_NAME,		\
>+	.type = DEVLINK_PORT_PARAM_GENERIC_##_id##_TYPE,		\
>+	.generic = true,						\
>+	.supported_cmodes = _cmodes,					\
>+	.get = _get,							\
>+	.set = _set,							\
>+	.validate = _validate,						\
>+}

Just use DEVLINK_PARAM_GENERIC


>+
> struct devlink_region;
> 
> typedef void devlink_snapshot_data_dest_t(const void *data);
>diff --git a/net/core/devlink.c b/net/core/devlink.c
>index 7598da1..24c2b9e 100644
>--- a/net/core/devlink.c
>+++ b/net/core/devlink.c
>@@ -3210,7 +3210,13 @@ static void devlink_param_unregister_one(struct devlink *devlink,
> 	kfree(param_item);
> }
> 
>-static const struct devlink_param devlink_port_param_generic[] = {};
>+static const struct devlink_param devlink_port_param_generic[] = {
>+	{
>+		.id = DEVLINK_PORT_PARAM_GENERIC_ID_WOL,
>+		.name = DEVLINK_PORT_PARAM_GENERIC_WOL_NAME,
>+		.type = DEVLINK_PORT_PARAM_GENERIC_WOL_TYPE,
>+	},
>+};
> 
> static int devlink_port_param_generic_verify(const struct devlink_param *param)
> {
>-- 
>1.8.3.1
>

[PATCH net-next RFC 7/7] bnxt_en: Add bnxt_en initial port params table and register it

[Vasundhara Volam]

Register devlink_port with devlink and create initial port params
table for bnxt_en. The table consists of a generic parameter:

wake-on-lan: Enables Wake on Lan for this port when magic packet
is received with this port's MAC address using ACPI pattern.
If enabled, the controller asserts a wake pin upon reception of
WoL packet.  ACPI (Advanced Configuration and Power Interface) is
an industry specification for the efficient handling of power
consumption in desktop and mobile computers.

Cc: Michael Chan <michael.chan@broadcom.com>
Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.h         |  1 +
 drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 35 +++++++++++++++++++++++
 drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h |  1 +
 3 files changed, 37 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
index 9e99d4a..0ba62b3 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
@@ -1585,6 +1585,7 @@ struct bnxt {
 
 	/* devlink interface and vf-rep structs */
 	struct devlink		*dl;
+	struct devlink_port	dl_port;
 	enum devlink_eswitch_mode eswitch_mode;
 	struct bnxt_vf_rep	**vf_reps; /* array of vf-rep ptrs */
 	u16			*cfa_code_map; /* cfa_code -> vf_idx map */
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
index 140dbd6..a2930d5 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
@@ -26,6 +26,10 @@ enum bnxt_dl_param_id {
 	BNXT_DEVLINK_PARAM_ID_GRE_VER_CHECK,
 };
 
+enum bnxt_dl_port_param_id {
+	BNXT_DEVLINK_PORT_PARAM_ID_BASE = DEVLINK_PORT_PARAM_GENERIC_ID_MAX,
+};
+
 static const struct bnxt_dl_nvm_param nvm_params[] = {
 	{DEVLINK_PARAM_GENERIC_ID_ENABLE_SRIOV, NVM_OFF_ENABLE_SRIOV,
 	 BNXT_NVM_SHARED_CFG, 1},
@@ -37,6 +41,8 @@ enum bnxt_dl_param_id {
 	 NVM_OFF_MSIX_VEC_PER_PF_MIN, BNXT_NVM_SHARED_CFG, 7},
 	{BNXT_DEVLINK_PARAM_ID_GRE_VER_CHECK, NVM_OFF_DIS_GRE_VER_CHECK,
 	 BNXT_NVM_SHARED_CFG, 1},
+
+	{DEVLINK_PORT_PARAM_GENERIC_ID_WOL, NVM_OFF_WOL, BNXT_NVM_PORT_CFG, 1},
 };
 
 static int bnxt_hwrm_nvm_req(struct bnxt *bp, u32 param_id, void *msg,
@@ -188,6 +194,12 @@ static int bnxt_dl_msix_validate(struct devlink *dl, u32 id,
 			     NULL),
 };
 
+static const struct devlink_param bnxt_dl_port_params[] = {
+	DEVLINK_PORT_PARAM_GENERIC(WOL, BIT(DEVLINK_PARAM_CMODE_PERMANENT),
+				   bnxt_dl_nvm_param_get, bnxt_dl_nvm_param_set,
+				   NULL),
+};
+
 int bnxt_dl_register(struct bnxt *bp)
 {
 	struct devlink *dl;
@@ -225,8 +237,28 @@ int bnxt_dl_register(struct bnxt *bp)
 		goto err_dl_unreg;
 	}
 
+	rc = devlink_port_register(dl, &bp->dl_port, bp->pf.port_id);
+	if (rc) {
+		netdev_warn(bp->dev, "devlink_port_register failed. rc=%d",
+			    rc);
+		goto err_dl_param_unreg;
+	}
+	bp->dl_port.type = DEVLINK_PORT_TYPE_ETH;
+
+	rc = devlink_port_params_register(&bp->dl_port, bnxt_dl_port_params,
+					  ARRAY_SIZE(bnxt_dl_port_params));
+	if (rc) {
+		netdev_warn(bp->dev, "devlink_port_params_register failed. rc=%d",
+			    rc);
+		goto err_dl_port_unreg;
+	}
 	return 0;
 
+err_dl_port_unreg:
+	devlink_port_unregister(&bp->dl_port);
+err_dl_param_unreg:
+	devlink_params_unregister(dl, bnxt_dl_params,
+				  ARRAY_SIZE(bnxt_dl_params));
 err_dl_unreg:
 	devlink_unregister(dl);
 err_dl_free:
@@ -242,6 +274,9 @@ void bnxt_dl_unregister(struct bnxt *bp)
 	if (!dl)
 		return;
 
+	devlink_port_params_unregister(&bp->dl_port, bnxt_dl_port_params,
+				       ARRAY_SIZE(bnxt_dl_port_params));
+	devlink_port_unregister(&bp->dl_port);
 	devlink_params_unregister(dl, bnxt_dl_params,
 				  ARRAY_SIZE(bnxt_dl_params));
 	devlink_unregister(dl);
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h
index 5b6b2c7..da065ca 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h
@@ -35,6 +35,7 @@ static inline void bnxt_link_bp_to_dl(struct bnxt *bp, struct devlink *dl)
 
 #define NVM_OFF_MSIX_VEC_PER_PF_MAX	108
 #define NVM_OFF_MSIX_VEC_PER_PF_MIN	114
+#define NVM_OFF_WOL			152
 #define NVM_OFF_IGNORE_ARI		164
 #define NVM_OFF_DIS_GRE_VER_CHECK	171
 #define NVM_OFF_ENABLE_SRIOV		401
-- 
1.8.3.1

Re: [PATCH net-next RFC 7/7] bnxt_en: Add bnxt_en initial port params table and register it

[Jiri Pirko]

Wed, Dec 05, 2018 at 06:57:00AM CET, vasundhara-v.volam@broadcom.com wrote:
>Register devlink_port with devlink and create initial port params
>table for bnxt_en. The table consists of a generic parameter:
>
>wake-on-lan: Enables Wake on Lan for this port when magic packet
>is received with this port's MAC address using ACPI pattern.
>If enabled, the controller asserts a wake pin upon reception of
>WoL packet.  ACPI (Advanced Configuration and Power Interface) is
>an industry specification for the efficient handling of power
>consumption in desktop and mobile computers.
>
>Cc: Michael Chan <michael.chan@broadcom.com>
>Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
>---
> drivers/net/ethernet/broadcom/bnxt/bnxt.h         |  1 +
> drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 35 +++++++++++++++++++++++
> drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h |  1 +
> 3 files changed, 37 insertions(+)
>
>diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
>index 9e99d4a..0ba62b3 100644
>--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h
>+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
>@@ -1585,6 +1585,7 @@ struct bnxt {
> 
> 	/* devlink interface and vf-rep structs */
> 	struct devlink		*dl;
>+	struct devlink_port	dl_port;
> 	enum devlink_eswitch_mode eswitch_mode;
> 	struct bnxt_vf_rep	**vf_reps; /* array of vf-rep ptrs */
> 	u16			*cfa_code_map; /* cfa_code -> vf_idx map */
>diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
>index 140dbd6..a2930d5 100644
>--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
>+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
>@@ -26,6 +26,10 @@ enum bnxt_dl_param_id {
> 	BNXT_DEVLINK_PARAM_ID_GRE_VER_CHECK,
> };
> 
>+enum bnxt_dl_port_param_id {
>+	BNXT_DEVLINK_PORT_PARAM_ID_BASE = DEVLINK_PORT_PARAM_GENERIC_ID_MAX,
>+};
>+
> static const struct bnxt_dl_nvm_param nvm_params[] = {
> 	{DEVLINK_PARAM_GENERIC_ID_ENABLE_SRIOV, NVM_OFF_ENABLE_SRIOV,
> 	 BNXT_NVM_SHARED_CFG, 1},
>@@ -37,6 +41,8 @@ enum bnxt_dl_param_id {
> 	 NVM_OFF_MSIX_VEC_PER_PF_MIN, BNXT_NVM_SHARED_CFG, 7},
> 	{BNXT_DEVLINK_PARAM_ID_GRE_VER_CHECK, NVM_OFF_DIS_GRE_VER_CHECK,
> 	 BNXT_NVM_SHARED_CFG, 1},
>+
>+	{DEVLINK_PORT_PARAM_GENERIC_ID_WOL, NVM_OFF_WOL, BNXT_NVM_PORT_CFG, 1},
> };
> 
> static int bnxt_hwrm_nvm_req(struct bnxt *bp, u32 param_id, void *msg,
>@@ -188,6 +194,12 @@ static int bnxt_dl_msix_validate(struct devlink *dl, u32 id,
> 			     NULL),
> };
> 
>+static const struct devlink_param bnxt_dl_port_params[] = {
>+	DEVLINK_PORT_PARAM_GENERIC(WOL, BIT(DEVLINK_PARAM_CMODE_PERMANENT),
>+				   bnxt_dl_nvm_param_get, bnxt_dl_nvm_param_set,
>+				   NULL),
>+};
>+
> int bnxt_dl_register(struct bnxt *bp)
> {
> 	struct devlink *dl;
>@@ -225,8 +237,28 @@ int bnxt_dl_register(struct bnxt *bp)
> 		goto err_dl_unreg;
> 	}
> 
>+	rc = devlink_port_register(dl, &bp->dl_port, bp->pf.port_id);
>+	if (rc) {
>+		netdev_warn(bp->dev, "devlink_port_register failed. rc=%d",
>+			    rc);
>+		goto err_dl_param_unreg;
>+	}
>+	bp->dl_port.type = DEVLINK_PORT_TYPE_ETH;

Please use devlink_port_type_set()


>+
>+	rc = devlink_port_params_register(&bp->dl_port, bnxt_dl_port_params,
>+					  ARRAY_SIZE(bnxt_dl_port_params));
>+	if (rc) {
>+		netdev_warn(bp->dev, "devlink_port_params_register failed. rc=%d",
>+			    rc);
>+		goto err_dl_port_unreg;
>+	}
> 	return 0;
> 
>+err_dl_port_unreg:
>+	devlink_port_unregister(&bp->dl_port);
>+err_dl_param_unreg:
>+	devlink_params_unregister(dl, bnxt_dl_params,
>+				  ARRAY_SIZE(bnxt_dl_params));
> err_dl_unreg:
> 	devlink_unregister(dl);
> err_dl_free:
>@@ -242,6 +274,9 @@ void bnxt_dl_unregister(struct bnxt *bp)
> 	if (!dl)
> 		return;
> 
>+	devlink_port_params_unregister(&bp->dl_port, bnxt_dl_port_params,
>+				       ARRAY_SIZE(bnxt_dl_port_params));
>+	devlink_port_unregister(&bp->dl_port);
> 	devlink_params_unregister(dl, bnxt_dl_params,
> 				  ARRAY_SIZE(bnxt_dl_params));
> 	devlink_unregister(dl);
>diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h
>index 5b6b2c7..da065ca 100644
>--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h
>+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h
>@@ -35,6 +35,7 @@ static inline void bnxt_link_bp_to_dl(struct bnxt *bp, struct devlink *dl)
> 
> #define NVM_OFF_MSIX_VEC_PER_PF_MAX	108
> #define NVM_OFF_MSIX_VEC_PER_PF_MIN	114
>+#define NVM_OFF_WOL			152
> #define NVM_OFF_IGNORE_ARI		164
> #define NVM_OFF_DIS_GRE_VER_CHECK	171
> #define NVM_OFF_ENABLE_SRIOV		401
>-- 
>1.8.3.1
>

Re: [PATCH net-next RFC 7/7] bnxt_en: Add bnxt_en initial port params table and register it

[Jakub Kicinski]

On Wed,  5 Dec 2018 11:27:00 +0530, Vasundhara Volam wrote:
> Register devlink_port with devlink and create initial port params
> table for bnxt_en. The table consists of a generic parameter:
> 
> wake-on-lan: Enables Wake on Lan for this port when magic packet
> is received with this port's MAC address using ACPI pattern.
> If enabled, the controller asserts a wake pin upon reception of
> WoL packet.  ACPI (Advanced Configuration and Power Interface) is
> an industry specification for the efficient handling of power
> consumption in desktop and mobile computers.
> 
> Cc: Michael Chan <michael.chan@broadcom.com>
> Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>

Why do we need a WoL as a devlink parameter (rather than ethtool -s)?

Re: [PATCH net-next RFC 7/7] bnxt_en: Add bnxt_en initial port params table and register it

[Michael Chan]

On Wed, Dec 5, 2018 at 3:33 PM Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> On Wed,  5 Dec 2018 11:27:00 +0530, Vasundhara Volam wrote:
> > Register devlink_port with devlink and create initial port params
> > table for bnxt_en. The table consists of a generic parameter:
> >
> > wake-on-lan: Enables Wake on Lan for this port when magic packet
> > is received with this port's MAC address using ACPI pattern.
> > If enabled, the controller asserts a wake pin upon reception of
> > WoL packet.  ACPI (Advanced Configuration and Power Interface) is
> > an industry specification for the efficient handling of power
> > consumption in desktop and mobile computers.
> >
> > Cc: Michael Chan <michael.chan@broadcom.com>
> > Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
>
> Why do we need a WoL as a devlink parameter (rather than ethtool -s)?

I believe ethtool -s for WoL is a non-persistent setting, meaning that
if you power cycle the system, the WoL setting will go back to
default.

devlink on the other hand is a permanent setting.  ethtool should
initially report the default WoL setting and it can then be changed
(in a non permanent way) using ethtool -s.

Re: [PATCH net-next RFC 7/7] bnxt_en: Add bnxt_en initial port params table and register it

[Jakub Kicinski]

On Wed, 5 Dec 2018 16:01:08 -0800, Michael Chan wrote:
> On Wed, Dec 5, 2018 at 3:33 PM Jakub Kicinski
> <jakub.kicinski@netronome.com> wrote:
> >
> > On Wed,  5 Dec 2018 11:27:00 +0530, Vasundhara Volam wrote:  
> > > Register devlink_port with devlink and create initial port params
> > > table for bnxt_en. The table consists of a generic parameter:
> > >
> > > wake-on-lan: Enables Wake on Lan for this port when magic packet
> > > is received with this port's MAC address using ACPI pattern.
> > > If enabled, the controller asserts a wake pin upon reception of
> > > WoL packet.  ACPI (Advanced Configuration and Power Interface) is
> > > an industry specification for the efficient handling of power
> > > consumption in desktop and mobile computers.
> > >
> > > Cc: Michael Chan <michael.chan@broadcom.com>
> > > Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>  
> >
> > Why do we need a WoL as a devlink parameter (rather than ethtool -s)?  
> 
> I believe ethtool -s for WoL is a non-persistent setting, meaning that
> if you power cycle the system, the WoL setting will go back to
> default.
> 
> devlink on the other hand is a permanent setting.  ethtool should
> initially report the default WoL setting and it can then be changed
> (in a non permanent way) using ethtool -s.

All network configuration settings in Linux are non-persistent AFAIK.
That's why network configuration daemons exist:

https://wiki.debian.org/WakeOnLan

Perhaps the objective to move more of the network configuration into the
firmware?  That'd be a bleak scenario, so probably not..

My understanding was the persistent devlink settings are for things
which have to be set at device init time.  Like say PCI endpoint
configuration.  FW loading configuration.

Besides, the parameter you add is just true/false, when ethtool has
multiple options.

It feels to me like we moved from ioctls to Netlink, and now even
before ethtool was converted to Netlink we may move to unstructured
strings.  That's not a step forward, if you ask me.

Re: [PATCH net-next RFC 7/7] bnxt_en: Add bnxt_en initial port params table and register it

[Michael Chan]

On Wed, Dec 5, 2018 at 4:42 PM Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> On Wed, 5 Dec 2018 16:01:08 -0800, Michael Chan wrote:
> > On Wed, Dec 5, 2018 at 3:33 PM Jakub Kicinski
> > <jakub.kicinski@netronome.com> wrote:
> > >
> > > On Wed,  5 Dec 2018 11:27:00 +0530, Vasundhara Volam wrote:
> > > > Register devlink_port with devlink and create initial port params
> > > > table for bnxt_en. The table consists of a generic parameter:
> > > >
> > > > wake-on-lan: Enables Wake on Lan for this port when magic packet
> > > > is received with this port's MAC address using ACPI pattern.
> > > > If enabled, the controller asserts a wake pin upon reception of
> > > > WoL packet.  ACPI (Advanced Configuration and Power Interface) is
> > > > an industry specification for the efficient handling of power
> > > > consumption in desktop and mobile computers.
> > > >
> > > > Cc: Michael Chan <michael.chan@broadcom.com>
> > > > Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
> > >
> > > Why do we need a WoL as a devlink parameter (rather than ethtool -s)?
> >
> > I believe ethtool -s for WoL is a non-persistent setting, meaning that
> > if you power cycle the system, the WoL setting will go back to
> > default.
> >
> > devlink on the other hand is a permanent setting.  ethtool should
> > initially report the default WoL setting and it can then be changed
> > (in a non permanent way) using ethtool -s.
>
> All network configuration settings in Linux are non-persistent AFAIK.
> That's why network configuration daemons exist:
>
> https://wiki.debian.org/WakeOnLan
>
> Perhaps the objective to move more of the network configuration into the
> firmware?  That'd be a bleak scenario, so probably not..
>
> My understanding was the persistent devlink settings are for things
> which have to be set at device init time.  Like say PCI endpoint
> configuration.  FW loading configuration.
>
> Besides, the parameter you add is just true/false, when ethtool has
> multiple options.
>
> It feels to me like we moved from ioctls to Netlink, and now even
> before ethtool was converted to Netlink we may move to unstructured
> strings.  That's not a step forward, if you ask me.

We do have a parameter in NVRAM that controls default WoL.  I think
this is to expose that parameter so it can be set one way or the
other. There are scenarios where Linux has not booted yet (and so
there is no opportunity to run ethtool -s or any daemons yet) and this
parameter will control whether the machine will wake up or not.

Re: [PATCH net-next RFC 7/7] bnxt_en: Add bnxt_en initial port params table and register it

[Jakub Kicinski]

On Wed, 5 Dec 2018 17:18:52 -0800, Michael Chan wrote:
> On Wed, Dec 5, 2018 at 4:42 PM Jakub Kicinski wrote:
> > On Wed, 5 Dec 2018 16:01:08 -0800, Michael Chan wrote:  
> > > On Wed, Dec 5, 2018 at 3:33 PM Jakub Kicinski wrote:
> > > > On Wed,  5 Dec 2018 11:27:00 +0530, Vasundhara Volam wrote:  
> > > > > Register devlink_port with devlink and create initial port params
> > > > > table for bnxt_en. The table consists of a generic parameter:
> > > > >
> > > > > wake-on-lan: Enables Wake on Lan for this port when magic packet
> > > > > is received with this port's MAC address using ACPI pattern.
> > > > > If enabled, the controller asserts a wake pin upon reception of
> > > > > WoL packet.  ACPI (Advanced Configuration and Power Interface) is
> > > > > an industry specification for the efficient handling of power
> > > > > consumption in desktop and mobile computers.
> > > > >
> > > > > Cc: Michael Chan <michael.chan@broadcom.com>
> > > > > Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>  
> > > >
> > > > Why do we need a WoL as a devlink parameter (rather than ethtool -s)?  
> > >
> > > I believe ethtool -s for WoL is a non-persistent setting, meaning that
> > > if you power cycle the system, the WoL setting will go back to
> > > default.
> > >
> > > devlink on the other hand is a permanent setting.  ethtool should
> > > initially report the default WoL setting and it can then be changed
> > > (in a non permanent way) using ethtool -s.  
> >
> > All network configuration settings in Linux are non-persistent AFAIK.
> > That's why network configuration daemons exist:
> >
> > https://wiki.debian.org/WakeOnLan
> >
> > Perhaps the objective to move more of the network configuration into the
> > firmware?  That'd be a bleak scenario, so probably not..
> >
> > My understanding was the persistent devlink settings are for things
> > which have to be set at device init time.  Like say PCI endpoint
> > configuration.  FW loading configuration.
> >
> > Besides, the parameter you add is just true/false, when ethtool has
> > multiple options.
> >
> > It feels to me like we moved from ioctls to Netlink, and now even
> > before ethtool was converted to Netlink we may move to unstructured
> > strings.  That's not a step forward, if you ask me.  
> 
> We do have a parameter in NVRAM that controls default WoL.  I think
> this is to expose that parameter so it can be set one way or the
> other. There are scenarios where Linux has not booted yet (and so
> there is no opportunity to run ethtool -s or any daemons yet) and this
> parameter will control whether the machine will wake up or not.

Isn't that set in BIOS/setup?  The config before any OS boots?  Because
the BMC or whatnot has to actually configure the board to power
appropriate things up.  Please clarify.

And *if* it is proven this config is more than just setting the default
IMHO the setting belongs in the ethtool API.  We can't just add devlink
params for all existing config APIs just because it has persistence.

Re: [PATCH net-next RFC 7/7] bnxt_en: Add bnxt_en initial port params table and register it

[Michael Chan]

On Wed, Dec 5, 2018 at 10:00 PM Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> On Wed, 5 Dec 2018 17:18:52 -0800, Michael Chan wrote:
> > On Wed, Dec 5, 2018 at 4:42 PM Jakub Kicinski wrote:
> > > On Wed, 5 Dec 2018 16:01:08 -0800, Michael Chan wrote:
> > > > On Wed, Dec 5, 2018 at 3:33 PM Jakub Kicinski wrote:
> > > > > On Wed,  5 Dec 2018 11:27:00 +0530, Vasundhara Volam wrote:
> > > > > > Register devlink_port with devlink and create initial port params
> > > > > > table for bnxt_en. The table consists of a generic parameter:
> > > > > >
> > > > > > wake-on-lan: Enables Wake on Lan for this port when magic packet
> > > > > > is received with this port's MAC address using ACPI pattern.
> > > > > > If enabled, the controller asserts a wake pin upon reception of
> > > > > > WoL packet.  ACPI (Advanced Configuration and Power Interface) is
> > > > > > an industry specification for the efficient handling of power
> > > > > > consumption in desktop and mobile computers.
> > > > > >
> > > > > > Cc: Michael Chan <michael.chan@broadcom.com>
> > > > > > Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
> > > > >
> > > > > Why do we need a WoL as a devlink parameter (rather than ethtool -s)?
> > > >
> > > > I believe ethtool -s for WoL is a non-persistent setting, meaning that
> > > > if you power cycle the system, the WoL setting will go back to
> > > > default.
> > > >
> > > > devlink on the other hand is a permanent setting.  ethtool should
> > > > initially report the default WoL setting and it can then be changed
> > > > (in a non permanent way) using ethtool -s.
> > >
> > > All network configuration settings in Linux are non-persistent AFAIK.
> > > That's why network configuration daemons exist:
> > >
> > > https://wiki.debian.org/WakeOnLan
> > >
> > > Perhaps the objective to move more of the network configuration into the
> > > firmware?  That'd be a bleak scenario, so probably not..
> > >
> > > My understanding was the persistent devlink settings are for things
> > > which have to be set at device init time.  Like say PCI endpoint
> > > configuration.  FW loading configuration.
> > >
> > > Besides, the parameter you add is just true/false, when ethtool has
> > > multiple options.
> > >
> > > It feels to me like we moved from ioctls to Netlink, and now even
> > > before ethtool was converted to Netlink we may move to unstructured
> > > strings.  That's not a step forward, if you ask me.
> >
> > We do have a parameter in NVRAM that controls default WoL.  I think
> > this is to expose that parameter so it can be set one way or the
> > other. There are scenarios where Linux has not booted yet (and so
> > there is no opportunity to run ethtool -s or any daemons yet) and this
> > parameter will control whether the machine will wake up or not.
>
> Isn't that set in BIOS/setup?  The config before any OS boots?  Because
> the BMC or whatnot has to actually configure the board to power
> appropriate things up.  Please clarify.

It will be in the BIOS only for a LOM, I think.  For a NIC, it should
be in the NIC's NVRAM.

>
> And *if* it is proven this config is more than just setting the default
> IMHO the setting belongs in the ethtool API.  We can't just add devlink
> params for all existing config APIs just because it has persistence.

I'm not sure I understand your point.  I believe the NIC firmware will
set up the NIC's WoL setting right after power up based on this NVRAM
parameter.  Similar to how the firmware will setup PCIe Gen2 or Gen3
right after power up, for example.  So why would this belong to
ethtool?  I understand the confusion that ethtool -s has a similar WoL
setting.  But again, that's different.  This one is the power up
setting that impacts whether a magic packet can or cannot wake up the
system right after power up (before booting up to Linux or other OS).

Re: [PATCH net-next RFC 7/7] bnxt_en: Add bnxt_en initial port params table and register it

[Jakub Kicinski]

On Wed, 5 Dec 2018 22:41:43 -0800, Michael Chan wrote:
> > > We do have a parameter in NVRAM that controls default WoL.  I think
> > > this is to expose that parameter so it can be set one way or the
> > > other. There are scenarios where Linux has not booted yet (and so
> > > there is no opportunity to run ethtool -s or any daemons yet) and this
> > > parameter will control whether the machine will wake up or not.  
> >
> > Isn't that set in BIOS/setup?  The config before any OS boots?  Because
> > the BMC or whatnot has to actually configure the board to power
> > appropriate things up.  Please clarify.  
> 
> It will be in the BIOS only for a LOM, I think.  For a NIC, it should
> be in the NIC's NVRAM.

This is all vague.  Could you please clearly state the use case.

> > And *if* it is proven this config is more than just setting the default
> > IMHO the setting belongs in the ethtool API.  We can't just add devlink
> > params for all existing config APIs just because it has persistence.  
> 
> I'm not sure I understand your point.  I believe the NIC firmware will
> set up the NIC's WoL setting right after power up based on this NVRAM
> parameter.  Similar to how the firmware will setup PCIe Gen2 or Gen3
> right after power up, for example.  

We have no PCIe config interface therefore the crutch of devlink params
was allowed there.  We *do* have an existing interface to configure WoL.

> So why would this belong to ethtool?  I understand the confusion that
> ethtool -s has a similar WoL setting.  But again, that's different.

Perhaps you're looking at this from firmware perspective?  FW NVM knob
== devlink param?

> This one is the power up setting that impacts whether a magic packet
> can or cannot wake up the system right after power up (before booting
> up to Linux or other OS).

Re: [PATCH net-next RFC 7/7] bnxt_en: Add bnxt_en initial port params table and register it

[Michael Chan]

On Wed, Dec 5, 2018 at 11:11 PM Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> On Wed, 5 Dec 2018 22:41:43 -0800, Michael Chan wrote:
> >
> > It will be in the BIOS only for a LOM, I think.  For a NIC, it should
> > be in the NIC's NVRAM.
>
> This is all vague.  Could you please clearly state the use case.
>
Well, the WoL setting's use case should be quite simple, right?  If
the card's NVRAM WoL setting is ON, when you plug the card in a slot
that has Vaux power, it will assert PME# when a magic packet is
received.  Again, the WoL setting in this context is similar to other
power up settings such as PCIe Gen2 or Gen3.

Let's say the power up setting is ON and it boots up to Linux for the
first time after receiving a magic packet.  The Linux user can then
run ethtool -s to set the driver's non persistent WoL setting.  It can
be the same as the NVRAM's power up setting, or different.  Ethtool
may support additional WoL packet types that the power up setting does
not support.  Let's say the Linux user sets the ethtool WoL setting to
OFF and shuts down the system.  That card now will not wake up the
system.  But if there is a power failure and power comes back on
later, the card will lose the ethtool setting and go back to the power
up WoL setting, which is ON in this example.

Re: [PATCH net-next RFC 7/7] bnxt_en: Add bnxt_en initial port params table and register it

[Jiri Pirko]

Thu, Dec 06, 2018 at 09:57:05AM CET, michael.chan@broadcom.com wrote:
>On Wed, Dec 5, 2018 at 11:11 PM Jakub Kicinski
><jakub.kicinski@netronome.com> wrote:
>>
>> On Wed, 5 Dec 2018 22:41:43 -0800, Michael Chan wrote:
>> >
>> > It will be in the BIOS only for a LOM, I think.  For a NIC, it should
>> > be in the NIC's NVRAM.
>>
>> This is all vague.  Could you please clearly state the use case.
>>
>Well, the WoL setting's use case should be quite simple, right?  If
>the card's NVRAM WoL setting is ON, when you plug the card in a slot
>that has Vaux power, it will assert PME# when a magic packet is
>received.  Again, the WoL setting in this context is similar to other
>power up settings such as PCIe Gen2 or Gen3.
>
>Let's say the power up setting is ON and it boots up to Linux for the
>first time after receiving a magic packet.  The Linux user can then
>run ethtool -s to set the driver's non persistent WoL setting.  It can
>be the same as the NVRAM's power up setting, or different.  Ethtool
>may support additional WoL packet types that the power up setting does

Wouldn't it make sense to also support multiple wol packet types in
devlink param, not just true/false? Your device may not support that but
others may. So enum instead of bool.


>not support.  Let's say the Linux user sets the ethtool WoL setting to
>OFF and shuts down the system.  That card now will not wake up the
>system.  But if there is a power failure and power comes back on
>later, the card will lose the ethtool setting and go back to the power
>up WoL setting, which is ON in this example.

Re: [PATCH net-next RFC 7/7] bnxt_en: Add bnxt_en initial port params table and register it

[Vasundhara Volam]

On Thu, Dec 6, 2018 at 2:41 PM Jiri Pirko <jiri@resnulli.us> wrote:
>
> Thu, Dec 06, 2018 at 09:57:05AM CET, michael.chan@broadcom.com wrote:
> >On Wed, Dec 5, 2018 at 11:11 PM Jakub Kicinski
> ><jakub.kicinski@netronome.com> wrote:
> >>
> >> On Wed, 5 Dec 2018 22:41:43 -0800, Michael Chan wrote:
> >> >
> >> > It will be in the BIOS only for a LOM, I think.  For a NIC, it should
> >> > be in the NIC's NVRAM.
> >>
> >> This is all vague.  Could you please clearly state the use case.
> >>
> >Well, the WoL setting's use case should be quite simple, right?  If
> >the card's NVRAM WoL setting is ON, when you plug the card in a slot
> >that has Vaux power, it will assert PME# when a magic packet is
> >received.  Again, the WoL setting in this context is similar to other
> >power up settings such as PCIe Gen2 or Gen3.
> >
> >Let's say the power up setting is ON and it boots up to Linux for the
> >first time after receiving a magic packet.  The Linux user can then
> >run ethtool -s to set the driver's non persistent WoL setting.  It can
> >be the same as the NVRAM's power up setting, or different.  Ethtool
> >may support additional WoL packet types that the power up setting does
>
> Wouldn't it make sense to also support multiple wol packet types in
> devlink param, not just true/false? Your device may not support that but
> others may. So enum instead of bool.
There is no type enum in devlink types as of now. Instead it can be
defined as u8 and
a enum structure can be defined for wol types.
>
>
> >not support.  Let's say the Linux user sets the ethtool WoL setting to
> >OFF and shuts down the system.  That card now will not wake up the
> >system.  But if there is a power failure and power comes back on
> >later, the card will lose the ethtool setting and go back to the power
> >up WoL setting, which is ON in this example.

Re: [PATCH net-next RFC 7/7] bnxt_en: Add bnxt_en initial port params table and register it

[Jiri Pirko]

Thu, Dec 06, 2018 at 11:31:26AM CET, vasundhara-v.volam@broadcom.com wrote:
>On Thu, Dec 6, 2018 at 2:41 PM Jiri Pirko <jiri@resnulli.us> wrote:
>>
>> Thu, Dec 06, 2018 at 09:57:05AM CET, michael.chan@broadcom.com wrote:
>> >On Wed, Dec 5, 2018 at 11:11 PM Jakub Kicinski
>> ><jakub.kicinski@netronome.com> wrote:
>> >>
>> >> On Wed, 5 Dec 2018 22:41:43 -0800, Michael Chan wrote:
>> >> >
>> >> > It will be in the BIOS only for a LOM, I think.  For a NIC, it should
>> >> > be in the NIC's NVRAM.
>> >>
>> >> This is all vague.  Could you please clearly state the use case.
>> >>
>> >Well, the WoL setting's use case should be quite simple, right?  If
>> >the card's NVRAM WoL setting is ON, when you plug the card in a slot
>> >that has Vaux power, it will assert PME# when a magic packet is
>> >received.  Again, the WoL setting in this context is similar to other
>> >power up settings such as PCIe Gen2 or Gen3.
>> >
>> >Let's say the power up setting is ON and it boots up to Linux for the
>> >first time after receiving a magic packet.  The Linux user can then
>> >run ethtool -s to set the driver's non persistent WoL setting.  It can
>> >be the same as the NVRAM's power up setting, or different.  Ethtool
>> >may support additional WoL packet types that the power up setting does
>>
>> Wouldn't it make sense to also support multiple wol packet types in
>> devlink param, not just true/false? Your device may not support that but
>> others may. So enum instead of bool.
>There is no type enum in devlink types as of now. Instead it can be
>defined as u8 and
>a enum structure can be defined for wol types.

Yes.

>>
>>
>> >not support.  Let's say the Linux user sets the ethtool WoL setting to
>> >OFF and shuts down the system.  That card now will not wake up the
>> >system.  But if there is a power failure and power comes back on
>> >later, the card will lose the ethtool setting and go back to the power
>> >up WoL setting, which is ON in this example.

Re: [PATCH net-next RFC 7/7] bnxt_en: Add bnxt_en initial port params table and register it

[Jakub Kicinski]

On Thu, 6 Dec 2018 00:57:05 -0800, Michael Chan wrote:
> On Wed, Dec 5, 2018 at 11:11 PM Jakub Kicinski wrote:
> >
> > On Wed, 5 Dec 2018 22:41:43 -0800, Michael Chan wrote:  
> > >
> > > It will be in the BIOS only for a LOM, I think.  For a NIC, it should
> > > be in the NIC's NVRAM.  
> >
> > This is all vague.  Could you please clearly state the use case.
> >  
> Well, the WoL setting's use case should be quite simple, right?  If
> the card's NVRAM WoL setting is ON, when you plug the card in a slot
> that has Vaux power, it will assert PME# when a magic packet is
> received.  Again, the WoL setting in this context is similar to other
> power up settings such as PCIe Gen2 or Gen3.

If there was some configuration of PME# involved, maybe, but
basic networking configuration has its APIs already.

> Let's say the power up setting is ON and it boots up to Linux for the
> first time after receiving a magic packet.  The Linux user can then
> run ethtool -s to set the driver's non persistent WoL setting.  It can
> be the same as the NVRAM's power up setting, or different.  Ethtool
> may support additional WoL packet types that the power up setting does
> not support.  Let's say the Linux user sets the ethtool WoL setting to
> OFF and shuts down the system.  That card now will not wake up the
> system.  But if there is a power failure and power comes back on
> later, the card will lose the ethtool setting and go back to the power
> up WoL setting, which is ON in this example.

So in your example there is a machine with a 25/40/100G NIC that
doesn't have any remote BMC control, and connected to a L2 network
where a magic packet can be received.

In my experience machines are either low end/embedded and they just
boot on power on fully (to Linux), or they are proper machines which
support IPMI etc.

If you could illuminate the use case some more I'd really appreciate
that.  In your hypothetical scenario you still have to get the link
up, so if we apply this patch a logical extension would be to add all
ethtool link settings as devlink parameters as well.  Florian recently
added an option to wake based on a packet that matched an n-tuple
filter.  If your use case is legit, doing the same thing with n-tuple
filters instead of Magic Packets is very much legit, too.  So we will
poke n-tuple filters via devlink params?

Re: [PATCH net-next RFC 7/7] bnxt_en: Add bnxt_en initial port params table and register it

[Michael Chan]

On Thu, Dec 6, 2018 at 2:37 AM Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> On Thu, 6 Dec 2018 00:57:05 -0800, Michael Chan wrote:
> > On Wed, Dec 5, 2018 at 11:11 PM Jakub Kicinski wrote:
> > >
> > > On Wed, 5 Dec 2018 22:41:43 -0800, Michael Chan wrote:
> > > >
> > > > It will be in the BIOS only for a LOM, I think.  For a NIC, it should
> > > > be in the NIC's NVRAM.
> > >
> > > This is all vague.  Could you please clearly state the use case.
> > >
> > Well, the WoL setting's use case should be quite simple, right?  If
> > the card's NVRAM WoL setting is ON, when you plug the card in a slot
> > that has Vaux power, it will assert PME# when a magic packet is
> > received.  Again, the WoL setting in this context is similar to other
> > power up settings such as PCIe Gen2 or Gen3.
>
> If there was some configuration of PME# involved, maybe, but
> basic networking configuration has its APIs already.
>
> > Let's say the power up setting is ON and it boots up to Linux for the
> > first time after receiving a magic packet.  The Linux user can then
> > run ethtool -s to set the driver's non persistent WoL setting.  It can
> > be the same as the NVRAM's power up setting, or different.  Ethtool
> > may support additional WoL packet types that the power up setting does
> > not support.  Let's say the Linux user sets the ethtool WoL setting to
> > OFF and shuts down the system.  That card now will not wake up the
> > system.  But if there is a power failure and power comes back on
> > later, the card will lose the ethtool setting and go back to the power
> > up WoL setting, which is ON in this example.
>
> So in your example there is a machine with a 25/40/100G NIC that
> doesn't have any remote BMC control, and connected to a L2 network
> where a magic packet can be received.
>
> In my experience machines are either low end/embedded and they just
> boot on power on fully (to Linux), or they are proper machines which
> support IPMI etc.
>
> If you could illuminate the use case some more I'd really appreciate
> that.  In your hypothetical scenario you still have to get the link
> up, so if we apply this patch a logical extension would be to add all
> ethtool link settings as devlink parameters as well.  Florian recently
> added an option to wake based on a packet that matched an n-tuple
> filter.  If your use case is legit, doing the same thing with n-tuple
> filters instead of Magic Packets is very much legit, too.  So we will
> poke n-tuple filters via devlink params?

We only store a magic packet WoL bit in the NVRAM for basic power up
WoL setting.  I doubt that people will store the entire n-tuple WoL
pattern in NVRAM for basic power up WoL.  The whole idea is to have a
basic method to wake up the machine after power up with Vaux.  If the
cable is connected, the NIC will autoneg to some lower speed that Vaux
can support.  I think we've been supporting this since the tg3 days.