Re: [PATCH] iio: magnetometer: yas530: Use signed integer type for clamp limits

[Jakob Hauser <jahau@rocketmail.com>]

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