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@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.

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@xxxxxxxxx>        )=-
-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-
--
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/