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@xxxxxxxxxx>
>>
>> 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);
>>>
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
