* [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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.