Re: [PATCH v1 1/1] iio:core: fix IIO_VAL_FRACTIONAL sign handling

[Jonathan Cameron <jic23@kernel.org>]

On 04/09/16 17:11, Gregor Boirie wrote:
> On Sat, Sep 03, 2016 at 04:13:30PM +0100, Jonathan Cameron wrote:
>> On 02/09/16 19:27, Gregor Boirie wrote:
>>> 7985e7c100 ("iio: Introduce a new fractional value type") introduced a
>>> new IIO_VAL_FRACTIONAL value type meant to represent rational type numbers
>>> expressed by a numerator and denominator combination.
>>>
>>> Formating of IIO_VAL_FRACTIONAL values relies upon do_div() usage. This
>>> fails handling negative values properly since parameters are reevaluated
>>> as unsigned values.
>>> Fix this by using div_s64_rem() instead. Computed integer part will carry
>>> properly signed value. Formatted fractional part will always be positive.
>>>
>>> Fixes: 7985e7c100 ("iio: Introduce a new fractional value type")
>>> Signed-off-by: Gregor Boirie <gregor.boirie@parrot.com>
>>
>> Hi Gregor,
> Hi,
> 
>>
>> While this looks sensible to me, I always gain an almighty headache when
>> I hit the various divide functions.
> So am I.
>>
>> Lars, the fractional code was yours in the first place.
>> If you have time can you sanity check this please.
> This patch may break a lot things indeed. I'd feel much more comfortable if
> someone else could perform additional testing too.
> 
How about adding a patch that adds a case of this to the iio_dummy driver?

That way we can all easily test it.

Jonathan
> Grégor.
>>
>> Jonathan
>>
>>> ---
>>>  drivers/iio/industrialio-core.c | 5 ++---
>>>  1 file changed, 2 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
>>> index f914d5d..d2b8899 100644
>>> --- a/drivers/iio/industrialio-core.c
>>> +++ b/drivers/iio/industrialio-core.c
>>> @@ -613,9 +613,8 @@ ssize_t iio_format_value(char *buf, unsigned int type, int size, int *vals)
>>>  			return sprintf(buf, "%d.%09u\n", vals[0], vals[1]);
>>>  	case IIO_VAL_FRACTIONAL:
>>>  		tmp = div_s64((s64)vals[0] * 1000000000LL, vals[1]);
>>> -		vals[1] = do_div(tmp, 1000000000LL);
>>> -		vals[0] = tmp;
>>> -		return sprintf(buf, "%d.%09u\n", vals[0], vals[1]);
>>> +		vals[0] = (int)div_s64_rem(tmp, 1000000000, &vals[1]);
>>> +		return sprintf(buf, "%d.%09u\n", vals[0], abs(vals[1]));
>>>  	case IIO_VAL_FRACTIONAL_LOG2:
>>>  		tmp = (s64)vals[0] * 1000000000LL >> vals[1];
>>>  		vals[1] = do_div(tmp, 1000000000LL);
>>>
>>