Linux-IIO Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] iio: imu: adis16480: clean up a condition
@ 2019-09-26  8:10 Dan Carpenter
  2019-09-26 11:06 ` Ardelean, Alexandru
  0 siblings, 1 reply; 7+ messages in thread
From: Dan Carpenter @ 2019-09-26  8:10 UTC (permalink / raw)
  To: Lars-Peter Clausen
  Cc: Michael Hennerich, Stefan Popa, Jonathan Cameron, Hartmut Knaack,
	Peter Meerwald-Stadler, linux-iio, linux-kernel, kernel-janitors

The "t" variable is unsigned so it can't be less than zero.  We really
are just trying to prevent divide by zero bugs so just checking against
zero is sufficient.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 drivers/iio/imu/adis16480.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/imu/adis16480.c b/drivers/iio/imu/adis16480.c
index b99d73887c9f..e144e567675d 100644
--- a/drivers/iio/imu/adis16480.c
+++ b/drivers/iio/imu/adis16480.c
@@ -318,7 +318,7 @@ static int adis16480_set_freq(struct iio_dev *indio_dev, int val, int val2)
 	unsigned int t, reg;
 
 	t =  val * 1000 + val2 / 1000;
-	if (t <= 0)
+	if (t == 0)
 		return -EINVAL;
 
 	/*
-- 
2.20.1


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

* Re: [PATCH] iio: imu: adis16480: clean up a condition
  2019-09-26  8:10 [PATCH] iio: imu: adis16480: clean up a condition Dan Carpenter
@ 2019-09-26 11:06 ` Ardelean, Alexandru
  2019-09-26 11:36   ` Dan Carpenter
  0 siblings, 1 reply; 7+ messages in thread
From: Ardelean, Alexandru @ 2019-09-26 11:06 UTC (permalink / raw)
  To: dan.carpenter, lars
  Cc: kernel-janitors, Popa, Stefan Serban, linux-iio, jic23,
	linux-kernel, Hennerich, Michael, pmeerw, knaack.h

On Thu, 2019-09-26 at 11:10 +0300, Dan Carpenter wrote:
> [External]
> 
> The "t" variable is unsigned so it can't be less than zero.  We really
> are just trying to prevent divide by zero bugs so just checking against
> zero is sufficient.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
>  drivers/iio/imu/adis16480.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/imu/adis16480.c b/drivers/iio/imu/adis16480.c
> index b99d73887c9f..e144e567675d 100644
> --- a/drivers/iio/imu/adis16480.c
> +++ b/drivers/iio/imu/adis16480.c
> @@ -318,7 +318,7 @@ static int adis16480_set_freq(struct iio_dev
> *indio_dev, int val, int val2)
>  	unsigned int t, reg;

I would just change the type of "t" to "int".
Especially, since "val" & "val2" are "int".

Thanks for the catch :)
Alex

>  
>  	t =  val * 1000 + val2 / 1000;
> -	if (t <= 0)
> +	if (t == 0)
>  		return -EINVAL;
>  
>  	/*

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

* Re: [PATCH] iio: imu: adis16480: clean up a condition
  2019-09-26 11:06 ` Ardelean, Alexandru
@ 2019-09-26 11:36   ` Dan Carpenter
  2019-10-06  8:51     ` Jonathan Cameron
  0 siblings, 1 reply; 7+ messages in thread
From: Dan Carpenter @ 2019-09-26 11:36 UTC (permalink / raw)
  To: Ardelean, Alexandru
  Cc: lars, kernel-janitors, Popa, Stefan Serban, linux-iio, jic23,
	linux-kernel, Hennerich, Michael, pmeerw, knaack.h

On Thu, Sep 26, 2019 at 11:06:39AM +0000, Ardelean, Alexandru wrote:
> On Thu, 2019-09-26 at 11:10 +0300, Dan Carpenter wrote:
> > [External]
> > 
> > The "t" variable is unsigned so it can't be less than zero.  We really
> > are just trying to prevent divide by zero bugs so just checking against
> > zero is sufficient.
> > 
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > ---
> >  drivers/iio/imu/adis16480.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/iio/imu/adis16480.c b/drivers/iio/imu/adis16480.c
> > index b99d73887c9f..e144e567675d 100644
> > --- a/drivers/iio/imu/adis16480.c
> > +++ b/drivers/iio/imu/adis16480.c
> > @@ -318,7 +318,7 @@ static int adis16480_set_freq(struct iio_dev
> > *indio_dev, int val, int val2)
> >  	unsigned int t, reg;
> 
> I would just change the type of "t" to "int".
> Especially, since "val" & "val2" are "int".
> 

Yeah, but negatives or high values are basically fine.  We only care
zero.

regards,
dan carpenter


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

* Re: [PATCH] iio: imu: adis16480: clean up a condition
  2019-09-26 11:36   ` Dan Carpenter
@ 2019-10-06  8:51     ` Jonathan Cameron
  2019-10-06 18:14       ` Dan Carpenter
  0 siblings, 1 reply; 7+ messages in thread
From: Jonathan Cameron @ 2019-10-06  8:51 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Ardelean, Alexandru, lars, kernel-janitors, Popa, Stefan Serban,
	linux-iio, linux-kernel, Hennerich, Michael, pmeerw, knaack.h

On Thu, 26 Sep 2019 14:36:30 +0300
Dan Carpenter <dan.carpenter@oracle.com> wrote:

> On Thu, Sep 26, 2019 at 11:06:39AM +0000, Ardelean, Alexandru wrote:
> > On Thu, 2019-09-26 at 11:10 +0300, Dan Carpenter wrote:  
> > > [External]
> > > 
> > > The "t" variable is unsigned so it can't be less than zero.  We really
> > > are just trying to prevent divide by zero bugs so just checking against
> > > zero is sufficient.

I'm not sure that true.  It if were signed we'd be detecting that the
input from userspace was negative.  That is clearly garbage for a frequency
control..  I'll hazard a guess that it was intended to catch that
as well as the divide by 0 case. 

This would be clearer if we first checked that val and val2 are both not
negative, then did the zero test on t.

If people agree, anyone want to spin a patch to do that?

Thanks,

Jonathan

> > > 
> > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > > ---
> > >  drivers/iio/imu/adis16480.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/iio/imu/adis16480.c b/drivers/iio/imu/adis16480.c
> > > index b99d73887c9f..e144e567675d 100644
> > > --- a/drivers/iio/imu/adis16480.c
> > > +++ b/drivers/iio/imu/adis16480.c
> > > @@ -318,7 +318,7 @@ static int adis16480_set_freq(struct iio_dev
> > > *indio_dev, int val, int val2)
> > >  	unsigned int t, reg;  
> > 
> > I would just change the type of "t" to "int".
> > Especially, since "val" & "val2" are "int".
> >   
> 
> Yeah, but negatives or high values are basically fine.  We only care
> zero.
> 
> regards,
> dan carpenter
> 


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

* Re: [PATCH] iio: imu: adis16480: clean up a condition
  2019-10-06  8:51     ` Jonathan Cameron
@ 2019-10-06 18:14       ` Dan Carpenter
  2019-10-07  9:21         ` Jonathan Cameron
  0 siblings, 1 reply; 7+ messages in thread
From: Dan Carpenter @ 2019-10-06 18:14 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Ardelean, Alexandru, lars, kernel-janitors, Popa, Stefan Serban,
	linux-iio, linux-kernel, Hennerich, Michael, pmeerw, knaack.h

On Sun, Oct 06, 2019 at 09:51:33AM +0100, Jonathan Cameron wrote:
> On Thu, 26 Sep 2019 14:36:30 +0300
> Dan Carpenter <dan.carpenter@oracle.com> wrote:
> 
> > On Thu, Sep 26, 2019 at 11:06:39AM +0000, Ardelean, Alexandru wrote:
> > > On Thu, 2019-09-26 at 11:10 +0300, Dan Carpenter wrote:  
> > > > [External]
> > > > 
> > > > The "t" variable is unsigned so it can't be less than zero.  We really
> > > > are just trying to prevent divide by zero bugs so just checking against
> > > > zero is sufficient.
> 
> I'm not sure that true.  It if were signed we'd be detecting that the
> input from userspace was negative.

It does a really bad job of that though so it raises more questions than
answers.  Maybe just one of the parameters is negative or maybe the
multiply or the addition overflowed?  Should scenarios those be checked?

It turns out none of those situations matter, only divide by zero needs
to be checked.

regards,
dan carpenter





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

* Re: [PATCH] iio: imu: adis16480: clean up a condition
  2019-10-06 18:14       ` Dan Carpenter
@ 2019-10-07  9:21         ` Jonathan Cameron
  2019-10-07 14:18           ` Dan Carpenter
  0 siblings, 1 reply; 7+ messages in thread
From: Jonathan Cameron @ 2019-10-07  9:21 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Jonathan Cameron, Ardelean, Alexandru, lars, kernel-janitors,
	Popa, Stefan Serban, linux-iio, linux-kernel, Hennerich, Michael,
	pmeerw, knaack.h

On Sun, 6 Oct 2019 21:14:40 +0300
Dan Carpenter <dan.carpenter@oracle.com> wrote:

> On Sun, Oct 06, 2019 at 09:51:33AM +0100, Jonathan Cameron wrote:
> > On Thu, 26 Sep 2019 14:36:30 +0300
> > Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >   
> > > On Thu, Sep 26, 2019 at 11:06:39AM +0000, Ardelean, Alexandru wrote:  
> > > > On Thu, 2019-09-26 at 11:10 +0300, Dan Carpenter wrote:    
> > > > > [External]
> > > > > 
> > > > > The "t" variable is unsigned so it can't be less than zero.  We really
> > > > > are just trying to prevent divide by zero bugs so just checking against
> > > > > zero is sufficient.  
> > 
> > I'm not sure that true.  It if were signed we'd be detecting that the
> > input from userspace was negative.  
> 
> It does a really bad job of that though so it raises more questions than
> answers.  Maybe just one of the parameters is negative or maybe the
> multiply or the addition overflowed?  Should scenarios those be checked?
> 
> It turns out none of those situations matter, only divide by zero needs
> to be checked.

It isn't being nearly paranoid enough. Either val or val2 being
negative is a reason to fault out.  Divide by zero needs handling after
that.  Obviously divide by zero is the only one that causes a crash but
negatives are going to cause rather 'unexpected' results.

What fun.

Jonathan


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

* Re: [PATCH] iio: imu: adis16480: clean up a condition
  2019-10-07  9:21         ` Jonathan Cameron
@ 2019-10-07 14:18           ` Dan Carpenter
  0 siblings, 0 replies; 7+ messages in thread
From: Dan Carpenter @ 2019-10-07 14:18 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Jonathan Cameron, Ardelean, Alexandru, lars, kernel-janitors,
	Popa, Stefan Serban, linux-iio, linux-kernel, Hennerich, Michael,
	pmeerw, knaack.h

On Mon, Oct 07, 2019 at 10:21:07AM +0100, Jonathan Cameron wrote:
> On Sun, 6 Oct 2019 21:14:40 +0300
> Dan Carpenter <dan.carpenter@oracle.com> wrote:
> 
> > On Sun, Oct 06, 2019 at 09:51:33AM +0100, Jonathan Cameron wrote:
> > > On Thu, 26 Sep 2019 14:36:30 +0300
> > > Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > >   
> > > > On Thu, Sep 26, 2019 at 11:06:39AM +0000, Ardelean, Alexandru wrote:  
> > > > > On Thu, 2019-09-26 at 11:10 +0300, Dan Carpenter wrote:    
> > > > > > [External]
> > > > > > 
> > > > > > The "t" variable is unsigned so it can't be less than zero.  We really
> > > > > > are just trying to prevent divide by zero bugs so just checking against
> > > > > > zero is sufficient.  
> > > 
> > > I'm not sure that true.  It if were signed we'd be detecting that the
> > > input from userspace was negative.  
> > 
> > It does a really bad job of that though so it raises more questions than
> > answers.  Maybe just one of the parameters is negative or maybe the
> > multiply or the addition overflowed?  Should scenarios those be checked?
> > 
> > It turns out none of those situations matter, only divide by zero needs
> > to be checked.
> 
> It isn't being nearly paranoid enough. Either val or val2 being
> negative is a reason to fault out.  Divide by zero needs handling after
> that.  Obviously divide by zero is the only one that causes a crash but
> negatives are going to cause rather 'unexpected' results.

The result is that it gets capped at st->chip_info->max_dec_rate so it's
not a horrible result.  :P  I don't know if it's unexpected or not.

regards,
dan carpenter


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

end of thread, back to index

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-26  8:10 [PATCH] iio: imu: adis16480: clean up a condition Dan Carpenter
2019-09-26 11:06 ` Ardelean, Alexandru
2019-09-26 11:36   ` Dan Carpenter
2019-10-06  8:51     ` Jonathan Cameron
2019-10-06 18:14       ` Dan Carpenter
2019-10-07  9:21         ` Jonathan Cameron
2019-10-07 14:18           ` Dan Carpenter

Linux-IIO Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-iio/0 linux-iio/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-iio linux-iio/ https://lore.kernel.org/linux-iio \
		linux-iio@vger.kernel.org linux-iio@archiver.kernel.org
	public-inbox-index linux-iio

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-iio


AGPL code for this site: git clone https://public-inbox.org/ public-inbox