Wed, Dec 12, 2018 at 02:43:14PM CET, vasundhara-v.vo...@broadcom.com wrote: >On Tue, Dec 11, 2018 at 2:39 PM Jiri Pirko <j...@resnulli.us> wrote: >> >> Tue, Dec 11, 2018 at 09:46:41AM 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 | 22 +++++++++ >> > net/core/devlink.c | 128 >> > ++++++++++++++++++++++++++++++++++++++++---------- >> > 2 files changed, 124 insertions(+), 26 deletions(-) >> > >> >diff --git a/include/net/devlink.h b/include/net/devlink.h >> >index 67f4293..98b8a66 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; >> >@@ -574,6 +575,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 +823,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..da7f6bd 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,18 +3165,18 @@ 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); >> >@@ -3954,6 +3954,7 @@ int devlink_port_register(struct devlink *devlink, >> > devlink_port->index = port_index; >> > devlink_port->registered = true; >> > list_add_tail(&devlink_port->list, &devlink->port_list); >> >+ INIT_LIST_HEAD(&devlink_port->param_list); >> > mutex_unlock(&devlink->lock); >> > devlink_port_notify(devlink_port, DEVLINK_CMD_PORT_NEW); >> > return 0; >> >@@ -4471,6 +4472,16 @@ void devlink_resource_occ_get_unregister(struct >> >devlink *devlink, >> > } >> > EXPORT_SYMBOL_GPL(devlink_resource_occ_get_unregister); >> > >> >+static int devlink_param_verify(const struct devlink_param *param) >> >+{ >> >+ if (!param || !param->name || !param->supported_cmodes) >> >+ return -EINVAL; >> >+ if (param->generic) >> >+ return devlink_param_generic_verify(param); >> >+ else >> >+ return devlink_param_driver_verify(param); >> >+} >> >+ >> > /** >> > * devlink_params_register - register configuration parameters >> > * >> >@@ -4490,20 +4501,12 @@ int devlink_params_register(struct devlink *devlink, >> > >> > mutex_lock(&devlink->lock); >> > for (i = 0; i < params_count; i++, param++) { >> >- if (!param || !param->name || !param->supported_cmodes) { >> >- err = -EINVAL; >> >+ err = devlink_param_verify(param); >> >+ if (err) >> > goto rollback; >> >- } >> >- if (param->generic) { >> >- err = devlink_param_generic_verify(param); >> >- if (err) >> >- goto rollback; >> >- } else { >> >- err = devlink_param_driver_verify(param); >> >- 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,13 +4518,28 @@ 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; >> > } >> > EXPORT_SYMBOL_GPL(devlink_params_register); >> > >> >+static void __devlink_params_unregister(struct devlink *devlink, >> >+ struct list_head *param_list, >> >+ const struct devlink_param *params, >> >+ size_t params_count) >> >+{ >> >+ 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, param_list, param); >> >+ mutex_unlock(&devlink->lock); >> >+} >> >> Please do devlink_params_register in a similar way. Have >> __devlink_params_register() with list_head * arg > >This cannot be optimised as you see the whole patch-set, unregister >also requires different command in case of failure of >param_register_one and sending 2 types of command as argument will not >simplify this code. It is better this way, as you see the final >function.
I don't see any problem in passing 2 cmd values. The point is to avoid code duplication and unnecessary bugs like I found below. > >> >> >> >+ >> > /** >> > * devlink_params_unregister - unregister configuration parameters >> > * @devlink: devlink >> >@@ -4532,13 +4550,8 @@ void devlink_params_unregister(struct devlink >> >*devlink, >> > const struct devlink_param *params, >> > size_t params_count) >> > { >> >- 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, param); >> >- mutex_unlock(&devlink->lock); >> >+ return __devlink_params_unregister(devlink, &devlink->param_list, >> >+ params, params_count); >> > } >> > EXPORT_SYMBOL_GPL(devlink_params_unregister); >> > >> >@@ -4657,6 +4670,69 @@ 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; >> >+ >> >+ mutex_lock(&devlink->lock); >> >+ for (i = 0; i < params_count; i++) { You need to increment param here. >> >+ err = devlink_param_verify(param); >> >+ if (err) >> >+ goto rollback; >> >+ >> >+ err = devlink_param_register_one(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, >> >&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) >> >+{ >> >+ return __devlink_params_unregister(devlink_port->devlink, >> >+ &devlink_port->param_list, >> >+ params, params_count); >> >+} >> >+EXPORT_SYMBOL_GPL(devlink_port_params_unregister); >> >+ >> >+/** >> > * devlink_region_create - create a new address region >> > * >> > * @devlink: devlink >> >-- >> >1.8.3.1 >> >