| Message ID | 20211222034646.222189-10-liambeguin@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: afe: add temperature rescaling support | expand |
On Wed, Dec 22, 2021 at 5:47 AM Liam Beguin <liambeguin@gmail.com> wrote: > > From: Liam Beguin <lvb@xiphos.com> > > Reduce the risk of integer overflow by doing the scale calculation on > a 64-bit integer. Since the rescaling is only performed on *val, reuse > the IIO_VAL_FRACTIONAL_LOG2 case. ... > - tmp = 1 << *val2; At some point this should be BIT() Rule of thumb (in accordance with C standard), always use unsigned value as left operand of the _left_ shift. > + if (scale_type == IIO_VAL_FRACTIONAL) > + tmp = *val2; > + else > + tmp = 1 << *val2;
On Wed, Dec 22, 2021 at 02:29:04PM +0200, Andy Shevchenko wrote: > On Wed, Dec 22, 2021 at 5:47 AM Liam Beguin <liambeguin@gmail.com> wrote: > > > > From: Liam Beguin <lvb@xiphos.com> > > > > Reduce the risk of integer overflow by doing the scale calculation on > > a 64-bit integer. Since the rescaling is only performed on *val, reuse > > the IIO_VAL_FRACTIONAL_LOG2 case. > > ... > > > - tmp = 1 << *val2; > > At some point this should be BIT() I'm not against changing this, but (to me at least) 1 << *val2 seems more explicit as we're not working with bitfields. No? > Rule of thumb (in accordance with C standard), always use unsigned > value as left operand of the _left_ shift. Right, that makes sense! In practice though, since we'll most likely never use higher bits of *val2 with IIO_VAL_FRACTIONAL_LOG2, would it be enough to simply typecast? tmp = 1 << (unsigned int)*val2; Cheers, Liam > > + if (scale_type == IIO_VAL_FRACTIONAL) > > + tmp = *val2; > > + else > > + tmp = 1 << *val2; > > > -- > With Best Regards, > Andy Shevchenko
On Wed, Dec 22, 2021 at 8:38 PM Liam Beguin <liambeguin@gmail.com> wrote: > On Wed, Dec 22, 2021 at 02:29:04PM +0200, Andy Shevchenko wrote: > > On Wed, Dec 22, 2021 at 5:47 AM Liam Beguin <liambeguin@gmail.com> wrote: ... > > > - tmp = 1 << *val2; > > > > At some point this should be BIT() Forgot to add, If it's 64-bit, then BIT_ULL(). > I'm not against changing this, but (to me at least) 1 << *val2 seems > more explicit as we're not working with bitfields. No? You may add a comment. You may use int_pow(), but it will be suboptimal. > > Rule of thumb (in accordance with C standard), always use unsigned > > value as left operand of the _left_ shift. > > Right, that makes sense! In practice though, since we'll most likely > never use higher bits of *val2 with IIO_VAL_FRACTIONAL_LOG2, would it be > enough to simply typecast? > > tmp = 1 << (unsigned int)*val2; No, it's about the _left_ operand. I haven't checked if tmp is 64-bit, then even that would be still wrong.
On Wed, Dec 22, 2021 at 08:56:12PM +0200, Andy Shevchenko wrote: > On Wed, Dec 22, 2021 at 8:38 PM Liam Beguin <liambeguin@gmail.com> wrote: > > On Wed, Dec 22, 2021 at 02:29:04PM +0200, Andy Shevchenko wrote: > > > On Wed, Dec 22, 2021 at 5:47 AM Liam Beguin <liambeguin@gmail.com> wrote: > > ... > > > > > - tmp = 1 << *val2; > > > > > > At some point this should be BIT() > > Forgot to add, If it's 64-bit, then BIT_ULL(). > > > I'm not against changing this, but (to me at least) 1 << *val2 seems > > more explicit as we're not working with bitfields. No? > > You may add a comment. You may use int_pow(), but it will be suboptimal. > > > > Rule of thumb (in accordance with C standard), always use unsigned > > > value as left operand of the _left_ shift. > > > > Right, that makes sense! In practice though, since we'll most likely > > never use higher bits of *val2 with IIO_VAL_FRACTIONAL_LOG2, would it be > > enough to simply typecast? > > > > tmp = 1 << (unsigned int)*val2; > > No, it's about the _left_ operand. > I haven't checked if tmp is 64-bit, then even that would be still wrong. Okay so your recommendation is to not use a left shift? I can look into that but given how unlikely it is to fall into those bad cases, I'd rather keep things as they are. Would that be okay? Also, I don't think using BIT() or BIT_ULL() would address this as they both do the same shift, with no extra checks. Cheers, Liam > -- > With Best Regards, > Andy Shevchenko
On Wed, Dec 22, 2021 at 9:59 PM Liam Beguin <liambeguin@gmail.com> wrote: > On Wed, Dec 22, 2021 at 08:56:12PM +0200, Andy Shevchenko wrote: > > On Wed, Dec 22, 2021 at 8:38 PM Liam Beguin <liambeguin@gmail.com> wrote: > > > On Wed, Dec 22, 2021 at 02:29:04PM +0200, Andy Shevchenko wrote: > > > > On Wed, Dec 22, 2021 at 5:47 AM Liam Beguin <liambeguin@gmail.com> wrote: ... > > > > > - tmp = 1 << *val2; > > > > > > > > At some point this should be BIT() > > > > Forgot to add, If it's 64-bit, then BIT_ULL(). > > > > > I'm not against changing this, but (to me at least) 1 << *val2 seems > > > more explicit as we're not working with bitfields. No? > > > > You may add a comment. You may use int_pow(), but it will be suboptimal. > > > > > > Rule of thumb (in accordance with C standard), always use unsigned > > > > value as left operand of the _left_ shift. > > > > > > Right, that makes sense! In practice though, since we'll most likely > > > never use higher bits of *val2 with IIO_VAL_FRACTIONAL_LOG2, would it be > > > enough to simply typecast? > > > > > > tmp = 1 << (unsigned int)*val2; > > > > No, it's about the _left_ operand. > > I haven't checked if tmp is 64-bit, then even that would be still wrong. > > Okay so your recommendation is to not use a left shift? No, I recommend not to use int type as a _leftside_ operand. BIT() / BIT_ULL() does a left shift anyway. > I can look into that but given how unlikely it is to fall into those bad > cases, I'd rather keep things as they are. Would that be okay? > Also, I don't think using BIT() or BIT_ULL() would address this as they > both do the same shift, with no extra checks. They do slightly different versions of it. They use an unsigned int type. Open coded or not, it's up to you. Just convert to unsigned int.
Hi Andy, On Wed, Dec 22, 2021 at 11:32:24PM +0200, Andy Shevchenko wrote: > On Wed, Dec 22, 2021 at 9:59 PM Liam Beguin <liambeguin@gmail.com> wrote: > > On Wed, Dec 22, 2021 at 08:56:12PM +0200, Andy Shevchenko wrote: > > > On Wed, Dec 22, 2021 at 8:38 PM Liam Beguin <liambeguin@gmail.com> wrote: > > > > On Wed, Dec 22, 2021 at 02:29:04PM +0200, Andy Shevchenko wrote: > > > > > On Wed, Dec 22, 2021 at 5:47 AM Liam Beguin <liambeguin@gmail.com> wrote: > > ... > > > > > > > - tmp = 1 << *val2; > > > > > > > > > > At some point this should be BIT() > > > > > > Forgot to add, If it's 64-bit, then BIT_ULL(). > > > > > > > I'm not against changing this, but (to me at least) 1 << *val2 seems > > > > more explicit as we're not working with bitfields. No? > > > > > > You may add a comment. You may use int_pow(), but it will be suboptimal. > > > > > > > > Rule of thumb (in accordance with C standard), always use unsigned > > > > > value as left operand of the _left_ shift. > > > > > > > > Right, that makes sense! In practice though, since we'll most likely > > > > never use higher bits of *val2 with IIO_VAL_FRACTIONAL_LOG2, would it be > > > > enough to simply typecast? > > > > > > > > tmp = 1 << (unsigned int)*val2; > > > > > > No, it's about the _left_ operand. > > > I haven't checked if tmp is 64-bit, then even that would be still wrong. > > > > Okay so your recommendation is to not use a left shift? > > No, I recommend not to use int type as a _leftside_ operand. > BIT() / BIT_ULL() does a left shift anyway. Oh, got it. Sorry for misreading your message. would something like this be good enough? s64 tmp; u64 tmp2; tmp2 = 1 << *val2; tmp = tmp2; ... How can I validate this? > > I can look into that but given how unlikely it is to fall into those bad > > cases, I'd rather keep things as they are. Would that be okay? > > > Also, I don't think using BIT() or BIT_ULL() would address this as they > > both do the same shift, with no extra checks. > > They do slightly different versions of it. They use an unsigned int type. > > Open coded or not, it's up to you. Just convert to unsigned int. > > -- > With Best Regards, > Andy Shevchenko Cheers, Liam
On Sat, Jan 8, 2022 at 6:34 PM Liam Beguin <liambeguin@gmail.com> wrote: > On Wed, Dec 22, 2021 at 11:32:24PM +0200, Andy Shevchenko wrote: > > On Wed, Dec 22, 2021 at 9:59 PM Liam Beguin <liambeguin@gmail.com> wrote: > > > On Wed, Dec 22, 2021 at 08:56:12PM +0200, Andy Shevchenko wrote: > > > > On Wed, Dec 22, 2021 at 8:38 PM Liam Beguin <liambeguin@gmail.com> wrote: > > > > > On Wed, Dec 22, 2021 at 02:29:04PM +0200, Andy Shevchenko wrote: > > > > > > On Wed, Dec 22, 2021 at 5:47 AM Liam Beguin <liambeguin@gmail.com> wrote: ... > > > > > > > - tmp = 1 << *val2; > > > > > > > > > > > > At some point this should be BIT() > > > > > > > > Forgot to add, If it's 64-bit, then BIT_ULL(). > > > > > > > > > I'm not against changing this, but (to me at least) 1 << *val2 seems > > > > > more explicit as we're not working with bitfields. No? > > > > > > > > You may add a comment. You may use int_pow(), but it will be suboptimal. > > > > > > > > > > Rule of thumb (in accordance with C standard), always use unsigned > > > > > > value as left operand of the _left_ shift. > > > > > > > > > > Right, that makes sense! In practice though, since we'll most likely > > > > > never use higher bits of *val2 with IIO_VAL_FRACTIONAL_LOG2, would it be > > > > > enough to simply typecast? > > > > > > > > > > tmp = 1 << (unsigned int)*val2; > > > > > > > > No, it's about the _left_ operand. > > > > I haven't checked if tmp is 64-bit, then even that would be still wrong. > > > > > > Okay so your recommendation is to not use a left shift? > > > > No, I recommend not to use int type as a _leftside_ operand. > > BIT() / BIT_ULL() does a left shift anyway. > > Oh, got it. Sorry for misreading your message. > would something like this be good enough? > > s64 tmp; > u64 tmp2; > tmp2 = 1 << *val2; This still has a UB according to the C standard. That's why BIT()/BIT_ULL() is preferable to use since they don't have such issues. You may open code it, of course (since I remember you wished to show that this is not a bit, but a number). > tmp = tmp2; > How can I validate this? By understanding the C standard? I dunno, actually. GCC will generate correct code, it's just a special warning you may get when supplying a parameter (Linux kernel doesn't use that one even on W=2 IIRC). -Wshift-overflow=2 > > > I can look into that but given how unlikely it is to fall into those bad > > > cases, I'd rather keep things as they are. Would that be okay? > > > > > Also, I don't think using BIT() or BIT_ULL() would address this as they > > > both do the same shift, with no extra checks. > > > > They do slightly different versions of it. They use an unsigned int type. > > > > Open coded or not, it's up to you. Just convert to unsigned int.
diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c index ca8fd69bfe46..0000a58bab9d 100644 --- a/drivers/iio/afe/iio-rescale.c +++ b/drivers/iio/afe/iio-rescale.c @@ -23,21 +23,31 @@ int rescale_process_scale(struct rescale *rescale, int scale_type, int *val, int *val2) { s64 tmp; + int _val, _val2; s32 rem, rem2; u32 mult; u32 neg; switch (scale_type) { - case IIO_VAL_FRACTIONAL: - *val *= rescale->numerator; - *val2 *= rescale->denominator; - return scale_type; case IIO_VAL_INT: *val *= rescale->numerator; if (rescale->denominator == 1) return scale_type; *val2 = rescale->denominator; return IIO_VAL_FRACTIONAL; + case IIO_VAL_FRACTIONAL: + /* + * When the product of both scales doesn't overflow, avoid + * potential accuracy loss (for in kernel consumers) by + * keeping a fractional representation. + */ + if (!check_mul_overflow(*val, rescale->numerator, &_val) && + !check_mul_overflow(*val2, rescale->denominator, &_val2)) { + *val = _val; + *val2 = _val2; + return IIO_VAL_FRACTIONAL; + } + fallthrough; case IIO_VAL_FRACTIONAL_LOG2: tmp = (s64)*val * 1000000000LL; tmp = div_s64(tmp, rescale->denominator); @@ -49,7 +59,10 @@ int rescale_process_scale(struct rescale *rescale, int scale_type, if (!rem) return scale_type; - tmp = 1 << *val2; + if (scale_type == IIO_VAL_FRACTIONAL) + tmp = *val2; + else + tmp = 1 << *val2; rem2 = *val % (int)tmp; *val = *val / (int)tmp;