Re: [PATCH net-next RFC 4/7] devlink: Add support for get/set driverinit value for devlink_port

[Jiri Pirko <jiri@resnulli.us>]

Wed, Dec 05, 2018 at 06:56:57AM CET, vasundhara-v.volam@broadcom.com wrote:
>Add support for "driverinit" configuration mode value for devlink_port
>configuration parameters. Two additional functions added to help the
>driver get/set the value from/to devlink:
>devlink_port_param_driverinit_value_set() and
>devlink_port_param_driverinit_value_get().
>
>Also, move the common code to __devlink_param_driverinit_value_get/set()
>to be used by both device and port params.
>
>Cc: Jiri Pirko <jiri@mellanox.com>
>Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
>---
> include/net/devlink.h |  19 ++++++++
> net/core/devlink.c    | 118 +++++++++++++++++++++++++++++++++++++++-----------
> 2 files changed, 112 insertions(+), 25 deletions(-)
>
>diff --git a/include/net/devlink.h b/include/net/devlink.h
>index 9b4d80b..c22b195 100644
>--- a/include/net/devlink.h
>+++ b/include/net/devlink.h
>@@ -588,6 +588,9 @@ int devlink_port_params_register(struct devlink_port *devlink_port,
> void devlink_port_params_unregister(struct devlink_port *devlink_port,
> 				    const struct devlink_param *params,
> 				    size_t params_count);
>+int devlink_port_param_driverinit_value_set(struct devlink_port *devlink_port,
>+					    u32 param_id,
>+					    union devlink_param_value init_val);
> struct devlink_region *devlink_region_create(struct devlink *devlink,
> 					     const char *region_name,
> 					     u32 region_max_snapshots,
>@@ -845,6 +848,22 @@ static inline bool devlink_dpipe_table_counter_enabled(struct devlink *devlink,
> {
> }
> 
>+static inline int
>+devlink_port_param_driverinit_value_get(struct devlink_port *devlink_port,
>+					u32 param_id,
>+					union devlink_param_value *init_val)
>+{
>+	return -EOPNOTSUPP;
>+}
>+
>+static inline int
>+devlink_port_param_driverinit_value_set(struct devlink_port *devlink_port,
>+					u32 param_id,
>+					union devlink_param_value init_val)
>+{
>+	return -EOPNOTSUPP;
>+}
>+
> 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 10e1c45..4f0052d 100644
>--- a/net/core/devlink.c
>+++ b/net/core/devlink.c
>@@ -4690,6 +4690,29 @@ void devlink_params_unregister(struct devlink *devlink,
> }
> EXPORT_SYMBOL_GPL(devlink_params_unregister);
> 
>+static int
>+__devlink_param_driverinit_value_get(struct list_head *param_list, u32 param_id,
>+				     union devlink_param_value *init_val)
>+{
>+	struct devlink_param_item *param_item;
>+
>+	param_item = devlink_param_find_by_id(param_list, param_id);
>+	if (!param_item)
>+		return -EINVAL;
>+
>+	if (!param_item->driverinit_value_valid ||
>+	    !devlink_param_cmode_is_supported(param_item->param,
>+					      DEVLINK_PARAM_CMODE_DRIVERINIT))
>+		return -EOPNOTSUPP;
>+
>+	if (param_item->param->type == DEVLINK_PARAM_TYPE_STRING)
>+		strcpy(init_val->vstr, param_item->driverinit_value.vstr);
>+	else
>+		*init_val = param_item->driverinit_value;
>+
>+	return 0;
>+}
>+
> /**
>  *	devlink_param_driverinit_value_get - get configuration parameter
>  *					     value for driver initializing
>@@ -4704,28 +4727,40 @@ void devlink_params_unregister(struct devlink *devlink,
> int devlink_param_driverinit_value_get(struct devlink *devlink, u32 param_id,
> 				       union devlink_param_value *init_val)
> {
>-	struct devlink_param_item *param_item;
>-
> 	if (!devlink->ops || !devlink->ops->reload)
> 		return -EOPNOTSUPP;
> 
>+	return __devlink_param_driverinit_value_get(&devlink->param_list,
>+						    param_id, init_val);
>+}
>+EXPORT_SYMBOL_GPL(devlink_param_driverinit_value_get);
>+
>+static int
>+__devlink_param_driverinit_value_set(struct devlink *devlink,
>+				     struct list_head *param_list,
>+				     u32 param_id,
>+				     union devlink_param_value init_val,
>+				     enum devlink_command cmd)
>+{
>+	struct devlink_param_item *param_item;
>+
> 	param_item = devlink_param_find_by_id(&devlink->param_list, param_id);
> 	if (!param_item)
> 		return -EINVAL;
> 
>-	if (!param_item->driverinit_value_valid ||
>-	    !devlink_param_cmode_is_supported(param_item->param,
>+	if (!devlink_param_cmode_is_supported(param_item->param,
> 					      DEVLINK_PARAM_CMODE_DRIVERINIT))
> 		return -EOPNOTSUPP;
> 
> 	if (param_item->param->type == DEVLINK_PARAM_TYPE_STRING)
>-		strcpy(init_val->vstr, param_item->driverinit_value.vstr);
>+		strcpy(param_item->driverinit_value.vstr, init_val.vstr);
> 	else
>-		*init_val = param_item->driverinit_value;
>+		param_item->driverinit_value = init_val;
>+	param_item->driverinit_value_valid = true;
>+	devlink_param_notify(devlink, param_item, cmd);
> 
> 	return 0;

Git does not understand what you do and it screwed thing up so the
review is not easy. I suggest to split this somehow, perhaps get and set
part separatelly.

Thanks.


> }
>-EXPORT_SYMBOL_GPL(devlink_param_driverinit_value_get);
> 

[...]