| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 6 Dec 2018 11:32:59 +0530
From: Vasundhara Volam <vasundhara-v.volam@...adcom.com>
To: Jiří Pírko <jiri@...nulli.us>
Cc: David Miller <davem@...emloft.net>,
"michael.chan@...adcom.com" <michael.chan@...adcom.com>,
Jiri Pirko <jiri@...lanox.com>, Netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next RFC 1/7] devlink: Add devlink_param for port
register and unregister
Thank you reviewing the patches.
On Wed, Dec 5, 2018 at 5:24 PM Jiri Pirko <jiri@...nulli.us> wrote:
>
> Wed, Dec 05, 2018 at 06:56:54AM CET, vasundhara-v.volam@...adcom.com wrote:
> >Add functions to register and unregister for the driver supported
> >configuration parameters table per port.
> >
> >Cc: Jiri Pirko <jiri@...lanox.com>
> >Signed-off-by: Vasundhara Volam <vasundhara-v.volam@...adcom.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
> >
Powered by blists - more mailing lists