| Message ID | 1598465676-28912-1-git-send-email-anand.ashok.dumbre@xilinx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] iio: core: Fix IIO_VAL_FRACTIONAL calculation for negative values | expand |
On Wed, 26 Aug 2020 11:14:36 -0700 Anand Ashok Dumbre <anand.ashok.dumbre@xilinx.com> wrote: > Fixes IIO_VAL_FRACTIONAL for case when the result is negative and > exponent is 0. > > example: if the result is -0.75, tmp0 will be 0 and tmp1 = 75 > This causes the output to lose sign because of %d in snprintf > which works for tmp0 <= -1. > > Signed-off-by: Anand Ashok Dumbre <anand.ashok.dumbre@xilinx.com> Looks good. Just one last thing. Is this actually hit in an existing driver? I'm just wondering how far back we need to push it in stable etc. Thanks, Jonathan > --- > changes since v1: > Changed -%d to -0 to make the fix clearer. > Removed the email footer. > Updated the commit description with an example > -- > drivers/iio/industrialio-core.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c > index cdcd16f1..a239fa2 100644 > --- a/drivers/iio/industrialio-core.c > +++ b/drivers/iio/industrialio-core.c > @@ -592,6 +592,7 @@ static ssize_t __iio_format_value(char *buf, size_t len, unsigned int type, > { > unsigned long long tmp; > int tmp0, tmp1; > + s64 tmp2; > bool scale_db = false; > > switch (type) { > @@ -614,10 +615,13 @@ static ssize_t __iio_format_value(char *buf, size_t len, unsigned int type, > else > return scnprintf(buf, len, "%d.%09u", vals[0], vals[1]); > case IIO_VAL_FRACTIONAL: > - tmp = div_s64((s64)vals[0] * 1000000000LL, vals[1]); > + tmp2 = div_s64((s64)vals[0] * 1000000000LL, vals[1]); > tmp1 = vals[1]; > tmp0 = (int)div_s64_rem(tmp, 1000000000, &tmp1); > - return scnprintf(buf, len, "%d.%09u", tmp0, abs(tmp1)); > + if ((tmp2 < 0) && (tmp0 == 0)) > + return snprintf(buf, len, "-0.%09u", abs(tmp1)); > + else > + return snprintf(buf, len, "%d.%09u", tmp0, abs(tmp1)); > case IIO_VAL_FRACTIONAL_LOG2: > tmp = shift_right((s64)vals[0] * 1000000000LL, vals[1]); > tmp0 = (int)div_s64_rem(tmp, 1000000000LL, &tmp1);
Hi Jonathan, I encountered this when I was developing a new driver. If you look at the function where this is used, all other IIO_VAL_MICRO and NANO have this fix added at some point. Thanks, Anand > -----Original Message----- > From: Jonathan Cameron <jic23@kernel.org> > Sent: Saturday, August 29, 2020 4:19 PM > To: Anand Ashok Dumbre <ANANDASH@xilinx.com> > Cc: knaack.h@gmx.de; lars@metafoo.de; pmeerw@pmeerw.net; Michal > Simek <michals@xilinx.com>; git <git@xilinx.com>; linux- > iio@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux- > kernel@vger.kernel.org; Anand Ashok Dumbre <ANANDASH@xilinx.com> > Subject: Re: [PATCH v2] iio: core: Fix IIO_VAL_FRACTIONAL calculation for > negative values > > On Wed, 26 Aug 2020 11:14:36 -0700 > Anand Ashok Dumbre <anand.ashok.dumbre@xilinx.com> wrote: > > > Fixes IIO_VAL_FRACTIONAL for case when the result is negative and > > exponent is 0. > > > > example: if the result is -0.75, tmp0 will be 0 and tmp1 = 75 This > > causes the output to lose sign because of %d in snprintf which works > > for tmp0 <= -1. > > > > Signed-off-by: Anand Ashok Dumbre <anand.ashok.dumbre@xilinx.com> > > Looks good. Just one last thing. > > Is this actually hit in an existing driver? I'm just wondering how far back we > need to push it in stable etc. > > Thanks, > > Jonathan > > > --- > > changes since v1: > > Changed -%d to -0 to make the fix clearer. > > Removed the email footer. > > Updated the commit description with an example > > -- > > drivers/iio/industrialio-core.c | 8 ++++++-- > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/iio/industrialio-core.c > > b/drivers/iio/industrialio-core.c index cdcd16f1..a239fa2 100644 > > --- a/drivers/iio/industrialio-core.c > > +++ b/drivers/iio/industrialio-core.c > > @@ -592,6 +592,7 @@ static ssize_t __iio_format_value(char *buf, > > size_t len, unsigned int type, { > > unsigned long long tmp; > > int tmp0, tmp1; > > + s64 tmp2; > > bool scale_db = false; > > > > switch (type) { > > @@ -614,10 +615,13 @@ static ssize_t __iio_format_value(char *buf, > size_t len, unsigned int type, > > else > > return scnprintf(buf, len, "%d.%09u", vals[0], vals[1]); > > case IIO_VAL_FRACTIONAL: > > - tmp = div_s64((s64)vals[0] * 1000000000LL, vals[1]); > > + tmp2 = div_s64((s64)vals[0] * 1000000000LL, vals[1]); > > tmp1 = vals[1]; > > tmp0 = (int)div_s64_rem(tmp, 1000000000, &tmp1); > > - return scnprintf(buf, len, "%d.%09u", tmp0, abs(tmp1)); > > + if ((tmp2 < 0) && (tmp0 == 0)) > > + return snprintf(buf, len, "-0.%09u", abs(tmp1)); > > + else > > + return snprintf(buf, len, "%d.%09u", tmp0, > abs(tmp1)); > > case IIO_VAL_FRACTIONAL_LOG2: > > tmp = shift_right((s64)vals[0] * 1000000000LL, vals[1]); > > tmp0 = (int)div_s64_rem(tmp, 1000000000LL, &tmp1);
Hi Anand,
url: https://github.com/0day-ci/linux/commits/Anand-Ashok-Dumbre/iio-core-Fix-IIO_VAL_FRACTIONAL-calculation-for-negative-values/20200827-021613
base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
config: x86_64-randconfig-m001-20200826 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
smatch warnings:
drivers/iio/industrialio-core.c:620 __iio_format_value() error: uninitialized symbol 'tmp'.
# https://github.com/0day-ci/linux/commit/c66f94e17db7eca581318d225d1fb41c56aced08
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Anand-Ashok-Dumbre/iio-core-Fix-IIO_VAL_FRACTIONAL-calculation-for-negative-values/20200827-021613
git checkout c66f94e17db7eca581318d225d1fb41c56aced08
vim +/tmp +620 drivers/iio/industrialio-core.c
51239600074bc9 drivers/iio/industrialio-core.c Jonathan Cameron 2016-11-08 590 static ssize_t __iio_format_value(char *buf, size_t len, unsigned int type,
51239600074bc9 drivers/iio/industrialio-core.c Jonathan Cameron 2016-11-08 591 int size, const int *vals)
1d892719e70e47 drivers/staging/iio/industrialio-core.c Jonathan Cameron 2011-05-18 592 {
7985e7c1003bc5 drivers/iio/industrialio-core.c Lars-Peter Clausen 2012-09-14 593 unsigned long long tmp;
51239600074bc9 drivers/iio/industrialio-core.c Jonathan Cameron 2016-11-08 594 int tmp0, tmp1;
c66f94e17db7ec drivers/iio/industrialio-core.c Anand Ashok Dumbre 2020-08-26 595 s64 tmp2;
67eedba39ed1ac drivers/iio/industrialio-core.c Michael Hennerich 2012-05-11 596 bool scale_db = false;
1d892719e70e47 drivers/staging/iio/industrialio-core.c Jonathan Cameron 2011-05-18 597
3661f3f5e961f7 drivers/iio/industrialio-core.c Lars-Peter Clausen 2013-10-07 598 switch (type) {
67eedba39ed1ac drivers/iio/industrialio-core.c Michael Hennerich 2012-05-11 599 case IIO_VAL_INT:
35a4eeb055c9c3 drivers/iio/industrialio-core.c Takashi Iwai 2020-03-11 600 return scnprintf(buf, len, "%d", vals[0]);
67eedba39ed1ac drivers/iio/industrialio-core.c Michael Hennerich 2012-05-11 601 case IIO_VAL_INT_PLUS_MICRO_DB:
67eedba39ed1ac drivers/iio/industrialio-core.c Michael Hennerich 2012-05-11 602 scale_db = true;
9d793c1a7f25d7 drivers/iio/industrialio-core.c Gustavo A. R. Silva 2017-11-08 603 /* fall through */
67eedba39ed1ac drivers/iio/industrialio-core.c Michael Hennerich 2012-05-11 604 case IIO_VAL_INT_PLUS_MICRO:
9fbfb4b37ed23f drivers/iio/industrialio-core.c Srinivas Pandruvada 2014-04-29 605 if (vals[1] < 0)
35a4eeb055c9c3 drivers/iio/industrialio-core.c Takashi Iwai 2020-03-11 606 return scnprintf(buf, len, "-%d.%06u%s", abs(vals[0]),
8f57e4d930d482 drivers/iio/industrialio-core.c Michal Nazarewicz 2016-01-15 607 -vals[1], scale_db ? " dB" : "");
1d892719e70e47 drivers/staging/iio/industrialio-core.c Jonathan Cameron 2011-05-18 608 else
35a4eeb055c9c3 drivers/iio/industrialio-core.c Takashi Iwai 2020-03-11 609 return scnprintf(buf, len, "%d.%06u%s", vals[0], vals[1],
67eedba39ed1ac drivers/iio/industrialio-core.c Michael Hennerich 2012-05-11 610 scale_db ? " dB" : "");
67eedba39ed1ac drivers/iio/industrialio-core.c Michael Hennerich 2012-05-11 611 case IIO_VAL_INT_PLUS_NANO:
9fbfb4b37ed23f drivers/iio/industrialio-core.c Srinivas Pandruvada 2014-04-29 612 if (vals[1] < 0)
35a4eeb055c9c3 drivers/iio/industrialio-core.c Takashi Iwai 2020-03-11 613 return scnprintf(buf, len, "-%d.%09u", abs(vals[0]),
9fbfb4b37ed23f drivers/iio/industrialio-core.c Srinivas Pandruvada 2014-04-29 614 -vals[1]);
71646e2c7ae4ed drivers/staging/iio/industrialio-core.c Michael Hennerich 2011-06-27 615 else
35a4eeb055c9c3 drivers/iio/industrialio-core.c Takashi Iwai 2020-03-11 616 return scnprintf(buf, len, "%d.%09u", vals[0], vals[1]);
7985e7c1003bc5 drivers/iio/industrialio-core.c Lars-Peter Clausen 2012-09-14 617 case IIO_VAL_FRACTIONAL:
c66f94e17db7ec drivers/iio/industrialio-core.c Anand Ashok Dumbre 2020-08-26 618 tmp2 = div_s64((s64)vals[0] * 1000000000LL, vals[1]);
51239600074bc9 drivers/iio/industrialio-core.c Jonathan Cameron 2016-11-08 619 tmp1 = vals[1];
51239600074bc9 drivers/iio/industrialio-core.c Jonathan Cameron 2016-11-08 @620 tmp0 = (int)div_s64_rem(tmp, 1000000000, &tmp1);
^^^
Probably "tmp2" was intended?
c66f94e17db7ec drivers/iio/industrialio-core.c Anand Ashok Dumbre 2020-08-26 621 if ((tmp2 < 0) && (tmp0 == 0))
c66f94e17db7ec drivers/iio/industrialio-core.c Anand Ashok Dumbre 2020-08-26 622 return snprintf(buf, len, "-0.%09u", abs(tmp1));
c66f94e17db7ec drivers/iio/industrialio-core.c Anand Ashok Dumbre 2020-08-26 623 else
c66f94e17db7ec drivers/iio/industrialio-core.c Anand Ashok Dumbre 2020-08-26 624 return snprintf(buf, len, "%d.%09u", tmp0, abs(tmp1));
103d9fb907058e drivers/iio/industrialio-core.c Lars-Peter Clausen 2012-10-16 625 case IIO_VAL_FRACTIONAL_LOG2:
7fd6592d128704 drivers/iio/industrialio-core.c Nikolaus Schulz 2017-03-24 626 tmp = shift_right((s64)vals[0] * 1000000000LL, vals[1]);
7fd6592d128704 drivers/iio/industrialio-core.c Nikolaus Schulz 2017-03-24 627 tmp0 = (int)div_s64_rem(tmp, 1000000000LL, &tmp1);
35a4eeb055c9c3 drivers/iio/industrialio-core.c Takashi Iwai 2020-03-11 628 return scnprintf(buf, len, "%d.%09u", tmp0, abs(tmp1));
9fbfb4b37ed23f drivers/iio/industrialio-core.c Srinivas Pandruvada 2014-04-29 629 case IIO_VAL_INT_MULTIPLE:
9fbfb4b37ed23f drivers/iio/industrialio-core.c Srinivas Pandruvada 2014-04-29 630 {
9fbfb4b37ed23f drivers/iio/industrialio-core.c Srinivas Pandruvada 2014-04-29 631 int i;
51239600074bc9 drivers/iio/industrialio-core.c Jonathan Cameron 2016-11-08 632 int l = 0;
9fbfb4b37ed23f drivers/iio/industrialio-core.c Srinivas Pandruvada 2014-04-29 633
51239600074bc9 drivers/iio/industrialio-core.c Jonathan Cameron 2016-11-08 634 for (i = 0; i < size; ++i) {
35a4eeb055c9c3 drivers/iio/industrialio-core.c Takashi Iwai 2020-03-11 635 l += scnprintf(&buf[l], len - l, "%d ", vals[i]);
51239600074bc9 drivers/iio/industrialio-core.c Jonathan Cameron 2016-11-08 636 if (l >= len)
51239600074bc9 drivers/iio/industrialio-core.c Jonathan Cameron 2016-11-08 637 break;
51239600074bc9 drivers/iio/industrialio-core.c Jonathan Cameron 2016-11-08 638 }
51239600074bc9 drivers/iio/industrialio-core.c Jonathan Cameron 2016-11-08 639 return l;
9fbfb4b37ed23f drivers/iio/industrialio-core.c Srinivas Pandruvada 2014-04-29 640 }
8cb3403633146a drivers/iio/industrialio-core.c Andrea Merello 2019-11-20 641 case IIO_VAL_CHAR:
35a4eeb055c9c3 drivers/iio/industrialio-core.c Takashi Iwai 2020-03-11 642 return scnprintf(buf, len, "%c", (char)vals[0]);
67eedba39ed1ac drivers/iio/industrialio-core.c Michael Hennerich 2012-05-11 643 default:
1d892719e70e47 drivers/staging/iio/industrialio-core.c Jonathan Cameron 2011-05-18 644 return 0;
1d892719e70e47 drivers/staging/iio/industrialio-core.c Jonathan Cameron 2011-05-18 645 }
67eedba39ed1ac drivers/iio/industrialio-core.c Michael Hennerich 2012-05-11 646 }
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c index cdcd16f1..a239fa2 100644 --- a/drivers/iio/industrialio-core.c +++ b/drivers/iio/industrialio-core.c @@ -592,6 +592,7 @@ static ssize_t __iio_format_value(char *buf, size_t len, unsigned int type, { unsigned long long tmp; int tmp0, tmp1; + s64 tmp2; bool scale_db = false; switch (type) { @@ -614,10 +615,13 @@ static ssize_t __iio_format_value(char *buf, size_t len, unsigned int type, else return scnprintf(buf, len, "%d.%09u", vals[0], vals[1]); case IIO_VAL_FRACTIONAL: - tmp = div_s64((s64)vals[0] * 1000000000LL, vals[1]); + tmp2 = div_s64((s64)vals[0] * 1000000000LL, vals[1]); tmp1 = vals[1]; tmp0 = (int)div_s64_rem(tmp, 1000000000, &tmp1); - return scnprintf(buf, len, "%d.%09u", tmp0, abs(tmp1)); + if ((tmp2 < 0) && (tmp0 == 0)) + return snprintf(buf, len, "-0.%09u", abs(tmp1)); + else + return snprintf(buf, len, "%d.%09u", tmp0, abs(tmp1)); case IIO_VAL_FRACTIONAL_LOG2: tmp = shift_right((s64)vals[0] * 1000000000LL, vals[1]); tmp0 = (int)div_s64_rem(tmp, 1000000000LL, &tmp1);
Fixes IIO_VAL_FRACTIONAL for case when the result is negative and exponent is 0. example: if the result is -0.75, tmp0 will be 0 and tmp1 = 75 This causes the output to lose sign because of %d in snprintf which works for tmp0 <= -1. Signed-off-by: Anand Ashok Dumbre <anand.ashok.dumbre@xilinx.com> --- changes since v1: Changed -%d to -0 to make the fix clearer. Removed the email footer. Updated the commit description with an example -- drivers/iio/industrialio-core.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)