diff mbox series

[v2] thermal: rcar_gen3_thermal: Fix undefined temperature if negative

Message ID 1591703110-14869-1-git-send-email-yoshihiro.shimoda.uh@renesas.com (mailing list archive)
State New, archived
Delegated to: Daniel Lezcano
Headers show
Series [v2] thermal: rcar_gen3_thermal: Fix undefined temperature if negative | expand

Commit Message

Yoshihiro Shimoda June 9, 2020, 11:45 a.m. UTC 
From: Dien Pham <dien.pham.ry@renesas.com>

As description for DIV_ROUND_CLOSEST in file include/linux/kernel.h.
  "Result is undefined for negative divisors if the dividend variable
   type is unsigned and for negative dividends if the divisor variable
   type is unsigned."

In current code, the FIXPT_DIV uses DIV_ROUND_CLOSEST but has not
checked sign of divisor before using. It makes undefined temperature
value in case the value is negative.

This patch fixes to satisfy DIV_ROUND_CLOSEST description
and fix bug too. Note that the name "reg" is not good because it should
be the same type as rcar_gen3_thermal_read(). So, rename it with "ctemp".

Signed-off-by: Van Do <van.do.xw@renesas.com>
Signed-off-by: Dien Pham <dien.pham.ry@renesas.com>
[shimoda: minor fixes, add Fixes tag]
Fixes: 564e73d283af ("thermal: rcar_gen3_thermal: Add R-Car Gen3 thermal driver")
Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 Changes from v1:
 - Use int instead of long.
 - Rename "reg" with "ctemp".
 https://patchwork.kernel.org/patch/11593051/

 drivers/thermal/rcar_gen3_thermal.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Comments

Niklas Söderlund June 10, 2020, 12:13 a.m. UTC | #1 
Hi Shimoda-san and Pham-san,

Thanks for your work.

On 2020-06-09 20:45:10 +0900, Yoshihiro Shimoda wrote:
> From: Dien Pham <dien.pham.ry@renesas.com>
> 
> As description for DIV_ROUND_CLOSEST in file include/linux/kernel.h.
>   "Result is undefined for negative divisors if the dividend variable
>    type is unsigned and for negative dividends if the divisor variable
>    type is unsigned."
> 
> In current code, the FIXPT_DIV uses DIV_ROUND_CLOSEST but has not
> checked sign of divisor before using. It makes undefined temperature
> value in case the value is negative.
> 
> This patch fixes to satisfy DIV_ROUND_CLOSEST description
> and fix bug too. Note that the name "reg" is not good because it should
> be the same type as rcar_gen3_thermal_read(). So, rename it with "ctemp".
> 
> Signed-off-by: Van Do <van.do.xw@renesas.com>
> Signed-off-by: Dien Pham <dien.pham.ry@renesas.com>
> [shimoda: minor fixes, add Fixes tag]
> Fixes: 564e73d283af ("thermal: rcar_gen3_thermal: Add R-Car Gen3 thermal driver")
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Tested-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

> ---
>  Changes from v1:
>  - Use int instead of long.
>  - Rename "reg" with "ctemp".
>  https://patchwork.kernel.org/patch/11593051/
> 
>  drivers/thermal/rcar_gen3_thermal.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
> index 58fe7c1..49ea330 100644
> --- a/drivers/thermal/rcar_gen3_thermal.c
> +++ b/drivers/thermal/rcar_gen3_thermal.c
> @@ -167,16 +167,16 @@ static int rcar_gen3_thermal_get_temp(void *devdata, int *temp)
>  {
>  	struct rcar_gen3_thermal_tsc *tsc = devdata;
>  	int mcelsius, val;
> -	u32 reg;
> +	int ctemp;
>  
>  	/* Read register and convert to mili Celsius */
> -	reg = rcar_gen3_thermal_read(tsc, REG_GEN3_TEMP) & CTEMP_MASK;
> +	ctemp = rcar_gen3_thermal_read(tsc, REG_GEN3_TEMP) & CTEMP_MASK;
>  
> -	if (reg <= thcode[tsc->id][1])
> -		val = FIXPT_DIV(FIXPT_INT(reg) - tsc->coef.b1,
> +	if (ctemp <= thcode[tsc->id][1])
> +		val = FIXPT_DIV(FIXPT_INT(ctemp) - tsc->coef.b1,
>  				tsc->coef.a1);
>  	else
> -		val = FIXPT_DIV(FIXPT_INT(reg) - tsc->coef.b2,
> +		val = FIXPT_DIV(FIXPT_INT(ctemp) - tsc->coef.b2,
>  				tsc->coef.a2);
>  	mcelsius = FIXPT_TO_MCELSIUS(val);
>  
> -- 
> 2.7.4
>
Amit Kucheria June 23, 2020, 12:06 p.m. UTC | #2 
Hi,

On Tue, Jun 9, 2020 at 5:15 PM Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
>
> From: Dien Pham <dien.pham.ry@renesas.com>
>
> As description for DIV_ROUND_CLOSEST in file include/linux/kernel.h.
>   "Result is undefined for negative divisors if the dividend variable
>    type is unsigned and for negative dividends if the divisor variable
>    type is unsigned."
>
> In current code, the FIXPT_DIV uses DIV_ROUND_CLOSEST but has not
> checked sign of divisor before using. It makes undefined temperature
> value in case the value is negative.
>
> This patch fixes to satisfy DIV_ROUND_CLOSEST description
> and fix bug too. Note that the name "reg" is not good because it should
> be the same type as rcar_gen3_thermal_read(). So, rename it with "ctemp".
>
> Signed-off-by: Van Do <van.do.xw@renesas.com>
> Signed-off-by: Dien Pham <dien.pham.ry@renesas.com>
> [shimoda: minor fixes, add Fixes tag]
> Fixes: 564e73d283af ("thermal: rcar_gen3_thermal: Add R-Car Gen3 thermal driver")
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
>  Changes from v1:
>  - Use int instead of long.
>  - Rename "reg" with "ctemp".
>  https://patchwork.kernel.org/patch/11593051/
>
>  drivers/thermal/rcar_gen3_thermal.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
> index 58fe7c1..49ea330 100644
> --- a/drivers/thermal/rcar_gen3_thermal.c
> +++ b/drivers/thermal/rcar_gen3_thermal.c
> @@ -167,16 +167,16 @@ static int rcar_gen3_thermal_get_temp(void *devdata, int *temp)
>  {
>         struct rcar_gen3_thermal_tsc *tsc = devdata;
>         int mcelsius, val;
> -       u32 reg;
> +       int ctemp;

Wouldn't it be better to change the variable type to fix the bug in
this patch and make a separate one for the variable rename?

Regards,
Amit

>         /* Read register and convert to mili Celsius */
> -       reg = rcar_gen3_thermal_read(tsc, REG_GEN3_TEMP) & CTEMP_MASK;
> +       ctemp = rcar_gen3_thermal_read(tsc, REG_GEN3_TEMP) & CTEMP_MASK;
>
> -       if (reg <= thcode[tsc->id][1])
> -               val = FIXPT_DIV(FIXPT_INT(reg) - tsc->coef.b1,
> +       if (ctemp <= thcode[tsc->id][1])
> +               val = FIXPT_DIV(FIXPT_INT(ctemp) - tsc->coef.b1,
>                                 tsc->coef.a1);
>         else
> -               val = FIXPT_DIV(FIXPT_INT(reg) - tsc->coef.b2,
> +               val = FIXPT_DIV(FIXPT_INT(ctemp) - tsc->coef.b2,
>                                 tsc->coef.a2);
>         mcelsius = FIXPT_TO_MCELSIUS(val);
>
> --
> 2.7.4
>
Yoshihiro Shimoda June 25, 2020, 2:06 a.m. UTC | #3 
Hi,

> From: Amit Kucheria, Sent: Tuesday, June 23, 2020 9:07 PM
<snip>
> > --- a/drivers/thermal/rcar_gen3_thermal.c
> > +++ b/drivers/thermal/rcar_gen3_thermal.c
> > @@ -167,16 +167,16 @@ static int rcar_gen3_thermal_get_temp(void *devdata, int *temp)
> >  {
> >         struct rcar_gen3_thermal_tsc *tsc = devdata;
> >         int mcelsius, val;
> > -       u32 reg;
> > +       int ctemp;
> 
> Wouldn't it be better to change the variable type to fix the bug in
> this patch and make a separate one for the variable rename?

I got it. I'll keep the variable name in this patch.

Best regards,
Yoshihiro Shimoda

> Regards,
> Amit
> 
> >         /* Read register and convert to mili Celsius */
> > -       reg = rcar_gen3_thermal_read(tsc, REG_GEN3_TEMP) & CTEMP_MASK;
> > +       ctemp = rcar_gen3_thermal_read(tsc, REG_GEN3_TEMP) & CTEMP_MASK;
> >
> > -       if (reg <= thcode[tsc->id][1])
> > -               val = FIXPT_DIV(FIXPT_INT(reg) - tsc->coef.b1,
> > +       if (ctemp <= thcode[tsc->id][1])
> > +               val = FIXPT_DIV(FIXPT_INT(ctemp) - tsc->coef.b1,
> >                                 tsc->coef.a1);
> >         else
> > -               val = FIXPT_DIV(FIXPT_INT(reg) - tsc->coef.b2,
> > +               val = FIXPT_DIV(FIXPT_INT(ctemp) - tsc->coef.b2,
> >                                 tsc->coef.a2);
> >         mcelsius = FIXPT_TO_MCELSIUS(val);
> >
> > --
> > 2.7.4
> >
diff mbox series

Patch

diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
index 58fe7c1..49ea330 100644
--- a/drivers/thermal/rcar_gen3_thermal.c
+++ b/drivers/thermal/rcar_gen3_thermal.c
@@ -167,16 +167,16 @@  static int rcar_gen3_thermal_get_temp(void *devdata, int *temp)
 {
 	struct rcar_gen3_thermal_tsc *tsc = devdata;
 	int mcelsius, val;
-	u32 reg;
+	int ctemp;
 
 	/* Read register and convert to mili Celsius */
-	reg = rcar_gen3_thermal_read(tsc, REG_GEN3_TEMP) & CTEMP_MASK;
+	ctemp = rcar_gen3_thermal_read(tsc, REG_GEN3_TEMP) & CTEMP_MASK;
 
-	if (reg <= thcode[tsc->id][1])
-		val = FIXPT_DIV(FIXPT_INT(reg) - tsc->coef.b1,
+	if (ctemp <= thcode[tsc->id][1])
+		val = FIXPT_DIV(FIXPT_INT(ctemp) - tsc->coef.b1,
 				tsc->coef.a1);
 	else
-		val = FIXPT_DIV(FIXPT_INT(reg) - tsc->coef.b2,
+		val = FIXPT_DIV(FIXPT_INT(ctemp) - tsc->coef.b2,
 				tsc->coef.a2);
 	mcelsius = FIXPT_TO_MCELSIUS(val);