* [PATCH v1 1/1] iio:core: fix IIO_VAL_FRACTIONAL sign handling
@ 2016-09-02 18:27 Gregor Boirie
2016-09-03 15:13 ` Jonathan Cameron
0 siblings, 1 reply; 9+ messages in thread
From: Gregor Boirie @ 2016-09-02 18:27 UTC (permalink / raw)
To: linux-iio
Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, Gregor Boirie
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>
---
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);
--
2.1.4
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH v1 1/1] iio:core: fix IIO_VAL_FRACTIONAL sign handling
2016-09-02 18:27 [PATCH v1 1/1] iio:core: fix IIO_VAL_FRACTIONAL sign handling Gregor Boirie
@ 2016-09-03 15:13 ` Jonathan Cameron
2016-09-04 16:11 ` Gregor Boirie
2016-09-05 7:59 ` Lars-Peter Clausen
0 siblings, 2 replies; 9+ messages in thread
From: Jonathan Cameron @ 2016-09-03 15:13 UTC (permalink / raw)
To: Gregor Boirie, linux-iio
Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler
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,
While this looks sensible to me, I always gain an almighty headache when
I hit the various divide functions.
Lars, the fractional code was yours in the first place.
If you have time can you sanity check this please.
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);
>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v1 1/1] iio:core: fix IIO_VAL_FRACTIONAL sign handling
2016-09-03 15:13 ` Jonathan Cameron
@ 2016-09-04 16:11 ` Gregor Boirie
2016-09-04 16:23 ` Jonathan Cameron
2016-09-05 7:59 ` Lars-Peter Clausen
1 sibling, 1 reply; 9+ messages in thread
From: Gregor Boirie @ 2016-09-04 16:11 UTC (permalink / raw)
To: Jonathan Cameron
Cc: linux-iio, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v1 1/1] iio:core: fix IIO_VAL_FRACTIONAL sign handling
2016-09-04 16:11 ` Gregor Boirie
@ 2016-09-04 16:23 ` Jonathan Cameron
0 siblings, 0 replies; 9+ messages in thread
From: Jonathan Cameron @ 2016-09-04 16:23 UTC (permalink / raw)
To: Gregor Boirie, linux-iio, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler
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);
>>>
>>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1 1/1] iio:core: fix IIO_VAL_FRACTIONAL sign handling
2016-09-03 15:13 ` Jonathan Cameron
2016-09-04 16:11 ` Gregor Boirie
@ 2016-09-05 7:59 ` Lars-Peter Clausen
2016-09-05 20:24 ` Jonathan Cameron
1 sibling, 1 reply; 9+ messages in thread
From: Lars-Peter Clausen @ 2016-09-05 7:59 UTC (permalink / raw)
To: Jonathan Cameron, Gregor Boirie, linux-iio
Cc: Hartmut Knaack, Peter Meerwald-Stadler
On 09/03/2016 05:13 PM, 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,
>
> While this looks sensible to me, I always gain an almighty headache when
> I hit the various divide functions.
>
> Lars, the fractional code was yours in the first place.
> If you have time can you sanity check this please.
Looks good. While looking into this I noticed that we have a very similar
patch[1] in our tree, seems like somebody forgot to send that upstream.
Reviewed-by: Lars-Peter Clausen <lars@metafoo.de>
[1]
https://github.com/analogdevicesinc/linux/commit/1dbcdcfd0c8d3f5513572697685d9f30ba49d851
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v1 1/1] iio:core: fix IIO_VAL_FRACTIONAL sign handling
2016-09-05 7:59 ` Lars-Peter Clausen
@ 2016-09-05 20:24 ` Jonathan Cameron
2016-09-07 18:11 ` Gregor Boirie
0 siblings, 1 reply; 9+ messages in thread
From: Jonathan Cameron @ 2016-09-05 20:24 UTC (permalink / raw)
To: Lars-Peter Clausen, Gregor Boirie, linux-iio
Cc: Hartmut Knaack, Peter Meerwald-Stadler
On 05/09/16 08:59, Lars-Peter Clausen wrote:
> On 09/03/2016 05:13 PM, 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,
>>
>> While this looks sensible to me, I always gain an almighty headache when
>> I hit the various divide functions.
>>
>> Lars, the fractional code was yours in the first place.
>> If you have time can you sanity check this please.
>
> Looks good. While looking into this I noticed that we have a very similar
> patch[1] in our tree, seems like somebody forgot to send that upstream.
>
> Reviewed-by: Lars-Peter Clausen <lars@metafoo.de>
Applied to the fixes-togreg branch of iio.git and marked for stable.
Good to know about the previous patch - if there was anything hiding around
this I would assume that would have shaken it out by now!
Thanks
Jonathan
>
>
> [1]
> https://github.com/analogdevicesinc/linux/commit/1dbcdcfd0c8d3f5513572697685d9f30ba49d851
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v1 1/1] iio:core: fix IIO_VAL_FRACTIONAL sign handling
2016-09-05 20:24 ` Jonathan Cameron
@ 2016-09-07 18:11 ` Gregor Boirie
2016-09-07 18:54 ` Lars-Peter Clausen
0 siblings, 1 reply; 9+ messages in thread
From: Gregor Boirie @ 2016-09-07 18:11 UTC (permalink / raw)
To: Jonathan Cameron, Lars-Peter Clausen, linux-iio
Cc: Hartmut Knaack, Peter Meerwald-Stadler
Just to mention that a somewhat similar patch might be needed to handle
negative
IIO_VAL_FRACTIONAL_LOG2 values.
On 09/05/2016 10:24 PM, Jonathan Cameron wrote:
> On 05/09/16 08:59, Lars-Peter Clausen wrote:
>> On 09/03/2016 05:13 PM, 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,
>>>
>>> While this looks sensible to me, I always gain an almighty headache when
>>> I hit the various divide functions.
>>>
>>> Lars, the fractional code was yours in the first place.
>>> If you have time can you sanity check this please.
>> Looks good. While looking into this I noticed that we have a very similar
>> patch[1] in our tree, seems like somebody forgot to send that upstream.
>>
>> Reviewed-by: Lars-Peter Clausen <lars@metafoo.de>
> Applied to the fixes-togreg branch of iio.git and marked for stable.
> Good to know about the previous patch - if there was anything hiding around
> this I would assume that would have shaken it out by now!
>
> Thanks
>
> Jonathan
>>
>> [1]
>> https://github.com/analogdevicesinc/linux/commit/1dbcdcfd0c8d3f5513572697685d9f30ba49d851
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v1 1/1] iio:core: fix IIO_VAL_FRACTIONAL sign handling
2016-09-07 18:11 ` Gregor Boirie
@ 2016-09-07 18:54 ` Lars-Peter Clausen
2016-09-08 8:21 ` Gregor Boirie
0 siblings, 1 reply; 9+ messages in thread
From: Lars-Peter Clausen @ 2016-09-07 18:54 UTC (permalink / raw)
To: Gregor Boirie, Jonathan Cameron, linux-iio
Cc: Hartmut Knaack, Peter Meerwald-Stadler
On 09/07/2016 08:11 PM, Gregor Boirie wrote:
> Just to mention that a somewhat similar patch might be needed to handle
> negative
> IIO_VAL_FRACTIONAL_LOG2 values.
Hm, right, Michael's patch handled that. Do you want to send a patch, or do
you prefer that somebody else sends it?
Thanks,
- Lars
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1 1/1] iio:core: fix IIO_VAL_FRACTIONAL sign handling
2016-09-07 18:54 ` Lars-Peter Clausen
@ 2016-09-08 8:21 ` Gregor Boirie
0 siblings, 0 replies; 9+ messages in thread
From: Gregor Boirie @ 2016-09-08 8:21 UTC (permalink / raw)
To: Lars-Peter Clausen, Jonathan Cameron, linux-iio
Cc: Hartmut Knaack, Peter Meerwald-Stadler
On 09/07/2016 08:54 PM, Lars-Peter Clausen wrote:
> On 09/07/2016 08:11 PM, Gregor Boirie wrote:
>> Just to mention that a somewhat similar patch might be needed to handle
>> negative
>> IIO_VAL_FRACTIONAL_LOG2 values.
> Hm, right, Michael's patch handled that. Do you want to send a patch, or do
> you prefer that somebody else sends it?
I'm sure Michael's patch will suit me, not to mention that I have no driver
requiring IIO_VAL_FRACTIONAL_LOG2 computation on my setup :)
Please, go ahead.
Regards,
Grégor.
>
> Thanks,
> - Lars
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2016-09-08 8:17 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-09-02 18:27 [PATCH v1 1/1] iio:core: fix IIO_VAL_FRACTIONAL sign handling Gregor Boirie
2016-09-03 15:13 ` Jonathan Cameron
2016-09-04 16:11 ` Gregor Boirie
2016-09-04 16:23 ` Jonathan Cameron
2016-09-05 7:59 ` Lars-Peter Clausen
2016-09-05 20:24 ` Jonathan Cameron
2016-09-07 18:11 ` Gregor Boirie
2016-09-07 18:54 ` Lars-Peter Clausen
2016-09-08 8:21 ` Gregor Boirie
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).