linux-iio.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] iio: core: Fix IIO_VAL_FRACTIONAL calculation for negative values
@ 2020-08-26 18:14 Anand Ashok Dumbre
  2020-08-29 15:19 ` Jonathan Cameron
  2020-08-31  9:51 ` Dan Carpenter
  0 siblings, 2 replies; 4+ messages in thread
From: Anand Ashok Dumbre @ 2020-08-26 18:14 UTC (permalink / raw)
  To: jic23, knaack.h, lars, pmeerw, michal.simek, git, linux-iio,
	linux-arm-kernel, linux-kernel
  Cc: anandash, Anand Ashok Dumbre

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(-)

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);
-- 
2.7.4


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] iio: core: Fix IIO_VAL_FRACTIONAL calculation for negative values
  2020-08-26 18:14 [PATCH v2] iio: core: Fix IIO_VAL_FRACTIONAL calculation for negative values Anand Ashok Dumbre
@ 2020-08-29 15:19 ` Jonathan Cameron
  2020-08-31  8:55   ` Anand Ashok Dumbre
  2020-08-31  9:51 ` Dan Carpenter
  1 sibling, 1 reply; 4+ messages in thread
From: Jonathan Cameron @ 2020-08-29 15:19 UTC (permalink / raw)
  To: Anand Ashok Dumbre
  Cc: knaack.h, lars, pmeerw, michal.simek, git, linux-iio,
	linux-arm-kernel, linux-kernel, anandash

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);


^ permalink raw reply	[flat|nested] 4+ messages in thread

* RE: [PATCH v2] iio: core: Fix IIO_VAL_FRACTIONAL calculation for negative values
  2020-08-29 15:19 ` Jonathan Cameron
@ 2020-08-31  8:55   ` Anand Ashok Dumbre
  0 siblings, 0 replies; 4+ messages in thread
From: Anand Ashok Dumbre @ 2020-08-31  8:55 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: knaack.h, lars, pmeerw, Michal Simek, git, linux-iio,
	linux-arm-kernel, linux-kernel

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);


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] iio: core: Fix IIO_VAL_FRACTIONAL calculation for negative values
  2020-08-26 18:14 [PATCH v2] iio: core: Fix IIO_VAL_FRACTIONAL calculation for negative values Anand Ashok Dumbre
  2020-08-29 15:19 ` Jonathan Cameron
@ 2020-08-31  9:51 ` Dan Carpenter
  1 sibling, 0 replies; 4+ messages in thread
From: Dan Carpenter @ 2020-08-31  9:51 UTC (permalink / raw)
  To: kbuild, Anand Ashok Dumbre, jic23, knaack.h, lars, pmeerw,
	michal.simek, git, linux-iio, linux-arm-kernel, linux-kernel
  Cc: lkp, kbuild-all, anandash

[-- Attachment #1: Type: text/plain, Size: 8070 bytes --]

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 40677 bytes --]

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2020-08-31  9:52 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-26 18:14 [PATCH v2] iio: core: Fix IIO_VAL_FRACTIONAL calculation for negative values Anand Ashok Dumbre
2020-08-29 15:19 ` Jonathan Cameron
2020-08-31  8:55   ` Anand Ashok Dumbre
2020-08-31  9:51 ` Dan Carpenter

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).