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

[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@xxxxxxxxx> 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@xxxxxxxxx/
> > > >
> > > > Do rounding when computing scales and gains.
> > > >
> > > > Fixes: 38416c28e168 ("iio: light: Add gain-time-scale helpers")
> > > > Signed-off-by: Matti Vaittinen <mazziesaccount@xxxxxxxxx>

...

> > > > +    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! ~~