Hi Shimoda-san, Pham-san,
On Mon, Jun 8, 2020 at 1:13 PM Yoshihiro Shimoda
<yoshihiro.shimoda.uh@xxxxxxxxxxx> wrote:
> From: Dien Pham <dien.pham.ry@xxxxxxxxxxx>
>
> 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@xxxxxxxxxxx>
> Signed-off-by: Dien Pham <dien.pham.ry@xxxxxxxxxxx>
> [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@xxxxxxxxxxx>
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@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
