fb: udlfb: fix scheduling while atomic.

Commit Message

Alexander Holler Jan. 5, 2013, 11:42 a.m. UTC

The console functions are using spinlocks while calling fb-driver ops
but udlfb waits for a semaphore in many ops. This results in the BUG
"scheduling while atomic". One of those call flows is e.g.

vt_console_print() (spinlock printing_lock)
	(...)
	dlfb_ops_imageblit()
                        dlfb_handle_damage()
                                dlfb_get_urb()
					down_timeout(semaphore)
BUG: scheduling while atomic
(...)
vt_console_print() (release spinlock printing_lock)

Fix this through a workqueue for dlfb_handle_damage().

Cc: <[e-mail]>
Signed-off-by: Alexander Holler <[e-mail]>
---
 drivers/video/udlfb.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 51 insertions(+), 2 deletions(-)

Comments

Alexander Holler Jan. 6, 2013, 12:46 p.m. UTC | #1

Am 05.01.2013 12:42, schrieb Alexander Holler:
> The console functions are using spinlocks while calling fb-driver ops
> but udlfb waits for a semaphore in many ops. This results in the BUG
> "scheduling while atomic". One of those call flows is e.g.
>
> vt_console_print() (spinlock printing_lock)
> 	(...)
> 	dlfb_ops_imageblit()
>                          dlfb_handle_damage()
>                                  dlfb_get_urb()
> 					down_timeout(semaphore)
> BUG: scheduling while atomic
> (...)
> vt_console_print() (release spinlock printing_lock)
>
> Fix this through a workqueue for dlfb_handle_damage().
>
> Cc: <[e-mail]>
> Signed-off-by: Alexander Holler <[e-mail]>


Having had a second look at my patch for udlfb, I'm not sure it will 
work with more than one of those devices attached. I think my approach 
to just add one (static) workqueue might not work in such a case, at 
least it looks so to me. But I'm unable to test it, as I only have one 
of those devices.

Having had a look at udl, I wonder why udlfb still has to be around. But 
because udl currently doesn't work here too, I'm not sure what 
functionality udl misses which udlfb still has.

So to conclude, my patch works as a workaround if only one of those 
devices will be attached, but currently should not be included into the 
kernel.

I don't know if I will make another version of that patch, as I will 
first have a deeper look at udl (if I find the time).

Regards,

Alexander

--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/video/udlfb.c b/drivers/video/udlfb.c
index 86d449e..5aadcb2 100644
--- a/drivers/video/udlfb.c
+++ b/drivers/video/udlfb.c
@@ -569,7 +569,7 @@  static int dlfb_render_hline(struct dlfb_data *dev, struct urb **urb_ptr,
 	return 0;
 }
 
-int dlfb_handle_damage(struct dlfb_data *dev, int x, int y,
+int dlfb_handle_damage_queued(struct dlfb_data *dev, int x, int y,
 	       int width, int height, char *data)
 {
 	int i, ret;
@@ -630,6 +630,44 @@  error:
 	return 0;
 }
 
+static struct workqueue_struct *dlfb_handle_damage_wq;
+
+struct dlfb_handle_damage_work {
+	struct work_struct my_work;
+	struct dlfb_data *dev;
+	char *data;
+	int x, y, width, height;
+};
+
+static void dlfb_handle_damage_work(struct work_struct *work)
+{
+	struct dlfb_handle_damage_work *my_work =
+		(struct dlfb_handle_damage_work *)work;
+
+	dlfb_handle_damage_queued(my_work->dev, my_work->x, my_work->y,
+			my_work->width, my_work->height, my_work->data);
+	kfree(work);
+	return;
+}
+
+void dlfb_handle_damage(struct dlfb_data *dev, int x, int y,
+	       int width, int height, char *data)
+{
+	struct dlfb_handle_damage_work *work =
+		kmalloc(sizeof(struct dlfb_handle_damage_work), GFP_KERNEL);
+
+	if (work) {
+		INIT_WORK((struct work_struct *)work, dlfb_handle_damage_work);
+		work->dev = dev;
+		work->x = x;
+		work->y = y;
+		work->width = width;
+		work->height = height;
+		work->data = data;
+		queue_work(dlfb_handle_damage_wq, (struct work_struct *)work);
+	}
+}
+
 /*
  * Path triggered by usermode clients who write to filesystem
  * e.g. cat filename > /dev/fb1
@@ -945,6 +983,9 @@  static void dlfb_free_framebuffer(struct dlfb_data *dev)
 
 		unregister_framebuffer(info);
 
+		if (dlfb_handle_damage_wq)
+			destroy_workqueue(dlfb_handle_damage_wq);
+
 		if (info->cmap.len != 0)
 			fb_dealloc_cmap(&info->cmap);
 		if (info->monspecs.modedb)
@@ -1626,13 +1667,13 @@  static int dlfb_usb_probe(struct usb_interface *interface,
 		dev->sku_pixel_limit = pixel_limit;
 	}
 
-
 	if (!dlfb_alloc_urb_list(dev, WRITES_IN_FLIGHT, MAX_TRANSFER)) {
 		retval = -ENOMEM;
 		pr_err("dlfb_alloc_urb_list failed\n");
 		goto error;
 	}
 
+
 	kref_get(&dev->kref); /* matching kref_put in free_framebuffer_work */
 
 	/* We don't register a new USB class. Our client interface is fbdev */
@@ -1694,6 +1735,13 @@  static void dlfb_init_framebuffer_work(struct work_struct *work)
 		goto error;
 	}
 
+	dlfb_handle_damage_wq = alloc_workqueue("udlfb_damage",
+						WQ_MEM_RECLAIM, 0);
+	if (dlfb_handle_damage_wq == NULL) {
+		pr_err("unable to allocate workqueue\n");
+		goto error;
+	}
+
 	/* ready to begin using device */
 
 	atomic_set(&dev->usb_active, 1);
@@ -1702,6 +1750,7 @@  static void dlfb_init_framebuffer_work(struct work_struct *work)
 	dlfb_ops_check_var(&info->var, info);
 	dlfb_ops_set_par(info);
 
+
 	retval = register_framebuffer(info);
 	if (retval < 0) {
 		pr_err("register_framebuffer failed %d\n", retval);