| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20140819154004.GF2896@e104805>
Date: Tue, 19 Aug 2014 16:40:04 +0100
From: "Javi Merino" <javi.merino@....com>
To: "edubezval@...il.com" <edubezval@...il.com>
Cc: "linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
LKML <linux-kernel@...r.kernel.org>,
Punit Agrawal <Punit.Agrawal@....com>,
"broonie@...nel.org" <broonie@...nel.org>,
Zhang Rui <rui.zhang@...el.com>
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@...il.com wrote:
> Javi,
Hi Eduardo,
> On Thu, Jul 10, 2014 at 10:18 AM, Javi Merino <javi.merino@....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@...el.com>
> > Cc: Eduardo Valentin <edubezval@...il.com>
> > Signed-off-by: Javi Merino <javi.merino@....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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists