Messages in this thread | | | From | "Javi Merino" <> | Date | Tue, 19 Aug 2014 16:40:04 +0100 | Subject | Re: [RFC PATCH v5 05/10] thermal: let governors have private data for each thermal zone |
| |
On Tue, Aug 19, 2014 at 01:49:32PM +0100, edubezval@gmail.com wrote: > Javi,
Hi Eduardo,
> On Thu, Jul 10, 2014 at 10:18 AM, Javi Merino <javi.merino@arm.com> wrote: > > A governor may need to store its current state between calls to > > throttle(). That state depends on the thermal zone, so store it as > > private data in struct thermal_zone_device. > > > > The governors may have two new ops: bind_to_tz() and unbind_from_tz(). > > When provided, these functions let governors do some initialization > > and teardown when they are bound/unbound to a tz and possibly store that > > information in the governor_data field of the struct > > thermal_zone_device. > > > > Cc: Zhang Rui <rui.zhang@intel.com> > > Cc: Eduardo Valentin <edubezval@gmail.com> > > Signed-off-by: Javi Merino <javi.merino@arm.com> > > --- > > drivers/thermal/thermal_core.c | 83 ++++++++++++++++++++++++++++++++++++++---- > > include/linux/thermal.h | 9 +++++ > > 2 files changed, 84 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c > > index 71b0ec0c370d..3da99dd80ad5 100644 > > --- a/drivers/thermal/thermal_core.c > > +++ b/drivers/thermal/thermal_core.c > > @@ -72,6 +72,58 @@ static struct thermal_governor *__find_governor(const char *name) > > return NULL; > > } > > > > +/** > > + * bind_previous_governor() - bind the previous governor of the thermal zone > > + * @tz: a valid pointer to a struct thermal_zone_device > > + * @failed_gov_name: the name of the governor that failed to register > > + * > > + * Register the previous governor of the thermal zone after a new > > + * governor has failed to be bound. > > + */ > > +static void bind_previous_governor(struct thermal_zone_device *tz, > > + const char *failed_gov_name) > > +{ > > + if (tz->governor && tz->governor->bind_to_tz) { > > + if (tz->governor->bind_to_tz(tz)) { > > + dev_warn(&tz->device, > > + "governor %s failed to bind and the previous one (%s) failed to register again, thermal zone %s has no governor\n", > > + failed_gov_name, tz->governor->name, tz->type); > > The above must be a dev_err(), not warn.
Sure
> Besides, I would prefer if > you could improve the message. What is the difference between register > and bind?
None. It should say bind in both, it'll make it clearer. I'll fix it.
> > + tz->governor = NULL; > > + } > > + } > > +} > > + > > +/** > > + * thermal_set_governor() - Switch to another governor > > + * @tz: a valid pointer to a struct thermal_zone_device > > + * @new_gov: pointer to the new governor > > + * > > + * Change the governor of thermal zone @tz. > > + * > > + * Return: 0 on success, an error if the new governor's bind_to_tz() failed. > > + */ > > +static int thermal_set_governor(struct thermal_zone_device *tz, > > + struct thermal_governor *new_gov) > > +{ > > + int ret = 0; > > + > > + if (tz->governor && tz->governor->unbind_from_tz) > > + tz->governor->unbind_from_tz(tz); > > + > > + if (new_gov && new_gov->bind_to_tz) { > > + ret = new_gov->bind_to_tz(tz); > > + if (ret) { > > + bind_previous_governor(tz, new_gov->name); > > + > > + return ret; > > + } > > + } > > + > > + tz->governor = new_gov; > > + > > + return ret; > > +} > > + > > int thermal_register_governor(struct thermal_governor *governor) > > { > > int err; > > @@ -104,8 +156,15 @@ int thermal_register_governor(struct thermal_governor *governor) > > > > name = pos->tzp->governor_name; > > > > - if (!strnicmp(name, governor->name, THERMAL_NAME_LENGTH)) > > - pos->governor = governor; > > + if (!strnicmp(name, governor->name, THERMAL_NAME_LENGTH)) { > > + int ret; > > + > > + ret = thermal_set_governor(pos, governor); > > + if (ret) > > + dev_warn(&pos->device, > > + "Failed to set governor %s for thermal zone %s: %d\n", > > + governor->name, pos->type, ret); > > dev_err here too.
Ok. Thanks! Javi
> > + } > > } > > > > mutex_unlock(&thermal_list_lock); > > @@ -131,7 +190,7 @@ void thermal_unregister_governor(struct thermal_governor *governor) > > list_for_each_entry(pos, &thermal_tz_list, node) { > > if (!strnicmp(pos->governor->name, governor->name, > > THERMAL_NAME_LENGTH)) > > - pos->governor = NULL; > > + thermal_set_governor(pos, NULL); > > } > > > > mutex_unlock(&thermal_list_lock); > > @@ -756,8 +815,9 @@ policy_store(struct device *dev, struct device_attribute *attr, > > if (!gov) > > goto exit; > > > > - tz->governor = gov; > > - ret = count; > > + ret = thermal_set_governor(tz, gov); > > + if (!ret) > > + ret = count; > > > > exit: > > mutex_unlock(&thermal_governor_lock); > > @@ -1452,6 +1512,7 @@ struct thermal_zone_device *thermal_zone_device_register(const char *type, > > int result; > > int count; > > int passive = 0; > > + struct thermal_governor *governor; > > > > if (type && strlen(type) >= THERMAL_NAME_LENGTH) > > return ERR_PTR(-EINVAL); > > @@ -1542,9 +1603,15 @@ struct thermal_zone_device *thermal_zone_device_register(const char *type, > > mutex_lock(&thermal_governor_lock); > > > > if (tz->tzp) > > - tz->governor = __find_governor(tz->tzp->governor_name); > > + governor = __find_governor(tz->tzp->governor_name); > > else > > - tz->governor = def_governor; > > + governor = def_governor; > > + > > + result = thermal_set_governor(tz, governor); > > + if (result) { > > + mutex_unlock(&thermal_governor_lock); > > + goto unregister; > > + } > > > > mutex_unlock(&thermal_governor_lock); > > > > @@ -1634,7 +1701,7 @@ void thermal_zone_device_unregister(struct thermal_zone_device *tz) > > device_remove_file(&tz->device, &dev_attr_mode); > > device_remove_file(&tz->device, &dev_attr_policy); > > remove_trip_attrs(tz); > > - tz->governor = NULL; > > + thermal_set_governor(tz, NULL); > > > > thermal_remove_hwmon_sysfs(tz); > > release_idr(&thermal_tz_idr, &thermal_idr_lock, tz->id); > > diff --git a/include/linux/thermal.h b/include/linux/thermal.h > > index 0305cde21a74..1124b7a9358a 100644 > > --- a/include/linux/thermal.h > > +++ b/include/linux/thermal.h > > @@ -187,6 +187,7 @@ struct thermal_attr { > > * @ops: operations this &thermal_zone_device supports > > * @tzp: thermal zone parameters > > * @governor: pointer to the governor for this thermal zone > > + * @governor_data: private pointer for governor data > > * @thermal_instances: list of &struct thermal_instance of this thermal zone > > * @idr: &struct idr to generate unique id for this zone's cooling > > * devices > > @@ -213,6 +214,7 @@ struct thermal_zone_device { > > struct thermal_zone_device_ops *ops; > > const struct thermal_zone_params *tzp; > > struct thermal_governor *governor; > > + void *governor_data; > > struct list_head thermal_instances; > > struct idr idr; > > struct mutex lock; > > @@ -223,12 +225,19 @@ struct thermal_zone_device { > > /** > > * struct thermal_governor - structure that holds thermal governor information > > * @name: name of the governor > > + * @bind_to_tz: callback called when binding to a thermal zone. If it > > + * returns 0, the governor is bound to the thermal zone, > > + * otherwise it fails. > > + * @unbind_from_tz: callback called when a governor is unbound from a > > + * thermal zone. > > * @throttle: callback called for every trip point even if temperature is > > * below the trip point temperature > > * @governor_list: node in thermal_governor_list (in thermal_core.c) > > */ > > struct thermal_governor { > > char name[THERMAL_NAME_LENGTH]; > > + int (*bind_to_tz)(struct thermal_zone_device *tz); > > + void (*unbind_from_tz)(struct thermal_zone_device *tz); > > int (*throttle)(struct thermal_zone_device *tz, int trip); > > struct list_head governor_list; > > }; > > -- > > 1.9.1 > > > > > > > > -- > Eduardo Bezerra Valentin >
| |