diff mbox series

[net-next,2/7] devlink: make sure driver does not read updated driverinit param before reload

Message ID 20230209154308.2984602-3-jiri@resnulli.us (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series devlink: params cleanups and devl_param_driverinit_value_get() fix | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 383 this patch: 383
netdev/cc_maintainers success CCed 6 of 6 maintainers
netdev/build_clang success Errors and warnings before: 18 this patch: 18
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 534 this patch: 534
netdev/checkpatch warning WARNING: line length of 82 exceeds 80 columns WARNING: line length of 88 exceeds 80 columns
netdev/kdoc success Errors and warnings before: 14 this patch: 14
netdev/source_inline success Was 0 now: 0

Commit Message

Jiri Pirko Feb. 9, 2023, 3:43 p.m. UTC 
From: Jiri Pirko <jiri@nvidia.com>

The driverinit param purpose is to serve the driver during init/reload
time to provide a value, either default or set by user.

Make sure that driver does not read value updated by user before the
reload is performed. Hold the new value in a separate struct and switch
it during reload.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
 include/net/devlink.h       |  4 ++++
 net/devlink/dev.c           |  2 ++
 net/devlink/devl_internal.h |  3 +++
 net/devlink/leftover.c      | 26 ++++++++++++++++++++++----
 4 files changed, 31 insertions(+), 4 deletions(-)

Comments

Simon Horman Feb. 9, 2023, 4:42 p.m. UTC | #1 
On Thu, Feb 09, 2023 at 04:43:03PM +0100, Jiri Pirko wrote:
> From: Jiri Pirko <jiri@nvidia.com>
> 
> The driverinit param purpose is to serve the driver during init/reload
> time to provide a value, either default or set by user.
> 
> Make sure that driver does not read value updated by user before the
> reload is performed. Hold the new value in a separate struct and switch
> it during reload.

I gather this is related to:

[patch net-next 6/7] devlink: allow to call
        devl_param_driverinit_value_get() without holding instance lock

Perhaps it is worth mentioning that here.

> Signed-off-by: Jiri Pirko <jiri@nvidia.com>

Irregardless, this looks good to me.

Reviewed-by: Simon Horman <simon.horman@corigine.com>
diff mbox series

Patch

diff --git a/include/net/devlink.h b/include/net/devlink.h
index 2e85a5970a32..8ed960345f37 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -489,6 +489,10 @@  struct devlink_param_item {
 	const struct devlink_param *param;
 	union devlink_param_value driverinit_value;
 	bool driverinit_value_valid;
+	union devlink_param_value driverinit_value_new; /* Not reachable
+							 * until reload.
+							 */
+	bool driverinit_value_new_valid;
 };
 
 enum devlink_param_generic_id {
diff --git a/net/devlink/dev.c b/net/devlink/dev.c
index 78d824eda5ec..32e5d1b28a47 100644
--- a/net/devlink/dev.c
+++ b/net/devlink/dev.c
@@ -369,6 +369,8 @@  int devlink_reload(struct devlink *devlink, struct net *dest_net,
 	if (dest_net && !net_eq(dest_net, curr_net))
 		devlink_reload_netns_change(devlink, curr_net, dest_net);
 
+	devlink_params_driverinit_load_new(devlink);
+
 	err = devlink->ops->reload_up(devlink, action, limit, actions_performed, extack);
 	devlink_reload_failed_set(devlink, !!err);
 	if (err)
diff --git a/net/devlink/devl_internal.h b/net/devlink/devl_internal.h
index 941174e157d4..5c117e8d4377 100644
--- a/net/devlink/devl_internal.h
+++ b/net/devlink/devl_internal.h
@@ -189,6 +189,9 @@  static inline bool devlink_reload_supported(const struct devlink_ops *ops)
 	return ops->reload_down && ops->reload_up;
 }
 
+/* Params */
+void devlink_params_driverinit_load_new(struct devlink *devlink);
+
 /* Resources */
 struct devlink_resource;
 int devlink_resources_validate(struct devlink *devlink,
diff --git a/net/devlink/leftover.c b/net/devlink/leftover.c
index 6225651e34b9..6c4c95c658c7 100644
--- a/net/devlink/leftover.c
+++ b/net/devlink/leftover.c
@@ -4098,9 +4098,12 @@  static int devlink_nl_param_fill(struct sk_buff *msg, struct devlink *devlink,
 		if (!devlink_param_cmode_is_supported(param, i))
 			continue;
 		if (i == DEVLINK_PARAM_CMODE_DRIVERINIT) {
-			if (!param_item->driverinit_value_valid)
+			if (param_item->driverinit_value_new_valid)
+				param_value[i] = param_item->driverinit_value_new;
+			else if (param_item->driverinit_value_valid)
+				param_value[i] = param_item->driverinit_value;
+			else
 				return -EOPNOTSUPP;
-			param_value[i] = param_item->driverinit_value;
 		} else {
 			ctx.cmode = i;
 			err = devlink_param_get(devlink, param, &ctx);
@@ -4388,8 +4391,8 @@  static int __devlink_nl_cmd_param_set_doit(struct devlink *devlink,
 		return -EOPNOTSUPP;
 
 	if (cmode == DEVLINK_PARAM_CMODE_DRIVERINIT) {
-		param_item->driverinit_value = value;
-		param_item->driverinit_value_valid = true;
+		param_item->driverinit_value_new = value;
+		param_item->driverinit_value_new_valid = true;
 	} else {
 		if (!param->set)
 			return -EOPNOTSUPP;
@@ -9687,6 +9690,21 @@  void devl_param_driverinit_value_set(struct devlink *devlink, u32 param_id,
 }
 EXPORT_SYMBOL_GPL(devl_param_driverinit_value_set);
 
+void devlink_params_driverinit_load_new(struct devlink *devlink)
+{
+	struct devlink_param_item *param_item;
+
+	list_for_each_entry(param_item, &devlink->param_list, list) {
+		if (!devlink_param_cmode_is_supported(param_item->param,
+						      DEVLINK_PARAM_CMODE_DRIVERINIT) ||
+		    !param_item->driverinit_value_new_valid)
+			continue;
+		param_item->driverinit_value = param_item->driverinit_value_new;
+		param_item->driverinit_value_valid = true;
+		param_item->driverinit_value_new_valid = false;
+	}
+}
+
 /**
  *	devl_param_value_changed - notify devlink on a parameter's value
  *				   change. Should be called by the driver