Re: [PATCH v2 1/5] thermal/drivers/core: Use a char pointer for the cooling device name

On 12/03/2021 19:49, Lukasz Luba wrote:

On 3/12/21 5:03 PM, Daniel Lezcano wrote:

quoted

We want to have any kind of name for the cooling devices as we do no
longer want to rely on auto-numbering. Let's replace the cooling
device's fixed array by a char pointer to be allocated dynamically
when registering the cooling device, so we don't limit the length of
the name.

Rework the error path at the same time as we have to rollback the
allocations in case of error.

Tested with a dummy device having the name:
  "Llanfairpwllgwyngyllgogerychwyrndrobwllllantysiliogogogoch"

A village on the island of Anglesey (Wales), known to have the longest
name in Europe.

Signed-off-by: Daniel Lezcano <redacted>
---
  .../ethernet/mellanox/mlxsw/core_thermal.c    |  2 +-
  drivers/thermal/thermal_core.c                | 38 +++++++++++--------
  include/linux/thermal.h                       |  2 +-
  3 files changed, 24 insertions(+), 18 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/core_thermal.c

b/drivers/net/ethernet/mellanox/mlxsw/core_thermal.c
index bf85ce9835d7..7447c2a73cbd 100644

--- a/drivers/net/ethernet/mellanox/mlxsw/core_thermal.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/core_thermal.c

@@ -141,7 +141,7 @@ static int mlxsw_get_cooling_device_idx(struct

mlxsw_thermal *thermal,
      /* Allow mlxsw thermal zone binding to an external cooling
device */
      for (i = 0; i < ARRAY_SIZE(mlxsw_thermal_external_allowed_cdev);
i++) {
          if (strnstr(cdev->type, mlxsw_thermal_external_allowed_cdev[i],
-                sizeof(cdev->type)))
+                strlen(cdev->type)))
              return 0;
      }
  diff --git a/drivers/thermal/thermal_core.c
b/drivers/thermal/thermal_core.c
index 996c038f83a4..9ef8090eb645 100644

--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c

@@ -960,10 +960,7 @@ __thermal_cooling_device_register(struct

device_node *np,
  {
      struct thermal_cooling_device *cdev;
      struct thermal_zone_device *pos = NULL;
-    int result;
-
-    if (type && strlen(type) >= THERMAL_NAME_LENGTH)
-        return ERR_PTR(-EINVAL);
+    int ret;
        if (!ops || !ops->get_max_state || !ops->get_cur_state ||
          !ops->set_cur_state)

@@ -973,14 +970,17 @@ __thermal_cooling_device_register(struct

device_node *np,
      if (!cdev)
          return ERR_PTR(-ENOMEM);
  -    result = ida_simple_get(&thermal_cdev_ida, 0, 0, GFP_KERNEL);
-    if (result < 0) {
-        kfree(cdev);
-        return ERR_PTR(result);
+    ret = ida_simple_get(&thermal_cdev_ida, 0, 0, GFP_KERNEL);
+    if (ret < 0)
+        goto out_kfree_cdev;
+    cdev->id = ret;
+
+    cdev->type = kstrdup(type ? type : "", GFP_KERNEL);
+    if (!cdev->type) {
+        ret = -ENOMEM;

Since we haven't called the device_register() yet, I would call here:
kfree(cdev);
and then jump

I'm not sure to understand, we have to remove the ida, no ?

Other than that, LGTM

Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>

Regards,
Lukasz

-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog