| Message ID | 20210706160942.3181474-5-liambeguin@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Jonathan Cameron |
| Headers | show |
| Series | iio: afe: add temperature rescaling support | expand |
On 2021-07-06 18:09, Liam Beguin wrote: > From: Liam Beguin <lvb@xiphos.com> > > Reduce the risk of integer overflow by doing the scale calculation with > 64bit integers and looking for a Greatest Common Divider for both parts > of the fractional value. > > Signed-off-by: Liam Beguin <lvb@xiphos.com> > --- > drivers/iio/afe/iio-rescale.c | 12 +++++++++--- > 1 file changed, 9 insertions(+), 3 deletions(-) > > diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c > index 774eb3044edd..ba3bdcc69b16 100644 > --- a/drivers/iio/afe/iio-rescale.c > +++ b/drivers/iio/afe/iio-rescale.c > @@ -39,7 +39,8 @@ static int rescale_read_raw(struct iio_dev *indio_dev, > int *val, int *val2, long mask) > { > struct rescale *rescale = iio_priv(indio_dev); > - unsigned long long tmp; > + s64 tmp, tmp2; > + u32 factor; > int ret; > > switch (mask) { > @@ -67,8 +68,13 @@ static int rescale_read_raw(struct iio_dev *indio_dev, > } > switch (ret) { > case IIO_VAL_FRACTIONAL: > - *val *= rescale->numerator; > - *val2 *= rescale->denominator; > + tmp = (s64)*val * rescale->numerator; > + tmp2 = (s64)*val2 * rescale->denominator; > + factor = gcd(tmp, tmp2); Hi! gcd() isn't exactly free. I do not think it is suitable to call it for each and every value. So, if you really need it, then it should only be used when there is an actual overflow (or if there is a high risk if that's somehow easier). Cheers, Peter > + do_div(tmp, factor); > + *val = tmp; > + do_div(tmp2, factor); > + *val2 = tmp2; > return ret; > case IIO_VAL_INT: > *val *= rescale->numerator; >
On Fri Jul 9, 2021 at 12:24 PM EDT, Peter Rosin wrote: > > On 2021-07-06 18:09, Liam Beguin wrote: > > From: Liam Beguin <lvb@xiphos.com> > > > > Reduce the risk of integer overflow by doing the scale calculation with > > 64bit integers and looking for a Greatest Common Divider for both parts > > of the fractional value. > > > > Signed-off-by: Liam Beguin <lvb@xiphos.com> > > --- > > drivers/iio/afe/iio-rescale.c | 12 +++++++++--- > > 1 file changed, 9 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c > > index 774eb3044edd..ba3bdcc69b16 100644 > > --- a/drivers/iio/afe/iio-rescale.c > > +++ b/drivers/iio/afe/iio-rescale.c > > @@ -39,7 +39,8 @@ static int rescale_read_raw(struct iio_dev *indio_dev, > > int *val, int *val2, long mask) > > { > > struct rescale *rescale = iio_priv(indio_dev); > > - unsigned long long tmp; > > + s64 tmp, tmp2; > > + u32 factor; > > int ret; > > > > switch (mask) { > > @@ -67,8 +68,13 @@ static int rescale_read_raw(struct iio_dev *indio_dev, > > } > > switch (ret) { > > case IIO_VAL_FRACTIONAL: > > - *val *= rescale->numerator; > > - *val2 *= rescale->denominator; > > + tmp = (s64)*val * rescale->numerator; > > + tmp2 = (s64)*val2 * rescale->denominator; > > + factor = gcd(tmp, tmp2); Hi Peter, > > Hi! > > gcd() isn't exactly free. I do not think it is suitable to call it for > each > and every value. So, if you really need it, then it should only be used > when there is an actual overflow (or if there is a high risk if that's > somehow easier). Understood, digging into this a little bit, it seems like check_mul_overflow() could be used here. I'll give it a try and will look at implementing Jonathan's suggestion in case we're dealing with a case where gcd() returns 1. Thanks, Liam > > Cheers, > Peter > > > + do_div(tmp, factor); > > + *val = tmp; > > + do_div(tmp2, factor); > > + *val2 = tmp2; > > return ret; > > case IIO_VAL_INT: > > *val *= rescale->numerator; > >
diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c index 774eb3044edd..ba3bdcc69b16 100644 --- a/drivers/iio/afe/iio-rescale.c +++ b/drivers/iio/afe/iio-rescale.c @@ -39,7 +39,8 @@ static int rescale_read_raw(struct iio_dev *indio_dev, int *val, int *val2, long mask) { struct rescale *rescale = iio_priv(indio_dev); - unsigned long long tmp; + s64 tmp, tmp2; + u32 factor; int ret; switch (mask) { @@ -67,8 +68,13 @@ static int rescale_read_raw(struct iio_dev *indio_dev, } switch (ret) { case IIO_VAL_FRACTIONAL: - *val *= rescale->numerator; - *val2 *= rescale->denominator; + tmp = (s64)*val * rescale->numerator; + tmp2 = (s64)*val2 * rescale->denominator; + factor = gcd(tmp, tmp2); + do_div(tmp, factor); + *val = tmp; + do_div(tmp2, factor); + *val2 = tmp2; return ret; case IIO_VAL_INT: *val *= rescale->numerator;