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

[Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>]

Hi Geert-san,

Thank you for your comments!

> From: Geert Uytterhoeven, Sent: Monday, June 8, 2020 8:38 PM
<snip>
> > --- 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;
> > +       long reg;
> 
> "long" is 64-bit, so "int" should be sufficient.

Oops.

> >         /* Read register and convert to mili Celsius */
> >         reg = rcar_gen3_thermal_read(tsc, REG_GEN3_TEMP) & CTEMP_MASK;
> 
> However, rcar_gen3_thermal_read() does return u32, so keeping u32 for
> reg looks more logical to me.
> 
> Successive lines are:
> 
>         if (reg <= thcode[tsc->id][1])
>                 val = FIXPT_DIV(FIXPT_INT(reg) - tsc->coef.b1,
>                                 tsc->coef.a1);
>         else
>                 val = FIXPT_DIV(FIXPT_INT(reg) - tsc->coef.b2,
>                                 tsc->coef.a2);
> 
> Perhaps it's safer to add an cast to FIXPT_INT(), so it always returns
> int, and/or add casts to FIXPT_DIV(), so only signed values are passed
> to DIV_ROUND_CLOSEST?
> That would prevent the issue from reappearing later.

There is not related to the issue but, thcode[] is also int.
So, if we use casts, we will see a lot of casts like below:
---
static int rcar_gen3_thermal_get_temp(void *devdata, int *temp)
{
	struct rcar_gen3_thermal_tsc *tsc = devdata;
	int mcelsius, val;
	u32 reg;

	/* Read register and convert to mili Celsius */
	reg = rcar_gen3_thermal_read(tsc, REG_GEN3_TEMP) & CTEMP_MASK;

	if ((int)reg <= thcode[tsc->id][1])
		val = (int)FIXPT_DIV((int)FIXPT_INT(reg) - tsc->coef.b1,
				tsc->coef.a1);
	else
		val = (int)FIXPT_DIV((int)FIXPT_INT(reg) - tsc->coef.b2,
				tsc->coef.a2);
---

I'm thinking the name "reg" is not good because it should be the same type as
rcar_gen3_thermal_read(). But, if we use other name like "int ctemp;", we can use
it like below. What do you think?
---
static int rcar_gen3_thermal_get_temp(void *devdata, int *temp)
{
	struct rcar_gen3_thermal_tsc *tsc = devdata;
	int mcelsius, val;
	int ctemp;

	/* Read register and convert to mili Celsius */
	ctemp = int(rcar_gen3_thermal_read(tsc, REG_GEN3_TEMP) & CTEMP_MASK);

	if (ctemp <= thcode[tsc->id][1])
		val = FIXPT_DIV(FIXPT_INT(ctemp) - tsc->coef.b1,
				tsc->coef.a1);
	else
		val = FIXPT_DIV(FIXPT_INT(ctemp) - tsc->coef.b2,
				tsc->coef.a2);
---

> BTW, rcar_gen3_thermal_mcelsius_to_temp() returns a value to store in a
> register, hence I think it should return u32 instead of int.

I think so.

Best regards,
Yoshihiro Shimoda

> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds