From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jiri Pirko Subject: Re: [PATCH net-next RFC 1/7] devlink: Add devlink_param for port register and unregister Date: Wed, 5 Dec 2018 12:47:51 +0100 Message-ID: <20181205114751.GB2318@nanopsycho> References: <1543989420-14859-1-git-send-email-vasundhara-v.volam@broadcom.com> <1543989420-14859-2-git-send-email-vasundhara-v.volam@broadcom.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: davem@davemloft.net, michael.chan@broadcom.com, jiri@mellanox.com, netdev@vger.kernel.org To: Vasundhara Volam Return-path: Received: from mail-wr1-f65.google.com ([209.85.221.65]:40577 "EHLO mail-wr1-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726924AbeLELy5 (ORCPT ); Wed, 5 Dec 2018 06:54:57 -0500 Received: by mail-wr1-f65.google.com with SMTP id p4so19366549wrt.7 for ; Wed, 05 Dec 2018 03:54:55 -0800 (PST) Content-Disposition: inline In-Reply-To: <1543989420-14859-2-git-send-email-vasundhara-v.volam@broadcom.com> Sender: netdev-owner@vger.kernel.org List-ID: 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 >Signed-off-by: Vasundhara Volam >--- > 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 >