Message-ID: <1fe658cd-bdd5-4afd-8564-d0dcf9eab4ab@gmail.com>
Date:   Tue, 28 Nov 2023 13:56:57 +0200
From:   Matti Vaittinen <mazziesaccount@...il.com>
To:     Jonathan Cameron <jic23@...nel.org>
Cc:     Matti Vaittinen <matti.vaittinen@...rohmeurope.com>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Subhajit Ghosh <subhajit.ghosh@...aklogic.com>,
        linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] iio: gts-helpers: Round gains and scales

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@...il.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@...il.com>
>>
>> Hi Matti,
>>
>> A few questions inline about the maths.
> 
> I appreciate the questions :) Thanks!
>>
>>>
>>> ---
>>> Subjahit, is there any chance you test this patch with your driver? Can
>>> you drop the:
>>>     if (val2 % 10)
>>>         val2 += 1;
>>> from scale setting and do you see written and read scales matching?
>>>
>>> I did run a few Kunit tests on this change - but I'm still a bit jumpy
>>> on it... Reviewing/testing is highly appreciated!
>>>
>>> Just in case someone is interested in seeing the Kunit tests, they're
>>> somewhat unpolished & crude and can emit noisy debug prints - but can
>>> anyways be found from:
>>> https://github.com/M-Vaittinen/linux/commits/iio-gts-helpers-test-v6.6
>>>
>>> ---
>>>   drivers/iio/industrialio-gts-helper.c | 58 +++++++++++++++++++++++----
>>>   1 file changed, 50 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/iio/industrialio-gts-helper.c 
>>> b/drivers/iio/industrialio-gts-helper.c
>>> index 7653261d2dc2..7dc144ac10c8 100644
>>> --- a/drivers/iio/industrialio-gts-helper.c
>>> +++ b/drivers/iio/industrialio-gts-helper.c
>>> @@ -18,6 +18,32 @@
>>>   #include <linux/iio/iio-gts-helper.h>
>>>   #include <linux/iio/types.h>
>>> +static int iio_gts_get_gain_32(u64 full, unsigned int scale)
>>> +{
>>> +    unsigned int full32 = (unsigned int) full;
>>> +    unsigned int rem;
>>> +    int result;
>>> +
>>> +    if (full == (u64)full32) {
>>> +        unsigned int rem;
>>> +
>>> +        result = full32 / scale;
>>> +        rem = full32 - scale * result;
>>> +        if (rem >= scale / 2)
>>> +            result++;
>>> +
>>> +        return result;
>>> +    }
>>> +
>>> +    rem = do_div(full, scale);
>>
>> As below, can we just add scale/2 to full in the do_div?
> 
> The rationale for doing is it in this way is to prevent (theoretical?) 
> overflow when adding scale/2 to full. Maybe this warrants adding a comment?
> 
>>
>>> +    if ((u64)rem >= scale / 2)
>>> +        result = full + 1;
>>> +    else
>>> +        result = full;
>>> +
>>> +    return result;
>>> +}
>>> +
>>>   /**
>>>    * iio_gts_get_gain - Convert scale to total gain
>>>    *
>>> @@ -28,30 +54,42 @@
>>>    *        scale is 64 100 000 000.
>>>    * @scale:    Linearized scale to compute the gain for.
>>>    *
>>> - * Return:    (floored) gain corresponding to the scale. -EINVAL if 
>>> scale
>>> + * Return:    (rounded) gain corresponding to the scale. -EINVAL if 
>>> scale
>>>    *        is invalid.
>>>    */
>>>   static int iio_gts_get_gain(const u64 max, const u64 scale)
>>>   {
>>> -    u64 full = max;
>>> +    u64 full = max, half_div;
>>> +    unsigned int scale32 = (unsigned int) scale;
>>>       int tmp = 1;
>>> -    if (scale > full || !scale)
>>> +    if (scale / 2 > full || !scale)
>>
>> Seems odd. Why are we checking scale / 2 here?
> 
> I am pretty sure I have been thinking of rounding 0.5 to 1.
> 
>>
>>>           return -EINVAL;
>>> +    /*
>>> +     * The loop-based implementation below will potentially run _long_
>>> +     * if we have a small scale and large 'max' - which may be 
>>> needed when
>>> +     * GTS is used for channels returning specific units. Luckily we 
>>> can
>>> +     * avoid the loop when scale is small and fits in 32 bits.
>>> +     */
>>> +    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 :)

>>> -    return tmp;
>>> +    return tmp - 1;
>>>   }
>>>   /**

Yours,
	-- Matti

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

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

Powered by blists - more mailing lists