* [PATCH] omapfb: Wrong test on unsigned?
@ 2009-10-16 18:23 Roel Kluin
2009-10-16 20:56 ` Aguirre Rodriguez, Sergio Alberto
0 siblings, 1 reply; 6+ messages in thread
From: Roel Kluin @ 2009-10-16 18:23 UTC (permalink / raw)
To: Imre Deak, linux-fbdev-devel, linux-omap, Andrew Morton
Only if the test is signed negative values can be spotted.
Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
---
Is this correct? please review.
diff --git a/drivers/video/omap/omapfb_main.c b/drivers/video/omap/omapfb_main.c
index 0d0c8c8..cc7dd93 100644
--- a/drivers/video/omap/omapfb_main.c
+++ b/drivers/video/omap/omapfb_main.c
@@ -286,7 +286,7 @@ static int _setcolreg(struct fb_info *info, u_int regno, u_int red, u_int green,
if (r != 0)
break;
- if (regno < 0) {
+ if ((int)regno < 0) {
r = -EINVAL;
break;
}
^ permalink raw reply related [flat|nested] 6+ messages in thread
* RE: [PATCH] omapfb: Wrong test on unsigned?
2009-10-16 18:23 [PATCH] omapfb: Wrong test on unsigned? Roel Kluin
@ 2009-10-16 20:56 ` Aguirre Rodriguez, Sergio Alberto
2009-10-21 15:43 ` Roel Kluin
0 siblings, 1 reply; 6+ messages in thread
From: Aguirre Rodriguez, Sergio Alberto @ 2009-10-16 20:56 UTC (permalink / raw)
To: Roel Kluin, Imre Deak, linux-fbdev-devel@lists.sourceforge.net,
linux-omap
> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org
> [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of Roel Kluin
> Sent: Friday, October 16, 2009 1:24 PM
> To: Imre Deak; linux-fbdev-devel@lists.sourceforge.net;
> linux-omap@vger.kernel.org; Andrew Morton
> Subject: [PATCH] omapfb: Wrong test on unsigned?
>
> Only if the test is signed negative values can be spotted.
>
> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> ---
> Is this correct? please review.
>
> diff --git a/drivers/video/omap/omapfb_main.c
> b/drivers/video/omap/omapfb_main.c
> index 0d0c8c8..cc7dd93 100644
> --- a/drivers/video/omap/omapfb_main.c
> +++ b/drivers/video/omap/omapfb_main.c
> @@ -286,7 +286,7 @@ static int _setcolreg(struct fb_info
> *info, u_int regno, u_int red, u_int green,
> if (r != 0)
> break;
>
> - if (regno < 0) {
> + if ((int)regno < 0) {
Hmm...
Isn't regno unsigned integer from the start?
2 things here:
- regno will never be negative.
- Casting won't make a difference in the meaning., it'll make a negative only when:
regno > ((2^32) / 2)
Which doesn't make any sense IMHO.
Isn't it?
Regards,
Sergio
> r = -EINVAL;
> break;
> }
> --
> To unsubscribe from this list: send the line "unsubscribe
> linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] omapfb: Wrong test on unsigned?
2009-10-21 15:43 ` Roel Kluin
@ 2009-10-21 15:39 ` Aguirre Rodriguez, Sergio Alberto
2009-10-22 18:22 ` Tony Lindgren
0 siblings, 1 reply; 6+ messages in thread
From: Aguirre Rodriguez, Sergio Alberto @ 2009-10-21 15:39 UTC (permalink / raw)
To: Roel Kluin
Cc: Imre Deak, linux-fbdev-devel@lists.sourceforge.net,
linux-omap@vger.kernel.org, Andrew Morton
> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org
> [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of Roel Kluin
> Sent: Wednesday, October 21, 2009 10:44 AM
> To: Aguirre Rodriguez, Sergio Alberto
> Cc: Imre Deak; linux-fbdev-devel@lists.sourceforge.net;
> linux-omap@vger.kernel.org; Andrew Morton
> Subject: Re: [PATCH] omapfb: Wrong test on unsigned?
>
> regno is unsigned so the test didn't work. If regno
> can't be used return an error.
>
> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> ---
> >> Is this correct? please review.
> >>
> >> diff --git a/drivers/video/omap/omapfb_main.c
> >> b/drivers/video/omap/omapfb_main.c
> >> index 0d0c8c8..cc7dd93 100644
> >> --- a/drivers/video/omap/omapfb_main.c
> >> +++ b/drivers/video/omap/omapfb_main.c
> >> @@ -286,7 +286,7 @@ static int _setcolreg(struct fb_info
> >> *info, u_int regno, u_int red, u_int green,
> >> if (r != 0)
> >> break;
> >>
> >> - if (regno < 0) {
> >> + if ((int)regno < 0) {
> >
> > Hmm...
> >
> > Isn't regno unsigned integer from the start?
>
> yes
>
> > 2 things here:
> >
> > - regno will never be negative.
> > - Casting won't make a difference in the meaning., it'll
> make a negative only when:
> >
> > regno > ((2^32) / 2)
> >
> > Which doesn't make any sense IMHO.
>
> I think it is strange that _setcolreg() accepts a regno that
> is invalid,
> ignores it and just returns as if all was OK. If you agree
> then you may
> like the patch below.
Yep. Looks nicer to me ;)
Acked-by: Sergio Aguirre <saaguirre@ti.com>
Regards,
Sergio
>
> > Regards,
> > Sergio
>
> Thanks,
>
> Roel
>
> diff --git a/drivers/video/omap/omapfb_main.c
> b/drivers/video/omap/omapfb_main.c
> index 0d0c8c8..4da94d0 100644
> --- a/drivers/video/omap/omapfb_main.c
> +++ b/drivers/video/omap/omapfb_main.c
> @@ -285,12 +285,6 @@ static int _setcolreg(struct fb_info
> *info, u_int regno, u_int red, u_int green,
> case OMAPFB_COLOR_RGB444:
> if (r != 0)
> break;
> -
> - if (regno < 0) {
> - r = -EINVAL;
> - break;
> - }
> -
> if (regno < 16) {
> u16 pal;
> pal = ((red >> (16 - var->red.length)) <<
> @@ -299,6 +293,8 @@ static int _setcolreg(struct fb_info
> *info, u_int regno, u_int red, u_int green,
> var->green.offset) |
> (blue >> (16 - var->blue.length));
> ((u32 *)(info->pseudo_palette))[regno] = pal;
> + } else {
> + r = -EINVAL;
> }
> break;
> default:
> --
> To unsubscribe from this list: send the line "unsubscribe
> linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] omapfb: Wrong test on unsigned?
2009-10-16 20:56 ` Aguirre Rodriguez, Sergio Alberto
@ 2009-10-21 15:43 ` Roel Kluin
2009-10-21 15:39 ` Aguirre Rodriguez, Sergio Alberto
0 siblings, 1 reply; 6+ messages in thread
From: Roel Kluin @ 2009-10-21 15:43 UTC (permalink / raw)
To: Aguirre Rodriguez, Sergio Alberto
Cc: Imre Deak, linux-fbdev-devel@lists.sourceforge.net,
linux-omap@vger.kernel.org, Andrew Morton
regno is unsigned so the test didn't work. If regno
can't be used return an error.
Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
---
>> Is this correct? please review.
>>
>> diff --git a/drivers/video/omap/omapfb_main.c
>> b/drivers/video/omap/omapfb_main.c
>> index 0d0c8c8..cc7dd93 100644
>> --- a/drivers/video/omap/omapfb_main.c
>> +++ b/drivers/video/omap/omapfb_main.c
>> @@ -286,7 +286,7 @@ static int _setcolreg(struct fb_info
>> *info, u_int regno, u_int red, u_int green,
>> if (r != 0)
>> break;
>>
>> - if (regno < 0) {
>> + if ((int)regno < 0) {
>
> Hmm...
>
> Isn't regno unsigned integer from the start?
yes
> 2 things here:
>
> - regno will never be negative.
> - Casting won't make a difference in the meaning., it'll make a negative only when:
>
> regno > ((2^32) / 2)
>
> Which doesn't make any sense IMHO.
I think it is strange that _setcolreg() accepts a regno that is invalid,
ignores it and just returns as if all was OK. If you agree then you may
like the patch below.
> Regards,
> Sergio
Thanks,
Roel
diff --git a/drivers/video/omap/omapfb_main.c b/drivers/video/omap/omapfb_main.c
index 0d0c8c8..4da94d0 100644
--- a/drivers/video/omap/omapfb_main.c
+++ b/drivers/video/omap/omapfb_main.c
@@ -285,12 +285,6 @@ static int _setcolreg(struct fb_info *info, u_int regno, u_int red, u_int green,
case OMAPFB_COLOR_RGB444:
if (r != 0)
break;
-
- if (regno < 0) {
- r = -EINVAL;
- break;
- }
-
if (regno < 16) {
u16 pal;
pal = ((red >> (16 - var->red.length)) <<
@@ -299,6 +293,8 @@ static int _setcolreg(struct fb_info *info, u_int regno, u_int red, u_int green,
var->green.offset) |
(blue >> (16 - var->blue.length));
((u32 *)(info->pseudo_palette))[regno] = pal;
+ } else {
+ r = -EINVAL;
}
break;
default:
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] omapfb: Wrong test on unsigned?
2008-10-07 19:03 DVB-T problems with MUSB Koen Kooi
@ 2009-10-21 15:43 ` Koen Kooi
0 siblings, 0 replies; 6+ messages in thread
From: Koen Kooi @ 2009-10-21 15:43 UTC (permalink / raw)
To: Roel Kluin; +Cc: linux-omap@vger.kernel.org List, Ajay Kumar Gupta
Op 21 okt 2009, om 17:43 heeft Roel Kluin het volgende geschreven:
> Hi,
>
> Ajays latest patches make MUSB behave a lot better on beagleboard,
> but my DVB stick still errors when beginning to stream data
> (scanning frequencies works):
>
> pgd = ced6c000
> [00000000] *pgd=8e2b7031, *pte=00000000, *ppte=00000000
> Internal error: Oops: 817 [#1]
> Modules linked in: mt2060 dvb_usb_dib0700 dib7000p ipv6 dib7000m
> dvb_usb dvb_core dib3000mc dibx000_common dib0070
> CPU: 0 Not tainted (2.6.27-rc7-omap1 #1)
> PC is at dma_cache_maint+0x40/0xa4
> LR is at usb_hcd_submit_urb+0x164/0x870
> pc : [<c00386cc>] lr : [<c01eae34>] psr: 20000013
> sp : cee5dca0 ip : cee5dcb0 fp : cee5dcac
> r10: ce02fca8 r9 : 00000020 r8 : 00000000
> r7 : 00000000 r6 : cf953800 r5 : ce02fca0 r4 : ffc47000
> r3 : 00000000 r2 : 00000002 r1 : ffc50a38 r0 : ffc47000
> Flags: nzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user
> Control: 00c5387f Table: 8ed6c018 DAC: 00000015
> Process w_scan (pid: 1824, stack limit = 0xcee5c2e8)
> Stack: (0xcee5dca0 to 0xcee5e000)
> dca0: cee5dd64 cee5dcb0 c01eae34 c0038698 ffffffff d8200000 d09ae000
> 0000005e
> dcc0: 60001782 cee5dcd0 c0033830 c003300c 0a000023 f03fe03a 00000006
> 00000004
> dce0: 00000007 0000001d d09ae000 0000005e c0007420 00000003 00000002
> 60001782
> dd00: cee5dd78 cee5dd1c c003b1d4 c003b1a0 20000013 ffffffff 00000001
> 00000000
> dd20: cf815eb8 c0634560 d09ae000 c0007420 cee5dd74 c00931cc 00000000
> cf86a400
> dd40: 00000002 00000020 00000000 00000010 d098606c cf9d77a4 cee5dd84
> cee5dd68
> dd60: c01eb8bc c01eacdc cf9d7a54 cf9d7a54 00000001 00000000 cee5dda4
> cee5dd88
> dd80: bf01faa4 c01eb694 cf9d772c d0872000 00000001 00000001 cee5ddc4
> cee5dda8
> dda0: bf01f120 bf01fa8c d0872000 cf9d77f8 00000000 cf9d79d0 cee5ddd4
> cee5ddc8
> ddc0: bf01f1c8 bf01f05c cee5ddf4 cee5ddd8 bf00d448 bf01f1c0 00000000
> d0986000
> dde0: 00000000 d0986000 cee5de3c cee5ddf8 bf00b694 bf00d34c 22222222
> 22222222
> de00: 22222222 22222222 cee7f5e0 403c6f2b d0986000 d0986008 d0986000
> cee5de78
> de20: d0986010 00000010 d098606c cf9d77e0 cee5de6c cee5de40 bf00ba04
> bf00b44c
> de40: cee5de78 bebf6ae4 cee5de78 0000403c 403c6f2b 00000001 cee6f0e0
> 00000000
> de60: cee5df24 cee5de70 bf00a204 bf00b838 cee7f5e0 cee7f5e0 00000000
> 00000000
> de80: 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> 00000000
> dea0: 00000000 00000000 00000000 00000000 00000005 c009d184 00000003
> cee5dec8
> dec0: 000192ec 00000002 00000026 0000a000 00000000 00000017 cf80cca0
> cf566db8
> dee0: 00000017 cee5dfb0 0000bd3c 00000101 00000001 00000000 cee5dfac
> 403c6f2b
> df00: bebf6ae4 00000004 cee7f5e0 c0033e04 cee5c000 339e1c80 cee5df3c
> cee5df28
> df20: bf00ae60 bf00a128 bf00b82c 00000000 cee5df54 cee5df40 c00aa988
> bf00ae50
> df40: cee7f5e0 bebf6ae4 cee5df7c cee5df58 c00aac08 c00aa92c cee5df94
> cee5df68
> df60: 00000004 bebf6ae4 403c6f2b cee7f5e0 cee5dfa4 cee5df80 c00aac58
> c00aa9a4
> df80: 00000005 00000000 bebf743c 00000000 000192fc 00000036 00000000
> cee5dfa8
> dfa0: c0033c80 c00aac24 bebf743c 00000000 00000004 403c6f2b bebf6ae4
> 00000005
> dfc0: bebf743c 00000000 000192fc 00000036 00000001 bebf7b1c 339e1c80
> 00000003
> dfe0: 00019188 bebf6ad8 0000a2a4 400e199c a0000010 00000004 00000000
> 00000000
> Backtrace:
> [<c003868c>] (dma_cache_maint+0x0/0xa4) from [<c01eae34>]
> (usb_hcd_submit_urb+0x164/0x870)
> [<c01eacd0>] (usb_hcd_submit_urb+0x0/0x870) from [<c01eb8bc>]
> (usb_submit_urb+0x234/0x250)
> [<c01eb688>] (usb_submit_urb+0x0/0x250) from [<bf01faa4>]
> (usb_urb_submit+0x24/0x78 [dvb_usb])
> r7:00000000 r6:00000001 r5:cf9d7a54 r4:cf9d7a54
> [<bf01fa80>] (usb_urb_submit+0x0/0x78 [dvb_usb]) from [<bf01f120>]
> (dvb_usb_ctrl_feed+0xd0/0x14c [dvb_usb])
> r7:00000001 r6:00000001 r5:d0872000 r4:cf9d772c
> [<bf01f050>] (dvb_usb_ctrl_feed+0x0/0x14c [dvb_usb]) from
> [<bf01f1c8>] (dvb_usb_start_feed+0x14/0x18 [dvb_usb])
> r7:cf9d79d0 r6:00000000 r5:cf9d77f8 r4:d0872000
> [<bf01f1b4>] (dvb_usb_start_feed+0x0/0x18 [dvb_usb]) from
> [<bf00d448>] (dmx_section_feed_start_filtering+0x108/0x150 [dvb_core])
> [<bf00d340>] (dmx_section_feed_start_filtering+0x0/0x150 [dvb_core])
> from [<bf00b694>] (dvb_dmxdev_filter_start+0x254/0x3ec [dvb_core])
> r7:d0986000 r6:00000000 r5:d0986000 r4:00000000
> [<bf00b440>] (dvb_dmxdev_filter_start+0x0/0x3ec [dvb_core]) from
> [<bf00ba04>] (dvb_demux_do_ioctl+0x1d8/0x38c [dvb_core])
> [<bf00b82c>] (dvb_demux_do_ioctl+0x0/0x38c [dvb_core]) from
> [<bf00a204>] (dvb_usercopy+0xe8/0x170 [dvb_core])
> [<bf00a11c>] (dvb_usercopy+0x0/0x170 [dvb_core]) from [<bf00ae60>]
> (dvb_demux_ioctl+0x1c/0x28 [dvb_core])
> [<bf00ae44>] (dvb_demux_ioctl+0x0/0x28 [dvb_core]) from [<c00aa988>]
> (vfs_ioctl+0x68/0x78)
> [<c00aa920>] (vfs_ioctl+0x0/0x78) from [<c00aac08>] (do_vfs_ioctl
> +0x270/0x280)
> r5:bebf6ae4 r4:cee7f5e0
> [<c00aa998>] (do_vfs_ioctl+0x0/0x280) from [<c00aac58>] (sys_ioctl
> +0x40/0x64)
> r7:cee7f5e0 r6:403c6f2b r5:bebf6ae4 r4:00000004
> [<c00aac18>] (sys_ioctl+0x0/0x64) from [<c0033c80>] (ret_fast_syscall
> +0x0/0x2c)
> r7:00000036 r6:000192fc r5:00000000 r4:bebf743c
> Code: 9a000001 e15c0003 3a000001 e3a03000 (e5833000)
> ---[ end trace 64b6e462f7b06513 ]---
>
> Is this a problem with MUSB, omap dma or the dvb stack?
I have a dvb stick using similar drivers and I'm seeing similar
problems on omap3-musb, omap3-ehci and marvell-kirkwood-ehci, so I
suspect the dvb dudes broke arm.
regards,
Koen
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] omapfb: Wrong test on unsigned?
2009-10-21 15:39 ` Aguirre Rodriguez, Sergio Alberto
@ 2009-10-22 18:22 ` Tony Lindgren
0 siblings, 0 replies; 6+ messages in thread
From: Tony Lindgren @ 2009-10-22 18:22 UTC (permalink / raw)
To: Aguirre Rodriguez, Sergio Alberto
Cc: Roel Kluin, Imre Deak, linux-fbdev-devel@lists.sourceforge.net,
linux-omap@vger.kernel.org, Andrew Morton
* Aguirre Rodriguez, Sergio Alberto <saaguirre@ti.com> [091021 08:41]:
>
>
> > -----Original Message-----
> > From: linux-omap-owner@vger.kernel.org
> > [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of Roel Kluin
> > Sent: Wednesday, October 21, 2009 10:44 AM
> > To: Aguirre Rodriguez, Sergio Alberto
> > Cc: Imre Deak; linux-fbdev-devel@lists.sourceforge.net;
> > linux-omap@vger.kernel.org; Andrew Morton
> > Subject: Re: [PATCH] omapfb: Wrong test on unsigned?
> >
> > regno is unsigned so the test didn't work. If regno
> > can't be used return an error.
> >
> > Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> > ---
> > >> Is this correct? please review.
> > >>
> > >> diff --git a/drivers/video/omap/omapfb_main.c
> > >> b/drivers/video/omap/omapfb_main.c
> > >> index 0d0c8c8..cc7dd93 100644
> > >> --- a/drivers/video/omap/omapfb_main.c
> > >> +++ b/drivers/video/omap/omapfb_main.c
> > >> @@ -286,7 +286,7 @@ static int _setcolreg(struct fb_info
> > >> *info, u_int regno, u_int red, u_int green,
> > >> if (r != 0)
> > >> break;
> > >>
> > >> - if (regno < 0) {
> > >> + if ((int)regno < 0) {
> > >
> > > Hmm...
> > >
> > > Isn't regno unsigned integer from the start?
> >
> > yes
> >
> > > 2 things here:
> > >
> > > - regno will never be negative.
> > > - Casting won't make a difference in the meaning., it'll
> > make a negative only when:
> > >
> > > regno > ((2^32) / 2)
> > >
> > > Which doesn't make any sense IMHO.
> >
> > I think it is strange that _setcolreg() accepts a regno that
> > is invalid,
> > ignores it and just returns as if all was OK. If you agree
> > then you may
> > like the patch below.
>
> Yep. Looks nicer to me ;)
>
> Acked-by: Sergio Aguirre <saaguirre@ti.com>
Looks good to me too.
Acked-by: Tony Lindgren <tony@atomide.com>
>
> Regards,
> Sergio
> >
> > > Regards,
> > > Sergio
> >
> > Thanks,
> >
> > Roel
> >
> > diff --git a/drivers/video/omap/omapfb_main.c
> > b/drivers/video/omap/omapfb_main.c
> > index 0d0c8c8..4da94d0 100644
> > --- a/drivers/video/omap/omapfb_main.c
> > +++ b/drivers/video/omap/omapfb_main.c
> > @@ -285,12 +285,6 @@ static int _setcolreg(struct fb_info
> > *info, u_int regno, u_int red, u_int green,
> > case OMAPFB_COLOR_RGB444:
> > if (r != 0)
> > break;
> > -
> > - if (regno < 0) {
> > - r = -EINVAL;
> > - break;
> > - }
> > -
> > if (regno < 16) {
> > u16 pal;
> > pal = ((red >> (16 - var->red.length)) <<
> > @@ -299,6 +293,8 @@ static int _setcolreg(struct fb_info
> > *info, u_int regno, u_int red, u_int green,
> > var->green.offset) |
> > (blue >> (16 - var->blue.length));
> > ((u32 *)(info->pseudo_palette))[regno] = pal;
> > + } else {
> > + r = -EINVAL;
> > }
> > break;
> > default:
> > --
> > To unsubscribe from this list: send the line "unsubscribe
> > linux-omap" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
> > --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2009-10-22 18:22 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-10-16 18:23 [PATCH] omapfb: Wrong test on unsigned? Roel Kluin
2009-10-16 20:56 ` Aguirre Rodriguez, Sergio Alberto
2009-10-21 15:43 ` Roel Kluin
2009-10-21 15:39 ` Aguirre Rodriguez, Sergio Alberto
2009-10-22 18:22 ` Tony Lindgren
-- strict thread matches above, loose matches on Subject: below --
2008-10-07 19:03 DVB-T problems with MUSB Koen Kooi
2009-10-21 15:43 ` [PATCH] omapfb: Wrong test on unsigned? Koen Kooi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox