| Message ID | 20190926081016.GA2332@mwanda (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iio: imu: adis16480: clean up a condition | expand |
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; > > /*
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
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 >
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
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
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
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; /*
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(-)