linux-kernel - Re: [PATCH v4 05/10] iio: afe: rescale: add INT_PLUS

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <353ceae9-ad7a-3175-d764-a9e590d3e8d3@axentia.se>
Date:   Sat, 10 Jul 2021 10:14:20 +0200
From:   Peter Rosin <peda@...ntia.se>
To:     Liam Beguin <liambeguin@...il.com>, jic23@...nel.org,
        lars@...afoo.de, pmeerw@...erw.net
Cc:     linux-kernel@...r.kernel.org, linux-iio@...r.kernel.org,
        devicetree@...r.kernel.org, robh+dt@...nel.org
Subject: Re: [PATCH v4 05/10] iio: afe: rescale: add INT_PLUS_{MICRO,NANO}
 support



On 2021-07-09 21:30, Liam Beguin wrote:
> On Fri Jul 9, 2021 at 12:29 PM EDT, Peter Rosin wrote:
>>
>>
>> On 2021-07-06 18:09, Liam Beguin wrote:
>>> From: Liam Beguin <lvb@...hos.com>
>>>
>>> Add IIO_VAL_INT_PLUS_{NANO,MICRO} scaling support.
>>> Scale the integer part and the decimal parts individually and keep the
>>> original scaling type.
>>>
>>> Signed-off-by: Liam Beguin <lvb@...hos.com>
>>> ---
>>>  drivers/iio/afe/iio-rescale.c | 8 ++++++++
>>>  1 file changed, 8 insertions(+)
>>>
>>> diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
>>> index ba3bdcc69b16..1d0e24145d87 100644
>>> --- a/drivers/iio/afe/iio-rescale.c
>>> +++ b/drivers/iio/afe/iio-rescale.c
>>> @@ -89,7 +89,15 @@ static int rescale_read_raw(struct iio_dev *indio_dev,
>>>  			do_div(tmp, 1000000000LL);
>>>  			*val = tmp;
>>>  			return ret;
>>> +		case IIO_VAL_INT_PLUS_NANO:
>>> +		case IIO_VAL_INT_PLUS_MICRO:
>>> +			tmp = (s64)*val * rescale->numerator;
>>> +			*val = div_s64(tmp, rescale->denominator);
>>> +			tmp = (s64)*val2 * rescale->numerator;
>>> +			*val2 = div_s64(tmp, rescale->denominator);
>>
> 
> Hi Peter,
> 
>> Hi!
>>
>> You are losing precision, and you are not mormalising after the
>> calculation.
> 
> Can you elaborate a little on what you mean here?
> 
> Do you mean that I should make sure that *val2, the PLUS_{NANO,MICRO}
> part, doesn't contain an integer part? And if so transfer that part back
> to *val?

Yes. On 32-bit, you will easily wrap, especially for PLUS_NANO. You'd
only need a scale factor of 10 or so and a fractional part above .5 to
hit the roof (10 * 500000000 > 2^32).

But I also mean that you are losing precision when you are scaling
the integer part and the fractional part separately. That deserves
at least a comment, but ideally it should be handled correctly.

>> I think it's better to not even attempt this given that the results can
>> be
>> really poor.
> 
> Unfortunatelly, I'm kinda stuck with this as some of my ADC use these
> types.

Ok. Crap. :-)

Cheers,
Peter