From mboxrd@z Thu Jan 1 00:00:00 1970 From: Bruno =?UTF-8?B?UHLDqW1vbnQ=?= Date: Wed, 26 May 2010 19:58:29 +0000 Subject: Re: vfree() and mmap()ed framebuffer with defio (Was: Deadlock Message-Id: <20100526215829.28a4aa47@neptune.home> List-Id: References: <20100509184911.3f136b77@neptune.home> <20100510080047.5adade6f@pluto.restena.lu> In-Reply-To: <20100510080047.5adade6f@pluto.restena.lu> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable To: Jaya Kumar Cc: linux-fbdev@vger.kernel.org, linux-kernel@vger.kernel.org, Jiri Kosina Hi Jaya, On Mon, 10 May 2010 Bruno Pr=C3=A9mont wrote: > I want to make sure that I won't be accessing the old framebuffer after > freeing it and also that defio monitors the new framebuffer. >=20 > For that reason I defio_cleanup(), replace framebuffer, defio_init(). > All of this happens while do_fb_ioctl() holds lock on fb_info and > console_sem. I've slightly changed the code path when releasing old valloc()ed framebuffer. Now I don't call defio_init/cleanup while framebuffer is registered so the deadlock cannot happen. But just calling vfree() to dispose old framebuffer is not sufficient when it was mmap()ed. Do you know how I could "get rid" of that old framebuffer in a proper way? Currently I'm getting the complaints from kernel when the process that mmap()ed frambuffer exits after depth changed via set_par() or even a "BUG: unable to handle kernel paging request" (depending on what exactly I do (pertinent hunk of patch, full patch at the end): @@ -574,7 +611,10 @@ static int picolcd_set_par(struct fb_info *info) info->screen_base =3D (char __force __iomem *)n_fb; info->fix.smem_start =3D (unsigned long)n_fb; info->fix.smem_len =3D PICOLCDFB_SIZE*data->fb_bpp; - fb_deferred_io_init(info); + for (i =3D 0; i < o_len; i +=3D PAGE_SIZE) { + struct page *page =3D vmalloc_to_page(o_fb + i); + page->mapping =3D NULL; + } vfree(o_fb); return 0; } If I run the for loop I end up with the "unable to handle paging request" B= UG: [ 4364.740056] BUG: unable to handle kernel paging request at 84db1980 [ 4364.741115] IP: [] __wake_up_bit+0xf/0x30 [ 4364.741806] *pde =3D 00000000=20 [ 4364.742165] Oops: 0000 [#1]=20 [ 4364.742526] last sysfs file: /sys/devices/platform/via_cputemp.0/temp1_i= nput [ 4364.743559] Modules linked in: hid_picolcd e_powersaver via_cputemp snd_= hda_codec_via snd_hda_intel snd_hda_codec backlight snd_pcm lcd led_class s= nd_timer fb_sys_fops sysimgblt sysfillrect syscopyarea snd soundcore snd_pa= ge_alloc sg [last unloaded: hid_picolcd] [ 4364.747026]=20 [ 4364.747393] Pid: 5, comm: events/0 Tainted: G B 2.6.34-venus #46= CX700+W697HG/CX700+W697HG [ 4364.748855] EIP: 0060:[] EFLAGS: 00010296 CPU: 0 [ 4364.749603] EIP is at __wake_up_bit+0xf/0x30 [ 4364.750012] EAX: 84db1980 EBX: 84db1980 ECX: 00000000 EDX: c14b6330 [ 4364.750012] ESI: f69b4874 EDI: f69b4864 EBP: f704af54 ESP: f704af44 [ 4364.750012] DS: 007b ES: 007b FS: 0000 GS: 0000 SS: 0068 [ 4364.750012] Process events/0 (pid: 5, ti=F704a000 task=F704edc0 task.ti= =F704a000) [ 4364.750012] Stack: [ 4364.750012] 00000000 c14b6330 00000000 c14b65cc f704af60 c105b39d c14b6= 330 f704af7c [ 4364.750012] <0> c11a2e96 f69b4868 f69b2800 f69b2a1c f7003300 c11a2e50 f7= 04afc0 c1035056 [ 4364.750012] <0> c10228ef f704e000 f704edc0 f704e000 f704efa0 f704edc0 f7= 003308 00000000 [ 4364.750012] Call Trace: [ 4364.750012] [] ? unlock_page+0x3d/0x40 [ 4364.750012] [] ? fb_deferred_io_work+0x46/0xc0 [ 4364.750012] [] ? fb_deferred_io_work+0x0/0xc0 [ 4364.750012] [] ? worker_thread+0xd6/0x180 [ 4364.750012] [] ? finish_task_switch+0x2f/0x80 [ 4364.750012] [] ? autoremove_wake_function+0x0/0x50 [ 4364.750012] [] ? worker_thread+0x0/0x180 [ 4364.750012] [] ? kthread+0x6c/0x80 [ 4364.750012] [] ? kthread+0x0/0x80 [ 4364.750012] [] ? kernel_thread_helper+0x6/0x10 [ 4364.750012] Code: 20 5d 4b c1 2b 8b cc 02 00 00 d3 e8 c1 e0 03 03 83 c4 = 02 00 00 5b 5d c3 8d 74 26 00 55 89 e5 53 89 c3 83 ec 0c 89 55 f4 89 4d f8 = <3b> 00 74 17 8d 45 f4 b9 01 00 00 00 89 04 24 ba 03 00 00 00 89=20 [ 4364.750012] EIP: [] __wake_up_bit+0xf/0x30 SS:ESP 0068:f704af44 [ 4364.750012] CR2: 0000000084db1980 [ 4364.772281] ---[ end trace a9d32c215502d31b ]--- If I don't do the for loop I get on of these for each page of the old framebuffer still mmap()ed by userspace: [ 1301.041395] BUG: Bad page state in process links pfn:3b8fa [ 1301.041867] page:c1d02f40 count:0 mapcount:0 mapping:f6a23be8 index:0x0 [ 1301.042641] page flags: 0x80000014(referenced|dirty) [ 1301.043420] Pid: 1688, comm: links Not tainted 2.6.34-venus #46 [ 1301.044552] Call Trace: [ 1301.044972] [] bad_page+0x82/0xd0 [ 1301.045727] [] free_hot_cold_page+0x191/0x310 [ 1301.046497] [] put_page+0x3e/0x100 [ 1301.047256] [] free_page_and_swap_cache+0x1d/0x40 [ 1301.048056] [] unmap_vmas+0x2a8/0x580 [ 1301.048846] [] ? dequeue_task_fair+0x27/0x1d0 [ 1301.049640] [] exit_mmap+0x97/0x110 [ 1301.050477] [] mmput+0x26/0x90 [ 1301.051259] [] exit_mm+0xb1/0xc0 [ 1301.052045] [] do_exit+0xba/0x6b0 [ 1301.052848] [] ? recalc_sigpending+0x11/0x30 [ 1301.053660] [] ? dequeue_signal+0x2c/0x130 [ 1301.054480] [] do_group_exit+0x2d/0x70 [ 1301.055293] [] get_signal_to_deliver+0x17c/0x370 [ 1301.056103] [] ? current_fs_time+0x16/0x20 [ 1301.056892] [] do_notify_resume+0x98/0x820 [ 1301.057690] [] ? tty_unthrottle+0x3e/0x50 [ 1301.058490] [] ? current_fs_time+0x16/0x20 [ 1301.059303] [] ? n_tty_read+0x0/0x680 [ 1301.060134] [] ? sys_select+0x3d/0xb0 [ 1301.060935] [] ? sys_gettimeofday+0x2b/0x70 [ 1301.061756] [] work_notifysig+0x13/0x19 What do I have to do to prevent both errors? I would guess unmapping the old framebuffer from userspace (or maybe fail set_par() with -EBUSY? - if so how do I determine if the framebuffer was mmap()ed?) Any suggestion is welcome! Thanks, Bruno --- Below patch fixes multiple issues running fbcon on top of hid-picolcd's framebuffer but still has issues on depth change (c.f. above). Fixed issues: - NULL pointer deref in 8-bpp mode when fbcon wants to parse access color map - bit-order for 1-bpp is the opposite from the one I expected - too quick update of framebuffer after registration can cause parts to the framebuffer to not be pushed to device diff --git a/drivers/hid/hid-picolcd.c b/drivers/hid/hid-picolcd.c index 95253b3..599bf4b 100644 --- a/drivers/hid/hid-picolcd.c +++ b/drivers/hid/hid-picolcd.c @@ -26,6 +26,7 @@ =20 #include #include +#include #include #include =20 @@ -127,6 +128,26 @@ static const struct fb_var_screeninfo picolcdfb_var = =3D { .height =3D 26, .bits_per_pixel =3D 1, .grayscale =3D 1, + .red =3D { + .offset =3D 0, + .length =3D 1, + .msb_right =3D 0, + }, + .green =3D { + .offset =3D 0, + .length =3D 1, + .msb_right =3D 0, + }, + .blue =3D { + .offset =3D 0, + .length =3D 1, + .msb_right =3D 0, + }, + .transp =3D { + .offset =3D 0, + .length =3D 0, + .msb_right =3D 0, + }, }; #endif /* CONFIG_HID_PICOLCD_FB */ =20 @@ -188,6 +209,7 @@ struct picolcd_data { /* Framebuffer stuff */ u8 fb_update_rate; u8 fb_bpp; + u8 fb_force; u8 *fb_vbitmap; /* local copy of what was sent to PicoLCD */ u8 *fb_bitmap; /* framebuffer */ struct fb_info *fb_info; @@ -346,7 +368,7 @@ static int picolcd_fb_update_tile(u8 *vbitmap, const u8= *bitmap, int bpp, const u8 *bdata =3D bitmap + tile * 256 + chip * 8 + b * 32; for (i =3D 0; i < 64; i++) { tdata[i] <<=3D 1; - tdata[i] |=3D (bdata[i/8] >> (7 - i % 8)) & 0x01; + tdata[i] |=3D (bdata[i/8] >> (i % 8)) & 0x01; } } } else if (bpp =3D 8) { @@ -399,13 +421,10 @@ static int picolcd_fb_reset(struct picolcd_data *data= , int clear) =20 if (data->fb_bitmap) { if (clear) { - memset(data->fb_vbitmap, 0xff, PICOLCDFB_SIZE); + memset(data->fb_vbitmap, 0, PICOLCDFB_SIZE); memset(data->fb_bitmap, 0, PICOLCDFB_SIZE*data->fb_bpp); - } else { - /* invert 1 byte in each tile to force resend */ - for (i =3D 0; i < PICOLCDFB_SIZE; i +=3D 64) - data->fb_vbitmap[i] =3D ~data->fb_vbitmap[i]; } + data->fb_force =3D 1; } =20 /* schedule first output of framebuffer */ @@ -421,6 +440,9 @@ static void picolcd_fb_update(struct picolcd_data *data) int chip, tile, n; unsigned long flags; =20 + if (!data) + return; + spin_lock_irqsave(&data->lock, flags); if (!(data->status & PICOLCD_READY_FB)) { spin_unlock_irqrestore(&data->lock, flags); @@ -440,14 +462,18 @@ static void picolcd_fb_update(struct picolcd_data *da= ta) for (chip =3D 0; chip < 4; chip++) for (tile =3D 0; tile < 8; tile++) if (picolcd_fb_update_tile(data->fb_vbitmap, - data->fb_bitmap, data->fb_bpp, chip, tile)) { + data->fb_bitmap, data->fb_bpp, chip, tile) || + data->fb_force) { n +=3D 2; + if (!data->fb_info->par) + return; /* device lost! */ if (n >=3D HID_OUTPUT_FIFO_SIZE / 2) { usbhid_wait_io(data->hdev); n =3D 0; } picolcd_fb_send_tile(data->hdev, chip, tile); } + data->fb_force =3D false; if (n) usbhid_wait_io(data->hdev); } @@ -526,10 +552,17 @@ static int picolcd_fb_check_var(struct fb_var_screeni= nfo *var, struct fb_info *i /* only allow 1/8 bit depth (8-bit is grayscale) */ *var =3D picolcdfb_var; var->activate =3D activate; - if (bpp >=3D 8) + if (bpp >=3D 8) { var->bits_per_pixel =3D 8; - else + var->red.length =3D 8; + var->green.length =3D 8; + var->blue.length =3D 8; + } else { var->bits_per_pixel =3D 1; + var->red.length =3D 1; + var->green.length =3D 1; + var->blue.length =3D 1; + } return 0; } =20 @@ -537,18 +570,22 @@ static int picolcd_set_par(struct fb_info *info) { struct picolcd_data *data =3D info->par; u8 *o_fb, *n_fb; + int i; + unsigned long o_len; + if (!data) + return -ENODEV; if (info->var.bits_per_pixel =3D data->fb_bpp) return 0; /* switch between 1/8 bit depths */ if (info->var.bits_per_pixel !=3D 1 && info->var.bits_per_pixel !=3D 8) return -EINVAL; =20 - o_fb =3D data->fb_bitmap; - n_fb =3D vmalloc(PICOLCDFB_SIZE*info->var.bits_per_pixel); + o_len =3D info->fix.smem_len; + o_fb =3D data->fb_bitmap; + n_fb =3D vmalloc(PICOLCDFB_SIZE*info->var.bits_per_pixel); if (!n_fb) return -ENOMEM; =20 - fb_deferred_io_cleanup(info); /* translate FB content to new bits-per-pixel */ if (info->var.bits_per_pixel =3D 1) { int i, b; @@ -565,7 +602,7 @@ static int picolcd_set_par(struct fb_info *info) int i; for (i =3D 0; i < PICOLCDFB_SIZE * 8; i++) n_fb[i] =3D o_fb[i/8] & (0x01 << (7 - i % 8)) ? 0xff : 0x00; - info->fix.visual =3D FB_VISUAL_TRUECOLOR; + info->fix.visual =3D FB_VISUAL_DIRECTCOLOR; info->fix.line_length =3D PICOLCDFB_WIDTH; } =20 @@ -574,7 +611,10 @@ static int picolcd_set_par(struct fb_info *info) info->screen_base =3D (char __force __iomem *)n_fb; info->fix.smem_start =3D (unsigned long)n_fb; info->fix.smem_len =3D PICOLCDFB_SIZE*data->fb_bpp; - fb_deferred_io_init(info); + for (i =3D 0; i < o_len; i +=3D PAGE_SIZE) { + struct page *page =3D vmalloc_to_page(o_fb + i); + page->mapping =3D NULL; + } vfree(o_fb); return 0; } @@ -660,9 +700,10 @@ static int picolcd_init_framebuffer(struct picolcd_dat= a *data) { struct device *dev =3D &data->hdev->dev; struct fb_info *info =3D NULL; - int error =3D -ENOMEM; + int i, error =3D -ENOMEM; u8 *fb_vbitmap =3D NULL; u8 *fb_bitmap =3D NULL; + u32 *palette; =20 fb_bitmap =3D vmalloc(PICOLCDFB_SIZE*picolcdfb_var.bits_per_pixel); if (fb_bitmap =3D NULL) { @@ -678,12 +719,16 @@ static int picolcd_init_framebuffer(struct picolcd_da= ta *data) =20 data->fb_update_rate =3D PICOLCDFB_UPDATE_RATE_DEFAULT; data->fb_defio =3D picolcd_fb_defio; - info =3D framebuffer_alloc(0, dev); + info =3D framebuffer_alloc(256 * sizeof(u32), dev); if (info =3D NULL) { dev_err(dev, "failed to allocate a framebuffer\n"); goto err_nomem; } =20 + palette =3D info->par; + for (i =3D 0; i < 256; i++) + palette[i] =3D i > 0 && i < 16 ? 0xff : 0; + info->pseudo_palette =3D info->par; info->fbdefio =3D &data->fb_defio; info->screen_base =3D (char __force __iomem *)fb_bitmap; info->fbops =3D &picolcdfb_ops; @@ -707,18 +752,20 @@ static int picolcd_init_framebuffer(struct picolcd_da= ta *data) dev_err(dev, "failed to create sysfs attributes\n"); goto err_cleanup; } + fb_deferred_io_init(info); data->fb_info =3D info; error =3D register_framebuffer(info); if (error) { dev_err(dev, "failed to register framebuffer\n"); goto err_sysfs; } - fb_deferred_io_init(info); /* schedule first output of framebuffer */ + data->fb_force =3D 1; schedule_delayed_work(&info->deferred_work, 0); return 0; =20 err_sysfs: + fb_deferred_io_cleanup(info); device_remove_file(dev, &dev_attr_fb_update_rate); err_cleanup: data->fb_vbitmap =3D NULL; @@ -742,13 +789,13 @@ static void picolcd_exit_framebuffer(struct picolcd_d= ata *data) if (!info) return; =20 + info->par =3D NULL; + device_remove_file(&data->hdev->dev, &dev_attr_fb_update_rate); + unregister_framebuffer(info); data->fb_vbitmap =3D NULL; data->fb_bitmap =3D NULL; data->fb_bpp =3D 0; data->fb_info =3D NULL; - device_remove_file(&data->hdev->dev, &dev_attr_fb_update_rate); - fb_deferred_io_cleanup(info); - unregister_framebuffer(info); vfree(fb_bitmap); kfree(fb_vbitmap); } @@ -2566,6 +2613,11 @@ static void picolcd_remove(struct hid_device *hdev) spin_lock_irqsave(&data->lock, flags); data->status |=3D PICOLCD_FAILED; spin_unlock_irqrestore(&data->lock, flags); + /* short-circuit FB as early as possible in order to + * avoid long delays if we host console. + */ + if (data->fb_info) + data->fb_info->par =3D NULL; =20 picolcd_exit_devfs(data); device_remove_file(&hdev->dev, &dev_attr_operation_mode);