drivers: Probable misuses of ||

drivers: Probable misuses of ||

[Joe Perches]

Likely these should be && not ||

drivers/scsi/FlashPoint.c:				if(bit_cnt != 0 || bit_cnt != 8)
drivers/media/dvb/dvb-usb/it913x.c:		if (ret == 0 || ret != -EBUSY || ret != -ETIMEDOUT)
drivers/media/dvb/dvb-usb/it913x.c:		if (ret == 0 || ret != -EBUSY || ret != -ETIMEDOUT)
drivers/media/rc/fintek-cir.c:	if ((chip != 0x0408) || (chip != 0x0804))

Re: drivers: Probable misuses of ||

[Joe Perches]

On Wed, 2012-04-04 at 16:00 -0700, Joe Perches wrote:
> Likely these should be && not ||
> 
> drivers/scsi/FlashPoint.c:				if(bit_cnt != 0 || bit_cnt != 8)
> drivers/media/dvb/dvb-usb/it913x.c:		if (ret == 0 || ret != -EBUSY || ret != -ETIMEDOUT)
> drivers/media/dvb/dvb-usb/it913x.c:		if (ret == 0 || ret != -EBUSY || ret != -ETIMEDOUT)
> drivers/media/rc/fintek-cir.c:	if ((chip != 0x0408) || (chip != 0x0804))

It's been a month.

Do you maintainers not care
about these obvious defects?

Re: drivers: Probable misuses of ||

[Mark Brown]

On Fri, May 04, 2012 at 02:54:37PM -0700, Joe Perches wrote:
> On Wed, 2012-04-04 at 16:00 -0700, Joe Perches wrote:

> > drivers/media/dvb/dvb-usb/it913x.c:		if (ret == 0 || ret != -EBUSY || ret != -ETIMEDOUT)
> > drivers/media/dvb/dvb-usb/it913x.c:		if (ret == 0 || ret != -EBUSY || ret != -ETIMEDOUT)
> > drivers/media/rc/fintek-cir.c:	if ((chip != 0x0408) || (chip != 0x0804))

> It's been a month.

> Do you maintainers not care
> about these obvious defects?

It'd probably help if you were to supply more analysis as to what the
issues you think you're seeing are - in the cases quoted above
(especially the last one) there's no obvious bug without further
analysis.

Re: drivers: Probable misuses of ||

[Joe Perches]

On Fri, 2012-05-04 at 23:02 +0100, Mark Brown wrote:
> On Fri, May 04, 2012 at 02:54:37PM -0700, Joe Perches wrote:
> > On Wed, 2012-04-04 at 16:00 -0700, Joe Perches wrote:
> 
> > > drivers/media/dvb/dvb-usb/it913x.c:		if (ret == 0 || ret != -EBUSY || ret != -ETIMEDOUT)
> > > drivers/media/dvb/dvb-usb/it913x.c:		if (ret == 0 || ret != -EBUSY || ret != -ETIMEDOUT)
> > > drivers/media/rc/fintek-cir.c:	if ((chip != 0x0408) || (chip != 0x0804))
> 
> > It's been a month.
> 
> > Do you maintainers not care
> > about these obvious defects?
> 
> It'd probably help if you were to supply more analysis as to what the
> issues you think you're seeing are - in the cases quoted above
> (especially the last one) there's no obvious bug without further
> analysis.

Likely the || should be &&.

	if (val != foo || test != bar)

is always true when foo != bar

Re: drivers: Probable misuses of ||

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 856 bytes --]

On Fri, May 04, 2012 at 03:09:38PM -0700, Joe Perches wrote:
> On Fri, 2012-05-04 at 23:02 +0100, Mark Brown wrote:
> > On Fri, May 04, 2012 at 02:54:37PM -0700, Joe Perches wrote:

> > > > drivers/media/rc/fintek-cir.c:	if ((chip != 0x0408) || (chip != 0x0804))

> > It'd probably help if you were to supply more analysis as to what the
> > issues you think you're seeing are - in the cases quoted above
> > (especially the last one) there's no obvious bug without further
> > analysis.

> Likely the || should be &&.

> 	if (val != foo || test != bar)

> is always true when foo != bar

Right, but you need to look at the code and explain why this is a
problem.  For example, the case I've left quoted above reads to me like
the intention is "If the chip isn't one I know doesn't like this then
let's do it" which is a perfectly sensible thing to write.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: drivers: Probable misuses of ||

[Guenter Roeck]

On Fri, May 04, 2012 at 06:13:33PM -0400, Mark Brown wrote:
> On Fri, May 04, 2012 at 03:09:38PM -0700, Joe Perches wrote:
> > On Fri, 2012-05-04 at 23:02 +0100, Mark Brown wrote:
> > > On Fri, May 04, 2012 at 02:54:37PM -0700, Joe Perches wrote:
> 
> > > > > drivers/media/rc/fintek-cir.c:	if ((chip != 0x0408) || (chip != 0x0804))
> 
> > > It'd probably help if you were to supply more analysis as to what the
> > > issues you think you're seeing are - in the cases quoted above
> > > (especially the last one) there's no obvious bug without further
> > > analysis.
> 
> > Likely the || should be &&.
> 
> > 	if (val != foo || test != bar)
> 
Guess that was meant to be 
	if (val != foo || val != bar)

> > is always true when foo != bar
> 
> Right, but you need to look at the code and explain why this is a
> problem.  For example, the case I've left quoted above reads to me like
> the intention is "If the chip isn't one I know doesn't like this then
> let's do it" which is a perfectly sensible thing to write.

I can not really follow your logic here; it is difficult for me to imagine a situation
where anything along the line of
	if (val != 1 || val != 2)
would provide value other than creating confusion. Maybe you can explain that a bit further.

Commit 83ec8225b6 ([media] fintek-cir: add support for newer chip version) suggests
that the code was added to support new chip revisions in addition to old revisions.
What it does instead is to drop support for old revisions. To accomplish that, it would
have been sufficient to replace the define of LOGICAL_DEV_CIR, ie change it from 0x05
to 0x08. That would have been a one-line patch, much simpler than the patch as it was
applied. If the idea was to drop support for old revisions in favor of new ones while
keeping a reference to the old definition, it would have been sufficient to add a comment
next to the definition of LOGICAL_DEV_CIR, without all the code complexity introduced
by the patch.

Maybe the author of the above patch might want to comment.

Thanks,
Guenter

Re: drivers: Probable misuses of ||

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1239 bytes --]

On Sat, May 05, 2012 at 09:00:42AM -0700, Guenter Roeck wrote:
> On Fri, May 04, 2012 at 06:13:33PM -0400, Mark Brown wrote:

> > Right, but you need to look at the code and explain why this is a
> > problem.  For example, the case I've left quoted above reads to me like
> > the intention is "If the chip isn't one I know doesn't like this then
> > let's do it" which is a perfectly sensible thing to write.

> I can not really follow your logic here; it is difficult for me to imagine a situation
> where anything along the line of
> 	if (val != 1 || val != 2)
> would provide value other than creating confusion. Maybe you can explain that a bit further.

Yeah, I hadn't actually read the code closely enough but it's not my
main point anyway - the main point was that the reports were very easy
to ignore because they're just a paste in of the error message with no
analysis they were very likely to just get ignored unless someone has a
particular interest in the code (which is essentially what I did - I
glanced at the report but only very briefly).

Compare this with the reports from people like Julia Lawall, for example
- they tend to be very clear.  Even simply adding "...as with || they
will always be true" would've helped.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: drivers: Probable misuses of ||

[Joe Perches]

On Mon, 2012-05-07 at 09:36 +0100, Mark Brown wrote:
> On Sat, May 05, 2012 at 09:00:42AM -0700, Guenter Roeck wrote:
> > On Fri, May 04, 2012 at 06:13:33PM -0400, Mark Brown wrote:
> 
> > > Right, but you need to look at the code and explain why this is a
> > > problem.  For example, the case I've left quoted above reads to me like
> > > the intention is "If the chip isn't one I know doesn't like this then
> > > let's do it" which is a perfectly sensible thing to write.
> 
> > I can not really follow your logic here; it is difficult for me to imagine a situation
> > where anything along the line of
> > 	if (val != 1 || val != 2)
> > would provide value other than creating confusion. Maybe you can explain that a bit further.
> 
> Yeah, I hadn't actually read the code closely enough but it's not my
> main point anyway - the main point was that the reports were very easy
> to ignore because they're just a paste in of the error message

Nope, these were the original source codes.

> with no analysis 

You did elide the "Likely the || should be &&" preface.

> they were very likely to just get ignored unless someone has a
> particular interest in the code (which is essentially what I did - I
> glanced at the report but only very briefly).

No worries, I miss things when I scan code too quickly
as well.

> Compare this with the reports from people like Julia Lawall, for example
> - they tend to be very clear.  Even simply adding "...as with || they
> will always be true" would've helped.

I think it's a pretty basic logic error that most all
lkml readers should be able to identify most of the time.

cheers, Joe

Re: drivers: Probable misuses of ||

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 955 bytes --]

On Mon, May 07, 2012 at 09:20:34AM -0700, Joe Perches wrote:
> On Mon, 2012-05-07 at 09:36 +0100, Mark Brown wrote:

> > with no analysis 

> You did elide the "Likely the || should be &&" preface.

By analysis I meant something like "...because X".

> > Compare this with the reports from people like Julia Lawall, for example
> > - they tend to be very clear.  Even simply adding "...as with || they
> > will always be true" would've helped.

> I think it's a pretty basic logic error that most all
> lkml readers should be able to identify most of the time.

"Can spot" and "will spot" are two different things; speaking as someone
on the receiving end of lots of mails it does make a big difference.
Cues in the human written text which show someone thought about the
problem help a lot with catching attention - the more effort it takes to
understand a mail the more likely it is to get skipped, especially if
there's no patch.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: drivers: Probable misuses of ||

[Alan Cox]

On Fri, 04 May 2012 14:54:37 -0700
Joe Perches <joe@perches.com> wrote:

> On Wed, 2012-04-04 at 16:00 -0700, Joe Perches wrote:
> > Likely these should be && not ||
> > 
> > drivers/scsi/FlashPoint.c:				if(bit_cnt != 0 || bit_cnt != 8)
> 
> It's been a month.
> 
> Do you maintainers not care about these obvious defects?

You are assuming the code in question has maintainers. FlashPoint hasn't
really had a maintainer since Leonard died. Also given it's been like
that for approaching twenty years and works there is a real danger in
changing it unless the patch is tested carefully - if you can find anyone
with the hardware.

It's not unknown for code to work *because* it is wrong.

Alan

Re: drivers: Probable misuses of ||

[Joe Perches]

On Fri, 2012-05-04 at 23:56 +0100, Alan Cox wrote:
> On Fri, 04 May 2012 14:54:37 -0700
> Joe Perches <joe@perches.com> wrote:
> 
> > On Wed, 2012-04-04 at 16:00 -0700, Joe Perches wrote:
> > > Likely these should be && not ||
> > > 
> > > drivers/scsi/FlashPoint.c:				if(bit_cnt != 0 || bit_cnt != 8)
> > 
> > It's been a month.
> > 
> > Do you maintainers not care about these obvious defects?
> 
> You are assuming the code in question has maintainers.

Nope.  I'm assuming the nominal subsystem maintainers
haven't bothered to look at it or are in "don't care"
mode and I am simply prompting people to look again.

Richard Weinberger replied showing that the flashpoint
bit was commented out.  Thanks Richard.

cheers, Joe

Re: drivers: Probable misuses of ||

[richard -rw- weinberger]

On Thu, Apr 5, 2012 at 1:00 AM, Joe Perches <joe@perches.com> wrote:
> Likely these should be && not ||
>
> drivers/scsi/FlashPoint.c:                              if(bit_cnt != 0 || bit_cnt != 8)

/*
                                if(bit_cnt != 0 || bit_cnt != 8)
                                {
                                        byte_cnt = 0;
                                        bit_cnt = 0;
                                        FPT_scxferc(p_port, SYNC_PTRN);
                                        FPT_scxferc(p_port, ASSIGN_ID);
                                        continue;
                                }
*/

dead code...

-- 
Thanks,
//richard

Re: drivers: Probable misuses of ||

[Mauro Carvalho Chehab]

Malcolm,

Em 04-04-2012 20:00, Joe Perches escreveu:
> Likely these should be && not ||
> 
> drivers/scsi/FlashPoint.c:				if(bit_cnt != 0 || bit_cnt != 8)

> drivers/media/dvb/dvb-usb/it913x.c:		if (ret == 0 || ret != -EBUSY || ret != -ETIMEDOUT)
> drivers/media/dvb/dvb-usb/it913x.c:		if (ret == 0 || ret != -EBUSY || ret != -ETIMEDOUT)

Could you please take a look on the above?

Thanks!
Mauro

> drivers/media/rc/fintek-cir.c:	if ((chip != 0x0408) || (chip != 0x0804))
> 
> 

[PATCH] re: drivers: Probable misuses of || - it913x.c

[Malcolm Priestley]

On Mon, 2012-05-07 at 07:45 -0300, Mauro Carvalho Chehab wrote:
> Malcolm,
> 
> Em 04-04-2012 20:00, Joe Perches escreveu:
> > Likely these should be && not ||
> > 
> > drivers/scsi/FlashPoint.c:				if(bit_cnt != 0 || bit_cnt != 8)
> 
> > drivers/media/dvb/dvb-usb/it913x.c:		if (ret == 0 || ret != -EBUSY || ret != -ETIMEDOUT)
> > drivers/media/dvb/dvb-usb/it913x.c:		if (ret == 0 || ret != -EBUSY || ret != -ETIMEDOUT)
> 
> Could you please take a look on the above?

Hmm... yes, thanks, also a bug.

Just check for -EBUSY && -ETIMEDOUT

Signed-off-by: Malcolm Priestley <tvboxspy@gmail.com>
---
 drivers/media/dvb/dvb-usb/it913x.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/dvb/dvb-usb/it913x.c b/drivers/media/dvb/dvb-usb/it913x.c
index 482d249..6244fe9 100644
--- a/drivers/media/dvb/dvb-usb/it913x.c
+++ b/drivers/media/dvb/dvb-usb/it913x.c
@@ -81,7 +81,7 @@ static int it913x_bulk_write(struct usb_device *dev,
 	for (i = 0; i < IT913X_RETRY; i++) {
 		ret = usb_bulk_msg(dev, usb_sndbulkpipe(dev, pipe),
 				snd, len , &actual_l, IT913X_SND_TIMEOUT);
-		if (ret == 0 || ret != -EBUSY || ret != -ETIMEDOUT)
+		if (ret != -EBUSY && ret != -ETIMEDOUT)
 			break;
 	}
 
@@ -99,7 +99,7 @@ static int it913x_bulk_read(struct usb_device *dev,
 	for (i = 0; i < IT913X_RETRY; i++) {
 		ret = usb_bulk_msg(dev, usb_rcvbulkpipe(dev, pipe),
 				 rev, len , &actual_l, IT913X_RCV_TIMEOUT);
-		if (ret == 0 || ret != -EBUSY || ret != -ETIMEDOUT)
+		if (ret != -EBUSY && ret != -ETIMEDOUT)
 			break;
 	}
 
-- 
1.7.9.5

Re: drivers: Probable misuses of ||

[Mauro Carvalho Chehab]

Em 04-04-2012 20:00, Joe Perches escreveu:
> Likely these should be && not ||

> drivers/media/rc/fintek-cir.c:	if ((chip != 0x0408) || (chip != 0x0804))

This one is handled by this patch:
	http://git.linuxtv.org/mchehab/media-next.git/commit/3e1fd4783b2b4ae887112e6af3ce1bb1a6be19c4

Regards,
Mauro