The issue was discovered with static analysis and has two instances in
this file. The code looks like this
if (x < 65536000) {
...
} else if (x < 655360000) {
...
} else if (x <= 0xffffffff /* 6553600000 */) {
...
} else if (x <= 0xffffffff /* 65536000000 */) {
...
}
The meaning of this block is to select appropriate clock frequency for
interval timer basing on "x", which is amount of time.
Notes:
1. That last condition matches previous one - that's the issue.
2. Decimal numbers in comments don't match hex numbers in expressions.
But in first case the numbers have same order, while in the second case
the hex number is the same, and the decimal one is 10 times bigger.
3. Actually type of "x" is "unsigned int", so its exact upper limit is
not obviously known.
4. There's no "else" block.
So it makes sense to make an "else" block from last "else if" case. The
code inside the block seems correct for such usage.
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=79871
Reported-by: David Binderman <[email protected]>
Signed-off-by: Andrey Utkin <[email protected]>
---
drivers/staging/comedi/drivers/ni_atmio16d.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/comedi/drivers/ni_atmio16d.c b/drivers/staging/comedi/drivers/ni_atmio16d.c
index 6ad27f5..895b56d 100644
--- a/drivers/staging/comedi/drivers/ni_atmio16d.c
+++ b/drivers/staging/comedi/drivers/ni_atmio16d.c
@@ -338,7 +338,7 @@ static int atmio16d_ai_cmd(struct comedi_device *dev,
} else if (cmd->convert_arg <= 0xffffffff /* 6553600000 */) {
base_clock = CLOCK_10_KHZ;
timer = cmd->convert_arg / 100000;
- } else if (cmd->convert_arg <= 0xffffffff /* 65536000000 */) {
+ } else {
base_clock = CLOCK_1_KHZ;
timer = cmd->convert_arg / 1000000;
}
@@ -406,7 +406,7 @@ static int atmio16d_ai_cmd(struct comedi_device *dev,
} else if (cmd->scan_begin_arg < 0xffffffff /* 6553600000 */) {
base_clock = CLOCK_10_KHZ;
timer = cmd->scan_begin_arg / 100000;
- } else if (cmd->scan_begin_arg < 0xffffffff /* 65536000000 */) {
+ } else {
base_clock = CLOCK_1_KHZ;
timer = cmd->scan_begin_arg / 1000000;
}
--
1.8.3.2
On 2014-07-11 11:13, Andrey Utkin wrote:
> The issue was discovered with static analysis and has two instances in
> this file. The code looks like this
> if (x < 65536000) {
> ...
> } else if (x < 655360000) {
> ...
> } else if (x <= 0xffffffff /* 6553600000 */) {
> ...
> } else if (x <= 0xffffffff /* 65536000000 */) {
> ...
> }
>
> The meaning of this block is to select appropriate clock frequency for
> interval timer basing on "x", which is amount of time.
>
> Notes:
> 1. That last condition matches previous one - that's the issue.
> 2. Decimal numbers in comments don't match hex numbers in expressions.
> But in first case the numbers have same order, while in the second case
> the hex number is the same, and the decimal one is 10 times bigger.
> 3. Actually type of "x" is "unsigned int", so its exact upper limit is
> not obviously known.
> 4. There's no "else" block.
>
> So it makes sense to make an "else" block from last "else if" case. The
> code inside the block seems correct for such usage.
>
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=79871
> Reported-by: David Binderman <[email protected]>
> Signed-off-by: Andrey Utkin <[email protected]>
> ---
> drivers/staging/comedi/drivers/ni_atmio16d.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/comedi/drivers/ni_atmio16d.c b/drivers/staging/comedi/drivers/ni_atmio16d.c
> index 6ad27f5..895b56d 100644
> --- a/drivers/staging/comedi/drivers/ni_atmio16d.c
> +++ b/drivers/staging/comedi/drivers/ni_atmio16d.c
> @@ -338,7 +338,7 @@ static int atmio16d_ai_cmd(struct comedi_device *dev,
> } else if (cmd->convert_arg <= 0xffffffff /* 6553600000 */) {
> base_clock = CLOCK_10_KHZ;
> timer = cmd->convert_arg / 100000;
> - } else if (cmd->convert_arg <= 0xffffffff /* 65536000000 */) {
> + } else {
> base_clock = CLOCK_1_KHZ;
> timer = cmd->convert_arg / 1000000;
> }
Since 0xffffffff is the maximum value 'cmd->convert_arg' can be, the
final else can be moved to the 'base_clock = CLOCK_10_KHZ' block and the
'base_clock = CLOCK_1_KHZ' block can be removed altogether.
> @@ -406,7 +406,7 @@ static int atmio16d_ai_cmd(struct comedi_device *dev,
> } else if (cmd->scan_begin_arg < 0xffffffff /* 6553600000 */) {
> base_clock = CLOCK_10_KHZ;
> timer = cmd->scan_begin_arg / 100000;
> - } else if (cmd->scan_begin_arg < 0xffffffff /* 65536000000 */) {
> + } else {
> base_clock = CLOCK_1_KHZ;
> timer = cmd->scan_begin_arg / 1000000;
> }
>
Same here as well for 'cmd->scan_begin_arg'. (And I think the original
code should have used '<= 0xffffffff' rather than '< 0xffffffff'.)
--
-=( Ian Abbott @ MEV Ltd. E-mail: <[email protected]> )=-
-=( Tel: +44 (0)161 477 1898 FAX: +44 (0)161 718 3587 )=-
2014-07-11 15:01 GMT+03:00 Ian Abbott <[email protected]>:
> On 2014-07-11 11:13, Andrey Utkin wrote:
>>
>> The issue was discovered with static analysis and has two instances in
>> this file. The code looks like this
>> if (x < 65536000) {
>> ...
>> } else if (x < 655360000) {
>> ...
>> } else if (x <= 0xffffffff /* 6553600000 */) {
>> ...
>> } else if (x <= 0xffffffff /* 65536000000 */) {
>> ...
>> }
>>
>> The meaning of this block is to select appropriate clock frequency for
>> interval timer basing on "x", which is amount of time.
>>
>> Notes:
>> 1. That last condition matches previous one - that's the issue.
>> 2. Decimal numbers in comments don't match hex numbers in expressions.
>> But in first case the numbers have same order, while in the second case
>> the hex number is the same, and the decimal one is 10 times bigger.
>> 3. Actually type of "x" is "unsigned int", so its exact upper limit is
>> not obviously known.
>> 4. There's no "else" block.
>>
>> So it makes sense to make an "else" block from last "else if" case. The
>> code inside the block seems correct for such usage.
>>
>> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=79871
>> Reported-by: David Binderman <[email protected]>
>> Signed-off-by: Andrey Utkin <[email protected]>
>> ---
>> drivers/staging/comedi/drivers/ni_atmio16d.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/staging/comedi/drivers/ni_atmio16d.c
>> b/drivers/staging/comedi/drivers/ni_atmio16d.c
>> index 6ad27f5..895b56d 100644
>> --- a/drivers/staging/comedi/drivers/ni_atmio16d.c
>> +++ b/drivers/staging/comedi/drivers/ni_atmio16d.c
>> @@ -338,7 +338,7 @@ static int atmio16d_ai_cmd(struct comedi_device *dev,
>> } else if (cmd->convert_arg <= 0xffffffff /* 6553600000 */) {
>> base_clock = CLOCK_10_KHZ;
>> timer = cmd->convert_arg / 100000;
>> - } else if (cmd->convert_arg <= 0xffffffff /* 65536000000 */) {
>> + } else {
>> base_clock = CLOCK_1_KHZ;
>> timer = cmd->convert_arg / 1000000;
>> }
>
>
> Since 0xffffffff is the maximum value 'cmd->convert_arg' can be,
Could you please substantiate this? I see that convert_arg has type
"unsigned int" which may be 8 bytes on 64-bit platform. I haven't
tracked where from actual values come, if the values are limited to 4
bytes, maybe we need to set the type to u32.
> the final
> else can be moved to the 'base_clock = CLOCK_10_KHZ' block and the
> 'base_clock = CLOCK_1_KHZ' block can be removed altogether.
Feel free to prepare such patch, i'm not really keen on the subject.
--
Andrey Utkin
Hi!
On Fre, 2014-07-11 at 15:30 +0300, Andrey Utkin wrote:
[...]
> Could you please substantiate this? I see that convert_arg has type
> "unsigned int" which may be 8 bytes on 64-bit platform. I haven't
At least in the x86_64 world, "unsigned int" has 32bit.
TTBOMK, it is similar on all other 64bit - otherwise there is no way to
address 32bit ("short int" is usually 16 bit).
Bernd
--
"I dislike type abstraction if it has no real reason. And saving
on typing is not a good reason - if your typing speed is the main
issue when you're coding, you're doing something seriously wrong."
- Linus Torvalds
On Fri, Jul 11, 2014 at 03:30:15PM +0300, Andrey Utkin wrote:
> 2014-07-11 15:01 GMT+03:00 Ian Abbott <[email protected]>:
> > On 2014-07-11 11:13, Andrey Utkin wrote:
> >>
> >> The issue was discovered with static analysis and has two instances in
> >> this file. The code looks like this
> >> if (x < 65536000) {
> >> ...
> >> } else if (x < 655360000) {
> >> ...
> >> } else if (x <= 0xffffffff /* 6553600000 */) {
> >> ...
> >> } else if (x <= 0xffffffff /* 65536000000 */) {
> >> ...
> >> }
> >>
> >> The meaning of this block is to select appropriate clock frequency for
> >> interval timer basing on "x", which is amount of time.
> >>
> >> Notes:
> >> 1. That last condition matches previous one - that's the issue.
> >> 2. Decimal numbers in comments don't match hex numbers in expressions.
> >> But in first case the numbers have same order, while in the second case
> >> the hex number is the same, and the decimal one is 10 times bigger.
> >> 3. Actually type of "x" is "unsigned int", so its exact upper limit is
> >> not obviously known.
> >> 4. There's no "else" block.
> >>
> >> So it makes sense to make an "else" block from last "else if" case. The
> >> code inside the block seems correct for such usage.
> >>
> >> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=79871
> >> Reported-by: David Binderman <[email protected]>
> >> Signed-off-by: Andrey Utkin <[email protected]>
> >> ---
> >> drivers/staging/comedi/drivers/ni_atmio16d.c | 4 ++--
> >> 1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/staging/comedi/drivers/ni_atmio16d.c
> >> b/drivers/staging/comedi/drivers/ni_atmio16d.c
> >> index 6ad27f5..895b56d 100644
> >> --- a/drivers/staging/comedi/drivers/ni_atmio16d.c
> >> +++ b/drivers/staging/comedi/drivers/ni_atmio16d.c
> >> @@ -338,7 +338,7 @@ static int atmio16d_ai_cmd(struct comedi_device *dev,
> >> } else if (cmd->convert_arg <= 0xffffffff /* 6553600000 */) {
> >> base_clock = CLOCK_10_KHZ;
> >> timer = cmd->convert_arg / 100000;
> >> - } else if (cmd->convert_arg <= 0xffffffff /* 65536000000 */) {
> >> + } else {
> >> base_clock = CLOCK_1_KHZ;
> >> timer = cmd->convert_arg / 1000000;
> >> }
> >
> >
> > Since 0xffffffff is the maximum value 'cmd->convert_arg' can be,
>
> Could you please substantiate this? I see that convert_arg has type
> "unsigned int" which may be 8 bytes on 64-bit platform.
No. On linux unsigned int is always 32 bits.
regards,
dan carpenter
From: Andrey Utkin <[email protected]>
drivers/staging/comedi/drivers/ni_atmio16d.c: remove pointless condition
The issue was discovered with static analysis and has two instances in
this file. The code looks like this
if (x < 65536000) {
...
} else if (x < 655360000) {
...
} else if (x <= 0xffffffff /* 6553600000 */) {
...
} else if (x <= 0xffffffff /* 65536000000 */) {
...
}
The meaning of this block is to select appropriate clock frequency for
interval timer basing on "x", which is amount of time.
Notes:
1. That last condition matches previous one - that's the issue.
2. Decimal numbers in comments don't match hex numbers in expressions.
But in first case the numbers have same order, while in the second case
the hex number is the same, and the decimal one is 10 times bigger.
3. Actually type of "x" is "unsigned int", so its exact upper limit is
not obviously known.
4. There's no "else" block.
So it makes sense to make an "else" block from last "else if" case. The
code inside the block seems correct for such usage.
[ Actually, get rid of the final "else if" case and change the
next-to-last "else if" case to an "else" as the upper limit of "x" _is_
known to be 0xffffffff (UINT_MAX), which is less than 6553600000 -- Ian ]
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=79871
Reported-by: David Binderman <[email protected]>
Signed-off-by: Andrey Utkin <[email protected]>
Signed-off-by: Ian Abbott <[email protected]>
---
v2: Removed final "else if" block and changed preceding "else if" block
to "else" block as the condition is always true due to limited range of
"unsigned int". -- Ian
---
drivers/staging/comedi/drivers/ni_atmio16d.c | 10 ++--------
1 file changed, 2 insertions(+), 8 deletions(-)
diff --git a/drivers/staging/comedi/drivers/ni_atmio16d.c b/drivers/staging/comedi/drivers/ni_atmio16d.c
index 6ad27f5..b1b4744 100644
--- a/drivers/staging/comedi/drivers/ni_atmio16d.c
+++ b/drivers/staging/comedi/drivers/ni_atmio16d.c
@@ -335,12 +335,9 @@ static int atmio16d_ai_cmd(struct comedi_device *dev,
} else if (cmd->convert_arg < 655360000) {
base_clock = CLOCK_100_KHZ;
timer = cmd->convert_arg / 10000;
- } else if (cmd->convert_arg <= 0xffffffff /* 6553600000 */) {
+ } else /* cmd->convert_arg < 6553600000 */ {
base_clock = CLOCK_10_KHZ;
timer = cmd->convert_arg / 100000;
- } else if (cmd->convert_arg <= 0xffffffff /* 65536000000 */) {
- base_clock = CLOCK_1_KHZ;
- timer = cmd->convert_arg / 1000000;
}
outw(0xFF03, dev->iobase + AM9513A_COM_REG);
outw(base_clock, dev->iobase + AM9513A_DATA_REG);
@@ -403,12 +400,9 @@ static int atmio16d_ai_cmd(struct comedi_device *dev,
} else if (cmd->scan_begin_arg < 655360000) {
base_clock = CLOCK_100_KHZ;
timer = cmd->scan_begin_arg / 10000;
- } else if (cmd->scan_begin_arg < 0xffffffff /* 6553600000 */) {
+ } else /* cmd->scan_begin_arg < 6553600000 */ {
base_clock = CLOCK_10_KHZ;
timer = cmd->scan_begin_arg / 100000;
- } else if (cmd->scan_begin_arg < 0xffffffff /* 65536000000 */) {
- base_clock = CLOCK_1_KHZ;
- timer = cmd->scan_begin_arg / 1000000;
}
outw(0xFF02, dev->iobase + AM9513A_COM_REG);
outw(base_clock, dev->iobase + AM9513A_DATA_REG);
--
2.0.0
On 2014-07-11 15:32, Ian Abbott wrote:
> From: Andrey Utkin <[email protected]>
>
> drivers/staging/comedi/drivers/ni_atmio16d.c: remove pointless condition
Sorry, I messed up the subject line. Let me try that again!
--
-=( Ian Abbott @ MEV Ltd. E-mail: <[email protected]> )=-
-=( Tel: +44 (0)161 477 1898 FAX: +44 (0)161 718 3587 )=-
From: Andrey Utkin <[email protected]>
From: Andrey Utkin <[email protected]>
The issue was discovered with static analysis and has two instances in
this file. The code looks like this
if (x < 65536000) {
...
} else if (x < 655360000) {
...
} else if (x <= 0xffffffff /* 6553600000 */) {
...
} else if (x <= 0xffffffff /* 65536000000 */) {
...
}
The meaning of this block is to select appropriate clock frequency for
interval timer basing on "x", which is amount of time.
Notes:
1. That last condition matches previous one - that's the issue.
2. Decimal numbers in comments don't match hex numbers in expressions.
But in first case the numbers have same order, while in the second case
the hex number is the same, and the decimal one is 10 times bigger.
3. Actually type of "x" is "unsigned int", so its exact upper limit is
not obviously known.
4. There's no "else" block.
So it makes sense to make an "else" block from last "else if" case. The
code inside the block seems correct for such usage.
[ Actually, get rid of the final "else if" case and change the
next-to-last "else if" case to an "else" as the upper limit of "x" _is_
known to be 0xffffffff (UINT_MAX), which is less than 6553600000 -- Ian ]
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=79871
Reported-by: David Binderman <[email protected]>
Signed-off-by: Andrey Utkin <[email protected]>
Signed-off-by: Ian Abbott <[email protected]>
---
v2: Removed final "else if" block and changed preceding "else if" block
to "else" block as the condition is always true due to limited range of
"unsigned int". -- Ian
v3: Corrected subject line I messed up in v2. -- Ian
---
drivers/staging/comedi/drivers/ni_atmio16d.c | 10 ++--------
1 file changed, 2 insertions(+), 8 deletions(-)
diff --git a/drivers/staging/comedi/drivers/ni_atmio16d.c b/drivers/staging/comedi/drivers/ni_atmio16d.c
index 6ad27f5..b1b4744 100644
--- a/drivers/staging/comedi/drivers/ni_atmio16d.c
+++ b/drivers/staging/comedi/drivers/ni_atmio16d.c
@@ -335,12 +335,9 @@ static int atmio16d_ai_cmd(struct comedi_device *dev,
} else if (cmd->convert_arg < 655360000) {
base_clock = CLOCK_100_KHZ;
timer = cmd->convert_arg / 10000;
- } else if (cmd->convert_arg <= 0xffffffff /* 6553600000 */) {
+ } else /* cmd->convert_arg < 6553600000 */ {
base_clock = CLOCK_10_KHZ;
timer = cmd->convert_arg / 100000;
- } else if (cmd->convert_arg <= 0xffffffff /* 65536000000 */) {
- base_clock = CLOCK_1_KHZ;
- timer = cmd->convert_arg / 1000000;
}
outw(0xFF03, dev->iobase + AM9513A_COM_REG);
outw(base_clock, dev->iobase + AM9513A_DATA_REG);
@@ -403,12 +400,9 @@ static int atmio16d_ai_cmd(struct comedi_device *dev,
} else if (cmd->scan_begin_arg < 655360000) {
base_clock = CLOCK_100_KHZ;
timer = cmd->scan_begin_arg / 10000;
- } else if (cmd->scan_begin_arg < 0xffffffff /* 6553600000 */) {
+ } else /* cmd->scan_begin_arg < 6553600000 */ {
base_clock = CLOCK_10_KHZ;
timer = cmd->scan_begin_arg / 100000;
- } else if (cmd->scan_begin_arg < 0xffffffff /* 65536000000 */) {
- base_clock = CLOCK_1_KHZ;
- timer = cmd->scan_begin_arg / 1000000;
}
outw(0xFF02, dev->iobase + AM9513A_COM_REG);
outw(base_clock, dev->iobase + AM9513A_DATA_REG);
--
2.0.0
On 2014-07-11 15:38, Ian Abbott wrote:
> From: Andrey Utkin <[email protected]>
>
> From: Andrey Utkin <[email protected]>
Dammit! Greg, do you want to sort that out or should I have another go?
--
-=( Ian Abbott @ MEV Ltd. E-mail: <[email protected]> )=-
-=( Tel: +44 (0)161 477 1898 FAX: +44 (0)161 718 3587 )=-
On Fri, Jul 11, 2014 at 03:38:30PM +0100, Ian Abbott wrote:
> From: Andrey Utkin <[email protected]>
>
> From: Andrey Utkin <[email protected]>
>
> The issue was discovered with static analysis and has two instances in
> this file. The code looks like this
> if (x < 65536000) {
> ...
> } else if (x < 655360000) {
> ...
> } else if (x <= 0xffffffff /* 6553600000 */) {
> ...
> } else if (x <= 0xffffffff /* 65536000000 */) {
> ...
> }
>
> The meaning of this block is to select appropriate clock frequency for
> interval timer basing on "x", which is amount of time.
>
> Notes:
> 1. That last condition matches previous one - that's the issue.
> 2. Decimal numbers in comments don't match hex numbers in expressions.
> But in first case the numbers have same order, while in the second case
> the hex number is the same, and the decimal one is 10 times bigger.
> 3. Actually type of "x" is "unsigned int", so its exact upper limit is
> not obviously known.
> 4. There's no "else" block.
>
> So it makes sense to make an "else" block from last "else if" case. The
> code inside the block seems correct for such usage.
>
> [ Actually, get rid of the final "else if" case and change the
> next-to-last "else if" case to an "else" as the upper limit of "x" _is_
> known to be 0xffffffff (UINT_MAX), which is less than 6553600000 -- Ian ]
>
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=79871
> Reported-by: David Binderman <[email protected]>
> Signed-off-by: Andrey Utkin <[email protected]>
> Signed-off-by: Ian Abbott <[email protected]>
> ---
> v2: Removed final "else if" block and changed preceding "else if" block
> to "else" block as the condition is always true due to limited range of
> "unsigned int". -- Ian
> v3: Corrected subject line I messed up in v2. -- Ian
> ---
> drivers/staging/comedi/drivers/ni_atmio16d.c | 10 ++--------
> 1 file changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/staging/comedi/drivers/ni_atmio16d.c b/drivers/staging/comedi/drivers/ni_atmio16d.c
> index 6ad27f5..b1b4744 100644
> --- a/drivers/staging/comedi/drivers/ni_atmio16d.c
> +++ b/drivers/staging/comedi/drivers/ni_atmio16d.c
> @@ -335,12 +335,9 @@ static int atmio16d_ai_cmd(struct comedi_device *dev,
> } else if (cmd->convert_arg < 655360000) {
> base_clock = CLOCK_100_KHZ;
> timer = cmd->convert_arg / 10000;
> - } else if (cmd->convert_arg <= 0xffffffff /* 6553600000 */) {
> + } else /* cmd->convert_arg < 6553600000 */ {
I think the comment is meant to be /* cmd->convert_arg >= 655360000 */
There is an extra zero on 6553600000.
The comment is obvious and should be removed.
Or maybe I haven't understood the comment and in that case it is a
useless sucky comment so we should delete it?
regards,
dan carpenter
On 2014-07-11 16:34, Dan Carpenter wrote:
> On Fri, Jul 11, 2014 at 03:38:30PM +0100, Ian Abbott wrote:
>> From: Andrey Utkin <[email protected]>
>>
>> From: Andrey Utkin <[email protected]>
>>
>> The issue was discovered with static analysis and has two instances in
>> this file. The code looks like this
>> if (x < 65536000) {
>> ...
>> } else if (x < 655360000) {
>> ...
>> } else if (x <= 0xffffffff /* 6553600000 */) {
>> ...
>> } else if (x <= 0xffffffff /* 65536000000 */) {
>> ...
>> }
>>
>> The meaning of this block is to select appropriate clock frequency for
>> interval timer basing on "x", which is amount of time.
>>
>> Notes:
>> 1. That last condition matches previous one - that's the issue.
>> 2. Decimal numbers in comments don't match hex numbers in expressions.
>> But in first case the numbers have same order, while in the second case
>> the hex number is the same, and the decimal one is 10 times bigger.
>> 3. Actually type of "x" is "unsigned int", so its exact upper limit is
>> not obviously known.
>> 4. There's no "else" block.
>>
>> So it makes sense to make an "else" block from last "else if" case. The
>> code inside the block seems correct for such usage.
>>
>> [ Actually, get rid of the final "else if" case and change the
>> next-to-last "else if" case to an "else" as the upper limit of "x" _is_
>> known to be 0xffffffff (UINT_MAX), which is less than 6553600000 -- Ian ]
>>
>> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=79871
>> Reported-by: David Binderman <[email protected]>
>> Signed-off-by: Andrey Utkin <[email protected]>
>> Signed-off-by: Ian Abbott <[email protected]>
>> ---
>> v2: Removed final "else if" block and changed preceding "else if" block
>> to "else" block as the condition is always true due to limited range of
>> "unsigned int". -- Ian
>> v3: Corrected subject line I messed up in v2. -- Ian
>> ---
>> drivers/staging/comedi/drivers/ni_atmio16d.c | 10 ++--------
>> 1 file changed, 2 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/staging/comedi/drivers/ni_atmio16d.c b/drivers/staging/comedi/drivers/ni_atmio16d.c
>> index 6ad27f5..b1b4744 100644
>> --- a/drivers/staging/comedi/drivers/ni_atmio16d.c
>> +++ b/drivers/staging/comedi/drivers/ni_atmio16d.c
>> @@ -335,12 +335,9 @@ static int atmio16d_ai_cmd(struct comedi_device *dev,
>> } else if (cmd->convert_arg < 655360000) {
>> base_clock = CLOCK_100_KHZ;
>> timer = cmd->convert_arg / 10000;
>> - } else if (cmd->convert_arg <= 0xffffffff /* 6553600000 */) {
>> + } else /* cmd->convert_arg < 6553600000 */ {
>
> I think the comment is meant to be /* cmd->convert_arg >= 655360000 */
> There is an extra zero on 6553600000.
That's not what I was intending to convey. The preceding chain of
if/else if was checking if cmd->convert_arg < 65536000, else if
cmd->convert_arg < 655360000, and I was trying to convey that the final
else part was valid for all remaining values less than 6553600000, which
in fact is all remaining unsigned int values. (Obviously, I failed to
convey this meaning to everyone!)
--
-=( Ian Abbott @ MEV Ltd. E-mail: <[email protected]> )=-
-=( Tel: +44 (0)161 477 1898 FAX: +44 (0)161 718 3587 )=-
On Fri, Jul 11, 2014 at 06:06:36PM +0100, Ian Abbott wrote:
> On 2014-07-11 16:34, Dan Carpenter wrote:
> >On Fri, Jul 11, 2014 at 03:38:30PM +0100, Ian Abbott wrote:
> >>From: Andrey Utkin <[email protected]>
> >>
> >>From: Andrey Utkin <[email protected]>
> >>
> >>The issue was discovered with static analysis and has two instances in
> >>this file. The code looks like this
> >>if (x < 65536000) {
> >> ...
> >>} else if (x < 655360000) {
> >> ...
> >>} else if (x <= 0xffffffff /* 6553600000 */) {
> >> ...
> >>} else if (x <= 0xffffffff /* 65536000000 */) {
> >> ...
> >>}
> >>
> >>The meaning of this block is to select appropriate clock frequency for
> >>interval timer basing on "x", which is amount of time.
> >>
> >>Notes:
> >>1. That last condition matches previous one - that's the issue.
> >>2. Decimal numbers in comments don't match hex numbers in expressions.
> >>But in first case the numbers have same order, while in the second case
> >>the hex number is the same, and the decimal one is 10 times bigger.
> >>3. Actually type of "x" is "unsigned int", so its exact upper limit is
> >>not obviously known.
> >>4. There's no "else" block.
> >>
> >>So it makes sense to make an "else" block from last "else if" case. The
> >>code inside the block seems correct for such usage.
> >>
> >>[ Actually, get rid of the final "else if" case and change the
> >>next-to-last "else if" case to an "else" as the upper limit of "x" _is_
> >>known to be 0xffffffff (UINT_MAX), which is less than 6553600000 -- Ian ]
> >>
> >>Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=79871
> >>Reported-by: David Binderman <[email protected]>
> >>Signed-off-by: Andrey Utkin <[email protected]>
> >>Signed-off-by: Ian Abbott <[email protected]>
> >>---
> >>v2: Removed final "else if" block and changed preceding "else if" block
> >>to "else" block as the condition is always true due to limited range of
> >>"unsigned int". -- Ian
> >>v3: Corrected subject line I messed up in v2. -- Ian
> >>---
> >> drivers/staging/comedi/drivers/ni_atmio16d.c | 10 ++--------
> >> 1 file changed, 2 insertions(+), 8 deletions(-)
> >>
> >>diff --git a/drivers/staging/comedi/drivers/ni_atmio16d.c b/drivers/staging/comedi/drivers/ni_atmio16d.c
> >>index 6ad27f5..b1b4744 100644
> >>--- a/drivers/staging/comedi/drivers/ni_atmio16d.c
> >>+++ b/drivers/staging/comedi/drivers/ni_atmio16d.c
> >>@@ -335,12 +335,9 @@ static int atmio16d_ai_cmd(struct comedi_device *dev,
> >> } else if (cmd->convert_arg < 655360000) {
> >> base_clock = CLOCK_100_KHZ;
> >> timer = cmd->convert_arg / 10000;
> >>- } else if (cmd->convert_arg <= 0xffffffff /* 6553600000 */) {
> >>+ } else /* cmd->convert_arg < 6553600000 */ {
> >
> >I think the comment is meant to be /* cmd->convert_arg >= 655360000 */
> >There is an extra zero on 6553600000.
>
> That's not what I was intending to convey. The preceding chain of
> if/else if was checking if cmd->convert_arg < 65536000, else if
> cmd->convert_arg < 655360000, and I was trying to convey that the
> final else part was valid for all remaining values less than
> 6553600000, which in fact is all remaining unsigned int values.
> (Obviously, I failed to convey this meaning to everyone!)
Oh. I could have figured it out if I had looked at the context maybe
instead of just in the patch. It's weird that we are saying
0x10000 x 1000 because we're mixing hex and decimal.
Anyway, ignore me... I am a bad drive by reviewer.
regards,
dan carpenter
2014-07-11 17:38 GMT+03:00 Ian Abbott <[email protected]>:
> Signed-off-by: Andrey Utkin <[email protected]>
I think it's incorrect that you have instantly placed my signoff
statement on this new patch.
Anyway, thanks for your work on the issue.
--
Andrey Utkin
On Fri, Jul 11, 2014 at 08:54:01PM +0300, Andrey Utkin wrote:
> 2014-07-11 17:38 GMT+03:00 Ian Abbott <[email protected]>:
> > Signed-off-by: Andrey Utkin <[email protected]>
>
> I think it's incorrect that you have instantly placed my signoff
> statement on this new patch.
Ian gave you credit but also wrote in the changelog that he modified the
patch from what you sent. That's pretty normal.
regards,
dan carpenter
On Fri, Jul 11, 2014 at 03:39:48PM +0100, Ian Abbott wrote:
> On 2014-07-11 15:38, Ian Abbott wrote:
> >From: Andrey Utkin <[email protected]>
> >
> >From: Andrey Utkin <[email protected]>
>
> Dammit! Greg, do you want to sort that out or should I have another go?
Heh, no worries, I can fix it up, thanks.
greg k-h