Skip to contentSkip to conversation

mlxsw: Misc devlink changes

PATCH Applied Petr Machata 4 participants 10 messages Feb 6, 2023 – Feb 8, 2023

Subsystems (3)
lore
Share & tools
lore

Petr Machata

author maintainer
#0 lore raw

cover mlxsw: Misc devlink changes

This patchset adjusts mlxsw to recent devlink changes in net-next.

Patch #1 removes a devl_param_driverinit_value_set() call that was unnecessary, but now additionally triggers a WARN_ON.

Patches #2-#4 are non-functional preparations for the following patches.

Patch #5 fixes a use-after-free that is triggered while changing network namespaces.

Patch #6 makes mlxsw consistent with netdevsim by having mlxsw register its devlink instance before its sub-objects. It helps us avoid a warning described in the commit message.

Danielle Ratson (1):
mlxsw: spectrum: Remove pointless call to
devlink_param_driverinit_value_set()

Ido Schimmel (5):
mlxsw: spectrum_acl_tcam: Add missing mutex_destroy()
mlxsw: spectrum_acl_tcam: Make fini symmetric to init
mlxsw: spectrum_acl_tcam: Reorder functions to avoid forward
declarations
mlxsw: spectrum_acl_tcam: Move devlink param to TCAM code
mlxsw: core: Register devlink instance before sub-objects

 drivers/net/ethernet/mellanox/mlxsw/core.c    |  31 +--
 drivers/net/ethernet/mellanox/mlxsw/core.h    |   2 -
 .../net/ethernet/mellanox/mlxsw/spectrum.c    |  62 -----
 .../net/ethernet/mellanox/mlxsw/spectrum.h    |   3 +-
 .../ethernet/mellanox/mlxsw/spectrum_acl.c    |  21 +-
 .../mellanox/mlxsw/spectrum_acl_tcam.c        | 244 +++++++++++-------
 .../mellanox/mlxsw/spectrum_acl_tcam.h        |   5 -
 7 files changed, 161 insertions(+), 207 deletions(-)
Signature · 1 line
2.39.0
author maintainer
#1 lore raw

PATCH 1/6 mlxsw: spectrum: Remove pointless call to devlink_param_driverinit_value_set()

From: Danielle Ratson <[email protected]>

The "acl_region_rehash_interval" devlink parameter is a "runtime" parameter, making the call to devl_param_driverinit_value_set() pointless. Before cited commit the function simply returned an error (that was not checked), but now it emits a WARNING [1].

Fix by removing the function call.

[1]

WARNING: CPU: 0 PID: 7 at net/devlink/leftover.c:10974
devl_param_driverinit_value_set+0x8c/0x90
[...]
Call Trace:
 <TASK>
 mlxsw_sp2_params_register+0x83/0xb0 [mlxsw_spectrum]
 __mlxsw_core_bus_device_register+0x5e5/0x990 [mlxsw_core]
 mlxsw_core_bus_device_register+0x42/0x60 [mlxsw_core]
 mlxsw_pci_probe+0x1f0/0x230 [mlxsw_pci]
 local_pci_probe+0x1a/0x40
 work_for_cpu_fn+0xf/0x20
 process_one_work+0x1db/0x390
 worker_thread+0x1d5/0x3b0
 kthread+0xe5/0x110
 ret_from_fork+0x1f/0x30
 </TASK>
Fixes 85fe0b324c83 ("devlink: make devlink_param_driverinit_value_set() return void")
---
 drivers/net/ethernet/mellanox/mlxsw/spectrum.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)
drivers/net/ethernet/mellanox/mlxsw/spectrum.c +2 -12
index b0bdb9640ebf..b150e97b97b7 100644
@@ -3895,19 +3895,9 @@ static const struct devlink_param mlxsw_sp2_devlink_params[] = {
 static int mlxsw_sp2_params_register(struct mlxsw_core *mlxsw_core)
 {
 	struct devlink *devlink = priv_to_devlink(mlxsw_core);
-	union devlink_param_value value;
-	int err;
 
-	err = devl_params_register(devlink, mlxsw_sp2_devlink_params,
-				   ARRAY_SIZE(mlxsw_sp2_devlink_params));
-	if (err)
-		return err;
-
-	value.vu32 = 0;
-	devl_param_driverinit_value_set(devlink,
-					MLXSW_DEVLINK_PARAM_ID_ACL_REGION_REHASH_INTERVAL,
-					value);
-	return 0;
+	return devl_params_register(devlink, mlxsw_sp2_devlink_params,
+				    ARRAY_SIZE(mlxsw_sp2_devlink_params));
 }
 
 static void mlxsw_sp2_params_unregister(struct mlxsw_core *mlxsw_core)
Signature · 1 line
2.39.0
Reviewed-by on v1 1/6
#2 lore raw

Re: [PATCH net-next 1/6] mlxsw: spectrum: Remove pointless call to devlink_param_driverinit_value_set()

Quoted 27 lines · Mon, Feb 06, 2023 at 04:39:18PM CET, [email protected] wrote:

From: Danielle Ratson <[email protected]>

The "acl_region_rehash_interval" devlink parameter is a "runtime" parameter, making the call to devl_param_driverinit_value_set() pointless. Before cited commit the function simply returned an error (that was not checked), but now it emits a WARNING [1].

Fix by removing the function call.

[1]

WARNING: CPU: 0 PID: 7 at net/devlink/leftover.c:10974
devl_param_driverinit_value_set+0x8c/0x90
[...]
Call Trace:
<TASK>
mlxsw_sp2_params_register+0x83/0xb0 [mlxsw_spectrum]
__mlxsw_core_bus_device_register+0x5e5/0x990 [mlxsw_core]
mlxsw_core_bus_device_register+0x42/0x60 [mlxsw_core]
mlxsw_pci_probe+0x1f0/0x230 [mlxsw_pci]
local_pci_probe+0x1a/0x40
work_for_cpu_fn+0xf/0x20
process_one_work+0x1db/0x390
worker_thread+0x1d5/0x3b0
kthread+0xe5/0x110
ret_from_fork+0x1f/0x30
</TASK>
Quoted 3 lines
Fixes 85fe0b324c83 ("devlink: make devlink_param_driverinit_value_set() return void")
Quoted 1 line
author maintainer
#3 lore raw

PATCH 2/6 mlxsw: spectrum_acl_tcam: Add missing mutex_destroy()

In reply to Petr Machata

From: Ido Schimmel <[email protected]>

Pair mutex_init() with a mutex_destroy() in the error path. Found during code review. No functional changes.

---
 drivers/net/ethernet/mellanox/mlxsw/spectrum_acl_tcam.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
drivers/net/ethernet/mellanox/mlxsw/spectrum_acl_tcam.c +6 -2
index 3b9ba8fa247a..7cbfd34d02de 100644
@@ -52,8 +52,10 @@ int mlxsw_sp_acl_tcam_init(struct mlxsw_sp *mlxsw_sp,
 		max_regions = max_tcam_regions;
 
 	tcam->used_regions = bitmap_zalloc(max_regions, GFP_KERNEL);
-	if (!tcam->used_regions)
-		return -ENOMEM;
+	if (!tcam->used_regions) {
+		err = -ENOMEM;
+		goto err_alloc_used_regions;
+	}
 	tcam->max_regions = max_regions;
 
 	max_groups = MLXSW_CORE_RES_GET(mlxsw_sp->core, ACL_MAX_GROUPS);
@@ -76,6 +78,8 @@ int mlxsw_sp_acl_tcam_init(struct mlxsw_sp *mlxsw_sp,
 	bitmap_free(tcam->used_groups);
 err_alloc_used_groups:
 	bitmap_free(tcam->used_regions);
+err_alloc_used_regions:
+	mutex_destroy(&tcam->lock);
 	return err;
 }
 
Signature · 1 line
2.39.0
author maintainer
#4 lore raw

PATCH 3/6 mlxsw: spectrum_acl_tcam: Make fini symmetric to init

In reply to Petr Machata

From: Ido Schimmel <[email protected]>

Move mutex_destroy() to the end to make the function symmetric with mlxsw_sp_acl_tcam_init(). No functional changes.

---
 drivers/net/ethernet/mellanox/mlxsw/spectrum_acl_tcam.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/ethernet/mellanox/mlxsw/spectrum_acl_tcam.c +1 -1
index 7cbfd34d02de..2445957355cd 100644
@@ -88,10 +88,10 @@ void mlxsw_sp_acl_tcam_fini(struct mlxsw_sp *mlxsw_sp,
 {
 	const struct mlxsw_sp_acl_tcam_ops *ops = mlxsw_sp->acl_tcam_ops;
 
-	mutex_destroy(&tcam->lock);
 	ops->fini(mlxsw_sp, tcam->priv);
 	bitmap_free(tcam->used_groups);
 	bitmap_free(tcam->used_regions);
+	mutex_destroy(&tcam->lock);
 }
 
 int mlxsw_sp_acl_tcam_priority_get(struct mlxsw_sp *mlxsw_sp,
Signature · 1 line
2.39.0
author maintainer
#5 lore raw

PATCH 4/6 mlxsw: spectrum_acl_tcam: Reorder functions to avoid forward declarations

In reply to Petr Machata

From: Ido Schimmel <[email protected]>

Move the initialization and de-initialization code further below in order to avoid forward declarations in the next patch. No functional changes.

---
 .../mellanox/mlxsw/spectrum_acl_tcam.c        | 130 +++++++++---------
 1 file changed, 65 insertions(+), 65 deletions(-)
drivers/net/ethernet/mellanox/mlxsw/spectrum_acl_tcam.c +65 -65
index 2445957355cd..dbc2f834f859 100644
@@ -29,71 +29,6 @@ size_t mlxsw_sp_acl_tcam_priv_size(struct mlxsw_sp *mlxsw_sp)
 #define MLXSW_SP_ACL_TCAM_VREGION_REHASH_INTRVL_MIN 3000 /* ms */
 #define MLXSW_SP_ACL_TCAM_VREGION_REHASH_CREDITS 100 /* number of entries */
 
-int mlxsw_sp_acl_tcam_init(struct mlxsw_sp *mlxsw_sp,
-			   struct mlxsw_sp_acl_tcam *tcam)
-{
-	const struct mlxsw_sp_acl_tcam_ops *ops = mlxsw_sp->acl_tcam_ops;
-	u64 max_tcam_regions;
-	u64 max_regions;
-	u64 max_groups;
-	int err;
-
-	mutex_init(&tcam->lock);
-	tcam->vregion_rehash_intrvl =
-			MLXSW_SP_ACL_TCAM_VREGION_REHASH_INTRVL_DFLT;
-	INIT_LIST_HEAD(&tcam->vregion_list);
-
-	max_tcam_regions = MLXSW_CORE_RES_GET(mlxsw_sp->core,
-					      ACL_MAX_TCAM_REGIONS);
-	max_regions = MLXSW_CORE_RES_GET(mlxsw_sp->core, ACL_MAX_REGIONS);
-
-	/* Use 1:1 mapping between ACL region and TCAM region */
-	if (max_tcam_regions < max_regions)
-		max_regions = max_tcam_regions;
-
-	tcam->used_regions = bitmap_zalloc(max_regions, GFP_KERNEL);
-	if (!tcam->used_regions) {
-		err = -ENOMEM;
-		goto err_alloc_used_regions;
-	}
-	tcam->max_regions = max_regions;
-
-	max_groups = MLXSW_CORE_RES_GET(mlxsw_sp->core, ACL_MAX_GROUPS);
-	tcam->used_groups = bitmap_zalloc(max_groups, GFP_KERNEL);
-	if (!tcam->used_groups) {
-		err = -ENOMEM;
-		goto err_alloc_used_groups;
-	}
-	tcam->max_groups = max_groups;
-	tcam->max_group_size = MLXSW_CORE_RES_GET(mlxsw_sp->core,
-						 ACL_MAX_GROUP_SIZE);
-
-	err = ops->init(mlxsw_sp, tcam->priv, tcam);
-	if (err)
-		goto err_tcam_init;
-
-	return 0;
-
-err_tcam_init:
-	bitmap_free(tcam->used_groups);
-err_alloc_used_groups:
-	bitmap_free(tcam->used_regions);
-err_alloc_used_regions:
-	mutex_destroy(&tcam->lock);
-	return err;
-}
-
-void mlxsw_sp_acl_tcam_fini(struct mlxsw_sp *mlxsw_sp,
-			    struct mlxsw_sp_acl_tcam *tcam)
-{
-	const struct mlxsw_sp_acl_tcam_ops *ops = mlxsw_sp->acl_tcam_ops;
-
-	ops->fini(mlxsw_sp, tcam->priv);
-	bitmap_free(tcam->used_groups);
-	bitmap_free(tcam->used_regions);
-	mutex_destroy(&tcam->lock);
-}
-
 int mlxsw_sp_acl_tcam_priority_get(struct mlxsw_sp *mlxsw_sp,
 				   struct mlxsw_sp_acl_rule_info *rulei,
 				   u32 *priority, bool fillup_priority)
@@ -1546,6 +1481,71 @@ mlxsw_sp_acl_tcam_vregion_rehash(struct mlxsw_sp *mlxsw_sp,
 		mlxsw_sp_acl_tcam_vregion_rehash_end(mlxsw_sp, vregion, ctx);
 }
 
+int mlxsw_sp_acl_tcam_init(struct mlxsw_sp *mlxsw_sp,
+			   struct mlxsw_sp_acl_tcam *tcam)
+{
+	const struct mlxsw_sp_acl_tcam_ops *ops = mlxsw_sp->acl_tcam_ops;
+	u64 max_tcam_regions;
+	u64 max_regions;
+	u64 max_groups;
+	int err;
+
+	mutex_init(&tcam->lock);
+	tcam->vregion_rehash_intrvl =
+			MLXSW_SP_ACL_TCAM_VREGION_REHASH_INTRVL_DFLT;
+	INIT_LIST_HEAD(&tcam->vregion_list);
+
+	max_tcam_regions = MLXSW_CORE_RES_GET(mlxsw_sp->core,
+					      ACL_MAX_TCAM_REGIONS);
+	max_regions = MLXSW_CORE_RES_GET(mlxsw_sp->core, ACL_MAX_REGIONS);
+
+	/* Use 1:1 mapping between ACL region and TCAM region */
+	if (max_tcam_regions < max_regions)
+		max_regions = max_tcam_regions;
+
+	tcam->used_regions = bitmap_zalloc(max_regions, GFP_KERNEL);
+	if (!tcam->used_regions) {
+		err = -ENOMEM;
+		goto err_alloc_used_regions;
+	}
+	tcam->max_regions = max_regions;
+
+	max_groups = MLXSW_CORE_RES_GET(mlxsw_sp->core, ACL_MAX_GROUPS);
+	tcam->used_groups = bitmap_zalloc(max_groups, GFP_KERNEL);
+	if (!tcam->used_groups) {
+		err = -ENOMEM;
+		goto err_alloc_used_groups;
+	}
+	tcam->max_groups = max_groups;
+	tcam->max_group_size = MLXSW_CORE_RES_GET(mlxsw_sp->core,
+						  ACL_MAX_GROUP_SIZE);
+
+	err = ops->init(mlxsw_sp, tcam->priv, tcam);
+	if (err)
+		goto err_tcam_init;
+
+	return 0;
+
+err_tcam_init:
+	bitmap_free(tcam->used_groups);
+err_alloc_used_groups:
+	bitmap_free(tcam->used_regions);
+err_alloc_used_regions:
+	mutex_destroy(&tcam->lock);
+	return err;
+}
+
+void mlxsw_sp_acl_tcam_fini(struct mlxsw_sp *mlxsw_sp,
+			    struct mlxsw_sp_acl_tcam *tcam)
+{
+	const struct mlxsw_sp_acl_tcam_ops *ops = mlxsw_sp->acl_tcam_ops;
+
+	ops->fini(mlxsw_sp, tcam->priv);
+	bitmap_free(tcam->used_groups);
+	bitmap_free(tcam->used_regions);
+	mutex_destroy(&tcam->lock);
+}
+
 static const enum mlxsw_afk_element mlxsw_sp_acl_tcam_pattern_ipv4[] = {
 	MLXSW_AFK_ELEMENT_SRC_SYS_PORT,
 	MLXSW_AFK_ELEMENT_DMAC_32_47,
Signature · 1 line
2.39.0
author maintainer
#6 lore raw

PATCH 5/6 mlxsw: spectrum_acl_tcam: Move devlink param to TCAM code

In reply to Petr Machata

From: Ido Schimmel <[email protected]>

Cited commit added 'DEVLINK_CMD_PARAM_DEL' notifications whenever the network namespace of the devlink instance is changed. Specifically, the notifications are generated after calling reload_down(), but before calling reload_up(). At this stage, the data structures accessed while reading the value of the "acl_region_rehash_interval" devlink parameter are uninitialized, resulting in a use-after-free [1].

Fix by moving the registration and unregistration of the devlink parameter to the TCAM code where it is actually used. This means that the parameter is unregistered during reload_down() and then re-registered during reload_up(), avoiding the use-after-free between these two operations.

Reproducer:

 # ip netns add test123
 # devlink dev reload pci/0000:06:00.0 netns test123

[1]

BUG: KASAN: use-after-free in mlxsw_sp_acl_tcam_vregion_rehash_intrvl_get+0xb2/0xd0
Read of size 4 at addr ffff888162fd37d8 by task devlink/1323
[...]
Call Trace:
 <TASK>
 dump_stack_lvl+0x95/0xbd
 print_report+0x181/0x4a1
 kasan_report+0xdb/0x200
 mlxsw_sp_acl_tcam_vregion_rehash_intrvl_get+0xb2/0xd0
 mlxsw_sp_params_acl_region_rehash_intrvl_get+0x32/0x80
 devlink_nl_param_fill.constprop.0+0x29a/0x11e0
 devlink_param_notify.constprop.0+0xb9/0x250
 devlink_notify_unregister+0xbc/0x470
 devlink_reload+0x1aa/0x440
 devlink_nl_cmd_reload+0x559/0x11b0
 genl_family_rcv_msg_doit.isra.0+0x1f8/0x2e0
 genl_rcv_msg+0x558/0x7f0
 netlink_rcv_skb+0x170/0x440
 genl_rcv+0x2d/0x40
 netlink_unicast+0x53f/0x810
 netlink_sendmsg+0x961/0xe80
 __sys_sendto+0x2a4/0x420
 __x64_sys_sendto+0xe5/0x1c0
 do_syscall_64+0x38/0x80
 entry_SYSCALL_64_after_hwframe+0x63/0xcd
Fixes 7d7e9169a3ec ("devlink: move devlink reload notifications back in between _down() and _up() calls")
---
 drivers/net/ethernet/mellanox/mlxsw/core.c    |  19 +--
 drivers/net/ethernet/mellanox/mlxsw/core.h    |   2 -
 .../net/ethernet/mellanox/mlxsw/spectrum.c    |  52 --------
 .../net/ethernet/mellanox/mlxsw/spectrum.h    |   3 +-
 .../ethernet/mellanox/mlxsw/spectrum_acl.c    |  21 +---
 .../mellanox/mlxsw/spectrum_acl_tcam.c        | 118 ++++++++++++------
 .../mellanox/mlxsw/spectrum_acl_tcam.h        |   5 -
 7 files changed, 90 insertions(+), 130 deletions(-)
drivers/net/ethernet/mellanox/mlxsw/core.c +1 -18
index 42422a106433..a08aec243ad6 100644
@@ -1750,29 +1750,12 @@ static const struct devlink_ops mlxsw_devlink_ops = {
 
 static int mlxsw_core_params_register(struct mlxsw_core *mlxsw_core)
 {
-	int err;
-
-	err = mlxsw_core_fw_params_register(mlxsw_core);
-	if (err)
-		return err;
-
-	if (mlxsw_core->driver->params_register) {
-		err = mlxsw_core->driver->params_register(mlxsw_core);
-		if (err)
-			goto err_params_register;
-	}
-	return 0;
-
-err_params_register:
-	mlxsw_core_fw_params_unregister(mlxsw_core);
-	return err;
+	return mlxsw_core_fw_params_register(mlxsw_core);
 }
 
 static void mlxsw_core_params_unregister(struct mlxsw_core *mlxsw_core)
 {
 	mlxsw_core_fw_params_unregister(mlxsw_core);
-	if (mlxsw_core->driver->params_register)
-		mlxsw_core->driver->params_unregister(mlxsw_core);
 }
 
 struct mlxsw_core_health_event {
drivers/net/ethernet/mellanox/mlxsw/core.h +0 -2
index a77cb0be7108..e5474d3e34db 100644
@@ -421,8 +421,6 @@ struct mlxsw_driver {
 			     const struct mlxsw_config_profile *profile,
 			     u64 *p_single_size, u64 *p_double_size,
 			     u64 *p_linear_size);
-	int (*params_register)(struct mlxsw_core *mlxsw_core);
-	void (*params_unregister)(struct mlxsw_core *mlxsw_core);
 
 	/* Notify a driver that a timestamped packet was transmitted. Driver
 	 * is responsible for freeing the passed-in SKB.
drivers/net/ethernet/mellanox/mlxsw/spectrum.c +0 -52

76 lines not on this page · raw message

drivers/net/ethernet/mellanox/mlxsw/spectrum.h +1 -2
index bbc73324451d..4c22f8004514 100644
@@ -973,6 +973,7 @@ enum mlxsw_sp_acl_profile {
 };
 
 struct mlxsw_afk *mlxsw_sp_acl_afk(struct mlxsw_sp_acl *acl);
+struct mlxsw_sp_acl_tcam *mlxsw_sp_acl_to_tcam(struct mlxsw_sp_acl *acl);
 
 int mlxsw_sp_acl_ruleset_bind(struct mlxsw_sp *mlxsw_sp,
 			      struct mlxsw_sp_flow_block *block,
@@ -1096,8 +1097,6 @@ mlxsw_sp_acl_act_cookie_lookup(struct mlxsw_sp *mlxsw_sp, u32 cookie_index)
 
 int mlxsw_sp_acl_init(struct mlxsw_sp *mlxsw_sp);
 void mlxsw_sp_acl_fini(struct mlxsw_sp *mlxsw_sp);
-u32 mlxsw_sp_acl_region_rehash_intrvl_get(struct mlxsw_sp *mlxsw_sp);
-int mlxsw_sp_acl_region_rehash_intrvl_set(struct mlxsw_sp *mlxsw_sp, u32 val);
 
 struct mlxsw_sp_acl_mangle_action;
 
drivers/net/ethernet/mellanox/mlxsw/spectrum_acl.c +5 -16
index 6c5af018546f..0423ac262d89 100644
@@ -40,6 +40,11 @@ struct mlxsw_afk *mlxsw_sp_acl_afk(struct mlxsw_sp_acl *acl)
 	return acl->afk;
 }
 
+struct mlxsw_sp_acl_tcam *mlxsw_sp_acl_to_tcam(struct mlxsw_sp_acl *acl)
+{
+	return &acl->tcam;
+}
+
 struct mlxsw_sp_acl_ruleset_ht_key {
 	struct mlxsw_sp_flow_block *block;
 	u32 chain_index;
@@ -1099,22 +1104,6 @@ void mlxsw_sp_acl_fini(struct mlxsw_sp *mlxsw_sp)
 	kfree(acl);
 }
 
-u32 mlxsw_sp_acl_region_rehash_intrvl_get(struct mlxsw_sp *mlxsw_sp)
-{
-	struct mlxsw_sp_acl *acl = mlxsw_sp->acl;
-
-	return mlxsw_sp_acl_tcam_vregion_rehash_intrvl_get(mlxsw_sp,
-							   &acl->tcam);
-}
-
-int mlxsw_sp_acl_region_rehash_intrvl_set(struct mlxsw_sp *mlxsw_sp, u32 val)
-{
-	struct mlxsw_sp_acl *acl = mlxsw_sp->acl;
-
-	return mlxsw_sp_acl_tcam_vregion_rehash_intrvl_set(mlxsw_sp,
-							   &acl->tcam, val);
-}
-
 struct mlxsw_sp_acl_rulei_ops mlxsw_sp1_acl_rulei_ops = {
 	.act_mangle_field = mlxsw_sp1_acl_rulei_act_mangle_field,
 };
drivers/net/ethernet/mellanox/mlxsw/spectrum_acl_tcam.c +83 -35

154 lines not on this page · raw message

drivers/net/ethernet/mellanox/mlxsw/spectrum_acl_tcam.h +0 -5
index edbbc89e7a71..462bf448497d 100644
@@ -29,11 +29,6 @@ int mlxsw_sp_acl_tcam_init(struct mlxsw_sp *mlxsw_sp,
 			   struct mlxsw_sp_acl_tcam *tcam);
 void mlxsw_sp_acl_tcam_fini(struct mlxsw_sp *mlxsw_sp,
 			    struct mlxsw_sp_acl_tcam *tcam);
-u32 mlxsw_sp_acl_tcam_vregion_rehash_intrvl_get(struct mlxsw_sp *mlxsw_sp,
-						struct mlxsw_sp_acl_tcam *tcam);
-int mlxsw_sp_acl_tcam_vregion_rehash_intrvl_set(struct mlxsw_sp *mlxsw_sp,
-						struct mlxsw_sp_acl_tcam *tcam,
-						u32 val);
 int mlxsw_sp_acl_tcam_priority_get(struct mlxsw_sp *mlxsw_sp,
 				   struct mlxsw_sp_acl_rule_info *rulei,
 				   u32 *priority, bool fillup_priority);
Signature · 1 line
2.39.0
author maintainer
#7 lore raw

PATCH 6/6 mlxsw: core: Register devlink instance before sub-objects

In reply to Petr Machata

From: Ido Schimmel <[email protected]>

Recent changes made it possible to register the devlink instance before its sub-objects and under the instance lock. Among other things, it allows us to avoid warnings such as this one [1]. The warning is generated because a buggy firmware is generating a health event during driver initialization, before the devlink instance is registered.

Move the registration of the devlink instance to the beginning of the initialization flow to avoid such problems.

A similar change was implemented in netdevsim in commit 82a3aef2e6af ("netdevsim: move devlink registration under the instance lock").

[1]

WARNING: CPU: 3 PID: 49 at net/devlink/leftover.c:7509 devlink_recover_notify.constprop.0+0xaf/0xc0
[...]
Call Trace:
 <TASK>
 devlink_health_report+0x45/0x1d0
 mlxsw_core_health_event_work+0x24/0x30 [mlxsw_core]
 process_one_work+0x1db/0x390
 worker_thread+0x49/0x3b0
 kthread+0xe5/0x110
 ret_from_fork+0x1f/0x30
 </TASK>
---
 drivers/net/ethernet/mellanox/mlxsw/core.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
drivers/net/ethernet/mellanox/mlxsw/core.c +6 -6
index a08aec243ad6..22db0bb15c45 100644
@@ -2184,6 +2184,7 @@ __mlxsw_core_bus_device_register(const struct mlxsw_bus_info *mlxsw_bus_info,
 			goto err_devlink_alloc;
 		}
 		devl_lock(devlink);
+		devl_register(devlink);
 	}
 
 	mlxsw_core = devlink_priv(devlink);
@@ -2267,10 +2268,8 @@ __mlxsw_core_bus_device_register(const struct mlxsw_bus_info *mlxsw_bus_info,
 			goto err_driver_init;
 	}
 
-	if (!reload) {
+	if (!reload)
 		devl_unlock(devlink);
-		devlink_register(devlink);
-	}
 	return 0;
 
 err_driver_init:
@@ -2302,6 +2301,7 @@ __mlxsw_core_bus_device_register(const struct mlxsw_bus_info *mlxsw_bus_info,
 err_bus_init:
 	mlxsw_core_irq_event_handler_fini(mlxsw_core);
 	if (!reload) {
+		devl_unregister(devlink);
 		devl_unlock(devlink);
 		devlink_free(devlink);
 	}
@@ -2340,10 +2340,8 @@ void mlxsw_core_bus_device_unregister(struct mlxsw_core *mlxsw_core,
 {
 	struct devlink *devlink = priv_to_devlink(mlxsw_core);
 
-	if (!reload) {
-		devlink_unregister(devlink);
+	if (!reload)
 		devl_lock(devlink);
-	}
 
 	if (devlink_is_reload_failed(devlink)) {
 		if (!reload)
@@ -2372,6 +2370,7 @@ void mlxsw_core_bus_device_unregister(struct mlxsw_core *mlxsw_core,
 	mlxsw_core->bus->fini(mlxsw_core->bus_priv);
 	mlxsw_core_irq_event_handler_fini(mlxsw_core);
 	if (!reload) {
+		devl_unregister(devlink);
 		devl_unlock(devlink);
 		devlink_free(devlink);
 	}
@@ -2381,6 +2380,7 @@ void mlxsw_core_bus_device_unregister(struct mlxsw_core *mlxsw_core,
 reload_fail_deinit:
 	mlxsw_core_params_unregister(mlxsw_core);
 	devl_resources_unregister(devlink);
+	devl_unregister(devlink);
 	devl_unlock(devlink);
 	devlink_free(devlink);
 }
Signature · 1 line
2.39.0
Reviewed-by on the v1 series
#8 lore raw
In reply to Petr Machata
Quoted 5 lines · Petr Machata

This patchset adjusts mlxsw to recent devlink changes in net-next.

Patch #1 removes a devl_param_driverinit_value_set() call that was unnecessary, but now additionally triggers a WARN_ON.

Quoted 9 lines

Patches #2-#4 are non-functional preparations for the following patches.

Patch #5 fixes a use-after-free that is triggered while changing network namespaces.

Patch #6 makes mlxsw consistent with netdevsim by having mlxsw register its devlink instance before its sub-objects. It helps us avoid a warning described in the commit message.

The whole series makes sense to me :)

Quoted 21 lines

Danielle Ratson (1):
mlxsw: spectrum: Remove pointless call to
devlink_param_driverinit_value_set()

Ido Schimmel (5):
mlxsw: spectrum_acl_tcam: Add missing mutex_destroy()
mlxsw: spectrum_acl_tcam: Make fini symmetric to init
mlxsw: spectrum_acl_tcam: Reorder functions to avoid forward
declarations
mlxsw: spectrum_acl_tcam: Move devlink param to TCAM code
mlxsw: core: Register devlink instance before sub-objects

 drivers/net/ethernet/mellanox/mlxsw/core.c    |  31 +--
 drivers/net/ethernet/mellanox/mlxsw/core.h    |   2 -
 .../net/ethernet/mellanox/mlxsw/spectrum.c    |  62 -----
 .../net/ethernet/mellanox/mlxsw/spectrum.h    |   3 +-
 .../ethernet/mellanox/mlxsw/spectrum_acl.c    |  21 +-
 .../mellanox/mlxsw/spectrum_acl_tcam.c        | 244 +++++++++++-------
 .../mellanox/mlxsw/spectrum_acl_tcam.h        |   5 -
 7 files changed, 161 insertions(+), 207 deletions(-)
#9 lore raw
In reply to Petr Machata
This series was applied to netdev/net-next.git (master) by Jakub Kicinski <[email protected]>:

Hello:

This series was applied to netdev/net-next.git (master) by Jakub Kicinski <[email protected]>:

Quoted 8 lines · On Mon, 6 Feb 2023 16:39:17 +0100 you wrote:

This patchset adjusts mlxsw to recent devlink changes in net-next.

Patch #1 removes a devl_param_driverinit_value_set() call that was unnecessary, but now additionally triggers a WARN_ON.

Patches #2-#4 are non-functional preparations for the following patches.

[...]

Here is the summary with links:
- [net-next,1/6] mlxsw: spectrum: Remove pointless call to devlink_param_driverinit_value_set()

    https://git.kernel.org/netdev/net-next/c/8b50ac29854a
  - [net-next,2/6] mlxsw: spectrum_acl_tcam: Add missing mutex_destroy()
    https://git.kernel.org/netdev/net-next/c/65823e07b1e4
  - [net-next,3/6] mlxsw: spectrum_acl_tcam: Make fini symmetric to init
    https://git.kernel.org/netdev/net-next/c/61fe3b9102ac
  - [net-next,4/6] mlxsw: spectrum_acl_tcam: Reorder functions to avoid forward declarations
    https://git.kernel.org/netdev/net-next/c/194ab9476089
  - [net-next,5/6] mlxsw: spectrum_acl_tcam: Move devlink param to TCAM code
    https://git.kernel.org/netdev/net-next/c/74cbc3c03c82
  - [net-next,6/6] mlxsw: core: Register devlink instance before sub-objects
    https://git.kernel.org/netdev/net-next/c/9d9a90cda415

You are awesome, thank you!

Signature · 2 lines
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html