Message-ID: <1523597945.16235.52.camel@intel.com>
Date:   Fri, 13 Apr 2018 13:39:05 +0800
From:   Zhang Rui <rui.zhang@...el.com>
To:     Eduardo Valentin <edubezval@...il.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Linux PM list <linux-pm@...r.kernel.org>,
        "Li, Philip" <philip.li@...el.com>
Subject: Re: [GIT PULL] Thermal management updates for v4.17-rc1

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@...el.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@...il.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@...il.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)

Powered by blists - more mailing lists