Hi David,
On 27.11.24 09:53, David Laight wrote:
From: Jakob Hauser <jahau@xxxxxxxxxxxxxx>
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@xxxxxxxxx>
Closes: https://lore.kernel.org/oe-kbuild-all/202411230458.dhZwh3TT-lkp@xxxxxxxxx/
Fixes: 65f79b501030 ("iio: magnetometer: yas530: Add YAS537 variant")
Cc: David Laight <david.laight@xxxxxxxxxx>
Signed-off-by: Jakob Hauser <jahau@xxxxxxxxxxxxxx>
---
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
