| Message ID | 20180413040855.GA29826@localhost.localdomain (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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
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)
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
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
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 >
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
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 ...;
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
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
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 >
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
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).
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
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 >
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
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
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
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)