by Mikko Perttunen

[permalink] [raw]

Subject: Re: [PATCH 01/15] thermal: consistently use int for temperatures

On 05/20/15 11:34, Sascha Hauer wrote:
> On Wed, May 20, 2015 at 10:12:44AM +0300, Mikko Perttunen wrote:
>> On 05/13/15 11:52, Sascha Hauer wrote:
>>> The thermal code uses int, long and unsigned long for temperatures
>>> in different places. Using an unsigned type limits the thermal framework
>>> to positive temperatures without need. 'long' is 64bit on several
>>> architectures which is not needed. Consistently use a plain 'int'
>>> for temperatures.
>>>
>>> Signed-off-by: Sascha Hauer <[email protected]>
>>> ---
>>> ...
>>
>> It looks like some longs remain in db8500_thermal.c, int340x_thermal/*,
>> intel_soc_dts_thermal.c, kirkwood_thermal.c, rockchip_thermal.c and
>> samsung/exynos_tmu.c. (found by 'grep -R long drivers/thermal/*)
>
> I looked over it again and found the following. Sorry, I really thought
> I had compile tested all drivers. I found nothing in db8500_thermal.c,
> intel_soc_dts_thermal.c and rockchip_thermal.c though. There are some
> (unsigned)longs used as temperatures in these files, but I haven't
> changed them on purpose to not make the patch bigger as necessary. I
> only changed the places where (unsigned)longs are used as pointers.
> Do you think it's necessary to change all temperatures, even the ones
> only used internally in drivers?
>
> Sascha

I guess changing just the framework interface would be fine for now,
though I don't think there are that many internal uses remaining either.

Reviewed-by: Mikko Perttunen <[email protected]>

Cheers,
Mikko.

>
> -----------------------8<------------------
>
>>From 4cf8fe3b3ef1bd9db6090305ea2b9995f0dbffa4 Mon Sep 17 00:00:00 2001
> From: Sascha Hauer <[email protected]>
> Date: Wed, 20 May 2015 10:28:39 +0200
> Subject: [PATCH] fixup! thermal: consistently use int for temperatures
>
> ---
> drivers/thermal/int340x_thermal/int3400_thermal.c | 4 ++--
> drivers/thermal/int340x_thermal/processor_thermal_device.c | 4 ++--
> drivers/thermal/kirkwood_thermal.c | 2 +-
> drivers/thermal/samsung/exynos_tmu.c | 2 +-
> 4 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/thermal/int340x_thermal/int3400_thermal.c b/drivers/thermal/int340x_thermal/int3400_thermal.c
> index 96bdf8a..5836e55 100644
> --- a/drivers/thermal/int340x_thermal/int3400_thermal.c
> +++ b/drivers/thermal/int340x_thermal/int3400_thermal.c
> @@ -186,7 +186,7 @@ static int int3400_thermal_run_osc(acpi_handle handle,
> }
>
> static int int3400_thermal_get_temp(struct thermal_zone_device *thermal,
> - unsigned long *temp)
> + int *temp)
> {
> *temp = 20 * 1000; /* faked temp sensor with 20C */
> return 0;
> @@ -231,7 +231,7 @@ static int int3400_thermal_set_mode(struct thermal_zone_device *thermal,
> return result;
> }
>
> -static const struct thermal_zone_device_ops int3400_thermal_ops = {
> +static struct thermal_zone_device_ops int3400_thermal_ops = {
> .get_temp = int3400_thermal_get_temp,
> };
>
> diff --git a/drivers/thermal/int340x_thermal/processor_thermal_device.c b/drivers/thermal/int340x_thermal/processor_thermal_device.c
> index 5e8d8e9..c8afd34 100644
> --- a/drivers/thermal/int340x_thermal/processor_thermal_device.c
> +++ b/drivers/thermal/int340x_thermal/processor_thermal_device.c
> @@ -139,7 +139,7 @@ static int get_tjmax(void)
> return -EINVAL;
> }
>
> -static int read_temp_msr(unsigned long *temp)
> +static int read_temp_msr(int *temp)
> {
> int cpu;
> u32 eax, edx;
> @@ -171,7 +171,7 @@ err_ret:
> }
>
> static int proc_thermal_get_zone_temp(struct thermal_zone_device *zone,
> - unsigned long *temp)
> + int *temp)
> {
> int ret;
>
> diff --git a/drivers/thermal/kirkwood_thermal.c b/drivers/thermal/kirkwood_thermal.c
> index abba3e2..5bcdbd6 100644
> --- a/drivers/thermal/kirkwood_thermal.c
> +++ b/drivers/thermal/kirkwood_thermal.c
> @@ -33,7 +33,7 @@ struct kirkwood_thermal_priv {
> };
>
> static int kirkwood_get_temp(struct thermal_zone_device *thermal,
> - unsigned long *temp)
> + int *temp)
> {
> unsigned long reg;
> struct kirkwood_thermal_priv *priv = thermal->devdata;
> diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
> index 29eaf4d..ed55fd9 100644
> --- a/drivers/thermal/samsung/exynos_tmu.c
> +++ b/drivers/thermal/samsung/exynos_tmu.c
> @@ -812,7 +812,7 @@ out:
> #else
> #define exynos4412_tmu_set_emulation NULL
> #define exynos5440_tmu_set_emulation NULL
> -static int exynos_tmu_set_emulation(void *drv_data, unsigned long temp)
> +static int exynos_tmu_set_emulation(void *drv_data, int temp)
> { return -EINVAL; }
> #endif /* CONFIG_THERMAL_EMULATION */
>
>

2015-05-20 13:23:57

by Sascha Hauer

[permalink] [raw]

Subject: Re: [PATCH 11/15] thermal: thermal: Add support for hardware-tracked trip points

On Tue, May 19, 2015 at 05:05:29PM +0300, Mikko Perttunen wrote:
> On 05/19/15 16:58, Sascha Hauer wrote:
> >On Mon, May 18, 2015 at 02:09:44PM +0200, Sascha Hauer wrote:
> >>Hi Mikko,
> >>
> >>On Mon, May 18, 2015 at 12:06:50PM +0300, Mikko Perttunen wrote:
> >>>>+ for (i = 0; i < tz->trips; i++) {
> >>>>+ int trip_low;
> >>>>+
> >>>>+ tz->ops->get_trip_temp(tz, i, &trip_temp);
> >>>>+ tz->ops->get_trip_hyst(tz, i, &hysteresis);
> >>>>+
> >>>>+ trip_low = trip_temp - hysteresis;
> >>>>+
> >>>>+ if (trip_low < temp && trip_low > low)
> >>>>+ low = trip_low;
> >>>>+
> >>>>+ if (trip_temp > temp && trip_temp < high)
> >>>>+ high = trip_temp;
> >>>>+ }
> >>>>+
> >>>>+ tz->prev_low_trip = low;
> >>>>+ tz->prev_high_trip = high;
> >>>>+
> >>>>+ dev_dbg(&tz->device, "new temperature boundaries: %d < x < %d\n",
> >>>>+ low, high);
> >>>>+
> >>>>+ tz->ops->set_trips(tz, low, high);
> >>>
> >>>This should probably do something if set_trips returns an error
> >>>code; at least an error message, perhaps enable polling? I'm not
> >>>exactly sure what safety features the thermal framework has in
> >>>general if errors happen..
> >>
> >>Currently a thermal zone has the passive_delay and polling_delay
> >>variables. If these are nonzero the thermal core will always poll. A
> >>purely interrupt driven thermal zone would set these values to zero.
> >>In this case the thermal core has no basis for polling, so we would
> >>have to make up polling intervals when set_trips fails. Another
> >>possibility would be to interpret the *_delay variables as 'when
> >>set_trips is available, do not poll. When something goes wrong, use
> >>*_delay as polling intervals'
> >>
> >>>
> >>>One interesting thing I noticed was that at least the bang-bang
> >>>governor only acts if the temperature is properly smaller than (trip
> >>>temp - hysteresis). So perhaps we should specify the non-tripping
> >>>range as [low, high)? Or we could change bang-bang.
> >>
> >>I wonder how we can protect against such off-by-one errors anyway.
> >>Generally a hardware might operate on raw values rather than directly
> >>in temperature values in ?C. This means a driver for this must have
> >>celsius_to_raw and raw_to_celsius conversion functions. Now it can
> >>happen that due to rounding errors celsius_to_raw(Tcrit) returns a raw
> >>value that when converted back to celsius is different from the
> >>original value in ?C. This would mean the hardware triggers an interrupt
> >>for a trip point and the thermal core does not react because get_temp
> >>actually returns a different temperature than previously programmed as
> >>interrupt trigger. This way we would lose hot (or cold) events.
> >
> >As a simple example we could imagine a 12bit adc which has:
> >
> >u32 mcelsius_to_raw(int temp)
> >{
> > return temp / 30;
> >}
> >
> >int raw_to_mcelsius(u32 raw)
> >{
> > return temp * 30;
> >}
> >
> >Now if the thermal framework requests an interrupt at 77000mC we
> >would program a raw value of 77000 / 30 = 2566.666667, due to integer
> >rounding we would program 2566. Now when the interrupt is triggered with
> >this exact raw value we would convert it back to 2566 * 30 = 76980. The
> >thermal framework would realize that this is below the threshold, do
> >nothing and go back to sleep.
> >I am beginning to think that implementing interrupts like this is not a
> >good idea, at least I found no convenient way out of this situation.
>
> Couldn't you just specify that the driver should do the best it can?
> That is, in this case, the driver would program the hardware for the
> least possible value x for which raw_to_mcelsius(x) >= 77000.

That's what I did now.

Sascha

--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |