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

[Gregor Boirie <gregor.boirie@parrot.com>]

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 numb=
ers
> > expressed by a numerator and denominator combination.
> >=20
> > 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 car=
ry
> > properly signed value. Formatted fractional part will always be positiv=
e.
> >=20
> > Fixes: 7985e7c100 ("iio: Introduce a new fractional value type")
> > Signed-off-by: Gregor Boirie <gregor.boirie@parrot.com>
>=20
> Hi Gregor,
Hi,

>=20
> While this looks sensible to me, I always gain an almighty headache when
> I hit the various divide functions.
So am I.
>=20
> 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.

Gr=C3=A9gor.
>=20
> Jonathan
>=20
> > ---
> >  drivers/iio/industrialio-core.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> >=20
> > 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 ty=
pe, int size, int *vals)
> >  			return sprintf(buf, "%d.%09u\n", vals[0], vals[1]);
> >  	case IIO_VAL_FRACTIONAL:
> >  		tmp =3D div_s64((s64)vals[0] * 1000000000LL, vals[1]);
> > -		vals[1] =3D do_div(tmp, 1000000000LL);
> > -		vals[0] =3D tmp;
> > -		return sprintf(buf, "%d.%09u\n", vals[0], vals[1]);
> > +		vals[0] =3D (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 =3D (s64)vals[0] * 1000000000LL >> vals[1];
> >  		vals[1] =3D do_div(tmp, 1000000000LL);
> >=20
>=20