[PATCH] drivers/staging/comedi/drivers/ni_atmio16d.c: remove pointless condition

[PATCH] drivers/staging/comedi/drivers/ni_atmio16d.c: remove pointless condition

[Andrey Utkin]

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 <dcb314@hotmail.com>
Signed-off-by: Andrey Utkin <andrey.krieger.utkin@gmail.com>
---
 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

Re: [PATCH] drivers/staging/comedi/drivers/ni_atmio16d.c: remove pointless condition

[Ian Abbott]

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 <dcb314@hotmail.com>
> Signed-off-by: Andrey Utkin <andrey.krieger.utkin@gmail.com>
> ---
>   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: <abbotti@mev.co.uk>        )=-
-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-

Re: [PATCH] drivers/staging/comedi/drivers/ni_atmio16d.c: remove pointless condition

[Andrey Utkin]

2014-07-11 15:01 GMT+03:00 Ian Abbott <abbotti@mev.co.uk>:
> 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 <dcb314@hotmail.com>
>> Signed-off-by: Andrey Utkin <andrey.krieger.utkin@gmail.com>
>> ---
>>   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

Re: [PATCH] drivers/staging/comedi/drivers/ni_atmio16d.c: remove pointless condition

[Bernd Petrovitsch]

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

Re: [PATCH] drivers/staging/comedi/drivers/ni_atmio16d.c: remove pointless condition

[Dan Carpenter]

On Fri, Jul 11, 2014 at 03:30:15PM +0300, Andrey Utkin wrote:
> 2014-07-11 15:01 GMT+03:00 Ian Abbott <abbotti@mev.co.uk>:
> > 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 <dcb314@hotmail.com>
> >> Signed-off-by: Andrey Utkin <andrey.krieger.utkin@gmail.com>
> >> ---
> >>   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

[PATCH v2] From: Andrey Utkin <andrey.krieger.utkin@gmail.com>

[Ian Abbott]

From: Andrey Utkin <andrey.krieger.utkin@gmail.com>

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 <dcb314@hotmail.com>
Signed-off-by: Andrey Utkin <andrey.krieger.utkin@gmail.com>
Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
---
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

Re: [PATCH v2] From: Andrey Utkin <andrey.krieger.utkin@gmail.com>

[Ian Abbott]

On 2014-07-11 15:32, Ian Abbott wrote:
> From: Andrey Utkin <andrey.krieger.utkin@gmail.com>
>
> 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: <abbotti@mev.co.uk>        )=-
-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-

[PATCH v3] drivers/staging/comedi/drivers/ni_atmio16d.c: remove pointless condition

[Ian Abbott]

From: Andrey Utkin <andrey.krieger.utkin@gmail.com>

From: Andrey Utkin <andrey.krieger.utkin@gmail.com>

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 <dcb314@hotmail.com>
Signed-off-by: Andrey Utkin <andrey.krieger.utkin@gmail.com>
Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
---
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

Re: [PATCH v3] drivers/staging/comedi/drivers/ni_atmio16d.c: remove pointless condition

[Ian Abbott]

On 2014-07-11 15:38, Ian Abbott wrote:
> From: Andrey Utkin <andrey.krieger.utkin@gmail.com>
>
> From: Andrey Utkin <andrey.krieger.utkin@gmail.com>

Dammit!  Greg, do you want to sort that out or should I have another go?

-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@mev.co.uk>        )=-
-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-

Re: [PATCH v3] drivers/staging/comedi/drivers/ni_atmio16d.c: remove pointless condition

[Greg KH]

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 <andrey.krieger.utkin@gmail.com>
> >
> >From: Andrey Utkin <andrey.krieger.utkin@gmail.com>
> 
> 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

Re: [PATCH v3] drivers/staging/comedi/drivers/ni_atmio16d.c: remove pointless condition

[Dan Carpenter]

On Fri, Jul 11, 2014 at 03:38:30PM +0100, Ian Abbott wrote:
> From: Andrey Utkin <andrey.krieger.utkin@gmail.com>
> 
> From: Andrey Utkin <andrey.krieger.utkin@gmail.com>
> 
> 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 <dcb314@hotmail.com>
> Signed-off-by: Andrey Utkin <andrey.krieger.utkin@gmail.com>
> Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
> ---
> 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

Re: [PATCH v3] drivers/staging/comedi/drivers/ni_atmio16d.c: remove pointless condition

[Ian Abbott]

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 <andrey.krieger.utkin@gmail.com>
>>
>> From: Andrey Utkin <andrey.krieger.utkin@gmail.com>
>>
>> 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 <dcb314@hotmail.com>
>> Signed-off-by: Andrey Utkin <andrey.krieger.utkin@gmail.com>
>> Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
>> ---
>> 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: <abbotti@mev.co.uk>        )=-
-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-

Re: [PATCH v3] drivers/staging/comedi/drivers/ni_atmio16d.c: remove pointless condition

[Dan Carpenter]

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 <andrey.krieger.utkin@gmail.com>
> >>
> >>From: Andrey Utkin <andrey.krieger.utkin@gmail.com>
> >>
> >>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 <dcb314@hotmail.com>
> >>Signed-off-by: Andrey Utkin <andrey.krieger.utkin@gmail.com>
> >>Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
> >>---
> >>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

Re: [PATCH v3] drivers/staging/comedi/drivers/ni_atmio16d.c: remove pointless condition

[Andrey Utkin]

2014-07-11 17:38 GMT+03:00 Ian Abbott <abbotti@mev.co.uk>:
> Signed-off-by: Andrey Utkin <andrey.krieger.utkin@gmail.com>

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

Re: [PATCH v3] drivers/staging/comedi/drivers/ni_atmio16d.c: remove pointless condition

[Dan Carpenter]

On Fri, Jul 11, 2014 at 08:54:01PM +0300, Andrey Utkin wrote:
> 2014-07-11 17:38 GMT+03:00 Ian Abbott <abbotti@mev.co.uk>:
> > Signed-off-by: Andrey Utkin <andrey.krieger.utkin@gmail.com>
> 
> 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