[PATCH] iio: imu: adis16480: clean up a condition

[PATCH] iio: imu: adis16480: clean up a condition

[Dan Carpenter]

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

[PATCH] iio: imu: adis16480: clean up a condition

[Dan Carpenter]

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

Re: [PATCH] iio: imu: adis16480: clean up a condition

[Ardelean, Alexandru]

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;
>  
>  	/*

Re: [PATCH] iio: imu: adis16480: clean up a condition

[Ardelean, Alexandru]

T24gVGh1LCAyMDE5LTA5LTI2IGF0IDExOjEwICswMzAwLCBEYW4gQ2FycGVudGVyIHdyb3RlOg0K
PiBbRXh0ZXJuYWxdDQo+IA0KPiBUaGUgInQiIHZhcmlhYmxlIGlzIHVuc2lnbmVkIHNvIGl0IGNh
bid0IGJlIGxlc3MgdGhhbiB6ZXJvLiAgV2UgcmVhbGx5DQo+IGFyZSBqdXN0IHRyeWluZyB0byBw
cmV2ZW50IGRpdmlkZSBieSB6ZXJvIGJ1Z3Mgc28ganVzdCBjaGVja2luZyBhZ2FpbnN0DQo+IHpl
cm8gaXMgc3VmZmljaWVudC4NCj4gDQo+IFNpZ25lZC1vZmYtYnk6IERhbiBDYXJwZW50ZXIgPGRh
bi5jYXJwZW50ZXJAb3JhY2xlLmNvbT4NCj4gLS0tDQo+ICBkcml2ZXJzL2lpby9pbXUvYWRpczE2
NDgwLmMgfCAyICstDQo+ICAxIGZpbGUgY2hhbmdlZCwgMSBpbnNlcnRpb24oKyksIDEgZGVsZXRp
b24oLSkNCj4gDQo+IGRpZmYgLS1naXQgYS9kcml2ZXJzL2lpby9pbXUvYWRpczE2NDgwLmMgYi9k
cml2ZXJzL2lpby9pbXUvYWRpczE2NDgwLmMNCj4gaW5kZXggYjk5ZDczODg3YzlmLi5lMTQ0ZTU2
NzY3NWQgMTAwNjQ0DQo+IC0tLSBhL2RyaXZlcnMvaWlvL2ltdS9hZGlzMTY0ODAuYw0KPiArKysg
Yi9kcml2ZXJzL2lpby9pbXUvYWRpczE2NDgwLmMNCj4gQEAgLTMxOCw3ICszMTgsNyBAQCBzdGF0
aWMgaW50IGFkaXMxNjQ4MF9zZXRfZnJlcShzdHJ1Y3QgaWlvX2Rldg0KPiAqaW5kaW9fZGV2LCBp
bnQgdmFsLCBpbnQgdmFsMikNCj4gIAl1bnNpZ25lZCBpbnQgdCwgcmVnOw0KDQpJIHdvdWxkIGp1
c3QgY2hhbmdlIHRoZSB0eXBlIG9mICJ0IiB0byAiaW50Ii4NCkVzcGVjaWFsbHksIHNpbmNlICJ2
YWwiICYgInZhbDIiIGFyZSAiaW50Ii4NCg0KVGhhbmtzIGZvciB0aGUgY2F0Y2ggOikNCkFsZXgN
Cg0KPiAgDQo+ICAJdCA9ICB2YWwgKiAxMDAwICsgdmFsMiAvIDEwMDA7DQo+IC0JaWYgKHQgPD0g
MCkNCj4gKwlpZiAodCA9PSAwKQ0KPiAgCQlyZXR1cm4gLUVJTlZBTDsNCj4gIA0KPiAgCS8qDQo

Re: [PATCH] iio: imu: adis16480: clean up a condition

[Dan Carpenter]

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

Re: [PATCH] iio: imu: adis16480: clean up a condition

[Dan Carpenter]

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

Re: [PATCH] iio: imu: adis16480: clean up a condition

[Jonathan Cameron]

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
> 

Re: [PATCH] iio: imu: adis16480: clean up a condition

[Jonathan Cameron]

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
> 

Re: [PATCH] iio: imu: adis16480: clean up a condition

[Dan Carpenter]

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

Re: [PATCH] iio: imu: adis16480: clean up a condition

[Dan Carpenter]

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

Re: [PATCH] iio: imu: adis16480: clean up a condition

[Jonathan Cameron]

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

Re: [PATCH] iio: imu: adis16480: clean up a condition

[Jonathan Cameron]

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

Re: [PATCH] iio: imu: adis16480: clean up a condition

[Dan Carpenter]

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

Re: [PATCH] iio: imu: adis16480: clean up a condition

[Dan Carpenter]

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