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