Thank you reviewing the patches. On Wed, Dec 5, 2018 at 5:24 PM Jiri Pirko <j...@resnulli.us> wrote: > > 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. 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(¶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() 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 > >