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

[Jiri Pirko <jiri@resnulli.us>]

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
>