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@xxxxxxxxx>
> 
> From: Andrey Utkin <andrey.krieger.utkin@xxxxxxxxx>
> 
> 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@xxxxxxxxxxx>
> Signed-off-by: Andrey Utkin <andrey.krieger.utkin@xxxxxxxxx>
> Signed-off-by: Ian Abbott <abbotti@xxxxxxxxx>
> ---
> 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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/