| Message ID | 20241126234021.19749-1-jahau@rocketmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | iio: magnetometer: yas530: Use signed integer type for clamp limits | expand |
From: Jakob Hauser <jahau@rocketmail.com> > Sent: 26 November 2024 23:40 > > In the function yas537_measure() there is a clamp_val() with limits of > -BIT(13) and BIT(13) - 1. The input clamp value h[] is of type s32. The BIT() > is of type unsigned long integer due to its define in include/vdso/bits.h. > The lower limit -BIT(13) is recognized as -8192 but expressed as an unsigned > long integer. The size of an unsigned long integer differs between 32-bit and > 64-bit architectures. Converting this to type s32 may lead to undesired > behavior. I think you also need to say that the unsigned divide generates erronous values on 32bit systems and that the clamp() call result is ignored. > > Declaring a signed integer with a value of BIT(13) allows to use it more > specifically as a negative value on the lower clamp limit. > > While at it, replace all BIT(13) in the function yas537_measure() by the signed > integer. > > Reported-by: kernel test robot <lkp@intel.com> > Closes: https://lore.kernel.org/oe-kbuild-all/202411230458.dhZwh3TT-lkp@intel.com/ > Fixes: 65f79b501030 ("iio: magnetometer: yas530: Add YAS537 variant") > Cc: David Laight <david.laight@aculab.com> > Signed-off-by: Jakob Hauser <jahau@rocketmail.com> > --- > The patch is based on torvalds/linux v6.12. > > The calculation lines h[0], h[1] and h[2] exceed the limit of 80 characters per > line. In terms of readability I would prefer to keep it that way. > --- > drivers/iio/magnetometer/yamaha-yas530.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/drivers/iio/magnetometer/yamaha-yas530.c b/drivers/iio/magnetometer/yamaha-yas530.c > index 65011a8598d3..938b35536e0d 100644 > --- a/drivers/iio/magnetometer/yamaha-yas530.c > +++ b/drivers/iio/magnetometer/yamaha-yas530.c > @@ -372,6 +372,7 @@ static int yas537_measure(struct yas5xx *yas5xx, u16 *t, u16 *x, u16 *y1, u16 *y > u8 data[8]; > u16 xy1y2[3]; > s32 h[3], s[3]; > + int half_range = BIT(13); > int i, ret; > > mutex_lock(&yas5xx->lock); > @@ -406,13 +407,13 @@ static int yas537_measure(struct yas5xx *yas5xx, u16 *t, u16 *x, u16 *y1, u16 *y > /* The second version of YAS537 needs to include calibration coefficients */ > if (yas5xx->version == YAS537_VERSION_1) { > for (i = 0; i < 3; i++) > - s[i] = xy1y2[i] - BIT(13); > - h[0] = (c->k * (128 * s[0] + c->a2 * s[1] + c->a3 * s[2])) / BIT(13); > - h[1] = (c->k * (c->a4 * s[0] + c->a5 * s[1] + c->a6 * s[2])) / BIT(13); > - h[2] = (c->k * (c->a7 * s[0] + c->a8 * s[1] + c->a9 * s[2])) / BIT(13); > + s[i] = xy1y2[i] - half_range; > + h[0] = (c->k * (128 * s[0] + c->a2 * s[1] + c->a3 * s[2])) / half_range; > + h[1] = (c->k * (c->a4 * s[0] + c->a5 * s[1] + c->a6 * s[2])) / half_range; > + h[2] = (c->k * (c->a7 * s[0] + c->a8 * s[1] + c->a9 * s[2])) / half_range; > for (i = 0; i < 3; i++) { > - clamp_val(h[i], -BIT(13), BIT(13) - 1); > - xy1y2[i] = h[i] + BIT(13); > + clamp_val(h[i], -half_range, half_range - 1); > + xy1y2[i] = h[i] + half_range; NAK - that still ignores the result of clamp. and it should be clamp() not clamp_val(). David > } > } > > -- > 2.43.0 - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Hi David, On 27.11.24 09:53, David Laight wrote: > From: Jakob Hauser <jahau@rocketmail.com> >> Sent: 26 November 2024 23:40 >> >> In the function yas537_measure() there is a clamp_val() with limits of >> -BIT(13) and BIT(13) - 1. The input clamp value h[] is of type s32. The BIT() >> is of type unsigned long integer due to its define in include/vdso/bits.h. >> The lower limit -BIT(13) is recognized as -8192 but expressed as an unsigned >> long integer. The size of an unsigned long integer differs between 32-bit and >> 64-bit architectures. Converting this to type s32 may lead to undesired >> behavior. > > I think you also need to say that the unsigned divide generates erronous > values on 32bit systems and that the clamp() call result is ignored. Ok, I'll expand the commit message in v2. >> Declaring a signed integer with a value of BIT(13) allows to use it more >> specifically as a negative value on the lower clamp limit. >> >> While at it, replace all BIT(13) in the function yas537_measure() by the signed >> integer. >> >> Reported-by: kernel test robot <lkp@intel.com> >> Closes: https://lore.kernel.org/oe-kbuild-all/202411230458.dhZwh3TT-lkp@intel.com/ >> Fixes: 65f79b501030 ("iio: magnetometer: yas530: Add YAS537 variant") >> Cc: David Laight <david.laight@aculab.com> >> Signed-off-by: Jakob Hauser <jahau@rocketmail.com> >> --- >> The patch is based on torvalds/linux v6.12. >> >> The calculation lines h[0], h[1] and h[2] exceed the limit of 80 characters per >> line. In terms of readability I would prefer to keep it that way. >> --- >> drivers/iio/magnetometer/yamaha-yas530.c | 13 +++++++------ >> 1 file changed, 7 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/iio/magnetometer/yamaha-yas530.c b/drivers/iio/magnetometer/yamaha-yas530.c >> index 65011a8598d3..938b35536e0d 100644 >> --- a/drivers/iio/magnetometer/yamaha-yas530.c >> +++ b/drivers/iio/magnetometer/yamaha-yas530.c >> @@ -372,6 +372,7 @@ static int yas537_measure(struct yas5xx *yas5xx, u16 *t, u16 *x, u16 *y1, u16 *y >> u8 data[8]; >> u16 xy1y2[3]; >> s32 h[3], s[3]; >> + int half_range = BIT(13); >> int i, ret; >> >> mutex_lock(&yas5xx->lock); >> @@ -406,13 +407,13 @@ static int yas537_measure(struct yas5xx *yas5xx, u16 *t, u16 *x, u16 *y1, u16 *y >> /* The second version of YAS537 needs to include calibration coefficients */ >> if (yas5xx->version == YAS537_VERSION_1) { >> for (i = 0; i < 3; i++) >> - s[i] = xy1y2[i] - BIT(13); >> - h[0] = (c->k * (128 * s[0] + c->a2 * s[1] + c->a3 * s[2])) / BIT(13); >> - h[1] = (c->k * (c->a4 * s[0] + c->a5 * s[1] + c->a6 * s[2])) / BIT(13); >> - h[2] = (c->k * (c->a7 * s[0] + c->a8 * s[1] + c->a9 * s[2])) / BIT(13); >> + s[i] = xy1y2[i] - half_range; >> + h[0] = (c->k * (128 * s[0] + c->a2 * s[1] + c->a3 * s[2])) / half_range; >> + h[1] = (c->k * (c->a4 * s[0] + c->a5 * s[1] + c->a6 * s[2])) / half_range; >> + h[2] = (c->k * (c->a7 * s[0] + c->a8 * s[1] + c->a9 * s[2])) / half_range; >> for (i = 0; i < 3; i++) { >> - clamp_val(h[i], -BIT(13), BIT(13) - 1); >> - xy1y2[i] = h[i] + BIT(13); >> + clamp_val(h[i], -half_range, half_range - 1); >> + xy1y2[i] = h[i] + half_range; > > NAK - that still ignores the result of clamp. Ah, I didn't get that point! Now I realize that clamp_val() returns a value and it's going nowhere here. I'll change that in v2. > and it should be clamp() not clamp_val(). I assumed that clamp_val() is still needed because according to its description in current mainline (6.12) include/linux/minmax.h, clamp() does "strict typechecking". The input value h[] is of type s32 and the limits derived from "half_range" are of type int. I had a try compiling with clamp() and didn't get any warnings or errors. Does that mean that clamp() isn't that strict in the current implementation (and considering the patch being backported)? Does it just check signedness and is this because in current __clamp_once() it uses __auto_type? Kind regards, Jakob
From: Jakob Hauser > Sent: 29 November 2024 00:20 ... > > and it should be clamp() not clamp_val(). > > I assumed that clamp_val() is still needed because according to its > description in current mainline (6.12) include/linux/minmax.h, clamp() > does "strict typechecking". The input value h[] is of type s32 and the > limits derived from "half_range" are of type int. I had a try compiling > with clamp() and didn't get any warnings or errors. Does that mean that > clamp() isn't that strict in the current implementation (and considering > the patch being backported)? Does it just check signedness and is this > because in current __clamp_once() it uses __auto_type? The current mainline contains relaxed checks - the comment is wrong. In any case, after the change, they ate all 'int'. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
diff --git a/drivers/iio/magnetometer/yamaha-yas530.c b/drivers/iio/magnetometer/yamaha-yas530.c index 65011a8598d3..938b35536e0d 100644 --- a/drivers/iio/magnetometer/yamaha-yas530.c +++ b/drivers/iio/magnetometer/yamaha-yas530.c @@ -372,6 +372,7 @@ static int yas537_measure(struct yas5xx *yas5xx, u16 *t, u16 *x, u16 *y1, u16 *y u8 data[8]; u16 xy1y2[3]; s32 h[3], s[3]; + int half_range = BIT(13); int i, ret; mutex_lock(&yas5xx->lock); @@ -406,13 +407,13 @@ static int yas537_measure(struct yas5xx *yas5xx, u16 *t, u16 *x, u16 *y1, u16 *y /* The second version of YAS537 needs to include calibration coefficients */ if (yas5xx->version == YAS537_VERSION_1) { for (i = 0; i < 3; i++) - s[i] = xy1y2[i] - BIT(13); - h[0] = (c->k * (128 * s[0] + c->a2 * s[1] + c->a3 * s[2])) / BIT(13); - h[1] = (c->k * (c->a4 * s[0] + c->a5 * s[1] + c->a6 * s[2])) / BIT(13); - h[2] = (c->k * (c->a7 * s[0] + c->a8 * s[1] + c->a9 * s[2])) / BIT(13); + s[i] = xy1y2[i] - half_range; + h[0] = (c->k * (128 * s[0] + c->a2 * s[1] + c->a3 * s[2])) / half_range; + h[1] = (c->k * (c->a4 * s[0] + c->a5 * s[1] + c->a6 * s[2])) / half_range; + h[2] = (c->k * (c->a7 * s[0] + c->a8 * s[1] + c->a9 * s[2])) / half_range; for (i = 0; i < 3; i++) { - clamp_val(h[i], -BIT(13), BIT(13) - 1); - xy1y2[i] = h[i] + BIT(13); + clamp_val(h[i], -half_range, half_range - 1); + xy1y2[i] = h[i] + half_range; } }
In the function yas537_measure() there is a clamp_val() with limits of -BIT(13) and BIT(13) - 1. The input clamp value h[] is of type s32. The BIT() is of type unsigned long integer due to its define in include/vdso/bits.h. The lower limit -BIT(13) is recognized as -8192 but expressed as an unsigned long integer. The size of an unsigned long integer differs between 32-bit and 64-bit architectures. Converting this to type s32 may lead to undesired behavior. Declaring a signed integer with a value of BIT(13) allows to use it more specifically as a negative value on the lower clamp limit. While at it, replace all BIT(13) in the function yas537_measure() by the signed integer. Reported-by: kernel test robot <lkp@intel.com> Closes: https://lore.kernel.org/oe-kbuild-all/202411230458.dhZwh3TT-lkp@intel.com/ Fixes: 65f79b501030 ("iio: magnetometer: yas530: Add YAS537 variant") Cc: David Laight <david.laight@aculab.com> Signed-off-by: Jakob Hauser <jahau@rocketmail.com> --- The patch is based on torvalds/linux v6.12. The calculation lines h[0], h[1] and h[2] exceed the limit of 80 characters per line. In terms of readability I would prefer to keep it that way. --- drivers/iio/magnetometer/yamaha-yas530.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)