* Fix: Bind Cooling Device Weight
@ 2014-12-18 12:19 Kapileshwar Singh
2014-12-18 13:43 ` Eduardo Valentin
0 siblings, 1 reply; 12+ messages in thread
From: Kapileshwar Singh @ 2014-12-18 12:19 UTC (permalink / raw)
To: linux-pm; +Cc: edubezval, rui.zhang, Javi Merino, Punit Agrawal
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):
diff --git a/drivers/thermal/db8500_thermal.c
b/drivers/thermal/db8500_thermal.c
index 1e3b3bf9f993..e3ccc2218eb3 100644
--- a/drivers/thermal/db8500_thermal.c
+++ b/drivers/thermal/db8500_thermal.c
@@ -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);
dev_info(&cdev->device, "%s bind to %d: %d-%s\n", cdev->type,
i, ret, ret ? "fail" : "succeed");
diff --git a/drivers/thermal/fair_share.c b/drivers/thermal/fair_share.c
index 6e0a3fbfae86..c3b25187b467 100644
--- a/drivers/thermal/fair_share.c
+++ b/drivers/thermal/fair_share.c
@@ -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);
diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
index 5a1f1070b702..74d0c9951eef 100644
--- a/drivers/thermal/imx_thermal.c
+++ b/drivers/thermal/imx_thermal.c
@@ -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",
diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
index e032b9bf4085..cf4036cbb671 100644
--- a/drivers/thermal/of-thermal.c
+++ b/drivers/thermal/of-thermal.c
@@ -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;
}
diff --git a/drivers/thermal/samsung/exynos_thermal_common.c
b/drivers/thermal/samsung/exynos_thermal_common.c
index b6be572704a4..d653798b519f 100644
--- a/drivers/thermal/samsung/exynos_thermal_common.c
+++ b/drivers/thermal/samsung/exynos_thermal_common.c
@@ -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;
diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index 4921e084c20b..199866864ef1 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -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);
}
@@ -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)
@@ -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.
@@ -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;
@@ -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)
diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
index b907be823527..1c61abe7801f 100644
--- a/drivers/thermal/thermal_core.h
+++ b/drivers/thermal/thermal_core.h
@@ -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 *);
diff --git a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
index 9eec26dc0448..772549ec9bd8 100644
--- a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
+++ b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
@@ -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 */
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index fc4fdcbbdecc..674795ba0900 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -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 *);
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: Fix: Bind Cooling Device Weight
2014-12-18 12:19 Fix: Bind Cooling Device Weight Kapileshwar Singh
@ 2014-12-18 13:43 ` Eduardo Valentin
2014-12-18 16:44 ` Kapileshwar Singh
2014-12-22 3:18 ` Zhang Rui
0 siblings, 2 replies; 12+ messages in thread
From: Eduardo Valentin @ 2014-12-18 13:43 UTC (permalink / raw)
To: Kapileshwar Singh; +Cc: linux-pm, rui.zhang, Javi Merino, Punit Agrawal
[-- Attachment #1: Type: text/plain, Size: 11953 bytes --]
On Thu, Dec 18, 2014 at 12:19:00PM +0000, Kapileshwar Singh wrote:
> 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.
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.
>
> 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):
>
> diff --git a/drivers/thermal/db8500_thermal.c
> b/drivers/thermal/db8500_thermal.c
> index 1e3b3bf9f993..e3ccc2218eb3 100644
> --- a/drivers/thermal/db8500_thermal.c
> +++ b/drivers/thermal/db8500_thermal.c
> @@ -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
> dev_info(&cdev->device, "%s bind to %d: %d-%s\n", cdev->type,
> i, ret, ret ? "fail" : "succeed");
> diff --git a/drivers/thermal/fair_share.c b/drivers/thermal/fair_share.c
> index 6e0a3fbfae86..c3b25187b467 100644
> --- a/drivers/thermal/fair_share.c
> +++ b/drivers/thermal/fair_share.c
> @@ -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);
> diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
> index 5a1f1070b702..74d0c9951eef 100644
> --- a/drivers/thermal/imx_thermal.c
> +++ b/drivers/thermal/imx_thermal.c
> @@ -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",
> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
> index e032b9bf4085..cf4036cbb671 100644
> --- a/drivers/thermal/of-thermal.c
> +++ b/drivers/thermal/of-thermal.c
> @@ -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;
> }
> diff --git a/drivers/thermal/samsung/exynos_thermal_common.c
> b/drivers/thermal/samsung/exynos_thermal_common.c
> index b6be572704a4..d653798b519f 100644
> --- a/drivers/thermal/samsung/exynos_thermal_common.c
> +++ b/drivers/thermal/samsung/exynos_thermal_common.c
> @@ -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;
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index 4921e084c20b..199866864ef1 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -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);
> }
> @@ -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)
> @@ -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.
> @@ -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;
> @@ -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)
> diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
> index b907be823527..1c61abe7801f 100644
> --- a/drivers/thermal/thermal_core.h
> +++ b/drivers/thermal/thermal_core.h
> @@ -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 *);
> diff --git a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
> b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
> index 9eec26dc0448..772549ec9bd8 100644
> --- a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
> +++ b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
> @@ -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 */
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index fc4fdcbbdecc..674795ba0900 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -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 *);
>
>
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Fix: Bind Cooling Device Weight
2014-12-18 13:43 ` Eduardo Valentin
@ 2014-12-18 16:44 ` Kapileshwar Singh
2014-12-22 3:23 ` Zhang Rui
2014-12-22 3:18 ` Zhang Rui
1 sibling, 1 reply; 12+ messages in thread
From: Kapileshwar Singh @ 2014-12-18 16:44 UTC (permalink / raw)
To: Eduardo Valentin; +Cc: linux-pm, rui.zhang, Javi Merino, Punit Agrawal
Thanks for Reviewing this!
On 18/12/14 13:43, Eduardo Valentin wrote:
> On Thu, Dec 18, 2014 at 12:19:00PM +0000, Kapileshwar Singh wrote:
>> 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.
> 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);
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.
>
> 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
>
> I would expect, with the current code, the governor be silent and
> non-functional, which needs to be fixed too.
>> 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):
>>
>> diff --git a/drivers/thermal/db8500_thermal.c
>> b/drivers/thermal/db8500_thermal.c
>> index 1e3b3bf9f993..e3ccc2218eb3 100644
>> --- a/drivers/thermal/db8500_thermal.c
>> +++ b/drivers/thermal/db8500_thermal.c
>> @@ -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
>> dev_info(&cdev->device, "%s bind to %d: %d-%s\n", cdev->type,
>> i, ret, ret ? "fail" : "succeed");
>> diff --git a/drivers/thermal/fair_share.c b/drivers/thermal/fair_share.c
>> index 6e0a3fbfae86..c3b25187b467 100644
>> --- a/drivers/thermal/fair_share.c
>> +++ b/drivers/thermal/fair_share.c
>> @@ -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);
>> diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
>> index 5a1f1070b702..74d0c9951eef 100644
>> --- a/drivers/thermal/imx_thermal.c
>> +++ b/drivers/thermal/imx_thermal.c
>> @@ -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",
>> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
>> index e032b9bf4085..cf4036cbb671 100644
>> --- a/drivers/thermal/of-thermal.c
>> +++ b/drivers/thermal/of-thermal.c
>> @@ -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;
>> }
>> diff --git a/drivers/thermal/samsung/exynos_thermal_common.c
>> b/drivers/thermal/samsung/exynos_thermal_common.c
>> index b6be572704a4..d653798b519f 100644
>> --- a/drivers/thermal/samsung/exynos_thermal_common.c
>> +++ b/drivers/thermal/samsung/exynos_thermal_common.c
>> @@ -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;
>> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
>> index 4921e084c20b..199866864ef1 100644
>> --- a/drivers/thermal/thermal_core.c
>> +++ b/drivers/thermal/thermal_core.c
>> @@ -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);
>> }
>> @@ -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)
>> @@ -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.
>> @@ -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;
>> @@ -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)
>> diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
>> index b907be823527..1c61abe7801f 100644
>> --- a/drivers/thermal/thermal_core.h
>> +++ b/drivers/thermal/thermal_core.h
>> @@ -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 *);
>> diff --git a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
>> b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
>> index 9eec26dc0448..772549ec9bd8 100644
>> --- a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
>> +++ b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
>> @@ -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 */
>> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
>> index fc4fdcbbdecc..674795ba0900 100644
>> --- a/include/linux/thermal.h
>> +++ b/include/linux/thermal.h
>> @@ -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 *);
>>
>>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Fix: Bind Cooling Device Weight
2014-12-18 13:43 ` Eduardo Valentin
2014-12-18 16:44 ` Kapileshwar Singh
@ 2014-12-22 3:18 ` Zhang Rui
2014-12-29 10:40 ` Javi Merino
1 sibling, 1 reply; 12+ messages in thread
From: Zhang Rui @ 2014-12-22 3:18 UTC (permalink / raw)
To: Eduardo Valentin; +Cc: Kapileshwar Singh, linux-pm, Javi Merino, Punit Agrawal
On Thu, 2014-12-18 at 09:43 -0400, Eduardo Valentin wrote:
> On Thu, Dec 18, 2014 at 12:19:00PM +0000, Kapileshwar Singh wrote:
> > 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.
>
> 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);
>
Agreed.
> 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.
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.
thanks,
rui
> >
> > 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):
> >
> > diff --git a/drivers/thermal/db8500_thermal.c
> > b/drivers/thermal/db8500_thermal.c
> > index 1e3b3bf9f993..e3ccc2218eb3 100644
> > --- a/drivers/thermal/db8500_thermal.c
> > +++ b/drivers/thermal/db8500_thermal.c
> > @@ -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
> > dev_info(&cdev->device, "%s bind to %d: %d-%s\n", cdev->type,
> > i, ret, ret ? "fail" : "succeed");
> > diff --git a/drivers/thermal/fair_share.c b/drivers/thermal/fair_share.c
> > index 6e0a3fbfae86..c3b25187b467 100644
> > --- a/drivers/thermal/fair_share.c
> > +++ b/drivers/thermal/fair_share.c
> > @@ -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);
> > diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
> > index 5a1f1070b702..74d0c9951eef 100644
> > --- a/drivers/thermal/imx_thermal.c
> > +++ b/drivers/thermal/imx_thermal.c
> > @@ -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",
> > diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
> > index e032b9bf4085..cf4036cbb671 100644
> > --- a/drivers/thermal/of-thermal.c
> > +++ b/drivers/thermal/of-thermal.c
> > @@ -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;
> > }
> > diff --git a/drivers/thermal/samsung/exynos_thermal_common.c
> > b/drivers/thermal/samsung/exynos_thermal_common.c
> > index b6be572704a4..d653798b519f 100644
> > --- a/drivers/thermal/samsung/exynos_thermal_common.c
> > +++ b/drivers/thermal/samsung/exynos_thermal_common.c
> > @@ -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;
> > diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> > index 4921e084c20b..199866864ef1 100644
> > --- a/drivers/thermal/thermal_core.c
> > +++ b/drivers/thermal/thermal_core.c
> > @@ -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);
> > }
> > @@ -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)
> > @@ -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.
> > @@ -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;
> > @@ -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)
> > diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
> > index b907be823527..1c61abe7801f 100644
> > --- a/drivers/thermal/thermal_core.h
> > +++ b/drivers/thermal/thermal_core.h
> > @@ -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 *);
> > diff --git a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
> > b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
> > index 9eec26dc0448..772549ec9bd8 100644
> > --- a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
> > +++ b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
> > @@ -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 */
> > diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> > index fc4fdcbbdecc..674795ba0900 100644
> > --- a/include/linux/thermal.h
> > +++ b/include/linux/thermal.h
> > @@ -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 *);
> >
> >
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Fix: Bind Cooling Device Weight
2014-12-18 16:44 ` Kapileshwar Singh
@ 2014-12-22 3:23 ` Zhang Rui
2014-12-22 16:36 ` Kapileshwar Singh
0 siblings, 1 reply; 12+ messages in thread
From: Zhang Rui @ 2014-12-22 3:23 UTC (permalink / raw)
To: Kapileshwar Singh; +Cc: Eduardo Valentin, linux-pm, Javi Merino, Punit Agrawal
On Thu, 2014-12-18 at 16:44 +0000, Kapileshwar Singh wrote:
> Thanks for Reviewing this!
>
> On 18/12/14 13:43, Eduardo Valentin wrote:
> > On Thu, Dec 18, 2014 at 12:19:00PM +0000, Kapileshwar Singh wrote:
> >> 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.
> > 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);
>
> 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.
thanks,
rui
>
> >
> > 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
>
>
> >
> > I would expect, with the current code, the governor be silent and
> > non-functional, which needs to be fixed too.
> >> 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):
> >>
> >> diff --git a/drivers/thermal/db8500_thermal.c
> >> b/drivers/thermal/db8500_thermal.c
> >> index 1e3b3bf9f993..e3ccc2218eb3 100644
> >> --- a/drivers/thermal/db8500_thermal.c
> >> +++ b/drivers/thermal/db8500_thermal.c
> >> @@ -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
> >> dev_info(&cdev->device, "%s bind to %d: %d-%s\n", cdev->type,
> >> i, ret, ret ? "fail" : "succeed");
> >> diff --git a/drivers/thermal/fair_share.c b/drivers/thermal/fair_share.c
> >> index 6e0a3fbfae86..c3b25187b467 100644
> >> --- a/drivers/thermal/fair_share.c
> >> +++ b/drivers/thermal/fair_share.c
> >> @@ -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);
> >> diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
> >> index 5a1f1070b702..74d0c9951eef 100644
> >> --- a/drivers/thermal/imx_thermal.c
> >> +++ b/drivers/thermal/imx_thermal.c
> >> @@ -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",
> >> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
> >> index e032b9bf4085..cf4036cbb671 100644
> >> --- a/drivers/thermal/of-thermal.c
> >> +++ b/drivers/thermal/of-thermal.c
> >> @@ -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;
> >> }
> >> diff --git a/drivers/thermal/samsung/exynos_thermal_common.c
> >> b/drivers/thermal/samsung/exynos_thermal_common.c
> >> index b6be572704a4..d653798b519f 100644
> >> --- a/drivers/thermal/samsung/exynos_thermal_common.c
> >> +++ b/drivers/thermal/samsung/exynos_thermal_common.c
> >> @@ -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;
> >> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> >> index 4921e084c20b..199866864ef1 100644
> >> --- a/drivers/thermal/thermal_core.c
> >> +++ b/drivers/thermal/thermal_core.c
> >> @@ -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);
> >> }
> >> @@ -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)
> >> @@ -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.
> >> @@ -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;
> >> @@ -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)
> >> diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
> >> index b907be823527..1c61abe7801f 100644
> >> --- a/drivers/thermal/thermal_core.h
> >> +++ b/drivers/thermal/thermal_core.h
> >> @@ -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 *);
> >> diff --git a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
> >> b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
> >> index 9eec26dc0448..772549ec9bd8 100644
> >> --- a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
> >> +++ b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
> >> @@ -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 */
> >> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> >> index fc4fdcbbdecc..674795ba0900 100644
> >> --- a/include/linux/thermal.h
> >> +++ b/include/linux/thermal.h
> >> @@ -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 *);
> >>
> >>
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Fix: Bind Cooling Device Weight
2014-12-22 3:23 ` Zhang Rui
@ 2014-12-22 16:36 ` Kapileshwar Singh
2015-01-05 21:31 ` Eduardo Valentin
0 siblings, 1 reply; 12+ messages in thread
From: Kapileshwar Singh @ 2014-12-22 16:36 UTC (permalink / raw)
To: Zhang Rui; +Cc: Eduardo Valentin, linux-pm, Javi Merino, Punit Agrawal
Many thanks for looking through this! Had a few observations regarding
the same:
On 22/12/14 03:23, Zhang Rui wrote:
> On Thu, 2014-12-18 at 16:44 +0000, Kapileshwar Singh wrote:
>> Thanks for Reviewing this!
>>
>> On 18/12/14 13:43, Eduardo Valentin wrote:
>>> On Thu, Dec 18, 2014 at 12:19:00PM +0000, Kapileshwar Singh wrote:
>>>> 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.
>>> 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);
>> 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
>>> 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
>>
>>
>>> I would expect, with the current code, the governor be silent and
>>> non-functional, which needs to be fixed too.
>>>> 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):
>>>>
>>>> diff --git a/drivers/thermal/db8500_thermal.c
>>>> b/drivers/thermal/db8500_thermal.c
>>>> index 1e3b3bf9f993..e3ccc2218eb3 100644
>>>> --- a/drivers/thermal/db8500_thermal.c
>>>> +++ b/drivers/thermal/db8500_thermal.c
>>>> @@ -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
>>>> dev_info(&cdev->device, "%s bind to %d: %d-%s\n", cdev->type,
>>>> i, ret, ret ? "fail" : "succeed");
>>>> diff --git a/drivers/thermal/fair_share.c b/drivers/thermal/fair_share.c
>>>> index 6e0a3fbfae86..c3b25187b467 100644
>>>> --- a/drivers/thermal/fair_share.c
>>>> +++ b/drivers/thermal/fair_share.c
>>>> @@ -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);
>>>> diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
>>>> index 5a1f1070b702..74d0c9951eef 100644
>>>> --- a/drivers/thermal/imx_thermal.c
>>>> +++ b/drivers/thermal/imx_thermal.c
>>>> @@ -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",
>>>> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
>>>> index e032b9bf4085..cf4036cbb671 100644
>>>> --- a/drivers/thermal/of-thermal.c
>>>> +++ b/drivers/thermal/of-thermal.c
>>>> @@ -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;
>>>> }
>>>> diff --git a/drivers/thermal/samsung/exynos_thermal_common.c
>>>> b/drivers/thermal/samsung/exynos_thermal_common.c
>>>> index b6be572704a4..d653798b519f 100644
>>>> --- a/drivers/thermal/samsung/exynos_thermal_common.c
>>>> +++ b/drivers/thermal/samsung/exynos_thermal_common.c
>>>> @@ -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;
>>>> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
>>>> index 4921e084c20b..199866864ef1 100644
>>>> --- a/drivers/thermal/thermal_core.c
>>>> +++ b/drivers/thermal/thermal_core.c
>>>> @@ -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);
>>>> }
>>>> @@ -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)
>>>> @@ -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.
>>>> @@ -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;
>>>> @@ -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)
>>>> diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
>>>> index b907be823527..1c61abe7801f 100644
>>>> --- a/drivers/thermal/thermal_core.h
>>>> +++ b/drivers/thermal/thermal_core.h
>>>> @@ -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 *);
>>>> diff --git a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
>>>> b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
>>>> index 9eec26dc0448..772549ec9bd8 100644
>>>> --- a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
>>>> +++ b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
>>>> @@ -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 */
>>>> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
>>>> index fc4fdcbbdecc..674795ba0900 100644
>>>> --- a/include/linux/thermal.h
>>>> +++ b/include/linux/thermal.h
>>>> @@ -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 *);
>>>>
>>>>
>>
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Fix: Bind Cooling Device Weight
2014-12-22 3:18 ` Zhang Rui
@ 2014-12-29 10:40 ` Javi Merino
2015-01-05 21:24 ` Eduardo Valentin
0 siblings, 1 reply; 12+ messages in thread
From: Javi Merino @ 2014-12-29 10:40 UTC (permalink / raw)
To: Zhang Rui; +Cc: Eduardo Valentin, Kapileshwar Singh, linux-pm, Punit Agrawal
On Mon, Dec 22, 2014 at 03:18:35AM +0000, Zhang Rui wrote:
> On Thu, 2014-12-18 at 09:43 -0400, Eduardo Valentin wrote:
> > On Thu, Dec 18, 2014 at 12:19:00PM +0000, Kapileshwar Singh wrote:
> > > 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.
> >
> > 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);
> >
> Agreed.
>
> > 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.
>
> 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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Fix: Bind Cooling Device Weight
2014-12-29 10:40 ` Javi Merino
@ 2015-01-05 21:24 ` Eduardo Valentin
0 siblings, 0 replies; 12+ messages in thread
From: Eduardo Valentin @ 2015-01-05 21:24 UTC (permalink / raw)
To: Javi Merino; +Cc: Zhang Rui, Kapileshwar Singh, linux-pm, Punit Agrawal
[-- Attachment #1: Type: text/plain, Size: 775 bytes --]
On Mon, Dec 29, 2014 at 10:40:21AM +0000, Javi Merino wrote:
<cut>
>
> 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
>
Agreed.
Rui, the naming '.bind_to_tz()' seams to be appropriated here, even
considering the validation perspective you mentioned. Binding to a
thermal zone should be the best opportunity to check if the zone makes sense
for that governor. Don't you agree?
I think the .bind_to_tz() should be enough for both cases.
> Cheers,
> Javi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Fix: Bind Cooling Device Weight
2014-12-22 16:36 ` Kapileshwar Singh
@ 2015-01-05 21:31 ` Eduardo Valentin
2015-01-06 16:38 ` Kapileshwar Singh
0 siblings, 1 reply; 12+ messages in thread
From: Eduardo Valentin @ 2015-01-05 21:31 UTC (permalink / raw)
To: Kapileshwar Singh; +Cc: Zhang Rui, linux-pm, Javi Merino, Punit Agrawal
[-- Attachment #1: Type: text/plain, Size: 17111 bytes --]
On Mon, Dec 22, 2014 at 04:36:26PM +0000, Kapileshwar Singh wrote:
> Many thanks for looking through this! Had a few observations regarding
> the same:
> On 22/12/14 03:23, Zhang Rui wrote:
> > On Thu, 2014-12-18 at 16:44 +0000, Kapileshwar Singh wrote:
> >> Thanks for Reviewing this!
> >>
> >> On 18/12/14 13:43, Eduardo Valentin wrote:
> >>> On Thu, Dec 18, 2014 at 12:19:00PM +0000, Kapileshwar Singh wrote:
> >>>> 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.
> >>> 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);
> >> 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 above description is a correct understanding. You are right.
>
> 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.
>
> 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)?
> 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.
> Regards,
> KP
>
>
> thanks, rui
> >>> 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
> >>
> >>
> >>> I would expect, with the current code, the governor be silent and
> >>> non-functional, which needs to be fixed too.
> >>>> 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):
> >>>>
> >>>> diff --git a/drivers/thermal/db8500_thermal.c
> >>>> b/drivers/thermal/db8500_thermal.c
> >>>> index 1e3b3bf9f993..e3ccc2218eb3 100644
> >>>> --- a/drivers/thermal/db8500_thermal.c
> >>>> +++ b/drivers/thermal/db8500_thermal.c
> >>>> @@ -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
> >>>> dev_info(&cdev->device, "%s bind to %d: %d-%s\n", cdev->type,
> >>>> i, ret, ret ? "fail" : "succeed");
> >>>> diff --git a/drivers/thermal/fair_share.c b/drivers/thermal/fair_share.c
> >>>> index 6e0a3fbfae86..c3b25187b467 100644
> >>>> --- a/drivers/thermal/fair_share.c
> >>>> +++ b/drivers/thermal/fair_share.c
> >>>> @@ -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);
> >>>> diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
> >>>> index 5a1f1070b702..74d0c9951eef 100644
> >>>> --- a/drivers/thermal/imx_thermal.c
> >>>> +++ b/drivers/thermal/imx_thermal.c
> >>>> @@ -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",
> >>>> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
> >>>> index e032b9bf4085..cf4036cbb671 100644
> >>>> --- a/drivers/thermal/of-thermal.c
> >>>> +++ b/drivers/thermal/of-thermal.c
> >>>> @@ -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;
> >>>> }
> >>>> diff --git a/drivers/thermal/samsung/exynos_thermal_common.c
> >>>> b/drivers/thermal/samsung/exynos_thermal_common.c
> >>>> index b6be572704a4..d653798b519f 100644
> >>>> --- a/drivers/thermal/samsung/exynos_thermal_common.c
> >>>> +++ b/drivers/thermal/samsung/exynos_thermal_common.c
> >>>> @@ -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;
> >>>> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> >>>> index 4921e084c20b..199866864ef1 100644
> >>>> --- a/drivers/thermal/thermal_core.c
> >>>> +++ b/drivers/thermal/thermal_core.c
> >>>> @@ -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);
> >>>> }
> >>>> @@ -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)
> >>>> @@ -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.
> >>>> @@ -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;
> >>>> @@ -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)
> >>>> diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
> >>>> index b907be823527..1c61abe7801f 100644
> >>>> --- a/drivers/thermal/thermal_core.h
> >>>> +++ b/drivers/thermal/thermal_core.h
> >>>> @@ -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 *);
> >>>> diff --git a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
> >>>> b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
> >>>> index 9eec26dc0448..772549ec9bd8 100644
> >>>> --- a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
> >>>> +++ b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
> >>>> @@ -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 */
> >>>> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> >>>> index fc4fdcbbdecc..674795ba0900 100644
> >>>> --- a/include/linux/thermal.h
> >>>> +++ b/include/linux/thermal.h
> >>>> @@ -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 *);
> >>>>
> >>>>
> >>
> >
> >
>
>
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Fix: Bind Cooling Device Weight
2015-01-05 21:31 ` Eduardo Valentin
@ 2015-01-06 16:38 ` Kapileshwar Singh
2015-01-06 18:37 ` Eduardo Valentin
0 siblings, 1 reply; 12+ messages in thread
From: Kapileshwar Singh @ 2015-01-06 16:38 UTC (permalink / raw)
To: Eduardo Valentin; +Cc: Zhang Rui, linux-pm, Javi Merino, Punit Agrawal
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:
diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
index e032b9bf4085..04bd5f5a806c 100644
--- a/drivers/thermal/of-thermal.c
+++ b/drivers/thermal/of-thermal.c
@@ -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;
+
+ 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;
+}
+
+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)
+ 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;
+ }
+
+ 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;
+ }
+
+ /* 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);
+ 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 */
+ tzp->no_hwmon = true;
+
+ return tzp;
+
+ /* Error handling code */
+ free_tbp:
+ /* 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:
+ kfree(tzp);
+ return ERR_PTR(err);
+}
+
/**
* 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;
}
- /* 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;
On 05/01/15 21:31, Eduardo Valentin wrote:
> On Mon, Dec 22, 2014 at 04:36:26PM +0000, Kapileshwar Singh wrote:
>> Many thanks for looking through this! Had a few observations regarding
>> the same:
>> On 22/12/14 03:23, Zhang Rui wrote:
>>> On Thu, 2014-12-18 at 16:44 +0000, Kapileshwar Singh wrote:
>>>> Thanks for Reviewing this!
>>>>
>>>> On 18/12/14 13:43, Eduardo Valentin wrote:
>>>>> On Thu, Dec 18, 2014 at 12:19:00PM +0000, Kapileshwar Singh wrote:
>>>>>> 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.
>>>>> 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);
>>>> 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 above description is a correct understanding. You are right.
>
>> 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.
>
>> 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)?
>
>
>> 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.
>
>> Regards,
>> KP
>>
>>
>> thanks, rui
>>>>> 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
>>>>
>>>>
>>>>> I would expect, with the current code, the governor be silent and
>>>>> non-functional, which needs to be fixed too.
>>>>>> 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):
>>>>>>
>>>>>> diff --git a/drivers/thermal/db8500_thermal.c
>>>>>> b/drivers/thermal/db8500_thermal.c
>>>>>> index 1e3b3bf9f993..e3ccc2218eb3 100644
>>>>>> --- a/drivers/thermal/db8500_thermal.c
>>>>>> +++ b/drivers/thermal/db8500_thermal.c
>>>>>> @@ -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
>>>>>> dev_info(&cdev->device, "%s bind to %d: %d-%s\n", cdev->type,
>>>>>> i, ret, ret ? "fail" : "succeed");
>>>>>> diff --git a/drivers/thermal/fair_share.c b/drivers/thermal/fair_share.c
>>>>>> index 6e0a3fbfae86..c3b25187b467 100644
>>>>>> --- a/drivers/thermal/fair_share.c
>>>>>> +++ b/drivers/thermal/fair_share.c
>>>>>> @@ -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);
>>>>>> diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
>>>>>> index 5a1f1070b702..74d0c9951eef 100644
>>>>>> --- a/drivers/thermal/imx_thermal.c
>>>>>> +++ b/drivers/thermal/imx_thermal.c
>>>>>> @@ -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",
>>>>>> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
>>>>>> index e032b9bf4085..cf4036cbb671 100644
>>>>>> --- a/drivers/thermal/of-thermal.c
>>>>>> +++ b/drivers/thermal/of-thermal.c
>>>>>> @@ -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;
>>>>>> }
>>>>>> diff --git a/drivers/thermal/samsung/exynos_thermal_common.c
>>>>>> b/drivers/thermal/samsung/exynos_thermal_common.c
>>>>>> index b6be572704a4..d653798b519f 100644
>>>>>> --- a/drivers/thermal/samsung/exynos_thermal_common.c
>>>>>> +++ b/drivers/thermal/samsung/exynos_thermal_common.c
>>>>>> @@ -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;
>>>>>> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
>>>>>> index 4921e084c20b..199866864ef1 100644
>>>>>> --- a/drivers/thermal/thermal_core.c
>>>>>> +++ b/drivers/thermal/thermal_core.c
>>>>>> @@ -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);
>>>>>> }
>>>>>> @@ -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)
>>>>>> @@ -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.
>>>>>> @@ -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;
>>>>>> @@ -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)
>>>>>> diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
>>>>>> index b907be823527..1c61abe7801f 100644
>>>>>> --- a/drivers/thermal/thermal_core.h
>>>>>> +++ b/drivers/thermal/thermal_core.h
>>>>>> @@ -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 *);
>>>>>> diff --git a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
>>>>>> b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
>>>>>> index 9eec26dc0448..772549ec9bd8 100644
>>>>>> --- a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
>>>>>> +++ b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
>>>>>> @@ -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 */
>>>>>> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
>>>>>> index fc4fdcbbdecc..674795ba0900 100644
>>>>>> --- a/include/linux/thermal.h
>>>>>> +++ b/include/linux/thermal.h
>>>>>> @@ -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 *);
>>>>>>
>>>>>>
>>>
>>
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: Fix: Bind Cooling Device Weight
2015-01-06 16:38 ` Kapileshwar Singh
@ 2015-01-06 18:37 ` Eduardo Valentin
2015-01-07 14:08 ` Kapileshwar Singh
0 siblings, 1 reply; 12+ messages in thread
From: Eduardo Valentin @ 2015-01-06 18:37 UTC (permalink / raw)
To: Kapileshwar Singh; +Cc: Zhang Rui, linux-pm, Javi Merino, Punit Agrawal
[-- Attachment #1: Type: text/plain, Size: 24926 bytes --]
Hello KP,
On Tue, Jan 06, 2015 at 04:38:20PM +0000, Kapileshwar Singh wrote:
> 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:
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.
>
> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
> index e032b9bf4085..04bd5f5a806c 100644
> --- a/drivers/thermal/of-thermal.c
> +++ b/drivers/thermal/of-thermal.c
> @@ -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;
> +
> + 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.
> +}
> +
> +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)
> + 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?
> + }
> +
> + 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;
> + }
> +
> + /* 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?
> + 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 */
> + tzp->no_hwmon = true;
> +
> + return tzp;
> +
> + /* Error handling code */
> + free_tbp:
it is common to have labels in the first column.
> + /* 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.
> + kfree(tzp);
> + return ERR_PTR(err);
Remember to free the resources also in the of_thermal_destroy_zones().
> +}
> +
> /**
> * 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;
> }
>
> - /* 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.
>
> On 05/01/15 21:31, Eduardo Valentin wrote:
> > On Mon, Dec 22, 2014 at 04:36:26PM +0000, Kapileshwar Singh wrote:
> >> Many thanks for looking through this! Had a few observations regarding
> >> the same:
> >> On 22/12/14 03:23, Zhang Rui wrote:
> >>> On Thu, 2014-12-18 at 16:44 +0000, Kapileshwar Singh wrote:
> >>>> Thanks for Reviewing this!
> >>>>
> >>>> On 18/12/14 13:43, Eduardo Valentin wrote:
> >>>>> On Thu, Dec 18, 2014 at 12:19:00PM +0000, Kapileshwar Singh wrote:
> >>>>>> 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.
> >>>>> 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);
> >>>> 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 above description is a correct understanding. You are right.
> >
> >> 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.
> >
> >> 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)?
> >
> >
> >> 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.
> >
> >> Regards,
> >> KP
> >>
> >>
> >> thanks, rui
> >>>>> 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
> >>>>
> >>>>
> >>>>> I would expect, with the current code, the governor be silent and
> >>>>> non-functional, which needs to be fixed too.
> >>>>>> 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):
> >>>>>>
> >>>>>> diff --git a/drivers/thermal/db8500_thermal.c
> >>>>>> b/drivers/thermal/db8500_thermal.c
> >>>>>> index 1e3b3bf9f993..e3ccc2218eb3 100644
> >>>>>> --- a/drivers/thermal/db8500_thermal.c
> >>>>>> +++ b/drivers/thermal/db8500_thermal.c
> >>>>>> @@ -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
> >>>>>> dev_info(&cdev->device, "%s bind to %d: %d-%s\n", cdev->type,
> >>>>>> i, ret, ret ? "fail" : "succeed");
> >>>>>> diff --git a/drivers/thermal/fair_share.c b/drivers/thermal/fair_share.c
> >>>>>> index 6e0a3fbfae86..c3b25187b467 100644
> >>>>>> --- a/drivers/thermal/fair_share.c
> >>>>>> +++ b/drivers/thermal/fair_share.c
> >>>>>> @@ -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);
> >>>>>> diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
> >>>>>> index 5a1f1070b702..74d0c9951eef 100644
> >>>>>> --- a/drivers/thermal/imx_thermal.c
> >>>>>> +++ b/drivers/thermal/imx_thermal.c
> >>>>>> @@ -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",
> >>>>>> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
> >>>>>> index e032b9bf4085..cf4036cbb671 100644
> >>>>>> --- a/drivers/thermal/of-thermal.c
> >>>>>> +++ b/drivers/thermal/of-thermal.c
> >>>>>> @@ -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;
> >>>>>> }
> >>>>>> diff --git a/drivers/thermal/samsung/exynos_thermal_common.c
> >>>>>> b/drivers/thermal/samsung/exynos_thermal_common.c
> >>>>>> index b6be572704a4..d653798b519f 100644
> >>>>>> --- a/drivers/thermal/samsung/exynos_thermal_common.c
> >>>>>> +++ b/drivers/thermal/samsung/exynos_thermal_common.c
> >>>>>> @@ -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;
> >>>>>> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> >>>>>> index 4921e084c20b..199866864ef1 100644
> >>>>>> --- a/drivers/thermal/thermal_core.c
> >>>>>> +++ b/drivers/thermal/thermal_core.c
> >>>>>> @@ -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);
> >>>>>> }
> >>>>>> @@ -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)
> >>>>>> @@ -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.
> >>>>>> @@ -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;
> >>>>>> @@ -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)
> >>>>>> diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
> >>>>>> index b907be823527..1c61abe7801f 100644
> >>>>>> --- a/drivers/thermal/thermal_core.h
> >>>>>> +++ b/drivers/thermal/thermal_core.h
> >>>>>> @@ -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 *);
> >>>>>> diff --git a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
> >>>>>> b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
> >>>>>> index 9eec26dc0448..772549ec9bd8 100644
> >>>>>> --- a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
> >>>>>> +++ b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
> >>>>>> @@ -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 */
> >>>>>> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> >>>>>> index fc4fdcbbdecc..674795ba0900 100644
> >>>>>> --- a/include/linux/thermal.h
> >>>>>> +++ b/include/linux/thermal.h
> >>>>>> @@ -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 *);
> >>>>>>
> >>>>>>
> >>>
> >>
>
>
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Fix: Bind Cooling Device Weight
2015-01-06 18:37 ` Eduardo Valentin
@ 2015-01-07 14:08 ` Kapileshwar Singh
0 siblings, 0 replies; 12+ messages in thread
From: Kapileshwar Singh @ 2015-01-07 14:08 UTC (permalink / raw)
To: Eduardo Valentin; +Cc: Zhang Rui, linux-pm, Javi Merino, Punit Agrawal
Hi Eduardo,
Thanks again for looking into this.
On 06/01/15 18:37, Eduardo Valentin wrote:
> Hello KP,
>
> On Tue, Jan 06, 2015 at 04:38:20PM +0000, Kapileshwar Singh wrote:
>> 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:
>
> 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.
>
Apologies for the garbling of the patches and would have this fixed for further iterations.
>>
>> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
>> index e032b9bf4085..04bd5f5a806c 100644
>> --- a/drivers/thermal/of-thermal.c
>> +++ b/drivers/thermal/of-thermal.c
>> @@ -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;
>> +
>> + 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;
>
We actually found that this patch suffers from a fundamental problem which boils down to a
limitation of the match function.
The match function, as it exists now, requires one function per cooling
device and has insufficient data to be passed as arguments.
tbp[i].match(tz, cdev)
This only matches the cdev to the thermal zone but not necessarily to
the correct bind parameters. For example:
When the __bind calls for a match function the first cooling device that
it tries to bind gets associated with the first set of bind parameters.
Proposed changes:
The match function can be changed to:
int (*match) (struct thermal_bind_params* param, struct thermal_cooling_device *cdev);
where the invocation would be:
match(&tbp[i], cdev).
Some extra explanation (with the risk of being too verbose):
Just to throw some more light on what I think is going on in of-thermal
Lets say for a given device tree configuration (omitting weight/contribution):
trips {
threshold: trip-point@0 {
temperature = <55000>;
hysteresis = <1000>;
type = "passive";
};
target: trip-point@1 {
temperature = <65000>;
hysteresis = <1000>;
type = "passive";
};
};
cooling-maps {
map0 {
trip = <&target>;
cooling-device = <&cluster0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
};
map1 {
trip = <&target>;
cooling-device = <&cluster1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
};
};
The internal private data structure "__thermal_bind_params" is populated as:
__tbp[0].cooling_device = <node for cdev_cluster0>
__tbp[0].trip_id = 1;
__tbp[0].min = THERMAL_NO_LIMIT
__tbp[0].max = THERMAL_NO_LIMIT
__tbp[1].cooling_device = <node for cdev_cluster1>
__tbp[1].trip_id = 1;
__tbp[1].min = THERMAL_NO_LIMIT
__tbp[1].max = THERMAL_NO_LIMIT
The internal private data structure __thermal_trips data structure is populated as:
__trip[0].temperature = 55000
__trip[0].hysteresis = 1000
__trip[0].type = passive
__trip[1].temperature = 65000
__trip[1].hysteresis = 1000
__trip[1].type = passive
The expected thermal_bind_params (as I understand) should be:
tbp[0].cdev = cdev0 (cdev struct)
tbp[0].trip_mask = 0x2;
tbp[0].binding_limits = [THERMAL_NO_LIMIT, THERMAL_NO_LIMIT, THERMAL_NO_LIMIT, THERMAL_NO_LIMIT]
tbp[1].cdev = cdev1 (cdev struct)
tbp[1].trip_mask = 0x2;
tbp[1].binding_limits = [THERMAL_NO_LIMIT, THERMAL_NO_LIMIT, THERMAL_NO_LIMIT, THERMAL_NO_LIMIT]
Currently the match function (and my implementation) has no way of figuring out if the cooling device is associated with the correct set of trip_mask/weight etc.
> Please, use kernel error codes.
>
>> +}
>> +
>> +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)
>> + 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?
You're right. This should be __tbp[i].trip_id
>
>> + }
>> +
>> + 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;
>> + }
>> +
>> + /* 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?
Would be correcting this in a future revision of this as the match
function seems to rely on the order of the cooling devices that are matched.
>
>> + 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 */
>> + tzp->no_hwmon = true;
>> +
>> + return tzp;
>> +
>> + /* Error handling code */
>> + free_tbp:
>
> it is common to have labels in the first column.
Thanks. Will be updating the patch with the correct formatting.
>
>> + /* 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.
>
>> + kfree(tzp);
>> + return ERR_PTR(err);
>
> Remember to free the resources also in the of_thermal_destroy_zones().
Thanks for pointing this out.
>
>> +}
>> +
>> /**
>> * 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;
>> }
>>
>> - /* 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.
Sure, would be basing the submission on a public tree.
Regards!
KP
>
>>
>> On 05/01/15 21:31, Eduardo Valentin wrote:
>>> On Mon, Dec 22, 2014 at 04:36:26PM +0000, Kapileshwar Singh wrote:
>>>> Many thanks for looking through this! Had a few observations regarding
>>>> the same:
>>>> On 22/12/14 03:23, Zhang Rui wrote:
>>>>> On Thu, 2014-12-18 at 16:44 +0000, Kapileshwar Singh wrote:
>>>>>> Thanks for Reviewing this!
>>>>>>
>>>>>> On 18/12/14 13:43, Eduardo Valentin wrote:
>>>>>>> On Thu, Dec 18, 2014 at 12:19:00PM +0000, Kapileshwar Singh wrote:
>>>>>>>> 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.
>>>>>>> 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);
>>>>>> 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 above description is a correct understanding. You are right.
>>>
>>>> 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.
>>>
>>>> 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)?
>>>
>>>
>>>> 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.
>>>
>>>> Regards,
>>>> KP
>>>>
>>>>
>>>> thanks, rui
>>>>>>> 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
>>>>>>
>>>>>>
>>>>>>> I would expect, with the current code, the governor be silent and
>>>>>>> non-functional, which needs to be fixed too.
>>>>>>>> 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):
>>>>>>>>
>>>>>>>> diff --git a/drivers/thermal/db8500_thermal.c
>>>>>>>> b/drivers/thermal/db8500_thermal.c
>>>>>>>> index 1e3b3bf9f993..e3ccc2218eb3 100644
>>>>>>>> --- a/drivers/thermal/db8500_thermal.c
>>>>>>>> +++ b/drivers/thermal/db8500_thermal.c
>>>>>>>> @@ -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
>>>>>>>> dev_info(&cdev->device, "%s bind to %d: %d-%s\n", cdev->type,
>>>>>>>> i, ret, ret ? "fail" : "succeed");
>>>>>>>> diff --git a/drivers/thermal/fair_share.c b/drivers/thermal/fair_share.c
>>>>>>>> index 6e0a3fbfae86..c3b25187b467 100644
>>>>>>>> --- a/drivers/thermal/fair_share.c
>>>>>>>> +++ b/drivers/thermal/fair_share.c
>>>>>>>> @@ -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);
>>>>>>>> diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
>>>>>>>> index 5a1f1070b702..74d0c9951eef 100644
>>>>>>>> --- a/drivers/thermal/imx_thermal.c
>>>>>>>> +++ b/drivers/thermal/imx_thermal.c
>>>>>>>> @@ -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",
>>>>>>>> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
>>>>>>>> index e032b9bf4085..cf4036cbb671 100644
>>>>>>>> --- a/drivers/thermal/of-thermal.c
>>>>>>>> +++ b/drivers/thermal/of-thermal.c
>>>>>>>> @@ -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;
>>>>>>>> }
>>>>>>>> diff --git a/drivers/thermal/samsung/exynos_thermal_common.c
>>>>>>>> b/drivers/thermal/samsung/exynos_thermal_common.c
>>>>>>>> index b6be572704a4..d653798b519f 100644
>>>>>>>> --- a/drivers/thermal/samsung/exynos_thermal_common.c
>>>>>>>> +++ b/drivers/thermal/samsung/exynos_thermal_common.c
>>>>>>>> @@ -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;
>>>>>>>> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
>>>>>>>> index 4921e084c20b..199866864ef1 100644
>>>>>>>> --- a/drivers/thermal/thermal_core.c
>>>>>>>> +++ b/drivers/thermal/thermal_core.c
>>>>>>>> @@ -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);
>>>>>>>> }
>>>>>>>> @@ -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)
>>>>>>>> @@ -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.
>>>>>>>> @@ -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;
>>>>>>>> @@ -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)
>>>>>>>> diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h
>>>>>>>> index b907be823527..1c61abe7801f 100644
>>>>>>>> --- a/drivers/thermal/thermal_core.h
>>>>>>>> +++ b/drivers/thermal/thermal_core.h
>>>>>>>> @@ -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 *);
>>>>>>>> diff --git a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
>>>>>>>> b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
>>>>>>>> index 9eec26dc0448..772549ec9bd8 100644
>>>>>>>> --- a/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
>>>>>>>> +++ b/drivers/thermal/ti-soc-thermal/ti-thermal-common.c
>>>>>>>> @@ -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 */
>>>>>>>> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
>>>>>>>> index fc4fdcbbdecc..674795ba0900 100644
>>>>>>>> --- a/include/linux/thermal.h
>>>>>>>> +++ b/include/linux/thermal.h
>>>>>>>> @@ -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 *);
>>>>>>>>
>>>>>>>>
>>>>>
>>>>
>>
>>
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2015-01-07 14:08 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-12-18 12:19 Fix: Bind Cooling Device Weight Kapileshwar Singh
2014-12-18 13:43 ` Eduardo Valentin
2014-12-18 16:44 ` Kapileshwar Singh
2014-12-22 3:23 ` Zhang Rui
2014-12-22 16:36 ` Kapileshwar Singh
2015-01-05 21:31 ` Eduardo Valentin
2015-01-06 16:38 ` Kapileshwar Singh
2015-01-06 18:37 ` Eduardo Valentin
2015-01-07 14:08 ` Kapileshwar Singh
2014-12-22 3:18 ` Zhang Rui
2014-12-29 10:40 ` Javi Merino
2015-01-05 21:24 ` Eduardo Valentin
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.