Re: [PATCH] iio: gts-helpers: Round gains and scales

From: Matti Vaittinen <mazziesaccount@gmail.com>
To: Jonathan Cameron <jic23@kernel.org>
Cc: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>,
	Lars-Peter Clausen <lars@metafoo.de>,
	Subhajit Ghosh <subhajit.ghosh@tweaklogic.com>,
	linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] iio: gts-helpers: Round gains and scales
Date: Tue, 28 Nov 2023 15:16:23 +0200	[thread overview]
Message-ID: <fabd0693-34c3-49c1-8680-17f6f8b9453a@gmail.com> (raw)
In-Reply-To: <1fe658cd-bdd5-4afd-8564-d0dcf9eab4ab@gmail.com>

On 11/28/23 13:56, Matti Vaittinen wrote:
> On 11/27/23 09:48, Matti Vaittinen wrote:
>> On 11/26/23 19:26, Jonathan Cameron wrote:
>>> On Tue, 31 Oct 2023 11:50:46 +0200
>>> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
>>>
>>>> The GTS helpers do flooring of scale when calculating available scales.
>>>> This results available-scales to be reported smaller than they should
>>>> when the division in scale computation resulted remainder greater than
>>>> half of the divider. (decimal part of result > 0.5)
>>>>
>>>> Furthermore, when gains are computed based on scale, the gain resulting
>>>> from the scale computation is also floored. As a consequence the
>>>> floored scales reported by available scales may not match the gains 
>>>> that
>>>> can be set.
>>>>
>>>> The related discussion can be found from:
>>>> https://lore.kernel.org/all/84d7c283-e8e5-4c98-835c-fe3f6ff94f4b@gmail.com/
>>>>
>>>> Do rounding when computing scales and gains.
>>>>
>>>> Fixes: 38416c28e168 ("iio: light: Add gain-time-scale helpers")
>>>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
>>>

...

>>>> +    if ((u64)scale32 == scale)
>>>> +        return iio_gts_get_gain_32(full, scale32);
>>>> +
>>>>       if (U64_MAX - full < scale) {
>>>>           /* Risk of overflow */
>>>> -        if (full - scale < scale)
>>>> +        if (full - scale / 2 < scale)
>>>>               return 1;
>>>>           full -= scale;
>>>>           tmp++;
>>>>       }
>>>> -    while (full > scale * (u64)tmp)
>>>> +    half_div = scale >> 2;
>>>
>>> Why divide by 4?  Looks like classic issue with using shifts for 
>>> division
>>> causing confusion.
>>
>> Yes. Looks like a brainfart to me. I need to fire-up my tests and 
>> revise this (and the check you asked about above). It seems to take a 
>> while from me to wrap my head around this again...
>>
>> Thanks for pointing this out!
>>
>>>
>>>> +
>>>> +    while (full + half_div >= scale * (u64)tmp)
>>>>           tmp++;
> 
> Oh. This is a problem. Adding half_div to full here can cause the scale 
> * (u64)tmp to overflow. The overflow-prevention above only ensures full 
> is smaller than the U64_MAX - scale. Here we should ensure full + 
> half_div is less than U64_MAX - scale to ensure the loop always stops.
> 
> All in all, this is horrible. Just ran a quick and dirty test on my 
> laptop, and using 0xFFFF FFFF FFFF FFFF as full and 0x1 0000 0000 as 
> scale (without the half_div addition) ran this loop for several seconds.
> 
> Sigh. My brains jammed. I know this can not be an unique problem. I am 
> sure there exists a better solution somewhere - any pointers would be 
> appreciated :)
> 

And as a reply to myself - is there something wrong with using the 
div64_u64()? Sorry for the noise...

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~