Re: [PATCH net-next v2 RFC 1/8] devlink: Add devlink_param for port register and unregister

[Jiri Pirko <jiri@resnulli.us>]

Wed, Dec 12, 2018 at 02:43:14PM CET, vasundhara-v.volam@broadcom.com wrote:
>On Tue, Dec 11, 2018 at 2:39 PM Jiri Pirko <jiri@resnulli.us> wrote:
>>
>> Tue, Dec 11, 2018 at 09:46:41AM 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 <jiri@mellanox.com>
>> >Signed-off-by: Vasundhara Volam <vasundhara-v.volam@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(&param_item->list, &devlink->param_list);
>> >+      list_add_tail(&param_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(&param_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
>> >