diff mbox

[4/4] zoran: fix &&/|| error

Message ID 200905122058.n4CKwj2I004399@imap1.linux-foundation.org (mailing list archive)
State Superseded, archived
Headers

Commit Message

Andrew Morton May 12, 2009, 8:39 p.m. UTC 
  From: Roel Kluin <roel.kluin@gmail.com>

Fix &&/|| typo. `default_norm' can be 0 (PAL), 1 (NTSC) or 2 (SECAM),
the condition tested was impossible.

Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
Cc: Mauro Carvalho Chehab <mchehab@redhat.com>
Cc: Hans Verkuil <hverkuil@xs4all.nl>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 drivers/media/video/zoran/zoran_card.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Devin Heitmueller May 12, 2009, 9:18 p.m. UTC | #1 
On Tue, May 12, 2009 at 4:39 PM,  <akpm@linux-foundation.org> wrote:
> From: Roel Kluin <roel.kluin@gmail.com>
>
> Fix &&/|| typo. `default_norm' can be 0 (PAL), 1 (NTSC) or 2 (SECAM),
> the condition tested was impossible.
>
> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> Cc: Mauro Carvalho Chehab <mchehab@redhat.com>
> Cc: Hans Verkuil <hverkuil@xs4all.nl>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---

Hello,

Was the patch actually tested against the hardware in question?  While
I agree that it looks ok, it can result in the default logic being
inverted in some cases, which could expose other bugs and result in a
regression.

I just want to be confident that this patch was tested by somebody
with the hardware and it isn't going into the codebase because "it
obviously cannot be right".

Cheers,

Devin
Mauro Carvalho Chehab May 12, 2009, 9:35 p.m. UTC | #2 
Em Tue, 12 May 2009 17:18:20 -0400
Devin Heitmueller <dheitmueller@kernellabs.com> escreveu:

> On Tue, May 12, 2009 at 4:39 PM,  <akpm@linux-foundation.org> wrote:
> > From: Roel Kluin <roel.kluin@gmail.com>
> >
> > Fix &&/|| typo. `default_norm' can be 0 (PAL), 1 (NTSC) or 2 (SECAM),
> > the condition tested was impossible.
> >
> > Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> > Cc: Mauro Carvalho Chehab <mchehab@redhat.com>
> > Cc: Hans Verkuil <hverkuil@xs4all.nl>
> > Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> > ---
> 
> Hello,
> 
> Was the patch actually tested against the hardware in question?  While
> I agree that it looks ok, it can result in the default logic being
> inverted in some cases, which could expose other bugs and result in a
> regression.
> 
> I just want to be confident that this patch was tested by somebody
> with the hardware and it isn't going into the codebase because "it
> obviously cannot be right".

Hans and Jean worked on it. Both are at PAL area, so they won't notice such
error without a standards generator, since the default is to assume that the
signal is PAL.

With this patch, PAL should keep working, but I can't see how NTSC or SECAM
would work without it.

Anyway, this patch were already committed on our tree, and it is on my
linux-next tree since yesterday night.

Hans,

Could you please confirm that the patch is ok with your standards generator?



Cheers,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Trent Piepho May 12, 2009, 9:50 p.m. UTC | #3 
On Tue, 12 May 2009, Mauro Carvalho Chehab wrote:
> Em Tue, 12 May 2009 17:18:20 -0400
> Devin Heitmueller <dheitmueller@kernellabs.com> escreveu:
>
> > On Tue, May 12, 2009 at 4:39 PM,  <akpm@linux-foundation.org> wrote:
> > > From: Roel Kluin <roel.kluin@gmail.com>
> > >
> > > Fix &&/|| typo. `default_norm' can be 0 (PAL), 1 (NTSC) or 2 (SECAM),
> > > the condition tested was impossible.
> > >
> > > Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> > > Cc: Mauro Carvalho Chehab <mchehab@redhat.com>
> > > Cc: Hans Verkuil <hverkuil@xs4all.nl>
> > > Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> > > ---
> >
> > Hello,
> >
> > Was the patch actually tested against the hardware in question?  While
> > I agree that it looks ok, it can result in the default logic being
> > inverted in some cases, which could expose other bugs and result in a
> > regression.
> >
> > I just want to be confident that this patch was tested by somebody
> > with the hardware and it isn't going into the codebase because "it
> > obviously cannot be right".
>
> Hans and Jean worked on it. Both are at PAL area, so they won't notice such
> error without a standards generator, since the default is to assume that the
> signal is PAL.
>
> With this patch, PAL should keep working, but I can't see how NTSC or SECAM
> would work without it.

NTSC works fine without it.  The code with the bug was supposed to check
for an out of range module parameter and fix it, but it was broken and did
nothing.  There is no problem if default_norm was set to an ok value, but
if someone specified default_norm=42 then the driver wouldn't fix it and
something bad might happen.  Maybe it would read off the end of the norms
array and crash?
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff -puN drivers/media/video/zoran/zoran_card.c~zoran-fix-error drivers/media/video/zoran/zoran_card.c
--- a/drivers/media/video/zoran/zoran_card.c~zoran-fix-error
+++ a/drivers/media/video/zoran/zoran_card.c
@@ -1022,7 +1022,7 @@  zr36057_init (struct zoran *zr)
 	zr->vbuf_bytesperline = 0;
 
 	/* Avoid nonsense settings from user for default input/norm */
-	if (default_norm < 0 && default_norm > 2)
+	if (default_norm < 0 || default_norm > 2)
 		default_norm = 0;
 	if (default_norm == 0) {
 		zr->norm = V4L2_STD_PAL;