Hi Shimoda-san,
On Tue, Jun 9, 2020 at 4:34 AM Yoshihiro Shimoda
<yoshihiro.shimoda.uh@xxxxxxxxxxx> 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
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
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
