| 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: <53DB7D0D.1070508@nvidia.com>
Date: Fri, 1 Aug 2014 14:42:05 +0300
From: Mikko Perttunen <mperttunen@...dia.com>
To: Eduardo Valentin <edubezval@...il.com>
CC: "rui.zhang@...el.com" <rui.zhang@...el.com>,
"swarren@...dotorg.org" <swarren@...dotorg.org>,
"thierry.reding@...il.com" <thierry.reding@...il.com>,
Peter De Schrijver <pdeschrijver@...dia.com>,
Matthew Longnecker <MLongnecker@...dia.com>,
"linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
"linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH 1/6] thermal: of: Add support for hardware-tracked trip
points
Moi Eduardo :)
On 30/07/14 17:16, Eduardo Valentin wrote:
> Terve Mikko,
>
> On Fri, Jun 27, 2014 at 11:11:34AM +0300, Mikko Perttunen wrote:
>> This adds support for hardware-tracked trip points to the device tree
>> thermal sensor framework.
>>
>> The framework supports an arbitrary number of trip points. Whenever
>> the current temperature is updated, the trip points immediately
>> below and above the current temperature are found. A sensor driver
>
> One thing I don't follow on your proposal is the groundings you need to
> 'set_trips' whenever temperature changes. Given your intention is to add
> support to interrupt driven devices, shouldn't we 'set_trips' just when
> we cross the previously set trips range?
I think the reasoning for this was that I didn't want to make changes to
thermal_core and thermal_zone_device_update only calls get_temp on the
zone, so I had to add this code to get_temp. set_trips would anyway only
be called if we had crossed a trip point.
>
>> callback `set_trips' is then called with the temperatures.
>> If there is no trip point above or below the current temperature,
>> the passed trip temperature will be LONG_MAX or LONG_MIN respectively.
>> In this callback, the driver should program the hardware such that
>> it is notified when either of these trip points are triggered.
>> When a trip point is triggered, the driver should call
>> `thermal_zone_device_update' for the respective thermal zone. This
>> will cause the trip points to be updated again.
>>
>> If the `set_trips' callback is not implemented (is NULL), the framework
>> behaves as before.
>
> As already mentioned by swarren, the proposal must be wider. We shall
> keep the same support in case the device is used in a system without
> device tree. In other words, if you want to see extra functionality for
> interrupt driven devices, you shall update the core part too, and draft
> a common messaging path.
Yeah, this is sensible. A simpler solution would be to just tell
of-thermal drivers about the trip points and let the driver do whatever
it wants. That would mirror the way normal thermal_core drivers are
done. What is your opinion on that?
>
> In general, interrupt driven devices are not mapped in the current
> thermal framework. That is, the current code is timer interrupt driven.
> Other interrupt updates from devices are propagated to
> the framework using thermal_zone_device_update(). In other words, you would
> reprogram your hardware trips from your interrupt handler/workqueue then
> just let the framework know what is going on with temperature, via a simple
> thermal_zone_device_update().
>
> The way I see this going forward it would be a common interface to
> configure the thermal zones to be monitored:
> a. via polling only
> b. via interrupt only
> c. both a + b
>
> obviously, the above shall be informative only for userland.
>
> keep in mind also that changing interrupt configuration for high and low
> temperature thresholds can be racy.
>
> This feature was kept in the TODO list of the of-thermal.c because the
> we lack a proper support from the thermal framework (never came out of
> the TODO list, I know, I apologize for this). And this missing feature
> was spotted by the hwmon folks also, as they do have such support. So,
> the major missing improvements on interrupt driven devices shall come in
> three steps: (i) thermal framework, (ii) of-thermal (iii) thermal
> framework and hwmon interface.
For now, I think I'll submit a driver with just polling support so that
we can get some support in.
>
>
> Cheers,
>
>
Thanks, Mikko
>>
>> Signed-off-by: Mikko Perttunen <mperttunen@...dia.com>
>> ---
>> drivers/thermal/of-thermal.c | 97 ++++++++++++++++++++++++++++++++++++++++++--
>> include/linux/thermal.h | 3 +-
>> 2 files changed, 95 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
>> index 04b1be7..bfccea5 100644
>> --- a/drivers/thermal/of-thermal.c
>> +++ b/drivers/thermal/of-thermal.c
>> @@ -89,6 +89,7 @@ struct __thermal_zone {
>> /* trip data */
>> int ntrips;
>> struct __thermal_trip *trips;
>> + long prev_low_trip, prev_high_trip;
>>
>> /* cooling binding data */
>> int num_tbps;
>> @@ -98,19 +99,66 @@ struct __thermal_zone {
>> void *sensor_data;
>> int (*get_temp)(void *, long *);
>> int (*get_trend)(void *, long *);
>> + int (*set_trips)(void *, long, long);
>> };
>>
>> +/*** Automatic trip handling ***/
>> +
>> +static int of_thermal_set_trips(struct thermal_zone_device *tz, long temp)
>> +{
>> + struct __thermal_zone *data = tz->devdata;
>> + long low = LONG_MIN, high = LONG_MAX;
>> + int i;
>> +
>> + /* Hardware trip points not supported */
>> + if (!data->set_trips)
>> + return 0;
>> +
>> + /* No need to change trip points */
>> + if (temp > data->prev_low_trip && temp < data->prev_high_trip)
>> + return 0;
>> +
>> + for (i = 0; i < data->ntrips; ++i) {
>> + struct __thermal_trip *trip = data->trips + i;
>> + long trip_low = trip->temperature - trip->hysteresis;
>> +
>> + if (trip_low < temp && trip_low > low)
>> + low = trip_low;
>> +
>> + if (trip->temperature > temp && trip->temperature < high)
>> + high = trip->temperature;
>> + }
>> +
>> + dev_dbg(&tz->device,
>> + "temperature %ld, updating trip points to %ld, %ld\n",
>> + temp, low, high);
>> +
>> + data->prev_low_trip = low;
>> + data->prev_high_trip = high;
>> +
>> + return data->set_trips(data->sensor_data, low, high);
>> +}
>> +
>> /*** DT thermal zone device callbacks ***/
>>
>> static int of_thermal_get_temp(struct thermal_zone_device *tz,
>> unsigned long *temp)
>> {
>> struct __thermal_zone *data = tz->devdata;
>> + int err;
>>
>> if (!data->get_temp)
>> return -EINVAL;
>>
>> - return data->get_temp(data->sensor_data, temp);
>> + err = data->get_temp(data->sensor_data, temp);
>> + if (err)
>> + return err;
>> +
>> + err = of_thermal_set_trips(tz, *temp);
>
> Here, if you update trips whenever you get_temp, you are possibly
> reprogramming your trips on every poll. Remember, this function will be
> called on every poll, in the current implementation.
>
>> + if (err)
>> + return err;
>> +
>> + return 0;
>> }
>>
>> static int of_thermal_get_trend(struct thermal_zone_device *tz, int trip,
>> @@ -222,6 +270,22 @@ static int of_thermal_set_mode(struct thermal_zone_device *tz,
>> return 0;
>> }
>>
>> +static int of_thermal_update_trips(struct thermal_zone_device *tz)
>> +{
>> + long temp;
>> + int err;
>> +
>> + err = of_thermal_get_temp(tz, &temp);
>> + if (err)
>> + return err;
>> +
>> + err = of_thermal_set_trips(tz, temp);
>> + if (err)
>> + return err;
>> +
>> + return 0;
>> +}
>> +
>> static int of_thermal_get_trip_type(struct thermal_zone_device *tz, int trip,
>> enum thermal_trip_type *type)
>> {
>> @@ -252,6 +316,7 @@ static int of_thermal_set_trip_temp(struct thermal_zone_device *tz, int trip,
>> unsigned long temp)
>> {
>> struct __thermal_zone *data = tz->devdata;
>> + int err;
>>
>> if (trip >= data->ntrips || trip < 0)
>> return -EDOM;
>> @@ -259,6 +324,10 @@ static int of_thermal_set_trip_temp(struct thermal_zone_device *tz, int trip,
>> /* thermal framework should take care of data->mask & (1 << trip) */
>> data->trips[trip].temperature = temp;
>>
>> + err = of_thermal_update_trips(tz);
>> + if (err)
>> + return err;
>> +
>> return 0;
>> }
>>
>> @@ -279,6 +348,7 @@ static int of_thermal_set_trip_hyst(struct thermal_zone_device *tz, int trip,
>> unsigned long hyst)
>> {
>> struct __thermal_zone *data = tz->devdata;
>> + int err;
>>
>> if (trip >= data->ntrips || trip < 0)
>> return -EDOM;
>> @@ -286,6 +356,10 @@ static int of_thermal_set_trip_hyst(struct thermal_zone_device *tz, int trip,
>> /* thermal framework should take care of data->mask & (1 << trip) */
>> data->trips[trip].hysteresis = hyst;
>>
>> + err = of_thermal_update_trips(tz);
>> + if (err)
>> + return err;
>> +
>> return 0;
>> }
>>
>> @@ -325,10 +399,12 @@ static struct thermal_zone_device *
>> thermal_zone_of_add_sensor(struct device_node *zone,
>> struct device_node *sensor, void *data,
>> int (*get_temp)(void *, long *),
>> - int (*get_trend)(void *, long *))
>> + int (*get_trend)(void *, long *),
>> + int (*set_trips)(void *, long, long))
>
> we need to clean the above arguments. they should become a .ops.
>
>> {
>> struct thermal_zone_device *tzd;
>> struct __thermal_zone *tz;
>> + int err;
>>
>> tzd = thermal_zone_get_zone_by_name(zone->name);
>> if (IS_ERR(tzd))
>> @@ -339,8 +415,15 @@ thermal_zone_of_add_sensor(struct device_node *zone,
>> mutex_lock(&tzd->lock);
>> tz->get_temp = get_temp;
>> tz->get_trend = get_trend;
>> + tz->set_trips = set_trips;
>> tz->sensor_data = data;
>>
>> + err = of_thermal_update_trips(tzd);
>> + if (err) {
>> + mutex_unlock(&tzd->lock);
>> + return ERR_PTR(err);
>> + }
>> +
>> tzd->ops->get_temp = of_thermal_get_temp;
>> tzd->ops->get_trend = of_thermal_get_trend;
>> mutex_unlock(&tzd->lock);
>> @@ -384,7 +467,8 @@ thermal_zone_of_add_sensor(struct device_node *zone,
>> struct thermal_zone_device *
>> thermal_zone_of_sensor_register(struct device *dev, int sensor_id,
>> void *data, int (*get_temp)(void *, long *),
>> - int (*get_trend)(void *, long *))
>> + int (*get_trend)(void *, long *),
>> + int (*set_trips)(void *, long, long))
>
> ditto.
>
>> {
>> struct device_node *np, *child, *sensor_np;
>>
>> @@ -422,7 +506,8 @@ thermal_zone_of_sensor_register(struct device *dev, int sensor_id,
>> return thermal_zone_of_add_sensor(child, sensor_np,
>> data,
>> get_temp,
>> - get_trend);
>> + get_trend,
>> + set_trips);
>
> ditto.
>
>> }
>> }
>> of_node_put(np);
>> @@ -466,6 +551,7 @@ void thermal_zone_of_sensor_unregister(struct device *dev,
>>
>> tz->get_temp = NULL;
>> tz->get_trend = NULL;
>> + tz->set_trips = NULL;
>> tz->sensor_data = NULL;
>> mutex_unlock(&tzd->lock);
>> }
>> @@ -671,6 +757,9 @@ thermal_of_build_thermal_zone(struct device_node *np)
>> /* trips */
>> child = of_get_child_by_name(np, "trips");
>>
>> + tz->prev_high_trip = LONG_MIN;
>> + tz->prev_low_trip = LONG_MAX;
>> +
>> /* No trips provided */
>> if (!child)
>> goto finish;
>> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
>> index f7e11c7..2f8951c 100644
>> --- a/include/linux/thermal.h
>> +++ b/include/linux/thermal.h
>> @@ -248,7 +248,8 @@ struct thermal_genl_event {
>> struct thermal_zone_device *
>> thermal_zone_of_sensor_register(struct device *dev, int id,
>> void *data, int (*get_temp)(void *, long *),
>> - int (*get_trend)(void *, long *));
>> + int (*get_trend)(void *, long *),
>> + int (*set_trips)(void *, long, long));
>> void thermal_zone_of_sensor_unregister(struct device *dev,
>> struct thermal_zone_device *tz);
>> #else
>> --
>> 1.8.1.5
>>
>
--
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