[GIT PULL] Thermal management updates for v4.17-rc1

[GIT PULL] Thermal management updates for v4.17-rc1

[Zhang Rui]

Hi, Linus,

Please pull from
  git://git.kernel.org/pub/scm/linux/kernel/git/rzhang/linux.git next

to receive the latest Thermal Management updates for v4.17-rc1 with
top-most commit f8837aac36cdc7430422cd65f4466071b42654bb:

  Merge branches 'thermal-core' and 'thermal-soc' into next (2018-04-02 
21:49:31 +0800)

on top of commit 0c8efd610b58cb23cefdfa12015799079aef94ae:

  Linux 4.16-rc5 (2018-03-11 17:25:09 -0700)

Specifics:

- Fix race condition in imx_thermal_probe(). (Mikhail Lappo)

- Add cooling device's statistics in sysfs. (Viresh Kumar)

- add support for i.MX7 thermal sensor in imx_thermal driver. (Anson
Huang)

- add support for MT7622 SoC in mtk_thermal driver. (Sean Wang)

- Remove unused min/max cpu cooling DT property. (Viresh Kumar).

- A series of fixes on exynos driver. (Bartlomiej Zolnierkiewicz,
Maciej Purski, Marek Szyprowski)

thanks,
rui



----------------------------------------------------------------
Anson Huang (1):
      thermal: imx: add i.MX7 thermal sensor support

Bartlomiej Zolnierkiewicz (10):
      thermal: exynos: remove unused "type" field from struct
exynos_tmu_platform_data
      thermal: exynos: remove parsing of samsung,
tmu_default_temp_offset property
      thermal: exynos: remove parsing of samsung, tmu_[first,
second]_point_trim properties
      thermal: exynos: remove parsing of samsung, tmu_noise_cancel_mode
property
      thermal: exynos: remove parsing of samsung, tmu[_min,
_max]_efuse_value properties
      thermal: exynos: remove parsing of samsung, tmu_reference_voltage
property
      thermal: exynos: remove parsing of samsung,tmu_gain property
      thermal: exynos: remove parsing of samsung, tmu_cal_type property
      thermal: exynos: remove separate exynos_tmu.h header file
      dt-bindings: thermal: remove no longer needed samsung thermal
properties

Maciej Purski (1):
      thermal: exynos: Read soc_type from match data

Marek Szyprowski (2):
      thermal: exynos: Reading temperature makes sense only when TMU is
turned on
      thermal: exynos: Propagate error value from tmu_read()

Mikhail Lappo (1):
      thermal: imx: Fix race condition in imx_thermal_probe()

Sean Wang (2):
      dt-bindings: thermal: add binding for MT7622 SoC
      thermal: mediatek: add support for MT7622 SoC

Viresh Kumar (2):
      dt-bindings: thermal: Remove "cooling-{min|max}-level" properties
      thermal: Add cooling device's statistics in sysfs

Zhang Rui (2):
      Merge branch 'linus' of git://git.kernel.org/.../evalenti/linux-
soc-thermal into thermal-soc
      Merge branches 'thermal-core' and 'thermal-soc' into next

 .../devicetree/bindings/thermal/exynos-thermal.txt |  23 +-
 .../devicetree/bindings/thermal/imx-thermal.txt    |   9 +-
 .../bindings/thermal/mediatek-thermal.txt          |   1 +
 .../devicetree/bindings/thermal/thermal.txt        |  16 +-
 Documentation/thermal/sysfs-api.txt                |  31 +++
 drivers/thermal/Kconfig                            |   7 +
 drivers/thermal/imx_thermal.c                      | 301
++++++++++++++++-----
 drivers/thermal/mtk_thermal.c                      |  35 +++
 drivers/thermal/samsung/exynos_tmu.c               | 268 +++++++++--
-------
 drivers/thermal/samsung/exynos_tmu.h               |  75 -----
 drivers/thermal/thermal_core.c                     |   3 +-
 drivers/thermal/thermal_core.h                     |  10 +
 drivers/thermal/thermal_helpers.c                  |   5 +-
 drivers/thermal/thermal_sysfs.c                    | 225
+++++++++++++++
 include/linux/thermal.h                            |   1 +
 15 files changed, 706 insertions(+), 304 deletions(-)
 delete mode 100644 drivers/thermal/samsung/exynos_tmu.h

Re: [GIT PULL] Thermal management updates for v4.17-rc1

[Linus Torvalds]

On Wed, Apr 11, 2018 at 1:41 AM, Zhang Rui <rui.zhang@intel.com> wrote:
>
> Please pull from
>   git://git.kernel.org/pub/scm/linux/kernel/git/rzhang/linux.git next

Pulled, and then immediately unpulled again.

The code causes new compiler warnings, and the warnings are valid.

If people don't care enough about their code to even check the
warnings, I'm not going to waste one second pulling the resulting
garbage. It's that simple.

                    Linus

Re: [GIT PULL] Thermal management updates for v4.17-rc1

[Zhang Rui]

Hi, Linus,

On 三, 2018-04-11 at 17:01 -0700, Linus Torvalds wrote:
> On Wed, Apr 11, 2018 at 1:41 AM, Zhang Rui <rui.zhang@intel.com>
> wrote:
> > 
> > 
> > Please pull from
> >   git://git.kernel.org/pub/scm/linux/kernel/git/rzhang/linux.git
> > next
> Pulled, and then immediately unpulled again.
> 
> The code causes new compiler warnings, and the warnings are valid.
> 
> If people don't care enough about their code to even check the
> warnings, I'm not going to waste one second pulling the resulting
> garbage. It's that simple.
> 

I'm really sorry for this.
could you please illustrate me what the kconfig & warning is?
I didn't
get such warnings from 0-day.

thanks,
rui

Re: [GIT PULL] Thermal management updates for v4.17-rc1

[Linus Torvalds]

On Wed, Apr 11, 2018 at 10:08 PM, Zhang Rui <rui.zhang@intel.com> wrote:
>
> could you please illustrate me what the kconfig & warning is?

Just "make allmodconfig" and the warning is about a uninitialized variable.

Line 304 in drivers/thermal/samsung/exynos_tmu.c if my shell history
is to be believed.

                Linus

Re: [GIT PULL] Thermal management updates for v4.17-rc1

[Daniel Lezcano]

On 12/04/2018 18:55, Linus Torvalds wrote:
> On Wed, Apr 11, 2018 at 10:08 PM, Zhang Rui <rui.zhang@intel.com> wrote:
>>
>> could you please illustrate me what the kconfig & warning is?
> 
> Just "make allmodconfig" and the warning is about a uninitialized variable.
> 
> Line 304 in drivers/thermal/samsung/exynos_tmu.c if my shell history
> is to be believed.
> 
>                 Linus

These couple of warnings were introduced by:

commit 480b5bfc16e17ef51ca1c55bfcebf55db8673ebf
Author: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Date:   Tue Mar 6 15:43:45 2018 +0100

    thermal: exynos: remove parsing of samsung, tmu_default_temp_offset
property

    Trimming (one point based or two points based) is always used for
    the temperature calibration and the default non-trimming code should
    never be reached.

    Modify temp_to_code() and code_to_temp() accordingly (WARN_ON(1)
    in the default cases) and then remove no longer needed parsing of
    samsung,tmu_default_temp_offset property.

    There should be no functional changes caused by this patch.

    Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
    Signed-off-by: Eduardo Valentin <edubezval@gmail.com>


After digging into, there is no obvious fix. It returns effectively an
uninitialized value and the callers are assuming the value is always
correct, so it is also not possible to simply return an error.


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [GIT PULL] Thermal management updates for v4.17-rc1

[Eduardo Valentin]

Hello,

On Thu, Apr 12, 2018 at 09:55:19AM -0700, Linus Torvalds wrote:
> On Wed, Apr 11, 2018 at 10:08 PM, Zhang Rui <rui.zhang@intel.com> wrote:
> >
> > could you please illustrate me what the kconfig & warning is?
> 
> Just "make allmodconfig" and the warning is about a uninitialized variable.
> 
> Line 304 in drivers/thermal/samsung/exynos_tmu.c if my shell history
> is to be believed.
> 
>                 Linus

Yeah, this has also passed my local compilation error. Somehow my gcc4.9
is not catching it. Using an older gcc (gcc4.6) does catch it.

Anyways, given that the conversion functions are written to cover
for unexpected cal_type, the right way of fixing this is to rewrite
the conversion functions to allow for returning error codes and
adjusting the callers as expected.

Rui, bzolnier, please consider the following fix:

>From 2aaf94f80c0021a21b4122c9f4197acff08ea398 Mon Sep 17 00:00:00 2001
From: Eduardo Valentin <edubezval@gmail.com>
Date: Thu, 12 Apr 2018 21:00:48 -0700
Subject: [PATCH 1/1] thermal: exynos: fix compilation warning around
 conversion functions

In order to fix the warns:
drivers/thermal/samsung/exynos_tmu.c:931:37: warning: 'temp' may be used uninitialized in this function [-Wmaybe-uninitialized]
drivers/thermal/samsung/exynos_tmu.c:304:9: warning: 'temp_code' may be used uninitialized in this function [-Wmaybe-uninitialized]

the conversion functions should allow return error codes
and the not mix the converted value with error code.

This patch change the conversion functions to return
error code or success and adjusts the callers accordingly.

Signed-off-by: Eduardo Valentin <edubezval@gmail.com>
---
 drivers/thermal/samsung/exynos_tmu.c | 120 ++++++++++++++++++++++++-----------
 1 file changed, 84 insertions(+), 36 deletions(-)

diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
index 2ec8548..b3f0704 100644
--- a/drivers/thermal/samsung/exynos_tmu.c
+++ b/drivers/thermal/samsung/exynos_tmu.c
@@ -282,52 +282,54 @@ static void exynos_report_trigger(struct exynos_tmu_data *p)
  * TMU treats temperature as a mapped temperature code.
  * The temperature is converted differently depending on the calibration type.
  */
-static int temp_to_code(struct exynos_tmu_data *data, u8 temp)
+static int temp_to_code(struct exynos_tmu_data *data, u8 temp, int *temp_code)
 {
-	int temp_code;
+	int ret = 0;
 
 	switch (data->cal_type) {
 	case TYPE_TWO_POINT_TRIMMING:
-		temp_code = (temp - EXYNOS_FIRST_POINT_TRIM) *
+		*temp_code = (temp - EXYNOS_FIRST_POINT_TRIM) *
 			(data->temp_error2 - data->temp_error1) /
 			(EXYNOS_SECOND_POINT_TRIM - EXYNOS_FIRST_POINT_TRIM) +
 			data->temp_error1;
 		break;
 	case TYPE_ONE_POINT_TRIMMING:
-		temp_code = temp + data->temp_error1 - EXYNOS_FIRST_POINT_TRIM;
+		*temp_code = temp + data->temp_error1 - EXYNOS_FIRST_POINT_TRIM;
 		break;
 	default:
 		WARN_ON(1);
+		ret = -EINVAL;
 		break;
 	}
 
-	return temp_code;
+	return ret;
 }
 
 /*
  * Calculate a temperature value from a temperature code.
  * The unit of the temperature is degree Celsius.
  */
-static int code_to_temp(struct exynos_tmu_data *data, u16 temp_code)
+static int code_to_temp(struct exynos_tmu_data *data, u16 temp_code, int *temp)
 {
-	int temp;
+	int ret = 0;
 
 	switch (data->cal_type) {
 	case TYPE_TWO_POINT_TRIMMING:
-		temp = (temp_code - data->temp_error1) *
+		*temp = (temp_code - data->temp_error1) *
 			(EXYNOS_SECOND_POINT_TRIM - EXYNOS_FIRST_POINT_TRIM) /
 			(data->temp_error2 - data->temp_error1) +
 			EXYNOS_FIRST_POINT_TRIM;
 		break;
 	case TYPE_ONE_POINT_TRIMMING:
-		temp = temp_code - data->temp_error1 + EXYNOS_FIRST_POINT_TRIM;
+		*temp = temp_code - data->temp_error1 + EXYNOS_FIRST_POINT_TRIM;
 		break;
 	default:
 		WARN_ON(1);
+		ret = -EINVAL;
 		break;
 	}
 
-	return temp;
+	return ret;
 }
 
 static void sanitize_temp_error(struct exynos_tmu_data *data, u32 trim_info)
@@ -352,7 +354,7 @@ static u32 get_th_reg(struct exynos_tmu_data *data, u32 threshold, bool falling)
 	struct thermal_zone_device *tz = data->tzd;
 	const struct thermal_trip * const trips =
 		of_thermal_get_trip_points(tz);
-	unsigned long temp;
+	int temp;
 	int i;
 
 	if (!trips) {
@@ -362,6 +364,8 @@ static u32 get_th_reg(struct exynos_tmu_data *data, u32 threshold, bool falling)
 	}
 
 	for (i = 0; i < of_thermal_get_ntrips(tz); i++) {
+		int val, ret;
+
 		if (trips[i].type == THERMAL_TRIP_CRITICAL)
 			continue;
 
@@ -371,7 +375,14 @@ static u32 get_th_reg(struct exynos_tmu_data *data, u32 threshold, bool falling)
 		else
 			threshold &= ~(0xff << 8 * i);
 
-		threshold |= temp_to_code(data, temp) << 8 * i;
+		ret = temp_to_code(data, temp, &val);
+		if (ret) {
+			pr_err("%s: Convertion error from temp (%d) to code: %d!\n",
+				__func__, temp, ret);
+			return 0;
+		}
+
+		threshold |= val << 8 * i;
 	}
 
 	return threshold;
@@ -460,11 +471,10 @@ static int exynos4210_tmu_initialize(struct platform_device *pdev)
 
 	/* Write temperature code for threshold */
 	reference = trips[0].temperature / MCELSIUS;
-	threshold_code = temp_to_code(data, reference);
-	if (threshold_code < 0) {
-		ret = threshold_code;
+	ret = temp_to_code(data, reference, &threshold_code);
+	if (ret < 0 || threshold_code < 0)
 		goto out;
-	}
+
 	writeb(threshold_code, data->base + EXYNOS4210_TMU_REG_THRESHOLD_TEMP);
 
 	for (i = 0; i < of_thermal_get_ntrips(tz); i++) {
@@ -537,7 +547,10 @@ static int exynos4412_tmu_initialize(struct platform_device *pdev)
 		goto out;
 	}
 
-	threshold_code = temp_to_code(data, crit_temp / MCELSIUS);
+	ret = temp_to_code(data, crit_temp / MCELSIUS, &threshold_code);
+	if (ret)
+		goto out;
+
 	/* 1-4 level to be assigned in th0 reg */
 	rising_threshold &= ~(0xff << 8 * i);
 	rising_threshold |= threshold_code << 8 * i;
@@ -620,7 +633,9 @@ static int exynos5433_tmu_initialize(struct platform_device *pdev)
 		/* Write temperature code for rising threshold */
 		tz->ops->get_trip_temp(tz, i, &temp);
 		temp /= MCELSIUS;
-		threshold_code = temp_to_code(data, temp);
+		ret = temp_to_code(data, temp, &threshold_code);
+		if (ret)
+			goto out;
 
 		rising_threshold = readl(data->base + rising_reg_offset);
 		rising_threshold |= (threshold_code << j * 8);
@@ -629,7 +644,9 @@ static int exynos5433_tmu_initialize(struct platform_device *pdev)
 		/* Write temperature code for falling threshold */
 		tz->ops->get_trip_hyst(tz, i, &temp_hist);
 		temp_hist = temp - (temp_hist / MCELSIUS);
-		threshold_code = temp_to_code(data, temp_hist);
+		ret = temp_to_code(data, temp_hist, &threshold_code);
+		if (ret)
+			goto out;
 
 		falling_threshold = readl(data->base + falling_reg_offset);
 		falling_threshold &= ~(0xff << j * 8);
@@ -677,7 +694,12 @@ static int exynos5440_tmu_initialize(struct platform_device *pdev)
 
 	/* if last threshold limit is also present */
 	if (!data->tzd->ops->get_crit_temp(data->tzd, &crit_temp)) {
-		threshold_code = temp_to_code(data, crit_temp / MCELSIUS);
+		int ret;
+
+		ret = temp_to_code(data, crit_temp / MCELSIUS, &threshold_code);
+		if (ret)
+			return ret;
+
 		/* 5th level to be assigned in th2 reg */
 		rising_threshold =
 			threshold_code << EXYNOS5440_TMU_TH_RISE4_SHIFT;
@@ -749,7 +771,10 @@ static int exynos7_tmu_initialize(struct platform_device *pdev)
 		temp_hist = temp - (temp_hist / MCELSIUS);
 
 		/* Set 9-bit temperature code for rising threshold levels */
-		threshold_code = temp_to_code(data, temp);
+		ret = temp_to_code(data, temp, &threshold_code);
+		if (ret)
+			goto out;
+
 		rising_threshold = readl(data->base +
 			EXYNOS7_THD_TEMP_RISE7_6 + reg_off);
 		rising_threshold &= ~(EXYNOS7_TMU_TEMP_MASK << (16 * bit_off));
@@ -758,7 +783,9 @@ static int exynos7_tmu_initialize(struct platform_device *pdev)
 		       data->base + EXYNOS7_THD_TEMP_RISE7_6 + reg_off);
 
 		/* Set 9-bit temperature code for falling threshold levels */
-		threshold_code = temp_to_code(data, temp_hist);
+		ret = temp_to_code(data, temp_hist, &threshold_code);
+		if (ret)
+			goto out;
 		falling_threshold &= ~(EXYNOS7_TMU_TEMP_MASK << (16 * bit_off));
 		falling_threshold |= threshold_code << (16 * bit_off);
 		writel(falling_threshold,
@@ -925,11 +952,18 @@ static int exynos_get_temp(void *p, int *temp)
 	clk_enable(data->clk);
 
 	value = data->tmu_read(data);
-	if (value < 0)
+	if (value < 0) {
 		ret = value;
-	else
-		*temp = code_to_temp(data, value) * MCELSIUS;
+		goto out;
+	}
+
+	ret = code_to_temp(data, value, temp);
+	if (ret)
+		goto out;
 
+	*temp *= MCELSIUS;
+
+out:
 	clk_disable(data->clk);
 	mutex_unlock(&data->lock);
 
@@ -937,9 +971,11 @@ static int exynos_get_temp(void *p, int *temp)
 }
 
 #ifdef CONFIG_THERMAL_EMULATION
-static u32 get_emul_con_reg(struct exynos_tmu_data *data, unsigned int val,
-			    int temp)
+static int get_emul_con_reg(struct exynos_tmu_data *data, unsigned int val,
+			    int temp, u32 *con_reg)
 {
+	int code, ret = 0;
+
 	if (temp) {
 		temp /= MCELSIUS;
 
@@ -950,27 +986,36 @@ static u32 get_emul_con_reg(struct exynos_tmu_data *data, unsigned int val,
 		if (data->soc == SOC_ARCH_EXYNOS7) {
 			val &= ~(EXYNOS7_EMUL_DATA_MASK <<
 				EXYNOS7_EMUL_DATA_SHIFT);
-			val |= (temp_to_code(data, temp) <<
-				EXYNOS7_EMUL_DATA_SHIFT) |
+			ret = temp_to_code(data, temp, &code);
+			if (ret)
+				goto out;
+
+			val |= (code << EXYNOS7_EMUL_DATA_SHIFT) |
 				EXYNOS_EMUL_ENABLE;
 		} else {
 			val &= ~(EXYNOS_EMUL_DATA_MASK <<
 				EXYNOS_EMUL_DATA_SHIFT);
-			val |= (temp_to_code(data, temp) <<
-				EXYNOS_EMUL_DATA_SHIFT) |
+			ret = temp_to_code(data, temp, &code);
+			if (ret)
+				goto out;
+
+			val |= (code << EXYNOS_EMUL_DATA_SHIFT) |
 				EXYNOS_EMUL_ENABLE;
 		}
 	} else {
 		val &= ~EXYNOS_EMUL_ENABLE;
 	}
 
-	return val;
+	*con_reg = val;
+out:
+	return ret;
 }
 
 static void exynos4412_tmu_set_emulation(struct exynos_tmu_data *data,
 					 int temp)
 {
 	unsigned int val;
+	int ret;
 	u32 emul_con;
 
 	if (data->soc == SOC_ARCH_EXYNOS5260)
@@ -983,18 +1028,21 @@ static void exynos4412_tmu_set_emulation(struct exynos_tmu_data *data,
 		emul_con = EXYNOS_EMUL_CON;
 
 	val = readl(data->base + emul_con);
-	val = get_emul_con_reg(data, val, temp);
-	writel(val, data->base + emul_con);
+	ret = get_emul_con_reg(data, val, temp, &val);
+	if (!ret)
+		writel(val, data->base + emul_con);
 }
 
 static void exynos5440_tmu_set_emulation(struct exynos_tmu_data *data,
 					 int temp)
 {
 	unsigned int val;
+	int ret;
 
 	val = readl(data->base + EXYNOS5440_TMU_S0_7_DEBUG);
-	val = get_emul_con_reg(data, val, temp);
-	writel(val, data->base + EXYNOS5440_TMU_S0_7_DEBUG);
+	ret = get_emul_con_reg(data, val, temp, &val);
+	if (!ret)
+		writel(val, data->base + EXYNOS5440_TMU_S0_7_DEBUG);
 }
 
 static int exynos_tmu_set_emulation(void *drv_data, int temp)
-- 
2.1.4

Re: [GIT PULL] Thermal management updates for v4.17-rc1

[Zhang Rui]

[-- Attachment #1: Type: text/plain, Size: 1074 bytes --]

On 四, 2018-04-12 at 21:08 -0700, Eduardo Valentin wrote:
> Hello,
> 
> On Thu, Apr 12, 2018 at 09:55:19AM -0700, Linus Torvalds wrote:
> > 
> > On Wed, Apr 11, 2018 at 10:08 PM, Zhang Rui <rui.zhang@intel.com>
> > wrote:
> > > 
> > > 
> > > could you please illustrate me what the kconfig & warning is?
> > Just "make allmodconfig" and the warning is about a uninitialized
> > variable.
> > 
> > Line 304 in drivers/thermal/samsung/exynos_tmu.c if my shell
> > history
> > is to be believed.
> > 
> >                 Linus
> Yeah, this has also passed my local compilation error. Somehow my
> gcc4.9
> is not catching it. Using an older gcc (gcc4.6) does catch it.
> 

I think there are two problems here

1. Actually, this error has been raised by 0-day earlier.
https://marc.info/?l=linux-pm&m=152107340117077&w=2
Don't know why it still goes into thermal-soc tree.

2. After pulled the thermal-soc changes, I also asked 0-day to run
build test, but I didn't get any warning report (email attached), CC
Philip and Shun to look at this issue.

thanks,
rui

[-- Attachment #2: [rui:next]_BUILD_SUCCESS_f8837aac36cdc7430422cd65f4466071b42654bb.mbox --]
[-- Type: application/mbox, Size: 7627 bytes --]

Re: [GIT PULL] Thermal management updates for v4.17-rc1

[Zhang Rui]

Hi, Eduardo,

On 四, 2018-04-12 at 21:08 -0700, Eduardo Valentin wrote:
> Hello,
> 
> On Thu, Apr 12, 2018 at 09:55:19AM -0700, Linus Torvalds wrote:
> > 
> > On Wed, Apr 11, 2018 at 10:08 PM, Zhang Rui <rui.zhang@intel.com>
> > wrote:
> > > 
> > > 
> > > could you please illustrate me what the kconfig & warning is?
> > Just "make allmodconfig" and the warning is about a uninitialized
> > variable.
> > 
> > Line 304 in drivers/thermal/samsung/exynos_tmu.c if my shell
> > history
> > is to be believed.
> > 
> >                 Linus
> Yeah, this has also passed my local compilation error. Somehow my
> gcc4.9
> is not catching it. Using an older gcc (gcc4.6) does catch it.
> 
> Anyways, given that the conversion functions are written to cover
> for unexpected cal_type, the right way of fixing this is to rewrite
> the conversion functions to allow for returning error codes and
> adjusting the callers as expected.
> 
> Rui, bzolnier, please consider the following fix:
> 
as it is late in this merge window, I'd prefer to
1. drop all the thermal-soc material in the first pull request which I
will send out soon.
2. you can prepare another pull request containing the thermal-soc
materials except the exynos fixes
3. exynos fixes with the problem solved can be queued for -rc2 or
later.

thanks,
rui

> From 2aaf94f80c0021a21b4122c9f4197acff08ea398 Mon Sep 17 00:00:00
> 2001
> From: Eduardo Valentin <edubezval@gmail.com>
> Date: Thu, 12 Apr 2018 21:00:48 -0700
> Subject: [PATCH 1/1] thermal: exynos: fix compilation warning around
>  conversion functions
> 
> In order to fix the warns:
> drivers/thermal/samsung/exynos_tmu.c:931:37: warning: 'temp' may be
> used uninitialized in this function [-Wmaybe-uninitialized]
> drivers/thermal/samsung/exynos_tmu.c:304:9: warning: 'temp_code' may
> be used uninitialized in this function [-Wmaybe-uninitialized]
> 
> the conversion functions should allow return error codes
> and the not mix the converted value with error code.
> 
> This patch change the conversion functions to return
> error code or success and adjusts the callers accordingly.
> 
> Signed-off-by: Eduardo Valentin <edubezval@gmail.com>
> ---
>  drivers/thermal/samsung/exynos_tmu.c | 120 ++++++++++++++++++++++++-
> ----------
>  1 file changed, 84 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/thermal/samsung/exynos_tmu.c
> b/drivers/thermal/samsung/exynos_tmu.c
> index 2ec8548..b3f0704 100644
> --- a/drivers/thermal/samsung/exynos_tmu.c
> +++ b/drivers/thermal/samsung/exynos_tmu.c
> @@ -282,52 +282,54 @@ static void exynos_report_trigger(struct
> exynos_tmu_data *p)
>   * TMU treats temperature as a mapped temperature code.
>   * The temperature is converted differently depending on the
> calibration type.
>   */
> -static int temp_to_code(struct exynos_tmu_data *data, u8 temp)
> +static int temp_to_code(struct exynos_tmu_data *data, u8 temp, int
> *temp_code)
>  {
> -	int temp_code;
> +	int ret = 0;
>  
>  	switch (data->cal_type) {
>  	case TYPE_TWO_POINT_TRIMMING:
> -		temp_code = (temp - EXYNOS_FIRST_POINT_TRIM) *
> +		*temp_code = (temp - EXYNOS_FIRST_POINT_TRIM) *
>  			(data->temp_error2 - data->temp_error1) /
>  			(EXYNOS_SECOND_POINT_TRIM -
> EXYNOS_FIRST_POINT_TRIM) +
>  			data->temp_error1;
>  		break;
>  	case TYPE_ONE_POINT_TRIMMING:
> -		temp_code = temp + data->temp_error1 -
> EXYNOS_FIRST_POINT_TRIM;
> +		*temp_code = temp + data->temp_error1 -
> EXYNOS_FIRST_POINT_TRIM;
>  		break;
>  	default:
>  		WARN_ON(1);
> +		ret = -EINVAL;
>  		break;
>  	}
>  
> -	return temp_code;
> +	return ret;
>  }
>  
>  /*
>   * Calculate a temperature value from a temperature code.
>   * The unit of the temperature is degree Celsius.
>   */
> -static int code_to_temp(struct exynos_tmu_data *data, u16 temp_code)
> +static int code_to_temp(struct exynos_tmu_data *data, u16 temp_code,
> int *temp)
>  {
> -	int temp;
> +	int ret = 0;
>  
>  	switch (data->cal_type) {
>  	case TYPE_TWO_POINT_TRIMMING:
> -		temp = (temp_code - data->temp_error1) *
> +		*temp = (temp_code - data->temp_error1) *
>  			(EXYNOS_SECOND_POINT_TRIM -
> EXYNOS_FIRST_POINT_TRIM) /
>  			(data->temp_error2 - data->temp_error1) +
>  			EXYNOS_FIRST_POINT_TRIM;
>  		break;
>  	case TYPE_ONE_POINT_TRIMMING:
> -		temp = temp_code - data->temp_error1 +
> EXYNOS_FIRST_POINT_TRIM;
> +		*temp = temp_code - data->temp_error1 +
> EXYNOS_FIRST_POINT_TRIM;
>  		break;
>  	default:
>  		WARN_ON(1);
> +		ret = -EINVAL;
>  		break;
>  	}
>  
> -	return temp;
> +	return ret;
>  }
>  
>  static void sanitize_temp_error(struct exynos_tmu_data *data, u32
> trim_info)
> @@ -352,7 +354,7 @@ static u32 get_th_reg(struct exynos_tmu_data
> *data, u32 threshold, bool falling)
>  	struct thermal_zone_device *tz = data->tzd;
>  	const struct thermal_trip * const trips =
>  		of_thermal_get_trip_points(tz);
> -	unsigned long temp;
> +	int temp;
>  	int i;
>  
>  	if (!trips) {
> @@ -362,6 +364,8 @@ static u32 get_th_reg(struct exynos_tmu_data
> *data, u32 threshold, bool falling)
>  	}
>  
>  	for (i = 0; i < of_thermal_get_ntrips(tz); i++) {
> +		int val, ret;
> +
>  		if (trips[i].type == THERMAL_TRIP_CRITICAL)
>  			continue;
>  
> @@ -371,7 +375,14 @@ static u32 get_th_reg(struct exynos_tmu_data
> *data, u32 threshold, bool falling)
>  		else
>  			threshold &= ~(0xff << 8 * i);
>  
> -		threshold |= temp_to_code(data, temp) << 8 * i;
> +		ret = temp_to_code(data, temp, &val);
> +		if (ret) {
> +			pr_err("%s: Convertion error from temp (%d)
> to code: %d!\n",
> +				__func__, temp, ret);
> +			return 0;
> +		}
> +
> +		threshold |= val << 8 * i;
>  	}
>  
>  	return threshold;
> @@ -460,11 +471,10 @@ static int exynos4210_tmu_initialize(struct
> platform_device *pdev)
>  
>  	/* Write temperature code for threshold */
>  	reference = trips[0].temperature / MCELSIUS;
> -	threshold_code = temp_to_code(data, reference);
> -	if (threshold_code < 0) {
> -		ret = threshold_code;
> +	ret = temp_to_code(data, reference, &threshold_code);
> +	if (ret < 0 || threshold_code < 0)
>  		goto out;
> -	}
> +
>  	writeb(threshold_code, data->base +
> EXYNOS4210_TMU_REG_THRESHOLD_TEMP);
>  
>  	for (i = 0; i < of_thermal_get_ntrips(tz); i++) {
> @@ -537,7 +547,10 @@ static int exynos4412_tmu_initialize(struct
> platform_device *pdev)
>  		goto out;
>  	}
>  
> -	threshold_code = temp_to_code(data, crit_temp / MCELSIUS);
> +	ret = temp_to_code(data, crit_temp / MCELSIUS,
> &threshold_code);
> +	if (ret)
> +		goto out;
> +
>  	/* 1-4 level to be assigned in th0 reg */
>  	rising_threshold &= ~(0xff << 8 * i);
>  	rising_threshold |= threshold_code << 8 * i;
> @@ -620,7 +633,9 @@ static int exynos5433_tmu_initialize(struct
> platform_device *pdev)
>  		/* Write temperature code for rising threshold */
>  		tz->ops->get_trip_temp(tz, i, &temp);
>  		temp /= MCELSIUS;
> -		threshold_code = temp_to_code(data, temp);
> +		ret = temp_to_code(data, temp, &threshold_code);
> +		if (ret)
> +			goto out;
>  
>  		rising_threshold = readl(data->base +
> rising_reg_offset);
>  		rising_threshold |= (threshold_code << j * 8);
> @@ -629,7 +644,9 @@ static int exynos5433_tmu_initialize(struct
> platform_device *pdev)
>  		/* Write temperature code for falling threshold */
>  		tz->ops->get_trip_hyst(tz, i, &temp_hist);
>  		temp_hist = temp - (temp_hist / MCELSIUS);
> -		threshold_code = temp_to_code(data, temp_hist);
> +		ret = temp_to_code(data, temp_hist,
> &threshold_code);
> +		if (ret)
> +			goto out;
>  
>  		falling_threshold = readl(data->base +
> falling_reg_offset);
>  		falling_threshold &= ~(0xff << j * 8);
> @@ -677,7 +694,12 @@ static int exynos5440_tmu_initialize(struct
> platform_device *pdev)
>  
>  	/* if last threshold limit is also present */
>  	if (!data->tzd->ops->get_crit_temp(data->tzd, &crit_temp)) {
> -		threshold_code = temp_to_code(data, crit_temp /
> MCELSIUS);
> +		int ret;
> +
> +		ret = temp_to_code(data, crit_temp / MCELSIUS,
> &threshold_code);
> +		if (ret)
> +			return ret;
> +
>  		/* 5th level to be assigned in th2 reg */
>  		rising_threshold =
>  			threshold_code <<
> EXYNOS5440_TMU_TH_RISE4_SHIFT;
> @@ -749,7 +771,10 @@ static int exynos7_tmu_initialize(struct
> platform_device *pdev)
>  		temp_hist = temp - (temp_hist / MCELSIUS);
>  
>  		/* Set 9-bit temperature code for rising threshold
> levels */
> -		threshold_code = temp_to_code(data, temp);
> +		ret = temp_to_code(data, temp, &threshold_code);
> +		if (ret)
> +			goto out;
> +
>  		rising_threshold = readl(data->base +
>  			EXYNOS7_THD_TEMP_RISE7_6 + reg_off);
>  		rising_threshold &= ~(EXYNOS7_TMU_TEMP_MASK << (16 *
> bit_off));
> @@ -758,7 +783,9 @@ static int exynos7_tmu_initialize(struct
> platform_device *pdev)
>  		       data->base + EXYNOS7_THD_TEMP_RISE7_6 +
> reg_off);
>  
>  		/* Set 9-bit temperature code for falling threshold
> levels */
> -		threshold_code = temp_to_code(data, temp_hist);
> +		ret = temp_to_code(data, temp_hist,
> &threshold_code);
> +		if (ret)
> +			goto out;
>  		falling_threshold &= ~(EXYNOS7_TMU_TEMP_MASK << (16
> * bit_off));
>  		falling_threshold |= threshold_code << (16 *
> bit_off);
>  		writel(falling_threshold,
> @@ -925,11 +952,18 @@ static int exynos_get_temp(void *p, int *temp)
>  	clk_enable(data->clk);
>  
>  	value = data->tmu_read(data);
> -	if (value < 0)
> +	if (value < 0) {
>  		ret = value;
> -	else
> -		*temp = code_to_temp(data, value) * MCELSIUS;
> +		goto out;
> +	}
> +
> +	ret = code_to_temp(data, value, temp);
> +	if (ret)
> +		goto out;
>  
> +	*temp *= MCELSIUS;
> +
> +out:
>  	clk_disable(data->clk);
>  	mutex_unlock(&data->lock);
>  
> @@ -937,9 +971,11 @@ static int exynos_get_temp(void *p, int *temp)
>  }
>  
>  #ifdef CONFIG_THERMAL_EMULATION
> -static u32 get_emul_con_reg(struct exynos_tmu_data *data, unsigned
> int val,
> -			    int temp)
> +static int get_emul_con_reg(struct exynos_tmu_data *data, unsigned
> int val,
> +			    int temp, u32 *con_reg)
>  {
> +	int code, ret = 0;
> +
>  	if (temp) {
>  		temp /= MCELSIUS;
>  
> @@ -950,27 +986,36 @@ static u32 get_emul_con_reg(struct
> exynos_tmu_data *data, unsigned int val,
>  		if (data->soc == SOC_ARCH_EXYNOS7) {
>  			val &= ~(EXYNOS7_EMUL_DATA_MASK <<
>  				EXYNOS7_EMUL_DATA_SHIFT);
> -			val |= (temp_to_code(data, temp) <<
> -				EXYNOS7_EMUL_DATA_SHIFT) |
> +			ret = temp_to_code(data, temp, &code);
> +			if (ret)
> +				goto out;
> +
> +			val |= (code << EXYNOS7_EMUL_DATA_SHIFT) |
>  				EXYNOS_EMUL_ENABLE;
>  		} else {
>  			val &= ~(EXYNOS_EMUL_DATA_MASK <<
>  				EXYNOS_EMUL_DATA_SHIFT);
> -			val |= (temp_to_code(data, temp) <<
> -				EXYNOS_EMUL_DATA_SHIFT) |
> +			ret = temp_to_code(data, temp, &code);
> +			if (ret)
> +				goto out;
> +
> +			val |= (code << EXYNOS_EMUL_DATA_SHIFT) |
>  				EXYNOS_EMUL_ENABLE;
>  		}
>  	} else {
>  		val &= ~EXYNOS_EMUL_ENABLE;
>  	}
>  
> -	return val;
> +	*con_reg = val;
> +out:
> +	return ret;
>  }
>  
>  static void exynos4412_tmu_set_emulation(struct exynos_tmu_data
> *data,
>  					 int temp)
>  {
>  	unsigned int val;
> +	int ret;
>  	u32 emul_con;
>  
>  	if (data->soc == SOC_ARCH_EXYNOS5260)
> @@ -983,18 +1028,21 @@ static void
> exynos4412_tmu_set_emulation(struct exynos_tmu_data *data,
>  		emul_con = EXYNOS_EMUL_CON;
>  
>  	val = readl(data->base + emul_con);
> -	val = get_emul_con_reg(data, val, temp);
> -	writel(val, data->base + emul_con);
> +	ret = get_emul_con_reg(data, val, temp, &val);
> +	if (!ret)
> +		writel(val, data->base + emul_con);
>  }
>  
>  static void exynos5440_tmu_set_emulation(struct exynos_tmu_data
> *data,
>  					 int temp)
>  {
>  	unsigned int val;
> +	int ret;
>  
>  	val = readl(data->base + EXYNOS5440_TMU_S0_7_DEBUG);
> -	val = get_emul_con_reg(data, val, temp);
> -	writel(val, data->base + EXYNOS5440_TMU_S0_7_DEBUG);
> +	ret = get_emul_con_reg(data, val, temp, &val);
> +	if (!ret)
> +		writel(val, data->base + EXYNOS5440_TMU_S0_7_DEBUG);
>  }
>  
>  static int exynos_tmu_set_emulation(void *drv_data, int temp)

Re: [GIT PULL] Thermal management updates for v4.17-rc1

[Bartlomiej Zolnierkiewicz]

On Thursday, April 12, 2018 09:08:57 PM Eduardo Valentin wrote:
> Hello,

Hi,

> On Thu, Apr 12, 2018 at 09:55:19AM -0700, Linus Torvalds wrote:
> > On Wed, Apr 11, 2018 at 10:08 PM, Zhang Rui <rui.zhang@intel.com> wrote:
> > >
> > > could you please illustrate me what the kconfig & warning is?
> > 
> > Just "make allmodconfig" and the warning is about a uninitialized variable.
> > 
> > Line 304 in drivers/thermal/samsung/exynos_tmu.c if my shell history
> > is to be believed.
> > 
> >                 Linus
> 
> Yeah, this has also passed my local compilation error. Somehow my gcc4.9
> is not catching it. Using an older gcc (gcc4.6) does catch it.
> 
> Anyways, given that the conversion functions are written to cover
> for unexpected cal_type, the right way of fixing this is to rewrite
> the conversion functions to allow for returning error codes and
> adjusting the callers as expected.
> 
> Rui, bzolnier, please consider the following fix:
> 
> From 2aaf94f80c0021a21b4122c9f4197acff08ea398 Mon Sep 17 00:00:00 2001
> From: Eduardo Valentin <edubezval@gmail.com>
> Date: Thu, 12 Apr 2018 21:00:48 -0700
> Subject: [PATCH 1/1] thermal: exynos: fix compilation warning around
>  conversion functions
> 
> In order to fix the warns:
> drivers/thermal/samsung/exynos_tmu.c:931:37: warning: 'temp' may be used uninitialized in this function [-Wmaybe-uninitialized]
> drivers/thermal/samsung/exynos_tmu.c:304:9: warning: 'temp_code' may be used uninitialized in this function [-Wmaybe-uninitialized]
> 
> the conversion functions should allow return error codes
> and the not mix the converted value with error code.
> 
> This patch change the conversion functions to return
> error code or success and adjusts the callers accordingly.
> 
> Signed-off-by: Eduardo Valentin <edubezval@gmail.com>
> ---
>  drivers/thermal/samsung/exynos_tmu.c | 120 ++++++++++++++++++++++++-----------
>  1 file changed, 84 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
> index 2ec8548..b3f0704 100644
> --- a/drivers/thermal/samsung/exynos_tmu.c
> +++ b/drivers/thermal/samsung/exynos_tmu.c
> @@ -282,52 +282,54 @@ static void exynos_report_trigger(struct exynos_tmu_data *p)
>   * TMU treats temperature as a mapped temperature code.
>   * The temperature is converted differently depending on the calibration type.
>   */
> -static int temp_to_code(struct exynos_tmu_data *data, u8 temp)
> +static int temp_to_code(struct exynos_tmu_data *data, u8 temp, int *temp_code)
>  {
> -	int temp_code;
> +	int ret = 0;
>  
>  	switch (data->cal_type) {
>  	case TYPE_TWO_POINT_TRIMMING:
> -		temp_code = (temp - EXYNOS_FIRST_POINT_TRIM) *
> +		*temp_code = (temp - EXYNOS_FIRST_POINT_TRIM) *
>  			(data->temp_error2 - data->temp_error1) /
>  			(EXYNOS_SECOND_POINT_TRIM - EXYNOS_FIRST_POINT_TRIM) +
>  			data->temp_error1;
>  		break;
>  	case TYPE_ONE_POINT_TRIMMING:
> -		temp_code = temp + data->temp_error1 - EXYNOS_FIRST_POINT_TRIM;
> +		*temp_code = temp + data->temp_error1 - EXYNOS_FIRST_POINT_TRIM;
>  		break;
>  	default:

Since this condition cannot happen (the driver makes sure of this during
probe) I would prefer much simpler fix from Arnd:

https://patchwork.kernel.org/patch/10313313/

(I've already ACKed it two weeks ago).

>  		WARN_ON(1);
> +		ret = -EINVAL;
>  		break;
>  	}
>  
> -	return temp_code;
> +	return ret;
>  }
>  
>  /*
>   * Calculate a temperature value from a temperature code.
>   * The unit of the temperature is degree Celsius.
>   */
> -static int code_to_temp(struct exynos_tmu_data *data, u16 temp_code)
> +static int code_to_temp(struct exynos_tmu_data *data, u16 temp_code, int *temp)
>  {
> -	int temp;
> +	int ret = 0;
>  
>  	switch (data->cal_type) {
>  	case TYPE_TWO_POINT_TRIMMING:
> -		temp = (temp_code - data->temp_error1) *
> +		*temp = (temp_code - data->temp_error1) *
>  			(EXYNOS_SECOND_POINT_TRIM - EXYNOS_FIRST_POINT_TRIM) /
>  			(data->temp_error2 - data->temp_error1) +
>  			EXYNOS_FIRST_POINT_TRIM;
>  		break;
>  	case TYPE_ONE_POINT_TRIMMING:
> -		temp = temp_code - data->temp_error1 + EXYNOS_FIRST_POINT_TRIM;
> +		*temp = temp_code - data->temp_error1 + EXYNOS_FIRST_POINT_TRIM;
>  		break;
>  	default:
>  		WARN_ON(1);
> +		ret = -EINVAL;

ditto

>  		break;
>  	}
>  
> -	return temp;
> +	return ret;
>  }

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Re: [GIT PULL] Thermal management updates for v4.17-rc1

[Bartlomiej Zolnierkiewicz]

On Friday, April 13, 2018 01:39:05 PM Zhang Rui wrote:
> Hi, Eduardo,
> 
> On 四, 2018-04-12 at 21:08 -0700, Eduardo Valentin wrote:
> > Hello,
> > 
> > On Thu, Apr 12, 2018 at 09:55:19AM -0700, Linus Torvalds wrote:
> > > 
> > > On Wed, Apr 11, 2018 at 10:08 PM, Zhang Rui <rui.zhang@intel.com>
> > > wrote:
> > > > 
> > > > 
> > > > could you please illustrate me what the kconfig & warning is?
> > > Just "make allmodconfig" and the warning is about a uninitialized
> > > variable.
> > > 
> > > Line 304 in drivers/thermal/samsung/exynos_tmu.c if my shell
> > > history
> > > is to be believed.
> > > 
> > >                 Linus
> > Yeah, this has also passed my local compilation error. Somehow my
> > gcc4.9
> > is not catching it. Using an older gcc (gcc4.6) does catch it.
> > 
> > Anyways, given that the conversion functions are written to cover
> > for unexpected cal_type, the right way of fixing this is to rewrite
> > the conversion functions to allow for returning error codes and
> > adjusting the callers as expected.
> > 
> > Rui, bzolnier, please consider the following fix:
> > 
> as it is late in this merge window, I'd prefer to
> 1. drop all the thermal-soc material in the first pull request which I
> will send out soon.
> 2. you can prepare another pull request containing the thermal-soc
> materials except the exynos fixes
> 3. exynos fixes with the problem solved can be queued for -rc2 or
> later.

Could you please just merge the obvious fix from Arnd instead?

[ it was posted two weeks ago and ACKed by me ]

https://patchwork.kernel.org/patch/10313313/

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Re: [GIT PULL] Thermal management updates for v4.17-rc1

[Daniel Lezcano]

On 13/04/2018 10:55, Bartlomiej Zolnierkiewicz wrote:
> On Friday, April 13, 2018 01:39:05 PM Zhang Rui wrote:
>> Hi, Eduardo,
>>
>> On 四, 2018-04-12 at 21:08 -0700, Eduardo Valentin wrote:
>>> Hello,
>>>
>>> On Thu, Apr 12, 2018 at 09:55:19AM -0700, Linus Torvalds wrote:
>>>>
>>>> On Wed, Apr 11, 2018 at 10:08 PM, Zhang Rui <rui.zhang@intel.com>
>>>> wrote:
>>>>>
>>>>>
>>>>> could you please illustrate me what the kconfig & warning is?
>>>> Just "make allmodconfig" and the warning is about a uninitialized
>>>> variable.
>>>>
>>>> Line 304 in drivers/thermal/samsung/exynos_tmu.c if my shell
>>>> history
>>>> is to be believed.
>>>>
>>>>                 Linus
>>> Yeah, this has also passed my local compilation error. Somehow my
>>> gcc4.9
>>> is not catching it. Using an older gcc (gcc4.6) does catch it.
>>>
>>> Anyways, given that the conversion functions are written to cover
>>> for unexpected cal_type, the right way of fixing this is to rewrite
>>> the conversion functions to allow for returning error codes and
>>> adjusting the callers as expected.
>>>
>>> Rui, bzolnier, please consider the following fix:
>>>
>> as it is late in this merge window, I'd prefer to
>> 1. drop all the thermal-soc material in the first pull request which I
>> will send out soon.
>> 2. you can prepare another pull request containing the thermal-soc
>> materials except the exynos fixes
>> 3. exynos fixes with the problem solved can be queued for -rc2 or
>> later.
> 
> Could you please just merge the obvious fix from Arnd instead?
> 
> [ it was posted two weeks ago and ACKed by me ]
> 
> https://patchwork.kernel.org/patch/10313313/

I'm not sure these are correct fixes.

The change 480b5bfc16e1 tells:

"There should be no functional changes caused by this patch."

but the fix above returns 0 as a default value instead of '50' or '25'
for the 5440 and that impacts the threshold etc ...

IMO, the correct fix would be to define a default value '50', override
it at init time to '25' if it is a 5440. And then the variable 'temp'
and 'temp_code' get this value in the default case.




> Best regards,
> --
> Bartlomiej Zolnierkiewicz
> Samsung R&D Institute Poland
> Samsung Electronics
> 


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [GIT PULL] Thermal management updates for v4.17-rc1

[Bartlomiej Zolnierkiewicz]

On Friday, April 13, 2018 11:00:43 AM Daniel Lezcano wrote:
> On 13/04/2018 10:55, Bartlomiej Zolnierkiewicz wrote:
> > On Friday, April 13, 2018 01:39:05 PM Zhang Rui wrote:
> >> Hi, Eduardo,
> >>
> >> On 四, 2018-04-12 at 21:08 -0700, Eduardo Valentin wrote:
> >>> Hello,
> >>>
> >>> On Thu, Apr 12, 2018 at 09:55:19AM -0700, Linus Torvalds wrote:
> >>>>
> >>>> On Wed, Apr 11, 2018 at 10:08 PM, Zhang Rui <rui.zhang@intel.com>
> >>>> wrote:
> >>>>>
> >>>>>
> >>>>> could you please illustrate me what the kconfig & warning is?
> >>>> Just "make allmodconfig" and the warning is about a uninitialized
> >>>> variable.
> >>>>
> >>>> Line 304 in drivers/thermal/samsung/exynos_tmu.c if my shell
> >>>> history
> >>>> is to be believed.
> >>>>
> >>>>                 Linus
> >>> Yeah, this has also passed my local compilation error. Somehow my
> >>> gcc4.9
> >>> is not catching it. Using an older gcc (gcc4.6) does catch it.
> >>>
> >>> Anyways, given that the conversion functions are written to cover
> >>> for unexpected cal_type, the right way of fixing this is to rewrite
> >>> the conversion functions to allow for returning error codes and
> >>> adjusting the callers as expected.
> >>>
> >>> Rui, bzolnier, please consider the following fix:
> >>>
> >> as it is late in this merge window, I'd prefer to
> >> 1. drop all the thermal-soc material in the first pull request which I
> >> will send out soon.
> >> 2. you can prepare another pull request containing the thermal-soc
> >> materials except the exynos fixes
> >> 3. exynos fixes with the problem solved can be queued for -rc2 or
> >> later.
> > 
> > Could you please just merge the obvious fix from Arnd instead?
> > 
> > [ it was posted two weeks ago and ACKed by me ]
> > 
> > https://patchwork.kernel.org/patch/10313313/
> 
> I'm not sure these are correct fixes.
> 
> The change 480b5bfc16e1 tells:
> 
> "There should be no functional changes caused by this patch."
> 
> but the fix above returns 0 as a default value instead of '50' or '25'
> for the 5440 and that impacts the threshold etc ...
> 
> IMO, the correct fix would be to define a default value '50', override
> it at init time to '25' if it is a 5440. And then the variable 'temp'
> and 'temp_code' get this value in the default case.

It is okay to return 0 because this code-path (the default one) will be
never hit by the driver (probe makes sure of it) - the default case is
here is just to silence compilation errors..

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Re: [GIT PULL] Thermal management updates for v4.17-rc1

[Daniel Lezcano]

On 13/04/2018 11:08, Bartlomiej Zolnierkiewicz wrote:
> On Friday, April 13, 2018 11:00:43 AM Daniel Lezcano wrote:
>> On 13/04/2018 10:55, Bartlomiej Zolnierkiewicz wrote:
>>> On Friday, April 13, 2018 01:39:05 PM Zhang Rui wrote:
>>>> Hi, Eduardo,
>>>>
>>>> On 四, 2018-04-12 at 21:08 -0700, Eduardo Valentin wrote:
>>>>> Hello,
>>>>>
>>>>> On Thu, Apr 12, 2018 at 09:55:19AM -0700, Linus Torvalds wrote:
>>>>>>
>>>>>> On Wed, Apr 11, 2018 at 10:08 PM, Zhang Rui <rui.zhang@intel.com>
>>>>>> wrote:
>>>>>>>
>>>>>>>
>>>>>>> could you please illustrate me what the kconfig & warning is?
>>>>>> Just "make allmodconfig" and the warning is about a uninitialized
>>>>>> variable.
>>>>>>
>>>>>> Line 304 in drivers/thermal/samsung/exynos_tmu.c if my shell
>>>>>> history
>>>>>> is to be believed.
>>>>>>
>>>>>>                 Linus
>>>>> Yeah, this has also passed my local compilation error. Somehow my
>>>>> gcc4.9
>>>>> is not catching it. Using an older gcc (gcc4.6) does catch it.
>>>>>
>>>>> Anyways, given that the conversion functions are written to cover
>>>>> for unexpected cal_type, the right way of fixing this is to rewrite
>>>>> the conversion functions to allow for returning error codes and
>>>>> adjusting the callers as expected.
>>>>>
>>>>> Rui, bzolnier, please consider the following fix:
>>>>>
>>>> as it is late in this merge window, I'd prefer to
>>>> 1. drop all the thermal-soc material in the first pull request which I
>>>> will send out soon.
>>>> 2. you can prepare another pull request containing the thermal-soc
>>>> materials except the exynos fixes
>>>> 3. exynos fixes with the problem solved can be queued for -rc2 or
>>>> later.
>>>
>>> Could you please just merge the obvious fix from Arnd instead?
>>>
>>> [ it was posted two weeks ago and ACKed by me ]
>>>
>>> https://patchwork.kernel.org/patch/10313313/
>>
>> I'm not sure these are correct fixes.
>>
>> The change 480b5bfc16e1 tells:
>>
>> "There should be no functional changes caused by this patch."
>>
>> but the fix above returns 0 as a default value instead of '50' or '25'
>> for the 5440 and that impacts the threshold etc ...
>>
>> IMO, the correct fix would be to define a default value '50', override
>> it at init time to '25' if it is a 5440. And then the variable 'temp'
>> and 'temp_code' get this value in the default case.
> 
> It is okay to return 0 because this code-path (the default one) will be
> never hit by the driver (probe makes sure of it) - the default case is
> here is just to silence compilation errors..

The init function is making sure cal_type is one or another. Can you fix
it correctly by replacing the 'switch' by a 'if' instead of adding dead
branches to please gcc?

if (data->cal_type == TYPE_TWO_POINT_TRIMMING) {
	return ...;
}

return ...;

-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [GIT PULL] Thermal management updates for v4.17-rc1

[Bartlomiej Zolnierkiewicz]

On Friday, April 13, 2018 11:19:40 AM Daniel Lezcano wrote:
> On 13/04/2018 11:08, Bartlomiej Zolnierkiewicz wrote:
> > On Friday, April 13, 2018 11:00:43 AM Daniel Lezcano wrote:
> >> On 13/04/2018 10:55, Bartlomiej Zolnierkiewicz wrote:
> >>> On Friday, April 13, 2018 01:39:05 PM Zhang Rui wrote:
> >>>> Hi, Eduardo,
> >>>>
> >>>> On 四, 2018-04-12 at 21:08 -0700, Eduardo Valentin wrote:
> >>>>> Hello,
> >>>>>
> >>>>> On Thu, Apr 12, 2018 at 09:55:19AM -0700, Linus Torvalds wrote:
> >>>>>>
> >>>>>> On Wed, Apr 11, 2018 at 10:08 PM, Zhang Rui <rui.zhang@intel.com>
> >>>>>> wrote:
> >>>>>>>
> >>>>>>>
> >>>>>>> could you please illustrate me what the kconfig & warning is?
> >>>>>> Just "make allmodconfig" and the warning is about a uninitialized
> >>>>>> variable.
> >>>>>>
> >>>>>> Line 304 in drivers/thermal/samsung/exynos_tmu.c if my shell
> >>>>>> history
> >>>>>> is to be believed.
> >>>>>>
> >>>>>>                 Linus
> >>>>> Yeah, this has also passed my local compilation error. Somehow my
> >>>>> gcc4.9
> >>>>> is not catching it. Using an older gcc (gcc4.6) does catch it.
> >>>>>
> >>>>> Anyways, given that the conversion functions are written to cover
> >>>>> for unexpected cal_type, the right way of fixing this is to rewrite
> >>>>> the conversion functions to allow for returning error codes and
> >>>>> adjusting the callers as expected.
> >>>>>
> >>>>> Rui, bzolnier, please consider the following fix:
> >>>>>
> >>>> as it is late in this merge window, I'd prefer to
> >>>> 1. drop all the thermal-soc material in the first pull request which I
> >>>> will send out soon.
> >>>> 2. you can prepare another pull request containing the thermal-soc
> >>>> materials except the exynos fixes
> >>>> 3. exynos fixes with the problem solved can be queued for -rc2 or
> >>>> later.
> >>>
> >>> Could you please just merge the obvious fix from Arnd instead?
> >>>
> >>> [ it was posted two weeks ago and ACKed by me ]
> >>>
> >>> https://patchwork.kernel.org/patch/10313313/
> >>
> >> I'm not sure these are correct fixes.
> >>
> >> The change 480b5bfc16e1 tells:
> >>
> >> "There should be no functional changes caused by this patch."
> >>
> >> but the fix above returns 0 as a default value instead of '50' or '25'
> >> for the 5440 and that impacts the threshold etc ...
> >>
> >> IMO, the correct fix would be to define a default value '50', override
> >> it at init time to '25' if it is a 5440. And then the variable 'temp'
> >> and 'temp_code' get this value in the default case.
> > 
> > It is okay to return 0 because this code-path (the default one) will be
> > never hit by the driver (probe makes sure of it) - the default case is
> > here is just to silence compilation errors..
> 
> The init function is making sure cal_type is one or another. Can you fix
> it correctly by replacing the 'switch' by a 'if' instead of adding dead
> branches to please gcc?
> 
> if (data->cal_type == TYPE_TWO_POINT_TRIMMING) {
> 	return ...;
> }
> 
> return ...;

I'm not the one that added this switch statement (it has been there since
2011) and I would be happy to remove it.  However could we please defer
this to v4.17 and merge the current set of Exynos thermal fixes/cleanups
(they simplify the driver a lot and make ground for future changes)?

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Re: [GIT PULL] Thermal management updates for v4.17-rc1

[Eduardo Valentin]

On Fri, Apr 13, 2018 at 01:39:05PM +0800, Zhang Rui wrote:
> Hi, Eduardo,
> 
> On 四, 2018-04-12 at 21:08 -0700, Eduardo Valentin wrote:
> > Hello,
> > 
> > On Thu, Apr 12, 2018 at 09:55:19AM -0700, Linus Torvalds wrote:
> > > 
> > > On Wed, Apr 11, 2018 at 10:08 PM, Zhang Rui <rui.zhang@intel.com>
> > > wrote:
> > > > 
> > > > 
> > > > could you please illustrate me what the kconfig & warning is?
> > > Just "make allmodconfig" and the warning is about a uninitialized
> > > variable.
> > > 
> > > Line 304 in drivers/thermal/samsung/exynos_tmu.c if my shell
> > > history
> > > is to be believed.
> > > 
> > >                 Linus
> > Yeah, this has also passed my local compilation error. Somehow my
> > gcc4.9
> > is not catching it. Using an older gcc (gcc4.6) does catch it.
> > 
> > Anyways, given that the conversion functions are written to cover
> > for unexpected cal_type, the right way of fixing this is to rewrite
> > the conversion functions to allow for returning error codes and
> > adjusting the callers as expected.
> > 
> > Rui, bzolnier, please consider the following fix:
> > 
> as it is late in this merge window, I'd prefer to
> 1. drop all the thermal-soc material in the first pull request which I
> will send out soon.
> 2. you can prepare another pull request containing the thermal-soc
> materials except the exynos fixes
> 3. exynos fixes with the problem solved can be queued for -rc2 or
> later.
> 

Agreed

Re: [GIT PULL] Thermal management updates for v4.17-rc1

[Eduardo Valentin]

On Fri, Apr 13, 2018 at 03:08:03AM -0700, Eduardo Valentin wrote:
> On Fri, Apr 13, 2018 at 01:39:05PM +0800, Zhang Rui wrote:
> > Hi, Eduardo,
> > 
> > On 四, 2018-04-12 at 21:08 -0700, Eduardo Valentin wrote:
> > > Hello,
> > > 
> > > On Thu, Apr 12, 2018 at 09:55:19AM -0700, Linus Torvalds wrote:
> > > > 
> > > > On Wed, Apr 11, 2018 at 10:08 PM, Zhang Rui <rui.zhang@intel.com>
> > > > wrote:
> > > > > 
> > > > > 
> > > > > could you please illustrate me what the kconfig & warning is?
> > > > Just "make allmodconfig" and the warning is about a uninitialized
> > > > variable.
> > > > 
> > > > Line 304 in drivers/thermal/samsung/exynos_tmu.c if my shell
> > > > history
> > > > is to be believed.
> > > > 
> > > >                 Linus
> > > Yeah, this has also passed my local compilation error. Somehow my
> > > gcc4.9
> > > is not catching it. Using an older gcc (gcc4.6) does catch it.
> > > 
> > > Anyways, given that the conversion functions are written to cover
> > > for unexpected cal_type, the right way of fixing this is to rewrite
> > > the conversion functions to allow for returning error codes and
> > > adjusting the callers as expected.
> > > 
> > > Rui, bzolnier, please consider the following fix:
> > > 
> > as it is late in this merge window, I'd prefer to
> > 1. drop all the thermal-soc material in the first pull request which I
> > will send out soon.
> > 2. you can prepare another pull request containing the thermal-soc
> > materials except the exynos fixes

Sent you this
https://marc.info/?l=linux-pm&m=152361492711499&w=2

> > 3. exynos fixes with the problem solved can be queued for -rc2 or
> > later.

I see there is still some discussion around the topic of how to fix
this. So, once we get to a point of agreement, I will send the remaining
with exynos fixes.

> > 
> 
> Agreed
> 

Re: [GIT PULL] Thermal management updates for v4.17-rc1

[Bartlomiej Zolnierkiewicz]

On Friday, April 13, 2018 03:08:03 AM Eduardo Valentin wrote:
> On Fri, Apr 13, 2018 at 01:39:05PM +0800, Zhang Rui wrote:
> > Hi, Eduardo,
> > 
> > On 四, 2018-04-12 at 21:08 -0700, Eduardo Valentin wrote:
> > > Hello,
> > > 
> > > On Thu, Apr 12, 2018 at 09:55:19AM -0700, Linus Torvalds wrote:
> > > > 
> > > > On Wed, Apr 11, 2018 at 10:08 PM, Zhang Rui <rui.zhang@intel.com>
> > > > wrote:
> > > > > 
> > > > > 
> > > > > could you please illustrate me what the kconfig & warning is?
> > > > Just "make allmodconfig" and the warning is about a uninitialized
> > > > variable.
> > > > 
> > > > Line 304 in drivers/thermal/samsung/exynos_tmu.c if my shell
> > > > history
> > > > is to be believed.
> > > > 
> > > >                 Linus
> > > Yeah, this has also passed my local compilation error. Somehow my
> > > gcc4.9
> > > is not catching it. Using an older gcc (gcc4.6) does catch it.
> > > 
> > > Anyways, given that the conversion functions are written to cover
> > > for unexpected cal_type, the right way of fixing this is to rewrite
> > > the conversion functions to allow for returning error codes and
> > > adjusting the callers as expected.
> > > 
> > > Rui, bzolnier, please consider the following fix:
> > > 
> > as it is late in this merge window, I'd prefer to
> > 1. drop all the thermal-soc material in the first pull request which I
> > will send out soon.
> > 2. you can prepare another pull request containing the thermal-soc
> > materials except the exynos fixes
> > 3. exynos fixes with the problem solved can be queued for -rc2 or
> > later.
> > 
> 
> Agreed

What should I do now to help resolve the issue?

[ There has been no action from you on Arnd's fix for over two weeks and
  also you have not commented on it now.. ]

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Re: [GIT PULL] Thermal management updates for v4.17-rc1

[Daniel Lezcano]

On 13/04/2018 11:28, Bartlomiej Zolnierkiewicz wrote:

[ ... ]

>>> It is okay to return 0 because this code-path (the default one) will be
>>> never hit by the driver (probe makes sure of it) - the default case is
>>> here is just to silence compilation errors..
>>
>> The init function is making sure cal_type is one or another. Can you fix
>> it correctly by replacing the 'switch' by a 'if' instead of adding dead
>> branches to please gcc?
>>
>> if (data->cal_type == TYPE_TWO_POINT_TRIMMING) {
>> 	return ...;
>> }
>>
>> return ...;
> 
> I'm not the one that added this switch statement (it has been there since
> 2011) and I would be happy to remove it. 

Actually the switch statement was fine until the cleanup.

> However could we please defer
> this to v4.17 and merge the current set of Exynos thermal fixes/cleanups
> (they simplify the driver a lot and make ground for future changes)?

Regarding the latest comment, this can be fixed properly by 'return' (or
whatever you want which does not get around of gcc warnings).

-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [GIT PULL] Thermal management updates for v4.17-rc1

[Bartlomiej Zolnierkiewicz]

On Friday, April 13, 2018 12:30:04 PM Daniel Lezcano wrote:
> On 13/04/2018 11:28, Bartlomiej Zolnierkiewicz wrote:
> 
> [ ... ]
> 
> >>> It is okay to return 0 because this code-path (the default one) will be
> >>> never hit by the driver (probe makes sure of it) - the default case is
> >>> here is just to silence compilation errors..
> >>
> >> The init function is making sure cal_type is one or another. Can you fix
> >> it correctly by replacing the 'switch' by a 'if' instead of adding dead
> >> branches to please gcc?
> >>
> >> if (data->cal_type == TYPE_TWO_POINT_TRIMMING) {
> >> 	return ...;
> >> }
> >>
> >> return ...;
> > 
> > I'm not the one that added this switch statement (it has been there since
> > 2011) and I would be happy to remove it. 
> 
> Actually the switch statement was fine until the cleanup.

I don't see how it was fine before as the driver has never used the default
case (always used TYPE_ONE_POINT_TRIMMING or TYPE_TWO_POINT_TRIMMING).

Could you please explain this more?

> > However could we please defer
> > this to v4.17 and merge the current set of Exynos thermal fixes/cleanups
> > (they simplify the driver a lot and make ground for future changes)?
> 
> Regarding the latest comment, this can be fixed properly by 'return' (or
> whatever you want which does not get around of gcc warnings).

Do you mean that you want the patch with switch statement removal?

Is incremental fix OK or do you want something else?

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

[PATCH] thermal/drivers/exynos_tmu: Fix warnings in temp_to_code / code_to_temp

[Daniel Lezcano]

The latest driver cleanup introduced a compilation warning

 drivers/thermal/samsung/exynos_tmu.c: In function ‘exynos_get_temp’:
 drivers/thermal/samsung/exynos_tmu.c:931:37: warning: ‘temp’ may be used uninitialized in this function [-Wmaybe-uninitialized]
   *temp = code_to_temp(data, value) * MCELSIUS;
                                        ^

 drivers/thermal/samsung/exynos_tmu.c: In function ‘temp_to_code’
 drivers/thermal/samsung/exynos_tmu.c:304:9: warning: ‘temp_code’ may be used uninitialized in this function [-Wmaybe-uninitialized]
					  return temp_code;
					           ^~~~~~~~~

The compiler gives a warning because semantically speaking the
function has no default value. However the code path, were the
variable is never initialized is a dead branch because the switch
statement always choose one of the two cases as the data->cal_type is
initialized in the init function to one of both values.

This is unclear as it adds a dependency on the initialization function
and it is prone to error. Make things clearer by converting the
functions with if ... return statements, thus showing we are expecting
the values to be correctly filled before calling this function.

This change fixes the couple of function warnings.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/thermal/samsung/exynos_tmu.c | 46 ++++++++++--------------------------
 1 file changed, 12 insertions(+), 34 deletions(-)

diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
index 2ec8548..197f267 100644
--- a/drivers/thermal/samsung/exynos_tmu.c
+++ b/drivers/thermal/samsung/exynos_tmu.c
@@ -284,24 +284,13 @@ static void exynos_report_trigger(struct exynos_tmu_data *p)
  */
 static int temp_to_code(struct exynos_tmu_data *data, u8 temp)
 {
-	int temp_code;
-
-	switch (data->cal_type) {
-	case TYPE_TWO_POINT_TRIMMING:
-		temp_code = (temp - EXYNOS_FIRST_POINT_TRIM) *
-			(data->temp_error2 - data->temp_error1) /
-			(EXYNOS_SECOND_POINT_TRIM - EXYNOS_FIRST_POINT_TRIM) +
-			data->temp_error1;
-		break;
-	case TYPE_ONE_POINT_TRIMMING:
-		temp_code = temp + data->temp_error1 - EXYNOS_FIRST_POINT_TRIM;
-		break;
-	default:
-		WARN_ON(1);
-		break;
-	}
+	if (data->cal_type == TYPE_ONE_POINT_TRIMMING)
+		return temp + data->temp_error1 - EXYNOS_FIRST_POINT_TRIM;
 
-	return temp_code;
+	return (temp - EXYNOS_FIRST_POINT_TRIM) *
+		(data->temp_error2 - data->temp_error1) /
+		(EXYNOS_SECOND_POINT_TRIM - EXYNOS_FIRST_POINT_TRIM) +
+		data->temp_error1;
 }
 
 /*
@@ -310,24 +299,13 @@ static int temp_to_code(struct exynos_tmu_data *data, u8 temp)
  */
 static int code_to_temp(struct exynos_tmu_data *data, u16 temp_code)
 {
-	int temp;
-
-	switch (data->cal_type) {
-	case TYPE_TWO_POINT_TRIMMING:
-		temp = (temp_code - data->temp_error1) *
-			(EXYNOS_SECOND_POINT_TRIM - EXYNOS_FIRST_POINT_TRIM) /
-			(data->temp_error2 - data->temp_error1) +
-			EXYNOS_FIRST_POINT_TRIM;
-		break;
-	case TYPE_ONE_POINT_TRIMMING:
-		temp = temp_code - data->temp_error1 + EXYNOS_FIRST_POINT_TRIM;
-		break;
-	default:
-		WARN_ON(1);
-		break;
-	}
+	if (data->cal_type == TYPE_ONE_POINT_TRIMMING)
+		return temp_code - data->temp_error1 + EXYNOS_FIRST_POINT_TRIM;
 
-	return temp;
+	return (temp_code - data->temp_error1) *
+		(EXYNOS_SECOND_POINT_TRIM - EXYNOS_FIRST_POINT_TRIM) /
+		(data->temp_error2 - data->temp_error1) +
+		EXYNOS_FIRST_POINT_TRIM;
 }
 
 static void sanitize_temp_error(struct exynos_tmu_data *data, u32 trim_info)
-- 
2.7.4

Re: [PATCH] thermal/drivers/exynos_tmu: Fix warnings in temp_to_code / code_to_temp

[Bartlomiej Zolnierkiewicz]

On Friday, April 13, 2018 01:00:09 PM Daniel Lezcano wrote:
> The latest driver cleanup introduced a compilation warning
> 
>  drivers/thermal/samsung/exynos_tmu.c: In function ‘exynos_get_temp’:
>  drivers/thermal/samsung/exynos_tmu.c:931:37: warning: ‘temp’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>    *temp = code_to_temp(data, value) * MCELSIUS;
>                                         ^
> 
>  drivers/thermal/samsung/exynos_tmu.c: In function ‘temp_to_code’
>  drivers/thermal/samsung/exynos_tmu.c:304:9: warning: ‘temp_code’ may be used uninitialized in this function [-Wmaybe-uninitialized]
> 					  return temp_code;
> 					           ^~~~~~~~~
> 
> The compiler gives a warning because semantically speaking the
> function has no default value. However the code path, were the
> variable is never initialized is a dead branch because the switch
> statement always choose one of the two cases as the data->cal_type is
> initialized in the init function to one of both values.
> 
> This is unclear as it adds a dependency on the initialization function
> and it is prone to error. Make things clearer by converting the
> functions with if ... return statements, thus showing we are expecting
> the values to be correctly filled before calling this function.
> 
> This change fixes the couple of function warnings.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

Thanks Daniel, this is much better fix.

Acked-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>

> ---
>  drivers/thermal/samsung/exynos_tmu.c | 46 ++++++++++--------------------------
>  1 file changed, 12 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
> index 2ec8548..197f267 100644
> --- a/drivers/thermal/samsung/exynos_tmu.c
> +++ b/drivers/thermal/samsung/exynos_tmu.c
> @@ -284,24 +284,13 @@ static void exynos_report_trigger(struct exynos_tmu_data *p)
>   */
>  static int temp_to_code(struct exynos_tmu_data *data, u8 temp)
>  {
> -	int temp_code;
> -
> -	switch (data->cal_type) {
> -	case TYPE_TWO_POINT_TRIMMING:
> -		temp_code = (temp - EXYNOS_FIRST_POINT_TRIM) *
> -			(data->temp_error2 - data->temp_error1) /
> -			(EXYNOS_SECOND_POINT_TRIM - EXYNOS_FIRST_POINT_TRIM) +
> -			data->temp_error1;
> -		break;
> -	case TYPE_ONE_POINT_TRIMMING:
> -		temp_code = temp + data->temp_error1 - EXYNOS_FIRST_POINT_TRIM;
> -		break;
> -	default:
> -		WARN_ON(1);
> -		break;
> -	}
> +	if (data->cal_type == TYPE_ONE_POINT_TRIMMING)
> +		return temp + data->temp_error1 - EXYNOS_FIRST_POINT_TRIM;
>  
> -	return temp_code;
> +	return (temp - EXYNOS_FIRST_POINT_TRIM) *
> +		(data->temp_error2 - data->temp_error1) /
> +		(EXYNOS_SECOND_POINT_TRIM - EXYNOS_FIRST_POINT_TRIM) +
> +		data->temp_error1;
>  }
>  
>  /*
> @@ -310,24 +299,13 @@ static int temp_to_code(struct exynos_tmu_data *data, u8 temp)
>   */
>  static int code_to_temp(struct exynos_tmu_data *data, u16 temp_code)
>  {
> -	int temp;
> -
> -	switch (data->cal_type) {
> -	case TYPE_TWO_POINT_TRIMMING:
> -		temp = (temp_code - data->temp_error1) *
> -			(EXYNOS_SECOND_POINT_TRIM - EXYNOS_FIRST_POINT_TRIM) /
> -			(data->temp_error2 - data->temp_error1) +
> -			EXYNOS_FIRST_POINT_TRIM;
> -		break;
> -	case TYPE_ONE_POINT_TRIMMING:
> -		temp = temp_code - data->temp_error1 + EXYNOS_FIRST_POINT_TRIM;
> -		break;
> -	default:
> -		WARN_ON(1);
> -		break;
> -	}
> +	if (data->cal_type == TYPE_ONE_POINT_TRIMMING)
> +		return temp_code - data->temp_error1 + EXYNOS_FIRST_POINT_TRIM;
>  
> -	return temp;
> +	return (temp_code - data->temp_error1) *
> +		(EXYNOS_SECOND_POINT_TRIM - EXYNOS_FIRST_POINT_TRIM) /
> +		(data->temp_error2 - data->temp_error1) +
> +		EXYNOS_FIRST_POINT_TRIM;
>  }
>  
>  static void sanitize_temp_error(struct exynos_tmu_data *data, u32 trim_info)

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Re: [GIT PULL] Thermal management updates for v4.17-rc1

[Daniel Lezcano]

On 13/04/2018 12:41, Bartlomiej Zolnierkiewicz wrote:
> On Friday, April 13, 2018 12:30:04 PM Daniel Lezcano wrote:
>> On 13/04/2018 11:28, Bartlomiej Zolnierkiewicz wrote:
>>
>> [ ... ]
>>
>>>>> It is okay to return 0 because this code-path (the default one) will be
>>>>> never hit by the driver (probe makes sure of it) - the default case is
>>>>> here is just to silence compilation errors..
>>>>
>>>> The init function is making sure cal_type is one or another. Can you fix
>>>> it correctly by replacing the 'switch' by a 'if' instead of adding dead
>>>> branches to please gcc?
>>>>
>>>> if (data->cal_type == TYPE_TWO_POINT_TRIMMING) {
>>>> 	return ...;
>>>> }
>>>>
>>>> return ...;
>>>
>>> I'm not the one that added this switch statement (it has been there since
>>> 2011) and I would be happy to remove it. 
>>
>> Actually the switch statement was fine until the cleanup.
> 
> I don't see how it was fine before as the driver has never used the default
> case (always used TYPE_ONE_POINT_TRIMMING or TYPE_TWO_POINT_TRIMMING).
> 
> Could you please explain this more?

>From commit 480b5bfc16e17ef51ca1c

+++ b/drivers/thermal/samsung/exynos_tmu.c
@@ -260,7 +260,7 @@ static int temp_to_code(struct exynos_tmu_data
*data, u8 temp)
                temp_code = temp + data->temp_error1 -
pdata->first_point_trim;
                break;
        default:
-               temp_code = temp + pdata->default_temp_offset;
+               WARN_ON(1);
                break;
        }

@@ -287,7 +287,7 @@ static int code_to_temp(struct exynos_tmu_data
*data, u16 temp_code)
                temp = temp_code - data->temp_error1 +
pdata->first_point_trim;
                break;
        default:
-               temp = temp_code - pdata->default_temp_offset;
+               WARN_ON(1);
                break;
        }

I'm not saying the code path was fine but from the compiler point of
view, it was. By removing the defaulting temp value there is a code path
gcc sees the temp variable as not initialized.

Your cleanups are relevant.


> --
> Bartlomiej Zolnierkiewicz
> Samsung R&D Institute Poland
> Samsung Electronics
> 


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [GIT PULL] Thermal management updates for v4.17-rc1

[Bartlomiej Zolnierkiewicz]

On Friday, April 13, 2018 12:41:18 PM Bartlomiej Zolnierkiewicz wrote:
> On Friday, April 13, 2018 12:30:04 PM Daniel Lezcano wrote:
> > On 13/04/2018 11:28, Bartlomiej Zolnierkiewicz wrote:
> > 
> > [ ... ]
> > 
> > >>> It is okay to return 0 because this code-path (the default one) will be
> > >>> never hit by the driver (probe makes sure of it) - the default case is
> > >>> here is just to silence compilation errors..
> > >>
> > >> The init function is making sure cal_type is one or another. Can you fix
> > >> it correctly by replacing the 'switch' by a 'if' instead of adding dead
> > >> branches to please gcc?
> > >>
> > >> if (data->cal_type == TYPE_TWO_POINT_TRIMMING) {
> > >> 	return ...;
> > >> }
> > >>
> > >> return ...;
> > > 
> > > I'm not the one that added this switch statement (it has been there since
> > > 2011) and I would be happy to remove it. 
> > 
> > Actually the switch statement was fine until the cleanup.
> 
> I don't see how it was fine before as the driver has never used the default
> case (always used TYPE_ONE_POINT_TRIMMING or TYPE_TWO_POINT_TRIMMING).
> 
> Could you please explain this more?
> 
> > > However could we please defer
> > > this to v4.17 and merge the current set of Exynos thermal fixes/cleanups
> > > (they simplify the driver a lot and make ground for future changes)?
> > 
> > Regarding the latest comment, this can be fixed properly by 'return' (or
> > whatever you want which does not get around of gcc warnings).
> 
> Do you mean that you want the patch with switch statement removal?
> 
> Is incremental fix OK or do you want something else?

Danial has already posted it, I hope the fix is fine with you.

Also sorry for the delay with handling issue - I was on holiday last two
days and for some reason I was under (wrong) impression that the previous
fix has been in thermal tree (so I was quite surprised today reading this
mail thread).

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Re: [GIT PULL] Thermal management updates for v4.17-rc1

[Bartlomiej Zolnierkiewicz]

On Friday, April 13, 2018 01:12:39 PM Bartlomiej Zolnierkiewicz wrote:
> On Friday, April 13, 2018 12:41:18 PM Bartlomiej Zolnierkiewicz wrote:
> > On Friday, April 13, 2018 12:30:04 PM Daniel Lezcano wrote:
> > > On 13/04/2018 11:28, Bartlomiej Zolnierkiewicz wrote:
> > > 
> > > [ ... ]
> > > 
> > > >>> It is okay to return 0 because this code-path (the default one) will be
> > > >>> never hit by the driver (probe makes sure of it) - the default case is
> > > >>> here is just to silence compilation errors..
> > > >>
> > > >> The init function is making sure cal_type is one or another. Can you fix
> > > >> it correctly by replacing the 'switch' by a 'if' instead of adding dead
> > > >> branches to please gcc?
> > > >>
> > > >> if (data->cal_type == TYPE_TWO_POINT_TRIMMING) {
> > > >> 	return ...;
> > > >> }
> > > >>
> > > >> return ...;
> > > > 
> > > > I'm not the one that added this switch statement (it has been there since
> > > > 2011) and I would be happy to remove it. 
> > > 
> > > Actually the switch statement was fine until the cleanup.
> > 
> > I don't see how it was fine before as the driver has never used the default
> > case (always used TYPE_ONE_POINT_TRIMMING or TYPE_TWO_POINT_TRIMMING).
> > 
> > Could you please explain this more?
> > 
> > > > However could we please defer
> > > > this to v4.17 and merge the current set of Exynos thermal fixes/cleanups
> > > > (they simplify the driver a lot and make ground for future changes)?
> > > 
> > > Regarding the latest comment, this can be fixed properly by 'return' (or
> > > whatever you want which does not get around of gcc warnings).
> > 
> > Do you mean that you want the patch with switch statement removal?
> > 
> > Is incremental fix OK or do you want something else?
> 
> Danial has already posted it, I hope the fix is fine with you.

should have been:

Eduardo: Daniel has already posted it, I hope the fix is fine with you.

(& sorry for the typo)

> Also sorry for the delay with handling issue - I was on holiday last two
> days and for some reason I was under (wrong) impression that the previous
> fix has been in thermal tree (so I was quite surprised today reading this
> mail thread).

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Re: [GIT PULL] Thermal management updates for v4.17-rc1

[Eduardo Valentin]

On Fri, Apr 13, 2018 at 12:27:17PM +0200, Bartlomiej Zolnierkiewicz wrote:
> On Friday, April 13, 2018 03:08:03 AM Eduardo Valentin wrote:
> > On Fri, Apr 13, 2018 at 01:39:05PM +0800, Zhang Rui wrote:
> > > Hi, Eduardo,
> > > 
> > > On 四, 2018-04-12 at 21:08 -0700, Eduardo Valentin wrote:
> > > > Hello,
> > > > 
> > > > On Thu, Apr 12, 2018 at 09:55:19AM -0700, Linus Torvalds wrote:
> > > > > 
> > > > > On Wed, Apr 11, 2018 at 10:08 PM, Zhang Rui <rui.zhang@intel.com>
> > > > > wrote:
> > > > > > 
> > > > > > 
> > > > > > could you please illustrate me what the kconfig & warning is?
> > > > > Just "make allmodconfig" and the warning is about a uninitialized
> > > > > variable.
> > > > > 
> > > > > Line 304 in drivers/thermal/samsung/exynos_tmu.c if my shell
> > > > > history
> > > > > is to be believed.
> > > > > 
> > > > >                 Linus
> > > > Yeah, this has also passed my local compilation error. Somehow my
> > > > gcc4.9
> > > > is not catching it. Using an older gcc (gcc4.6) does catch it.
> > > > 
> > > > Anyways, given that the conversion functions are written to cover
> > > > for unexpected cal_type, the right way of fixing this is to rewrite
> > > > the conversion functions to allow for returning error codes and
> > > > adjusting the callers as expected.
> > > > 
> > > > Rui, bzolnier, please consider the following fix:
> > > > 
> > > as it is late in this merge window, I'd prefer to
> > > 1. drop all the thermal-soc material in the first pull request which I
> > > will send out soon.
> > > 2. you can prepare another pull request containing the thermal-soc
> > > materials except the exynos fixes
> > > 3. exynos fixes with the problem solved can be queued for -rc2 or
> > > later.
> > > 
> > 
> > Agreed
> 
> What should I do now to help resolve the issue?

Please resend your series with the patches without the warnings..

Thanks,

Eduardo