* Re: [PATCH 2.6.28 ] i810: kernel crash fix when struct fb_var_screeninfo is supplied
[not found] <02E43B9E8855E74E886358B5184B4CC9104620C0@mail2-aub1fr.esi-supinfo.com>
@ 2009-03-04 10:09 ` Jiri Kosina
2009-03-04 22:20 ` Andrew Morton
0 siblings, 1 reply; 6+ messages in thread
From: Jiri Kosina @ 2009-03-04 10:09 UTC (permalink / raw)
To: CUELLA Samuel, Andrew Morton; +Cc: adaplas, linux-kernel, trivial
On Thu, 26 Feb 2009, CUELLA Samuel wrote:
> from: Samuel CUELLA <samuel.cuella@supinfo.com>
>
> This patch prevents the kernel from being crashed by a divide-by-zero operation when supplied an incorrectly filled 'struct fb_var_screeninfo' from userland.
>
> Previously i810_main.c:1005 (i810_check_params) was using the global 'yres' symbol previously defined at i810_main.c:145
> as a module parameter value holder (i810_main.c:2174). If i810fb is compiled-in or if this param doesn't get a default value,
> this direct usage leads to a divide-by-zero at i810_main.c:1005 (i810_check_params). The patch simply replace the 'yres' global,
> perhaps undefined symbol usage by a given parameter structure lookup.
>
> This problem occurs with directfb, mplayer -vo fbdev, SDL library.
> It was also reported ( but non solved ) at : http://mail.directfb.org/pipermail/directfb-dev/2008-March/004050.html
> Sample code to reproduce :
> /*Comile with gcc crashfb.c -o crashfb*/
> #include <fcntl.h>
> #include <linux/fb.h>
> #include <stdio.h>
> #include <sys/ioctl.h>
> #include <sys/mman.h>
> #include <sys/stat.h>
> #include <string.h>
> #include <stdlib.h>
>
>
> #define FB "/dev/fb0"
>
> int main(){
> int fd;
> int rv;
> struct fb_var_screeninfo vinfo;
>
> fd = open(FB,O_RDWR);
> if( fd ){
> vinfo.xres = 800;
> vinfo.yres = 600;
> rv =ioctl(fd, FBIOPUT_VSCREENINFO, &vinfo);
> }
> return(rv);
> }
> Leads to this crash dump:
> divide error: 0000 [#1]
> last sysfs file: /sys/kernel/uevent_seqnum
> Modules linked in:
>
> Pid: 4058, comm: crashfb Not tainted (2.6.28 #4)
> EIP: 0060:[<c02558c8>] EFLAGS: 00010202 CPU: 0
> EIP is at i810fb_check_var+0x428/0x520
> EAX: 00400000 EBX: ce9d5e44 ECX: 001209a0 EDX: 00000000
> ESI: 00000020 EDI: 00000004 EBP: 00000000 ESP: ce9d5d0c
> DS: 007b ES: 007b FS: 0000 GS: 0033 SS: 0068
> Process crashfb (pid: 4058, ti=ce9d4000 task=cf8af0e0 task.ti=ce9d4000)
> Stack:
> c014f993 00000000 00000001 00000000 00000000 00400000 0000001a cf811000
> 08048268 cf81123c 00000258 00000320 ffffffed cf811015 ce9d5e45 cf811000
> c0224821 ce9d5e44 ce9b09a0 00000000 00012000 00000000 c0111a89 00000001
> Call Trace:
> [<c014f993>] handle_mm_fault+0x5c3/0x650
> [<c0224821>] fb_set_var+0x61/0x2d0
> [<c0111a89>] do_page_fault+0x3a9/0x8b0
> [<c016c935>] do_lookup+0x65/0x1a0
> [<c02257aa>] fb_ioctl+0x21a/0x3c0
> [<c014f577>] handle_mm_fault+0x1a7/0x650
> [<c0225590>] fb_ioctl+0x0/0x3c0
> [<c017077f>] vfs_ioctl+0x1f/0x70
> [<c017096c>] do_vfs_ioctl+0x5c/0x430
> [<c0111a89>] do_page_fault+0x3a9/0x8b0
> [<c0170d7d>] sys_ioctl+0x3d/0x70
> [<c0103af9>] sysenter_do_call+0x12/0x25
> Code: c0 0f 44 d0 89 54 24 04 e8 b6 5a ec ff b8 ea ff ff ff 83 c4 30 5b 5e 5f 5d c3 8b 2d ac 0e 4a c0 31 d2 89 f7 8b 44 24 14 c1 ef 03 <f7> f5 31 d2 f7 f7 3b 03 89 c7 0f 83 3c fd ff ff 89 c2 89 f1 89
> EIP: [<c02558c8>] i810fb_check_var+0x428/0x520 SS:ESP 0068:ce9d5d0c
> ---[ end trace 1840767f449d222e ]---
>
> Despite this dump says that EIP was in 'i810fb_check_var' the divide by zero truly occurs in 'i810_check_params' called by 'i810fb_check_var' (i810_main.c:1466).
>
> Signed-off-by: Samuel CUELLA <samuel.cuella@supinfo.com>
> ---
> --- linux-2.6.28/drivers/video/i810/i810_main.c.orig 2009-02-26 15:23:03.000000000 +0100
> +++ linux-2.6.28/drivers/video/i810/i810_main.c 2009-02-26 14:50:06.000000000 +0100
> @@ -993,6 +993,8 @@ static int i810_check_params(struct fb_v
> struct i810fb_par *par = info->par;
> int line_length, vidmem, mode_valid = 0, retval = 0;
> u32 vyres = var->yres_virtual, vxres = var->xres_virtual;
> + u32 yres = info->var.yres;
> +
> /*
> * Memory limit
> */
>
This is not appropriate for trivial tree. CCing akpm and lkml.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2.6.28 ] i810: kernel crash fix when struct fb_var_screeninfo is supplied
2009-03-04 10:09 ` [PATCH 2.6.28 ] i810: kernel crash fix when struct fb_var_screeninfo is supplied Jiri Kosina
@ 2009-03-04 22:20 ` Andrew Morton
2009-03-04 22:54 ` Jiri Kosina
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Morton @ 2009-03-04 22:20 UTC (permalink / raw)
To: Jiri Kosina
Cc: Samuel.CUELLA, adaplas, linux-kernel, trivial, linux-fbdev-devel
On Wed, 4 Mar 2009 11:09:21 +0100 (CET)
Jiri Kosina <jkosina@suse.cz> wrote:
> On Thu, 26 Feb 2009, CUELLA Samuel wrote:
>
> > from: Samuel CUELLA <samuel.cuella@supinfo.com>
> >
> > This patch prevents the kernel from being crashed by a divide-by-zero operation when supplied an incorrectly filled 'struct fb_var_screeninfo' from userland.
> >
> > Previously i810_main.c:1005 (i810_check_params) was using the global 'yres' symbol previously defined at i810_main.c:145
> > as a module parameter value holder (i810_main.c:2174). If i810fb is compiled-in or if this param doesn't get a default value,
> > this direct usage leads to a divide-by-zero at i810_main.c:1005 (i810_check_params). The patch simply replace the 'yres' global,
> > perhaps undefined symbol usage by a given parameter structure lookup.
> >
> > This problem occurs with directfb, mplayer -vo fbdev, SDL library.
> > It was also reported ( but non solved ) at : http://mail.directfb.org/pipermail/directfb-dev/2008-March/004050.html
> > Sample code to reproduce :
> > /*Comile with gcc crashfb.c -o crashfb*/
> > #include <fcntl.h>
> > #include <linux/fb.h>
> > #include <stdio.h>
> > #include <sys/ioctl.h>
> > #include <sys/mman.h>
> > #include <sys/stat.h>
> > #include <string.h>
> > #include <stdlib.h>
> >
> >
> > #define FB "/dev/fb0"
> >
> > int main(){
> > int fd;
> > int rv;
> > struct fb_var_screeninfo vinfo;
> >
> > fd = open(FB,O_RDWR);
> > if( fd ){
> > vinfo.xres = 800;
> > vinfo.yres = 600;
> > rv =ioctl(fd, FBIOPUT_VSCREENINFO, &vinfo);
> > }
> > return(rv);
> > }
> > Leads to this crash dump:
> > divide error: 0000 [#1]
> > last sysfs file: /sys/kernel/uevent_seqnum
> > Modules linked in:
> >
> > Pid: 4058, comm: crashfb Not tainted (2.6.28 #4)
> > EIP: 0060:[<c02558c8>] EFLAGS: 00010202 CPU: 0
> > EIP is at i810fb_check_var+0x428/0x520
> > EAX: 00400000 EBX: ce9d5e44 ECX: 001209a0 EDX: 00000000
> > ESI: 00000020 EDI: 00000004 EBP: 00000000 ESP: ce9d5d0c
> > DS: 007b ES: 007b FS: 0000 GS: 0033 SS: 0068
> > Process crashfb (pid: 4058, ti=ce9d4000 task=cf8af0e0 task.ti=ce9d4000)
> > Stack:
> > c014f993 00000000 00000001 00000000 00000000 00400000 0000001a cf811000
> > 08048268 cf81123c 00000258 00000320 ffffffed cf811015 ce9d5e45 cf811000
> > c0224821 ce9d5e44 ce9b09a0 00000000 00012000 00000000 c0111a89 00000001
> > Call Trace:
> > [<c014f993>] handle_mm_fault+0x5c3/0x650
> > [<c0224821>] fb_set_var+0x61/0x2d0
> > [<c0111a89>] do_page_fault+0x3a9/0x8b0
> > [<c016c935>] do_lookup+0x65/0x1a0
> > [<c02257aa>] fb_ioctl+0x21a/0x3c0
> > [<c014f577>] handle_mm_fault+0x1a7/0x650
> > [<c0225590>] fb_ioctl+0x0/0x3c0
> > [<c017077f>] vfs_ioctl+0x1f/0x70
> > [<c017096c>] do_vfs_ioctl+0x5c/0x430
> > [<c0111a89>] do_page_fault+0x3a9/0x8b0
> > [<c0170d7d>] sys_ioctl+0x3d/0x70
> > [<c0103af9>] sysenter_do_call+0x12/0x25
> > Code: c0 0f 44 d0 89 54 24 04 e8 b6 5a ec ff b8 ea ff ff ff 83 c4 30 5b 5e 5f 5d c3 8b 2d ac 0e 4a c0 31 d2 89 f7 8b 44 24 14 c1 ef 03 <f7> f5 31 d2 f7 f7 3b 03 89 c7 0f 83 3c fd ff ff 89 c2 89 f1 89
> > EIP: [<c02558c8>] i810fb_check_var+0x428/0x520 SS:ESP 0068:ce9d5d0c
> > ---[ end trace 1840767f449d222e ]---
> >
> > Despite this dump says that EIP was in 'i810fb_check_var' the divide by zero truly occurs in 'i810_check_params' called by 'i810fb_check_var' (i810_main.c:1466).
> >
> > Signed-off-by: Samuel CUELLA <samuel.cuella@supinfo.com>
> > ---
> > --- linux-2.6.28/drivers/video/i810/i810_main.c.orig 2009-02-26 15:23:03.000000000 +0100
> > +++ linux-2.6.28/drivers/video/i810/i810_main.c 2009-02-26 14:50:06.000000000 +0100
> > @@ -993,6 +993,8 @@ static int i810_check_params(struct fb_v
> > struct i810fb_par *par = info->par;
> > int line_length, vidmem, mode_valid = 0, retval = 0;
> > u32 vyres = var->yres_virtual, vxres = var->xres_virtual;
> > + u32 yres = info->var.yres;
> > +
> > /*
> > * Memory limit
> > */
> >
>
> This is not appropriate for trivial tree. CCing akpm and lkml.
>
I don't have a copy of the original patch.
Please resend everything, with full changelog and a Signed-off-by: as
per Documentation/SubmittingPatches. Please also cc
linux-fbdev-devel@lists.sourceforge.net.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2.6.28 ] i810: kernel crash fix when struct fb_var_screeninfo is supplied
2009-03-04 22:20 ` Andrew Morton
@ 2009-03-04 22:54 ` Jiri Kosina
2009-03-04 23:17 ` Andrew Morton
0 siblings, 1 reply; 6+ messages in thread
From: Jiri Kosina @ 2009-03-04 22:54 UTC (permalink / raw)
To: Andrew Morton
Cc: Samuel.CUELLA, adaplas, linux-kernel, trivial, linux-fbdev-devel
On Wed, 4 Mar 2009, Andrew Morton wrote:
> > This is not appropriate for trivial tree. CCing akpm and lkml.
> I don't have a copy of the original patch.
This is what I got from Samuel (and rejected it for trivial tree, not
being trivial enough :) )
From: Samuel CUELLA <samuel.cuella@supinfo.com>
This patch prevents the kernel from being crashed by a divide-by-zero
operation when supplied an incorrectly filled 'struct fb_var_screeninfo'
from userland.
Previously i810_main.c:1005 (i810_check_params) was using the global
'yres' symbol previously defined at i810_main.c:145 as a module parameter
value holder (i810_main.c:2174). If i810fb is compiled-in or if this param
doesn't get a default value, this direct usage leads to a divide-by-zero
at i810_main.c:1005 (i810_check_params). The patch simply replace the
'yres' global, perhaps undefined symbol usage by a given parameter
structure lookup.
This problem occurs with directfb, mplayer -vo fbdev, SDL library.
It was also reported ( but non solved ) at : http://mail.directfb.org/pipermail/directfb-dev/2008-March/004050.html
Sample code to reproduce :
/*Comile with gcc crashfb.c -o crashfb*/
#include <fcntl.h>
#include <linux/fb.h>
#include <stdio.h>
#include <sys/ioctl.h>
#include <sys/mman.h>
#include <sys/stat.h>
#include <string.h>
#include <stdlib.h>
#define FB "/dev/fb0"
int main(){
int fd;
int rv;
struct fb_var_screeninfo vinfo;
fd = open(FB,O_RDWR);
if( fd ){
vinfo.xres = 800;
vinfo.yres = 600;
rv =ioctl(fd, FBIOPUT_VSCREENINFO, &vinfo);
}
return(rv);
}
Leads to this crash dump:
divide error: 0000 [#1]
last sysfs file: /sys/kernel/uevent_seqnum
Modules linked in:
Pid: 4058, comm: crashfb Not tainted (2.6.28 #4)
EIP: 0060:[<c02558c8>] EFLAGS: 00010202 CPU: 0
EIP is at i810fb_check_var+0x428/0x520
EAX: 00400000 EBX: ce9d5e44 ECX: 001209a0 EDX: 00000000
ESI: 00000020 EDI: 00000004 EBP: 00000000 ESP: ce9d5d0c
DS: 007b ES: 007b FS: 0000 GS: 0033 SS: 0068
Process crashfb (pid: 4058, ti=ce9d4000 task=cf8af0e0 task.ti=ce9d4000)
Stack:
c014f993 00000000 00000001 00000000 00000000 00400000 0000001a cf811000
08048268 cf81123c 00000258 00000320 ffffffed cf811015 ce9d5e45 cf811000
c0224821 ce9d5e44 ce9b09a0 00000000 00012000 00000000 c0111a89 00000001
Call Trace:
[<c014f993>] handle_mm_fault+0x5c3/0x650
[<c0224821>] fb_set_var+0x61/0x2d0
[<c0111a89>] do_page_fault+0x3a9/0x8b0
[<c016c935>] do_lookup+0x65/0x1a0
[<c02257aa>] fb_ioctl+0x21a/0x3c0
[<c014f577>] handle_mm_fault+0x1a7/0x650
[<c0225590>] fb_ioctl+0x0/0x3c0
[<c017077f>] vfs_ioctl+0x1f/0x70
[<c017096c>] do_vfs_ioctl+0x5c/0x430
[<c0111a89>] do_page_fault+0x3a9/0x8b0
[<c0170d7d>] sys_ioctl+0x3d/0x70
[<c0103af9>] sysenter_do_call+0x12/0x25
Code: c0 0f 44 d0 89 54 24 04 e8 b6 5a ec ff b8 ea ff ff ff 83 c4 30 5b 5e 5f 5d c3 8b 2d ac 0e 4a c0 31 d2 89 f7 8b 44 24 14 c1 ef 03 <f7> f5 31 d2 f7 f7 3b 03 89 c7 0f 83 3c fd ff ff 89 c2 89 f1 89
EIP: [<c02558c8>] i810fb_check_var+0x428/0x520 SS:ESP 0068:ce9d5d0c
---[ end trace 1840767f449d222e ]---
Despite this dump says that EIP was in 'i810fb_check_var' the divide by zero truly occurs in 'i810_check_params' called by 'i810fb_check_var' (i810_main.c:1466).
Signed-off-by: Samuel CUELLA <samuel.cuella@supinfo.com>
--- linux-2.6.28/drivers/video/i810/i810_main.c.orig 2009-02-26 15:23:03.000000000 +0100
+++ linux-2.6.28/drivers/video/i810/i810_main.c 2009-02-26 14:50:06.000000000 +0100
@@ -993,6 +993,8 @@ static int i810_check_params(struct fb_v
struct i810fb_par *par = info->par;
int line_length, vidmem, mode_valid = 0, retval = 0;
u32 vyres = var->yres_virtual, vxres = var->xres_virtual;
+ u32 yres = info->var.yres;
+
/*
* Memory limit
*/
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2.6.28 ] i810: kernel crash fix when struct fb_var_screeninfo is supplied
2009-03-04 22:54 ` Jiri Kosina
@ 2009-03-04 23:17 ` Andrew Morton
2009-03-04 23:45 ` CUELLA Samuel
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Morton @ 2009-03-04 23:17 UTC (permalink / raw)
To: Jiri Kosina
Cc: Samuel.CUELLA, adaplas, linux-kernel, trivial, linux-fbdev-devel
On Wed, 4 Mar 2009 23:54:49 +0100 (CET)
Jiri Kosina <jkosina@suse.cz> wrote:
> On Wed, 4 Mar 2009, Andrew Morton wrote:
>
> > > This is not appropriate for trivial tree. CCing akpm and lkml.
> > I don't have a copy of the original patch.
>
> This is what I got from Samuel (and rejected it for trivial tree, not
> being trivial enough :) )
>
ok, thanks.
> --- linux-2.6.28/drivers/video/i810/i810_main.c.orig 2009-02-26 15:23:03.000000000 +0100
> +++ linux-2.6.28/drivers/video/i810/i810_main.c 2009-02-26 14:50:06.000000000 +0100
> @@ -993,6 +993,8 @@ static int i810_check_params(struct fb_v
> struct i810fb_par *par = info->par;
> int line_length, vidmem, mode_valid = 0, retval = 0;
> u32 vyres = var->yres_virtual, vxres = var->xres_virtual;
> + u32 yres = info->var.yres;
> +
> /*
> * Memory limit
> */
So we have a file-wide static called `yres' - seems a bit dangerous.
Rather than defining a local which shadows the global, it would be
clearer to do it in this equivalent way, yes?
--- a/drivers/video/i810/i810_main.c~i810-fix-kernel-crash-fix-when-struct-fb_var_screeninfo-is-supplied
+++ a/drivers/video/i810/i810_main.c
@@ -993,6 +993,7 @@ static int i810_check_params(struct fb_v
struct i810fb_par *par = info->par;
int line_length, vidmem, mode_valid = 0, retval = 0;
u32 vyres = var->yres_virtual, vxres = var->xres_virtual;
+
/*
* Memory limit
*/
@@ -1002,12 +1003,12 @@ static int i810_check_params(struct fb_v
if (vidmem > par->fb.size) {
vyres = par->fb.size/line_length;
if (vyres < var->yres) {
- vyres = yres;
+ vyres = info->var.yres;
vxres = par->fb.size/vyres;
vxres /= var->bits_per_pixel >> 3;
line_length = get_line_length(par, vxres,
var->bits_per_pixel);
- vidmem = line_length * yres;
+ vidmem = line_length * info->var.yres;
if (vxres < var->xres) {
printk("i810fb: required video memory, "
"%d bytes, for %dx%d-%d (virtual) "
_
This assumes that the original change was actually correct. Should
both sites whcih use the global `yres' have been converted to use
info->var.yres, or just one of them?
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE : [PATCH 2.6.28 ] i810: kernel crash fix when struct fb_var_screeninfo is supplied
2009-03-04 23:17 ` Andrew Morton
@ 2009-03-04 23:45 ` CUELLA Samuel
0 siblings, 0 replies; 6+ messages in thread
From: CUELLA Samuel @ 2009-03-04 23:45 UTC (permalink / raw)
To: Andrew Morton, Jiri Kosina
Cc: adaplas, linux-kernel, trivial, linux-fbdev-devel
On jeudi 5 mars 2009 00:17, Andrew Morton[akpm@linux-foundation.org] wrote :
>On Wed, 4 Mar 2009 23:54:49 +0100 (CET)
>Jiri Kosina <jkosina@suse.cz> wrote:
>> On Wed, 4 Mar 2009, Andrew Morton wrote:
>>
>> > > This is not appropriate for trivial tree. CCing akpm and lkml.
>> > I don't have a copy of the original patch.
>>
>> This is what I got from Samuel (and rejected it for trivial tree, not
>> being trivial enough :) )
>>
>ok, thanks.
I was away, thanks for taking care of my patch :)
>> --- linux-2.6.28/drivers/video/i810/i810_main.c.orig 2009-02-26 15:23:03.000000000 +0100
>> +++ linux-2.6.28/drivers/video/i810/i810_main.c 2009-02-26 14:50:06.000000000 +0100
>> @@ -993,6 +993,8 @@ static int i810_check_params(struct fb_v
>> struct i810fb_par *par = info->par;
>> int line_length, vidmem, mode_valid = 0, retval = 0;
>> u32 vyres = var->yres_virtual, vxres = var->xres_virtual;
>> + u32 yres = info->var.yres;
>> +
>> /*
>> * Memory limit
>> */
>So we have a file-wide static called `yres' - seems a bit dangerous.
Yes, for sure, but there is the same kind of symbol for xres, so I leave it.
As this variable is used to hold the 'yres' module parameter value, I think it's named upon this one.
>Rather than defining a local which shadows the global, it would be
>clearer to do it in this equivalent way, yes?
Yes, you're totaly right.
--- a/drivers/video/i810/i810_main.c~i810-fix-kernel-crash-fix-when-struct-fb_var_screeninfo-is-supplied
+++ a/drivers/video/i810/i810_main.c
@@ -993,6 +993,7 @@ static int i810_check_params(struct fb_v
struct i810fb_par *par = info->par;
int line_length, vidmem, mode_valid = 0, retval = 0;
u32 vyres = var->yres_virtual, vxres = var->xres_virtual;
+
/*
* Memory limit
*/
@@ -1002,12 +1003,12 @@ static int i810_check_params(struct fb_v
if (vidmem > par->fb.size) {
vyres = par->fb.size/line_length;
if (vyres < var->yres) {
- vyres = yres;
+ vyres = info->var.yres;
vxres = par->fb.size/vyres;
vxres /= var->bits_per_pixel >> 3;
line_length = get_line_length(par, vxres,
var->bits_per_pixel);
- vidmem = line_length * yres;
+ vidmem = line_length * info->var.yres;
if (vxres < var->xres) {
printk("i810fb: required video memory, "
"%d bytes, for %dx%d-%d (virtual) "
_
>This assumes that the original change was actually correct. Should
>both sites whcih use the global `yres' have been converted to use
>info->var.yres, or just one of them?
Yes, the two sites have to be changed. The orginal 'yres' global var is intended to be used as a module paramter value holder ( i810_main.c:2174), so it's not intended to be used to do other things than
module initialization.
P.S: Sorry for my bad english.
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE : [PATCH 2.6.28 ] i810: kernel crash fix when struct fb_var_screeninfo is supplied
@ 2009-03-04 23:45 ` CUELLA Samuel
0 siblings, 0 replies; 6+ messages in thread
From: CUELLA Samuel @ 2009-03-04 23:45 UTC (permalink / raw)
To: Andrew Morton, Jiri Kosina
Cc: adaplas, linux-kernel, trivial, linux-fbdev-devel
On jeudi 5 mars 2009 00:17, Andrew Morton[akpm@linux-foundation.org] wrote :
>On Wed, 4 Mar 2009 23:54:49 +0100 (CET)
>Jiri Kosina <jkosina@suse.cz> wrote:
>> On Wed, 4 Mar 2009, Andrew Morton wrote:
>>
>> > > This is not appropriate for trivial tree. CCing akpm and lkml.
>> > I don't have a copy of the original patch.
>>
>> This is what I got from Samuel (and rejected it for trivial tree, not
>> being trivial enough :) )
>>
>ok, thanks.
I was away, thanks for taking care of my patch :)
>> --- linux-2.6.28/drivers/video/i810/i810_main.c.orig 2009-02-26 15:23:03.000000000 +0100
>> +++ linux-2.6.28/drivers/video/i810/i810_main.c 2009-02-26 14:50:06.000000000 +0100
>> @@ -993,6 +993,8 @@ static int i810_check_params(struct fb_v
>> struct i810fb_par *par = info->par;
>> int line_length, vidmem, mode_valid = 0, retval = 0;
>> u32 vyres = var->yres_virtual, vxres = var->xres_virtual;
>> + u32 yres = info->var.yres;
>> +
>> /*
>> * Memory limit
>> */
>So we have a file-wide static called `yres' - seems a bit dangerous.
Yes, for sure, but there is the same kind of symbol for xres, so I leave it.
As this variable is used to hold the 'yres' module parameter value, I think it's named upon this one.
>Rather than defining a local which shadows the global, it would be
>clearer to do it in this equivalent way, yes?
Yes, you're totaly right.
--- a/drivers/video/i810/i810_main.c~i810-fix-kernel-crash-fix-when-struct-fb_var_screeninfo-is-supplied
+++ a/drivers/video/i810/i810_main.c
@@ -993,6 +993,7 @@ static int i810_check_params(struct fb_v
struct i810fb_par *par = info->par;
int line_length, vidmem, mode_valid = 0, retval = 0;
u32 vyres = var->yres_virtual, vxres = var->xres_virtual;
+
/*
* Memory limit
*/
@@ -1002,12 +1003,12 @@ static int i810_check_params(struct fb_v
if (vidmem > par->fb.size) {
vyres = par->fb.size/line_length;
if (vyres < var->yres) {
- vyres = yres;
+ vyres = info->var.yres;
vxres = par->fb.size/vyres;
vxres /= var->bits_per_pixel >> 3;
line_length = get_line_length(par, vxres,
var->bits_per_pixel);
- vidmem = line_length * yres;
+ vidmem = line_length * info->var.yres;
if (vxres < var->xres) {
printk("i810fb: required video memory, "
"%d bytes, for %dx%d-%d (virtual) "
_
>This assumes that the original change was actually correct. Should
>both sites whcih use the global `yres' have been converted to use
>info->var.yres, or just one of them?
Yes, the two sites have to be changed. The orginal 'yres' global var is intended to be used as a module paramter value holder ( i810_main.c:2174), so it's not intended to be used to do other things than
module initialization.
P.S: Sorry for my bad english.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2009-03-04 23:52 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <02E43B9E8855E74E886358B5184B4CC9104620C0@mail2-aub1fr.esi-supinfo.com>
2009-03-04 10:09 ` [PATCH 2.6.28 ] i810: kernel crash fix when struct fb_var_screeninfo is supplied Jiri Kosina
2009-03-04 22:20 ` Andrew Morton
2009-03-04 22:54 ` Jiri Kosina
2009-03-04 23:17 ` Andrew Morton
2009-03-04 23:45 ` RE : " CUELLA Samuel
2009-03-04 23:45 ` CUELLA Samuel
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.