| Message ID | 5492C634.5030604@arm.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Eduardo Valentin |
| Headers | show |
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 *); > >
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 *); >> >> -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 *); > > > > -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 *); > >> > >> > > -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 *); >>>> >>>> >> > > -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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 *); > >>>> > >>>> > >> > > > > > >
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 *);
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): void thermal_zone_device_update(struct thermal_zone_device *); -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html