| 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
| ||
|
Message-ID: <CAACQVJr1VLpZ+s5dCPsf=ba43Ft4wVtZAWQhXvE2qGMeybm-Gw@mail.gmail.com>
Date: Fri, 14 Dec 2018 10:22:47 +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>, jakub.kicinski@...ronome.com,
Netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next v2 RFC 1/8] devlink: Add devlink_param for port
register and unregister
On Thu, Dec 13, 2018 at 6:32 PM Jiri Pirko <jiri@...nulli.us> wrote:
>
> Wed, Dec 12, 2018 at 02:43:14PM CET, vasundhara-v.volam@...adcom.com wrote:
> >On Tue, Dec 11, 2018 at 2:39 PM Jiri Pirko <jiri@...nulli.us> wrote:
> >>
> >> Tue, Dec 11, 2018 at 09:46:41AM 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 | 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.
>
Okay, got your point. Since this will be easy to maintain, it is good to move to
common function. I will modify this in next version. Thank you.
>
> >
> >>
> >>
> >> >+
> >> > /**
> >> > * 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
> >> >
Powered by blists - more mailing lists