Deadlock between fbcon and fb_defio?

Deadlock between fbcon and fb_defio?

[Bruno Prémont]

Hi Jaya,

While working on hid-picolcd to get fbcon working on top of it (fixing
up color handling and other issues) I've hit what looks like a dead-lock
apparently between fbcon and fb_defio.

In set_par() I'm calling fb_deferred_io_cleanup() before switching
to new framebuffer (on bits-per-pixel change) after which I resume
deferred_io by calling fb_deferred_io_init().

See code at (fixes I'm working on are at the end):
http://git.kernel.org/?p=linux/kernel/git/jikos/hid.git;a=blob;f=drivers/hid/hid-picolcd.c;hb=picolcd


Any idea why things deadlock here and how to avoid it? (it seems
to be more of a race condition as I already called fbset successfully
but that was from a ssh session instead of from console itself).

One option seems to be to release console_sem for the time I stop
fb_defio and re-acquiring it afterwards. But this rather looks like
a work-around as the events kernel thread seems to have both fbcon
fb_defio work interleaved (which I think a better fix would prevent)...

Trace below.

Thanks,
Bruno


[ 3480.400053] INFO: task events/0:5 blocked for more than 120 seconds.
[ 3480.400072] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
[ 3480.400094] events/0      D c1038220  2600     5      2 0x00000000
[ 3480.400133]  f704aed0 00000046 f704edc0 c1038220 f704aeb4 f704ef40 f6157f00 f5cd31c0
[ 3480.400172]  f704edc0 7fffffff c119c4e0 f704af20 c1337e05 000007c0 ffffffff f5af27c0
[ 3480.400208]  00000014 00000008 00000004 f6157f00 f5af27c0 00000246 f6325c00 0000000d
[ 3480.400245] Call Trace:
[ 3480.400290]  [<c1038220>] ? autoremove_wake_function+0x0/0x50
[ 3480.400329]  [<c119c4e0>] ? fb_flashcursor+0x0/0x100
[ 3480.400364]  [<c1337e05>] schedule_timeout+0xf5/0x170
[ 3480.400398]  [<c119c4e0>] ? fb_flashcursor+0x0/0x100
[ 3480.400427]  [<c13387a8>] __down+0x48/0x70
[ 3480.400457]  [<c103c770>] down+0x20/0x30
[ 3480.400490]  [<c10263fd>] acquire_console_sem+0x1d/0x50
[ 3480.400520]  [<c119c500>] fb_flashcursor+0x20/0x100
[ 3480.400547]  [<c11a300f>] ? fb_deferred_io_work+0xaf/0xc0
[ 3480.400577]  [<c119c4e0>] ? fb_flashcursor+0x0/0x100
[ 3480.400605]  [<c1035066>] worker_thread+0xd6/0x180
[ 3480.400633]  [<c10228ef>] ? finish_task_switch+0x2f/0x80
[ 3480.400666]  [<c1038220>] ? autoremove_wake_function+0x0/0x50
[ 3480.400694]  [<c1034f90>] ? worker_thread+0x0/0x180
[ 3480.400723]  [<c1037e5c>] kthread+0x6c/0x80
[ 3480.400750]  [<c1037df0>] ? kthread+0x0/0x80
[ 3480.400777]  [<c1003036>] kernel_thread_helper+0x6/0x10
[ 3480.400832] INFO: task fbset:18512 blocked for more than 120 seconds.
[ 3480.400848] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
[ 3480.400867] fbset         D 00000002  1828 18512   1504 0x00000000
[ 3480.400902]  f33b7b34 00000086 f712f810 00000002 00000000 f712f990 00000001 f5cd31c0
[ 3480.400937]  f712f810 7fffffff f33b7bd8 f33b7b84 c1337e05 c14b58b0 00000002 00000041
[ 3480.400974]  c14b52e8 00000000 f5cc600c f33b7b64 c10760c3 c14b58b0 00000000 000200d2
[ 3480.401009] Call Trace:
[ 3480.401037]  [<c1337e05>] schedule_timeout+0xf5/0x170
[ 3480.401068]  [<c10760c3>] ? __insert_vmap_area+0x53/0xb0
[ 3480.401099]  [<c13377b8>] wait_for_common+0x98/0xf0
[ 3480.401128]  [<c1022f80>] ? default_wake_function+0x0/0x10
[ 3480.401158]  [<c13378a2>] wait_for_completion+0x12/0x20
[ 3480.401186]  [<c1034bcb>] flush_cpu_workqueue+0x3b/0x70
[ 3480.401214]  [<c1034e70>] ? wq_barrier_func+0x0/0x10
[ 3480.401241]  [<c1034c7a>] flush_workqueue+0xa/0x10
[ 3480.401268]  [<c1034c8d>] flush_scheduled_work+0xd/0x10
[ 3480.401294]  [<c11a3192>] fb_deferred_io_cleanup+0x32/0x80
[ 3480.401335]  [<f8127eb4>] picolcd_set_par+0x74/0x120 [hid_picolcd]
[ 3480.401387]  [<c1194491>] fb_set_var+0x191/0x340
[ 3480.401431]  [<c1107477>] ? xfs_bmap_search_extents+0x47/0xf0
[ 3480.401461]  [<c110f726>] ? xfs_bmapi+0x2e6/0x1330
[ 3480.401490]  [<c100483e>] ? handle_irq+0x1e/0xd0
[ 3480.401518]  [<c1194a5a>] do_fb_ioctl+0x41a/0x550
[ 3480.401553]  [<c1081907>] ? nameidata_to_filp+0x47/0x60
[ 3480.401583]  [<c105b3bd>] ? unlock_page+0x3d/0x40
[ 3480.401614]  [<c106e8e0>] ? __do_fault+0x330/0x400
[ 3480.401640]  [<c108aa00>] ? path_put+0x20/0x30
[ 3480.401668]  [<c1194b90>] ? fb_ioctl+0x0/0x20
[ 3480.401694]  [<c1194bad>] fb_ioctl+0x1d/0x20
[ 3480.401719]  [<c108ede0>] vfs_ioctl+0x20/0x80
[ 3480.401745]  [<c108f7d2>] do_vfs_ioctl+0x362/0x560
[ 3480.401780]  [<c101a417>] ? do_page_fault+0x1c7/0x360
[ 3480.401811]  [<c1003a10>] ? do_device_not_available+0x0/0x50
[ 3480.401843]  [<c1009a76>] ? init_fpu+0x66/0x170
[ 3480.401868]  [<c108fa09>] sys_ioctl+0x39/0x70
[ 3480.401893]  [<c1002b10>] sysenter_do_call+0x12/0x26

---

Fix crash if we are the first framebuffer loaded as in that case fbcon
wants to flush framebuffer at the end of fb registration, before we
have setup fb_defio.

Add a flag for forcing full re-transmit of framebuffer as otherwise
we have a long race period after initialization during which fb can
change and match startup inverted shadow copy, thus cause initial transmit
of tiles to be skipped (visible with fbcon).

Adjust visual handling so fbcon gets properly displayed in 8bpp grayscale
mode.

Reorder tear-down so we avoid spamming kernel log with messages regarding
full USB queue and also avoid long delay from fb_defio cleanup when
fb was active during unplug.

diff --git a/drivers/hid/hid-picolcd.c b/drivers/hid/hid-picolcd.c
index 95253b3..ffaa39c 100644
--- a/drivers/hid/hid-picolcd.c
+++ b/drivers/hid/hid-picolcd.c
@@ -127,6 +127,26 @@ static const struct fb_var_screeninfo picolcdfb_var = {
 	.height         = 26,
 	.bits_per_pixel = 1,
 	.grayscale      = 1,
+	.red            = {
+		.offset = 0,
+		.length = 1,
+		.msb_right = 0,
+	},
+	.green          = {
+		.offset = 0,
+		.length = 1,
+		.msb_right = 0,
+	},
+	.blue           = {
+		.offset = 0,
+		.length = 1,
+		.msb_right = 0,
+	},
+	.transp         = {
+		.offset = 0,
+		.length = 0,
+		.msb_right = 0,
+	},
 };
 #endif /* CONFIG_HID_PICOLCD_FB */
 
@@ -188,6 +208,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 +367,7 @@ static int picolcd_fb_update_tile(u8 *vbitmap, const u8 *bitmap, int bpp,
 			const u8 *bdata = bitmap + tile * 256 + chip * 8 + b * 32;
 			for (i = 0; i < 64; i++) {
 				tdata[i] <<= 1;
-				tdata[i] |= (bdata[i/8] >> (7 - i % 8)) & 0x01;
+				tdata[i] |= (bdata[i/8] >> (/* 7 - */ i % 8)) & 0x01;
 			}
 		}
 	} else if (bpp = 8) {
@@ -399,13 +420,10 @@ static int picolcd_fb_reset(struct picolcd_data *data, int clear)
 
 	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 = 0; i < PICOLCDFB_SIZE; i += 64)
-				data->fb_vbitmap[i] = ~data->fb_vbitmap[i];
 		}
+		data->fb_force = 1;
 	}
 
 	/* schedule first output of framebuffer */
@@ -421,6 +439,9 @@ static void picolcd_fb_update(struct picolcd_data *data)
 	int chip, tile, n;
 	unsigned long flags;
 
+	if (!data)
+		return;
+
 	spin_lock_irqsave(&data->lock, flags);
 	if (!(data->status & PICOLCD_READY_FB)) {
 		spin_unlock_irqrestore(&data->lock, flags);
@@ -440,14 +461,18 @@ static void picolcd_fb_update(struct picolcd_data *data)
 	for (chip = 0; chip < 4; chip++)
 		for (tile = 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 += 2;
+				if (!data->fb_info->par)
+					return; /* device lost! */
 				if (n >= HID_OUTPUT_FIFO_SIZE / 2) {
 					usbhid_wait_io(data->hdev);
 					n = 0;
 				}
 				picolcd_fb_send_tile(data->hdev, chip, tile);
 			}
+	data->fb_force = false;
 	if (n)
 		usbhid_wait_io(data->hdev);
 }
@@ -511,10 +536,11 @@ static int picolcd_fb_blank(int blank, struct fb_info *info)
 static void picolcd_fb_destroy(struct fb_info *info)
 {
 	struct picolcd_data *data = info->par;
+	fb_deferred_io_cleanup(info);
+
 	info->par = NULL;
 	if (data)
 		data->fb_info = NULL;
-	fb_deferred_io_cleanup(info);
 	framebuffer_release(info);
 }
 
@@ -526,10 +552,17 @@ static int picolcd_fb_check_var(struct fb_var_screeninfo *var, struct fb_info *i
 	/* only allow 1/8 bit depth (8-bit is grayscale) */
 	*var = picolcdfb_var;
 	var->activate = activate;
-	if (bpp >= 8)
+	if (bpp >= 8) {
 		var->bits_per_pixel = 8;
-	else
+		var->red.length     = 8;
+		var->green.length   = 8;
+		var->blue.length    = 8;
+	} else {
 		var->bits_per_pixel = 1;
+		var->red.length     = 1;
+		var->green.length   = 1;
+		var->blue.length    = 1;
+	}
 	return 0;
 }
 
@@ -537,6 +570,8 @@ static int picolcd_set_par(struct fb_info *info)
 {
 	struct picolcd_data *data = info->par;
 	u8 *o_fb, *n_fb;
+	if (!data)
+		return -ENODEV;
 	if (info->var.bits_per_pixel = data->fb_bpp)
 		return 0;
 	/* switch between 1/8 bit depths */
@@ -565,7 +600,7 @@ static int picolcd_set_par(struct fb_info *info)
 		int i;
 		for (i = 0; i < PICOLCDFB_SIZE * 8; i++)
 			n_fb[i] = o_fb[i/8] & (0x01 << (7 - i % 8)) ? 0xff : 0x00;
-		info->fix.visual = FB_VISUAL_TRUECOLOR;
+		info->fix.visual = FB_VISUAL_DIRECTCOLOR;
 		info->fix.line_length = PICOLCDFB_WIDTH;
 	}
 
@@ -660,9 +695,10 @@ static int picolcd_init_framebuffer(struct picolcd_data *data)
 {
 	struct device *dev = &data->hdev->dev;
 	struct fb_info *info = NULL;
-	int error = -ENOMEM;
+	int i, error = -ENOMEM;
 	u8 *fb_vbitmap = NULL;
 	u8 *fb_bitmap  = NULL;
+	u32 *palette;
 
 	fb_bitmap = vmalloc(PICOLCDFB_SIZE*picolcdfb_var.bits_per_pixel);
 	if (fb_bitmap = NULL) {
@@ -678,12 +714,16 @@ static int picolcd_init_framebuffer(struct picolcd_data *data)
 
 	data->fb_update_rate = PICOLCDFB_UPDATE_RATE_DEFAULT;
 	data->fb_defio = picolcd_fb_defio;
-	info = framebuffer_alloc(0, dev);
+	info = framebuffer_alloc(256 * sizeof(u32), dev);
 	if (info = NULL) {
 		dev_err(dev, "failed to allocate a framebuffer\n");
 		goto err_nomem;
 	}
 
+	palette = info->par;
+	for (i = 0; i < 256; i++)
+		palette[i] = i > 0 && i < 16 ? 0xff : 0;
+	info->pseudo_palette = info->par;
 	info->fbdefio = &data->fb_defio;
 	info->screen_base = (char __force __iomem *)fb_bitmap;
 	info->fbops = &picolcdfb_ops;
@@ -707,18 +747,20 @@ static int picolcd_init_framebuffer(struct picolcd_data *data)
 		dev_err(dev, "failed to create sysfs attributes\n");
 		goto err_cleanup;
 	}
+	fb_deferred_io_init(info);
 	data->fb_info    = info;
 	error = 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 = 1;
 	schedule_delayed_work(&info->deferred_work, 0);
 	return 0;
 
 err_sysfs:
+	fb_deferred_io_cleanup(info);
 	device_remove_file(dev, &dev_attr_fb_update_rate);
 err_cleanup:
 	data->fb_vbitmap = NULL;
@@ -742,13 +784,13 @@ static void picolcd_exit_framebuffer(struct picolcd_data *data)
 	if (!info)
 		return;
 
+	info->par = NULL;
+	device_remove_file(&data->hdev->dev, &dev_attr_fb_update_rate);
+	unregister_framebuffer(info);
 	data->fb_vbitmap = NULL;
 	data->fb_bitmap  = NULL;
 	data->fb_bpp     = 0;
 	data->fb_info    = 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 +2608,11 @@ static void picolcd_remove(struct hid_device *hdev)
 	spin_lock_irqsave(&data->lock, flags);
 	data->status |= PICOLCD_FAILED;
 	spin_unlock_irqrestore(&data->lock, flags);
+	/* short-circuit FB as early as possible in order to
+	 * avoid long details if we hosted console.
+	 */
+	if (data->fb_info)
+		data->fb_info->par = NULL;
 
 	picolcd_exit_devfs(data);
 	device_remove_file(&hdev->dev, &dev_attr_operation_mode);

Re: Deadlock between fbcon and fb_defio?

[Jaya Kumar]

On Mon, May 10, 2010 at 12:49 AM, Bruno Prémont
<bonbons@linux-vserver.org> wrote:
>
> Fix crash if we are the first framebuffer loaded as in that case fbcon
> wants to flush framebuffer at the end of fb registration, before we
> have setup fb_defio.

Bruno,

Please help me understand, how does this scenario occur? I'm
interpreting what you've written above to mean that fbcon is accessing
the framebuffer before you've called defio_init()? Is that correct?

The typical defio use sequence is: defio_init(),
register_framebuffer() and the typical remove sequence is in the
reverse order unregister_framebuffer(), defio_cleanup(). So, I don't
see how fbcon is accessing the framebuffer either before
register_framebuffer() completes (at which point defio init is already
done) or after unregister_framebuffer() completes.

Thanks,
jaya

Re: Deadlock between fbcon and fb_defio?

[Bruno Prémont]

Hi Jaya,

On Mon, 10 May 2010 08:00:51 Jaya Kumar wrote:
> On Mon, May 10, 2010 at 12:49 AM, Bruno Prémont
> <bonbons@linux-vserver.org> wrote:
> >
> > Fix crash if we are the first framebuffer loaded as in that case
> > fbcon wants to flush framebuffer at the end of fb registration,
> > before we have setup fb_defio.
> 
> Bruno,
> 
> Please help me understand, how does this scenario occur? I'm
> interpreting what you've written above to mean that fbcon is accessing
> the framebuffer before you've called defio_init()? Is that correct?

That was the original state as I called defio_init after
register_framebuffer() and defio_cleanup() before
unregister_framebuffer(), the opposite to the typical sequence you
detail below. Fixed by the patch.

My deadlock issue, after applying the patch, is during set_par() when I
replace the framebuffer and wish defio to start using the new page(s)
instead of the old one(s).

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.

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.

Thanks,
Bruno

> The typical defio use sequence is: defio_init(),
> register_framebuffer() and the typical remove sequence is in the
> reverse order unregister_framebuffer(), defio_cleanup(). So, I don't
> see how fbcon is accessing the framebuffer either before
> register_framebuffer() completes (at which point defio init is already
> done) or after unregister_framebuffer() completes.
> 
> Thanks,
> jaya

Re: vfree() and mmap()ed framebuffer with defio (Was: Deadlock

[Bruno Prémont]

Hi Jaya,

On Mon, 10 May 2010 Bruno Prémont <bonbons@linux-vserver.org> 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.
> 
> 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 = (char __force __iomem *)n_fb;
 	info->fix.smem_start = (unsigned long)n_fb;
 	info->fix.smem_len   = PICOLCDFB_SIZE*data->fb_bpp;
-	fb_deferred_io_init(info);
+	for (i = 0; i < o_len; i += PAGE_SIZE) {
+		struct page *page = vmalloc_to_page(o_fb + i);
+		page->mapping = NULL;
+	}
 	vfree(o_fb);
 	return 0;
 }


If I run the for loop I end up with the "unable to handle paging request" BUG:
[ 4364.740056] BUG: unable to handle kernel paging request at 84db1980
[ 4364.741115] IP: [<c103816f>] __wake_up_bit+0xf/0x30
[ 4364.741806] *pde = 00000000 
[ 4364.742165] Oops: 0000 [#1] 
[ 4364.742526] last sysfs file: /sys/devices/platform/via_cputemp.0/temp1_input
[ 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 snd_timer fb_sys_fops sysimgblt sysfillrect syscopyarea snd soundcore snd_page_alloc sg [last unloaded: hid_picolcd]
[ 4364.747026] 
[ 4364.747393] Pid: 5, comm: events/0 Tainted: G    B      2.6.34-venus #46 CX700+W697HG/CX700+W697HG
[ 4364.748855] EIP: 0060:[<c103816f>] 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÷04a000 task÷04edc0 task.ti÷04a000)
[ 4364.750012] Stack:
[ 4364.750012]  00000000 c14b6330 00000000 c14b65cc f704af60 c105b39d c14b6330 f704af7c
[ 4364.750012] <0> c11a2e96 f69b4868 f69b2800 f69b2a1c f7003300 c11a2e50 f704afc0 c1035056
[ 4364.750012] <0> c10228ef f704e000 f704edc0 f704e000 f704efa0 f704edc0 f7003308 00000000
[ 4364.750012] Call Trace:
[ 4364.750012]  [<c105b39d>] ? unlock_page+0x3d/0x40
[ 4364.750012]  [<c11a2e96>] ? fb_deferred_io_work+0x46/0xc0
[ 4364.750012]  [<c11a2e50>] ? fb_deferred_io_work+0x0/0xc0
[ 4364.750012]  [<c1035056>] ? worker_thread+0xd6/0x180
[ 4364.750012]  [<c10228ef>] ? finish_task_switch+0x2f/0x80
[ 4364.750012]  [<c1038210>] ? autoremove_wake_function+0x0/0x50
[ 4364.750012]  [<c1034f80>] ? worker_thread+0x0/0x180
[ 4364.750012]  [<c1037e4c>] ? kthread+0x6c/0x80
[ 4364.750012]  [<c1037de0>] ? kthread+0x0/0x80
[ 4364.750012]  [<c1003036>] ? 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 
[ 4364.750012] EIP: [<c103816f>] __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]  [<c105eea2>] bad_page+0x82/0xd0
[ 1301.045727]  [<c1060341>] free_hot_cold_page+0x191/0x310
[ 1301.046497]  [<c1062c1e>] put_page+0x3e/0x100
[ 1301.047256]  [<c1078d9d>] free_page_and_swap_cache+0x1d/0x40
[ 1301.048056]  [<c106ddc8>] unmap_vmas+0x2a8/0x580
[ 1301.048846]  [<c1021ce7>] ? dequeue_task_fair+0x27/0x1d0
[ 1301.049640]  [<c1071837>] exit_mmap+0x97/0x110
[ 1301.050477]  [<c1023e56>] mmput+0x26/0x90
[ 1301.051259]  [<c10274f1>] exit_mm+0xb1/0xc0
[ 1301.052045]  [<c10288ba>] do_exit+0xba/0x6b0
[ 1301.052848]  [<c102f721>] ? recalc_sigpending+0x11/0x30
[ 1301.053660]  [<c1030b9c>] ? dequeue_signal+0x2c/0x130
[ 1301.054480]  [<c1028edd>] do_group_exit+0x2d/0x70
[ 1301.055293]  [<c10321bc>] get_signal_to_deliver+0x17c/0x370
[ 1301.056103]  [<c10299f6>] ? current_fs_time+0x16/0x20
[ 1301.056892]  [<c10022f8>] do_notify_resume+0x98/0x820
[ 1301.057690]  [<c11d4b6e>] ? tty_unthrottle+0x3e/0x50
[ 1301.058490]  [<c10299f6>] ? current_fs_time+0x16/0x20
[ 1301.059303]  [<c11d2db0>] ? n_tty_read+0x0/0x680
[ 1301.060134]  [<c109153d>] ? sys_select+0x3d/0xb0
[ 1301.060935]  [<c1029c2b>] ? sys_gettimeofday+0x2b/0x70
[ 1301.061756]  [<c13394d2>] 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 @@
 
 #include <linux/fb.h>
 #include <linux/vmalloc.h>
+#include <linux/mm.h>
 #include <linux/backlight.h>
 #include <linux/lcd.h>
 
@@ -127,6 +128,26 @@ static const struct fb_var_screeninfo picolcdfb_var = {
 	.height         = 26,
 	.bits_per_pixel = 1,
 	.grayscale      = 1,
+	.red            = {
+		.offset = 0,
+		.length = 1,
+		.msb_right = 0,
+	},
+	.green          = {
+		.offset = 0,
+		.length = 1,
+		.msb_right = 0,
+	},
+	.blue           = {
+		.offset = 0,
+		.length = 1,
+		.msb_right = 0,
+	},
+	.transp         = {
+		.offset = 0,
+		.length = 0,
+		.msb_right = 0,
+	},
 };
 #endif /* CONFIG_HID_PICOLCD_FB */
 
@@ -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 = bitmap + tile * 256 + chip * 8 + b * 32;
 			for (i = 0; i < 64; i++) {
 				tdata[i] <<= 1;
-				tdata[i] |= (bdata[i/8] >> (7 - i % 8)) & 0x01;
+				tdata[i] |= (bdata[i/8] >> (i % 8)) & 0x01;
 			}
 		}
 	} else if (bpp = 8) {
@@ -399,13 +421,10 @@ static int picolcd_fb_reset(struct picolcd_data *data, int clear)
 
 	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 = 0; i < PICOLCDFB_SIZE; i += 64)
-				data->fb_vbitmap[i] = ~data->fb_vbitmap[i];
 		}
+		data->fb_force = 1;
 	}
 
 	/* 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;
 
+	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 *data)
 	for (chip = 0; chip < 4; chip++)
 		for (tile = 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 += 2;
+				if (!data->fb_info->par)
+					return; /* device lost! */
 				if (n >= HID_OUTPUT_FIFO_SIZE / 2) {
 					usbhid_wait_io(data->hdev);
 					n = 0;
 				}
 				picolcd_fb_send_tile(data->hdev, chip, tile);
 			}
+	data->fb_force = false;
 	if (n)
 		usbhid_wait_io(data->hdev);
 }
@@ -526,10 +552,17 @@ static int picolcd_fb_check_var(struct fb_var_screeninfo *var, struct fb_info *i
 	/* only allow 1/8 bit depth (8-bit is grayscale) */
 	*var = picolcdfb_var;
 	var->activate = activate;
-	if (bpp >= 8)
+	if (bpp >= 8) {
 		var->bits_per_pixel = 8;
-	else
+		var->red.length     = 8;
+		var->green.length   = 8;
+		var->blue.length    = 8;
+	} else {
 		var->bits_per_pixel = 1;
+		var->red.length     = 1;
+		var->green.length   = 1;
+		var->blue.length    = 1;
+	}
 	return 0;
 }
 
@@ -537,18 +570,22 @@ static int picolcd_set_par(struct fb_info *info)
 {
 	struct picolcd_data *data = info->par;
 	u8 *o_fb, *n_fb;
+	int i;
+	unsigned long o_len;
+	if (!data)
+		return -ENODEV;
 	if (info->var.bits_per_pixel = data->fb_bpp)
 		return 0;
 	/* switch between 1/8 bit depths */
 	if (info->var.bits_per_pixel != 1 && info->var.bits_per_pixel != 8)
 		return -EINVAL;
 
-	o_fb = data->fb_bitmap;
-	n_fb = vmalloc(PICOLCDFB_SIZE*info->var.bits_per_pixel);
+	o_len = info->fix.smem_len;
+	o_fb  = data->fb_bitmap;
+	n_fb  = vmalloc(PICOLCDFB_SIZE*info->var.bits_per_pixel);
 	if (!n_fb)
 		return -ENOMEM;
 
-	fb_deferred_io_cleanup(info);
 	/* translate FB content to new bits-per-pixel */
 	if (info->var.bits_per_pixel = 1) {
 		int i, b;
@@ -565,7 +602,7 @@ static int picolcd_set_par(struct fb_info *info)
 		int i;
 		for (i = 0; i < PICOLCDFB_SIZE * 8; i++)
 			n_fb[i] = o_fb[i/8] & (0x01 << (7 - i % 8)) ? 0xff : 0x00;
-		info->fix.visual = FB_VISUAL_TRUECOLOR;
+		info->fix.visual = FB_VISUAL_DIRECTCOLOR;
 		info->fix.line_length = PICOLCDFB_WIDTH;
 	}
 
@@ -574,7 +611,10 @@ static int picolcd_set_par(struct fb_info *info)
 	info->screen_base = (char __force __iomem *)n_fb;
 	info->fix.smem_start = (unsigned long)n_fb;
 	info->fix.smem_len   = PICOLCDFB_SIZE*data->fb_bpp;
-	fb_deferred_io_init(info);
+	for (i = 0; i < o_len; i += PAGE_SIZE) {
+		struct page *page = vmalloc_to_page(o_fb + i);
+		page->mapping = NULL;
+	}
 	vfree(o_fb);
 	return 0;
 }
@@ -660,9 +700,10 @@ static int picolcd_init_framebuffer(struct picolcd_data *data)
 {
 	struct device *dev = &data->hdev->dev;
 	struct fb_info *info = NULL;
-	int error = -ENOMEM;
+	int i, error = -ENOMEM;
 	u8 *fb_vbitmap = NULL;
 	u8 *fb_bitmap  = NULL;
+	u32 *palette;
 
 	fb_bitmap = vmalloc(PICOLCDFB_SIZE*picolcdfb_var.bits_per_pixel);
 	if (fb_bitmap = NULL) {
@@ -678,12 +719,16 @@ static int picolcd_init_framebuffer(struct picolcd_data *data)
 
 	data->fb_update_rate = PICOLCDFB_UPDATE_RATE_DEFAULT;
 	data->fb_defio = picolcd_fb_defio;
-	info = framebuffer_alloc(0, dev);
+	info = framebuffer_alloc(256 * sizeof(u32), dev);
 	if (info = NULL) {
 		dev_err(dev, "failed to allocate a framebuffer\n");
 		goto err_nomem;
 	}
 
+	palette = info->par;
+	for (i = 0; i < 256; i++)
+		palette[i] = i > 0 && i < 16 ? 0xff : 0;
+	info->pseudo_palette = info->par;
 	info->fbdefio = &data->fb_defio;
 	info->screen_base = (char __force __iomem *)fb_bitmap;
 	info->fbops = &picolcdfb_ops;
@@ -707,18 +752,20 @@ static int picolcd_init_framebuffer(struct picolcd_data *data)
 		dev_err(dev, "failed to create sysfs attributes\n");
 		goto err_cleanup;
 	}
+	fb_deferred_io_init(info);
 	data->fb_info    = info;
 	error = 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 = 1;
 	schedule_delayed_work(&info->deferred_work, 0);
 	return 0;
 
 err_sysfs:
+	fb_deferred_io_cleanup(info);
 	device_remove_file(dev, &dev_attr_fb_update_rate);
 err_cleanup:
 	data->fb_vbitmap = NULL;
@@ -742,13 +789,13 @@ static void picolcd_exit_framebuffer(struct picolcd_data *data)
 	if (!info)
 		return;
 
+	info->par = NULL;
+	device_remove_file(&data->hdev->dev, &dev_attr_fb_update_rate);
+	unregister_framebuffer(info);
 	data->fb_vbitmap = NULL;
 	data->fb_bitmap  = NULL;
 	data->fb_bpp     = 0;
 	data->fb_info    = 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 |= 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 = NULL;
 
 	picolcd_exit_devfs(data);
 	device_remove_file(&hdev->dev, &dev_attr_operation_mode);

[Patch] HID: Fix PicoLCD to allow it to run fbcon and handle unplug

[Bruno Prémont]

This fixes multiple issues related to FB use:
- NULL-pointer dereference if fbcon wants to use our FB on
  8-bpp framebuffer.
- Getting new vmalloc()ed framebuffer on each depth change
  causes pain if the FB was mmap()ed by userspace, so allocate
  biggest FB needed and just convert its content on depth change
  to avoid the issue
- When FB is in use and our device gets unplugged, wait until
  last user stops using FB before we free fb_info and framebuffer
  (via deferred work)

Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org>
---

This should fix all issues I've seen running fbcon and userspace fb
applications on top of picolcd, even when it gets unplugged.

I would appreciate if a second pair of eyes could could review the
locking and releasing of FB resources (I've tested it on UP, currently
no SMP system available for testing)

Thanks,
Bruno



 drivers/hid/hid-picolcd.c |  201 +++++++++++++++++++++++++++++++++++++--------
 1 files changed, 167 insertions(+), 34 deletions(-)

diff --git a/drivers/hid/hid-picolcd.c b/drivers/hid/hid-picolcd.c
index 95253b3..1a0dacc 100644
--- a/drivers/hid/hid-picolcd.c
+++ b/drivers/hid/hid-picolcd.c
@@ -127,6 +127,26 @@ static const struct fb_var_screeninfo picolcdfb_var = {
 	.height         = 26,
 	.bits_per_pixel = 1,
 	.grayscale      = 1,
+	.red            = {
+		.offset = 0,
+		.length = 1,
+		.msb_right = 0,
+	},
+	.green          = {
+		.offset = 0,
+		.length = 1,
+		.msb_right = 0,
+	},
+	.blue           = {
+		.offset = 0,
+		.length = 1,
+		.msb_right = 0,
+	},
+	.transp         = {
+		.offset = 0,
+		.length = 0,
+		.msb_right = 0,
+	},
 };
 #endif /* CONFIG_HID_PICOLCD_FB */
 
@@ -188,6 +208,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 +367,7 @@ static int picolcd_fb_update_tile(u8 *vbitmap, const u8 *bitmap, int bpp,
 			const u8 *bdata = bitmap + tile * 256 + chip * 8 + b * 32;
 			for (i = 0; i < 64; i++) {
 				tdata[i] <<= 1;
-				tdata[i] |= (bdata[i/8] >> (7 - i % 8)) & 0x01;
+				tdata[i] |= (bdata[i/8] >> (i % 8)) & 0x01;
 			}
 		}
 	} else if (bpp = 8) {
@@ -399,13 +420,10 @@ static int picolcd_fb_reset(struct picolcd_data *data, int clear)
 
 	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 = 0; i < PICOLCDFB_SIZE; i += 64)
-				data->fb_vbitmap[i] = ~data->fb_vbitmap[i];
 		}
+		data->fb_force = 1;
 	}
 
 	/* schedule first output of framebuffer */
@@ -421,6 +439,9 @@ static void picolcd_fb_update(struct picolcd_data *data)
 	int chip, tile, n;
 	unsigned long flags;
 
+	if (!data)
+		return;
+
 	spin_lock_irqsave(&data->lock, flags);
 	if (!(data->status & PICOLCD_READY_FB)) {
 		spin_unlock_irqrestore(&data->lock, flags);
@@ -440,14 +461,18 @@ static void picolcd_fb_update(struct picolcd_data *data)
 	for (chip = 0; chip < 4; chip++)
 		for (tile = 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 += 2;
+				if (!data->fb_info->par)
+					return; /* device lost! */
 				if (n >= HID_OUTPUT_FIFO_SIZE / 2) {
 					usbhid_wait_io(data->hdev);
 					n = 0;
 				}
 				picolcd_fb_send_tile(data->hdev, chip, tile);
 			}
+	data->fb_force = false;
 	if (n)
 		usbhid_wait_io(data->hdev);
 }
@@ -511,11 +536,23 @@ static int picolcd_fb_blank(int blank, struct fb_info *info)
 static void picolcd_fb_destroy(struct fb_info *info)
 {
 	struct picolcd_data *data = info->par;
+	u32 *ref_cnt = info->pseudo_palette;
+	int may_release;
+
 	info->par = NULL;
 	if (data)
 		data->fb_info = NULL;
 	fb_deferred_io_cleanup(info);
-	framebuffer_release(info);
+
+	ref_cnt--;
+	mutex_lock(&info->lock);
+	(*ref_cnt)--;
+	may_release = !ref_cnt;
+	mutex_unlock(&info->lock);
+	if (may_release) {
+		framebuffer_release(info);
+		vfree((u8 *)info->fix.smem_start);
+	}
 }
 
 static int picolcd_fb_check_var(struct fb_var_screeninfo *var, struct fb_info *info)
@@ -526,29 +563,37 @@ static int picolcd_fb_check_var(struct fb_var_screeninfo *var, struct fb_info *i
 	/* only allow 1/8 bit depth (8-bit is grayscale) */
 	*var = picolcdfb_var;
 	var->activate = activate;
-	if (bpp >= 8)
+	if (bpp >= 8) {
 		var->bits_per_pixel = 8;
-	else
+		var->red.length     = 8;
+		var->green.length   = 8;
+		var->blue.length    = 8;
+	} else {
 		var->bits_per_pixel = 1;
+		var->red.length     = 1;
+		var->green.length   = 1;
+		var->blue.length    = 1;
+	}
 	return 0;
 }
 
 static int picolcd_set_par(struct fb_info *info)
 {
 	struct picolcd_data *data = info->par;
-	u8 *o_fb, *n_fb;
+	u8 *tmp_fb, *o_fb;
+	if (!data)
+		return -ENODEV;
 	if (info->var.bits_per_pixel = data->fb_bpp)
 		return 0;
 	/* switch between 1/8 bit depths */
 	if (info->var.bits_per_pixel != 1 && info->var.bits_per_pixel != 8)
 		return -EINVAL;
 
-	o_fb = data->fb_bitmap;
-	n_fb = vmalloc(PICOLCDFB_SIZE*info->var.bits_per_pixel);
-	if (!n_fb)
+	o_fb   = data->fb_bitmap;
+	tmp_fb = kmalloc(PICOLCDFB_SIZE*info->var.bits_per_pixel, GFP_KERNEL);
+	if (!tmp_fb)
 		return -ENOMEM;
 
-	fb_deferred_io_cleanup(info);
 	/* translate FB content to new bits-per-pixel */
 	if (info->var.bits_per_pixel = 1) {
 		int i, b;
@@ -558,24 +603,87 @@ static int picolcd_set_par(struct fb_info *info)
 				p <<= 1;
 				p |= o_fb[i*8+b] ? 0x01 : 0x00;
 			}
+			tmp_fb[i] = p;
 		}
+		memcpy(o_fb, tmp_fb, PICOLCDFB_SIZE);
 		info->fix.visual = FB_VISUAL_MONO01;
 		info->fix.line_length = PICOLCDFB_WIDTH / 8;
 	} else {
 		int i;
+		memcpy(tmp_fb, o_fb, PICOLCDFB_SIZE);
 		for (i = 0; i < PICOLCDFB_SIZE * 8; i++)
-			n_fb[i] = o_fb[i/8] & (0x01 << (7 - i % 8)) ? 0xff : 0x00;
-		info->fix.visual = FB_VISUAL_TRUECOLOR;
+			o_fb[i] = tmp_fb[i/8] & (0x01 << (7 - i % 8)) ? 0xff : 0x00;
+		info->fix.visual = FB_VISUAL_DIRECTCOLOR;
 		info->fix.line_length = PICOLCDFB_WIDTH;
 	}
 
-	data->fb_bitmap   = n_fb;
+	kfree(tmp_fb);
 	data->fb_bpp      = info->var.bits_per_pixel;
-	info->screen_base = (char __force __iomem *)n_fb;
-	info->fix.smem_start = (unsigned long)n_fb;
-	info->fix.smem_len   = PICOLCDFB_SIZE*data->fb_bpp;
-	fb_deferred_io_init(info);
-	vfree(o_fb);
+	return 0;
+}
+
+/* Do refcounting on our FB and cleanup per worker if FB is
+ * closed after unplug of our device
+ * (fb_release holds info->lock and still touches info after
+ *  we return so we can't release it immediately.
+ */
+struct picolcd_fb_cleanup_item {
+	struct fb_info *info;
+	struct picolcd_fb_cleanup_item *next;
+};
+static struct picolcd_fb_cleanup_item *fb_pending;
+DEFINE_SPINLOCK(fb_pending_lock);
+
+static void picolcd_fb_do_cleanup(struct work_struct *data)
+{
+	struct picolcd_fb_cleanup_item *item;
+	unsigned long flags;
+
+	do {
+		spin_lock_irqsave(&fb_pending_lock, flags);
+		item = fb_pending;
+		fb_pending = item ? item->next : NULL;
+		spin_unlock_irqrestore(&fb_pending_lock, flags);
+
+		if (item) {
+			u8 *fb = (u8 *)item->info->fix.smem_start;
+			/* make sure we do not race against fb core when
+			 * releasing */
+			mutex_lock(&item->info->lock);
+			mutex_unlock(&item->info->lock);
+			framebuffer_release(item->info);
+			vfree(fb);
+		}
+	} while (item);
+}
+
+DECLARE_WORK(picolcd_fb_cleanup, picolcd_fb_do_cleanup);
+
+static int picolcd_fb_open(struct fb_info *info, int u)
+{
+	u32 *ref_cnt = info->pseudo_palette;
+	ref_cnt--;
+
+	(*ref_cnt)++;
+	return 0;
+}
+
+static int picolcd_fb_release(struct fb_info *info, int u)
+{
+	u32 *ref_cnt = info->pseudo_palette;
+	ref_cnt--;
+
+	(*ref_cnt)++;
+	if (!*ref_cnt) {
+		unsigned long flags;
+		struct picolcd_fb_cleanup_item *item = (struct picolcd_fb_cleanup_item *)ref_cnt;
+		item--;
+		spin_lock_irqsave(&fb_pending_lock, flags);
+		item->next = fb_pending;
+		fb_pending = item;
+		spin_unlock_irqrestore(&fb_pending_lock, flags);
+		schedule_work(&picolcd_fb_cleanup);
+	}
 	return 0;
 }
 
@@ -583,6 +691,8 @@ static int picolcd_set_par(struct fb_info *info)
 static struct fb_ops picolcdfb_ops = {
 	.owner        = THIS_MODULE,
 	.fb_destroy   = picolcd_fb_destroy,
+	.fb_open      = picolcd_fb_open,
+	.fb_release   = picolcd_fb_release,
 	.fb_read      = fb_sys_read,
 	.fb_write     = picolcd_fb_write,
 	.fb_blank     = picolcd_fb_blank,
@@ -660,11 +770,12 @@ static int picolcd_init_framebuffer(struct picolcd_data *data)
 {
 	struct device *dev = &data->hdev->dev;
 	struct fb_info *info = NULL;
-	int error = -ENOMEM;
+	int i, error = -ENOMEM;
 	u8 *fb_vbitmap = NULL;
 	u8 *fb_bitmap  = NULL;
+	u32 *palette;
 
-	fb_bitmap = vmalloc(PICOLCDFB_SIZE*picolcdfb_var.bits_per_pixel);
+	fb_bitmap = vmalloc(PICOLCDFB_SIZE*8);
 	if (fb_bitmap = NULL) {
 		dev_err(dev, "can't get a free page for framebuffer\n");
 		goto err_nomem;
@@ -678,18 +789,29 @@ static int picolcd_init_framebuffer(struct picolcd_data *data)
 
 	data->fb_update_rate = PICOLCDFB_UPDATE_RATE_DEFAULT;
 	data->fb_defio = picolcd_fb_defio;
-	info = framebuffer_alloc(0, dev);
+	/* The extra memory is:
+	 * - struct picolcd_fb_cleanup_item
+	 * - u32 for ref_count
+	 * - 256*u32 for pseudo_palette
+	 */
+	info = framebuffer_alloc(257 * sizeof(u32) + sizeof(struct picolcd_fb_cleanup_item), dev);
 	if (info = NULL) {
 		dev_err(dev, "failed to allocate a framebuffer\n");
 		goto err_nomem;
 	}
 
+	palette  = info->par + sizeof(struct picolcd_fb_cleanup_item);
+	*palette = 1;
+	palette++;
+	for (i = 0; i < 256; i++)
+		palette[i] = i > 0 && i < 16 ? 0xff : 0;
+	info->pseudo_palette = palette;
 	info->fbdefio = &data->fb_defio;
 	info->screen_base = (char __force __iomem *)fb_bitmap;
 	info->fbops = &picolcdfb_ops;
 	info->var = picolcdfb_var;
 	info->fix = picolcdfb_fix;
-	info->fix.smem_len   = PICOLCDFB_SIZE;
+	info->fix.smem_len   = PICOLCDFB_SIZE*8;
 	info->fix.smem_start = (unsigned long)fb_bitmap;
 	info->par = data;
 	info->flags = FBINFO_FLAG_DEFAULT;
@@ -707,18 +829,20 @@ static int picolcd_init_framebuffer(struct picolcd_data *data)
 		dev_err(dev, "failed to create sysfs attributes\n");
 		goto err_cleanup;
 	}
+	fb_deferred_io_init(info);
 	data->fb_info    = info;
 	error = 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 = 1;
 	schedule_delayed_work(&info->deferred_work, 0);
 	return 0;
 
 err_sysfs:
+	fb_deferred_io_cleanup(info);
 	device_remove_file(dev, &dev_attr_fb_update_rate);
 err_cleanup:
 	data->fb_vbitmap = NULL;
@@ -728,8 +852,8 @@ err_cleanup:
 
 err_nomem:
 	framebuffer_release(info);
-	vfree(fb_bitmap);
 	kfree(fb_vbitmap);
+	vfree(fb_bitmap);
 	return error;
 }
 
@@ -737,19 +861,17 @@ static void picolcd_exit_framebuffer(struct picolcd_data *data)
 {
 	struct fb_info *info = data->fb_info;
 	u8 *fb_vbitmap = data->fb_vbitmap;
-	u8 *fb_bitmap  = data->fb_bitmap;
 
 	if (!info)
 		return;
 
+	info->par = NULL;
+	device_remove_file(&data->hdev->dev, &dev_attr_fb_update_rate);
+	unregister_framebuffer(info);
 	data->fb_vbitmap = NULL;
 	data->fb_bitmap  = NULL;
 	data->fb_bpp     = 0;
 	data->fb_info    = 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 +2688,13 @@ static void picolcd_remove(struct hid_device *hdev)
 	spin_lock_irqsave(&data->lock, flags);
 	data->status |= PICOLCD_FAILED;
 	spin_unlock_irqrestore(&data->lock, flags);
+#ifdef CONFIG_HID_PICOLCD_FB
+	/* 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 = NULL;
+#endif
 
 	picolcd_exit_devfs(data);
 	device_remove_file(&hdev->dev, &dev_attr_operation_mode);
@@ -2623,6 +2752,10 @@ static int __init picolcd_init(void)
 static void __exit picolcd_exit(void)
 {
 	hid_unregister_driver(&picolcd_driver);
+#ifdef CONFIG_HID_PICOLCD_FB
+	flush_scheduled_work();
+	WARN_ON(fb_pending);
+#endif
 }
 
 module_init(picolcd_init);
-- 
1.6.4.4

Re: [Patch] HID: Fix PicoLCD to allow it to run fbcon and handle

[Bruno Prémont]

Hi Jiri,

Do you think This patch can be applied as-is or should I break it up
into 2 or 3 patches (one for the 8bpp NULL-pointer dereference,
one for switch between 1bpp and 8bpp and one for the refcounting of
framebuffer to get things polite on unplug while framebuffer is still
in use?

Thanks,
Bruno


On Sun, 30 May 2010 Bruno Prémont <bonbons@linux-vserver.org> wrote:
> This fixes multiple issues related to FB use:
> - NULL-pointer dereference if fbcon wants to use our FB on
>   8-bpp framebuffer.
> - Getting new vmalloc()ed framebuffer on each depth change
>   causes pain if the FB was mmap()ed by userspace, so allocate
>   biggest FB needed and just convert its content on depth change
>   to avoid the issue
> - When FB is in use and our device gets unplugged, wait until
>   last user stops using FB before we free fb_info and framebuffer
>   (via deferred work)
> 
> Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org>
> ---
> 
> This should fix all issues I've seen running fbcon and userspace fb
> applications on top of picolcd, even when it gets unplugged.
> 
> I would appreciate if a second pair of eyes could could review the
> locking and releasing of FB resources (I've tested it on UP, currently
> no SMP system available for testing)
> 
> Thanks,
> Bruno
> 
> 
> 
>  drivers/hid/hid-picolcd.c |  201 +++++++++++++++++++++++++++++++++++++--------
>  1 files changed, 167 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/hid/hid-picolcd.c b/drivers/hid/hid-picolcd.c
> index 95253b3..1a0dacc 100644
> --- a/drivers/hid/hid-picolcd.c
> +++ b/drivers/hid/hid-picolcd.c
> @@ -127,6 +127,26 @@ static const struct fb_var_screeninfo picolcdfb_var = {
>  	.height         = 26,
>  	.bits_per_pixel = 1,
>  	.grayscale      = 1,
> +	.red            = {
> +		.offset = 0,
> +		.length = 1,
> +		.msb_right = 0,
> +	},
> +	.green          = {
> +		.offset = 0,
> +		.length = 1,
> +		.msb_right = 0,
> +	},
> +	.blue           = {
> +		.offset = 0,
> +		.length = 1,
> +		.msb_right = 0,
> +	},
> +	.transp         = {
> +		.offset = 0,
> +		.length = 0,
> +		.msb_right = 0,
> +	},
>  };
>  #endif /* CONFIG_HID_PICOLCD_FB */
>  
> @@ -188,6 +208,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 +367,7 @@ static int picolcd_fb_update_tile(u8 *vbitmap, const u8 *bitmap, int bpp,
>  			const u8 *bdata = bitmap + tile * 256 + chip * 8 + b * 32;
>  			for (i = 0; i < 64; i++) {
>  				tdata[i] <<= 1;
> -				tdata[i] |= (bdata[i/8] >> (7 - i % 8)) & 0x01;
> +				tdata[i] |= (bdata[i/8] >> (i % 8)) & 0x01;
>  			}
>  		}
>  	} else if (bpp = 8) {
> @@ -399,13 +420,10 @@ static int picolcd_fb_reset(struct picolcd_data *data, int clear)
>  
>  	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 = 0; i < PICOLCDFB_SIZE; i += 64)
> -				data->fb_vbitmap[i] = ~data->fb_vbitmap[i];
>  		}
> +		data->fb_force = 1;
>  	}
>  
>  	/* schedule first output of framebuffer */
> @@ -421,6 +439,9 @@ static void picolcd_fb_update(struct picolcd_data *data)
>  	int chip, tile, n;
>  	unsigned long flags;
>  
> +	if (!data)
> +		return;
> +
>  	spin_lock_irqsave(&data->lock, flags);
>  	if (!(data->status & PICOLCD_READY_FB)) {
>  		spin_unlock_irqrestore(&data->lock, flags);
> @@ -440,14 +461,18 @@ static void picolcd_fb_update(struct picolcd_data *data)
>  	for (chip = 0; chip < 4; chip++)
>  		for (tile = 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 += 2;
> +				if (!data->fb_info->par)
> +					return; /* device lost! */
>  				if (n >= HID_OUTPUT_FIFO_SIZE / 2) {
>  					usbhid_wait_io(data->hdev);
>  					n = 0;
>  				}
>  				picolcd_fb_send_tile(data->hdev, chip, tile);
>  			}
> +	data->fb_force = false;
>  	if (n)
>  		usbhid_wait_io(data->hdev);
>  }
> @@ -511,11 +536,23 @@ static int picolcd_fb_blank(int blank, struct fb_info *info)
>  static void picolcd_fb_destroy(struct fb_info *info)
>  {
>  	struct picolcd_data *data = info->par;
> +	u32 *ref_cnt = info->pseudo_palette;
> +	int may_release;
> +
>  	info->par = NULL;
>  	if (data)
>  		data->fb_info = NULL;
>  	fb_deferred_io_cleanup(info);
> -	framebuffer_release(info);
> +
> +	ref_cnt--;
> +	mutex_lock(&info->lock);
> +	(*ref_cnt)--;
> +	may_release = !ref_cnt;
> +	mutex_unlock(&info->lock);
> +	if (may_release) {
> +		framebuffer_release(info);
> +		vfree((u8 *)info->fix.smem_start);
> +	}
>  }
>  
>  static int picolcd_fb_check_var(struct fb_var_screeninfo *var, struct fb_info *info)
> @@ -526,29 +563,37 @@ static int picolcd_fb_check_var(struct fb_var_screeninfo *var, struct fb_info *i
>  	/* only allow 1/8 bit depth (8-bit is grayscale) */
>  	*var = picolcdfb_var;
>  	var->activate = activate;
> -	if (bpp >= 8)
> +	if (bpp >= 8) {
>  		var->bits_per_pixel = 8;
> -	else
> +		var->red.length     = 8;
> +		var->green.length   = 8;
> +		var->blue.length    = 8;
> +	} else {
>  		var->bits_per_pixel = 1;
> +		var->red.length     = 1;
> +		var->green.length   = 1;
> +		var->blue.length    = 1;
> +	}
>  	return 0;
>  }
>  
>  static int picolcd_set_par(struct fb_info *info)
>  {
>  	struct picolcd_data *data = info->par;
> -	u8 *o_fb, *n_fb;
> +	u8 *tmp_fb, *o_fb;
> +	if (!data)
> +		return -ENODEV;
>  	if (info->var.bits_per_pixel = data->fb_bpp)
>  		return 0;
>  	/* switch between 1/8 bit depths */
>  	if (info->var.bits_per_pixel != 1 && info->var.bits_per_pixel != 8)
>  		return -EINVAL;
>  
> -	o_fb = data->fb_bitmap;
> -	n_fb = vmalloc(PICOLCDFB_SIZE*info->var.bits_per_pixel);
> -	if (!n_fb)
> +	o_fb   = data->fb_bitmap;
> +	tmp_fb = kmalloc(PICOLCDFB_SIZE*info->var.bits_per_pixel, GFP_KERNEL);
> +	if (!tmp_fb)
>  		return -ENOMEM;
>  
> -	fb_deferred_io_cleanup(info);
>  	/* translate FB content to new bits-per-pixel */
>  	if (info->var.bits_per_pixel = 1) {
>  		int i, b;
> @@ -558,24 +603,87 @@ static int picolcd_set_par(struct fb_info *info)
>  				p <<= 1;
>  				p |= o_fb[i*8+b] ? 0x01 : 0x00;
>  			}
> +			tmp_fb[i] = p;
>  		}
> +		memcpy(o_fb, tmp_fb, PICOLCDFB_SIZE);
>  		info->fix.visual = FB_VISUAL_MONO01;
>  		info->fix.line_length = PICOLCDFB_WIDTH / 8;
>  	} else {
>  		int i;
> +		memcpy(tmp_fb, o_fb, PICOLCDFB_SIZE);
>  		for (i = 0; i < PICOLCDFB_SIZE * 8; i++)
> -			n_fb[i] = o_fb[i/8] & (0x01 << (7 - i % 8)) ? 0xff : 0x00;
> -		info->fix.visual = FB_VISUAL_TRUECOLOR;
> +			o_fb[i] = tmp_fb[i/8] & (0x01 << (7 - i % 8)) ? 0xff : 0x00;
> +		info->fix.visual = FB_VISUAL_DIRECTCOLOR;
>  		info->fix.line_length = PICOLCDFB_WIDTH;
>  	}
>  
> -	data->fb_bitmap   = n_fb;
> +	kfree(tmp_fb);
>  	data->fb_bpp      = info->var.bits_per_pixel;
> -	info->screen_base = (char __force __iomem *)n_fb;
> -	info->fix.smem_start = (unsigned long)n_fb;
> -	info->fix.smem_len   = PICOLCDFB_SIZE*data->fb_bpp;
> -	fb_deferred_io_init(info);
> -	vfree(o_fb);
> +	return 0;
> +}
> +
> +/* Do refcounting on our FB and cleanup per worker if FB is
> + * closed after unplug of our device
> + * (fb_release holds info->lock and still touches info after
> + *  we return so we can't release it immediately.
> + */
> +struct picolcd_fb_cleanup_item {
> +	struct fb_info *info;
> +	struct picolcd_fb_cleanup_item *next;
> +};
> +static struct picolcd_fb_cleanup_item *fb_pending;
> +DEFINE_SPINLOCK(fb_pending_lock);
> +
> +static void picolcd_fb_do_cleanup(struct work_struct *data)
> +{
> +	struct picolcd_fb_cleanup_item *item;
> +	unsigned long flags;
> +
> +	do {
> +		spin_lock_irqsave(&fb_pending_lock, flags);
> +		item = fb_pending;
> +		fb_pending = item ? item->next : NULL;
> +		spin_unlock_irqrestore(&fb_pending_lock, flags);
> +
> +		if (item) {
> +			u8 *fb = (u8 *)item->info->fix.smem_start;
> +			/* make sure we do not race against fb core when
> +			 * releasing */
> +			mutex_lock(&item->info->lock);
> +			mutex_unlock(&item->info->lock);
> +			framebuffer_release(item->info);
> +			vfree(fb);
> +		}
> +	} while (item);
> +}
> +
> +DECLARE_WORK(picolcd_fb_cleanup, picolcd_fb_do_cleanup);
> +
> +static int picolcd_fb_open(struct fb_info *info, int u)
> +{
> +	u32 *ref_cnt = info->pseudo_palette;
> +	ref_cnt--;
> +
> +	(*ref_cnt)++;
> +	return 0;
> +}
> +
> +static int picolcd_fb_release(struct fb_info *info, int u)
> +{
> +	u32 *ref_cnt = info->pseudo_palette;
> +	ref_cnt--;
> +
> +	(*ref_cnt)++;
> +	if (!*ref_cnt) {
> +		unsigned long flags;
> +		struct picolcd_fb_cleanup_item *item = (struct picolcd_fb_cleanup_item *)ref_cnt;
> +		item--;
> +		spin_lock_irqsave(&fb_pending_lock, flags);
> +		item->next = fb_pending;
> +		fb_pending = item;
> +		spin_unlock_irqrestore(&fb_pending_lock, flags);
> +		schedule_work(&picolcd_fb_cleanup);
> +	}
>  	return 0;
>  }
>  
> @@ -583,6 +691,8 @@ static int picolcd_set_par(struct fb_info *info)
>  static struct fb_ops picolcdfb_ops = {
>  	.owner        = THIS_MODULE,
>  	.fb_destroy   = picolcd_fb_destroy,
> +	.fb_open      = picolcd_fb_open,
> +	.fb_release   = picolcd_fb_release,
>  	.fb_read      = fb_sys_read,
>  	.fb_write     = picolcd_fb_write,
>  	.fb_blank     = picolcd_fb_blank,
> @@ -660,11 +770,12 @@ static int picolcd_init_framebuffer(struct picolcd_data *data)
>  {
>  	struct device *dev = &data->hdev->dev;
>  	struct fb_info *info = NULL;
> -	int error = -ENOMEM;
> +	int i, error = -ENOMEM;
>  	u8 *fb_vbitmap = NULL;
>  	u8 *fb_bitmap  = NULL;
> +	u32 *palette;
>  
> -	fb_bitmap = vmalloc(PICOLCDFB_SIZE*picolcdfb_var.bits_per_pixel);
> +	fb_bitmap = vmalloc(PICOLCDFB_SIZE*8);
>  	if (fb_bitmap = NULL) {
>  		dev_err(dev, "can't get a free page for framebuffer\n");
>  		goto err_nomem;
> @@ -678,18 +789,29 @@ static int picolcd_init_framebuffer(struct picolcd_data *data)
>  
>  	data->fb_update_rate = PICOLCDFB_UPDATE_RATE_DEFAULT;
>  	data->fb_defio = picolcd_fb_defio;
> -	info = framebuffer_alloc(0, dev);
> +	/* The extra memory is:
> +	 * - struct picolcd_fb_cleanup_item
> +	 * - u32 for ref_count
> +	 * - 256*u32 for pseudo_palette
> +	 */
> +	info = framebuffer_alloc(257 * sizeof(u32) + sizeof(struct picolcd_fb_cleanup_item), dev);
>  	if (info = NULL) {
>  		dev_err(dev, "failed to allocate a framebuffer\n");
>  		goto err_nomem;
>  	}
>  
> +	palette  = info->par + sizeof(struct picolcd_fb_cleanup_item);
> +	*palette = 1;
> +	palette++;
> +	for (i = 0; i < 256; i++)
> +		palette[i] = i > 0 && i < 16 ? 0xff : 0;
> +	info->pseudo_palette = palette;
>  	info->fbdefio = &data->fb_defio;
>  	info->screen_base = (char __force __iomem *)fb_bitmap;
>  	info->fbops = &picolcdfb_ops;
>  	info->var = picolcdfb_var;
>  	info->fix = picolcdfb_fix;
> -	info->fix.smem_len   = PICOLCDFB_SIZE;
> +	info->fix.smem_len   = PICOLCDFB_SIZE*8;
>  	info->fix.smem_start = (unsigned long)fb_bitmap;
>  	info->par = data;
>  	info->flags = FBINFO_FLAG_DEFAULT;
> @@ -707,18 +829,20 @@ static int picolcd_init_framebuffer(struct picolcd_data *data)
>  		dev_err(dev, "failed to create sysfs attributes\n");
>  		goto err_cleanup;
>  	}
> +	fb_deferred_io_init(info);
>  	data->fb_info    = info;
>  	error = 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 = 1;
>  	schedule_delayed_work(&info->deferred_work, 0);
>  	return 0;
>  
>  err_sysfs:
> +	fb_deferred_io_cleanup(info);
>  	device_remove_file(dev, &dev_attr_fb_update_rate);
>  err_cleanup:
>  	data->fb_vbitmap = NULL;
> @@ -728,8 +852,8 @@ err_cleanup:
>  
>  err_nomem:
>  	framebuffer_release(info);
> -	vfree(fb_bitmap);
>  	kfree(fb_vbitmap);
> +	vfree(fb_bitmap);
>  	return error;
>  }
>  
> @@ -737,19 +861,17 @@ static void picolcd_exit_framebuffer(struct picolcd_data *data)
>  {
>  	struct fb_info *info = data->fb_info;
>  	u8 *fb_vbitmap = data->fb_vbitmap;
> -	u8 *fb_bitmap  = data->fb_bitmap;
>  
>  	if (!info)
>  		return;
>  
> +	info->par = NULL;
> +	device_remove_file(&data->hdev->dev, &dev_attr_fb_update_rate);
> +	unregister_framebuffer(info);
>  	data->fb_vbitmap = NULL;
>  	data->fb_bitmap  = NULL;
>  	data->fb_bpp     = 0;
>  	data->fb_info    = 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 +2688,13 @@ static void picolcd_remove(struct hid_device *hdev)
>  	spin_lock_irqsave(&data->lock, flags);
>  	data->status |= PICOLCD_FAILED;
>  	spin_unlock_irqrestore(&data->lock, flags);
> +#ifdef CONFIG_HID_PICOLCD_FB
> +	/* 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 = NULL;
> +#endif
>  
>  	picolcd_exit_devfs(data);
>  	device_remove_file(&hdev->dev, &dev_attr_operation_mode);
> @@ -2623,6 +2752,10 @@ static int __init picolcd_init(void)
>  static void __exit picolcd_exit(void)
>  {
>  	hid_unregister_driver(&picolcd_driver);
> +#ifdef CONFIG_HID_PICOLCD_FB
> +	flush_scheduled_work();
> +	WARN_ON(fb_pending);
> +#endif
>  }
>  
>  module_init(picolcd_init);

Re: [Patch] HID: Fix PicoLCD to allow it to run fbcon and handle

[Jiri Kosina]

On Wed, 23 Jun 2010, Bruno Prémont wrote:

> Do you think This patch can be applied as-is or should I break it up
> into 2 or 3 patches (one for the 8bpp NULL-pointer dereference,
> one for switch between 1bpp and 8bpp and one for the refcounting of
> framebuffer to get things polite on unplug while framebuffer is still
> in use?

Hi Bruno,

splitting would definitely improve readability and reviewability (even 
more so for someone like me, who is not really familiar with the 
framebuffer stuff).

Still it'd be nice if you could collect Ack from someone more familiar 
with the framebuffer code.

Thanks,

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

[Patch 0/4] HID: Fix PicoLCD to allow it to run fbcon and handle

[Bruno Prémont]

Hi Jiri,

On Thu, 24 June 2010 Jiri Kosina <jkosina@suse.cz> wrote:
> On Wed, 23 Jun 2010, Bruno Prémont wrote:
> > Do you think This patch can be applied as-is or should I break it up
> > into 2 or 3 patches (one for the 8bpp NULL-pointer dereference,
> > one for switch between 1bpp and 8bpp and one for the refcounting of
> > framebuffer to get things polite on unplug while framebuffer is still
> > in use?
> 
> Hi Bruno,
> 
> splitting would definitely improve readability and reviewability (even 
> more so for someone like me, who is not really familiar with the 
> framebuffer stuff).
> 
> Still it'd be nice if you could collect Ack from someone more familiar 
> with the framebuffer code.

Here it comes, split into 4 patches.

Thanks,
Bruno

[PATCH 1/4] HID: picolcd: fix deferred_io init/cleanup to

[Bruno Prémont]

We need to call fb_deferred_io_init() before we register_framebuffer()
as otherwise, in case fbcon uses our framebuffer, we will get a BUG()
because in picolcd_fb_imageblit() we schedule defio which has not
been initialized yet.

Note: this BUG() deadlocks ttys.

Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org>
---
 drivers/hid/hid-picolcd.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-picolcd.c b/drivers/hid/hid-picolcd.c
index 95253b3..883d720 100644
--- a/drivers/hid/hid-picolcd.c
+++ b/drivers/hid/hid-picolcd.c
@@ -707,18 +707,19 @@ static int picolcd_init_framebuffer(struct picolcd_data *data)
 		dev_err(dev, "failed to create sysfs attributes\n");
 		goto err_cleanup;
 	}
+	fb_deferred_io_init(info);
 	data->fb_info    = info;
 	error = 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 */
 	schedule_delayed_work(&info->deferred_work, 0);
 	return 0;
 
 err_sysfs:
+	fb_deferred_io_cleanup(info);
 	device_remove_file(dev, &dev_attr_fb_update_rate);
 err_cleanup:
 	data->fb_vbitmap = NULL;
@@ -747,7 +748,6 @@ static void picolcd_exit_framebuffer(struct picolcd_data *data)
 	data->fb_bpp     = 0;
 	data->fb_info    = 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);
-- 
1.7.1

Re: [PATCH 1/4] HID: picolcd: fix deferred_io init/cleanup to

[Jaya Kumar]

On Tue, Jun 29, 2010 at 4:29 AM, Bruno Prémont
<bonbons@linux-vserver.org> wrote:
> We need to call fb_deferred_io_init() before we register_framebuffer()
> as otherwise, in case fbcon uses our framebuffer, we will get a BUG()
> because in picolcd_fb_imageblit() we schedule defio which has not
> been initialized yet.

Hi Bruno,

The comment above seems confusing to me in that it talks about fbcon
and defio schedule.

What I see is that you originally had in picolcd:

>        error = register_framebuffer(info);
...
>        fb_deferred_io_init(info);

which was a bug because it called register_framebuffer (ie: made the
framebuffer available for use) and only then initialized the defio
handlers which were needed for that framebuffer memory to be used.
Drivers must always call defio_init _before_ register_framebuffer. The
bug fix to picolcd below looks correct because it now does that
sequence in the correct order.

Thanks,
jaya



>
> Note: this BUG() deadlocks ttys.
>
> Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org>
> ---
>  drivers/hid/hid-picolcd.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hid/hid-picolcd.c b/drivers/hid/hid-picolcd.c
> index 95253b3..883d720 100644
> --- a/drivers/hid/hid-picolcd.c
> +++ b/drivers/hid/hid-picolcd.c
> @@ -707,18 +707,19 @@ static int picolcd_init_framebuffer(struct picolcd_data *data)
>                dev_err(dev, "failed to create sysfs attributes\n");
>                goto err_cleanup;
>        }
> +       fb_deferred_io_init(info);
>        data->fb_info    = info;
>        error = 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 */
>        schedule_delayed_work(&info->deferred_work, 0);
>        return 0;
>
>  err_sysfs:
> +       fb_deferred_io_cleanup(info);
>        device_remove_file(dev, &dev_attr_fb_update_rate);
>  err_cleanup:
>        data->fb_vbitmap = NULL;
> @@ -747,7 +748,6 @@ static void picolcd_exit_framebuffer(struct picolcd_data *data)
>        data->fb_bpp     = 0;
>        data->fb_info    = 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);
> --
> 1.7.1
>
>

Re: [PATCH 1/4] HID: picolcd: fix deferred_io init/cleanup to

[Bruno Prémont]

Hi Jaya,

On Wed, 30 Jun 2010 09:52:13 Jaya Kumar wrote:
> On Tue, Jun 29, 2010 at 4:29 AM, Bruno Prémont
> <bonbons@linux-vserver.org> wrote:
> > We need to call fb_deferred_io_init() before we
> > register_framebuffer() as otherwise, in case fbcon uses our
> > framebuffer, we will get a BUG() because in picolcd_fb_imageblit()
> > we schedule defio which has not been initialized yet.
> 
> Hi Bruno,
> 
> The comment above seems confusing to me in that it talks about fbcon
> and defio schedule.

Well I talk about fbcon as that's the trigger I have seen causing an
issue.

I'm fine with rewriting the changelog as to just talk about the
correct/expected order of initialization.

Thanks for looking at it,
Bruno


> What I see is that you originally had in picolcd:
> 
> >        error = register_framebuffer(info);
> ...
> >        fb_deferred_io_init(info);
> 
> which was a bug because it called register_framebuffer (ie: made the
> framebuffer available for use) and only then initialized the defio
> handlers which were needed for that framebuffer memory to be used.
> Drivers must always call defio_init _before_ register_framebuffer. The
> bug fix to picolcd below looks correct because it now does that
> sequence in the correct order.
> 
> Thanks,
> jaya
> 
> 
> 
> >
> > Note: this BUG() deadlocks ttys.
> >
> > Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org>
> > ---
> >  drivers/hid/hid-picolcd.c |    4 ++--
> >  1 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/hid/hid-picolcd.c b/drivers/hid/hid-picolcd.c
> > index 95253b3..883d720 100644
> > --- a/drivers/hid/hid-picolcd.c
> > +++ b/drivers/hid/hid-picolcd.c
> > @@ -707,18 +707,19 @@ static int picolcd_init_framebuffer(struct
> > picolcd_data *data) dev_err(dev, "failed to create sysfs
> > attributes\n"); goto err_cleanup;
> >        }
> > +       fb_deferred_io_init(info);
> >        data->fb_info    = info;
> >        error = 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 */
> >        schedule_delayed_work(&info->deferred_work, 0);
> >        return 0;
> >
> >  err_sysfs:
> > +       fb_deferred_io_cleanup(info);
> >        device_remove_file(dev, &dev_attr_fb_update_rate);
> >  err_cleanup:
> >        data->fb_vbitmap = NULL;
> > @@ -747,7 +748,6 @@ static void picolcd_exit_framebuffer(struct
> > picolcd_data *data) data->fb_bpp     = 0;
> >        data->fb_info    = 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);
> > --
> > 1.7.1
> >
> >

[PATCH 1/4 - adjusted changelog] HID: picolcd: fix deferred_io

[Bruno Prémont]

Adjust ordering if framebuffer (un)registration and defio init/cleanup
to match the correct order (init defio, register FB ... unregister FB,
cleanup defio)

Acked-by: Jaya Kumar <jayakumar.lkml@gmail.com>
Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org>
---
I consider Jaya's reply as an ack to the patch.


 drivers/hid/hid-picolcd.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-picolcd.c b/drivers/hid/hid-picolcd.c
index 95253b3..883d720 100644
--- a/drivers/hid/hid-picolcd.c
+++ b/drivers/hid/hid-picolcd.c
@@ -707,18 +707,19 @@ static int picolcd_init_framebuffer(struct picolcd_data *data)
 		dev_err(dev, "failed to create sysfs attributes\n");
 		goto err_cleanup;
 	}
+	fb_deferred_io_init(info);
 	data->fb_info    = info;
 	error = 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 */
 	schedule_delayed_work(&info->deferred_work, 0);
 	return 0;
 
 err_sysfs:
+	fb_deferred_io_cleanup(info);
 	device_remove_file(dev, &dev_attr_fb_update_rate);
 err_cleanup:
 	data->fb_vbitmap = NULL;
@@ -747,7 +748,6 @@ static void picolcd_exit_framebuffer(struct picolcd_data *data)
 	data->fb_bpp     = 0;
 	data->fb_info    = 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);
-- 
1.7.1

Re: [PATCH 1/4 - adjusted changelog] HID: picolcd: fix deferred_io

[Jiri Kosina]

On Wed, 30 Jun 2010, Bruno Prémont wrote:

> Adjust ordering if framebuffer (un)registration and defio init/cleanup
> to match the correct order (init defio, register FB ... unregister FB,
> cleanup defio)
> 
> Acked-by: Jaya Kumar <jayakumar.lkml@gmail.com>
> Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org>
> ---
> I consider Jaya's reply as an ack to the patch.

Applied, thanks Bruno.

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

Re: [PATCH 1/4 - adjusted changelog] HID: picolcd: fix deferred_io

[Bruno Prémont]

On Sun, 11 Jul 2010 22:58:37 Jiri Kosina <jkosina@suse.cz> wrote:
> On Wed, 30 Jun 2010, Bruno Prémont wrote:
> 
> > Adjust ordering if framebuffer (un)registration and defio
> > init/cleanup to match the correct order (init defio, register
> > FB ... unregister FB, cleanup defio)
> > 
> > Acked-by: Jaya Kumar <jayakumar.lkml@gmail.com>
> > Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org>
> > ---
> > I consider Jaya's reply as an ack to the patch.
> 
> Applied, thanks Bruno.

Thanks for applying, what about the 3 other patches of the series?

For the last one, I will have a look at putting ref-counting support
info FB so this can be shared, e.g. with Bernie's udlfb (unless he
is quicker than me). Until that is written and merged it's certainly
wise to have the refcounting locally.

Thanks,
Bruno

Re: [PATCH 1/4 - adjusted changelog] HID: picolcd: fix deferred_io

[Jiri Kosina]

On Mon, 12 Jul 2010, Bruno Prémont wrote:

> > > Adjust ordering if framebuffer (un)registration and defio
> > > init/cleanup to match the correct order (init defio, register
> > > FB ... unregister FB, cleanup defio)
> > > 
> > > Acked-by: Jaya Kumar <jayakumar.lkml@gmail.com>
> > > Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org>
> > > ---
> > > I consider Jaya's reply as an ack to the patch.
> > 
> > Applied, thanks Bruno.
> 
> Thanks for applying, what about the 3 other patches of the series?

Hi Bruno,

I have come from vacation, so I have quite some backlog, but will get to 
your patches hopefully soon. Thanks for patience.

> For the last one, I will have a look at putting ref-counting support 
> info FB so this can be shared, e.g. with Bernie's udlfb (unless he is 
> quicker than me). Until that is written and merged it's certainly wise 
> to have the refcounting locally.

Thanks.

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

Re: [PATCH 1/4 - adjusted changelog] HID: picolcd: fix deferred_io

[Jiri Kosina]

On Mon, 12 Jul 2010, Jiri Kosina wrote:

> > > > Adjust ordering if framebuffer (un)registration and defio
> > > > init/cleanup to match the correct order (init defio, register
> > > > FB ... unregister FB, cleanup defio)
> > > > 
> > > > Acked-by: Jaya Kumar <jayakumar.lkml@gmail.com>
> > > > Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org>
> > > > ---
> > > > I consider Jaya's reply as an ack to the patch.
> > > 
> > > Applied, thanks Bruno.
> > 
> > Thanks for applying, what about the 3 other patches of the series?
> 
> Hi Bruno,
> 
> I have come from vacation, so I have quite some backlog, but will get to 
> your patches hopefully soon. Thanks for patience.

OK, I have now applied the patches. Thanks,

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

[PATCH 2/4] HID: picolcd: Add minimal palette required by fbcon on

[Bruno Prémont]

Add a minimal palette so fbcon does not try to dereference
a NULL point when fb is set to 8bpp.

fbcon stores pixels the other way around in bytes for 1bpp
than intially implemented, correct this.

Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org>
---
 drivers/hid/hid-picolcd.c |   62 +++++++++++++++++++++++++++++++++++++--------
 1 files changed, 51 insertions(+), 11 deletions(-)

diff --git a/drivers/hid/hid-picolcd.c b/drivers/hid/hid-picolcd.c
index 883d720..ac7aece 100644
--- a/drivers/hid/hid-picolcd.c
+++ b/drivers/hid/hid-picolcd.c
@@ -127,6 +127,26 @@ static const struct fb_var_screeninfo picolcdfb_var = {
 	.height         = 26,
 	.bits_per_pixel = 1,
 	.grayscale      = 1,
+	.red            = {
+		.offset = 0,
+		.length = 1,
+		.msb_right = 0,
+	},
+	.green          = {
+		.offset = 0,
+		.length = 1,
+		.msb_right = 0,
+	},
+	.blue           = {
+		.offset = 0,
+		.length = 1,
+		.msb_right = 0,
+	},
+	.transp         = {
+		.offset = 0,
+		.length = 0,
+		.msb_right = 0,
+	},
 };
 #endif /* CONFIG_HID_PICOLCD_FB */
 
@@ -188,6 +208,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 +367,7 @@ static int picolcd_fb_update_tile(u8 *vbitmap, const u8 *bitmap, int bpp,
 			const u8 *bdata = bitmap + tile * 256 + chip * 8 + b * 32;
 			for (i = 0; i < 64; i++) {
 				tdata[i] <<= 1;
-				tdata[i] |= (bdata[i/8] >> (7 - i % 8)) & 0x01;
+				tdata[i] |= (bdata[i/8] >> (i % 8)) & 0x01;
 			}
 		}
 	} else if (bpp = 8) {
@@ -399,13 +420,10 @@ static int picolcd_fb_reset(struct picolcd_data *data, int clear)
 
 	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 = 0; i < PICOLCDFB_SIZE; i += 64)
-				data->fb_vbitmap[i] = ~data->fb_vbitmap[i];
 		}
+		data->fb_force = 1;
 	}
 
 	/* schedule first output of framebuffer */
@@ -440,7 +458,8 @@ static void picolcd_fb_update(struct picolcd_data *data)
 	for (chip = 0; chip < 4; chip++)
 		for (tile = 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 += 2;
 				if (n >= HID_OUTPUT_FIFO_SIZE / 2) {
 					usbhid_wait_io(data->hdev);
@@ -448,6 +467,7 @@ static void picolcd_fb_update(struct picolcd_data *data)
 				}
 				picolcd_fb_send_tile(data->hdev, chip, tile);
 			}
+	data->fb_force = false;
 	if (n)
 		usbhid_wait_io(data->hdev);
 }
@@ -526,10 +546,17 @@ static int picolcd_fb_check_var(struct fb_var_screeninfo *var, struct fb_info *i
 	/* only allow 1/8 bit depth (8-bit is grayscale) */
 	*var = picolcdfb_var;
 	var->activate = activate;
-	if (bpp >= 8)
+	if (bpp >= 8) {
 		var->bits_per_pixel = 8;
-	else
+		var->red.length     = 8;
+		var->green.length   = 8;
+		var->blue.length    = 8;
+	} else {
 		var->bits_per_pixel = 1;
+		var->red.length     = 1;
+		var->green.length   = 1;
+		var->blue.length    = 1;
+	}
 	return 0;
 }
 
@@ -660,9 +687,10 @@ static int picolcd_init_framebuffer(struct picolcd_data *data)
 {
 	struct device *dev = &data->hdev->dev;
 	struct fb_info *info = NULL;
-	int error = -ENOMEM;
+	int i, error = -ENOMEM;
 	u8 *fb_vbitmap = NULL;
 	u8 *fb_bitmap  = NULL;
+	u32 *palette;
 
 	fb_bitmap = vmalloc(PICOLCDFB_SIZE*picolcdfb_var.bits_per_pixel);
 	if (fb_bitmap = NULL) {
@@ -678,12 +706,23 @@ static int picolcd_init_framebuffer(struct picolcd_data *data)
 
 	data->fb_update_rate = PICOLCDFB_UPDATE_RATE_DEFAULT;
 	data->fb_defio = picolcd_fb_defio;
-	info = framebuffer_alloc(0, dev);
+	/* The extra memory is:
+	 * - struct picolcd_fb_cleanup_item
+	 * - u32 for ref_count
+	 * - 256*u32 for pseudo_palette
+	 */
+	info = framebuffer_alloc(257 * sizeof(u32), dev);
 	if (info = NULL) {
 		dev_err(dev, "failed to allocate a framebuffer\n");
 		goto err_nomem;
 	}
 
+	palette  = info->par;
+	*palette = 1;
+	palette++;
+	for (i = 0; i < 256; i++)
+		palette[i] = i > 0 && i < 16 ? 0xff : 0;
+	info->pseudo_palette = palette;
 	info->fbdefio = &data->fb_defio;
 	info->screen_base = (char __force __iomem *)fb_bitmap;
 	info->fbops = &picolcdfb_ops;
@@ -715,6 +754,7 @@ static int picolcd_init_framebuffer(struct picolcd_data *data)
 		goto err_sysfs;
 	}
 	/* schedule first output of framebuffer */
+	data->fb_force = 1;
 	schedule_delayed_work(&info->deferred_work, 0);
 	return 0;
 
-- 
1.7.1

[PATCH 3/4] HID: picolcd: do not reallocate memory on depth change

[Bruno Prémont]

Reallocating memory in depth change does not work well if some
userspace application has mmapped() the framebuffer as that mapping
does not get adjusted (thus application continues to write to old
buffer).
In addition doing deferred_io_cleanup() and init() inside of set_par()
tends to deadlock with fbcon's flashing cursor.

Avoid all this by allocating a buffer that can hold 8bpp framebuffer
and just use 1/8 of it while running at 1bpp.

Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org>
---
 drivers/hid/hid-picolcd.c |   27 ++++++++++++---------------
 1 files changed, 12 insertions(+), 15 deletions(-)

diff --git a/drivers/hid/hid-picolcd.c b/drivers/hid/hid-picolcd.c
index ac7aece..31a0abd 100644
--- a/drivers/hid/hid-picolcd.c
+++ b/drivers/hid/hid-picolcd.c
@@ -563,19 +563,18 @@ static int picolcd_fb_check_var(struct fb_var_screeninfo *var, struct fb_info *i
 static int picolcd_set_par(struct fb_info *info)
 {
 	struct picolcd_data *data = info->par;
-	u8 *o_fb, *n_fb;
+	u8 *tmp_fb, *o_fb;
 	if (info->var.bits_per_pixel = data->fb_bpp)
 		return 0;
 	/* switch between 1/8 bit depths */
 	if (info->var.bits_per_pixel != 1 && info->var.bits_per_pixel != 8)
 		return -EINVAL;
 
-	o_fb = data->fb_bitmap;
-	n_fb = vmalloc(PICOLCDFB_SIZE*info->var.bits_per_pixel);
-	if (!n_fb)
+	o_fb   = data->fb_bitmap;
+	tmp_fb = kmalloc(PICOLCDFB_SIZE*info->var.bits_per_pixel, GFP_KERNEL);
+	if (!tmp_fb)
 		return -ENOMEM;
 
-	fb_deferred_io_cleanup(info);
 	/* translate FB content to new bits-per-pixel */
 	if (info->var.bits_per_pixel = 1) {
 		int i, b;
@@ -585,24 +584,22 @@ static int picolcd_set_par(struct fb_info *info)
 				p <<= 1;
 				p |= o_fb[i*8+b] ? 0x01 : 0x00;
 			}
+			tmp_fb[i] = p;
 		}
+		memcpy(o_fb, tmp_fb, PICOLCDFB_SIZE);
 		info->fix.visual = FB_VISUAL_MONO01;
 		info->fix.line_length = PICOLCDFB_WIDTH / 8;
 	} else {
 		int i;
+		memcpy(tmp_fb, o_fb, PICOLCDFB_SIZE);
 		for (i = 0; i < PICOLCDFB_SIZE * 8; i++)
-			n_fb[i] = o_fb[i/8] & (0x01 << (7 - i % 8)) ? 0xff : 0x00;
-		info->fix.visual = FB_VISUAL_TRUECOLOR;
+			o_fb[i] = tmp_fb[i/8] & (0x01 << (7 - i % 8)) ? 0xff : 0x00;
+		info->fix.visual = FB_VISUAL_DIRECTCOLOR;
 		info->fix.line_length = PICOLCDFB_WIDTH;
 	}
 
-	data->fb_bitmap   = n_fb;
+	kfree(tmp_fb);
 	data->fb_bpp      = info->var.bits_per_pixel;
-	info->screen_base = (char __force __iomem *)n_fb;
-	info->fix.smem_start = (unsigned long)n_fb;
-	info->fix.smem_len   = PICOLCDFB_SIZE*data->fb_bpp;
-	fb_deferred_io_init(info);
-	vfree(o_fb);
 	return 0;
 }
 
@@ -692,7 +689,7 @@ static int picolcd_init_framebuffer(struct picolcd_data *data)
 	u8 *fb_bitmap  = NULL;
 	u32 *palette;
 
-	fb_bitmap = vmalloc(PICOLCDFB_SIZE*picolcdfb_var.bits_per_pixel);
+	fb_bitmap = vmalloc(PICOLCDFB_SIZE*8);
 	if (fb_bitmap = NULL) {
 		dev_err(dev, "can't get a free page for framebuffer\n");
 		goto err_nomem;
@@ -728,7 +725,7 @@ static int picolcd_init_framebuffer(struct picolcd_data *data)
 	info->fbops = &picolcdfb_ops;
 	info->var = picolcdfb_var;
 	info->fix = picolcdfb_fix;
-	info->fix.smem_len   = PICOLCDFB_SIZE;
+	info->fix.smem_len   = PICOLCDFB_SIZE*8;
 	info->fix.smem_start = (unsigned long)fb_bitmap;
 	info->par = data;
 	info->flags = FBINFO_FLAG_DEFAULT;
-- 
1.7.1

[PATCH 4/4] HID: picolcd: implement refcounting of framebuffer

[Bruno Prémont]

As our device may be hot-unplugged and framebuffer cannot handle
this case by itself we need to keep track of usage count so as
to release fb_info and framebuffer memory only after the last user
has closed framebuffer.

We need to do the freeing in a scheduled work as fb_release()
is called with fb_info lock held.

Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org>
---
 drivers/hid/hid-picolcd.c |  110 ++++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 103 insertions(+), 7 deletions(-)

diff --git a/drivers/hid/hid-picolcd.c b/drivers/hid/hid-picolcd.c
index 31a0abd..85a105e 100644
--- a/drivers/hid/hid-picolcd.c
+++ b/drivers/hid/hid-picolcd.c
@@ -439,6 +439,9 @@ static void picolcd_fb_update(struct picolcd_data *data)
 	int chip, tile, n;
 	unsigned long flags;
 
+	if (!data)
+		return;
+
 	spin_lock_irqsave(&data->lock, flags);
 	if (!(data->status & PICOLCD_READY_FB)) {
 		spin_unlock_irqrestore(&data->lock, flags);
@@ -461,6 +464,8 @@ static void picolcd_fb_update(struct picolcd_data *data)
 					data->fb_bitmap, data->fb_bpp, chip, tile) ||
 				data->fb_force) {
 				n += 2;
+				if (!data->fb_info->par)
+					return; /* device lost! */
 				if (n >= HID_OUTPUT_FIFO_SIZE / 2) {
 					usbhid_wait_io(data->hdev);
 					n = 0;
@@ -531,11 +536,23 @@ static int picolcd_fb_blank(int blank, struct fb_info *info)
 static void picolcd_fb_destroy(struct fb_info *info)
 {
 	struct picolcd_data *data = info->par;
+	u32 *ref_cnt = info->pseudo_palette;
+	int may_release;
+
 	info->par = NULL;
 	if (data)
 		data->fb_info = NULL;
 	fb_deferred_io_cleanup(info);
-	framebuffer_release(info);
+
+	ref_cnt--;
+	mutex_lock(&info->lock);
+	(*ref_cnt)--;
+	may_release = !ref_cnt;
+	mutex_unlock(&info->lock);
+	if (may_release) {
+		framebuffer_release(info);
+		vfree((u8 *)info->fix.smem_start);
+	}
 }
 
 static int picolcd_fb_check_var(struct fb_var_screeninfo *var, struct fb_info *info)
@@ -564,6 +581,8 @@ static int picolcd_set_par(struct fb_info *info)
 {
 	struct picolcd_data *data = info->par;
 	u8 *tmp_fb, *o_fb;
+	if (!data)
+		return -ENODEV;
 	if (info->var.bits_per_pixel = data->fb_bpp)
 		return 0;
 	/* switch between 1/8 bit depths */
@@ -603,10 +622,77 @@ static int picolcd_set_par(struct fb_info *info)
 	return 0;
 }
 
+/* Do refcounting on our FB and cleanup per worker if FB is
+ * closed after unplug of our device
+ * (fb_release holds info->lock and still touches info after
+ *  we return so we can't release it immediately.
+ */
+struct picolcd_fb_cleanup_item {
+	struct fb_info *info;
+	struct picolcd_fb_cleanup_item *next;
+};
+static struct picolcd_fb_cleanup_item *fb_pending;
+DEFINE_SPINLOCK(fb_pending_lock);
+
+static void picolcd_fb_do_cleanup(struct work_struct *data)
+{
+	struct picolcd_fb_cleanup_item *item;
+	unsigned long flags;
+
+	do {
+		spin_lock_irqsave(&fb_pending_lock, flags);
+		item = fb_pending;
+		fb_pending = item ? item->next : NULL;
+		spin_unlock_irqrestore(&fb_pending_lock, flags);
+
+		if (item) {
+			u8 *fb = (u8 *)item->info->fix.smem_start;
+			/* make sure we do not race against fb core when
+			 * releasing */
+			mutex_lock(&item->info->lock);
+			mutex_unlock(&item->info->lock);
+			framebuffer_release(item->info);
+			vfree(fb);
+		}
+	} while (item);
+}
+
+DECLARE_WORK(picolcd_fb_cleanup, picolcd_fb_do_cleanup);
+
+static int picolcd_fb_open(struct fb_info *info, int u)
+{
+	u32 *ref_cnt = info->pseudo_palette;
+	ref_cnt--;
+
+	(*ref_cnt)++;
+	return 0;
+}
+
+static int picolcd_fb_release(struct fb_info *info, int u)
+{
+	u32 *ref_cnt = info->pseudo_palette;
+	ref_cnt--;
+
+	(*ref_cnt)++;
+	if (!*ref_cnt) {
+		unsigned long flags;
+		struct picolcd_fb_cleanup_item *item = (struct picolcd_fb_cleanup_item *)ref_cnt;
+		item--;
+		spin_lock_irqsave(&fb_pending_lock, flags);
+		item->next = fb_pending;
+		fb_pending = item;
+		spin_unlock_irqrestore(&fb_pending_lock, flags);
+		schedule_work(&picolcd_fb_cleanup);
+	}
+	return 0;
+}
+
 /* Note this can't be const because of struct fb_info definition */
 static struct fb_ops picolcdfb_ops = {
 	.owner        = THIS_MODULE,
 	.fb_destroy   = picolcd_fb_destroy,
+	.fb_open      = picolcd_fb_open,
+	.fb_release   = picolcd_fb_release,
 	.fb_read      = fb_sys_read,
 	.fb_write     = picolcd_fb_write,
 	.fb_blank     = picolcd_fb_blank,
@@ -708,13 +794,13 @@ static int picolcd_init_framebuffer(struct picolcd_data *data)
 	 * - u32 for ref_count
 	 * - 256*u32 for pseudo_palette
 	 */
-	info = framebuffer_alloc(257 * sizeof(u32), dev);
+	info = framebuffer_alloc(257 * sizeof(u32) + sizeof(struct picolcd_fb_cleanup_item), dev);
 	if (info = NULL) {
 		dev_err(dev, "failed to allocate a framebuffer\n");
 		goto err_nomem;
 	}
 
-	palette  = info->par;
+	palette  = info->par + sizeof(struct picolcd_fb_cleanup_item);
 	*palette = 1;
 	palette++;
 	for (i = 0; i < 256; i++)
@@ -775,18 +861,17 @@ static void picolcd_exit_framebuffer(struct picolcd_data *data)
 {
 	struct fb_info *info = data->fb_info;
 	u8 *fb_vbitmap = data->fb_vbitmap;
-	u8 *fb_bitmap  = data->fb_bitmap;
 
 	if (!info)
 		return;
 
+	info->par = NULL;
+	device_remove_file(&data->hdev->dev, &dev_attr_fb_update_rate);
+	unregister_framebuffer(info);
 	data->fb_vbitmap = NULL;
 	data->fb_bitmap  = NULL;
 	data->fb_bpp     = 0;
 	data->fb_info    = NULL;
-	device_remove_file(&data->hdev->dev, &dev_attr_fb_update_rate);
-	unregister_framebuffer(info);
-	vfree(fb_bitmap);
 	kfree(fb_vbitmap);
 }
 
@@ -2603,6 +2688,13 @@ static void picolcd_remove(struct hid_device *hdev)
 	spin_lock_irqsave(&data->lock, flags);
 	data->status |= PICOLCD_FAILED;
 	spin_unlock_irqrestore(&data->lock, flags);
+#ifdef CONFIG_HID_PICOLCD_FB
+	/* 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 = NULL;
+#endif
 
 	picolcd_exit_devfs(data);
 	device_remove_file(&hdev->dev, &dev_attr_operation_mode);
@@ -2660,6 +2752,10 @@ static int __init picolcd_init(void)
 static void __exit picolcd_exit(void)
 {
 	hid_unregister_driver(&picolcd_driver);
+#ifdef CONFIG_HID_PICOLCD_FB
+	flush_scheduled_work();
+	WARN_ON(fb_pending);
+#endif
 }
 
 module_init(picolcd_init);
-- 
1.7.1

Re: [PATCH 4/4] HID: picolcd: implement refcounting of framebuffer

[Bernie Thompson]

On Mon, Jun 28, 2010 at 1:33 PM, Bruno Prémont
<bonbons@linux-vserver.org> wrote:
> As our device may be hot-unplugged and framebuffer cannot handle
> this case by itself we need to keep track of usage count so as
> to release fb_info and framebuffer memory only after the last user
> has closed framebuffer.
>
> We need to do the freeing in a scheduled work as fb_release()
> is called with fb_info lock held.

The udlfb (DisplayLink) framebuffer driver just got a similar fix in
the last few days:
http://git.plugable.com/gitphp/index.php?p=udlfb&a=commit&h®0502457e54904b1a9e0d67db6719182824da7c

The need to schedule work in fb_release() because fbmem.c touches it
after release is especially unfortunate (we had to do the same).

Anyone have existing thoughts about fixing the release path more centrally?

Best wishes,
Bernie
http://plugable.com/

Re: [PATCH 4/4] HID: picolcd: implement refcounting of framebuffer

[Bruno Prémont]

On Mon, 28 June 2010 Bernie Thompson <bernie@plugable.com> wrote:
> On Mon, Jun 28, 2010 at 1:33 PM, Bruno Prémont
> <bonbons@linux-vserver.org> wrote:
> > As our device may be hot-unplugged and framebuffer cannot handle
> > this case by itself we need to keep track of usage count so as
> > to release fb_info and framebuffer memory only after the last user
> > has closed framebuffer.
> >
> > We need to do the freeing in a scheduled work as fb_release()
> > is called with fb_info lock held.
> 
> The udlfb (DisplayLink) framebuffer driver just got a similar fix in
> the last few days:
> http://git.plugable.com/gitphp/index.php?p=udlfb&a=commit&h®0502457e54904b1a9e0d67db6719182824da7c
> 
> The need to schedule work in fb_release() because fbmem.c touches it
> after release is especially unfortunate (we had to do the same).
> 
> Anyone have existing thoughts about fixing the release path more centrally?

The best option IMHO would be that framebuffer_release() would free up
memory when framebuffer is not in use anymore (or last close if FB was
in use when framebuffer_release() was called).

Something like
  framebuffer_alloc()              <-- set refcount to 1
  ...
  register_framebuffer()           <-- inc refcount
  ...
  and open() of fb device would inc refcount
  and close() would decrement it

  unregister_framebuffer()         <-- dec refcount
  framebuffer_release()            <-- dec refcount

When after decrementing refcount, refcount reaches zero
close() or framebuffer_release() would free the framebuffer, calling
fbops fb_release() callback so owner can do cleanup if it has something
to do.

That way it would be easiest for drivers of removable framebuffers,
possibly also useful for kms adding/removing framebuffers for extra
monitors.

Thanks,
Bruno

Re: [PATCH 4/4] HID: picolcd: implement refcounting of framebuffer

[Bernie Thompson]

On Tue, Jun 29, 2010 at 1:42 PM, Bruno Prémont
<bonbons@linux-vserver.org> wrote:
> Something like
>  framebuffer_alloc()              <-- set refcount to 1
>  ...
>  register_framebuffer()           <-- inc refcount
>  ...
>  and open() of fb device would inc refcount
>  and close() would decrement it
>
>  unregister_framebuffer()         <-- dec refcount
>  framebuffer_release()            <-- dec refcount

Totally agree.  Just verbalizing smaller details - wiith a struct kref
added to fb_info, initialized in framebuffer_alloc, get_kref() and
put_kref() at the points you list, and a framebuffer_free() for use
with put_kref (static to start, assuming only fbmem itself will
directly touch refs), this is sounds like you'd have a good fix for
fbdev's current problems here.

Best wishes,
Bernie

Re: [Patch 0/4] HID: Fix PicoLCD to allow it to run fbcon and handle

[Jiri Kosina]

On Mon, 28 Jun 2010, Bruno Prémont wrote:

> > splitting would definitely improve readability and reviewability (even 
> > more so for someone like me, who is not really familiar with the 
> > framebuffer stuff).
> > 
> > Still it'd be nice if you could collect Ack from someone more familiar 
> > with the framebuffer code.
> 
> Here it comes, split into 4 patches.

As the only objection from framebuffer people has been for changelog 
wording of 1/4, could you please fix that up, and I'll apply the series?

Thanks,

-- 
Jiri Kosina
SUSE Labs, Novell Inc.