diff mbox series

[net-next,1/2] devlink: Allow parameter registration/unregistration during runtime

Message ID 20220310231235.2721368-2-anthony.l.nguyen@intel.com (mailing list archive)
State Deferred
Delegated to: Netdev Maintainers
Headers show
Series 10GbE Intel Wired LAN Driver Updates 2022-03-10 | 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: 2 this patch: 2
netdev/cc_maintainers success CCed 4 of 4 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/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 7 this patch: 7
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 65 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Tony Nguyen March 10, 2022, 11:12 p.m. UTC 
From: Sridhar Samudrala <sridhar.samudrala@intel.com>

commit 7a690ad499e7 ("devlink: Clean not-executed param notifications")
added ASSERTs and removed notifications when devlink parameters are
registered by the drivers after the associated devlink instance is
already registered.
The next patch in this series adds a feature in 'ice' driver that is
only enabled when ADQ queue groups are created and introduces a
devlink parameter to configure this feature per queue group.

To allow dynamic parameter registration/unregistration during runtime,
revert the changes added by the above commit.

Signed-off-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
Signed-off-by: Sudheer Mogilappagari <sudheer.mogilappagari@intel.com>
Tested-by: Bharathi Sreenivas <bharathi.sreenivas@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 net/core/devlink.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

Comments

Jakub Kicinski March 11, 2022, 4:33 a.m. UTC | #1 
On Thu, 10 Mar 2022 15:12:34 -0800 Tony Nguyen wrote:
> From: Sridhar Samudrala <sridhar.samudrala@intel.com>
> 
> commit 7a690ad499e7 ("devlink: Clean not-executed param notifications")
> added ASSERTs and removed notifications when devlink parameters are
> registered by the drivers after the associated devlink instance is
> already registered.
> The next patch in this series adds a feature in 'ice' driver that is
> only enabled when ADQ queue groups are created and introduces a
> devlink parameter to configure this feature per queue group.
> 
> To allow dynamic parameter registration/unregistration during runtime,
> revert the changes added by the above commit.

I'm pretty sure what you're doing is broken. You should probably wait
until my patches to allow explicit devlink locking are merged and build
on top of that.
Leon Romanovsky March 11, 2022, 6:21 a.m. UTC | #2 
On Thu, Mar 10, 2022 at 08:33:48PM -0800, Jakub Kicinski wrote:
> On Thu, 10 Mar 2022 15:12:34 -0800 Tony Nguyen wrote:
> > From: Sridhar Samudrala <sridhar.samudrala@intel.com>
> > 
> > commit 7a690ad499e7 ("devlink: Clean not-executed param notifications")
> > added ASSERTs and removed notifications when devlink parameters are
> > registered by the drivers after the associated devlink instance is
> > already registered.
> > The next patch in this series adds a feature in 'ice' driver that is
> > only enabled when ADQ queue groups are created and introduces a
> > devlink parameter to configure this feature per queue group.
> > 
> > To allow dynamic parameter registration/unregistration during runtime,
> > revert the changes added by the above commit.
> 
> I'm pretty sure what you're doing is broken. You should probably wait
> until my patches to allow explicit devlink locking are merged and build
> on top of that.

Yes, it is broken, but I don't see how your devlink locking series will
help here. IMHO, devlink_params_register() should not be dynamic [1]. 

Thanks

[1] https://lore.kernel.org/all/YirRQWT7dtTV4fwG@unreal
Jakub Kicinski March 11, 2022, 6:28 a.m. UTC | #3 
On Fri, 11 Mar 2022 08:21:01 +0200 Leon Romanovsky wrote:
> > I'm pretty sure what you're doing is broken. You should probably wait
> > until my patches to allow explicit devlink locking are merged and build
> > on top of that.  
> 
> Yes, it is broken, but I don't see how your devlink locking series will
> help here. IMHO, devlink_params_register() should not be dynamic [1]. 

Quite possible, I can't think of an in-tree use case that may require
adding and removing params.
diff mbox series

Patch

diff --git a/net/core/devlink.c b/net/core/devlink.c
index fcd9f6d85cf1..d09f2aa4f48f 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -4724,7 +4724,6 @@  static void devlink_param_notify(struct devlink *devlink,
 	WARN_ON(cmd != DEVLINK_CMD_PARAM_NEW && cmd != DEVLINK_CMD_PARAM_DEL &&
 		cmd != DEVLINK_CMD_PORT_PARAM_NEW &&
 		cmd != DEVLINK_CMD_PORT_PARAM_DEL);
-	ASSERT_DEVLINK_REGISTERED(devlink);
 
 	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
 	if (!msg)
@@ -10120,8 +10119,6 @@  int devlink_params_register(struct devlink *devlink,
 	const struct devlink_param *param = params;
 	int i, err;
 
-	ASSERT_DEVLINK_NOT_REGISTERED(devlink);
-
 	for (i = 0; i < params_count; i++, param++) {
 		err = devlink_param_register(devlink, param);
 		if (err)
@@ -10152,8 +10149,6 @@  void devlink_params_unregister(struct devlink *devlink,
 	const struct devlink_param *param = params;
 	int i;
 
-	ASSERT_DEVLINK_NOT_REGISTERED(devlink);
-
 	for (i = 0; i < params_count; i++, param++)
 		devlink_param_unregister(devlink, param);
 }
@@ -10173,8 +10168,6 @@  int devlink_param_register(struct devlink *devlink,
 {
 	struct devlink_param_item *param_item;
 
-	ASSERT_DEVLINK_NOT_REGISTERED(devlink);
-
 	WARN_ON(devlink_param_verify(param));
 	WARN_ON(devlink_param_find_by_name(&devlink->param_list, param->name));
 
@@ -10190,6 +10183,7 @@  int devlink_param_register(struct devlink *devlink,
 	param_item->param = param;
 
 	list_add_tail(&param_item->list, &devlink->param_list);
+	devlink_param_notify(devlink, 0, param_item, DEVLINK_CMD_PARAM_NEW);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(devlink_param_register);
@@ -10204,11 +10198,10 @@  void devlink_param_unregister(struct devlink *devlink,
 {
 	struct devlink_param_item *param_item;
 
-	ASSERT_DEVLINK_NOT_REGISTERED(devlink);
-
 	param_item =
 		devlink_param_find_by_name(&devlink->param_list, param->name);
 	WARN_ON(!param_item);
+	devlink_param_notify(devlink, 0, param_item, DEVLINK_CMD_PARAM_DEL);
 	list_del(&param_item->list);
 	kfree(param_item);
 }
@@ -10268,8 +10261,6 @@  int devlink_param_driverinit_value_set(struct devlink *devlink, u32 param_id,
 {
 	struct devlink_param_item *param_item;
 
-	ASSERT_DEVLINK_NOT_REGISTERED(devlink);
-
 	param_item = devlink_param_find_by_id(&devlink->param_list, param_id);
 	if (!param_item)
 		return -EINVAL;
@@ -10283,6 +10274,7 @@  int devlink_param_driverinit_value_set(struct devlink *devlink, u32 param_id,
 	else
 		param_item->driverinit_value = init_val;
 	param_item->driverinit_value_valid = true;
+	devlink_param_notify(devlink, 0, param_item, DEVLINK_CMD_PARAM_NEW);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(devlink_param_driverinit_value_set);