mlxsw: core_thermal: Small cleanups

Ido Schimmel says:

Clean up various issues which I noticed while addressing feedback on a different patchset.

Ido Schimmel (10):
mlxsw: core_thermal: Call thermal_zone_device_unregister()
unconditionally
mlxsw: core_thermal: Remove unnecessary check
mlxsw: core_thermal: Remove another unnecessary check
mlxsw: core_thermal: Fold two loops into one
mlxsw: core_thermal: Remove unused arguments
mlxsw: core_thermal: Make mlxsw_thermal_module_{init, fini} symmetric
mlxsw: core_thermal: Simplify rollback
mlxsw: core_thermal: Remove unnecessary checks
mlxsw: core_thermal: Remove unnecessary assignments
mlxsw: core_thermal: Fix -Wformat-truncation warning

 .../ethernet/mellanox/mlxsw/core_thermal.c    | 43 ++++++-------------
 1 file changed, 12 insertions(+), 31 deletions(-)

2.45.0

From: Ido Schimmel <[email protected]>

The function returns immediately if the thermal zone pointer is NULL so there is no need to check it before calling the function.

Remove the check.

---
 drivers/net/ethernet/mellanox/mlxsw/core_thermal.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

index d61478c0c632..0b38bab4eaa8 100644
@@ -821,10 +821,7 @@ int mlxsw_thermal_init(struct mlxsw_core *core,
 err_thermal_gearboxes_init:
 	mlxsw_thermal_modules_fini(thermal, &thermal->line_cards[0]);
 err_thermal_modules_init:
-	if (thermal->tzdev) {
-		thermal_zone_device_unregister(thermal->tzdev);
-		thermal->tzdev = NULL;
-	}
+	thermal_zone_device_unregister(thermal->tzdev);
 err_thermal_zone_device_register:
 err_thermal_cooling_device_register:
 	for (i = 0; i < MLXSW_MFCR_PWMS_MAX; i++)

@@ -845,10 +842,7 @@ void mlxsw_thermal_fini(struct mlxsw_thermal *thermal)
 					     thermal);
 	mlxsw_thermal_gearboxes_fini(thermal, &thermal->line_cards[0]);
 	mlxsw_thermal_modules_fini(thermal, &thermal->line_cards[0]);
-	if (thermal->tzdev) {
-		thermal_zone_device_unregister(thermal->tzdev);
-		thermal->tzdev = NULL;
-	}
+	thermal_zone_device_unregister(thermal->tzdev);
 
 	for (i = 0; i < MLXSW_MFCR_PWMS_MAX; i++)
 		thermal_cooling_device_unregister(thermal->cdevs[i].cdev);

2.45.0

From: Ido Schimmel <[email protected]>

mlxsw_thermal_modules_init() allocates an array of modules and then calls mlxsw_thermal_module_init() to initialize each entry in the array. It is therefore impossible for mlxsw_thermal_module_init() to encounter an entry that is already initialized and has its 'parent' pointer set.

Remove the unnecessary check.

---
 drivers/net/ethernet/mellanox/mlxsw/core_thermal.c | 3 ---
 1 file changed, 3 deletions(-)

index 0b38bab4eaa8..394e4fd633ef 100644
@@ -453,9 +453,6 @@ mlxsw_thermal_module_init(struct device *dev, struct mlxsw_core *core,
 	struct mlxsw_thermal_module *module_tz;
 
 	module_tz = &area->tz_module_arr[module];
-	/* Skip if parent is already set (case of port split). */
-	if (module_tz->parent)
-		return;
 	module_tz->module = module;
 	module_tz->slot_index = area->slot_index;
 	module_tz->parent = thermal;

2.45.0

From: Ido Schimmel <[email protected]>

mlxsw_thermal_modules_init() allocates an array of modules and then initializes each entry by calling mlxsw_thermal_module_init() which among other things initializes the 'parent' pointer of the entry.

mlxsw_thermal_modules_init() then traverses over the array again, but skips over entries that do not have their 'parent' pointer set which is impossible given the above.

Therefore, remove the unnecessary check.

---
 drivers/net/ethernet/mellanox/mlxsw/core_thermal.c | 2 --
 1 file changed, 2 deletions(-)

index 394e4fd633ef..afd8fe85a94d 100644
@@ -505,8 +505,6 @@ mlxsw_thermal_modules_init(struct device *dev, struct mlxsw_core *core,
 
 	for (i = 0; i < area->tz_module_num; i++) {
 		module_tz = &area->tz_module_arr[i];
-		if (!module_tz->parent)
-			continue;
 		err = mlxsw_thermal_module_tz_init(module_tz);
 		if (err)
 			goto err_thermal_module_tz_init;

2.45.0

From: Ido Schimmel <[email protected]>

There is no need to traverse the same array twice. Do it once by folding both loops into one.

---
 drivers/net/ethernet/mellanox/mlxsw/core_thermal.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

index afd8fe85a94d..b2a4eea859d1 100644
@@ -500,10 +500,8 @@ mlxsw_thermal_modules_init(struct device *dev, struct mlxsw_core *core,
 	if (!area->tz_module_arr)
 		return -ENOMEM;
 
-	for (i = 0; i < area->tz_module_num; i++)
-		mlxsw_thermal_module_init(dev, core, thermal, area, i);
-
 	for (i = 0; i < area->tz_module_num; i++) {
+		mlxsw_thermal_module_init(dev, core, thermal, area, i);
 		module_tz = &area->tz_module_arr[i];
 		err = mlxsw_thermal_module_tz_init(module_tz);
 		if (err)

2.45.0

From: Ido Schimmel <[email protected]>

'dev' and 'core' arguments are not used by mlxsw_thermal_module_init(). Remove them.

---
 drivers/net/ethernet/mellanox/mlxsw/core_thermal.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

index b2a4eea859d1..95821e91da18 100644
@@ -446,8 +446,7 @@ static void mlxsw_thermal_module_tz_fini(struct thermal_zone_device *tzdev)
 }
 
 static void
-mlxsw_thermal_module_init(struct device *dev, struct mlxsw_core *core,
-			  struct mlxsw_thermal *thermal,
+mlxsw_thermal_module_init(struct mlxsw_thermal *thermal,
 			  struct mlxsw_thermal_area *area, u8 module)
 {
 	struct mlxsw_thermal_module *module_tz;

@@ -501,7 +500,7 @@ mlxsw_thermal_modules_init(struct device *dev, struct mlxsw_core *core,
 		return -ENOMEM;
 
 	for (i = 0; i < area->tz_module_num; i++) {
-		mlxsw_thermal_module_init(dev, core, thermal, area, i);
+		mlxsw_thermal_module_init(thermal, area, i);
 		module_tz = &area->tz_module_arr[i];
 		err = mlxsw_thermal_module_tz_init(module_tz);
 		if (err)

2.45.0

From: Ido Schimmel <[email protected]>

mlxsw_thermal_module_fini() de-initializes the module's thermal zone, but mlxsw_thermal_module_init() does not initialize it. Make both functions symmetric by moving the initialization of the module's thermal zone to mlxsw_thermal_module_init().

---
 drivers/net/ethernet/mellanox/mlxsw/core_thermal.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

index 95821e91da18..36b883a7ee60 100644
@@ -445,7 +445,7 @@ static void mlxsw_thermal_module_tz_fini(struct thermal_zone_device *tzdev)
 	thermal_zone_device_unregister(tzdev);
 }
 
-static void
+static int
 mlxsw_thermal_module_init(struct mlxsw_thermal *thermal,
 			  struct mlxsw_thermal_area *area, u8 module)
 {

@@ -461,6 +461,8 @@ mlxsw_thermal_module_init(struct mlxsw_thermal *thermal,
 	       sizeof(thermal->trips));
 	memcpy(module_tz->cooling_states, default_cooling_states,
 	       sizeof(thermal->cooling_states));
+
+	return mlxsw_thermal_module_tz_init(module_tz);
 }
 
 static void mlxsw_thermal_module_fini(struct mlxsw_thermal_module *module_tz)

@@ -477,7 +479,6 @@ mlxsw_thermal_modules_init(struct device *dev, struct mlxsw_core *core,
 			   struct mlxsw_thermal *thermal,
 			   struct mlxsw_thermal_area *area)
 {
-	struct mlxsw_thermal_module *module_tz;
 	char mgpir_pl[MLXSW_REG_MGPIR_LEN];
 	int i, err;
 

@@ -500,16 +501,14 @@ mlxsw_thermal_modules_init(struct device *dev, struct mlxsw_core *core,
 		return -ENOMEM;
 
 	for (i = 0; i < area->tz_module_num; i++) {
-		mlxsw_thermal_module_init(thermal, area, i);
-		module_tz = &area->tz_module_arr[i];
-		err = mlxsw_thermal_module_tz_init(module_tz);
+		err = mlxsw_thermal_module_init(thermal, area, i);
 		if (err)
-			goto err_thermal_module_tz_init;
+			goto err_thermal_module_init;
 	}
 
 	return 0;
 
-err_thermal_module_tz_init:
+err_thermal_module_init:
 	for (i = area->tz_module_num - 1; i >= 0; i--)
 		mlxsw_thermal_module_fini(&area->tz_module_arr[i]);
 	kfree(area->tz_module_arr);

2.45.0

From: Ido Schimmel <[email protected]>

During rollback, instead of calling mlxsw_thermal_module_fini() for all the modules, only call it for modules that were successfully initialized. This is not a bug fix since mlxsw_thermal_module_fini() first checks that the module was initialized.

---
 drivers/net/ethernet/mellanox/mlxsw/core_thermal.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 36b883a7ee60..e9bf11a38ae9 100644
@@ -509,7 +509,7 @@ mlxsw_thermal_modules_init(struct device *dev, struct mlxsw_core *core,
 	return 0;
 
 err_thermal_module_init:
-	for (i = area->tz_module_num - 1; i >= 0; i--)
+	for (i--; i >= 0; i--)
 		mlxsw_thermal_module_fini(&area->tz_module_arr[i]);
 	kfree(area->tz_module_arr);
 	return err;

2.45.0

From: Ido Schimmel <[email protected]>

mlxsw_thermal_module_fini() cannot be invoked with a thermal module which is NULL or which is not associated with a thermal zone, so remove these checks.

---
 drivers/net/ethernet/mellanox/mlxsw/core_thermal.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

index e9bf11a38ae9..cfbfabec816e 100644
@@ -467,11 +467,9 @@ mlxsw_thermal_module_init(struct mlxsw_thermal *thermal,
 
 static void mlxsw_thermal_module_fini(struct mlxsw_thermal_module *module_tz)
 {
-	if (module_tz && module_tz->tzdev) {
-		mlxsw_thermal_module_tz_fini(module_tz->tzdev);
-		module_tz->tzdev = NULL;
-		module_tz->parent = NULL;
-	}
+	mlxsw_thermal_module_tz_fini(module_tz->tzdev);
+	module_tz->tzdev = NULL;
+	module_tz->parent = NULL;
 }
 
 static int

2.45.0

From: Ido Schimmel <[email protected]>

Setting both pointers to NULL is unnecessary since the code never checks whether these pointers are NULL or not. Remove the assignments.

---
 drivers/net/ethernet/mellanox/mlxsw/core_thermal.c | 2 --
 1 file changed, 2 deletions(-)

index cfbfabec816e..269c4986ea24 100644
@@ -468,8 +468,6 @@ mlxsw_thermal_module_init(struct mlxsw_thermal *thermal,
 static void mlxsw_thermal_module_fini(struct mlxsw_thermal_module *module_tz)
 {
 	mlxsw_thermal_module_tz_fini(module_tz->tzdev);
-	module_tz->tzdev = NULL;
-	module_tz->parent = NULL;
 }
 
 static int

2.45.0

From: Ido Schimmel <[email protected]>

The name of a thermal zone device cannot be longer than 19 characters ('THERMAL_NAME_LENGTH - 1'). The format string 'mlxsw-lc%d-module%d' can exceed this limitation if the maximum number of line cards cannot be represented using a single digit and the maximum number of transceiver modules cannot be represented using two digits.

This is not the case with current systems nor future ones. Therefore, increase the size of the result buffer beyond 'THERMAL_NAME_LENGTH' and suppress the following build warning [1].

If this limitation is ever exceeded, we will know about it since the thermal core validates the thermal device's name during registration.

[1]
drivers/net/ethernet/mellanox/mlxsw/core_thermal.c: In function ‘mlxsw_thermal_modules_init.part.0’:
drivers/net/ethernet/mellanox/mlxsw/core_thermal.c:418:70: error: ‘%d’ directive output may be truncated writing between 1 and 3 bytes into a region of size between 2 and 4 [-Werror=format-truncation=]
418 | snprintf(tz_name, sizeof(tz_name), "mlxsw-lc%d-module%d",
| ^~
In function ‘mlxsw_thermal_module_tz_init’,
inlined from ‘mlxsw_thermal_module_init’ at drivers/net/ethernet/mellanox/mlxsw/core_thermal.c:465:9,
inlined from ‘mlxsw_thermal_modules_init.part.0’ at drivers/net/ethernet/mellanox/mlxsw/core_thermal.c:500:9:
drivers/net/ethernet/mellanox/mlxsw/core_thermal.c:418:52: note: directive argument in the range [1, 256]
418 | snprintf(tz_name, sizeof(tz_name), "mlxsw-lc%d-module%d",

      |                                                    ^~~~~~~~~~~~~~~~~~~~~
drivers/net/ethernet/mellanox/mlxsw/core_thermal.c:418:17: note: ‘snprintf’ output between 18 and 22 bytes into a destination of size 20
  418 |                 snprintf(tz_name, sizeof(tz_name), "mlxsw-lc%d-module%d",
      |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  419 |                          module_tz->slot_index, module_tz->module + 1);
      |                          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

---
 drivers/net/ethernet/mellanox/mlxsw/core_thermal.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 269c4986ea24..303d2ce4dc1e 100644
@@ -411,7 +411,7 @@ static const struct thermal_cooling_device_ops mlxsw_cooling_ops = {
 static int
 mlxsw_thermal_module_tz_init(struct mlxsw_thermal_module *module_tz)
 {
-	char tz_name[THERMAL_NAME_LENGTH];
+	char tz_name[40];
 	int err;
 
 	if (module_tz->slot_index)

2.45.0

The ordering could have been better since this comes from patch 6 in the same series :(

For the whole series: