Re: [PATCH] iio: imu: inv_icm42600: fix 64bit multiplication

[Antonio Quartulli <antonio@mandelbit.com>]

On 16/07/2025 21:52, David Lechner wrote:
> On 7/16/25 2:44 PM, Antonio Quartulli wrote:
>> On 16/07/2025 21:36, Andy Shevchenko wrote:
>>> On Wed, Jul 16, 2025 at 10:22 PM Antonio Quartulli
>>> <antonio@mandelbit.com> wrote:
>>>>
>>>> In inv_icm42600_accel_convert_wom_to_roc() multiplying
>>>> `threshold` by `convert` may result in a number requiring more
>>>> than 32bit.
>>>> In this case, although `value` is 64bit wide, the result is
>>>> truncated because the multiplication is performed in the
>>>> 32bit domain, due to both operands being 32bit long.
>>>>
>>>> Cast the first operand to u64 to ensure the multiplication is
>>>> performed in the expected domain.
>>>
>>> Is this a theoretical or practical issue?
>>
>> Can't say if it's practical because I don't know how large `threshold` can be.
> 
> 
> The code is clear that it is between 0 and 255 inclusive.
> 
> unsigned int inv_icm42600_accel_convert_roc_to_wom(...)
> {
> 	...
> 	if (roc == 0)
> 		return 0;
> 	...
> 	return clamp(value, 1, 255);
> }
> 
> threshold = inv_icm42600_accel_convert_roc_to_wom(value, accel_hz, accel_uhz);
> value = inv_icm42600_accel_convert_wom_to_roc(threshold, accel_hz, accel_uhz)

Thanks for pointing this out - I overlooked that.

> 
> So I would not call this a fix, just making the code more robust against
> any changes in the future.

It's the type being used that convinced me something could be off.
Maybe threshold could just be u8 and value be u32? (just to improve self 
documentation)
Also inv_icm42600_accel_convert_roc_to_wom() could return u8 for 
consistency.

I any case, this is a bit code cleanup and a bit bike shedding :-)
I agree there is nothing to "fix" at this point.

Regards,


-- 
Antonio Quartulli

CEO and Co-Founder
Mandelbit Srl
https://www.mandelbit.com