| Message ID | 200905122058.n4CKwj2I004399@imap1.linux-foundation.org (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
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
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
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 -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;