Messages in this thread | | | Date | Tue, 28 Nov 2023 15:16:23 +0200 | Subject | Re: [PATCH] iio: gts-helpers: Round gains and scales | From | Matti Vaittinen <> |
| |
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! ~~
| |