diff mbox series

thermal: rcar_gen3_thermal: Fix undefined temperature if negative

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

Commit Message

Yoshihiro Shimoda June 8, 2020, 11:12 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.

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>
---
 drivers/thermal/rcar_gen3_thermal.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Geert Uytterhoeven June 8, 2020, 11:38 a.m. UTC | #1 
Hi Shimoda-san, Pham-san,

On Mon, Jun 8, 2020 at 1:13 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.
>
> 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>

Thanks for your patch!

> --- 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.

>         /* 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.

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.

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
Yoshihiro Shimoda June 9, 2020, 2:34 a.m. UTC | #2 
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
Geert Uytterhoeven June 9, 2020, 7:24 a.m. UTC | #3 
Hi Shimoda-san,

On Tue, Jun 9, 2020 at 4:34 AM Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
> > 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:

Sorry for being unclear: I literally meant to add casts to the macros,
not to the callers.  If the macros are safe against this issue, then the
callers don't have to care anymore.
But this might be overkill, as the issue is present in one place only.

> 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);

No need for a cast to int here, as assignment to ctemp takes care
of that.

>
>         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);

That would work too.

Gr{oetje,eeting}s,

                        Geert
Yoshihiro Shimoda June 9, 2020, 11 a.m. UTC | #4 
Hi Geert-san,

> From: Geert Uytterhoeven, Sent: Tuesday, June 9, 2020 4:24 PM
> 
> Hi Shimoda-san,
> 
> On Tue, Jun 9, 2020 at 4:34 AM Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@renesas.com> wrote:
> > > 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:
> 
> Sorry for being unclear: I literally meant to add casts to the macros,
> not to the callers.  If the macros are safe against this issue, then the
> callers don't have to care anymore.
> But this might be overkill, as the issue is present in one place only.

I got it.

> > 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);
> 
> No need for a cast to int here, as assignment to ctemp takes care
> of that.

Thank you for your comment. I'll remove this cast.

> >
> >         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);
> 
> That would work too.

Thank you for your review! I'll submit v2 patch later.

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
diff mbox series

Patch

diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
index 58fe7c1..537116e 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;
+	long reg;
 
 	/* Read register and convert to mili Celsius */
 	reg = rcar_gen3_thermal_read(tsc, REG_GEN3_TEMP) & CTEMP_MASK;