Wed, Dec 05, 2018 at 06:56:54AM CET, vasundhara-v.vo...@broadcom.com wrote: >Add functions to register and unregister for the driver supported >configuration parameters table per port. > >Cc: Jiri Pirko <j...@mellanox.com> >Signed-off-by: Vasundhara Volam <vasundhara-v.vo...@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(¶m_item->list, &devlink->param_list); >+ list_add_tail(¶m_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(¶m_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 >