| Message ID | 20220906083356.21067-6-farbere@amazon.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Variety of fixes and new features for mr75203 driver | expand |
On Tue, Sep 06, 2022 at 08:33:40AM +0000, Eliav Farber wrote: > According to Moortec Embedded Voltage Monitor (MEVM) series 3 data > sheet, the minimum input signal is -100mv and maximum input signal > is +1000mv. > > The equation used to convert the digital word to voltage uses mixed > types (*val signed and n unsigned), and on 64 bit machines also has > different size, since sizeof(u32) = 4 and sizeof(long) = 8. > > So when measuring a negative input, n will be small enough, such that > PVT_N_CONST * n < PVT_R_CONST, and the result of > (PVT_N_CONST * n - PVT_R_CONST) will overflow to a very big positive > 32 bit number. Then when storing the result in *val it will be the same > value just in 64 bit (instead of it representing a negative number which > will what happen when sizeof(long) = 4). > > When -1023 <= (PVT_N_CONST * n - PVT_R_CONST) <= -1 > dividing the number by 1024 should result of in 0, but because ">> 10" > is used it results in -1 (0xf...fffff). > > This change fixes the sign problem and supports negative values by > casting n to long and replacing the shift right with div operation. This is really downside of C... ... > - *val = (PVT_N_CONST * n - PVT_R_CONST) >> PVT_CONV_BITS; > + *val = (PVT_N_CONST * (long)n - PVT_R_CONST) / (1 << PVT_CONV_BITS); Wondering if we can use BIT(PVT_CONV_BITS) for two (quite unlikely to happen, I hope) purposes: 1) Somebody copies such code where PVT_CONV_BITS analogue can be 31, which is according to C standard is UB (undefined behaviour). 2) It makes shorter the line and also drops the pattern where some dumb robot may propose a patch to basically revert the division change.
On 9/6/2022 3:03 PM, Andy Shevchenko wrote: > On Tue, Sep 06, 2022 at 08:33:40AM +0000, Eliav Farber wrote: >> According to Moortec Embedded Voltage Monitor (MEVM) series 3 data >> sheet, the minimum input signal is -100mv and maximum input signal >> is +1000mv. >> >> The equation used to convert the digital word to voltage uses mixed >> types (*val signed and n unsigned), and on 64 bit machines also has >> different size, since sizeof(u32) = 4 and sizeof(long) = 8. >> >> So when measuring a negative input, n will be small enough, such that >> PVT_N_CONST * n < PVT_R_CONST, and the result of >> (PVT_N_CONST * n - PVT_R_CONST) will overflow to a very big positive >> 32 bit number. Then when storing the result in *val it will be the same >> value just in 64 bit (instead of it representing a negative number which >> will what happen when sizeof(long) = 4). >> >> When -1023 <= (PVT_N_CONST * n - PVT_R_CONST) <= -1 >> dividing the number by 1024 should result of in 0, but because ">> 10" >> is used it results in -1 (0xf...fffff). >> >> This change fixes the sign problem and supports negative values by >> casting n to long and replacing the shift right with div operation. > > This is really downside of C... > > ... > >> - *val = (PVT_N_CONST * n - PVT_R_CONST) >> PVT_CONV_BITS; >> + *val = (PVT_N_CONST * (long)n - PVT_R_CONST) / (1 << >> PVT_CONV_BITS); > > Wondering if we can use BIT(PVT_CONV_BITS) for two (quite unlikely to > happen, > I hope) purposes: > > 1) Somebody copies such code where PVT_CONV_BITS analogue can be 31, > which is according to C standard is UB (undefined behaviour). > > 2) It makes shorter the line and also drops the pattern where some > dumb robot may propose a patch to basically revert the division > change. I originally tried to use BIT(PVT_CONV_BITS) but it gave a different result. e.g. If n = 2720 *val = (PVT_N_CONST * (long)n - PVT_R_CONST) / (1 << PVT_CONV_BITS) = 0 *val = (PVT_N_CONST * (long)n - PVT_R_CONST) / BIT(PVT_CONV_BITS) = 18014398509481983 I can try fitting it in one line, either by adding a define for (1 << PVT_CONV_BITS) or exceeding 80 characters, but keep in mind that in a later patch (#15) it gets even longer (and I must use more than one line) since it is multiplied by a pre-scaler factor. -- Regards, Eliav
On Tue, Sep 06, 2022 at 04:27:13PM +0300, Farber, Eliav wrote: > On 9/6/2022 3:03 PM, Andy Shevchenko wrote: > > On Tue, Sep 06, 2022 at 08:33:40AM +0000, Eliav Farber wrote: ... > > > - *val = (PVT_N_CONST * n - PVT_R_CONST) >> PVT_CONV_BITS; > > > + *val = (PVT_N_CONST * (long)n - PVT_R_CONST) / (1 << > > > PVT_CONV_BITS); > > > > Wondering if we can use BIT(PVT_CONV_BITS) for two (quite unlikely to > > happen, > > I hope) purposes: > > > > 1) Somebody copies such code where PVT_CONV_BITS analogue can be 31, > > which is according to C standard is UB (undefined behaviour). > > > > 2) It makes shorter the line and also drops the pattern where some > > dumb robot may propose a patch to basically revert the division > > change. > I originally tried to use BIT(PVT_CONV_BITS) but it gave a different > result. > e.g. > If n = 2720 > *val = (PVT_N_CONST * (long)n - PVT_R_CONST) / (1 << PVT_CONV_BITS) = 0 > *val = (PVT_N_CONST * (long)n - PVT_R_CONST) / BIT(PVT_CONV_BITS) = > 18014398509481983 > > I can try fitting it in one line, either by adding a define for > (1 << PVT_CONV_BITS) or exceeding 80 characters, but keep in mind that > in a later patch (#15) it gets even longer (and I must use more than > one line) since it is multiplied by a pre-scaler factor. Don't get me wrong, it's not about style, it's about preventing followup "fixes" of this. All the problems here due to (hidden) unsigned type(s). What you can do is to add a good comment on top of that line explaining why division instead of right shift and why BIT() may not be used (because it's unsigned).
diff --git a/drivers/hwmon/mr75203.c b/drivers/hwmon/mr75203.c index a209f5d95f4b..78dc471e843c 100644 --- a/drivers/hwmon/mr75203.c +++ b/drivers/hwmon/mr75203.c @@ -202,7 +202,7 @@ static int pvt_read_in(struct device *dev, u32 attr, int channel, long *val) n &= SAMPLE_DATA_MSK; /* Convert the N bitstream count into voltage */ - *val = (PVT_N_CONST * n - PVT_R_CONST) >> PVT_CONV_BITS; + *val = (PVT_N_CONST * (long)n - PVT_R_CONST) / (1 << PVT_CONV_BITS); return 0; default:
According to Moortec Embedded Voltage Monitor (MEVM) series 3 data sheet, the minimum input signal is -100mv and maximum input signal is +1000mv. The equation used to convert the digital word to voltage uses mixed types (*val signed and n unsigned), and on 64 bit machines also has different size, since sizeof(u32) = 4 and sizeof(long) = 8. So when measuring a negative input, n will be small enough, such that PVT_N_CONST * n < PVT_R_CONST, and the result of (PVT_N_CONST * n - PVT_R_CONST) will overflow to a very big positive 32 bit number. Then when storing the result in *val it will be the same value just in 64 bit (instead of it representing a negative number which will what happen when sizeof(long) = 4). When -1023 <= (PVT_N_CONST * n - PVT_R_CONST) <= -1 dividing the number by 1024 should result of in 0, but because ">> 10" is used it results in -1 (0xf...fffff). This change fixes the sign problem and supports negative values by casting n to long and replacing the shift right with div operation. Fixes: 9d823351a337 ("hwmon: Add hardware monitoring driver for Moortec MR75203 PVT controller") Signed-off-by: Eliav Farber <farbere@amazon.com> --- V4 -> V3: - Remove unrelated change (add of empty line). V3 -> V2: - Fix equation to support negative values instead of limiting value to zero. drivers/hwmon/mr75203.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)