Fix: Bind Cooling Device Weight

The Problem:

In the current code, the weight of the cooling device, which is read as contribution from the device tree in of-thermal.c as:

        ret = of_property_read_u32(np, "contribution", &prop);
        if (ret == 0)
                __tbp->usage = prop;

This is only stored in the private devicedata as:

        ((__thernal_zone *)cdev->devdata)->__tbp->usage

As of-thermal.c specifies its "bind" operation and does not populate tzd->tzp->tbp, this causes an erroneous access in the fair-share governor when it tries to access the weight:

instance->target = get_target_state(tz, cdev,

                                        tzp->tbp[i].weight,

cur_trip_level);

The Proposed solution:

The proposed solution has the following changes:

1. Passing the weight as an argument to thermal_zone_bind_cooling_device

2. Storing the weight in the thermal_instance data structure created in
the thermal_zone_bind_cooling_device function

3. Changing the accesses in the governor accordingly.

I am not sure of what default value should be assigned to the weight
member in the instance data structure and would like to leave this open to discussion.

Suggested Patch (Not Signed off):

drivers/thermal/db8500_thermal.c +0 -0

index 1e3b3bf9f993..e3ccc2218eb3 100644

drivers/thermal/db8500_thermal.c +0 -0

         dev_info(&cdev->device, "%s bind to %d: %d-%s\n", cdev->type,

             i, ret, ret ? "fail" : "succeed");

drivers/thermal/fair_share.c +1 -1

index 6e0a3fbfae86..c3b25187b467 100644
@@ -109,7 +109,7 @@ static int fair_share_throttle(struct
 thermal_zone_device *tz, int trip)
             continue;
 
         instance->target = get_target_state(tz, cdev,
-                    tzp->tbp[i].weight, cur_trip_level);
+                    instance->weight, cur_trip_level);
 
         instance->cdev->updated = false;

         thermal_cdev_update(cdev);

drivers/thermal/imx_thermal.c +2 -1

index 5a1f1070b702..74d0c9951eef 100644
@@ -307,7 +307,8 @@ static int imx_bind(struct thermal_zone_device *tz,
 
     ret = thermal_zone_bind_cooling_device(tz, IMX_TRIP_PASSIVE, cdev,
                            THERMAL_NO_LIMIT,
-                           THERMAL_NO_LIMIT);
+                           THERMAL_NO_LIMIT
+                           THERMAL_WEIGHT_DEFAULT);
     if (ret) {
         dev_err(&tz->device,
             "binding zone %s with cdev %s failed:%d\n",

drivers/thermal/of-thermal.c +2 -1

index e032b9bf4085..cf4036cbb671 100644
@@ -157,7 +157,8 @@ static int of_thermal_bind(struct
 thermal_zone_device *thermal,
             ret = thermal_zone_bind_cooling_device(thermal,
                         tbp->trip_id, cdev,
                         tbp->max,
-                        tbp->min);
+                        tbp->min,
+                        tbp->usage);
             if (ret)
                 return ret;

         }

drivers/thermal/samsung/exynos_thermal_common.c +1 -1

index b6be572704a4..d653798b519f 100644
@@ -163,7 +163,7 @@ static int exynos_bind(struct thermal_zone_device
 *thermal,
         case MONITOR_ZONE:
         case WARN_ZONE:
             if (thermal_zone_bind_cooling_device(thermal, i, cdev,
-                                level, 0)) {
+                                level, 0, THERMAL_WEIGHT_DEFAULT)) {
                 dev_err(data->dev,
                     "error unbinding cdev inst=%d\n", i);

                 ret = -EINVAL;

drivers/thermal/thermal_core.c +4 -2

index 4921e084c20b..199866864ef1 100644
@@ -277,7 +277,8 @@ static void print_bind_err_msg(struct
 thermal_zone_device *tz,
 
 static void __bind(struct thermal_zone_device *tz, int mask,
             struct thermal_cooling_device *cdev,
-            unsigned long *limits)
+            unsigned long *limits,
+            unsigned int weight)
 {
     int i, ret;

@@ -292,7 +293,8 @@ static void __bind(struct thermal_zone_device *tz,
 int mask,
                 upper = limits[i * 2 + 1];
             }
             ret = thermal_zone_bind_cooling_device(tz, i, cdev,
-                                   upper, lower);
+                                   upper, lower,
+                                   weight);
             if (ret)
                 print_bind_err_msg(tz, cdev, ret);

         }

drivers/thermal/thermal_core.c +6 -3

@@ -339,7 +341,8 @@ static void bind_cdev(struct thermal_cooling_device
 *cdev)
                 continue;
             tzp->tbp[i].cdev = cdev;
             __bind(pos, tzp->tbp[i].trip_mask, cdev,
-                   tzp->tbp[i].binding_limits);
+                   tzp->tbp[i].binding_limits,
+                   tzp->tbp[i].weight);
         }
     }

@@ -378,7 +381,8 @@ static void bind_tz(struct thermal_zone_device *tz)
                 continue;
             tzp->tbp[i].cdev = pos;
             __bind(tz, tzp->tbp[i].trip_mask, pos,
-                   tzp->tbp[i].binding_limits);
+                   tzp->tbp[i].binding_limits,
+                   tzp->tbp[i].weight);
         }
     }
 exit:

@@ -770,7 +774,8 @@ passive_store(struct device *dev, struct
 device_attribute *attr,
                 thermal_zone_bind_cooling_device(tz,
                         THERMAL_TRIPS_NONE, cdev,
                         THERMAL_NO_LIMIT,
-                        THERMAL_NO_LIMIT);
+                        THERMAL_NO_LIMIT,
+                        THERMAL_WEIGHT_DEFAULT);
         }
         mutex_unlock(&thermal_list_lock);

         if (!tz->passive_delay)

drivers/thermal/thermal_core.c +3 -1

@@ -1009,7 +1014,9 @@ thermal_cooling_device_trip_point_show(struct
 device *dev,
  * @lower:    the Minimum cooling state can be used for this trip point.
  *        THERMAL_NO_LIMIT means no lower limit,
  *        and the cooling device can be in cooling state 0.
- *
+ * @weight:    The weight of the cooling device to be bound to the
+        thermal zone. THERMAL_WEIGHT_DEFAULT for default value
+
  * This interface function bind a thermal cooling device to the certain

trip
* point of a thermal zone device.
* This function is usually called in the thermal zone device .bind
callback.

drivers/thermal/thermal_core.c +2 -1

@@ -1019,7 +1026,8 @@ thermal_cooling_device_trip_point_show(struct
 device *dev,
 int thermal_zone_bind_cooling_device(struct thermal_zone_device *tz,
                      int trip,
                      struct thermal_cooling_device *cdev,
-                     unsigned long upper, unsigned long lower)
+                     unsigned long upper, unsigned long lower,
+                     unsigned int weight)
 {
     struct thermal_instance *dev;

     struct thermal_instance *pos;

drivers/thermal/thermal_core.c +1 -0

@@ -1062,6 +1070,7 @@ int thermal_zone_bind_cooling_device(struct
 thermal_zone_device *tz,
     dev->upper = upper;
     dev->lower = lower;
     dev->target = THERMAL_NO_TARGET;
+    dev->weight = weight;
 
     result = get_idr(&tz->idr, &tz->lock, &dev->id);

if (result)

drivers/thermal/thermal_core.h +1 -0

index b907be823527..1c61abe7801f 100644
@@ -48,6 +48,7 @@ struct thermal_instance {
     struct device_attribute attr;
     struct list_head tz_node; /* node in tz->thermal_instances */
     struct list_head cdev_node; /* node in cdev->thermal_instances */
+    unsigned int weight; /* The weight of the cooling device */
 };
 
 int thermal_register_governor(struct thermal_governor *);

drivers/thermal/ti-soc-thermal/ti-thermal-common.c +2 -1

index 9eec26dc0448..772549ec9bd8 100644
@@ -147,7 +147,8 @@ static int ti_thermal_bind(struct
 thermal_zone_device *thermal,
     return thermal_zone_bind_cooling_device(thermal, 0, cdev,
     /* bind with min and max states defined by cpu_cooling */
                         THERMAL_NO_LIMIT,
-                        THERMAL_NO_LIMIT);
+                        THERMAL_NO_LIMIT,
+                        THERMAL_WEIGHT_DEFAULT);
 }
 

/* Unbind callback functions for thermal zone */

include/linux/thermal.h +5 -1

index fc4fdcbbdecc..674795ba0900 100644
@@ -40,6 +40,9 @@
 /* No upper/lower limit requirement */
 #define THERMAL_NO_LIMIT    ((u32)~0)
 
+/* Default weight of a bound cooling device */
+#define THERMAL_WEIGHT_DEFAULT 1
+
 /* Unit conversion macros */
 #define KELVIN_TO_CELSIUS(t)    (long)(((long)t-2732 >= 0) ?    \
                 ((long)t-2732+5)/10 : ((long)t-2732-5)/10)

@@ -375,7 +378,8 @@ void thermal_zone_device_unregister(struct
 thermal_zone_device *);
 
 int thermal_zone_bind_cooling_device(struct thermal_zone_device *, int,
                      struct thermal_cooling_device *,
-                     unsigned long, unsigned long);
+                     unsigned long, unsigned long,
+                     unsigned int);
 int thermal_zone_unbind_cooling_device(struct thermal_zone_device *, int,
                        struct thermal_cooling_device *);

void thermal_zone_device_update(struct thermal_zone_device *);

Quoted 25 lines · Kapileshwar Singh

The Problem:

In the current code, the weight of the cooling device, which is read as contribution from the device tree in of-thermal.c as:

        ret = of_property_read_u32(np, "contribution", &prop);
        if (ret == 0)
                __tbp->usage = prop;

This is only stored in the private devicedata as:

        ((__thernal_zone *)cdev->devdata)->__tbp->usage

As of-thermal.c specifies its "bind" operation and does not populate tzd->tzp->tbp, this causes an erroneous access in the fair-share governor when it tries to access the weight:

instance->target = get_target_state(tz, cdev,

                                        tzp->tbp[i].weight,

cur_trip_level);

The Proposed solution:

Quoted 8 lines

The proposed solution has the following changes:

1. Passing the weight as an argument to thermal_zone_bind_cooling_device

2. Storing the weight in the thermal_instance data structure created in
the thermal_zone_bind_cooling_device function

3. Changing the accesses in the governor accordingly.

Shouldn't we simply:
1. In of-thermal, populate tzd->tzp->tbp by passing a tbp with correct tbp's in the registration call:

	zone = thermal_zone_device_register(child->name, tz->ntrips,
						0,
						tz,
						ops,
/* This guy needs to be filled properly */	tzp,
						tz->passive_delay,
						tz->polling_delay);

2. Add a proper check in the governor to avoid accessing thermal zones with missing data.

I know there is a check in place:

        if (!tz->tzp || !tz->tzp->tbp)
	          return -EINVAL;

which based in your description seams to be failing. Here, I need more info from your side describing what exactly you are seeing. Can you please post the kernel log when the failure happens? Does it output a kernel segfault or is the governor simply not working?

I would expect, with the current code, the governor be silent and non-functional, which needs to be fixed too.

Quoted 12 lines

I am not sure of what default value should be assigned to the weight
member in the instance data structure and would like to leave this open to discussion.

Suggested Patch (Not Signed off):

drivers/thermal/db8500_thermal.c +0 -0

index 1e3b3bf9f993..e3ccc2218eb3 100644

drivers/thermal/db8500_thermal.c static int db8500_cdev_bind(struct thermal_zone_device · line 81

Quoted 7 lines

drivers/thermal/db8500_thermal.c +1 -1

@@ -76,7 +76,7 @@ static int db8500_cdev_bind(struct thermal_zone_device
 *thermal,
         upper = lower = i > max_state ? max_state : i;
 
         ret = thermal_zone_bind_cooling_device(thermal, i, cdev,
-            upper, lower);
+            upper, lower, THERMAL_WEIGHT_DEFAULT);
 

I think spreading such parameter, which is known to be part of tbp, is not a good thing to do. Can we avoid that?

Cheers, Eduardo

Quoted 207 lines

drivers/thermal/db8500_thermal.c +0 -0

         dev_info(&cdev->device, "%s bind to %d: %d-%s\n", cdev->type,

             i, ret, ret ? "fail" : "succeed");

drivers/thermal/fair_share.c +1 -1

index 6e0a3fbfae86..c3b25187b467 100644
@@ -109,7 +109,7 @@ static int fair_share_throttle(struct
 thermal_zone_device *tz, int trip)
             continue;
 
         instance->target = get_target_state(tz, cdev,
-                    tzp->tbp[i].weight, cur_trip_level);
+                    instance->weight, cur_trip_level);
 
         instance->cdev->updated = false;

         thermal_cdev_update(cdev);

drivers/thermal/imx_thermal.c +2 -1

index 5a1f1070b702..74d0c9951eef 100644
@@ -307,7 +307,8 @@ static int imx_bind(struct thermal_zone_device *tz,
 
     ret = thermal_zone_bind_cooling_device(tz, IMX_TRIP_PASSIVE, cdev,
                            THERMAL_NO_LIMIT,
-                           THERMAL_NO_LIMIT);
+                           THERMAL_NO_LIMIT
+                           THERMAL_WEIGHT_DEFAULT);
     if (ret) {
         dev_err(&tz->device,
             "binding zone %s with cdev %s failed:%d\n",

drivers/thermal/of-thermal.c +2 -1

index e032b9bf4085..cf4036cbb671 100644
@@ -157,7 +157,8 @@ static int of_thermal_bind(struct
 thermal_zone_device *thermal,
             ret = thermal_zone_bind_cooling_device(thermal,
                         tbp->trip_id, cdev,
                         tbp->max,
-                        tbp->min);
+                        tbp->min,
+                        tbp->usage);
             if (ret)
                 return ret;

         }

drivers/thermal/samsung/exynos_thermal_common.c +1 -1

index b6be572704a4..d653798b519f 100644
@@ -163,7 +163,7 @@ static int exynos_bind(struct thermal_zone_device
 *thermal,
         case MONITOR_ZONE:
         case WARN_ZONE:
             if (thermal_zone_bind_cooling_device(thermal, i, cdev,
-                                level, 0)) {
+                                level, 0, THERMAL_WEIGHT_DEFAULT)) {
                 dev_err(data->dev,
                     "error unbinding cdev inst=%d\n", i);

                 ret = -EINVAL;

drivers/thermal/thermal_core.c +4 -2

index 4921e084c20b..199866864ef1 100644
@@ -277,7 +277,8 @@ static void print_bind_err_msg(struct
 thermal_zone_device *tz,
 
 static void __bind(struct thermal_zone_device *tz, int mask,
             struct thermal_cooling_device *cdev,
-            unsigned long *limits)
+            unsigned long *limits,
+            unsigned int weight)
 {
     int i, ret;

@@ -292,7 +293,8 @@ static void __bind(struct thermal_zone_device *tz,
 int mask,
                 upper = limits[i * 2 + 1];
             }
             ret = thermal_zone_bind_cooling_device(tz, i, cdev,
-                                   upper, lower);
+                                   upper, lower,
+                                   weight);
             if (ret)
                 print_bind_err_msg(tz, cdev, ret);

         }

drivers/thermal/thermal_core.c +6 -3

@@ -339,7 +341,8 @@ static void bind_cdev(struct thermal_cooling_device
 *cdev)
                 continue;
             tzp->tbp[i].cdev = cdev;
             __bind(pos, tzp->tbp[i].trip_mask, cdev,
-                   tzp->tbp[i].binding_limits);
+                   tzp->tbp[i].binding_limits,
+                   tzp->tbp[i].weight);
         }
     }

@@ -378,7 +381,8 @@ static void bind_tz(struct thermal_zone_device *tz)
                 continue;
             tzp->tbp[i].cdev = pos;
             __bind(tz, tzp->tbp[i].trip_mask, pos,
-                   tzp->tbp[i].binding_limits);
+                   tzp->tbp[i].binding_limits,
+                   tzp->tbp[i].weight);
         }
     }
 exit:

@@ -770,7 +774,8 @@ passive_store(struct device *dev, struct
 device_attribute *attr,
                 thermal_zone_bind_cooling_device(tz,
                         THERMAL_TRIPS_NONE, cdev,
                         THERMAL_NO_LIMIT,
-                        THERMAL_NO_LIMIT);
+                        THERMAL_NO_LIMIT,
+                        THERMAL_WEIGHT_DEFAULT);
         }
         mutex_unlock(&thermal_list_lock);

         if (!tz->passive_delay)

drivers/thermal/thermal_core.c +3 -1

@@ -1009,7 +1014,9 @@ thermal_cooling_device_trip_point_show(struct
 device *dev,
  * @lower:    the Minimum cooling state can be used for this trip point.
  *        THERMAL_NO_LIMIT means no lower limit,
  *        and the cooling device can be in cooling state 0.
- *
+ * @weight:    The weight of the cooling device to be bound to the
+        thermal zone. THERMAL_WEIGHT_DEFAULT for default value
+
  * This interface function bind a thermal cooling device to the certain

trip
* point of a thermal zone device.
* This function is usually called in the thermal zone device .bind
callback.

drivers/thermal/thermal_core.c +2 -1

@@ -1019,7 +1026,8 @@ thermal_cooling_device_trip_point_show(struct
 device *dev,
 int thermal_zone_bind_cooling_device(struct thermal_zone_device *tz,
                      int trip,
                      struct thermal_cooling_device *cdev,
-                     unsigned long upper, unsigned long lower)
+                     unsigned long upper, unsigned long lower,
+                     unsigned int weight)
 {
     struct thermal_instance *dev;

     struct thermal_instance *pos;

drivers/thermal/thermal_core.c +1 -0

@@ -1062,6 +1070,7 @@ int thermal_zone_bind_cooling_device(struct
 thermal_zone_device *tz,
     dev->upper = upper;
     dev->lower = lower;
     dev->target = THERMAL_NO_TARGET;
+    dev->weight = weight;
 
     result = get_idr(&tz->idr, &tz->lock, &dev->id);

if (result)

drivers/thermal/thermal_core.h +1 -0

index b907be823527..1c61abe7801f 100644
@@ -48,6 +48,7 @@ struct thermal_instance {
     struct device_attribute attr;
     struct list_head tz_node; /* node in tz->thermal_instances */
     struct list_head cdev_node; /* node in cdev->thermal_instances */
+    unsigned int weight; /* The weight of the cooling device */
 };
 
 int thermal_register_governor(struct thermal_governor *);

drivers/thermal/ti-soc-thermal/ti-thermal-common.c +2 -1

index 9eec26dc0448..772549ec9bd8 100644
@@ -147,7 +147,8 @@ static int ti_thermal_bind(struct
 thermal_zone_device *thermal,
     return thermal_zone_bind_cooling_device(thermal, 0, cdev,
     /* bind with min and max states defined by cpu_cooling */
                         THERMAL_NO_LIMIT,
-                        THERMAL_NO_LIMIT);
+                        THERMAL_NO_LIMIT,
+                        THERMAL_WEIGHT_DEFAULT);
 }
 

/* Unbind callback functions for thermal zone */

include/linux/thermal.h +5 -1

index fc4fdcbbdecc..674795ba0900 100644
@@ -40,6 +40,9 @@
 /* No upper/lower limit requirement */
 #define THERMAL_NO_LIMIT    ((u32)~0)
 
+/* Default weight of a bound cooling device */
+#define THERMAL_WEIGHT_DEFAULT 1
+
 /* Unit conversion macros */
 #define KELVIN_TO_CELSIUS(t)    (long)(((long)t-2732 >= 0) ?    \
                 ((long)t-2732+5)/10 : ((long)t-2732-5)/10)

@@ -375,7 +378,8 @@ void thermal_zone_device_unregister(struct
 thermal_zone_device *);
 
 int thermal_zone_bind_cooling_device(struct thermal_zone_device *, int,
                      struct thermal_cooling_device *,
-                     unsigned long, unsigned long);
+                     unsigned long, unsigned long,
+                     unsigned int);
 int thermal_zone_unbind_cooling_device(struct thermal_zone_device *, int,
                        struct thermal_cooling_device *);

void thermal_zone_device_update(struct thermal_zone_device *);

Thanks for Reviewing this!

Quoted 37 lines · Eduardo Valentin

Quoted 25 lines · Kapileshwar Singh

The Problem:

In the current code, the weight of the cooling device, which is read as contribution from the device tree in of-thermal.c as:

        ret = of_property_read_u32(np, "contribution", &prop);
        if (ret == 0)
                __tbp->usage = prop;

This is only stored in the private devicedata as:

        ((__thernal_zone *)cdev->devdata)->__tbp->usage

As of-thermal.c specifies its "bind" operation and does not populate tzd->tzp->tbp, this causes an erroneous access in the fair-share governor when it tries to access the weight:

instance->target = get_target_state(tz, cdev,

                                        tzp->tbp[i].weight,

cur_trip_level);

The Proposed solution:

Quoted 8 lines

The proposed solution has the following changes:

1. Passing the weight as an argument to thermal_zone_bind_cooling_device

2. Storing the weight in the thermal_instance data structure created in
the thermal_zone_bind_cooling_device function

3. Changing the accesses in the governor accordingly.

Shouldn't we simply:
1. In of-thermal, populate tzd->tzp->tbp by passing a tbp with correct tbp's in the registration call:

Quoted 7 lines

	zone = thermal_zone_device_register(child->name, tz->ntrips,
						0,
						tz,
						ops,
/* This guy needs to be filled properly */	tzp,
						tz->passive_delay,
						tz->polling_delay);

The reason why I think this might not work is because we do not have the cooling device information (pointer) when we register the thermal zone device.

Quoted 5 lines

2. Add a proper check in the governor to avoid accessing thermal zones with missing data.

Quoted 8 lines

I know there is a check in place:

        if (!tz->tzp || !tz->tzp->tbp)
	          return -EINVAL;

which based in your description seams to be failing. Here, I need more info from your side describing what exactly you are seeing. Can you please post the kernel log when the failure happens? Does it output a kernel segfault or is the governor simply not working?

There is no crash as such, it is more of a semantic failure where the weight being read from the device tree is not passed to the bind parameters.

Cheers!
KP

Quoted 234 lines

I would expect, with the current code, the governor be silent and non-functional, which needs to be fixed too.

Quoted 12 lines

I am not sure of what default value should be assigned to the weight
member in the instance data structure and would like to leave this open to discussion.

Suggested Patch (Not Signed off):

drivers/thermal/db8500_thermal.c +0 -0

index 1e3b3bf9f993..e3ccc2218eb3 100644

drivers/thermal/db8500_thermal.c static int db8500_cdev_bind(struct thermal_zone_device · line 81

Quoted 7 lines

drivers/thermal/db8500_thermal.c +1 -1

@@ -76,7 +76,7 @@ static int db8500_cdev_bind(struct thermal_zone_device
 *thermal,
         upper = lower = i > max_state ? max_state : i;
 
         ret = thermal_zone_bind_cooling_device(thermal, i, cdev,
-            upper, lower);
+            upper, lower, THERMAL_WEIGHT_DEFAULT);
 

I think spreading such parameter, which is known to be part of tbp, is not a good thing to do. Can we avoid that?

Cheers, Eduardo

Quoted 207 lines

drivers/thermal/db8500_thermal.c +0 -0

         dev_info(&cdev->device, "%s bind to %d: %d-%s\n", cdev->type,

             i, ret, ret ? "fail" : "succeed");

drivers/thermal/fair_share.c +1 -1

index 6e0a3fbfae86..c3b25187b467 100644
@@ -109,7 +109,7 @@ static int fair_share_throttle(struct
 thermal_zone_device *tz, int trip)
             continue;
 
         instance->target = get_target_state(tz, cdev,
-                    tzp->tbp[i].weight, cur_trip_level);
+                    instance->weight, cur_trip_level);
 
         instance->cdev->updated = false;

         thermal_cdev_update(cdev);

drivers/thermal/imx_thermal.c +2 -1

index 5a1f1070b702..74d0c9951eef 100644
@@ -307,7 +307,8 @@ static int imx_bind(struct thermal_zone_device *tz,
 
     ret = thermal_zone_bind_cooling_device(tz, IMX_TRIP_PASSIVE, cdev,
                            THERMAL_NO_LIMIT,
-                           THERMAL_NO_LIMIT);
+                           THERMAL_NO_LIMIT
+                           THERMAL_WEIGHT_DEFAULT);
     if (ret) {
         dev_err(&tz->device,
             "binding zone %s with cdev %s failed:%d\n",

drivers/thermal/of-thermal.c +2 -1

index e032b9bf4085..cf4036cbb671 100644
@@ -157,7 +157,8 @@ static int of_thermal_bind(struct
 thermal_zone_device *thermal,
             ret = thermal_zone_bind_cooling_device(thermal,
                         tbp->trip_id, cdev,
                         tbp->max,
-                        tbp->min);
+                        tbp->min,
+                        tbp->usage);
             if (ret)
                 return ret;

         }

drivers/thermal/samsung/exynos_thermal_common.c +1 -1

index b6be572704a4..d653798b519f 100644
@@ -163,7 +163,7 @@ static int exynos_bind(struct thermal_zone_device
 *thermal,
         case MONITOR_ZONE:
         case WARN_ZONE:
             if (thermal_zone_bind_cooling_device(thermal, i, cdev,
-                                level, 0)) {
+                                level, 0, THERMAL_WEIGHT_DEFAULT)) {
                 dev_err(data->dev,
                     "error unbinding cdev inst=%d\n", i);

                 ret = -EINVAL;

drivers/thermal/thermal_core.c +4 -2

index 4921e084c20b..199866864ef1 100644
@@ -277,7 +277,8 @@ static void print_bind_err_msg(struct
 thermal_zone_device *tz,
 
 static void __bind(struct thermal_zone_device *tz, int mask,
             struct thermal_cooling_device *cdev,
-            unsigned long *limits)
+            unsigned long *limits,
+            unsigned int weight)
 {
     int i, ret;

@@ -292,7 +293,8 @@ static void __bind(struct thermal_zone_device *tz,
 int mask,
                 upper = limits[i * 2 + 1];
             }
             ret = thermal_zone_bind_cooling_device(tz, i, cdev,
-                                   upper, lower);
+                                   upper, lower,
+                                   weight);
             if (ret)
                 print_bind_err_msg(tz, cdev, ret);

         }

drivers/thermal/thermal_core.c +6 -3

@@ -339,7 +341,8 @@ static void bind_cdev(struct thermal_cooling_device
 *cdev)
                 continue;
             tzp->tbp[i].cdev = cdev;
             __bind(pos, tzp->tbp[i].trip_mask, cdev,
-                   tzp->tbp[i].binding_limits);
+                   tzp->tbp[i].binding_limits,
+                   tzp->tbp[i].weight);
         }
     }

@@ -378,7 +381,8 @@ static void bind_tz(struct thermal_zone_device *tz)
                 continue;
             tzp->tbp[i].cdev = pos;
             __bind(tz, tzp->tbp[i].trip_mask, pos,
-                   tzp->tbp[i].binding_limits);
+                   tzp->tbp[i].binding_limits,
+                   tzp->tbp[i].weight);
         }
     }
 exit:

@@ -770,7 +774,8 @@ passive_store(struct device *dev, struct
 device_attribute *attr,
                 thermal_zone_bind_cooling_device(tz,
                         THERMAL_TRIPS_NONE, cdev,
                         THERMAL_NO_LIMIT,
-                        THERMAL_NO_LIMIT);
+                        THERMAL_NO_LIMIT,
+                        THERMAL_WEIGHT_DEFAULT);
         }
         mutex_unlock(&thermal_list_lock);

         if (!tz->passive_delay)

drivers/thermal/thermal_core.c +3 -1

@@ -1009,7 +1014,9 @@ thermal_cooling_device_trip_point_show(struct
 device *dev,
  * @lower:    the Minimum cooling state can be used for this trip point.
  *        THERMAL_NO_LIMIT means no lower limit,
  *        and the cooling device can be in cooling state 0.
- *
+ * @weight:    The weight of the cooling device to be bound to the
+        thermal zone. THERMAL_WEIGHT_DEFAULT for default value
+
  * This interface function bind a thermal cooling device to the certain

trip
* point of a thermal zone device.
* This function is usually called in the thermal zone device .bind
callback.

drivers/thermal/thermal_core.c +2 -1

@@ -1019,7 +1026,8 @@ thermal_cooling_device_trip_point_show(struct
 device *dev,
 int thermal_zone_bind_cooling_device(struct thermal_zone_device *tz,
                      int trip,
                      struct thermal_cooling_device *cdev,
-                     unsigned long upper, unsigned long lower)
+                     unsigned long upper, unsigned long lower,
+                     unsigned int weight)
 {
     struct thermal_instance *dev;

     struct thermal_instance *pos;

drivers/thermal/thermal_core.c +1 -0

@@ -1062,6 +1070,7 @@ int thermal_zone_bind_cooling_device(struct
 thermal_zone_device *tz,
     dev->upper = upper;
     dev->lower = lower;
     dev->target = THERMAL_NO_TARGET;
+    dev->weight = weight;
 
     result = get_idr(&tz->idr, &tz->lock, &dev->id);

if (result)

drivers/thermal/thermal_core.h +1 -0

index b907be823527..1c61abe7801f 100644
@@ -48,6 +48,7 @@ struct thermal_instance {
     struct device_attribute attr;
     struct list_head tz_node; /* node in tz->thermal_instances */
     struct list_head cdev_node; /* node in cdev->thermal_instances */
+    unsigned int weight; /* The weight of the cooling device */
 };
 
 int thermal_register_governor(struct thermal_governor *);

drivers/thermal/ti-soc-thermal/ti-thermal-common.c +2 -1

index 9eec26dc0448..772549ec9bd8 100644
@@ -147,7 +147,8 @@ static int ti_thermal_bind(struct
 thermal_zone_device *thermal,
     return thermal_zone_bind_cooling_device(thermal, 0, cdev,
     /* bind with min and max states defined by cpu_cooling */
                         THERMAL_NO_LIMIT,
-                        THERMAL_NO_LIMIT);
+                        THERMAL_NO_LIMIT,
+                        THERMAL_WEIGHT_DEFAULT);
 }
 

/* Unbind callback functions for thermal zone */

include/linux/thermal.h +5 -1

index fc4fdcbbdecc..674795ba0900 100644
@@ -40,6 +40,9 @@
 /* No upper/lower limit requirement */
 #define THERMAL_NO_LIMIT    ((u32)~0)
 
+/* Default weight of a bound cooling device */
+#define THERMAL_WEIGHT_DEFAULT 1
+
 /* Unit conversion macros */
 #define KELVIN_TO_CELSIUS(t)    (long)(((long)t-2732 >= 0) ?    \
                 ((long)t-2732+5)/10 : ((long)t-2732-5)/10)

@@ -375,7 +378,8 @@ void thermal_zone_device_unregister(struct
 thermal_zone_device *);
 
 int thermal_zone_bind_cooling_device(struct thermal_zone_device *, int,
                      struct thermal_cooling_device *,
-                     unsigned long, unsigned long);
+                     unsigned long, unsigned long,
+                     unsigned int);
 int thermal_zone_unbind_cooling_device(struct thermal_zone_device *, int,
                        struct thermal_cooling_device *);

void thermal_zone_device_update(struct thermal_zone_device *);

Quoted 47 lines · Kapileshwar Singh

Thanks for Reviewing this!

Quoted 44 lines · Eduardo Valentin

Quoted 25 lines · Kapileshwar Singh

The Problem:

In the current code, the weight of the cooling device, which is read as contribution from the device tree in of-thermal.c as:

        ret = of_property_read_u32(np, "contribution", &prop);
        if (ret == 0)
                __tbp->usage = prop;

This is only stored in the private devicedata as:

        ((__thernal_zone *)cdev->devdata)->__tbp->usage

As of-thermal.c specifies its "bind" operation and does not populate tzd->tzp->tbp, this causes an erroneous access in the fair-share governor when it tries to access the weight:

instance->target = get_target_state(tz, cdev,

                                        tzp->tbp[i].weight,

cur_trip_level);

The Proposed solution:

Quoted 8 lines

The proposed solution has the following changes:

1. Passing the weight as an argument to thermal_zone_bind_cooling_device

2. Storing the weight in the thermal_instance data structure created in
the thermal_zone_bind_cooling_device function

3. Changing the accesses in the governor accordingly.

Shouldn't we simply:
1. In of-thermal, populate tzd->tzp->tbp by passing a tbp with correct tbp's in the registration call:

    zone = thermal_zone_device_register(child->name, tz->ntrips,
                                            0,
                                            tz,
                                            ops,
/* This guy needs to be filled properly */  tzp,
                                            tz->passive_delay,
                                            tz->polling_delay);

Quoted 2 lines

The reason why I think this might not work is because we do not have the cooling device information (pointer) when we register the thermal zone device.

and I think that is why .match() callback in struct thermal_bind_params is introduced.

Quoted 251 lines

Quoted 11 lines

2. Add a proper check in the governor to avoid accessing thermal zones with missing data.

I know there is a check in place:

        if (!tz->tzp || !tz->tzp->tbp)
              return -EINVAL;

which based in your description seams to be failing. Here, I need more info from your side describing what exactly you are seeing. Can you please post the kernel log when the failure happens? Does it output a kernel segfault or is the governor simply not working?

There is no crash as such, it is more of a semantic failure where the weight being read from the device tree is not passed to the bind parameters.

Cheers!
KP

Quoted 233 lines

I would expect, with the current code, the governor be silent and non-functional, which needs to be fixed too.

Quoted 12 lines

I am not sure of what default value should be assigned to the weight
member in the instance data structure and would like to leave this open to discussion.

Suggested Patch (Not Signed off):

drivers/thermal/db8500_thermal.c +0 -0

index 1e3b3bf9f993..e3ccc2218eb3 100644

drivers/thermal/db8500_thermal.c static int db8500_cdev_bind(struct thermal_zone_device · line 81

Quoted 7 lines

drivers/thermal/db8500_thermal.c +1 -1

@@ -76,7 +76,7 @@ static int db8500_cdev_bind(struct thermal_zone_device
 *thermal,
         upper = lower = i > max_state ? max_state : i;
 
         ret = thermal_zone_bind_cooling_device(thermal, i, cdev,
-            upper, lower);
+            upper, lower, THERMAL_WEIGHT_DEFAULT);
 

I think spreading such parameter, which is known to be part of tbp, is not a good thing to do. Can we avoid that?

Cheers, Eduardo

Quoted 207 lines

drivers/thermal/db8500_thermal.c +0 -0

         dev_info(&cdev->device, "%s bind to %d: %d-%s\n", cdev->type,

             i, ret, ret ? "fail" : "succeed");

drivers/thermal/fair_share.c +1 -1

index 6e0a3fbfae86..c3b25187b467 100644
@@ -109,7 +109,7 @@ static int fair_share_throttle(struct
 thermal_zone_device *tz, int trip)
             continue;
 
         instance->target = get_target_state(tz, cdev,
-                    tzp->tbp[i].weight, cur_trip_level);
+                    instance->weight, cur_trip_level);
 
         instance->cdev->updated = false;

         thermal_cdev_update(cdev);

drivers/thermal/imx_thermal.c +2 -1

index 5a1f1070b702..74d0c9951eef 100644
@@ -307,7 +307,8 @@ static int imx_bind(struct thermal_zone_device *tz,
 
     ret = thermal_zone_bind_cooling_device(tz, IMX_TRIP_PASSIVE, cdev,
                            THERMAL_NO_LIMIT,
-                           THERMAL_NO_LIMIT);
+                           THERMAL_NO_LIMIT
+                           THERMAL_WEIGHT_DEFAULT);
     if (ret) {
         dev_err(&tz->device,
             "binding zone %s with cdev %s failed:%d\n",

drivers/thermal/of-thermal.c +2 -1

index e032b9bf4085..cf4036cbb671 100644
@@ -157,7 +157,8 @@ static int of_thermal_bind(struct
 thermal_zone_device *thermal,
             ret = thermal_zone_bind_cooling_device(thermal,
                         tbp->trip_id, cdev,
                         tbp->max,
-                        tbp->min);
+                        tbp->min,
+                        tbp->usage);
             if (ret)
                 return ret;

         }

drivers/thermal/samsung/exynos_thermal_common.c +1 -1

index b6be572704a4..d653798b519f 100644
@@ -163,7 +163,7 @@ static int exynos_bind(struct thermal_zone_device
 *thermal,
         case MONITOR_ZONE:
         case WARN_ZONE:
             if (thermal_zone_bind_cooling_device(thermal, i, cdev,
-                                level, 0)) {
+                                level, 0, THERMAL_WEIGHT_DEFAULT)) {
                 dev_err(data->dev,
                     "error unbinding cdev inst=%d\n", i);

                 ret = -EINVAL;

drivers/thermal/thermal_core.c +4 -2

index 4921e084c20b..199866864ef1 100644
@@ -277,7 +277,8 @@ static void print_bind_err_msg(struct
 thermal_zone_device *tz,
 
 static void __bind(struct thermal_zone_device *tz, int mask,
             struct thermal_cooling_device *cdev,
-            unsigned long *limits)
+            unsigned long *limits,
+            unsigned int weight)
 {
     int i, ret;

@@ -292,7 +293,8 @@ static void __bind(struct thermal_zone_device *tz,
 int mask,
                 upper = limits[i * 2 + 1];
             }
             ret = thermal_zone_bind_cooling_device(tz, i, cdev,
-                                   upper, lower);
+                                   upper, lower,
+                                   weight);
             if (ret)
                 print_bind_err_msg(tz, cdev, ret);

         }

drivers/thermal/thermal_core.c +6 -3

@@ -339,7 +341,8 @@ static void bind_cdev(struct thermal_cooling_device
 *cdev)
                 continue;
             tzp->tbp[i].cdev = cdev;
             __bind(pos, tzp->tbp[i].trip_mask, cdev,
-                   tzp->tbp[i].binding_limits);
+                   tzp->tbp[i].binding_limits,
+                   tzp->tbp[i].weight);
         }
     }

@@ -378,7 +381,8 @@ static void bind_tz(struct thermal_zone_device *tz)
                 continue;
             tzp->tbp[i].cdev = pos;
             __bind(tz, tzp->tbp[i].trip_mask, pos,
-                   tzp->tbp[i].binding_limits);
+                   tzp->tbp[i].binding_limits,
+                   tzp->tbp[i].weight);
         }
     }
 exit:

@@ -770,7 +774,8 @@ passive_store(struct device *dev, struct
 device_attribute *attr,
                 thermal_zone_bind_cooling_device(tz,
                         THERMAL_TRIPS_NONE, cdev,
                         THERMAL_NO_LIMIT,
-                        THERMAL_NO_LIMIT);
+                        THERMAL_NO_LIMIT,
+                        THERMAL_WEIGHT_DEFAULT);
         }
         mutex_unlock(&thermal_list_lock);

         if (!tz->passive_delay)

drivers/thermal/thermal_core.c +3 -1

@@ -1009,7 +1014,9 @@ thermal_cooling_device_trip_point_show(struct
 device *dev,
  * @lower:    the Minimum cooling state can be used for this trip point.
  *        THERMAL_NO_LIMIT means no lower limit,
  *        and the cooling device can be in cooling state 0.
- *
+ * @weight:    The weight of the cooling device to be bound to the
+        thermal zone. THERMAL_WEIGHT_DEFAULT for default value
+
  * This interface function bind a thermal cooling device to the certain

trip
* point of a thermal zone device.
* This function is usually called in the thermal zone device .bind
callback.

drivers/thermal/thermal_core.c +2 -1

@@ -1019,7 +1026,8 @@ thermal_cooling_device_trip_point_show(struct
 device *dev,
 int thermal_zone_bind_cooling_device(struct thermal_zone_device *tz,
                      int trip,
                      struct thermal_cooling_device *cdev,
-                     unsigned long upper, unsigned long lower)
+                     unsigned long upper, unsigned long lower,
+                     unsigned int weight)
 {
     struct thermal_instance *dev;

     struct thermal_instance *pos;

drivers/thermal/thermal_core.c +1 -0

@@ -1062,6 +1070,7 @@ int thermal_zone_bind_cooling_device(struct
 thermal_zone_device *tz,
     dev->upper = upper;
     dev->lower = lower;
     dev->target = THERMAL_NO_TARGET;
+    dev->weight = weight;
 
     result = get_idr(&tz->idr, &tz->lock, &dev->id);

if (result)

drivers/thermal/thermal_core.h +1 -0

index b907be823527..1c61abe7801f 100644
@@ -48,6 +48,7 @@ struct thermal_instance {
     struct device_attribute attr;
     struct list_head tz_node; /* node in tz->thermal_instances */
     struct list_head cdev_node; /* node in cdev->thermal_instances */
+    unsigned int weight; /* The weight of the cooling device */
 };
 
 int thermal_register_governor(struct thermal_governor *);

drivers/thermal/ti-soc-thermal/ti-thermal-common.c +2 -1

index 9eec26dc0448..772549ec9bd8 100644
@@ -147,7 +147,8 @@ static int ti_thermal_bind(struct
 thermal_zone_device *thermal,
     return thermal_zone_bind_cooling_device(thermal, 0, cdev,
     /* bind with min and max states defined by cpu_cooling */
                         THERMAL_NO_LIMIT,
-                        THERMAL_NO_LIMIT);
+                        THERMAL_NO_LIMIT,
+                        THERMAL_WEIGHT_DEFAULT);
 }
 

/* Unbind callback functions for thermal zone */

include/linux/thermal.h +5 -1

index fc4fdcbbdecc..674795ba0900 100644
@@ -40,6 +40,9 @@
 /* No upper/lower limit requirement */
 #define THERMAL_NO_LIMIT    ((u32)~0)
 
+/* Default weight of a bound cooling device */
+#define THERMAL_WEIGHT_DEFAULT 1
+
 /* Unit conversion macros */
 #define KELVIN_TO_CELSIUS(t)    (long)(((long)t-2732 >= 0) ?    \
                 ((long)t-2732+5)/10 : ((long)t-2732-5)/10)

@@ -375,7 +378,8 @@ void thermal_zone_device_unregister(struct
 thermal_zone_device *);
 
 int thermal_zone_bind_cooling_device(struct thermal_zone_device *, int,
                      struct thermal_cooling_device *,
-                     unsigned long, unsigned long);
+                     unsigned long, unsigned long,
+                     unsigned int);
 int thermal_zone_unbind_cooling_device(struct thermal_zone_device *, int,
                        struct thermal_cooling_device *);

void thermal_zone_device_update(struct thermal_zone_device *);

Many thanks for looking through this! Had a few observations regarding the same:

Quoted 48 lines · Zhang Rui

Quoted 47 lines · Kapileshwar Singh

Thanks for Reviewing this!

Quoted 44 lines · Eduardo Valentin

Quoted 25 lines · Kapileshwar Singh

The Problem:

In the current code, the weight of the cooling device, which is read as contribution from the device tree in of-thermal.c as:

        ret = of_property_read_u32(np, "contribution", &prop);
        if (ret == 0)
                __tbp->usage = prop;

This is only stored in the private devicedata as:

        ((__thernal_zone *)cdev->devdata)->__tbp->usage

As of-thermal.c specifies its "bind" operation and does not populate tzd->tzp->tbp, this causes an erroneous access in the fair-share governor when it tries to access the weight:

instance->target = get_target_state(tz, cdev,

                                        tzp->tbp[i].weight,

cur_trip_level);

The Proposed solution:

Quoted 8 lines

The proposed solution has the following changes:

1. Passing the weight as an argument to thermal_zone_bind_cooling_device

2. Storing the weight in the thermal_instance data structure created in
the thermal_zone_bind_cooling_device function

3. Changing the accesses in the governor accordingly.

Shouldn't we simply:
1. In of-thermal, populate tzd->tzp->tbp by passing a tbp with correct tbp's in the registration call:

    zone = thermal_zone_device_register(child->name, tz->ntrips,
                                            0,
                                            tz,
                                            ops,
/* This guy needs to be filled properly */  tzp,
                                            tz->passive_delay,
                                            tz->polling_delay);

Quoted 4 lines

Quoted 2 lines

The reason why I think this might not work is because we do not have the cooling device information (pointer) when we register the thermal zone device.

and I think that is why .match() callback in struct thermal_bind_params is introduced.

of-thermal defines a bind function and stores the binding information in a private data structure.

Looking at the code where a cooling device is bound there seem to be two options:

1. There is a bind operation defined that figures out a set of bind parameters and the cooling device associated with them (ops->bind) and calls back into thermal_zone_bind_cooling_device.

2. Or there can be bind parameters (tbp) defined that specify a match operation to match the cooling device to the thermal zone device.

1 and 2 being mutually exclusive as follows (in thermal-core.c: bind_tz and bind_cdev:

                if (pos->ops->bind) {
                        ret = pos->ops->bind(pos, cdev);
                        if (ret)
                                print_bind_err_msg(pos, cdev, ret);
                        continue;
                }

                tzp = pos->tzp;
                if (!tzp || !tzp->tbp)
                        continue;

                for (i = 0; i < tzp->num_tbps; i++) {
                        if (tzp->tbp[i].cdev || !tzp->tbp[i].match)
                                continue;
                        if (tzp->tbp[i].match(pos, cdev))
                                continue;
                        tzp->tbp[i].cdev = cdev;
                        __bind(pos, tzp->tbp[i].trip_mask, cdev,
                               tzp->tbp[i].binding_limits);
                }

the match function wont be called if a tz->ops->bind is defined.

The fix suggested above would involve the removal of the __thermal_zone and __thermal_bind_params private data structures and also removing the bind operation from of-thermal.c.
Would that be a direction we would like to proceed in?

I also think since thermal_instance is populated by thermal_zone_bind_cooling_device and represents a "bound" configuration for both the branches (ops->bind and .match()) it could make sense to add weight as a part of the thermal_instance as something that is a part of the "bound" configuration.

I do realize that I could be completely wrong here and would appreciate if you can point me towards the right direction.

Regards,
KP

thanks, rui

Quoted 253 lines

Quoted 251 lines

Quoted 11 lines

2. Add a proper check in the governor to avoid accessing thermal zones with missing data.

I know there is a check in place:

        if (!tz->tzp || !tz->tzp->tbp)
              return -EINVAL;

which based in your description seams to be failing. Here, I need more info from your side describing what exactly you are seeing. Can you please post the kernel log when the failure happens? Does it output a kernel segfault or is the governor simply not working?

There is no crash as such, it is more of a semantic failure where the weight being read from the device tree is not passed to the bind parameters.

Cheers!
KP

Quoted 233 lines

I would expect, with the current code, the governor be silent and non-functional, which needs to be fixed too.

Quoted 12 lines

I am not sure of what default value should be assigned to the weight
member in the instance data structure and would like to leave this open to discussion.

Suggested Patch (Not Signed off):

drivers/thermal/db8500_thermal.c +0 -0

index 1e3b3bf9f993..e3ccc2218eb3 100644

drivers/thermal/db8500_thermal.c static int db8500_cdev_bind(struct thermal_zone_device · line 81

Quoted 7 lines

drivers/thermal/db8500_thermal.c +1 -1

@@ -76,7 +76,7 @@ static int db8500_cdev_bind(struct thermal_zone_device
 *thermal,
         upper = lower = i > max_state ? max_state : i;
 
         ret = thermal_zone_bind_cooling_device(thermal, i, cdev,
-            upper, lower);
+            upper, lower, THERMAL_WEIGHT_DEFAULT);
 

I think spreading such parameter, which is known to be part of tbp, is not a good thing to do. Can we avoid that?

Cheers, Eduardo

Quoted 207 lines

drivers/thermal/db8500_thermal.c +0 -0

         dev_info(&cdev->device, "%s bind to %d: %d-%s\n", cdev->type,

             i, ret, ret ? "fail" : "succeed");

drivers/thermal/fair_share.c +1 -1

index 6e0a3fbfae86..c3b25187b467 100644
@@ -109,7 +109,7 @@ static int fair_share_throttle(struct
 thermal_zone_device *tz, int trip)
             continue;
 
         instance->target = get_target_state(tz, cdev,
-                    tzp->tbp[i].weight, cur_trip_level);
+                    instance->weight, cur_trip_level);
 
         instance->cdev->updated = false;

         thermal_cdev_update(cdev);

drivers/thermal/imx_thermal.c +2 -1

index 5a1f1070b702..74d0c9951eef 100644
@@ -307,7 +307,8 @@ static int imx_bind(struct thermal_zone_device *tz,
 
     ret = thermal_zone_bind_cooling_device(tz, IMX_TRIP_PASSIVE, cdev,
                            THERMAL_NO_LIMIT,
-                           THERMAL_NO_LIMIT);
+                           THERMAL_NO_LIMIT
+                           THERMAL_WEIGHT_DEFAULT);
     if (ret) {
         dev_err(&tz->device,
             "binding zone %s with cdev %s failed:%d\n",

drivers/thermal/of-thermal.c +2 -1

index e032b9bf4085..cf4036cbb671 100644
@@ -157,7 +157,8 @@ static int of_thermal_bind(struct
 thermal_zone_device *thermal,
             ret = thermal_zone_bind_cooling_device(thermal,
                         tbp->trip_id, cdev,
                         tbp->max,
-                        tbp->min);
+                        tbp->min,
+                        tbp->usage);
             if (ret)
                 return ret;

         }

drivers/thermal/samsung/exynos_thermal_common.c +1 -1

index b6be572704a4..d653798b519f 100644
@@ -163,7 +163,7 @@ static int exynos_bind(struct thermal_zone_device
 *thermal,
         case MONITOR_ZONE:
         case WARN_ZONE:
             if (thermal_zone_bind_cooling_device(thermal, i, cdev,
-                                level, 0)) {
+                                level, 0, THERMAL_WEIGHT_DEFAULT)) {
                 dev_err(data->dev,
                     "error unbinding cdev inst=%d\n", i);

                 ret = -EINVAL;

drivers/thermal/thermal_core.c +4 -2

index 4921e084c20b..199866864ef1 100644
@@ -277,7 +277,8 @@ static void print_bind_err_msg(struct
 thermal_zone_device *tz,
 
 static void __bind(struct thermal_zone_device *tz, int mask,
             struct thermal_cooling_device *cdev,
-            unsigned long *limits)
+            unsigned long *limits,
+            unsigned int weight)
 {
     int i, ret;

@@ -292,7 +293,8 @@ static void __bind(struct thermal_zone_device *tz,
 int mask,
                 upper = limits[i * 2 + 1];
             }
             ret = thermal_zone_bind_cooling_device(tz, i, cdev,
-                                   upper, lower);
+                                   upper, lower,
+                                   weight);
             if (ret)
                 print_bind_err_msg(tz, cdev, ret);

         }

drivers/thermal/thermal_core.c +6 -3

@@ -339,7 +341,8 @@ static void bind_cdev(struct thermal_cooling_device
 *cdev)
                 continue;
             tzp->tbp[i].cdev = cdev;
             __bind(pos, tzp->tbp[i].trip_mask, cdev,
-                   tzp->tbp[i].binding_limits);
+                   tzp->tbp[i].binding_limits,
+                   tzp->tbp[i].weight);
         }
     }

@@ -378,7 +381,8 @@ static void bind_tz(struct thermal_zone_device *tz)
                 continue;
             tzp->tbp[i].cdev = pos;
             __bind(tz, tzp->tbp[i].trip_mask, pos,
-                   tzp->tbp[i].binding_limits);
+                   tzp->tbp[i].binding_limits,
+                   tzp->tbp[i].weight);
         }
     }
 exit:

@@ -770,7 +774,8 @@ passive_store(struct device *dev, struct
 device_attribute *attr,
                 thermal_zone_bind_cooling_device(tz,
                         THERMAL_TRIPS_NONE, cdev,
                         THERMAL_NO_LIMIT,
-                        THERMAL_NO_LIMIT);
+                        THERMAL_NO_LIMIT,
+                        THERMAL_WEIGHT_DEFAULT);
         }
         mutex_unlock(&thermal_list_lock);

         if (!tz->passive_delay)

drivers/thermal/thermal_core.c +3 -1

@@ -1009,7 +1014,9 @@ thermal_cooling_device_trip_point_show(struct
 device *dev,
  * @lower:    the Minimum cooling state can be used for this trip point.
  *        THERMAL_NO_LIMIT means no lower limit,
  *        and the cooling device can be in cooling state 0.
- *
+ * @weight:    The weight of the cooling device to be bound to the
+        thermal zone. THERMAL_WEIGHT_DEFAULT for default value
+
  * This interface function bind a thermal cooling device to the certain

trip
* point of a thermal zone device.
* This function is usually called in the thermal zone device .bind
callback.

drivers/thermal/thermal_core.c +2 -1

@@ -1019,7 +1026,8 @@ thermal_cooling_device_trip_point_show(struct
 device *dev,
 int thermal_zone_bind_cooling_device(struct thermal_zone_device *tz,
                      int trip,
                      struct thermal_cooling_device *cdev,
-                     unsigned long upper, unsigned long lower)
+                     unsigned long upper, unsigned long lower,
+                     unsigned int weight)
 {
     struct thermal_instance *dev;

     struct thermal_instance *pos;

drivers/thermal/thermal_core.c +1 -0

@@ -1062,6 +1070,7 @@ int thermal_zone_bind_cooling_device(struct
 thermal_zone_device *tz,
     dev->upper = upper;
     dev->lower = lower;
     dev->target = THERMAL_NO_TARGET;
+    dev->weight = weight;
 
     result = get_idr(&tz->idr, &tz->lock, &dev->id);

if (result)

drivers/thermal/thermal_core.h +1 -0

index b907be823527..1c61abe7801f 100644
@@ -48,6 +48,7 @@ struct thermal_instance {
     struct device_attribute attr;
     struct list_head tz_node; /* node in tz->thermal_instances */
     struct list_head cdev_node; /* node in cdev->thermal_instances */
+    unsigned int weight; /* The weight of the cooling device */
 };
 
 int thermal_register_governor(struct thermal_governor *);

drivers/thermal/ti-soc-thermal/ti-thermal-common.c +2 -1

index 9eec26dc0448..772549ec9bd8 100644
@@ -147,7 +147,8 @@ static int ti_thermal_bind(struct
 thermal_zone_device *thermal,
     return thermal_zone_bind_cooling_device(thermal, 0, cdev,
     /* bind with min and max states defined by cpu_cooling */
                         THERMAL_NO_LIMIT,
-                        THERMAL_NO_LIMIT);
+                        THERMAL_NO_LIMIT,
+                        THERMAL_WEIGHT_DEFAULT);
 }
 

/* Unbind callback functions for thermal zone */

include/linux/thermal.h +5 -1

index fc4fdcbbdecc..674795ba0900 100644
@@ -40,6 +40,9 @@
 /* No upper/lower limit requirement */
 #define THERMAL_NO_LIMIT    ((u32)~0)
 
+/* Default weight of a bound cooling device */
+#define THERMAL_WEIGHT_DEFAULT 1
+
 /* Unit conversion macros */
 #define KELVIN_TO_CELSIUS(t)    (long)(((long)t-2732 >= 0) ?    \
                 ((long)t-2732+5)/10 : ((long)t-2732-5)/10)

@@ -375,7 +378,8 @@ void thermal_zone_device_unregister(struct
 thermal_zone_device *);
 
 int thermal_zone_bind_cooling_device(struct thermal_zone_device *, int,
                      struct thermal_cooling_device *,
-                     unsigned long, unsigned long);
+                     unsigned long, unsigned long,
+                     unsigned int);
 int thermal_zone_unbind_cooling_device(struct thermal_zone_device *, int,
                        struct thermal_cooling_device *);

void thermal_zone_device_update(struct thermal_zone_device *);

Quoted 92 lines · Kapileshwar Singh

Many thanks for looking through this! Had a few observations regarding the same:

Quoted 48 lines · Zhang Rui

Quoted 47 lines · Kapileshwar Singh

Thanks for Reviewing this!

Quoted 44 lines · Eduardo Valentin

Quoted 25 lines · Kapileshwar Singh

The Problem:

In the current code, the weight of the cooling device, which is read as contribution from the device tree in of-thermal.c as:

        ret = of_property_read_u32(np, "contribution", &prop);
        if (ret == 0)
                __tbp->usage = prop;

This is only stored in the private devicedata as:

        ((__thernal_zone *)cdev->devdata)->__tbp->usage

As of-thermal.c specifies its "bind" operation and does not populate tzd->tzp->tbp, this causes an erroneous access in the fair-share governor when it tries to access the weight:

instance->target = get_target_state(tz, cdev,

                                        tzp->tbp[i].weight,

cur_trip_level);

The Proposed solution:

Quoted 8 lines

The proposed solution has the following changes:

1. Passing the weight as an argument to thermal_zone_bind_cooling_device

2. Storing the weight in the thermal_instance data structure created in
the thermal_zone_bind_cooling_device function

3. Changing the accesses in the governor accordingly.

Shouldn't we simply:
1. In of-thermal, populate tzd->tzp->tbp by passing a tbp with correct tbp's in the registration call:

    zone = thermal_zone_device_register(child->name, tz->ntrips,
                                            0,
                                            tz,
                                            ops,
/* This guy needs to be filled properly */  tzp,
                                            tz->passive_delay,
                                            tz->polling_delay);

Quoted 4 lines

Quoted 2 lines

The reason why I think this might not work is because we do not have the cooling device information (pointer) when we register the thermal zone device.

and I think that is why .match() callback in struct thermal_bind_params is introduced.

of-thermal defines a bind function and stores the binding information in a private data structure.

Looking at the code where a cooling device is bound there seem to be two options:

1. There is a bind operation defined that figures out a set of bind parameters and the cooling device associated with them (ops->bind) and calls back into thermal_zone_bind_cooling_device.

2. Or there can be bind parameters (tbp) defined that specify a match operation to match the cooling device to the thermal zone device.

1 and 2 being mutually exclusive as follows (in thermal-core.c: bind_tz and bind_cdev:

                if (pos->ops->bind) {
                        ret = pos->ops->bind(pos, cdev);
                        if (ret)
                                print_bind_err_msg(pos, cdev, ret);
                        continue;
                }

                tzp = pos->tzp;
                if (!tzp || !tzp->tbp)
                        continue;

                for (i = 0; i < tzp->num_tbps; i++) {
                        if (tzp->tbp[i].cdev || !tzp->tbp[i].match)
                                continue;
                        if (tzp->tbp[i].match(pos, cdev))
                                continue;
                        tzp->tbp[i].cdev = cdev;
                        __bind(pos, tzp->tbp[i].trip_mask, cdev,
                               tzp->tbp[i].binding_limits);
                }

Quoted 1 line

the match function wont be called if a tz->ops->bind is defined.

The above description is a correct understanding. You are right.

Quoted 4 lines

The fix suggested above would involve the removal of the __thermal_zone and __thermal_bind_params private data structures and also removing the bind operation from of-thermal.c.
Would that be a direction we would like to proceed in?

well, we would need to see the code to answer that :-) But sounds like a sane idea, yes.

Quoted 6 lines

I also think since thermal_instance is populated by thermal_zone_bind_cooling_device and represents a "bound" configuration for both the branches (ops->bind and .match()) it could make sense to add weight as a part of the thermal_instance as something that is a part of the "bound" configuration.

here we jump into a different subject. Why would the weight be instance specific and not platform specific (as currently coded)?

Quoted 3 lines

I do realize that I could be completely wrong here and would appreciate if you can point me towards the right direction.

there is no issue, the right direction is open discussion, as we are doing already.

Quoted 257 lines

Regards,
KP

thanks, rui

Quoted 251 lines

Quoted 250 lines

Quoted 11 lines

2. Add a proper check in the governor to avoid accessing thermal zones with missing data.

I know there is a check in place:

        if (!tz->tzp || !tz->tzp->tbp)
              return -EINVAL;

which based in your description seams to be failing. Here, I need more info from your side describing what exactly you are seeing. Can you please post the kernel log when the failure happens? Does it output a kernel segfault or is the governor simply not working?

There is no crash as such, it is more of a semantic failure where the weight being read from the device tree is not passed to the bind parameters.

Cheers!
KP

Quoted 233 lines

I would expect, with the current code, the governor be silent and non-functional, which needs to be fixed too.

Quoted 12 lines

I am not sure of what default value should be assigned to the weight
member in the instance data structure and would like to leave this open to discussion.

Suggested Patch (Not Signed off):

drivers/thermal/db8500_thermal.c +0 -0

index 1e3b3bf9f993..e3ccc2218eb3 100644

drivers/thermal/db8500_thermal.c static int db8500_cdev_bind(struct thermal_zone_device · line 81

Quoted 7 lines

drivers/thermal/db8500_thermal.c +1 -1

@@ -76,7 +76,7 @@ static int db8500_cdev_bind(struct thermal_zone_device
 *thermal,
         upper = lower = i > max_state ? max_state : i;
 
         ret = thermal_zone_bind_cooling_device(thermal, i, cdev,
-            upper, lower);
+            upper, lower, THERMAL_WEIGHT_DEFAULT);
 

I think spreading such parameter, which is known to be part of tbp, is not a good thing to do. Can we avoid that?

Cheers, Eduardo

Quoted 207 lines

drivers/thermal/db8500_thermal.c +0 -0

         dev_info(&cdev->device, "%s bind to %d: %d-%s\n", cdev->type,

             i, ret, ret ? "fail" : "succeed");

drivers/thermal/fair_share.c +1 -1

index 6e0a3fbfae86..c3b25187b467 100644
@@ -109,7 +109,7 @@ static int fair_share_throttle(struct
 thermal_zone_device *tz, int trip)
             continue;
 
         instance->target = get_target_state(tz, cdev,
-                    tzp->tbp[i].weight, cur_trip_level);
+                    instance->weight, cur_trip_level);
 
         instance->cdev->updated = false;

         thermal_cdev_update(cdev);

drivers/thermal/imx_thermal.c +2 -1

index 5a1f1070b702..74d0c9951eef 100644
@@ -307,7 +307,8 @@ static int imx_bind(struct thermal_zone_device *tz,
 
     ret = thermal_zone_bind_cooling_device(tz, IMX_TRIP_PASSIVE, cdev,
                            THERMAL_NO_LIMIT,
-                           THERMAL_NO_LIMIT);
+                           THERMAL_NO_LIMIT
+                           THERMAL_WEIGHT_DEFAULT);
     if (ret) {
         dev_err(&tz->device,
             "binding zone %s with cdev %s failed:%d\n",

drivers/thermal/of-thermal.c +2 -1

index e032b9bf4085..cf4036cbb671 100644
@@ -157,7 +157,8 @@ static int of_thermal_bind(struct
 thermal_zone_device *thermal,
             ret = thermal_zone_bind_cooling_device(thermal,
                         tbp->trip_id, cdev,
                         tbp->max,
-                        tbp->min);
+                        tbp->min,
+                        tbp->usage);
             if (ret)
                 return ret;

         }

drivers/thermal/samsung/exynos_thermal_common.c +1 -1

index b6be572704a4..d653798b519f 100644
@@ -163,7 +163,7 @@ static int exynos_bind(struct thermal_zone_device
 *thermal,
         case MONITOR_ZONE:
         case WARN_ZONE:
             if (thermal_zone_bind_cooling_device(thermal, i, cdev,
-                                level, 0)) {
+                                level, 0, THERMAL_WEIGHT_DEFAULT)) {
                 dev_err(data->dev,
                     "error unbinding cdev inst=%d\n", i);

                 ret = -EINVAL;

drivers/thermal/thermal_core.c +4 -2

index 4921e084c20b..199866864ef1 100644
@@ -277,7 +277,8 @@ static void print_bind_err_msg(struct
 thermal_zone_device *tz,
 
 static void __bind(struct thermal_zone_device *tz, int mask,
             struct thermal_cooling_device *cdev,
-            unsigned long *limits)
+            unsigned long *limits,
+            unsigned int weight)
 {
     int i, ret;

@@ -292,7 +293,8 @@ static void __bind(struct thermal_zone_device *tz,
 int mask,
                 upper = limits[i * 2 + 1];
             }
             ret = thermal_zone_bind_cooling_device(tz, i, cdev,
-                                   upper, lower);
+                                   upper, lower,
+                                   weight);
             if (ret)
                 print_bind_err_msg(tz, cdev, ret);

         }

drivers/thermal/thermal_core.c +6 -3

@@ -339,7 +341,8 @@ static void bind_cdev(struct thermal_cooling_device
 *cdev)
                 continue;
             tzp->tbp[i].cdev = cdev;
             __bind(pos, tzp->tbp[i].trip_mask, cdev,
-                   tzp->tbp[i].binding_limits);
+                   tzp->tbp[i].binding_limits,
+                   tzp->tbp[i].weight);
         }
     }

@@ -378,7 +381,8 @@ static void bind_tz(struct thermal_zone_device *tz)
                 continue;
             tzp->tbp[i].cdev = pos;
             __bind(tz, tzp->tbp[i].trip_mask, pos,
-                   tzp->tbp[i].binding_limits);
+                   tzp->tbp[i].binding_limits,
+                   tzp->tbp[i].weight);
         }
     }
 exit:

@@ -770,7 +774,8 @@ passive_store(struct device *dev, struct
 device_attribute *attr,
                 thermal_zone_bind_cooling_device(tz,
                         THERMAL_TRIPS_NONE, cdev,
                         THERMAL_NO_LIMIT,
-                        THERMAL_NO_LIMIT);
+                        THERMAL_NO_LIMIT,
+                        THERMAL_WEIGHT_DEFAULT);
         }
         mutex_unlock(&thermal_list_lock);

         if (!tz->passive_delay)

drivers/thermal/thermal_core.c +3 -1

@@ -1009,7 +1014,9 @@ thermal_cooling_device_trip_point_show(struct
 device *dev,
  * @lower:    the Minimum cooling state can be used for this trip point.
  *        THERMAL_NO_LIMIT means no lower limit,
  *        and the cooling device can be in cooling state 0.
- *
+ * @weight:    The weight of the cooling device to be bound to the
+        thermal zone. THERMAL_WEIGHT_DEFAULT for default value
+
  * This interface function bind a thermal cooling device to the certain

trip
* point of a thermal zone device.
* This function is usually called in the thermal zone device .bind
callback.

drivers/thermal/thermal_core.c +2 -1

@@ -1019,7 +1026,8 @@ thermal_cooling_device_trip_point_show(struct
 device *dev,
 int thermal_zone_bind_cooling_device(struct thermal_zone_device *tz,
                      int trip,
                      struct thermal_cooling_device *cdev,
-                     unsigned long upper, unsigned long lower)
+                     unsigned long upper, unsigned long lower,
+                     unsigned int weight)
 {
     struct thermal_instance *dev;

     struct thermal_instance *pos;

drivers/thermal/thermal_core.c +1 -0

@@ -1062,6 +1070,7 @@ int thermal_zone_bind_cooling_device(struct
 thermal_zone_device *tz,
     dev->upper = upper;
     dev->lower = lower;
     dev->target = THERMAL_NO_TARGET;
+    dev->weight = weight;
 
     result = get_idr(&tz->idr, &tz->lock, &dev->id);

if (result)

drivers/thermal/thermal_core.h +1 -0

index b907be823527..1c61abe7801f 100644
@@ -48,6 +48,7 @@ struct thermal_instance {
     struct device_attribute attr;
     struct list_head tz_node; /* node in tz->thermal_instances */
     struct list_head cdev_node; /* node in cdev->thermal_instances */
+    unsigned int weight; /* The weight of the cooling device */
 };
 
 int thermal_register_governor(struct thermal_governor *);

drivers/thermal/ti-soc-thermal/ti-thermal-common.c +2 -1

index 9eec26dc0448..772549ec9bd8 100644
@@ -147,7 +147,8 @@ static int ti_thermal_bind(struct
 thermal_zone_device *thermal,
     return thermal_zone_bind_cooling_device(thermal, 0, cdev,
     /* bind with min and max states defined by cpu_cooling */
                         THERMAL_NO_LIMIT,
-                        THERMAL_NO_LIMIT);
+                        THERMAL_NO_LIMIT,
+                        THERMAL_WEIGHT_DEFAULT);
 }
 

/* Unbind callback functions for thermal zone */

include/linux/thermal.h +5 -1

index fc4fdcbbdecc..674795ba0900 100644
@@ -40,6 +40,9 @@
 /* No upper/lower limit requirement */
 #define THERMAL_NO_LIMIT    ((u32)~0)
 
+/* Default weight of a bound cooling device */
+#define THERMAL_WEIGHT_DEFAULT 1
+
 /* Unit conversion macros */
 #define KELVIN_TO_CELSIUS(t)    (long)(((long)t-2732 >= 0) ?    \
                 ((long)t-2732+5)/10 : ((long)t-2732-5)/10)

@@ -375,7 +378,8 @@ void thermal_zone_device_unregister(struct
 thermal_zone_device *);
 
 int thermal_zone_bind_cooling_device(struct thermal_zone_device *, int,
                      struct thermal_cooling_device *,
-                     unsigned long, unsigned long);
+                     unsigned long, unsigned long,
+                     unsigned int);
 int thermal_zone_unbind_cooling_device(struct thermal_zone_device *, int,
                        struct thermal_cooling_device *);

void thermal_zone_device_update(struct thermal_zone_device *);

Thanks for looking into this Eduardo!

The patch (appended) aims at accomplishing the following:

* populate the thermal_zone_parameters and tbps correctly in of-thermal
* provide a match callback for of-thermal
* Remove the of-thermal specific bind and unbind operations.

I also see that in of-thermal.c:

struct __thermal_bind_params {
        struct device_node *cooling_device;

The device_node is named as cooling_device which is a misnomer and would like to change it to (as a separate patch)

struct __thermal_bind_params {
        struct device_node *cdev_node;

Cheers!
KP

Possible Patch:

drivers/thermal/of-thermal.c +7 -57

index e032b9bf4085..04bd5f5a806c 100644
@@ -138,60 +138,6 @@ static int of_thermal_get_trend(struct thermal_zone_device *tz, int trip,
 	return 0;
 }
 
-static int of_thermal_bind(struct thermal_zone_device *thermal,
-			   struct thermal_cooling_device *cdev)
-{
-	struct __thermal_zone *data = thermal->devdata;
-	int i;
-
-	if (!data || IS_ERR(data))
-		return -ENODEV;
-
-	/* find where to bind */
-	for (i = 0; i < data->num_tbps; i++) {
-		struct __thermal_bind_params *tbp = data->tbps + i;
-
-		if (tbp->cooling_device == cdev->np) {
-			int ret;
-
-			ret = thermal_zone_bind_cooling_device(thermal,
-						tbp->trip_id, cdev,
-						tbp->max,
-						tbp->min);
-			if (ret)
-				return ret;
-		}
-	}
-
-	return 0;
-}
-
-static int of_thermal_unbind(struct thermal_zone_device *thermal,
-			     struct thermal_cooling_device *cdev)
-{
-	struct __thermal_zone *data = thermal->devdata;
-	int i;
-
-	if (!data || IS_ERR(data))
-		return -ENODEV;
-
-	/* find where to unbind */
-	for (i = 0; i < data->num_tbps; i++) {
-		struct __thermal_bind_params *tbp = data->tbps + i;
-
-		if (tbp->cooling_device == cdev->np) {
-			int ret;
-
-			ret = thermal_zone_unbind_cooling_device(thermal,
-						tbp->trip_id, cdev);
-			if (ret)
-				return ret;
-		}
-	}
-
-	return 0;
-}
-
 static int of_thermal_get_mode(struct thermal_zone_device *tz,
 			       enum thermal_device_mode *mode)
 {

@@ -314,9 +260,6 @@ static struct thermal_zone_device_ops of_thermal_ops = {
 	.get_trip_hyst = of_thermal_get_trip_hyst,
 	.set_trip_hyst = of_thermal_set_trip_hyst,
 	.get_crit_temp = of_thermal_get_crit_temp,
-
-	.bind = of_thermal_bind,
-	.unbind = of_thermal_unbind,
 };
 
 /***   sensor API   ***/

@@ -553,6 +496,109 @@ static int thermal_of_populate_bind_params(struct device_node *np,
 	return ret;
 }
 
+int of_thermal_match_bind_param(struct thermal_zone_device *tz,
+				struct thermal_cooling_device *cdev)
+{
+	struct __thermal_zone *data = tz->devdata;
+	int i;
+
+	if (!data || IS_ERR(data))

drivers/thermal/of-thermal.c +6 -0

+}
+
+static unsigned long* thermal_of_build_trip_limits(struct __thermal_zone *__tz)
+{
+
+	struct __thermal_bind_params *__tbp = __tz->tbps;

drivers/thermal/of-thermal.c +23 -0

+	}
+
+	return limits;
+}
+
+static struct thermal_zone_params* of_thermal_populate_tzp(struct __thermal_zone *__tz)
+{
+	struct thermal_zone_params *tzp;
+	struct __thermal_bind_params *__tbp = __tz->tbps;
+	struct thermal_bind_params *tbp;
+	unsigned long *limits = NULL;
+	int err;
+	int i;
+
+	tzp = kzalloc(sizeof(*tzp), GFP_KERNEL);
+	if (!tzp)
+		return ERR_PTR(ENOMEM);
+
+	if (!__tbp || !__tz->num_tbps) {
+		err = ENODEV;
+		goto free_tzp;
+	}
+

drivers/thermal/of-thermal.c +13 -0

+		else
+			limits = thermal_of_build_trip_limits(__tz);
+
+		if (IS_ERR(limits)) {
+			err = ENOMEM;
+			goto free_tbp;
+		}
+
+		tbp[i].weight = __tbp[i].usage;
+		tbp[i].binding_limits = limits;
+		tbp[i].match = of_thermal_match_bind_param;
+	}
+

drivers/thermal/of-thermal.c +6 -2

+}
+
 /**
  * It maps 'enum thermal_trip_type' found in include/linux/thermal.h
  * into the device tree binding of 'trip', property type.

@@ -812,15 +858,14 @@ int __init of_parse_thermal_zones(void)
 		if (!ops)
 			goto exit_free;
 
-		tzp = kzalloc(sizeof(*tzp), GFP_KERNEL);
-		if (!tzp) {
+
+		tzp = of_thermal_populate_tzp(tz);
+
+		if (IS_ERR(tzp)) {

Quoted 376 lines · Kapileshwar Singh

Quoted 92 lines

Many thanks for looking through this! Had a few observations regarding the same:

Quoted 48 lines · Zhang Rui

Quoted 47 lines · Kapileshwar Singh

Thanks for Reviewing this!

Quoted 44 lines · Eduardo Valentin

Quoted 25 lines · Kapileshwar Singh

The Problem:

In the current code, the weight of the cooling device, which is read as contribution from the device tree in of-thermal.c as:

        ret = of_property_read_u32(np, "contribution", &prop);
        if (ret == 0)
                __tbp->usage = prop;

This is only stored in the private devicedata as:

        ((__thernal_zone *)cdev->devdata)->__tbp->usage

As of-thermal.c specifies its "bind" operation and does not populate tzd->tzp->tbp, this causes an erroneous access in the fair-share governor when it tries to access the weight:

instance->target = get_target_state(tz, cdev,

                                        tzp->tbp[i].weight,

cur_trip_level);

The Proposed solution:

Quoted 8 lines

The proposed solution has the following changes:

1. Passing the weight as an argument to thermal_zone_bind_cooling_device

2. Storing the weight in the thermal_instance data structure created in
the thermal_zone_bind_cooling_device function

3. Changing the accesses in the governor accordingly.

Shouldn't we simply:
1. In of-thermal, populate tzd->tzp->tbp by passing a tbp with correct tbp's in the registration call:

    zone = thermal_zone_device_register(child->name, tz->ntrips,
                                            0,
                                            tz,
                                            ops,
/* This guy needs to be filled properly */  tzp,
                                            tz->passive_delay,
                                            tz->polling_delay);

Quoted 4 lines

Quoted 2 lines

The reason why I think this might not work is because we do not have the cooling device information (pointer) when we register the thermal zone device.

and I think that is why .match() callback in struct thermal_bind_params is introduced.

of-thermal defines a bind function and stores the binding information in a private data structure.

Looking at the code where a cooling device is bound there seem to be two options:

1. There is a bind operation defined that figures out a set of bind parameters and the cooling device associated with them (ops->bind) and calls back into thermal_zone_bind_cooling_device.

2. Or there can be bind parameters (tbp) defined that specify a match operation to match the cooling device to the thermal zone device.

1 and 2 being mutually exclusive as follows (in thermal-core.c: bind_tz and bind_cdev:

                if (pos->ops->bind) {
                        ret = pos->ops->bind(pos, cdev);
                        if (ret)
                                print_bind_err_msg(pos, cdev, ret);
                        continue;
                }

                tzp = pos->tzp;
                if (!tzp || !tzp->tbp)
                        continue;

                for (i = 0; i < tzp->num_tbps; i++) {
                        if (tzp->tbp[i].cdev || !tzp->tbp[i].match)
                                continue;
                        if (tzp->tbp[i].match(pos, cdev))
                                continue;
                        tzp->tbp[i].cdev = cdev;
                        __bind(pos, tzp->tbp[i].trip_mask, cdev,
                               tzp->tbp[i].binding_limits);
                }

Quoted 1 line

the match function wont be called if a tz->ops->bind is defined.

The above description is a correct understanding. You are right.

Quoted 4 lines

The fix suggested above would involve the removal of the __thermal_zone and __thermal_bind_params private data structures and also removing the bind operation from of-thermal.c.
Would that be a direction we would like to proceed in?

well, we would need to see the code to answer that :-) But sounds like a sane idea, yes.

Quoted 6 lines

I also think since thermal_instance is populated by thermal_zone_bind_cooling_device and represents a "bound" configuration for both the branches (ops->bind and .match()) it could make sense to add weight as a part of the thermal_instance as something that is a part of the "bound" configuration.

here we jump into a different subject. Why would the weight be instance specific and not platform specific (as currently coded)?

Quoted 3 lines

I do realize that I could be completely wrong here and would appreciate if you can point me towards the right direction.

there is no issue, the right direction is open discussion, as we are doing already.

Quoted 257 lines

Regards,
KP

thanks, rui

Quoted 251 lines

Quoted 250 lines

Quoted 11 lines

2. Add a proper check in the governor to avoid accessing thermal zones with missing data.

I know there is a check in place:

        if (!tz->tzp || !tz->tzp->tbp)
              return -EINVAL;

which based in your description seams to be failing. Here, I need more info from your side describing what exactly you are seeing. Can you please post the kernel log when the failure happens? Does it output a kernel segfault or is the governor simply not working?

There is no crash as such, it is more of a semantic failure where the weight being read from the device tree is not passed to the bind parameters.

Cheers!
KP

Quoted 233 lines

I would expect, with the current code, the governor be silent and non-functional, which needs to be fixed too.

Quoted 12 lines

I am not sure of what default value should be assigned to the weight
member in the instance data structure and would like to leave this open to discussion.

Suggested Patch (Not Signed off):

drivers/thermal/db8500_thermal.c +0 -0

index 1e3b3bf9f993..e3ccc2218eb3 100644

drivers/thermal/db8500_thermal.c static int db8500_cdev_bind(struct thermal_zone_device · line 81

Quoted 7 lines

drivers/thermal/db8500_thermal.c +1 -1

@@ -76,7 +76,7 @@ static int db8500_cdev_bind(struct thermal_zone_device
 *thermal,
         upper = lower = i > max_state ? max_state : i;
 
         ret = thermal_zone_bind_cooling_device(thermal, i, cdev,
-            upper, lower);
+            upper, lower, THERMAL_WEIGHT_DEFAULT);
 

I think spreading such parameter, which is known to be part of tbp, is not a good thing to do. Can we avoid that?

Cheers, Eduardo

Quoted 207 lines

drivers/thermal/db8500_thermal.c +0 -0

         dev_info(&cdev->device, "%s bind to %d: %d-%s\n", cdev->type,

             i, ret, ret ? "fail" : "succeed");

drivers/thermal/fair_share.c +1 -1

index 6e0a3fbfae86..c3b25187b467 100644
@@ -109,7 +109,7 @@ static int fair_share_throttle(struct
 thermal_zone_device *tz, int trip)
             continue;
 
         instance->target = get_target_state(tz, cdev,
-                    tzp->tbp[i].weight, cur_trip_level);
+                    instance->weight, cur_trip_level);
 
         instance->cdev->updated = false;

         thermal_cdev_update(cdev);

drivers/thermal/imx_thermal.c +2 -1

index 5a1f1070b702..74d0c9951eef 100644
@@ -307,7 +307,8 @@ static int imx_bind(struct thermal_zone_device *tz,
 
     ret = thermal_zone_bind_cooling_device(tz, IMX_TRIP_PASSIVE, cdev,
                            THERMAL_NO_LIMIT,
-                           THERMAL_NO_LIMIT);
+                           THERMAL_NO_LIMIT
+                           THERMAL_WEIGHT_DEFAULT);
     if (ret) {
         dev_err(&tz->device,
             "binding zone %s with cdev %s failed:%d\n",

drivers/thermal/of-thermal.c +2 -1

index e032b9bf4085..cf4036cbb671 100644
@@ -157,7 +157,8 @@ static int of_thermal_bind(struct
 thermal_zone_device *thermal,
             ret = thermal_zone_bind_cooling_device(thermal,
                         tbp->trip_id, cdev,
                         tbp->max,
-                        tbp->min);
+                        tbp->min,
+                        tbp->usage);
             if (ret)
                 return ret;

         }

drivers/thermal/samsung/exynos_thermal_common.c +1 -1

index b6be572704a4..d653798b519f 100644
@@ -163,7 +163,7 @@ static int exynos_bind(struct thermal_zone_device
 *thermal,
         case MONITOR_ZONE:
         case WARN_ZONE:
             if (thermal_zone_bind_cooling_device(thermal, i, cdev,
-                                level, 0)) {
+                                level, 0, THERMAL_WEIGHT_DEFAULT)) {
                 dev_err(data->dev,
                     "error unbinding cdev inst=%d\n", i);

                 ret = -EINVAL;

drivers/thermal/thermal_core.c +4 -2

index 4921e084c20b..199866864ef1 100644
@@ -277,7 +277,8 @@ static void print_bind_err_msg(struct
 thermal_zone_device *tz,
 
 static void __bind(struct thermal_zone_device *tz, int mask,
             struct thermal_cooling_device *cdev,
-            unsigned long *limits)
+            unsigned long *limits,
+            unsigned int weight)
 {
     int i, ret;

@@ -292,7 +293,8 @@ static void __bind(struct thermal_zone_device *tz,
 int mask,
                 upper = limits[i * 2 + 1];
             }
             ret = thermal_zone_bind_cooling_device(tz, i, cdev,
-                                   upper, lower);
+                                   upper, lower,
+                                   weight);
             if (ret)
                 print_bind_err_msg(tz, cdev, ret);

         }

drivers/thermal/thermal_core.c +6 -3

@@ -339,7 +341,8 @@ static void bind_cdev(struct thermal_cooling_device
 *cdev)
                 continue;
             tzp->tbp[i].cdev = cdev;
             __bind(pos, tzp->tbp[i].trip_mask, cdev,
-                   tzp->tbp[i].binding_limits);
+                   tzp->tbp[i].binding_limits,
+                   tzp->tbp[i].weight);
         }
     }

@@ -378,7 +381,8 @@ static void bind_tz(struct thermal_zone_device *tz)
                 continue;
             tzp->tbp[i].cdev = pos;
             __bind(tz, tzp->tbp[i].trip_mask, pos,
-                   tzp->tbp[i].binding_limits);
+                   tzp->tbp[i].binding_limits,
+                   tzp->tbp[i].weight);
         }
     }
 exit:

@@ -770,7 +774,8 @@ passive_store(struct device *dev, struct
 device_attribute *attr,
                 thermal_zone_bind_cooling_device(tz,
                         THERMAL_TRIPS_NONE, cdev,
                         THERMAL_NO_LIMIT,
-                        THERMAL_NO_LIMIT);
+                        THERMAL_NO_LIMIT,
+                        THERMAL_WEIGHT_DEFAULT);
         }
         mutex_unlock(&thermal_list_lock);

         if (!tz->passive_delay)

drivers/thermal/thermal_core.c +3 -1

@@ -1009,7 +1014,9 @@ thermal_cooling_device_trip_point_show(struct
 device *dev,
  * @lower:    the Minimum cooling state can be used for this trip point.
  *        THERMAL_NO_LIMIT means no lower limit,
  *        and the cooling device can be in cooling state 0.
- *
+ * @weight:    The weight of the cooling device to be bound to the
+        thermal zone. THERMAL_WEIGHT_DEFAULT for default value
+
  * This interface function bind a thermal cooling device to the certain

trip
* point of a thermal zone device.
* This function is usually called in the thermal zone device .bind
callback.

drivers/thermal/thermal_core.c +2 -1

@@ -1019,7 +1026,8 @@ thermal_cooling_device_trip_point_show(struct
 device *dev,
 int thermal_zone_bind_cooling_device(struct thermal_zone_device *tz,
                      int trip,
                      struct thermal_cooling_device *cdev,
-                     unsigned long upper, unsigned long lower)
+                     unsigned long upper, unsigned long lower,
+                     unsigned int weight)
 {
     struct thermal_instance *dev;

     struct thermal_instance *pos;

drivers/thermal/thermal_core.c +1 -0

@@ -1062,6 +1070,7 @@ int thermal_zone_bind_cooling_device(struct
 thermal_zone_device *tz,
     dev->upper = upper;
     dev->lower = lower;
     dev->target = THERMAL_NO_TARGET;
+    dev->weight = weight;
 
     result = get_idr(&tz->idr, &tz->lock, &dev->id);

if (result)

drivers/thermal/thermal_core.h +1 -0

index b907be823527..1c61abe7801f 100644
@@ -48,6 +48,7 @@ struct thermal_instance {
     struct device_attribute attr;
     struct list_head tz_node; /* node in tz->thermal_instances */
     struct list_head cdev_node; /* node in cdev->thermal_instances */
+    unsigned int weight; /* The weight of the cooling device */
 };
 
 int thermal_register_governor(struct thermal_governor *);

drivers/thermal/ti-soc-thermal/ti-thermal-common.c +2 -1

index 9eec26dc0448..772549ec9bd8 100644
@@ -147,7 +147,8 @@ static int ti_thermal_bind(struct
 thermal_zone_device *thermal,
     return thermal_zone_bind_cooling_device(thermal, 0, cdev,
     /* bind with min and max states defined by cpu_cooling */
                         THERMAL_NO_LIMIT,
-                        THERMAL_NO_LIMIT);
+                        THERMAL_NO_LIMIT,
+                        THERMAL_WEIGHT_DEFAULT);
 }
 

/* Unbind callback functions for thermal zone */

include/linux/thermal.h +5 -1

index fc4fdcbbdecc..674795ba0900 100644
@@ -40,6 +40,9 @@
 /* No upper/lower limit requirement */
 #define THERMAL_NO_LIMIT    ((u32)~0)
 
+/* Default weight of a bound cooling device */
+#define THERMAL_WEIGHT_DEFAULT 1
+
 /* Unit conversion macros */
 #define KELVIN_TO_CELSIUS(t)    (long)(((long)t-2732 >= 0) ?    \
                 ((long)t-2732+5)/10 : ((long)t-2732-5)/10)

@@ -375,7 +378,8 @@ void thermal_zone_device_unregister(struct
 thermal_zone_device *);
 
 int thermal_zone_bind_cooling_device(struct thermal_zone_device *, int,
                      struct thermal_cooling_device *,
-                     unsigned long, unsigned long);
+                     unsigned long, unsigned long,
+                     unsigned int);
 int thermal_zone_unbind_cooling_device(struct thermal_zone_device *, int,
                        struct thermal_cooling_device *);

void thermal_zone_device_update(struct thermal_zone_device *);

Hello KP,

Quoted 15 lines · Kapileshwar Singh

Thanks for looking into this Eduardo!

The patch (appended) aims at accomplishing the following:

* populate the thermal_zone_parameters and tbps correctly in of-thermal
* provide a match callback for of-thermal
* Remove the of-thermal specific bind and unbind operations.

I also see that in of-thermal.c:

struct __thermal_bind_params {
        struct device_node *cooling_device;

The device_node is named as cooling_device which is a misnomer and would like to change it to (as a separate patch)

Quoted 7 lines

struct __thermal_bind_params {
        struct device_node *cdev_node;

Cheers!
KP

Possible Patch:

Can you please resend this patch separately? Just asking because it got the same encoding issue that Javi had. Maybe, discussing with him to do the same fix for you, the you resend it.

It is a bit annoying to review your patches with encoding scrambled, as most of the scripting we have is not useful. Besides, we cannot apply it.

Quoted 89 lines

drivers/thermal/of-thermal.c +9 -57

index e032b9bf4085..04bd5f5a806c 100644
@@ -138,60 +138,6 @@ static int of_thermal_get_trend(struct thermal_zone_device *tz, int trip,
 	return 0;
 }
 
-static int of_thermal_bind(struct thermal_zone_device *thermal,
-			   struct thermal_cooling_device *cdev)
-{
-	struct __thermal_zone *data = thermal->devdata;
-	int i;
-
-	if (!data || IS_ERR(data))
-		return -ENODEV;
-
-	/* find where to bind */
-	for (i = 0; i < data->num_tbps; i++) {
-		struct __thermal_bind_params *tbp = data->tbps + i;
-
-		if (tbp->cooling_device == cdev->np) {
-			int ret;
-
-			ret = thermal_zone_bind_cooling_device(thermal,
-						tbp->trip_id, cdev,
-						tbp->max,
-						tbp->min);
-			if (ret)
-				return ret;
-		}
-	}
-
-	return 0;
-}
-
-static int of_thermal_unbind(struct thermal_zone_device *thermal,
-			     struct thermal_cooling_device *cdev)
-{
-	struct __thermal_zone *data = thermal->devdata;
-	int i;
-
-	if (!data || IS_ERR(data))
-		return -ENODEV;
-
-	/* find where to unbind */
-	for (i = 0; i < data->num_tbps; i++) {
-		struct __thermal_bind_params *tbp = data->tbps + i;
-
-		if (tbp->cooling_device == cdev->np) {
-			int ret;
-
-			ret = thermal_zone_unbind_cooling_device(thermal,
-						tbp->trip_id, cdev);
-			if (ret)
-				return ret;
-		}
-	}
-
-	return 0;
-}
-
 static int of_thermal_get_mode(struct thermal_zone_device *tz,
 			       enum thermal_device_mode *mode)
 {

@@ -314,9 +260,6 @@ static struct thermal_zone_device_ops of_thermal_ops = {
 	.get_trip_hyst = of_thermal_get_trip_hyst,
 	.set_trip_hyst = of_thermal_set_trip_hyst,
 	.get_crit_temp = of_thermal_get_crit_temp,
-
-	.bind = of_thermal_bind,
-	.unbind = of_thermal_unbind,
 };
 
 /***   sensor API   ***/

@@ -553,6 +496,109 @@ static int thermal_of_populate_bind_params(struct device_node *np,
 	return ret;
 }
 
+int of_thermal_match_bind_param(struct thermal_zone_device *tz,
+				struct thermal_cooling_device *cdev)
+{
+	struct __thermal_zone *data = tz->devdata;
+	int i;
+
+	if (!data || IS_ERR(data))
+		return -ENODEV;
+

drivers/thermal/of-thermal.c static int thermal_of_populate_bind_params(struct device_node *np, · line 515

Quoted 8 lines

drivers/thermal/of-thermal.c +8 -0

+	for (i = 0; i < data->num_tbps; i++) {
+		struct __thermal_bind_params *__tbp = data->tbps + i;
+
+		if (__tbp->cooling_device == cdev->np)
+			return 0;
+	}
+
+	return -1;

Please, use kernel error codes.

Quoted 11 lines

drivers/thermal/of-thermal.c +11 -0

+}
+
+static unsigned long* thermal_of_build_trip_limits(struct __thermal_zone *__tz)
+{
+
+	struct __thermal_bind_params *__tbp = __tz->tbps;
+	unsigned long *limits;
+	int i;
+
+	limits = kcalloc(2 * __tz->ntrips, sizeof(*limits), GFP_KERNEL);
+	if (!limits)

drivers/thermal/of-thermal.c static int thermal_of_populate_bind_params(struct device_node *np, · line 531

Quoted 5 lines

drivers/thermal/of-thermal.c +5 -0

+		return ERR_PTR(ENOMEM);
+
+	for (i = 0; i < __tz->num_tbps; i++) {
+		limits[ __tbp->trip_id * 2 ] = __tbp[i].min;
+		limits[ __tbp->trip_id * 2 + 1] = __tbp[i].max;

Did I miss something or are you always assigning to the same index of limits?

Quoted 28 lines

drivers/thermal/of-thermal.c +27 -0

+	}
+
+	return limits;
+}
+
+static struct thermal_zone_params* of_thermal_populate_tzp(struct __thermal_zone *__tz)
+{
+	struct thermal_zone_params *tzp;
+	struct __thermal_bind_params *__tbp = __tz->tbps;
+	struct thermal_bind_params *tbp;
+	unsigned long *limits = NULL;
+	int err;
+	int i;
+
+	tzp = kzalloc(sizeof(*tzp), GFP_KERNEL);
+	if (!tzp)
+		return ERR_PTR(ENOMEM);
+
+	if (!__tbp || !__tz->num_tbps) {
+		err = ENODEV;
+		goto free_tzp;
+	}
+
+	tbp = kcalloc(__tz->num_tbps, sizeof(*tbp), GFP_KERNEL);
+	if (!tbp) {
+		err = ENOMEM;
+		goto free_tzp;

drivers/thermal/of-thermal.c static int thermal_of_populate_bind_params(struct device_node *np, · line 564

Quoted 6 lines

drivers/thermal/of-thermal.c +6 -0

+	}
+
+	/* We have a number of trips in tz */
+	for (i = 0; i < __tz->num_tbps; i++) {
+		if (limits)
+			limits = kmemdup(&limits, 2 * __tz->ntrips * sizeof(*limits), GFP_KERNEL);

Why is this required?

Quoted 18 lines

drivers/thermal/of-thermal.c +17 -0

+		else
+			limits = thermal_of_build_trip_limits(__tz);
+
+		if (IS_ERR(limits)) {
+			err = ENOMEM;
+			goto free_tbp;
+		}
+
+		tbp[i].weight = __tbp[i].usage;
+		tbp[i].binding_limits = limits;
+		tbp[i].match = of_thermal_match_bind_param;
+	}
+
+	tzp->tbp = tbp;
+	tzp->num_tbps = __tz->num_tbps;
+
+	/* No hwmon because there might be hwmon drivers registering */

drivers/thermal/of-thermal.c static int thermal_of_populate_bind_params(struct device_node *np, · line 587

Quoted 6 lines

drivers/thermal/of-thermal.c +6 -0

+	tzp->no_hwmon = true;
+
+	return tzp;
+
+	/* Error handling code */
+	free_tbp:

it is common to have labels in the first column.

drivers/thermal/of-thermal.c static int thermal_of_populate_bind_params(struct device_node *np, · line 597

Quoted 11 lines

drivers/thermal/of-thermal.c +10 -0

+		/* Free all the correctly allocated binding_limits */
+		for (i = 0; i < __tz->num_tbps; i++) {
+			if (tbp[i].binding_limits)
+				kfree(tbp[i].binding_limits);
+		}
+
+		/* Free the binding parameters */
+		kfree(tbp);
+
+	free_tzp:

ditto.

drivers/thermal/of-thermal.c static int thermal_of_populate_bind_params(struct device_node *np, · line 599

Quoted 2 lines

drivers/thermal/of-thermal.c +2 -0

+		kfree(tzp);
+		return ERR_PTR(err);

Remember to free the resources also in the of_thermal_destroy_zones().

Quoted 20 lines

drivers/thermal/of-thermal.c +6 -2

+}
+
 /**
  * It maps 'enum thermal_trip_type' found in include/linux/thermal.h
  * into the device tree binding of 'trip', property type.

@@ -812,15 +858,14 @@ int __init of_parse_thermal_zones(void)
 		if (!ops)
 			goto exit_free;
 
-		tzp = kzalloc(sizeof(*tzp), GFP_KERNEL);
-		if (!tzp) {
+
+		tzp = of_thermal_populate_tzp(tz);
+
+		if (IS_ERR(tzp)) {
 			kfree(ops);
 			goto exit_free;
 		}
 

drivers/thermal/of-thermal.c int __init of_parse_thermal_zones(void) · line 871

Quoted 6 lines

drivers/thermal/of-thermal.c +0 -3

-		/* No hwmon because there might be hwmon drivers registering */
-		tzp->no_hwmon = true;
-
 		if (!of_property_read_u32(child, "sustainable-power", &prop))
 			tzp->sustainable_power = prop;
 

you may want to base your code on public trees, unless you want to make this one part of Javi's series.

Quoted 381 lines · Eduardo Valentin

Quoted 376 lines · Kapileshwar Singh

Quoted 92 lines

Many thanks for looking through this! Had a few observations regarding the same:

Quoted 48 lines · Zhang Rui

Quoted 47 lines · Kapileshwar Singh

Thanks for Reviewing this!

Quoted 44 lines · Eduardo Valentin

Quoted 25 lines · Kapileshwar Singh

The Problem:

In the current code, the weight of the cooling device, which is read as contribution from the device tree in of-thermal.c as:

        ret = of_property_read_u32(np, "contribution", &prop);
        if (ret == 0)
                __tbp->usage = prop;

This is only stored in the private devicedata as:

        ((__thernal_zone *)cdev->devdata)->__tbp->usage

As of-thermal.c specifies its "bind" operation and does not populate tzd->tzp->tbp, this causes an erroneous access in the fair-share governor when it tries to access the weight:

instance->target = get_target_state(tz, cdev,

                                        tzp->tbp[i].weight,

cur_trip_level);

The Proposed solution:

Quoted 8 lines

The proposed solution has the following changes:

1. Passing the weight as an argument to thermal_zone_bind_cooling_device

2. Storing the weight in the thermal_instance data structure created in
the thermal_zone_bind_cooling_device function

3. Changing the accesses in the governor accordingly.

Shouldn't we simply:
1. In of-thermal, populate tzd->tzp->tbp by passing a tbp with correct tbp's in the registration call:

    zone = thermal_zone_device_register(child->name, tz->ntrips,
                                            0,
                                            tz,
                                            ops,
/* This guy needs to be filled properly */  tzp,
                                            tz->passive_delay,
                                            tz->polling_delay);

Quoted 4 lines

Quoted 2 lines

The reason why I think this might not work is because we do not have the cooling device information (pointer) when we register the thermal zone device.

and I think that is why .match() callback in struct thermal_bind_params is introduced.

of-thermal defines a bind function and stores the binding information in a private data structure.

Looking at the code where a cooling device is bound there seem to be two options:

1. There is a bind operation defined that figures out a set of bind parameters and the cooling device associated with them (ops->bind) and calls back into thermal_zone_bind_cooling_device.

2. Or there can be bind parameters (tbp) defined that specify a match operation to match the cooling device to the thermal zone device.

1 and 2 being mutually exclusive as follows (in thermal-core.c: bind_tz and bind_cdev:

                if (pos->ops->bind) {
                        ret = pos->ops->bind(pos, cdev);
                        if (ret)
                                print_bind_err_msg(pos, cdev, ret);
                        continue;
                }

                tzp = pos->tzp;
                if (!tzp || !tzp->tbp)
                        continue;

                for (i = 0; i < tzp->num_tbps; i++) {
                        if (tzp->tbp[i].cdev || !tzp->tbp[i].match)
                                continue;
                        if (tzp->tbp[i].match(pos, cdev))
                                continue;
                        tzp->tbp[i].cdev = cdev;
                        __bind(pos, tzp->tbp[i].trip_mask, cdev,
                               tzp->tbp[i].binding_limits);
                }

Quoted 1 line

the match function wont be called if a tz->ops->bind is defined.

The above description is a correct understanding. You are right.

Quoted 4 lines

The fix suggested above would involve the removal of the __thermal_zone and __thermal_bind_params private data structures and also removing the bind operation from of-thermal.c.
Would that be a direction we would like to proceed in?

well, we would need to see the code to answer that :-) But sounds like a sane idea, yes.

Quoted 6 lines

I also think since thermal_instance is populated by thermal_zone_bind_cooling_device and represents a "bound" configuration for both the branches (ops->bind and .match()) it could make sense to add weight as a part of the thermal_instance as something that is a part of the "bound" configuration.

here we jump into a different subject. Why would the weight be instance specific and not platform specific (as currently coded)?

Quoted 3 lines

I do realize that I could be completely wrong here and would appreciate if you can point me towards the right direction.

there is no issue, the right direction is open discussion, as we are doing already.

Quoted 257 lines

Regards,
KP

thanks, rui

Quoted 251 lines

Quoted 250 lines

Quoted 11 lines

2. Add a proper check in the governor to avoid accessing thermal zones with missing data.

I know there is a check in place:

        if (!tz->tzp || !tz->tzp->tbp)
              return -EINVAL;

which based in your description seams to be failing. Here, I need more info from your side describing what exactly you are seeing. Can you please post the kernel log when the failure happens? Does it output a kernel segfault or is the governor simply not working?

There is no crash as such, it is more of a semantic failure where the weight being read from the device tree is not passed to the bind parameters.

Cheers!
KP

Quoted 233 lines

I would expect, with the current code, the governor be silent and non-functional, which needs to be fixed too.

Quoted 12 lines

I am not sure of what default value should be assigned to the weight
member in the instance data structure and would like to leave this open to discussion.

Suggested Patch (Not Signed off):

drivers/thermal/db8500_thermal.c +0 -0

index 1e3b3bf9f993..e3ccc2218eb3 100644

drivers/thermal/db8500_thermal.c static int db8500_cdev_bind(struct thermal_zone_device · line 81

Quoted 7 lines

drivers/thermal/db8500_thermal.c +1 -1

@@ -76,7 +76,7 @@ static int db8500_cdev_bind(struct thermal_zone_device
 *thermal,
         upper = lower = i > max_state ? max_state : i;
 
         ret = thermal_zone_bind_cooling_device(thermal, i, cdev,
-            upper, lower);
+            upper, lower, THERMAL_WEIGHT_DEFAULT);
 

I think spreading such parameter, which is known to be part of tbp, is not a good thing to do. Can we avoid that?

Cheers, Eduardo

Quoted 207 lines

drivers/thermal/db8500_thermal.c +0 -0

         dev_info(&cdev->device, "%s bind to %d: %d-%s\n", cdev->type,

             i, ret, ret ? "fail" : "succeed");

drivers/thermal/fair_share.c +1 -1

index 6e0a3fbfae86..c3b25187b467 100644
@@ -109,7 +109,7 @@ static int fair_share_throttle(struct
 thermal_zone_device *tz, int trip)
             continue;
 
         instance->target = get_target_state(tz, cdev,
-                    tzp->tbp[i].weight, cur_trip_level);
+                    instance->weight, cur_trip_level);
 
         instance->cdev->updated = false;

         thermal_cdev_update(cdev);

drivers/thermal/imx_thermal.c +2 -1

index 5a1f1070b702..74d0c9951eef 100644
@@ -307,7 +307,8 @@ static int imx_bind(struct thermal_zone_device *tz,
 
     ret = thermal_zone_bind_cooling_device(tz, IMX_TRIP_PASSIVE, cdev,
                            THERMAL_NO_LIMIT,
-                           THERMAL_NO_LIMIT);
+                           THERMAL_NO_LIMIT
+                           THERMAL_WEIGHT_DEFAULT);
     if (ret) {
         dev_err(&tz->device,
             "binding zone %s with cdev %s failed:%d\n",

drivers/thermal/of-thermal.c +2 -1

index e032b9bf4085..cf4036cbb671 100644
@@ -157,7 +157,8 @@ static int of_thermal_bind(struct
 thermal_zone_device *thermal,
             ret = thermal_zone_bind_cooling_device(thermal,
                         tbp->trip_id, cdev,
                         tbp->max,
-                        tbp->min);
+                        tbp->min,
+                        tbp->usage);
             if (ret)
                 return ret;

         }

drivers/thermal/samsung/exynos_thermal_common.c +1 -1

index b6be572704a4..d653798b519f 100644
@@ -163,7 +163,7 @@ static int exynos_bind(struct thermal_zone_device
 *thermal,
         case MONITOR_ZONE:
         case WARN_ZONE:
             if (thermal_zone_bind_cooling_device(thermal, i, cdev,
-                                level, 0)) {
+                                level, 0, THERMAL_WEIGHT_DEFAULT)) {
                 dev_err(data->dev,
                     "error unbinding cdev inst=%d\n", i);

                 ret = -EINVAL;

drivers/thermal/thermal_core.c +4 -2

index 4921e084c20b..199866864ef1 100644
@@ -277,7 +277,8 @@ static void print_bind_err_msg(struct
 thermal_zone_device *tz,
 
 static void __bind(struct thermal_zone_device *tz, int mask,
             struct thermal_cooling_device *cdev,
-            unsigned long *limits)
+            unsigned long *limits,
+            unsigned int weight)
 {
     int i, ret;

@@ -292,7 +293,8 @@ static void __bind(struct thermal_zone_device *tz,
 int mask,
                 upper = limits[i * 2 + 1];
             }
             ret = thermal_zone_bind_cooling_device(tz, i, cdev,
-                                   upper, lower);
+                                   upper, lower,
+                                   weight);
             if (ret)
                 print_bind_err_msg(tz, cdev, ret);

         }

drivers/thermal/thermal_core.c +6 -3

@@ -339,7 +341,8 @@ static void bind_cdev(struct thermal_cooling_device
 *cdev)
                 continue;
             tzp->tbp[i].cdev = cdev;
             __bind(pos, tzp->tbp[i].trip_mask, cdev,
-                   tzp->tbp[i].binding_limits);
+                   tzp->tbp[i].binding_limits,
+                   tzp->tbp[i].weight);
         }
     }

@@ -378,7 +381,8 @@ static void bind_tz(struct thermal_zone_device *tz)
                 continue;
             tzp->tbp[i].cdev = pos;
             __bind(tz, tzp->tbp[i].trip_mask, pos,
-                   tzp->tbp[i].binding_limits);
+                   tzp->tbp[i].binding_limits,
+                   tzp->tbp[i].weight);
         }
     }
 exit:

@@ -770,7 +774,8 @@ passive_store(struct device *dev, struct
 device_attribute *attr,
                 thermal_zone_bind_cooling_device(tz,
                         THERMAL_TRIPS_NONE, cdev,
                         THERMAL_NO_LIMIT,
-                        THERMAL_NO_LIMIT);
+                        THERMAL_NO_LIMIT,
+                        THERMAL_WEIGHT_DEFAULT);
         }
         mutex_unlock(&thermal_list_lock);

         if (!tz->passive_delay)

drivers/thermal/thermal_core.c +3 -1

@@ -1009,7 +1014,9 @@ thermal_cooling_device_trip_point_show(struct
 device *dev,
  * @lower:    the Minimum cooling state can be used for this trip point.
  *        THERMAL_NO_LIMIT means no lower limit,
  *        and the cooling device can be in cooling state 0.
- *
+ * @weight:    The weight of the cooling device to be bound to the
+        thermal zone. THERMAL_WEIGHT_DEFAULT for default value
+
  * This interface function bind a thermal cooling device to the certain

trip
* point of a thermal zone device.
* This function is usually called in the thermal zone device .bind
callback.

drivers/thermal/thermal_core.c +2 -1

@@ -1019,7 +1026,8 @@ thermal_cooling_device_trip_point_show(struct
 device *dev,
 int thermal_zone_bind_cooling_device(struct thermal_zone_device *tz,
                      int trip,
                      struct thermal_cooling_device *cdev,
-                     unsigned long upper, unsigned long lower)
+                     unsigned long upper, unsigned long lower,
+                     unsigned int weight)
 {
     struct thermal_instance *dev;

     struct thermal_instance *pos;

drivers/thermal/thermal_core.c +1 -0

@@ -1062,6 +1070,7 @@ int thermal_zone_bind_cooling_device(struct
 thermal_zone_device *tz,
     dev->upper = upper;
     dev->lower = lower;
     dev->target = THERMAL_NO_TARGET;
+    dev->weight = weight;
 
     result = get_idr(&tz->idr, &tz->lock, &dev->id);

if (result)

drivers/thermal/thermal_core.h +1 -0

index b907be823527..1c61abe7801f 100644
@@ -48,6 +48,7 @@ struct thermal_instance {
     struct device_attribute attr;
     struct list_head tz_node; /* node in tz->thermal_instances */
     struct list_head cdev_node; /* node in cdev->thermal_instances */
+    unsigned int weight; /* The weight of the cooling device */
 };
 
 int thermal_register_governor(struct thermal_governor *);

drivers/thermal/ti-soc-thermal/ti-thermal-common.c +2 -1

index 9eec26dc0448..772549ec9bd8 100644
@@ -147,7 +147,8 @@ static int ti_thermal_bind(struct
 thermal_zone_device *thermal,
     return thermal_zone_bind_cooling_device(thermal, 0, cdev,
     /* bind with min and max states defined by cpu_cooling */
                         THERMAL_NO_LIMIT,
-                        THERMAL_NO_LIMIT);
+                        THERMAL_NO_LIMIT,
+                        THERMAL_WEIGHT_DEFAULT);
 }
 

/* Unbind callback functions for thermal zone */

include/linux/thermal.h +5 -1

index fc4fdcbbdecc..674795ba0900 100644
@@ -40,6 +40,9 @@
 /* No upper/lower limit requirement */
 #define THERMAL_NO_LIMIT    ((u32)~0)
 
+/* Default weight of a bound cooling device */
+#define THERMAL_WEIGHT_DEFAULT 1
+
 /* Unit conversion macros */
 #define KELVIN_TO_CELSIUS(t)    (long)(((long)t-2732 >= 0) ?    \
                 ((long)t-2732+5)/10 : ((long)t-2732-5)/10)

@@ -375,7 +378,8 @@ void thermal_zone_device_unregister(struct
 thermal_zone_device *);
 
 int thermal_zone_bind_cooling_device(struct thermal_zone_device *, int,
                      struct thermal_cooling_device *,
-                     unsigned long, unsigned long);
+                     unsigned long, unsigned long,
+                     unsigned int);
 int thermal_zone_unbind_cooling_device(struct thermal_zone_device *, int,
                        struct thermal_cooling_device *);

void thermal_zone_device_update(struct thermal_zone_device *);

Quoted 25 lines · Kapileshwar Singh

The Problem:

In the current code, the weight of the cooling device, which is read as contribution from the device tree in of-thermal.c as:

        ret = of_property_read_u32(np, "contribution", &prop);
        if (ret == 0)
                __tbp->usage = prop;

This is only stored in the private devicedata as:

        ((__thernal_zone *)cdev->devdata)->__tbp->usage

As of-thermal.c specifies its "bind" operation and does not populate tzd->tzp->tbp, this causes an erroneous access in the fair-share governor when it tries to access the weight:

instance->target = get_target_state(tz, cdev,

                                        tzp->tbp[i].weight,

cur_trip_level);

The Proposed solution:

Quoted 8 lines

The proposed solution has the following changes:

1. Passing the weight as an argument to thermal_zone_bind_cooling_device

2. Storing the weight in the thermal_instance data structure created in
the thermal_zone_bind_cooling_device function

3. Changing the accesses in the governor accordingly.

Shouldn't we simply:
1. In of-thermal, populate tzd->tzp->tbp by passing a tbp with correct tbp's in the registration call:

	zone = thermal_zone_device_register(child->name, tz->ntrips,
						0,
						tz,
						ops,
/* This guy needs to be filled properly */	tzp,
						tz->passive_delay,
						tz->polling_delay);

2. Add a proper check in the governor to avoid accessing thermal zones with missing data.

I know there is a check in place:

        if (!tz->tzp || !tz->tzp->tbp)
	          return -EINVAL;

which based in your description seams to be failing. Here, I need more info from your side describing what exactly you are seeing. Can you please post the kernel log when the failure happens? Does it output a kernel segfault or is the governor simply not working?

I would expect, with the current code, the governor be silent and non-functional, which needs to be fixed too.

Quoted 13 lines

I am not sure of what default value should be assigned to the weight
member in the instance data structure and would like to leave this open to discussion.

Suggested Patch (Not Signed off):

drivers/thermal/db8500_thermal.c +0 -0

index 1e3b3bf9f993..e3ccc2218eb3 100644

drivers/thermal/db8500_thermal.c static int db8500_cdev_bind(struct thermal_zone_device · line 81

Quoted 7 lines

drivers/thermal/db8500_thermal.c +1 -1

@@ -76,7 +76,7 @@ static int db8500_cdev_bind(struct thermal_zone_device
 *thermal,
         upper = lower = i > max_state ? max_state : i;
 
         ret = thermal_zone_bind_cooling_device(thermal, i, cdev,
-            upper, lower);
+            upper, lower, THERMAL_WEIGHT_DEFAULT);
 

I think spreading such parameter, which is known to be part of tbp, is not a good thing to do. Can we avoid that?

Cheers, Eduardo

Quoted 207 lines

drivers/thermal/db8500_thermal.c +0 -0

         dev_info(&cdev->device, "%s bind to %d: %d-%s\n", cdev->type,

             i, ret, ret ? "fail" : "succeed");

drivers/thermal/fair_share.c +1 -1

index 6e0a3fbfae86..c3b25187b467 100644
@@ -109,7 +109,7 @@ static int fair_share_throttle(struct
 thermal_zone_device *tz, int trip)
             continue;
 
         instance->target = get_target_state(tz, cdev,
-                    tzp->tbp[i].weight, cur_trip_level);
+                    instance->weight, cur_trip_level);
 
         instance->cdev->updated = false;

         thermal_cdev_update(cdev);

drivers/thermal/imx_thermal.c +2 -1

index 5a1f1070b702..74d0c9951eef 100644
@@ -307,7 +307,8 @@ static int imx_bind(struct thermal_zone_device *tz,
 
     ret = thermal_zone_bind_cooling_device(tz, IMX_TRIP_PASSIVE, cdev,
                            THERMAL_NO_LIMIT,
-                           THERMAL_NO_LIMIT);
+                           THERMAL_NO_LIMIT
+                           THERMAL_WEIGHT_DEFAULT);
     if (ret) {
         dev_err(&tz->device,
             "binding zone %s with cdev %s failed:%d\n",

drivers/thermal/of-thermal.c +2 -1

index e032b9bf4085..cf4036cbb671 100644
@@ -157,7 +157,8 @@ static int of_thermal_bind(struct
 thermal_zone_device *thermal,
             ret = thermal_zone_bind_cooling_device(thermal,
                         tbp->trip_id, cdev,
                         tbp->max,
-                        tbp->min);
+                        tbp->min,
+                        tbp->usage);
             if (ret)
                 return ret;

         }

drivers/thermal/samsung/exynos_thermal_common.c +1 -1

index b6be572704a4..d653798b519f 100644
@@ -163,7 +163,7 @@ static int exynos_bind(struct thermal_zone_device
 *thermal,
         case MONITOR_ZONE:
         case WARN_ZONE:
             if (thermal_zone_bind_cooling_device(thermal, i, cdev,
-                                level, 0)) {
+                                level, 0, THERMAL_WEIGHT_DEFAULT)) {
                 dev_err(data->dev,
                     "error unbinding cdev inst=%d\n", i);

                 ret = -EINVAL;

drivers/thermal/thermal_core.c +4 -2

index 4921e084c20b..199866864ef1 100644
@@ -277,7 +277,8 @@ static void print_bind_err_msg(struct
 thermal_zone_device *tz,
 
 static void __bind(struct thermal_zone_device *tz, int mask,
             struct thermal_cooling_device *cdev,
-            unsigned long *limits)
+            unsigned long *limits,
+            unsigned int weight)
 {
     int i, ret;

@@ -292,7 +293,8 @@ static void __bind(struct thermal_zone_device *tz,
 int mask,
                 upper = limits[i * 2 + 1];
             }
             ret = thermal_zone_bind_cooling_device(tz, i, cdev,
-                                   upper, lower);
+                                   upper, lower,
+                                   weight);
             if (ret)
                 print_bind_err_msg(tz, cdev, ret);

         }

drivers/thermal/thermal_core.c +6 -3

@@ -339,7 +341,8 @@ static void bind_cdev(struct thermal_cooling_device
 *cdev)
                 continue;
             tzp->tbp[i].cdev = cdev;
             __bind(pos, tzp->tbp[i].trip_mask, cdev,
-                   tzp->tbp[i].binding_limits);
+                   tzp->tbp[i].binding_limits,
+                   tzp->tbp[i].weight);
         }
     }

@@ -378,7 +381,8 @@ static void bind_tz(struct thermal_zone_device *tz)
                 continue;
             tzp->tbp[i].cdev = pos;
             __bind(tz, tzp->tbp[i].trip_mask, pos,
-                   tzp->tbp[i].binding_limits);
+                   tzp->tbp[i].binding_limits,
+                   tzp->tbp[i].weight);
         }
     }
 exit:

@@ -770,7 +774,8 @@ passive_store(struct device *dev, struct
 device_attribute *attr,
                 thermal_zone_bind_cooling_device(tz,
                         THERMAL_TRIPS_NONE, cdev,
                         THERMAL_NO_LIMIT,
-                        THERMAL_NO_LIMIT);
+                        THERMAL_NO_LIMIT,
+                        THERMAL_WEIGHT_DEFAULT);
         }
         mutex_unlock(&thermal_list_lock);

         if (!tz->passive_delay)

drivers/thermal/thermal_core.c +3 -1

@@ -1009,7 +1014,9 @@ thermal_cooling_device_trip_point_show(struct
 device *dev,
  * @lower:    the Minimum cooling state can be used for this trip point.
  *        THERMAL_NO_LIMIT means no lower limit,
  *        and the cooling device can be in cooling state 0.
- *
+ * @weight:    The weight of the cooling device to be bound to the
+        thermal zone. THERMAL_WEIGHT_DEFAULT for default value
+
  * This interface function bind a thermal cooling device to the certain

trip
* point of a thermal zone device.
* This function is usually called in the thermal zone device .bind
callback.

drivers/thermal/thermal_core.c +2 -1

@@ -1019,7 +1026,8 @@ thermal_cooling_device_trip_point_show(struct
 device *dev,
 int thermal_zone_bind_cooling_device(struct thermal_zone_device *tz,
                      int trip,
                      struct thermal_cooling_device *cdev,
-                     unsigned long upper, unsigned long lower)
+                     unsigned long upper, unsigned long lower,
+                     unsigned int weight)
 {
     struct thermal_instance *dev;

     struct thermal_instance *pos;

drivers/thermal/thermal_core.c +1 -0

@@ -1062,6 +1070,7 @@ int thermal_zone_bind_cooling_device(struct
 thermal_zone_device *tz,
     dev->upper = upper;
     dev->lower = lower;
     dev->target = THERMAL_NO_TARGET;
+    dev->weight = weight;
 
     result = get_idr(&tz->idr, &tz->lock, &dev->id);

if (result)

drivers/thermal/thermal_core.h +1 -0

index b907be823527..1c61abe7801f 100644
@@ -48,6 +48,7 @@ struct thermal_instance {
     struct device_attribute attr;
     struct list_head tz_node; /* node in tz->thermal_instances */
     struct list_head cdev_node; /* node in cdev->thermal_instances */
+    unsigned int weight; /* The weight of the cooling device */
 };
 
 int thermal_register_governor(struct thermal_governor *);

drivers/thermal/ti-soc-thermal/ti-thermal-common.c +2 -1

index 9eec26dc0448..772549ec9bd8 100644
@@ -147,7 +147,8 @@ static int ti_thermal_bind(struct
 thermal_zone_device *thermal,
     return thermal_zone_bind_cooling_device(thermal, 0, cdev,
     /* bind with min and max states defined by cpu_cooling */
                         THERMAL_NO_LIMIT,
-                        THERMAL_NO_LIMIT);
+                        THERMAL_NO_LIMIT,
+                        THERMAL_WEIGHT_DEFAULT);
 }
 

/* Unbind callback functions for thermal zone */

include/linux/thermal.h +5 -1

index fc4fdcbbdecc..674795ba0900 100644
@@ -40,6 +40,9 @@
 /* No upper/lower limit requirement */
 #define THERMAL_NO_LIMIT    ((u32)~0)
 
+/* Default weight of a bound cooling device */
+#define THERMAL_WEIGHT_DEFAULT 1
+
 /* Unit conversion macros */
 #define KELVIN_TO_CELSIUS(t)    (long)(((long)t-2732 >= 0) ?    \
                 ((long)t-2732+5)/10 : ((long)t-2732-5)/10)

@@ -375,7 +378,8 @@ void thermal_zone_device_unregister(struct
 thermal_zone_device *);
 
 int thermal_zone_bind_cooling_device(struct thermal_zone_device *, int,
                      struct thermal_cooling_device *,
-                     unsigned long, unsigned long);
+                     unsigned long, unsigned long,
+                     unsigned int);
 int thermal_zone_unbind_cooling_device(struct thermal_zone_device *, int,
                        struct thermal_cooling_device *);

void thermal_zone_device_update(struct thermal_zone_device *);

Quoted 38 lines · Eduardo Valentin

Quoted 25 lines · Kapileshwar Singh

The Problem:

In the current code, the weight of the cooling device, which is read as contribution from the device tree in of-thermal.c as:

        ret = of_property_read_u32(np, "contribution", &prop);
        if (ret == 0)
                __tbp->usage = prop;

This is only stored in the private devicedata as:

        ((__thernal_zone *)cdev->devdata)->__tbp->usage

As of-thermal.c specifies its "bind" operation and does not populate tzd->tzp->tbp, this causes an erroneous access in the fair-share governor when it tries to access the weight:

instance->target = get_target_state(tz, cdev,

                                        tzp->tbp[i].weight,

cur_trip_level);

The Proposed solution:

Quoted 8 lines

The proposed solution has the following changes:

1. Passing the weight as an argument to thermal_zone_bind_cooling_device

2. Storing the weight in the thermal_instance data structure created in
the thermal_zone_bind_cooling_device function

3. Changing the accesses in the governor accordingly.

Shouldn't we simply:
1. In of-thermal, populate tzd->tzp->tbp by passing a tbp with correct tbp's in the registration call:

Quoted 8 lines

	zone = thermal_zone_device_register(child->name, tz->ntrips,
						0,
						tz,
						ops,
/* This guy needs to be filled properly */	tzp,
						tz->passive_delay,
						tz->polling_delay);

Agreed.

Quoted 12 lines

2. Add a proper check in the governor to avoid accessing thermal zones with missing data.

I know there is a check in place:

        if (!tz->tzp || !tz->tzp->tbp)
	          return -EINVAL;

which based in your description seams to be failing. Here, I need more info from your side describing what exactly you are seeing. Can you please post the kernel log when the failure happens? Does it output a kernel segfault or is the governor simply not working?

Quoted 2 lines

I would expect, with the current code, the governor be silent and non-functional, which needs to be fixed too.

Plus, I think we should add an optional .validate() callback for each governor, to check if the thermal zone meets the requirement of the governor or not before switching to it.

The patch "thermal: let governors have private data for each thermal zone" that you applied[0] allows you to do that. Governors can have a bind_to_tz() op that can fail. If it does, then the switch doesn't happen and you continue with the old governor.

[0] http://article.gmane.org/gmane.linux.power-management.general/54163

Cheers,
Javi