The function viafb_cursor() uses 2 stack-variables of CURSOR_SIZE bits;
CURSOR_SIZE is defined as (8 * 1024). Using up twice 1k on stack is too much
for 4k-stack (though it works with 8k-stacks).
Make those two variables kzalloc'ed to preserve stack space.
Signed-off-by: Bruno Prémont <[email protected]>
---
--- linux-2.6.28-rc3.orig/drviers/video/via/viafbdev.c 2008-11-09 19:22:15.000000000 +0100
+++ linux-2.6.28-rc3/drivers/video/via/viafbdev.c 2008-11-09 19:36:15.000000000 +0100
@@ -1052,10 +1052,8 @@ static void viafb_imageblit(struct fb_in
static int viafb_cursor(struct fb_info *info, struct fb_cursor *cursor)
{
- u8 data[CURSOR_SIZE / 8];
- u32 data_bak[CURSOR_SIZE / 32];
u32 temp, xx, yy, bg_col = 0, fg_col = 0;
- int size, i, j = 0;
+ int i, j = 0;
static int hw_cursor;
struct viafb_par *p_viafb_par;
@@ -1178,10 +1176,15 @@ static int viafb_cursor(struct fb_info *
}
if (cursor->set & FB_CUR_SETSHAPE) {
- size =
+ u8 *data = kzalloc(CURSOR_SIZE / 8, GFP_KERNEL);
+ u32 *data_bak = kzalloc(CURSOR_SIZE / 32, GFP_KERNEL);
+ int size =
((viacursor.image.width + 7) >> 3) *
viacursor.image.height;
+ if (data == NULL || data_bak == NULL)
+ goto out;
+
if (MAX_CURS == 32) {
for (i = 0; i < (CURSOR_SIZE / 32); i++) {
data_bak[i] = 0x0;
@@ -1231,6 +1234,9 @@ static int viafb_cursor(struct fb_info *
memcpy(((struct viafb_par *)(info->par))->fbmem_virt +
((struct viafb_par *)(info->par))->cursor_start,
data_bak, CURSOR_SIZE);
+out:
+ kfree(data);
+ kfree(data_bak);
}
if (viacursor.enable)
On Sun, 9 Nov 2008 20:25:37 +0100 Bruno Pr__mont <[email protected]> wrote:
> The function viafb_cursor() uses 2 stack-variables of CURSOR_SIZE bits;
> CURSOR_SIZE is defined as (8 * 1024). Using up twice 1k on stack is too much
> for 4k-stack (though it works with 8k-stacks).
yup, we should fix that.
> Make those two variables kzalloc'ed to preserve stack space.
>
> Signed-off-by: Bruno Pr__mont <[email protected]>
>
> ---
> --- linux-2.6.28-rc3.orig/drviers/video/via/viafbdev.c 2008-11-09 19:22:15.000000000 +0100
^^ typo in pathname?
> +++ linux-2.6.28-rc3/drivers/video/via/viafbdev.c 2008-11-09 19:36:15.000000000 +0100
> @@ -1052,10 +1052,8 @@ static void viafb_imageblit(struct fb_in
>
> static int viafb_cursor(struct fb_info *info, struct fb_cursor *cursor)
> {
> - u8 data[CURSOR_SIZE / 8];
> - u32 data_bak[CURSOR_SIZE / 32];
> u32 temp, xx, yy, bg_col = 0, fg_col = 0;
> - int size, i, j = 0;
> + int i, j = 0;
> static int hw_cursor;
> struct viafb_par *p_viafb_par;
>
> @@ -1178,10 +1176,15 @@ static int viafb_cursor(struct fb_info *
> }
>
> if (cursor->set & FB_CUR_SETSHAPE) {
> - size =
> + u8 *data = kzalloc(CURSOR_SIZE / 8, GFP_KERNEL);
> + u32 *data_bak = kzalloc(CURSOR_SIZE / 32, GFP_KERNEL);
> + int size =
> ((viacursor.image.width + 7) >> 3) *
> viacursor.image.height;
>
> + if (data == NULL || data_bak == NULL)
> + goto out;
> +
> if (MAX_CURS == 32) {
> for (i = 0; i < (CURSOR_SIZE / 32); i++) {
> data_bak[i] = 0x0;
> @@ -1231,6 +1234,9 @@ static int viafb_cursor(struct fb_info *
> memcpy(((struct viafb_par *)(info->par))->fbmem_virt +
> ((struct viafb_par *)(info->par))->cursor_start,
> data_bak, CURSOR_SIZE);
> +out:
> + kfree(data);
> + kfree(data_bak);
> }
>
> if (viacursor.enable)
Is the ->fb_cursor handler allowed to perform GFP_KERNEL memory
allocations? It's never called from atomic contexts?
On Sun, 9 Nov 2008 11:36:03 -0800
Andrew Morton <[email protected]> wrote:
> On Sun, 9 Nov 2008 20:25:37 +0100 Bruno Pr__mont
> <[email protected]> wrote:
>
> > The function viafb_cursor() uses 2 stack-variables of CURSOR_SIZE
> > bits; CURSOR_SIZE is defined as (8 * 1024). Using up twice 1k on
> > stack is too much for 4k-stack (though it works with 8k-stacks).
>
> >
> > if (viacursor.enable)
>
> Is the ->fb_cursor handler allowed to perform GFP_KERNEL memory
> allocations? It's never called from atomic contexts?
if it's allowed to do GFP_KERNEL memory allocations the statement that
it works with 8k stacks is a bit overstated... since irq's can come in
and take several KB of stack as well ;)
--
Arjan van de Ven Intel Open Source Technology Centre
For development, discussion and tips for power savings,
visit http://www.lesswatts.org
On Sun, 09 November 2008 Arjan van de Ven wrote:
> On Sun, 9 Nov 2008 Andrew Morton wrote:
> > On Sun, 9 Nov 2008 Bruno Prémont wrote:
> >
> > > The function viafb_cursor() uses 2 stack-variables of CURSOR_SIZE
> > > bits; CURSOR_SIZE is defined as (8 * 1024). Using up twice 1k on
> > > stack is too much for 4k-stack (though it works with 8k-stacks).
> >
> > >
> > > if (viacursor.enable)
> >
> > Is the ->fb_cursor handler allowed to perform GFP_KERNEL memory
> > allocations? It's never called from atomic contexts?
>
> if it's allowed to do GFP_KERNEL memory allocations the statement that
> it works with 8k stacks is a bit overstated... since irq's can come in
> and take several KB of stack as well ;)
I don't know if it can be called from atomic contexts or not :(
In addition I get panics some time after start-up which I'm not sure
what to associate them with (apparently unrelated)
It could be some stack overflow by calling fbset (I will to more testing
in order to find out)
First attempt: calling fbset via ssh:
[ 1806.952151] BUG: unable to handle kernel NULL pointer dereference at 00000123
[ 1806.952601] IP: [<c03d2737>] icmpv6_send+0x387/0x580
[ 1806.952934] *pde = 00000000
[ 1806.953125] Oops: 0000 [#1]
[ 1806.953310] last sysfs file: /sys/devices/platform/w83627hf.656/temp2_input
[ 1806.953717] Modules linked in: snd_hda_intel snd_pcm snd_timer snd soundcore snd_page_alloc sg
[ 1806.954328]
[ 1806.954430] Pid: 1855, comm: sshd Not tainted (2.6.28-rc3-git6 #1) CX700+W697HG
[ 1806.954863] EIP: 0060:[<c03d2737>] EFLAGS: 00010206 CPU: 0
[ 1806.955194] EIP is at icmpv6_send+0x387/0x580
[ 1806.955456] EAX: ffffffff EBX: f713c704 ECX: f6bc26a8 EDX: 0000006c
[ 1806.955827] ESI: f713c500 EDI: 00000040 EBP: f6babca0 ESP: f6babbf8
[ 1806.956197] DS: 007b ES: 007b FS: 0000 GS: 0033 SS: 0068
[ 1806.956520] Process sshd (pid: 1855, ti=f6bab000 task=f70e7440 task.ti=f6bab000)
[ 1806.956952] Stack:
[ 1806.957074] 00000000 00007515 ffffffcc f6babc51 f6babc51 c05f6045 f6babc8c 00000296
[ 1806.957614] f6babc3c 00000000 00000002 f6bc26a8 00000200 38b782ca f713c704 00000000
[ 1806.958321] 00000000 00000000 00000000 00000000 60090120 0000ab07 ff1d0302 300005fe
[ 1806.958882] Call Trace:
[ 1806.959037] [<c03bf600>] ? ip6_xmit+0x230/0x3f0
[ 1806.959339] [<c03de873>] ? inet6_csk_xmit+0x103/0x190
[ 1806.959669] [<c03d9281>] ? tcp_v6_send_check+0x51/0x100
[ 1806.960011] [<c0399103>] ? tcp_transmit_skb+0x373/0x670
[ 1806.960016] [<c039a6f0>] ? tcp_push_one+0xa0/0xd0
[ 1806.960016] [<c038fd64>] ? tcp_sendmsg+0x264/0xa30
[ 1806.960016] [<c0179157>] ? core_sys_select+0x207/0x2c0
[ 1806.960016] [<c03581ab>] ? sock_aio_write+0xeb/0x110
[ 1806.960016] [<c016c76c>] ? do_sync_write+0xcc/0x110
[ 1806.960016] [<c02b9ee5>] ? pty_unthrottle+0x15/0x20
[ 1806.960016] [<c0133400>] ? autoremove_wake_function+0x0/0x50
[ 1806.960016] [<c01263e6>] ? current_fs_time+0x16/0x20
[ 1806.960016] [<c016d0d0>] ? vfs_write+0x110/0x120
[ 1806.960016] [<c016d18d>] ? sys_write+0x3d/0x70
[ 1806.960016] [<c0103bc1>] ? sysenter_do_call+0x12/0x25
[ 1806.960016] Code: 0f b6 4d 89 89 45 dc 88 4d e0 8b 52 50 29 c2 b8 d0 04 00 00 81 fa d0 04 00 00 0f 47 d0 85 d2 0f 88 91 01 00 00 8b 4d 84 8b 41 14 <8b> 98 24 01 00 00 85 db 74 06 ff 83 80 00 00 00 b8 40 00 00 00
[ 1806.960016] EIP: [<c03d2737>] icmpv6_send+0x387/0x580 SS:ESP 0068:f6babbf8
[ 1807.067511] Kernel panic - not syncing: Fatal exception in interrupt
Second attempt, delayed after calling fbset from console:
[ 217.260426] BUG: unable to handle kernel NULL pointer dereference at 000000c7
[ 217.260915] IP: [<c0380b46>] rt_worker_func+0xb6/0x160
[ 217.261264] *pde = 00000000
[ 217.261458] Oops: 0000 [#1]
[ 217.261649] last sysfs file: /sys/devices/platform/w83627hf.656/temp2_input
[ 217.262058] Modules linked in: snd_hda_intel snd_pcm snd_timer snd soundcore snd_page_alloc sg
[ 217.262691]
[ 217.262795] Pid: 5, comm: events/0 Not tainted (2.6.28-rc3-git6 #1) CX700+W697HG
[ 217.263236] EIP: 0060:[<c0380b46>] EFLAGS: 00010286 CPU: 0
[ 217.263570] EIP is at rt_worker_func+0xb6/0x160
[ 217.263846] EAX: 00000002 EBX: ffffffff ECX: c0606e20 EDX: fffffed4
[ 217.270015] ESI: f7172c5c EDI: 00007530 EBP: f7032f80 ESP: f7032f6c
[ 217.270015] DS: 007b ES: 007b FS: 0000 GS: 0000 SS: 0068
[ 217.270015] Process events/0 (pid: 5, ti=f7032000 task=f702ad80 task.ti=f7032000)
[ 217.270015] Stack:
[ 217.270015] 000001b5 00004b17 c053d7a0 f7008180 c0380a90 f7032fa4 c0130117 f702b440
[ 217.270015] f702ad80 c0510180 00000246 f7008188 f7008180 f7032fac f7032fcc c0130747
[ 217.270015] 00000000 f702ad80 c0133400 f7032fb8 f7032fb8 fffffffc f7008180 c01306b0
[ 217.270015] Call Trace:
[ 217.270015] [<c0380a90>] ? rt_worker_func+0x0/0x160
[ 217.270015] [<c0130117>] ? run_workqueue+0x67/0xe0
[ 217.270015] [<c0130747>] ? worker_thread+0x97/0xf0
[ 217.270015] [<c0133400>] ? autoremove_wake_function+0x0/0x50
[ 217.270015] [<c01306b0>] ? worker_thread+0x0/0xf0
[ 217.270015] [<c0133032>] ? kthread+0x42/0x70
[ 217.270015] [<c0132ff0>] ? kthread+0x0/0x70
[ 217.270015] [<c0104847>] ? kernel_thread_helper+0x7/0x10
[ 217.270015] Code: f0 ff ff f6 40 08 08 0f 85 bb 00 00 00 8b 06 85 c0 74 49 89 df e8 8b 5c da ff 8d 74 26 00 8d bc 27 00 00 00 00 8b 1e 85 db 74 2c <8b> 83 c8 00 00 00 3b 05 dc c9 61 c0 75 4c 8b 53 18 85 d2 74 2c
[ 217.270015] EIP: [<c0380b46>] rt_worker_func+0xb6/0x160 SS:ESP 0068:f7032f6c
[ 217.526097] Kernel panic - not syncing: Fatal exception in interrupt
On Sun, 9 Nov 2008 21:38:11 +0100 Bruno Pr__mont <[email protected]> wrote:
> On Sun, 09 November 2008 Arjan van de Ven wrote:
> > On Sun, 9 Nov 2008 Andrew Morton wrote:
> > > On Sun, 9 Nov 2008 Bruno Pr__mont wrote:
> > >
> > > > The function viafb_cursor() uses 2 stack-variables of CURSOR_SIZE
> > > > bits; CURSOR_SIZE is defined as (8 * 1024). Using up twice 1k on
> > > > stack is too much for 4k-stack (though it works with 8k-stacks).
> > >
> > > >
> > > > if (viacursor.enable)
> > >
> > > Is the ->fb_cursor handler allowed to perform GFP_KERNEL memory
> > > allocations? It's never called from atomic contexts?
> >
> > if it's allowed to do GFP_KERNEL memory allocations the statement that
> > it works with 8k stacks is a bit overstated... since irq's can come in
> > and take several KB of stack as well ;)
> I don't know if it can be called from atomic contexts or not :(
>
>
> In addition I get panics some time after start-up which I'm not sure
> what to associate them with (apparently unrelated)
> It could be some stack overflow by calling fbset (I will to more testing
> in order to find out)
>
> First attempt: calling fbset via ssh:
>
> [ 1806.952151] BUG: unable to handle kernel NULL pointer dereference at 00000123
> [ 1806.952601] IP: [<c03d2737>] icmpv6_send+0x387/0x580
>
> ...
>
> Second attempt, delayed after calling fbset from console:
>
> [ 217.260426] BUG: unable to handle kernel NULL pointer dereference at 000000c7
> [ 217.260915] IP: [<c0380b46>] rt_worker_func+0xb6/0x160
gack. Your kernel was destroyed.
Stack overflow might well explain this. Does it work OK with 8k stacks?
scripts/checkstack.pl should help find the problems.
On Sun, 09 November 2008 Andrew Morton wrote:
> On Sun, 9 Nov 2008 21:38:11 +0100 Bruno Prémont wrote:
> > On Sun, 09 November 2008 Arjan van de Ven wrote:
> > > On Sun, 9 Nov 2008 Andrew Morton wrote:
> > > > On Sun, 9 Nov 2008 Bruno Pr__mont wrote:
> > > >
> > > > > The function viafb_cursor() uses 2 stack-variables of
> > > > > CURSOR_SIZE bits; CURSOR_SIZE is defined as (8 * 1024). Using
> > > > > up twice 1k on stack is too much for 4k-stack (though it
> > > > > works with 8k-stacks).
> > > >
> > > > >
> > > > > if (viacursor.enable)
> > > >
> > > > Is the ->fb_cursor handler allowed to perform GFP_KERNEL memory
> > > > allocations? It's never called from atomic contexts?
> > >
> > > if it's allowed to do GFP_KERNEL memory allocations the statement
> > > that it works with 8k stacks is a bit overstated... since irq's
> > > can come in and take several KB of stack as well ;)
> > I don't know if it can be called from atomic contexts or not :(
Ok scanned under drivers/video/ for users of fb_cursor() and all those
(under drivers/video/console/) do GPT_ATOMIC allocations before calling
fbops->fb_cursor, so my patch chooses the wrong allocation constraint.
Fixed patch below
> > In addition I get panics some time after start-up which I'm not sure
> > what to associate them with (apparently unrelated)
> > It could be some stack overflow by calling fbset (I will to more
> > testing in order to find out)
> >
> > First attempt: calling fbset via ssh:
> >
> > [ 1806.952151] BUG: unable to handle kernel NULL pointer
> > dereference at 00000123 [ 1806.952601] IP: [<c03d2737>]
> > icmpv6_send+0x387/0x580
> >
> > ...
> >
> > Second attempt, delayed after calling fbset from console:
> >
> > [ 217.260426] BUG: unable to handle kernel NULL pointer
> > dereference at 000000c7 [ 217.260915] IP: [<c0380b46>]
> > rt_worker_func+0xb6/0x160
>
> gack. Your kernel was destroyed.
>
> Stack overflow might well explain this. Does it work OK with 8k
> stacks?
My last attempt with 8k stacks is a bit dated (2.6.27-rc) but back then
I saw the same kind of behavior than now with viafb, crash with 4k-stack
but running system with 8k-stack. Running system does of course not mean
that stack cannot overflow :)
> scripts/checkstack.pl should help find the problems.
Thanks for the pointer!
It show a nice candidate, viafb_ioctl in top-6:
0xc011612b identity_mapped [vmlinux]: 4096
0xc017896b do_sys_poll [vmlinux]: 888
0xc0178bdd do_sys_poll [vmlinux]: 888
0xc024506b sha256_transform [vmlinux]: 752
0xc024768b sha256_transform [vmlinux]: 752
0xc027d933 viafb_ioctl [vmlinux]: 728
0xc01783c8 do_select [vmlinux]: 708
0xc0178853 do_select [vmlinux]: 708
On a new attempt the box survived fbset "1280x1024-60" though
I did wait some time after boot up before calling it.
So it's pretty probable that either it gets near the limit of stack
or this time the neighborhood of the stack was not just as critical :/
Shall I trim down that function's stack usage as well?
(many structs allocated from stack)
What is preferred, allocating a big block of memory and point various
variables inside that block or do multiple individual allocations?
e.g.
u8 data[CURSOR_SIZE/8]
u32 data_bak[CURSOR_SIZE/32]
=>
u8 *data = kzalloc(...)
u32 *data_bak = kzalloc(...)
or
u8 *data = kzalloc(CURSOR_SIZE/8 + CURSOR_SIZE/32, ...)
u32 *data_bak = (u32*)(data+CURSOR_SIZE/8);
First option is more readable, second should be more efficient...
The end result readability would suffer even more in case of viafb_ioctl()
with the big amount of different structs that could be allocated from heap
instead of stack...
Bruno
---
--- linux-2.6.28-rc3.orig/drivers/video/via/viafbdev.c 2008-11-09 19:22:15.000000000 +0100
+++ linux-2.6.28-rc3/drivers/video/via/viafbdev.c 2008-11-09 21:25:47.000000000 +0100
@@ -1052,10 +1052,8 @@ static void viafb_imageblit(struct fb_in
static int viafb_cursor(struct fb_info *info, struct fb_cursor *cursor)
{
- u8 data[CURSOR_SIZE / 8];
- u32 data_bak[CURSOR_SIZE / 32];
u32 temp, xx, yy, bg_col = 0, fg_col = 0;
- int size, i, j = 0;
+ int i, j = 0;
static int hw_cursor;
struct viafb_par *p_viafb_par;
@@ -1178,10 +1176,15 @@ static int viafb_cursor(struct fb_info *
}
if (cursor->set & FB_CUR_SETSHAPE) {
- size =
+ u8 *data = kzalloc(CURSOR_SIZE / 8, GFP_ATOMIC);
+ u32 *data_bak = kzalloc(CURSOR_SIZE / 32, GFP_ATOMIC);
+ int size =
((viacursor.image.width + 7) >> 3) *
viacursor.image.height;
+ if (data == NULL || data_bak == NULL)
+ goto out;
+
if (MAX_CURS == 32) {
for (i = 0; i < (CURSOR_SIZE / 32); i++) {
data_bak[i] = 0x0;
@@ -1231,6 +1234,9 @@ static int viafb_cursor(struct fb_info *
memcpy(((struct viafb_par *)(info->par))->fbmem_virt +
((struct viafb_par *)(info->par))->cursor_start,
data_bak, CURSOR_SIZE);
+out:
+ kfree(data);
+ kfree(data_bak);
}
if (viacursor.enable)
On Sun, 9 Nov 2008 22:37:48 +0100 Bruno Pr__mont <[email protected]> wrote:
>
> Ok scanned under drivers/video/ for users of fb_cursor() and all those
> (under drivers/video/console/) do GPT_ATOMIC allocations before calling
> fbops->fb_cursor, so my patch chooses the wrong allocation constraint.
>
> Fixed patch below
Updated, thanks.
> > > In addition I get panics some time after start-up which I'm not sure
> > > what to associate them with (apparently unrelated)
> > > It could be some stack overflow by calling fbset (I will to more
> > > testing in order to find out)
> > >
> > > First attempt: calling fbset via ssh:
> > >
> > > [ 1806.952151] BUG: unable to handle kernel NULL pointer
> > > dereference at 00000123 [ 1806.952601] IP: [<c03d2737>]
> > > icmpv6_send+0x387/0x580
> > >
> > > ...
> > >
> > > Second attempt, delayed after calling fbset from console:
> > >
> > > [ 217.260426] BUG: unable to handle kernel NULL pointer
> > > dereference at 000000c7 [ 217.260915] IP: [<c0380b46>]
> > > rt_worker_func+0xb6/0x160
> >
> > gack. Your kernel was destroyed.
> >
> > Stack overflow might well explain this. Does it work OK with 8k
> > stacks?
> My last attempt with 8k stacks is a bit dated (2.6.27-rc) but back then
> I saw the same kind of behavior than now with viafb, crash with 4k-stack
> but running system with 8k-stack. Running system does of course not mean
> that stack cannot overflow :)
>
> > scripts/checkstack.pl should help find the problems.
>
> Thanks for the pointer!
>
> It show a nice candidate, viafb_ioctl in top-6:
> 0xc011612b identity_mapped [vmlinux]: 4096
erk!
> 0xc017896b do_sys_poll [vmlinux]: 888
> 0xc0178bdd do_sys_poll [vmlinux]: 888
> 0xc024506b sha256_transform [vmlinux]: 752
> 0xc024768b sha256_transform [vmlinux]: 752
> 0xc027d933 viafb_ioctl [vmlinux]: 728
> 0xc01783c8 do_select [vmlinux]: 708
> 0xc0178853 do_select [vmlinux]: 708
>
>
> On a new attempt the box survived fbset "1280x1024-60" though
> I did wait some time after boot up before calling it.
> So it's pretty probable that either it gets near the limit of stack
> or this time the neighborhood of the stack was not just as critical :/
>
> Shall I trim down that function's stack usage as well?
> (many structs allocated from stack)
Yes please.
> What is preferred, allocating a big block of memory and point various
> variables inside that block or do multiple individual allocations?
>
> e.g.
> u8 data[CURSOR_SIZE/8]
> u32 data_bak[CURSOR_SIZE/32]
> =>
> u8 *data = kzalloc(...)
> u32 *data_bak = kzalloc(...)
> or
> u8 *data = kzalloc(CURSOR_SIZE/8 + CURSOR_SIZE/32, ...)
> u32 *data_bak = (u32*)(data+CURSOR_SIZE/8);
>
> First option is more readable, second should be more efficient...
If the total allocation is greater than 4096 then it will need a
larger-than-order-zero allocation, and there are some reliability
risks there. But if it's 4096 we're OK.
A good way to code this would be
struct {
u8 data[CURSOR_SIZE/8];
u32 data_bak[CURSOR_SIZE/32];
...
} *local_data = kzalloc(sizeof(*local_data, GFP_WHATEVER));
if (!local_data)
bummer();
...
local_data->data = foo;
...
kfree(local_data);
where GFP_WHATEVER should be the reliable GFP_KERNEL if possible, and
the unreliable GFP_ATOMIC otherwise.
> The end result readability would suffer even more in case of viafb_ioctl()
> with the big amount of different structs that could be allocated from heap
> instead of stack...
Perhaps the above texhnique could be used there as well.
On Sun, 9 Nov 2008, Bruno Prémont wrote:
> What is preferred, allocating a big block of memory and point various
> variables inside that block or do multiple individual allocations?
>
> e.g.
> u8 data[CURSOR_SIZE/8]
> u32 data_bak[CURSOR_SIZE/32]
> =>
> u8 *data = kzalloc(...)
> u32 *data_bak = kzalloc(...)
> or
> u8 *data = kzalloc(CURSOR_SIZE/8 + CURSOR_SIZE/32, ...)
> u32 *data_bak = (u32*)(data+CURSOR_SIZE/8);
>
> First option is more readable, second should be more efficient...
I like this method:
foo()
{
/* anonymous struct, you don't need to think of a name */
struct {
u8 data[CURSOR_SIZE/8];
u32 data_bak[CURSOR_SIZE/32];
} *cursor;
cursor = kzalloc(sizeof(*cursor), ...);
/* for remaining code:
* s/data/cursor->data/
* s/data_bak/cursor->data_bak/
*/
free(cursor);
}
A number of advantages over multiple allocations:
The alloc code and free code only has to run once.
Likely less memory will be allocated, due to allocation granularity.
Only one pointer is needed to keep track of the allocations instead of two.
This frees up a pointer's worth of stack space and/or another register for
the compiler to use.
More readable than u32 *data_bak = (u32*)(data+CURSOR_SIZE/8) and so on.
If you check for kzalloc failure, you only need code to check once. And
you can avoid the need for rolling back the first allocation if the second
fails.
The disadvantage is that you can't free just one of the allocations and big
allocations are harder to satisfy than small ones. When you get up to the
megabyte range, combining allocations into bigger ones becomes a bad idea.
On Sun, 09 November 2008 Andrew Morton wrote:
> > It show a nice candidate, viafb_ioctl in top-6:
> > 0xc011612b identity_mapped [vmlinux]: 4096
>
> erk!
>
> > 0xc017896b do_sys_poll [vmlinux]: 888
> > 0xc0178bdd do_sys_poll [vmlinux]: 888
> > 0xc024506b sha256_transform [vmlinux]: 752
> > 0xc024768b sha256_transform [vmlinux]: 752
> > 0xc027d933 viafb_ioctl [vmlinux]: 728
> > 0xc01783c8 do_select [vmlinux]: 708
> > 0xc0178853 do_select [vmlinux]: 708
> >
> >
> > On a new attempt the box survived fbset "1280x1024-60" though
> > I did wait some time after boot up before calling it.
> > So it's pretty probable that either it gets near the limit of stack
> > or this time the neighborhood of the stack was not just as
> > critical :/
> >
> > Shall I trim down that function's stack usage as well?
> > (many structs allocated from stack)
>
> Yes please.
I did a first attempt, based on suggested technique.
For viafb_ioctl() I did put all the structs in a single union (on stack)
and used the appropriate one depending on the cmd.
This saves 276 bytes out of 728 according to check_stack.pl
The most clean approach would probably be to split out the parts using
bigger structs into separate functions and allocating the required
memory from there.
I also switched viafb_cursor() to the struct technique.
See patch below for both.
During conversion of viafb_ioctl() I noticed the following:
Those two cases just copy_from_user and do nothing with copied data,
incomplete implementation?:
case VIAFB_SET_PANEL_POSITION:
if (copy_from_user(&u.panel_pos_size_para, argp,
sizeof(u.panel_pos_size_para)))
return -EFAULT;
break;
case VIAFB_SET_PANEL_SIZE:
if (copy_from_user(&u.panel_pos_size_para, argp,
sizeof(u.panel_pos_size_para)))
return -EFAULT;
break;
Handling of GET/SET GAMMA looks buggy:
In each case 256*4 bytes are allocated but only 4 bytes (size of pointer)
are copied to/from userspace though viafb_(get|set)_gamma_table() operates
on the full 256 elements...
case VIAFB_SET_GAMMA_LUT:
viafb_gamma_table = kmalloc(256 * sizeof(u32), GFP_KERNEL);
if (!viafb_gamma_table)
return -ENOMEM;
if (copy_from_user(viafb_gamma_table, argp,
sizeof(viafb_gamma_table))) {
kfree(viafb_gamma_table);
return -EFAULT;
}
viafb_set_gamma_table(viafb_bpp, viafb_gamma_table);
kfree(viafb_gamma_table);
break;
case VIAFB_GET_GAMMA_LUT:
viafb_gamma_table = kmalloc(256 * sizeof(u32), GFP_KERNEL);
if (!viafb_gamma_table)
return -ENOMEM;
viafb_get_gamma_table(viafb_gamma_table);
if (copy_to_user(argp, viafb_gamma_table,
sizeof(viafb_gamma_table))) {
kfree(viafb_gamma_table);
return -EFAULT;
}
kfree(viafb_gamma_table);
break;
I don't know if there is a userspace app that uses these VIA IOCTLs...
so the ioctl part is just compile-tested.
After checking, fbset just uses some generic framebuffer IOCTLs outside
of viafb's scope, thus not passing through viafb_ioctl().
---
--- linux-2.6.28-rc3/drivers/video/via/viafbdev.c.orig 2008-11-09 19:36:47.000000000 +0100
+++ linux-2.6.28-rc3/drivers/video/via/viafbdev.c 2008-11-10 20:50:32.000000000 +0100
@@ -546,23 +546,25 @@ static int viafb_blank(int blank_mode, s
static int viafb_ioctl(struct fb_info *info, u_int cmd, u_long arg)
{
- struct viafb_ioctl_mode viamode;
- struct viafb_ioctl_samm viasamm;
- struct viafb_driver_version driver_version;
- struct fb_var_screeninfo sec_var;
- struct _panel_size_pos_info panel_pos_size_para;
+ union {
+ struct viafb_ioctl_mode viamode;
+ struct viafb_ioctl_samm viasamm;
+ struct viafb_driver_version driver_version;
+ struct fb_var_screeninfo sec_var;
+ struct _panel_size_pos_info panel_pos_size_para;
+ struct viafb_ioctl_setting viafb_setting;
+ struct device_t active_dev;
+ } u;
u32 state_info = 0;
- u32 viainfo_size = sizeof(struct viafb_ioctl_info);
u32 *viafb_gamma_table;
char driver_name[] = "viafb";
u32 __user *argp = (u32 __user *) arg;
u32 gpu32;
u32 video_dev_info = 0;
- struct viafb_ioctl_setting viafb_setting = {};
- struct device_t active_dev = {};
DEBUG_MSG(KERN_INFO "viafb_ioctl: 0x%X !!\n", cmd);
+ memset(&u, 0, sizeof(u));
switch (cmd) {
case VIAFB_GET_CHIP_INFO:
@@ -571,7 +573,7 @@ static int viafb_ioctl(struct fb_info *i
return -EFAULT;
break;
case VIAFB_GET_INFO_SIZE:
- return put_user(viainfo_size, argp);
+ return put_user((u32)sizeof(struct viafb_ioctl_info), argp);
case VIAFB_GET_INFO:
return viafb_ioctl_get_viafb_info(arg);
case VIAFB_HOTPLUG:
@@ -584,60 +586,60 @@ static int viafb_ioctl(struct fb_info *i
viafb_hotplug = (gpu32) ? 1 : 0;
break;
case VIAFB_GET_RESOLUTION:
- viamode.xres = (u32) viafb_hotplug_Xres;
- viamode.yres = (u32) viafb_hotplug_Yres;
- viamode.refresh = (u32) viafb_hotplug_refresh;
- viamode.bpp = (u32) viafb_hotplug_bpp;
+ u.viamode.xres = (u32) viafb_hotplug_Xres;
+ u.viamode.yres = (u32) viafb_hotplug_Yres;
+ u.viamode.refresh = (u32) viafb_hotplug_refresh;
+ u.viamode.bpp = (u32) viafb_hotplug_bpp;
if (viafb_SAMM_ON == 1) {
- viamode.xres_sec = viafb_second_xres;
- viamode.yres_sec = viafb_second_yres;
- viamode.virtual_xres_sec = viafb_second_virtual_xres;
- viamode.virtual_yres_sec = viafb_second_virtual_yres;
- viamode.refresh_sec = viafb_refresh1;
- viamode.bpp_sec = viafb_bpp1;
+ u.viamode.xres_sec = viafb_second_xres;
+ u.viamode.yres_sec = viafb_second_yres;
+ u.viamode.virtual_xres_sec = viafb_second_virtual_xres;
+ u.viamode.virtual_yres_sec = viafb_second_virtual_yres;
+ u.viamode.refresh_sec = viafb_refresh1;
+ u.viamode.bpp_sec = viafb_bpp1;
} else {
- viamode.xres_sec = 0;
- viamode.yres_sec = 0;
- viamode.virtual_xres_sec = 0;
- viamode.virtual_yres_sec = 0;
- viamode.refresh_sec = 0;
- viamode.bpp_sec = 0;
+ u.viamode.xres_sec = 0;
+ u.viamode.yres_sec = 0;
+ u.viamode.virtual_xres_sec = 0;
+ u.viamode.virtual_yres_sec = 0;
+ u.viamode.refresh_sec = 0;
+ u.viamode.bpp_sec = 0;
}
- if (copy_to_user(argp, &viamode, sizeof(viamode)))
+ if (copy_to_user(argp, &u.viamode, sizeof(u.viamode)))
return -EFAULT;
break;
case VIAFB_GET_SAMM_INFO:
- viasamm.samm_status = viafb_SAMM_ON;
+ u.viasamm.samm_status = viafb_SAMM_ON;
if (viafb_SAMM_ON == 1) {
if (viafb_dual_fb) {
- viasamm.size_prim = viaparinfo->fbmem_free;
- viasamm.size_sec = viaparinfo1->fbmem_free;
+ u.viasamm.size_prim = viaparinfo->fbmem_free;
+ u.viasamm.size_sec = viaparinfo1->fbmem_free;
} else {
if (viafb_second_size) {
- viasamm.size_prim =
+ u.viasamm.size_prim =
viaparinfo->fbmem_free -
viafb_second_size * 1024 * 1024;
- viasamm.size_sec =
+ u.viasamm.size_sec =
viafb_second_size * 1024 * 1024;
} else {
- viasamm.size_prim =
+ u.viasamm.size_prim =
viaparinfo->fbmem_free >> 1;
- viasamm.size_sec =
+ u.viasamm.size_sec =
(viaparinfo->fbmem_free >> 1);
}
}
- viasamm.mem_base = viaparinfo->fbmem;
- viasamm.offset_sec = viafb_second_offset;
+ u.viasamm.mem_base = viaparinfo->fbmem;
+ u.viasamm.offset_sec = viafb_second_offset;
} else {
- viasamm.size_prim =
+ u.viasamm.size_prim =
viaparinfo->memsize - viaparinfo->fbmem_used;
- viasamm.size_sec = 0;
- viasamm.mem_base = viaparinfo->fbmem;
- viasamm.offset_sec = 0;
+ u.viasamm.size_sec = 0;
+ u.viasamm.mem_base = viaparinfo->fbmem;
+ u.viasamm.offset_sec = 0;
}
- if (copy_to_user(argp, &viasamm, sizeof(viasamm)))
+ if (copy_to_user(argp, &u.viasamm, sizeof(u.viasamm)))
return -EFAULT;
break;
@@ -662,74 +664,75 @@ static int viafb_ioctl(struct fb_info *i
viafb_lcd_disable();
break;
case VIAFB_SET_DEVICE:
- if (copy_from_user(&active_dev, (void *)argp,
- sizeof(active_dev)))
+ if (copy_from_user(&u.active_dev, (void *)argp,
+ sizeof(u.active_dev)))
return -EFAULT;
- viafb_set_device(active_dev);
+ viafb_set_device(u.active_dev);
viafb_set_par(info);
break;
case VIAFB_GET_DEVICE:
- active_dev.crt = viafb_CRT_ON;
- active_dev.dvi = viafb_DVI_ON;
- active_dev.lcd = viafb_LCD_ON;
- active_dev.samm = viafb_SAMM_ON;
- active_dev.primary_dev = viafb_primary_dev;
-
- active_dev.lcd_dsp_cent = viafb_lcd_dsp_method;
- active_dev.lcd_panel_id = viafb_lcd_panel_id;
- active_dev.lcd_mode = viafb_lcd_mode;
-
- active_dev.xres = viafb_hotplug_Xres;
- active_dev.yres = viafb_hotplug_Yres;
-
- active_dev.xres1 = viafb_second_xres;
- active_dev.yres1 = viafb_second_yres;
-
- active_dev.bpp = viafb_bpp;
- active_dev.bpp1 = viafb_bpp1;
- active_dev.refresh = viafb_refresh;
- active_dev.refresh1 = viafb_refresh1;
-
- active_dev.epia_dvi = viafb_platform_epia_dvi;
- active_dev.lcd_dual_edge = viafb_device_lcd_dualedge;
- active_dev.bus_width = viafb_bus_width;
+ u.active_dev.crt = viafb_CRT_ON;
+ u.active_dev.dvi = viafb_DVI_ON;
+ u.active_dev.lcd = viafb_LCD_ON;
+ u.active_dev.samm = viafb_SAMM_ON;
+ u.active_dev.primary_dev = viafb_primary_dev;
+
+ u.active_dev.lcd_dsp_cent = viafb_lcd_dsp_method;
+ u.active_dev.lcd_panel_id = viafb_lcd_panel_id;
+ u.active_dev.lcd_mode = viafb_lcd_mode;
+
+ u.active_dev.xres = viafb_hotplug_Xres;
+ u.active_dev.yres = viafb_hotplug_Yres;
+
+ u.active_dev.xres1 = viafb_second_xres;
+ u.active_dev.yres1 = viafb_second_yres;
+
+ u.active_dev.bpp = viafb_bpp;
+ u.active_dev.bpp1 = viafb_bpp1;
+ u.active_dev.refresh = viafb_refresh;
+ u.active_dev.refresh1 = viafb_refresh1;
+
+ u.active_dev.epia_dvi = viafb_platform_epia_dvi;
+ u.active_dev.lcd_dual_edge = viafb_device_lcd_dualedge;
+ u.active_dev.bus_width = viafb_bus_width;
- if (copy_to_user(argp, &active_dev, sizeof(active_dev)))
+ if (copy_to_user(argp, &u.active_dev, sizeof(u.active_dev)))
return -EFAULT;
break;
case VIAFB_GET_DRIVER_VERSION:
- driver_version.iMajorNum = VERSION_MAJOR;
- driver_version.iKernelNum = VERSION_KERNEL;
- driver_version.iOSNum = VERSION_OS;
- driver_version.iMinorNum = VERSION_MINOR;
+ u.driver_version.iMajorNum = VERSION_MAJOR;
+ u.driver_version.iKernelNum = VERSION_KERNEL;
+ u.driver_version.iOSNum = VERSION_OS;
+ u.driver_version.iMinorNum = VERSION_MINOR;
- if (copy_to_user(argp, &driver_version,
- sizeof(driver_version)))
+ if (copy_to_user(argp, &u.driver_version,
+ sizeof(u.driver_version)))
return -EFAULT;
break;
case VIAFB_SET_DEVICE_INFO:
- if (copy_from_user(&viafb_setting,
- argp, sizeof(viafb_setting)))
+ if (copy_from_user(&u.viafb_setting,
+ argp, sizeof(u.viafb_setting)))
return -EFAULT;
- if (apply_device_setting(viafb_setting, info) < 0)
+ if (apply_device_setting(u.viafb_setting, info) < 0)
return -EINVAL;
break;
case VIAFB_SET_SECOND_MODE:
- if (copy_from_user(&sec_var, argp, sizeof(sec_var)))
+ if (copy_from_user(&u.sec_var, argp, sizeof(u.sec_var)))
return -EFAULT;
- apply_second_mode_setting(&sec_var);
+ apply_second_mode_setting(&u.sec_var);
break;
case VIAFB_GET_DEVICE_INFO:
- retrieve_device_setting(&viafb_setting);
+ retrieve_device_setting(&u.viafb_setting);
- if (copy_to_user(argp, &viafb_setting, sizeof(viafb_setting)))
+ if (copy_to_user(argp, &u.viafb_setting,
+ sizeof(u.viafb_setting)))
return -EFAULT;
break;
@@ -806,51 +809,51 @@ static int viafb_ioctl(struct fb_info *i
break;
case VIAFB_GET_PANEL_MAX_SIZE:
- if (copy_from_user
- (&panel_pos_size_para, argp, sizeof(panel_pos_size_para)))
+ if (copy_from_user(&u.panel_pos_size_para, argp,
+ sizeof(u.panel_pos_size_para)))
return -EFAULT;
- panel_pos_size_para.x = panel_pos_size_para.y = 0;
- if (copy_to_user(argp, &panel_pos_size_para,
- sizeof(panel_pos_size_para)))
+ u.panel_pos_size_para.x = u.panel_pos_size_para.y = 0;
+ if (copy_to_user(argp, &u.panel_pos_size_para,
+ sizeof(u.panel_pos_size_para)))
return -EFAULT;
break;
case VIAFB_GET_PANEL_MAX_POSITION:
- if (copy_from_user
- (&panel_pos_size_para, argp, sizeof(panel_pos_size_para)))
+ if (copy_from_user(&u.panel_pos_size_para, argp,
+ sizeof(u.panel_pos_size_para)))
return -EFAULT;
- panel_pos_size_para.x = panel_pos_size_para.y = 0;
- if (copy_to_user(argp, &panel_pos_size_para,
- sizeof(panel_pos_size_para)))
+ u.panel_pos_size_para.x = u.panel_pos_size_para.y = 0;
+ if (copy_to_user(argp, &u.panel_pos_size_para,
+ sizeof(u.panel_pos_size_para)))
return -EFAULT;
break;
case VIAFB_GET_PANEL_POSITION:
- if (copy_from_user
- (&panel_pos_size_para, argp, sizeof(panel_pos_size_para)))
+ if (copy_from_user(&u.panel_pos_size_para, argp,
+ sizeof(u.panel_pos_size_para)))
return -EFAULT;
- panel_pos_size_para.x = panel_pos_size_para.y = 0;
- if (copy_to_user(argp, &panel_pos_size_para,
- sizeof(panel_pos_size_para)))
+ u.panel_pos_size_para.x = u.panel_pos_size_para.y = 0;
+ if (copy_to_user(argp, &u.panel_pos_size_para,
+ sizeof(u.panel_pos_size_para)))
return -EFAULT;
break;
case VIAFB_GET_PANEL_SIZE:
- if (copy_from_user
- (&panel_pos_size_para, argp, sizeof(panel_pos_size_para)))
+ if (copy_from_user(&u.panel_pos_size_para, argp,
+ sizeof(u.panel_pos_size_para)))
return -EFAULT;
- panel_pos_size_para.x = panel_pos_size_para.y = 0;
- if (copy_to_user(argp, &panel_pos_size_para,
- sizeof(panel_pos_size_para)))
+ u.panel_pos_size_para.x = u.panel_pos_size_para.y = 0;
+ if (copy_to_user(argp, &u.panel_pos_size_para,
+ sizeof(u.panel_pos_size_para)))
return -EFAULT;
break;
case VIAFB_SET_PANEL_POSITION:
- if (copy_from_user
- (&panel_pos_size_para, argp, sizeof(panel_pos_size_para)))
+ if (copy_from_user(&u.panel_pos_size_para, argp,
+ sizeof(u.panel_pos_size_para)))
return -EFAULT;
break;
case VIAFB_SET_PANEL_SIZE:
- if (copy_from_user
- (&panel_pos_size_para, argp, sizeof(panel_pos_size_para)))
+ if (copy_from_user(&u.panel_pos_size_para, argp,
+ sizeof(u.panel_pos_size_para)))
return -EFAULT;
break;
@@ -1052,10 +1055,8 @@ static void viafb_imageblit(struct fb_in
static int viafb_cursor(struct fb_info *info, struct fb_cursor *cursor)
{
- u8 data[CURSOR_SIZE / 8];
- u32 data_bak[CURSOR_SIZE / 32];
u32 temp, xx, yy, bg_col = 0, fg_col = 0;
- int size, i, j = 0;
+ int i, j = 0;
static int hw_cursor;
struct viafb_par *p_viafb_par;
@@ -1178,22 +1179,29 @@ static int viafb_cursor(struct fb_info *
}
if (cursor->set & FB_CUR_SETSHAPE) {
- size =
+ struct {
+ u8 data[CURSOR_SIZE / 8];
+ u32 bak[CURSOR_SIZE / 32];
+ } *cr_data = kzalloc(sizeof(*cr_data), GFP_ATOMIC);
+ int size =
((viacursor.image.width + 7) >> 3) *
viacursor.image.height;
+ if (cr_data == NULL)
+ goto out;
+
if (MAX_CURS == 32) {
for (i = 0; i < (CURSOR_SIZE / 32); i++) {
- data_bak[i] = 0x0;
- data_bak[i + 1] = 0xFFFFFFFF;
+ cr_data->bak[i] = 0x0;
+ cr_data->bak[i + 1] = 0xFFFFFFFF;
i += 1;
}
} else if (MAX_CURS == 64) {
for (i = 0; i < (CURSOR_SIZE / 32); i++) {
- data_bak[i] = 0x0;
- data_bak[i + 1] = 0x0;
- data_bak[i + 2] = 0xFFFFFFFF;
- data_bak[i + 3] = 0xFFFFFFFF;
+ cr_data->bak[i] = 0x0;
+ cr_data->bak[i + 1] = 0x0;
+ cr_data->bak[i + 2] = 0xFFFFFFFF;
+ cr_data->bak[i + 3] = 0xFFFFFFFF;
i += 3;
}
}
@@ -1201,12 +1209,12 @@ static int viafb_cursor(struct fb_info *
switch (viacursor.rop) {
case ROP_XOR:
for (i = 0; i < size; i++)
- data[i] = viacursor.mask[i];
+ cr_data->data[i] = viacursor.mask[i];
break;
case ROP_COPY:
for (i = 0; i < size; i++)
- data[i] = viacursor.mask[i];
+ cr_data->data[i] = viacursor.mask[i];
break;
default:
break;
@@ -1214,23 +1222,25 @@ static int viafb_cursor(struct fb_info *
if (MAX_CURS == 32) {
for (i = 0; i < size; i++) {
- data_bak[j] = (u32) data[i];
- data_bak[j + 1] = ~data_bak[j];
+ cr_data->bak[j] = (u32) cr_data->data[i];
+ cr_data->bak[j + 1] = ~cr_data->bak[j];
j += 2;
}
} else if (MAX_CURS == 64) {
for (i = 0; i < size; i++) {
- data_bak[j] = (u32) data[i];
- data_bak[j + 1] = 0x0;
- data_bak[j + 2] = ~data_bak[j];
- data_bak[j + 3] = ~data_bak[j + 1];
+ cr_data->bak[j] = (u32) cr_data->data[i];
+ cr_data->bak[j + 1] = 0x0;
+ cr_data->bak[j + 2] = ~cr_data->bak[j];
+ cr_data->bak[j + 3] = ~cr_data->bak[j + 1];
j += 4;
}
}
memcpy(((struct viafb_par *)(info->par))->fbmem_virt +
((struct viafb_par *)(info->par))->cursor_start,
- data_bak, CURSOR_SIZE);
+ cr_data->bak, CURSOR_SIZE);
+out:
+ kfree(cr_data);
}
if (viacursor.enable)
On Mon, 10 Nov 2008 22:00:46 +0100
Bruno Pr__mont <[email protected]> wrote:
> During conversion of viafb_ioctl() I noticed the following:
>
> Those two cases just copy_from_user and do nothing with copied data,
> incomplete implementation?:
> case VIAFB_SET_PANEL_POSITION:
> if (copy_from_user(&u.panel_pos_size_para, argp,
> sizeof(u.panel_pos_size_para)))
> return -EFAULT;
> break;
> case VIAFB_SET_PANEL_SIZE:
> if (copy_from_user(&u.panel_pos_size_para, argp,
> sizeof(u.panel_pos_size_para)))
> return -EFAULT;
> break;
>
> Handling of GET/SET GAMMA looks buggy:
> In each case 256*4 bytes are allocated but only 4 bytes (size of pointer)
> are copied to/from userspace though viafb_(get|set)_gamma_table() operates
> on the full 256 elements...
> case VIAFB_SET_GAMMA_LUT:
> viafb_gamma_table = kmalloc(256 * sizeof(u32), GFP_KERNEL);
> if (!viafb_gamma_table)
> return -ENOMEM;
> if (copy_from_user(viafb_gamma_table, argp,
> sizeof(viafb_gamma_table))) {
> kfree(viafb_gamma_table);
> return -EFAULT;
> }
> viafb_set_gamma_table(viafb_bpp, viafb_gamma_table);
> kfree(viafb_gamma_table);
> break;
>
> case VIAFB_GET_GAMMA_LUT:
> viafb_gamma_table = kmalloc(256 * sizeof(u32), GFP_KERNEL);
> if (!viafb_gamma_table)
> return -ENOMEM;
> viafb_get_gamma_table(viafb_gamma_table);
> if (copy_to_user(argp, viafb_gamma_table,
> sizeof(viafb_gamma_table))) {
> kfree(viafb_gamma_table);
> return -EFAULT;
> }
> kfree(viafb_gamma_table);
> break;
>
> I don't know if there is a userspace app that uses these VIA IOCTLs...
> so the ioctl part is just compile-tested.
> After checking, fbset just uses some generic framebuffer IOCTLs outside
> of viafb's scope, thus not passing through viafb_ioctl().
>
>
>
> ---
> --- linux-2.6.28-rc3/drivers/video/via/viafbdev.c.orig 2008-11-09 19:36:47.000000000 +0100
> +++ linux-2.6.28-rc3/drivers/video/via/viafbdev.c 2008-11-10 20:50:32.000000000 +0100
hm, OK, I dropped the old patch and merged this one.
It'd be nice to have it runtime tested and reviewed by Joseph, but we
haven't heard from him yet. viafb might be 8k-stacks-only in 2.6.27,
which would be bad.
I will try this new patch later. :)
BRs,
Joseph Chan
-----Original Message-----
From: Andrew Morton [mailto:[email protected]]
Sent: Thursday, November 13, 2008 7:01 AM
To: Bruno Pr?mont
Cc: [email protected]; Joseph Chan; [email protected]; [email protected]
Subject: Re: [PATCH] Fix crash in viafb due to 4k stack overflow
On Mon, 10 Nov 2008 22:00:46 +0100
Bruno Pr__mont <[email protected]> wrote:
> During conversion of viafb_ioctl() I noticed the following:
>
> Those two cases just copy_from_user and do nothing with copied data,
> incomplete implementation?:
> case VIAFB_SET_PANEL_POSITION:
> if (copy_from_user(&u.panel_pos_size_para, argp,
> sizeof(u.panel_pos_size_para)))
> return -EFAULT;
> break;
> case VIAFB_SET_PANEL_SIZE:
> if (copy_from_user(&u.panel_pos_size_para, argp,
> sizeof(u.panel_pos_size_para)))
> return -EFAULT;
> break;
>
> Handling of GET/SET GAMMA looks buggy:
> In each case 256*4 bytes are allocated but only 4 bytes (size of
> pointer) are copied to/from userspace though
> viafb_(get|set)_gamma_table() operates on the full 256 elements...
> case VIAFB_SET_GAMMA_LUT:
> viafb_gamma_table = kmalloc(256 * sizeof(u32), GFP_KERNEL);
> if (!viafb_gamma_table)
> return -ENOMEM;
> if (copy_from_user(viafb_gamma_table, argp,
> sizeof(viafb_gamma_table))) {
> kfree(viafb_gamma_table);
> return -EFAULT;
> }
> viafb_set_gamma_table(viafb_bpp, viafb_gamma_table);
> kfree(viafb_gamma_table);
> break;
>
> case VIAFB_GET_GAMMA_LUT:
> viafb_gamma_table = kmalloc(256 * sizeof(u32), GFP_KERNEL);
> if (!viafb_gamma_table)
> return -ENOMEM;
> viafb_get_gamma_table(viafb_gamma_table);
> if (copy_to_user(argp, viafb_gamma_table,
> sizeof(viafb_gamma_table))) {
> kfree(viafb_gamma_table);
> return -EFAULT;
> }
> kfree(viafb_gamma_table);
> break;
>
> I don't know if there is a userspace app that uses these VIA IOCTLs...
> so the ioctl part is just compile-tested.
> After checking, fbset just uses some generic framebuffer IOCTLs
> outside of viafb's scope, thus not passing through viafb_ioctl().
>
>
>
> ---
> --- linux-2.6.28-rc3/drivers/video/via/viafbdev.c.orig 2008-11-09 19:36:47.000000000 +0100
> +++ linux-2.6.28-rc3/drivers/video/via/viafbdev.c 2008-11-10 20:50:32.000000000 +0100
hm, OK, I dropped the old patch and merged this one.
It'd be nice to have it runtime tested and reviewed by Joseph, but we haven't heard from him yet. viafb might be 8k-stacks-only in 2.6.27, which would be bad.
After testing the patch with 2.6.28-rc4 kernel, I found something strange in both viafb built-in and module modes.
See the result on my EPIA-EX (CX700) below. Based on the result, it looks like something wrong with the new patch.
Kernel 2.6.28-rc4 + viafb-fix-crash-due-to-4k-stack-overflow.patch
1. w/o patching the viafb-fix-crash-due-to-4k-stack-overflow.patch
=> System works properly without "kernel panic" issue.
2. w/ patching the viafb-fix-crash-due-to-4k-stack-overflow.patch
=> System hangs with "kernel panic" randomly after loading the viafb module.
Such as:
BUG: unable to handle kernel NULL pointer dereference at 000000c7
IP: [<c03dc6ec>] rt_worker_func+0xc7/0x15d
Last sysfs file: /sys/class/graphics/fb0/dev
Modules linked in: viafb i2c_algo_bit via_rhine sg pata_via sd_mod ehci_hcd
Pid: 6, comm: events/0 Not tainted (2.6.28-rc4b #1) pn test
EIP: 0060:[<c03dc6ec>] EFLAGS: 00010286 CPU:0
EIP is at rt_worker_func+0xc7/0x15d
......
....
.....
BRs,
Joseph
-----Original Message-----
From: Joseph Chan
Sent: Thursday, November 13, 2008 8:58 AM
To: 'Andrew Morton'; Bruno Pr?mont
Cc: [email protected]; [email protected]; [email protected]
Subject: RE: [PATCH] Fix crash in viafb due to 4k stack overflow
I will try this new patch later. :)
BRs,
Joseph Chan
-----Original Message-----
From: Andrew Morton [mailto:[email protected]]
Sent: Thursday, November 13, 2008 7:01 AM
To: Bruno Pr?mont
Cc: [email protected]; Joseph Chan; [email protected]; [email protected]
Subject: Re: [PATCH] Fix crash in viafb due to 4k stack overflow
On Mon, 10 Nov 2008 22:00:46 +0100
Bruno Pr__mont <[email protected]> wrote:
> During conversion of viafb_ioctl() I noticed the following:
>
> Those two cases just copy_from_user and do nothing with copied data,
> incomplete implementation?:
> case VIAFB_SET_PANEL_POSITION:
> if (copy_from_user(&u.panel_pos_size_para, argp,
> sizeof(u.panel_pos_size_para)))
> return -EFAULT;
> break;
> case VIAFB_SET_PANEL_SIZE:
> if (copy_from_user(&u.panel_pos_size_para, argp,
> sizeof(u.panel_pos_size_para)))
> return -EFAULT;
> break;
>
> Handling of GET/SET GAMMA looks buggy:
> In each case 256*4 bytes are allocated but only 4 bytes (size of
> pointer) are copied to/from userspace though
> viafb_(get|set)_gamma_table() operates on the full 256 elements...
> case VIAFB_SET_GAMMA_LUT:
> viafb_gamma_table = kmalloc(256 * sizeof(u32), GFP_KERNEL);
> if (!viafb_gamma_table)
> return -ENOMEM;
> if (copy_from_user(viafb_gamma_table, argp,
> sizeof(viafb_gamma_table))) {
> kfree(viafb_gamma_table);
> return -EFAULT;
> }
> viafb_set_gamma_table(viafb_bpp, viafb_gamma_table);
> kfree(viafb_gamma_table);
> break;
>
> case VIAFB_GET_GAMMA_LUT:
> viafb_gamma_table = kmalloc(256 * sizeof(u32), GFP_KERNEL);
> if (!viafb_gamma_table)
> return -ENOMEM;
> viafb_get_gamma_table(viafb_gamma_table);
> if (copy_to_user(argp, viafb_gamma_table,
> sizeof(viafb_gamma_table))) {
> kfree(viafb_gamma_table);
> return -EFAULT;
> }
> kfree(viafb_gamma_table);
> break;
>
> I don't know if there is a userspace app that uses these VIA IOCTLs...
> so the ioctl part is just compile-tested.
> After checking, fbset just uses some generic framebuffer IOCTLs
> outside of viafb's scope, thus not passing through viafb_ioctl().
>
>
>
> ---
> --- linux-2.6.28-rc3/drivers/video/via/viafbdev.c.orig 2008-11-09 19:36:47.000000000 +0100
> +++ linux-2.6.28-rc3/drivers/video/via/viafbdev.c 2008-11-10 20:50:32.000000000 +0100
hm, OK, I dropped the old patch and merged this one.
It'd be nice to have it runtime tested and reviewed by Joseph, but we haven't heard from him yet. viafb might be 8k-stacks-only in 2.6.27, which would be bad.
Joseph,
Are you testing with 4k stacks or with 8k stacks (CONFIG_4KSTACKS)?
Note that your failure looks pretty much like the ones I mentionned in
http://lkml.org/lkml/2008/11/9/204
It looks like some kernel text/data got corrupted though I have no
idea what caused it. For me this kind of NULL pointer dereference did
happen a few times then stopped showing up (just a reboot, not kernel
change/recompile)
Bruno
On Fri, 14 Nov 2008 <[email protected]> wrote:
> After testing the patch with 2.6.28-rc4 kernel, I found something
> strange in both viafb built-in and module modes. See the result on my
> EPIA-EX (CX700) below. Based on the result, it looks like something
> wrong with the new patch.
>
> Kernel 2.6.28-rc4 + viafb-fix-crash-due-to-4k-stack-overflow.patch
>
> 1. w/o patching the viafb-fix-crash-due-to-4k-stack-overflow.patch
> => System works properly without "kernel panic" issue.
>
> 2. w/ patching the viafb-fix-crash-due-to-4k-stack-overflow.patch
> => System hangs with "kernel panic" randomly after loading the viafb
> module. Such as:
> BUG: unable to handle kernel NULL pointer dereference at 000000c7
> IP: [<c03dc6ec>] rt_worker_func+0xc7/0x15d
> Last sysfs file: /sys/class/graphics/fb0/dev
> Modules linked in: viafb i2c_algo_bit via_rhine sg pata_via sd_mod
> ehci_hcd
>
> Pid: 6, comm: events/0 Not tainted (2.6.28-rc4b #1) pn test
> EIP: 0060:[<c03dc6ec>] EFLAGS: 00010286 CPU:0
> EIP is at rt_worker_func+0xc7/0x15d
> ......
> ....
> .....
>
>
> BRs,
> Joseph
>
> -----Original Message-----
> From: Joseph Chan
> Sent: Thursday, November 13, 2008 8:58 AM
> To: 'Andrew Morton'; Bruno Prémont
> Cc: [email protected]; [email protected];
> [email protected] Subject: RE: [PATCH] Fix crash in viafb
> due to 4k stack overflow
>
> I will try this new patch later. :)
>
> BRs,
> Joseph Chan
>
> -----Original Message-----
> From: Andrew Morton [mailto:[email protected]]
> Sent: Thursday, November 13, 2008 7:01 AM
> To: Bruno Prémont
> Cc: [email protected]; Joseph Chan;
> [email protected]; [email protected]
> Subject: Re: [PATCH] Fix crash in viafb due to 4k stack overflow
>
> On Mon, 10 Nov 2008 22:00:46 +0100
> Bruno Pr__mont <[email protected]> wrote:
>
> > During conversion of viafb_ioctl() I noticed the following:
> >
> > Those two cases just copy_from_user and do nothing with copied
> > data, incomplete implementation?:
> > case VIAFB_SET_PANEL_POSITION:
> > if (copy_from_user(&u.panel_pos_size_para, argp,
> > sizeof(u.panel_pos_size_para)))
> > return -EFAULT;
> > break;
> > case VIAFB_SET_PANEL_SIZE:
> > if (copy_from_user(&u.panel_pos_size_para, argp,
> > sizeof(u.panel_pos_size_para)))
> > return -EFAULT;
> > break;
> >
> > Handling of GET/SET GAMMA looks buggy:
> > In each case 256*4 bytes are allocated but only 4 bytes (size of
> > pointer) are copied to/from userspace though
> > viafb_(get|set)_gamma_table() operates on the full 256 elements...
> > case VIAFB_SET_GAMMA_LUT:
> > viafb_gamma_table = kmalloc(256 * sizeof(u32),
> > GFP_KERNEL); if (!viafb_gamma_table)
> > return -ENOMEM;
> > if (copy_from_user(viafb_gamma_table, argp,
> > sizeof(viafb_gamma_table))) {
> > kfree(viafb_gamma_table);
> > return -EFAULT;
> > }
> > viafb_set_gamma_table(viafb_bpp, viafb_gamma_table);
> > kfree(viafb_gamma_table);
> > break;
> >
> > case VIAFB_GET_GAMMA_LUT:
> > viafb_gamma_table = kmalloc(256 * sizeof(u32),
> > GFP_KERNEL); if (!viafb_gamma_table)
> > return -ENOMEM;
> > viafb_get_gamma_table(viafb_gamma_table);
> > if (copy_to_user(argp, viafb_gamma_table,
> > sizeof(viafb_gamma_table))) {
> > kfree(viafb_gamma_table);
> > return -EFAULT;
> > }
> > kfree(viafb_gamma_table);
> > break;
> >
> > I don't know if there is a userspace app that uses these VIA
> > IOCTLs... so the ioctl part is just compile-tested.
> > After checking, fbset just uses some generic framebuffer IOCTLs
> > outside of viafb's scope, thus not passing through viafb_ioctl().
> >
> >
> >
> > ---
> > --- linux-2.6.28-rc3/drivers/video/via/viafbdev.c.orig
> > 2008-11-09 19:36:47.000000000 +0100 +++
> > linux-2.6.28-rc3/drivers/video/via/viafbdev.c 2008-11-10
> > 20:50:32.000000000 +0100
>
> hm, OK, I dropped the old patch and merged this one.
>
> It'd be nice to have it runtime tested and reviewed by Joseph, but we
> haven't heard from him yet. viafb might be 8k-stacks-only in 2.6.27,
> which would be bad.
>
8K stacks, I don't enable the CONFIG_4KSTACKS.
In addition, I also tried to enable CONFIG_4KSTACKS, the system still returns me lots "panic" during system bootup. :(
BRs,
Joseph Chan
-----Original Message-----
From: Bruno Prémont [mailto:[email protected]]
Sent: Friday, November 14, 2008 5:02 PM
To: Joseph Chan
Cc: [email protected]; [email protected]; [email protected]; [email protected]; Scott Fang
Subject: Re: [PATCH] Fix crash in viafb due to 4k stack overflow
Joseph,
Are you testing with 4k stacks or with 8k stacks (CONFIG_4KSTACKS)?
Note that your failure looks pretty much like the ones I mentionned in
http://lkml.org/lkml/2008/11/9/204
It looks like some kernel text/data got corrupted though I have no idea what caused it. For me this kind of NULL pointer dereference did happen a few times then stopped showing up (just a reboot, not kernel
change/recompile)
Bruno
On Fri, 14 Nov 2008 <[email protected]> wrote:
> After testing the patch with 2.6.28-rc4 kernel, I found something
> strange in both viafb built-in and module modes. See the result on my
> EPIA-EX (CX700) below. Based on the result, it looks like something
> wrong with the new patch.
>
> Kernel 2.6.28-rc4 + viafb-fix-crash-due-to-4k-stack-overflow.patch
>
> 1. w/o patching the viafb-fix-crash-due-to-4k-stack-overflow.patch
> => System works properly without "kernel panic" issue.
>
> 2. w/ patching the viafb-fix-crash-due-to-4k-stack-overflow.patch
> => System hangs with "kernel panic" randomly after loading the viafb
> module. Such as:
> BUG: unable to handle kernel NULL pointer dereference at 000000c7
> IP: [<c03dc6ec>] rt_worker_func+0xc7/0x15d Last sysfs file:
> /sys/class/graphics/fb0/dev Modules linked in: viafb i2c_algo_bit
> via_rhine sg pata_via sd_mod ehci_hcd
>
> Pid: 6, comm: events/0 Not tainted (2.6.28-rc4b #1) pn test
> EIP: 0060:[<c03dc6ec>] EFLAGS: 00010286 CPU:0 EIP is at
> rt_worker_func+0xc7/0x15d ......
> ....
> .....
>
>
> BRs,
> Joseph
>
> -----Original Message-----
> From: Joseph Chan
> Sent: Thursday, November 13, 2008 8:58 AM
> To: 'Andrew Morton'; Bruno Prémont
> Cc: [email protected]; [email protected];
> [email protected] Subject: RE: [PATCH] Fix crash in viafb
> due to 4k stack overflow
>
> I will try this new patch later. :)
>
> BRs,
> Joseph Chan
>
> -----Original Message-----
> From: Andrew Morton [mailto:[email protected]]
> Sent: Thursday, November 13, 2008 7:01 AM
> To: Bruno Prémont
> Cc: [email protected]; Joseph Chan;
> [email protected]; [email protected]
> Subject: Re: [PATCH] Fix crash in viafb due to 4k stack overflow
>
> On Mon, 10 Nov 2008 22:00:46 +0100
> Bruno Pr__mont <[email protected]> wrote:
>
> > During conversion of viafb_ioctl() I noticed the following:
> >
> > Those two cases just copy_from_user and do nothing with copied data,
> > incomplete implementation?:
> > case VIAFB_SET_PANEL_POSITION:
> > if (copy_from_user(&u.panel_pos_size_para, argp,
> > sizeof(u.panel_pos_size_para)))
> > return -EFAULT;
> > break;
> > case VIAFB_SET_PANEL_SIZE:
> > if (copy_from_user(&u.panel_pos_size_para, argp,
> > sizeof(u.panel_pos_size_para)))
> > return -EFAULT;
> > break;
> >
> > Handling of GET/SET GAMMA looks buggy:
> > In each case 256*4 bytes are allocated but only 4 bytes (size of
> > pointer) are copied to/from userspace though
> > viafb_(get|set)_gamma_table() operates on the full 256 elements...
> > case VIAFB_SET_GAMMA_LUT:
> > viafb_gamma_table = kmalloc(256 * sizeof(u32),
> > GFP_KERNEL); if (!viafb_gamma_table)
> > return -ENOMEM;
> > if (copy_from_user(viafb_gamma_table, argp,
> > sizeof(viafb_gamma_table))) {
> > kfree(viafb_gamma_table);
> > return -EFAULT;
> > }
> > viafb_set_gamma_table(viafb_bpp, viafb_gamma_table);
> > kfree(viafb_gamma_table);
> > break;
> >
> > case VIAFB_GET_GAMMA_LUT:
> > viafb_gamma_table = kmalloc(256 * sizeof(u32),
> > GFP_KERNEL); if (!viafb_gamma_table)
> > return -ENOMEM;
> > viafb_get_gamma_table(viafb_gamma_table);
> > if (copy_to_user(argp, viafb_gamma_table,
> > sizeof(viafb_gamma_table))) {
> > kfree(viafb_gamma_table);
> > return -EFAULT;
> > }
> > kfree(viafb_gamma_table);
> > break;
> >
> > I don't know if there is a userspace app that uses these VIA
> > IOCTLs... so the ioctl part is just compile-tested.
> > After checking, fbset just uses some generic framebuffer IOCTLs
> > outside of viafb's scope, thus not passing through viafb_ioctl().
> >
> >
> >
> > ---
> > --- linux-2.6.28-rc3/drivers/video/via/viafbdev.c.orig
> > 2008-11-09 19:36:47.000000000 +0100 +++
> > linux-2.6.28-rc3/drivers/video/via/viafbdev.c 2008-11-10
> > 20:50:32.000000000 +0100
>
> hm, OK, I dropped the old patch and merged this one.
>
> It'd be nice to have it runtime tested and reviewed by Joseph, but we
> haven't heard from him yet. viafb might be 8k-stacks-only in 2.6.27,
> which would be bad.
>
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?