| Message ID | 1593053768-31016-1-git-send-email-yoshihiro.shimoda.uh@renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | [v3] thermal: rcar_gen3_thermal: Fix undefined temperature if negative | expand |
On Thu, Jun 25, 2020 at 8:26 AM 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. Note that the variable name "reg" is not good > because it should be the same type as rcar_gen3_thermal_read(). > However, there is better to rename it in a further patch as > cleanup. > > 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> > Reviewed-by: Niklas Soderlund <niklas.soderlund+renesas@ragnatech.se> > Tested-by: Niklas Soderlund <niklas.soderlund+renesas@ragnatech.se> Reviewed-by: Amit Kucheria <amit.kucheria@linaro.org> > --- > Changes from v2: > - Add Niklas-san's Reviewed-by and Tested-by. > - Keep the variable name "reg" and revise the commit description for it. > https://patchwork.kernel.org/patch/11595327/ > > Changes from v1: > - Use int instead of long. > - Rename "reg" with "ctemp". > https://patchwork.kernel.org/patch/11593051/ > > drivers/thermal/rcar_gen3_thermal.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c > index 58fe7c1..c48c5e9 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; > + int reg; > > /* Read register and convert to mili Celsius */ > reg = rcar_gen3_thermal_read(tsc, REG_GEN3_TEMP) & CTEMP_MASK; > -- > 2.7.4 >
Hello! On 25.06.2020 5:56, Yoshihiro Shimoda 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. Note that the variable name "reg" is not good > because it should be the same type as rcar_gen3_thermal_read(). > However, there is better to rename it in a further patch as It's better. > cleanup. > > 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> > Reviewed-by: Niklas Soderlund <niklas.soderlund+renesas@ragnatech.se> > Tested-by: Niklas Soderlund <niklas.soderlund+renesas@ragnatech.se> [...] MBR, Sergei
Hello! > From: Sergei Shtylyov, Sent: Thursday, June 25, 2020 5:58 PM > > Hello! > > On 25.06.2020 5:56, Yoshihiro Shimoda 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. Note that the variable name "reg" is not good > > because it should be the same type as rcar_gen3_thermal_read(). > > However, there is better to rename it in a further patch as > > It's better. Thank you for your review! I'll fix it. Best regards, Yoshihiro Shimoda
On 06/25/2020 02:34 PM, Yoshihiro Shimoda 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. Note that the variable name "reg" is not good >>> because it should be the same type as rcar_gen3_thermal_read(). >>> However, there is better to rename it in a further patch as >> >> It's better. > > Thank you for your review! I'll fix it. Note that "there" isn't needed there, I wasn't explicit enough, it seems... > Best regards, > Yoshihiro Shimoda MBR, Sergei
diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c index 58fe7c1..c48c5e9 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; + int reg; /* Read register and convert to mili Celsius */ reg = rcar_gen3_thermal_read(tsc, REG_GEN3_TEMP) & CTEMP_MASK;