[PATCH v3] thermal: rcar_gen3_thermal: Fix undefined temperature if negative

[PATCH v3] thermal: rcar_gen3_thermal: Fix undefined temperature if negative

[Yoshihiro Shimoda]

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 variable name "reg" is not good
because it should be the same type as rcar_gen3_thermal_read().
However, there is better to rename it in a further patch as
cleanup.

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 Soderlund <niklas.soderlund+renesas@ragnatech.se>
Tested-by: Niklas Soderlund <niklas.soderlund+renesas@ragnatech.se>
---
 Changes from v2:
 - Add Niklas-san's Reviewed-by and Tested-by.
 - Keep the variable name "reg" and revise the commit description for it.
 https://patchwork.kernel.org/patch/11595327/

 Changes from v1:
 - Use int instead of long.
 - Rename "reg" with "ctemp".
 https://patchwork.kernel.org/patch/11593051/

 drivers/thermal/rcar_gen3_thermal.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
index 58fe7c1..c48c5e9 100644
--- a/drivers/thermal/rcar_gen3_thermal.c
+++ b/drivers/thermal/rcar_gen3_thermal.c
@@ -167,7 +167,7 @@ static int rcar_gen3_thermal_get_temp(void *devdata, int *temp)
 {
 	struct rcar_gen3_thermal_tsc *tsc = devdata;
 	int mcelsius, val;
-	u32 reg;
+	int reg;
 
 	/* Read register and convert to mili Celsius */
 	reg = rcar_gen3_thermal_read(tsc, REG_GEN3_TEMP) & CTEMP_MASK;
-- 
2.7.4

Re: [PATCH v3] thermal: rcar_gen3_thermal: Fix undefined temperature if negative

[Amit Kucheria]

On Thu, Jun 25, 2020 at 8:26 AM 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 variable name "reg" is not good
> because it should be the same type as rcar_gen3_thermal_read().
> However, there is better to rename it in a further patch as
> cleanup.
>
> 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 Soderlund <niklas.soderlund+renesas@ragnatech.se>
> Tested-by: Niklas Soderlund <niklas.soderlund+renesas@ragnatech.se>

Reviewed-by: Amit Kucheria <amit.kucheria@linaro.org>

> ---
>  Changes from v2:
>  - Add Niklas-san's Reviewed-by and Tested-by.
>  - Keep the variable name "reg" and revise the commit description for it.
>  https://patchwork.kernel.org/patch/11595327/
>
>  Changes from v1:
>  - Use int instead of long.
>  - Rename "reg" with "ctemp".
>  https://patchwork.kernel.org/patch/11593051/
>
>  drivers/thermal/rcar_gen3_thermal.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
> index 58fe7c1..c48c5e9 100644
> --- a/drivers/thermal/rcar_gen3_thermal.c
> +++ b/drivers/thermal/rcar_gen3_thermal.c
> @@ -167,7 +167,7 @@ static int rcar_gen3_thermal_get_temp(void *devdata, int *temp)
>  {
>         struct rcar_gen3_thermal_tsc *tsc = devdata;
>         int mcelsius, val;
> -       u32 reg;
> +       int reg;
>
>         /* Read register and convert to mili Celsius */
>         reg = rcar_gen3_thermal_read(tsc, REG_GEN3_TEMP) & CTEMP_MASK;
> --
> 2.7.4
>

Re: [PATCH v3] thermal: rcar_gen3_thermal: Fix undefined temperature if negative

[Sergei Shtylyov]

Hello!

On 25.06.2020 5:56, 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 variable name "reg" is not good
> because it should be the same type as rcar_gen3_thermal_read().
> However, there is better to rename it in a further patch as

    It's better.

> cleanup.
> 
> 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 Soderlund <niklas.soderlund+renesas@ragnatech.se>
> Tested-by: Niklas Soderlund <niklas.soderlund+renesas@ragnatech.se>
[...]

MBR, Sergei

RE: [PATCH v3] thermal: rcar_gen3_thermal: Fix undefined temperature if negative

[Yoshihiro Shimoda]

Hello!

> From: Sergei Shtylyov, Sent: Thursday, June 25, 2020 5:58 PM
> 
> Hello!
> 
> On 25.06.2020 5:56, 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 variable name "reg" is not good
> > because it should be the same type as rcar_gen3_thermal_read().
> > However, there is better to rename it in a further patch as
> 
>     It's better.

Thank you for your review! I'll fix it.

Best regards,
Yoshihiro Shimoda

Re: [PATCH v3] thermal: rcar_gen3_thermal: Fix undefined temperature if negative

[Sergei Shtylyov]

On 06/25/2020 02:34 PM, 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 variable name "reg" is not good
>>> because it should be the same type as rcar_gen3_thermal_read().
>>> However, there is better to rename it in a further patch as
>>
>>     It's better.
> 
> Thank you for your review! I'll fix it.

   Note that "there" isn't needed there, I wasn't explicit enough, it seems... 

> Best regards,
> Yoshihiro Shimoda

MBR, Sergei