* [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 related [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, other threads:[~2019-10-07 14:18 UTC | newest] 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
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).